Skip to content

Commit

Permalink
fix: Fallback to using list auth if details auth fails, remove double…
Browse files Browse the repository at this point in the history
… 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)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## 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.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
  • Loading branch information
elsand and coderabbitai[bot] authored Oct 14, 2024
1 parent 7267546 commit 54425e7
Show file tree
Hide file tree
Showing 6 changed files with 37 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ public Task<DialogSearchAuthorizationResult> GetAuthorizedResourcesForSearch(

public Task<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool flatten = false,
CancellationToken cancellationToken = default);

Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken);
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,19 @@ public async Task<GetDialogResult> Handle(GetDialogQuery request, CancellationTo

if (!authorizationResult.HasAccessToMainResource())
{
return new EntityNotFound<DialogEntity>(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<DialogEntity>(request.DialogId);
}
}

if (dialog.Deleted)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ public async Task<DialogSearchAuthorizationResult> 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<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool flatten = false,
Expand All @@ -87,6 +88,16 @@ public async Task<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier
return flatten ? GetFlattenedAuthorizedParties(authorizedParties) : authorizedParties;
}

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);
}

private static AuthorizedPartiesResult GetFlattenedAuthorizedParties(AuthorizedPartiesResult authorizedParties)
{
var flattenedAuthorizedParties = new AuthorizedPartiesResult();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,6 @@ public async Task<DialogSearchAuthorizationResult> GetAuthorizedResourcesForSear
[SuppressMessage("Performance", "CA1822:Mark members as static")]
public async Task<AuthorizedPartiesResult> GetAuthorizedParties(IPartyIdentifier authenticatedParty, bool _ = false, CancellationToken __ = default)
=> await Task.FromResult(new AuthorizedPartiesResult { AuthorizedParties = [new() { Name = "Local Party", Party = authenticatedParty.FullId, IsCurrentEndUser = true }] });

public Task<bool> HasListAuthorizationForDialog(DialogEntity dialog, CancellationToken cancellationToken) => Task.FromResult(true);
}
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit 54425e7

Please sign in to comment.