chore: Consolidate all scopes to one location with xml doc. #1864
Conversation
📝 WalkthroughWalkthroughThis pull request restructures how authorization scopes are managed across the application. A new static class, AuthorizationScope, is introduced in the Application layer to encapsulate scope constants and a lazy-loaded collection of all scopes. Existing references to scope constants in various components—including application logic, endpoint summaries, middleware, and tests—have been updated to use the new class. Additionally, several using directives have been refactored, replacing references from the WebApi layer with those from the Application layer. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (2)
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs (1)
23-79: Comprehensive consolidation of scope constants
Excellent approach to centralizing all scope constants and providing XML documentation. This significantly improves maintainability.
- For line 57 (
/// ???), consider completing the XML summary to clarify its usage and rationale.- The
GetAll()method correctly uses reflection to gather constants, though be mindful that it will include any future string constants as well.src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs (1)
1-1: Removal of old AuthorizationScope from this file
Removing the duplicatedAuthorizationScopedefinitions helps avoid confusion. However, it might be worth considering if the constants inAuthorizationPolicyshould also be migrated into the newAuthorizationScopeclass if they represent overlapping functionalities.src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Get/GetDialogTransmissionEndpointSummary.cs (1)
1-1: Consider adding XML documentation for the TBD link.The endpoint summary mentions "documentation (link TBD)". Since this PR focuses on improving documentation through XML docs, consider adding the actual documentation link.
Also applies to: 4-4, 17-17
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Get/GetDialogEndpointSummary.cs (2)
1-1: Consider adding XML documentation for the TBD link.Similar to other endpoint summaries, the documentation link is marked as TBD. Consider adding the actual documentation link as part of this documentation improvement effort.
Also applies to: 4-4, 13-16
20-22: Consider improving code formatting.The string formatting for the unauthorized response message is split across multiple lines. Consider consolidating it for better readability:
- Responses[StatusCodes.Status401Unauthorized] = - Constants.SwaggerSummary.ServiceOwnerAuthenticationFailure.FormatInvariant(AuthorizationScope - .ServiceProvider); + Responses[StatusCodes.Status401Unauthorized] = Constants.SwaggerSummary.ServiceOwnerAuthenticationFailure + .FormatInvariant(AuthorizationScope.ServiceProvider);src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Search/SearchDialogTransmissionEndpointSummary.cs (1)
1-1: LGTM! Successfully completes scope consolidation.The changes complete the consistent pattern of moving authorization scopes to the Application layer across all endpoint summaries.
This refactoring improves the codebase by:
- Centralizing all authorization scopes in one location
- Making scope management more maintainable
- Establishing a clear separation of concerns between layers
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
15-15: Update documentation link placeholder.Consider replacing the "TBD" placeholder with the actual documentation link if available.
Also applies to: 15-15
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Common/IUserResourceRegistry.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/LocalDevelopmentUser.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationPolicy.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/ServiceOwnerOnBehalfOfPersonMiddleware.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/LocalDevelopmentUser.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Get/GetDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Search/SearchDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Get/GetDialogActivitySwaggerConfig.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/NotificationCondition/SearchDialogActivityEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Search/SearchDialogActivityEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Get/GetDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Search/SearchDialogTransmissionEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Get/GetDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Purge/PurgeDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Search/ListDialogEndpointSummary.cs(1 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs(4 hunks)
✅ Files skipped from review due to trivial changes (9)
- src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationOptionsSetup.cs
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Search/ListDialogEndpointSummary.cs
- src/Digdir.Domain.Dialogporten.GraphQL/Common/Authorization/AuthorizationOptionsSetup.cs
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs
- src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Create/CreateDialogActivityEndpointSummary.cs
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Create/CreateDialogEndpointSummary.cs
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogTransmissions/Search/SearchDialogTransmissionEndpointSummary.cs
- src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Get/GetDialogActivitySwaggerConfig.cs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Dry run deploy apps / Deploy web-api-so to test
- GitHub Check: Dry run deploy apps / Deploy web-api-eu to test
- GitHub Check: build / build-and-test
🔇 Additional comments (15)
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs (1)
2-2: Import statement looks fine
No issues here.src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (1)
99-99: Adaptation to new LegacyHtmlScope constant
The update to useAuthorizationScope.LegacyHtmlScopeis correct, and the null check onuserprevents potential runtime errors.src/Digdir.Domain.Dialogporten.WebApi/Common/LocalDevelopmentUser.cs (1)
3-3: LGTM! Namespace update aligns with scope consolidation.The change correctly updates the authorization namespace to use the consolidated scopes from the Application layer.
Also applies to: 14-14
src/Digdir.Domain.Dialogporten.GraphQL/Common/Authentication/LocalDevelopmentUser.cs (1)
2-2: LGTM! Consistent with WebApi layer changes.The changes maintain consistency with the WebApi layer by using the same consolidated scopes from the Application layer.
Also applies to: 14-14
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Get/GetDialogTransmissionEndpointSummary.cs (1)
1-1: LGTM! Changes align with scope consolidation.The refactoring of using directives correctly moves authorization scope references to the Application layer while maintaining access to WebApi constants through an alias.
Run this script to verify that all authorization scope references are now from the Application layer:
Also applies to: 4-4
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Description: Verify that no WebApi authorization scopes are used in endpoint summaries # Test: Search for any remaining references to WebApi authorization rg -l "Digdir\.Domain\.Dialogporten\.WebApi\.Common\.Authorization" "src/Digdir.Domain.Dialogporten.WebApi/Endpoints"Length of output: 3320
Authorization Directive Updated in Summary Endpoint – Further Verification Needed
- The file
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Get/GetDialogTransmissionEndpointSummary.cscorrectly uses the newDigdir.Domain.Dialogporten.Application.Common.Authorizationdirective, with no trace of the old WebApi dependency.- However, the verify script shows that several other endpoint files still reference
Digdir.Domain.Dialogporten.WebApi.Common.Authorization. Please confirm if the consolidation is intended to affect only the summary endpoint (and any similar “4-4” endpoints) or if the remaining endpoints should also be updated to use the Application layer.src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/NotificationCondition/SearchDialogActivityEndpointSummary.cs (1)
1-1: LGTM! Consistent with scope consolidation pattern.The changes correctly implement the new authorization scope structure, using the appropriate NotificationConditionCheck scope from the Application layer.
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/Search/SearchDialogActivityEndpointSummary.cs (1)
1-1: LGTM! Maintains consistency with scope consolidation.The changes follow the established pattern of using Application layer scopes, correctly using ServiceProvider scope for authentication checks.
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Purge/PurgeDialogEndpointSummary.cs (1)
1-1: LGTM! Changes align with scope consolidation.The namespace changes correctly move authorization scopes to the Application layer while maintaining access to other constants via the alias.
Also applies to: 4-4
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Update/UpdateDialogEndpointSummary.cs (1)
1-1: LGTM! Consistent with scope consolidation pattern.The changes maintain the same pattern of moving authorization scopes to the Application layer while preserving access to other constants.
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogTransmissions/Create/CreateDialogTransmissionEndpointSummary.cs (1)
1-1: LGTM! Follows the established pattern.The namespace changes are consistent with the scope consolidation effort across the codebase.
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/Dialogs/Delete/DeleteDialogEndpointSummary.cs (1)
1-1: LGTM! Consistent implementation.The changes align with the scope consolidation pattern seen across other files.
Also applies to: 5-5
src/Digdir.Domain.Dialogporten.WebApi/Common/Authentication/ServiceOwnerOnBehalfOfPersonMiddleware.cs (2)
2-2: LGTM! Using directive aligns with scope consolidation.The change is consistent with the PR objective to consolidate all scopes into a single location.
27-27: LGTM! Scope check uses consolidated scope constant.The change maintains the same functionality while using the new centralized scope constant.
src/Digdir.Domain.Dialogporten.Application/Common/IUserResourceRegistry.cs (2)
47-47: LGTM! Scope check uses consolidated scope constant.The change maintains the same functionality while using the new centralized scope constant.
52-52: LGTM! Scope check uses consolidated scope constant.The change maintains the same functionality while using the new centralized scope constant.
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs
Outdated
Show resolved
Hide resolved
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.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/Common/Authorization/Constants.cs (2)
41-44: Consider adding security implications to the ServiceOwnerAdminScope documentation.The ServiceOwnerAdminScope is a highly privileged scope that grants access to all service owners' dialogs. Consider adding security implications and usage guidelines to the documentation.
/// <summary> - /// Allows the modification (create/update/delete) of dialogs on behalf of all service owners regardless of the authenticated user. + /// Allows the modification (create/update/delete) of dialogs on behalf of all service owners regardless of the authenticated user. + /// SECURITY: This is a highly privileged scope that should be granted with caution and only to trusted administrative applications. + /// Usage should be monitored and audited. /// </summary>
71-79: Consider enhancing the GetAll method's robustness.While the current implementation is clean, consider adding null checks and error handling to make it more robust.
private static ReadOnlyCollection<string> GetAll() => typeof(AuthorizationScope) .GetFields() - .Where(x => x.IsLiteral && !x.IsInitOnly && x.FieldType == typeof(string)) - .Select(x => (string)x.GetRawConstantValue()!) + .Where(x => x is not null && x.IsLiteral && !x.IsInitOnly && x.FieldType == typeof(string)) + .Select(x => x.GetRawConstantValue() as string) + .Where(x => x is not null) .ToList() .AsReadOnly();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs(2 hunks)src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Digdir.Domain.Dialogporten.WebApi/Common/Authorization/AuthorizationPolicy.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy web-api-eu to test
- GitHub Check: build / build-and-test
🔇 Additional comments (3)
src/Digdir.Domain.Dialogporten.Application/Common/Authorization/Constants.cs (3)
6-22: LGTM! Good separation of concerns.The
Constantsclass maintains a clear separation between authorization scopes and other constants, with well-organized and appropriately named members.
24-25: LGTM! Well-structured authorization scope container.The static class follows the single responsibility principle by focusing solely on authorization scopes.
61-64: Consider aligning the NotificationConditionCheck scope naming pattern.The
NotificationConditionCheckscope uses a different naming pattern (altinn:system/*) compared to other scopes (digdir:dialogporten.*). Consider documenting the reason for this difference or aligning it with the standard pattern if possible.Could you explain why this scope follows a different naming pattern? This might help other developers understand the design decision.
MagnusSandgren
deleted the
chore/refactor-authorization-scope-constants
branch
🤖 I have created a release *beep* *boop* --- ## [1.50.3](v1.50.2...v1.50.3) (2025-02-12) ### Miscellaneous Chores * **ci:** Purge Application Insights data older than 30 days in yt01 ([#1863](#1863)) ([70931cf](70931cf)) * Consolidate all scopes to one location with xml doc. ([#1864](#1864)) ([f8cdb36](f8cdb36)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Forsøker meg i første omgang på å samle alle scopene vi bruker i dialogporten slik at det er lett å få en oversikt over dem. @knuhau og jeg forsøkte oss også på å komme med noen summaries for hvert scope. Her tar vi veldig gjerne imot innspill på en bedre forklarende tekst @oskogstad @elsand 🙏
Related issue #647