XWIKI-22656: Add a UI for setting required rights on a document #4158

michitux · 2025-05-13T14:11:20Z

Jira URL

https://jira.xwiki.org/browse/XWIKI-22656

Changes

Description

Add a new REST API for getting analysis results and the current required rights including some more metadata for a document
Add a UIX in the document information tab for showing information about the current required right(s) and a button to review the required rights.
Add a UIX above the document to show a warning when a document doesn't have enough required rights.
Add a modal to modify required rights.
Add UI tests for the new UI.

TODO:

Add a REST API for saving required rights to properly handle cases with several existing required right objects.
Display a proper, localized warning when unexpected rights are configured.
Reload the content in view mode when saving required rights.
Reload the WYSIWYG editor when saving required rights in edit mode.
Improve the appearance of the UI.

Clarifications

The commits represent the history how I implemented it, but I wouldn't recommend them for review. In particular, they contain code like a script service that I've removed again.
I found it very difficult to implement all this logic in Velocity, that's why I've implemented it in Java and JavaScript.
There are two ways to reason about possible required rights updates, one is which rights are needed by the content, mwarking some rights as "required" or "maybe required", and the other is which operations (decrease/increase) the user performs. I found it good to have both views, the operations make sense for the "call to action" in the UI, but for the actual modal, I found it better to have the rights always in the same place and not "actions" that are different every time. This is because I want to avoid that users blindly click on the suggested operation without considering if it makes sense (or if they're approving an attacker's script).
I would love to get some feedback on the appearance of the UI. I tried to implement the forum proposal, but I didn't find it that easy. @tkrieck maybe you could propose some improvements?

Screenshots & Video

2025-05-13-Required-Rights-Demo.mp4

Executed Tests

Built all changed modules with the quality profile:

LANG=C.UTF-8 mvn clean install -Pdocker,legacy,integration-tests,snapshotModules,quality -pl $(git diff --name-only  --merge-base origin/master | awk -F'/' '{for(i=NF-1; i>0; i--){if($i ~ /src$/){printf(":%s\n", $(i-1)); break;}}}' | sort -u | paste -sd "," -),:xwiki-platform-security-test

Expected merging strategy

Prefers squash: Yes
Backport on branches:
- None for now

* Add a script service for required rights.

* WIP UI.

* Continue WIP UI

* Add an API to api.Document to get required rights.

* Remove the WIP UI hack.

* Move the code of the script service into dedicated components. * Add an analyzer for analyzing just the content and title of the document. * Add an analyzer for analyzing a whole document including all translations. * Update and add tests.

* Add a REST API to get the analysis results and the available rights. * Add a JavaScript component for a dialog to set required rights. * Add a UI Extension to warn users about missing rights. * Add a UI Extension for the page information to display and set required rights.

* Update the module versions to 17.2.0-SNAPSHOT. * Update the design based on the proposal. This is WIP as we can't use color theme variables currently. * Move the missing required rights warning to XWiki syntax to be able to get a proper warning box with icon etc. * Add a dependency to the UI to the flavor.

* Update module versions to 17.3.0-SNAPSHOT

* Add tests and localization for the REST resource. * Add localization to the required rights dialog. * Use CSS variables for the color theme. * Move the existing localization out of oldcore. * Add events to the required rights dialog.

* Update the version in the POM

* Reload the information and the warning above the document when the document is saved.

* Remove the script service for required rights again as it isn't used.

* Remove the Document API for getting required rights and instead use the required rights manager in the REST API.

* Change the wording for "None" to fit the fact that it's not a right. * Add the right to the tooltip as in particular screen reader and keyboard users might encounter the tooltip out of context. * Remove example data added for development.

* Add page objects. * Add first integration tests. * Make the code easier to test by disabling the buttons and enabling them when the click handler has been attached.

* Update since-versions. * Move TODO into the method body.

* Don't expose the dialog in the JavaScript module.

* Support updating required rights via the REST API. * Change the dialog to use the REST API for saving required rights. * Display a proper warning when there are unrecognized required rights and improve handling for several required rights. * Add a UI test for setting all supported rights. * Extend the Document script API with methods that take EntityReference instead of String for the class name.

manuelleduc · 2025-05-15T07:44:39Z

I just watched the video. This looks great!
Shouldn't we update the "check the rights of the last author [...]" error message on the velocity macro to mention the notion of required rights?

...orm/security/requiredrights/internal/analyzer/XWikiDocumentContentRequiredRightAnalyzer.java

...i/platform/security/requiredrights/internal/analyzer/XWikiDocumentRequiredRightAnalyzer.java

surli · 2025-05-15T07:52:24Z

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/api/Document.java

@@ -1229,6 +1263,21 @@ public Vector<Object> getObjects(String className)
        return getXObjects(objects);
    }

+    /**
+     * Retrieves and returns all objects corresponding to the class reference corresponding to the resolution of the
+     * given entity reference, or an empty list if there are none.


I would specify in the javadoc that the list might contain null...

It actually cannot contain null elements as they are filtered out by getXObjects(objects). It's the same as for the method taking a String. If we want to change this, I think we should give the new method a new name in order to avoid that is accidentally called (some scripts might already pass an entity reference that is currently implicitly converted to string).

Ah no it's good then, I missed the filter and I was being extra cautious...

I guess in the future we should deprecate the API you replaced that are taking a String instead of a reference. But maybe not now...

But maybe not now...

Yeah, as usually with this kind of API, there is a lot of work before we can but the @Dreprecated annotation (if we don't want to explode the log). Now what we do sometimes is put at least the javadoc @deprecated (which does not have any log impact).

surli · 2025-05-15T07:52:42Z

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/api/Document.java

+                return newObjectApi(obj, getXWikiContext());
+            }
+        } catch (Exception e) {
+            return null;


no log or anything?

I copied this behavior from the other methods that take a String instead of an EntityReference. Now I noticed that actually none of the called methods declare any exception, so we could also just remove it. It might be a breaking change, though, in case a script already calls this method with an EntityReference.

Well if no called method declare it I'm not sure how it could be a breaking change? I think I would clean up the code to remove the catch, and maybe add a log error to the original method?

It could catch non-declared exceptions, that would afaik stop the Velocity script when not catched anymore.

Yeah, it would be much cleaner to remove the catch. Unfortunately, it indeed might not look that much like a new method from Velocity code point of view, so better keep the same behavior.

An alternative would be to name all those new methods getXObject(s), which is closer to XWikiDocument naming. But it's also probably less obvious, and I like the idea that your version will improve performances a bit of any Velocity code which happen to call those methods with EntityReference already.

...in/java/org/xwiki/platform/security/requiredrights/rest/internal/AvailableRightsManager.java

.../java/org/xwiki/platform/security/requiredrights/internal/RequiredRightChangeSuggestion.java

surli · 2025-05-15T07:58:08Z

...rc/main/java/org/xwiki/platform/security/requiredrights/rest/RequiredRightsRestResource.java

+     * @throws XWikiRestException when failing to parse space
+     */
+    @GET
+    DocumentRightsAnalysisResult analyze(@PathParam("spaceName") @Encoded String spaceNames,


I triggers me a bit to see that a GET request triggers an analysis here... is there a risk it could be used to perform DDOS?

The analysis should be less expensive than rendering the page. What we could do is to limit this API to users with edit right as we currently only use this API for users with edit right.

It would probably make sense then. Do you see a limitation with doing that?

Does not make much sense to me, this API is manipulating things which are accessible to a user with VIEW right on the document, so I find it very strange to limit it to EDIT users.

...in/java/org/xwiki/platform/security/requiredrights/rest/internal/AvailableRightsManager.java

surli · 2025-05-15T08:06:10Z

...va/org/xwiki/platform/security/requiredrights/ui/MissingRequiredRightWarningUIExtension.java

+ */
+@Component
+@Named(MissingRequiredRightWarningUIExtension.ROLE_HINT)
+@Priority(100)


I would document that priority.

Good point, I'm actually not sure why I chose that particular priority… I think I'll actually increase it to be above the default of 1000, maybe set it to 2000 such that it is displayed closer to the content. What do you think? From what I could find, the relations application is the only (public) code using this UIXP.

You actually have a few contrib extensions using it: e.g. replication or book version AFAICS; https://github.com/search?q=org%3Axwiki-contrib%20org.xwiki.platform.template.content.header.after&type=code

Indeed, I must have remembered this incorrectly. Now I think none of them define a priority as the UIXP doesn't support any parameters, and we don't support specifying the component priority in UIX defined in wiki pages. And the one defined in a Java component also doesn't specify an order. So basically the question is: should it before or after all those UIX. Do you have any opinions/do you know if any of them has any requirements?

For replication:

readonly uix: it's a crappy workaround that will disappear as soon as replication move to XWiki 15.10.0 (and reuse the page protection system)

conflict uix: honestly, I don't know which of this one of the required right one should be first. I guess I have a slight preference for showing the required right one first just because it's an XWiki Standard thing

as the UIXP doesn't support any parameters

We should probably fix that to introduce a parameter order no? Would be probably cleaner.

Do you have any opinions/do you know if any of them has any requirements?

Well I agree with you that your UIXP should probably be closer to the content than any other currently existing.

Hm, based on the comment of @tmortagne I've now chosen 900 but @surli you're saying the inverse, so it should more be 2000? I don't really care too much, I think one can find good arguments for both options.

Honestly I don't have a strong opinion so either way is fine with me.

surli · 2025-05-15T08:10:54Z

...rity-requiredrights-ui/src/main/resources/js/security/requiredrights/requiredRightsDialog.js

+                                </div>
+                            </div>
+                        </div>
+                    `;


hmmm wouldn't it be cleaner to have that html in a template loaded on demand?

This is the big question of JavaScript vs. Velocity code for UI. I actually started defining the dialog in a Velocity template with script services for getting the data (why would you call a REST API for loading the data if you already do an API call for loading the HTML). I personally found it easier to write this code in JavaScript and the logic for getting the data in a REST API and my hope is that this kind of code that just relies on REST APIs would be easier to port to Cristal. However, I see the point, with Velocity we could for example easily use proper icon HTML.

Basically, this was a combination of a) I don't like Velocity and b) I want to try transitioning to have UI components defined in JavaScript. This is basically similar to defining a UI in Vue.js where also the whole HTML would be in the Vue.js component except that I'm not using Vue.js here.

I've the feeling that we need to define a best practice for this kind of need. Maybe using Vue in this kind of situation would be possible? I've no idea.

* Add a new JavaScript event that signals when required rights have been updated. * Reload the content both in view and edit mode when required rights are updated. * Trigger saved event only when the rights actually changed to avoid unnecessary reloads.

* Add a proper message to the exception in XWikiDocumentContentRequiredRightAnalyzer. * Don't use ArrayList as the type of a variable. * Remove strange line break in AvailableRightsManager.

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/api/Document.java

...g/xwiki/platform/security/requiredrights/internal/RequiredRightsChangeSuggestionManager.java

...ki/platform/security/requiredrights/internal/analyzer/FullDocumentRequiredRightAnalyzer.java

* Don't introduce a new variant of Document#createNewObject.

...ity/xwiki-platform-security-requiredrights/xwiki-platform-security-requiredrights-ui/pom.xml

...va/org/xwiki/platform/security/requiredrights/ui/MissingRequiredRightWarningUIExtension.java

...c/main/java/org/xwiki/platform/security/requiredrights/ui/RequiredRightsInfoUIExtension.java

tkrieck · 2025-05-15T11:41:15Z

The overall design is pretty close to the proposal. A few minor details though:

The internal border on the selected right. This can be changed to a 1px solid border with lighter blue, or removed altogether (if we don't have a theme color for that, for example)

The background color of the panel seems to be lighter than ideal. Even lighter than the panels, however this can be an artifact of the recording, I am not sure.
Again, this can be a recording artifact, but the gray borders on suggested rights can also have their contrast increased.
The Cancel and Save buttons don't seem to be standard from XWiki. At least, the colors seem off.

Thank you for implementing the UI so close to the proposal 👍, as I said earlier these suggestions are mostly details.

tmortagne · 2025-05-15T11:58:04Z

Shouldn't we update the "check the rights of the last author [...]" error message on the velocity macro to mention the notion of required rights?

This message is already wrong for various cases even before this PR, but it requires some refactoring of the handling of this error (it's based on a boolean right now and the current message is just a guess). But that should probably be in another PR.

tmortagne · 2025-05-15T12:04:00Z

By the way, the UX shown in the video looks quite good already.

* Add an integration test for the reloading of the editor on rights changes.

* Add an integration test for the reloading of the editor on rights changes. * Remove the priority on the RequiredRightsInfoUIExtension as it's useless. * Add comments on the priorities of the two UI extensions and since versions. * Fix the module metadata for the required rights UI. * Add comment for None right in AvailableRightsManager.

* Add an integration test for reloading the content after a required rights change.

* Use jakarta.inject instead of javax.inject.

* Rename full analyzer to withTranslations.

* Improve the REST API module description.

michitux added 19 commits May 9, 2025 15:58

XWIKI-20907: Introduce the notion of required rights

caa90c8

* Add a script service for required rights.

XWIKI-20907: Introduce the notion of required rights

68ff5b8

* WIP UI.

XWIKI-20907: Introduce the notion of required rights

74b8343

* Continue WIP UI

XWIKI-20907: Introduce the notion of required rights

f148b7d

* Add an API to api.Document to get required rights.

XWIKI-22656: Add a UI for setting required rights on a document

1d2b73e

* Remove the WIP UI hack.

XWIKI-22656: Add a UI for setting required rights on a document

34f6cfa

* Update module versions to 17.3.0-SNAPSHOT

XWIKI-22656: Add a UI for setting required rights on a document

b1a1c85

* Update the version in the POM

XWIKI-22656: Add a UI for setting required rights on a document

93442d0

* Reload the information and the warning above the document when the document is saved.

XWIKI-22656: Add a UI for setting required rights on a document

13a2175

* Remove the script service for required rights again as it isn't used.

XWIKI-22656: Add a UI for setting required rights on a document

6859771

* Remove the Document API for getting required rights and instead use the required rights manager in the REST API.

XWIKI-22656: Add a UI for setting required rights on a document

b3adcbd

* Add page objects. * Add first integration tests. * Make the code easier to test by disabling the buttons and enabling them when the click handler has been attached.

XWIKI-22656: Add a UI for setting required rights on a document

305a764

* Update since-versions. * Move TODO into the method body.

XWIKI-22656: Add a UI for setting required rights on a document

0a84852

* Don't expose the dialog in the JavaScript module.

manuelleduc reviewed May 15, 2025

View reviewed changes

...orm/security/requiredrights/internal/analyzer/XWikiDocumentContentRequiredRightAnalyzer.java Outdated Show resolved Hide resolved

manuelleduc reviewed May 15, 2025

View reviewed changes

...i/platform/security/requiredrights/internal/analyzer/XWikiDocumentRequiredRightAnalyzer.java Outdated Show resolved Hide resolved

surli reviewed May 15, 2025

View reviewed changes

manuelleduc reviewed May 15, 2025

View reviewed changes

...in/java/org/xwiki/platform/security/requiredrights/rest/internal/AvailableRightsManager.java Outdated Show resolved Hide resolved

surli reviewed May 15, 2025

View reviewed changes

.../java/org/xwiki/platform/security/requiredrights/internal/RequiredRightChangeSuggestion.java Show resolved Hide resolved

surli reviewed May 15, 2025

View reviewed changes

...in/java/org/xwiki/platform/security/requiredrights/rest/internal/AvailableRightsManager.java Show resolved Hide resolved

surli reviewed May 15, 2025

View reviewed changes

michitux added 2 commits May 15, 2025 11:00

XWIKI-22656: Add a UI for setting required rights on a document

b5f13dc

* Add a proper message to the exception in XWikiDocumentContentRequiredRightAnalyzer. * Don't use ArrayList as the type of a variable. * Remove strange line break in AvailableRightsManager.