Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Amend user start node handling #16094

Merged
merged 10 commits into from
May 3, 2024
Merged

Conversation

kjac
Copy link
Contributor

@kjac kjac commented Apr 18, 2024

Prerequisites

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

🚨 Please coordinate the merge of this PR with the client 🚨

The matching client PR is here: umbraco/Umbraco.CMS.Backoffice#1727

Description

A long time ago we hardcoded tree root access, because we did not have user permissions properly wired up. We do now 😄 so now it's time to enforce it.

Another thing missing is the ability to define root access at user level in V14, just as we can in V13. It is currently possible at group level in V14, but not yet at user level.

The last piece of this particular puzzle is making sure we don't request the recycle bins if the current user does not have root access. This will yield a 401, which will trigger a "please log in again" dialog --- and 'round and 'round we go. In other words, the "current user" model needs to be enriched with "has root access" to document and media, respectively. The client can then use this to hide the recycle bin when applicable.

Testing this PR

It's probably best to test this in Swagger. The initial setup of users and groups can be done in the V14 UI, though.

  1. Users without root access should not be allowed to request the recycle bin endpoints.
  2. It should be possible to define root access at user level.
  3. The current user's calculated root access should be exposed correctly through the "current user" endpoint.
  4. Users with root access should not experience any changes by this PR.

@kjac kjac marked this pull request as ready for review April 30, 2024 07:21
Copy link
Contributor

@Migaroez Migaroez left a comment

Choose a reason for hiding this comment

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

I think the integration tests can be improved as there is some multi assert smell present.

Comment on lines +22 to +24
IEntitySlim[] userStartEntities = userStartNodeIds.Any()
? _entityService.GetAll(umbracoObjectType, userStartNodeIds).ToArray()
: Array.Empty<IEntitySlim>();
Copy link
Contributor

Choose a reason for hiding this comment

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

🥰

@Migaroez
Copy link
Contributor

Migaroez commented May 1, 2024

Bit of a strange message when trying the recycle bin root endpoint

  "type": "Error",
  "title": "Unauthorized user",
  "status": 401,
  "detail": "The current backoffice user should have access to the tree root"
}

The current backoffice user does not have access to the tree root seems a better reason for why its a 401

Copy link
Contributor

@Migaroez Migaroez left a comment

Choose a reason for hiding this comment

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

Some minor remarks about the code.
Tests out as described.

# Conflicts:
#	src/Umbraco.Cms.Api.Management/Factories/UserPresentationFactory.cs
@madsrasmussen madsrasmussen merged commit 8ad6c36 into v14/dev May 3, 2024
4 of 7 checks passed
@madsrasmussen madsrasmussen deleted the v14/fix/user-start-nodes branch May 3, 2024 06:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants