2

I have this method below that gets the current user, but it fetches the user 2 times, which seems a little redundant, so I'd like to see if there is a way I can reduce it to just once.

The first "User" fetch comes from "FindByEmailFromClaimsPrinciple()" and then the second comes from "_dbContext.Users"

Problem - I need User properties(email, username, roles) to pass to "CreateToken(User user)" for the claims. So it seems like I have no option but to fetch database data twice!

Quasi Solution - I can rework the extension method to only fetch the Id, UserName, and Email and that will save me a little DB bandwidth, but trying to see if I can reduce 2 fetches to 1.

Question - Is there a way I can refactor this code so that I only need to fetch User 1 time? More specifically, get my 2 User properties (most importantly roles), without having to get my whole User first, as it's a fairly large entity with ~30 columns.

[HttpGet]
public async Task <IActionResult> GetCurrentUser() {
  var userFromRepo = await _userManager.FindByEmailFromClaimsPrinciple(HttpContext.User);
  if (userFromRepo == null) return Unauthorized(new ApiResponse(401));

  var token = await _tokenService.CreateToken(userFromRepo);

  var user = await _dbContext.Users
    .Select(p => new {
        Id = p.Id,
        Email = p.Email,
        UserName = p.UserName,
        Hosted = p.Hosted,
        Instructed = p.Instructed,
        Attended = p.Attended,
        IsBoarded = p.IsBoarded,
        Likers = p.Likers.Count(),
        Rating = p.Rating,
        CreatedDate = p.CreatedDate,
        PhotoUrl = p.IsBoarded ? p.UserPhotos.FirstOrDefault(p => p.IsMain).Url : "assets/images/user.png",
        Age = p.DateOfBirth.CalculateAge(),
        DateOfBirth = p.DateOfBirth,
        ExperienceLevel = p.ExperienceLevel.GetEnumName(),
        Points = p.UserPoints.Sum(p => p.Points),
        Tokens = p.UserTokens.Sum(p => p.Tokens),
        Memberships = p.YogabandMemberships.Count(x => x.Status == YogabandMemberStatus.Active),
        Token = token,
        IsInstructor = p.IsInstructor,
    })
    .FirstOrDefaultAsync(p => p.Id == userFromRepo.Id);

  if (user == null) return NotFound(new ApiResponse(404));

  return Ok(user);
}

Here is CreateToken

public async Task <string> CreateToken(User user) {
  var claims = new List <Claim> {
    new Claim(JwtRegisteredClaimNames.Email, user.Email),
    new Claim(JwtRegisteredClaimNames.GivenName, user.UserName),
    new Claim(JwtRegisteredClaimNames.NameId, user.Id.ToString()),
  };

  // GetRolesAync requires a User entity
  var roles = await _userManager.GetRolesAsync(user);

  claims.AddRange(roles.Select(role => new Claim(ClaimTypes.Role, role)));

  var creds = new SigningCredentials(_key, SecurityAlgorithms.HmacSha512Signature);

  var tokenDescriptor = new SecurityTokenDescriptor {
    Subject = new ClaimsIdentity(claims),
      Expires = DateTime.Now.AddDays(365),
      SigningCredentials = creds,
      Issuer = _config["Token:Issuer"]
  };

  var tokenHandler = new JwtSecurityTokenHandler();

  var token = tokenHandler.CreateToken(tokenDescriptor);

  return tokenHandler.WriteToken(token);
}

Here is the extension method

public static async Task<User> FindByEmailFromClaimsPrinciple(this UserManager<User> input, ClaimsPrincipal user)
{
    var email = user?.Claims?.FirstOrDefault(x => x.Type == ClaimTypes.Email)?.Value;
    return await input.Users.SingleOrDefaultAsync(x => x.Email == email);
}
chuckd
  • 13,460
  • 29
  • 152
  • 331
  • I think your problem is not that you're calling the DB twice, is that you're doing 2 different things in the controller 1. generating a token and 2. fetching data. usually generating a token is done by an authentication server / endpoint and then the client uses that token to fetch data through APIs. – LLL Jun 11 '23 at 09:49
  • "authentication server / endpoint" - huh! Do you have any examples you can point me too? In my app, this method does get the token and fetches user data. The client then uses the token in the httpheaders, so I think I am doing what you are suggesting! – chuckd Jun 11 '23 at 09:54
  • seems you don't need the first user call as your token is based on claims. Please consider the eShopOnWeb repo to check the token generation - it doesn't uses db roundtrips (https://github.com/dotnet-architecture/eShopOnWeb/blob/e7d71ea57289a1c3e7b322a53d5a2d2dc36d01f9/src/Web/Controllers/UserController.cs#L100). – Adalyat Nazirov Jun 11 '23 at 10:17
  • This repo still uses _userManager.GetRolesAsync(user) to get roles, where it takes a user as parameter. – chuckd Jun 11 '23 at 10:37
  • I agree with @AdalyatNazirov that you mix two different things in a single action. However if you really want to get the roles of a user, the [default implementation](https://source.dot.net/#Microsoft.AspNetCore.Identity.EntityFrameworkCore/UserStore.cs,6a121790e9a41f24) only touches the `Id` property of the user. And you are still need Email and Name from the db. If you **really** need the roles of a given user without loading the entire entity, you could use `await _userManager.GetRolesAsync(new User { Id = userId });` though. – mbd Jun 12 '23 at 17:45
  • oh! I didn't realize I could pass in a new User object with the id I'm looking for? Why doesn't MS just have a method that accepts an id then? ex. GetRolesAsync(2) – chuckd Jun 13 '23 at 19:13

1 Answers1

0
  1. Step 1 : Create a token when User Registers or Logs in.

  2. Step 2 : Use HttpContext on remaining controllers to get the current user.

In the above GetCurrentUser() method, you can mark [Authorize] attribute and get the current user directly from HttpContext than getting it from DB.

Ajay Managaon
  • 450
  • 2
  • 9
  • I havent followed the tutorials : But from what I see, I can tell that, GetCurrentUser method does not have any route mentioned at all. Where you see for login you have "HttpPost('login')". I think that, on the client side, he is trying to fetch Current Logged in user on the subsequent pages, directly by the controller. Where as Login is where you are passing credentials and getting token. The are serving different purposes here. – Ajay Managaon Jun 14 '23 at 06:07
  • The is the way its done, when you first authenticate & authorize a user you need to fetch that user and their permissions, you only want to call this once. – Jeremy Thompson Jun 14 '23 at 08:07
  • @AjayManagaon I know HttpContext is memory based, do you know how much faster fetching a "User" from HttpContext would be versus UserManager(DB)? Given current DB's are most likely SSD's, wouldn't the difference probably be negligible? – chuckd Jun 15 '23 at 05:17
  • There is some variability, but I would generally expect HttpContext to be microseconds and DB to be milliseconds. – iwalkbarefoot Jun 15 '23 at 22:13
  • @iwalkbarefoot Do you know how long my data (id, name, email) will be available in Httpcontext. Does their data persist as long as the user is logged in and their token hasn't expired? What if there's 1 million+ users logged in, wouldn't this consume a hearty amount of memory to store info for each user? – chuckd Jun 15 '23 at 23:07
  • @user1186050 That information is being stored on the token passed in with the request. Take one of the jwt tokens from your test environment and run it through https://jwt-decoder.com/ and you'll see e-mail, name and id are visible. That is why it's important to limit how many claims you put on the token as it increases every authenticated request. – iwalkbarefoot Jun 16 '23 at 13:18
  • @iwalkbarefoot Maybe you can provide me with some advice. In my app, I store 3 values (name, id, email) in the token because these three values are needed in ~80% of my controllers methods. Id to fetch data, then in most cases I need the users email and name to send out an email at the end of each controller. I thought that storing the name/email in the token would be a time saver as I wouldn't have query the DB just for the users name and email saving me 1 DB fetch per controller method. Any thoughts on this approach good/bad/no opinion? Ex. above method shows 2 DB fetches. – chuckd Jun 16 '23 at 15:26
  • But if I have the name and email stored in the HttpContext.User then I would only need to fetch from my DB 1 time. Saving time and DB cycles. – chuckd Jun 16 '23 at 15:48
  • @user1186050 I think that's a reasonable design, id, name and email are all pretty common claims, things to watch out for, if a user updates their name or e-mail inside of your application those updates would not go into effect immediately – iwalkbarefoot Jun 19 '23 at 19:07