XWIKI-12171: Add a script right to manage script macro execution permissions #410
Conversation
…issions - Added a new 'script' right - Checking it in: -- VelocityMacroPermissionPolicy (2.x+ syntax) -- XWikiVelocityRenderer (1.0 syntax) -- AbstractDocumentTitleDisplayer - Added support to check it in DefaultContextualAuthorizationManager (since it is checked for the document content's author instead of the current user, just like PR is checked) - Did not touch com.xpn.xwiki.objects.classes.PropertyClass#displayCustom since it will be delegated to rendering, which was handled above
…issions - Included the new right in the rights management UI in Administration
…issions - Also check the script right for groovy scripts running under a 'secure' Groovy Customizer -- This also makes it consistent with the Velocity macro's behavior, as suggested by Thomas Mortagne
XWIKI-12171: Add a script right to manage script macro execution permissions
There was a problem hiding this comment.
I wonder whether it's good to check rights in the displayer component. I know it's more convenient, but it feels like we're mixing the concerns. Ideally, the component (e.g. a script service) that calls the displayer should check the rights (view, script, etc.). The displayer could have a parameter to control whether the displayed text must be evaluated or not. It's a bit more cumbersome but at lest we separate the concerns.
There was a problem hiding this comment.
The general architecture is to check for security at the boundaries of the XWiki system and once in, to not check again for perf reasons. So I think I agree with Marius.
There was a problem hiding this comment.
Below is the initial reply I wanted to give you:
I think we can use the existing DocumentDisplayerParameters#isTransformationContextRestricted() flag and check it here to determine if we use evaluateTitle (when the flag is not set) or if we use the rawTitle (when the flag is set).
Regarding existing usage that we need to fix:
- In api.Document.getDisplayTitle/getPlainTitle/getRenderedTitle(..) we check the script right and if it's denied, we set the isTransformationContextRestricted flag and call the document displayer.
What we also need is to add a new public String getRenderedTitle(Syntax outputSyntax, DocumentDisplayerParameters parameters, XWikiContext context) method signature in XWikiDocument that we can call from api.Document or from wherever else we want to perform the same thing in java code.
- In DisplayScriptService we need to check the script rights on the document to display, but only for the
Basically doing this, we make sure we don`t get in the way of platform code that does not care about rights but still allow high-level code (api) to check the rights during the display/rendering.
WDYT?
However, after thinking a bit more, by using MacroPermissionPolicy we already introduce the script right checking during the rendering/transformation phase, not before, as you suggest we should do for the title displayer.
So if we handle the title displayer that way, we are not consisten with the way we handle the script macros.
I think that, above all, we should be consistent.
IMO, if we move all the rights checking before the rendering, for both the title displayer and the macro transformations (though we would have to think about how we could do that consistently), we risk introducing a lot of security holes for code that forgets to make the security checks and we also can not be backwards compatible with code that does not expect the need to check the rights first.
So what I am saying is that, indeed, it could be done for title displayers that would require extra plumbing every time we want to use them internally (non-public api way), but is it worth it?
WDYT?
4 participants
No description provided.