U4-10075 - Inherited permissions for content nodes

Created by Shannon Deminick 29 Jun 2017, 07:16:17 Updated by Shannon Deminick 01 Aug 2017, 07:53:10

Relates to: U4-10173

Relates to: U4-2474

Subtask of: U4-8632

Currently permissions are not inherited, they are explicitly assigned or the user group's default permissions are used.

Comments

Shannon Deminick 11 Jul 2017, 06:11:47

OK, so we sort of have inherited permissions. For example: http://issues.umbraco.org/issue/U4-2161, http://issues.umbraco.org/issue/U4-4213

What we currently do is:

  • all new content created will have it's parent permissions applied to it (if any exist)
  • the same goes for any copied content, if you copy content, the copied item will have it's new parent permissions applied to it (if any exist)

Here are the problems:

#This is a lot of data to be stored and looked up for all user/usergroups/content that have permissions assigned. This is discussed in-depth here: http://issues.umbraco.org/issue/U4-2474 #When a node is created under a parent that has permissions assigned it will get the parent permissions copied to itself, then if it is moved it will retain it's original parent's copied permissions ##Since we currently don't know if permissions have been assigned explicitly or by being copied from it's parent, it's not possible to potentially inherit the permissions from the new parent. ##Even if we could re-inherit the permissions of the new parent, this would mean we'd have to update a mass amount of data if the node being moved has a lot of descendants #If a parent exists with descendants and no permissions have been assigned anywhere on this branch and then permissions are assigned to the parent, should these new permissions be replicated to all descendants? This is really what inherited permissions should do but this is not happening currently. ##In a different case however, the permissions would not be replicated to any descendants that already have any explicit permissions assigned ... which would mean we'd have to be able to track explicit permissions!

Options to fix this:

*Update the umbracoUserGroup2NodePermission table to include a new boolean column called isExplicit which we can use to determine if a permission assigned to the node is a copied/inherited permission or if it was explicitly set on that node. This would allow us to 'solve' points 2 and 3 above. However this doesn't solve point # 1 *We update the way it works now and remove all of the replication for the data and instead look into a way to lookup inherited permissions in an efficient way. This will solve points 1,2,3 above but will take more work. It will mean that we are storing far less data and that the permissions structure is much more flexible


Shannon Deminick 12 Jul 2017, 04:35:12

Here's a mockup of how user group permissions will work with inheritance. It's really the same as it is now except without the overhead of copying everything but this should hopefully clarify what is happening:

For a user called UserX, it belongs to these groups which have the following default permission characters applied:

||Groups||G1||G2||G3|| |Defaults|sdf|sdgk|fg|

The aggregate defaults for UserX is "sdfgk"

As an example node structure, here is the node tree and where any explicit permissions are assigned and it shows you the resulting permissions for each node that the user has:

||Nodes||G1||G2||G3||Result|| |- A| | | |sdfgk (Defaults)| |- - B| |fr| |sdfgr (Defaults + Explicit)| |- - - C| | |qz |sdfrqz (Defaults + Explicit + Inherited)| |- - - - D| | | |sdfrqz (Inherited)| |- - - - E| | | |sdfrqz (Inherited)|

*Notice that 'k' is no longer in the result for node B and below *Notice that 'g' is no longer in the result for node C and below


Stephan 12 Jul 2017, 07:02:39

Just to make sure I understand correctly: perms on a node are either inherited, or explicit, and that's a global thing (not per permission) - so because G2 sets 'fr' on B, the whole default 'sdgk' is ignored. correct?

Does it mean that if G2 adds 'x' on C, and wants to add 'y' on D, one has to remember to add 'xy' on D else 'x' would be missing?


Shannon Deminick 12 Jul 2017, 08:19:05

Permissions are either: Default (based on the group's defaults), explicit (based on permissions assigned to a node for a group), inherited (based on the parent node)

so because G2 sets 'fr' on B, the whole default 'sdgk' is ignored. correct?

Because G2 sets 'fr' on B means that the default's for G2 no longer apply for node B

Does it mean that if G2 adds 'x' on C, and wants to add 'y' on D, one has to remember to add 'xy' on D else 'x' would be missing?

Correct, and that is no different than it is today, we don't merge inheritance for a single group over all ancestors since that would become insane and if we did that we'd also have to have Deny permissions


Shannon Deminick 13 Jul 2017, 08:49:28

PR here: https://github.com/umbraco/Umbraco-CMS/pull/2046

To test:

  • Review the code, there's a lot updated so please just check to make sure there's nothing obviously wrong
  • Run unit tests, they should all pass (app veyor is passing them all too)
  • Test that you can do all of the things to content that you normally should be allowed to (i.e. nothing broke from your last tests)

Then we need to run the actual tests in the UI

  • Create a tree structure A -> B -> C as a hierarchy
  • Create 3 groups without any permissions and no start nodes: G1, G2, G3, make sure to give at least one access to the Content and User section (since we can then edit it's own permissions)
  • Create a user: PTest and add to G1, G2, G3 - copy their password since we'll need it
  • Log in as PTest - you should have no access to anything
  • Navigate to user section and update G1 to have: Browse, Create, Update
  • Go to A, B, and C and verify: Only Preview/Save buttons are available when editing the content item, only Create, Notifications, Reload are available from the ctx menu
  • Go edit G2 group, add permission: 'Permissions'
  • Go to node B, go to permissions menu, add permission for G2, select Copy and Move, save these permissions
  • Check the ctx menu on B and C, you should have Copy and Move now. On node A you should not have Copy or Move
  • Go to node A, go to permissions menu, add permissions for G3, select Delete, save these permissions
  • You should now have Delete for A, B and C
  • Go to node B, go to permissions menu, remove permissions for G2, save these permissions
  • You should no longer have Copy or Move for B or C


Shannon Deminick 14 Jul 2017, 00:58:43

This issue also resolves: http://issues.umbraco.org/issue/U4-2474, http://issues.umbraco.org/issue/U4-10146


Stephan 17 Jul 2017, 11:03:25

Code review:

  • nothing obviously wrong
  • all tests green
  • should be marked as non-backward compatible?


Stephan 17 Jul 2017, 13:17:29

Found a nasty AutoMapper issue preventing from saving a user group, fixed in PR


Stephan 17 Jul 2017, 13:53:40

Running test in the UI, so far, works as expected.

Now this:

If group G1 gives browse & update access to the entire tree (start node = content root), and user U is in group G1, but we want to restrict its scope to B's branch - one would think that setting U's start node to B would do it, but because start nodes all add together, it won't work and U still sees the entire tree.

It's a common practice to use a group to define common permissions on the entire tree, eg ''Editors'', and then use the user start node to scope to a branch. This does not work anymore, and in fact the user's start node is basically useless:

  • if it's more restrictive than that of the group, it's ignored
  • it it's less restrictive then it points to a node with no perms

One would need to remove the user from the group, and then create a separate group with a separate start node -- but then the user start node is not needed.

I ''think'' that as soon as a user has its own starting node, it should replace anything inherited from groups.

Is this making sense?


Priority: Normal

Type: Task

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 7.7.0

Sprint: Sprint 63

Story Points: 5

Cycle: