[MGNLUI-2510] UI shouldn't enable actions for which the user has no permissions Created: 09/Dec/13  Updated: 21/Apr/15  Resolved: 12/Mar/14

Status: Closed
Project: Magnolia UI
Component/s: page editor, pages app
Affects Version/s: 5.2
Fix Version/s: 5.2.3

Type: Bug Priority: Critical
Reporter: Samuel Staehelin Assignee: Mikaël Geljić
Resolution: Fixed Votes: 0
Labels: support, ux
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified
Environment:

demo.magnolia-cms.com


Attachments: PNG File MGNLUI-2510-page-readonly.png     PNG File MGNLUI-2510-page-readwrite.png    
Issue Links:
Relates
relates to MGNLUI-2557 Show permissions in status column Closed
relates to MGNLUI-3395 WritePermissionRequiredRule incorrect... Closed
causality
caused by MAGNOLIA-5537 DefaultACLBasedPermissions do not acc... Closed
caused by MAGNOLIA-5541 MarkNodeAsDeletedCommand should check... Closed
caused by MGNLWORKFLOW-179 Any user can launch a workflow regard... Closed
is causing MGNLUI-2642 Publisher should not be allowed to op... Closed
is causing MGNLUI-3036 Availability of duplicate action does... Closed
relation
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   

This ticket originally helped us uncover security issues in core (see issue links, first comments), but the UI should also adjust itself correctly to denote correct user permissions, by disabling unauthorized actions, and maintaining proper state according to such permissions.

For 5.2.x, AvailabilityDefinition should gain a #writePermissionRequired flag.
For 5.3+, we will have multiple/configurable AvailabilityRules (will be captured by another issue).
More details in linked concept page 'Permissions for UI availability'.

– ORIGINAL DESCRIPTION –

I changed the permission of the editors in a subtree to read only.
The following issues occurred if I'm logged in as a editor:

  • Sometimes it renders a page without the components (fine) but I still can edit the page properties
  • I can exclude channels (page title I can't change)
  • I can add a page (just not selecting a template)
  • Some pages do not render (stay grey)


 Comments   
Comment by Philipp Bärfuss [ 10/Dec/13 ]

We have to analyze wether this is a security or app related issue. In any case this is what should happen:

  • the edit action is only available if the user has edit permissions
  • the detail view renders in preview if the user has only read permissions
Comment by Mikaël Geljić [ 10/Dec/13 ]

First, all actions are available, regardless of such ACLs.

By default, with user eric and an ACL to make the whole About subtree read-only, we were able to:

  • mark a page as deleted
  • move a page relatively to its siblings
  • start publication workflow
  • open a read-only page in page-editor, although Action bar is empty then

We were not able to:

  • edit the page properties when in page editor, nor select any area or component
  • expectedly: change templates, duplicate, import, rename, add a page (since we cannot select any template, the creation dialog validation won't pass)

So far we were not able to reproduce:

  • page rendering issues
  • opening page properties dialog to change them

Will now run more testing with "colliding" groups or roles.

Comment by Mikaël Geljić [ 10/Dec/13 ]

Further tests with "conflicting" roles: we added a new role to eric with an ACL granting RW access to the Large Article page.

  • Editing page and properties works nicely on Large Article
  • Editing sibling pages is correctly prevented
  • Page bar and Action bar are not reflecting navigation though. They still refer to large-article, tricking you into thinking you can edit another page's properties.
Comment by Mikaël Geljić [ 10/Dec/13 ]

debugging the Move use case, the following permission check seems to (wrongly) pass:

if (!acMgr.isGranted(childPath, Permission.MODIFY_CHILD_NODE_COLLECTION)) {

This is in org.apache.jackrabbit.core.NodeImpl.orderBefore(Element, Element)

Comment by Mikaël Geljić [ 11/Dec/13 ]

Repurposing ticket for UI glitches related to permissions, though they do not affect security of data. These are action availability issues (currently they don't take permissions into account) and page-editor state when navigating between RW and RO pages.

Comment by Andreas Weder [ 18/Dec/13 ]

In general, I prefer to disable actions over simply not showing them.

The list of actions should remain constant. This is particularly true, if you e.g. select different pages in the page tree, for some of which you do have the permission to e.g. edit the page, for others you don't. Disabling an action helps in understanding that an action is actually there, but is just unavailable now and/or to me. So I would prefer to always get the "edit" action, but have it disabled if I lack necessary permissions.

In contrast, I would like to be able hide all actions for a specific product feature.

To explain this, let's say that we have a section in the Action bar that allowed a user to personalize a page. If I don't have access to the entire personalization feature, I should not see any actions related to that feature - the entire section and its actions should be hidden, not merely disabled.

This is different from disabling actions due to access rights. I might have a specific access right for one page, but not for another one: disabling the action is the right thing to do then. If I lack access to an entire product feature, I should not see anything related to it in the action bar, as I won't be able to use it anyway.

Comment by Mikaël Geljić [ 21/Jan/14 ]

Stopped progress, now compiling results and input so far into the wiki - requires further discussion. See issue links: wiki page 'Permissions for UI availability'.

Comment by Mikaël Geljić [ 29/Jan/14 ]

Results from 'Permissions for UI' meeting

  • We aim at unifying availability's access, ruleClass and other criteria using voters, in a future major version.
    • supports custom permissions (forum), even non-JCR based, using dedicated voters
  • For 5.2.x, we introduce a delegating AvailabilityRule which helps us already start working with voters, getting well prepared for migrating to the next approach
  • MGNLUI-2510 is off the radar for 5.2.2

This issue will only track work for 5.2.x.

Comment by Andreas Weder [ 20/Feb/14 ]

Added ux label as I'd like to track this in the UX team.

Comment by Mikaël Geljić [ 07/Mar/14 ]

Updated description following architecture review of Availability for 5.2.x and 5.3. In the end voters are out of the picture.

Comment by Mikaël Geljić [ 07/Mar/14 ]

implemented and added #writePermissionRequired to the following set of pages browser actions:
"add", "confirmDeletion", "edit", "editPageName", "editTemplate", "restorePreviousVersion", "import", "move", "restoreVersion".
- see attached screenshots

Comment by Roman Kovařík [ 10/Mar/14 ]
  • Task name and description is missing:
     public SetWritePermissionForActionsTask(String actionsPath, String... actionNames) {
     	 	        super("", "");
    
  • Duplicate action is missing?:
    .addTask(new SetWritePermissionForActionsTask("/modules/pages/apps/pages/subApps/browser/actions",
                            new String[] { "add", "confirmDeletion", "edit", "editPageName", "editTemplate", "restorePreviousVersion", "import", "move", "restoreVersion" }))
    
  • The linked support ticket reports this issue with security app..not sure if we have time to fix it as part of this ticket or rather create new one or wait for more complex solution...
Comment by Mikaël Geljić [ 10/Mar/14 ]

Thanks Roman,
1. I knew I would forget filling up the description by not doing it immediately
2. For the duplicate action, I "chose" not to disable it because there are cases where it can work (duplicating a readonly page when parent is readwrite). In this case I thought it would be harder to explain why duplicate is disabled than getting anyway the error that duplicate doesn't work within readonly hierarchy…
3. Indeed, if that's what the support ticket claims, we should do it for security too then. Not too much of a big deal IMO, I need to check which actions we have there.

Comment by Mikaël Geljić [ 10/Mar/14 ]
  • Added task name/description
  • Added write permission checks for security app

However there's a known general limitation that is worth mentioning on availability: root node availability bypasses all other conditions. It relies on checking null item instead of the actual root node, so we have no workspace and root path info to check availability against. This won't be fixed until availability is revamped in 5.3.
A concrete visible side-effect is that if a user has readonly access to security app (whatever sense that makes), when browsing through subapps for the first time (without selecting anything) actions (e.g. Add group, Add folder) are still available.

Comment by Christoph Meier [ 11/Mar/14 ]

QA-comment:
If a user can only preview a page (not edit), clicks the action "preview", then the actionbar still has the action "Edit page" ... which will lead to allow to edit page properties. (The attempt to save changed page properties fails with AccessDeniedException, ...).
Disable the action "Edit page".

Comment by Milan Divilek [ 11/Mar/14 ]

Reopen: As a superuser I can't edit superuser user and forum_ALL_admin role (not sure if something other I found this two case).

Generated at Mon Feb 12 08:57:22 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.