fix: Fix ID-porten acr claim parsing #1299
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to 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
|
elsand
changed the title
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (2)
42-58: LGTM: Well-structured test for Altinn authentication level prioritizationThe test is well-implemented, following the Arrange-Act-Assert pattern. It correctly verifies that the "urn:altinn:authlevel" claim takes precedence over the "acr" claim when both are present.
Consider adding an additional test case where the "urn:altinn:authlevel" claim has a lower value than the "acr" claim to ensure the prioritization works correctly in all scenarios.
6-59: Consider adding more test cases for comprehensive coverageThe current test suite covers the main scenarios well. To further improve the test coverage, consider adding the following test cases:
- Test with an invalid "acr" claim value to ensure proper error handling.
- Test with a missing "acr" claim to verify the behavior when the claim is not present.
- Test with an "acr" claim value that doesn't match any known level to ensure proper handling of unknown values.
- Test with a non-numeric "urn:altinn:authlevel" claim value to verify error handling.
These additional test cases will help ensure the robustness of the
TryGetAuthenticationLevelmethod across various scenarios.src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (2)
183-195: Defineacrclaim values as constantsFor maintainability and to prevent potential typos, consider defining the
acrclaim values as constants. This practice promotes reuse and makes it easier to manage these values across the codebase.You can add constants at the beginning of the class:
private const string IdportenLoaSubstantial = "idporten-loa-substantial"; private const string IdportenLoaHigh = "idporten-loa-high";And update the switch statement:
authenticationLevel = claimValue switch { - "idporten-loa-substantial" => 3, - "idporten-loa-high" => 4, + IdportenLoaSubstantial => 3, + IdportenLoaHigh => 4, _ => null };
183-195: Handle unexpectedacrclaim values explicitlyIf an unrecognized
acrclaim value is encountered, it might be helpful to log a warning for diagnostic purposes. This can aid in identifying misconfigurations or updates to the authentication system.Would you like assistance in adding logging to handle unexpected
acrclaim values?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (1 hunks)
🧰 Additional context used
🔇 Additional comments (4)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (3)
8-23: LGTM: Well-structured test for Level 3 authenticationThe test is well-implemented, following the Arrange-Act-Assert pattern. It correctly verifies that the "idporten-loa-substantial" claim is parsed as authentication level 3.
25-40: LGTM: Well-structured test for Level 4 authenticationThe test is properly implemented, adhering to the Arrange-Act-Assert pattern. It accurately verifies that the "idporten-loa-high" claim is parsed as authentication level 4.
1-59: Overall, well-implemented test suite for authentication level parsingThe test suite provides good coverage for the main scenarios of ID-porten and Altinn authentication level parsing. The tests are well-structured, following the Arrange-Act-Assert pattern, and effectively validate the behavior of the
TryGetAuthenticationLevelmethod.The implementation aligns well with the PR objectives of fixing ID-porten acr claim parsing. The tests cover both ID-porten levels (3 and 4) and the prioritization of Altinn authentication levels.
To further enhance the robustness of the test suite, consider implementing the additional test cases suggested in the previous comments. This will ensure comprehensive coverage of edge cases and error scenarios.
Great job on implementing these tests! They will help maintain the reliability of the authentication level parsing functionality.
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1)
177-180: Proper handling ofAltinnAuthLevelClaimThe code correctly attempts to retrieve the
AltinnAuthLevelClaimand parses it into an integer. This ensures that authentication levels from Altinn are appropriately handled.
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs
Show resolved
Hide resolved
🤖 I have created a release *beep* *boop* --- ## [1.25.0](v1.24.0...v1.25.0) (2024-10-17) ### Features * **applications:** add scalers for cpu and memory ([#1295](#1295)) ([eb0f19b](eb0f19b)) * **infrastructure:** create new yt01 app environment ([#1291](#1291)) ([1a1ccc0](1a1ccc0)) * **service:** add permissions for service-bus ([#1305](#1305)) ([7bf4177](7bf4177)) * **service:** deploy application in container apps ([#1303](#1303)) ([a309044](a309044)) ### Bug Fixes * **applications:** add missing property for scale configuration ([3ffb724](3ffb724)) * **applications:** use correct scale configuration ([#1311](#1311)) ([b8fb3cc](b8fb3cc)) * Fix ID-porten acr claim parsing ([#1299](#1299)) ([8b8862f](8b8862f)) * **service:** ensure default credentials work ([#1306](#1306)) ([b1e6a14](b1e6a14)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
This fixes acr-parsing (authentication level) for real ID-porten tokens
Related Issue(s)
N/A
Verification
Note
There is a bug in the token generator in https://github.com/Altinn/AltinnTestTools, which is still producing the old "Level3" and "Level4" acr-values.
Summary by CodeRabbit
New Features
Tests