Conversation
📝 WalkthroughWalkthroughThe changes introduce validation constraints on multi-resource rights in authentication services. SystemRegisterService and SystemUserService now flag or reject rights containing multiple resources, treating them as invalid. Accompanying test updates replace previously passing sub-resource test cases with negative test scenarios that expect failures when multi-resource rights are provided. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Fix all issues with AI Agents 🤖
In @src/Authentication/Services/SystemRegisterService.cs:
- Around line 161-165: The validation only adds the second resource
(right.Resource[1].Value) to invalidFormatResourceIds when there are multiple
resources, so any additional resources beyond the second are ignored; change the
logic in the SystemRegisterService validation block to iterate over all
resources after the first (e.g., for each resource in right.Resource.Skip(1) or
a for-loop from index 1 to right.Resource.Count-1) and add each resource.Value
to invalidFormatResourceIds so every extra resource is flagged.
🧹 Nitpick comments (2)
test/Altinn.Platform.Authentication.Tests/Controllers/ChangeRequestControllerTest.cs (2)
249-253: Update the doc comment to reflect the test's actual purpose.The XML doc comment describes creating a ChangeRequest successfully, but this test validates that creating a ChangeRequest with sub-resource rights returns
BadRequest. Consider updating the summary to clarify:/// <summary> -/// After having verified that the ChangeRequest is needed, create a ChangeRequest +/// Verifies that creating a ChangeRequest with multi-resource (sub-resource) rights returns BadRequest /// </summary>
349-352: Remove unused variables.The variables
orgno,ext, andsysare declared but never used in this test. Onlyidis used in the endpoint URL.🔎 Proposed fix
Guid id = Guid.NewGuid(); - string orgno = "910493353"; - string ext = "external"; - string sys = "991825827_the_matrix";
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Authentication/Services/SystemRegisterService.cssrc/Authentication/Services/SystemUserService.cstest/Altinn.Platform.Authentication.Tests/Controllers/ChangeRequestControllerTest.cstest/Altinn.Platform.Authentication.Tests/Controllers/RequestControllerTests.cstest/Altinn.Platform.Authentication.Tests/Controllers/SystemUserControllerTest.cs
💤 Files with no reviewable changes (1)
- test/Altinn.Platform.Authentication.Tests/Controllers/SystemUserControllerTest.cs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-04T17:38:15.650Z
Learnt from: simen-rekkedal
Repo: Altinn/altinn-authentication PR: 1581
File: src/Authentication/Controllers/RequestSystemUserController.cs:459-485
Timestamp: 2025-11-04T17:38:15.650Z
Learning: In the Altinn Authentication codebase (src/Authentication/Controllers/RequestSystemUserController.cs), party parameter validation for system user request operations is handled by the Policy Enforcement Point (PEP) layer, not at the controller level. Controller methods rely on authorization policies like AuthzConstants.POLICY_ACCESS_MANAGEMENT_WRITE to enforce party-level access control.
Applied to files:
test/Altinn.Platform.Authentication.Tests/Controllers/RequestControllerTests.cs
🧬 Code graph analysis (2)
src/Authentication/Services/SystemRegisterService.cs (1)
test/Altinn.Platform.Authentication.SystemIntegrationTests/Domain/Right.cs (1)
Resource(10-15)
src/Authentication/Services/SystemUserService.cs (2)
test/Altinn.Platform.Authentication.SystemIntegrationTests/Domain/Right.cs (1)
Resource(10-15)src/Core/Problems/Problem.cs (1)
Problem(8-410)
🔇 Additional comments (5)
src/Authentication/Services/SystemUserService.cs (1)
642-645: LGTM! Clean enforcement of single-resource policy.The strict check ensures exactly one resource per right before proceeding with validation. This is a clear and effective guard that aligns with the broader validation policy changes across the PR.
test/Altinn.Platform.Authentication.Tests/Controllers/RequestControllerTests.cs (3)
20-20: LGTM! Import supports validation error checking.The added import enables the test to verify specific validation error messages.
236-246: LGTM! Test correctly validates early rejection of multi-resource rights.The refactored test now validates that system register creation fails immediately when rights contain multiple resources, aligning with the new pre-check validation in
SystemRegisterService.GetInvalidResourceIdsDetailed.
2115-2159: LGTM! Comprehensive test for multi-resource rejection.The new test validates that requests with multi-resource rights are properly rejected with
BadRequest, ensuring the single-resource policy is enforced at the request creation layer.test/Altinn.Platform.Authentication.Tests/Controllers/ChangeRequestControllerTest.cs (1)
282-297: Test logic for sub-resource validation looks correct.The test correctly constructs
right2with twoAttributePairentries under the sameResourcecollection, simulating a multi-resource right. The assertion forHttpStatusCode.BadRequestaligns with the PR objective to reject such rights during ChangeRequest creation.Consider adding an assertion on the response body to verify the error message or problem details, ensuring the correct validation error is returned:
// Optional: verify error details var problemDetails = await createdResponseMessage.Content.ReadFromJsonAsync<ProblemDetails>(); Assert.NotNull(problemDetails); // Assert specific error code or message if applicableAlso applies to: 357-368
| // For now we only support single resource per right, no sub-resources | ||
| if (right.Resource.Count > 1) | ||
| { | ||
| invalidFormatResourceIds.Add(right.Resource[1].Value); | ||
| } |
There was a problem hiding this comment.
Flag all extra resources beyond the first, not just the second.
If a right contains 3 or more resources, only the second resource (index 1) is added to invalidFormatResourceIds. Resources at indices 2, 3, etc. are silently ignored. Consider flagging all resources beyond the first for complete validation feedback.
🔎 Proposed fix to flag all extra resources
// For now we only support single resource per right, no sub-resources
if (right.Resource.Count > 1)
{
- invalidFormatResourceIds.Add(right.Resource[1].Value);
+ for (int i = 1; i < right.Resource.Count; i++)
+ {
+ invalidFormatResourceIds.Add(right.Resource[i].Value);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // For now we only support single resource per right, no sub-resources | |
| if (right.Resource.Count > 1) | |
| { | |
| invalidFormatResourceIds.Add(right.Resource[1].Value); | |
| } | |
| // For now we only support single resource per right, no sub-resources | |
| if (right.Resource.Count > 1) | |
| { | |
| for (int i = 1; i < right.Resource.Count; i++) | |
| { | |
| invalidFormatResourceIds.Add(right.Resource[i].Value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/Authentication/Services/SystemRegisterService.cs around lines 161-165,
The validation only adds the second resource (right.Resource[1].Value) to
invalidFormatResourceIds when there are multiple resources, so any additional
resources beyond the second are ignored; change the logic in the
SystemRegisterService validation block to iterate over all resources after the
first (e.g., for each resource in right.Resource.Skip(1) or a for-loop from
index 1 to right.Resource.Count-1) and add each resource.Value to
invalidFormatResourceIds so every extra resource is flagged.
|
1 participant
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.