Umbraco-CMS icon indicating copy to clipboard operation
Umbraco-CMS copied to clipboard

Media upload in root media errors when user has media start item (15665)

Open AdamPrendergast opened this issue 1 year ago • 3 comments

Prerequisites

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

Description

Refactor media access check to ensure root access validation

Updated the logic for determining access to root media types based on the current user's start media IDs and their group memberships.

It checks for null or empty user media start IDs, and then checks to return an empty set of ContentTypeBasic only if neither the user nor their groups have root access.

Testing Notes

  1. Create a user.
  2. Check that the user with no start Media item set can successfully create media items at the root.
  3. Update the users setting so the have a 'Media start nodes' that is not the Root
  4. Check that the users start media is updated
  5. Check that they no longer have the option to create media at the root (see below image).
  6. Now remove the users start node, and change the users 'Group' so that it has a start node other than the root.
  7. Check that they no longer have the option to create media at the root (see below image).

The call to action for creating media at the root is highlighted below. image

Setting Users Start node: image

Setting User Group Start node: image

For Discussion

The code update that has been put in place will remove the option to create root media if either the individual user or the group have a start node that does not contain root.

Due to the multiple competing scenarios, it would be great to get a second opinion on this and also ensure it goes through some user testing of each possibility.

I do anticipate having to refine this initial PR, but it's a good starter for discussion.

AdamPrendergast avatar Feb 01 '24 17:02 AdamPrendergast

Hi there @AdamPrendergast, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
  • If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
  • [x] 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

github-actions[bot] avatar Feb 01 '24 17:02 github-actions[bot]

@emmagarland @Migaroez

Just rethinking the approach for this one.

The logic is most likely the correct approach, however it may be better sat with the IUser to determine if they have access rights.

We could add somthing like bool CanAccessMediaRoot(); to the IUser

We can then add something like the below to the MediaTypeController

if (_backofficeSecurityAccessor.BackOfficeSecurity?.CurrentUser?.CanAccessMediaRoot() == false)
{
    return Enumerable.Empty<ContentTypeBasic>();
}

That would avoid us making the controller code more complex and also allow us to more easily get some Unit Tests around the various scenareos.

Let me know what you think and I can adapt as needed.

Thanks :)

AdamPrendergast avatar Feb 02 '24 09:02 AdamPrendergast

Thanks @AdamPrendergast!

Looks like @Migaroez has self-assigned this, so I'll wait for the feedback.

Best regards

Emma

emmagarland avatar Feb 11 '24 15:02 emmagarland