Document Rights Management Improvement

Development
#1

Hello all,

I have started a design page to improve the way we currently handle rights on document.

The two issues that lead to this proposal are the following:

  • CKEditor executes the content with the rights of the current user: It is dangerous to use CKEditor for any user with script/programming rights
  • The rights of the current users are always granted to the page on save: It is dangerous to edit

I’ll try to summarize the design page below.

The idea is to no longer rely only on the rights of the current user/content author when checking edit/script/programming rights on a document.
In addition, we’d store a list of required rights (RRs) for each document.

RRs would then be used to check of edit/script/programming rights based on the following logic:

When the EDIT, SCRIPT or PROGRAMMING right R is checked for a a document author U for document D.

  • check if U has all the rights listed in RRs on D
    • if not, false is returned
    • otherwise, the result of the usual check is returned

In case of undefined RRs, different configurable strategies can apply:

  • strict: the document is rendered with no script or programing rights
  • legacy: the document is rendered with the rights of the last author

This has several consequences for the user experience:

  • user with edit rights on a document, but missing one of the required rights (e.g., the user does not have script rights but it is a required rights), will not have the right to edit the document in practice (e.g., the edit button will not be displayed)
  • users with script or programming rights will have the possibility to configure the RRs of a page using a dedicated form (see the design page)

Migration
Of course we are starting from a situation where no pages has RRs defined. What I propose is:

  • in a first time, the strategy is set to legacy
  • all documents of XS are moved to requiredRights and missing required rights are added during 15.x cycle
  • For extensions:
    • option 1: migrate to 15.x+ and define the RRs
    • option 2: propose a way to define RRs on older version?
      • adding unknown entries in the xar
      • offering to define the RRs on a separate file that would only be read by 15.x+
    • option 3: auto-detection feature: each imported file would be executed with minimal rights and scanned for XObject requiring PR/Script rights (e.g., stylesheets). If no error is raised, the document is saved with requiredRights set to true and an empty list of RRs, otherwise the user is asked to fix the missing RRs.

Decreasing the rights

While I feel like this should not be allowed, I’d like to open the discussion on the situation where a user with a missing RRs but with edit rights wishes to edit a page nonetheless.

Should it be possible:

  • All the time. In this case the RRs would be removed according to the rights of the editor
  • A new checkbox is added to the RRs configuration form, allowing users to decrease the rights only if allowed
  • never

WDYT? Let me know if I’ve missed something.

1 Like
A content-analysis-based approach for required rights
Required Rights administration location
#2

You mean the rights of the last author?

In general I’m +1 for this change as I think it’s an important step to make editing documents safer.

I think apart from what you’ve mentioned there is also the important question what to do for new documents and for documents created through some API (think AWM creating an app) or through the REST API. Further, I think we should consider some auto-detection feature that suggests adding required rights to users who may add them, both for documents that already have them and for documents that are currently in some legacy state.

I’m also +1 for not allowing users without a certain right to remove that required right as it basically re-creates at least some of the current mess. Note, though, that we might still end up in a similar situation when the current last author looses a right. With the required rights, we could offer admins way better tools for fixing this situation, though.

Regarding extensions, while it would be nice to support required rights as soon as possible, I’m not sure it’s worth the hassle. Maybe we could instead offer a feature to protect editing of documents installed through extensions (at least those documents that aren’t supposed to be edited, maybe also just those with “legacy” required rights)? Just to reduce the chance that some user edits them, introduces an exploit and then gets an admin to “fix” them without removing the exploit code (seems unlikely, though, I hope an admin would rather roll back the change). What I’m wondering is how we can make sure that extensions that create documents continue to work and maybe even support versions of XWiki with and without required rights.

#3

Are you saying that there can be cases where:

  • $services.security.authorization.hasAccess('edit', 'xwiki:Some.Page') returns true
  • but the current user doesn’t see the Edit button? (i.e. it doesn’t actually have edit right)

Whether or not this is the case, there will be an inconsistency somewhere:

  • either between $services.security.authorization.hasAccess() and the UI (“It seems I have edit right but I still don’t see the Edit button”)
  • or between $services.security.authorization.hasAccess() and the rights UI (“I gave them edit right but they still can’t edit the page”)

I understand, I think, the reason behind this proposal, but it adds more complication to an already very complicated rights system…

#4

My proposal would be this variant but to integrate the required rights into the rights UI such that it is immediately visible there.

#5

I agree. One way to find a tradeoff could be to show a disabled edit button with an explanation displayed on hover. For instance: “You don’t have enough rights to edit this page. The programming rights is required in addition to the edit right”.
Also, I believe this is mainly going to be the case for hidden technical pages.

#6

Yes, fixed. Thanks

Indeed, I’ll improve the design on this topic.

I have two questions about this:

  • what would be the heuristic here?
  • from an UX standpoint, how do you see this working?
#7

Regarding the heuristic, I would suggest something like checking for XObjects that require script right or any wiki text property (or content) that contains a script macro, similar to the static analysis done for link rewriting. The heuristic would only be for script right, not for programming right.

Regarding the UX, I would display some message for all users with script right if the heuristic determines that the document might have a problem due to missing required rights setting. E.g., when a user adds something that requires script right, after saving, the user would see the message and could set the required right. This should probably only be done for documents that don’t use “legacy” mode, for them it might be more appropriate to integrate this into some tool for admins to mass-add the required rights setting with values based on the suggestion by the heuristic.

#8

I’ve updated the design page. The interesting part are presented below.

Programmatically create and edit pages

The current API expects the create page to be defined with requiredRights=false

For instance, a page created using AWM will require some rights that would now be part of the RRs.

  • It should be possible to force requiredRights to true in the API (note: at some point in the (far) future, we could decide to always set requiredRights to true)
  • If no requiredRights value is explicitly set, we have two options:
    • do nothing and let use the legacy behavior
    • use some heuristics to set requiredRights to true and add the RRs based on the content of the page (e.g., the presence of some XObject on create and edit)

Note: pages created though the UI would have requiredRights=true by default.

Auto detection of RRs

Warning: The scope of the auto-detection is currently only for the Script right. The Programming right is intentionally left aside.

When requiredRights is set to false we are left in the dark regarding the restrictions to apply a document (e.g., pages created programmatically without requiredRights set, or pages imported from an extensions with version < 15.x).

In this case, it is interesting to automatically set the RRs based on a set of matchers:

  • The presence of some macros (e.g., the velocity macro)
  • The presence of XObjects (e.g., related to translations, or stylesheet) that requires script rights to work correctly
  • other: the design would be open to 1) add missing matchers in the standard distribution 2) allow extension to define their own matchers

If the strategy allows for the auto-update of the RRs based on the matchers, requiredRights would be set to true, RRs would be fillet with the script right if one of the matchers returned true.

Identifying pages with missing required rights

A dashboard with a list of pages with requiredRights=false is proposed in the administration.
This dashboard would also propose to analyze documents to automatically set the required rights (based on the auto-detection described below).

We can consider different strategies:

  • let users manually request for the analysis of a document
  • introduce a new document analysis task that would apply the auto-detection feature on documents with requiredRights=false when they are created or updated. The result of the analysis would be stored and displayed in the dashboard. Admins would just have to click an auto-apply button to update the RRs of a page.
1 Like
#9

I’m generally +1 for this direction, but it’s going to be a migration challenge.

This section should be several different sections, as there is no monolithic “extensions” use case:

  • extension we maintain: I’m not worried about those that much, as it’s always easy to find workarounds for those
  • active extensions but which are not maintained by us: if the workaround is not too complex, I guess it’s not a big problem either with a bit of documentation
  • unmaintained but working extensions: that’s what we should focus on, as I don’t see us fixing all extensions out there (especially since many are out of our reach anyway)

Not a fan of the extra file idea. Note that it’s already possible to have unknown document properties in the XAR XML, it simply produces an “Unknown element…” warning (and we could decide to not produce warnings for know future elements in 14.10.x even if we don’t really take them into account in this branch).

I think this is a must-have.

It’s probably safer, yes. Note that we already have a configurable system like this (extension.xar.protection in xwiki.properties), we could decide to change the default to deny.

#10

+1

Added in the design page.

A user can be disabled editing a page for two reasons:

  1. The user does not have edit rights
  2. The user does is missing some RRs

A explanation of the reasons for which is user is not allowed to edit a given page should be proposed to the user for the second reason. In this case the edit button should still be displayed to the user but:

  • disabled
  • a text explaining the restriction should be displayed on hover

Is there some exception to this rule when extension.xar.protection=deny? It is probably interesting that at least a few privileged users can still edit those pages (e.g., to fix bugs or patch security issues)

#11

To be double-checked, but from what I remember at least superadmin is an exception (because superadmin right is “evaluated” too early for this kind of subtilities to be taken into account).

#12

ok. I feel like it is going to be quite restrictive if this is the default (though I’m not an expert on the exploitation side of XWiki instances).

#13

Note that the required rights will need to be exported/imported from XARs. Meaning that we’ll need to increase to version 1.6 (1.5 was introduce for the concept of original metadata document author).

#14

A short progress update:

  • the groundwork (storing the required rights and taking them into account when checking rights) is almost done and is now a matter of polishing the UI integration (see Required Rights administration location)

What I propose as the following steps is:

  • provide a mechanism to automatically detect which rights should be applied on pages where the required rights are not activated
  • provide an UI to allow admins to apply the suggested rights on existing pages
  • make the required rights activated by default on new pages created from the UI
  • apply the required rights on all pages provided by XS
1 Like
#15

I had a closer look at the current implementation and I think there are several flaws:

  1. We have cases like UIX object at wiki level that require wiki admin right. To have a complete solution for document rights, I think we need to consider this case.
  2. The context property to disable required rights during a right check feels like a hack and is insecure when exposed as script API.
  3. ContectualAuthorizationManager overrides required rights with the required rights from the programming document. This feels like a hack and breaks when AuthorExecutor is used as it reloads the document from the database.

I therefore propose the following changes:

  1. Required rights become a pair of a Right and an EntityType. By default, EntityType would be “document” but it could be “wiki” for admin right. Initially, we implement a hierarchy “None < Script < Wiki Admin < Programming” for required rights.
  2. The default AuthorizationManager considers required rights only for edit right but not script or programming right. For considering required rights with other rights, a new AuthorAuthorizationManager is introduced that explicitly checks rights for the author of a document. In addition to the right to check and an (optional) target reference, it would either take a document or object (property) reference, or a (potentially modified) document instance with an indication if content or metadata author shall be used. The target reference would be assumed to be the document itself when omitted. The ContextualAuthorizationManager would still always consider required rights.
  3. We introduce new APIs for AuthorExecutor that take an actual document instance such that required rights from potentially modified document can be considered. At the same time, the API could be changed to accept UserReference.

In particular the second change seems dangerous as it means that existing code that checks for script or programming right wouldn’t consider required rights. Unfortunately, I see no real other way how to avoid such hacks and at the same time provide clean APIs to check rights both with and without required rights which is important to be able to check if a user may add a required right. I believe this change might be acceptable as checking such rights should be rare as the most frequent checks are in scripts or macros and there not using ContextualAuthorizationManager is a serious security issue, anyways. But please let me know if you’re aware of an important use case where this is a problem. We would of course fix use cases like JSX in XWiki itself.

You might have noticed an important gap: There is no more way to check if edit right is denied because of required rights. I think what would be interesting to fill this gap is to have a generic way to explain right decisions. I haven’t thought about the details yet but basically the idea would be to store, e.g., at which level (required X right, document itself, space X, space Y, wiki, …) a right was denied together with the computed right and to make this information available via some API. That way, we could give users a detailed explanation why they cannot edit, view, comment, … a document. The entries could have specific types (or enum values) to allow automatic analysis but also translation keys for displaying messages to the user. If we consider required rights so special compared to regular edit rights, we could then display a disabled edit button with tooltip in case edit right is denied just because of required rights. I’m not sure if I would expose the rest of the information to regular users but it could, e.g., be used in the right administration to let administrators check if a certain user has a specific right and if not where it was denied.

There is another gap in the current implementation: com.xpn.xwiki.api.Document#saveAsAuthor needs to consider required rights. I suggest the following semantics if the current programming document has required rights enabled:

  1. the saved document must have required rights enabled, too.
  2. the saved document must not have more rights than the current document (e.g., a document with just script right must not save a document with wiki admin right).

Any opinions?

1 Like
#16

Another thing: What do you think about renaming “Required Rights” to “Document Rights”? Basically, the idea would be to differentiate between rights that a user has on the document and what rights the code or settings in the document have. I’m also open for other suggestions, but “Required Rights” as a name for a set of rights that primarily limits what the code in the document seems wrong to me.

1 Like
#17

To be honest, it’s not easy to be sure about such a system before actually seeing it working and using it, but yeah, let’s try to reduce surprises as much as we can.

I’m not sure I fully understand what you mean here. You mention a very generic Right+EntityType pair based setup, but you seem to suggest that there would only be a limited choice of valid pairs, actually ?

I briefly thought about “Author Rights” as it supposed to be what the author of the script is allowed to do. Now, from what I understand, the plan is to have the setup for the whole document while there can be several different authors in the same document (content author, xobjects author, etc.) so it might be confusing.

#18

The idea would be to keep the back-end API as generic as possible but to only offer a few meaningful choices in the front-end. Extensions could offer the option to add other required rights, if, e.g., an XObject of an XClass from an extension needs a special right to work. For this, the extension would primarily need to add a UI for adding that required right.

I had two further ideas that we should implement for the migration to required rights:

  1. An API for macros to indicate which rights they require, with concrete content and concrete parameters. For wiki macros, it would either be a static property or some Velocity evaluation I assume.
  2. A similar API for an XClass to indicate what rights it requires, again taking into account a concrete object.

Both of these APIs would return the same (Right, EntityType) pairs. In my opinion, such a generic return type is easier than having, e.g., RequiredRight interface with several implementations, as we would, e.g., need string equivalents for every implementation to be able to use them from Velocity.

The objective of these APIs would be the following:

  1. Help identifying documents that are missing (required) rights.
  2. Prompt the user to add required rights when a missing required right is found.
  3. Possibly prevent saving if the user/document doesn’t have the indicated rights.

“Author Rights” also sounds good, but indeed it might be confusing. Still, I think it is also matching as it is the right that we a) require from each author and b) to which we limit each author during right checks. Something like Author(Right)AuthorizationManager also doesn’t sound too bad in my opinion.

#19

There are two different aspects (and APIs) in this area:

  • some xobjects needs to be in a document with the proper rights to have the expected effect (translations, ui extension, and other wiki components), I guess that’s the one you had in mind
  • some xclasses properties might need script or programming right because of some customization (scripted DB lists, custom displayer, etc.)