fix: Fallback to using list auth if details auth fails, remove double cache #1274
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request implements several changes to enhance authorization handling within the Dialogporten application. Key modifications include the addition of a method for checking list authorization in the Changes
Possibly related issues
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
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: 3
🧹 Outside diff range and nitpick comments (7)
src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs (1)
20-20: Approve the addition with a minor suggestion.The new
HasListAuthorizationForDialogmethod is a valuable addition that aligns well with the PR objectives. It provides a clear way to check for list authorization, which is crucial for implementing the fallback mechanism described in the PR summary.The method signature is consistent with other methods in the interface, following the async pattern and including a
CancellationToken.Consider making the
CancellationTokenparameter optional by providing a default value:- Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken); + Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken = default);This change would make it consistent with the
GetDialogDetailsAuthorizationmethod and provide more flexibility when calling the method.src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (3)
59-59: LGTM with a minor suggestion.The changes to the
GetAuthorizedPartiesmethod look good. The new syntax for initializingAuthorizedPartiesResultis more concise and readable.Consider adding a comment explaining the purpose of the unused
bool _parameter, or use the discard pattern_instead of naming it if it's intentionally unused:-public async Task<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool _ = false, CancellationToken __ = default) +public async Task<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool _, CancellationToken __ = default)
60-60: LGTM with a suggestion for improved documentation.The addition of the
HasListAuthorizationForDialogmethod is appropriate for this local development implementation.Consider adding a comment to explain that this method always returns true for local development purposes:
- public Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken) => Task.FromResult(true); + // Always allow list authorization in local development + public Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken) => Task.FromResult(true);
59-60: Changes align well with PR objectives.The modifications to
GetAuthorizedPartiesand the addition ofHasListAuthorizationForDialogare consistent with the PR's goal of implementing a fallback mechanism for authorization. These changes in the local development implementation provide a good foundation for testing the new authorization flow without restrictions.Ensure that the production implementation of
IAltinnAuthorizationproperly handles the fallback logic as described in the PR objectives, particularly when details authorization fails.src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs (1)
77-81: Approve changes with a minor optimization suggestion.The addition of an early return for empty
AuthorizedAltinnActionsis a good optimization. It aligns with the PR objectives by handling cases where a user might not have any authorized actions, which could occur when falling back to list authorization.To further optimize, consider using
string.Emptyinstead of"":if (authorizationResult.AuthorizedAltinnActions.Count == 0) { - return ""; + return string.Empty; }This change is minor but adheres to C# best practices for using
string.Emptyinstead of empty string literals.src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2)
76-78: LGTM! Consider clarifying the comment slightly.The removal of caching at this level is appropriate and aligns with the PR objective of removing double caching. The direct call to
PerformDialogSearchAuthorizationis correct.Consider slightly rewording the comment for clarity:
- // We don't cache at this level, as the principal information is received from GetAuthorizedParties, - // which is already cached + // We don't cache at this level because the principal information comes from GetAuthorizedParties, + // which is already cached
91-100: LGTM! Consider a minor optimization.The new
HasListAuthorizationForDialogmethod is well-implemented and aligns perfectly with the PR objective. It correctly checks for list authorization using theGetAuthorizedResourcesForSearchmethod and covers all possible cases of authorization.Consider a minor optimization to short-circuit the evaluation:
public async Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken) { var authorizedResourcesForSearch = await GetAuthorizedResourcesForSearch( [dialog.Party], [dialog.ServiceResource], cancellationToken); - return authorizedResourcesForSearch.ResourcesByParties.Count > 0 - || authorizedResourcesForSearch.SubjectsByParties.Count > 0 - || authorizedResourcesForSearch.DialogIds.Contains(dialog.Id); + return authorizedResourcesForSearch.ResourcesByParties.Count > 0 + || authorizedResourcesForSearch.SubjectsByParties.Count > 0 + || (authorizedResourcesForSearch.DialogIds?.Contains(dialog.Id) ?? false); }This change ensures that
DialogIdsis not null before callingContains, which could prevent a potentialNullReferenceException.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
- src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (1 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
Learnt from: oskogstad PR: digdir/dialogporten#1210 File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282 Timestamp: 2024-10-08T15:39:24.789Z Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.Learnt from: oskogstad PR: digdir/dialogporten#1210 File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282 Timestamp: 2024-10-01T17:03:49.291Z Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.
🔇 Additional comments (1)
src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs (1)
Line range hint
1-100: Summary: Changes align well with PR objectivesThe modifications in this file successfully implement the fallback mechanism using list authorization and remove double caching as intended. The new
HasListAuthorizationForDialogmethod provides the necessary functionality for the fallback mechanism, ensuring that dialogs visible in the list can be accessed in the details view, even if the user lacks authorization for specific actions.These changes directly address the issues raised in the related issue (#1247) regarding the inconsistency between list and details authorization. The implementation is correct, comprehensive, and aligns perfectly with the PR objectives.
...ir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs
Show resolved
Hide resolved
...ir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs
Outdated
Show resolved
Hide resolved
...Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs
Show resolved
Hide resolved
elsand
changed the title
…/Dialogs/Queries/Get/GetDialogQuery.cs Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Description
This implements a fall back to using list authorization if the details authorization returns without access to the main resource. This might happen if
This ensures that dialogs that is visible in the list, also can be viewed in details view, even if the user has isn't authorized for any actions. He/she might still have access to transmissions using authorization attributes (depending on if the authorization attribute refers a subresource or external resource; either having "transmissionread" in the ServiceResource policy, or having "read" on the external resource policy)
Also, this removes a redundant double caching of list authorization. This was a leftover after the non-scalable PDP-based authorization.
Related Issue(s)
This adresses the principal problem raised in #1247, which is the discrepancy between perceived list and details authorization. We still need to consider if GetAltinnActions should be policy-based, as that will allow us to implement action-property validation in Create/Update commands. This will also let us include all authorized actions in dialog tokens in the
a(actions) claim, not just the actions referred to in the dialog.Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Chores