Problem/Motivation

Webform module already has a powerful framework to define access/permissions on per-webform basis. However, right now there is no way to introduce additional operations into this framework from an external module.

We are developing a webform_score rewrite and I would love to allow access to see scores on per-webform basis. However, at the moment it is impossible for webform_score module to introduce the 2 additional operations that should be managed on per-webform basis: "see overall webform submission score" (to see an overall score a webform submission has) and "see webform element score" (to see scored points on each individual webform element).

Proposed resolution

It would be awesome if we could allow external modules to supply additional permissions that should be managed on per-webform basis. That way we get to reuse the feature-rich access logic webform has and offer it in a developer-friendly way to developers of external modules.

Remaining tasks

On Alex:

  • Introduce an intermediate WebformEntityAccessRulesManager service which will encapsulate the logic of working with access rules so both WebformEntityAccessControlHandler and WebformSubmissionAccessControlHandler can use it. #2996996: Introduce service to encapsulate access rules logic therein
  • Introduce the hook itself. Switch the existing codebase onto the hook. Webform::checkAccessRules() will not be affected. Webform::getDefaultAccessRules() will be affected, it should additionally merge the hardcoded array of defaults with whatever the hook invocation has collected. WebformEntitySettingsAccessForm::form() will be affected because it now has to build the form not on a static hardcoded array of known permissions, but rather collect them from outside. #2996249: Introduce a hook to collect additional webform permissions
  • Extract permissions from Webform::checkAccessRules() into the hook implementation. That way we keep Webform class a little simpler and our own implementation of our hook can act an example for external developers who ever mean to impelment for their own needs. #2996549: Migrate existing access rules onto the hook
  • Adapt webform_access module. Since the list of known permissions is no longer static, we need to replace the static array in WebformAccessGroupForm::form() with a dynamic list of permissions collected from the hook invocation. #2996551: Adapt webform_access module to access rules being supplied through hook
  • Have a look at everything :) Make sure all important parts are covered with tests.
  • Write a change record document.

User interface changes

None. It's a refactoring ticket.

API changes

A new hook is being introduced. Let's call it webform_permission_info(). It should return meta information about known permissions (operations) that can be executed on a webform/webform submission. Here's a simple PHP snippet of sample implementation:

function MY_MODULE_webform_permission_info() {
  return [
    'update_own' => [
      'title' => t('Update own submissions'),
    ],
    'update_any' => [
      'title' => t('Update any submissions'),
    ],
 // ... and so on.
  ];
}

Data model changes

Since now we are talking about unlimited amount of possible operations, we should change the data type of webform.webform.*.access from mapping to a sequence. Done.

Comments

bucefal91 created an issue. See original summary.

jrockowitz’s picture

It makes sense to allow access rules to be extended. This improvement would help address things #2985913: Decouple the permission to duplicate submissions from the ability to update submissions.

This is major change so let's consider this the [meta] ticket and make small non disruptive tickets and easy to review patches that can be individually committed. Ideally, anyone should be able to review each patch mark something RTBC.

We need to also consider also updating the webform_access.module (https://www.drupal.org/node/2991957)

The next steps would be to update this ticket to use this template. (https://www.drupal.org/node/1155816)

jrockowitz’s picture

bucefal91’s picture

Roger that. I am glad to see enthusiasm and support from your side. I will think through in depth this part and will then present you a set of issues that are atomic and small/easy to review.

bucefal91’s picture

Issue summary: View changes

Re-writing the issue description based on drupal standard issue template. Still has some blank spots that I will be filling out as time goes by.

jrockowitz’s picture

Title: Allow to extend access rules from other modules » [meta] Allow to extend access rules from other modules
Issue summary: View changes
bucefal91’s picture

Issue summary: View changes
bucefal91’s picture

Issue summary: View changes

I feel this thing is defined now. I'll spawn a few child tickets and will get busy coding.

jrockowitz’s picture

I think it should be hook_webform_access_rules() which corresponds with Webform::getDefaultAccessRules(). Not using the word 'permissions' in the hook might be a good thing because it removes any confusion with hook_permissions().

We should also provide a hook_webform_access_rules_alter().

bucefal91’s picture

Agreed :)

bucefal91’s picture

Issue summary: View changes

Referencing sub-tickets on appropriate remaining tasks bullet points so it's easier to map verbal description with the actual sub-ticket.

bucefal91’s picture

Issue summary: View changes

Documenting the additional step of introducing access rules manager service as we have discussed it in https://www.drupal.org/project/webform/issues/2996249#comment-12756619

bucefal91’s picture

Issue summary: View changes

Adding one more sub ticket for access rules manager.

bucefal91’s picture

Alright. Now that the last sub-task has been committed, we are entering the "wrap-up" phase with this meta ticket. I'll try to cut some time on Monday or Tuesday to give it a last thought of whether we need anything more here.

Do you want to introduce some "overall" change record for this meta ticket or the change records we've been publishing for individual sub-tasks is enoug? I personally favor the 2nd, but I wonder what you think.

As far as tests, I think we are still covered at decent level. I could see into writing a quick unit test to cover WebformAccessRulesManager. Covering it with a unit test makes better sense after we refactor away the usage of $_SESSION within the class (#2998830: Provide API for checking if $account is owner of a submission).

jrockowitz’s picture

This change record is fine.

bucefal91’s picture

Status: Active » Fixed

I believe we are all set here. I just tried to think of anything else I'd like to add within this refactoring and nothing came up to my mind. So I am marking this issue as 'fixed' :)

Thank you, Jacob, for your willing to dig through these changes/refactoring. Next thing I am going to do is the JSON security issue you pointed to me the other day.

Status: Fixed » Closed (fixed)

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