[MGNLFORUM-270] DefaultForumManager#isModerator() checks only the directly attached roles of the user, but ignores his roles inherited from his groups Created: 11/Aug/14  Updated: 28/Aug/14  Resolved: 12/Aug/14

Status: Closed
Project: Forum (closed)
Component/s: moderation
Affects Version/s: 3.3.3, 3.4.1
Fix Version/s: 3.3.4, 3.4.3

Type: Bug Priority: Critical
Reporter: Christian Ringele Assignee: Milan Divilek
Resolution: Fixed Votes: 0
Labels: maintenance, next, support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: Text File DefaultForumManager.patch    
Issue Links:
causality
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Date of First Response:

 Description   

When trying to edit a form message, not only the ACL is checked, but also the method isModerator() is called on the DefaultForumManager.

This method only checks if the user has the roles "forum_ALL-admin" and "forum_ALL-moderator" directly assigned to the user. (currentUser.hasRole()).
But it is not checking, if the user has this role "inherited" be a group he is part of.

This means, if you have the role "forum_ALL-admin" only as a role of a group, you won't be able to edit a message, even if you have the content access rights to the data.

This is a big problem for all AD/LDap users. As AD/LDap users are matched by their user name or user group, one can not directly assign a role to a ad user, only groups. So even if the logged in AD user has the role by one if its group, he can not edit a message.

Former code:

@Override
    public void isModerator() throws AccessDeniedException{
        User currentUser = MgnlContext.getUser();
        if (!currentUser.hasRole(ROLE_FORUM_ALL_MODERATOR) && !currentUser.hasRole(ROLE_FORUM_ALL_ADMIN)) {
            throw new AccessDeniedException("User not allowed to perform that action.");
        }
    }

Should be changed to:

@Override
    public void isModerator() throws AccessDeniedException{
        User currentUser = MgnlContext.getUser();
        boolean hasRole = false;
        // Needs to use getAllRoles() instead of .hasRole() because .hasRole() will only check for the roles directly attached to the user, but not the ones inherited from the group.
        // As roles can not directly be attached to a AD user, it is crucial to be able to define it over its group.
        Collection<String> allRoles = currentUser.getAllRoles();
        for (Iterator<String> iterator = allRoles.iterator(); iterator.hasNext();) {
            String roleName = iterator.next();
            if (roleName.equals(ROLE_FORUM_ALL_MODERATOR) || roleName.equals(ROLE_FORUM_ALL_ADMIN)) {
                hasRole = true;
            }
        }

        if (!hasRole) {
            throw new AccessDeniedException("User not allowed to perform that action.");
        }
    }

The "currentUser.getAllRoles();" returns all roles also the ones form the user's groups.

I added the patch of the class.
But tests are failing because the mock user returns a empty list on .getAllRoles();
Test should be fixed accordingly.



 Comments   
Comment by Peili Liang [ 13/Aug/14 ]

The master fixVersion should be 3.4.3, it will be created when we release 3.4.2.

Generated at Mon Feb 12 02:02:19 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.