Views needs a access check class, which allows you to check access by role. It's certainly not really helpful
for code-only routes, though if you configure for example a View people really often uses role based access, primarily
because you don't have an UI for adding new permissions, just for adding roles.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
7.47 KB

Here is a first patch with a test, though I would love to not use a WebTest for that.

damiankloip’s picture

Status: Needs review » Needs work

Nice work. I know you have a couple of todo things in there, but hey, reviews are always welcome right? :)

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    // Setup one user with the first role, one with the second, one with both

Nice doc, I would just use 'Create' instead of 'Setup'?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    $all_uids[] = $account_1->id();
...
+    $all_uids[] = $account_2->id();
...
+    $all_uids[] = $account_12->id();
...
+    $all_uids[] = $account_none->id();

I get this is used below. I think it might be better to just do $all_uids = array_keys($accounts); after you've created the accounts?

+++ b/core/modules/system/lib/Drupal/system/Tests/Routing/RouterRoleTest.phpundefined
@@ -0,0 +1,102 @@
+    $account_12 = $this->drupalCreateUser();

I get this is account 1 and 2 kind of thing, but would just account 3 be better?

+++ b/core/modules/system/tests/modules/router_test/lib/Drupal/router_test/TestControllers.phpundefined
@@ -46,4 +46,8 @@ public function test8() {
+  public function testRole() {

Needs a docblock.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+   * Implements AccessCheckInterface::applies().

We are still using the full namespaces?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+    $roles = array_keys($account->roles);

Not sure if we are bothering, but should we use $account->get('roles')?

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.87 KB

Wow this was a long route ;)

Additional this is so full of hacks, so I'm wondering whether it's worth to do now, but with #1620010: Move LANGUAGE constants to the Language class this hacks in bootstrap.inc aren't required anymore.

Tried to actually get a working kernel + access manager etc, but then I realized all what's needed is to use the RoleAccessCheck() class directly.

dawehner’s picture

Status: Needs review » Needs work

I didn't checked the review in #2, sorry

dawehner’s picture

I'm waiting for WSCCI to tell whether the hack in bootstrap.inc is okay.

Crell’s picture

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,151 @@
+  /**
+   * Tests role requirements on routes.
+   */
+  public function testRoleAccess() {

PHPUnit has annotations that can let you inject all of this setup code, including getTestRouteCollection(), from other methods. That would keep the test cleaner, IMO.

dawehner’s picture

We could also wait until #1825332: Introduce an AccountInterface to represent the current user is in, all this removes the requirement for creating User Entities.

msonnabaum’s picture

FileSize
7.99 KB

Here's a version that uses @dataProvider like Crell suggested. It's not a *huge* win here because the setup is still a bit complex, but it does make the assertion method much simpler and we could probably use more examples of this in core.

I also removed the bootstrap.inc include. That's verboten in UnitTestCase. Instead, I just defined the one language constant that User needs.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.08 KB
2.26 KB

Oh right this works also. Then please let's also remove this bootstrap.inc hack.

Status: Needs review » Needs work

The last submitted patch, drupal-1956896-9.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.2 KB
7.18 KB

This exceptions happen if you execute these tests via simpletest.

Crell’s picture

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,157 @@
+    // Setup expected values, so which path can be access by which user.

Wouldn't this be "say which path"? In which case it should be preceeded by a semi-colon, not comma.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,157 @@
+  public function accountsToDeny($grant_users) {

Needs docblock.

Other than those nits, this is looking good.

dawehner’s picture

FileSize
1.3 KB
7.48 KB

Fixed the documentation.

Crell’s picture

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -106,7 +106,7 @@ public function testRoleAccessProvider() {
+    // Setup expected values; so which path can be access by which user.

This still doesn't make sense grammatically. "Setup expected values; specify which paths can be accessed by which user." would be what you're looking for, I think.

dawehner’s picture

FileSize
733 bytes
7.49 KB

Respect, this has been exactly 80 chars :)

damiankloip’s picture

Looking pretty good.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,47 @@
+    $roles = array_keys($account->roles);

Looks like we don't really need to use a $role variable, can just be in the array_diff?

dawehner’s picture

FileSize
740 bytes
7.47 KB

Yeah we could do it. No idea which of these two alternatives are easier to read.

dawehner’s picture

Issue tags: +WSCCI

Adding the tag.

tim.plunkett’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,46 @@
+   * Implements AccessCheckInterface::applies().
...
+   * Implements AccessCheckInterface::access().

Either use the new {@inheritdoc} or the full \Drupal\... part, but not this :)

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.phpundefined
@@ -0,0 +1,46 @@
+    $diff = array_diff(array_filter($rids), array_keys($account->roles));
+    if (empty($diff)) {
+      return TRUE;
+    }
+    else {
+      return NULL;

If a user doesn't have a role, shouldn't they be denied access? NULL means "no opinion", that doesn't seem to be correct here. So replace with return empty($diff);?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_1',
+      )
+    ));
+    $route_collection->add('role_test_2', new Route('/role_test_2',
+      array(
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_2',
+      )
+    ));
+    $route_collection->add('role_test_3', new Route('/role_test_3',
+      array(
+        '_controller' => '\Drupal\router_test\TestControllers::test1'
+      ),
+      array(
+        '_role' => 'role_test_1, role_test_2',
+      )
+    ));

Missing some trailing commas here.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+  public function accountsToDeny($grant_users) {

Is making this public a PHPUnit thing, or can it be protected?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+//    return array_diff_assoc($this->accounts, $grant_users);

Delete this?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.phpundefined
@@ -0,0 +1,167 @@
+   * @dataProvider testRoleAccessProvider

I don't know what this is. Missing param.

dawehner’s picture

Status: Needs work » Needs review
FileSize
3.09 KB
7.61 KB

Thanks for your review!

If a user doesn't have a role, shouldn't they be denied access? NULL means "no opinion", that doesn't seem to be correct here. So replace with return empty($diff);?

Yeah I totally wasn't sure about that, so I went with the way PermissionAccessCheck works. http://api.drupal.org/api/drupal/core!modules!user!lib!Drupal!user!Acces...

I don't know what this is. Missing param.

That's pretty cool stuff: http://www.phpunit.de/manual/3.6/en/appendixes.annotations.html#appendix...

Crell’s picture

I disagree. Why should we force-deny if the role doesn't match? Most of the access checkers are TRUE/NULL, and don't return FALSE. Remember, if a checker returns false then NOTHING can grant access after that. FALSE is terminal.

tim.plunkett’s picture

Well, this comes back to the fact that access checkers aren't ANDed together. So if you want to say "This route must meet both of these access checkers", there is no way to do that. (that I can see).

Crell’s picture

True, but that's by design. Individual checkers shouldn't declare themselves the Final Authority(tm) without very good reason. I don't see a reason for generic role checks to be a special snowflake.

dawehner’s picture

I agree with crell. If we return FALSE, you can't connect multiple one. I guess in the custom world there seems to be a lot of usecases for that.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+/**
+ * Determines access to routes based on roles defined via hook_permission().

wat? hook_permission() does not define roles?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+    else {
+      return NULL;

That could use a comment why we return NULL instead of false. Yes, it is already documented elsewhere what NULL means, but I think we need to repeat it more often so that people get it.

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+
+// Needed because the Entity class calls it.
+if (!defined('LANGUAGE_NOT_SPECIFIED')) {
+  define('LANGUAGE_NOT_SPECIFIED', 'und');
+}

What does the entity class call? A constant cannot be called? In what case is this constant not available?

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+      'description' => 'Function Tests for the routing role system.',

Should be "Test for the role based access checker in the routing system."

+++ b/core/tests/Drupal/Tests/Core/Route/RouterRoleTest.php
@@ -0,0 +1,173 @@
+  /**
+   * Provides data for the role access test.
+   */
+  public function testRoleAccessProvider() {

So this is not a test method. Then it should at least have a reference or @see to the actual test method that uses it. Is there no PHPUnit doc standard for that?

olli’s picture

Dataprovider could have a prefix other than "test".

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.87 KB
2.62 KB

Thanks for the reviews!

What does the entity class call? A constant cannot be called? In what case is this constant not available?

... Everything needs to be autoloadable or not usuable at all. Added a todo and improved the comment.

Adapted all the other comments as well.

olli’s picture

Is there an issue on that exception Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest}..?

tim.plunkett’s picture

Title: Add role based access checker. » Add role based access checker

Sorry.

dawehner’s picture

@Olli
Not that I know of.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks RTBC to me now!

olli’s picture

Status: Reviewed & tested by the community » Needs review
tim.plunkett’s picture

Issue tags: -WSCCI, -VDC

#27: drupal-1956896-27.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +WSCCI, +VDC

The last submitted patch, drupal-1956896-27.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.66 KB

No autocompletion in the bundle anymore :(

Crell’s picture

From the testbot log:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1366 Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest} (test_id, test_class, status, message, message_group, function, line, file) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1, :db_insert_placeholder_2, :db_insert_placeholder_3, :db_insert_placeholder_4, :db_insert_placeholder_5, :db_insert_placeholder_6, :db_insert_placeholder_7), (:db_insert_placeholder_8, :db_insert_placeholder_9, :db_insert_placeholder_10, :db_insert_placeholder_11, :db_insert_placeholder_12, :db_insert_placeholder_13, :db_insert_placeholder_14, :db_insert_placeholder_15), (:db_insert_placeholder_16, :db_insert_placeholder_17, :db_insert_placeholder_18, :db_insert_placeholder_19, :db_insert_placeholder_20, :db_insert_placeholder_21, :db_insert_placeholder_22, :db_insert_placeholder_23); Array
(
    [:db_insert_placeholder_0] => 825
    [:db_insert_placeholder_1] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_2] => pass
    [:db_insert_placeholder_3] =>
    [:db_insert_placeholder_4] => Other
    [:db_insert_placeholder_5] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testCRUD()
    [:db_insert_placeholder_6] => 42
    [:db_insert_placeholder_7] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_8] => 825
    [:db_insert_placeholder_9] => Drupal\Tests\Component\PhpStorage\FileStorageTest
    [:db_insert_placeholder_10] => pass
    [:db_insert_placeholder_11] =>
    [:db_insert_placeholder_12] => Other
    [:db_insert_placeholder_13] => Drupal\Tests\Component\PhpStorage\FileStorageTest->testReadOnly()
    [:db_insert_placeholder_14] => 51
    [:db_insert_placeholder_15] => /var/lib/drupaltestbot/sites/default/files/checkout/core/tests/Drupal/Tests/Component/PhpStorage/FileStorageTest.php
    [:db_insert_placeholder_16] => 825
    [:db_insert_placeholder_17] =>
    [:db_insert_placeholder_18] => pass
    [:db_insert_placeholder_19] =>
    [:db_insert_placeholder_20] => Other
    [:db_insert_placeholder_21] => ->Drupal\Tests\Core\Route\RouterRoleTest::testRoleAccess()
    [:db_insert_placeholder_22] =>
    [:db_insert_placeholder_23] =>
)
 in Drupal\Core\Database\Connection->query() (line 554 of /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php).].

I have no idea what it's complaining about...

klausi’s picture

That sounds like those special PHP exceptions/warnings that don't have a line number, for example http://stackoverflow.com/questions/9076899/notice-unknown-on-line-0-how-...

klausi’s picture

Making simpletest more robust to see the actual error.

dawehner’s picture

moshe weitzman’s picture

Issue tags: -WSCCI, -VDC

#38: views-role-access-1956896-38.patch queued for re-testing.

tim.plunkett’s picture

Issue tags: +WSCCI, +VDC

#38: views-role-access-1956896-38.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I'm not sure if that first hunk really belongs, but this whole thing looks pretty great!

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
22.8 KB

Uploading a recent version + a screenshot from the simpletest page.

dawehner’s picture

FileSize
7.66 KB

urgs.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Is the first hook related to the broken phpunit reporting that got fixed in #1969406: PHPUnit Test asserting logging broken?

dawehner’s picture

@Berdir
Yes, the patch from klausi has been just for debugging.

dawehner’s picture

#44: drupal-1956896-41.patch queued for re-testing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +LONDON_2013_APRIL
FileSize
6.93 KB
2.99 KB

I don't think data providers should be providing data to the test through a class property... looks weird to me... about this?

alexpott’s picture

FileSize
7.03 KB
665 bytes

Adding missing @param :(

alexpott’s picture

FileSize
7.03 KB
395 bytes

@dawehner noticed a spurious blank line

dawehner’s picture

I personally think this patch is now much easier to read with the changes of alex.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Nice changes! Still RTBC.

andypost’s picture

Nice example but without usage. Maybe better introduce it's usage within patch?

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -0,0 +1,47 @@
+    // If there is no allowed role, return NULL to give other checks a chance.
+    else {
+      return NULL;
+    }

this could be removed, 'else' not needed

dawehner’s picture

Thanks for your review.

Nice example but without usage. Maybe better introduce it's usage within patch?

The problem is that I doubt that there is any usage in core beside views and maybe SCOTCH. You know, as a module developer I think it's bad practice to rely on roles, but it's a useful feature elsewhere, so I can't find an actual usecase.

Regarding the else bit, I think it's helpful to explain why NULL is returned. This also matches the documentation of PermissionAccessCheck.

dawehner’s picture

FileSize
550 bytes
7.01 KB

Removed the else

andypost’s picture

Thanx a lot , RTBC as it was

Crell’s picture

Can we document on the checker class that it is an "all" check, not an "any" check (vis, you can specify multiple roles, but you then need all of them)? I had to read the code 4 times to determine that it was an "all" check... and even then I'm not 100% certain of that. :-)

Probably just something in the class's docblock is sufficient.

dawehner’s picture

It's indeed a good question whether we want to have both behavior? all and any. Does someone has an oppinion about that?

alexpott’s picture

Well doesn't the answer to #58 depend on what @andypost has been asking for... use cases :)

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
7.17 KB
11.89 KB

I renamed the test file to match it with the classname. I think that's an improvement.

Additional I implemented some logic, how to connect with OR and AND.

dawehner’s picture

FileSize
1.99 KB
12.68 KB

Let's extend the test coverage to test for non-trimmed values.

dawehner’s picture

Alex asked why #60 changed phpunit_error.xml.

When #1963022: Problem with PHPUnit test results when using @dataProvider was written I used this issue to generate a proper phpunit_error.xml. On that time I had named the PHPUnit Test "RouteRoleTest",
so that also landed in the phpunit_error.xml file. Note: the RouteRoleTest did not get added by that patch, just the example phpunit_error.xml.

On #60 of this issue I realized: Let's just make the test class named as parallel as possible to the actual access checker,
so there is now RoleAccessCheck and RoleAccessCheckTest. Just for sanity, I also changed phpunit_error.xml to point to the actual existing file.
Nothing fancy at the end.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

In that case, this looks great! (That was the only part I wasn't sure of).

@dawehner++

webchick’s picture

Title: Add role based access checker » Change notice: Add role based access checker
Category: feature » task
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record, +Needs followup

So according to #1800998-75: Use route system instead of hook_menu() in Views, this is a blocker for REST stuff, which I think makes this more of a task than a feature.

Seems sensible though, and something other modules outside of views could make good use of. I'm a little concerned about adding more $GLOBALS['user'] to our code, but I guess if that could be refactored out here, it would. Is this blocked on #1825332: Introduce an AccountInterface to represent the current user or..?

When I reviewed earlier with Alex, Jess, and Moshe, Jess had a concern about whether or not "," and "+" were clear enough for OR or AND. Let's spin off a follow-up to discuss whether we can keep that as-is or need to change it. Personally, I like it because it's what taxonomy does/used to do when combining multiple terms into a single URL.

Committed and pushed to 8.x. Thanks!

This will need a change notice to inform people of the new check.

dawehner’s picture

Status: Active » Needs review

Thanks for finally getting this in!

I added a small bit to the more or less central route access change notice: http://drupal.org/node/1851490/revisions/view/2461630/2675054

Seems sensible though, and something other modules outside of views could make good use of. I'm a little concerned about adding more $GLOBALS['user'] to our code, but I guess if that could be refactored out here, it would. Is this blocked on #1825332: Introduce an AccountInterface / UserInterface interface for use in \Drupal\Core namespaces or..?

I don't 100% follow the plans to get rid of global user, but I guess the plan is to get the AccountInterface and place it on the request object at some point.
From there the access checkers would have access to it.

When I reviewed earlier with Alex, Jess, and Moshe, Jess had a concern about whether or not "," and "+" were clear enough for OR or AND. Let's spin off a follow-up to discuss whether we can keep that as-is or need to change it. Personally, I like it because it's what taxonomy does/used to do when combining multiple terms into a single URL.

Yeah indeed, this let's me always think a bit which one is which.

I opened an issue for that: #1986640: Support AND/OR conjunctions for permission checks

aspilicious’s picture

I would add a simple example. :)

dawehner’s picture

Good idea.

Berdir’s picture

Title: Change notice: Add role based access checker » Add role based access checker
Status: Needs review » Fixed

Looks good.

The change notice is getting long and somewhat hard to scan, we might want to shorten it down and instead have a documentation page and just link to that to describe the different default access checkers. There' also the entity access one that has a separate change notice. But putting it in there means it is written down and someone who is better a structuring/writing documentation than us developers can do that :) That's what those checkboxes at the bottom are for...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Priority: Normal » Major
Status: Closed (fixed) » Active

Whatever change notice was updated needs a reference to this issue.

Also please untag when the change notice is complete.

dawehner’s picture

Status: Active » Fixed

Added

xjm’s picture

Issue tags: -Needs change record

Thanks! and removing the tag... :)

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