Support [AllowAnonymous] and [Authorize] simultaneously on a field #88
Conversation
|
Warning Rate limit exceeded@Shane32 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe conditional logic in the authorization visitor was updated so that validation is now performed on all fields, regardless of anonymous access settings. Additionally, a new parameterized test was introduced to verify authorization behavior when both parent and child fields have role requirements, with the child optionally allowing anonymous access. The README was updated to clarify the meaning of Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AuthorizationVisitor
participant Field
participant Validator
User->>AuthorizationVisitor: Request query execution
loop For each field in query
AuthorizationVisitor->>Validator: ValidateAsync(field)
Validator-->>AuthorizationVisitor: Validation result
end
AuthorizationVisitor-->>User: Authorization result
sequenceDiagram
participant Test
participant AuthorizationVisitor
participant ParentField
participant ChildField
participant Validator
Test->>AuthorizationVisitor: Validate query { parent { child } }
AuthorizationVisitor->>Validator: ValidateAsync(ParentField)
Validator-->>AuthorizationVisitor: Parent field validation result
AuthorizationVisitor->>Validator: ValidateAsync(ChildField)
Validator-->>AuthorizationVisitor: Child field validation result
AuthorizationVisitor-->>Test: Combined authorization result
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Tests/AuthorizationTests.cs (2)
765-772: Comment does not match the code – clarify to avoid confusionThe code applies the Role 2 requirement and (optionally)
AllowAnonymous()to_field(the parent field), not to the child field as the comment states. This works, but the mismatch will trip readers up.- // Set up child field to require Role2 and optionally be anonymous + // Set up the 'parent' field to require Role2 and, optionally, allow anonymous access
774-779: Minor robustness: trim split rolesWhen splitting the comma-separated role list, any accidental spaces (e.g.
"Role1, Role2") will become part of the role string. A small trim keeps the helper resilient:- var roles = userRoles.Split(','); - var claims = roles.Select(role => new Claim(ClaimTypes.Role, role)).ToArray(); + var claims = userRoles.Split(',') + .Select(r => r.Trim()) + .Where(r => r.Length > 0) + .Select(r => new Claim(ClaimTypes.Role, r)) + .ToArray();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/GraphQL.AspNetCore3/AuthorizationVisitorBase.cs(1 hunks)src/Tests/AuthorizationTests.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
- GitHub Check: format
🔇 Additional comments (1)
src/GraphQL.AspNetCore3/AuthorizationVisitorBase.cs (1)
60-60: ValidateAsync now always runs – double-check perf & duplicate-error impactWith the conditional check removed,
ValidateAsyncis awaited for every field, even whenfieldAnonymousAllowedistrue.
That is the intended functional change, but please confirm:
ValidateAsyncis inexpensive when early-exiting on anonymous fields, otherwise a hot query could see noticeable overhead.- Fields that are both
[AllowAnonymous]and[Authorize]will now receive two validation passes (type + field). Ensure this cannot surface two identicalAccessDeniedErrors for the same location.If either risk materialises, consider adding an inexpensive short-circuit inside
ValidateAsyncfor anonymous-only paths.
Coverage Report
|
Pull Request Test Coverage Report for Build 15600772670Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
This allows bypassing parent type's authorization while defining new requirements on the field. Previously any requirements on the field were ignored.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation