- πΊπΈUnited States smustgrave
This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request β as a guide.
As a bug it will need a test case.
Did not review.
- ππΊHungary mxr576 Hungary
- if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated() && $account->id() == $uid) { - return AccessResult::allowed()->cachePerPermissions()->cachePerUser()->addCacheableDependency($node); + if ($operation === 'view' && !$status && $account->hasPermission('view own unpublished content') && $account->isAuthenticated()) { + return AccessResult::allowedIf($account->id() == $uid)->cachePerUser()->addCacheableDependency($node); }
Did some testing today and unfortunately this is not that simple. By removing
$account->id() == $uid
we eliminate the chance for granting access to an unpublished node created by somebody else. (Got to this conclusion when I worked on the fix for #3449181)So maybe the only way is what @wim.leers suggested in #2982770-5: NodeAccessControlHandler::checkAccess() does not add a necessary cache context β .
- ππΊHungary mxr576 Hungary
I have spent several hours trying to reproduce this issue using only the available testing tools, aiming for a more minimalist approach than described here.
I attempted to follow the reproduction steps from #2982770-5: NodeAccessControlHandler::checkAccess() does not add a necessary cache context β , but I discovered significant differences between how Views and JSONAPI handle entity access checks. JSONAPI performs entity access checks, including the "view own unpublished content" check, whereas Views relies on hook_node_grants() via query alteration out of the box. (See my failed attempts in the attached unable_to_reproduce.patch).
As a result, I had to create a custom test controller, which I will include in my next merge request (MR).
- Merge request !8198Access cacheability is not correct when "view own unpublished content" is in use β (Open) created by mxr576
- ππΊHungary mxr576 Hungary
Closed π NodeAccessControlHandler::checkAccess() does not add a necessary cache context Needs work as duplicate of this one. Also transferred the latest proposed fix from there.
Issue credits should be transferred.
- ππΊHungary mxr576 Hungary
Fixing π Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review first is a blocker of this issue.
- ππΊHungary mxr576 Hungary
Fixing this issue probably also resolves this one.
- Status changed to Needs review
5 months ago 12:58pm 10 June 2024 - ππΊHungary mxr576 Hungary
hm, this should have been in "needs review" for a while :)
- Status changed to Needs work
5 months ago 4:08pm 11 June 2024 The Needs Review Queue Bot β tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide β to find step-by-step guides for working with issues.
- Status changed to Needs review
5 months ago 7:38pm 11 June 2024 - Status changed to Needs work
5 months ago 8:18am 14 June 2024 - π³π±Netherlands bbrala Netherlands
Did a review. Caching is hard and hurts my head. Left some comments for clarification and some small changes.
In general this makes a lot of sense and a i see no glaring issues.
- Status changed to Postponed
5 months ago 10:42am 14 June 2024 - π³π±Netherlands bbrala Netherlands
Ok, i reviewed this and would RTBC. Although caching is scary we have pretty good proof on the effects. Code looks good, the ->id() issue is fine like this.
Dont think we need a CR here.
Unfortunatly this is postponed on π Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review so we need to postpone on that.
- ππΊHungary mxr576 Hungary
Thanks for the review and the tentative RTBC. @bbrala.
I have synced with @kristiaanvandeneynde on Slack and described the current plan to get the blocker in sooner than later :) See: #2973356-32: Cacheability information from route access checker access results are ignored by dynamic_page_cache β :fingers-crossed:
- π³πΏNew Zealand quietone
This was a bugsmash target today. This looks well in hand. I have added the 'blocker' tag to π Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review because of comment #31. It is good to see the the issue this is postponed on is in the remaining tasks.
- ππΊHungary mxr576 Hungary
Rebased and also cherry-picked the latest MR state from π Cacheability information from route access checker access results are ignored by dynamic_page_cache Needs review .
- ππΊHungary mxr576 Hungary
Added the latest version of https://git.drupalcode.org/project/drupal/-/merge_requests/8221 to the top of the commit change.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di... - Assigned to mxr576
- ππΊHungary mxr576 Hungary
No blockers anymore! \o/ just need to rebase this branch tomorrow.
- Issue was unassigned.
- Status changed to Needs review
2 months ago 8:28pm 9 September 2024 - ππΊHungary mxr576 Hungary
Opened β¨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review for the improvements that I made on
\Drupal\Tests\jsonapi\Functional\ResourceTestBase1
to ensure the test suite is less fragile and more intuitive to use in context of Dynamic Page Cache assertions.I am going to reorganize existing changes in MR#8198 to separate changes that belongs to here from those that belongs to 3473374.
- ππΊHungary mxr576 Hungary
This should be it... now we have 3 meaningful commits in the branch.
https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...I may saw some other areas in ResourceTestBase still with hardcoded Dynamic Cache Header values... checking those.
- ππΊHungary mxr576 Hungary
Yep, those three could be still automated. https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?di...
- Status changed to Needs work
2 months ago 7:36am 13 September 2024 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Cacheable metadata, most notably cache contexts, is not being carried over to the next checks in the access control handler. This can lead to cache misses or worst case information disclosure.
- Status changed to Needs review
2 months ago 11:38am 13 September 2024 - Status changed to Needs work
2 months ago 12:05pm 13 September 2024 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Until π Introduce AccessResult::andAllowedIf(), orAllowedIf(), andForbiddenIf() and orForbiddenIf() to eventually replace orIf() and andIf() Needs review lands, I wouldn't use andIf() like you do. It means we are running code when we could have already exited early.
Gradually build a $cacheable_metadata object and add that to $result whenever you want to return it. That still allows for early returns, but does not lose important cacheable metadata.
E.g.:
$cacheable_metadata->addCacheContexts(['foo']); if (is_foo()) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } $cacheable_metadata->addCacheContexts(['bar']); if (is_bar()) { return AccessResult::allowed()->addCacheableDependency($cacheable_metadata); } // etc.
- ππΊHungary mxr576 Hungary
Do you have concrete suggestions for code adjustments? I know those new methods will bring improvements but with the current tooling I tried hard in the latest commits to return as early as possible, but also keeping collected cacheability instead of ignoring them. But the chance is there that something can be still further improved
- ππΊHungary mxr576 Hungary
user.roles:authenticated
context bugs me since the latest change... maybe I can delay that and first check instead whether the user has the given permission, if yes, I can double check whether it is anonymous or not and only add that context, otherwise just vary per permissions. - Status changed to Needs review
2 months ago 5:02pm 13 September 2024 - ππΊHungary mxr576 Hungary
β¨ Improve Dynamic Page Cache header assertions in JSON:API tests Needs review got an RTBC \o/
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
I think I noticed something off about the MR, will comment in detail during contrib time at DrupalCon
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
The cache per user needs to be up higher where I left the remark. Changes are looking really good otherwise! Will do a full review once you've updated it.
- ππΊHungary mxr576 Hungary
The issue here is that we're dealing with a passed in account rather than the current user so any user.foo cache context is technically not correct here.
YES...YES...YES!!! And I always feel bad when I have to bubble up
cache per current user
in anyways in an entity access handler method just to ensure things "do not get broken" but semantically that is completely incorrect. That said, this is what we have currently... :(Thanks for the review, to be honest, I have lost a bit now... :) Could you please commit the expected change to the MR?
And before you do that, let's remember that we try to avoid cache per user as long as we can, this is the reason why
user.roles:antonymous
oruser.roles:authenticated
(in the current version) s the primary driver to differentiate access for logged in and non-logged in users instead of returning something that varies per user. We only do this as long as we can, so when a node_access module enabled than we have adduser.node_grants:
that basically makes every result vary by user in a different way. - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Thanks for the review, to be honest, I have lost a bit now... :) Could you please commit the expected change to the MR?
It's late so could have made a mistake, but I stayed closer to the original code while making sure cacheable metadata is passed down after every check. Please review.
- ππΊHungary mxr576 Hungary
Did a check on the changes and I generally agree with those. I hope nobody is going to raise scope creep just because for consistency reasons not on the view operation was touched.
There are some tests that needs to be fixed, I am going to working on that.
- ππΊHungary mxr576 Hungary
My changes after Kristiaan's changes: https://git.drupalcode.org/project/drupal/-/merge_requests/8198/diffs?st...
- ππΊHungary mxr576 Hungary
I take the opportunity that after so many back and forth code reviews with Kristiaan, mark this as RTBC. My changes since his last changes mainly impacted failing tests after his changes, that were changed by me previously.
- ππΊHungary mxr576 Hungary
Rebased and brought in latest changes from [##3473374] which also had to be rebased, but we found other regressions in 11.x in that: https://git.drupalcode.org/project/drupal/-/merge_requests/9465#note_386330
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Left some comments on the MR
I'm wondering if the cure is worse than the symptom here
- ππΊHungary mxr576 Hungary
Thanks for the review, let me start with a the rebase since there is work to be done it seems.
β― git rebase origin/11.x Successfully rebased and updated refs/heads/3278759-access-cacheability.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
I'll discuss this with other core committers
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
I wonder if we can expand the description of the permission to convey that there is a performance impact of granting this permission. And whether we should also have a hook_requirements that shows a warning if this permission has been granted. Will report back on other committers' thoughts.
- π¨πSwitzerland berdir Switzerland
Not fully caught up with the implementation, but there are plenty of valid cases to use own permissions, it's only wrong when granted unnecessarily. But the situation around unpublished view and edit access is messy, we can't really point to a different permission that might not actually exist depending on the modules you use. So I don't think we can remove or warn about its usage.
- ππΊHungary mxr576 Hungary
So I don't think we can remove or warn about its usage.
+1
For changing the world, please join this other issue to brainstorm on how to make node grants and entity access work better together (less confusing to use): https://www.drupal.org/project/drupal/issues/3452449 β¨ Grant query level access to own unpublished nodes Active
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
I'm wondering if the cure is worse than the symptom here
The symptom is a potentially unstable or even insecure website, so the cure is definitely not worse :)
- ππΊHungary mxr576 Hungary
Thanks for the additional feedbacks, I have pushed back on some with answers and accepted one. Back to review