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-22815: Variable is not computed correctly on subwikis #3877

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manuelleduc
Copy link
Contributor

@manuelleduc manuelleduc commented Feb 6, 2025

Jira URL

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

Changes

Description

  • generalize the condition for $isSuperAdmin to make it work on sub-wikis.

Clarifications

  • the current implementation stays on string comparison. Let me know if using a proper entity reference comparison is preferable (couldn't find an existing script API).

Screenshots & Video

N/A

Executed Tests

  • Build of xwiki-platform-web-templates
  • Manual tests on sub-wikis
  • Also, I found the condition $isAdvancedUser || $isSuperAdmin in at least 16 places, but afaiu $isSuperAdmin implies $isAdvancedUser, making the right hand side of the OR condition superfluous

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • stable-16.4.x
    • stable-16.10.x

@@ -21,7 +21,7 @@
##!unique=request
#template("frequentlyUsedDocs.vm")
#set ($isGuest = $xcontext.getUser().equals('XWiki.XWikiGuest'))
#set ($isSuperAdmin = ($xcontext.user == 'XWiki.superadmin'))
#set ($isSuperAdmin = ($xcontext.user == 'XWiki.superadmin') || ($xcontext.user == "${services.wiki.mainWikiId}:XWiki.superadmin"))
Copy link
Member

@tmortagne tmortagne Feb 6, 2025

Choose a reason for hiding this comment

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

Feels like we should stop have any kind of logic in this template and instead call some Java API which check that as it's supposed to be checked, in a single common location (for example, what your new version is doing is still quite different from what the authorization manager is doing which is currently what should be the reference).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants