fix(auth): Allow .noconsent scope in EndUser auth policy #1760
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new authorization scope Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
![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 (4)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicyBuilderExtensions.cs (1)
8-8: Consider documenting the reason for making ScopeClaim public.While making the constant public allows for reuse, it's good practice to document why this visibility change was necessary, especially for security-related constants.
Consider adding a code comment explaining the rationale:
+ // Made public to enable consistent scope claim validation across the application public const string ScopeClaim = "scope";src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs (1)
17-17: LGTM! Consider adding documentation for the new scope.The EndUserNoConsent scope is correctly implemented and matches the GraphQL version. Consider adding XML documentation to explain its purpose and usage scenarios.
Add documentation to clarify the scope's purpose:
+ /// <summary> + /// Scope for end-user access without requiring explicit consent. + /// Used in scenarios where consent requirements are waived. + /// </summary> public const string EndUserNoConsent = "digdir:dialogporten.noconsent";src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs (1)
31-40: Optimize scope validation performance and improve code organization.The current implementation has some minor performance and maintainability considerations:
- The
ToList()call creates an unnecessary allocation sinceContainsworks withIEnumerable<T>.- The scope validation logic could be extracted to improve reusability.
Consider applying these improvements:
+ private static bool HasValidEndUserScope(AuthorizationHandlerContext context) + { + var userScopes = context.User + .FindAll(AuthorizationPolicyBuilderExtensions.ScopeClaim) + .Select(s => s.Value); + + return userScopes.Contains(AuthorizationScope.EndUser) || + userScopes.Contains(AuthorizationScope.EndUserNoConsent); + } options.AddPolicy(AuthorizationPolicy.EndUser, builder => builder .Combine(options.DefaultPolicy) - .RequireAssertion(context => - { - var userScopes = context.User - .FindAll(AuthorizationPolicyBuilderExtensions.ScopeClaim) - .Select(s => s.Value) - .ToList(); - - return userScopes.Contains(AuthorizationScope.EndUser) || - userScopes.Contains(AuthorizationScope.EndUserNoConsent); - })); + .RequireAssertion(HasValidEndUserScope));src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (1)
35-44: Consider extracting shared authorization logic to avoid duplication.The EndUser authorization logic is duplicated between the WebApi and GraphQL AuthorizationOptionsSetup classes. This could lead to maintenance issues if the logic needs to be updated.
Consider creating a shared authorization helper class that both implementations can use:
+ namespace Digdir.Domain.Dialogporten.Common.Authorization; + + public static class AuthorizationHelpers + { + public static bool HasValidEndUserScope(AuthorizationHandlerContext context) + { + var userScopes = context.User + .FindAll(AuthorizationPolicyBuilderExtensions.ScopeClaim) + .Select(s => s.Value); + + return userScopes.Contains(AuthorizationScope.EndUser) || + userScopes.Contains(AuthorizationScope.EndUserNoConsent); + } + }Then use it in both places:
options.AddPolicy(AuthorizationPolicy.EndUser, builder => builder .Combine(options.DefaultPolicy) - .RequireAssertion(context => - { - var userScopes = context.User - .FindAll(AuthorizationPolicyBuilderExtensions.ScopeClaim) - .Select(s => s.Value) - .ToList(); - - return userScopes.Contains(AuthorizationScope.EndUser) || - userScopes.Contains(AuthorizationScope.EndUserNoConsent); - })); + .RequireAssertion(AuthorizationHelpers.HasValidEndUserScope));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicyBuilderExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicyBuilderExtensions.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
- GitHub Check: build / build-and-test
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicyBuilderExtensions.cs (1)
8-8: LGTM! Maintains consistency with WebApi implementation.The visibility change matches the WebApi version, ensuring consistent authorization handling across both implementations.
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs (1)
17-17: Verify the new scope's integration with existing authorization flows.The new EndUserNoConsent scope has been added following the correct naming convention. Let's verify its usage across the codebase.
✅ Verification successful
The EndUserNoConsent scope is properly integrated into authorization flows
The scope is correctly configured in both GraphQL and WebApi authorization setups, working as an alternative to the EndUser scope. The implementation is consistent across both projects, ensuring uniform authorization behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usage of the new scope in authorization policies and tests echo "Searching for EndUserNoConsent scope usage..." rg -l "EndUserNoConsent" --type cs echo -e "\nSearching for related authorization policy configurations..." ast-grep --pattern 'options.AddPolicy($_, policy => policy.RequireScope($_))' --lang csLength of output: 704
Script:
#!/bin/bash # Check scope configuration and usage echo "=== GraphQL AuthorizationOptionsSetup ===" rg -A 5 -B 5 "EndUserNoConsent" src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs echo -e "\n=== WebApi AuthorizationOptionsSetup ===" rg -A 5 -B 5 "EndUserNoConsent" src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs echo -e "\n=== Searching for [Authorize] attributes with EndUserNoConsent ===" ast-grep --pattern '[Authorize(Policy = $_)]' --lang cs | rg -A 2 -B 2 "EndUserNoConsent"Length of output: 1699
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs (1)
31-40: Verify the security implications of allowing EndUserNoConsent scope.The changes broaden the authorization scope by allowing either
EndUserorEndUserNoConsent. Please ensure this aligns with the security requirements and document any limitations or restrictions associated with theEndUserNoConsentscope.Run this script to analyze the usage and implications:
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (1)
35-44: Verify consistent behavior between WebApi and GraphQL implementations.Since both WebApi and GraphQL implementations now support the
EndUserNoConsentscope, ensure they behave consistently across different authentication scenarios.Run this script to compare the implementations:
✅ Verification successful
Authorization implementations are identical and consistent
The WebApi and GraphQL implementations handle the
EndUserNoConsentscope in exactly the same way, with identical scope definitions and authorization logic. Both implementations use the same policy configuration that allows access with either theEndUserorEndUserNoConsentscope.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Compare authorization implementations echo "Comparing authorization setup..." rg -l "AuthorizationOptionsSetup" --type cs echo -e "\nComparing EndUser policy configurations..." rg "AddPolicy.*EndUser.*RequireAssertion" -A 10 --type cs echo -e "\nSearching for potential differences in scope handling..." rg "AuthorizationScope" -A 5 --type csLength of output: 30830
oskogstad
deleted the
fix/using-noconsent-scope-on-enduser-api-fails-with-500-internal-server-error
branch
🤖 I have created a release *beep* *boop* --- ## [1.47.5](v1.47.4...v1.47.5) (2025-01-30) ### Bug Fixes * **auth:** Allow .noconsent scope in EndUser auth policy ([#1760](#1760)) ([d770779](d770779)) * **auth:** Split values when checking EndUser scopes ([#1764](#1764)) ([5957e7d](5957e7d)) ### Miscellaneous Chores * Add FormSubmitted and FormSaved to ActivityType ([#1742](#1742)) ([4b9bad0](4b9bad0)) * **deps:** update dependency ziggycreatures.fusioncache to v2 ([#1752](#1752)) ([dd24928](dd24928)) * **perfomance:** Fixing github action to run performance tests in k8s ([#1739](#1739)) ([166d53d](166d53d)) * Remove old OccuredAt property on DomainEvent ([#1758](#1758)) ([67ee75d](67ee75d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: Are Almaas <[email protected]>
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)