Should denied view right deny other rights or be overridden by other rights?

Development
#1

Hi everyone,

when fixing XWIKI-19549 I did a bigger change than I originally wanted to. What I did was to ensure that if a user doesn’t have view right on a document, it won’t end up in the context as many actions including back then the login action allow overriding the template and thus viewing the document. However, this not only applies to login which is kind of special, but also the comment action. When you had comment but not view right before that change, you were still able to view the document by passing xpage=view when adding a comment. With this change, comment right is effectively denied when denying view right.

Unfortunately, this denial is not very clean and bad things can happen in some scenarios that I don’t want to discuss in detail here. What I would like to ask here is to get a choice between two ways to continue evolving XWiki’s rights:

  1. Properly deny all (standard?) rights when view right is denied, directly in the right computation. This already happened in practice since 13.10.4/14.2 by effectively denying all actions as the context document isn’t loaded and I haven’t heard any complaints.
  2. Undo parts of the changes for XWIKI-19549 and thereby basically grant an undeniable view right when the user has, e.g., comment, edit or delete right. For this, there are two options:
    1. Don’t explicitly grant view right but accept that it is implicitly granted and not consider this a security issue. This was the case before 13.10.4/14.2.
    2. Modify right resolution to make view right undeniable when other rights are granted. The question is which rights? Comment for example currently doesn’t imply view right.

I think the primary problem here is that we have lots of ways to end up with inconsistent and conflicting right resolutions and making this more consistent and more easily explainable to users is a big task.

To me, option 1 sounds like the safest security-wise and as we basically tested this in already two LTS releases now I think it should also be safe, so +1 for 1 from me.

Thank you very much for your opinions!

#2

+1 for option 1.

Thanks,
Marius

#3

It’s not very clear to me how far to propose to go here. Could a wiki admin loose admin right when view is denied ?

#4

My idea would be - but I haven’t checked the implementation in detail - that this would be after all implied rights are applied. I.e., after everything we do today, if view right is denied, you also loose all other rights. As admin right implies an undeniable view right, the situation that an admin looses view right should never occur.

#5

OK so the difference with now is that if you don’t have ADMIN right, but you have EDIT or DELETE right, you would lose them if VIEW is explicitly denied (currently you can directly access the /edit/ action of a page for which you have EDIT right but are denied VIEW right).

Make sense to me, +1 for Option 1.

#6

+1 for option 1,
I think this is the best security wise, and from a user perspective it does not seem too difficult to understand how denying the view right would override all over rights.
IMO keeping this behavior stable throughout versions is the most important thing to consider for usability, thanks for making it clear :slight_smile:

I think we should add a line about it in the Admin documentation for access rights here or here once this matter is settled.

Thanks,
Lucas C.

#7

Generally speaking I’m also +1 with Option 1.
Now I think there’s a few things we need to check:

  1. Register right shouldn’t be denied when view right is denied, so there’s a special case to handle here
  2. We might need to double check impacts on some features on closed wiki: e.g. does resetting the user password will still work as it involves editing the user profile (honestly I don’t think it will be a problem, but better be safe)

Also, I’m wondering if we shouldn’t improve the design to allow a right skipping this automatic denial. I don’t see yet a UC except for Register, but maybe some custom extensions rights would need to bypass this for some obscure reasons.

#8

Thank you for the feedback. I’m wondering if we instead of denying all rights, we shouldn’t just deny edit, delete and comment right and introduce a way how additional denials can be requested similar to how rights can imply other rights. Alternatively, we could also do the following two changes:

  1. Comment right implies view right
  2. After the right resolution, we check for every right if the rights it implies have been granted. If not, the right is denied. Whenever a right has been denied, check all rights again to handle recursive implication.

As an alternative to using the existing implied rights, we could also introduce a new “required rights” field on Right to list rights that are required for the right. We could then list view right for edit, delete and comment right, maybe also script right.

#9

It certainly does not make much sense to be able to comment something you cannot view so I don’t have anything against it.

It makes sense to me, too. We can always introduce a new metadata if we find a case where the two concepts are not in sync (but it might be that it’s missing an implied right, like in the case of comment).

#10

I’m not so sure about this: my understand is that it would mean if UserA is granted Comment right, but doesn’t have View right because it’s explicitely granted to GroupB that UserA doesn’t belongs to, then he’ll obtain access to the View right by the implied right. (that’s what I understand by reading the code here)

IMO it’s best: to have two distinct mechanisms one corresponding to the current imply rights which is allowing the implied right when needed. And another for denying a right when needed: so we could express that Deny view implies to Deny comment, edit, etc. IMO this mechanism should probably works for both explicit and implicit denies but it should be called after the resolution of allowed implied rights. Or to simplify we could consider it only for explicit denies. Honestly I haven’t dig enough in the examples.

#11

I would have formulated it the other way. A right can specify which other rights it requires to work. And then when that other right is denied, it is denied too. That way, extensions can easily add a requirement for view right to their rights.

And yes, my proposal is that we always first fully resolve what we have right now and then resolve these requirements.

#12

Ok indeed the design is probably better. Now I’m not sure that during the resolution of another right you can now if the required right is denied because of implicit or explicit rule, so we’ll have to ensure it’s working for both cases (which is probably better IMO).

#13

I started implementing this. It seems indeed possible to just put another step here after applying defaults to resolve any “conflicts” by denying rights where the implied/needed right is denied.

If we all agree that this is the way to go, there is still an open question: Should this be a new property on the right or the existing implied rights? Note that any new property is an API breakage as Right is Serializable and we thus cannot add any new property without breaking backwards-compatibility.

So new options to choose:

  1. Use the existing implied rights.
  2. Add a new property, accepting the API breakage. I initially named this requiredRights but this name is kind of already taken, so what about neededRights? Feel free to also suggest another name, possibly using a synonym of required.
  3. If we go with 1, should comment imply view? Note that it is currently not possible to add a comment when you don’t have view right as the commentadd action shows an error when the document isn’t in the context.
#14

While writing some tests, I noticed that we have currently a strange situation with creator right:

Creator right implies delete right and delete right implies view right, but implied rights currently aren’t transitive. This means that when denying view right to the creator of a document, the creator will have delete right but not view right. Due to my fix for XWIKI-19549, however, the delete action cannot be used. The same is true for the REST API, it also checks view right first.

Again, we have two ways to resolve this:

  1. Let creator right imply view right, basically manually making implied rights transitive.
  2. Accept that view right can be denied and this now means that delete right is also denied, basically making the situation we had for quite some releases more obvious and consistent. If we use the existing implied rights, even the creator right itself would be denied. Note that while denying view right would deny view and delete right, denying delete right alone wouldn’t be possible as creator right is undeniable.

I’m +1 for 2 as it seems quite unexpected to be unable to deny view right to the creator of the page. Imagine user A works on a project, creates some pages for it and then leaves the department that has access to the project - wouldn’t it be natural that user A looses access even to the pages that user A created?

#15

I’ve created a pull request that implements option 2 in both cases, i.e., adding a new property with needed rights and letting deny view imply deny edit even when the creator right is set. I think I would actually be in favor of using the implied rights instead of a new property but I’m also okay with the new property. It wouldn’t change the implementation too much and I hope the pull request clarifies a bit what I propose here.

#16

Let me try summarizing this thread to maybe provide a way for others to provide their opinion without reading through the mess in this thread:

The main problem I’m trying to solve here is that it currently it is possible in XWiki to have edit or comment right but not view right. This is super confusing and also a security problem as you can still view the document through the edit or comment actions. For this reason, since 13.10.4/14.2, all actions are effectively denied when a user doesn’t have view right. However, this change was never properly done in the right resolver, leading to strange bugs due to inconsistencies between action handling and access control logic.

My proposal is to fix this by denying a right when a user doesn’t have other rights that are “necessary” for this right. For example, to deny edit right when the user cannot view a document. These denials would be applied after fully resolving rights including implied rights, see my pull request for a possible implementation.

The main question for me is: Should “necessary” rights be a new concept, further complicating the rights system, or should we use the existing implied rights?

Adding “necessary” rights as new concept has the advantage that we can apply them selectively, e.g., letting comment right need view right even though comment right does not imply view right. Letting comment right imply view right could grant users view right who previously didn’t have view right which is something I would like to avoid.

I’m therefore +1 for introducing “needed” rights as a new concept.

Overall, I’m not that happy with the change as it seems to make rights in XWiki even more complicated, but I don’t have any better suggestions.

#17

I still don’t have super deep knowledge of XWiki but to me “neccessary” rights seems very logical, but at the same time, when setting an right via the GUI (or other ways) that needs another right, the needed right should automatically be set (checkmarks in the GUI) accordingly to not confuse users and to make things more self-explanatory.

I think the NTFS rights GUI in Windows could be an very good example:
When setting the permissions to “modify”, all checkmarks for the neccessary rights are automatically set. When you then uncheck for example “read”, the checkmarks and rights for “modify” are also deactivated as “modify” won’t work without “read”.

Currently, the permissions system of XWiki and the rights GUI is super confusing compared to other software products that have an granular permissions system.

Also it would be great to “inherit” rights from pages above. Currently as soon as a right is set to a specific user/group to a hild page, all other users / groups that had rights on the parent page (and its sub pages) don’t have the particular right on that child page anymore and have to set up there too. When setting up an larger XWiki with more granular rights, many rights have to be set up multiple times which feels quite redundant.