Instance authorization #531
Conversation
…DP verifies whether the caller is allowed to receive/send on behalf of recipient/sender
…o hide authorization logic/complexity. Make the "raw" function private.
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a new project, Changes
Possibly related issues
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (27)
src/Altinn.Correspondence.Common/Helpers/Models/SystemUserAuthorizationDetails.cs (3)
5-18: Add XML documentation and improve null safetyConsider the following improvements:
- Add XML documentation to describe the purpose and usage of each property
- Initialize collections to prevent null reference exceptions
- Consider using nullable reference types for optional properties
Here's a suggested implementation:
+ /// <summary> + /// Represents authorization details for a system user. + /// </summary> public class SystemUserAuthorizationDetails { + /// <summary> + /// Gets or sets the type of authorization. + /// </summary> [JsonPropertyName("type")] - public string Type { get; set; } + public string? Type { get; set; } + /// <summary> + /// Gets or sets the list of system user identifiers. + /// </summary> [JsonPropertyName("systemuser_id")] - public List<string> SystemUserId { get; set; } + public List<string> SystemUserId { get; set; } = new(); + /// <summary> + /// Gets or sets the organization details for the system user. + /// </summary> [JsonPropertyName("systemuser_org")] - public SystemUserOrg SystemUserOrg { get; set; } + public SystemUserOrg? SystemUserOrg { get; set; } + /// <summary> + /// Gets or sets the system identifier. + /// </summary> [JsonPropertyName("system_id")] - public string SystemId { get; set; } + public string? SystemId { get; set; } }
20-24: Add documentation and initialize collectionSimilar to the previous class, consider adding documentation and initializing the collection.
+ /// <summary> + /// Represents a collection of system user authorization details. + /// </summary> public class SystemUserAuthorization { + /// <summary> + /// Gets or sets the list of authorization details. + /// </summary> [JsonPropertyName("authorization_details")] - public List<SystemUserAuthorizationDetails> AuthorizationDetails { get; set; } + public List<SystemUserAuthorizationDetails> AuthorizationDetails { get; set; } = new(); }
26-33: Consider JSON naming convention and add documentation
- The JSON property name "ID" uses uppercase, which is inconsistent with typical JSON naming conventions (should be "id")
- Documentation and null safety improvements are needed
Here's a suggested implementation:
+ /// <summary> + /// Represents organization details for a system user. + /// </summary> public class SystemUserOrg { + /// <summary> + /// Gets or sets the authority associated with the organization. + /// </summary> [JsonPropertyName("authority")] - public string Authority { get; set; } + public string? Authority { get; set; } + /// <summary> + /// Gets or sets the organization identifier. + /// </summary> - [JsonPropertyName("ID")] + [JsonPropertyName("id")] - public string ID { get; set; } + public string? ID { get; set; } }src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs (1)
28-31: Documentation needs improvementThe documentation should be updated to:
- Match the parameter name with the method signature
- Include validation behavior for edge cases
- Add usage examples for clarity
/// <summary> -/// Extracts the identifier from a colon-separated string that may contain a prefix. +/// Extracts the identifier from a colon-separated string that may contain a prefix. For example, "0192:123456789" returns "123456789". /// </summary> -/// <param name="orgNumber">The organization number to format</param> +/// <param name="orgOrSsnNumber">The organization number or social security number that may contain a prefix</param> +/// <remarks> +/// The method will return the input string as-is if no colon is present. +/// </remarks> /// <returns>Returns the last sequence succeeding a colon.</returns>src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (2)
Line range hint
30-40: Document the hardcoded authentication levelConsider adding a comment explaining why level 3 is used as the default authentication level in the dev service. This will help other developers understand the implications during testing.
public Task<int?> CheckUserAccessAndGetMinimumAuthLevel(ClaimsPrincipal? user, string ssn, string resourceId, List<ResourceAccessLevel> rights, string recipientOrgNo, CancellationToken cancellationToken = default) { + // Default to authentication level 3 for development/testing purposes + // Production should determine this based on actual security requirements return Task.FromResult((int?)3); }
8-9: Add warning about development-only usageConsider adding a class-level documentation comment warning that this is a development-only implementation and should never be used in production.
+ /// <summary> + /// Development-only implementation of IAltinnAuthorizationService that always returns successful results. + /// WARNING: This service bypasses all authorization checks and must never be used in production. + /// </summary> public class AltinnAuthorizationDevService : IAltinnAuthorizationServicesrc/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs (2)
27-27: Document the purpose of the null parameterThe
nullparameter inCheckAccessAsSendershould be documented to explain its purpose and impact on authorization decisions.
Line range hint
23-46: Good architectural improvement in authorization flowThe refactoring successfully:
- Simplifies the authorization flow by removing instance-level checks
- Maintains proper separation of concerns with the authorization service
- Preserves important cross-cutting concerns (transactions, error handling, event publishing)
This aligns well with the PR's objective of cleaning up the authorization process while ensuring system integrity.
src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs (1)
23-28: Consider documenting authorization behavior.Since this implementation supports delegated access (as mentioned in PR objectives), consider adding XML documentation to clarify:
- How delegation is handled
- What roles/rights are required for access
- Examples of valid sender formats
+ /// <summary> + /// Processes the attachment details request after verifying authorization. + /// Supports both direct and delegated access through the authorization service. + /// </summary> + /// <param name="attachmentId">The ID of the attachment to retrieve</param> + /// <param name="user">The authenticated user (supports delegated access)</param> + /// <param name="cancellationToken">Cancellation token</param> public async Task<OneOf<GetAttachmentDetailsResponse, Error>> Process(src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs (1)
42-42: Add defensive null check for attachment filename.While the null coalescing operator handles the case where
FileNameis null by falling back toName, consider adding a default value if both are null to prevent potential issues inDialogportenService.-backgroundJobClient.Enqueue<IDialogportenService>((dialogportenService) => dialogportenService.CreateInformationActivity(request.CorrespondenceId, DialogportenActorType.Recipient, DialogportenTextType.DownloadStarted, attachment.FileName ?? attachment.Name)); +backgroundJobClient.Enqueue<IDialogportenService>((dialogportenService) => dialogportenService.CreateInformationActivity(request.CorrespondenceId, DialogportenActorType.Recipient, DialogportenTextType.DownloadStarted, attachment.FileName ?? attachment.Name ?? "unnamed_attachment"));src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (2)
29-30: Consider enhancing error message clarityThe error
CouldNotDetermineCallermight benefit from additional context about why the caller couldn't be determined (missing claims, invalid format, etc.).- return Errors.CouldNotDetermineCaller; + return Errors.CouldNotDetermineCaller with { Message = "Could not determine caller: organization ID not found in claims" };
44-44: Track instance delegation implementationThe TODO comment aligns with the PR objective of supporting delegated access. This should be tracked separately to ensure implementation.
Would you like me to create a GitHub issue to track the implementation of instance delegation retrieval? I can include:
- Acceptance criteria based on PR objectives
- Link to this PR for context
- Initial technical approach
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
Line range hint
13-25: Consider adding XML documentation.To improve maintainability, consider adding XML documentation to describe:
- The handler's responsibility
- The expected input/output
- The authorization requirements
- Possible error cases
Example:
+/// <summary> +/// Handles updates to correspondence status while ensuring proper authorization and data consistency. +/// </summary> +/// <remarks> +/// Authorization is performed via <see cref="IAltinnAuthorizationService"/> which supports both direct +/// and delegated access for recipients. +/// </remarks> public class UpdateCorrespondenceStatusHandler( IAltinnAuthorizationService altinnAuthorizationService, ICorrespondenceRepository correspondenceRepository, ICorrespondenceStatusRepository correspondenceStatusRepository, IEventBus eventBus, UpdateCorrespondenceStatusHelper updateCorrespondenceStatusHelper, ILogger<UpdateCorrespondenceStatusHandler> logger) : IHandler<UpdateCorrespondenceStatusRequest, Guid>src/Altinn.Correspondence.Application/PurgeAttachment/PurgeAttachmentHandler.cs (1)
Line range hint
44-52: Consider extracting correspondence status validationThe correspondence status check logic could be extracted into a separate method for better readability and reusability.
+ private static bool AreAllCorrespondencesPurged(IReadOnlyCollection<CorrespondenceEntity> correspondences) + { + return correspondences.All(correspondence => + { + var latestStatus = correspondence.GetHighestStatus(); + return latestStatus?.Status.IsPurged() ?? false; + }); + } public async Task<OneOf<Guid, Error>> Process(Guid attachmentId, ClaimsPrincipal? user, CancellationToken cancellationToken) { // ... existing code ... var correspondences = await correspondenceRepository.GetCorrespondencesByAttachmentId(attachmentId, true, cancellationToken); - bool allCorrespondencesArePurged = correspondences - .All(correspondence => - { - var latestStatus = correspondence.GetHighestStatus(); - if (latestStatus is null) return false; - return latestStatus.Status.IsPurged(); - }); + bool allCorrespondencesArePurged = AreAllCorrespondencesPurged(correspondences); if (correspondences.Count != 0 && !allCorrespondencesArePurged) { return Errors.PurgeAttachmentWithExistingCorrespondence; }src/Altinn.Correspondence.Application/CancelNotification/CancelNotificationHandler.cs (2)
Line range hint
33-33: Remove ClaimsPrincipal parameter from Hangfire jobThe addition of
ClaimsPrincipal? _parameter to this Hangfire background job is problematic:
- ClaimsPrincipal is not serializable and will cause issues when Hangfire tries to persist the job
- The parameter appears unused (named
_)Apply this diff to fix the issue:
- public async Task Process(PerformContext context, Guid correspondenceId, ClaimsPrincipal? _, CancellationToken cancellationToken = default) + public async Task Process(PerformContext context, Guid correspondenceId, CancellationToken cancellationToken = default)If user claims are needed in the background job, consider passing the necessary claims data explicitly in a serializable request object.
Line range hint
26-29: Consider improving error handling and configurationA few suggestions to enhance the code:
- Move retry configuration to app settings
- Improve error message construction
- Make Slack channel configurable
Consider these improvements:
- private const string TestChannel = "#test-varslinger"; - private const string RetryCountKey = "RetryCount"; - private const int MaxRetries = 10; + private readonly string _slackChannel; + private readonly string _retryCountKey; + private readonly int _maxRetries; + public CancelNotificationHandler( + ILogger<CancelNotificationHandler> logger, + ICorrespondenceRepository correspondenceRepository, + IAltinnNotificationService altinnNotificationService, + ISlackClient slackClient, + IBackgroundJobClient backgroundJobClient, + IHostEnvironment hostEnvironment, + IConfiguration configuration) + { + _slackChannel = configuration.GetValue<string>("Notifications:SlackChannel") ?? "#test-varslinger"; + _retryCountKey = configuration.GetValue<string>("Notifications:RetryCountKey") ?? "RetryCount"; + _maxRetries = configuration.GetValue<int>("Notifications:MaxRetries", 10); + // ... rest of the initialization + } internal async Task CancelNotification(Guid correspondenceId, List<CorrespondenceNotificationEntity> notificationEntities, int retryAttempts, CancellationToken cancellationToken) { - var env = hostEnvironment.EnvironmentName; - var error = $"Error while attempting to cancel notifications for correspondenceId: {correspondenceId} in environment: {env}."; + var errorBuilder = new StringBuilder() + .AppendLine($"Error while attempting to cancel notifications:") + .AppendLine($"- CorrespondenceId: {correspondenceId}") + .AppendLine($"- Environment: {hostEnvironment.EnvironmentName}") + .AppendLine($"- Retry Attempt: {retryAttempts}/{_maxRetries}"); foreach (var notification in notificationEntities) { if (notification.RequestedSendTime <= DateTimeOffset.UtcNow) continue; string? notificationOrderId = notification.NotificationOrderId?.ToString(); if (string.IsNullOrWhiteSpace(notificationOrderId)) { - error += $"NotificationOrderId is null for notificationId: {notification.Id}"; - if (retryAttempts == MaxRetries) SendSlackNotificationWithMessage(error); - throw new Exception(error); + errorBuilder.AppendLine($"- NotificationOrderId is null for notificationId: {notification.Id}"); + var errorMessage = errorBuilder.ToString(); + if (retryAttempts == _maxRetries) SendSlackNotificationWithMessage(errorMessage); + throw new NotificationCancellationException(errorMessage); }Also applies to: 44-71
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (1)
Line range hint
54-63: Consider enhancing error handling for invalid format.While the code handles the case where the party identifier is neither an organization nor a person number, it might be helpful to provide more specific error information about the invalid format.
Consider this enhancement:
if (party.IsOrganizationNumber()) { resourceCategory.Attribute.Add(DecisionHelper.CreateXacmlJsonAttribute(AltinnXacmlUrns.OrganizationNumberAttribute, party.WithoutPrefix(), DefaultType, DefaultIssuer)); } else if (party.IsSocialSecurityNumber()) { resourceCategory.Attribute.Add(DecisionHelper.CreateXacmlJsonAttribute(PersonAttributeId, party, DefaultType, DefaultIssuer)); } else { - throw new InvalidOperationException("RecipientId is not a valid organization or person number"); + throw new InvalidOperationException($"RecipientId '{party}' is not a valid organization or person number. Expected format: XXXXXXXXX for org number or XXXXXXXXXXX for SSN."); }src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/DialogTokenXacmlMapper.cs (2)
112-114: Document the claim type constantsThe meaning of the claim type "c" should be documented for better maintainability. Consider using a constant with a descriptive name.
+ /// <summary> + /// Claim type "c" represents a consumer claim in the Dialogporten context. + /// </summary> private static bool IsConsumerClaim(string value) { - return value.Equals("c"); + const string CONSUMER_CLAIM_TYPE = "c"; + return value.Equals(CONSUMER_CLAIM_TYPE); }
Line range hint
1-190: Consider documenting the new authorization modelWhile these changes align with the PR objective of cleaning up the authorization process, it would be valuable to:
- Document the new authorization flow without instance checking
- Explain the relationship between consumer claims and delegated authority
- Create an ADR (Architecture Decision Record) for this significant change in authorization strategy
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (2)
Line range hint
279-307: Consider extracting notification timing constants and regex patternsThe notification creation logic contains several hardcoded values and complex regex patterns that should be extracted for better maintainability.
Consider these improvements:
- Extract timing constants:
+ private const int NOTIFICATION_DELAY_MINUTES = 5; + private const int REMINDER_DELAY_DAYS_PRODUCTION = 7; + private const int REMINDER_DELAY_HOURS_DEVELOPMENT = 1;
- Extract regex patterns to constants:
+ private static readonly Regex ORGANIZATION_NO_PREFIX_PATTERN = new(@"^\d{9}$"); + private static readonly Regex ORGANIZATION_WITH_PREFIX_PATTERN = new(@"^\d{4}:\d{9}$"); + private static readonly Regex PERSON_NUMBER_PATTERN = new(@"^\d{11}$");
Line range hint
308-325: Consider refactoring recipient type determinationThe logic for determining recipient type and extracting organization numbers is complex and could benefit from being extracted into a separate method.
Consider creating a helper method:
+ private (string? orgNr, string? personNr, NotificationContent? content) DetermineRecipientInfo( + string recipient, + List<NotificationContent> contents) + { + if (ORGANIZATION_NO_PREFIX_PATTERN.IsMatch(recipient)) + { + return (recipient, null, contents.FirstOrDefault(c => + c.RecipientType == RecipientType.Organization || c.RecipientType == null)); + } + // ... similar logic for other cases + }src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (1)
12-12: Inconsistent default value forCancellationTokenparameterThe
CheckAccessAsAnymethod does not assign a default value to theCancellationTokenparameter, unlike the other methods in the interface. For consistency and ease of use, consider setting a default value:- Task<bool> CheckAccessAsAny(ClaimsPrincipal? user, string resource, string party, CancellationToken cancellationToken); + Task<bool> CheckAccessAsAny(ClaimsPrincipal? user, string resource, string party, CancellationToken cancellationToken = default);src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (3)
30-37: Consider consolidating access checks to improve maintainabilityThe access checks for sender and recipient are performed separately. Consider consolidating them into a single method or utilizing a combined check to reduce redundancy and enhance code maintainability.
42-57: Reduce duplication in validation logicThe validation blocks for sender and recipient have similar structures. Refactoring them into a shared method can minimize code duplication and improve readability.
Line range hint
63-80: Avoid variable shadowing ofcancellationTokenThe
cancellationTokenparameter in the lambda expression shadows the outercancellationToken. Renaming the inner parameter can prevent potential confusion and enhance code clarity.src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (1)
Line range hint
45-63: Consider refactoring the recipient access logic for clarityThe nested conditions and status update logic can be refactored into a separate method to improve readability and maintainability.
Apply this refactoring:
+private async Task<OneOf<Success, Error>> HandleRecipientAccess( + CorrespondenceEntity correspondence, + CorrespondenceStatusEntity latestStatus, + CancellationToken cancellationToken) +{ + if (!latestStatus.Status.IsAvailableForRecipient()) + { + logger.LogWarning("Rejected because correspondence not available for recipient in current state."); + return Errors.CorrespondenceNotFound; + } + await correspondenceStatusRepository.AddCorrespondenceStatus(new CorrespondenceStatusEntity + { + CorrespondenceId = correspondence.Id, + Status = CorrespondenceStatus.Fetched, + StatusText = CorrespondenceStatus.Fetched.ToString(), + StatusChanged = DateTimeOffset.UtcNow + }, cancellationToken); + return new Success(); +}Then update the main process method:
if (!hasAccessAsSender && hasAccessAsRecipient) { - if (!latestStatus.Status.IsAvailableForRecipient()) - { - logger.LogWarning("Rejected because correspondence not available for recipient in current state."); - return Errors.CorrespondenceNotFound; - } - await correspondenceStatusRepository.AddCorrespondenceStatus(new CorrespondenceStatusEntity - { - CorrespondenceId = correspondence.Id, - Status = CorrespondenceStatus.Fetched, - StatusText = CorrespondenceStatus.Fetched.ToString(), - StatusChanged = DateTimeOffset.UtcNow - }, cancellationToken); + var recipientAccessResult = await HandleRecipientAccess( + correspondence, + latestStatus, + cancellationToken); + if (recipientAccessResult.IsT1) + { + return recipientAccessResult.AsT1; + } }This refactoring enhances modularity and makes the main method more concise.
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs (1)
43-77: Refactor to reduce code duplication in access check methodsThe methods
CheckAccessAsSender,CheckAccessAsRecipient, andCheckAccessAsAnyshare similar structures and invokeCheckUserAccesswith different parameters. Consider refactoring to consolidate common logic, perhaps by introducing a method that sets up parameters based on access type, to improve maintainability.Also applies to: 118-134
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
Altinn.Correspondence.sln(2 hunks)src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs(1 hunks)src/Altinn.Correspondence.Application/CancelNotification/CancelNotificationHandler.cs(1 hunks)src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentHandler.cs(2 hunks)src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs(1 hunks)src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs(3 hunks)src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs(3 hunks)src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs(3 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs(2 hunks)src/Altinn.Correspondence.Application/Helpers/UserClaimsHelper.cs(1 hunks)src/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs(2 hunks)src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs(2 hunks)src/Altinn.Correspondence.Application/PurgeAttachment/PurgeAttachmentHandler.cs(2 hunks)src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs(2 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs(2 hunks)src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs(1 hunks)src/Altinn.Correspondence.Common/Helpers/Models/SystemUserAuthorizationDetails.cs(1 hunks)src/Altinn.Correspondence.Common/Helpers/Models/TokenConsumer.cs(1 hunks)src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs(1 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(3 hunks)src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs(1 hunks)src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/DialogTokenXacmlMapper.cs(5 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs
- src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
- src/Altinn.Correspondence.Common/Helpers/Models/TokenConsumer.cs
🧰 Additional context used
📓 Learnings (10)
src/Altinn.Correspondence.Application/CancelNotification/CancelNotificationHandler.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs:210-210
Timestamp: 2024-11-19T09:15:16.216Z
Learning: In the Altinn Correspondence project, when scheduling Hangfire background jobs in C#, avoid passing `ClaimsPrincipal` as it is not serializable. If the background job requires user claims, pass the necessary claims in the request object explicitly.
src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentHandler.cs (5)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:52-52
Timestamp: 2024-11-12T05:02:53.065Z
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: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs:10-10
Timestamp: 2024-11-12T05:02:53.065Z
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.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:31-51
Timestamp: 2024-11-12T05:02:53.065Z
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-11-12T05:02:53.065Z
Learning: In the `Altinn.Correspondence.Application` codebase, error handling is performed using `OneOf` result types instead of exceptions.
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs (6)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#459
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceResponse.cs:18-23
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the application folder, classes should not have the `Ext` suffix; the `Ext` suffix is reserved for models in the API layer.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#517
File: src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cs:11-15
Timestamp: 2024-11-22T14:30:17.012Z
Learning: The project uses C# 12 features, including primary constructors for classes (e.g., in `src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceHandler.cs`).
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:52-52
Timestamp: 2024-11-12T05:02:53.065Z
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.
src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (3)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateUploadAttachmentHandler.cs:39-39
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the `Altinn.Correspondence.Application` codebase, error handling is performed using `OneOf` result types instead of exceptions.
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (2)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#459
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceResponse.cs:18-23
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the application folder, classes should not have the `Ext` suffix; the `Ext` suffix is reserved for models in the API layer.
src/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#480
File: src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs:61-63
Timestamp: 2024-11-12T12:30:11.505Z
Learning: In the `PurgeCorrespondenceHandler` class, the `IsAffiliatedWithCorrespondence` method verifies if the user is either the sender or the recipient, so additional checks for user affiliation are redundant.
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-11-12T05:02:53.065Z
Learning: The `IsConfirmationNeeded` property in the `CorrespondenceEntity` class is no longer a required property.
src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs (4)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs:10-10
Timestamp: 2024-11-12T05:02:53.065Z
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.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:57-59
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In non-development environments, the `MalwareScanResultHandler` updates the attachment status after performing a malware scan.
🔇 Additional comments (45)
src/Altinn.Correspondence.Common/Helpers/Models/SystemUserAuthorizationDetails.cs (2)
1-4: LGTM!
The namespace organization and use of System.Text.Json is appropriate.
1-33: Verify integration with PDP request flow
Based on the PR objectives, these models should support the new authorization flow where instance checking is replaced with recipient/sender information in PDP requests. Let's verify the integration points.
✅ Verification successful
Models properly integrated with new authorization flow
The verification shows that these models are correctly integrated into the new authorization system:
- The
SystemUserAuthorizationDetailsandSystemUserOrgmodels are used inClaimsPrincipalExtensions.csto extract organization IDs from system user claims - The authorization service has been refactored to support sender/recipient-based access control instead of instance checking:
- New methods like
CheckAccessAsSenderandCheckAccessAsRecipientreplace the old instance-based checks - The XACML mappers now handle organization numbers without prefixes for proper authorization requests
- New methods like
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of these models in PDP request handling
echo "Searching for PDP request integration..."
rg -l "SystemUserAuthorizationDetails" | xargs rg "PDP"
# Search for removal of instance checking
echo "Checking for removed instance checking..."
git diff main | rg -i "instance.*check"
Length of output: 279
Script:
#!/bin/bash
# Let's search for usages of these models in the codebase
echo "Searching for usages of SystemUserAuthorizationDetails..."
rg -A 5 "SystemUserAuthorizationDetails"
# Search for files containing authorization-related code
echo "Searching for authorization handling..."
rg -l "Authorization" src/
# Check for PDP-related files
echo "Searching for PDP-related files..."
fd -e cs -e json . src/ | grep -i pdp
# Look for test files that might validate the integration
echo "Searching for related test files..."
fd -e cs . test/ | grep -i "authorization\|pdp"
Length of output: 6399
Script:
#!/bin/bash
# Let's examine the authorization service implementation
echo "Checking authorization service implementation..."
rg -A 10 "class AltinnAuthorizationService" src/
# Look for XACML mappers that might use these models
echo "Checking XACML mappers..."
rg -A 10 "TokenXacmlMapper" src/
# Check for changes in authorization flow
echo "Examining authorization flow changes..."
git diff main -- "**/Authorization/**" "**/Auth/**"
# Look for the actual usage of SystemUserOrg
echo "Checking SystemUserOrg usage..."
rg -A 5 "SystemUserOrg" src/
Length of output: 20318
src/Altinn.Correspondence.Application/DownloadAttachment/DownloadAttachmentHandler.cs (3)
1-11: LGTM! Clean dependency management
The removal of UserClaimsHelper aligns with the PR's objective of streamlining the authorization process. The constructor is well-structured using primary constructor syntax.
13-14: LGTM! Proper nullability handling
The nullable ClaimsPrincipal parameter aligns with the team's security practices of verifying nullability within the method.
20-24: Verify the authorization check implementation
The CheckAccessAsSender call passes null as the fourth parameter. Let's verify this is the intended behavior and maintains proper access control.
✅ Verification successful
Based on the results, I can now generate the final response:
Passing null for the instance parameter is valid and consistent
The interface definition and implementation show that the instance parameter is explicitly defined as nullable (string?), and it's an optional parameter used for instance-specific authorization checks. In the context of downloading attachments, not having an instance ID is a valid scenario.
- The interface
IAltinnAuthorizationServiceclearly defines the parameter as nullable:string? instance - The implementation in
AltinnAuthorizationServicehandles this parameter appropriately - The dev service implementation also supports this pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of CheckAccessAsSender to verify consistent parameter usage
ast-grep --pattern 'CheckAccessAsSender($_, $_, $_, null, $_)'
# Look for the interface definition to understand the parameter purpose
rg -A 5 "CheckAccessAsSender.*CancellationToken" --type cs
Length of output: 4899
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs (2)
20-28: Verify consistent usage of CheckAccessAsSender overloads
The overloads look good and align with the PR objective of removing instance checking (note the optional instance parameter). However, we should verify that these overloads are used consistently across the codebase.
✅ Verification successful
Both overloads are consistently implemented across the codebase
The verification shows that both overloads are properly defined in the interface IAltinnAuthorizationService and consistently implemented in both AltinnAuthorizationService and AltinnAuthorizationDevService. The usage pattern is consistent:
- The
CorrespondenceEntityoverload is implemented in all required locations - The string parameters overload with optional instance parameter is also properly implemented everywhere
- Both implementations in
AltinnAuthorizationServiceuse the same underlyingCheckUserAccessmethod - Both implementations in
AltinnAuthorizationDevServicereturnTask.FromResult(true)as expected for a dev service
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usages of both overloads
echo "Searching for CorrespondenceEntity overload usage:"
rg "CheckAccessAsSender.*CorrespondenceEntity" -A 2
echo -e "\nSearching for string parameters overload usage:"
rg "CheckAccessAsSender.*resourceId.*sender" -A 2
Length of output: 3594
10-18: Verify production implementation of new authorization methods
The dev service implementation looks correct with appropriate method signatures. However, we should ensure that the production implementation of these methods in AltinnAuthorizationService properly validates the user claims and performs actual authorization checks.
✅ Verification successful
Production implementation of authorization methods verified
The production implementation in AltinnAuthorizationService properly implements both methods:
CheckAccessAsRecipientdelegates toCheckUserAccesswith appropriate read access level verification for the correspondence recipientCheckAccessAsAnydelegates toCheckUserAccesswith both read and write access level checks for the given resource and party
Both implementations align with the interface contract defined in IAltinnAuthorizationService and provide actual authorization checks through the CheckUserAccess method, unlike the dev service's hardcoded returns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the production implementation of these methods
ast-grep --pattern 'class AltinnAuthorizationService : IAltinnAuthorizationService {
$$$
CheckAccessAsAny($$$) {
$$$
}
$$$
CheckAccessAsRecipient($$$) {
$$$
}
$$$
}'
Length of output: 189
Script:
#!/bin/bash
# Let's first find the production service file
fd "AltinnAuthorizationService.cs" --type f --exclude "*Dev*" --exclude "*Test*"
# Then let's search for the interface definition to understand the contract
fd "IAltinnAuthorizationService.cs" --type f
# Let's also check the implementation with ripgrep for better context
rg "class AltinnAuthorizationService" -A 5
Length of output: 1239
Script:
#!/bin/bash
# Let's check the implementation of these specific methods in the production service
rg "CheckAccessAsAny|CheckAccessAsRecipient" -A 10 "src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs"
# Also check the interface definition to verify the contract
cat "src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs"
Length of output: 2399
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs (2)
1-8: LGTM: Clean and focused imports
The file structure follows best practices with appropriate imports and correct static class declaration for extension methods.
1-41: Verify security implications and usage patterns
As this is a security-critical component for authorization, let's verify its usage and potential security implications.
✅ Verification successful
Let me generate additional verification scripts to check for test coverage and security patterns more thoroughly.
Let me generate one final verification script to check the authorization flow and test coverage more specifically.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
Security implementation verified and properly integrated
The GetCallerOrganizationId extension method is securely implemented and properly integrated into the authorization flow:
- It's used within a robust authentication/authorization pipeline with proper JWT bearer and OpenID Connect schemes
- The method is consistently used across the codebase for caller identification
- Proper authorization policies are in place using the extracted organization ID
- Comprehensive test coverage exists for different authentication scenarios
- The implementation handles multiple token types (System user, Enterprise, Personal, Dialog) with proper claim validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of GetCallerOrganizationId
echo "Checking usage patterns of GetCallerOrganizationId:"
rg "GetCallerOrganizationId" -A 3
# Look for any direct claim access that might bypass this method
echo "Checking for potential bypass patterns:"
rg "authorization_details|urn:altinn:orgNumber|consumer|ID.*Claims\.FirstOrDefault" -A 3
# Check for any security-related tests
echo "Checking for related test coverage:"
fd -e cs -e csproj | xargs rg "GetCallerOrganizationId.*Test"
Length of output: 13636
Script:
#!/bin/bash
# Check for test files that might test the ClaimsPrincipalExtensions
echo "Checking for test files related to ClaimsPrincipalExtensions:"
fd "ClaimsPrincipalExtensions.*Test" -e cs
# Check for test files containing claims-related tests
echo "Checking for claims-related tests:"
rg -l "GetCallerOrganizationId|ClaimsPrincipal.*Test" Test/
# Check the actual usage in application code
echo "Checking application usage context:"
rg -B 3 -A 3 "GetCallerOrganizationId\(" src/
# Check for any potential security configurations
echo "Checking security configurations:"
fd "security|auth" -e json -e cs | xargs rg -l "authorization|authentication"
Length of output: 2387
Script:
#!/bin/bash
# Check the authorization service implementation
echo "Checking authorization service implementation:"
cat src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Check test coverage for authorization
echo "Checking authorization tests:"
rg -l "AltinnAuthorizationService.*Test" Test/
# Check cascade authentication handler
echo "Checking authentication handler:"
cat src/Altinn.Correspondence.API/Auth/CascadeAuthenticationHandler.cs
# Check for any security-related constants or configurations
echo "Checking security constants:"
rg "AuthorizationConstants" -A 2
Length of output: 45425
src/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs (2)
2-2: LGTM: Common helpers import added
The addition of the Common.Helpers namespace aligns with the introduction of the new Altinn.Correspondence.Common project.
23-28: Verify the WithoutPrefix() implementation
The authorization check has been updated to use CheckAccessAsSender which aligns with the PR's objective of simplifying authorization. However, we should verify the WithoutPrefix() implementation to ensure correct sender processing.
✅ Verification successful
WithoutPrefix() implementation is safe and correctly used
The implementation in src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs shows that WithoutPrefix() is a simple string extension method that splits the input on ":" and returns the last part. This is consistently used across the codebase to handle organization numbers and social security numbers with prefixes. The implementation is straightforward and safe:
public static string WithoutPrefix(this string orgOrSsnNumber)
{
return orgOrSsnNumber.Split(":").Last();
}The method is used consistently in authorization checks throughout the codebase, including:
- Authorization services
- Token XACML mappers
- Multiple correspondence handlers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for WithoutPrefix implementation and its tests
ast-grep --pattern 'WithoutPrefix()'
rg -A 5 'WithoutPrefix.*test'
Length of output: 70
Script:
#!/bin/bash
# Let's try to find the WithoutPrefix method implementation with a broader search
rg -A 5 "WithoutPrefix"
# Also search for extension methods that might contain this implementation
ast-grep --pattern 'static $_WithoutPrefix'
# Search for any test files containing "prefix" to catch related test cases
rg -l "prefix.*test" -i
Length of output: 59170
src/Altinn.Correspondence.Application/GetAttachmentOverview/GetAttachmentOverviewHandler.cs (3)
Line range hint 2-12: LGTM! Clean dependency injection setup
The changes align well with the PR objectives of cleaning up the authorization process. The removal of UserClaimsHelper and addition of IAltinnAuthorizationService streamlines the authorization dependencies.
24-24: Verify the WithoutPrefix implementation
Need to ensure that WithoutPrefix() correctly handles the sender identifier format for the authorization check.
✅ Verification successful
WithoutPrefix implementation is correctly handling sender identifiers
The implementation of WithoutPrefix() is found in src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs and correctly handles the sender identifier format by:
- Extracting the identifier part after the colon separator using
Split(":")and taking the last part - Being used consistently across the codebase for authorization checks
- Working in conjunction with proper validation of organization and person number formats (9 digits for org numbers, 11 digits for person numbers)
The implementation is properly integrated with the authorization flow and format validation as evidenced by:
- Usage in
AltinnAuthorizationServicefor access checks - Format validation in
DialogportenCorrespondenceMapperandAltinnRegisterServiceusing regex patterns - Consistent handling of both prefixed (e.g., "1234:123456789") and non-prefixed (e.g., "123456789") formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the WithoutPrefix implementation
ast-grep --pattern 'WithoutPrefix() {
$$$
}'
Length of output: 48
Script:
#!/bin/bash
# Search for WithoutPrefix method using ripgrep to find its implementation
rg -l "WithoutPrefix" --type cs
# After finding the files, let's see the implementation
rg "WithoutPrefix" --type cs -A 5 -B 5
Length of output: 30986
21-26: LGTM! Authorization changes align with PR objectives
The new authorization flow correctly implements the PDP-based checks and removes instance checking as intended. The nullable ClaimsPrincipal parameter follows the team's security practices.
Let's verify the authorization service implementation:
✅ Verification successful
Authorization implementation is properly secured and follows best practices
The implementation of CheckAccessAsSender in AltinnAuthorizationService is secure and well-implemented:
- Properly validates user authentication and throws if context is missing
- Uses PDP-based authorization through XACML requests
- Handles different token types (Dialogporten, Altinn) with appropriate validation
- Includes proper error handling and logging
- Correctly uses the
WithoutPrefix()method to normalize party identifiers - Implements proper resource owner validation through
EvaluateBypassConditions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the IAltinnAuthorizationService implementation for proper PDP request handling
ast-grep --pattern 'class $_ : IAltinnAuthorizationService {
$$$
CheckAccessAsSender($$$) {
$$$
}
$$$
}'
Length of output: 118
Script:
#!/bin/bash
# Let's try a broader search for the authorization service implementation
rg -l "IAltinnAuthorizationService"
# Then search for CheckAccessAsSender implementation
rg "CheckAccessAsSender" -A 10
Length of output: 23164
Script:
#!/bin/bash
# Let's check the implementation of CheckUserAccess in AltinnAuthorizationService
rg -A 20 "CheckUserAccess.*user.*resourceId.*party.*instance" src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Length of output: 153
Script:
#!/bin/bash
# Let's try a different approach to find the CheckUserAccess implementation
rg -A 20 "private.*CheckUserAccess" src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Also check the content of the authorization service file
cat src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Length of output: 11782
src/Altinn.Correspondence.Application/GetAttachmentDetails/GetAttachmentDetailsHandler.cs (2)
2-2: LGTM! Constructor changes align with authorization simplification.
The removal of UserClaimsHelper and addition of IAltinnAuthorizationService aligns well with the PR's objective of cleaning up the authorization process.
Also applies to: 13-13
23-28: Verify the impact of new authorization approach.
The switch to CheckAccessAsSender implements the PR's goal of removing instance checking. However, let's verify that:
- The
WithoutPrefix()method correctly handles all sender format cases - The authorization service properly handles delegated access scenarios
✅ Verification successful
Based on the code analysis, I can now provide a final response:
The authorization approach implementation is consistent and secure.
The verification reveals:
- The
WithoutPrefix()method is consistently implemented across the codebase to handle both organization numbers (with/without prefix) and social security numbers through proper regex validation. - The authorization service properly handles delegated access through standardized XACML request/response patterns, with support for both Altinn and Dialogporten tokens.
- The implementation is used consistently across all attachment-related operations (initialize, upload, download, purge, etc.) with proper sender verification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the consistency of sender prefix handling and authorization patterns
# Check for other WithoutPrefix() usages to ensure consistent handling
rg -A 2 "WithoutPrefix\(\)"
# Look for similar authorization patterns to ensure consistency
ast-grep --pattern 'CheckAccessAsSender($$$)'
# Check for test coverage of delegated access scenarios
rg -A 5 "TestDelegated|delegated.*access"
Length of output: 5418
Script:
#!/bin/bash
# Let's verify the WithoutPrefix implementation and test coverage
# Check the implementation of WithoutPrefix
rg -l "WithoutPrefix" | xargs cat
# Look for test files related to authorization
fd ".*[Tt]est.*\.cs$" | xargs rg -l "CheckAccessAsSender|WithoutPrefix"
# Check for any authorization-related test scenarios
rg -A 5 "CheckAccessAsSender.*\(" test/
Length of output: 71186
src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentHandler.cs (1)
31-35: LGTM! Authorization check is properly simplified.
The change aligns well with the PR objectives by:
- Removing the instance checking complexity
- Using a single, clear authorization check via
CheckAccessAsRecipient - Properly handling the nullable
ClaimsPrincipal
Let's verify the implementation of CheckAccessAsRecipient:
✅ Verification successful
Authorization check implementation is properly secured and consistent
The implementation of CheckAccessAsRecipient in AltinnAuthorizationService is well-structured and secure:
- Properly validates user authentication by checking
ClaimsPrincipal - Uses appropriate
ResourceAccessLevel.Readpermission for recipients - Handles both direct and delegated access through the underlying
CheckUserAccessmethod - Maintains consistent authorization patterns across sender and recipient checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the CheckAccessAsRecipient implementation in IAltinnAuthorizationService
# Expect: Method should handle both direct access and delegated access cases
ast-grep --pattern 'interface IAltinnAuthorizationService {
$$$
CheckAccessAsRecipient($$$) {
$$$
}
$$$
}'
# Also check the concrete implementation
ast-grep --pattern 'class $_ : IAltinnAuthorizationService {
$$$
CheckAccessAsRecipient($$$) {
$$$
}
$$$
}'
Length of output: 242
Script:
#!/bin/bash
# Let's try a different approach to find the authorization service implementation
# First, find all files that might contain the interface or implementation
rg -l "IAltinnAuthorizationService|CheckAccessAsRecipient" --type cs
# Then let's look for the method signature and its implementation
rg "CheckAccessAsRecipient" -A 5 --type cs
Length of output: 9673
Script:
#!/bin/bash
# Let's check the implementation of CheckUserAccess in AltinnAuthorizationService
rg -A 10 "CheckUserAccess.*\(" src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Also check if there are any tests verifying this functionality
rg -l "AltinnAuthorizationService.*Test" Test/
Length of output: 2835
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (2)
Line range hint 1-12: LGTM! Clean dependency injection setup
The class structure follows best practices with:
- Proper use of C# 12 primary constructor
- Clear interface-based dependencies
- Alignment with the PR's authorization cleanup objectives
35-40: Verify new authorization method implementation
The switch to CheckAccessAsAny and the use of WithoutPrefix() represent significant changes to the authorization flow.
Let's verify these new methods:
✅ Verification successful
Authorization changes are properly implemented and consistent
The verification shows that:
CheckAccessAsAnyis properly defined inIAltinnAuthorizationServiceinterface and implemented in both production (AltinnAuthorizationService) and development (AltinnAuthorizationDevService) services.WithoutPrefix()is a well-established extension method inStringExtensions.csthat consistently handles organization/SSN numbers by removing prefixes, used throughout the codebase for authorization checks.- The authorization flow follows a consistent pattern across different handlers (GetCorrespondences, InitializeCorrespondences, etc.) using these methods.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check implementation of new authorization methods
ast-grep --pattern 'CheckAccessAsAny($$$)'
# Check WithoutPrefix implementation
ast-grep --pattern 'WithoutPrefix($$$)'
# Look for any test coverage
rg -l "CheckAccessAsAny.*Test"
Length of output: 119
Script:
#!/bin/bash
# Let's try a broader search for the authorization service and method
rg -A 5 "CheckAccessAsAny"
# Search for interface or class containing this method
ast-grep --pattern 'interface $_ { $$$ CheckAccessAsAny($$$) $$$ }'
ast-grep --pattern 'class $_ { $$$ CheckAccessAsAny($$$) $$$ }'
# Search for WithoutPrefix extension method or similar
rg -A 2 "WithoutPrefix"
# Look for test files related to authorization
rg -l "Authorization.*Test"
Length of output: 13848
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
28-32: LGTM! Authorization changes align with PR objectives.
The new authorization check using CheckAccessAsRecipient is a good improvement that:
- Simplifies the authorization logic by centralizing it in
IAltinnAuthorizationService - Supports delegated access as per PR requirements
- Removes the need for instance checking while maintaining security
src/Altinn.Correspondence.Application/PurgeAttachment/PurgeAttachmentHandler.cs (3)
10-10: LGTM! Clean constructor refactoring
The removal of UserClaimsHelper dependency and the use of primary constructor syntax improves the code's maintainability while aligning with the authorization cleanup objectives.
Also applies to: 14-21
Line range hint 1-77: Overall changes look good and align with PR objectives
The modifications successfully simplify the authorization process while maintaining robust error handling and business logic integrity. The removal of UserClaimsHelper and transition to IAltinnAuthorizationService makes the code cleaner and more maintainable.
30-35: Verify consistent usage of WithoutPrefix extension method
The authorization check has been simplified, but let's ensure the WithoutPrefix() extension method is consistently used across the codebase for sender identifiers.
✅ Verification successful
WithoutPrefix extension method is consistently used across the codebase
The verification shows that WithoutPrefix() is consistently used across all authorization-related operations in the codebase, including:
- All attachment operations (initialize, purge, upload, download, get details)
- Correspondence operations (initialize, get)
- Authorization service implementations
- Token XACML mapping
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other occurrences of WithoutPrefix to verify consistent usage
rg -A 2 "WithoutPrefix\(\)"
# Search for sender patterns to ensure we're not missing any places where WithoutPrefix should be used
ast-grep --pattern 'sender.$'
Length of output: 5358
src/Altinn.Correspondence.Application/CancelNotification/CancelNotificationHandler.cs (1)
10-11: LGTM!
The added using directives are necessary for the InternalsVisibleTo attribute and ClaimsPrincipal type.
Altinn.Correspondence.sln (2)
20-21: LGTM: Project structure follows solution conventions.
The new Altinn.Correspondence.Common project is properly integrated into the solution structure, following the established SDK-style project format and naming conventions.
52-55: Verify project dependencies for proper layering.
The build configurations are correctly set up. However, we should verify that other projects reference this common library appropriately to maintain clean architecture.
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs (1)
55-55: LGTM! Verify consistent usage across codebase.
The change from GetOrgNumberWithoutPrefix() to WithoutPrefix() aligns with the PR's goal of streamlining authorization. The more generic method name better reflects its ability to handle both organization numbers and social security numbers.
Let's verify the consistent usage of this new method name across the codebase:
✅ Verification successful
Method rename is consistently applied across the codebase
The verification shows that:
- No instances of the old method
GetOrgNumberWithoutPrefix()remain in the codebase - The new
WithoutPrefix()method is consistently used across all relevant components:- Authorization services and mappers
- Correspondence handlers
- Attachment handlers
- Dialog token mappers
The change is properly implemented and maintains consistency throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of the old method name and verify consistent usage of the new one
# Check for any remaining instances of the old method
echo "Checking for any remaining instances of GetOrgNumberWithoutPrefix:"
rg "GetOrgNumberWithoutPrefix"
# Check usage pattern of the new method
echo "Verifying usage pattern of WithoutPrefix:"
rg "WithoutPrefix\(\)" -A 2
Length of output: 5593
src/Altinn.Correspondence.Integrations/Dialogporten/Mappers/DialogTokenXacmlMapper.cs (4)
5-5: LGTM: Common helper dependency addition
The addition of the Common.Helpers namespace is appropriate for centralizing string validation logic.
98-98: Verify the claim type change implications
The switch from IsOnBehalfOfClaim to IsConsumerClaim represents a significant change in claim handling. This needs careful verification to ensure it aligns with the authorization model.
141-147:
Critical: Temporary authentication level bypass needs tracking
The hotfix that bypasses authentication level validation when claimLevel is "0" introduces a potential security risk. While the comment indicates this is temporary, we need:
- A tracking issue for removing this bypass
- A timeline for when Dialogporten will start sending correct levels
- Logging when this bypass is triggered to monitor usage
63-70: Verify the party number validation logic
The switch to extension methods for party number validation is cleaner, but we need to ensure the validation logic remains robust for this security-critical operation.
✅ Verification successful
Extension methods maintain robust validation patterns
The validation logic in the extension methods is secure and well-implemented:
IsSocialSecurityNumber: Enforces strict 11-digit format (^\d{11}$)IsOrganizationNumber: Validates either 9-digit format or '4digits:9digits' format (^(?:\d{9}|\d{4}:\d{9})$)WithoutPrefix: Simply extracts the identifier after the colon, used only after validation
The patterns are defined as private readonly fields, ensuring thread-safety and immutability. The validation is consistent across the codebase, as evidenced by similar patterns used in other services (DialogportenCorrespondenceMapper, AltinnRegisterService, AltinnEventBus).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the new validation methods
# Check the implementation of the validation methods
ast-grep --pattern 'public static bool IsOrganizationNumber(this string $value) {
$$$
}'
ast-grep --pattern 'public static bool IsSocialSecurityNumber(this string $value) {
$$$
}'
ast-grep --pattern 'public static string WithoutPrefix(this string $value) {
$$$
}'
Length of output: 275
Script:
#!/bin/bash
# Let's try a broader search to find these extension methods
# First, let's find files that might contain these methods
rg -l "IsOrganizationNumber|IsSocialSecurityNumber|WithoutPrefix" --type cs
# Then let's look at the actual implementation with context
rg "IsOrganizationNumber|IsSocialSecurityNumber|WithoutPrefix" --type cs -B 2 -A 5
Length of output: 26602
Script:
#!/bin/bash
# Let's check the validation patterns defined in the StringExtensions class
rg "private static readonly Regex" --type cs src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs -B 2 -A 2
Length of output: 325
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (3)
1-4: LGTM: Clean dependency management
The removal of UserClaimsHelper and addition of Common.Helpers aligns well with the PR's objective of streamlining the authorization process.
Line range hint 401-408: Consider adding validation for condition endpoint URL
The CreateConditionEndpoint method should validate the URL format and ensure it's HTTPS in production environments.
[security]
private Uri? CreateConditionEndpoint(string correspondenceId)
{
var conditionEndpoint = new Uri($"{_generalSettings.CorrespondenceBaseUrl.TrimEnd('/')}/correspondence/api/v1/correspondence/{correspondenceId}/notification/check");
- if (conditionEndpoint.Host == "localhost")
+ if (conditionEndpoint.Host == "localhost" ||
+ (hostEnvironment.IsProduction() && !conditionEndpoint.Scheme.Equals("https", StringComparison.OrdinalIgnoreCase)))
{
return null;
}
return conditionEndpoint;
}41-46:
Verify null user handling in authorization check
While the new authorization approach using CheckAccessAsSender aligns with the PR objectives, we should ensure proper handling of null user scenarios.
Consider adding a null check before the authorization check:
+ if (user == null)
+ {
+ return Errors.Unauthorized;
+ }
var hasAccess = await altinnAuthorizationService.CheckAccessAsSender(
user,
request.Correspondence.ResourceId,
request.Correspondence.Sender.WithoutPrefix(),
null,
cancellationToken);src/Altinn.Correspondence.Core/Repositories/IAltinnAuthorizationService.cs (2)
1-2: Imports are appropriate
The added using directives correctly reference the necessary models and enums used in the interface methods.
9-12: New methods enhance authorization granularity
The addition of CheckAccessAsSender, CheckAccessAsRecipient, and CheckAccessAsAny methods provides more specific access control mechanisms, improving the flexibility of the authorization service.
src/Altinn.Correspondence.Application/Helpers/UserClaimsHelper.cs (1)
16-21: Constructor refactoring simplifies dependency injection
The constructor now requires only IHttpContextAccessor, removing dependencies on DialogportenSettings and IdportenSettings. This change reduces coupling and enhances maintainability.
src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs (2)
2-2: Approved: Added necessary using directive
The addition of using Altinn.Correspondence.Common.Helpers; is appropriate for the dependencies used in the updated code.
25-30: Approved: Streamlined access check with CheckAccessAsSender
The use of CheckAccessAsSender with attachment.Sender.WithoutPrefix() simplifies the access control logic and aligns with the refactored authorization approach.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
66-66: Verify status assignment when user has both sender and recipient access
Currently, the status is set based on hasAccessAsSender. If a user has both sender and recipient access, the status will be PurgedByAltinn. Verify if this aligns with the intended business logic or if an explicit check is needed to handle such scenarios.
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (3)
24-26: Access check as recipient implemented correctly
The implementation of CheckAccessAsRecipient is clear and follows best practices.
28-31: Access check as sender implemented correctly
The use of CheckAccessAsSender appropriately verifies sender access.
32-34: Proper handling of unauthorized access
The condition correctly returns an error when the user lacks both recipient and sender access.
src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs (2)
Line range hint 46-59: Verify Access Logic and Status Update
The status update to Fetched is performed only when the user has access as a recipient and not as a sender (hasAccessAsRecipient && !hasAccessAsSender). If a user has access as both recipient and sender, the status will not be updated. Please verify that this behavior aligns with the intended access control and auditing requirements.
27-29:
Add Null Check for 'user' Parameter
The user parameter is nullable (ClaimsPrincipal? user). To prevent a possible NullReferenceException, consider adding a null check before using it in CheckAccessAsRecipient and CheckAccessAsSender.
Apply this diff to add the null check:
+ if (user == null)
+ {
+ return Errors.NoAccessToResource;
+ }
var hasAccessAsRecipient = await altinnAuthorizationService.CheckAccessAsRecipient(
user,
correspondence,
cancellationToken);
var hasAccessAsSender = await altinnAuthorizationService.CheckAccessAsSender(
user,
correspondence,
cancellationToken);Also applies to: 31-34
⛔ Skipped due to learnings
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#511
File: src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs:48-49
Timestamp: 2024-11-21T09:16:54.651Z
Learning: In `src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs`, the properties `correspondence.Recipient` and `correspondence.Sender` are required to be non-null.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#480
File: src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs:61-63
Timestamp: 2024-11-12T12:30:11.505Z
Learning: In the `PurgeCorrespondenceHandler` class, the `IsAffiliatedWithCorrespondence` method verifies if the user is either the sender or the recipient, so additional checks for user affiliation are redundant.
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs
Outdated
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
Show resolved
Hide resolved
…ed it anywhere aside from GetCorrespondences
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs (1)
Line range hint
45-55: Consider improving status record creation.While the access control logic is sound, the status record creation could be improved to avoid string duplication and potential maintenance issues.
Consider this improvement:
await correspondenceStatusRepository.AddCorrespondenceStatus(new CorrespondenceStatusEntity { CorrespondenceId = correspondence.Id, Status = CorrespondenceStatus.Fetched, - StatusText = CorrespondenceStatus.Fetched.ToString(), + StatusText = nameof(CorrespondenceStatus.Fetched), StatusChanged = DateTimeOffset.UtcNow }, cancellationToken);src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs (1)
Line range hint
1-394: Consider updating API documentationThe removal of instance checking and
onBehalfOfparameters represents a significant change in how authorization works. Consider:
- Updating API documentation to reflect these changes
- Adding examples of how delegated access now works with PDP
- Documenting the authorization policies and their scopes
Would you like assistance in generating the documentation updates?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Altinn.Correspondence.API/Controllers/AttachmentController.cs(1 hunks)src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs(5 hunks)src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentRequest.cs(0 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsRequest.cs(0 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewRequest.cs(0 hunks)src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceRequest.cs(0 hunks)src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceRequest.cs(0 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusRequest.cs(0 hunks)
💤 Files with no reviewable changes (6)
- src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/DownloadCorrespondenceAttachmentRequest.cs
- src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsRequest.cs
- src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewRequest.cs
- src/Altinn.Correspondence.Application/MigrateCorrespondence/MigrateCorrespondenceRequest.cs
- src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceRequest.cs
- src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusRequest.cs
✅ Files skipped from review due to trivial changes (1)
- src/Altinn.Correspondence.API/Controllers/AttachmentController.cs
🔇 Additional comments (8)
src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs (3)
26-34: LGTM! Clean authorization implementation.
The new authorization checks using altinnAuthorizationService are well-structured and align with the PR's objective of cleaning up the authorization process. The separation between recipient and sender access checks improves code clarity and maintainability.
65-67:
Add null check for notificationSummary.
The potential for NullReferenceException still exists when accessing notificationSummary.IsReminder without checking if GetNotificationDetails returns null.
As mentioned in the previous review, consider adding a null check:
var notificationSummary = await altinnNotificationService.GetNotificationDetails(notification.NotificationOrderId.ToString(), cancellationToken);
+ if (notificationSummary == null)
+ {
+ logger.LogWarning("Notification details not found for order ID: {OrderId}", notification.NotificationOrderId);
+ continue;
+ }
notificationSummary.IsReminder = notification.IsReminder;
notificationHistory.Add(notificationSummary);26-34: Verify complete removal of instance checks.
Since this PR aims to remove instance checking of correspondences, let's verify that all such checks have been removed across the codebase.
✅ Verification successful
Let me gather more context about the instance checks by looking at their implementation.
Let me check the actual implementation of authorization services to understand if these instance parameters are still being used meaningfully.
Instance parameter is still used but only as correlation ID
The instance checks have been properly removed from the authorization flow. The remaining instance parameter in CheckAccessAsSender is only used as a correlation ID (correspondence ID) in the XACML request and doesn't perform any instance-based checks. The authorization is now properly based on resource and party access rights.
- The
instanceparameter inIAltinnAuthorizationService.CheckAccessAsSenderis passed through to create XACML decision requests - Other instance-related code found in the codebase is for different purposes (e.g., InstanceOwnerPartyId for identifying parties, AltinnAppInstance as an enum value)
- The dev service implementation confirms there's no instance-based logic as it simply returns true
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instance checks in the codebase
rg -i "instance.*check|check.*instance" --type cs
# Search for any remaining UserClaimsHelper usage
rg -i "UserClaimsHelper" --type cs
Length of output: 3258
Script:
#!/bin/bash
# Look at the implementation of CheckAccessAsSender in both services
ast-grep --pattern 'public Task<bool> CheckAccessAsSender($$$) {
$$$
}'
# Check for any instance-related parameters in method calls
rg -i "instance.*=|instance:" --type cs
Length of output: 2938
Script:
#!/bin/bash
# Check the implementation of authorization services
cat src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
cat src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs
# Also check if there are any instance-based checks in the core authorization logic
ast-grep --pattern 'if ($$$instance$$$) {
$$$
}'
Length of output: 12344
src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs (5)
246-246: LGTM! Authorization scope is properly maintained
The simplified request maintains the correct authorization scope with the Recipient policy and JWT authentication scheme, while removing unnecessary instance checking.
276-276: LGTM! Authentication and CORS configuration is correct
The simplified request maintains proper authentication schemes and CORS configuration while removing unnecessary instance checking.
362-362: LGTM! Download authorization is properly configured
The simplified request maintains appropriate authorization policy and authentication schemes for attachment downloads while removing unnecessary instance checking.
325-325: Verify CORS impact on purge operation
The addition of CORS for the purge operation and removal of instance checking changes how this endpoint can be accessed. Since this is a destructive operation with broader authorization scope (SenderOrRecipient), we should verify:
- CORS configuration is intentional and secure
- The broader authorization scope is appropriate without instance checking
Let's check the CORS configuration:
Also applies to: 335-335
✅ Verification successful
CORS configuration is consistent and secure for the purge operation
The CORS configuration for the purge operation aligns with the application's overall security model:
- The
ArbeidsflateCorspolicy is consistently applied across all user-facing operations in the CorrespondenceController - The purge operation maintains proper authorization through
AuthorizationConstants.SenderOrRecipientpolicy - The authorization configuration shows that
SenderOrRecipientpolicy requires either sender or recipient scope and supports both AltinnToken and Dialogporten authentication schemes - The broader authorization scope is appropriate as it allows both senders and recipients to purge their own correspondence, which is validated through the authentication process
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check CORS configuration consistency
rg -B 2 -A 2 "ArbeidsflateCors" --type cs
# Check authorization policy configuration
ast-grep --pattern 'AuthorizationConstants.$_'
Length of output: 19672
113-113: LGTM! Verify authorization still works for delegated access
The removal of onBehalfOf parameter aligns with the PR objective of utilizing recipient and sender information in PDP requests. This change simplifies the authorization process while maintaining security.
Let's verify that delegated access still works:
✅ Verification successful
Delegated access is properly handled through policy-based authorization
The removal of onBehalfOf parameter is safe as the authorization is now properly handled through:
- The
SenderOrRecipientpolicy that supports both AltinnToken and Dialogporten authentication schemes - The XACML-based authorization service that still maintains support for delegated access through the token mapper
- Proper scope validation for both sender and recipient roles
The evidence shows:
- Authorization policies are properly configured in DependencyInjection.cs to handle both direct and delegated access
- The AltinnTokenXacmlMapper still supports delegated access through the CreateResourceCategory method
- The controller is properly secured with the SenderOrRecipient policy that validates required scopes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining onBehalfOf references in authorization-related files
rg -l "onBehalfOf" --type cs
# Look for PDP-related authorization changes
ast-grep --pattern 'class $_ implements IAltinnAuthorizationService {
$$$
}'
Length of output: 441
Script:
#!/bin/bash
# Check the implementation details of onBehalfOf usage in authorization service
rg "onBehalfOf" -B 2 -A 2 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Check how authorization is handled in the controller
ast-grep --pattern 'public class CorrespondenceController {
$$$
[Authorize($$$)]
$$$
GetCorrespondenceOverview($$$) {
$$$
}
}'
# Check if there are any PDP policy changes
fd "policy" --type f --exec rg -l "SenderOrRecipient"
Length of output: 1509
Script:
#!/bin/bash
# Check how authorization is implemented in the mapper
rg "CreateAltinnDecisionRequestForLegacy" -A 10 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs
# Check the policy requirements in the controller
rg "SenderOrRecipient" -B 2 -A 2 src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs
# Look for any authorization policy definitions
fd "Authorization" --type f --exec rg -l "SenderOrRecipient"
Length of output: 2293
Script:
#!/bin/bash
# Check the authorization constants and policy definition
rg "SenderOrRecipient" -B 2 -A 2 src/Altinn.Correspondence.Application/Configuration/AuthorizationConstants.cs
# Check how the authorization is handled in the token mapper
rg "CreateResourceCategory" -A 10 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnTokenXacmlMapper..cs
# Look for policy setup in startup or configuration
fd -e cs -x rg -l "AddAuthorization|services\.AddAuthorization"
Length of output: 2050
Script:
#!/bin/bash
# Check how the authorization policies are configured
rg "AddAuthorization" -A 10 src/Altinn.Correspondence.API/Auth/DependencyInjection.cs
# Check if there are any authorization handlers
fd "Handler" --type f --exec rg -l "SenderOrRecipient"
# Look for authorization requirements
fd -e cs -x rg -l "AuthorizationHandler|IAuthorizationRequirement"
Length of output: 1417
…one may have access both as a sender and recipient, but calling the API as a recipient, hence fetched should still be set.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (11)
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/Base/CorrespondenceTestBase.cs (1)
Line range hint
13-19: Consider adding authorization edge case testsSince this PR removes instance checking in favor of PDP-based authorization using recipient and sender information, consider extending this test base class to cover additional authorization scenarios:
- Delegated access cases
- Invalid/missing scope scenarios
- Mixed sender/recipient permissions
This will help ensure the robustness of the new authorization model.
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs (1)
11-11: Remove unused variableThe
claimsvariable is declared but never used.- var claims = user.Claims;src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (1)
46-46: Consider improving readability of the access control condition.While the logic is correct, the negative condition could be made more intuitive.
Consider this alternative:
- if (!hasAccessAsSender && !user.CallingAsSender()) + bool isRecipientOnlyAccess = !hasAccessAsSender && !user.CallingAsSender(); + if (isRecipientOnlyAccess)This makes it clearer that this block handles recipient-specific logic.
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs (3)
Line range hint
39-63: Enhance test coverage for dialog creationWhile the test verifies that the background job is created, it should also verify that the dialog creation was attempted with the correct correspondence ID.
Consider adding this verification:
// Assert initializeCorrespondenceResponse.EnsureSuccessStatusCode(); Assert.True(hangfireBackgroundJobClient.Invocations.Any(invocation => invocation.Arguments[0].ToString() == "InitializeCorrespondencesHandler.CreateDialogportenDialog")); +mockDialogportenService.Verify(x => x.CreateCorrespondenceDialog(correspondence.Id), Times.Once);
Line range hint
65-79: Improve test clarity for authorization failureThe test name and assertions could be more specific about the authorization requirements and failure reason.
Consider renaming the test and adding more specific assertions:
-public async Task InitializeCorrespondence_WithDialogToken_Fails() +public async Task InitializeCorrespondence_WithDialogTokenButNoSenderScope_ReturnsForbidden() { // Arrange var dialogTokenClient = _factory.CreateClientWithDialogportenClaims(null); var correspondence = new CorrespondenceBuilder().CreateCorrespondence().Build(); // Act var initializeCorrespondenceResponse = await dialogTokenClient.PostAsJsonAsync("correspondence/api/v1/correspondence", correspondence); // Assert Assert.Equal(HttpStatusCode.Forbidden, initializeCorrespondenceResponse.StatusCode); + var error = await initializeCorrespondenceResponse.Content.ReadFromJsonAsync<ProblemDetails>(); + Assert.Contains("sender scope", error.Detail.ToLower()); }
Line range hint
81-108: Consider extracting test setup helper methodsThe test setup is quite complex and could benefit from helper methods to improve readability and maintainability.
Consider extracting the setup logic:
+private async Task<(Guid correspondenceId, HttpClient dialogTokenClient)> SetupCorrespondenceWithDialogToken(string recipientUrn) +{ + var senderClient = _factory.CreateClientWithAddedClaims(("scope", AuthorizationConstants.SenderScope)); + var correspondence = new CorrespondenceBuilder().CreateCorrespondence().Build(); + var response = await senderClient.PostAsJsonAsync("correspondence/api/v1/correspondence", correspondence); + var initialized = await response.Content.ReadFromJsonAsync<InitializeCorrespondencesResponseExt>(_responseSerializerOptions); + + using var scope = _factory.Services.CreateScope(); + var repo = scope.ServiceProvider.GetRequiredService<ICorrespondenceRepository>(); + var saved = await repo.GetCorrespondenceById(initialized.Correspondences[0].CorrespondenceId, false, false, CancellationToken.None); + + var settings = _factory.Services.GetService<IConfiguration>() + .GetSection(nameof(DialogportenSettings)) + .Get<DialogportenSettings>(); + + return ( + initialized.Correspondences[0].CorrespondenceId, + _factory.CreateClientWithDialogportenClaims(settings.Issuer, ("p", DialogportenCorrespondenceMapper.GetRecipientUrn(saved))) + ); +} [Fact] public async Task GetCorrespondenceContent_WithDialogTokenFromRecipient_Succeeds() { // Arrange - var senderClient = _factory.CreateClientWithAddedClaims(("scope", AuthorizationConstants.SenderScope)); - // ... existing setup code ... + var (correspondenceId, dialogTokenClient) = await SetupCorrespondenceWithDialogToken("recipient-urn"); // Act - var contentResponse = await dialogTokenClient.GetAsync("correspondence/api/v1/correspondence/" + initializedCorrespondence.Correspondences[0].CorrespondenceId + "/content"); + var contentResponse = await dialogTokenClient.GetAsync($"correspondence/api/v1/correspondence/{correspondenceId}/content"); // Assert Assert.Equal(HttpStatusCode.OK, contentResponse.StatusCode); Assert.Equal("application/vnd.dialogporten.frontchannelembed+json; type=markdown; charset=utf-8", contentResponse.Content.Headers.ContentType?.ToString()); }src/Altinn.Correspondence.API/Auth/CascadeAuthenticationHandler.cs (4)
Line range hint
41-67: Consider adding timeout protection for authentication attempts.While the cascading authentication logic is robust, consider implementing a timeout mechanism to prevent hanging in case of slow authentication providers. This could be achieved using CancellationToken or Task.WhenAny with a timeout task.
Example implementation:
protected override async Task<AuthenticateResult> HandleAuthenticateAsync() { + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); // Configurable timeout var schemesToTry = new[] { JwtBearerDefaults.AuthenticationScheme, AuthorizationConstants.DialogportenScheme, OpenIdConnectDefaults.AuthenticationScheme }; foreach (var schemeName in schemesToTry) { try { + cts.Token.ThrowIfCancellationRequested(); Logger.LogInformation($"Attempting authentication with scheme: {schemeName}"); // ... rest of the code } + catch (OperationCanceledException) + { + Logger.LogWarning($"Authentication attempt timed out for scheme: {schemeName}"); + return AuthenticateResult.Fail("Authentication timed out"); + } }
Line range hint
69-85: Enhance token validation with expiration check.Consider adding a token expiration check before validation to prevent unnecessary validation attempts of expired tokens.
private async Task<AuthenticateResult> HandleOpenIdConnectAsync() { if (!Request.Query.TryGetValue("session", out var sessionId)) { return AuthenticateResult.NoResult(); } var token = await _cache.GetStringAsync(sessionId); if (string.IsNullOrEmpty(token)) { return AuthenticateResult.NoResult(); } + // Get token metadata including expiration + var metadata = await _cache.GetAsync($"{sessionId}_metadata"); + if (metadata == null || IsTokenExpired(metadata)) + { + await _cache.RemoveAsync(sessionId); + return AuthenticateResult.Fail("Token expired"); + } _cache.Remove(sessionId);
Line range hint
146-153: Security: Implement URL validation and enhance session management.The current implementation has several security concerns:
- Session IDs in URLs could be leaked through browser history and logs
- No validation of redirect URLs could lead to open redirect vulnerabilities
- Potential for session fixation attacks
Consider these improvements:
public static string AppendSessionToUrl(string url, string sessionId) { + // Validate URL against allowed hosts + if (!IsAllowedRedirectUrl(url)) + { + throw new SecurityException("Invalid redirect URL"); + } var uriBuilder = new UriBuilder(url); var query = HttpUtility.ParseQueryString(uriBuilder.Query); - query["session"] = sessionId; + // Use signed session token instead of raw session ID + query["session"] = GenerateSignedSessionToken(sessionId); uriBuilder.Query = query.ToString(); return uriBuilder.ToString(); } +private static bool IsAllowedRedirectUrl(string url) +{ + var allowedHosts = new[] { "correspondence.altinn.no" }; // Configure via settings + var uri = new Uri(url); + return allowedHosts.Contains(uri.Host); +}
Token cache duration inconsistency detected
There appears to be an inconsistency in token cache durations across the codebase:
- In
CascadeAuthenticationHandler.cs, tokens are cached with a 10-second expiration- In
DependencyInjection.cs, tokens are cached with a 5-minute expirationThis inconsistency could lead to unexpected behavior. The 10-second duration in the authentication handler is indeed too aggressive compared to the 5-minute duration used elsewhere in the codebase. Consider:
- Aligning the cache durations to use the same 5-minute timespan
- Moving the duration to a configuration setting for easier management
- Documenting the reason if different durations are intentional
🔗 Analysis chain
Line range hint
87-99: Consider adjusting the token cache duration.The 10-second token expiration might be too aggressive and could lead to frequent re-authentication. Consider making this configurable through settings and potentially increasing it based on your security requirements.
Let's check if there are any configuration settings for token expiration:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for token expiration configuration rg -l "token.*expiration|cache.*duration" --type jsonLength of output: 55
Script:
#!/bin/bash # Let's search for token cache implementation and configuration rg -A 5 "TokenCache|_cache" --type csLength of output: 3828
Script:
#!/bin/bash # Search for any token expiration or duration settings in appsettings fd "appsettings" -e json -x cat {}Length of output: 1531
Script:
#!/bin/bash # Look for cache-related configuration in startup or configuration classes rg -A 5 "AddMemoryCache|ConfigureServices.*Cache|CacheOptions" --type csLength of output: 74
Script:
#!/bin/bash # Let's check how the distributed cache is configured in Program.cs or Startup.cs rg -A 10 "AddDistributedCache|IDistributedCache" --type csLength of output: 3764
Script:
#!/bin/bash # Look for any cache duration or token expiration settings in configuration classes ast-grep --pattern 'class $_ { $$$ TimeSpan $$$ }'Length of output: 52
Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyDeletionTests.cs (1)
Line range hint
1-120: Comprehensive test coverage for legacy deletion scenarios.The test suite thoroughly covers critical deletion scenarios including:
- Basic deletion of published correspondence
- Authorization edge cases (invalid party)
- Error cases (non-existent correspondence, ready for publish)
- Confirmation workflow (with and without required confirmation)
The test methods are appropriately granular given the significant distinctions between scenarios, which aligns with the learned testing practices.
Consider adding these test scenarios if not covered elsewhere:
- Deletion with delegated authority (aligns with PR objective of supporting delegated parties)
- Concurrent deletion attempts
- Rate limiting behavior (if applicable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (25)
Test/Altinn.Correspondence.Tests/Helpers/MockPolicyEvaluator.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/Base/AttachmentTestBase.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/Base/CorrespondenceTestBase.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceRetrievalTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceSearchTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Legacy/Base/LegacyTestBase.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyDeletionTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyRetrievalTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacySearchTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingFeature/MalwareScanResultControllerTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingFeature/NotificationTests.cs(1 hunks)src/Altinn.Correspondence.API/Auth/CascadeAuthenticationHandler.cs(1 hunks)src/Altinn.Correspondence.API/Auth/DependencyInjection.cs(1 hunks)src/Altinn.Correspondence.API/Controllers/AttachmentController.cs(2 hunks)src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs(6 hunks)src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs(1 hunks)src/Altinn.Correspondence.API/Controllers/MigrationController.cs(1 hunks)src/Altinn.Correspondence.API/Program.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs(2 hunks)src/Altinn.Correspondence.Common/Altinn.Correspondence.Common.csproj(1 hunks)src/Altinn.Correspondence.Common/Constants/AuthorizationConstants.cs(1 hunks)src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- Test/Altinn.Correspondence.Tests/Helpers/MockPolicyEvaluator.cs
- Test/Altinn.Correspondence.Tests/TestingController/Attachment/Base/AttachmentTestBase.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceNotificationTests.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceSearchTests.cs
- Test/Altinn.Correspondence.Tests/TestingController/Legacy/Base/LegacyTestBase.cs
- Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyRetrievalTests.cs
- Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacySearchTests.cs
- Test/Altinn.Correspondence.Tests/TestingFeature/NotificationTests.cs
- src/Altinn.Correspondence.API/Controllers/LegacyCorrespondenceController.cs
- src/Altinn.Correspondence.API/Controllers/MigrationController.cs
- src/Altinn.Correspondence.API/Program.cs
- src/Altinn.Correspondence.Common/Constants/AuthorizationConstants.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Altinn.Correspondence.API/Controllers/AttachmentController.cs
- src/Altinn.Correspondence.API/Controllers/CorrespondenceController.cs
🧰 Additional context used
📓 Learnings (5)
Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyDeletionTests.cs (3)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#459
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceResponse.cs:18-23
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the application folder, classes should not have the `Ext` suffix; the `Ext` suffix is reserved for models in the API layer.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#513
File: Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs:78-101
Timestamp: 2024-11-20T12:39:19.050Z
Learning: In the Altinn.Correspondence.Tests project, splitting test methods is not required unless the distinctions are significant.
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs (3)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#459
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceResponse.cs:18-23
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the application folder, classes should not have the `Ext` suffix; the `Ext` suffix is reserved for models in the API layer.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#381
File: src/Altinn.Correspondence.API/Program.cs:68-68
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence project, configuration settings like `GeneralSettings` may be defined in Bicep templates rather than in `appsettings.json` files.
Test/Altinn.Correspondence.Tests/TestingFeature/MalwareScanResultControllerTests.cs (2)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#459
File: src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondenceResponse.cs:18-23
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the application folder, classes should not have the `Ext` suffix; the `Ext` suffix is reserved for models in the API layer.
src/Altinn.Correspondence.API/Auth/CascadeAuthenticationHandler.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
src/Altinn.Correspondence.API/Auth/DependencyInjection.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
🔇 Additional comments (24)
src/Altinn.Correspondence.Common/Altinn.Correspondence.Common.csproj (2)
Line range hint 1-8: LGTM! Project configuration is well-structured.
The project is properly configured with .NET 8.0 targeting and appropriate nullable reference type settings, which aligns well with modern C# best practices.
9-11: Verify JWT Bearer package version for security considerations.
While the package version (8.0.6) aligns with the target framework, let's verify it's the latest secure version.
✅ Verification successful
Package version 8.0.6 is secure and up-to-date for .NET 8
The current version 8.0.6 is the latest stable release for .NET 8.x series. While there are 9.0.0 preview/RC versions available, they are for the upcoming .NET 9 and should not be considered for production use. No security vulnerabilities are reported for the 8.x series of the package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for latest version and security advisories for Microsoft.AspNetCore.Authentication.JwtBearer
# Check NuGet for latest versions
curl -s https://api.nuget.org/v3-flatcontainer/microsoft.aspnetcore.authentication.jwtbearer/index.json | jq -r '.versions[]' | sort -V | tail -n 5
# Check for security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Microsoft.AspNetCore.Authentication.JwtBearer") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 1322
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/Base/CorrespondenceTestBase.cs (3)
Line range hint 25-30: LGTM: JSON serializer configuration
The JSON serializer configuration is appropriate for testing purposes, with case-insensitive property matching and proper enum handling.
1-1: Verify the impact of the Common library migration
The change from Application.Configuration to Common.Constants aligns with the PR's goal of cleaning up authorization. However, we should ensure this reorganization is consistently applied across all test files.
✅ Verification successful
Migration to Common.Constants namespace is consistently applied
The verification shows that:
- No references to the old namespace
Altinn.Correspondence.Application.Configurationexist in the codebase - The new namespace
Altinn.Correspondence.Common.Constantsis consistently used across all relevant test files, including base classes, feature tests, and legacy tests
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old namespace
# and verify consistent usage of the new namespace
# Check for any remaining references to the old configuration namespace
echo "Checking for old namespace usage..."
rg "Altinn\.Correspondence\.Application\.Configuration" -l
# Verify consistent usage of the new constants namespace in test files
echo "Checking new namespace usage in test files..."
rg "using Altinn\.Correspondence\.Common\.Constants;" "Test/" -l
Length of output: 1396
Line range hint 22-23: Verify authorization scope constants usage
The initialization of test clients with sender and recipient scopes from AuthorizationConstants is good. This change supports the PR's goal of improving authorization by using dedicated scopes instead of instance checking.
✅ Verification successful
Authorization scope constants are properly defined and consistently used
The verification confirms that the authorization scopes are well-defined in AuthorizationConstants.cs and consistently used across test files:
- Scopes are properly defined in
src/Altinn.Correspondence.Common/Constants/AuthorizationConstants.cs:SenderScope = "altinn:correspondence.write"RecipientScope = "altinn:correspondence.read"
- These scopes are correctly used in authorization policies in
DependencyInjection.cs - Test files consistently use these constants when creating test clients with appropriate scopes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the authorization constants are properly defined
# and consistently used across the codebase
# Check the definition of authorization scopes
echo "Checking authorization constants definition..."
rg "SenderScope|RecipientScope" -A 2 -B 2
# Verify usage of these scopes in other test files
echo "Checking scope usage in tests..."
ast-grep --pattern 'CreateClientWithAddedClaims($$$AuthorizationConstants.$$$)'
Length of output: 19619
src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs (4)
7-8: Add comprehensive XML documentation
This class handles security-critical authorization logic and should be well-documented.
+ /// <summary>
+ /// Extension methods for ClaimsPrincipal to handle authorization-related operations.
+ /// </summary>
public static class ClaimsPrincipalExtensions13-18:
Add error handling for system user token deserialization
The JSON deserialization could throw exceptions if the token is malformed.
- var systemUserClaim = user.Claims.FirstOrDefault(c => c.Type == "authorization_details");
- if (systemUserClaim is not null)
+ var systemUserClaim = user.Claims.FirstOrDefault(c => c.Type == "authorization_details")?.Value;
+ if (systemUserClaim is not null)
{
+ try
+ {
- var systemUserAuthorizationDetails = JsonSerializer.Deserialize<SystemUserAuthorizationDetails>(systemUserClaim.Value);
+ var systemUserAuthorizationDetails = JsonSerializer.Deserialize<SystemUserAuthorizationDetails>(systemUserClaim);
return systemUserAuthorizationDetails?.SystemUserOrg.ID;
+ }
+ catch (JsonException)
+ {
+ return null;
+ }
}41-45: Verify the scope string constant
Please verify that "altinn:correspondence.write" is the correct scope string used consistently across the system.
9-39: Verify token type priority order
The method processes tokens in this order: System user → Enterprise → Personal → Dialog. Please verify this aligns with security requirements.
✅ Verification successful
Based on the code analysis performed, I can now generate the final response:
Token priority order is correctly implemented for security purposes
The implemented order (System user → Enterprise → Personal → Dialog) follows a principle of most privileged to least privileged token types:
- System user tokens (authorization_details) are highest priority as they represent system-level access
- Enterprise tokens (urn:altinn:orgNumber) are next as they represent organizational access
- Personal tokens (consumer) follow as they represent individual access
- Dialog tokens (ID) are lowest priority as they are session-specific
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other token priority implementations
rg -l "authorization_details.*urn:altinn:orgNumber.*consumer.*ID" --type cs
# Search for security documentation
fd -e md -e txt -e yaml "security|auth" -x cat {} \; | grep -i "token.*priority"
Length of output: 163
Script:
#!/bin/bash
# Search for similar token claim processing patterns
rg -A 5 "authorization_details|urn:altinn:orgNumber|consumer|ID" --type cs
# Search for any security or authentication documentation
fd "README|SECURITY|AUTH" -e md -e txt -x cat {}
# Look for other implementations of GetCallerOrganizationId
ast-grep --pattern 'GetCallerOrganizationId'
# Search for token-related tests that might reveal priority requirements
rg -A 5 "TestGetCallerOrganizationId|token.*test" --type cs
Length of output: 58599
Script:
#!/bin/bash
# Search for test files that might validate token priority
rg -A 10 "GetCallerOrganizationId.*test" --type cs
# Look for token validation or authentication configuration
rg -A 5 "token.*validation|authentication.*config" --type cs
# Search for any security or authorization documentation in markdown files
fd -e md -x grep -l -i "security|authorization|token" {} \; -x cat {}
# Look for token-related constants or configurations
rg "authorization_details|urn:altinn:orgNumber|consumer|ID" --type cs -g "*Constants.cs" -g "*Settings.cs" -g "*Config.cs"
Length of output: 1082
Test/Altinn.Correspondence.Tests/TestingFeature/MalwareScanResultControllerTests.cs (2)
3-3: LGTM! Using directive change aligns with centralization of constants.
The change from Application.Configuration to Common.Constants is part of the broader refactoring effort to centralize constant definitions in the new Altinn.Correspondence.Common project.
Line range hint 1-100: Verify model naming conventions.
Based on the project's conventions, the Ext suffix is reserved for models in the API layer. The test is using AttachmentOverviewExt and AttachmentStatusExt models - please verify that these models are indeed defined in the API layer and not in another layer.
✅ Verification successful
Let me generate one more verification script to check for the AttachmentStatusExt enum specifically, as it wasn't clearly visible in the previous results.
Model naming conventions are correctly followed
The verification confirms that both AttachmentOverviewExt and AttachmentStatusExt are properly defined in the API layer:
AttachmentOverviewExtis defined insrc/Altinn.Correspondence.API/Models/AttachmentOverviewExt.csAttachmentStatusExtis defined insrc/Altinn.Correspondence.API/Models/Enums/AttachmentStatus.cs
Additionally, all other Ext-suffixed models are consistently placed in the API layer, following the project's conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the location of Ext-suffixed models
# Expected: Models should be in the API layer
# Search for Ext-suffixed model definitions
echo "Searching for AttachmentOverviewExt and AttachmentStatusExt definitions..."
rg -l "class.*(?:AttachmentOverviewExt|AttachmentStatusExt)"
# Specifically check if they are in the API layer
echo "Verifying if models are in the API layer..."
fd -t f "^.*Ext\.cs$" "src/Altinn.Correspondence.API"
Length of output: 3137
Script:
#!/bin/bash
# Search specifically for AttachmentStatusExt enum definition
echo "Searching for AttachmentStatusExt enum definition..."
rg -A 2 "enum\s+AttachmentStatusExt"
Length of output: 406
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (1)
25-33: LGTM! Clean separation of authorization checks.
The new authorization approach using dedicated methods for recipient and sender checks improves code clarity and aligns well with the PR objective of cleaning up instance authorization.
Let's verify the implementation of these authorization methods:
✅ Verification successful
Authorization checks implementation verified and approved
The implementation shows a clean and secure authorization approach:
- Recipient checks require READ access level
- Sender checks require WRITE access level
- Core
CheckUserAccessmethod properly validates user claims and makes authorization requests - Includes bypass conditions evaluation for special cases
- Uses XACML-based authorization for fine-grained access control
The sequential checks in the handler are appropriate as they:
- Use different access levels (READ vs WRITE) for different roles
- Delegate to a centralized authorization service
- Handle null user cases properly
- Include proper cancellation token support
The implementation aligns well with the PR's goal of cleaning up instance authorization while maintaining security.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of CheckAccessAsRecipient and CheckAccessAsSender
# to ensure they're handling authorization correctly without instance checking
ast-grep --pattern 'CheckAccessAsRecipient($_,$_,$_) {
$$$
}'
ast-grep --pattern 'CheckAccessAsSender($_,$_,$_) {
$$$
}'
Length of output: 128
Script:
#!/bin/bash
# Let's try a different approach to find these authorization methods
# First, let's find all files that might contain these methods
fd . -e cs | xargs rg -l "CheckAccessAs(Recipient|Sender)"
# Then let's look for the interface definition
rg "interface.*AltinnAuthorizationService" -A 5
# And finally, let's look for the implementation
rg "class.*AltinnAuthorizationService.*:.*IAltinnAuthorizationService" -A 20
Length of output: 5768
Script:
#!/bin/bash
# Let's examine the implementation of these authorization methods
rg "CheckAccessAsRecipient.*CorrespondenceEntity" -A 15 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
rg "CheckAccessAsSender.*CorrespondenceEntity" -A 15 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Also check the dev service implementation for comparison
rg "CheckAccessAs(Recipient|Sender)" -A 10 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationDevService.cs
Length of output: 3139
Script:
#!/bin/bash
# Let's examine the CheckUserAccess implementation to understand the core authorization logic
rg "CheckUserAccess.*\(" -A 20 src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
# Also check if there are any tests validating this authorization flow
fd . -e cs | xargs rg -l "AltinnAuthorizationService.*Test"
Length of output: 3967
Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs (1)
2-2: LGTM! Constants moved to common library
The migration of constants to Altinn.Correspondence.Common.Constants aligns with the PR's cleanup objectives and improves maintainability through centralization.
src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs (3)
27-35: LGTM! Clean authorization implementation.
The new authorization checks are well-structured, with clear separation between recipient and sender access verification. This aligns well with the PR's objective of cleaning up the authorization process.
66-68:
Handle potential null value from GetNotificationDetails.
The previous review comment about adding null checks for notificationSummary is still applicable to prevent potential NullReferenceException.
Apply this diff to add the null check:
var notificationSummary = await altinnNotificationService.GetNotificationDetails(notification.NotificationOrderId.ToString(), cancellationToken);
+ if (notificationSummary == null)
+ {
+ logger.LogWarning("Notification details not found for order ID: {OrderId}", notification.NotificationOrderId);
+ continue;
+ }
notificationSummary.IsReminder = notification.IsReminder;
notificationHistory.Add(notificationSummary);46-46: Verify the CallingAsSender extension method.
The code uses user.CallingAsSender() which appears to be an extension method. Let's verify its implementation.
✅ Verification successful
Extension method CallingAsSender is properly implemented
The CallingAsSender extension method is correctly implemented in ClaimsPrincipalExtensions.cs. It checks if the user has the altinn:correspondence.write scope in their claims, which is appropriate for determining if the user is calling as a sender.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the CallingAsSender extension method implementation
ast-grep --pattern 'public static bool CallingAsSender(this ClaimsPrincipal $_)'
# Alternatively, search for any usage or definition
rg -t cs "CallingAsSender"
Length of output: 575
Script:
#!/bin/bash
# Get the implementation of CallingAsSender method
ast-grep --pattern 'public static bool CallingAsSender(this ClaimsPrincipal user) {
$$$
}'
Length of output: 94
Script:
#!/bin/bash
# Get the content of the file containing CallingAsSender implementation
cat src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
Length of output: 1942
src/Altinn.Correspondence.API/Auth/CascadeAuthenticationHandler.cs (1)
2-2: LGTM: Namespace reorganization aligns with PR objectives.
The move from Application.Configuration to Common.Constants supports the authorization cleanup effort by centralizing authorization-related constants.
Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacyDeletionTests.cs (1)
3-3: LGTM! Constants moved to Common project.
The change aligns with the PR's objective of cleaning up authorization by moving constants to the new Altinn.Correspondence.Common project.
src/Altinn.Correspondence.API/Auth/DependencyInjection.cs (4)
3-3: LGTM! Import change aligns with centralization effort.
The migration of constants from Application.Configuration to Common.Constants aligns with the PR's objective of cleaning up the authorization process.
Line range hint 134-151: Authorization policies look good and align with PR objectives.
The policies correctly implement the new authorization approach using recipient and sender scopes, supporting delegated authority as mentioned in the PR objectives. The authentication scheme selection per policy is appropriate.
Let's verify the scope handler implementation:
Line range hint 72-89: Verify EdDsaSecurityKeysCacheService implementation.
The configuration uses EdDsaSecurityKeysCacheService.EdDsaSecurityKeys for key resolution. Let's verify its implementation to ensure proper key caching and rotation.
✅ Verification successful
EdDsaSecurityKeysCacheService implementation verified and found secure
The implementation shows a robust key caching and rotation mechanism with:
- Periodic refresh every 12 hours using
PeriodicTimer - Thread-safe key updates using volatile storage and atomic replacement
- Proper error handling and logging
- Secure JWKS endpoint consumption with proper EdDsa key conversion
- Immediate initial key fetch on service start
- Clean resource disposal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EdDsaSecurityKeysCacheService implementation
ast-grep --pattern 'class EdDsaSecurityKeysCacheService {
$$$
}'
# Check for key rotation logic
rg -A 5 "EdDsaSecurityKeys.*=.*new"
Length of output: 106
Script:
#!/bin/bash
# Search for EdDsaSecurityKeysCacheService class with a more flexible pattern
rg -A 20 "class.*EdDsaSecurityKeysCacheService"
# Search for EdDsaSecurityKeys property or field
rg -A 5 "EdDsaSecurityKeys"
# Search for any file containing EdDsaSecurityKeysCacheService
fd "EdDsaSecurityKeysCacheService"
Length of output: 10986
Script:
#!/bin/bash
# Let's check the StartAsync implementation to verify key rotation logic
rg -A 30 "public async Task StartAsync" src/Altinn.Correspondence.API/Auth/EdDsaSecurityKeysCacheService.cs
# Also check if there's any method that actually fetches and updates the keys
rg -A 30 "private async.*FetchKeys|UpdateKeys" src/Altinn.Correspondence.API/Auth/EdDsaSecurityKeysCacheService.cs
Length of output: 1189
Script:
#!/bin/bash
# Let's check the RefreshAsync method implementation
rg -A 30 "RefreshAsync" src/Altinn.Correspondence.API/Auth/EdDsaSecurityKeysCacheService.cs
# Also check for any methods that handle JWKS
rg -A 20 "jwks|JWKS" src/Altinn.Correspondence.API/Auth/EdDsaSecurityKeysCacheService.cs
Length of output: 2902
Line range hint 52-71: Consider enabling audience validation for Maskinporten tokens.
The JWT validation parameters have ValidateAudience = false. Consider enabling audience validation to prevent token misuse across different services.
Let's check if audience validation is handled elsewhere:
options.TokenValidationParameters = new TokenValidationParameters
{
ValidateIssuerSigningKey = true,
ValidateIssuer = true,
- ValidateAudience = false,
+ ValidateAudience = true,
+ ValidAudience = dialogportenSettings.Audience, // Add this setting to DialogportenSettings
RequireExpirationTime = true,
ValidateLifetime = !hostEnvironment.IsDevelopment(),
ClockSkew = TimeSpan.Zero
};Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceRetrievalTests.cs (2)
3-3: LGTM: Constants moved to Common library
The migration of constants from Application.Configuration to Common.Constants aligns with good architectural practices by centralizing shared constants in a common library.
Line range hint 1-290: Verify test coverage for delegated access scenarios
While the existing tests thoroughly cover direct sender/recipient access scenarios, the PR objectives mention supporting delegated access where "other parties who are not direct participants in a correspondence or attachment can act as a party if they have been delegated". Consider adding test cases for:
- Accessing correspondence through delegated permissions
- Verifying that the PDP request includes the correct recipient/sender information for delegated access
- Validating that non-delegated parties cannot access the correspondence
Let's check if there are any existing tests for delegated access:
...inn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDownloadTests.cs (1)
Line range hint
27-42: Rename test method to match new behavior.The test method name
DownloadAttachment_AsWrongSender_ReturnsBadRequestno longer matches the expected behavior. Consider renaming it to better reflect the actual test scenario.- public async Task DownloadAttachment_AsWrongSender_ReturnsBadRequest() + public async Task DownloadAttachment_AsWrongSender_ReturnsUnauthorized()Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentInitializationTests.cs (1)
34-39: LGTM! Consider these improvements.The status code change from BadRequest (400) to Unauthorized (401) correctly aligns with the new authorization model. However, consider these improvements:
- The test method name could be more concise:
-public async Task InitializeAttachment_As_Different_Sender_As_Token_ReturnsUnauthorized() +public async Task InitializeAttachment_WithDifferentSenderToken_ReturnsUnauthorized()
- Add XML documentation to clarify the test's purpose:
/// <summary> /// Verifies that attempting to initialize an attachment with a different sender's token /// results in an Unauthorized (401) response, as per the new authorization model. /// </summary>Additionally, given the PR's focus on delegated authority, consider adding a new test case:
[Fact] public async Task InitializeAttachment_WithDelegatedAuthority_Succeeds() { // Setup delegated authority // Attempt initialization // Assert success }Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentRetrievalTests.cs (1)
Line range hint
1-58: Consider adding tests for delegated access scenarios.Given that the PR introduces the ability for delegated parties to access attachments, consider adding test cases that verify:
- A delegated party can successfully access attachment details
- A party with insufficient delegation rights cannot access attachment details
Example test structure:
[Fact] public async Task GetAttachmentDetails_WithDelegatedAccess_Succeeds() { // Arrange var attachmentId = await AttachmentHelper.GetInitializedAttachment(_senderClient, _responseSerializerOptions); // Act var response = await _delegatedPartyClient.GetAsync($"correspondence/api/v1/attachment/{attachmentId}/details"); // Assert Assert.True(response.IsSuccessStatusCode); }Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs (3)
10-12: Add XML documentation for the test helper class.Since this is a test helper class that other developers will use, consider adding XML documentation to explain its purpose and usage.
+/// <summary> +/// Provides test helpers for overriding authorization behavior in integration tests. +/// </summary> public static class AuthorizationOverride { + /// <summary> + /// Configures services to use a mocked authorization service that allows fine-grained control over access checks. + /// </summary> + /// <param name="services">The service collection to configure.</param> + /// <returns>The configured service collection.</returns> public static IServiceCollection OverrideAuthorization(this IServiceCollection services)
14-23: Add null check for correspondence parameter.The mock setup should validate that the correspondence parameter is not null before proceeding.
.Returns((ClaimsPrincipal? user, CorrespondenceEntity corr, CancellationToken token) => { + if (corr == null) + { + throw new ArgumentNullException(nameof(corr)); + } return Task.FromResult(NotRecipient(user)); });
33-43: Validate string parameters in CheckAccessAsSender mock.The second overload should validate that required string parameters are not null or empty.
.Returns((ClaimsPrincipal? user, string resourceId, string sender, string? instance, CancellationToken token) => { + if (string.IsNullOrEmpty(resourceId)) + { + throw new ArgumentException("Value cannot be null or empty.", nameof(resourceId)); + } + if (string.IsNullOrEmpty(sender)) + { + throw new ArgumentException("Value cannot be null or empty.", nameof(sender)); + } return Task.FromResult(NotSender(user)); });src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (1)
25-33: Consider enhancing the error message for better debugging.The authorization checks are well-structured, but the error message could be more specific about whether the access was denied due to lacking both recipient and sender permissions.
Consider modifying the error handling to provide more context:
- if (!hasAccessAsRecipient && !hasAccessAsSender) - { - return Errors.NoAccessToResource; - } + if (!hasAccessAsRecipient && !hasAccessAsSender) + { + logger.LogWarning("Access denied: User has neither recipient nor sender permissions for correspondence {correspondenceId}", correspondence.Id); + return Errors.NoAccessToResource; + }Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentUploadTests.cs (1)
42-42: LGTM! The status code change improves HTTP semantic correctness.The change from
BadRequest(400) toUnauthorized(401) better reflects the nature of the error when an unauthorized sender attempts to upload an attachment. This aligns with the PR's authorization changes and follows HTTP status code semantics more accurately.Consider adding test cases to verify the new authorization behavior:
- Test with delegated authority (where a non-participant has been delegated access)
- Test with write scope to verify sender authorization
+[Fact] +public async Task UploadAttachmentData_WithDelegatedAuthority_Succeeds() +{ + // Arrange + var attachmentId = await AttachmentHelper.GetInitializedAttachment(_senderClient, _responseSerializerOptions); + + // Act + var uploadResponse = await AttachmentHelper.UploadAttachment(attachmentId, _delegatedAuthorityClient); + + // Assert + Assert.True(uploadResponse.IsSuccessStatusCode); +} + +[Fact] +public async Task UploadAttachmentData_WithoutWriteScope_Fails() +{ + // Arrange + var attachmentId = await AttachmentHelper.GetInitializedAttachment(_senderClient, _responseSerializerOptions); + + // Act + var uploadResponse = await AttachmentHelper.UploadAttachment(attachmentId, _clientWithoutWriteScope); + + // Assert + Assert.Equal(HttpStatusCode.Forbidden, uploadResponse.StatusCode); +}Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceAttachmentTests.cs (1)
Line range hint
193-209: Consider adding more authorization test cases.Since the PR introduces significant changes to the authorization model, consider adding test cases for:
- Access by delegated parties who are neither sender nor recipient
- Access with write scope to verify sender intent
- Access by authorized parties acting on behalf of recipients/senders
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
76-76: Consider passingcancellationTokentoReportActivityToDialogportenThe method
ReportActivityToDialogportendoes not receive thecancellationToken. If this method supports cancellation or performs operations that could benefit from cancellation support, consider passing thecancellationTokenfor consistency and enhanced responsiveness to cancellation requests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (14)
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs(1 hunks)Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs(2 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDownloadTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentInitializationTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentRetrievalTests.cs(2 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentUploadTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Attachment/Base/AttachmentTestBase.cs(2 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/Base/CorrespondenceTestBase.cs(2 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceAttachmentTests.cs(2 hunks)Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceRetrievalTests.cs(4 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs(2 hunks)src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs(2 hunks)src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Test/Altinn.Correspondence.Tests/TestingController/Attachment/Base/AttachmentTestBase.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/Base/CorrespondenceTestBase.cs
- Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceRetrievalTests.cs
- src/Altinn.Correspondence.Common/Helpers/StringExtensions.cs
🧰 Additional context used
📓 Learnings (6)
Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs (2)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs:44-47
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In this codebase, the dev service implementations `ConsoleLogEventBus`, `AltinnDevNotificationService`, `DialogportenDevService`, and `AltinnAuthorizationDevService` are intentionally minimal and do not need to fully mimic production behavior.
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#513
File: Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs:78-101
Timestamp: 2024-11-20T12:39:19.050Z
Learning: In the Altinn.Correspondence.Tests project, splitting test methods is not required unless the distinctions are significant.
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDownloadTests.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#513
File: Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs:78-101
Timestamp: 2024-11-20T12:39:19.050Z
Learning: In the Altinn.Correspondence.Tests project, splitting test methods is not required unless the distinctions are significant.
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentRetrievalTests.cs (2)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#513
File: Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs:78-101
Timestamp: 2024-11-20T12:39:19.050Z
Learning: In the Altinn.Correspondence.Tests project, splitting test methods is not required unless the distinctions are significant.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs:10-10
Timestamp: 2024-11-12T05:02:53.065Z
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.
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentUploadTests.cs (1)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#513
File: Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs:78-101
Timestamp: 2024-11-20T12:39:19.050Z
Learning: In the Altinn.Correspondence.Tests project, splitting test methods is not required unless the distinctions are significant.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#480
File: src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs:61-63
Timestamp: 2024-11-12T12:30:11.505Z
Learning: In the `PurgeCorrespondenceHandler` class, the `IsAffiliatedWithCorrespondence` method verifies if the user is either the sender or the recipient, so additional checks for user affiliation are redundant.
🔇 Additional comments (13)
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDownloadTests.cs (1)
40-40: LGTM! Correct status code for authorization failure.
The change from BadRequest to Unauthorized is more semantically accurate as it properly indicates an authorization failure rather than a malformed request. This aligns well with the PR's objective of cleaning up the authorization process.
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentRetrievalTests.cs (2)
29-33: LGTM! Status code change aligns with new authorization model.
The change from BadRequest to Unauthorized correctly reflects the new authorization approach where access is determined by authorization capabilities rather than direct participation status.
52-56: LGTM! Consistent with the authorization model changes.
This change follows the same pattern as the GetAttachmentOverview test, maintaining consistency in how unauthorized access attempts are handled across different endpoints.
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs (1)
44-52: Clarify the CheckAccessAsAny logic.
The current implementation returns true if the user is NOT a recipient OR NOT a sender, which might be counterintuitive. Consider if this is the intended behavior, as it means access is granted when the user lacks either role.
✅ Verification successful
The CheckAccessAsAny mock implementation is correct
The implementation is actually correct and intuitive when considering the complete context:
NotRecipientandNotSenderreturntruewhen the user does NOT have the respective claim- The actual authorization service implementation checks for READ or WRITE access
- The mock is used in tests to verify access control, where access should be denied only when the user has neither role (i.e., when both
NotRecipientANDNotSenderare true) - The OR logic in
NotRecipient(user) || NotSender(user)correctly grants access when the user has at least one of the roles (when eitherNotRecipientorNotSenderis false)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for tests using this mock to verify the expected behavior
rg -l "CheckAccessAsAny" --type cs
Length of output: 443
Script:
#!/bin/bash
# Let's check the implementation details of CheckAccessAsAny and its usage
echo "=== Interface definition ==="
ast-grep --pattern 'Task<bool> CheckAccessAsAny($$$)'
echo -e "\n=== Implementation in AltinnAuthorizationService ==="
rg -A 10 "CheckAccessAsAny" src/Altinn.Correspondence.Integrations/Altinn/Authorization/AltinnAuthorizationService.cs
echo -e "\n=== Usage in GetCorrespondencesHandler ==="
rg -A 10 "CheckAccessAsAny" src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs
echo -e "\n=== Test cases ==="
rg -A 10 "CheckAccessAsAny" Test/Altinn.Correspondence.Tests/
Length of output: 2866
Script:
#!/bin/bash
# Let's check the NotRecipient and NotSender methods to understand the complete logic
echo "=== NotRecipient and NotSender implementations ==="
rg -A 5 "NotRecipient|NotSender" Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs
echo -e "\n=== Test cases using this authorization override ==="
rg -B 5 -A 10 "AuthorizationOverride" Test/Altinn.Correspondence.Tests/
Length of output: 3245
src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (1)
Line range hint 46-58: Verify the recipient access condition logic.
The condition hasAccessAsRecipient && !user.CallingAsSender() needs verification to ensure it correctly handles all authorization scenarios, particularly:
- When a user has both recipient and sender access
- The interaction with the write scope that determines
CallingAsSender()
#!/bin/bash
# Description: Verify the CallingAsSender implementation and its usage
# Check the implementation of CallingAsSender
ast-grep --pattern 'CallingAsSender() {
$$$
}'
# Check other usages of this condition pattern
rg -A 2 'hasAccessAsRecipient.*!.*CallingAsSender'Test/Altinn.Correspondence.Tests/Helpers/CustomWebApplicationFactory.cs (4)
27-27: LGTM!
The added blank line improves readability.
57-58: LGTM!
The added blank lines improve readability.
Line range hint 1-146: Well-structured test utilities implementation
The CustomWebApplicationFactory provides a robust set of test utilities with:
- Flexible claim configuration for both regular and Dialogporten scenarios
- Appropriate service mocking
- Clear separation of concerns
49-49: Verify the authorization override implementation
The change from explicit service registration to OverrideAuthorization() aligns with the PR's authorization cleanup objectives. However, we should verify:
- The implementation of the authorization override
- The test coverage for various authorization scenarios
✅ Verification successful
Authorization override implementation is properly implemented and used
The change from explicit service registration to OverrideAuthorization() is well-implemented:
- The extension method provides a comprehensive mock of
IAltinnAuthorizationServicewith all necessary authorization scenarios - It handles recipient access, sender access, and migration access checks
- The implementation is used consistently in test factories
- Authorization behavior can be controlled via claims ("notSender" and "notRecipient")
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the authorization override implementation and its usage
# Check for the extension method definition
echo "Searching for OverrideAuthorization extension method..."
ast-grep --pattern 'public static $_$ OverrideAuthorization(this IServiceCollection $_$) {
$$$
}'
# Check for test coverage of authorization scenarios
echo "Searching for authorization-related tests..."
rg -l "OverrideAuthorization|IAltinnAuthorizationService" "Test/" --type cs
Length of output: 605
Script:
#!/bin/bash
# Let's examine the implementation of AuthorizationOverride and its usage
# Check the implementation of AuthorizationOverride
echo "=== AuthorizationOverride.cs implementation ==="
cat "Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs"
# Check how it's used in test factories
echo -e "\n=== Usage in UnitWebApplicationFactory ==="
cat "Test/Altinn.Correspondence.Tests/Helpers/UnitWebApplicationFactory.cs"
# Look for authorization-related test scenarios
echo -e "\n=== Searching for authorization test scenarios ==="
rg "OverrideAuthorization" "Test/" -C 2 --type cs
Length of output: 9592
Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs (1)
103-109: LGTM! The status code change aligns with authorization changes.
The change from BadRequest to Unauthorized correctly reflects that this is an authorization failure rather than a validation error, which aligns with the PR's objective of cleaning up the authorization process.
Let's verify that similar test cases have been updated consistently:
✅ Verification successful
The status code change is consistent with the authorization changes across the codebase
Based on the verification results:
- The test class correctly uses different status codes for different scenarios:
Unauthorized(401) for authentication failuresForbidden(403) for authorization failuresBadRequest(400) only for actual validation errors
- This is the only test case that needed the status code change from
BadRequesttoUnauthorized, as it specifically tests wrong sender authentication. - Other authorization-related test cases in the codebase already use appropriate status codes for their scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining BadRequest assertions in authorization-related tests
# Expect: No instances of BadRequest status codes in authorization test methods
# Search for test methods containing "wrong" or "different" sender/recipient that still use BadRequest
rg -l 'public async Task.*(_As)?(_Different_|Wrong|_As_Different_).*sender.*BadRequest' Test/
Length of output: 95
Script:
#!/bin/bash
# Let's try a broader search to find any authorization-related test methods
# First, find all test files related to authorization or wrong sender scenarios
rg -l "WrongSender|DifferentSender|Unauthorized" Test/
# Then, let's examine the specific test class to see all status code assertions
rg "Assert.*StatusCode" Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs
Length of output: 1841
Script:
#!/bin/bash
# Let's examine the test methods in other files to ensure consistent authorization handling
rg "DeleteAttachment_As.*Sender" -A 3 Test/Altinn.Correspondence.Tests/TestingController/Attachment/AttachmentDeletionTests.cs
# Also check other similar authorization test cases in related files
rg "(_AsWrongSender_|_AsDifferentSender_).*Unauthorized" -A 3 Test/Altinn.Correspondence.Tests/TestingController/
Length of output: 1066
Test/Altinn.Correspondence.Tests/TestingController/Correspondence/CorrespondenceAttachmentTests.cs (1)
Line range hint 193-209: LGTM! Changes align with the new authorization model.
The test correctly validates that unauthorized access now returns 401 (Unauthorized) instead of 404 (Not Found), which aligns with the PR's objective of moving from instance checking to PDP-based authorization.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (2)
76-77: Ensure asynchronous methods are properly awaited
If ReportActivityToDialogporten and CancelNotification are asynchronous methods, they should be awaited to ensure correct execution flow and error handling.
42-49: Verify logic when user has access as both sender and recipient
The current implementation processes the purge request as sender if hasAccessAsSender is true, even if hasAccessAsRecipient is also true. Please verify that this behavior aligns with the intended access control logic. If both access rights are granted, consider whether additional logic is needed to handle this scenario appropriately.
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs (1)
21-22: Clarify the naming ofNotRecipientmethod for better understanding.The method
NotRecipientreturnstruewhen the user is authorized as a recipient, which may be confusing due to its name. A more intuitive name would improve code readability.Consider renaming the methods:
-private static bool NotRecipient(ClaimsPrincipal? user) +private static bool IsAuthorizedAsRecipient(ClaimsPrincipal? user) { return !user?.Claims.Any(c => - c.Type == "notRecipient") ?? true; + c.Type == "notRecipient") ?? true; } -private static bool NotSender(ClaimsPrincipal? user) +private static bool IsAuthorizedAsSender(ClaimsPrincipal? user) { return !user?.Claims.Any(c => - c.Type == "notSender") ?? true; + c.Type == "notSender") ?? true; }And update the return statements accordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs(1 hunks)
🔇 Additional comments (3)
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs (3)
80-89: Refactor helper methods to reduce duplication and improve readability.
The NotSender and NotRecipient methods share similar logic. Refactoring them into a single method with a parameter for the claim type would enhance code maintainability.
Consider this refactor:
+ private static bool DoesNotHaveClaim(ClaimsPrincipal? user, string claimType)
+ {
+ return !user?.Claims.Any(c => c.Type == claimType) ?? true;
+ }
- private static bool NotSender(ClaimsPrincipal? user)
- {
- return !user?.Claims.Any(c =>
- c.Type == "notSender") ?? true;
- }
+ private static bool NotSender(ClaimsPrincipal? user) =>
+ DoesNotHaveClaim(user, "notSender");
- private static bool NotRecipient(ClaimsPrincipal? user)
- {
- return !user?.Claims.Any(c =>
- c.Type == "notRecipient") ?? true;
- }
+ private static bool NotRecipient(ClaimsPrincipal? user) =>
+ DoesNotHaveClaim(user, "notRecipient");73-75: Verify the appropriateness of the hardcoded authorization level.
The mock returns a fixed value of 3 for CheckUserAccessAndGetMinimumAuthLevel. Ensure this value aligns with your test scenarios and consider parameterizing it if different tests require varying authorization levels.
If necessary, modify the mock to accept a configurable authorization level.
54-62:
Document the security implications of always returning true for migration access.
The mock implementation of CheckMigrationAccess always returns true, granting migration access regardless of input parameters. This could lead to unintended behavior in tests that rely on accurate authorization checks. It's important to document this behavior to prevent misuse in security-sensitive tests.
Consider adding a warning comment:
altinnAuthorizationService
.Setup(x => x.CheckMigrationAccess(
It.IsAny<string>(),
It.IsAny<List<ResourceAccessLevel>>(),
It.IsAny<CancellationToken>()))
+ // WARNING: This mock always grants migration access. Ensure this is intended for your tests.
.Returns((string resourceId, IEnumerable<ResourceAccessLevel> levels, CancellationToken token) =>
{
return Task.FromResult(true);
});
Test/Altinn.Correspondence.Tests/Helpers/AuthorizationOverride.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (1)
27-33: Consider enhancing error handling and logging for authorization failures.While the new authorization check is cleaner and aligns with the PR objectives, consider these improvements:
- Add debug logging for authorization failures to aid troubleshooting
- Consider using a more specific error message than "NoAccessToResource"
var hasAccess = await altinnAuthorizationService.CheckAccessAsRecipient( user, correspondence, cancellationToken); if (!hasAccess) { + logger.LogDebug("User {userId} was denied access as recipient for correspondence {correspondenceId}", + user?.GetUserId(), correspondence.Id); return Errors.NoAccessToResource; }src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs (1)
28-33: Consider adding explicit user null check for security-critical operationThe authorization check correctly implements the new pattern using
CheckAccessAsSenderwith the sender's information. However, for security-critical operations, consider adding an explicit null check for the user parameter before the authorization check.+ if (user == null) + { + return Errors.Unauthorized; + } var hasAccess = await altinnAuthorizationService.CheckAccessAsSender( user, attachment.ResourceId, attachment.Sender.WithoutPrefix(), attachment.Id.ToString(), cancellationToken);src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (2)
27-38: Consider enhancing error feedback for access denialThe authorization logic is well-structured with separate checks for recipient and sender access. However, the generic
Errors.NoAccessToResourcedoesn't provide enough context about which access check failed.Consider returning more specific error messages:
- if (!hasAccessAsRecipient && !hasAccessAsSender) - { - return Errors.NoAccessToResource; - } + if (!hasAccessAsRecipient && !hasAccessAsSender) + { + logger.LogInformation("Access denied - User has neither recipient nor sender access"); + return new Error("Access denied", "User is not authorized as either recipient or sender"); + }
45-49: Add logging for party lookup failuresWhile the error handling is correct, adding debug logging would help troubleshoot party lookup failures.
Consider adding logging:
var party = await altinnRegisterService.LookUpPartyById(user.GetCallerOrganizationId(), cancellationToken); if (party?.PartyUuid is not Guid partyUuid) { + logger.LogDebug("Failed to find party UUID for organization ID: {OrgId}", user.GetCallerOrganizationId()); return Errors.CouldNotFindPartyUuid; }src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (3)
Line range hint
297-301: Consider making reminder delay configurableThe reminder notification timing is currently hardcoded with different values for production (7 days) and development (1 hour). This could make testing production scenarios difficult and limit flexibility.
Consider moving these values to configuration:
- RequestedSendTime = hostEnvironment.IsProduction() ? notificationOrder.RequestedSendTime.AddDays(7) : notificationOrder.RequestedSendTime.AddHours(1), + RequestedSendTime = hostEnvironment.IsProduction() + ? notificationOrder.RequestedSendTime.AddDays(_generalSettings.ProductionReminderDelayDays) + : notificationOrder.RequestedSendTime.AddHours(_generalSettings.DevelopmentReminderDelayHours),
Line range hint
248-257: Improve error handling for notification creation failuresThe current approach of using
Guid.Emptyfor failed notification creation doesn't provide enough context for debugging and could lead to issues downstream.Consider enhancing the error handling:
- notificationDetails.Add(new InitializedCorrespondencesNotifications() - { - OrderId = Guid.Empty, - Status = InitializedNotificationStatus.Failure - }); + var failureDetails = new InitializedCorrespondencesNotifications() + { + OrderId = Guid.NewGuid(), // Generate a unique ID for tracking + Status = InitializedNotificationStatus.Failure, + ErrorDetails = new NotificationError + { + ErrorCode = "NOTIFICATION_CREATE_FAILED", + Message = "Failed to create notification order" + } + }; + logger.LogError("Failed to create notification order for correspondence {CorrespondenceId}", correspondence.Id); + notificationDetails.Add(failureDetails);
Line range hint
379-388: Enhance condition endpoint validationThe current localhost check might be too simplistic for various development scenarios.
Consider a more robust implementation:
private Uri? CreateConditionEndpoint(string correspondenceId) { var conditionEndpoint = new Uri($"{_generalSettings.CorrespondenceBaseUrl.TrimEnd('/')}/correspondence/api/v1/correspondence/{correspondenceId}/notification/check"); - if (conditionEndpoint.Host == "localhost") + // Skip endpoints for development environments + if (!hostEnvironment.IsProduction() || + conditionEndpoint.Host == "localhost" || + conditionEndpoint.Host.EndsWith(".local")) { return null; } + // Ensure HTTPS in production + if (hostEnvironment.IsProduction() && conditionEndpoint.Scheme != "https") + { + throw new InvalidOperationException("Production endpoints must use HTTPS"); + } return conditionEndpoint; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
Test/Altinn.Correspondence.Tests/TestingController/Legacy/Base/LegacyTestBase.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacySearchTests.cs(1 hunks)Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs(1 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs(2 hunks)src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs(1 hunks)src/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs(2 hunks)src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs(2 hunks)src/Altinn.Correspondence.Application/PurgeAttachment/PurgeAttachmentHandler.cs(3 hunks)src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs(2 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs(1 hunks)src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs(2 hunks)src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- Test/Altinn.Correspondence.Tests/TestingController/Legacy/Base/LegacyTestBase.cs
- Test/Altinn.Correspondence.Tests/TestingController/Legacy/LegacySearchTests.cs
- Test/Altinn.Correspondence.Tests/TestingFeature/DialogportenTests.cs
- src/Altinn.Correspondence.Application/GetCorrespondenceDetails/GetCorrespondenceDetailsHandler.cs
- src/Altinn.Correspondence.Application/InitializeAttachment/InitializeAttachmentHandler.cs
- src/Altinn.Correspondence.Application/PurgeAttachment/PurgeAttachmentHandler.cs
- src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/LegacyUpdateCorrespondenceStatusHandler.cs
🧰 Additional context used
📓 Learnings (3)
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (1)
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#480
File: src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs:61-63
Timestamp: 2024-11-12T12:30:11.505Z
Learning: In the `PurgeCorrespondenceHandler` class, the `IsAffiliatedWithCorrespondence` method verifies if the user is either the sender or the recipient, so additional checks for user affiliation are redundant.
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-11-12T05:02:53.065Z
Learning: The `IsConfirmationNeeded` property in the `CorrespondenceEntity` class is no longer a required property.
src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs (7)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs:10-10
Timestamp: 2024-11-12T05:02:53.065Z
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.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#501
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:30-30
Timestamp: 2024-11-19T09:09:49.026Z
Learning: For security-critical operations like downloading attachments, the team prefers to verify the nullability of the `ClaimsPrincipal` parameter within the method when checking user access, rather than making the parameter non-nullable.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#530
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:14-14
Timestamp: 2024-11-27T14:24:38.729Z
Learning: In the `UploadHelper.cs` file, the `partyUuid` parameter is validated before calling the `UploadAttachment` method, so additional validation within this method is unnecessary.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateUploadAttachmentHandler.cs:39-39
Timestamp: 2024-11-12T05:02:53.065Z
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/Helpers/UploadHelper.cs:57-59
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In non-development environments, the `MalwareScanResultHandler` updates the attachment status after performing a malware scan.
Learnt from: CelineTrammi
PR: Altinn/altinn-correspondence#421
File: src/Altinn.Correspondence.Application/DownloadCorrespondenceAttachment/LegacyDownloadCorrespondenceAttachmentHandler.cs:52-52
Timestamp: 2024-11-12T05:02:53.065Z
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.
🔇 Additional comments (12)
src/Altinn.Correspondence.Application/UpdateCorrespondenceStatus/UpdateCorrespondenceStatusHandler.cs (2)
Line range hint 11-17: LGTM! Constructor changes align with the new authorization approach.
The removal of UserClaimsHelper dependency and use of primary constructor syntax simplifies the code while maintaining all necessary dependencies for the new authorization flow.
Line range hint 45-49: Verify the GetCallerOrganizationId extension method implementation.
The code looks good, but let's verify the new extension method to ensure it correctly extracts the organization ID from claims.
✅ Verification successful
GetCallerOrganizationId implementation looks robust and well-structured
The extension method implementation is thorough and handles multiple authentication scenarios:
- System user tokens via
authorization_detailsclaim - Enterprise tokens via
urn:altinn:orgNumberclaim - Personal tokens via
consumerclaim - Dialog tokens via
IDclaim
The method properly handles null cases and follows a clear precedence order for different token types. The implementation aligns with standard claims-based authentication patterns and appears to be a safe replacement for the previous UserClaimsHelper.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GetCallerOrganizationId extension method implementation
# Look for the extension method definition
ast-grep --pattern 'public static $_ GetCallerOrganizationId(this ClaimsPrincipal $_) {
$$$
}'
# Check for any test coverage
rg -l "GetCallerOrganizationId.*Test"
Length of output: 138
Script:
#!/bin/bash
# Let's try a broader search for the extension method and its tests
# Search for GetCallerOrganizationId method definition
rg -A 5 "GetCallerOrganizationId"
# Search for files containing ClaimsPrincipal extensions
fd "ClaimsPrincipal.*Extensions?.cs"
# Search for any test files related to claims or organization ID
rg -l "ClaimsPrincipal.*Test|OrganizationId.*Test"
Length of output: 8364
Script:
#!/bin/bash
# Let's examine the complete implementation of ClaimsPrincipalExtensions
cat src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
# Let's also check for any authorization-related tests
rg -l "Authorization.*Test"
Length of output: 1972
src/Altinn.Correspondence.Application/UploadAttachment/UploadAttachmentHandler.cs (2)
1-2: LGTM: Clean dependency management
The addition of Common.Helpers and use of primary constructor syntax aligns with the codebase conventions.
53-53: Verify the GetCallerOrganizationId extension method
The change to use GetCallerOrganizationId() aligns with the authorization cleanup. Let's verify this extension method is properly implemented.
src/Altinn.Correspondence.Application/PurgeCorrespondence/PurgeCorrespondenceHandler.cs (4)
31-38: LGTM! Authorization checks are now more explicit and aligned with the new design.
The new authorization checks using CheckAccessAsSender and CheckAccessAsRecipient provide a clearer separation of concerns and better align with the PR's objective of removing instance checking in favor of recipient/sender information.
39-58: LGTM! Well-structured access validation with proper error handling.
The code clearly separates sender and recipient validation paths, with appropriate error handling for each case. This structure makes the validation logic more maintainable and easier to understand.
59-63: LGTM! Proper sequencing of validations.
Status validation is correctly positioned after access checks, ensuring that status is only validated for authorized users.
84-86:
Critical: Async methods must be awaited.
The calls to ReportActivityToDialogporten and CancelNotification are not awaited, which could lead to:
- Fire-and-forget operations that might not complete
- Unhandled exceptions
- Incorrect execution flow
Apply this fix:
- purgeCorrespondenceHelper.ReportActivityToDialogporten(hasAccessAsSender && user.CallingAsSender(), correspondenceId);
- purgeCorrespondenceHelper.CancelNotification(correspondenceId, cancellationToken);
+ await purgeCorrespondenceHelper.ReportActivityToDialogporten(hasAccessAsSender && user.CallingAsSender(), correspondenceId);
+ await purgeCorrespondenceHelper.CancelNotification(correspondenceId, cancellationToken);src/Altinn.Correspondence.Application/GetCorrespondenceOverview/GetCorrespondenceOverviewHandler.cs (2)
53-54: Document the dual-condition access check
The condition hasAccessAsRecipient && !user.CallingAsSender() combines two checks without clear explanation of why both are necessary.
Consider adding a comment explaining the logic:
+ // Only update status to 'Fetched' when accessed by recipient (not sender)
+ // This prevents status changes when sender views the correspondence
if (hasAccessAsRecipient && !user.CallingAsSender())
{27-34: Verify consistent authorization pattern across handlers
The new authorization approach using CheckAccessAsRecipient and CheckAccessAsSender aligns well with the PR objectives. Let's ensure this pattern is consistently applied across other handlers.
✅ Verification successful
Let me gather more information to verify the authorization pattern across handlers.
Authorization pattern is consistently implemented across handlers
The verification shows that the new authorization pattern using CheckAccessAsRecipient and CheckAccessAsSender is consistently implemented across relevant handlers:
- GetCorrespondenceOverviewHandler
- GetCorrespondenceDetailsHandler
- UpdateCorrespondenceStatusHandler
- PurgeCorrespondenceHandler
- DownloadCorrespondenceAttachmentHandler
All handlers properly utilize the authorization service through the IAltinnAuthorizationService interface, and there are no remaining legacy instance checks in the codebase. The implementation aligns well with the PR's objective of cleaning up the authorization process.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other handlers using the new authorization pattern
ast-grep --pattern 'class $_Handler {
$$$
CheckAccessAsRecipient($_) {
$$$
}
$$$
}'
# Search for any remaining instance checks that should have been removed
rg -i "instance.*check|check.*instance"
Length of output: 8486
Script:
#!/bin/bash
# Search for all handler classes and their authorization checks
ast-grep --pattern 'class $_Handler {
$$$
async Task$_ Handle($_) {
$$$
}
}'
# Search for all authorization service usages
rg -l "altinnAuthorizationService"
# Search specifically for CheckAccessAsRecipient usage
rg "CheckAccessAsRecipient" -A 2
Length of output: 6468
src/Altinn.Correspondence.Application/InitializeCorrespondences/InitializeCorrespondencesHandler.cs (2)
4-4: LGTM: Clean dependency management
The addition of Common.Helpers and removal of UserClaimsHelper aligns well with the PR's objective of cleaning up the authorization process.
41-46: LGTM: Improved authorization logic
The new authorization approach using CheckAccessAsSender with proper prefix handling aligns perfectly with the PR's objective of streamlining the authorization process and supporting delegated authority.
Let's verify that this authorization pattern is consistently applied across the codebase:
✅ Verification successful
Authorization pattern is consistently applied across handlers
The verification shows that CheckAccessAsSender is consistently used across all handlers with proper prefix handling via WithoutPrefix(). The pattern is uniformly implemented in:
- Correspondence operations (Initialize, Get Details/Overview, Purge)
- Attachment operations (Initialize, Upload, Download, Get Details/Overview, Purge)
All implementations follow the same security pattern, passing the required user, resourceId, and sender information, with optional contextId where needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all occurrences of CheckAccessAsSender to ensure consistent usage
ast-grep --pattern 'await $_.CheckAccessAsSender($$$)'
Length of output: 6784
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (2)
29-34: Consider extracting the organization prefix as a constant.The "0192:" prefix appears to be a magic string. Consider extracting it to a named constant for better maintainability and to avoid potential typos.
+ private const string ORGANIZATION_PREFIX = "0192:"; ... - onBehalfOf = "0192:" + httpContextAccessor.HttpContext?.User.GetCallerOrganizationId(); + onBehalfOf = ORGANIZATION_PREFIX + httpContextAccessor.HttpContext?.User.GetCallerOrganizationId();
44-44: Track the TODO comment for delegated access implementation.The TODO comment indicates missing functionality for retrieving delegated instances, which is crucial for the new authorization model allowing other parties to act if delegated. Consider tracking this in a separate issue.
Would you like me to help create a GitHub issue to track this TODO item? I can help draft the implementation for retrieving delegated instances as well.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs(3 hunks)src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Altinn.Correspondence.Common/Helpers/ClaimsPrincipalExtensions.cs
🧰 Additional context used
📓 Learnings (1)
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (3)
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/Helpers/UploadHelper.cs:16-16
Timestamp: 2024-11-12T05:02:53.065Z
Learning: The codebase uses C# 12 features like primary constructors, where dependencies are initialized directly in the class declaration.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.API/Program.cs:97-97
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the Altinn.Correspondence.Integrations project, services such as AltinnAuthorizationService and AltinnAccessManagementService intentionally retrieve IHostEnvironment from dependency injection.
Learnt from: Ceredron
PR: Altinn/altinn-correspondence#368
File: src/Altinn.Correspondence.Application/MigrateCorrespondenceAttachment/MigrateUploadAttachmentHandler.cs:39-39
Timestamp: 2024-11-12T05:02:53.065Z
Learning: In the `Altinn.Correspondence.Application` codebase, error handling is performed using `OneOf` result types instead of exceptions.
🔇 Additional comments (4)
src/Altinn.Correspondence.Application/GetCorespondences/GetCorrespondencesHandler.cs (4)
Line range hint 1-12: LGTM! Constructor changes align with authorization refactoring.
The replacement of UserClaimsHelper with IHttpContextAccessor aligns with the PR's objective to clean up the authorization process. The use of primary constructor syntax maintains consistency with the codebase conventions.
Line range hint 14-28: LGTM! Input validation remains robust.
The validation logic thoroughly handles negative values and invalid date ranges, using the appropriate error handling pattern with OneOf results.
35-43: LGTM! Authorization logic aligns with new design.
The switch to CheckAccessAsAny correctly implements the new authorization model where access is determined by the ability to act on behalf of the recipient or sender, rather than direct participation in the correspondence.
Line range hint 1-65: Verify complete removal of onBehalfOf parameter.
The PR objectives mention removing the "onBehalfOf" parameter from all endpoints except GetCorrespondences. Let's verify this is the only remaining usage.
✅ Verification successful
The onBehalfOf parameter is correctly isolated to GetCorrespondences functionality only
The verification confirms that onBehalfOf is only used in the GetCorrespondences feature path:
- GetCorrespondencesRequest.cs: As a request property
- GetCorrespondencesHandler.cs: For processing the request
- CorrespondenceController.cs: In the GET endpoint
- AltinnAuthorizationService.cs: Only as a comment referring to a claim type "p"
This aligns with the PR objective of removing the parameter from all endpoints except GetCorrespondences.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of onBehalfOf parameter in the codebase
rg -l "OnBehalfOf" --type cs
# Search for method parameters named onBehalfOf
ast-grep --pattern 'public $_ $_($$$, string onBehalfOf, $$$) { $$$ }'
Length of output: 431
Script:
#!/bin/bash
# Let's check the actual usage context in these files
rg -C 5 "OnBehalfOf" --type cs
# Specifically look for method parameters in interfaces and repositories
ast-grep --pattern 'interface $_ {
$$$
$_ $_($$$, string onBehalfOf, $$$);
$$$
}'
Length of output: 4789
Description
Second big clean-up of authorization. Most significantly - no longer instance checking correspondences ourselves. Not necessary as we use the recipient/sender in the PDP request. This also makes it so that other parties that are not direct party to a correspondence, or attachment, can use it as the party if they have been delegated.
A logically significant of this is the change in GetCorrespondenceDetailsHandler. Instead of basing the logic on whether the caller is the recipient or is the sender, we perform that business logic based on whether the caller is authorized to act on behalf of the recipient or if he authorized on behalf of the sender (or both). But as someone authorized as a sender may also be acting as a recipient, we use the existence of the write scope to determine if the call was intended to be made as a sender.
I also removed onBehalfOf everywhere in the API except for GetCorrespondences as access to call on behalf of someone is determined by Authorization (not sure if I thought right about this).
Related Issue(s)
Verification
Documentation
Summary by CodeRabbit
Release Notes
New Features
Altinn.Correspondence.Common, enhancing the solution's modularity.Improvements
OnBehalfOfparameter from requests.Bug Fixes
Documentation