Permissions not working on admin/dashboard

This is because the individual blocks are accesible by permission. Its a bit strange indeed.

Well, its a bug? I think its pretty clear that it is.

@Bojhan: could you elaborate:

This is because the individual blocks are accesible by permission.

... what you mean by this?

The blocks you can see, because permissions allow you to see it.

As in,
because they have not explicitly been restricted to certain roles via the block visibility settings?

I can't reproduce on fresh HEAD, getting a plain 'acces denied' for anonymous on /admin (which is where dashboard lives now), without seeing anything dashboardy.

Downgrading for now…

Here's a quick patch that should fix the bug.

This doesn't address the awkward markup, though. The problem is that Drupal 7 theoretically allows the main content block to be placed in any region of the page (too much flexibility?) so it's therefore not so easy for the dashboard module to find where it is and insert stuff into it. I'm guessing it's possible to fix - but probably best as a separate issue.

I'd be interested in a little explanation of what you are doing here. I see in the patch that you add some kind of acces check. Can you do the 'for dummies' breakdown of this problem and fix? Just for kicks and learning :)

Not sure whether this patch is based on the assumption that #652122: Fix dashboard as the default /admin local task is given. If it is, then it needs a different solution.

@yoroy: I'll try for #13. I'm not too sure myself, so please correct me, if that's incorrect.

When you visit the admin/dashboard page (or whatever the URL is currently), the menu router (is that the right word?) is called to determine what to show to you. Part of that menur router is the page callback, which is already being referenced in $is_visible as you can see in the patch. Menu routers also have an access callback though, to see if you (as in the user) is allowed to see the page. That access callback returns either 1 or 0 depending on if you're allowed to see it. So the patch introduces a check to see if the access callback actually does return 1.

Yes, I think that explanation is about right... Although actually, this is probably a really bad example to learn from, since the dashboard is doing some odd things :)

But a bit more detail: If you look at the list of dashboard module menu items, you see this code:

  $items['admin/dashboard'] = array(
    'title' => 'Dashboard',
    'description' => 'View and customize your dashboard',
    'page callback' => 'dashboard_admin',
    'access arguments' => array('access dashboard'),
  );

The last part tells Drupal that only users with the 'access dashboard' permission can view the page. When the page is actually visited, the menu item gets processed to add a new element 'access' which is set to 0 or 1 (or maybe FALSE or TRUE - same thing basically) depending on whether or not the current user actually has that permission.

Drupal automatically uses this information to replace the main page content with the "Access Denied" message when necessary. However, the other blocks on the page besides the main page content still need to be displayed; that's how "access denied" pages still get things like menus, the user login block, etc. For most pages in Drupal, that automatic behavior is enough and is exactly what you want.

In the case of the Dashboard module, the dashboard content is actually added in these other page regions (not part of the main page content) - basically in order to interface with the Block module's method of managing page regions. So it has a custom function dashboard_is_visible() that is called from various places and determines when to add all the dashboard content to the page. So from the patch:

-    $is_visible = isset($menu_item['page_callback']) && $menu_item['page_callback'] == 'dashboard_admin';
+    $is_visible = isset($menu_item['page_callback']) && $menu_item['page_callback'] == 'dashboard_admin' && !empty($menu_item['access']);

Previously it just checked the 'page callback' from above - to see if we are on the correct page. And afterwards, it checks the 'access' property too, to make sure the current user is actually supposed to be able to see it.

Re #14:

Not sure whether this patch is based on the assumption that #652122: Acces to dashboard permission required to access Management menu is given. If it is, then it needs a different solution.

I think it's OK since it's specifically checking the page callback, not the URL, so it shouldn't matter what happens there. We are basically just saying that dashboard_is_visible() should return TRUE only when we are visiting whichever menu item is trying to display the dashboard (and if the current user has access to that menu item).

Wow, thanks. #16 is a *really* good explanation!

Excellent, thanks for this to you both :)

I recommend that we include some of the explanation in #16 in the PHPdoc of dashboard_is_visible(). I do think, though, if we want Drupal core to support much more advanced layouts/regions, that this needs to be addressed in a better way. Might be a good topic for the Drupal Developer Summit in SF.

I think it could also be a security concern. Menu routers are applied to paths (i.e. pages or URLs) so one would expect the entire page to be protected. It doesn't make sense to apply it to one region only.

Needs work to add the comments then. Agree that this is a summit-worthy topic :)

Here's a patch with code comments.

Also marking this as critical, since it's a (minor) access bypass vulnerability.

Patch looks fine but this looks like it a test would be easy to write and quite useful.

Subscribing.

I'm writing a test.

Here's the patch with a corresponding dashboard.test

Yay, the first dashboard module test!

But...

1. What happened to all the code comments I was told to add in #22? :)

2. +    $this->assertNoText(t('Access denied'), t('Admin has access to the dashboard.'));
   +    $this->assertRaw($custom_block['title'], t('Admin has access to a dasboard block.'));
   

   For the first one, I'd use assertResponse() - check for 200 vs 403 - rather than looking for text on the page. For the second one, "dashboard" is misspelled. (And the same thing goes for the similar block of code that appears in the patch below this one.)

sub

Reroll #22 and #26 against CVS, with suggestions from #27.

Oops, didn't get my changes regarding #27 part 2 into that patch.

Thanks!

Found a couple minor issues:

 files[] = dashboard.module
 dependencies[] = block
+files[] = dashboard.test

I think this should be moved up one line so that the files[] stuff is all together.

+ * @see dashboard_block_info_alter()

This is a out of date (due to API changes in the interim). It should now refer to dashboard_block_list_alter() instead.

+ * Tests for the dashboard module

Sentence should end in a period.

+class DashboardTestCase extends DrupalWebTestCase {
+  
+  public static function getInfo() {

(very minor) Probably no need for a space in between these.

+      'description' => 'Test access control to dashboard',

This should be a complete sentence - I'd suggest maybe "Test access control for the dashboard."

+  function testDashboardAccess() {
+
+    // Add a new custom block to a dashboard region.

(very minor) Probably no need for a space in between these either.

Suggestions from #33, check application against CVS.

Awesome... dashboard.test! A couple suggestions didn't make it into the patch...

+ * @see dashboard_block_info_alter()

should now refer to dashboard_block_list_alter() instead.

+      'description' => 'Test access control for the dashboard',

should end with a period.

fix those and it's RTBC.

Oops. Missed those.

+++ modules/dashboard/dashboard.test
@@ -0,0 +1,54 @@
+    theme_enable(array('stark'));
+    variable_set('theme_default', 'stark');
+    variable_set('admin_theme', 'stark');
+  }

It is not clear why we fiddle with the theme?

I assume it's because the bug above only happens under those circumstances. And it's certainly the case that the test will (erroneously) pass even without the rest of the patch, if the code were taken out...

I rerolled with some code comments attempting to explain that, and also moved it into the test function itself since it's so intimately tied to that particular test. I also renamed the overall test case to "Dashboard access" since that's specifically what it's testing (and is what the description already said).

and also moved it into the test function itself since it's so intimately tied to that particular test. I also renamed the overall test case to "Dashboard access" since that's specifically what it's testing (and is what the description already said).

Er, scratch that - I only did the second of those two things, not the first :) I was going to do the first, but then realized the second would be better. Given that the test case is now specifically only for dashboard access, it is OK to leave this code in the setUp() method, since its purpose is to be able to correctly do dashboard access tests.

I'm thinking that's a good enough explanation :)

Nice. This looks great, adds tests, has awesome comments, and nails a critical at the same time! What's not to love? :)

Committed to HEAD!

What's not to love?

The fact that the dashboard tests added here were never committed!! :)

These still pass when run locally and were already RTBC above as part of the original patch, so I'm boldly setting this directly to RTBC.

Seems like if all that is left is tests then it's no longer critical, no?

D'oh! My bad. :( Committed to HEAD for real this time. ;) Thanks, David!