umbraco / Umbraco-CMS

Umbraco is a free and open source .NET content management system helping you deliver delightful digital experiences.
https://umbraco.com
MIT License
4.45k stars 2.68k forks source link

Changing public access settings affects parent and siblings. #6057

Closed Luksor closed 5 years ago

Luksor commented 5 years ago

Reproduction

Bug summary

When a node is configured with public access it should be possible to modify a child node to have different public access settings. However it looks like this feature is not working correctly and the modified settings propagate to other nodes as well.

Specifics

Steps to reproduce

Expected result

The child node settings should not affect siblings and the parent.

Actual result

Public access settings are propagated to all nodes having that original public access configuration.

stevemegson commented 5 years ago

It's not necessarily a bad thing to be able to edit an existing rule from any page affected by it, but the current behaviour doesn't make it very clear that you're doing that, and it gives you no way to create a new rule for the child node.

Perhaps when you open public access settings for a node which is currently inheriting a rule from an ancestor, you should first get a variation of the "choose how to restrict access" step: public-access The first option would give you the current editing behaviour, while the other two would create a new rule in the same way as for a node which has no existing protection (or perhaps create a new rule starting from a copy of the inherited settings).

On a bit of a tangent, could we also have a different icon in the tree for nodes which are inheriting protection from an ancestor? Perhaps a faded version of the current icon.

nul800sebastiaan commented 5 years ago

Hi @Luksor - I am not sure if we even want to support multiple levels of public access in ancestors in the tree, or how that is supposed to work right now.

The way I always understood this to work is that there's only 1 public access configuration possible for a parent and all of it's ancestors. I understand you might WANT to do something else on an ancestor but I am not sure if we want to support this scenario.

I'll get back to you on this one after I've discussed it at HQ.

clausjensen commented 5 years ago

We've talked a bit about this and decided that it needs to be split in two different issues.

This first issue will be taking care of the actual bug allowing you to edit parent permissions by editing permissions of one of the affected children.

This should not be possible and we will need to simply disable the option for editing inherited permissions, with a message telling the editor to navigate to the parent and edit this instead.

We however need to consider the scenario of:

What happens in this situation currently? - My guess is that it will actually work as expected due to the lookup for a 'permission' just being a recursive lookup through ancestors or current node. But if we decide to simply block out the option of editing a child node's permissions if a parent has anything set, then there would be no way to edit or remove the permissions already set on a child, when a parent node has permissions applied.

The suggested solution (with PR) will need to be added as a new issue/feature request and then it can be discussed how the actual implementation of that should be.

nul800sebastiaan commented 5 years ago

As for the current problem, I think it would be good to fix the bug and not allow updates to public access from the descendant nodes, that is too confusing.

If you'd like to help out fixing this then that would be much appreciated!

As Claus mentioned, the feature request to make more granular permissions should be discussed in a separate issue so we can work out the details first before work can start on it. 👍

umbrabot commented 5 years ago

Hi @Luksor,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

stevemegson commented 5 years ago

What's the expected behaviour after the initial bug fix?

Currently, setting permissions on a parent after a child leaves the child and its descendants with their original permissions, which seems reasonable.

When opening 'Public access' on a child after setting permissions on the parent, I think there are three easy options before considering a possible feature request for a more complex change:

  1. Always edit the inherited rule. The current behaviour, which might be what some editors expect but is more likely to be confusing.
  2. Always create a new rule for the child. Gives the flexibility to set different permissions on a child, but could be confusing if you expected the current behaviour.
  3. Do nothing and display a message saying that setting permissions on a child is not supported when an ancestor already has permissions. Avoids any confusion about what the menu item does, since it does nothing. It's still possible to set up nested permissions, but only by setting up the parent after the child.

Personally, I think option 2 is what I expected to happen.

kjac commented 5 years ago

I agree with @stevemegson here. Since the you can "reverse" this issue by applying access restrictions to children first and then parents, we need to handle the case where children have specific access restrictions regardless.

Perhaps all that's needed is something as simple as a message like "This item currently inherits public access settings from [Parent]. If you apply new settings you will break this inheritance." and then let people apply whatever access restrictions they want? Obviously the initially reported bug should still be fixed then.

nul800sebastiaan commented 5 years ago

Well, egg on my face... It seems like this used to work as following in v7, so let's restore that functionality in v8.

In other words, the current behavior is a bug: parents/ancestors definitely should NOT be updated when you update a child or descendant.

kjac commented 5 years ago

I'll have a look later unless someone beats me to it 😛 I did the V8-ification of public access, so I'm sure it's my fault 🙈

kjac commented 5 years ago

PR in #6118

nul800sebastiaan commented 5 years ago

Fixed in https://github.com/umbraco/Umbraco-CMS/pull/6118

Cherry picked for 8.1.4 16ac2c5589dabf65747b95e7122e58b9a82858d3

thenexus00 commented 2 months ago

Just re-opening this as this may be broken again? This is regarding the last version in 13.

Based on this comment: "When you then set up Public Access setting for a child or decendant, that specific child will have it's own Public Access settings and all descendants inherit those Public Access settings"

The parent is set:

Screenshot 2024-08-01 at 4 16 09 pm

All children inherit this. I would imagine the child would have then have the same options so you could remove the access restriction. Instead you get the interface as it has no restriction despite it actually doing so:

Screenshot 2024-08-01 at 4 18 27 pm

If you add it in again on the child, then remove the restriction, it remains restricted.

kjac commented 2 months ago

@thenexus00 I think it's working as intended 😄 at least if I understood you correctly.

  1. Public Access restrictions are inherited for all descendants of a restricted item.
  2. Public Access restrictions cannot be lifted (entirely) anywhere below a restricted item; they can only be re-defined.
  3. Re-defining Public Access restrictions on a child (or descendant) of a restricted item means that the newly defined child restrictions apply for all descendants below that child.

The "Restrict Public Access" dialog might not reflect in an intuitive manner; but the document tree will reflect the restrictions by all the little stop signs added to all restricted items.

thenexus00 commented 2 months ago

@thenexus00 I think it's working as intended 😄 at least if I understood you correctly.

  1. Public Access restrictions are inherited for all descendants of a restricted item.
  2. Public Access restrictions cannot be lifted (entirely) anywhere below a restricted item; they can only be re-defined.
  3. Re-defining Public Access restrictions on a child (or descendant) of a restricted item means that the newly defined child restrictions apply for all descendants below that child.

The "Restrict Public Access" dialog might not reflect in an intuitive manner; but the document tree will reflect the restrictions by all the little stop signs added to all restricted items.

Hi Kjac, Partly see this but there should be no option on a child to remove restrictions if you can not remove restrictions and the slide out indicates this but saving has no effect and as you noted the tree has the stop sign.

Most things make sense but I think it is odd that there is a way to change the type or restriction on child items but then not remove it. If you said it is child share parent and that is your lot - I would understand that and would want the feature. Right now though it is nether here nor there.

kjac commented 2 months ago

Hiya @thenexus00,

Thanks a lot for your feedback! I'm not trying to argue if it's right or wrong 😄 this is just "how it's always been", and how the feature is intended today.

I believe the expected use case is to make limit restrictions further as you move down the tree. Again, the UI is debatable.

If you'd like to suggest a change in behavior, the GitHub Discussions would be a great place to start a ... well, a discussion 😆