fix(webapi): Require base service provider scope on search endpoint #1476
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve the 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
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1)
32-36: LGTM! Consider extracting scope constants to a shared configuration.The changes look good:
- Making
ServiceProviderScopeprivate improves encapsulation- The new
ServiceProviderSearchScopefollows consistent naming- TODO comment accurately reflects multiple scopes
As mentioned in issue #647, consider extracting these scope constants to a shared auth library or configuration to avoid duplication and maintain consistency across the codebase.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs(2 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1)
Line range hint 248-252: LGTM! Verify scope usage in authentication middleware.
The changes correctly address the 403 error by allowing either ServiceProviderScope or ServiceProviderSearchScope to grant ServiceOwner access.
Let's verify that these are the only places where the scopes are used:
✅ Verification successful
Let me gather more information about the scope usage in authorization checks.
LGTM! The scope usage is consistent across the codebase.
The verification confirms that both scopes (ServiceProviderScope and ServiceProviderSearchScope) are properly handled:
- Both scopes are defined as constants in WebApi and GraphQL authorization policies
- Authorization middleware correctly configures distinct policies for each scope
- The
GetUserTypemethod's changes align with the authorization setup, grantingServiceOwneraccess for either scope - No inconsistencies found in scope usage across the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other usages of the service provider scopes
# to ensure consistent handling across the codebase
# Search for any references to the scopes
rg -i "dialogporten\.serviceprovider"
# Search for scope-based authorization checks
ast-grep --pattern 'HasScope($scope)'
Length of output: 5788
Script:
#!/bin/bash
# Search for HasScope method usage with context
rg -A 3 "HasScope\("
# Search for authorization policy configurations
rg -A 5 "AddAuthorization|RequireScope"
Length of output: 12351
oskogstad
changed the title
oskogstad
dismissed
elsand’s stale review
Ref. Slack, keeping scopes separate for .search
This reverts commit a9d120f.
oskogstad
changed the title
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs (1)
38-39: LGTM! Consider adding documentation about policy inheritance.The change correctly implements the authorization hierarchy by requiring both ServiceProvider and ServiceProviderSearch scopes. This makes sense from a security perspective as search capabilities should be restricted to authenticated service providers.
Consider adding a comment to document the policy inheritance chain for clarity:
options.AddPolicy(AuthorizationPolicy.ServiceProviderSearch, builder => builder + // Inherits: DefaultPolicy -> ServiceProvider -> ServiceProviderSearch .Combine(options.GetPolicy(AuthorizationPolicy.ServiceProvider)!) .RequireScope(AuthorizationScope.ServiceProviderSearch));src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (1)
42-43: Consider adding a code comment explaining the policy inheritance.While the code is correct, adding a comment would help future maintainers understand why
ServiceProviderSearchspecifically inherits fromServiceProviderrather than using the default policy.+ // ServiceProviderSearch must inherit ServiceProvider policy to ensure proper authorization context .Combine(options.GetPolicy(AuthorizationPolicy.ServiceProvider)!)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (1)
42-43: LGTM! The authorization policy change correctly addresses the 403 issue.
The modification to combine ServiceProviderSearch with ServiceProvider policy instead of the default policy is the right approach, as it ensures that search operations inherit all necessary permissions from the service provider context.
Let's verify the authorization scopes are properly defined:
✅ Verification successful
Authorization scopes and policies are correctly defined and consistently used across both WebAPI and GraphQL
The verification confirms:
ServiceProviderSearchscope is properly defined as "digdir:dialogporten.serviceprovider.search"- Both WebAPI and GraphQL implementations consistently:
- Combine
ServiceProviderSearchpolicy withServiceProviderpolicy - Require the correct scope for authorization
- Combine
- The policy inheritance structure matches between both implementations, ensuring consistent behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the AuthorizationScope definitions and usage
# Expected: Find definitions of ServiceProvider and ServiceProviderSearch scopes
# and their usage in authorization policies
# Search for scope definitions
echo "=== Searching for scope definitions ==="
rg -A 1 "AuthorizationScope\." --type cs
# Search for policy usage
echo "=== Searching for policy usage ==="
rg -A 2 "ServiceProviderSearch|ServiceProvider.*scope" --type cs
Length of output: 13808
oskogstad
deleted the
fix/so-search-with-correct-scope-results-in-forbidden
branch
🤖 I have created a release *beep* *boop* --- ## [1.36.0](v1.35.0...v1.36.0) (2024-11-19) ### Features * **azure:** create azure monitor workspace ([#1485](#1485)) ([da0aa8f](da0aa8f)) ### Bug Fixes * **app:** Error details missing when user type is unknown ([#1493](#1493)) ([9fbd2cf](9fbd2cf)) * **azure:** enable public access for azure monitor ([#1496](#1496)) ([b0d5794](b0d5794)) * **azure:** ensure monitor workspace is reachable ([#1494](#1494)) ([dc7fc1f](dc7fc1f)) * **webapi:** Require base service provider scope on search endpoint ([#1476](#1476)) ([8c41f3d](8c41f3d)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
ServiceProviderSearchpolicy, improving specificity and security.