Cache POST/PATCH in ResourceObjectNormalizationCacher breaks MenuLinkContentTest

Problem/Motivation

Seems caching opst and patch is possible, but there is a very very weird issue where only menucontentlinktest fails. I've investigated this and have not found a solution.

Below the slack conversation. But to try and summarize:

1. Only menucontentlinktest fails when we start caching with variation cache
2. Acces check does check the right persmission
3. There seems to be a discrepancy with the resulting 'options' change that is cached, in the new situation it is not appended to the normalsation cache. This might point at something weird in that field, or just the failure for the permission. not sure.

This could use extra investigation, having normalisation cache available for posts could help loads in returning the patched resource and make things like a sync through jsonapi faster.

Steps to reproduce

Disable the following code in ResourceObjectNormalizationCacher:

    // This class used to rely on the render cache, which only caches things for
    // GET and HEAD requests, so let's keep that functionality for now while we
    // evaluate whether this behavior is desired.
    // @todo Follow up on https://www.drupal.org/project/drupal/issues/3379984.
    if (!$this->requestStack->getCurrentRequest()->isMethodCacheable()) {
      return FALSE;
    }

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Log:
Reaching out to the JSON:API maintainers according to MAINTAINERS.txt
I'd hate to be the boy who cried wolf, but the more I look into https://www.drupal.org/project/drupal/issues/3379984 📌 Refactor ResourceObjectNormalizationCacher to use VariationCache Fixed , the more uneasy I become because there seems to be something wrong with the combination of caching+access, which tends to result in security issues sometimes. I can't prove there is something wrong yet, but it seems that normalizations were only being cached for GET and HEAD (because of render cache) and swapping to VariationCache would make it cache all HTTP methods and that (again) seems to be causing an issue in the tests.
Because I can't prove there is something wrong so far, I won't be opening a security issue just yet. But I'd feel a bit more at ease if someone who actually knows that jsonapi does internally could have a look. I shamefully have to admit that I haven't used it all that much yet.
Drupal.org
Refactor ResourceObjectNormalizationCacher to use VariationCache
The issue in that @todo has landed, so it's time to stop abusing the render cache :)
Aug 8th

e0ipso
:lullabot2: 9:47 PM
Thanks for reaching out
@kristiaanvde
. Unfortunately it's been ages for me as well. Let's see what the rest think

Björn Brala (bbrala)
11:28 PM
Ahh there it is. I got this notification today but lost it in Slack. As
@e0ipso
thx for reaching out.
I'll have a closer look tomorrow and see if I can find out what does or does not make sense. There are a few assumptions and ideas, but I'd like to verify those first.
Hopefully I don't get distracted to much by colleagues.
I was already following that issue with an half eye :wink:

kristiaanvde
9:20 AM
Cool. As I said, it's starting to make my security issue senses tingle but I don't have any hard evidence. Which is why I brought it up in this manner

Björn Brala (bbrala)
9:21 AM
No worries.
9:23
First thing I was thinkings was about the fact you might start caching POST/PATCH. That kinda feeld wrong in a way. JSON:API always returns the resource you post/patch after such a request, since you are trying to adjust the resource it kinda feels like a lot of times this would be really weird to cache. This might've been the reason for not allowing those requests to be cached initially.
In a way it should be possible though since you might invalidate while saving if it has been changes and therefor get a fresh resource IF it is changed. But there might be rabbitholes there that I've not thought of.

kristiaanvde
9:44 AM
Yeah, the thing is that this is something the caching layer should not be aware of. The key difference between using render cache and variation cache is that before, by virtue of filtering HTTP methods in render cache, you were getting this behavior. If JSON:API has some HTTP methods it does not want to cache, that needs to happen on the jsonapi end.
9:45
So render cache does it right (IMO), and you just happened to benefit from that :slightly_smiling_face: Having said that, if you want to disable it and see if that keeps the tests as is, we could by adding the same lines from render cache in the normalizer cacher.

kristiaanvde
9:52 AM
Picture this: The new access policy layer will also use VariationCache to store your permissions based on cache contexts. This does not care if you are using POST, PATCH, GET or whatever because none of them influence your permissions and if even they did it would invalidate the cache so you'd get a new copy next time you ask for them.
Now render cache rightfully wants to prevent a form from being cached because the form itself is influenced by POST and getting a cache hit during a form submissions would break the entire form.
Hence my statement that it's up to the consumer of VariationCache to decide whether they want to opt out based on certain logic (be it HTTP method or else) (edited)
9:54
I don't know enough about JSON:API, but if you think it makes sense to also ban everything but GET and HEAD, then we should copy that over to the normalizer cacher and not rely on render cache taking care of that for us.

Björn Brala (bbrala)
10:08 AM
Few things.
I wonder what render cache brengservice jsonapi other than this specific logica. In a way rendercache makes a lot of sense, are rendering, but just representing the rendered resources in a bit of an unusual view.
You say post won't affect permissions? Not sure if I follow that. You probably mean that the permissions you have will not change based on the contents of the post right?
10:10
In regarsd to your suggestion. Jsonapi already does some checking on what reposnse to serve (cachable or not) based on the method. I kinda wonder is there are more places that currently just have rendercache handle that and which those would be. (not done divind yet ;))
10:10
I don't have the complete codebase memorized, it is kindaof a beast lol
10:10
(and i didnt write it)

kristiaanvde
10:10 AM
I'm not privy to how jsonapi works, but the whole idea behind making people stop using the render cache is that they aren't rendering things so that cache is not fit for caching their stuff. Think DynamicPageCache, it wasn't rendering the response using the renderer so it had no business caching it via the renderer.
Complex to explain but essentially yes, nothing in the request changes your permissions. A POST on the permissions form would, but then roles would get saved and cache tags would get invalidated, so that's safe and expected behavior
10:10
Nothing but JSON:API uses render cache anymore aside from the renderer

Björn Brala (bbrala)
10:11 AM
2. Yeah that makes sense.

kristiaanvde
10:12 AM
DPC now uses VariationCache with its own cache bin
RenderCache uses VariationCache with its own cache bin
The new permission layer I'm working on uses VariationCache with its own cache bin
The end goal is that nothing uses render cache anymore but the renderer. This way, we can tailor the render cache to renderer's needs without having to worry about breaking other stuff such as jsonapi

Björn Brala (bbrala)
10:20 AM
What have been instance where we can't do things because of jsonapi then?
10:20
What would be a usecase that won't work for jsonapis representation of data vs normal rendering?
10:21
(Sorry if I ask stupid questions :sweat_smile:)

kristiaanvde
10:21 AM
Okay so now the render cache bans everything but GET and HEAD, right? Now let's say there turns out to be something in the rendering of forms that now allows us to cache POST too. So the render cache is updated and all of a sudden jsnoapi breaks because it did not expect POST to be cached. This is because jsonapi relied too heavily on logic in places of Drupal it could not control.
10:22
If jsonapi were to use its own cache, render cache could be updated without breaking jsonapi
10:23
And there is talk of doing just that in https://www.drupal.org/node/2367555 →

Björn Brala (bbrala)
10:24 AM
Which seems to boils down to isMethodCacheable in ResourceObjectNormalizationCacher

kristiaanvde
10:25 AM
Yeah, if we move said method to ResourceObjectNormalizationCacher then we'd have safely decoupled the jsonapi cacher from the render cache
10:26
I'll try to update the patch to see if that makes everything go green again, but unless there is something specific about caching PATCH requests that I'm unaware of, the test fails still seem a bit weird
10:26
Because the tests did not fail for POST and that wasn't cached either before and now is

Björn Brala (bbrala)
10:27 AM
I only would still worry on what rendercache features jsonapi might assume. Some of the structures that jsonapi passes are quire rendercachey

kristiaanvde
10:27 AM
Unless you call the renderer, you are not using the render cache

Björn Brala (bbrala)
10:27 AM
Like this? EntityResource::executeQueryInRenderContext(...) ?

kristiaanvde
10:28 AM
Aside from the last remaning hack in jsonapi that I'm trying to remove, nothing in core calls the render cache aside from things that are pulled through the renderer service
10:28
Usually that is used to make the cacheable metadata bubble up to the page

Björn Brala (bbrala)
10:28 AM

10:28
that seems to be the reason yes

kristiaanvde
10:29 AM
Yeah the doc even says so :slightly_smiling_face:

Björn Brala (bbrala)
10:29 AM
Which links to this to be able to remove: https://www.drupal.org/project/drupal/issues/3028976 ✨ Enable an entity query's return value to carry cacheability Active

Drupal.org
Enable an entity query's return value to carry cacheability
Problem/Motivation >3 years ago, #2557815: Automatically bubble the "user.node_grants:$op" cache context in node_query_node_access_alter() → introduced automatic cacheability bubbling for node_query_node_access_alter(). But ideally, this wouldn't need to bubble (onto the currently active render context if any) at all, ideally this cacheability would just be associated with the query itself. We encountered this in the API-First initiative too, in #3005826: Follow-up for #2984964: JSON API + hook_node_grants() implementations: count queries still result in cacheability metadata leak → . Proposed resolution Remaining tasks User interface changes API changes Data model changes Release notes snippet
Jan 28th, 2019
10:30
Which has been dead for a while

kristiaanvde
10:30 AM
Yeah ideally, the renderer isn't in charge of the bubbling cacheability, but that's another quest for me to go on maybe a year or two from now :smile:

Björn Brala (bbrala)
10:30 AM
hehehe

kristiaanvde
10:31 AM
It took me I think 4 years to get the cache to be properly abstracted, not touching the cacheable metadata bubbling just yet

Björn Brala (bbrala)
10:31 AM
Hehe, i applaud you for the effort
:drupalparrot:
1

10:31
cache is great, but so hard.
10:32
I'm about to receive a visitor. bbl

kristiaanvde
10:32 AM
Okay cya

Björn Brala (bbrala)
12:05 PM
Ok my appointment is done.
12:08
So does this mean we need to find an alternatieve to the bubbling of cacheability that is handled by the renderer? This issue is why it seems to be as it is right now: https://www.drupal.org/project/jsonapi/issues/3005826 →

Drupal.org
Follow-up for #2984964: JSON API + hook_node_grants() implementations: count queries still result in cacheability metadata leak
This is related to https://www.drupal.org/node/2984964 → I found that the fix was not sufficient for responses that use count queries. The attached patch needs some additional tests to validate that the issue no longer occurs with count queries, but it's a start.
Oct 11th, 2018
12:08
Knowing how these issues are handled there might be a regressiontest for that in the suite, let me check
12:10
No regression test, but the count code path is tested if i remember correctly
12:12
yeah JsonApiFunctionalTest::testRead() // 25. Test collection count.
12:12
so that seems covered
12:15
So next up is probably, why is patch different from post.

Björn Brala (bbrala)
12:25 PM
Is this the failing test you were talking about? https://www.drupal.org/project/drupal/issues/3379984#comment-15195452 📌 Refactor ResourceObjectNormalizationCacher to use VariationCache Fixed

Drupal.org
Refactor ResourceObjectNormalizationCacher to use VariationCache
The issue in that @todo has landed, so it's time to stop abusing the render cache :)
Aug 8th
12:25
Or comment #4? https://www.drupal.org/project/drupal/issues/3379984#comment-15184347 📌 Refactor ResourceObjectNormalizationCacher to use VariationCache Fixed

Drupal.org
Refactor ResourceObjectNormalizationCacher to use VariationCache
The issue in that @todo has landed, so it's time to stop abusing the render cache :)
Aug 8th
12:27
So appearntly for the menulinkcontenttest there is some reason that the user doesn't have PATCH (which should be edit persmission) on the changed field.

Björn Brala (bbrala)
12:36 PM
In the MenuLinkkContentTest there is this property:
/**
* {@inheritdoc}
*/
protected function setUpAuthorization($method) {
$this->grantPermissionsToTestedRole(['administer menu']);
}
so that seems good, but the pattern is a little different for other resources, think next is checking how acces is done in MenuLinkCOntent
12:38
protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
switch ($operation) {
case 'view':
// There is no direct viewing of a menu link, but still for purposes of
// content_translation we need a generic way to check access.
return AccessResult::allowedIfHasPermission($account, 'administer menu');

case 'update':
if (!$account->hasPermission('administer menu')) {
return AccessResult::neutral("The 'administer menu' permission is required.")->cachePerPermissions();
}
else {
// Assume that access is allowed.
$access = AccessResult::allowed()->cachePerPermissions()->addCacheableDependency($entity);
/** @var \Drupal\menu_link_content\MenuLinkContentInterface $entity */
// If the link is routed determine whether the user has access unless
// they have the 'link to any page' permission.
if (!$account->hasPermission('link to any page') && ($url_object = $entity->getUrlObject()) && $url_object->isRouted()) {
$link_access = $this->accessManager->checkNamedRoute($url_object->getRouteName(), $url_object->getRouteParameters(), $account, TRUE);
$access = $access->andIf($link_access);
}
return $access;
}

case 'delete':
return AccessResult::allowedIfHasPermission($account, 'administer menu')
->andIf(AccessResult::allowedIf(!$entity->isNew())->addCacheableDependency($entity));

default:
return parent::checkAccess($entity, $operation, $account);
Hmm, seems there is no edit operation there.
12:38
When looking at EntityResource::createIndividual i see the following:
12:38

12:43
Could it be that the operation we check form the jsonapi side, is not implemented in the MenuLinkContentAccessControlHandler and therefor it returns the default?
Not sure if that matters though, the parent method checks if there are administer permissions.

12:43
which we should have in the test
12:47
But I think it might be related. The fact that 'edit' is an operation in the entity api that jsonapi defaults to also for 'update' operations (no idea what the difference is there). And that the acces check handler for menulinkcontent doesnt really handle edit :x

Björn Brala (bbrala)
12:54 PM
Another hint: FieldStorageConfigAccessControlHandlerTest does not test 'edit'
public function testAccess() {
$this->assertAllowOperations([], $this->anon);
$this->assertAllowOperations(['view', 'update', 'delete'], $this->member);
Also FieldConfigAccessControlHandlerTest doesnt.
And FieldAccessTest (both version), only test view
12:55
(sorry for the wall of text
@kristiaanvde
, i'm just documenting my exploration).
12:56
@wimleers (he/him)
I think you could probably come to a conclusion based on my little research path lol :wink:
12:56
(pray he sees my popup ;p)

Björn Brala (bbrala)
1:03 PM
Bottom line, we might be able to enable post/patch caching if we have 'edit' added in the MenuLinkContentAccessControlHandler next to 'update'.
1:05
Or check for update and edit permissions in patch/post and see if we net positive? (edited)

Björn Brala (bbrala)
2:13 PM
Well, unfortunately not the case it seems.

kristiaanvde
2:21 PM
Hmm, your research seems like a good start to figuring this out. In the meantime, I've posted a new patch to https://www.drupal.org/project/drupal/issues/3379984 📌 Refactor ResourceObjectNormalizationCacher to use VariationCache Fixed that goes green because it keeps the render cache's check for GET/HEAD methods. I've copied that over into the normalizer cacher and now every test goes green.
So what I did is I added a todo to the patch saying this might need a follow-up. Which is where your research and my finding that POST and PATCH are handled differently might come in handy. I still think it's "weird" that the test failed the way it failed in comments 4 and 8, but for the time being we already have a committable patch. It just kicks the can further down the road when it comes to what jsonapi will and won't cache. If you ever want to cache more than GET/HEAD, this issue is your starting point. (edited)
Drupal.org
Refactor ResourceObjectNormalizationCacher to use VariationCache
The issue in that @todo has landed, so it's time to stop abusing the render cache :)
Aug 8th

Björn Brala (bbrala)
2:23 PM
The permission check is just using administer menu.
2:23
I did just all cache that was written bij normalisation in that test before and after the change
2:23
This seems weird:
2:23

2:24
Although the fact ihe fragment is not changed might just be cause of the error. Let me double check i used a green test for the camparison of the new situation :x

kristiaanvde
2:24 PM
To be fair, every test aside from MenuLinkContent went green, so that (IMO) proves that JSON:API is ready to have its cacher cache all HTTP methods and smart enough to know when it needs to invalidate that. So in that aspect, it's quite sad my patch has to carry over the render cache method check. But at least it's narrow in scope now and does what it promises (swapping out the cache, no other changes) and you can decide whether or not to open up the cache to more methods in a follow-up

Björn Brala (bbrala)
2:30 PM
I think the menu content thing is weird.
2:30
It shouldn't fail.
2:30
This same pattern is used on loads of entities. ;(
2:31
Also checked if it was the type of link that was modified, but its an external link so no extra access checks.
2:32
(just documenting a little more so i can save it somewhere)
2:32
Also checked if the right permission was asked, no problem.

kristiaanvde
2:32 PM
Yeah, so something is definitely wrong there. But for now I'll avoid making it fail by copying over the render cache logic. Then we can file a follow-up and dig into what's wrong with menu links. The only thing I'm still not sure is if that would be a security risk or not, I am too much of a jsonapi noob to understand that much

Björn Brala (bbrala)
2:32 PM
Checked if the pattern for using changes was weird, nope.
2:33
Well, less permissions normally is no risk. xD
2:33
As you said just now, the fact this is only menu content points at something weird in that part of core.