fix(auth): Split values when checking EndUser scopes #1764
Conversation
📝 WalkthroughWalkthroughThe pull request modifies 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
🧹 Nitpick comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs (1)
35-35: Consider adding additional string handling for robustness.While the current implementation works for well-formed scopes, consider adding guards against malformed input:
- .SelectMany(s => s.Value.Split(" ")) + .SelectMany(s => s.Value?.Split(" ", StringSplitOptions.RemoveEmptyEntries) ?? Array.Empty<string>()) + .Select(s => s.Trim()) + .Where(s => !string.IsNullOrEmpty(s))src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (2)
39-39: Consider adding additional string handling for robustness.Apply the same robustness improvements as suggested for the WebApi version:
- .SelectMany(s => s.Value.Split(" ")) + .SelectMany(s => s.Value?.Split(" ", StringSplitOptions.RemoveEmptyEntries) ?? Array.Empty<string>()) + .Select(s => s.Trim()) + .Where(s => !string.IsNullOrEmpty(s))
Line range hint
1-70: Consider extracting common authorization logic.Both WebApi and GraphQL implementations share identical authorization setup code. Consider extracting this common logic into a shared authorization configuration class to maintain consistency and reduce duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dry run deploy apps / Deploy graphql to test
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: build / build-and-test
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs (1)
35-35: LGTM! This correctly implements OAuth 2.0 scope handling.The change from
SelecttoSelectManywith space splitting properly handles OAuth 2.0's space-separated scope format, fixing how EndUser scopes are processed.src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs (1)
39-39: LGTM! Maintains consistency with WebApi implementation.The change correctly implements the same scope splitting logic as the WebApi version, ensuring consistent OAuth 2.0 scope handling across both endpoints.
🤖 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)