Problem/Motivation

Assume that we have user 'A' and user 'B'. User 'A' has the required permissions to access the moderation dashboard, user 'B' doesn't.

Right now if user 'A' navigates user/B/moderation/dashboard, he/she will get a hit and can access that page (however, until #3000637: Add personalized elements to the dashboard, use the User object from the route context instead of the current user won't be merged, he/she will see his/her own moderation dashboard page).

Proposed resolution

Route access checking should be improved. I see two main ways to do this:

  • Create a new Condition plugin which checks that the user provided by context IS the current user as well and add it to the page manager page.
  • Extend the _moderation_dashboard_page_access() function and check {user-from-context} === {current-user} there (adding the $user argument seems to work)

I think that throwing a NotFoundHttpException in the needed case/phase would be the best solution if it is feasible.

Remaining tasks

Decide whether the described case should be resolved or not.
If the answer is yes, then

  • decide how to solve the access check
  • add test coverage
  • provide a patch
CommentFileSizeAuthor
#25 3000641-25.patch20.53 KBbendeguz.csirmaz
#23 3000641-23.patch18.72 KBbendeguz.csirmaz
#20 3000641-20.patch18.79 KBbendeguz.csirmaz
#20 interdiff-3000641-18-20.txt3.14 KBbendeguz.csirmaz
#18 3000641-18.patch16.35 KBbendeguz.csirmaz
#17 3000641-17.patch16.35 KBbendeguz.csirmaz
#12 interdiff-3000641-11-12.patch825 bytesbendeguz.csirmaz
#12 3000641-12.patch13.53 KBbendeguz.csirmaz
#11 3000641-11.patch13.38 KBbendeguz.csirmaz

Comments

huzooka created an issue. See original summary.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

Accessing another user's dashboard is desired behavior, I think. If you're a reviewer or manager it would be useful to check in on your content editors to see what's in their dashboard. The case where user B doesn't have the permissions to use the dashboard and user A visits their dashboard is interesting but not something we need to handle as a special case, I think.

So maybe resolving #3000637: Add personalized elements to the dashboard, use the User object from the route context instead of the current user is the best next step?

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented

What about adding a permission for the case you described?

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

@huzooka I'd be OK with that, if you think it would be useful. Since Views does its own access checking I don't think any sensitive information would be disclosed by viewing another user's dashboard, but maybe it would be confusing if content editors could click that tab on a non-content-editor's user page.

Maybe we should only show the tab on another user's page if the current user has the new permission _and_ the other user would normally be able to visit the dashboard. You wouldn't want to see the dashboard tab for a user who's not a content editor, if that makes sense.

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Category: Task » Feature request

My suggestion is to have the following two permissions:

  • view own moderation dashboard
    • users with this permission can view their own dashboard
    • users without this permission don't have a moderation dashboard at all, meaning nor they, nor anyone else can view their moderation dashboard
  • view any moderation dashboard
    • users with this permission can view the dashboards of those who have the "view own moderation dashboard" permission
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented

The problem with this is, what if you don't want the content editor (someone with the "view own moderation dashboard" permission) to see their dashboard, but you still want to see it as an admin user (someone with the "view any moderation dashboard" permission)?
We need something else for deciding whether a user has a moderation dashboard or not.
It could either be a third permission, or a flag on the User entity, or something else.

We could use the existing "use moderation dashboard" permission for this.

What do you think @samuel.mortenson?

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

@bendeguz.csirmaz I think "use moderation dashboard" and "view own moderation dashboard" are equivalent permissions, if you want to rename it you can, but if a user has the "use moderation dashboard" permission, they have a dashboard. So the two cases we should cover:

1. User is viewing their own dashboard, check if they have the "use/view own moderation dashboard" permission.
2. User A is viewing User B's dashboard, check if User B has the "use/view own moderation dashboard" permission and User A has the "view any moderation dashboard" permission.

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented

Okay, so just to confirm: we shouldn't cover the case where where User A can view User B's dashboard, but User B can't?

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented

Confirmed, in that case - I would expect access to be denied. Otherwise the dashboard tab would be available to admins on all user accounts and they wouldn't know when it's relevant for a specific user. I see your point about a third permission now but don't want to complicate the logic too much.

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Assigned: Unassigned » bendeguz.csirmaz
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Assigned: bendeguz.csirmaz » Unassigned
Status: Active » Needs review
StatusFileSize
new 3000641-11.patch13.38 KB
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
StatusFileSize
new 3000641-12.patch13.53 KB
new interdiff-3000641-11-12.patch825 bytes

Fixed coding standards.

Status: Needs review » Needs work

The last submitted patch, 12: interdiff-3000641-11-12.patch, failed testing. View results

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Status: Needs work » Needs review

My bad, interdiff is not a patch.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/src/PageAccess.php
@@ -0,0 +1,139 @@
+namespace Drupal\moderation_dashboard;

    I cant see the advantages of solving the access control by a service.

    This should be solved by a module-provided Condition plugin with two required context, one for the dashboard owner, and one for the current user. If we latter would need it, we'll be able to use the plugin.manager.condition service to get the plugin, set the contexts and check it's result.

    That would simplify the codebase.
    * the condition plugin can be set up in the page_manager.page configuration
    * the moderation_dashboard.route_subscriber service and it's class could be removed as well
    * we don't have to use a procedural function just for asking a service (_moderation_dashboard_page_access)
    * well, _moderation_dashboard_page_access could be removed completely as well
    * we would be able to test the condition plugin with a kernel test as well, without the need of visiting routes. 

  2. +++ b/src/PageAccess.php
@@ -0,0 +1,139 @@
+    if (is_numeric($owner)) {

    If the user parameter would be set up properly in the page_manager.page.moderation_dashboard, you won't need this. 

  3. +++ b/src/Routing/RouteSubscriber.php
+++ b/src/Routing/RouteSubscriber.php
@@ -16,7 +16,6 @@ class RouteSubscriber extends RouteSubscriberBase {

    Remove this completely.

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Assigned: Unassigned » bendeguz.csirmaz
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Assigned: bendeguz.csirmaz » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3000641-17.patch16.35 KB

A few words about this:

  • Validating the configuration form would've been great, because we don't want the same user assigned to the "dashboard owner" and "current user" contexts. However, I couldn't figure out how to do this, because the validation callbacks are never called. These are the ConditionPluginBase::validateConfigurationForm and the ConditionPluginBase::validateContexts functions.
  • Caching: the base class provides caching based on the context values. I also manually tested if the cache invalidates correctly when workflows change, and it does.
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
StatusFileSize
new 3000641-18.patch16.35 KB

Fixed coding standard.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Needs work 
  1. +++ b/src/Plugin/Condition/HasModeratedContentType.php
@@ -0,0 +1,106 @@
+  public function evaluate() {

    I think we need a status config here for this plugin and for moderation_dashboard_access as well since without that it's impossible to use these conditions on the standard block layout.

  2. +++ b/src/Plugin/Condition/ModerationDashboardAccess.php
@@ -0,0 +1,53 @@
+  public function evaluate() {

    Add status for this as well. 

  3. src/Routing/ResponseSubscriber.php

    The response subscriber's onResponse method deserves a little refactor as well.

    • I think that it does not makes sense to redirect the user to a route if it throws 4xx client error. Let's check that the new has_moderated_content_type condition plugin returns TRUE before the response's target url is changed
    • User may have only the view any moderation dashboard permission, so check that as well
bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
StatusFileSize
new interdiff-3000641-18-20.txt3.14 KB
new 3000641-20.patch18.79 KB

I think it would be easier if we just didn't display those conditions on the block form. This can easily be done with the hook_plugin_filter_TYPE__CONSUMER_alter.

Added a check for moderated content types before redirecting.

If a user only has the view any moderation dashboard permission they can't view their own dashboard because they do not have one (this is extensively covered in the test).
Are you saying this permission should also come with dashboard ownership?

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 20: 3000641-20.patch, failed testing. View results

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Status: Needs work » Needs review
StatusFileSize
new 3000641-23.patch18.72 KB

Rerolled.

Status: Needs review » Needs work

The last submitted patch, 23: 3000641-23.patch, failed testing. View results

bendeguz.csirmaz’s picture

bendeguz.csirmaz commented
Status: Needs work » Needs review
StatusFileSize
new 3000641-25.patch20.53 KB

Fixed test.

huzooka’s picture

huzooka
Primary language Hungarian
Location Hungary 🇭🇺🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

Excellent work, looks right to me now, thanks!

@samuel.mortenson, any thoughts?

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

This looks great - little details like not showing the new condition plugins on block forms is a smart addition too. Thanks for all the hard work!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.