Views entity operations lack cacheability support, resulting in incorrect dropbuttons

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.

acbramley → closed merge request !3555

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.
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 🐛 Content translation access manager may use the wrong revision Needs work and 🐛 The renderer throws away cache metadata from access result if it is not allowed Fixed .

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, 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 on resolves the entiry 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?

I have reroll patch no 126 as per drupal version 10.3.1.

donaldp → changed the visibility of the branch 2473873-views-entity-operations to active.

donaldp → changed the visibility of the branch 2473873-views-entity-operations to active.

gaëlg → made their first commit to this issue’s fork.

I rebased the ...-10.1 branch against core 10.3.x, so that I could generate a patch against 10.3.5.
Not sure I did the right changes on core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php, I just blindly made the same number changes (if the commit added 42 to some number, I added 42 to that same number from core).

> 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.

We tried the patch and did just run into this as well.

On one hand, I think that can be improved in masquerade and is arguably even a bug. The controller still checks access, so it's not a security issue.. masquerade_target_user_access() or masquerade_switch_user_validate() really should be exposed as an access check and the route should not _only_ have the csrf access check.

But I'm also not sure that the approach here is correct. It's kind a neat to fall back to the url access, but probably also has some performance overhead, because it involves creating route matches and lookups and directly doing access checks on the entity really should be faster. And it's an implicit API change anyway because all implementations need to now always return their URL.

An alternative, more "direct" approach would be to pass around a cacheability object to getDefaultOperations (tricky to add with BC) and the hook (easy to add an extra argument) and require that implementations add their access cacheability to do that and let calls deal with that. Similar to how hook_tokens() works for example.

I have rerolled patch #133 as per Drupal version 10.4.

acbramley → changed the visibility of the branch 11.x to hidden.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.3.x to hidden.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to hidden.

acbramley → changed the visibility of the branch 11.x to active.

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to active.

acbramley → changed the visibility of the branch 11.x to hidden.

acbramley → changed the visibility of the branch 2473873-views-entity-operations to hidden.

acbramley → closed merge request !3556

acbramley → changed the visibility of the branch 2473873-views-entity-operations-10.1.x to hidden.

The previous MR was very out of date with a lot of merge conflicts and merge commits. I've squashed everything on to a new MR and hidden all the old branches. I think this is still NW but now we're up to date :D