EntityAccessControlHandler::createAccess() and EntityAccessControlHandler::access() return false positive cache hits because it ignores context

This is at least Major if not Critical

We had converged on consensus on the solution in #36. But #48 makes this point:

including the $context that's in createAccess() isn't a complete fix for this bug: A module might implement hook_entity_create_access() and return an access value that's based on something else. It would correctly add a cache tag for that dependency into the returned AccessResult, only to have that ignored by the static caching.

I think this is a valid concern.

It seems therefore that in order to fix the bug here, we should use MemoryCache.

I think I'm wrong above in #55.
I don't see a way to get information back from the cacheable metadata added by hooks about what createAccess() $context they used in their logic. Therefore we cannot handle context intelligently even with MemoryCache.
So we either have to serialize the whole context, which has performance concerns, or we just don't do static caching when there is context. Which is the approach in #36 we mostly all like.

This is valid regardless of whether we use MemoryCache or not in the future. $context could contain anything, and so we shouldn't automatically be stuffing it into MemoryCache either.

MemoryCache would solve the issue identified in #48, but it's not the right solution for this, so let's rule #48 out of scope here and move it to another issue.

The Needs Review Queue Bot → tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request → . Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

#46 is wrong to say "this affects access() as well as createAccess()." access() does not use $context. It's true that cacheability metadata of hooks and handler results in both access() and createAccess() is not considered. This is a bug, and the correct solution is a memory cache. But it's a different bug to this issue, and solving it does not necessarily solve the context bug discussed in this issue because context may contain items that are not cacheable dependencies. I created 📌 Use a memory cache in EntityAccessControlHandler Active to address this other bug.

The MR is based on #33/36, which @hctom in #37 and @claudiu.cristea in #44 supported. We cease to use the cache if a context is provided. As discussed in #36/44 this may have a performance impact on some custom code, but that's better than the present situation of a bug with possible security implications. As @hctom said in #37:

Everything is better than getting wrongly cached results that might even result in security issues!

Improving performance by caching even if context is present is a task for the follow-up: ✨ EntityAccessControlHandler::createAccess() should use whole context array when caching Active .

Left some comments on the MR.