Skip to content

Aggregate document permissions for current user in API response#18721

Merged
Zeegaan merged 4 commits intov15/devfrom
v15/feature/aggregate-document-permissions-for-current-user
Apr 9, 2025
Merged

Aggregate document permissions for current user in API response#18721
Zeegaan merged 4 commits intov15/devfrom
v15/feature/aggregate-document-permissions-for-current-user

Conversation

@AndyButland
Copy link
Contributor

@AndyButland AndyButland commented Mar 18, 2025

Prerequisites

  • I have added steps to test this contribution in the description below

Goes toward fixing #18517

Description

Users belong to one or more user groups, user groups have various default permissions, and these can be overridden on a per group, per document basis, with the net result inheriting to descendants. There's some fairly involved server side logic to figure out the result when checking for access for a given user and document.

With 13, both the decision of what UI elements to expose and server-side authorization used the same logic, server-side. With 14+ we have a split, with the latter remaining on the server but the presentation of features to users being decided on the client.

Currently the API response for the current user exposes all the raw information: the groups the user is in, the net of the default permissions from the user groups, and the individual document granular permissions. So in theory the client could do the same aggregation as is done on the server. That's not ideal though - means duplicating the logic and it's not currently aligned, hence the reported issue.

So this PR amends the output of the granular permissions per document to expose not the raw information, but the aggregated details - leading to one record per document with the net permissions obtained using the existing server-side logic.

This means that the client can use this directly, and likely needs to just amend the existing implementation such that it looks for document granular permissions not just on the current document, but also on the closest ancestor.

To Test:

  • Review the provided integration test.
  • Manually you can set granular permissions on user groups and check the results of the umbraco/management/api/v1/user/current to verify the expected aggregated results under the permissions element.

Things to consider:

  • I've replaced the existing permissions element here. We could consider keeping this as is, and providing a new element for the aggregatedPermissions.
    • But if we have the aggregated data, we likely don't need the raw records on the client, so that's why I've replaced rather than added a new element.
  • If we have granular permissions per document, the affected endpoint has got more expensive to create, as we need to retrieve the path for each document, which requires retrieving it from the content service.
    • Given we generally expect few or no granular permissions, and if the current request is not requested too often from the client, considering that this should be OK.

@AndyButland AndyButland requested a review from Migaroez March 18, 2025 17:14
…s-for-current-user

# Conflicts:
#	tests/Umbraco.Tests.Common/Builders/UserGroupBuilder.cs
Copy link
Member

@Zeegaan Zeegaan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, tests good 🚀

@Zeegaan Zeegaan merged commit a0e3ca6 into v15/dev Apr 9, 2025
20 of 21 checks passed
@Zeegaan Zeegaan deleted the v15/feature/aggregate-document-permissions-for-current-user branch April 9, 2025 08:57
nielslyngsoe pushed a commit that referenced this pull request Apr 9, 2025
* Create integration test verifying existing behaviour.

* Aggregate permissions per document for the current user response.

* Refactoring following Codescene warnings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants