Repository navigation
fix(identity): enforce group and endpoint RequirePermission together - #1452
Merged
Merged
Conversation
RequiredPermissionAuthorizationHandler read only the most specific IRequiredPermissionMetadata, so an endpoint's .RequirePermission(B) replaced its route group's .RequirePermission(A) and the group gate failed open. The handler now reads every instance with GetOrderedMetadata, unions their permissions and requires all of them. A blank entry anywhere in the chain still denies. Closes #1429
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RequiredPermissionAuthorizationHandlerread the permission metadata withGetMetadata<IRequiredPermissionMetadata>(), which only returns the most specific entry. So if a route group called.RequirePermission(A)and an endpoint inside it called.RequirePermission(B), onlyBwas checked and the group's gate silently failed open for that endpoint. No kit endpoint gates a route group today, so nothing shipped was exposed, but anyone following the normal Minimal API group pattern could hit it.The handler now reads every entry with
GetOrderedMetadata, unions their permissions (case-insensitive, same as the attribute) and applies the all-of check from #1424 to that combined set. Endpoints with a single gate behave exactly as before. One small tightening: if any entry in the chain is blank (for example.RequirePermission("")on the group), the request is denied, matching the existing rule for a blank endpoint gate. The handler lives in the Identity module, so there is no BuildingBlocks change.Testing: I added a test-only endpoint
/__test/permissions/group/endpoint(group gate =Groups.Create, endpoint gate =Groups.Delete) next to the existing #1424 probe, and three integration tests inRequirePermissionAllOfTests: only the endpoint permission → 403, only the group permission → 403, both → 200. Against the old handler the "endpoint permission only" test failed (the request got through); with the fix all 5 tests in the class pass. The build is clean with warnings as errors, and the full solution suite passes: 2,140 tests, 0 failures, including 793 integration tests and 55 architecture tests.Docs: fullstackhero/docs#274
Closes #1429