Webform submission entity access is not enforced in view query

Oh lord, you stamped upon a big chunk of work. Let me transfer this ticket to the webform issue queue because that's where it must be dealt with. (I am a co-maintainer of Webform too, so it doesn't mean I am just ditching you :) )

Jacob, there is basically access bypass when we run a views query against webform_submission table. Let me describe what I was able to find out.

Let's first consider how Views run a query against Node entities. The NodeViewsData defines the property 'access query tag' on the 'node' base table:

  public function getViewsData() {
    $data = parent::getViewsData();

//  .................
    $data['node_field_data']['table']['base']['access query tag'] = 'node_access';
// .................
  }

This property is then picked up by Drupal\views\Plugin\views\query::execute():

  public function execute(ViewExecutable $view) {
// ....................

    if (empty($this->options['disable_sql_rewrite'])) {
      $base_table_data = Views::viewsData()->get($this->view->storage->get('base_table'));
      if (isset($base_table_data['table']['base']['access query tag'])) {
        $access_tag = $base_table_data['table']['base']['access query tag'];
        $query->addTag($access_tag);
        $count_query->addTag($access_tag);
      }
    }
// ...........................
  }

So then node_query_node_access_alter() hook gets invoked and that's where they supply necessary additional WHERE clauses to filter out nodes which are unaccessible for current user. I thought D8 was doing that automatically but it actually seems like it's the responsibility of the module that provides a new entity type to enforce the access on SQL level that way.

Apparently, in the case of webform_submission nothing of the above happens, so the view successfully queries all the submissions independently of user access.

You can try building some simple view.. you do not even need Webform Views, just throw in a 'completed' field, for example. Then create such a user that does not have access to a submission (i.e. he should face access denied page if he opens /admin/structure/webform/manage/[webform-id]/submission/[webform-submission-id]). Then navigate to the view being logged in as such user and observe the 'completed' field successfully displayed.

See the 2 screenshots I am attaching. I can volunteer myself for the patch. I have a pretty clear picture by now of how to do and what goes wrong. I'll make sure to put up tests too to make sure it stays covered.

@bucefal91 I definitely need and appreciate your help with this issue. You have a much greater understanding of Views. When possible please provide comments and references in your patch. It helps to know which code from core is being used to inspire your solution. Thanks.

Jacob, here are 2 tests that pin point the issue. They will fail now because access enforcement is not made in views query.

There are 2 actually similar issues:

• Access is not enforced via access rules
• and access is not enforced via user's permissions

hence the 2 tests instead of 1.

I will try to 'walk through' this ticket on my training in Atlanta the day after tomorrow :)

Jake :) hi!

Today I had time to review and polish the patch I showed you during Atlanta camp.

Basically it has 2 logical additions on top of my patch #4 which barely exposed the issue via tests.

The 1st chunk is bugfix itself - as I was trying to explain it in Atlanta, we try to 'map' whatever user's permissions and access rules from webforms onto SQL language so the database returns exactly what user is allowed to view.

The 2nd part is the thing about caching - views by default uses cache-tag based caching algorithm. And the views result would get cached and wouldn't be refreshed if you change access rules of a webform. Just a few lines of code in ::doPostSave() take care of that - they double check if the webform that has just been saved has any changes in access rules; in case it does, it resets the webform_submission_list cache tag which is basically responsible for any listing of webform submission entities, views being among such listing. That way we manage to reset views cache and do it in a relatively gentle way.

I attach the interdiff and the overall patch to this comment.

I messed up a little bit my git local repo when uploading my last patches. Let me upload the correct set of files.

Let's see if this addition solves the test failures. It should solve at least part of them. \Drupal::entityQuery() enforces access check by default for the current user, so I had to adjust the simpletest trait to explicitly tell it not to enforce access check since that ::getLastSubmissionId() is being used only from within test code and thus should not be access restricted.

Let's see if this one takes us any further. There are actually a lot of places where \Drupal::entityQuery() is being used and by default this guy enforces access checks. In some spots these access checks make sense, in others they do not. For example, at the moment of checking submission limits we do not want to use the access checks because even if the user does not have access to the submission, the submission itself should still count towards the limit.

Alright, here's a next iteration. A few additional spots where ->accessCheck(FALSE) was required on entity query. Additionally, from the failing tests it became obvious I should not enforce access check if the user has "administer webform" permission.

At this point I hope to see only a couple of failing tests.

Ahm... let's see if I am lucky :)

Yes, I am!!! :)

Jacob, all the spot lights are on you now :) Basically I have implemented the access restrictions on SQL level. Views and \Drupal::entityQuery() automatically pick it up. In case you do not want the result of entity query to contain stuff exclusively visible to the currently logged in user, then just invoke ->accessCheck(FALSE) on the query object to disable it.

Rest of the patch is tailoring the pre-existing test cases. Many of them had somewhat relaxed their setup, i.e. some lacked a permission on the dummy user or things of that kind and so far it wasn't a problem because there was no access enforcement on SQL level. But as it kicked in, all of these minor aspects came afloat on the surface.

What do you say? Ain't it look beautiful? :)

The patch looks marvelous, especially because all the tests are passing. The webform_query_webform_submission_access_alter() looks really understandable. I will review the patch.

I am not sure we need WebformAccessRulesManager::queryTagWebformSubmissionAccessAlter. Maybe all this logic could be moved in to webform_query_webform_submission_access_alter()

I having a hard time following WebformSubmissionViewsAccessRulesTest and WebformSubmissionViewsPermissionsTest. There is a lot going on in the tests and maybe the code's setup and flow need to be better documented.

BTW, I would like to make these tweaks and some minor code cleanup. I could use more experience writing PHPUnit tests.

I am marking this as critical because it is a stable release blocker.

Hi!

I introduced WebformAccessRulesManager::queryTagWebformSubmissionAccessAlter() with the idea that 'everything related to interpreting access rules belongs to WebformAccessRulesManager class'. I do not insist that we keep this method around. I can incorporate most of it into the webform_query_webform_submission_access_alter(). Let me see if I can attach an alternative patch where I'll try to get this done.

About the 2 tests: hehe, I actually thought they would appear straight forward to you :) Let me try to explain them.. They both function pretty much along the same line: we are testing 3 difference scenarios:

1. User without any access to submissions
2. User with 'view own' access to submissions
3. User with 'view any' access to submissions

The difference between the 2 tests is that one articulates access through user permission (WebformSubmissionViewsPermissionsTest) and another through webform access rules (WebformSubmissionViewsAccessRulesTest). Then, for each of the 3 test cases, we run the following:

• Execute a pre-crafted view that just displays a table of submissions. We are going to test that view contains only a subset of submissions to which the currently logged in user has access to.
• So once we've opened admin/structure/webform/test/views_access we just parse the output and figure out which submissions were visible on that page, the page produced by the view.
• Then, for all the webform submissions in the system, check if current user actually has 'view' access to it. In case he does, assert the submission was present in the view output, otherwise assert the submission was absent in the view output.

What i was trying to test there is that view's output blindly follows the actual access logic.. i.e. view must obey whatever yields $webform_submission->access('view'). By 'obey' I mean either showing or hiding the row.

@bucefal91 Thanks for the explanation. Are you okay if I create the next patch and make some minor tweaks to the tests.

Let's hold off from doing anything to WebformAccessRulesManager::queryTagWebformSubmissionAccessAlter().

Yes, sure. Do take the lead :)

My changes to webform_query_webform_submission_access_alter*() include…

• Using ?: operator to set $op and $account
• Move any and own permission logic into if/then code block.
• Adding more comments

Changes to tests…

• Removed webform_test_submission_views.module and moved the new view to webform_test_views.module
• Use the default 'contact' webform for tests
• Add API comments to all variables
• Whenever possible use single quotes.
• Consolidate tests into one test class
• Allowing for some duplicate code to improve readability.

Some personal notes

Your code and approach to solving this challenging problem and even your tests are really smart. Still, try to make sure to keep it simple.
For the tests, it is okay to have some duplicate code. Once there are more than two instances of any code, it is worth refactoring and removing the duplication.

Next steps

Remove WebformAccessRulesManager::queryTagWebformSubmissionAccessAlter

So I ran into a major issue with cache invalidation because adding cache tags to the renderer via webform_query_webform_submission_access_alter() does account for the actual View being cached and needing it's actual cache tags updated.

Conceptually, it started to get real tricky deciding how to add cache tags to the executed View based on webform_query_webform_submission_access_alter() so I decided to take a different approach and rely on WebformSubmission's list_cache_tags.

By adding "config:webform_list" to the WebformSubmission's list_cache_tags it makes sure anytime a webform is saved any list of submission is updated. For changes to users and permissions (aka roles) I had no choice but to add _webform_clear_webform_submission_list_cache_tag($entity);

I also adding caching for webform access rules to prevent having to load every webform for each query.

@bucefal91 Maybe we can figure out how to add the 'user' cache context to all webform submission views.

Fiufff.... I wanna go to sleep soon, so I'll just post what I got so far. It is all superficial:

• After your refactoring WebformAccessRulesManager no longer needs entity_type.manager service to be injected into it.
• In the same class, since there are a couple of methods you exposed as 'public', the @inheritdoc comment will suffice on them since now they are documented on the interface level.
• In the test class, I corrected the assert message from "has incorrect access" to "has correct access" :)
• In the same test class, I renamed "anonymous_user" to "without_access" because that's what it technically is. That user is a logged in one, just without any access granted.

A few points I will investigate tomorrow and will post additionally:

• In the test class, the $check_access argument of ::checkUserSubmissionAccess() looks worrying. I see the test fails without it. But I'd expect the test to pass both with granted and revoked permissions. After all, it's not a rocket science that we are doing there - just making sure $webform_submission->access('view') is replicated onto the view result. I would like to investigate why the test fails without the $check_access tomorrow.
• That's a lot of action there about the cache invalidation. Views design indeed looks a little short in terms of cache tags and cache contexts. I have found a couple of clues on how to append 'user' context into the view but I need some time to confirm the result looks prettier than the current code :) I also favor adding 'user' context instead of doing all this stuff.

Thanks for reviewing the patch. All your changes make sense.

In the test class, the $check_access argument of ::checkUserSubmissionAccess() looks worrying.

I did this because the WebformSubmission::access() results are being cached within the actual tests. I did not see any immediate way to clear them without probably clearing the Views cache. The purpose of the second ::checkUserSubmissionAccess() call is to make sure no submissions are exposed.

That's a lot of action there about the cache invalidation.

I felt it was safer to be sure that the webform submission cache was cleared verse accidentally exposing data to the wrong user. We might only need to clear the 'webform_submission_list' cache tag when a role is updated since almost everything in Drupal is cached (and cleared) per user.

Yep. I understand you about the $check_access. It just smells to me like a possible crack where at some point a bug might sneak through. Through error&trial I discovered we had to reset static cache of webform submission storage and access handlers. I am attaching an interdiff that simplifies away this $check_access parameter and still yields all assertions positive.

As a next step I will now see if we can inject 'user' context into the view's caching so we can simplify the cache invalidation procedures.

Are you sure that resetting the cache does not invalidate the second test which is to confirm that when a permission or access rule is changed the submission view's cache is reset.

To the best of my knowledge, we are good there. If we go to examine the source code of both ::resetCache() methods, they turn out to be pretty simple.

Access control handler:

  public function resetCache() {
    $this->accessCache = [];
  }

Really not much, just resetting an in-memory cache for this script run.

Storage handler:

  public function resetCache(array $ids = NULL) {
    if ($ids) {
      parent::resetCache($ids);
      if ($this->entityType->isPersistentlyCacheable()) {
        $cids = [];
        foreach ($ids as $id) {
          unset($this->latestRevisionIds[$id]);
          $cids[] = $this->buildCacheId($id);
        }
        $this->cacheBackend->deleteMultiple($cids);
      }
    }
    else {
      parent::resetCache();
      if ($this->entityType->isPersistentlyCacheable()) {
        Cache::invalidateTags([$this->entityTypeId . '_values']);
      }
      $this->latestRevisionIds = [];
    }
  }

and the parent::resetCache() from above ends up being:

  public function resetCache(array $ids = NULL) {
    if ($this->entityType->isStaticallyCacheable() && isset($ids)) {
      foreach ($ids as $id) {
        $this->memoryCache->delete($this->buildCacheId($id));
      }
    }
    else {
      // Call the backend method directly.
      $this->memoryCache->invalidateTags([$this->memoryCacheTag]);
    }
  }

A little more of action, but still quite limited - it resets in-memory cache ($this->memoryCache) if it's statically cacheable and in case it is also cacheable in some kind of persistent storage, reset there too ($this->cacheBackend). I do not see that we could affect views caching with these 2 methods.

Alternative way around it would be to check whether the user is supposed to access the webform submission through an HTTP request in the test browser. Since we'd be using the underlying test browser, it will be an independent script run and we will not be facing issues of static caching.
I just replaced locally this

      foreach ($webform_submissions as $webform_submission) {
        if ($webform_submission->access('view', $account)) {
          $expected_sids[] = $webform_submission->id();
        }
      }

with

      foreach ($webform_submissions as $webform_submission) {
        $this->drupalGet($webform_submission->toUrl());
        if ($this->getSession()->getStatusCode() == 200) {
          $expected_sids[] = $webform_submission->id();
        }
      }

So we basically try to open the webform submission with currently logged in user and take it as a 'yes' or 'no' on whether user is expected to see that submission in the view results.

This alternative approach gives me green light in local tests. I personally do not have preference between these 2 implementations. Though in the 2nd implementation it's bulletproof we are not messing with views caching.

Your explanation from #30 is good enough for me.

I've given it something like 3 hours now... No positive result. By now I come to conclusion the code you have written is the best we can do about invalidating views cache.

There is views core issue which approaches this problem: #2811041: Allow views base tables to define additional cache tags and max age, but it's not committed yet. I personally deem the last patch OK.

Yep, and it took me 3 hours to come up with that solution.

Committed :)

This looks like a good and necessary fix, but I'm wondering if there should be a change record.

I'm not 100% sure the fix in this issue is my culprit, but it seems likely. I'm extending a webform with some custom code that relies on submission data and after updating to 8.x-5.0-rc27 from -rc21 my code stopped working for anon users. Adding accessCheck(FALSE) to my query was simple and did the trick, but it took me a little while to track it down.

Thanks for your great work. Cheers -

Here is the change record.

Thanks very much.