fix: Authentication level claim is 0 in dialog token #1654
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes to JWT claim handling in two authentication extension files. Specifically, it adds a new using directive for Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Finishing Touches
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
|
Fargekritt
changed the title
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs
Outdated
Show resolved
Hide resolved
![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 (1)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/AuthenticationBuilderExtensions.cs (1)
14-14: Remove the commented out duplicate setting.This line duplicates the setting at line 29. Having multiple instances of the same configuration, even if commented out, can lead to confusion.
- // JwtSecurityTokenHandler.DefaultMapInboundClaims = false;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/AuthenticationBuilderExtensions.cs(3 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/AuthenticationBuilderExtensions.cs(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/AuthenticationBuilderExtensions.cs (1)
Learnt from: oskogstad
PR: digdir/dialogporten#1124
File: src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs:56-59
Timestamp: 2024-11-12T05:32:45.311Z
Learning: The `DialogTokenMiddleware` only adds claims to user identities; the `AuthorizationOptionsSetup` validates that the `dialogID` in the token matches the `dialogID` in the subscription query and returns a 401 Unauthorized if they don't match.
⏰ 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.WebApi/Common/Authentication/AuthenticationBuilderExtensions.cs (2)
4-4: LGTM!The added using directive is necessary for JWT token handling.
28-30: LGTM! This change fixes the authentication level claim parsing.Disabling the default claim mapping prevents the "acr" claim from being automatically mapped to its longer URI form, which should resolve the authentication level parsing issue.
Let's verify the impact on claim mapping:
✅ Verification successful
Verification successful: Claim mapping change is consistently applied and safe
The change to disable default claim mapping is correctly synchronized across both WebApi and GraphQL authentication configurations. All claim handling code in the codebase already uses the short "acr" form, with no dependencies on the long URI form found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any other code that might rely on the mapped claim name rg -l "http://schemas.microsoft.com/claims/authnclassreference" # Search for code handling the "acr" claim rg '"acr"' -A 5Length of output: 5609
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/AuthenticationBuilderExtensions.cs (2)
4-4: LGTM!The added using directive is necessary for JWT token handling.
27-29: LGTM! This change maintains consistency with WebApi implementation.Disabling the default claim mapping here ensures consistent behavior across both WebApi and GraphQL implementations.
Let's verify the claim handling in DialogTokenMiddleware:
✅ Verification successful
Verification successful: Claim handling is consistent across implementations
The disabled claim mapping aligns perfectly with both the DialogTokenMiddleware implementation and the JWT handling in WebApi, ensuring claims remain in their original format throughout the system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for DialogTokenMiddleware claim handling ast-grep --pattern 'class DialogTokenMiddleware { $$$ $_(ClaimsPrincipal $_) { $$$ } $$$ }'Length of output: 105
Script:
#!/bin/bash # Find files containing DialogToken rg -l "DialogToken" # Search for claim handling patterns ast-grep --pattern 'ClaimsPrincipal' # Backup search for middleware implementations rg -l "Middleware" --type csLength of output: 1803
Script:
#!/bin/bash # Get content of DialogTokenMiddleware cat src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/DialogTokenMiddleware.cs # Get content of both JWT selector middlewares for comparison cat src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/JwtSchemeSelectorMiddleware.cs cat src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/JwtSchemeSelectorMiddleware.csLength of output: 5839
|
Description
Changed IdportenAuthLevelClaim constant to fix parsing of Acr field in dialog token
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)