Replace content moderation specific access check in JSON:API with generic revision access check

Hi @Dylan Donkersgoed! Thanks for filing this issue. And for providing a patch! This is the second issue to reference this problem (I can't find the other though). Found it! #3034532: Workbench Moderation's `access_check.latest_revision` service ID conflicts with Content Moderation in core

Unfortunately, the intention really was to prevent any other access checkers from being used. Our intention wasn't to cause fatal errors, of course! However, since we don't want any other revision access checkers to be used (frankly, we don't trust them) I don't think we want to use an interface.

The problem is that these access checkers were all written on the assumption that they would be used for admin UI routes, not entity access in JSON:API. So their logic isn't always safe to use.

More context:

Part of the problem is that the Entity API does not have any access control mechanisms that respect revisions. The only access control in Drupal core is all about access to revision pages. There's a major revision refactoring underway in #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing and access control has been brought up several times. I've asked if there is a specific issue we can track.

In the meantime, it seems we should find a workaround that doesn't brick people's sites in this issue.

OK, first copying over my comment and patch from [#12988384-16]. Also changing title to address the full scope.
Patch flying in that changes all class-typehinted methods to interface typehints (and removes obsolete use statements) (It is identical to #2 plus the other typehints).

Ran into the same problem with group module and #2906085: Content moderation integration: transition permissions and latest version access.

Setting to critical as WSOD. Setting to jsonapi due to the following.

Short version: By typehinting a class instead of an interface, jsonapi does not play by the rules.

Long version (or why this rule is important): If like suggested and implemented above, we subclass the content_moderation class, everything looks fine. But what should the next module do? The only correct way to enrich a service is to decorate it and implement the interface. The group patch referenced above does this well. And any component must accept decorated services as long as they implement the interface. That's the rules in DI land.

Patch flying in that fixes the typehint in jsonapi.

Thanks a lot for the above explanations why the class typehinting had been done in the first place. I'd suggest
a) Fix the typehints like in the attached patch
b) For now, add some hook_requirements implementation that checks the said class and warns the user if it differs. (it's the best we can do...)
c) Work upstream that we get a proper API for what we need here

Setting critical bug as this leads to WSOD.

Looked into the upstream core issue, it looks amazing and has good progress.

You wrote:

Fix the typehints like in the attached patch

But did you read

Unfortunately, the intention really was to prevent any other access checkers from being used. Our intention wasn't to cause fatal errors, of course! However, since we don't want any other revision access checkers to be used (frankly, we don't trust them) I don't think we want to use an interface.

?

#2906085: Content moderation integration: transition permissions and latest version access is only mentioned in the issue summary, also adding it to the list of related issues.

#11: Yes i read that, and i did not want to sound like i lightheadedly disregard this, the proposal is the only solutio i can see.
Did you grok the passage above about decorating services & coding against interfaces? Do you see any other way to proceed in this light?

Yes, of course I know about interfaces.

#3 and #6 explain why we explicitly and intentionally chose to not typehint to the interface, since it doesn't provide sufficient guarantees.

I do see another way: let #2906085: Content moderation integration: transition permissions and latest version access subclass the classes its decorating. That would signal that they provide a customized version of the same behavior and provide the same guarantees. And it'd automatically work.

I turned this into a concrete proposal and change request over at #2906085-48: Content moderation integration: transition permissions and latest version access.

This was never a critical bug in the JSON:API module, but rather a consequence of a design choice in another contrib module. Changing the issue metadata accordingly.

I believe we've explained in detail the rationale behind the design choice in the JSON:API module. It's a matter of security. So, closing this.

The problem is that these access checkers were all written on the assumption that they would be used for admin UI routes, not entity access in JSON:API.

subclass the classes its decorating. That would signal that they provide a customized version of the same behavior and provide the same guarantees.

Do you see this as the correct long term solution, or is there something else long term that will address the lack of consideration for jsonapi in access checkers? Something there an alternative but empty interface that access checkers could implement in order to declare their safety for use in this context.

@gabesullice explained in #6:

More context:

Part of the problem is that the Entity API does not have any access control mechanisms that respect revisions. The only access control in Drupal core is all about access to revision pages. There's a major revision refactoring underway in #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing and access control has been brought up several times. I've asked if there is a specific issue we can track.

In the meantime, it seems we should find a workaround that doesn't brick people's sites in this issue.

If i get it right, this meantime is over, and now that #2942907: Entity system does not provide an API for retrieving an entity variant that is safe for editing is in, it sheds new light on this issue..

#16: see #17 :)

#17: no, what Gabe said is blocked on #3043321: Use generic access API for node and media revision UI (which was only created a few days ago, as a subtask of #2350939: Implement a generic revision UI, which is very old).

OK so i leant that this module now lives in core (and understand the harsh insisting on this not be a bug ;-) - great work to have this in core!!).
So i guess we move this over to core and postpone on #3043321: Use generic access API for node and media revision UI.

Sure, we can do that. But that'll probably take months to complete, and will ship with Drupal 8.8 (end of 2019) at the earliest.

So if Workbench Moderation users want to move forward, the only option is to do #3034532: Workbench Moderation's `access_check.latest_revision` service ID conflicts with Content Moderation in core. And for Group users that have the #2906085: Content moderation integration: transition permissions and latest version access patch applied (it's not even committed yet!), the only option is to slightly refine the patch.

Then once #3043321: Use generic access API for node and media revision UI lands, yes, we'd love to change this typehint! 😄Believe me, we hate that we have to have it there. But it's the prudent thing to do.

Now that JSON API has been moved into core in 8.7, I have rolled this patch for core.

This patch works with me perfectly, in case if this still needed

Closed #3081831: TypeError: Argument 1 passed to Drupal\jsonapi\Access\EntityAccessChecker::setLatestRevisionCheck() must be an instance of Drupal\content_moderation\Access\LatestRevisionCheck as a duplicate.

We found another module that overrides this: Workflow Participants — issue: #3075426: Using module with Content moderation throws Revision check errors.

#3043321: Use generic access API for node and media revision UI has landed. :)

Rerolled for 9.3.x ... just in case someone needed like we did...

Attached patch against Drupal 10.1.x

Patch #36 is conflicting to apply to D10.1.x, so Updated the patch to make it compatible and Fix the errors to make it pass all test cases.

Let me get a review in :)

1. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
   @@ -67,9 +67,9 @@ class EntityAccessChecker {
   +  protected $AccessInterface = NULL;
   

   This feels weird, the variable name shouldn't start with a capital letter.

2. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
   @@ -95,14 +95,14 @@ public function __construct(ResourceTypeRepositoryInterface $resource_type_repos
   +  public function setLatestRevisionCheck(AccessInterface $latest_revision_check) {
   

   Since we are broadening the hint, i think this is not bc-breaking. So this is fine.

3. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
   @@ -196,8 +196,8 @@ protected function checkRevisionViewAccess(EntityInterface $entity, AccountInter
   +    if ($entity_type->getLinkTemplate('latest-version') && $entity->isLatestRevision() && isset($this->AccessInterface)) {
   

   I wonder, if you already have access to view all revisions, do we even need to check for the specific revision? Or could you have permissions to see all revisions and then get denied for some specific revision? Although technically possible i kinda wonder why that would be the case.

Summarizing; some small changes needed, the change seems reasonable now that the blocking issue is merged.

We are still talking about revision checks while it should be the role of the EntityAccessControlHandler to do the revision access check.

And it actually does that already, so why is there still a revision access check in the jsonAPI module ? Because the EntityAccessControlHandler can not be trusted for revisions, as said in #3, four years ago ?

I just attached a patch that removes simply the revisions check, to see what will fail.

I've created a merge request to remove the content_moderation specific access checking logic in favour of a generic 'view revision' check.

https://git.drupalcode.org/project/drupal/-/merge_requests/15455

The code was previously checking 'view all revisions' as well as the content moderation one, but it seems like 'view revision' makes more sense as we're specifically checking access for the working copy revision. Although from what I can see nearly all the core access control handlers treat both operations the same?

Only catch was that the workspace access control didn't seem to handle 'view revision' at all. Possibly a separate issue but it seemed better to fix that here rather than try and handle that edge case inside JSON:API by checking both.

I've updated the issue summary to better describe the change here and hidden old patch files.

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.