Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
8.98 KB

Started with some basic unit testing, though it still fails.

Status: Needs review » Needs work

The last submitted patch, drupal-1984528-1.patch, failed testing.

Crell’s picture

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -99,21 +99,33 @@ public function check(Route $route, Request $request) {
+    $conjunction = $route->getRequirement('_conjunction') ?: 'OR';

If we're going to do this, should we go ahead and default AND? Since that seems to be the more common case in practice?

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -99,21 +99,33 @@ public function check(Route $route, Request $request) {
+
       if ($service_access === FALSE) {
         // A check has denied access, no need to continue checking.
-        $access = FALSE;
-        break;
+        if ($conjunction == 'AND') {
+          $access = FALSE;
+          break;
+        }
       }
       elseif ($service_access === TRUE) {
         // A check has explicitly granted access, so we need to remember that.
-        $access = TRUE;
+        if ($conjunction == 'OR') {
+          $access = TRUE;
+          break;
+        }
+        else {
+          $access = TRUE;
+        }

I feel like the code would read cleaner if we moved the if statement outside the loop entirely. The nested conditionals here are just really hard to make sense of.

I'm a little uneasy using another requirement property here. Shouldn't it be in options, as, say, _access_conjunction? It's not a requirement per se, and we're already abusing requirements badly enough... :-)

dawehner’s picture

Status: Needs work » Needs review
FileSize
9.22 KB

Let's go on here and fix the issues.

dawehner’s picture

Issue tags: +PHPUnit

.

Status: Needs review » Needs work

The last submitted patch, drupal-1984528-4.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,177 @@
+  protected function setUp() {

i think we always use public for this one

also you miss the getInfo function, UnitTestCase throws a nice exception now

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,177 @@
+    $this->routeCollection->add('test_route_4', new Route('/test-route-4', array(), array(
+      '_access' => 'TRUE',
+      '_test_access' => 'TRUE',
+    )));
...
+    $this->routeCollection->add('test_route_5', new Route('/test-route-5', array(), array(
...
+    $this->routeCollection->add('test_route_6', new Route('/test-route-6', array(), array(
...
+    $this->routeCollection->add('test_route_7', new Route('/test-route-7', array(), array(
...
+    $this->routeCollection->add('test_route_8', new Route('/test-route-8', array(), array(
...
+    $expected = array();
+    $expected['test_route_4'] = TRUE;
+    $expected['test_route_5'] = TRUE;
+    $expected['test_route_6'] = TRUE;
+    $expected['test_route_7'] = TRUE;
+    $expected['test_route_8'] = FALSE;
...
+    foreach ($expected as $route_name => $expected_access) {

you could use dataproviders for this:) would be much readable i think.

Crell’s picture

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -96,24 +96,39 @@ protected function applies(Route $route) {
 
+    $conjunction = $route->getRequirement('_access_conjunction') ?: 'AND';
+    $access = $conjunction == 'AND' ? TRUE : FALSE;

This can be collapsed. It's a tautology. :-)

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -96,24 +96,39 @@ protected function applies(Route $route) {
+    else {
+      foreach ($checks as $service_id) {
+        if (empty($this->checks[$service_id])) {
+          $this->loadCheck($service_id);
+        }
+
+        $service_access = $this->checks[$service_id]->access($route, $request);
+        if ($service_access === TRUE) {
+          return TRUE;

I think this logic is wrong. A checker can still return FALSE, which even in OR should result in access-deny.

And now that I'm reading it, we should probably split the AND and OR routines off to separate methods on this object to keep everything as simple and contained as possible. (Sorry, I didn't think of that before.)

you could use dataproviders for this:) would be much readable i think.

That's only a PHPUnit feature. Which is a perfectly good reason these tests should be done in PHPUnit. :-)

dawehner’s picture

FileSize
4.52 KB
10.61 KB

i think we always use public for this one

This is different on PHPUnit

you could use dataproviders for this:) would be much readable i think.

I tried hard but as setUp() is not called before dataproviders you can't setup objects before and yeah then there are all kind of problems.

This can be collapsed. It's a tautology. :-)

He!

Status: Needs review » Needs work

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

dawehner’s picture

Status: Needs work » Needs review
FileSize
5.5 KB
13.41 KB

Ensured that NULL does not block access.

damiankloip’s picture

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -89,17 +89,46 @@ protected function applies(Route $route) {
+      $access = $this->checkAnd($checks, $route, $request);
...
+      $access = $this->checkOr($checks, $route, $request);

Do you think we should just return these directly instead, and not bother assigning the variable.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -89,17 +89,46 @@ protected function applies(Route $route) {
+   * Checks access for and conjunction.

'AND'

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -107,12 +136,9 @@ public function check(Route $route, Request $request) {
         $access = TRUE;

We should not use the $access variable, at the moment there is a mixture of variable assignment and jsut returning the bool.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -121,6 +147,40 @@ public function check(Route $route, Request $request) {
+   * Checks access for or conjunction.

'OR'

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -121,6 +147,40 @@ public function check(Route $route, Request $request) {
+    $access = FALSE;
...
+    return $access;

This could just return FALSE instead.

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,198 @@
+   * Add a default access check service to the container and the access manager.

Adds.

dawehner’s picture

FileSize
2.51 KB
13.4 KB

Thank you, fixed your points.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -89,17 +89,44 @@ protected function applies(Route $route) {
+    $conjunction = $route->getRequirement('_access_conjunction') ?: 'AND';

I think we need to put this in the options, not in requirements. It's a configuration option of the route behavior, basically.

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -121,6 +146,42 @@ public function check(Route $route, Request $request) {
+      $service_access = $this->checks[$service_id]->access($route, $request);
+      if ($service_access === TRUE) {
+        $access = TRUE;
+        break;
+      }
+      if ($service_access === FALSE) {
+        $access = FALSE;
+        break;
+      }
+    }

I don't think this is quite right. Consider 2 checkers, the first returns false and the second true. That should result in a false result, since there is at least one fail. However, in this case the first one would set $access to false, and the second would set it back to true, causing true to be returned.

Instead, $service_access FALSE should return FALSE immediately.

Also, the breaks aren't necessary. It's the end of the loop anyway.

dawehner’s picture

Status: Needs work » Needs review
FileSize
13.59 KB
3.32 KB

I don't think this is quite right. Consider 2 checkers, the first returns false and the second true. That should result in a false result, since there is at least one fail. However, in this case the first one would set $access to false, and the second would set it back to true, causing true to be returned.

Instead, $service_access FALSE should return FALSE immediately.

So a logical OR with TRUE and FALSE should return FALSE ... I don't agree here :)

Crell’s picture

Title: Add a conjunction setting into the requirements. » Allow for more robust access checking
FileSize
2.16 MB

Daniel, chx, and I had a nice detailed discussion about this at the extended sprints. Final result:

AND and OR are replaced by they keywords ALL and ANY, respectively. I recommend an option key of _access_mode: ANY.

Instead of TRUE, NULL, FALSE, we use more semantically-correct constants ALLOW, DENY, and STOP, respectively. (Probably those belong on the access checker interface: AccessCheckerInterface::ALLOW, AccessCheckerInterface::DENY, etc.)

The logic is:
- In ANY mode, access is true iff at least one checker returns ALLOW.
- In ALL mode, access is true iff all checkers return ALLOW.
- If any checker returns STOP, access is false regardless of mode.

That is essentially what I was suggesting above, but the better naming makes it much clearer how the logic works so it's not confusing. :-)

I've attached a screen shot of the pseduo-truth-table we had on the whiteboard.

dawehner’s picture

FileSize
4.3 KB
16.04 KB

I wasn't sure about our exact termonoligy, so I went with ACCEPT/DENY/KILL but we can discuss about it.
Isn't it just great that there is no problem with existing access checkers...

Crell’s picture

+++ b/core/lib/Drupal/Core/Access/AccessCheckInterface.php
@@ -16,6 +16,21 @@
   /**
+   * @TODO
+   */
+  const ACCEPT = TRUE;
+
+  /**
+   * @TODO
+   */
+  const KILL = FALSE;
+
+  /**
+   * @TODO
+   */
+  const DENY = NULL;

I thought we were going to call this ALLOW, not ACCEPT?

Suggested text:

Allow: Grant access

A Checker should return this value to indicate that it grants access to a route.

Deny: Deny access

A Checker should return this value to indicate that it does not grant access to a route.

Kill: Block access

A Checker should return this value to indicate that it wants to completely block access to this route, regardless of any other access checkers. Most checkers will not need to use this value.

+++ b/core/lib/Drupal/Core/Access/AccessManager.php
@@ -170,12 +170,11 @@ protected function checkOr(array $checks, $route, $request) {
       $service_access = $this->checks[$service_id]->access($route, $request);
-      if ($service_access === TRUE) {
+      if ($service_access === AccessCheckinterface::ACCEPT) {
         return TRUE;
       }
-      if ($service_access === FALSE) {
-        $access = FALSE;
-        break;
+      if ($service_access === AccessCheckInterface::KILL) {
+        return FALSE;

We need to run through all checkers, even if one returns Allow, in case a later one returns Kill. We can't just return TRUE if we find an Accept response.

+++ b/core/modules/user/lib/Drupal/user/Access/RoleAccessCheck.php
@@ -42,19 +42,19 @@ public function access(Route $route, Request $request) {
+    // If there is no allowed role, return DENY to give other checks a chance.
+    return static::DENY;

Given the rename, this comment is now confusing. I suggest instead "Do not allow access if the user does not have the necessary role."

dawehner’s picture

FileSize
12.66 KB
19.01 KB

Thanks for the review.

I rewrote the tests to just map our table.

One possible follow up would be to replace 'TRUE' with 'ALLOW' etc. on the _access access checker.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-1984528-19.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#19: drupal-1984528-19.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +PHPUnit

The last submitted patch, drupal-1984528-19.patch, failed testing.

Crell’s picture

ne possible follow up would be to replace 'TRUE' with 'ALLOW' etc. on the _access access checker.

Good idea!

+++ b/core/lib/Drupal/Core/Access/DefaultAccessCheck.php
@@ -23,9 +23,18 @@ public function applies(Route $route) {
+      return static::ALLOW;

Side note: I love that the way we're doing this lets the simple static:: syntax work here in all checkers. That will be great for DX.

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.php
@@ -0,0 +1,270 @@
+  /**
+   * Test \Drupal\Core\Access\AccessManager::check() with conjunctions.
+   */
+  public function testCheckConjunctions() {
+    $this->setupAccessChecker();
+    $access_check = new DefinedTestAccessCheck();
+    $this->container->register('test_access_defined', $access_check);
+    $this->accessManager->addCheckService('test_access_defined');
+
+    $request = new Request();
+
+    $access_configurations = array();
+    $access_configurations[] = array(
+      'conjunction' => 'AND',
+      'name' => 'test_route_4',
+      'condition_one' => AccessCheckInterface::ALLOW,
+      'condition_two' => AccessCheckInterface::KILL,
+      'expected' => FALSE,

This looks like a good use case for @dataProvider for PHPUnit.

Looks like testbot is still being janky, so will retry again momentarily.

Crell’s picture

Status: Needs work » Needs review

#19: drupal-1984528-19.patch queued for re-testing.

dawehner’s picture

This looks like a good use case for @dataProvider for PHPUnit.

See https://drupal.org/node/1984528#comment-7437470

damiankloip’s picture

Looking pretty good, and as always, nice test additions!

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -89,32 +91,95 @@ protected function applies(Route $route) {
+   *  Returns TRUE if the user has access to the route, else FALSE.

'FALSE otherwise' sounds better IMO

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -89,32 +91,95 @@ protected function applies(Route $route) {
     $checks = $route->getOption('_access_checks') ?: array();
...
+    $conjunction = $route->getOption('_access_conjunction') ?: 'AND';

This does a getOption() twice, wont make a massive difference but would it be better to do something like $conjuction = $route->getOption('_access_conjuction'); if (empty($conjuction)) {$conjuction = 'AND'} or something? You get what I mean.

+++ b/core/lib/Drupal/Core/Access/AccessManager.phpundefined
@@ -89,32 +91,95 @@ protected function applies(Route $route) {
+      if ($service_access === AccessCheckInterface::KILL) {
...
+      if ($service_access === AccessCheckInterface::DENY) {

Couldn't these two be just be in the same condition?

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,270 @@
+    $access_configurations = array();

Agree with Crell, after this whole new world of data providers (new for me :)), this seems like a perfect use case.

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,270 @@
+   * Little helper function to convert AccessCheckInterface constants to string.

I'm fine with it being called little, but I think this might need to be changed to just 'Converts Blah to a string'

Status: Needs review » Needs work

The last submitted patch, drupal-1984528-19.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
4.74 KB
19.05 KB

This does a getOption() twice, wont make a massive difference but would it be better to do something like $conjuction = $route->getOption('_access_conjuction'); if (empty($conjuction)) {$conjuction = 'AND'} or something? You get what I mean.

I checked that with my debugger and that's actually not TRUE. getOption in total is just called twice in the full function.

Used a dataprovider now.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, drupal-1984528-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#28: drupal-1984528-28.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +PHPUnit

The last submitted patch, drupal-1984528-28.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
663 bytes
19.05 KB

Let's fallback back to OR for now and see whether everything passes with it.

Crell’s picture

Well that's annoying...

Also, I thought we were going to switch to _access_mode: [ANY|ALL], not "conjunction" AND/OR?

dawehner’s picture

Hopefully I have not done total shit at 1am.

tim.plunkett’s picture

Category: feature » task
Priority: Normal » Major
Issue tags: +DrupalWTF, +WSCCI

Currently you are limited to only using ANY (or) for access checks, which means any route that would normally combine them, would need a custom checker.
This restores some sanity.

damiankloip’s picture

+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,277 @@
+   * @see \Drupal\Tests\Core\Access\AccessManagerTest::testCHeckConjunctions()

Sorry..

@dawehner, also, are you going to change conjunction to access_mode?

dawehner’s picture

FileSize
1.79 KB
19.1 KB

Let's do that.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Whew! Thanks, dawehner! I think we finally figured it out. :-)

catch’s picture

Really like this one but it no longer applies.

catch’s picture

Issue tags: -DrupalWTF, -PHPUnit, -WSCCI

#37: drupal-1984528-37.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +DrupalWTF, +PHPUnit, +WSCCI

The last submitted patch, drupal-1984528-37.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.56 KB

Just another rerole.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/tests/Drupal/Tests/Core/Access/AccessManagerTest.phpundefined
@@ -0,0 +1,277 @@
+  public function checkConjunctionsProvider() {

usually we prefix with provider and then use the method name.eg here
providerTestCheckConjuctions

shouldnt hold this patch though

ParisLiakos’s picture

and just for the sake of consistency

Status: Reviewed & tested by the community » Needs work
Issue tags: -DrupalWTF, -PHPUnit, -WSCCI

The last submitted patch, access_manager-1984528-44.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +DrupalWTF, +PHPUnit, +WSCCI

#44: access_manager-1984528-44.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

jibran’s picture

Title: Allow for more robust access checking » Change notice: Allow for more robust access checking
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

I miss @webchick.

catch’s picture

Oops, thanks!

dawehner’s picture

Does someone has an oppinion whether it should be added to https://drupal.org/node/1851490 or https://drupal.org/node/1800686 or there should be even a new one?

Crell’s picture

Update the former I'd say. https://drupal.org/node/1851490 Doesn't need a new one.

Wim Leers’s picture

Title: Change notice: Allow for more robust access checking » Allow for more robust access checking
Status: Active » Needs review
Issue tags: -Needs change record
FileSize
2.54 KB

Change notice was updated: https://drupal.org/node/1851490/revisions/view/2675322/2725683

However… I've just lost half a day over the changes made here. See below.


  1. Why is the default ANY and not ALL? AFAICT nobody intended for this to happen, but the patch got committed *very* fast, without proper review it seems, after there was a reroll to just see if defaulting to ANY would make tests pass. Did we check the consequences of that? I think *everybody* is going to assume these requirements are ANDed by default…
  2. Why is the change notice so utterly wrong? It says _access_checks: 'ANY', not _access_mode: 'ANY'! And that is the default, not 'ALL' (see point 1), so why is it even an example?
  3. Why is there no integration test (WebTestBase) coverage for this at all? That would also provide useful examples. If there's anything worth integration testing, then it's routing system access checks, is it not?
  4. Why did this issue not convert all other access check implementations to use static::(ALLOW|DENY|KILL)?

Attached is a commentless initial patch with basic integration test coverage.

Wim Leers’s picture

And the thing I did not yet even mention: the change from defaulting from ALL to ANY has completely broken existing code. I can't believe it is our goal to have integration tests for *every* route, to ensure their declarative access checks are indeed handled correctly by the routing system. But it seems that's what we have to do.

E.g. this Edit route is affected by the changed default:

edit_field_form:
  pattern: '/edit/form/{entity_type}/{entity}/{field_name}/{langcode}/{view_mode_id}'
  defaults:
    _controller: '\Drupal\edit\EditController::fieldForm'
  requirements:
    _permission: 'access in-place editing'
    _access_edit_entity_field: 'TRUE'

Because the default has changed from ALL to ANY, as of this commit it means that a user without the in-place editing permission can edit any content…!

This is just one example. There likely are more.

dawehner’s picture

Because the default has changed from ALL to ANY, as of this commit it means that a user without the in-place editing permission can edit any content…!

Just to be sure, this was not changed, but rather some of the access checkers now implement the API properly by returning NULL/static::DENY instead of FALSE/static::KILL.

dawehner’s picture

Wim Leers’s picture

#55:

Just to be sure, this was not changed, but rather some of the access checkers now implement the API properly by returning NULL/static::DENY instead of FALSE/static::KILL.

Heh, you're, right … but only in a very subtle way.

As you can see in #2063405-1: Update all access checkers to use static::ALLOW/static::DENY/static::KILL, just about everything in Drupal core was returning TRUE or FALSE. Even the most typical example of all, EntityAccessCheck returned a boolean and never NULL, yet only now it is being changed (fixed) to return ALLOW (TRUE) or DENY (FALSE):

-        return $entity->access($operation);
+        return $entity->access($operation) ? static::ALLOW : static::DENY;

So… yes, you're right in theory, but in practice just about everything was using FALSE, not NULL, which is now being fixed in #2063405: Update all access checkers to use static::ALLOW/static::DENY/static::KILL. So, it's kind of harsh to say that everybody was just using it wrong, when most of core was wrong, and everybody else was looking at existing core examples, thus perpetuating the existing flaws. Since everybody was using FALSE, not NULL (i.e. KILL, not DENY), that effectively meant a default of ALL for the majority of use cases.

Conclusion: when introducing a tri state return value, ensure all of core is using it correctly, or otherwise problems and assumptions will permeate, especially if it *looks* like a boolean.

dawehner’s picture

I totally agree with you that it was problematic that all this access checkers went in without proper review,
as from the first days of the access manager this tri-logic existed. Let's concentrate now on fixing all the places.

Wim Leers’s picture

catch’s picture

Title: Allow for more robust access checking » Follow-up: Allow for more robust access checking
Category: task » bug
Priority: Critical » Major

Hmm this was RTBC for four days before commit excluding re-rolls, it wasn't that fast :(

What needs to happen in this issue? Looks it's a bug rather than task now - would've been better to open new issues.

ParisLiakos’s picture

Status: Needs review » Needs work

so, should this issue be closed? i agree new issues should be open.
anyway setting to needs work, since the patch needs at least a reroll

jibran’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 53: access_mode_all_tests-1984528-53.patch, failed testing.

The last submitted patch, 53: access_mode_all_tests-1984528-53.patch, failed testing.

dawehner’s picture

I don't really think that we should do anything here.

dawehner’s picture

Issue summary: View changes
Status: Needs work » Fixed

I am pretty sure we can close this issue now.

Wim Leers’s picture

Yay :)

Status: Fixed » Closed (fixed)

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