Roles with "Update any submissions" access in a webform gives access to webform configuration via direct link

Sounds like a bug and it is probably not specific to Drupal 8.6.x.

I can't replicate this issue.

Can you please export the role and cut-n-paste it here?

• Go to /admin/config/development/configuration/single/export
• Select Config Type => Role
• Select the role
• Cut-n-past the config here

@see https://www.drupal.org/docs/8/configuration-management/managing-your-sit...

What is the path to the confirmation page?

Can you please include a screenshot showing where you are checking "Update any submissions"?

Keep in mind that anonymous and authenticated users can access a webform's confirmation page by default.

I can replicate this issue and it is a regression in rc22. I am pretty sure it is related to changes to the access rules.

@see https://www.drupal.org/node/3000979

The attached patches pinpoints and fixes the issue. It might also break some tests.

Attached patch fixes the broken test but I think we need to create a [meta] ticket to clean up the Webform module's access controls to prevent regressions like this from occurring.

Yep, I confirm it's a residual effect from #2995587: [meta] Allow to extend access rules from other modules.

Let me share my understanding with the problem. I hope I will be able to convey my point of view.

We have a list of known access rules, basically have a look at webform_webform_access_rules(). Some of them apply to webform, such as "administer" and "test". Whereas others apply to webform submissions: "create", "delete_any", "delete_own", etc.

Before #2995587: [meta] Allow to extend access rules from other modules it was more or less hard wired the association between which access rule applies to which entity type. After that ticket, since we had to relax things and make it more flexible,

The "settings" tab requires "update" access on webform entity. The WebformEntityAccessControlHandler, among other things, also checks granted access rules to the current user. It probes $operation, $operation . "_any", and $operation . "_own" rules. There is actually a rule "update_any"... though we intend to use it for webform submission entity type, it also gets checked for webform entity type and if it is granted, we unintentionally grant "update" access to the webform.

Sort of, it is a name collision because we keep webform & webform submission access rules within one flat list without any distinction.

I believe if we introduce such distinction the issue in particular will be solved and we will invest a reasonable effort into making sure such things do not happen again.

I'd introduce one more property into the hook: 'entity type' => array() so the access rule can tell to the webform access system whether it wants to apply to only webform, only webform submission, or both. Then, correspondingly updating the access handlers we can enforce to check only those access rules which support the entity type in question.

---------------------

Not sure if it makes sense to anybody else (hehe, it does to me :) ). But I often find it easier to communicate in PHP language than in English (though both are not my native ones :D ). I am attaching a patch that roughly implements my suggestion. I am sure it is going to generate some number of test failures, but I wanted to quickly sketch it so I can show you my idea in code.

I genuinely believe if we separate access rules that govern webfrom from those that govern submissions, we should nail this ticket's problem on 1 level further of abstraction.

Jacob, what is your opinion?

I think the approach from #18 is going to make the APIs too confusing.

As stated in #16, we need to fix the regression which includes temporarily removing support for the magic permissions *_own and *_any.

From there we can work to improve and fix all access controls and rules.

The think long-term solution might be to make access rules permission name more specific.

• create => submission_create
• view_any => submission_view_any
• update_any => submission_update_any
• delete_any => submission_delete_any
• purge_any => submission_purge_any
• view_own => submission_view_own
• update_own => submission_update_own
• delete_own => submission_delete_own
• administer => administer
• test => test

I see your point.

Ahm... but if we chop off the magical suffixes of _own and _any, then we will pretty much kill the whole functionality of "Access" tab because most of the existing access rules make use of the suffixes, for e.g. we have update_any & update_own access rules but the actual operation they apply to is update, i.e. you run $webform_submission->access('update') and not $webform_submission->access('update_any'). If we disable the magical suffixes then the update access through access rules will seize to function.

Maybe the least intrusive and backwards compatible solution is to introduce a black-list of access rules that are not allowed in the context of a webform but are OK in the context of webform submission? Have a look at the patch I attach in this comment.

I think supporting *_any and *_own should be implemented by each module. It is just more secure. Maybe during the access controls clean up we can use hook_webform_submission_access() for the webform submission *_own and *_any access rules.

If you look at core's node.module they are using hook_node_access() to add node.module only support for these magic permissions.

/**
 * Implements hook_node_access().
 */
function node_node_access(NodeInterface $node, $op, $account) {
  $type = $node->bundle();

  switch ($op) {
    case 'create':
      return AccessResult::allowedIfHasPermission($account, 'create ' . $type . ' content');

    case 'update':
      if ($account->hasPermission('edit any ' . $type . ' content', $account)) {
        return AccessResult::allowed()->cachePerPermissions();
      }
      else {
        return AccessResult::allowedIf($account->hasPermission('edit own ' . $type . ' content', $account) && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
      }

    case 'delete':
      if ($account->hasPermission('delete any ' . $type . ' content', $account)) {
        return AccessResult::allowed()->cachePerPermissions();
      }
      else {
        return AccessResult::allowedIf($account->hasPermission('delete own ' . $type . ' content', $account) && ($account->id() == $node->getOwnerId()))->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
      }

    default:
      // No opinion.
      return AccessResult::neutral();
  }
}

Rerolling the patch from #17.

The below code shows how a module could implement the magic *_any and *_own operations.

/**
 * Implement hook_webform_access().
 */
function CONTRIB_MODULE_webform_access(\Drupal\webform\WebformInterface $webform, $op, \Drupal\Core\Session\AccountInterface $account) {
  /** @var \Drupal\webform\WebformAccessRulesManagerInterface $access_rules_manager */
  $access_rules_manager = \Drupal::service('webform.access_rules_manager');

  // Add support for *_any and *_own access rules.
  $access_rules = $access_rules_manager->getAccessRulesInfo();
  if (isset($access_rules[$op . '_any'])
    || (isset($access_rules[$op . '_own']) && $webform->getOwnerId() === $account->id())) {
    return $access_rules_manager->checkWebformAccess($op, $account, $webform);
  }
  else {
    return AccessResult::neutral();
  }
}

/**
 * Implement hook_webform_submission_access().
 */
function CONTRIB_MODULE_webform_submission_access(\Drupal\webform\WebformSubmissionInterface $webform_submission, $op, \Drupal\Core\Session\AccountInterface $account) {
  /** @var \Drupal\webform\WebformAccessRulesManagerInterface $access_rules_manager */
  $access_rules_manager = \Drupal::service('webform.access_rules_manager');

  // Add support for *_any and *_own access rules.
  $access_rules = $access_rules_manager->getAccessRulesInfo();
  if (isset($access_rules[$op . '_any'])
    || (isset($access_rules[$op . '_own']) && $webform_submission->getOwnerId() === $account->id())) {
    return $access_rules_manager->checkWebformSubmissionAccess($op, $account, $webform_submission);
  }
  else {
    return AccessResult::neutral();
  }
}

The above code snippet should be limited to only the contrib module's access rules by changing…

$access_rules = $access_rules_manager->getAccessRulesInfo();
- to -
$access_rules = CONTRIB_MODULE_webform_access_rules()

Next step would be to update the API docs to remove the notes about the magic methods.

Attached patch updates hook_webform_access_rules() API documentation.

After this patch is reviewed, approved, and committed we need to update https://www.drupal.org/node/3000979.

Some minor fixes.

Hello, Jacob!

I fell out for a couple of day, had a family emergency.

I have reviewed your patches and follow your logic. I just have one point.. In the example of hook_webform_submission_access this

  if (isset($access_rules[$op . '_any'])
    || (isset($access_rules[$op . '_own']) && $webform_submission->getOwnerId() === $account->id())) {
    return $access_rules_manager->checkWebformSubmissionAccess($op, $account, $webform_submission);
  }

should rather be:

  $access_any = isset($access_rules[$op . '_any'] ? $access_rules_manager->checkWebformSubmissionAccess($op . '_any', $account, $webform_submission) : AccessResult::forbidden();
  $access_own = isset($access_rules[$op . '_own'] && $webform_submission->isOwner($account) ? $access_rules_manager->checkWebformSubmissionAccess($op . '_own', $account, $webform_submission) : AccessResult::forbidden();
  $access = $access_any->orIf($access_own);
  if ($access->isAllowed()) {
    return $access;
  }

And then I also replaced direct invocation of MY_MODULE_webform_access_rules() with a more 'best practice' invocation via 'module handler' service. I attach the patch that elaborates both of these suggestions on top of your last patch.

I think the patch is fine and it should be committed ASAP.

I do think we need to review how we are handling within the Webform module the returning of AccessResult and see if we can simplify our code. @see https://www.drupal.org/project/webform/issues/2956771#comment-12772536

I will create a ticket to review and refactor AccessResult handling.

Marking RTBC. I will commit the patch next week and tag a new release.

I committed the patch. @bucefal91 Thanks for the help with this issue.

From my side I'd like to offer my apologies for introducing this security breach into the module.

@bucefal91 Apology accepted!