Views entity operations lack cacheability support, resulting in incorrect dropbuttons

As spoken in IRC for now we can just do it like NodeViewBuilder and push the operation links (like node links) into a #post_render_cache.

Tagging this with contributed project blocker... file_entity tests are currently failing because of this.

I've confirmed that this fixes them, sounds like we need tests.

The problem with the test coverage in RowRenderCacheTest is that we have a single view that has separate view, edit, delete link fields *and* the operations link. It's enough for the tests to pass if *one* of them is implemented correctly. So operations look like they work right now, but they don't.. it only works because the other fields are adding the user.permissions cache context it and counts for the whole row.

Sugestion: Split up the test view into 4 different ones, test each field on a separate view.

Suggestion sounds good, I was already planning to do something like that, just forgot it.

Tried that. Splitting up the view into 4 makes the patch/test part pretty big.

The test shows that the title and operations column still dont' have the user.permissions cache context as expected. Need to look into #2335661: Outbound path & route processors must specify cacheability metadata to figure that out for the make_link option (title), didn't look into operations yet.

Also fixed a fun bug in the new access check that resulted in denying access to anything without #access. Result was an empty page for *everything*, confused me for a minute ;)

We need to default link access to TRUE. While that might be uncommon, it matches the current behavior. This will hopefully fix most of the failing tests...

Possible, but I'm not exactly sure how to write the test then, as we directly interact with the view object and don't render them in advance. Not quite sure how that would work, is it possible to have the same view with a different active display at the same time? Maybe cloning it and switching?

That doesn't fit well with how the test is structured now. We first preview the view(s), and then loop over all nodes and check the different fields. We'd need to change it to do 4/5 loops so that each field + the cache check is in a separate loop.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -138,7 +139,10 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
+    if (isset($elements['#access']) && (($elements['#access'] instanceof AccessResultInterface && !$elements['#access']->isAllowed()) || !$elements['#access'])) {
       return '';
     }

The reason this still fails is that we _need_ to return the cacheable metadata when this is an object.

And strictly speaking after this change we need to return max-age = 0 when its not an object, but just 'FALSE'.

Related issue #2495171: Block access results' cacheability metadata is not applied to the render arrays.

I think we want to move the #access portion of this patch be moved to #2487600: #access should support AccessResultInterface objects or better has to always use it?

Possibly this bug is causing #2504663: Entity operations links tied to original entity language, ignore both views row and UI languages, but in any case it is related.

No, not arguing that. Proceeding here is fine. I think I only meant to say that the #access portion of this issue, which is something we need to fix regardless of this particular issue, probably makes more sense to fix in a separate issue. That's all.

Reroll, didn't try it at all yet.

Fixing the tests.

* Fix operations to a) bubble up the cache metadata ( this might need to be fixed for #type links too, we just don't have tests for that) and b) skip inaccessible links completely. The code that was committed to HEAD renders empty list elements, and the RowRenderCacheTest explicitly checks for an empty string.
* Fixed the add menu link operation access
* Fixed test exceptions that are looking for the exact operation arrays
* the title view, despite having a link does not currently check access (as it is a path configuration), so we special case that behavior in the test. This is not the issue to fix that and we're not even sure we want it to be fixed.

Also discussed this with @plach, @dawehner and @xjm in regards to being critical or not and we think it is not, because there is no real security issue, you just might see some links you then don't have access to or you are missing links that you would have access to. So we can remove it from the must list of the cache meta issue.

(This also means I was working on a major issue, but figuring out if it is critical or not was critical :p)

Also discussed this with @plach, @dawehner and @xjm in regards to being critical or not and we think it is not, because there is no real security issue, you just might see some links you then don't have access to or you are missing links that you would have access to. So we can remove it from the must list of the cache meta issue.

(This also means I was working on a major issue, but figuring out if it is critical or not was critical :p)

berdir++

Also updated the meta: #2429287-114: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance.

NW per #31.

I think we may need to do something like this, because EntityInterface::access() returns AccessResultInterface, which does not extend CacheableDependencyInterface. This also means we're mixing up access and cacheability metadata, which is a smell. Instead, use a cache key for links which contains CacheableDependencyInterface. We can easily generate those from the AccessResult instances which are returned by Entity::access(), and it keeps the API more self-descriptive.

This fixes some test failures, and removes what I believe are redundant cacheability metadata merges.

@Wim Leers: This results in per-entity cache tags being added to page cache items. On a list page with 50 entities, that means 50 additional cache tags, and the user context that are added. Is this what we what, or should we lazy-build the operations to keep accurate page output, but prevent ridiculously granular cache items?

Patch didn't apply anymore, attached one does.

Patch still applies. All the tests are very much broken though.

#2530634: Value of last_render_text leaks into the next Dropbutton also sounds related -- the resulting behavior at seems similar, at least, though the title sounds like a different bug.

@tim.plunkett, @dawehner, @alexpott, and I discussed this issue at DrupalCon New Orleans and agreed that it was major. It results in cached administrative views not being properly updated when access changes, which results in both missing links and inaccessible stale links. It is especially confusing because it misleads the user about access to the entities and can result in people doing inadvisable or insecure things to work around the problem.

It is not critical because there is no risk of information disclosure, just missing links or links to 403s (see #29 for earlier discussion of that).

Let's try and tackle this at DS2019!

Just a quick review of the last patch to point out some issues

1. +++ b/core/includes/theme.inc
   @@ -648,12 +651,19 @@ function template_preprocess_links(&$variables) {
   +        continue;
   

   We'll need to add cacheability from a access denied here too. Maybe it's best to add it into $variables['links']?

2. +++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityListBuilder.php
   @@ -37,12 +38,15 @@ public function getDefaultOperations(EntityInterface $entity) {
   +      $update_cacheability = CacheableMetadata::createFromObject($update_access);
   

   We have to return this cacheability metadata even when access is denied and bubble it up in template_preprocess_links.

3. +++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
   @@ -82,6 +82,14 @@ public function access(EntityInterface $entity, $operation, $langcode = Language
   +    $return = AccessResult::neutral()->orIf($return);
   +    $return->cachePerUser();
   +    $return->addCacheableDependency($entity);
   

   Not sure what the intention here was but we definitely don't want the user context added to every access call.

I would also suggest we scope this issue to entity operations generated from EntityListBuilder and split the other builder classes out?

Something like this would work although using array keys like this is pretty ugly IMO, let's see what breaks.

We need to add back some of the logic from #45 in template_preprocess_links for lists that aren't using views. This is currently incomplete as it causes empty <li> elements to be rendered.

Woops, missed the use statements!

Having a bad day with this 😅 errors were from undefined index notices.

Oh boy...

Simple rerolls for 9.1 and 9.0

Trying to fix the test case.

Fixed all test case #70 patch, Kindly check.

+++ b/core/modules/views/tests/src/Unit/Plugin/views/field/EntityOperationsUnitTest.php
@@ -130,7 +130,7 @@ public function testRenderWithDestination() {
-    $this->assertSame($expected_build, $build);
+    $this->assertNotSame($expected_build, $build);

@@ -171,7 +171,7 @@ public function testRenderWithoutDestination() {
-    $this->assertSame($expected_build, $build);
+    $this->assertNotSame($expected_build, $build);

That's one way to fix a test haha. I think we need to figure out why these are different and fix $expected_build.

The test operation has no cache dependencies, so all we need to add to the expected build in order to pass the test is a "permanently cacheable" designation; ie (new CacheableMetadata())->applyTo($expectedBuild);

But maybe we want to expand test coverage by adding (and then expecting) an actual cache dependency to test that it is merged into the render array correctly.

This patch adds a real cache tag to the operation and checks that it passes through into the final render array.

LGTM

Great find. I think since this is resolving a major cache invalidation/access bug with no other disruptive changes, it's potentially backportable to 8.9.x.

In addition to the required unit test issues, I'd suggest a functional test for this (maybe a JS test in this case), since the bug is at a functional level. It should theoretically be straightforward to write a test that fails on HEAD and passes when combined with the fix. I'd also want the test to make assertions about the expected access before and after cache invalidation to prove that we're not introducing an access bypass by fixing this. Thoughts?

+1 for Functional test coverage

1. +++ b/core/includes/theme.inc
   @@ -725,6 +727,12 @@ function template_preprocess_links(&$variables) {
          ];
   +      if (isset($link['access'])) {
   +        $link_element['#access'] = $link['access'] instanceof AccessResultInterface ? $link['access']->isAllowed() : $link['access'];
   +      }
   

   if access is an AccessResultObject, we could also consider its cacheability metadata directly, that would make the implementations a lot simpler?

2. +++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
   @@ -128,18 +129,24 @@ public function getOperations(EntityInterface $entity) {
            'weight' => 10,
            'url' => $this->ensureDestination($entity->toUrl('edit-form')),
   +        'access' => $update_access->isAllowed(),
   +        'cache' => CacheableMetadata::createFromObject($update_access),
   
   +++ b/core/modules/config/tests/src/Functional/ConfigEntityListTest.php
   @@ -137,11 +144,16 @@ public function testList() {
   +        'access' => $update_access->isAllowed(),
   +        'cache' => CacheableMetadata::createFromObject($update_access),
   +
          ],
          'delete' => [
            'title' => t('Delete'),
            'weight' => 100,
            'url' => $entity->toUrl('delete-form')->setOption('query', $this->getRedirectDestination()->getAsArray()),
   +        'access' => $delete_access->isAllowed(),
   +        'cache' => CacheableMetadata::createFromObject($delete_access),
          ],
        ];
    
   diff --git a/core/modules/views/src/Plugin/views/field/EntityOperations.php b/core/modules/views/src/Plugin/views/field/EntityOperations.php
   

   Concerned that changing this could break things. In multiple ways.

   One one side, we have the problem that \Drupal\Core\Entity\EntityListBuilder::getOperations() is a public API, it may or may not be used together with #links. Now we expect it to be used with something that respects the new access key. I didn't find anything, so this is possibly unlikely.

   On the other side, we have the problem that there are a lot of custom operations, and as a result of this, we implicitly make them cacheable and might introduce bugs.

   This patch only updates the default method, but there are tons of subclasses implementing this. Just looking at a few:

   \Drupal\filter\FilterFormatListBuilder::getDefaultOperations() checks for the default format, separate config entity.

   \Drupal\Core\Config\Entity\ConfigEntityListBuilder::getDefaultOperations() checks the entity status, so would need top update if the entity changes.

   These and most are config entities, so they are unlikely to be cached, but there's also for example \Drupal\webform\WebformSubmissionListBuilder::getDefaultOperations in contrib, with lots of dynamic conditions.

   This is annoying, I know :)

   One option might be to treat a non-existing cache key as uncacheable, to keep the existing behavior. So every operation needs to opt-in to being cacheable. They could return an empty object to indicate that they are safe, like \Drupal\Core\Cache\Context\CacheContextInterface::getCacheableMetadata.

@Berdir yeah it is annoying, back in #59 I suggested stripping this issue back to just the default class to keep the patch size manageable the idea was to post follow ups to address the other subclasses. Fixing caching bugs always has the opportunity to break things but I'm not really sure what the best solution is here.

I like your idea of returning uncacheable, I'm just not entirely sure what that would look like - maybe I need more coffee first :P

I've made some alterations:
1. Made operations caching per user by default.
2. Simplified the data by adding only 'access' objects to operations that include the caching information in themselves.
3. Made patches for 8.9 and 9.x.x

Here are fixes for the patches. Can't run the full tests locally so fingers crossed this time.

Mistake in the 9.x.x version of the patch.

Updates of the tests.

@SimeonKesmev The naming convention of your patch file is incorrect, Commonly we use like issue number and then comment number.
EX: [issue-number]-[comment-number].patch

Also required interdiff, It will help to understand the changes you have done. Create Interdiff

Hi @vsujeetkumar, here's the proper naming and the interdiff.

Thanks for the updates. Patches still both apply cleanly.

I've reviewed the code but this is quite complex so I'll defer to others with more insight there. I didn't notice any nitpicks at least :)

Adding some more tags because this issue really needs more tags </silliness>.

I skimmed the comments and noticed that a Functional test was asked for in #83, setting to NW for that.

The Issue Summary should explain the proposed resolution and list the remaining tasks.

And adding some more tags!

Reroll against 9.3.x is done
For 8.9.x reroll was not needed.

I have closed #3012036: Views entity operations field does not contain required cache metadata and #2653690: Operations links field in Views fails between users with "edit own" user permissions as duplicates.

I was testing #2653690: Operations links field in Views fails between users with "edit own" user permissions as part of bug smash triage and was able to confirm the problem as stated in the IS over there is reproducible. I then found this issue and applied the patch and retested. And it fixed the problem. The views for the two users show the operation link on the correct content.

The proposed resolution section of the IS needs to be updated.

Moving to an MR, rebased against 10.1.x. First pushed a failing functional test and will follow with the rest of the patch. Also updated the IS.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Missed changing the version.

Hiding files as fix has been moved to MR 3556
Moving to NW as there appears to be 1 failure in it though.

Did not review yet

We already have cache metadata for the operations indirectly through their URLs, but most list builders [and the Views Operations field] simply throw it away after evaluating the access condition (at operations build time), so the context of when an operation could be valid or not is lost.

I think this is what the failing test is actually hinting about, a responsibility creep. To fix the failing test I would have assumed the access control handler for the "disable" operation was not returning the proper [user] context. However, adding it there has no effect at all on whether the test passes or fails since that data is thrown away by the list builder when it check if access is currently allowed.
(We try to avoid early rendering, maybe we should also try to avoid early access evaluation in similar situations?)

I ran into this when debugging specifically why our cache contexts added to custom entity access control handlers seemed to have odd effects despite the value of the context(s) we included was different for different users, even after applying the latest patch - because contrib's entity module's list builder has the same flaw as core's list builders currently do.

If the list builders (and Views' Operations field) always added the operations instead of evaluating access at the point of building the operation links, and the links preprocess template hook performs the actual access evaluation (storing the result in $link['#access'] and merging in the cache metadatada into $link['#cache']), we'd be all good.

This would keep access control handlers as the ones responsible for adding the correct access related cache metadata to their responses instead of also having list builders juggle it around, and relieve them of even having to do the operations access checks they do now - they would not have to care about it all about which operations are valid or not.

For now I've had to resort to always include a custom cache context (which varies with anything that could affect access for the different groups of users we have) in the links preprocess hook, otherwise the operations access in the lists/views we have would not get rebuilt at all. If it worked as described above (and entity module always included all of its operations) I would not have had to do anything in our site specific code because the cache metadata from the access handler would have been propagated there instead.

I'll take a stab at refactoring the branch to show what I mean.

Now that all tests finally pass, I should write some notes on the changes outside the implementation of #108 itself.

There were at least two major issues discovered while working on this which could potentially be extracted and handled on their own.
I tried searching the issue queue for existing patches and came up blank, but maybe someone knows if they have already been covered elsewhere.

The renderer throws away cache metadata from access results if they are not allowed. This, obviously, prevents something like #108 from working; we need that metadata to be bubbled the same was as if there was a cache hit/miss when access is granted. Without this the new ::testEntityOperationsCacheability() fails because the user cache context is dropped.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -233,6 +233,14 @@ protected function doRender(&$elements, $is_root_call = FALSE) {
       if ($elements['#access'] instanceof AccessResultInterface) {
         $this->addCacheableDependency($elements, $elements['#access']);
         if (!$elements['#access']->isAllowed()) {
+          // Abort, but bubble new cache metadata from the access result.
+          $context = $this->getCurrentRenderContext();
+          if (!isset($context)) {
+            throw new \LogicException("Render context is empty, because render() was called outside of a renderRoot() or renderPlain() call. Use renderPlain()/renderRoot() or #lazy_builder/#pre_render instead.");
+          }
+          $context->push(new BubbleableMetadata());
+          $context->update($elements);
+          $context->bubble();
           return '';
         }
       }

(Added null to the annotated return types of ::getCurrentRenderContext() as that was missing.)

The content translation manager sometimes looks for existing translations on the wrong revision.
This was exposed by ContentTranslationRevisionTranslationDeletionTest::testOverview() failing because the 'Add' operation was now missing for Italian on line 142. They still had access to the translate form if the current interface language was either Italian or French, but not in English. While the Add operation [correctly?] points to the Italian version of the translation form it being added depended on an access check performed on the "active" revision - which already has an Italian translation - so the link no longer show up because the URL is inaccessible.

The test performs these steps:
(This is iteration 2 of the test, with the editor role. Iteration 1 passes because access checks are bypassed.)

1. Create an English published node "Test 2.1 EN"
   Verify translations can be added.
2. Add an Italian draft "Test 2.2 IT".
   Verify the draft can not be deleted because it's unpublished.
3. Publish the Italian translation "Test 2.3 IT"
   Verify it can be deleted.
4. Create an English draft "Test 2.4 EN".
   Verify Italian translation still exists.
5. Delete the Italian translation.
   Verify the 'Add' operation for Italian reappears. This failed
6. Publish the English draft "Test 2.6 EN".
   Verify the Italian translation does not reappear.
7. Add an Italian published translation "Test 2.t IT".
   Verify it can be deleted.
8. Create an Italian draft "Test 2.8 IT".
   Verify it can not be deleted.
9. Delete the Italian draft.
   Verify it can be deleted again.
10. Delete the Italian translation.
    Verify it can be added again.

When the Italian translation is deleted in step 5 the English draft "Test 2.4 EN" is the latest revision and no longer has a translation. The access manager gets its argument from the route match for the translation route, which has load_latest_revision = TRUE, but it loads the latest "active" revision - which the entity repository says is the last one which was translation affected.
The last English revision which was translation affected (titled "Test 2.1 EN") still had "Test 2.3 IT" as a translation, as the deletion created a new revision without any translations affected.
ContentTranslationController::add() deals with this by explicitly loading the latest revision, so we can do the same thing in the translation access control manager and it correctly sees that there is no Italian translation there and allows the 'Add' operation.

The controller does however cause a similar issue elsewhere by adding the translation to the entity it loads (after the access control manager has allowed the route). If you have the language switcher block enabled the links there are now access checked via the same manager, and it now sees the newly added translation (which isn't considered "new" since an Italian translation did exist earlier) and prevents the links from showing.
Adding a clone to the controller prevents polluting the entity instance in the context, and the subsequent access checks then see the true "current" state of the entity and lets the language switcher links show.

Other changes made to make tests pass include adding _access: 'TRUE' to the <current> route, which caused a problem in CommentNewIndicatorTest<code> because <code>CommentLinkBuilder uses <current> for the "comment-new-comments" hidden jump link and renders it as a link list.
I can't think of any reason why this would not be safe off the top of my head, but there's also no other test which directly depends on it it being either allowed or denied.

A few places in FunctionsTest had similar issues with the access checks but some routes could be switched to existing ones with correct access requirements or replaced with a new one.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Splitting those out sounds good and nice one tracking them down!

Thanks, working on that now.

Do you know what's up with the bot? This is my first time I see it so trying to figure out what it's trying to tell me.
The patch applies fine against 11.1.x which it wants to merge into, but I guess it's testing against 11.x?

This now depends on #3374234: Content translation access manager may use the wrong revision and #3374253: The renderer throws away cache metadata from access result if it is not allowed.

I've rebased the MR onto 11.x, hopefully it went well 🤞

Just tested it to be working fine with Drupal 10.0.9.

After leaving comment #117, I ran into a new issue. There seems to be a permission anomaly for certain operations. I am marking this as needs work.

For example: With the changes proposed in the merge request at this point, I stop seeing the "Masquerade" operation for user entities even though I'm logged in as User 1. As soon as a I remove the patch, I start seeing it again. Additionally, I implemented a hook_entity_operation() in a custom module and the patch makes that custom operation disappear as well. Removing the patch brings it back.

Yes, this will need some more rebase work once the issues mentioned in #115 are done.

The changes to the Renderer have been committed to the 11.x branch, but not the 10.x or 10.1.x branches (now committed to 10.1), so if you are using D10 you are likely missing the corresponding change from that issue - which is likely to cause problems like those you describe as cache metadata is lost in some situations.

It'd be good to get some steps to reproduce #118 or the sample code for the custom operation. We are running this patch with many custom operations and all work as intended.

For example: With the changes proposed in the merge request at this point, I stop seeing the "Masquerade" operation for user entities even though I'm logged in as User 1. As soon as a I remove the patch, I start seeing it again.

I just debugged this and it's in fact correct. The masquerade operation's URL route is entity.user.masquerade

That route has the _csrf_token requirement. That gets removed by the array_diff in AccessManager::check because there is no request, which the csrf_token requirement needs.

I created a 10.1.x-compatible patch from the merge request changes if anyone is in need.

Hiding patches to avoid confusion.

Appears to have gone through a few rounds of reviews

All threads appear to have been addressed in the MR. Took a look at the MR and nothing stands out. But seems like a large change so will wait to see if anyone else chimes in.

#117 set this to reviewed and tested by the community.
#118 re-set this to needs work with a vague bug report.
#121 spoke to the bug report and reported it as correct (meaning that 'works as expected' I believe).

Therefore setting this back to 'Reviewed and tested by the community'.

I have done some testing and can confirm that MR3556 as of 14 Jan 2024 resolves the entity operations problems reported on this issue, and also the username issues reported on Incorrect user linked on content page when switching users.

Attaching also a patch for Drupal 10.2.x

Checked the latest state of MR, also RTBC++.

Left some minor comments on the MR

#85 asks the same question about sub-classes in contrib/custom code - I think we need an CR for that.

Did we get to the bottom of #121?