Problem/Motivation
The node access grant system does not add a cacheable dependency on nodes in all circumstances. It's important that this dependency be added because variations to the node can most certainly have an affect on the node access query logic.
Proposed resolution
Add the missing cache dependency!
Original report
A content type has an entity reference field.
A node "foo" of that content type references an unpublished node "bar".
Viewing the node "foo" as an anonymous user will create caches.
When publishing the node "bar" it is expected to become visible on the page "foo" but that page won't show the referenced node "bar" that is now published.
EntityReferenceEntityFormatter::viewElements only adds accessible nodes to the render array (not the unpublished or inaccessible ones) but a node that is referenced might be inaccessible at first and become accessible later.
Comment | File | Size | Author |
---|---|---|---|
#34 | 2946073-34.patch | 4.42 KB | claudiu.cristea |
#24 | 2946073-24-test-only.patch | 2.02 KB | claudiu.cristea |
Comments
Comment #2
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #4
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #5
BerdirThe formatter adds cacheability metadata to the render array. The access implementation is responsible for adding the entity as a cacheable dependency to the access result if changing it can result in a change of the access result.
Node is doing that, are you sure you really had this problem with nodes and not with another entity type?
Comment #6
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedHi Berdir, thanks for the follow up.
I'm quite sure the problem occurs with nodes. It only works when I explicitly add the tags in viewElements().
I assume you refer to the code in EntityReferenceFormatterBase::getEntitiesToView:
And in EntityReferenceFormatterBase::view:
I find it peculiar that the EntityReferenceLabelFormatter explicitly adds the tags:
But only for the accessible (published) terms:
While I'd expect the tags of the inaccessible entities to be added to the field ($elements array) itself.
These are the cache tags returned by ContentEntityBase::access():
I don't see anything here that could invalidate the entity reference field when a referenced node gets published?
Comment #7
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #8
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #9
BerdirI just recently wrote tests for paragraph (library) that relies on that working: #2943601: Make Paragraphs Library items publishable.
I also tested it manually and it works fine for me on a standard D8 site with a reference field on page.
But I just noticed a problem in \Drupal\node\NodeAccessControlHandler::checkAccess(), you can see that it adds the addCacheableDependency() call but only *if* it grants access.
If access isn't allowed, it continues to the grant storage, and then in \Drupal\node\NodeGrantDatabaseStorage::access() does it correctly:
return AccessResult::allowedIf($node->isPublished())->addCacheableDependency($node);
It always adds the cacheable dependency, but only if you are not using node grants. So my wild guess would be that you are using access grants?
So there is a problem, but it's a node specific problem of not always adding the cacheable dependency on the node. Not a problem with entity reference formatters or even the cache system. Whether or not the cache tag needs to be added depends on the access logic. E.g. for entity types without a status, saving them can't change whether you have access or not, so we don't want to add unnecessary cache tags.
Comment #10
timmillwoodComment #11
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedHi @berdir, you're right we have the view_unpublished module enabled:
I added a patch that should fix it in the node system or do you consider it the responsability of view_unpublished to add the cacheable metadata?
Triggering the testbot. If we want to test this in the node system I assume we can do so in NodeAccessAutoBubblingTest::testNodeAccessCacheabilitySafeguard?
Comment #12
BerdirYes, I agree it should be fixed in the node module. I'd try to do it in NodeAccessControlHandler, possibly by merging multiple access results together, like I did in the referenced paragraphs issue.
I'll also ping Wim about this.
Comment #13
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedAfter some further debugging I noticed that the cacheMaxAge is always -1 for these nodes so my last patch will completely disable caching.
I have to admit I'm digging deeper here than usual but it's been very educational :-)
I had hoped to meet Wim at this event (https://www.meetup.com/DUG-BE/events/247580511/) but I see he hasn't registered (yet *wink*).
Comment #14
remkoklein CreditAttribution: remkoklein commentedWe have the same issue with view_unpublished enabled. It appears to be an issue when the hook 'hook_node_grants' is used. At that point the access function in NodeGrantDatabaseStorage doesn't add the node as cacheable dependency anymore.
I've used patch #11 and it appears to be adding the correct tags to the cache entries and it works!
Comment #16
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #17
mpp CreditAttribution: mpp as a volunteer and at AmeXio for District09 commentedComment #19
bkosborneUpdating title and summary to better reflect what's going on here. I also came across this bug when debugging cacheability issues related to the access denied exception cacheability data missing the node:[nid] cache tag. This is the cause.
Comment #20
bkosborneI don't understand the comment in #13, but this patch shouldn't cause all your nodes to become uncacheable? The patch in #13 looks correct to me. So this just needs a regression test I think.
Comment #21
bkosborneComment #23
neclimdul#11 looks functionally fine but Berdir's suggestions make sense. I'm not really clear _how_ he was suggesting it though. Maybe this?
Comment #24
claudiu.cristeaHere's a test that acts also as interdiff.
Comment #26
claudiu.cristeaFix coding standards issue.
Comment #27
claudiu.cristeaNote that
node_test
module is enabled in test because is implementinghook_node_grants()
.Comment #28
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedI also noticed the
node_test
. Thanks for clearing that out @claudiu.cristea. However, I think we should have a comment in the$modules
array since if someone comes across it and sees it as a needless dependency and removes it, the test will still be green without it but will be a false positive.Comment #29
claudiu.cristea@idimopoulos that is a good remark. In fact I want that this test fails if someone is removing that module. Better that any explanation.
Comment #30
claudiu.cristeaImproving the comment.
Comment #31
idimopoulos CreditAttribution: idimopoulos at Randstad Digital for European Commission and European Union Institutions, Agencies and Bodies commentedThe test provides good cover and the fix is proved. Moving to RTBC.
Comment #33
catchThis could use a comment to explain why we only add the cacheable dependency on the view operation and not the edit or delete operations.
Also moving the version back to 8.9.x since this could go into a patch release.
Comment #34
claudiu.cristeaDocumented.
Comment #35
idimopoulos CreditAttribution: idimopoulos for European Commission and European Union Institutions, Agencies and Bodies commentedSetting this as RTBC again then if everyone is ok.
Comment #39
catchCommitted/pushed to 9.0.x and cherry-picked back to 8.9.x, thanks!