NodeAccessControlHandler::checkAccess() does not add a necessary cache context

Unpublished to verify this is not a security vulnerability. It sounds like it is one.

Bug confirmed. The root cause is that the response contains a user.permissions cache context only, not a user cache context. This is why the response does not vary correctly between users odin and jim. Fortunately, it's not a security vulnerability AFAICT. So, republishing.

But why doesn't it? JSON API does correctly call entity access. But if jim created the unpublished article, and odin is the first to access it (to prime the cache), then an unfortunate combination of circumstances occurs in \Drupal\node\NodeAccessControlHandler::checkAccess():

    // Check if authors can view their own unpublished nodes.
    if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) {
      return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($node);
    }

    // Evaluate node grants.
    return $this->grantStorage->access($node, $operation, $account);

When odin first accesses it, the if-test evaluates to FALSE, and we evaluate node grants instead to determine access. Access is then denied, which is good, but it doesn't vary by user. It only varies by user.permissions. This leads to a perfectly cacheable response, so Dynamic Page Cache will cache it and … also serve it to any other user with the same set of permissions, which includes jim!

However, when jim (the owner) first access it, the if-test evaluates to TRUE. Access is granted, which is good, and the access result varies by user, as it should! In this case however, Dynamic Page Cache won't cache the response, because it deems responses varying by user not worthy of caching, since there's potentially millions of variations to cache (when there are millions of users).

Combined, this means that the owner's response will never be cached, but any non-owner's request will be. And after a non-owner has requested this, and the response is cached, then the same cached response will be sent to the owner. Users who aren't supposed to be able to access this data are never able to, but users who are supposed to have access to the data lose access as soon as any user who isn't has requested it!

The solution is trivial:

-    return $this->grantStorage->access($node, $operation, $account);
+    return $this->grantStorage->access($node, $operation, $account)->cachePerUser();

Everything in #10 sounds like just another consequence of the root cause I explained in #5. I completely agree the consequences can be big!

#11: adding ->cachePerUser() is not the solution that is always correct. It's important to look at the access control logic. It makes sense in the case of NodeAccessControlHandler::checkAccess() because it's first checking something that is true per user. In case of the Group module, there are severe known issues with cacheability, see #2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability.

@Wim Leers Besides, thanks for the so many guidance for me in drupal.org. I learned a lot from you.

I'm glad that you're finding it helpful! ❤️👍 I also want to thank you for creating such excellent bug reports, and helping to dig deeper to find the root cause, and then fix it properly! 👏 5 JSON API issues already in the past 12 months!

First you have to give permission "Administer content" to Jim to create unpulished content.

After analysis, I can confirm that there is a problem of cache dependencies, and indeed it is necessary to add a context user but it's not the good place (cf. next patch)

Check this patch.

@ntijani Thanks for working on this! It's good to have my conclusion verified. The patch in #14 is identical to the one I posted in #6 though. It'd be great if you could work on making tests pass! :)

@Wim Leers thank you for your message.

You made the modification in the class "NodeAccessControlHandler" which is particular, I think it must be put in the method access() of the class "NodeGrantDatabaseStorage", i will try to make tests pass.

:)

Removed DevDaysLisboa to find it in the same place.

Update.

This issue seems old and abandoned but is similar to a problem I'm facing.
So admin users have permissions to view unpublished content.
If the admin user unpublished content and an unauthenticated user was first to visit the jsonapi node endpoint, then the unauthenticated user would not load content as expected.
But…
If the Admin user is the first to visit the jsonapi node endpoint after unpublishing the node, then unauthenticated users visiting same endpoint can also see the unpublished data until I manually clear cache(not expected)