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

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)

So MR #8157 currently implements the suggested approach from [#2982770#comment-12669550] but it only adds cache per user when there are no node_grants implementation in the system. It should be sufficient because when there are node_grants implementations then user.node_grants:view cache context gets bubbled up.

This looks very much related to 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work . Actually they looks like dupes. Should probably close that one since it's way more recent. There's a patch on that one that might be worth reviewing though.

I found some nice related issues while reporting that one. In particular, I felt like the real solution was to be found in 📌 Introduce entity permission providers Needs work .

Adding the related issue I noted in #35. The steps to reproduce on that one don't involve jsonapi which may simplify things.

@danflanagan8 Thanks for the issue reference! Actually the fix in that issue is more to the point and less invasive as it does not impact the cacheability of pages that much as the suggested fix here.

If 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work would get a test coverage and it would be proved that it also fixes this issue (I do not see atm why not) then both issues could be marked as resolved with the fix in the other issue.

Related issue you mentioned that perhaps this issue is best after all.

I checked the test failures, all functional test failures are because of widened context i think. That kinda makes sense. So 'user' instead of 'user.permission'. Those are all good.

The 2 javascript failures are a bit more cryptic. Might be a header count, which then would make sense again.

Wish every assertion had a failure text... ;(

Wish every assertion had a failure text... ;(

+1 on that, I wish all assert methods would allow adding a failure text...

Now that I have a minimalist failing test in 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work that has nothing to do with the beast-that-love-and-called-JSONAPI, I think this issue can and should be closed as duplicate and issue credits should be transferred to the other issue.

I checked the test failures, all functional test failures are because of widened context i think. That kinda makes sense. So 'user' instead of 'user.permission'. Those are all good.

I also believe that they are correct, my only question is there is a less invasive way to fix this... but maybe there is no other :S

beast-that-love-and-called-JSONAPI haha ;)