correct auth for legacy overview #460
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant changes to the handling of user access and correspondence history retrieval within the Changes
Possibly related PRs
Suggested labels
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
CodeRabbit Configuration File (
|
Andreass2
added
kind/feature-request
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)
8-8: Ensure secure handling of SSN.The method now accepts SSN (Social Security Number) which is sensitive PII data. Ensure all implementations:
- Use secure transport (HTTPS)
- Implement proper logging practices (no SSN in logs)
- Follow data protection regulations
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (1)
20-20: Document the significance of auth level 3Consider extracting the magic number 3 to a named constant and documenting its significance in the context of authentication levels.
+ private const int DefaultDevAuthLevel = 3; // Represents highest security level for development - return Task.FromResult((int?)3); + return Task.FromResult((int?)DefaultDevAuthLevel);src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (3)
115-115: Update method documentation to reflect signature changeThe
AuthorizeRequestmethod now accepts aXacmlJsonRequestRootparameter. Ensure that any documentation or XML comments for this method are updated to reflect this change for clarity.
149-157: Refactor to eliminate code duplication in decision request methodsThe methods
CreateDecisionRequestandCreateDecisionRequestForLegacyshare similar logic regardingpersonIdClaimand issuer checks. Consider refactoring shared logic into a private helper method to reduce duplication and improve maintainability.Apply this diff to refactor the shared logic:
+ private bool IsAltinnTokenIssuer(Claim? personIdClaim) + { + return personIdClaim is null || personIdClaim.Issuer == $"{_altinnOptions.PlatformGatewayUrl.TrimEnd('/')}/authentication/api/v1/openid/"; + } - if (personIdClaim is null || personIdClaim.Issuer == $"{_altinnOptions.PlatformGatewayUrl.TrimEnd('/')}/authentication/api/v1/openid/") + if (IsAltinnTokenIssuer(personIdClaim)) { // Existing logic }
152-156: Consider more informative exception handlingIn
CreateDecisionRequestForLegacy, throwing a genericSecurityTokenInvalidIssuerExceptionmay not provide sufficient context. Consider throwing a more specific exception or including a descriptive message to aid in debugging.Apply this diff to enhance exception handling:
- throw new SecurityTokenInvalidIssuerException(); + throw new SecurityTokenInvalidIssuerException($"Unsupported issuer: {personIdClaim?.Issuer}");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs(1 hunks)src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs(1 hunks)src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs(3 hunks)src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs:17-21
Timestamp: 2024-10-29T13:10:17.164Z
Learning: The `PartyId` and `AuthenticationLevel` properties in the `LegacyUser` class (in `src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryResponse.cs`) are still under development, and the correct validation has not been determined yet.
🔇 Additional comments (7)
src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)
8-8: LGTM! Verify implementation updates.
The signature change aligns well with the PR objective of correcting authentication for legacy overview. The addition of SSN parameter and non-nullable recipientOrgNo strengthens the authentication process.
Let's verify that all implementations have been updated:
✅ Verification successful
Interface implementation and usage verified successfully
The verification shows that:
- The interface is properly implemented in both
AltinnAuthorizationServiceandAltinnAuthorizationDevService - All calls to
CheckUserAccessAndGetMinimumAuthLevelin handlers (LegacyGetCorrespondenceOverviewHandlerandLegacyGetCorrespondenceHistoryHandler) use the correct signature with SSN and required recipientOrgNo parameters - The implementations and usages are consistent with the interface changes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for implementations of IAltinnAuthorizationService
# and verify they implement the new method signature correctly
# Find all implementations
ast-grep --pattern 'class $_ : $$$IAltinnAuthorizationService$$$'
# Find all calls to CheckUserAccessAndGetMinimumAuthLevel
rg -A 3 'CheckUserAccessAndGetMinimumAuthLevel'
Length of output: 3596
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (1)
18-21: Verify development service usage
This development service implementation always returns auth level 3 and grants all access. Please ensure this is only used in development/test environments.
Consider adding environment checks or throwing NotSupportedException if accidentally used in production.
✅ Verification successful
Based on the search results, I can now generate a final response:
Development service is properly isolated
The AltinnAuthorizationDevService is only used in development scenarios when maskinportenSettings.ClientId is empty, while production uses the real AltinnAuthorizationService. The service registration in DependencyInjection.cs shows proper environment-based switching:
- Development: Uses
AltinnAuthorizationDevServicewhen no Maskinporten client ID is configured - Production: Uses real
AltinnAuthorizationServicewith Maskinporten HTTP client registration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if this development service is referenced in production configurations
# Expected: No matches in production config files
# Search for references to AltinnAuthorizationDevService in config files
rg -l "AltinnAuthorizationDevService" | rg -i "prod|release|deployment"
Length of output: 71
Script:
#!/bin/bash
# Let's try a more comprehensive verification approach:
# 1. Find all references to this service class
echo "=== Direct references to AltinnAuthorizationDevService ==="
rg "AltinnAuthorizationDevService"
# 2. Check where IAltinnAuthorizationService is registered in DI
echo -e "\n=== DI registrations for IAltinnAuthorizationService ==="
rg "IAltinnAuthorizationService" -A 5 -B 5
# 3. Look for environment-specific configuration files
echo -e "\n=== Environment configurations ==="
fd -e json -e xml "appsettings|config" --exec grep -l "Authorization"
Length of output: 63213
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (1)
Line range hint 37-159: Verify the security implications of the legacy authorization path.
The introduction of a new authorization path for legacy systems warrants careful security review:
- Verify that the legacy path is only used when absolutely necessary
- Ensure proper access controls are in place
- Check for potential security vulnerabilities
Run the following script to analyze the usage:
✅ Verification successful
Security review of legacy authorization path reveals controlled usage and proper safeguards
Based on the analysis of the codebase:
-
The legacy authorization path (
CreateAltinnDecisionRequestForLegacy) is only used within theAltinnAuthorizationServicein a controlled manner, specifically for legacy correspondence operations. -
SSN handling follows secure practices:
- No hardcoded sensitive data found
- SSNs are properly mapped to URN identifiers (
urn:altinn:person:identifier-no) - Access checks are consistently performed before operations involving SSNs
-
Authorization checks are properly implemented:
- Minimum auth level verification is required for all operations
- Resource-based access control is enforced
- Claims and scopes are validated before granting access
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Analyze the usage of legacy authorization methods and potential security implications
# Test 1: Find all usages of the legacy decision request method
echo "=== Legacy Decision Request Usage ==="
rg -A 5 "CreateAltinnDecisionRequestForLegacy"
# Test 2: Check for potential hardcoded SSNs or sensitive data
echo -e "\n=== Potential Sensitive Data ==="
rg -i "ssn|socialsecurity|personnumber"
# Test 3: Analyze authorization service implementations
echo -e "\n=== Authorization Service Usage ==="
ast-grep --pattern 'class $_ : IAltinnAuthorizationService {
$$$
CheckUserAccessAndGetMinimumAuthLevel($$$) {
$$$
}
$$$
}'
Length of output: 11531
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (1)
49-49: Verify updated method calls across the codebase
The method CheckUserAccessAndGetMinimumAuthLevel now includes userParty.SSN as a parameter. Please ensure that all other invocations of this method throughout the codebase have been updated to include the new ssn parameter to prevent any potential runtime errors due to mismatched method signatures.
Run the following script to locate all usages of the method and verify their parameter lists:
✅ Verification successful
Based on the search results, I can generate the final response as we have comprehensive information about all usages of the CheckUserAccessAndGetMinimumAuthLevel method in the codebase:
All usages of CheckUserAccessAndGetMinimumAuthLevel are correctly implemented
All invocations of the method are consistent with the interface definition and include the required ssn parameter:
- Interface definition in
IAltinnAuthorizationService.csspecifiesssnas first parameter - Implementation in
AltinnAuthorizationDevService.csmatches interface - Implementation in
AltinnAuthorizationService.csmatches interface - Usage in
LegacyGetCorrespondenceHistoryHandler.cscorrectly passesrecipientParty.SSN - Usage in
LegacyGetCorrespondenceOverviewHandler.cscorrectly passesuserParty.SSN
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all calls to CheckUserAccessAndGetMinimumAuthLevel include the new 'ssn' parameter.
# Test: Search for all invocations of the method. Expect: All calls should include 'ssn' as the first argument.
rg 'CheckUserAccessAndGetMinimumAuthLevel' --type cs -C 2
Length of output: 4176
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (3)
50-51: Refactored code enhances readability and consistency
The extraction of actionIds and creation of jsonRequest using CreateDecisionRequest improves code clarity and aligns with the updated authorization flow.
64-64: Verify handling of null return value
Changing the return value to null when validation fails may impact calling methods that expect a different type. Verify that all consumers of CheckUserAccessAndGetMinimumAuthLevel correctly handle a null return to prevent possible NullReferenceExceptions or unexpected behavior.
151-151:
Ensure personIdClaim is not null before accessing properties
In CreateDecisionRequestForLegacy, ensure that personIdClaim is not null before accessing its Issuer property to prevent a potential NullReferenceException.
Apply this diff to safely access personIdClaim:
- if (personIdClaim is null || personIdClaim.Issuer == /* condition */)
+ if (personIdClaim == null || personIdClaim.Issuer == /* condition */)
{
// Existing logic
}Likely invalid or redundant comment.
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs
Show resolved
Hide resolved
...Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (2)
60-68: Consider making the minimum authentication level configurable.The method returns a hard-coded authentication level of 3 when validation succeeds. This might need to be configurable based on different security requirements or environments.
Consider:
- Moving this to configuration
- Making it dependent on the resource type or access level requested
- Adding documentation explaining why level 3 was chosen
Line range hint
115-133: LGTM! Consider enhancing error details in response.The error handling is comprehensive, but we could improve the logging of failure responses.
Consider adding more detailed error logging:
if (!response.IsSuccessStatusCode) { + var errorContent = await response.Content.ReadAsStringAsync(cancellationToken); + _logger.LogError("Authorization request failed. Status: {StatusCode}, Error: {Error}", + response.StatusCode, errorContent); return null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs(3 hunks)
🔇 Additional comments (2)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (2)
Line range hint 60-157: Verify token validation and authentication flow.
The changes handle sensitive operations and multiple authentication methods. Let's verify the security implications:
#!/bin/bash
# Check for proper token validation across the codebase
echo "Checking token validation patterns..."
rg -A 5 "ValidateToken|ValidateAsync|SecurityToken"
echo "Checking authentication middleware configuration..."
rg -A 10 "UseAuthentication|AddAuthentication"
echo "Checking for potential authentication bypasses..."
ast-grep --pattern 'if.*IsDevelopment.*return true'50-52: LGTM! Verify CreateDecisionRequest usage across codebase.
The refactoring to use CreateDecisionRequest improves code organization. Let's verify its usage pattern across the codebase.
✅ Verification successful
Let me try a different search pattern to find the CreateDecisionRequest usage.
Based on the search results, I can now generate the final response:
CreateDecisionRequest usage is consistent and properly implemented
The method is used correctly in two scenarios:
- Standard flow using
CreateDecisionRequest(user, actionIds, resourceId, recipientOrgNo) - Legacy flow using
CreateDecisionRequestForLegacy(user, ssn, actionTypes, resourceId, orgnr)
Both implementations properly handle token validation and map to the appropriate XACML request format through AltinnTokenXacmlMapper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all calls to CreateDecisionRequest to ensure consistent usage
ast-grep --pattern 'CreateDecisionRequest($$$)'
Length of output: 17160
Script:
#!/bin/bash
# Search for CreateDecisionRequest method definition and its usages
ast-grep --pattern 'CreateDecisionRequest($$$)'
# Also search with ripgrep for better context
rg -A 5 'CreateDecisionRequest'
Length of output: 4152
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (1)
71-81: Consider refactoring to enhance code maintainabilityThe
CreateResourceCategorymethod now includes additional parameterslegacyandorgNr, introducing conditional logic based on thelegacyflag. To improve readability and maintainability, consider separating the legacy and non-legacy logic into distinct methods or refactoring the method to reduce complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs(4 hunks)
🔇 Additional comments (2)
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (2)
37-57: Method implementation aligns with existing patterns
The CreateAltinnDecisionRequestForLegacy method correctly constructs the XacmlJsonRequestRoot for legacy scenarios, ensuring that the access subject, action, and resource categories are appropriately set. The logic is consistent with the existing CreateAltinnDecisionRequest method.
121-133:
Validation of the ssn parameter is recommended
The CreateSubjectCategoryForLegacy method accepts an ssn parameter but does not validate its format. For security and data integrity, consider implementing validation to ensure the ssn conforms to the expected Norwegian SSN format.
Apply this diff to add SSN validation:
private static XacmlJsonCategory CreateSubjectCategoryForLegacy(ClaimsPrincipal user, string ssn)
{
+ if (!IsValidNorwegianSSN(ssn))
+ {
+ throw new ArgumentException("Invalid SSN format", nameof(ssn));
+ }
XacmlJsonCategory xacmlJsonCategory = new XacmlJsonCategory();
List<XacmlJsonAttribute> list = new List<XacmlJsonAttribute>();
var claim = user.Claims.FirstOrDefault(claim => IsScopeClaim(claim.Type));
if (claim is not null)
{
list.Add(CreateXacmlJsonAttribute("urn:altinn:person:identifier-no", ssn, "string", claim.Issuer));
list.Add(CreateXacmlJsonAttribute("urn:scope", claim.Value, "string", claim.Issuer));
}
xacmlJsonCategory.Attribute = list;
return xacmlJsonCategory;
}
+private static bool IsValidNorwegianSSN(string ssn)
+{
+ // Norwegian SSN validation logic
+ return Regex.IsMatch(ssn, @"^\d{11}$");
+}Likely invalid or redundant comment.
Description
Correct authentication for legacy based on logged in user.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Refactor