Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
I suspect that if a privileged user A
does a request with an included entity X
, the results may get cached. When a user B
makes the same request without having access to X
they may get access to X
.
The same happens if B
has access to X
, but A
had access to more fields than B
(field access).
Proposed resolution
Spike if that is true and if so, pass the ResourceResponse
down in the $context
and use the ->addCacheabilityMetadata($field_access)
to it.
Comment | File | Size | Author |
---|---|---|---|
#18 | 2759951--interdiff--14-18.txt | 28.04 KB | e0ipso |
#18 | 2759951--cacheability-metadata--18.patch | 32.68 KB | e0ipso |
#6 | 2759951-6.patch | 3.5 KB | dawehner |
#3 | 2759951--cacheability-metadata--3.patch | 525 bytes | e0ipso |
Comments
Comment #2
dawehnerComment #3
e0ipsoThis is the first I found. This still needs all the work to be done, I just wanted to have this somewhere.
Comment #4
dawehnerI'm wondering what does core REST do there?
Comment #5
e0ipsoWe're doing the same REST core does, the problem comes with the includes. Since core is not supporting that feature we're on our own.
I believe I know what needs to happen, but I want to get familiar with the render cache internals first.
Comment #6
dawehnerI just continued a bit.
Comment #7
e0ipsoYou're gonna make me want to write an article about the Null object pattern! Article bait.
:+1:
<?php
return $this->cardinality == 1 ?
NULL :
[];
Comment #8
e0ipsoComment #9
dawehnerIf you don't write one, I would like to take it a stab. It gonna be hard to decide whether one should mention maybe monad there.
Comment #10
e0ipsoI've given a try to the issue. There is probably a better way to do this but I didn't know how to bubble the cacheability metadata. The url generator is bubbling up some metadata for the render context, but I'm not sure how to do that in a clean way.
This approach makes all the Normalizer Value classes aware of the cacheability metadata. Then the recursivity in the serializer surfaces that to the
DocumentRootNormalizer
. TheDocumentRootNormalizer
now gets the $response object's cacheability metadata and alters it to add the gathered dependencies.Maybe Wim can look at the patch and give some recommendations without having to get a lot of context in the jsonapi code.
Comment #11
e0ipsoComment #14
e0ipsoI'm sorry testbot, this should make you happy.
Comment #15
e0ipsoComment #16
e0ipsoOld tests are green. We need new tests before we merge this.
Comment #17
dawehnerI'm working on some new tests, if you don't mind.
Comment #18
e0ipsoThis is now ready for code review, @dawehner.
Comment #19
e0ipsoI'm going to self-merge this because it's blocking #2766147: [BUGFIX] Apply error format during serialization.
@dawehner please review it anyways and I'll create a follow up to address any concern you may have.
Comment #21
dawehnerNice work! It looks really rock solid.
Well, we will fix things over time in case there are things.
Comment #22
e0ipsoFeature video https://youtu.be/-r60edg1Tfg. In case anyone is interested.
Comment #24
Wim LeersThis introduced a small bug: #2926291: Fatal error on 8.5.x: RelationshipItemNormalizer implements RefinableCacheableDependencyInterface, but should not.