Support AND/OR conjunctions for permission checks

As long as we document it and are consistent, I have no objection to + and , used in that way.

Now that the role checker is using it, it may make sense to do the same to the permission checker.

I agree with everything Crell says :)

Well, it's a Drupalism, and aren't we trying to get rid of those? Wouldn't | and & or || and && be a bit more self documenting?

Note that for most route requirements (including _format and _method), Symfony evaluates the requirement as a regular expression, so you could see YAML like this:

requirements:
  _format:  html|rss

Where the "|" in a regular expression functions similarly to a logical OR.

However, this issue is related to a route requirement applied to an array ($account->roles) rather than a string, so I don't think regexes are a good fit for expressing that.

To avoid confusion with regex syntax, this patch implements the || and && approach, with single space required on each side.

Ah, so we still need a string syntax that's URL friendly for contextual filters. I thought this issue was just about a syntax for YAML files. Hm, need to think about it some more then.

My initial thought on removing trimming is that Symfony doesn't do auto-trimming of other evaluations in _requirements, and I wanted to be consistent with that. No strong preference though.

I'd prefer to keep the trimming for the improved DX.

The reason for using + and , is exactly as dawehner said, that's what Views uses, and what Taxonomy has always used. Switching to &&/|| in the YAML file would be more sensible in the YAML, but inconsistent elsewhere. URLs have to stick with + and ,.

We either have a consistent Drupalism or a Drupalism in some places but not others. I don't have a strong opinion either way.

This code doesn't support mixing and and or operations. Are we OK with that? (I am, because it's a crazy edge case to mix both here, but that should be a conscious decision.)

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
@@ -28,9 +33,22 @@ public function applies(Route $route) {
-    // @todo Replace user_access() with a correctly injected and session-using
-    //   alternative.

Let's keep this @todo.

If you need such an edge case you ca write your own system and plug it in :)

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
@@ -20,7 +20,12 @@ class PermissionAccessCheck implements AccessCheckInterface {
-    return array_key_exists('_permission', $route->getRequirements());
+    foreach (array_keys($route->getRequirements()) as $key) {
+      if (strpos($key, '_permission') === 0) {
+        return TRUE;
+      }
+    }
+    return FALSE;
   }

I'm not sure I see the purpose for this change.

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
@@ -28,9 +33,24 @@ public function applies(Route $route) {
+    $split = explode(',', $permission);

We need a comment or docblock explaining that ,-separated means "any of" and +-separated means "all of". In the docblock of this method is probably sufficient.

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.phpundefined
@@ -17,20 +17,40 @@
+   * The requirement used as key.

Maybe 'The requirement key to be used' instead?

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.phpundefined
@@ -17,20 +17,40 @@
+    $split = explode(',', $permission);
...
+      $split = explode('+', $permission);

Maybe a couple of one liners around here for OR and AND logic.

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.phpundefined
@@ -17,20 +17,40 @@
+      return $access ? static::ALLOW : static::DENY;

Is it almost worth having a helper for this? not sure.

+++ b/core/modules/user/tests/Drupal/user/Tests/Access/PermissionAccessCheckTest.phpundefined
@@ -0,0 +1,98 @@
+    $request = new Request(array());
...
+    $access_check = new PermissionAccessCheck();

Could we just move these into a setUp() method? or does the use of data providers do a setUp/teadDown between each iteration?

+++ b/core/modules/user/tests/Drupal/user/Tests/Access/PermissionAccessCheckTest.phpundefined
@@ -0,0 +1,98 @@
+  if (!function_exists('user_access')) {

Just out of interest, is the an issue to do something with this? user access is going to, and is already, being used in OO code. Not a part of this issue, just to be clear.

Actually, now that global $user is a UserSession object, and is available via the request, we could probably do this instead:

$request->attributes->get('account')->hasPermission($permission);

... Except, crap, no, because there's only a getRoles() method, not a hasPermission() method. So that has to wait for #1966334: Convert user_access to User::hasPermission(), which adds that method. *sigh*

That's an internal followup, so can happen later. No sense blocking these patches on each other.

So the thing is there is nothing to stop us from declaring permissions like so...

/**
 * Implements hook_permission().
 */
function action_permission() {
  return array(
    'administer actions, events, and parties' => array(
      'title' => t('Administer actions, events and parties'),
    ),
  );
}

We also need to consider the upgrade path here...

A change notice that permission names cannot have those characters and (as suggested in meatspace by xjm) just an exception if you try to declare a permission with non-alphanumeric-plus-space characters should suffice here.

For Alex...

OK, alphanumeric plus period? :-) Is there a reason we can't just blacklist , and + (and maybe others, maybe not) to resolve this issue?

It probably affects roles too at the moment, and we already have conjunction support for those...

For route rebuild, we've taken the approach of "if your route has invalid properties in it, watchdog it and skip that entry".

A similar approach for permission rebuild (which then gets cached anyway) seems reasonable.

Well, I expected that all calls to get permission data went through some common front-end utility that we could modify. It looks like that's not the case, however, as there are module_invoke_all('permission') and $moduleHandler->getImplementations('permission') and $moduleHandler->invokeAll('permission') calls scattered all over the place. :-( That's a separate thing to fix. Drat.

1. +++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
   @@ -17,20 +17,42 @@
   +   * The requirement key to be used.
   

   @var ...

2. +++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
   @@ -17,20 +17,42 @@
   +   */
   +  protected $requirementKey = '_permission';
   ...
   +    return $this->requirementKey;
   

   Also, do we actually need to use a property for this? vs just returning the '_permission' string. I guess currently we kind of mix both ways.

3. +++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
   @@ -17,20 +17,42 @@
   +    $permission = $route->getRequirement($this->requirementKey);
   +    // Separate for matching ANY checks.
   +    $split = explode(',', $permission);
   +    if (count($split) > 1) {
   +      $access = FALSE;
   +      foreach ($split as $permission) {
   +        // @todo Replace user_access() with a correctly injected and
   +        // session-using alternative.
   +        $access |= user_access($permission);
   +      }
   +      return $access ? static::ALLOW : static::DENY;
   +    }
   +    else {
   +      // Separate for matching ALL checks.
   +      $split = explode('+', $permission);
   +      $access = TRUE;
   +      foreach ($split as $permission) {
   +        $access &= user_access($permission);
   +      }
   +      return $access ? static::ALLOW : static::DENY;
   +    }
   

   So, we are using this pattern in a few places now (or atleast will be). I'm thinking it would be cool if we could abstract this out but not sure how possible that is right now as we'd need some nice way to invoke 'things'

4. +++ b/core/modules/user/tests/Drupal/user/Tests/Access/PermissionAccessCheckTest.php
   @@ -0,0 +1,100 @@
   +  public function providerTestApplies() {
   

   Same as other comment above, do we need this much from our appliesTo() method? If we keep this, these methods should be testAppliesTo. actually where is the testApplies() test method?!

+++ b/core/modules/user/lib/Drupal/user/Access/PermissionAccessCheck.php
@@ -17,20 +17,42 @@
+      foreach ($split as $permission) {
+        // @todo Replace user_access() with a correctly injected and
+        // session-using alternative.
+        $access |= user_access($permission);
+      }

The OR condition loop can return as soon as TRUE condition is met. I'll concede it's a micro optimization, but if this is going to get broken out and re-used... efficiency is good.

I'm pretty sure this won't apply anymore, but after a year it would be good to get back to. I still think permissions should parallel roles in their syntax.

+++ b/core/modules/user/user.services.yml
@@ -1,6 +1,7 @@
     class: Drupal\user\Access\PermissionAccessCheck
+    arguments: ['@current_user']
     tags:

This isn't needed, right? Otherwise this looks great.

Assuming bot OKs it.

Note we currently use human names for machine names ie. ->hasPermission('access content') instead of ->hasPermission('access_content') and so it's not entirely impossible to encounter a plus or comma in there; however the chances of there being an actual permission on either side is next to nothing so I believe we can go ahead with this, but we should put such a rename on the radar and discuss whether it's doable in the 8.x at all. Otherwise I would very strongly suggest showing a warning somewhere when working with permissions if there are permissions containing such.

What has happened here? I am not supposed to be able to delete files made by others.

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -159,6 +159,23 @@ public static function allowedIfHasPermission(AccountInterface $account, $permis
   +    return static::create()->allowIfHasPermissions($account, $permissions, $conjunction);
   
   @@ -407,6 +424,46 @@ public function allowIfHasPermission(AccountInterface $account, $permission) {
   +  public function allowIfHasPermissions(AccountInterface $account, $permissions, $conjunction = 'AND') {
   

   Can typehint this with array.

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -407,6 +424,46 @@ public function allowIfHasPermission(AccountInterface $account, $permission) {
   +    if ($conjunction == 'AND' && $permissions) {
   

   lets !empty($permissions)

Glad to see this issue was made simpler by #2287071: Add cacheability metadata to access checks :)

1. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -159,6 +159,23 @@ public static function allowedIfHasPermission(AccountInterface $account, $permis
   +   *   The account for which to check a permission.
   
   @@ -407,6 +424,46 @@ public function allowIfHasPermission(AccountInterface $account, $permission) {
   +   *   The account for which to check a permission.
   

   s/a permission/permissions/

2. +++ b/core/lib/Drupal/Core/Access/AccessResult.php
   @@ -159,6 +159,23 @@ public static function allowedIfHasPermission(AccountInterface $account, $permis
   +   *   The permissions to check for.
   
   @@ -407,6 +424,46 @@ public function allowIfHasPermission(AccountInterface $account, $permission) {
   +   *   The permissions to check for.
   

   s/for//?

3. +++ b/core/modules/shortcut/src/ShortcutSetAccessControlHandler.php
   @@ -11,6 +11,7 @@
   +use Drupal\node\Plugin\views\filter\Access;
   

   This is accidental?

4. +++ b/core/modules/user/src/Access/PermissionAccessCheck.php
   @@ -30,6 +30,15 @@ class PermissionAccessCheck implements AccessInterface {
   +    // Separate for matching ANY checks.
   +    $split = explode(',', $permission);
   +    if (count($split) > 1) {
   +      return AccessResult::allowedIfHasPermissions($account, $split, 'OR');
   +    }
   +    else {
   +      $split = explode('+', $permission);
   +      return AccessResult::allowedIfHasPermissions($account, $split, 'AND');
   +    }
   

   A bit more documentation would be welcome here I think?

   This also needs a change record.

   Also, s/$split/$permissions/?

5. +++ b/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php
   @@ -0,0 +1,75 @@
   + * @group Routing
   

   Please also add @group Access.

6. +++ b/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php
   @@ -0,0 +1,75 @@
   +      [[], AccessResult::allowedIf(FALSE)->cachePerRole()],
   +      [['_permission' => 'allowed'], AccessResult::allowedIf(TRUE)->cachePerRole()],
   +      [['_permission' => 'denied'], AccessResult::allowedIf(FALSE)->cachePerRole()],
   +      [['_permission' => 'allowed,denied'], AccessResult::allowedIf(TRUE)->cachePerRole()],
   +      [['_permission' => 'allowed,denied,other'], AccessResult::allowedIf(TRUE)->cachePerRole()],
   +      [['_permission' => 'allowed+denied'], AccessResult::allowedIf(FALSE)->cachePerRole()],
   

   Each case is repeated 3 times. Put it in variables, reuse those? Much more legible.

I've corrected the change record (it said _permissions rather than _permission).

Fixed two nitpicks.

I'd love to see at least one *.routing.yml file in Drupal core use this. Don't we have a single use case for this? I think we might've solved it using an access check, which makes it a bit difficult to find?

Drupal\views\ViewsAccessCheck maybe?

SwitchShortcutSet has a custom access callback that just calls out to a function. We can probably refactor that to eliminate the function anyway. The function has several permission checks but may be more complex than this can handle. Worth checking.

Those are the only promising examples I could find off hand.

I don't think neither is viable. I also searched and couldn't find any. I guess this will have to do then.

Back to RTBC.

Needs a reroll for the changes in AccessResult due to #2340507: Make the new AccessResult API and implementation even better

Hmmm wrong status :)

Heh :)

Rerolled!

#54 lost the new unit test hunk. #54 interdiff applies to this patch.

Back to RTBC per #50 — this is only a reroll to chase HEAD.

Per @dawehener, this issue blocks a proper solution in #2157541: Views sets access to ANY on routes - could result in information disclosure, so bumping to critical.

1. +++ b/core/modules/user/src/Access/PermissionAccessCheck.php
   @@ -31,6 +31,15 @@ class PermissionAccessCheck implements AccessInterface {
   +    // Allow to conjunct the permissions with OR (',') or AND ('+').
   

   Shouldn't this be + for OR and , for AND? Filed #2346105: Use 1+2+3 for OR, 1,2,3 for AND.

2. +++ b/core/modules/user/tests/src/Unit/PermissionAccessCheckTest.php
   @@ -0,0 +1,78 @@
   + * @group AccessF
   

   AccessF..?

Needs work for #58 - I've committed #2346105: Use 1+2+3 for OR, 1,2,3 for AND
So

+    // Allow to conjunct the permissions with OR (',') or AND ('+').
+    $split = explode(',', $permission);
+    if (count($split) > 1) {
+      return AccessResult::allowedIfHasPermissions($account, $split, 'OR');
+    }
+    else {
+      $split = explode('+', $permission);
+      return AccessResult::allowedIfHasPermissions($account, $split, 'AND');
+    }

Needs fixing.

Unless bot objects.

Brought issue summary inline with #2346105: Use 1+2+3 for OR, 1,2,3 for AND

Committed and pushed to 8.x. Thanks!

Published the CR