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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

e0ipso created an issue. See original summary.

dawehner’s picture

Issue tags: +Needs tests
e0ipso’s picture

This is the first I found. This still needs all the work to be done, I just wanted to have this somewhere.

dawehner’s picture

I'm wondering what does core REST do there?

e0ipso’s picture

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

dawehner’s picture

Status: Active » Needs review
FileSize
3.5 KB
2.99 KB

I just continued a bit.

e0ipso’s picture

  1. +++ b/src/Normalizer/EntityNormalizer.php
    @@ -178,13 +180,20 @@ class EntityNormalizer extends NormalizerBase implements DenormalizerInterface,
    +      return (new NullFieldNormalizerValue())->addCacheableDependency($access);
    

    You're gonna make me want to write an article about the Null object pattern! Article bait.

    :+1:

  2. +++ b/src/Normalizer/Value/NullFieldNormalizerValue.php
    @@ -0,0 +1,36 @@
    +    return NULL;
    

    <?php
    return $this->cardinality == 1 ?
    NULL :
    [];

e0ipso’s picture

Assigned: Unassigned » e0ipso
dawehner’s picture

You're gonna make me want to write an article about the Null object pattern! Article bait.

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

e0ipso’s picture

I'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. The DocumentRootNormalizer 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.

e0ipso’s picture

Title: [TASK] Make sure all cacheability metadata is added » [FEATURE] Make sure all cacheability metadata is added
Category: Task » Feature request

Status: Needs review » Needs work

The last submitted patch, 10: 2759951--cacheability-metadata--10.patch, failed testing.

The last submitted patch, 10: 2759951--cacheability-metadata--10.patch, failed testing.

e0ipso’s picture

e0ipso’s picture

Status: Needs work » Needs review
e0ipso’s picture

Old tests are green. We need new tests before we merge this.

dawehner’s picture

I'm working on some new tests, if you don't mind.

e0ipso’s picture

e0ipso’s picture

Status: Needs review » Fixed

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

  • e0ipso committed c7c0f6f on 8.x-1.x
    Issue #2759951 by e0ipso, dawehner: [FEATURE] Make sure all cacheability...
dawehner’s picture

Nice work! It looks really rock solid.

Well, we will fix things over time in case there are things.

e0ipso’s picture

Feature video https://youtu.be/-r60edg1Tfg. In case anyone is interested.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.