Allow webform handlers to control element access

Here's a rough draft (i.e. it runs, but hasn't been tested) for your consideration. It makes the hook and webform handler element access handling be similar. If this is acceptable to you, I'll keep going. (To do: handle when submission is absent.)

I think the issue is WebformElementBase::checkAccessRules(). It's unfortunately that PHP doesn't offer any way to distinguish "public" as in "I'm gonna use this in another class, but all you consumers of my code keep your mitts off" from "You should use this in your code". I very, very much doubt that any code calls checkAccessRules, but it certainly is theoretically possible.

Solutions:
1) Pass the submission ID in the element and re-load the submission and webform in checkAccessRules. This would allow these to be passed to the hook and handler implementations. In 8.x-6.x, this could be cleaned up without having to change the signature of the hook and handler. But that is potentially a lot of calls to Entity::load(), even though the actual entity will be cached, it's not nice for performance for a large webform.

2) Pass the submission ID to the element and defer the re-load of the webform and submission to the hook and handler. This would be less computational work for when the webform and/or submission isn't needed, but it creates a weak signature that doesn't supply enough context to the hook and handler implementation. If this is fixed in 8.x-6.x, the hook and handlers would have to be updated.

3) Pass the webform and submission at the end of the signature to checkAccessRules. This would allow other callers to use the old signature. The implementation could load the webform, but it would not be able to load the submission (since the caller would not have loaded it in #webform_submission.

4) Leave the signature of checkAccessRules and document this in the release notes. The down side is that should anyone call this function, they would have to update before their code will run. OTOH, with solutions 1-3, they code will run but may not provide correct access checks because the code won't have the submission.

5) Create a new function to replace checkAccessRules() with the preferred signature, leaving the old version in place. Then rename the function and remove the old version in 8.x-6.x. This gives us signatures for the hooks and handlers which have context and won't break code (although it won't work correctly if the context is required by the site).

I propose 5) with the name checkAccessRulesWithContext to be renamed back to just checkAccessRules in 8.x-6.x.

UPDATE: I think we should do 3) and revise the signature in 8.x-6.x with a change record to put the account back at the end where one typically finds it.

@jrockowitz This is very inner-loop code. I stepped through Entity::load() in checkAccessRules</code). Each <code>Entity::load() is 57 PHP debugger steps. So that 57 x 2 loads x #elements x #operations x (1 + #hooks or handlers that load too). For example, with 100 elements, view+update+delete, and one handler/hook, that's 68,400 unnecessary debugger steps. I'm not sure if this access is checked for reports, but that would be that x # rows in the report.

In my mind it would be better to pass the webform and submission to the checkAccessRules and any hook / handler implementations to avoid all this unnecessary work. And a side benefit is that you are creating an API for the future that is easier to use and understand. Plus I think Entity::load() will be removed in D9 (although webform could implement it for its entities).

This can be done in a backwards-compatible way by passing webform and submission as new arguments after the account.

Your call of course. If you prefer to keep it as it, it looks good to me but I want write an implementation and test it.

By way of context (pun intended), the port of my webapp from D7 to D8 has yielded performance about 10x slower. I just ordered the fastest laptop I could find to make development less painful. I'm not sure how much caching is going to help me because the usage as an app is all authenticated and individual (people doing different things). It may be that I abandon this project after over a year of work and port D7 to backdrop. :(

Thanks for this patch. Some thoughts:

1) There are 67 calls to getElementInstance. In each of these, the applicable context should be determined and passed. That's a pretty big set of changes, but I'd be happy to help if that's the way you want to go. The problem is that we can't be guaranteed that the proper context will be passed, which would lead to access (i.e. security) errors. Therefore the webform and submission context in the WebformElement probably cannot be relied upon since getElementInstance is public. If we required PHP 7.1, we could use a nullable type (... ?WebformInterface $webform ...). This would at least cause runtime failures if someone outside of webform calls getElementInstance without any context. But I don't think we can require PHP 7.1. Poo.

2) Minor point: I don't see any advantage in passing only one context variable to getElementInstance since it just then has to tease out the webform vs submission. I'd pass both and use the submission to find the webform if it is passed null. Also it provides better type checking on the paramenters.

3) Since we can't rely on the context in the WebformElement, we would have to manually set it using the two new setter methods immediately prior to calling checkAccessRules(). From a personal style perspective, I see this as more brittle than passing them arguments. It would be easy in the future to forget to set the context. Minor point though.

4) I still see no reason to not pass the context down to the hook implementation (but then I'm not using them so I don't really care).

5) For the handlers, it seems like the submission is set in the invokehandlers method. Is the webform guaranteed to always be set? I'm confused about why the submission is passed to the submission-related handler methods (e.g. postLoad, preSave, etc). If the handler can always get the webform and/or submission by its own getter, then certainly it doesn't have to be passed.

Not sure if I'm helping or hurting progress at this point. :)

Comments on #19

1) WebformSubmission::checkAccessRules I suggest passing $context = ['webform' => $webform, 'webform_submission' => $webform_submission] to webform_element_access hook implementations. Update the example in webform.api.php.

2) WebformSubmission::checkAccessRules Remove && $webform_submission from code protecting invokeHandlers call. The handlers should be called if there is a webform but no submission. Not every handler implementation will need a submission.

Also, wondering if it would be wise to throw an exception if $webform or $webform_submission is NULL but $element[[x] isn't empty. This would indicate an error condition and a possible security vulnerability.

3) WebformElementBase::prepare Unless you are absolutely assured that $this had had its context set, call ::setWebformSubmission and maybe ::setWebform before calling checkAcccessRules. I don't know enough about when prepare is called to know whether this is necessary.

Otherwise, it looks good to me. Super job!

Philosophical thought. I don't think it's super wonderful that the webform submission in the WebformElements is usually set but can't unequivocally be relied upon.

Ideally, I think the webform and webform_submission should be set when elements objects are created and updated when outdated (if that happens). Then you wouldn't have to set them before using them in checkAccessRules, the hooks, and the handlers. This would be something to consider for 8.x-6.x. For development purposes, you could throw an exception when the submission isn't what you expect it to be to help find issues.

I think this stems from the WebformElement objects not encapsulating the form elements because form elements are dumb arrays, rather than objects. I'm not sure this can be improved until core makes the rendering and FAPI more object oriented.

I'm happy to help more with this issue, but I haven't been very successful so far. We were working in parallel and you were faster than I. :)

Looking over #24:

- WebformElementBase::checkAccessRules There is an inconsistency when no $webform context is present. If the element is private, it just returns false. If not, it throws an exception. I suggest a test for !$webform early in the code and throw the exception there. As a plus, the code is shorter/cleaner.

Patch attached.

I'm not sure how I indicate to apply #26 on the issue branch for testing.

I was surprised to have my handler::accessElement called with an element that lacked a #webform_key. I see that this is used in WebformOtherBase::processWebformOther() to build the sub-elements of a select-other element.

I'm don't see how WebformElementBase::checkAccessRules can have an opinion on access to these sub-elements since a) it lacks context to know what they are and b) they won't have any access rules. So I'm thinking that at the top, (right after the #access check) it should have:

  // Always grant access to sub-elements of a main webform element.
  if (empty($element['#webform_key']) {
    return TRUE;
  }

Without this, hook and handler implementations will be called on elements that they might not be expecting. And it's a tiny bit faster.

Am I wrong? Setting back to Needs Work pending your thoughts.

How would #private get set on a sub-element? Through an alter webformhandler? Are these sub-elements used for view or does the main element view for them (in which case private wouldn't matter)?

If access control for sub-elements is a useful feature, then this could just be documented with a comment in WebformHandler::accessElement and the hook example in webform.api. Maybe something like "Note: This hook/handler may be called with any sub-elements used to implement a main element, in which case '#webform_key' will be absent from $element.

I don't have an opinion. I just didn't expect to be called with an element without a #webform_key.

I see the new comments in webform.api. I didn't see anywhere that documents the ability to set '#SUBELEMENT__PROPERTY' or that accessElement should expect sub-elemens without a webform_key. Did I miss it or maybe you didn't intend to document these?

Aside from any desired additional docblock comments, I think this is RTBC.

Commit / revert. Problem?

I assume in webform group you deleted changed checkAccessRules and revised webform_group_webform_element_access() to return AccessResult?

Looking at the webform_group implementation, it should return return allowed where it returns true and neutral where it returns false or null.

Returning false in it's current implementation will be logically or'd with the result of checkAccessRule (by virtue of the array_sum). checkAccessRule never returns null.

I'm guessing that you had webform_group's hook returning forbidden, which is not at all what returning FALSE meant in the previous implementation in webform_group.

Suggest you apply this issue, then revert the changes in webform_group checkAccessRules, then in the hook implementation change TRUE to allowed and FALSE and NULL to neutral.

BTW, maybe you are aware of this, but the AccessResult rules are inconsistent and confusing. Entity access uses orIf() and this is consistent with the names allowed, neutral, and forbidden. However route access uses andIf() and in that case "neutral" really means "deny" and "forbidden" really means "super deny, even if you orIf() this result subsequently".

I took a stab in the webform group issue to merge webform group on top of this issue's #39. I have no idea why tests are failing.