From 54425e76ecaf3d8cedd06aaa30506b59de019da3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B8rn=20Dybvik=20Langfors?= Date: Mon, 14 Oct 2024 16:49:16 +0200 Subject: [PATCH] fix: Fallback to using list auth if details auth fails, remove double cache (#1274) ## 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 - The XACML policy doesn't define a "read" rule - There are no GUI/API actions in the dialog referring to XACML actions the user is granted access to 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) - #1247 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 - [x] **Your** code builds clean without any errors or warnings - [x] Manual testing done (required) - [ ] Relevant automated test added (if you find this hard, leave it and we'll help out) ## Documentation - [ ] Documentation is updated (either in `docs`-directory, Altinnpedia or a separate linked PR in [altinn-studio-docs.](https://github.com/Altinn/altinn-studio-docs), if applicable) ## Summary by CodeRabbit - **New Features** - Introduced a new method to check list authorization for dialogs, enhancing user access control. - Added additional authorization checks for dialogs, allowing users with list access to retrieve dialogs even without main resource access. - **Bug Fixes** - Improved error handling and validation in dialog creation tests, ensuring robust and localized feedback. - **Chores** - Updated caching strategy for search authorization results to improve performance. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --- .../Common/IDialogTokenGenerator.cs | 5 +++++ .../AltinnAuthorization/IAltinnAuthorization.cs | 2 ++ .../EndUser/Dialogs/Queries/Get/GetDialogQuery.cs | 14 +++++++++++++- .../Authorization/AltinnAuthorizationClient.cs | 15 +++++++++++++-- .../LocalDevelopmentAltinnAuthorization.cs | 2 ++ .../Dialogs/Commands/CreateDialogTests.cs | 3 ++- 6 files changed, 37 insertions(+), 4 deletions(-) diff --git a/src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs b/src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs index f6234b31f..b14d22402 100644 --- a/src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs +++ b/src/Digdir.Domain.Dialogporten.Application/Common/IDialogTokenGenerator.cs @@ -74,6 +74,11 @@ public string GetDialogToken(DialogEntity dialog, DialogDetailsAuthorizationResu private static string GetAuthorizedActions(DialogDetailsAuthorizationResult authorizationResult) { + if (authorizationResult.AuthorizedAltinnActions.Count == 0) + { + return string.Empty; + } + var actions = new StringBuilder(); foreach (var (action, resource) in authorizationResult.AuthorizedAltinnActions) { diff --git a/src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs b/src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs index 601edd44a..c75b7360f 100644 --- a/src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs +++ b/src/Digdir.Domain.Dialogporten.Application/Externals/AltinnAuthorization/IAltinnAuthorization.cs @@ -16,4 +16,6 @@ public Task GetAuthorizedResourcesForSearch( public Task GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool flatten = false, CancellationToken cancellationToken = default); + + Task HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken); } diff --git a/src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs b/src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs index e2a9267f5..2889d9e60 100644 --- a/src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs +++ b/src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs @@ -98,7 +98,19 @@ public async Task Handle(GetDialogQuery request, CancellationTo if (!authorizationResult.HasAccessToMainResource()) { - return new EntityNotFound(request.DialogId); + // If the user for some reason does not have access to the main resource, which might be + // because they are granted access to XACML-actions besides "read" not explicitly defined in the dialog, + // we do a recheck if the user has access to the dialog via the list authorization. If this is the case, + // we return the dialog and let DecorateWithAuthorization flag the actions as unauthorized. Note that + // there might be transmissions that the user has access to, even though there are no authorized actions. + var listAuthorizationResult = await _altinnAuthorization.HasListAuthorizationForDialog( + dialog, + cancellationToken: cancellationToken); + + if (!listAuthorizationResult) + { + return new EntityNotFound(request.DialogId); + } } if (dialog.Deleted) diff --git a/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs b/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs index 16aeb8978..3d127348e 100644 --- a/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs +++ b/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/AltinnAuthorizationClient.cs @@ -73,8 +73,9 @@ public async Task GetAuthorizedResourcesForSear ConstraintServiceResources = serviceResources }; - return await _pdpCache.GetOrSetAsync(request.GenerateCacheKey(), async token - => await PerformDialogSearchAuthorization(request, token), token: cancellationToken); + // We don't cache at this level, as the principal information is received from GetAuthorizedParties, + // which is already cached + return await PerformDialogSearchAuthorization(request, cancellationToken); } public async Task GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool flatten = false, @@ -87,6 +88,16 @@ public async Task GetAuthorizedParties(IPartyIdentifier return flatten ? GetFlattenedAuthorizedParties(authorizedParties) : authorizedParties; } + public async Task 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); + } + private static AuthorizedPartiesResult GetFlattenedAuthorizedParties(AuthorizedPartiesResult authorizedParties) { var flattenedAuthorizedParties = new AuthorizedPartiesResult(); diff --git a/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs b/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs index 2957f7714..0a9b51751 100644 --- a/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs +++ b/src/Digdir.Domain.Dialogporten.Infrastructure/Altinn/Authorization/LocalDevelopmentAltinnAuthorization.cs @@ -56,4 +56,6 @@ public async Task GetAuthorizedResourcesForSear [SuppressMessage("Performance", "CA1822:Mark members as static")] public async Task GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool _ = false, CancellationToken __ = default) => await Task.FromResult(new AuthorizedPartiesResult { AuthorizedParties = [new() { Name = "Local Party", Party = authenticatedParty.FullId, IsCurrentEndUser = true }] }); + + public Task HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken) => Task.FromResult(true); } diff --git a/tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs b/tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs index 54f08226b..134f9270c 100644 --- a/tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs +++ b/tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs @@ -228,7 +228,8 @@ public async Task Cannot_Create_Transmission_Without_Content() response.TryPickT2(out var validationError, out _).Should().BeTrue(); validationError.Should().NotBeNull(); validationError.Errors.Should().HaveCount(1); - validationError.Errors.First().ErrorMessage.Should().Contain("'Content' must not be empty"); + // The error message might be localized, so we just check for the property name + validationError.Errors.First().ErrorMessage.Should().Contain("'Content'"); } [Fact]