Skip to content
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

Merged
merged 3 commits into from
Nov 8, 2024
Merged

add auth to all legacy enpoints #468

merged 3 commits into from
Nov 8, 2024

Conversation

Andreass2
Copy link
Collaborator

@Andreass2 Andreass2 commented Nov 8, 2024

Description

Adds Authorization to all legacy enpoints

Related Issue(s)

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)
  • All tests run green

Documentation

  • User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

Summary by CodeRabbit

Release Notes

  • New Features

    • Enhanced authorization checks for correspondence handling processes.
  • Bug Fixes

    • Improved error handling for invalid party IDs and correspondence statuses.
  • Refactor

    • Removed the OnbehalfOfPartyId property from various request and mapper classes to streamline data handling.
  • Documentation

    • Updated error messages to provide clearer feedback to users regarding access issues.

Copy link
Contributor

coderabbitai bot commented Nov 8, 2024

📝 Walkthrough

Walkthrough

The pull request introduces changes primarily focused on the removal of the OnbehalfOfPartyId property from multiple classes, including LegacyGetCorrespondencesRequest, LegacyGetCorrespondencesRequestExt, and the mapping logic in LegacyGetCorrespondencesMapper. Additionally, it updates authorization and error handling logic in several handler classes, ensuring that the access control mechanisms are more robust and that the minimum authentication level is dynamically retrieved. Overall, the changes enhance the handling of correspondence requests and improve the consistency of error responses.

Changes

File Change Summary
src/Altinn.Correspondence.API/Mappers/LegacyGetCorrespondencesMapper.cs Removed OnbehalfOfPartyId from MapToRequest method.
src/Altinn.Correspondence.API/Models/LegacyGetCorrespondencesRequestExt.cs Deleted OnbehalfOfPartyId property from the class.
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs Updated party ID verification logic, replaced OnbehalfOfPartyId checks with _userClaimsHelper.GetPartyId(). Enhanced error handling.
src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesRequest.cs Removed OnbehalfOfPartyId property from the class.
src/Altinn.Correspondence.Application/GetCorrespondenceHistory/LegacyGetCorrespondenceHistoryHandler.cs Enhanced authorization logic using _altinnAuthorizationService.CheckUserAccessAndGetMinimumAuthLevel.
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs Updated error handling for invalid party ID and refined correspondence availability checks.
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs Added IAltinnAuthorizationService dependency, integrated authorization checks into Process method.

Possibly related PRs

Suggested labels

kind/enhancement, kind/breaking-change

Suggested reviewers

  • CelineTrammi

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Andreass2 Andreass2 added kind/feature-request New feature or request ignore-for-release pull request wont be included in release notes labels Nov 8, 2024
@Andreass2 Andreass2 marked this pull request as ready for review November 8, 2024 11:32
Copy link
Collaborator

@CelineTrammi CelineTrammi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing! 🥇

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 messages

The authorization flow is robust but uses the same LegacyNoAccessToCorrespondence error for different scenarios:

  1. When minimum auth level check fails
  2. When recipient validation fails
  3. 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 updates

The current implementation silently continues after logging status update errors. This could lead to inconsistent state tracking if the status update fails.

Consider either:

  1. Propagating the error to inform the client
  2. 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 objectives

The 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 like MissingPartyIdentifiers.

- 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 utilizing minimumAuthLevel 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

📥 Commits

Reviewing files that changed from the base of the PR and between 817b4b5 and 8a7aa47.

📒 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

@Andreass2 Andreass2 merged commit 8656258 into main Nov 8, 2024
6 checks passed
@Andreass2 Andreass2 deleted the legacy/list-auth branch November 8, 2024 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ignore-for-release pull request wont be included in release notes kind/feature-request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants