Add access control methods to webform handlers

Great work. As of D8.9, the meaning of "neutral" is context-dependent. Route access checking (andIf) and Entity access checking (orIf) is confusing. Care should (continue to) be taken when using AccessResult object for routes because "neutral" is really "deny" -- a single neutral result will deny route access. This isn't true of entity access checks. I'm not saying anything is wrong, but rather that we need to keep this in mind since "neutral" is very "non-neutral" for route access. It acts as "deny" for routes.

I changed the strategy so that the webform handler is always consulted, so that it can both allow or deny access.

Note: Untested. Just for comment really.

Here's a working patch. I do wonder whether we are naming WebformHanderBase::access the right thing. It might be better to call it ::submissionAccess so that if we want to implement webform entity and element access in handlers, we can do so without regretting using the generic ::access method name.

I have tested this manually to the extent that one handler does the expected thing. My handler returns neutral or forbidden.

The testbot seems to be sick. I requeued the tests, but they have been aborting for the last few days.

In looking at the code, the method "elementAccess" is used to control the ability to add elements of a given type to a webform, rather than access to specific elements within a submission that the user otherwise has access too. Should we go forward with extending webform handler access control to specific elements, some other method name should be used. Maybe submissionElementAccess or something.

I currently don't have a need for either webform entity or element instance access control via webform handlers, so I'm marking #10 RTBC. We can always add more methods later, either as part of this issue or another.

I do need element instance access control. I'll work up a patch to be applied after #10.

Thanks for the link to the element access hook. A problem that I'm struggling with is that I need the context of the webform submission where I check the access to the element. This isn't readily available in WebformElementBase::checkAccessRules, which itself is called from a few different places, not all of which have the submission readily available.

Use case: An evaluation system where the instructor writes evaluations of the student. One of these fields is confidential. Instructors can read confidential comments but student (reviewee) cannot. However, in the case where the student is also an instructor, the instructor needs to be prevented from viewing the confidential field. To check access to the confidential fields, the other fields in the evaluation that indicate who is being evaluated need to be compared to the current user account.

Thanks for your reply.

Looking at webform_group_webform_element_access(), I don't see any reference to either the webform or webformsubmission entities. I do see elsewhere where you get the submission from the request handler.

IMO, supplying just the element, operation, and account to the access function -- whether it's a hook or a handler method -- isn't supplying enough context to handle lots of use cases. I think both the element key and webform or submission entity would be extremely helpful. If you're creating a hook for webform_group, I suggest you create one with enough context to make implementations possible (or at least not inconvenient). This same signature could be used for the handler if desired.

A big advantage of the handler is not having to have a big switch statement to handle all the different webforms. Much better encapsulation.

I think the root issue is that some access can be determined just by the element and the account. This includes webform groups. But other access requirements need to consider the relationship between other webform submission data, the user, and the element in question. My use case example is knowing whether a confidential notes fields was written about the current user and denying access to such.

Or maybe I'm missing something. :)

$element['#webform'] contains the id of the webform. So you're thinking that $element['#webform_submission'] would contain the id of the submission? Bit of a bummer to have to go through the overhead of loading the entity but maybe that's the best we can do. Linking the actual entity will create caching problems I think (?).

Assume this happens for hook_webform_element_access, I suggest we do it for webform handlers too. It's right there (call the hooks, then call the handlers).

I'm also a little uncertain about how access is being managed. In WebformElementBase::checkAccessRules() the element's access rules are or'd together and then or'd with any of the webform_element_access hook results. I believe the idea is that a hook can set #access false to override this.

I think it would be cleaner and D8-ish to use AccessResults here. Create an allowed or neutral AccessResult and orIf() the results of each hook and webform implementation. Thoughts?

Excellent. Two changes:

1) I added the missing return from WebformSubmissionStorage::invokeWebformHandlers(). I don't know that this access method is ever called, so it's really just making the implementation match the interface and the other invoke methods.

2) I removed a bit more cruft from your refactoring of Webform::invokeHandlers():

            $result = $result->orIf($handler->$method($data, $context1, $context2));

You don't have to test result since it initialized to neutral and neutral orIf'd with any other value is the other value.

As a style, I don't love returning out of the bottom of one case in a switch because I think it invites future bugs if someone adds code to the bottom of the function, but there's certainly nothing technically wrong so I didn't make that change.

I have manually tested this code, including stepping through the invokeHandlers with an access callback in a WebformHandler and it all works as expected. I have also done a code review. I would say that assuming you find nothing wrong and tests pass, this is RTBC.

Marking as RTBC since most of this patch isn't mine.

Super. Once #23 is committed, do you want to continue with element access webform handlers in this issue or a new one?