-
-
Notifications
You must be signed in to change notification settings - Fork 749
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
Align authorization behavior to regular ASP.NET Core #7408
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7408 +/- ##
==========================================
- Coverage 78.81% 77.83% -0.98%
==========================================
Files 2434 2815 +381
Lines 119868 141032 +21164
==========================================
+ Hits 94469 109770 +15301
- Misses 25399 31262 +5863
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
I don't get this part in the test setup: The iterator returns the same exact setup twice. Is this an oversight or intended like this? |
src/HotChocolate/AspNetCore/src/AspNetCore.Authorization/DefaultAuthorizationHandler.cs
Outdated
Show resolved
Hide resolved
dbe05f6
to
8f61b4e
Compare
Why do we output the name of the missing policy in the GraphQL error message?
The name could leak details about how the authorization works. Can we make this error generic? |
4bceb0d
to
c64dd91
Compare
6780041
to
8ca0dd3
Compare
We noticed some discrepancies between the ASP.NET Core
[Authorize]
attribute and Hot Chocolate's one.This PR
[Authorize]
attribute to properly generate the@authorize
directive, if bothroles
andpolicy
are specified (Diff: cb798d3)DefaultAuthorizationHandler
to construct anAuthorizationPolicy
out of the roles and specified policy (similar to what theAuthorizationMiddleware
of ASP.NET Core is doing), which is then evaluated through the registeredIAuthorizationService
. Previously the authorization code was custom and there wouldn't be aRoleRequirement
passed to theIAuthorizationService
. In our code we use a customIAuthorizationService
to apply fallbacks, so there was a mismatch between what was happening for our REST and GraphQL APIs. (Diff: bfde46f)null
, as the property isn't nullable (setting it tonull
would also have unexpected implications in regular ASP.NET Core) (Diff: c64dd91)I have also run this against our internal test suite of authorization tests and they're all passing now with these changes.