-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add auth to all legacy enpoints #468
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes primarily focused on the removal of 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! 🥇
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (3)
Line range hint
42-60
: Consider differentiating authorization error messagesThe authorization flow is robust but uses the same
LegacyNoAccessToCorrespondence
error for different scenarios:
- When minimum auth level check fails
- When recipient validation fails
- When authorized parties check fails
Consider providing more specific error messages to help diagnose access issues.
Example implementation:
if (minimumAuthLevel == null) { - return Errors.LegacyNoAccessToCorrespondence; + return Errors.InsufficientAuthenticationLevel; } if (correspondence.Recipient != userParty.SSN && correspondence.Recipient != ("0192:" + userParty.OrgNumber)) { var authorizedParties = await _altinnAccessManagementService.GetAuthorizedParties(userParty, cancellationToken); var isAuthorized = authorizedParties.Any(p => ("0192:" + p.OrgNumber) == correspondence.Recipient || p.SSN == correspondence.Recipient); if (!isAuthorized) { - return Errors.LegacyNoAccessToCorrespondence; + return Errors.NotAuthorizedForRecipient; } }
Line range hint
77-86
: Reconsider silent error handling for status updatesThe current implementation silently continues after logging status update errors. This could lead to inconsistent state tracking if the status update fails.
Consider either:
- Propagating the error to inform the client
- Adding retry logic for transient failures
try { await _correspondenceStatusRepository.AddCorrespondenceStatus(new CorrespondenceStatusEntity { CorrespondenceId = correspondence.Id, Status = CorrespondenceStatus.Fetched, StatusText = CorrespondenceStatus.Fetched.ToString(), StatusChanged = DateTimeOffset.UtcNow }, cancellationToken); } catch (Exception e) { _logger.LogError(e, "Error when adding status to correspondence"); + // Option 1: Propagate error + return Errors.StatusUpdateFailed; + // Option 2: Add retry logic + // await RetryHelper.ExecuteWithRetry(() => _correspondenceStatusRepository.AddCorrespondenceStatus(...)); }
Line range hint
1-168
: Overall implementation aligns well with PR objectivesThe handler successfully implements the required authorization checks with a comprehensive flow that includes:
- Party validation
- Minimum authentication level verification
- Recipient access control
- Delegation checks
The error handling improvements make the responses more specific and helpful. The implementation aligns well with the PR's goal of adding auth to legacy endpoints.
Consider documenting the authorization flow in the API documentation to help consumers understand the requirements and possible error scenarios.
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs (3)
45-49
: Consider improving the error message.While the error handling is appropriate,
CouldNotFindOrgNo
might not fully convey that both SSN and organization number are missing. Consider introducing a more specific error likeMissingPartyIdentifiers
.- return Errors.CouldNotFindOrgNo; + return Errors.MissingPartyIdentifiers; // New error constant needed
69-70
: Extract the organization number prefix as a constant.The "0192:" prefix appears to be a magic string. Consider extracting it to a constant for better maintainability.
+ private const string ORG_NUMBER_PREFIX = "0192:"; ... - if (!string.IsNullOrEmpty(userParty.OrgNumber)) recipients.Add("0192:" + userParty.OrgNumber); + if (!string.IsNullOrEmpty(userParty.OrgNumber)) recipients.Add(ORG_NUMBER_PREFIX + userParty.OrgNumber);
112-116
: Add logging for skipped correspondences.When a correspondence is skipped due to insufficient access, it would be helpful to log this for debugging purposes.
if (minAuthLevel == null) { + _logger.LogDebug("Skipping correspondence {Id} due to insufficient access", correspondence.Id); continue; }
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (1)
51-55
: Consider utilizingminimumAuthLevel
or simplifying the access check.The variable
minimumAuthLevel
is retrieved but not used after the null check. If the minimum authentication level is not required elsewhere in the code, consider the following options:
- Option 1: Use an access-check method that returns a boolean to clarify intent.
- Option 2: If
minimumAuthLevel
is needed for future enhancements, add a comment indicating its planned use to avoid confusion.Option 1: Use a boolean access check method.
If only access verification is needed, modify the code as follows:
-var minimumAuthLevel = await _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel( ... ); +var hasAccess = await _altinnAuthorizationService.HasUserAccess( ... ); -if (minimumAuthLevel == null) +if (!hasAccess) { return Errors.LegacyNoAccessToCorrespondence; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Altinn.Correspondence.API/Mappers/LegacyGetCorrespondencesMapper.cs
(0 hunks)src/Altinn.Correspondence.API/Models/LegacyGetCorrespondencesRequestExt.cs
(0 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs
(5 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesRequest.cs
(0 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs
(4 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs
(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
(3 hunks)
💤 Files with no reviewable changes (3)
- src/Altinn.Correspondence.API/Mappers/LegacyGetCorrespondencesMapper.cs
- src/Altinn.Correspondence.API/Models/LegacyGetCorrespondencesRequestExt.cs
- src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesRequest.cs
🧰 Additional context used
📓 Learnings (2)
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.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.
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.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (1)
37-37
: LGTM: Error message improvement
The change from NoAccessToResource
to InvalidPartyId
provides a more precise error message, better indicating the specific validation failure.
Let's verify consistent error handling across other handlers:
✅ Verification successful
Error handling is consistent across legacy handlers
The verification confirms that all legacy handlers (GetCorrespondenceOverview
, GetCorrespondences
, UpdateCorrespondenceStatus
, GetCorrespondenceHistory
) consistently use Errors.InvalidPartyId
when party ID validation fails, showing proper error handling standardization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for similar party ID validation patterns
rg -A 3 "GetPartyId\(\) is not int" --type cs
Length of output: 2192
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs (1)
129-129
: Validate the authentication level before casting.
The cast from minAuthLevel
to int
might need validation to ensure it's within expected bounds.
Let's check if there are any validation patterns in use:
#!/bin/bash
# Search for authentication level validation patterns
rg -A 3 "AuthenticationLevel" --type cs
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs (3)
13-13
: Dependency injection of IAltinnAuthorizationService
added correctly.
The private readonly field _altinnAuthorizationService
has been appropriately added to the class for dependency injection.
21-21
: Constructor updated to include IAltinnAuthorizationService
.
The constructor now includes IAltinnAuthorizationService altinnAuthorizationService
, ensuring that the authorization service is injected properly.
29-29
: Assignment of the injected authorization service is implemented.
The injected altinnAuthorizationService
is correctly assigned to the private field _altinnAuthorizationService
.
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs (2)
62-62
: Verify casting minimumAuthLevel
to int
is safe
Ensure that casting minimumAuthLevel
to int
is appropriate. If minimumAuthLevel
is of a nullable type or an enum, casting it directly to int
without checking for null could result in a NullReferenceException
. Consider adding a null check or using a default value.
82-82
: Consistent handling of minimumAuthLevel
in notification statuses
When passing minimumAuthLevel
to the GetNotificationStatus
method, it's cast to int
using (int)minimumAuthLevel
. Verify that minimumAuthLevel
is not null before casting to prevent potential exceptions.
Also applies to: 91-91
Description
Adds Authorization to all legacy enpoints
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
OnbehalfOfPartyId
property from various request and mapper classes to streamline data handling.Documentation