fix: Add system user id to identifying claims #1362
Conversation
|
Warning Rate limit exceeded@elsand has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 Walkthrough📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces enhancements 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: 0
🧹 Outside diff range and nitpick comments (2)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (1)
57-70: LGTM! The test follows good practices and verifies the core functionality.The test effectively verifies the extraction of system user ID from authorization details, following the established Arrange-Act-Assert pattern.
Consider enhancing the test coverage with additional test cases:
[Theory] [InlineData( // Valid case with multiple claims "[{\"type\":\"urn:altinn:systemuser\",\"systemuser_id\":[\"id1\"]}, {\"type\":\"other\"}]", "id1" )] [InlineData( // Empty array case "[]", null )] [InlineData( // Missing systemuser_id case "[{\"type\":\"urn:altinn:systemuser\"}]", null )] public void GetIdentifyingClaims_Should_Handle_Various_AuthorizationDetails(string authDetails, string? expectedId) { // Arrange var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity([ new Claim("authorization_details", authDetails) ])); // Act var identifyingClaims = claimsPrincipal.Claims.GetIdentifyingClaims(); // Assert if (expectedId != null) { Assert.Contains(identifyingClaims, c => c.Type == "urn:altinn:systemuser" && c.Value == expectedId); } else { Assert.DoesNotContain(identifyingClaims, c => c.Type == "urn:altinn:systemuser"); } }Also consider using a raw string literal for better JSON readability:
- new Claim("authorization_details", "[{\"type\":\"urn:altinn:systemuser\",\"systemuser_id\":[\"e3b87b08-dce6-4edd-8308-db887950a83b\"],\"systemuser_org\":{\"authority\":\"iso6523-actorid-upis\",\"ID\":\"0192:991825827\"},\"system_id\":\"1d81b874-f139-4842-bd0a-e5cc64319272\"}]") + new Claim("authorization_details", """ + [{ + "type": "urn:altinn:systemuser", + "systemuser_id": ["e3b87b08-dce6-4edd-8308-db887950a83b"], + "systemuser_org": { + "authority": "iso6523-actorid-upis", + "ID": "0192:991825827" + }, + "system_id": "1d81b874-f139-4842-bd0a-e5cc64319272" + }] + """)src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1)
113-119: Add null checks and consider performance optimization.While the implementation maintains consistency by reusing existing logic, consider these improvements:
- Add null parameter validation
- Consider caching the ClaimsPrincipal creation for List overload
Apply this diff to add null checks:
public static bool TryGetSystemUserId(this Claim claim, [NotNullWhen(true)] out string? systemUserId) => + ArgumentNullException.ThrowIfNull(claim), new List<Claim> { claim }.TryGetSystemUserId(out systemUserId); public static bool TryGetSystemUserId(this List<Claim> claimsList, [NotNullWhen(true)] out string? systemUserId) => + ArgumentNullException.ThrowIfNull(claimsList), new ClaimsPrincipal(new ClaimsIdentity(claimsList.ToArray())).TryGetSystemUserId(out systemUserId);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs(2 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs(1 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 (4)
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (1)
57-70: Verify cache key generation impact.
Since this change affects the generation of cache keys for authorization requests, ensure that there are corresponding tests in the caching-related test classes that verify the impact of including system user ID in the cache key generation.
✅ Verification successful
Cache key generation is properly implemented and includes system user claims
The verification shows that the cache key generation is correctly implemented and will include the system user identifier:
- The
DialogDetailsAuthorizationRequestusesGetIdentifyingClaims()in its cache key generation, which we verified in the test includes the system user identifier - The cache key is composed using all identifying claims:
claimsKey = string.Join(";", request.Claims.GetIdentifyingClaims().Select(c => $"{c.Type}:{c.Value}")); - The implementation ensures that different system users will get different cache keys, preventing authorization cache conflicts
No additional tests are needed as the current test verifies the core functionality that the cache key generation relies upon.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for cache-related test files that might need updates
rg -l "cache.*key.*test" --type cs
# Search for existing cache key generation tests
ast-grep --pattern 'class $_ {
$$$
[Fact]
$$$
void $_CacheKey$_() {
$$$
}
$$$
}'
Length of output: 135
Script:
#!/bin/bash
# Let's try a broader search for cache-related tests and implementations
rg -l "cache" --type cs
# Also search for any test files related to authorization or claims
rg -l "authorization.*test|claims.*test" --type cs
# Look for cache key generation implementation
ast-grep --pattern 'GetCacheKey($$$)'
Length of output: 880
Script:
#!/bin/bash
# Let's examine the AltinnAuthorizationClient implementation since it's likely to contain the cache key generation
cat src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
# Also look for any test files specifically for AltinnAuthorizationClient
rg -l "AltinnAuthorizationClient.*Tests?" --type cs
Length of output: 10030
Script:
#!/bin/bash
# Let's look at the cache key generation implementation and related tests
rg -l "GenerateCacheKey" --type cs
# And examine the implementation of the request classes
rg -A 10 "class DialogDetailsAuthorizationRequest" --type cs
rg -A 10 "class AuthorizedPartiesRequest" --type cs
Length of output: 6156
src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (2)
209-233:
Optimize performance and enhance null safety.
The implementation has several areas for improvement:
- Multiple enumerations of the claims collection could impact performance
- Potential null reference when using First without checking
- Consider documenting the impact on cache key generation
Apply this diff to optimize the implementation:
public static IEnumerable<Claim> GetIdentifyingClaims(this IEnumerable<Claim> claims)
{
- var claimsList = claims.ToList();
+ ArgumentNullException.ThrowIfNull(claims);
+
+ // Enumerate claims once
+ var (identifyingClaims, authDetailsClaim) = claims.Aggregate(
+ (Identifying: new List<Claim>(), AuthDetails: (Claim?)null),
+ (acc, claim) =>
+ {
+ if (claim.Type == AuthorizationDetailsClaim)
+ {
+ acc.AuthDetails = claim;
+ }
+ else if (claim.Type == PidClaim ||
+ claim.Type == ConsumerClaim ||
+ claim.Type == SupplierClaim ||
+ claim.Type == IdportenAuthLevelClaim ||
+ claim.Type.StartsWith(AltinnClaimPrefix, StringComparison.Ordinal))
+ {
+ acc.Identifying.Add(claim);
+ }
+ return acc;
+ });
- var identifyingClaims = claimsList.Where(c =>
- c.Type == PidClaim ||
- c.Type == ConsumerClaim ||
- c.Type == SupplierClaim ||
- c.Type == IdportenAuthLevelClaim ||
- c.Type.StartsWith(AltinnClaimPrefix, StringComparison.Ordinal)
- ).OrderBy(c => c.Type).ToList();
// If we have a RAR-claim, this is most likely a system user. We need to extract the systemuser-uuid
// from the authorization_details claim
- if (claimsList.Any(c => c.Type == AuthorizationDetailsClaim))
+ if (authDetailsClaim != null && authDetailsClaim.TryGetSystemUserId(out var systemUserId))
{
- var rarClaim = claimsList.First(c => c.Type == AuthorizationDetailsClaim);
- if (rarClaim.TryGetSystemUserId(out var systemUserId))
- {
- identifyingClaims.Add(new Claim(AuthorizationDetailsType, systemUserId));
- }
+ identifyingClaims.Add(new Claim(AuthorizationDetailsType, systemUserId));
}
- return identifyingClaims;
+ return identifyingClaims.OrderBy(c => c.Type);
}Let's verify the impact on cache key generation:
✅ Verification successful
Performance optimization suggestion can be simplified
The review comment's concerns about performance and null safety can be addressed with a simpler solution since the search results show that GetIdentifyingClaims is primarily used for generating cache keys in authorization requests (DialogSearchAuthorizationRequest and DialogDetailsAuthorizationRequest).
The current ordering by Type is important as it's used in cache key generation, so we should keep it. However, we can simplify the optimization to:
public static IEnumerable<Claim> GetIdentifyingClaims(this IEnumerable<Claim> claims)
{
- var claimsList = claims.ToList();
+ ArgumentNullException.ThrowIfNull(claims);
+ var claimsList = claims as List<Claim> ?? claims.ToList();
var identifyingClaims = claimsList.Where(c =>
c.Type == PidClaim ||
c.Type == ConsumerClaim ||
c.Type == SupplierClaim ||
c.Type == IdportenAuthLevelClaim ||
c.Type.StartsWith(AltinnClaimPrefix, StringComparison.Ordinal)
).OrderBy(c => c.Type).ToList();
var authDetailsClaim = claimsList.FirstOrDefault(c => c.Type == AuthorizationDetailsClaim);
if (authDetailsClaim != null && authDetailsClaim.TryGetSystemUserId(out var systemUserId))
{
identifyingClaims.Add(new Claim(AuthorizationDetailsType, systemUserId));
}
return identifyingClaims;
}This simpler change:
- Adds null check
- Optimizes for cases where input is already a List
- Reduces enumerations by using FirstOrDefault
- Maintains the ordering required for cache key generation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for cache key generation using identifying claims
rg -A 5 "GetIdentifyingClaims.*cache"
Length of output: 3308
Script:
#!/bin/bash
# Search for usages of GetIdentifyingClaims to understand its impact
rg -A 5 "GetIdentifyingClaims"
# Search for cache key generation or caching related code
rg -A 5 "\.GetCacheKey|\.CacheKey|IMemoryCache|ICacheEntry"
Length of output: 4894
221-230: Verify security implications of including system user ID in identifying claims.
The addition of system user ID to identifying claims could affect authorization caching and access control. Please ensure:
- The system user ID extraction is properly validated
- The cache key generation remains secure with the additional claim
- No unintended access is granted through cache key collisions
Let's verify the security implications:
✅ Verification successful
Based on the gathered information, I can now provide the final response:
Security implications of system user ID in identifying claims have been verified
The implementation shows proper validation and secure handling:
- System user ID is extracted only from valid authorization_details claims with proper JSON structure
- The TryGetSystemUserId method includes multiple validation layers:
- Validates authorization details claim existence
- Verifies JSON structure and required fields
- Ensures non-empty system user IDs array
- Cache key generation uses secure hashing (SHA256) of all identifying claims
- Unit tests verify the correct extraction and validation of system user claims
The changes maintain security while properly identifying system users in the authorization flow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for authorization checks using system user claims
rg -A 5 "AuthorizationDetailsType|systemuser-uuid"
# Description: Search for cache implementations
rg -A 5 "cache.*GetIdentifyingClaims"
Length of output: 3308
Script:
#!/bin/bash
# Search for TryGetSystemUserId implementation to verify validation
ast-grep --pattern 'TryGetSystemUserId($_) {
$$$
}'
# Search for usage of identifying claims in authorization context
rg -A 10 "GetIdentifyingClaims|identifyingClaims" --type cs
# Search for cache key generation
rg -A 5 "CacheKey|cacheKey" --type cs
Length of output: 18123
Script:
#!/bin/bash
# Search for TryGetSystemUserId implementation and related validation
rg -A 10 "TryGetSystemUserId|systemuser_id" --type cs
# Search for authorization_details claim validation
rg -A 10 "authorization_details.*json" --type cs
# Search for any security-related tests for system user validation
rg -A 10 "systemuser.*test" --type cs
Length of output: 12936
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/DecisionRequestHelper.cs (1)
Line range hint 104-111: LGTM! Clean implementation with performance benefits.
The changes effectively integrate system user ID handling while improving performance by eliminating the need for temporary ClaimsPrincipal objects. The implementation maintains proper security handling of sensitive claims and follows the established priority order.
|
🤖 I have created a release *beep* *boop* --- ## [1.28.0](v1.27.1...v1.28.0) (2024-11-05) ### Features * update swagger name generation ([#1350](#1350)) ([94c5544](94c5544)) * **webapi:** Add ExternalReference to dialog search result ([#1384](#1384)) ([431fe16](431fe16)) * **webapi:** Return 410 GONE for notification checks on deleted dialogs ([#1387](#1387)) ([198bebd](198bebd)) ### Bug Fixes * Add system user id to identifying claims ([#1362](#1362)) ([16f160d](16f160d)) * **e2e:** Use pagination in sentinel ([#1372](#1372)) ([a1df0ff](a1df0ff)) * fixed placement of referenced workflow-file ([#1365](#1365)) ([49c1d80](49c1d80)) * workaround for github number error in dispatch workflow ([#1367](#1367)) ([06ee356](06ee356)) --- 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 adds a check to include the system user id in the list of identifiable claims, which is in turn used to generate a cache key for authorization requests on dialog details accesses.
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes
Tests