feat: Enforce minimum auth level requirements on dialogs #1875
Conversation
|
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 23 minutes and 12 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis change set adds new error handling and authorization features across schema definitions, API endpoints, and internal DTOs. A new GraphQL error type ( Changes
Possibly related PRs
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
Documentation and Community
|
oskogstad
force-pushed
the
feat/enforce-auth-level-requirements-on-dialogs
branch
from
8964f7e to
b9e893e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (3)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Get/GetDialogActivityEndpoint.cs (1)
25-28: 🛠️ Refactor suggestionUpdate API documentation to include Forbidden status code.
The endpoint now handles forbidden access cases, but the Description doesn't reflect this in ProducesOneOf.
Update the Description to include the forbidden status:
Description(b => b.ProducesOneOf<ActivityDto>( StatusCodes.Status200OK, - StatusCodes.Status404NotFound)); + StatusCodes.Status404NotFound, + StatusCodes.Status403Forbidden));src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Get/GetDialogTransmissionEndpoint.cs (1)
25-28: 🛠️ Refactor suggestionUpdate API documentation to include Forbidden status code.
The endpoint now handles forbidden access cases, but the Description doesn't reflect this in ProducesOneOf.
Update the Description to include the forbidden status:
Description(b => b.ProducesOneOf<TransmissionDto>( StatusCodes.Status200OK, - StatusCodes.Status404NotFound)); + StatusCodes.Status404NotFound, + StatusCodes.Status403Forbidden));src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssignmentLogs/Search/SearchDialogLabelAssignmentLogEndpoint.cs (1)
24-28: 🛠️ Refactor suggestionUpdate API documentation to include Forbidden status code.
The endpoint now handles forbidden access cases, but the Description doesn't reflect this in ProducesOneOf.
Update the Description to include the forbidden status:
Description(d => d.ProducesOneOf<List<LabelAssignmentLogDto>>( StatusCodes.Status200OK, StatusCodes.Status404NotFound, - StatusCodes.Status410Gone)); + StatusCodes.Status410Gone, + StatusCodes.Status403Forbidden));
🧹 Nitpick comments (11)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/DialogContentExtensions.cs (1)
7-22: Add unit tests for edge cases.Please add unit tests to verify the behavior when:
- Non-sensitive content exists but regular content is missing
- Non-sensitive content has null values
- The content list is empty or null
Would you like me to help generate these unit tests?
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs (1)
22-23: Add XML documentation and input validation.The new methods lack documentation and input validation. This could lead to confusion about their usage and potential issues with invalid inputs.
Consider adding XML documentation and input validation:
+ /// <summary> + /// Checks if the current user's authentication level meets or exceeds the specified minimum level. + /// </summary> + /// <param name="minimumAuthenticationLevel">The minimum required authentication level. Must be greater than 0.</param> + /// <returns>True if the user meets the minimum authentication level; otherwise, false.</returns> + /// <exception cref="ArgumentOutOfRangeException">Thrown when minimumAuthenticationLevel is less than 1.</exception> bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel); + /// <summary> + /// Checks if the current user's authentication level meets the minimum level required for the specified service resource. + /// </summary> + /// <param name="serviceResource">The service resource identifier. Cannot be null or empty.</param> + /// <returns>True if the user meets the required authentication level; otherwise, false.</returns> + /// <exception cref="ArgumentNullException">Thrown when serviceResource is null or empty.</exception> bool UserHasRequiredAuthLevel(string serviceResource);src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs (1)
71-74: Consider reordering null check and auth level check.The activity null check should be moved before the auth level check to avoid unnecessary authorization checks when the activity doesn't exist.
if (dialog.Deleted) { return new EntityDeleted<DialogEntity>(request.DialogId); } + var activity = dialog.Activities.FirstOrDefault(); + + if (activity is null) + { + return new EntityNotFound<DialogActivity>(request.ActivityId); + } + if (!_altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource)) { return new Forbidden(Constants.AltinnAuthLevelToLow); } - var activity = dialog.Activities.FirstOrDefault(); - - if (activity is null) - { - return new EntityNotFound<DialogActivity>(request.ActivityId); - }Also applies to: 76-81
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs (1)
13-13: Fix grammatical error in constant name and message.The constant name and message have a grammatical error: "ToLow" should be "TooLow".
Apply this diff to fix the grammatical error:
- public const string AltinnAuthLevelToLow = "Altinn authentication level too low."; + public const string AltinnAuthLevelTooLow = "Altinn authentication level is too low for this operation.";src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (1)
74-75: Add warning logs for development-only implementation.While returning
trueis appropriate for local development, adding warning logs would help prevent accidental use in production.Apply this diff to add warning logs:
- public bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel) => true; - public bool UserHasRequiredAuthLevel(string serviceResource) => true; + public bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel) + { + Console.WriteLine("WARNING: Using development-only authorization that always returns true!"); + return true; + } + + public bool UserHasRequiredAuthLevel(string serviceResource) + { + Console.WriteLine("WARNING: Using development-only authorization that always returns true!"); + return true; + }src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (1)
81-84: Consider moving auth check before seen log existence check.While the implementation is correct, moving the authorization check before the seen log existence check would optimize the flow by failing fast when authorization is insufficient.
Apply this diff to optimize the check order:
if (dialog.Deleted) { return new EntityDeleted<DialogEntity>(request.DialogId); } + if (!_altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource)) + { + return new Forbidden(Constants.AltinnAuthLevelToLow); + } + var seenLog = dialog.SeenLog.FirstOrDefault(); if (seenLog is null) { return new EntityNotFound<DialogSeenLog>(request.SeenLogId); } - - if (!_altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource)) - { - return new Forbidden(Constants.AltinnAuthLevelToLow); - }src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs (1)
27-41: LGTM! Consider extracting error handling to a separate method.The changes improve error handling by adding specific error types for authorization failures. The code is now more readable with proper initialization of the response object.
Consider extracting the error handling logic to a separate method for better maintainability:
- forbidden => - { - var response = new DialogByIdPayload(); - - if (forbidden.Reasons.Any(x => x.Contains(Constants.AltinnAuthLevelToLow))) - { - response.Errors.Add(new DialogByIdForbiddenAuthLevelToLow()); - } - else - { - response.Errors.Add(new DialogByIdForbidden()); - } - - return response; - }); + forbidden => HandleForbiddenError(forbidden)); + + private static DialogByIdPayload HandleForbiddenError(Forbidden forbidden) + { + var response = new DialogByIdPayload(); + response.Errors.Add(forbidden.Reasons.Any(x => x.Contains(Constants.AltinnAuthLevelToLow)) + ? new DialogByIdForbiddenAuthLevelToLow() + : new DialogByIdForbidden()); + return response; + }src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (2)
191-198: Consider optimizing the resource policy information query.The current implementation makes a separate database query to fetch resource policy information. Consider including this in the initial dialog query using a join operation for better performance.
- var serviceResources = paginatedList.Items - .Select(x => x.ServiceResource) - .ToList(); - - var resourcePolicyInformation = await _db.ResourcePolicyInformation - .Where(x => serviceResources.Contains(x.Resource)) - .ToDictionaryAsync(x => x.Resource, x => x.MinimumAuthenticationLevel, cancellationToken); + var resourcePolicyInformation = await _db.Dialogs + .Where(x => paginatedList.Items.Select(d => d.ServiceResource).Contains(x.ServiceResource)) + .Join(_db.ResourcePolicyInformation, + d => d.ServiceResource, + r => r.Resource, + (d, r) => new { d.ServiceResource, r.MinimumAuthenticationLevel }) + .Distinct() + .ToDictionaryAsync(x => x.ServiceResource, x => x.MinimumAuthenticationLevel, cancellationToken);
201-204: Improve error handling for missing resource policy information.The code silently continues when resource policy information is not found. Consider logging this scenario as it might indicate a configuration issue.
+ _logger.LogWarning("No resource policy information found for service resource: {ServiceResource}", dialog.ServiceResource); if (!resourcePolicyInformation.TryGetValue(dialog.ServiceResource, out var minimumAuthenticationLevel)) { continue; }src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1)
222-238: Ensure proper validation of non-sensitive content.The non-sensitive properties are used when authentication level is too low, making it crucial to validate that they don't contain any sensitive information. Consider adding validation logic to ensure:
- Content is properly sanitized
- No PII or sensitive data leakage
- Consistent security level across all languages
docs/schema/V1/schema.verified.graphql (1)
144-146: Fix typo in error type name.The error type name has a grammatical error: "ToLow" should be "TooLow".
Apply this diff to fix the type name:
-type DialogByIdForbiddenAuthLevelToLow implements DialogByIdError { +type DialogByIdForbiddenAuthLevelTooLow implements DialogByIdError { message: String! }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250214154726_AddNewNonSensitiveContentTypes.Designer.csis excluded by!**/Migrations/**/*Designer.cssrc/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.csis excluded by!**/Migrations/DialogDbContextModelSnapshot.cs
📒 Files selected for processing (32)
docs/schema/V1/schema.verified.graphql(2 hunks)docs/schema/V1/swagger.verified.json(4 hunks)src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Search/SearchActivityQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/DialogContentExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/DialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Contents/DialogContentType.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogQueries.cs(2 hunks)src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(5 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250214154726_AddNewNonSensitiveContentTypes.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Get/GetDialogActivityEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Search/SearchDialogActivityEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssignmentLogs/Search/SearchDialogLabelAssignmentLogEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Get/GetDialogTransmissionEndpoint.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Search/SearchDialogTransmissionEndpoint.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/ContentTypeTests.cs(3 hunks)tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/ContentTypeTests.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (26)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogActivities/Search/SearchDialogActivityEndpoint.cs (1)
31-32: LGTM! Consistent error handling for forbidden responses.The addition of the forbidden case in the Match function aligns with the PR objectives and maintains consistency across endpoints.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Search/SearchDialogTransmissionEndpoint.cs (1)
31-32: LGTM! Consistent error handling for forbidden responses.The addition of the forbidden case in the Match function aligns with the PR objectives and maintains consistency across endpoints.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20250214154726_AddNewNonSensitiveContentTypes.cs (1)
15-22:❓ Verification inconclusive
LGTM! Migration adds non-sensitive content types correctly.
The migration properly adds two new content types with appropriate constraints. The text/plain media type and 255-character limit are suitable for title and summary fields.
Run this script to verify existing content types and ensure no ID conflicts:
🏁 Script executed:
#!/bin/bash # Description: Check existing content types to ensure no ID conflicts rg -A 10 "table: \"DialogContentType\"" --type csLength of output: 198
Migration Verification: Manual Check Recommended
The migration script accurately adds the two new non-sensitive content types (NonSensitiveTitle and NonSensitiveSummary) with the correct constraints (text/plain media type and a 255-character limit). However, the automated search using ripgrep with a C# file filter did not yield any output. This suggests that the file type filter may have inadvertently excluded files from the search.
Please manually verify that there are no conflicts with existing IDs (7 and 8) in related migration files or any database seeding logic.
src/Digdir.Domain.Dialogporten.GraphQL/ServiceCollectionExtensions.cs (1)
27-27: LGTM! Error type registration aligns with PR objectives.The addition of
DialogByIdForbiddenAuthLevelToLowerror type is consistent with other error type registrations and supports the new auth level enforcement feature.tests/Digdir.Domain.Dialogporten.GraphQl.Unit.Tests/ObjectTypes/ContentTypeTests.cs (1)
14-15: LGTM! Test filtering updated to handle non-sensitive content types.The exclusion of non-sensitive content types from property name matching is applied consistently and maintains test accuracy.
Also applies to: 36-37
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Search/SearchActivityQuery.cs (1)
19-19: LGTM! Auth level check implementation is robust.The changes correctly implement auth level enforcement:
- SearchActivityResult is extended to include Forbidden result
- Auth level check is added in a logical order after basic access checks
- Error message uses a constant for consistency
Also applies to: 65-68
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Contents/DialogContentType.cs (2)
16-18: LGTM! Good addition of non-sensitive content types.The new enum values
NonSensitiveTitleandNonSensitiveSummaryprovide a good way to handle different authentication levels by allowing fallback content when the user's auth level is too low.
72-85: Verify the consistency of non-sensitive content properties.The properties for non-sensitive content types look good, but let's verify they match the intended design:
- They are optional (Required = false) which is correct as they serve as fallbacks
- They use plain text only which is appropriate for non-sensitive content
- They are included in list views (OutputInList = true) which aligns with their purpose
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs (2)
19-19: LGTM! Good addition of Forbidden result type.The addition of
Forbiddento the result types properly handles the new authorization failure case.
59-62: LGTM! Well-placed authorization check.The auth level check is correctly placed after basic authorization and deletion checks, following a logical security flow.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs (1)
21-21: LGTM! Good addition of Forbidden result type.The addition of
Forbiddento the result types properly handles the new authorization failure case.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs (1)
73-76: LGTM! Well-placed authorization check.The auth level check is correctly placed after basic authorization and deletion checks, following a consistent pattern across the codebase.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
72-75: LGTM! Auth check placement is correct.The authorization check is correctly placed after entity existence and deletion checks, following a consistent pattern across handlers.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (2)
20-21: LGTM! The result type is properly updated.The
GetTransmissionResulttype now includesForbiddenas a possible return type, aligning with the new authorization requirements.
75-78: LGTM! Auth level check is properly implemented.The authorization check is added at the right place in the control flow, after confirming dialog existence but before accessing transmission details.
tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/ContentTypeTests.cs (2)
3-9: LGTM! Type aliases are clear and well-organized.The type aliases are properly renamed and organized by their usage (Create, Update, EU, SO), improving code readability.
46-65: LGTM! End-user DTO tests properly handle non-sensitive properties.The tests correctly filter out non-sensitive properties (
NonSensitiveSummaryandNonSensitiveTitle) when testing end-user DTOs, ensuring proper access control.Also applies to: 88-108
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1)
27-30: LGTM! Error type is properly implemented.The new error type
DialogByIdForbiddenAuthLevelToLowcorrectly implements theIDialogByIdErrorinterface and uses the appropriate constant for the error message.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (1)
206-210:❓ Verification inconclusive
Verify the TrySetNonSensitiveContent implementation.
Ensure that the
TrySetNonSensitiveContent()method properly handles cases where non-sensitive content is not provided.
🏁 Script executed:
#!/bin/bash # Search for TrySetNonSensitiveContent implementation ast-grep --pattern 'TrySetNonSensitiveContent() { $$$ }'Length of output: 60
I've noticed that the initial AST search for the implementation of
TrySetNonSensitiveContent()yielded no output. This could mean one of a few things:
- The method might be implemented as an extension method or with a signature that doesn’t match the exact pattern used.
- The definition might reside in a different file or use a different syntax than expected by the AST search.
Action Required:
Please manually verify that theTrySetNonSensitiveContent()method is implemented in a way that gracefully handles cases where non-sensitive content isn’t provided. In particular, check whether:
- The method is defined as an extension method or in an alternative location.
- Its implementation properly defaults or safely ignores missing content without causing errors or unintended behavior.
For further investigation, you might run a broader search for its usage/definition in the repository, for example:
Once you’ve located and reviewed the implementation, please ensure that it adequately handles edge cases such as null or missing non-sensitive content.
🏁 Script executed:
[run_scripts] #!/bin/bash # Search for any occurrence of 'TrySetNonSensitiveContent' across all C# files rg -n "TrySetNonSensitiveContent\(" .Length of output: 161
Action Required: Manual Verification of
TrySetNonSensitiveContentImplementationAutomated searches (both AST and regex-based) did not yield results for
TrySetNonSensitiveContent, which might indicate that its implementation is either defined in a non-standard manner (e.g., as an extension method) or located in an unexpected part of the codebase.Please manually verify the following:
- Implementation Location: Check if
TrySetNonSensitiveContentis implemented as an extension method or under a different signature.- Graceful Handling: Confirm that it appropriately handles cases where non-sensitive content is missing (e.g., by safely defaulting values or avoiding errors).
- Null or Edge Cases: Ensure that any potential null dereference or improper state management is avoided when non-sensitive content is not provided.
docs/schema/V1/swagger.verified.json (7)
2261-2278: LGTM: Added nonSensitiveSummary property with appropriate schema and description.The property is well-defined with:
- Clear description explaining its purpose for search/list views when user auth doesn't meet eIDAS level
- Correct schema reference to V1CommonContent_ContentValue
- Proper nullable flag
2270-2278: LGTM: Added nonSensitiveTitle property with appropriate schema and description.The property is well-defined with:
- Clear description explaining its purpose for search/list views when user auth doesn't meet eIDAS level
- Correct schema reference to V1CommonContent_ContentValue
- Proper nullable flag
2933-2950: LGTM: Added nonSensitiveSummary property to update schema.The property definition in the update schema matches the create schema, maintaining consistency.
2942-2950: LGTM: Added nonSensitiveTitle property to update schema.The property definition in the update schema matches the create schema, maintaining consistency.
3396-3413: LGTM: Added nonSensitiveSummary and nonSensitiveTitle properties to get schema.The properties are consistently defined across create, update and get schemas, ensuring API consistency.
4109-4126: LGTM: Added nonSensitiveSummary and nonSensitiveTitle properties to search schema.The properties are consistently defined across all relevant schemas (create, update, get, search), ensuring complete API coverage.
2261-4126:❓ Verification inconclusive
Verify the impact on existing clients.
The addition of new optional properties should be backward compatible, but it's good practice to verify:
- Existing clients ignore unknown properties
- Documentation is updated to explain the new fields
- Clients are notified about the new functionality
🏁 Script executed:
#!/bin/bash # Description: Check for breaking changes in the API schema # Search for any required/non-nullable usage of the new fields rg -A 5 '"nonSensitiveSummary"|"nonSensitiveTitle"' | grep -E '"required"|"nullable": false' # Search for any validation rules that might affect these fields ast-grep --pattern 'class $_ { $$$ nonSensitive$_($$) { $$$ } $$$ }'Length of output: 177
Verify Backward Compatibility of New Optional Properties
The new fields “nonSensitiveSummary” and “nonSensitiveTitle” are defined as optional (with
"nullable": trueand using theoneOfconstruct), which should make them backward compatible. However, please manually verify the following to ensure there are no unintended impacts on existing clients:
- Confirm that existing clients gracefully ignore these unrecognized fields.
- Verify that the API documentation is updated to explain the purpose and behavior of these new fields.
- Ensure that notifications or release notes have been provided to clients regarding the new functionality.
No breaking changes were detected via static schema searches, but please perform manual verification to confirm these conditions in the client implementations.
...alogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/DialogContentExtensions.cs
Outdated
Show resolved
Hide resolved
...Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDto.cs
Outdated
Show resolved
Hide resolved
...ir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
Outdated
Show resolved
Hide resolved
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs
Outdated
Show resolved
Hide resolved
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
Outdated
Show resolved
Hide resolved
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
Outdated
Show resolved
Hide resolved
...ir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/DialogDto.cs
Outdated
Show resolved
Hide resolved
...ir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/DialogDto.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (2)
191-199: Consider optimizing the resource policy lookup.The current implementation loads all resource policies into memory after the main query. Consider optimizing this by including the resource policies in the main query using a join operation.
Here's a suggested optimization:
var paginatedList = await _db.Dialogs .PrefilterAuthorizedDialogs(authorizedResources) .AsNoTracking() .Include(x => x.Content) .ThenInclude(x => x.Value.Localizations) + .Select(x => new + { + Dialog = x, + MinimumAuthLevel = _db.ResourcePolicyInformation + .Where(p => p.Resource == x.ServiceResource) + .Select(p => p.MinimumAuthenticationLevel) + .FirstOrDefault() + }) // ... rest of the query ... .ProjectTo<IntermediateDialogDto>(_mapper.ConfigurationProvider) .ToPaginatedListAsync(request, cancellationToken: cancellationToken);
202-205: Add logging for missing resource policies.The code silently continues when a resource policy is not found. Consider adding logging to help identify configuration issues.
+private readonly ILogger<SearchDialogQueryHandler> _logger; if (!resourcePolicyInformation.TryGetValue(dialog.ServiceResource, out var minimumAuthenticationLevel)) { + _logger.LogWarning("No resource policy found for service resource {ServiceResource}", dialog.ServiceResource); continue; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/DialogContentExtensions.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/DialogContentExtensions.cs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs (1)
22-22: Consider making the sync method async for consistency.The synchronous
UserHasRequiredAuthLevelmethod might need to be asynchronous to maintain consistency with other interface methods and to avoid potential blocking operations in implementations.-bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel); +Task<bool> UserHasRequiredAuthLevel(int minimumAuthenticationLevel, CancellationToken cancellationToken = default);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(5 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (3)
30-30: LGTM!The field declaration and constructor changes follow good practices:
- Field is marked as readonly for thread safety
- Proper null check is implemented in the constructor
Also applies to: 44-44, 53-53
113-115: Add logging when authentication level cannot be retrieved.The method should log a warning when the authentication level cannot be retrieved from the principal to aid in debugging.
Apply this diff:
- public bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel) => - _user.GetPrincipal().TryGetAuthenticationLevel(out var authLevel) && - minimumAuthenticationLevel <= authLevel; + public bool UserHasRequiredAuthLevel(int minimumAuthenticationLevel) + { + if (!_user.GetPrincipal().TryGetAuthenticationLevel(out var authLevel)) + { + _logger.LogWarning("Could not retrieve authentication level from principal"); + return false; + } + + return minimumAuthenticationLevel <= authLevel; + }
117-125: Add caching for resource policy information.The method queries the database for each authorization check. Consider caching the results to improve performance.
Apply this diff:
public async Task<bool> UserHasRequiredAuthLevel(string serviceResource, CancellationToken cancellationToken) { - var minimumAuthenticationLevel = await _db.ResourcePolicyInformation - .Where(x => x.Resource == serviceResource) - .Select(x => x.MinimumAuthenticationLevel) - .FirstOrDefaultAsync(cancellationToken); + var minimumAuthenticationLevel = await _pdpCache.GetOrSetAsync( + $"resource_policy_{serviceResource}", + async token => await _db.ResourcePolicyInformation + .Where(x => x.Resource == serviceResource) + .Select(x => x.MinimumAuthenticationLevel) + .FirstOrDefaultAsync(token), + token: cancellationToken); return UserHasRequiredAuthLevel(minimumAuthenticationLevel); }
...Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
72-75: Fix spelling in the constant name while keeping the secure implementation.The authorization check is well-placed after the deletion check to prevent information leakage. However, there appears to be a spelling error in the constant name.
The constant
AltinnAuthLevelToLowshould be renamed toAltinnAuthLevelTooLow(note the double 'o').src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (2)
75-78: Consider moving the auth level check earlier.The auth level check could be moved before the deletion check to optimize performance by failing fast. This would prevent unnecessary database operations for users with insufficient auth levels.
if (!authorizationResult.HasAccessToMainResource()) { return new EntityNotFound<DialogEntity>(request.DialogId); } + if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) + { + return new Forbidden(Constants.AltinnAuthLevelToLow); + } + if (dialog.Deleted) { return new EntityDeleted<DialogEntity>(request.DialogId); } - - if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) - { - return new Forbidden(Constants.AltinnAuthLevelToLow); - }
77-77: Consider localizing the error message.The error message
Constants.AltinnAuthLevelToLowshould be localized to support multiple languages.tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (1)
40-53: Add a comment explaining the precedence rule.The test verifies that Altinn authentication level takes precedence over ID-porten level when both claims are present. Consider adding a comment to document this important behavior.
[Fact] public void GetAuthenticationLevel_Should_Parse_Altinn_Authlevel_First() { + // When both Altinn and ID-porten authentication level claims are present, + // the Altinn level should take precedence // Arrange var claimsPrincipal = new ClaimsPrincipal(new ClaimsIdentity([src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs (1)
179-199: Consider using a more specific exception for missing claims.The method has been improved by:
- Returning the authentication level directly instead of using an out parameter
- Using constants for ID-porten LOA values
- Adding explicit error handling
However,
UnreachableExceptionmight not be the best choice for missing claims as it suggests a code path that should never be reached, while missing claims could be a valid runtime scenario.Consider using a more appropriate exception type:
- throw new UnreachableException("No authentication level claim found"); + throw new ArgumentException("No authentication level claim found", nameof(claimsPrincipal));src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs (1)
71-74: Verify authorization check placement.The authorization check is correctly implemented, but consider moving it before the deletion check to fail fast and maintain consistency with the existing authorization check at line 61.
Apply this diff to reorder the checks:
if (!authorizationResult.HasAccessToMainResource()) { return new EntityNotFound<DialogEntity>(request.DialogId); } + if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) + { + return new Forbidden(Constants.AltinnAuthLevelToLow); + } + if (dialog.Deleted) { return new EntityDeleted<DialogEntity>(request.DialogId); } - if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) - { - return new Forbidden(Constants.AltinnAuthLevelToLow); - }src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (1)
81-84: Verify authorization check placement.The authorization check is correctly implemented, but consider moving it before the seen log retrieval to fail fast and maintain consistency with the existing authorization check at line 65.
Apply this diff to reorder the checks:
if (!authorizationResult.HasAccessToMainResource()) { return new EntityNotFound<DialogEntity>(request.DialogId); } if (dialog.Deleted) { return new EntityDeleted<DialogEntity>(request.DialogId); } + if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) + { + return new Forbidden(Constants.AltinnAuthLevelToLow); + } + var seenLog = dialog.SeenLog.FirstOrDefault(); if (seenLog is null) { return new EntityNotFound<DialogSeenLog>(request.SeenLogId); } - if (!await _altinnAuthorization.UserHasRequiredAuthLevel(dialog.ServiceResource, cancellationToken)) - { - return new Forbidden(Constants.AltinnAuthLevelToLow); - }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/schema/V1/swagger.verified.json(4 hunks)src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Common/Extensions/ClaimsPrincipalExtensions.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Search/SearchActivityQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/DialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDto.cs(1 hunks)src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs(5 hunks)tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/DialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Search/SearchActivityQuery.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchLabelAssignmentLogQuery.cs
- src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/DialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Search/SearchSeenLogQuery.cs
- docs/schema/V1/swagger.verified.json
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy job sync-resource-policy-information-job to test
- GitHub Check: Dry run deploy apps / Deploy job sync-subject-resource-mappings-job to test
🔇 Additional comments (8)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchTransmissionQuery.cs (1)
19-19: LGTM! Good addition of theForbiddenresult type.The explicit modeling of authorization failures through the OneOf pattern enables proper type-safe error handling at the API level.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetTransmissionQuery.cs (1)
21-21: LGTM! Good addition of the Forbidden result type.The addition of
Forbiddento the OneOf base types properly supports the new authorization flow.tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/Common/Extensions/ClaimsPrincipalExtensionsTests.cs (2)
10-22: LGTM! Improved test maintainability.The test has been improved by using constants and simplifying the assertions.
25-37: LGTM! Consistent test improvements.The test follows the same pattern of improvements as the Level 3 test.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogActivities/Queries/Get/GetActivityQuery.cs (2)
2-2: LGTM!The import is correctly added and necessary for using the authorization constants.
21-21: LGTM!The
Forbiddentype is correctly added to handle authorization failures, aligning with the PR objectives.src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSeenLogs/Queries/Get/GetSeenLogQuery.cs (1)
3-3: LGTM!The import is correctly added and necessary for using the authorization constants.
src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs (1)
53-53:❓ Verification inconclusive
LGTM! Verify error handling in upstream code.
The change to use
GetAuthenticationLevel()directly improves authentication level handling by being more strict and explicit about failures. This aligns well with the PR's objective of enforcing minimum authentication level requirements.Please ensure that upstream code properly handles the potential exceptions from
GetAuthenticationLevel():
ArgumentExceptionfor unknown ACR valuesUnreachableExceptionfor missing authentication level claims
🏁 Script executed:
#!/bin/bash # Search for error handling of GetAuthenticationLevel exceptions rg -A 5 "GetAuthenticationLevel" | rg -B 5 "try|catch"Length of output: 54
LGTM – Ensure Upstream Exception Handling is Verified
The updated code in
src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.csnow callsGetAuthenticationLevel()directly, which enforces stricter error signaling for cases like unknown ACR values or missing authentication level claims. However, our initial shell script didn’t reveal any explicit try/catch blocks handling the potentialArgumentExceptionorUnreachableException.Please manually verify that any upstream code which invokes
GetAuthenticationLevel()properly catches and manages these exceptions. This confirmation will ensure that the overall error handling strategy remains robust.
oskogstad
deleted the
feat/enforce-auth-level-requirements-on-dialogs
branch
🤖 I have created a release *beep* *boop* --- ## [1.53.0](v1.52.0...v1.53.0) (2025-02-19) ### Features * Enforce minimum auth level requirements on dialogs ([#1875](#1875)) ([37febf6](37febf6)) ### Bug Fixes * **graphql:** Typo in auth level error type name ([#1904](#1904)) ([b3d9ad8](b3d9ad8)) * Return new revision ETag on system label update ([#1903](#1903)) ([2e763cd](2e763cd)) ### Miscellaneous Chores * **deps:** update dependency testcontainers.postgresql to 4.2.0 ([#1908](#1908)) ([9f76f69](9f76f69)) * **deps:** update jaegertracing/all-in-one docker tag to v1.66.0 ([#1910](#1910)) ([29877d7](29877d7)) * **deps:** update microsoft dependencies ([#1907](#1907)) ([c916575](c916575)) * updated TypeNameConverter ([#1900](#1900)) ([f68c112](f68c112)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)