jasontaylordev / CleanArchitecture

Clean Architecture Solution Template for ASP.NET Core
MIT License
16.15k stars 3.46k forks source link

✨ Improve authorization performance #1046

Open MichelJansson opened 8 months ago

MichelJansson commented 8 months ago

Hi all! I've been looking into the IdentityService and specifically the AuthorizeAsync and IsInRolesAsync method that I use extensively to authorize the current user's commands and queries.

Each authorization is quite expensive since each request has to make at least one roundtrip to the database to fetch the user by id to create a ClaimsPrincipal. This of course is fine if we need to authorize any arbitrary user, but for the current user this seem unnecessary as we already have a claims principal - after all, that's where we extract the user id from in the first place in the CurrentUser service.

Would it not be better to use the claims principal we already have as much as possible? Especially for the AuthorizationBehaviour that might have a high hit rate.

This could be solved in a few ways;

  1. Add AuthorizeAsync method to the IUser service. I think this would be the nicest API, but for some reason does not work for me, as the application hangs (not crashes) when injecting IAuthorizationService into CurrentUser.
  2. Expose a ClaimsPrincipal property on the IUser service, and add a new AuthorizeAsync overload to the IdentityService that accepts a ClaimsPrincipal instead of a user id.
  3. ...?

Do you see any issues with this approach, or have better proposals?

Thanks

AkosLukacs commented 4 months ago

To add one more point: If I have an Authorize attribute with multiple Roles, like [Authorize(Roles = "Foo, Bar, Baz")] generates two database queries for each role request - one for getting the user, and one for getting the role information.

The quick fix would be to change _userManager.Users.SingleOrDefault(u => u.Id == userId) and _userManager.Users.FirstAsync(u => u.Id == userId) to _userManager.FindByIdAsync(userId) because the SQL UserStore calls FindAsync, and that call is cached because it's getting the database record by it's PK. The PR I sent reduces the number of select * from [AspNetUsers] where Id = xxxx db queries to one.

But!

The user and role information is already present in the ClaimsPrincipal:

image

And can be queried like _httpContextAccessor.HttpContext?.User.IsInRole("Administrator")

image

So some refactoring might be justified? Or is there some specific issue you tried to avoid organizing code this way? I do remember some weird issues the ClaimsPrincipal not noticing Role changes, but that could have been because I was hacking directly into the database...