Remove not supported moderation-permission (MGNLFORUM-250)

[MGNLFORUM-253] DefaultForumManager#isModerator should work based on roles Created: 05/Mar/14  Updated: 13/Mar/14  Resolved: 13/Mar/14

Status: Closed
Project: Forum (closed)
Component/s: security
Affects Version/s: None
Fix Version/s: 3.3

Type: Sub-task Priority: Neutral
Reporter: Christoph Meier Assignee: Christoph Meier
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Template:
Date of First Response:

 Comments   
Comment by Roman Kovařík [ 10/Mar/14 ]
  • Check for Moderator permission is also performed in:
    • info.magnolia.module.forum.admin.ForumTree.canModerate(Content) ...this file is obsolete and should be removed
    • info.magnolia.module.forum.admin.moderation.ModerationListModel.getResult(...) ...not sure if it's used
  • '1' instead of '0':
     throw new AccessDeniedException(MessageFormat.format("User not allowed to moderate path [{1}].", new Object[] { node.getHandle() }));
    
Comment by Christoph Meier [ 11/Mar/14 ]

The commit onto master was done against the parent ticket MGNLFORUM-250; all subtasks of MGNLFORUM-250 (251, 252, 253, 254, 255) have been committed against 250 on master.

Comment by Milan Divilek [ 11/Mar/14 ]

Reopen:
1.Lets have forum app available for users who has role forum-moderator-base. /modules/forum/apps/forum/permissions/roles/forum-moderator-base
2.Create new user Karel and add him forum-moderator-base and forum-ALL-user roles
3.Login as Karel and open Forum app
4.He can do everything what he wants. e.g. add, remove forum. He doesn't need forum_ALL-admin or forum_ALL-moderator role.

Comment by Roman Kovařík [ 12/Mar/14 ]

display a message (from a task) which tells the user that „theses roles“ are no more used in the module and that he should delete it from its users/groups, etc. …

Comment by Milan Divilek [ 12/Mar/14 ]

Reopen:
addForum, editForum, deleteForum, addThread, editThread, deleteThread, editMessage, deleteMessage actions can be triggered without forum_ALL-admin or forum_ALL-moderator role.

lockForum, unlockForum, approveMessage, rejectMessage actions can't be triggered without those roles, because they are call method info.magnolia.module.forum.DefaultForumManager#isModerator which check if user has one of those roles.

addForum, editForum, deleteForum, addThread, editThread, deleteThread, editMessage, deleteMessage actions should also check if user can moderate forum.

Comment by Christoph Meier [ 12/Mar/14 ]

editForum, editThread, editMessage are NOT handle in ForumManager but by "standard" SaveDialogActionDefinition.
So i added a custom OpenEditForumItemDialogAction which calls ForumManager#isModerator.

To enable that, #isModerator (without args.!) was added to the interface.

While fixing tests, i also refactored those which test action-classes to use JUnit4-pattern; and one twst was added.

Comment by Milan Divilek [ 12/Mar/14 ]

info.magnolia.module.forum.app.action.OpenEditForumItemDialogAction#execute

} catch (AccessDeniedException e) {
    throw new ActionExecutionException(e);
}

This will lead into ugly "Error banner". Instead of re-throw AccessDeniedException as ActionExecutionException we should simply show "Error notification" and stop action. Same for info.magnolia.module.forum.app.action.SaveDialogNewForumAction, info.magnolia.module.forum.app.action.SaveDialogNewThreadAction, etc.

addForum, addThread actions will open dialog also when user is not admin or moderator. info.magnolia.module.forum.DefaultForumManager#isModerator is triggered during save dialog action, but when user is not moderator then "Error banner" is shown and dialog is not closed. If user is not moderator we should not open dialog at all or at least dialog should be closed.

Comment by Christoph Meier [ 13/Mar/14 ]

I'm not sure whether it makes sense to check forumManager.isModerator() in every action.
But if we don't, then this may lead to the (ugly?) "Error banner".
...
Actually all actions should be greyed out, if the user hasn't the appropriate permissions. This seems more reasonable (for me). But this must be confogured and again this config could be changed by a user which has accidentally to much rights.

The ticket was reopend before since it was possible to "outsmart" security which isn't possible anymore.
The last mentioned issue is more a UX-thing which isnt't throughout consequently done everywhere the same way.
I would like to discuss that with an architect and postpone this "issue".

I'll do the change in OpenEditForumItemDialogAction where i already use forumManager.isModerator(), the others i postpone.

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