U4-10173 - Start node calculation should take into account more restrictive start nodes

Created by Shannon Deminick 18 Jul 2017, 01:59:57 Updated by Shannon Deminick 01 Aug 2017, 07:53:10

Tags: Unscheduled

Relates to: U4-239

Relates to: U4-8428

Relates to: U4-10075

Subtask of: U4-8632

Here are the current (simple) rules:

*A user can have zero, one or many start nodes, which can include the 'root' or an ID *A group can have zero or one start node which can be the 'root' or an ID *If a user's combined start nodes is empty or contains 'root', then they have access to root, otherwise they have multiple start nodes

This however means that if there are more restrictive start nodes set they really won't make any difference if the user or user group has less restrictive or root access start nodes so we have 2 options:

*Make a user's explicit start nodes overrule any of it's user group's start nodes OR *Calculate start nodes based on: **If the user or user group has root access but there are other non root start nodes selected, then we can decide that root is actually not allowed **We'd have to lookup all Paths for all nodes in the start node list and then de-duplicate based on path matches and only keep ones with the deepest level

Comments

Stephan 18 Jul 2017, 11:33:11

First, if a user's combined start nodes is empty, then the user should have no start node. At all. Just to be consistent.

Next, trying to recap things:

Permissions:

  • permissions are defined on groups
  • a group has default permissions
  • a group can have granular permissions on a node

Combined permissions:

  • on a given node, permissions are the combination of:
  • for each group the user belongs to,
    • permissions are inherited from the nearest granular permissions, or default

Start nodes:

  • a group has zero or one start node
    • group permissions are not related to (limited to) its start node
    • to set permissions on & below a given node only, use granular permissions
  • a user has zero, one or many start nodes

Combined start nodes:

  • user's groups start nodes are always de-duplicated, keeping the upmost nodes
  • ''how shall we combine user start nodes + user's groups start nodes?''

Start nodes are the only thing we can set on a user without creating a group.

Option A (current): user start nodes ''add'' start nodes, so they are added and de-duplicated with combined groups start nodes, keeping the upmost nodes

Option B: user start nodes ''define'' start nodes, so they entirely replace combined groups start nodes, then are de-duplicated, keeping the upmost nodes

Option C: user start nodes ''add or restrict'' start nodes, so they are added and de-duplicated with combined groups start nodes, keeping the deepest nodes

I am not a big fan of B, and think A is not enough. Would prefer C, which is similar to what you propose, but in a more general way: if a user start node is above or below a group start node, then it takes over. Else it's just added.

Now it means that getting the start nodes requires processing the paths which (a) has a perfs impact and (b) means we need paths ie IEntityService


Stephan 18 Jul 2017, 13:22:56

See PR https://github.com/umbraco/Umbraco-CMS/pull/2056

It does implement option C in UserExtensions:

  • groups start nodes are de-duplicated, keeping the ''upmost'' nodes
  • user start nodes are de-duplicated, keeping the ''deepest'' nodes
  • de-duplicated user start nodes are merged with groups start nodes -- groups start nodes being ancestor or descendant of a user start node are removed -- each de-duplicated user start node is added

So if the tree is N1

  • N2
  • N3 ++ N4
  • if groups start nodes are (N2,N3,N4), then de-dup = (N2,N3)
  • if user start nodes are (N2,N3,N4), then de-dup = (N2,N4)
  • if de-dup groups = (N2,N3) and users = (N4) then consolidated = (N2,N4)
  • if de-dup groups = (N2,N4) and users = (N3) then consolidated = (N2,N3)

Can be explained to users this way:

  • if a user start node ends up being under a group start node, then it restricts = replaces it
  • if a user start node ends up being above a group start node, then it broadens = replaces it
  • else it's just added

Would this make sense?

It all works quite well by my tests - but I hate having to carry the entity service around to get paths, or reference ApplicationContext.Current = ideas?


Stephan 18 Jul 2017, 13:23:27

(not moving to review bc we cannot merge the code as-is due to ugly entity service usage)


Shannon Deminick 19 Jul 2017, 06:18:21

I've updated the code since we shouldn't have a Model (IUser) calling back to services:

*The properties AllStartMediaIds and AllStartContentIds are new properties so we have the luxury of just removing them ... which I've now done. *Now anytime these calculated start nodes need to be retrieved, the extension methods will be used *I've added object level caching for the result of these calculations so they are not re-calculated over and over again *I've renamed the methods since GetAllMediaStartNodes and GetAllContentStartNodes isn't really what this doing, these methods are calculating them. *I've updated all of the unit tests that relied on the "AllStart..." properties *I've added some TODO notes

Let me know what you think.


Stephan 19 Jul 2017, 08:53:27

all good - and have done some of the TODOs (actually, did it yesterday I forgot to push) - now testing the caching part of it


Stephan 19 Jul 2017, 09:05:12

happy w/caching - changing start nodes on a user currently logged in, and it immediately refreshes - good, merging


Stephan 19 Jul 2017, 09:05:37

ah no actually it's my code, for you to merge ;)


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: 3

Cycle: