[MGNLUI-318] Deletion of assigned roles and/or groups should be prevented Created: 03/Dec/12  Updated: 16/Aug/13  Resolved: 16/Jul/13

Status: Closed
Project: Magnolia UI
Component/s: security app
Affects Version/s: 5.0
Fix Version/s: 5.1

Type: Task Priority: Critical
Reporter: Federico Grilli Assignee: Jozef Chocholacek
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Acceptance criteria:
Empty
Task DoR:
Empty
Date of First Response:

 Description   

at the very least a message should warn beforehand that a given role/group is assigned to an existing (AND enabled?) user.



 Comments   
Comment by Jozef Chocholacek [ 12/Jul/13 ]

In contrary to the MGNLUI-316 where I've opted for disabling the action for the node corresponding to the current user, here I will change the delete action itself (i.e. create new one) where I will check whether the role/group is already assigned, and display an error message to the user if so.

Comment by Eric Hechinger [ 16/Jul/13 ]

In DeleteRoleAction

  • in getErrorMessage() you check
    if (assignedTo == null || assignedTo.isEmpty()) {
    but this is already done in the executeAfterConfirmation just before calling getErrorMessage8...
  • in getErrorMessage()
    Should use StringBuffer instead of String concatenation

Same remarks for DeleteGroupAction.

Would it not be better to create an Abstract Action that expose getErrorMessage instead of duplicate them.

Comment by Daniel Lipp [ 16/Jul/13 ]

I think there's more potential to avoid DRY - the two actions are almost identical. Two proposals to simplify: a) introduced common supertype e.g. AbstractDeleteGroupOrRoleAction (sry - can't think of a better name now) or even just have one DeleteGroupOrRoleAction that e.g. has a flag telling whether it's currently used for deleting a Group or a Role.

Comment by Daniel Lipp [ 16/Jul/13 ]

a) there's more potential to be DRY

  • getUsersAndGroupsThisItemIsAssignedTo could be implemented in new abstract super type like:
        protected List<String> getUsersAndGroupsThisItemIsAssignedTo() throws RepositoryException {
            List<String> assignedTo = new ArrayList<String>();
    
            String groupName = getItem().getJcrItem().getName();
            // users
            for (User user : Security.getUserManager().getAllUsers()) {
                if (getGroupsOrRolesFor(user).contains(groupName)) {
                    assignedTo.add(PREFIX_USER + user.getName());
                }
            }
            // groups
            for (Group group : Security.getGroupManager().getAllGroups()) {
                if (getGroupsOrRolesFor(group).contains(groupName)) {
                    assignedTo.add(PREFIX_GROUP + group.getName());
                }
            }
            return assignedTo;
        }
    

only getGroupsOrRolesFor(User) and getGroupsOrRolesFor(Group) have to be implemented in the subclasses

b) we don't have i18n for error messages - so executeAfterConfirmation could just have the line

            log.error("Cannot get the users/groups the group or role is assigned to.", e);

and one log on the abstract supertype would be enough

c) StringBuilder would be even better than StringBuffer - the builder isn't synchronized so it's much faster

d) looks like there's no unit-tests for the new types - pls add

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