removepartId and disable auth #445
Conversation
📝 WalkthroughWalkthroughThe changes primarily involve 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
ignore-for-release
![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 (3)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (1)
Line range hint
59-59: Address TODO comment regarding response type.The comment questions whether
LegacyGetCorrespondenceHistoryResponseshould beLegacyCorrespondenceHistoryExt. This should be resolved to maintain API consistency.Would you like me to help analyze the response types and suggest the most appropriate one to use?
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (2)
19-19: Ensure_userClaimsHelperIs Properly InitializedA new private field
_userClaimsHelperis introduced. Consider adding a null check in the constructor to prevent a potentialNullReferenceExceptionifuserClaimsHelperis not provided.Apply this diff to add a null check:
public LegacyGetCorrespondenceOverviewHandler( IAltinnAccessManagementService altinnAccessManagementService, IAltinnAuthorizationService altinnAuthorizationService, IAltinnRegisterService altinnRegisterService, ICorrespondenceRepository correspondenceRepository, ICorrespondenceStatusRepository correspondenceStatusRepository, UserClaimsHelper userClaimsHelper, ILogger<LegacyGetCorrespondenceOverviewHandler> logger) { _altinnAccessManagementService = altinnAccessManagementService; _altinnAuthorizationService = altinnAuthorizationService; _altinnRegisterService = altinnRegisterService; _correspondenceRepository = correspondenceRepository; _correspondenceStatusRepository = correspondenceStatusRepository; _logger = logger; - _userClaimsHelper = userClaimsHelper; + _userClaimsHelper = userClaimsHelper ?? throw new ArgumentNullException(nameof(userClaimsHelper)); }
33-37: Improve Error Handling When Retrieving Party IDWhen
_userClaimsHelper.GetPartyId()fails to retrieve a validpartyId, the method returnsErrors.NoAccessToResource. Consider adding logging to capture this event for easier debugging and to provide more context in logs.Apply this diff to add a warning log:
if (_userClaimsHelper.GetPartyId() is not int partyId) { + _logger.LogWarning("Failed to retrieve valid Party ID from user claims."); return Errors.NoAccessToResource; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs(2 hunks)
🔇 Additional comments (3)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
Line range hint 18-21: Verify if authorization should be disabled.
The PR title mentions "disable auth", but the controller still has the [Authorize(Policy = AuthorizationConstants.Legacy)] attribute. Please clarify if the authorization should be removed or maintained.
Let's check for any pending authorization changes:
Line range hint 48-52: LGTM! Method signature and processing flow have been simplified.
The removal of onBehalfOfPartyId parameter and direct passing of correspondenceId to the handler makes the code cleaner and more straightforward.
Let's verify the authorization implications of this change:
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (1)
11-11:
Update Interface Implementation Carefully
The class now implements IHandler<Guid, LegacyGetCorrespondenceOverviewResponse> instead of IHandler<LegacyGetCorrespondenceOverviewRequest, LegacyGetCorrespondenceOverviewResponse>. Ensure that all references to this handler and its interface are updated throughout the codebase to prevent interface mismatches or runtime exceptions.
Run the following script to identify any references to the old interface:
✅ Verification successful
Interface Change is Safe and Properly Implemented
The change from IHandler<LegacyGetCorrespondenceOverviewRequest, LegacyGetCorrespondenceOverviewResponse> to IHandler<Guid, LegacyGetCorrespondenceOverviewResponse> is safe and properly implemented across the codebase:
- The handler is correctly registered in DependencyInjection.cs
- The controller correctly injects and uses the handler with a Guid parameter
- No references to the old interface signature were found
- The change aligns with similar handlers in the codebase that also use Guid as input parameter (e.g., GetCorrespondenceOverviewHandler, GetCorrespondenceDetailsHandler)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all references to the old interface implementation.
# Test: Search for usages of the old interface. Expect: No matches.
rg 'IHandler<LegacyGetCorrespondenceOverviewRequest, LegacyGetCorrespondenceOverviewResponse>'
Length of output: 96
Script:
#!/bin/bash
# Let's try a broader search approach to find any references to this handler class and its usage
# Search for any references to the handler class
rg "LegacyGetCorrespondenceOverviewHandler" -A 3 -B 3
# Search for any dependency injection registrations
rg "AddScoped|AddTransient|AddSingleton.*LegacyGetCorrespondenceOverviewHandler"
# Search for any references to the request type
rg "LegacyGetCorrespondenceOverviewRequest"
# Search for IHandler interface usage with any type parameters
rg "IHandler<.*>" -A 1 -B 1
Length of output: 22035
| /*var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel(correspondence.ResourceId, new List<ResourceAccessLevel> { ResourceAccessLevel.Read }, cancellationToken); | ||
| if (minimumAuthLevel == null) | ||
| { | ||
| return Errors.LegacyNoAccessToCorrespondence; | ||
| } | ||
| } */ | ||
| var minimumAuthLevel = 2; |
There was a problem hiding this comment.
Security Concern: Hardcoded Authorization Level Bypasses Checks
The original authorization check using _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel is commented out and replaced with var minimumAuthLevel = 2;. This hardcodes the authorization level, potentially allowing unauthorized access and bypassing security measures.
Apply this diff to reinstate the authorization check:
-/*var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel(correspondence.ResourceId, new List<ResourceAccessLevel> { ResourceAccessLevel.Read }, cancellationToken);
- if (minimumAuthLevel == null)
- {
- return Errors.LegacyNoAccessToCorrespondence;
- } */
+var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel(
+ correspondence.ResourceId,
+ new List<ResourceAccessLevel> { ResourceAccessLevel.Read },
+ cancellationToken);
+if (minimumAuthLevel == null)
+{
+ return Errors.LegacyNoAccessToCorrespondence;
+}Do you need assistance in properly re-implementing the authorization logic?
📝 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.
| /*var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel(correspondence.ResourceId, new List<ResourceAccessLevel> { ResourceAccessLevel.Read }, cancellationToken); | |
| if (minimumAuthLevel == null) | |
| { | |
| return Errors.LegacyNoAccessToCorrespondence; | |
| } | |
| } */ | |
| var minimumAuthLevel = 2; | |
| var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel( | |
| correspondence.ResourceId, | |
| new List<ResourceAccessLevel> { ResourceAccessLevel.Read }, | |
| cancellationToken); | |
| if (minimumAuthLevel == null) | |
| { | |
| return Errors.LegacyNoAccessToCorrespondence; | |
| } |
Description
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
GetCorrespondenceOverviewmethod by removing unnecessary parameters, enhancing user experience.Bug Fixes
Chores
LegacyGetCorrespondenceOverviewRequestclass, reducing complexity in the correspondence overview process.