We need a hook that allows external modules to throw in extra permissions into the pot of webform access logic. The hook should look like: 


function MY_MODULE_webform_access_rules() {
  return [
    'update_own' => [
      'title' => t('Update own submissions'),
    ],
    'update_any' => [
      'title' => t('Update any submissions'),
    ],
 // ... and so on.
  ];
}
CommentFileSizeAuthor
#17 interdiff-15-17.txt13.07 KBbucefal91
#17 2996249-permission-hook-17.patch30.5 KBbucefal91
#15 interdiff-11-15.txt32.82 KBbucefal91
#15 2996249-permission-hook-15.patch32.82 KBbucefal91
#11 interdiff-9-11.txt5.62 KBbucefal91
#11 2996249-permission-hook-11.patch15.03 KBbucefal91
#9 interdiff-7-9.txt4.02 KBbucefal91
#9 2996249-permission-hook-9.patch12.59 KBbucefal91
#7 interdiff-5-7.txt821 bytesbucefal91
#7 2996249-permission-hook-7.patch10.4 KBbucefal91
#5 2996249-permission-hook-5.patch10.39 KBbucefal91
#2 2996249-permission-hook-2.patch8.81 KBbucefal91

Comments

bucefal91 created an issue. See original summary.

bucefal91’s picture

bucefal91 commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new 2996249-permission-hook-2.patch8.81 KB

... Renaming the hook to its final name in the main description.

So here's the patch. It does:

  • Introduce a new hook to collect access rules from other modules
  • Introduce a hook to alter access rules collected from other modules
  • A helper method WebformInterface::getAccessRulesInfo() which takes care of actually invoking the hooks, so whoever needs a clean shining set of $access_rules can simply invoke that method.
  • Adapts WebformEntitySettingsAccessForm so it becomes aware of additional access rules supplied through hook.
  • Modifies access handlers for Webform & Webform Submission entity types so they incur to check $access_rules a little more eager - because now we could receive a request like \Drupal::entityTypeManager()->getAccessControlHandler('webform')->access('my_new_access_rule_from_another_module'); and the only way to resolve it is to consult the existing 'access' settings of a webform, i.e. to delegate it to Webform::checkAccessRules()
  • Due to changes in the bullet point above I had to slightly adapt the unit test WebformEntityAccessControlHandlerTest.

I testify the following works:

  • Tests pass 100% on my localhost
  • The hook actually damn works! :) I implemented a hook with dummy new access rule and it is possible to set up access logic for this new operation in the 'Access' tab of webform settings.
  • Such new access rules do yield correctly TRUE or FALSE both on Webform & WebformSubmission entity types. I.e. \Drupal::entityTypeManager()->getAccessControlHandler('webform')->access('my_new_access_rule_from_another_module'); yields TRUE when it is supposed to per current access settings of the webform.. and so does \Drupal::entityTypeManager()->getAccessControlHandler('webform_submission')->access('my_new_access_rule_from_another_module');.

As far as I can see, we are ready to commit this thing.

Status: Needs review » Needs work

The last submitted patch, 2: 2996249-permission-hook-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz commented

The static Webform::getAccessRulesInfo() feels awkard. Should the access rules related code even be in the Webform entity class?

Should all access rules related logic be moved to WebformEntityAccessControlHandler?

We would need to define a WebformEntityAccessControlHandlerInterface.

$access_control_handler = \Drupal::entityTypeManager()->getAccessControlHandler('webform');
$access_control_handler->getAccessRules()

Or should we create a WebformEntityAccessRulesManager?

The WebformEntityAccessRulesManager could be injected into WebformEntityAccessControlHandler.

@bucefal91 What do you think? I am not able to find any similar patterns in core.

I am leaning toward a WebformEntityAccessRulesManager service.

bucefal91’s picture

bucefal91 commented
Status: Needs work » Needs review
StatusFileSize
new 2996249-permission-hook-5.patch10.39 KB

Let me first nail down the test failures (which I hope are nailed with this patch I am uploading). So I make sure it contains no bugs and then I'll get started with re-arranging the code so it looks&feels beautiful.

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

I think WebformEntityAccessRulesManager service would be nice because both WebformEntityAccessControlHandler and WebformSubmissionAccessControlHandler are using it, so packing it into WebformEntityAccessControlHandler wouldn't be appropriate (because it'd make submission access handler to "depend" on webform access handler).

Ehm, then let me file another sub-task where I will introduce the rule manager. Then I'll return to this issue and will re-write the current patch to use the new rule manager. That way we'll keep the patches small

Status: Needs review » Needs work

The last submitted patch, 5: 2996249-permission-hook-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 commented
Status: Needs work » Needs review
StatusFileSize
new 2996249-permission-hook-7.patch10.4 KB
new interdiff-5-7.txt821 bytes

Let's see this one.

Status: Needs review » Needs work

The last submitted patch, 7: 2996249-permission-hook-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 commented
Status: Needs work » Needs review
StatusFileSize
new 2996249-permission-hook-9.patch12.59 KB
new interdiff-7-9.txt4.02 KB

My localhost tests timed out while running, so I guess I should leverage d.org CI infrastructure. I hope this time we are going to see 100% pass.

Jacob, I do not think it's worth the effort to review the patch yet for you as I am still going to introduce the access rule manager service and it will inevitably affect the patch from this issue. I'll ping you when I beleive I have all this stuff ready for your review :) Just don't want to disturb you away from your things.

Status: Needs review » Needs work

The last submitted patch, 9: 2996249-permission-hook-9.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 commented
Status: Needs work » Needs review
StatusFileSize
new 2996249-permission-hook-11.patch15.03 KB
new interdiff-9-11.txt5.62 KB

let's see. I managed to introduce an infinite recursion with my last patch. :)

This one I am attaching is dirty, but I hope it does pass the tests. It's been a challenge for me... By fixing one test failure I introduce another and then it goes in circles. Since I am trying to amplify the access system it was a little bit difficult to amplify it without altering pre-existing behavior.

If this patch pass tests (which I hope it does), then I'll go to code the access rules manager. Once that one lands, I will rewrite this patch with heavy usage of the access rules manager and the "dirt" this patch contains will dissolve with smart use of access rules manager.

Status: Needs review » Needs work

The last submitted patch, 11: 2996249-permission-hook-11.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz commented 
+++ b/src/WebformEntityAccessControlHandler.php
@@ -9,6 +9,8 @@ use Drupal\Core\Entity\EntityInterface;
@@ -134,8 +136,9 @@ class WebformEntityAccessControlHandler extends EntityAccessControlHandler imple

@@ -134,8 +136,9 @@ class WebformEntityAccessControlHandler extends EntityAccessControlHandler imple
-    // Check test operation.
-    if ($operation === 'test') {
+    // Check if access rules has anything to say about this operation.
+    $access_rules = array_keys(Webform::getDefaultAccessRules());
+    if ($operation == 'page' || in_array($operation, $access_rules) || in_array($operation . '_any', $access_rules) || in_array($operation . '_own', $access_rules)) {

I think this change is causing the problems because the 'test' operation is no longer being handled.

You might be tweaking too much in the WebformEntityAccessControlHandler.

bucefal91’s picture

bucefal91 commented

"test" operation is part of the access rules, so it would be covered by this part of the condition in_array($operation, $access_rules).

I ran this failing test locally it and did pass. I see there were some recent commits related to "test_confirmation_none" webform (the spot where tests failed). Maybe it was just a case of bad luck.. when the tests started the commits landed and somehow interfered with normal test execution. Re-submitting for testing did get me green light.

Let me switch to the access rules manager now. I hope to have that one coded either today or tomorrow.

bucefal91’s picture

bucefal91 commented
Status: Needs work » Needs review
StatusFileSize
new 2996249-permission-hook-15.patch32.82 KB
new interdiff-11-15.txt32.82 KB

Actually since we've split the access logic between WebformEntityAccessControlHandler and WebformSubmissionAccessControlHandler, my latest patch here is obsolete. So it took me some time to analyze it and come up with a new one. I am attaching it to this comment.

If it passes tests, I believe it's ready for commit.

jrockowitz’s picture

jrockowitz commented

On /admin/structure/webform/manage/contact/access. I am getting the below error message

Notice: Undefined index: say hi in Drupal\webform\EntitySettings\WebformEntitySettingsAccessForm->form() (line 54 of /private/var/www/sites/d8_webform/web/modules/sandbox/webform/src/EntitySettings/WebformEntitySettingsAccessForm.php) #0 

User error: "description" is an invalid render array key in Drupal\Core\Render\Element::children() (line 97 of core/lib/Drupal/Core/Render/Element.php).

The array of test parameters in WebformEntityAccessControlHandlerTest ::providerCheckAccess should use a consistent formatting. Maybe all cache_contexts should be displayed on one line.

bucefal91’s picture

bucefal91 commented
StatusFileSize
new 2996249-permission-hook-17.patch30.5 KB
new interdiff-15-17.txt13.07 KB

Hehe, the "say hi" was a dummy access rule that I introduced to be able to test the hook. I just forgot to remove it .

My local Drupal had disabled the error message for some reason, so I overlooked those warnings. I've fixed them in this patch + did the formatting for 'cache_contexts' array.

jrockowitz’s picture

jrockowitz commented
Status: Needs review » Reviewed & tested by the community

The last task is to write a change record where you can pretty much cut-n-paste the API documentation.

  • bucefal91 committed b094a1a on 8.x-5.x 
    Issue #2996249 by bucefal91: Introduce a hook to collect additional...
bucefal91’s picture

bucefal91 commented
Status: Reviewed & tested by the community » Fixed

I've put up the change record (in "draft" state) https://www.drupal.org/node/3000979 and committed the patch :)

Status: Fixed » Closed (fixed)

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