Add logic for downloading attachment for A2 GUI #421
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces enhancements to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
112-114: Enhance method documentation with response types.The XML documentation should include possible response types and status codes for better API documentation.
/// <summary> /// Download an attachment from a Correspondence /// </summary> +/// <response code="200">Returns the attachment file stream</response> +/// <response code="404">If the correspondence or attachment is not found</response> +/// <response code="403">If the user is not authorized to access this attachment</response>
124-129: Consider validating input parameters.The method directly uses the input parameters without validation. While the handler might handle validation, consider adding basic checks at the controller level.
+ if (correspondenceId == Guid.Empty || attachmentId == Guid.Empty) + { + return BadRequest("Invalid correspondence or attachment ID"); + } + var commandResult = await handler.Process(new DownloadCorrespondenceAttachmentRequest()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(2 hunks)src/Altinn.Correspondence.Application/DependencyInjection.cs(1 hunks)src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/DependencyInjection.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#413
File: src/Altinn.Correspondence.Application/DependencyInjection.cs:0-0
Timestamp: 2024-10-29T13:40:14.651Z
Learning: In Altinn.Correspondence.Application, handlers with similar names—such as UploadAttachmentHandler, GetAttachmentDetailsHandler, GetAttachmentOverviewHandler, and PurgeAttachmentHandler—may have multiple distinct implementations and are intentionally registered separately in DependencyInjection.cs.
🔇 Additional comments (6)
src/Altinn.Correspondence.Application/DependencyInjection.cs (2)
60-60: LGTM! Handler registration follows established patterns.
The registration of LegacyDownloadCorrespondenceAttachmentHandler as a scoped service is consistent with other handlers and correctly placed in the "Legacy" section.
60-60: Verify the handler implementation exists.
Let's verify that the referenced handler class exists in the codebase.
✅ Verification successful
Let me analyze the output and generate additional verification scripts to get more context about the handler implementation.
Handler implementation exists and is properly structured
The LegacyDownloadCorrespondenceAttachmentHandler class exists and is correctly implemented:
- Implements
IHandler<DownloadCorrespondenceAttachmentRequest, DownloadCorrespondenceAttachmentResponse> - Has proper dependency injection through constructor
- Contains required repository and service dependencies
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the LegacyDownloadCorrespondenceAttachmentHandler class exists
# and verify its basic structure
# Test 1: Search for the handler class definition
echo "Searching for handler class definition..."
ast-grep --pattern 'class LegacyDownloadCorrespondenceAttachmentHandler'
# Test 2: Verify it implements IHandler interface
echo "Verifying interface implementation..."
ast-grep --pattern 'class LegacyDownloadCorrespondenceAttachmentHandler : IHandler'
Length of output: 42097
Script:
#!/bin/bash
# Search for the specific handler implementation to verify its structure
echo "Searching for LegacyDownloadCorrespondenceAttachmentHandler implementation..."
rg -A 10 "class LegacyDownloadCorrespondenceAttachmentHandler" --type cs
Length of output: 2444
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs (2)
1-27: LGTM! Well-structured class with appropriate dependencies.
The class follows SOLID principles with clear separation of concerns and proper dependency injection.
51-51: Verify stream disposal.
Ensure the attachment stream is properly disposed if an error occurs after downloading.
✅ Verification successful
Based on the implementation shown, I can now provide the final response:
Stream disposal is properly handled by the Azure SDK
The stream returned by blobClient.DownloadContentAsync() and transformed via ToStream() is managed by the Azure SDK's BinaryData class. The SDK handles proper resource cleanup, and there's no need for explicit disposal in this case. The stream is automatically disposed when the HTTP response is completed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other implementations of DownloadAttachment to verify stream handling
ast-grep --pattern 'await $_storageRepository.DownloadAttachment($$$)'
Length of output: 72
Script:
#!/bin/bash
# Let's search for the file content to understand the stream handling
rg -l "DownloadCorrespondenceAttachmentHandler"
# Also search for any using statements or dispose patterns in the file
rg -A 5 "DownloadAttachment"
# Search for interface or implementation of storage repository
rg "IStorageRepository|StorageRepository" -A 5
Length of output: 31206
Script:
#!/bin/bash
# Let's look at the implementation of DownloadAttachment in StorageRepository to understand stream handling
rg -A 10 "public async Task<Stream> DownloadAttachment"
# Also search for any using statements in the file to see if it implements IDisposable
rg "using" src/Altinn.Correspondence.Persistence/Repositories/StorageRepository.cs
Length of output: 1914
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs (2)
1-11: LGTM! Clean import statements.
The imports have been streamlined to include only the necessary dependencies, improving code maintainability.
115-118: Verify authorization requirements for attachment downloads.
While the PR description mentions that authorization will be handled in a future PR, it's important to note that the current class-level [Authorize(Policy = AuthorizationConstants.Legacy)] might not provide sufficient granular control for attachment access.
✅ Verification successful
Authorization requirements for attachment downloads are properly configured
The codebase shows that attachment downloads in the legacy controller are protected by:
- Class-level authorization with
[Authorize(Policy = AuthorizationConstants.Legacy)] - Implementation-level checks in
LegacyDownloadCorrespondenceAttachmentHandlerthat verify:- User is a recipient
- Correspondence is in a state available for recipient access
This matches the pattern used in the main CorrespondenceController which uses [Authorize(Policy = AuthorizationConstants.DownloadAttachmentPolicy)] with additional authentication schemes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing authorization patterns in attachment handling
rg -l 'attachment.*download.*auth'
# Look for similar endpoints to understand authorization patterns
ast-grep --pattern 'Route.*download.*attachment'
Length of output: 87
Script:
#!/bin/bash
# Look for authorization patterns in the controller and related files
rg -A 5 -B 5 "\[Authorize" src/
# Check for any existing attachment download implementations
rg -A 10 "download.*attachment|attachment.*download" src/
# Look for authorization constants and policies
rg -A 3 "AuthorizationConstants|AuthorizationPolicy" src/
Length of output: 63680
...pplication/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs
Outdated
Show resolved
Hide resolved
...pplication/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs
Outdated
Show resolved
Hide resolved
...pplication/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs
Show resolved
Hide resolved
...pplication/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs
Show resolved
Hide resolved
…irmed (#427) * fix: validate correspondence has been fetched before it can be read and confirmed * update tests * typo fix * Handle potential null value for IsConfirmationNeeded * reuse factory for client initialization * rename testname * add Fetched to statuses available for Recipient
* Initial functionality for legacy correspondenceOverview * improvements * better naming * better text for api route * add auth for legacy overview * better error message * fix variable names * improve minAuthLevel function * Better naming --------- Co-authored-by: Hammerbeck <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (20)
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs (2)
6-7: Add XML documentation for the class.Consider adding XML documentation to explain the purpose of this legacy response class and its relationship with the base class. This will help future maintainers understand when to use this specific implementation versus the base class.
+/// <summary> +/// Legacy response model for correspondence overview that includes additional authorization and archival properties. +/// This class extends the base GetCorrespondenceOverviewResponse to maintain backward compatibility. +/// </summary> public class LegacyGetCorrespondenceOverviewResponse : GetCorrespondenceOverviewResponse
12-12: Consider using an enum for MinimumAuthenticationLevel.Instead of using an integer for authentication levels, consider using an enum to ensure type safety and valid values.
- public int MinimumAuthenticationLevel { get; set; } + public AuthenticationLevel MinimumAuthenticationLevel { get; set; }This would require:
- Using an appropriate enum from your authentication framework
- Updating any code that sets or reads this property
src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)
8-8: Add XML documentation for the new method.The method signature looks good, but please add XML documentation to describe:
- The purpose of the method
- The expected return values (including when null is returned)
- Parameter descriptions
- Any exceptions that might be thrown
Example:
+ /// <summary> + /// Checks if the user has access to the specified resource and returns the minimum authentication level required. + /// </summary> + /// <param name="resourceId">The ID of the resource to check access for</param> + /// <param name="rights">List of access levels to check</param> + /// <param name="cancellationToken">Optional cancellation token</param> + /// <param name="recipientOrgNo">Optional organization number of the recipient</param> + /// <returns>The minimum authentication level required if access is granted, null if access is denied</returns> Task<int?> CheckUserAccessAndGetMinimumAuthLevel(string resourceId, List<ResourceAccessLevel> rights, CancellationToken cancellationToken = default, string? recipientOrgNo = null);src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (2)
18-21: Document the significance of authentication level 3.While this is a development service implementation, it would be helpful to document why level 3 was chosen as the default authentication level. This helps other developers understand the implications during testing.
public Task<int?> CheckUserAccessAndGetMinimumAuthLevel(string resourceId, List<ResourceAccessLevel> rights, CancellationToken cancellationToken = default, string? recipientOrgNo = null) { + // Default to authentication level 3 for development/testing purposes + // This represents a medium-high security level typically used for [describe typical use case] return Task.FromResult((int?)3); }
18-21: Consider making the authentication level configurable.Since this is a development service, it would be beneficial to make the authentication level configurable. This would allow testing different scenarios and authentication levels without code changes.
public class AltinnAuthorizationDevService : IAltinnAuthorizationService { + private readonly int _defaultAuthLevel; + + public AltinnAuthorizationDevService(int defaultAuthLevel = 3) + { + _defaultAuthLevel = defaultAuthLevel; + } + // ... other methods ... public Task<int?> CheckUserAccessAndGetMinimumAuthLevel(string resourceId, List<ResourceAccessLevel> rights, CancellationToken cancellationToken = default, string? recipientOrgNo = null) { - return Task.FromResult((int?)3); + return Task.FromResult((int?)_defaultAuthLevel); }src/Altinn.Correspondence.API/Models/LegacyCorrespondenceItemExt.cs (1)
41-41: Fix typo in XML documentation.The word "minimum" is misspelled in the documentation comment.
- /// The minumum authentication level required to view this correspondence + /// The minimum authentication level required to view this correspondencesrc/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs (1)
21-22: Document default values for boolean properties.The non-nullable boolean properties
AuthorizedForSignandAllowDeletewill default tofalse. Please document these default values in the XML comments to make the behavior explicit, especially since they control access permissions./// <summary> /// Indicates if the user is authorized to sign the correspondence + /// Default value is false. /// </summary> [JsonPropertyName("authorizedForSign")] public bool AuthorizedForSign { get; set; } /// <summary> /// Indicates if the correspondence can be deleted + /// Default value is false. /// </summary> [JsonPropertyName("allowDelete")] public bool AllowDelete { get; set; }Also applies to: 33-34
src/Altinn.Correspondence.Application/Helpers/CorrespondenceExtensions.cs (1)
34-37: Consider documenting status transition rules.The valid statuses list would benefit from documentation explaining the allowed state transitions and the significance of each status, particularly how the
Fetchedstatus interacts with other states.Add XML documentation explaining the status flow:
public static bool IsAvailableForRecipient(this CorrespondenceStatus correspondenceStatus) { + // Valid status progression: + // Published -> Fetched -> Read/Replied/Confirmed -> Archived + // Reserved status is a special case that... List<CorrespondenceStatus> validStatuses = [ CorrespondenceStatus.Published, CorrespondenceStatus.Fetched, CorrespondenceStatus.Read, CorrespondenceStatus.Replied, CorrespondenceStatus.Confirmed, CorrespondenceStatus.Archived, CorrespondenceStatus.Reserved ];src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs (3)
11-11: Follow C# naming conventions.Local variables should start with a lowercase letter.
- var Correspondence = new LegacyCorrespondenceOverviewExt + var correspondence = new LegacyCorrespondenceOverviewExtAlso applies to: 40-40
26-26: Use invariant format for string conversion.Specify the string format for ResourceId to ensure consistent output across different cultures.
- ResourceId = correspondenceOverview.ResourceId.ToString(), + ResourceId = correspondenceOverview.ResourceId.ToString(System.Globalization.CultureInfo.InvariantCulture),
9-41: Consider breaking down the mapping method.The method is quite long and handles multiple concerns. Consider breaking it down into smaller, focused methods for better maintainability.
Example structure:
private static void MapBasicProperties(LegacyCorrespondenceOverviewExt target, LegacyGetCorrespondenceOverviewResponse source) { target.CorrespondenceId = source.CorrespondenceId; target.Status = (CorrespondenceStatusExt)source.Status; // ... other basic properties } private static void MapComplexProperties(LegacyCorrespondenceOverviewExt target, LegacyGetCorrespondenceOverviewResponse source) { target.Content = source.Content != null ? CorrespondenceContentMapper.MapToExternal(source.Content) : null; // ... other complex properties }Test/Altinn.Correspondence.Tests/NotificationTests.cs (1)
69-69: Fix double semicolon typo.There's a double semicolon at the end of the line.
-var correspondenceId = JsonSerializer.Deserialize<InitializeCorrespondencesResponseExt>(responseContent, _responseSerializerOptions).Correspondences.First().CorrespondenceId;; +var correspondenceId = JsonSerializer.Deserialize<InitializeCorrespondencesResponseExt>(responseContent, _responseSerializerOptions).Correspondences.First().CorrespondenceId;.github/workflows/ci-cd.yaml (2)
Line range hint
46-138: Consider workflow optimization opportunities.The current workflow structure has several areas for potential improvement:
- Consider creating a reusable workflow for deployment steps to reduce duplication across environments.
- Look into using environment files or composite actions to centralize common configuration.
- Consider adding deployment verification steps and explicit failure notifications.
Would you like assistance in creating a reusable workflow template?
🧰 Tools
🪛 yamllint
[error] 131-131: trailing spaces
(trailing-spaces)
[error] 135-135: trailing spaces
(trailing-spaces)
47-47: Remove trailing spaces.There are trailing spaces on several lines that should be removed for consistency.
Affected lines: 47, 49, 131, 135
Also applies to: 49-49, 131-131, 135-135
🧰 Tools
🪛 yamllint
[error] 47-47: trailing spaces
(trailing-spaces)
src/Altinn.Correspondence.Integrations/Idporten/IdPortenXacmlMapper.cs (1)
131-157: LGTM! Consider adding documentation.The implementation is clean and handles edge cases well. However, XML documentation would be beneficial for clarity on usage.
Add XML documentation to clarify the method's purpose and return value semantics:
+ /// <summary> + /// Extracts the minimum authentication level required from a XACML response. + /// </summary> + /// <param name="response">The XACML response containing authorization decisions</param> + /// <param name="user">The claims principal representing the authenticated user</param> + /// <returns>The minimum required authentication level if found; null if no level specified or decision is not Permit</returns> public static int? GetMinimumAuthLevel(XacmlJsonResponse response, ClaimsPrincipal user)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (1)
50-54: Add logging when access is denied due to insufficient authentication levelWhen
minimumAuthLevelis null, the method returnsErrors.LegacyNoAccessToCorrespondencewithout logging the denial. Adding a log entry can aid in monitoring and troubleshooting access issues.Apply this diff to include logging:
if (minimumAuthLevel == null) { + _logger.LogWarning("Access denied: User does not meet the minimum authentication level for ResourceId {ResourceId}", correspondence.ResourceId); return Errors.LegacyNoAccessToCorrespondence; }src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (3)
48-55: Simplify null check and casting invalidationIn the
CheckUserAccessmethod, the null check and casting ofvalidationcan be simplified for better readability.Consider using the
HasValueandValueproperties of nullable types:var validation = await ValidateCheckUserAccess(user, resourceId, cancellationToken); -if (validation != null) return (bool)validation; +if (validation.HasValue) return validation.Value;
Line range hint
89-109: Add null check foruserto prevent exceptionsThe
ValidateCheckUserAccessmethod usesuserwithout null checks, which could lead to aNullReferenceExceptionifuseris null.Add a null check for
userbefore accessing its properties:+if (user is null) +{ + _logger.LogError("User is null when checking access to resource"); + return false; +}
110-125: Log unsuccessful authorization requests for better debuggingWhen the authorization request fails in
AuthorizeRequest, the method returnsnullwithout logging the failure, which can hinder troubleshooting.Consider logging unsuccessful responses to aid in debugging:
if (!response.IsSuccessStatusCode) { + _logger.LogError("Authorization request failed with status code {StatusCode}", response.StatusCode); return null; }Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs (1)
1070-1073: Rename variableconfirmResponsetoreadResponsefor clarityIn the test method
UpdateCorrespondenceStatus_MarkAsRead_WithoutFetched_ReturnsBadRequest, the variableconfirmResponseis used to store the response from themarkasreadendpoint. To improve readability and avoid confusion, consider renaming the variable toreadResponseto reflect the action being tested.Apply this diff to rename the variable:
- var confirmResponse = await _recipientClient.PostAsync($"correspondence/api/v1/correspondence/{correspondenceId}/markasread", null); - Assert.Equal(HttpStatusCode.BadRequest, confirmResponse.StatusCode); + var readResponse = await _recipientClient.PostAsync($"correspondence/api/v1/correspondence/{correspondenceId}/markasread", null); + Assert.Equal(HttpStatusCode.BadRequest, readResponse.StatusCode);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
.github/actions/update-infrastructure/action.yml(0 hunks).github/workflows/ci-cd.yaml(4 hunks)Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs(7 hunks)Test/Altinn.Correspondence.Tests/NotificationTests.cs(3 hunks)src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(3 hunks)src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs(1 hunks)src/Altinn.Correspondence.API/Models/LegacyCorrespondenceItemExt.cs(1 hunks)src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs(1 hunks)src/Altinn.Correspondence.Application/DependencyInjection.cs(2 hunks)src/Altinn.Correspondence.Application/Errors.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesResponse.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs(4 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs(1 hunks)src/Altinn.Correspondence.Application/Helpers/CorrespondenceExtensions.cs(1 hunks)src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(2 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(2 hunks)src/Altinn.Correspondence.Integrations/Idporten/IdPortenXacmlMapper.cs(1 hunks)
💤 Files with no reviewable changes (1)
- .github/actions/update-infrastructure/action.yml
✅ Files skipped from review due to trivial changes (2)
- src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesHandler.cs
- src/Altinn.Correspondence.Application/GetCorespondences/LegacyGetCorrespondencesResponse.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs
- src/Altinn.Correspondence.Application/DependencyInjection.cs
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#387
File: src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs:72-75
Timestamp: 2024-10-24T12:47:27.417Z
Learning: The `IsConfirmationNeeded` property in the `CorrespondenceEntity` class is no longer a required property.
🪛 yamllint
.github/workflows/ci-cd.yaml
[error] 47-47: trailing spaces
(trailing-spaces)
[error] 49-49: trailing spaces
(trailing-spaces)
[error] 131-131: trailing spaces
(trailing-spaces)
[error] 135-135: trailing spaces
(trailing-spaces)
🔇 Additional comments (31)
src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)
8-8: Verify the nullable return type design.
The nullable return type suggests that the method might return null in some cases. Let's verify if this is the intended behavior.
Consider whether returning a non-nullable int with exceptions for access denial might be more explicit:
// Alternative signature:
Task<int> CheckUserAccessAndGetMinimumAuthLevel(...); // Throws UnauthorizedAccessException if access is denied✅ Verification successful
Based on the shell script results, I can now generate the final response as the implementation and usage patterns are clear.
The nullable return type is correctly designed and well-handled
The nullable int return type is intentional and properly implemented:
- Returns
nullto indicate access denial inAltinnAuthorizationService - Consumers like
LegacyGetCorrespondenceOverviewHandlerexplicitly handle the null case by returning an error - Dev service implementation maintains the contract by returning a nullable int
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check how the return value is handled in the implementation and usage
# Check implementation in dev and production services
echo "Checking service implementations:"
rg -A 10 "CheckUserAccessAndGetMinimumAuthLevel.*=.*" --type cs
# Check how the return value is handled by consumers
echo "\nChecking method usage:"
rg -A 5 "CheckUserAccessAndGetMinimumAuthLevel\(" --type cs
Length of output: 6818
src/Altinn.Correspondence.API/Models/LegacyCorrespondenceOverviewExt.cs (3)
1-11: LGTM! Well-structured class declaration with clear documentation.
The class is properly organized with appropriate namespaces, clear XML documentation, and follows naming conventions. The inheritance from CorrespondenceOverviewExt makes sense for legacy API compatibility.
15-40: Verify authorization checks before exposing sensitive data.
This model exposes several authorization-related properties (MinimumAuthenticationLevel, AuthorizedForSign, AllowDelete). Ensure proper authorization checks are implemented before returning this data to clients.
#!/bin/bash
# Look for authorization checks in handlers using this model
rg -A 5 "LegacyCorrespondenceOverviewExt.*=.*new" --type cs
rg -A 5 "IAuthorizationService|Authorize" --type cs15-16: Consider making MinimumAuthenticationLevel nullable.
The non-nullable int could be problematic if there's no authentication level set. Consider using int? to explicitly indicate when no authentication level is specified, or document the default value's meaning.
src/Altinn.Correspondence.Application/Helpers/CorrespondenceExtensions.cs (1)
35-35: Verify state transition rules for Fetched status.
The addition of CorrespondenceStatus.Fetched to valid statuses is logical for enabling attachment downloads. However, let's verify the state transition rules.
✅ Verification successful
Let me analyze the results from the shell scripts to verify the state transition rules for the Fetched status.
From the search results, I can see:
- The status transitions are tracked in the database with timestamps (
StatusChanged) and text descriptions (StatusText) - The codebase maintains status history for both correspondence and attachments
- The
IsAvailableForRecipientmethod inCorrespondenceExtensions.csis the key validation point for status access - The status transitions appear to be one-way (no reverting back to previous states)
- The order of statuses in the list represents the valid progression:
- Published -> Fetched -> Read -> Replied
- Each status change is timestamped and preserved in history
Based on this evidence, I can now generate the final response:
Addition of Fetched status is safe and follows existing patterns
The Fetched status is correctly positioned between Published and Read states in the valid status list. The codebase:
- Maintains full status history with timestamps
- Preserves state transition order
- Allows correspondence to remain available after being fetched
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for state transition rules or documentation
# to ensure Fetched status is handled correctly
# Look for state transition documentation
echo "Searching for state transition documentation..."
rg -i "correspondence.*status.*transition" --type md
# Look for status enum definition to understand all possible states
echo "Searching for CorrespondenceStatus enum definition..."
ast-grep --pattern 'enum CorrespondenceStatus {
$$$
}'
# Look for any validation or guard conditions around status transitions
echo "Searching for status transition validation..."
rg -A 5 "Status.*Changed|UpdateStatus|SetStatus"
Length of output: 120315
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs (2)
1-5: LGTM! Clean and minimal imports.
The imports are well-organized and contain only the necessary dependencies.
7-8: LGTM! Appropriate class design.
The class is correctly designed as a static utility class with appropriate access level.
Test/Altinn.Correspondence.Tests/NotificationTests.cs (2)
Line range hint 42-58: LGTM! Good refactoring of client creation.
The change to use _factory.CreateClientWithAddedClaims improves code reuse and maintains consistent client creation across tests.
70-72: LGTM! Good enhancement to test reliability.
Adding the fetch operation before marking as read ensures the test follows the correct business logic flow and prevents potential ReadBeforeFetched errors.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
92-92: LGTM! Consider verifying error handling in consumers.
The semantic change from CorrespondenceNotConfirmed to ArchiveBeforeConfirmed better describes the specific scenario where purging is blocked due to pending confirmation.
Let's verify if all consumers handle this error correctly:
src/Altinn.Correspondence.Integrations/Idporten/IdPortenXacmlMapper.cs (1)
131-157: Verify integration with attachment download logic.
Since this PR implements attachment download functionality, we should verify how this authentication level check is integrated with the download process.
Let's check for the usage of this method in the attachment download context:
✅ Verification successful
Let's gather more specific information about how the authentication level check is integrated with the download process.
Let's check the actual download handler implementation and authorization constants to understand the complete flow.
Based on the gathered information, I can now provide the final response:
Authentication level check is properly integrated with attachment download process
The implementation shows a secure multi-layer authorization approach for attachment downloads:
- Controller level: Protected by
[Authorize(Policy = AuthorizationConstants.Sender)]ensuring only authenticated senders can access - Handler level: Uses
IAltinnAuthorizationService.CheckUserAccesswhich internally utilizes theGetMinimumAuthLevelcheck - Proper error handling for unauthorized access scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to GetMinimumAuthLevel in attachment download related code
rg -l "GetMinimumAuthLevel|DownloadAttachment" --type cs
# Look for any authorization checks in attachment handlers
ast-grep --pattern 'class $_ {
$$$
Download$_($$$) {
$$$
}
$$$
}'
Length of output: 1667
Script:
#!/bin/bash
# Check how GetMinimumAuthLevel is used in download handlers
rg -A 10 "GetMinimumAuthLevel" src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentHandler.cs
# Check authorization service implementation
rg -A 10 "GetMinimumAuthLevel" src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Check attachment controller for authorization flow
rg -A 10 "Download" src/Altinn.Correspondence.API/Controllers/AttachmentController.cs
Length of output: 2968
Script:
#!/bin/bash
# Check download handler implementation
cat src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentHandler.cs
# Check authorization constants
cat src/Altinn.Correspondence.Application/Configuration/AuthorizationConstants.cs
# Check if there are any specific authorization requirements in the request
cat src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentRequest.cs
Length of output: 3558
src/Altinn.Correspondence.Application/Errors.cs (2)
49-49: LGTM! Improved error naming.
The rename from CorrespondenceNotConfirmed to ArchiveBeforeConfirmed makes the error more specific and clearer about the actual validation failure, while maintaining the same error code and message.
57-59: LGTM! Well-structured error definitions.
The new error definitions are:
- Properly sequenced with unique error codes
- Have clear, concise messages
- Use appropriate HTTP status codes
- Support the workflow requirements for correspondence operations
These additions align well with the PR's objective of implementing attachment download functionality.
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (2)
67-70: Good practice: Extracted validation logic into a separate method
Extracting the validation logic into ValidateUpdateRequest enhances readability and maintainability by isolating validation concerns from the main Process method.
85-100: Ensure all previous validation checks are preserved
The new ValidateUpdateRequest method centralizes status validation effectively. However, please verify that all validation conditions from the original Process method are included in this new method, especially any checks against invalid status transitions that may have been present before refactoring.
Run the following script to compare previous and current validation logic:
✅ Verification successful
All validation checks are properly preserved in the refactored code
After comparing both versions, I can confirm that all essential validation checks are present and properly handled. The new ValidateUpdateRequest method correctly encapsulates the status transition validations that were previously in the Process method, specifically:
- Read status requires prior Fetched status
- Confirm status requires prior Fetched status
- Archive status requires prior Confirmed status (when confirmation is needed)
The other validations remain in the Process method as they deal with different concerns:
- Correspondence existence
- Authorization checks
- Recipient validation
- Status availability and purge checks
- Marked unread handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare old and new validation logic to ensure all cases are covered.
# Expected result: All important validation checks are present in the new method.
# Fetch the previous version of the handler
git show HEAD~1:src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs > old_handler.cs
# Extract validation conditions from the old handler
grep -E 'if\s*\(' old_handler.cs > old_validations.txt
# Extract validation conditions from the new ValidateUpdateRequest method
grep -E 'if\s*\(' -A 5 src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs | grep -A 5 'private static Error\?' > new_validations.txt
# Compare the old and new validations
diff old_validations.txt new_validations.txt
Length of output: 1285
Script:
#!/bin/bash
# Let's get both the old and new handler content for a proper comparison
echo "=== Previous version ==="
git show HEAD~1:src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs
echo -e "\n=== Current version ==="
cat src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs
Length of output: 12439
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs (5)
12-12: Consistency in response types
Updating the handler to use LegacyGetCorrespondenceOverviewResponse ensures alignment with the new response model and enhances code clarity.
33-33: Update method return type to match new response model
Changing the return type of the Process method to OneOf<LegacyGetCorrespondenceOverviewResponse, Error> reflects the updated response structure and maintains consistency.
37-37: Appropriate use of InvalidPartyId error
Returning Errors.InvalidPartyId when the PartyId is invalid provides precise feedback to the caller about the nature of the error.
126-126: Verify the safe casting of minimumAuthLevel to int
Ensure that minimumAuthLevel can be safely cast to an int without risking data loss or runtime exceptions. If minimumAuthLevel is already an int or an enum with underlying int values, the cast is appropriate.
43-43: 🛠️ Refactor suggestion
Use a more general error code when party information is missing
When userParty is null or lacks identification numbers, returning Errors.CouldNotFindOrgNo might not accurately represent the issue, especially for individuals without an organization number. Consider using a more general error code like Errors.PartyNotFound to improve clarity.
Apply this diff to enhance error handling:
if (userParty == null || (string.IsNullOrEmpty(userParty.SSN) && string.IsNullOrEmpty(userParty.OrgNumber)))
{
- return Errors.CouldNotFindOrgNo;
+ return Errors.PartyNotFound;
}Likely invalid or redundant comment.
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (1)
75-88: Verify unrestricted access in development environment
In the CheckMigrationAccess method, returning true when running in a development environment may pose security risks if not handled carefully.
Please ensure that granting full access in development is intentional and complies with security policies.
Test/Altinn.Correspondence.Tests/CorrespondenceControllerTests.cs (10)
872-872: Test correctly verifies status remains Published after recipient retrieval
The assertion ensures that the correspondence status remains Published even after the recipient fetches the correspondence overview, which is the expected behavior.
897-897: Test confirms status remains Published for sender accessing correspondence
The test correctly verifies that when the sender accesses the correspondence overview, the status remains Published and the Fetched status is not added to the status history.
991-991: Test ensures status remains Published when sender fetches details
The assertion correctly checks that the status remains Published when the sender retrieves the correspondence details, and that the Fetched status is not recorded in the status history.
Line range hint 995-1007: Test verifies 404 response for non-existent correspondence
The test method UpdateCorrespondenceStatus_CorrespondenceNotExists_Return404 appropriately checks that attempts to update the status of a non-existent correspondence return a 404 Not Found response.
1008-1025: Test validates 404 response when updating unpublished correspondence
The test UpdateCorrespondenceStatus_CorrespondenceNotPublished_Return404 correctly ensures that attempting to update the status of an unpublished correspondence results in a 404 Not Found response.
1030-1055: Test verifies successful status updates on published correspondence
The test UpdateCorrespondenceStatus_CorrespondencePublished_ReturnOk effectively confirms that status updates (mark as read, mark as unread) on a published correspondence return 200 OK and the correspondence status updates appropriately.
1089-1092: Test correctly handles BadRequest when confirming without fetching
The test UpdateCorrespondenceStatus_ToConfirmed_WithoutFetched_ReturnsBadRequest appropriately verifies that attempting to confirm a correspondence without first fetching it results in a 400 Bad Request.
1108-1113: Test confirms successful confirmation after fetching correspondence
The test UpdateCorrespondenceStatus_ToConfirmed_WhenCorrespondenceIsFetched_GivesOk accurately checks that confirming a fetched correspondence succeeds with a 200 OK response.
1152-1155: Test verifies successful archival after confirmation
In UpdateCorrespondenceStatus_ToArchived_WithConfirmation_WhenConfirmationNeeded_GivesOk, the test correctly ensures that after fetching and confirming a correspondence requiring confirmation, archiving it succeeds.
1336-1338: Test confirms deletion of confirmed correspondence requiring confirmation
The test Delete_Published_Correspondence_WithConfirmation_WhenConfirmationNeeded_Gives_OK appropriately verifies that after fetching and confirming a correspondence that requires confirmation, deleting it returns 200 OK.
...respondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewResponse.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Models/LegacyCorrespondenceItemExt.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.API/Mappers/LegacyCorrespondenceOverviewMapper.cs
Outdated
Show resolved
Hide resolved
...n.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs
Outdated
Show resolved
Hide resolved
...rrespondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs
Outdated
Show resolved
Hide resolved
...rrespondence.Application/GetCorrespondenceOverview/LegacyGetCorrespondenceOverviewHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs (3)
8-8: Remove unused import.The
ReverseMarkdown.Convertersimport is not used in this file.-using ReverseMarkdown.Converters;
57-61: Consider using authorization service for recipient check.The recipient check is currently done by direct comparison, but this logic should ideally be moved to the authorization service for better separation of concerns and future extensibility.
- bool isRecipient = correspondence.Recipient == party.OrgNumber || correspondence.Recipient == party.SSN; - if (!isRecipient) - { - return Errors.CorrespondenceNotFound; - } + var isRecipient = await _altinnAuthorizationService.IsRecipient(correspondence, party); + if (!isRecipient) + { + return Errors.CorrespondenceNotFound; + }
67-68: Consider moving background job to a separate method.The background job enqueuing logic could be moved to a separate private method for better readability and maintainability.
var attachmentStream = await _storageRepository.DownloadAttachment(attachment.Id, cancellationToken); - _backgroundJobClient.Enqueue<IDialogportenService>((dialogportenService) => dialogportenService.CreateInformationActivity(request.CorrespondenceId, DialogportenActorType.Recipient, DialogportenTextType.DownloadStarted, attachment.FileName ?? attachment.Name)); + await EnqueueDownloadActivity(request.CorrespondenceId, attachment.FileName ?? attachment.Name);Add the following private method:
private Task EnqueueDownloadActivity(Guid correspondenceId, string fileName) { _backgroundJobClient.Enqueue<IDialogportenService>((dialogportenService) => dialogportenService.CreateInformationActivity( correspondenceId, DialogportenActorType.Recipient, DialogportenTextType.DownloadStarted, fileName)); return Task.CompletedTask; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs(1 hunks)src/Altinn.Correspondence.Application/Helpers/UserClaimsHelper.cs(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs (4)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:52-52
Timestamp: 2024-11-01T13:32:16.888Z
Learning: When reviewing pull requests in the `Altinn/altinn-correspondence` repository, suggestions for adding error handling may be considered outside the scope if they are not directly related to the PR's objectives.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:31-51
Timestamp: 2024-11-01T13:31:21.051Z
Learning: In the `LegacyDownloadCorrespondenceAttachmentHandler.Process` method in `src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs`, when request parameters are omitted, a 404 is returned, so explicit request validation may not be necessary.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateUploadAttachmentHandler.cs:39-39
Timestamp: 2024-10-21T09:58:25.465Z
Learning: In the `Altinn.Correspondence.Application` codebase, error handling is performed using `OneOf` result types instead of exceptions.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs:10-10
Timestamp: 2024-10-21T10:04:04.975Z
Learning: In the Altinn.Correspondence application, when injecting dependencies such as `UploadHelper`, it's acceptable to inject the concrete class directly without defining an interface, especially when it is a dependency inside the same project.
🔇 Additional comments (3)
src/Altinn.Correspondence.Application/Helpers/UserClaimsHelper.cs (2)
21-21: LGTM! Constant follows established patterns.
The constant follows the class's naming conventions and uses the correct URN format for Altinn claims.
30-34: Verify party ID validation in authorization flow.
The current implementation might need additional validation:
- Ensure downstream code properly handles null returns for authorization decisions
- Consider validating that the party ID is a positive number
Let's verify the usage context:
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs (1)
34-73: LGTM with minor suggestions.
The overall implementation looks good:
- Proper error handling using OneOf
- Comprehensive validation steps
- Proper null checks
- Correct usage of cancellation token
Note: As mentioned in the PR objectives, authorization will be addressed in a subsequent PR.
src/Altinn.Correspondence.Application/Helpers/UserClaimsHelper.cs
Outdated
Show resolved
Hide resolved
...pplication/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs
Outdated
Show resolved
Hide resolved
…irmed (#427) * fix: validate correspondence has been fetched before it can be read and confirmed * update tests * typo fix * Handle potential null value for IsConfirmationNeeded * reuse factory for client initialization * rename testname * add Fetched to statuses available for Recipient
CelineTrammi
deleted the
feat/legacy-download-correspondence-attachment
branch
Description
Add initial logic for downloading attachment for correspondence in A2 GUI.
This is missing authorization, but will be implemented in a future PR.
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
New Features
Bug Fixes
Chores