Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ Improve authorization performance #1046

Open
MichelJansson opened this issue Nov 10, 2023 · 1 comment
Open

✨ Improve authorization performance #1046

MichelJansson opened this issue Nov 10, 2023 · 1 comment

Comments

@MichelJansson
Copy link

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
Copy link
Contributor

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...

Lanz86 added a commit to Lanz86/CleanArchitecture that referenced this issue Apr 20, 2024


Moved Roles authorization from Identity Service DB verification to verify Roles user Claim.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants