Right now users see all macros in the WYSIWYG dialog, even macros that they don’t have the rights to execute (e.g. using a scripting macro if you don’t have script rights). This is because it’s the macro that perform permissions checks at execution time. It would be nicer, from a usability POV that users only see macros that they can use.
I’m proposing to add a “required permissions” field in the Macro Descriptor UI.
This would not replace the permission evaluation at macro execution time since that evaluation can be complex and dynamic, based on lots of different contextual information. So the “required permissions” should be seen as an indication only.
Work to be done:
Modify MacroDescriptor to add a list of rights required
Modify the WYSIWYG backend service to only return macros for which the current user has the required rights
Update the Macro Descriptor of existing macros requiring some permissions (at least the ones bundled in XS)
How would that list be interpreted ? Does a user need all those rights or only one of them ? I feel like a single “minimum” right would probably be enough.
Not sure about that. I agree that it should not replace it fully, but we could still check it since it’s supposed to be the minimum required right (after all if it’s strong enough to prevent using this macro at all in the WYSIWYG we could also prevent its execution when it’s added using the wiki editor for example) and then the macro can also add other dynamic restrictions on top of it during the execution. It would ease the implementation of macros with very simple static restrictions.
This information would also be very interesting in the WYSIWYG for something else than adding new macros: adding a visual warning in the WYSIWYG to existing macros that would not work with the current editor as author (like breaking a script macro when you edit an existing document, and you don’t have script right).
One thing to have in mind for the implementation: the right and the user are not enough as input to check a right, you also need to check the right for a specific entity (in the case of the WYSIWYG the currently edited document usually I guess, to be checked if the WYSIWYG macros service do receive this information right now).
+1 for introducing the concept of required permissions.
I’m wondering if it would be a good idea to make this more granular that there can also be parameters or even parameter values that require the right (e.g., the clean parameter of the HTML macro could require a certain right to be disabled). For users without that right, the WYSIWYG editor would not display the parameter or would disable editing the parameter.
For the initial rendering of the document in the WYSIWYG editor, the rights of the saving author are used so in this case, the author is still the same (as this is just the result of rendering the existing, saved document). As soon as a new macro is inserted or something else triggers rendering of changed content, you will see the errors from the executed script macro, anyways, so I’m not sure this is really helping.
I had a different idea (that I wanted to propose at some point) for this situation that I want to mention here as it is related:
Introduce a minimum edit right for each page. Users without that right are no longer allowed to edit that page in a way that makes them the content (or effective metadata) author. From what I understand the uses I have in mind concern primarily scripting and programming rights but it might be interesting to keep this generic. RequiredRightClass seems to be something in this direction.
Any right is only available on a page if it is explicitly set as minimum right. If the user has a right, he can set this right as minimum right on the page. He should be automatically prompted to do this if he introduces a macro, XObject or setting on an XObject that requires this right. If the right is not available but the user still uses this feature, XWiki should prevent saving as much as possible. In some cases this might not be possible as it is undecidable if a Velocity macro will need programming rights or not.
To remove such a required right, the user needs to have that right and a check if content exists that requires this right should not find anything. If there is anything, it should be listed which objects/properties of objects and which macros/macro parameters report that they require the right.
The idea has two goals:
Prevent users without the appropriate rights from accidentally breaking a page that requires a certain right to work correctly.
Prevent social engineering attacks where a user without, e.g., programming rights, adds something to a page and then gets an admin with the right to change the page such that whatever the user added is now executed with programming rights. This “something” might be an additional XObject that is not visible in the default editor which is why I believe this is a potentially dangerous attack. The attacker could further use the gained programming rights to immediately change the history of the page (assuming this is possible with programming rights, I haven’t checked) to hide the attack.
A similar idea could also be to just prevent saving if there is content that requires more permissions than the user has or warn users with more rights if they are giving rights to some content that wanted them but didn’t have them so far. However, I think this is problematic if we cannot say for sure if a right is required or not and might thus produce a lot of false positives where we ask users to give additional programming rights for scripts such that it could a) annoy users and b) lead to users just clicking yes without checking what actually happened.
The user which is used to execute the content is not relevant, and it’s really not what I’m talking about here. My point is that the WYSIWYG has all the needed information to add this warning after the rendering.
Yes, but in many cases the user will get the same information from the red error that is displayed because the macro execution just failed. What you suggest is primarily relevant for the initial load I think.
