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 bothWebformEntityAccessControlHandler
andWebformSubmissionAccessControlHandler
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 keepWebform
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 Done.webform.webform.*.access
from mapping to a sequence.
Comments
Comment #2
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedIt 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)
Comment #3
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedHere is the first small incremental improvement.
#2995847: Convert access rules scheme from a static list of rules to a mapping
Comment #4
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedRoger 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.
Comment #5
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedRe-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.
Comment #6
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedComment #7
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedComment #8
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedI feel this thing is defined now. I'll spawn a few child tickets and will get busy coding.
Comment #9
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedI 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()
.Comment #10
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedAgreed :)
Comment #11
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedReferencing sub-tickets on appropriate remaining tasks bullet points so it's easier to map verbal description with the actual sub-ticket.
Comment #12
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedDocumenting 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
Comment #13
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedAdding one more sub ticket for access rules manager.
Comment #14
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedAlright. 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).Comment #15
jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commentedThis change record is fine.
Comment #16
bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commentedI 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.