Support IAuthorizationRequirementData in the implementation-first approach #8303
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR introduces support for IAuthorizationRequirementData in the annotation based schema by extending the AuthorizeDirective with a metadata parameter. Key changes include adding a metadata parameter/property to AuthorizeDirective, updating the construction of authorization policies to include metadata requirements, and modifying the caching mechanism to use the AuthorizeDirective directly as a key.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/HotChocolate/Core/src/Authorization/AuthorizeDirective.cs | Removed cache key generation and added the metadata parameter and property. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Authorization.Tests/snapshots/* | Updated snapshot files to reflect authorization error responses. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Authorization.Tests/AuthorizeDirectiveTests.cs | Removed tests relying on cache key retrieval from AuthorizeDirective. |
| src/HotChocolate/AspNetCore/test/AspNetCore.Authorization.Tests/AuthorizationRequirementDataTests.cs | Added tests covering authorization logic with metadata requirements. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Authorization/DefaultAuthorizationHandler.cs | Updated BuildAuthorizationPolicy to incorporate metadata. |
| src/HotChocolate/AspNetCore/src/AspNetCore.Authorization/AuthorizationPolicyCache.cs | Changed cache keys from string values to AuthorizeDirective instances. |
| @@ -6,19 +6,15 @@ namespace HotChocolate.AspNetCore.Authorization; | |||
|
|
|||
| internal sealed class AuthorizationPolicyCache | |||
| { | |||
| private readonly ConcurrentDictionary<string, AuthorizationPolicy> _cache = new(); | |||
| private readonly ConcurrentDictionary<AuthorizeDirective, AuthorizationPolicy> _cache = new(); | |||
There was a problem hiding this comment.
Using AuthorizeDirective as the dictionary key may lead to cache misses if two different instances have equivalent values. Consider overriding Equals and GetHashCode on AuthorizeDirective or reverting to a value-based cache key to ensure consistent behavior.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
I think this is okay because the instance is at least bound to a field or type, and has reference-equality across requests.
This is needed for a directive that have no policy, no roles, but metadata.
AuthorizationRequirementDataTests.Multiple_NotAuthorized is a test...
sunghwan2789
changed the title
No description provided.