Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-17533: Allow to set custom rights in administration #1644

Merged
merged 4 commits into from Jun 17, 2021
Merged

Conversation

surli
Copy link
Member

@surli surli commented Jun 14, 2021

JIRA

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

Description

This PR is an experiment to find a trade-off for allowing to set custom rights, without having to rewrite entirely the whole rights UI, and without breaking the existing one.

Here's what I said about it on the chat:

guys I'm thinking about a trade-off solution for the custom right UI problem we currently have. To sum up, right now when a custom right is injected it's not possible to configure it through the Rights UI and there's no extension point to inject it. So currently the only solution is to code a dedicated UI for the custom right. In my case, I'm currently writing an extension that will need at least 2 custom rights and I'm thinking how do I deal with that issue. So far I was seeing 3 options:

  1. to re-write entirely the rights UI with the custom rights taken into account -> obviously that's a long term solution which will take a lot of time that I don't have right now
  2. to only perform the work allowing to inject the custom rights in the existing rights UI -> I was checking that one, and it seems not that easy without breaking things, moreover right now we're displaying all the rights as table columns so it will quickly won't fit
  3. to develop my own UI for configuring the rights elsewhere -> besides the fact that we'd need to fix https://jira.xwiki.org/browse/XWIKI-18723 it also implies a bit of work to allow configuring them properly for all users / groups. I surely could take inspirations from the currently rights UI but I can avoid it I would gladly.

BUT, now that I looked a bit more the code of the rights UI, I might see a 4th solution:
4. to provide a dedicated administration section for the custom rights. So it's basically solution 2, without the problem of the size of the table, and with the advantage that it seems easy to code, since it's almost how the UI for "Create wiki right" has been implemented (cf: https://github.com/xwiki/xwiki-platform/blob/master/xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-resources/src/main/resources/flamingo/rightsUI.vm#L43).

So IMO that solution 4 would be a quickfix allowing to quickly answer a real need we have without implying a big change or to recode something multiple times in the mid-term, even if on the long run it would be discarded in favor of solution 1. WDYT?

Implementation

  • Provide a new API to retrieve all rights names in
    SecurityAuthorizationScriptService
  • Provide a new administration section to configure the custom rights
  • Edit rightsUI.vm to allow customize custom rights, without breaking
    the existing rights and UIs mechanisms

Screenshot

Here's a screenshot after the changerequest extension has been installed (which currently misses a translation for its custom right)

ui-custom-rights

  * Provide a new API to retrieve all rights names in
    SecurityAuthorizationScriptService
  * Provide a new administration section to configure the custom rights
  * Edit rightsUI.vm to allow customize custom rights, without breaking
    the existing rights and UIs mechanisms
@tmortagne
Copy link
Member

No way to have "Custom Rights" right below "Rights" ? I feel it would help discoverability.

@vmassol
Copy link
Member

vmassol commented Jun 14, 2021

No way to have "Custom Rights" right below "Rights" ? I feel it would help discoverability.

I definitely agree

@surli
Copy link
Member Author

surli commented Jun 14, 2021

So for information, after a discussion with @vmassol on the chat, I checked if there was XWiki contrib extensions registering custom rights and providing an UI for them. My findings are that one contrib extension is using custom rights: the Discussion application, which does not provide any UI for setting them right now.
Besides that one, we have the Like application which is providing a custom right, but it's in XWiki Standard already.

In my knowledge, and putting aside the Change request application, those are the two usages of the custom rights registration, and none of them is providing a custom UI for settings the rights.

@surli
Copy link
Member Author

surli commented Jun 14, 2021

No way to have "Custom Rights" right below "Rights" ? I feel it would help discoverability.

I need to check a bit more, but AFAIR the section in "Users and groups" are hardcoded, except the last current one "Authorization". So I can probably put it below rights, but it would involve either to recode all sections to use order, or to hardcode it too.

  * Put the custom right section at the right place
@surli
Copy link
Member Author

surli commented Jun 15, 2021

I need to check a bit more, but AFAIR the section in "Users and groups" are hardcoded, except the last current one "Authorization". So I can probably put it below rights, but it would involve either to recode all sections to use order, or to hardcode it too.

So I was actually wrong about that, I can put the section on the right place using the appropriate order, which I did in my last commit. I updated the screenshot accordingly.

Note that right now the changes only allow to configure those custom rights globally in the wiki since I'm using a ConfigurableClass (see https://jira.xwiki.org/browse/XWIKI-18723). Once this issue will be fixed, we can imagine having it also for space administration, but there will be the question of calling it for the space or for the page only: right now we have two rights UI in space administration, so it would double it when using the custom rights.

Comment on lines 39 to 51
<content>{{velocity}}
### Sheet used to generically display the XWikiPreferences object fields in the administration sheets.
{{html}}
&lt;form method="post" action="$xwiki.getURL($currentDoc, 'saveandcontinue')" class="xform"&gt;
############################################################################################
## RIGHTS
############################################################################################
&lt;fieldset&gt;
#template('rightsUI.vm')
&lt;/fieldset&gt;
&lt;/form&gt;
{{/html}}
{{/velocity}}</content>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding a PageTest for this document?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quite frankly I'm not sure it worthes it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would you not? Templates are very easy to break on change, so adding some test (even if it's only testing what you changed) seems valuable to me.

Copy link
Member

@vmassol vmassol Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@surli said differently, where do you validate that the HTML is correct and that it works? do you have a functional test? it's definitely needed to test somewhere this code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I don't have test for now for this. I'll try to provide it for 13.5, I'll merge it like that for now so that I can ensure it's possible to have it for 13.5RC1. Note that I tested it manually and also this code is a copy/paste of what's in AdminWikisRightsSheet.

@@ -40,13 +40,25 @@ $xwiki.ssfx.use('js/xwiki/usersandgroups/usersandgroups.css', true)
$xwiki.jsfx.use('js/xwiki/table/livetable.js', true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of adding a PageTest for this template?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same answer

#if("$!request.section"=='wikis.rights')
#set ($standardRights = ['view', 'comment', 'edit', 'script', 'delete', 'admin', 'register', 'programming', 'login',
'createwiki'])
#set ($sectionWikiRights = "wikis.rights")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very minor, could be in single quotes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any advantage to use single quote vs double quote? I generally use double quote because of the Java habit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC single quote content is not interpreted whereas double quotes content is, making the single quote alternative slightly faster on static content such as this one.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so indeed might worth it to change that, thanks

Copy link
Member

@vmassol vmassol Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it's our best practice to use single quotes to the max (for the reason mentioned by Manuel)

@surli
Copy link
Member Author

surli commented Jun 15, 2021

For information I opened a vote for the wording of the section: https://forum.xwiki.org/t/section-name-for-displaying-non-standard-rights/8892

Also after discussing with @mflorea on the chat I investigated on the possibility to integrate the new rights LT directly in the same Rights section that currently exists, however I abandoned the idea since it was lots of work to make it work properly and ensuring to not cause any regression. I preferred to play it safe here.

  * Rename Custom Rights to Extension Rights and improve a bit the
    template
  * Use simple quote in the velocity constants
@surli surli merged commit f5ba791 into master Jun 17, 2021
@surli surli deleted the XWIKI-17533 branch June 17, 2021 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants