fix: Simplify subject attribute matching #1348
Conversation
elsand
changed the title
📝 WalkthroughWalkthroughThe pull request introduces significant 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (3)
Line range hint
81-101: LGTM: Test refactoring improves clarityThe test renaming from legacy enterprise users to exchanged tokens better reflects the current authorization model. The comment clearly documents the claim priority logic.
Consider adding a brief XML documentation comment to explain the purpose of this test case and the claim priority rules it validates.
138-140: Document claim priority for system usersWhile the test correctly validates the single attribute policy, it would be valuable to document the priority order when multiple claims are present (authorization_details, pid, consumer).
Consider adding a comment explaining the expected priority order:
public void CreateDialogDetailsRequestShouldReturnCorrectRequestForSystemUser() { + // Claim priority order: + // 1. authorization_details (systemuser_id) + // 2. pid + // 3. consumer // Arrange var request = CreateDialogDetailsAuthorizationRequest(Also applies to: 156-156
Line range hint
1-365: Consider adding error case testsWhile the test coverage for successful scenarios is comprehensive, consider adding tests for error cases such as:
- Invalid claim formats
- Missing required claims
- Unsupported claim combinations
Would you like me to help generate these additional test cases?
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
104-104: Specify authentication type when creatingClaimsIdentityWhen creating a new
ClaimsPrincipalwithClaimsIdentity, the authentication type is not specified. To ensure accurate representation, consider specifying an authentication type to avoid potential issues with authentication-sensitive logic.Apply this diff to specify authentication type:
- var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(new[] { claim })); + var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity(new[] { claim }, authenticationType: "Custom"));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3 hunks)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (6 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
🔇 Additional comments (2)
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (2)
Line range hint
1-365: Verify removal of pure Maskinporten token supportThe changes effectively remove support for pure Maskinporten tokens using only consumer claims. However, let's verify this is consistently applied across the codebase.
#!/bin/bash # Search for any remaining support of pure consumer claims rg -A 5 "consumer.*only" --type cs rg -A 5 "Maskinporten.*token" --type cs
26-26: LGTM: Changes align with single attribute policyThe changes correctly implement and validate the new policy of using a single subject attribute. The switch from "consumer" to "authorization_details" claim and the assertion for a single attribute align with the PR objectives.
Let's verify this is consistently applied across the codebase:
Also applies to: 46-46
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (1)
Line range hint
1-348: Consider adding tests for negative scenariosTo fully validate the PR objectives, consider adding tests for:
- Verifying that multiple subject attributes are rejected
- Confirming that pure Maskinporten tokens with only consumer claims are not supported
Would you like me to help generate these additional test cases?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3 hunks)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (10 hunks)
🔇 Additional comments (7)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3)
81-87: LGTM! Well-documented PDP requirementsThe comments clearly explain the rationale behind single subject attribute restriction and the optimization strategy.
113-113: Verify UserTypeValidationMiddleware referenceThe error message references
UserTypeValidationMiddleware. Let's verify its existence to ensure the error message remains accurate.✅ Verification successful
The error message correctly references the UserTypeValidationMiddleware
The error message in
DecisionRequestHelper.csaccurately refers to theUserTypeValidationMiddleware, which indeed enforces user type validation during authentication. The middleware exists and properly handles unknown user types by returning a 403 Forbidden response, making the UnreachableException's message both accurate and helpful for debugging.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for UserTypeValidationMiddleware in the codebase rg "UserTypeValidationMiddleware" -lLength of output: 220
Script:
#!/bin/bash # Let's verify the implementation of UserTypeValidationMiddleware ast-grep --pattern 'class UserTypeValidationMiddleware { $$$ }'Length of output: 67
Script:
#!/bin/bash # Let's check the content of the middleware file to verify its implementation cat src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/UserTypeValidationMiddleware.csLength of output: 1613
16-19: 🛠️ Refactor suggestionConsider consolidating duplicate claim type constants
The constants
UserIdClaimType("urn:altinn:userid") andAttributeIdUserId("urn:altinn:userid") have identical values. Consider consolidating them to maintain a single source of truth.Apply this diff to use a single constant:
- private const string UserIdClaimType = "urn:altinn:userid"; - private const string AttributeIdUserId = "urn:altinn:userid"; + private const string AttributeIdUserId = "urn:altinn:userid"; + private const string UserIdClaimType = AttributeIdUserId;Also applies to: 28-28
⛔ Skipped due to learnings
Learnt from: elsand PR: digdir/dialogporten#1348 File: src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs:0-0 Timestamp: 2024-10-25T07:39:05.807Z Learning: In the codebase, constants for token claims and XACML attribute IDs may have the same value but are kept separate to maintain clear categorization, as not all claims map to identical attribute IDs.tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (4)
26-26: LGTM: Changes align with single subject attribute policyThe modifications correctly implement the PR's objective of restricting subject attribute matching to a single attribute. The assertion at line 46 effectively validates this requirement.
Also applies to: 46-46
81-81: LGTM: Improved test clarity and alignment with token handlingThe method rename better reflects its purpose, testing exchanged tokens rather than legacy enterprise users. The implementation correctly validates single attribute matching and uses appropriate claims.
Also applies to: 89-89, 100-100
165-165: LGTM: Consistent claim usage across test methodsThe changes demonstrate consistent use of the
pidclaim across different test scenarios, properly aligning with the PR's objective of simplifying subject attribute matching.Also applies to: 190-190, 212-218
138-140: Verify the necessity of multiple claims for system usersWhile the single attribute assertion is correctly implemented, the test maintains both
authorization_detailsandconsumerclaims. According to the PR objectives, we're moving away from consumer claims.Also applies to: 156-156
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs
Outdated
Show resolved
Hide resolved
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (1)
264-276: Consider using a more relevant invalid claim for the testThe test correctly verifies the error case, but uses the deprecated "consumer" claim. Consider using a different invalid claim that better represents the current authentication model.
- ("consumer", "somevalue") + ("some_invalid_claim", "somevalue")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3 hunks)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (12 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
Learnt from: elsand PR: digdir/dialogporten#1348 File: src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs:0-0 Timestamp: 2024-10-25T07:39:05.807Z Learning: In the codebase, constants for token claims and XACML attribute IDs may have the same value but are kept separate to maintain clear categorization, as not all claims map to identical attribute IDs.
🔇 Additional comments (7)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (2)
18-18: Verify the "Rar" prefix in constant nameThe prefix "Rar" in
RarAuthorizationDetailsClaimTypeseems unusual. Should this be "Raw" or is it an acronym? Consider clarifying the naming if it's not intentional.
79-110: Well-structured implementation of single subject attribute selectionThe refactored implementation excellently achieves the PR's objective of restricting to a single subject attribute. The code:
- Clearly documents the PDP requirements and rationale
- Implements proper priority order for attribute selection
- Uses modern C# pattern matching for clean and maintainable code
- Provides clear error handling with actionable messages
tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (5)
25-25: LGTM: Changes align with single subject attribute requirementThe changes correctly implement the simplified subject attribute matching by:
- Using the appropriate authorization_details claim
- Adding assertion to verify only one attribute is present
Also applies to: 45-45
Line range hint
80-99: LGTM: Method rename and implementation improve clarityThe changes appropriately:
- Rename the method to better reflect its purpose
- Update the claims to match the new single attribute requirement
- Add assertion to verify single attribute constraint
137-138: LGTM: System user test properly implements single attribute requirementThe changes correctly implement the simplified attribute matching for system users and verify the single attribute constraint.
Also applies to: 154-154
163-163: LGTM: Consistent claim updates across test methodsThe changes maintain consistency by using "pid" claim across different test scenarios, aligning with the simplified subject attribute approach.
Also applies to: 188-188, 215-215
240-240: Comment already addressed in previous review
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
114-116: Consider enhancing the error message with attempted claim types.The error message could be more helpful for debugging by including which claim types were attempted.
Consider this enhancement:
- "Unable to find a suitable subject attribute for the authorization request. Having a known user type should be enforced during authentication (see UserTypeValidationMiddleware)."), + $"Unable to find a suitable subject attribute for the authorization request. Attempted claim types: {string.Join(", ", claims.Select(c => c.Type))}. Having a known user type should be enforced during authentication (see UserTypeValidationMiddleware)."),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3 hunks)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs (11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
Learnt from: elsand PR: digdir/dialogporten#1348 File: src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs:0-0 Timestamp: 2024-10-25T07:39:05.807Z Learning: In the codebase, constants for token claims and XACML attribute IDs may have the same value but are kept separate to maintain clear categorization, as not all claims map to identical attribute IDs.
🔇 Additional comments (4)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (4)
17-18: LGTM! Well-structured constants with clear prioritization.The new constants and prioritized list align well with the PR objective of simplifying subject attribute matching. The comment clearly explains the importance of the order in
PrioritizedClaimTypes.Also applies to: 28-30
81-88: LGTM! Well-documented implementation of single subject attribute selection.The implementation effectively uses pattern matching and clearly documents the PDP's requirements regarding subject attributes. The prioritization logic aligns perfectly with the PR objectives.
89-109: LGTM! Clean and modern implementation using C# features.The code effectively uses modern C# features like collection expressions and pattern matching, making it both concise and readable.
81-116: Verify the impact of removing multiple subject attributes support.Since this change removes support for multiple subject attributes and pure Maskinporten tokens, we should verify that no existing code depends on this functionality.
Let's check for any direct usage of the removed functionality:
✅ Verification successful
Let me analyze the usage patterns and dependencies before making a conclusion about the impact of this change.
Based on the gathered information, I can now generate the final response:
The change to enforce single subject attribute is safe to proceed
The modification is aligned with the security requirements of the PDP (Policy Decision Point) and is properly enforced through the UserTypeValidationMiddleware. The middleware ensures that a valid user type is determined during authentication, making the single subject attribute approach both secure and consistent.
- The change is documented in the code comments explaining the security reasons
- UserTypeValidationMiddleware validates the user type during authentication
- No existing code relies on multiple subject attributes
- Tests verify the single attribute behavior for each user type (pid, userid, systemuser)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for potential usage of multiple subject attributes or Maskinporten tokens # Look for patterns that might indicate usage of multiple subject attributes rg -l "consumer.*claims?" .Length of output: 29
Script:
#!/bin/bash # Let's search for references to Maskinporten and authorization patterns rg -i "maskinporten|subject.*attribute" -A 3 . # Search for any tests or configurations related to authorization fd -e cs -e json "auth|test" --exec grep -l "claims\|subject\|maskinporten" {} \; # Look for the specific claim types being used ast-grep --pattern 'AttributeId = $_'Length of output: 53832
Script:
#!/bin/bash # Let's examine the test files to understand the expected behavior rg -A 10 "CreateAccessSubjectCategory" ./tests/Digdir.Domain.Dialogporten.Infrastructure.Unit.Tests/DecisionRequestHelperTests.cs # Look for any middleware or authentication related code that might be affected rg -l "UserTypeValidationMiddleware" . # Check for any related authorization request implementations ast-grep --pattern 'class $_ : $_AuthorizationRequest'Length of output: 413
Script:
#!/bin/bash # Let's check the UserTypeValidationMiddleware implementation to understand the authentication flow cat ./src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/UserTypeValidationMiddleware.cs # Look for any usage of multiple subject attributes in tests rg -A 5 "multiple.*subject.*attributes?" ./tests/ # Check for any migration or breaking changes documentation rg -i "breaking.*change|migration|upgrade" ./CHANGELOG.mdLength of output: 3371
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
114-120: Consider enhancing error handling robustness
The MinBy operation assumes all attribute IDs exist in PrioritizedClaimTypes. Consider adding a safety check:
.MinBy(x => PrioritizedClaimTypes.IndexOf(x!.Attribute[0].AttributeId)) switch
{
- { } validCategory => new List<XacmlJsonCategory> { validCategory },
+ { } validCategory when PrioritizedClaimTypes.Contains(validCategory.Attribute[0].AttributeId)
+ => new List<XacmlJsonCategory> { validCategory },
_ => throw new UnreachableException(
"Unable to find a suitable subject attribute for the authorization request. Having a known user type should be enforced during authentication (see UserTypeValidationMiddleware)."),
};Also, consider making the error message more self-contained by including the expected claim types directly in the message rather than referencing the middleware.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
Learnt from: elsand
PR: digdir/dialogporten#1348
File: src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs:0-0
Timestamp: 2024-10-25T07:39:05.807Z
Learning: In the codebase, constants for token claims and XACML attribute IDs may have the same value but are kept separate to maintain clear categorization, as not all claims map to identical attribute IDs.
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (2)
15-34: Well-organized constants with clear prioritization logic!
The constants are logically grouped, and the addition of PrioritizedClaimTypes with its explanatory comment effectively supports the PR's objective of restricting to a single subject attribute.
85-92: Excellent documentation of PDP requirements and constraints!
The comments clearly explain the rationale behind single subject attribute restriction and the prioritization logic, making the code self-documenting and maintainable.
🤖 I have created a release *beep* *boop* --- ## [1.27.1](v1.27.0...v1.27.1) (2024-10-30) ### Bug Fixes * Simplify subject attribute matching ([#1348](#1348)) ([55159b7](55159b7)) --- 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 simplifies the subject attribute mapping to only use a single attribute, as multiple subject attributes are explicitly disallowed by the PDP (eg. for system users). In fact, for external use, only a single subject attribute is ever required (eg. sending the authlvl attribute is not supported, as it is the PEPs responsibility to enforce any obligations returned from the PDP).
In addition, support for allowing pure Maskinporten tokens (using only consumer-claims) has been removed, as this is not officially supported in the Altinn Authorization model; only userid/pid/systemuserid will cause the PDP to resolve roles/access packages in order to match policy rules, so the only way sending organization numbers as subject claims is if the policy itself contains hard coded organization numbers, which is discouraged (should access lists for that).
Note that urn:altinn:org (ie serviceowner acronym claim types) are left out, as authenticated service owners should not use the end user APIs (this would potentially leak information that we only want to make available to the end users).
Related Issue(s)
See previous PR (#1340) and slack thread.
Verification
Summary by CodeRabbit
New Features
UnreachableExceptionfor invalid user types.Bug Fixes
Tests