[MGNLUI-3395] WritePermissionRequiredRule incorrectly skips superuser role from the checks Created: 03/Apr/15  Updated: 01/Jul/15  Resolved: 30/Jun/15

Status: Closed
Project: Magnolia UI
Component/s: None
Affects Version/s: None
Fix Version/s: 5.3.10

Type: Bug Priority: Neutral
Reporter: Jan Haderka Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
Relates
relates to MGNLUI-2510 UI shouldn't enable actions for which... Closed
relates to MGNLUI-2760 Streamline availability checking. Closed
causality
is causing MGNLHARDLK-57 As an editor I want actions on locked... Closed
dependency
depends upon MAGNOLIA-6161 Allow setting of access manager in mo... Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

Which permissions superuser has is actually governed by the setting, but somehow there is a shortcut in the rule that essentially forbids controlling permissions for anyone who has superuser role even when setting such permissions in the role itself.



 Comments   
Comment by Jan Haderka [ 08/Apr/15 ]

To be merged to 5.3.x after review. Fix on branch is against master/5.4.

Comment by Mikaël Geljić [ 21/Apr/15 ]

Code review:

  • test class has bad name and is in wrong package
    • => no need for extended rule class with increased visibility
  • test doesn't compile: MockContext#setAccessManager???
    • -=> partially mock context instead-
    • EDIT: do as you prefer there, I just noticed the related ticket in main
  • rename test cases in behavior-driven manner

To save time, I did those changes already and pushed them to the branch.

History/QA:
This superuser check was introduced in MGNLUI-2510 (initial ticket about this rule). In particular, it was last reopened after Milan noticed:

  • superuser could no longer edit himself
  • superuser could no longer edit some roles (e.g. rest, forum_ALL-admin)

Unfortunately, this still holds true if we simply remove the check.

Comment by Milan Divilek [ 12/May/15 ]

History/QA:
This superuser check was introduced in MGNLUI-2510 (initial ticket about this rule). In particular, it was last reopened after Milan noticed:
superuser could no longer edit himself
superuser could no longer edit some roles (e.g. rest, forum_ALL-admin)
Unfortunately, this still holds true if we simply remove the check.

Superuser role has read/write ACL to root of userroles repository, but e.g. forum_ALL-admin role has only read ACL to itself. The problem is that if superuser user has both roles (superuser and forum_ALL-admin) then the read-only ACL (from forum_ALL-admin role ) is applied, because the ACL with the longest pattern determines the permission.
To solve this problem info.magnolia.security.app.action.availability.SecurityAppWritePermissionRequiredRule was implemented. The SecurityAppWritePermissionRequiredRule returns true if current user has write permissions for the evaluated items or if current user has superuser role assigned. This rule class is used for systemUsers, users, groups and roles subapps.

Comment by Mikaël Geljić [ 12/May/15 ]

That's the way to go

Comment by Mikaël Geljić [ 13/May/15 ]
  1. Not too keen on making *all* default rules protected, and replacing the whole AvailabilityChecker
    • Rather instantiate those rules via ComponentProvider (make them final while you're at it), and only override implementation mapping for the one rule
  2. What's the rationale for using subapp container ids (times 4) rather than app-security in module descriptor?
  3. There's a wrong copyright somewhere in there (2013-2014), pbly worth remembering where you copied that from
Comment by Milan Divilek [ 19/May/15 ]

2. What's the rationale for using subapp container ids (times 4) rather than app-security in module descriptor?

Component info.magnolia.ui.api.availability.AvailabilityChecker is defined for subapp container In ui-framework module descriptor. So using app-security container had not effect and that's why it was defined for those 4 subapps in security-app.

Comment by Mikaël Geljić [ 15/Jun/15 ]

Well, sadly I haven't got time to get back to this one myself, so I'll quickly drop a comment:
Anyway, if I recall correctly after double-checking within Platform, I was wrong when I called for instantiating the rules via ComponentProvider; essentially rules get configured while the AvailabilityChecker gets injected, and we try not to mix those two worlds too much.

So extending the AvailabilityChecker was the way to go.
However, let's move instantiation of the rules to that #prepareRules methods and make it protected, that should ease things a bit.

Hope that helps, sorry for "holding" that in review for that long :/

Comment by Milan Divilek [ 26/Jun/15 ]

However, let's move instantiation of the rules to that #prepareRules methods and make it protected, that should ease things a bit.

That wouldn't help. Shorthand (default) rules need to be defined globally in the class, because they are in certain use cases removed from list of rule in info.magnolia.ui.framework.availability.AvailabilityCheckerImpl#isAvailable method. See

// Since we can't combine rules with AND/OR operands, 'root' availability should keep precedence over nodes/nodeTypes-allowed rules when the default item is selected.
if (definition.isRoot() && idsToCheck.size() == 1 && ObjectUtils.equals(idsToCheck.get(0), defaultItemId)) {
    rules.remove(jcrNodesAllowedRule);
    rules.remove(jcrNodeTypesAllowedRule);
}

So if we don't want instantiate shorthand rules via Component provider then making shorthand rules protected is best and easiest solution without rewriting whole info.magnolia.ui.framework.availability.AvailabilityCheckerImpl.

I just reverted the commit which introduced instantiating shorthand (default) rules via Component provider.

Generated at Mon Feb 12 09:06:11 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.