Use generic access API for node and media revision UI

#2809177: Introduce entity permission providers might be related here.

Thanks to @larowlan for linking this at #2795279-66: [PP-2] [META] Revisions support.

We would also have to consider #3023194: [PP-2] Add parallel revisioning support.

While I understand the problem, I think that introducing more operations might make the access API more complex.

\Drupal\Core\Entity\EntityAccessControlHandlerInterface::access() has the entity object as a parameter and therefore one could check its revision metadata.

What if instead of adding new operations we add new permissions? A very rough example:

// For the default revision branch check revision access only for older revisions.
if ($entity->wasDefaultRevision()) {
  if (!$account->hasPermission("view {$entity->getEntityTypeId()} {$entity->bundle()} default revision")) {
    return AccessResultForbidden::forbidden();
  }
}

if we could attempt to move Node away from using its own access checks on the revision routes and bundle that into the node access control handler and have the routes just use _entity_access. If we could make that work, then the 'generic revision access API' is just the existing access API we have now, and the handlers need to deal with revisions

Something like #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()?

I note that nodes permissions are poorly named - view all revisions, should probably mention 'node' in the name, same for delete.

There was a time when nodes were the only revision-able entity. :P

I'm leaning towards preferring Option 2, I also think that for the most part simple permissions are fine to cover the majority of use cases.

Also wondering if we should postpone this issue until we know what is happening with #2809177: Introduce entity permission providers as it will likely have an impact on the implementation here?

There is now a more agreed approach for #2925532: Provide bulk action to delete all old revisions of an entity , see comment #43 and #51 there. It would be worth identifying if there will need to be any changes to this issue to effectively accommodate the new revision action and the background clean-up, and if so determine if this issue will need to be postponed on that issue.

Formatting only

Re option 2, I think a "view all revisions" operation would make sense, to control permissions for the Revisions listing page itself. It would control access to view all revisions for a singular entity. This operation would also as a master view access, before checking individual revision view access rights via "view revision" operation.

I'm not sure if this comment fits here -- if not, lmk, I can repost elsewhere :)

I want to be able to restrict access to viewing revisions of a node to users who have permission to edit that node -- rather than the current setup, "To view a revision, you also need permission to view the content item." We really don't like that users see "View" and "Revisions" tabs when they can't even edit that node :)

Also, it's awkward that we can grant permission to revert revisions on a node that a user can edit, but they can't get to the "Revisions" tab unless we allow them to get to the "Revisions" tab for all nodes on the site.

Anyway, just sharing! Thank you!

Sorry, bit late in posting this patch.

So as I understand it, going in operation direction is more about setting a standard and precedent, rather than creating a rigid API. Setting the standard with core involves converting nodes, and tests for generic entity types if any, plus making the node revision UI and revision routes use this operation while retaining existing behaviour.

Using 'revert' op instead of 'update revision' from summary since it maps to current Node permission. I suppose the operation effectively means to change default/change latest revision/point-to-this-revision-instead, as opposed to edit/updating a revision.

Node

Per-revision operations such as delete revision or view revision fall back to OP all revisions/OP BundleName revisions permissions. In cases where per revision access is desired without granting all revisions permissions, it may be implemented later, or in contrib.

NodeRevisionAccessCheck

NodeRevisionAccessCheck was gutted to use new operations, maintaining backwards compatibility.

The ultimate desire is to move node routes from using _access_node_revision to _entity_access. However _entity_access (\Drupal\Core\Entity\EntityAccessCheck::access) requires the entity to be upcasted, which they are not currently. Nodes routes are due to be upcasted in #2730631: Upcast node and node_revision parameters of node revision routes. We can add a follow up to this issue to switch over to to _entity_access, a @todo and deprecation listener exception was added in the meantime.

The following routes currently use _access_node_revision:

For now, the access checker entity.node.revision route makes use of view all revisions operation. Even after this route is switched to use _entity_access: node.view revision, it will use the same logic to determine access.

Code notes for NodeRevisionAccessCheck

• NodeRevisionAccessCheck had static caching, \Drupal\Core\Entity\EntityAccessControlHandler::access already has internal static caching with the same contexts, and more (ID, Revision ID, Langcode, Account ID, Operation).
• Deleted now-unused $this->nodeAccess variable created in constructor of NodeRevisionAccessCheck. I believe this is permitted.
• Logic to get $bundle_entity has been cleaned up; made more efficient.

Other notes

• Converted usage of revision permissions to associated operation, via route access checker, only usage is revision overview page. Using route access checker is friendlier to sites/modules customising the route requirements, and is therefore more consistent: Revert or Delete links only shown if user can truely access the route.
• When testing, please take note of pre-existing behavior: not only do you need to have either OP all revisions/OP BundleName revisions permissions, but you also need the associated entity operation permission. E.g to view all revisions, you must also have access to 'view' operation on the entity. See also permission description for view all revisions and friends.
• Added new unit test covering new revision operations.

Overall I think the patch is an improvement despite the overall logic remaining complex.

Kudos and credit to @larowlan for review and providing feedback.

WOW, @dpi, thanks so much for this enormous push forward!

1. I really like what I'm seeing :)

2. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -90,64 +79,15 @@ class NodeRevisionAccessCheck implements AccessInterface {
   +      'update' => 'revert',
   

   Can you add some inline documentation for this? Or at least an @see to a place where I can find out how to interpret this mapping.

3. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -27,6 +28,35 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod
   +   * Supplemental entity operation is used to determine access, for
   

   Does "supplemental" imply "optional"?

4. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -103,6 +142,46 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod
   +    // Revision operations.
   +    if ($revisionPermissionOperation) {
   +      $bundle = $node->bundle();
   +      // If user doesn't have any of these then quit.
   +      if (!$account->hasPermission("$revisionPermissionOperation all revisions") && !$account->hasPermission("$revisionPermissionOperation $bundle revisions") && !$account->hasPermission('administer nodes')) {
   +        return AccessResult::neutral();
   

   The idea is to generalize this to all revisionable entity types' access control handlers, right?

@Wim Leers

Can you add some inline documentation for this? Or at least an @see to a place where I can find out how to interpret this mapping.

Added documentation there

Does "supplemental" imply "optional"?

No it doesnt, I've reworded it, thanks :)

The idea is to generalize this to all revisionable entity types' access control handlers, right?

Once a standard is set here, #2350939: Implement a generic revision UI would create UI for media/taxonomy/menu link content, along with appropriate permissions. Those permissions would map to the operations standardized here.

It would be the responsibility of each entity to map permissions to revision operations. Currently EntityAccessControlHandler only looks for admin permission. I don't think \Drupal\Core\Entity\EntityAccessControlHandler should be modified to look for revision permissions.

Media

I've just checked in on media and there is a revision route and checker very similar to the node implementation. Since entity.media.revision is upcasting entities, MediaRevisionAccessCheck can be fully deprecated without exceptions in DeprecationListener. Media already has revision tests.

Updated patch

• Above feedback
• Media access checker and operation implementation.
• Switched class deprecation to service definition deprecation per https://www.drupal.org/core/deprecation docs.
• Modified deprecation notice as removal is dependant on #2730631: Upcast node and node_revision parameters of node revision routes landing.
• Switched entity.node.version_history to use _entity_access as its param is already upcast thanks to `\Drupal\Core\Entity\EntityResolverManager::setParametersFromReflection`

If we're going to introduce a standard approach here then personally I'm not a fan of using white-space in the operation keys, e.g. "revert all revisions". I know we do it for permission keys but I'm really keen on finding a way (if we can) to move away from that. Would introducing these new access operations with underscores instead of spaces be too much of a BC break?

I'm not a fan of using white-space in the operation

Theres no technical reason not to for operations, operations are mostly just passed around during requests, arn't saved to any kind of storage. Any non-empty string of any length should be valid for operations, including odd symbols, whitespace, unicode.

We already have a couple of operations in core containing whitespace, including 'access taxonomy overview' (via #1848686: Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)) and 'view label' (via #2692091: Use the new 'view label' entity access check in the entity reference label formatter).

Theres no technical reason not to for operations, operations are mostly just passed around during requests, arn't saved to any kind of storage. Any non-empty string of any length should be valid for operations, including odd symbols, whitespace, unicode.

Oh yeah I know technically it doesn't matter, I just feel that as a developer white-space in keys always feels a bit wrong/odd/weird. Also from a consistency point of view there are more places in Drupal which don't allow white-space (mostly for technical reasons I imagine), and even places where underscores are converted to hyphens and vise-versa; I just feel it's good practice to be consistent. Of course you could equally argue that to be consistent would be to stick with spaces since they are common place in the existing commonly used permission keys. So I guess it comes down to which consistency is the better one to adhere to.

After writing and reading my own comment above now I'm just on the fence about it.

Think the summary could use an update, since we seem to have settled on option #2 and it would be good to list all of the different operations which will be introduced.

Would also be great if we could get this in to 8.8.

@dpi Any chance you could fix the failing tests? :) This is looking really good! It would be lovely to still get this in Drupal 8.8 :)

Added two more deprecated messages.

Updated summary to reflect consensus reached on the second option.

Rerolling against 9.0.x

Just noticed I forgot to remove the rreroll tag.

So the reason NodeRevisionsUiBypassAccessTest is failing is due to access result caching. Let's see what happens now.

Edit:

+++ b/core/modules/node/src/Entity/NodeType.php
@@ -220,4 +220,8 @@ public function shouldCreateNewRevision() {
+  public function getCacheTags() {
+    return parent::getCacheTags(); // TODO: Change the autogenerated stub
+  }

Uhh... that shouldn't be there, my bad :)

Added:

• Change deprecation to target removal 10.x instead of 9.x.

I think that issue #3035113 is waiting for this one too. (https://www.drupal.org/project/drupal/issues/3035113)

#36: correct. And since JSON:API is now in core, we can tag this blocker :)

Reroll, three-way merge did the trick.

Actually ignore #38, incorrect reroll. This one should be good.

Just getting the patch up to date, bumping the deprecation version numbers.

Fixed trivial test fails

Fixed my c/p errors.

We are missing coverage for bundle specific permissions here. We also need a change notice for the deprecated warnings added here.

I noticed in the patch there is at least one deprecation trigger_error message that links to this issue, when a change record is created then that link should be updated to point to the change record.

Also, is the title technically correct? Reading the patch it doesn't look like we're providing a new API as such, we're just standardising some access operations and updating existing parts of core to use those new operations.

Thanks,
-Aaron

Per #47

This should pass as per mapping shared in #14.

This is actually blocked on #2730631: Upcast node and node_revision parameters of node revision routes so combined current patch with #2730631-128: Upcast node and node_revision parameters of node revision routes. Hopefully this will be green.

Disclaimer: Coming in pretty blind her, didn't look at the code before, just had a quick look at the issue summary.

1. +++ b/core/modules/media/media.routing.yml
   @@ -10,7 +10,7 @@ entity.media.revision:
      requirements:
   -    _access_media_revision: 'view'
   +    _entity_access: 'media_revision.view all revisions'
        media: \d+
   

   spaces look weird, but we already have that with "view label", so fine.

2. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -4,19 +4,61 @@
   +   */
   +  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $mediaStorage = NULL) {
   +    parent::__construct($entity_type);
   +    if (!isset($mediaStorage)) {
   +      @trigger_error('The $mediaStorage parameter is deprecated in Drupal 9.1.0 and will be required in 10.0.0.', E_USER_DEPRECATED);
   +      $mediaStorage = \Drupal::entityTypeManager()->getStorage('media');
   +    }
   +    $this->mediaStorage = $mediaStorage;
   +  }
   +
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
   

   I know we have a new-ish rule that says you can use camel case if it's consistent in the file, but I think it's still rarely used and mixed, already here, so $media_storage.

   Gut reaction to injecting one entity handler into another is a bit strange. Also, not a fan of injecting handlers to construct in general, I prefer injecting the entity type manager and getting the storage when we need it. Can get weird if a cache clear tries to re-create handlers (see \Drupal\Core\Entity\EntityTypeManager::clearCachedDefinitions), to be fair, the access handler should also be reset then.

3. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -90,64 +76,18 @@ public function access(Route $route, AccountInterface $account, $node_revision =
          'view' => 'view all revisions',
   -      'update' => 'revert all revisions',
   -      'delete' => 'delete all revisions',
   +      'update' => 'revert',
   +      'delete' => 'delete revision',
   

   should we go with revert revision for consistency, that all those operations contain the revision keyword?

4. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -228,19 +223,21 @@ public function revisionOverview(NodeInterface $node) {
   -          if ($revert_permission) {
   +          $revertLink = $has_translations ?
   +            Url::fromRoute('node.revision_revert_translation_confirm', ['node' => $node->id(), 'node_revision' => $vid, 'langcode' => $langcode]) :
   +            Url::fromRoute('node.revision_revert_confirm', ['node' => $node->id(), 'node_revision' => $vid]);
   +          if ($revertLink->access($account)) {
                $links['revert'] = [
   

   url access checking is rather slow, as every access check is basically a route match, that could hurt quite a bit on a page with 50 revisions. also camelCase variables again.

5. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -104,6 +143,46 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
   +      }
   +      elseif ($account->hasPermission('administer nodes')) {
   +        return AccessResult::allowed()->cachePerPermissions();
   +      }
   +      else {
   

   administer nodes vs bypass node access is such a mess :-/ nothing else use administer nodes on the entity access level, only for specific fields. but this isn't the place to unify that.

6. +++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
   @@ -192,6 +192,12 @@ public static function getSkippedDeprecations() {
   +      // The following deprecation relates to generic entity revision access
   +      // API.
   +      'The "access_check.node.revision" service is deprecated. You should use the \'access_check.entity\' service instead. See https://www.drupal.org/node/3043321',
   +      'NodeRevisionAccessCheck is deprecated in Drupal 9.1.x and will be removed before Drupal 10.0.x. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321',
   +      'The "access_check.media.revision" service is deprecated. You should use the \'access_check.entity\' service instead. See https://www.drupal.org/node/3043321',
   +      'MediaRevisionAccessCheck is deprecated in Drupal 9.1.x and will be removed before Drupal 10.0.x. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321',
   

   we will have to get rid of this, this method is only for deprecations that are out of our control, aka vendor deprecations.

   That means we can't deprecate the access checks, only specific methods. Been there before and had to revert that again.

This addresses #53.

Reverted #53.6. We need that for patch to pass. It includes patch from #2730631-140: Upcast node and node_revision parameters of node revision routes.

Better title. Also created change record as well https://www.drupal.org/node/3161210.

Hm, this is still blocked on #2730631: Upcast node and node_revision parameters of node revision routes?

Looks like that issue has now passed.

(And now?)

(And now?)

Still blocked on #2730631: Upcast node and node_revision parameters of node revision routes, we need that to be committed first, once that's done the patch here needs to be updated to remove the code brought in from that issue.

Question is this ticket suppose to add a revisions UI to blocks and media?

Question is this ticket suppose to add a revisions UI to blocks and media?

@smustgrave's question was asked and answered in #2350939-160: Implement a generic revision UI.

This appears to need a re-roll:

error: while searching for:
      'AssertLegacyTrait::assertNoEscaped() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->assertNoEscaped() instead. See https://www.drupal.org/node/3129738',
      'AssertLegacyTrait::assertPattern() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseMatches() instead. See https://www.drupal.org/node/3129738',
      'AssertLegacyTrait::constructFieldXpath() is deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use $this->getSession()->getPage()->findField() instead. See https://www.drupal.org/node/3129738',
    ];
  }

Here is a re-roll of #56 against 9.2.x which includes #2730631-140 as well.

Fixed custom command fails of patch #66.

Fixed custom command fails of patch #67.

Fixed failed the test of patch #68.

Rerolled #56 after #2730631: Upcast node and node_revision parameters of node revision routes.

Unassigning for now.

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -4,19 +4,61 @@
+      @trigger_error('The $entity_type_manager parameter is deprecated in Drupal 9.1.0 and will be required in 10.0.0.', E_USER_DEPRECATED);

+++ b/core/modules/node/src/NodeAccessControlHandler.php
@@ -35,10 +65,17 @@ class NodeAccessControlHandler extends EntityAccessControlHandler implements Nod
+      @trigger_error('The $entity_type_manager parameter is deprecated in Drupal 9.1.0 and will be required in 10.0.0.', E_USER_DEPRECATED);

This needs to be updated as per standard and maybe deprecation tests as well.

Updated IS.
Updated existing change record.
Added new change record.
Fixed the fails in #72.
Addressed #75.
Deprecated \Drupal\node\Access\NodeRevisionAccessCheck::__construct() and \Drupal\media\Access\MediaRevisionAccessCheck::__construct() as well as https://www.drupal.org/project/media_revisions_ui is extending that class and overriding the method.

Updated one too many things.

hi,
sorry, I try to apply this patch #78 on 9.3.x.dev. The patch failed to apply.
I have attached a screenshot.

It seems like your 9.3.x branch is not up to date. Please pull down all the latest changes to apply the patch.

hi jibran,
you are right, my 9.3.x branch is behind
i am sorry for that.
now the patch applied successfully.

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -137,6 +137,12 @@ public static function getSkippedDeprecations() {
       'assertFileNotIsWritable() is deprecated and will be removed in PHPUnit 10. Refactor your code to use assertFileIsNotWritable() instead.',
       'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Please refactor your test to not rely on the order in which methods are invoked.',
+      // The following deprecation relates to generic entity revision access
+      // API.
+      'The "access_check.node.revision" service is deprecated. You should use the \'access_check.entity\' service instead. See https://www.drupal.org/node/3043321',
+      'NodeRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321',
+      'The "access_check.media.revision" service is deprecated. You should use the \'access_check.entity\' service instead. See https://www.drupal.org/node/3043321',
+      'MediaRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321',
     ];

mentioned that in an earlier review, we need to try and deprecate it in a way so that only actual usages trigger the deprecation.

Adding exceptions pretty much defeats the purpose of having deprecations as they will not be reported then in tests. AFAIK we should only use that for third party deprecations that we can't resolve yet.

It's not quite the same but try pudding a deprecation directly in the access implementation method. we might not catch some uses then. Although we could possibly do both? http://grep.xnddx.ru/search?text=NodeRevisionAccessCheck shows some subclases and direct calls to that.

This is also hiding some usages that we have in core right now, specifically the jsonapi module, we need to update that to use the new entity operations instead of injecting the node service.

Removed the deprecation exceptions. Update tests and JSON:API to use updated logic.

+++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
@@ -246,13 +196,8 @@ protected function checkRevisionViewAccess(EntityInterface $entity, AccountInter
     $entity_type = $entity->getEntityType();
     switch ($entity_type->id()) {
       case 'node':
-        assert($entity instanceof NodeInterface);
-        $access = AccessResult::allowedIf($this->nodeRevisionAccessCheck->checkAccess($entity, $account, 'view'))->cachePerPermissions()->addCacheableDependency($entity);
-        break;
-
       case 'media':
-        assert($entity instanceof MediaInterface);
-        $access = AccessResult::allowedIf($this->mediaRevisionAccessCheck->checkAccess($entity, $account, 'view'))->cachePerPermissions()->addCacheableDependency($entity);
+        $access = $entity->access('view all revisions', $account, TRUE)->cachePerPermissions()->addCacheableDependency($entity);
         break;
 

I think the cache per permissions and add cacheable dependency can go, if that is relevant then it's the responsibility of the access handler to add that. In fact, I think you can remove the whole switch here, if you see below the default is just saying that it's not supported. So any existing entity type that is not implementing the operations yet will simply continue to not allow access as that is the default.

Lots of deprecations now because of the service, you need to remove the @deprecated there for now. That said, I seem to remember an issue about improving the router access logic to only call service if necessary which would allow to do this.

Updated logic in EntityAccessChecker

Lots of deprecations now because of the service, you need to remove the @deprecated there for now.

Yeah, it is either this or update the access logic.

That said, I seem to remember an issue about improving the router access logic to only call service if necessary which would allow to do this.

\Drupal\Core\Access\CheckProvider::loadCheck() is the culprit here.

  /**
   * {@inheritdoc}
   */
  public function loadCheck($service_id) {
    if (empty($this->checks[$service_id])) {
      if (!in_array($service_id, $this->checkIds)) {
        throw new \InvalidArgumentException(sprintf('No check has been registered for %s', $service_id));
      }

      $check = $this->container->get($service_id);

      if (!($check instanceof AccessInterface)) {
        throw new AccessException('All access checks must implement AccessInterface.');
      }
      if (!is_callable([$check, $this->checkMethods[$service_id]])) {
        throw new AccessException(sprintf('Access check method %s in service %s must be callable.', $this->checkMethods[$service_id], $service_id));
      }

      $this->checks[$service_id] = $check;
    }
    return [$this->checks[$service_id], $this->checkMethods[$service_id]];
  }

$check = $this->container->get($service_id); triggers deprecation exception. There is no reason at all to create an instance here. If we can get the service definition then we can check all the same things. It will probably save a lot of time as well. I don't think we can get service definition from container unless we add a getter or use reflection. :P

Do we need a third change notice to announce new revision operations?

• 'view all revisions' to view individual revision or revision listing page.
• 'view revision' to view individual revision
• 'revert revision'to revert revision
• 'delete revision' to delete revision

Or NodeRevisionAccessCheck and MediaRevisionAccessCheck are deprecated should cover it?

Found the issue I was thinking of: #3183036: Don't instantiate access checkers not used by any route

@jibran in #87:

Personally I'd argue that the current change record is enough for now, because:

1. Right now these are only relevant in the context of Node and (to a lesser extent) Media;
2. It's more of introducing a new naming pattern across Drupal gradually, rathern than a sudden hard API change, which everyone needs to react to now;
3. The access checker classes accept any arbitrary string as the operation, so contrib/custom entity types can use whatever they want, and this change doesn't impact them;
4. Annoucning this as a new standard/pattern as part of #2350939: Implement a generic revision UI is probably more relevant, as there we are intrdoucing the generic UI and APIs where these access operations will be used across Drupal, so that's where I see these becoming relevant outside the world of Nodes and Media.

@AaronMcHale good point. I think the word generic in the title confused me. :D

This might fix some fails.

DIfference between self and static just in case:

Psy Shell v0.10.8 (PHP 8.0.6 — cli) by Justin Hileman
Drupal (Drupal 9.3.0-dev)
>>> class A {public function __construct(){print static::class . PHP_EOL; print self::class . PHP_EOL;}};
>>> class B extends A{};
>>> $a = new A();
A
A
=> A {#5104}
>>> $b = new B();
B
A
=> B {#5102}

From 98 fails to only 3, I think your theory worked there @jibran :D

Turns out JSON:API changes are not straightforward. In \Drupal\Tests\jsonapi\Functional\ResourceTestBase::testRevisions() we have

    // JSON:API will only support node and media revisions until Drupal core has
    // a generic revision access API.
    if (!static::$resourceTypeIsVersionable) {
      $this->setUpRevisionAuthorization('GET');
      $url = Url::fromRoute(sprintf('jsonapi.%s.individual', static::$resourceTypeName), ['entity' => $this->entity->uuid()])->setAbsolute();
      $url->setOption('query', ['resourceVersion' => 'id:' . $this->entity->getRevisionId()]);
      $request_options = [];
      $request_options[RequestOptions::HEADERS]['Accept'] = 'application/vnd.api+json';
      $request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions());
      $response = $this->request('GET', $url, $request_options);
      $detail = 'JSON:API does not yet support resource versioning for this resource type.';
      $detail .= ' For context, see https://www.drupal.org/project/drupal/issues/2992833#comment-12818258.';
      $detail .= ' To contribute, see https://www.drupal.org/project/drupal/issues/2350939 and https://www.drupal.org/project/drupal/issues/2809177.';
      $expected_cache_contexts = [
        'url.path',
        'url.query_args:resourceVersion',
        'url.site',
      ];
      $this->assertResourceErrorResponse(501, $detail, $url, $response, FALSE, ['http_response'], $expected_cache_contexts);
      return;
    }

Which tests \Drupal\jsonapi\Revisions\ResourceVersionRouteEnhancer::enhance() specifically. \Drupal\jsonapi\ResourceType\ResourceTypeRepository::createResourceType() uses \Drupal\jsonapi\ResourceType\ResourceTypeRepository::isVersionableResourceType() to makes sure that
nothing is supported other than media and nodes. I tried to write a test for \Drupal\Tests\jsonapi\Functional\EntityTestRevTest to update\Drupal\Tests\jsonapi\Functional\ResourceTestBase::testRevisions() and it became very clear that changes in JSON:API should be done in its own issue.

Given all that I'm going to keep changes in JSON:API to a minimum.

+++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
@@ -194,18 +194,7 @@ protected function checkRevisionViewAccess(EntityInterface $entity, AccountInter
-      default:
-        $reason = 'Only node and media revisions are supported by JSON:API.';
-        $reason .= ' For context, see https://www.drupal.org/project/drupal/issues/2992833#comment-12818258.';
-        $reason .= ' To contribute, see https://www.drupal.org/project/drupal/issues/2350939 and https://www.drupal.org/project/drupal/issues/2809177.';
-        $access = AccessResult::neutral($reason);

I have re-added the default statement.

That's fair, I've been in the same situation several times with those generic rest/jsonapi tests. They are certainly fancy, but their extensibility is built on top of how things work right now, and changing those things can be painful. Lets do a follow-up.

The last test makes sense, the generic logic depends on node type settings and adds the cache tag for that, so that's technically required (although I'm always on the fence on the overhead of checking that cache tag on every request vs invalidating a common cache tag, as that setting basically changes never on a running site).

1. +++ b/core/modules/jsonapi/src/Access/EntityAccessChecker.php
   @@ -64,24 +60,6 @@ class EntityAccessChecker {
   -   *
   -   * This will be NULL unless the node module is installed.
   -   *
   -   * @var \Drupal\node\Access\NodeRevisionAccessCheck|null
   -   */
   -  protected $nodeRevisionAccessCheck = NULL;
   -
   -  /**
   -   * The media revision access check service.
   -   *
   -   * This will be NULL unless the media module is installed.
   -   *
   -   * @var \Drupal\media\Access\MediaRevisionAccessCheck|null
   -   */
   -  protected $mediaRevisionAccessCheck = NULL;
   

   do we need BC for those properties? We could use the trait that we added for entity manager? but I think json api also likes to mark a lot of its classes as internal, if that's here then fine to remove I guess.

2. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -549,4 +551,16 @@ public function providerCreateAccess() {
   +    $entity_type->id()->willReturn(mt_rand(1, 128));
   +    new MediaAccessControlHandler($entity_type->reveal());
   

   do we really gain something from making this random? it's not even used, and this isn't even a generic entity test, we know that it would be media? Took me a few seconds to understand why we do this.

3. +++ b/core/modules/node/node.routing.yml
   @@ -70,7 +70,7 @@ entity.node.revision:
        _title_callback: '\Drupal\node\Controller\NodeController::revisionPageTitle'
      requirements:
   -    _access_node_revision: 'view'
   +    _entity_access: 'node_revision.view revision'
        node: \d+
   

   so one thing here is a bit tricky. the entity access control handler does static caching, that means if you implement something that would really vary between revisions, for whatever reason, then doing multiple access checks in the same request for different revisions will return wrong results. And we do that for the revision overview page.

   I suppose the static cache could detect a non-default revision and in that case use the revision id for the static cache?

+++ b/core/modules/media/src/MediaAccessControlHandler.php
@@ -4,19 +4,61 @@
+    // Default revisions must be checked for 'view all revisions' operation,
+    // administer permission must not override it.

Why should the admin permission not override this? Isn't that the whole purpose of an admin permission?

I was just reviewing the access checks in core and here because I've been sponsored to add revision support to the Group module and I was trying to understand the access checks behind the revision UI so I could adjust them to Group's architecture. While doing so I found that the "meat" of the access checks in core and here seem a bit rough to understand at first sight so I tried to rewrite it and came up with this instead (see below). Would now perhaps be a good time to adopt similar changes here. If tests remain green, it should be fine, no?

  public function checkAccess(GroupInterface $group, AccountInterface $account, $operation = 'view') {
    $map = [
      'view' => 'view group revisions',
      'update' => 'revert group revisions',
      'delete' => 'delete group revisions',
    ];

    if (!isset($map[$operation])) {
      return AccessResult::neutral();
    }

    $cid = implode(':', [
      $group->getRevisionId(),
      $group->language()->getId(),
      $account->id(),
      $operation,
    ]);

    if (isset($this->accessCache[$cid])) {
      return $this->accessCache[$cid];
    }

    // You cannot manipulate the default revision through the revision UI.
    if ($operation !== 'view' && $group->isDefaultRevision()) {
      $this->accessCache[$cid] = AccessResult::forbidden()->addCacheableDependency($group);
      return $this->accessCache[$cid];
    }

    // You cannot view the default revision through the revision UI if it is
    // the only revision available and the group type is not set to create new
    // revisions. The latter is checked because if it was set, we might allow
    // access so the revisions tab shows up.
    $group_type = $group->getGroupType();
    if ($operation === 'view' && $group->isDefaultRevision() && $this->countDefaultLanguageRevisions($group) === 1 && !$group_type->shouldCreateNewRevision()) {
      $this->accessCache[$cid] = AccessResult::forbidden()->addCacheableDependency($group_type)->addCacheableDependency($group);
      return $this->accessCache[$cid];
    }

    // Perform basic permission checks first and return no-access results.
    $this->accessCache[$cid] = GroupAccessResult::allowedIfHasGroupPermission($group, $account, $map[$operation]);
    if (!$this->accessCache[$cid]->isAllowed()) {
      return $this->accessCache[$cid];
    }

    // Now that the edge cases are out of the way, check for entity access on
    // both the default revision and the passed in revision (if any).
    $entity_access = $group->access($operation, $account, TRUE);
    if (!$group->isDefaultRevision()) {
      $default_revision = $this->entityTypeManager->getStorage('group')->load($group->id());
      $entity_access = $entity_access->andIf($default_revision->access($operation, $account, TRUE));
    }
    $this->accessCache[$cid] = $this->accessCache[$cid]->andIf($entity_access);

    // Because of the group type checks above when dealing with the 'view'
    // operation, we must add the group type as a cacheable dependency so we can
    // return a different result in case shouldCreateNewRevision() changes.
    if ($operation === 'view') {
      $this->accessCache[$cid] = $this->accessCache[$cid]->addCacheableDependency($group_type);
    }

    return $this->accessCache[$cid];
  }

I didn't bother to swap out the group stuff because I think all in all it's still pretty easy to read.

RE: #96

1. No, we don't need BC. The class is marked internal.
2. Removed rand call.
3. Is it not coverd in \Drupal\Core\Entity\EntityAccessControlHandler::access()?
   

       // If an entity does not have a UUID, either from not being set or from not
       // having them, use the 'entity type:ID' pattern as the cache $cid.
       $cid = $entity->uuid() ?: $entity->getEntityTypeId() . ':' . $entity->id();
   
       // If the entity is revisionable, then append the revision ID to allow
       // individual revisions to have specific access control and be cached
       // separately.
       if ($entity instanceof RevisionableInterface) {
         /** @var \Drupal\Core\Entity\RevisionableInterface $entity */
         $cid .= ':' . $entity->getRevisionId();
       }
   
       if (($return = $this->getCache($cid, $operation, $langcode, $account)) !== NULL) {
         // Cache hit, no work necessary.
         return $return_as_object ? $return : $return->isAllowed();
       }
   

PS: Updating core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php is so irritating.

3. Ha, you are right. I completely forgot that we already have that. All good then. That was actually a security issue: "Entity access bypass for entities that do not have UUIDs or have protected revisions - Access Bypass - Critical - Drupal 8 - CVE-2017-6925"

1. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -2870,7 +2871,13 @@ public function testRevisions() {
        $expected_document['data']['links']['working-copy']['href'] = $rel_working_copy_url->setAbsolute()->toString();
   -    $this->assertResourceResponse(200, $expected_document, $actual_response, $expected_cache_tags, $expected_cache_contexts, FALSE, 'MISS');
   +    if ($entity instanceof NodeInterface) {
   +      $prior_revision_expected_cache_tags = Cache::mergeTags($expected_cache_tags, $entity->type->entity->getCacheTags());
   +      $this->assertResourceResponse(200, $expected_document, $actual_response, $prior_revision_expected_cache_tags, $expected_cache_contexts, FALSE, 'MISS');
   +    }
   +    else {
   +      $this->assertResourceResponse(200, $expected_document, $actual_response, $expected_cache_tags, $expected_cache_contexts, FALSE, 'MISS');
   +    }
   

   I suspect Wim isn't going to like this :)

   Maybe we can add a method like getExtraRevisionCacheTags() or something, and then the node subclass can override that?

   That said, are we sure that Media shouldn't have that too? (update: see below)

2. +++ b/core/modules/media/tests/src/Kernel/MediaAccessControlHandlerTest.php
   @@ -559,7 +559,7 @@ public function providerCreateAccess() {
        $this->expectDeprecation('Calling Drupal\media\MediaAccessControlHandler::__construct() without the $entity_type_manager argument is deprecated in drupal:9.3.0 and will be required in drupal:10.0.0. See https://www.drupal.org/node/3214171');
        $entity_type = $this->prophesize(EntityTypeInterface::class);
   -    $entity_type->id()->willReturn(mt_rand(1, 128));
   +    $entity_type->id()->willReturn(1);
        new MediaAccessControlHandler($entity_type->reveal());
   

   this is actually the entity *type* id, so it should be 'media', not 1.

3. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -4,19 +4,61 @@
   -    if ($account->hasPermission('administer media')) {
   +    $administerPermission = $this->entityType->getAdminPermission();
   +    /** @var \Drupal\media\MediaInterface $entity */
   +    // Default revisions must be checked for 'view all revisions' operation,
   +    // administer permission must not override it.
   +    if ($operation !== 'view all revisions' && $account->hasPermission($administerPermission)) {
          return AccessResult::allowed()->cachePerPermissions();
   

   this is interesting, actually.

   the comment could be better, but if check the access below, is that we have "access" logic about multiple revisions. And that we don't allow access to the revision overview if there aren't at least two revisions.

   That does make sense and is how it works now, so we need something for it. But is it really *entity access* logic? should we really have that complexity in here?

   It's not so much about access but that we don't want to show the revisions tab/page if you don't have revisions.

   That said, this has actually been changed for nodes a while ago: #808730: Show the Revisions tab/page even when only one revision exists.. And I guess that's why only node has the node type cache tag and not media.

   So.. not sure what to do. This does not seem like a good time to refactor that behavior. I'm glad at least that node is different, because node would have been even more complicated (with the much earlier bypass permission for example).

   At least we should have a follow-up to align media on node? And how should the generic entity access look like? Right now, it doesn't care at all. Should we have at least the logic about not being able to delete the default revision as default entity access logic? Because if not, the only thing we are standardizing here is the those entity operations exist, but we can't expect the default logic to be sufficient for a generic revision UI.

   I guess the minimum is to update the comment to explain the why, not just what.

It's not so much about access but that we don't want to show the revisions tab/page if you don't have revisions.

Agreed, this could be a simple route access check _has_revisions: TRUE or something like that.

Fixes #99 2 and 3.

This does not seem like a good time to refactor that behavior

100% agree, this should be done in a separate issue. Also agree it doesn't seem like logic that should exist in an access control handler.

Still need to fix #99.1 but not entirely sure how to go about it, will continue to try and dig into it.

Interdiff may be a bit wonky because I manually changed it due to a partial re-roll.

Hehe, thanks @acbramley for updating the patch. 30 days ago I started fixing #99.1 and ran out of steam so good luck 🙂. I can add my WIP patch if that helps?

@jibran that's fine, I'm just running tests locally and if that passes will upload what I've got

Fixes #99.1

Woops!

There are a few after // Test relationship responses. which need update see interdiff in #98 https://www.drupal.org/files/issues/2021-06-05/interdiff-440182.txt.

Fixes the remaining instance of checks.

Fixes linting

Nice work @acbramley.

+++ b/core/modules/media/media.services.yml
@@ -7,7 +7,6 @@ services:
-    deprecated: The "%service_id%" service is deprecated. You should use the 'access_check.entity' service instead. See https://www.drupal.org/node/3043321

+++ b/core/modules/node/node.services.yml
@@ -13,7 +13,6 @@ services:
-    deprecated: The "%service_id%" service is deprecated. You should use the 'access_check.entity' service instead. See https://www.drupal.org/node/3043321

Now that #3183036: Don't instantiate access checkers not used by any route is fixed we can re-add these service depreciations.

Adds deprecated keys back.

+++ b/core/modules/media/media.services.yml
@@ -7,6 +7,7 @@ services:
+    deprecated: The "%service_id%" service is deprecated. You should use the 'access_check.entity' service instead. See https://www.drupal.org/node/3043321

+++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
@@ -44,6 +37,11 @@ class MediaRevisionAccessCheck implements AccessInterface {
+      @trigger_error('MediaRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321', E_USER_DEPRECATED);

@@ -90,42 +90,14 @@ public function access(Route $route, AccountInterface $account, $media_revision
+    @trigger_error('MediaRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321', E_USER_DEPRECATED);

+++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
@@ -44,8 +30,12 @@ class NodeRevisionAccessCheck implements AccessInterface {
+      @trigger_error('NodeRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321', E_USER_DEPRECATED);

@@ -68,6 +58,7 @@ public function __construct(EntityTypeManagerInterface $entity_type_manager) {
+    @trigger_error('NodeRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321', E_USER_DEPRECATED);

@@ -90,64 +81,18 @@ public function access(Route $route, AccountInterface $account, $node_revision =
+    @trigger_error('NodeRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3043321', E_USER_DEPRECATED);

These URLs should be updated to change the record URL https://www.drupal.org/node/3161210.

Fixes #112

Hi @acbramley thanks a lot for your commit! Great work!!
Is there still anything open you know from the previous comments or ready to be community-reviewed?

Reading through the comments I only found left:

This does not seem like a good time to refactor that behavior

100% agree, this should be done in a separate issue. Also agree it doesn't seem like logic that should exist in an access control handler.

Is there already an issue for that part you know about?

Can #2350939: Implement a generic revision UI be unblocked on THIS issue or does it also depend on that refactoring? What do you think?

We also need a follow up for #94 and #96.0, an issue which will add revision support for non-node and non-media entities in JSON:API.

The next steps here are to get a review from the API-First team and entity maintainer.

#2350939: Implement a generic revision UI is blocked on this issue but not on any other potential follow-ups.

To be honest i'm not completely sure of all the implication this has for jsonapi, still learning some of the internals of jsonapi. I'll also ping the other maintainers to see if one of them can have a look.

The point @larowan makes in #116 is a valid conceirn, but i cannot address it.

1. +++ b/core/modules/jsonapi/tests/src/Functional/NodeTest.php
   @@ -520,4 +520,11 @@ public function testCollectionFilterAccess() {
   +    return $this->entity->type->entity->getCacheTags();
   

   Shouldn't this merge the parent cache tags also? I know it isn't usefull right now, but should we add perhaps add some more generic cache tags this might break. Strategy could ofcourse be adding that when we need to instead which might just be the safer option for the current set of changes.

2. +++ b/core/modules/jsonapi/tests/src/Functional/ResourceTestBase.php
   @@ -3110,46 +3120,47 @@ public function testRevisions() {
   +      [$revision_id, $relationship_url, $related_url] = $revision_case;
   
   @@ -3170,11 +3183,11 @@ public function testRevisions() {
   +      [$revision_id, $relationship_url, $related_url] = $revision_case;
   

   Is it OK to start doing this now? Currently core doesn't really use array unpacking like this, but with list(). I did make an issue regarding that a bit back #3222769: [November 8, 2021] Replace all list (array destructuring) assignment to the array syntax.

I talked to @e0ipso on slack about this issue. Other than the fact we are quite exscited to start expanding revision support in json:api after this he did raise a conceirn.

Just a question. I assume the old one-off services covered the same granularity as the new ones in terms of permissions?

Meaning there are no permissions for revisions at the entity level now:

-        $access = AccessResult::allowedIf($this->mediaRevisionAccessCheck->checkAccess($entity, $account, 'view'))->cachePerPermissions()->addCacheableDependency($entity);
+        $access = $entity->access('view all revisions', $account, TRUE);

but we likely didn't have them before either.

When i look at the changes it seems we should be O.K. but i think it's important to log it here also.

I think the simplest form is to make media align with Node, and always show the tab if you have permission, regardless of the number of revisions - assuming I understand the question 😹

From a UX perspective, I think this is fine. We already have an existing pattern in core - in the form of how Node behaves - and this issue probably isn't the right place to decide whether we should diverge from that.

For instance, you could make the argument that it's better to always show the tab because then it appears consistently, even if there's only one revision; We don't want to start introducing an inconsistent behaviour which leads to the user wondering: "why is there no revision tab here, is something broken", or "why can't my keyboard shortcut or screen reader find the revision tab?".

So let's just keep it simple and have Node and Media behave in the same way Node currently does, then if we want to discuss it further, we can open a follow-up issue to look at whether we want to change this behaviour.

Thanks,
-Aaron

> Meaning there are no permissions for revisions at the entity level now:

We have the "view all revisions" operation, so I think that is covered. We don't need a global API I think (which does not exist for entity listings either), as there is no built-in use case for that. You can create a view of all revisions, but then you can also configure permissions there directly.

I think the jsonapi scope is challenging because we'd be expanding the current hardcoded support for node/media to support all entity types that implementation revision access. Beside the actual change itself, that specifically has relatively complex implications for the very generic and abstracted jsonapi test suite, so we likely need to introduce new methods on the base class to indicate whether the revision UI should be tested, what kind of access and cacheability metadata is expected and so on. But we can push all that to a follow-up and limit the support for now still to node/media with minimal test changes.

> So let's just keep it simple and have Node and Media behave in the same way Node currently does, then if we want to discuss it further, we can open a follow-up issue to look at whether we want to change this behaviour.

Note: Fully agree with this decision, but from a technical perspective, this isn't actually that "simple", as it is changing the current behavior of media entities. So it wouldn't really make sense to change it now and then open a follow-up issue to discuss if we really want to make that change :)

> So let's just keep it simple and have Node and Media behave in the same way Node currently does, then if we want to discuss it further, we can open a follow-up issue to look at whether we want to change this behaviour.

Note: Fully agree with this decision, but from a technical perspective, this isn't actually that "simple", as it is changing the current behavior of media entities. So it wouldn't really make sense to change it now and then open a follow-up issue to discuss if we really want to make that change :)

Ah yes that's a fair point :) My thinking was mostly on how this would currently impact the Node Revision UI, since Media doesn't have an actual UI for revisions, and likely won't until #2350939: Implement a generic revision UI is done, which if there's a desire to discuss it, should give us time to discuss and agree a pattern for the generic UI; But of course the access checking does not just impact the UI, as is being discussed here it would also impact JSON:API.

So, if I were to make a decision now for this issue and for the future, in my opinion let's not involve checking the number of revisions in the access check. Even if there's one or no revisions, at least having an interface that tells you that is better than having no interface and having to make a guess based on the fact the interface decided not to show itself - like some magic geni in a bottle :D

Also, from a caching perspective, I can only assume there's some performance advantages to not having a the "Revision" tab be conditional on how many revisions are shown. No idea if that is even a consideration for the caching system (in this case for Nodes), but if it is, then surely that's a win because then there's less variations.

Thanks,
-Aaron

Conveniently, this week's Usability meeting has just taken place, which gave me the opportunity to get wider feedback on whether the revision UI should be conditional on whether there is more than one revision.

After discussing the problem and my thoughts at #3225198: Drupal Usability Meeting 2021-07-30, there was unanimous agreement that the revision UI should not be conditional on the number of revisions. For all the reasons I previously stated, but an interesting point around training users and documentation was mentioned, where it is easier if the tab was not conditional.

This means we now have sound footing for that approach going forward.

Thanks,
-Aaron

So while we all agree on the fact that the revisions tab requires a refactor, I'm not sure if we've decided if it needs to be done in this issue or not? It seems like there's pros/cons to either option...

EDIT: Confirmed with @larowlan that the consensus seems to be that we should remove that logic entirely in this issue. I can work on that this afternoon/tomorrow.

And the media logic was copied in #2885486: Media entity is revisionable but doesn't have a revision link template

We removed the node logic in #808730: Show the Revisions tab/page even when only one revision exists. - it took eight years, which was long enough for media to have copied the logic in the meantime.

@AaronMcHale do you agree that removing that limitation makes sense?

Yes I would completely agree with that, and was also the unanimous opinion of the UX meeting.

@catch just to confirm - this is regarding revision view access, not the revision list (media doesn't even have one in core).

I've split this out into #3226588: Allow revision view access for node and media when there is only one revision and will work on this today, this issue should probably now be blocked on it?

Sorry, just caught up on the slack thread. Have closed the above and uploaded a patch to #3226487: Always allow access to revisions based on access/permissions, not on the count of revisions

Progress! #3226487: Always allow access to revisions based on access/permissions, not on the count of revisions has been committed! I believe that means the patch here will need to be rerolled.

Working on the re-roll

Unfortunately no interdiff here, I've tried my best to keep the changes 1-1 with what we did in #3226487: Always allow access to revisions based on access/permissions, not on the count of revisions but the 2 access control handlers will need another good review.

Fixes the tests and adds back the check on view all revisions operation. We need to cachePerPermissions there too because we check for permissions beforehand.

1. +++ b/core/modules/media/src/Access/MediaRevisionAccessCheck.php
   @@ -44,6 +37,11 @@ class MediaRevisionAccessCheck implements AccessInterface {
      public function __construct(EntityTypeManagerInterface $entity_type_manager) {
   +    // The access check provider create an instance for each access handler so
   +    // only trigger deprecation exception for the extended classes.
   +    if (static::class !== self::class) {
   +      @trigger_error('MediaRevisionAccessCheck is deprecated in drupal:9.3.0 and will be removed before drupal:10.0.0. Use "_entity_access" requirement with relevant operation instead. See https://www.drupal.org/node/3161210', E_USER_DEPRECATED);
   +    }
   

   #3183036: Don't instantiate access checkers not used by any route landed, so I think this is no longer true and we can simplify this?

   We can then possibly also skip the @trigger_error() calls in the methods because we can be certain that the deprecation was already triggered if used.

2. +++ b/core/modules/media/src/MediaAccessControlHandler.php
   @@ -83,6 +123,20 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
   +
   +        // First check the access to the default revision and finally, if the
   +        // media passed in is not the default revision then access to that,
   +        // too.
   +        return AccessResult::allowedIf(
   +          $this->access($this->entityTypeManager->getStorage($entity->getEntityTypeId())->load($entity->id()), 'view', $account) &&
   +          ($entity->isDefaultRevision() || $this->access($entity, 'view', $account))
   +        )->cachePerPermissions()->addCacheableDependency($entity);
   ...
        }
   

   We're getting into personal preference territory again where I know I have different opinions than some others, but this is IMHO hard to read. I much prefer having separate statements with local variables than such multi-line statements.

   I know it's mostly copied, but I'd suggest a $media_storage local variable, similar to the original code that had that as a property (I do prefer using the entity type manager property!).

   Also, the inlined access() checks mean we lose the cacheability info from those checks. I believe we could write it like this instead:

   $access = $this->access($media_storage->load($entity->id()), 'view', $account, TRUE);
   if (!$entity->isDefaultRevision() {
     $access = $access->orIf($this->access($entity, 'view', $account, TRUE);
   }
   

   I believe this is the equivalent, does merge cacheablity and is easier to read?

   Same for node of course.

3. +++ b/core/modules/node/src/Access/NodeRevisionAccessCheck.php
   @@ -90,53 +81,18 @@ public function access(Route $route, AccountInterface $account, $node_revision =
   +    // E.g route _access_node_revision: 'update' to $node->access('revert')
   +    $entityOperationMap = [
          'view' => 'view all revisions',
   -      'update' => 'revert all revisions',
   -      'delete' => 'delete all revisions',
   +      'update' => 'revert revision',
   +      'delete' => 'delete revision',
   

   the example in the comment is outdated. I'm also not sure if that example is really needed. It seems to mix a routing definition with php code and it seems pretty straight-forward to me?

   also, is this using a camelCase variable name on purpose? afaik it is technically allowed but only if the whole file uses that?

4. +++ b/core/modules/node/src/Controller/NodeController.php
   @@ -165,8 +165,8 @@ public function revisionOverview(NodeInterface $node) {
   -    $revert_permission = (($account->hasPermission("revert $type revisions") || $account->hasPermission('revert all revisions') || $account->hasPermission('administer nodes')) && $node->access('update'));
   -    $delete_permission = (($account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes')) && $node->access('delete'));
   +    $revert_permission = $account->hasPermission("revert $type revisions") || $account->hasPermission('revert all revisions') || $account->hasPermission('administer nodes') || $node->access('revert revision');
   +    $delete_permission = $account->hasPermission("delete $type revisions") || $account->hasPermission('delete all revisions') || $account->hasPermission('administer nodes') || $node->access('delete revision');
    
   

   do we really still need all those explicit access checks? doesn't $node->access('revert revision') cover them?

5. +++ b/core/modules/node/src/NodeAccessControlHandler.php
   @@ -104,6 +143,43 @@ protected function checkAccess(EntityInterface $node, $operation, AccountInterfa
    
   +    [$revisionPermissionOperation, $entityOperation] = static::REVISION_OPERATION_MAP[$operation] ?? [
   +      NULL,
   +      NULL,
   +    ];
   +
   

   this is also missing snake and camel case in the same function.

6. +++ b/core/modules/node/tests/src/Functional/NodeRevisionPermissionsTest.php
   @@ -125,12 +124,12 @@ public function testNodeRevisionAccessAnyType() {
          ->execute();
        foreach ($permutations as $case) {
          // Skip this test if there are no revisions for the node.
   -      if (!($revision->isDefaultRevision() && (count($vids) == 1 || $case['op'] == 'update' || $case['op'] == 'delete'))) {
   +      if (!($revision->isDefaultRevision() && (count($vids) == 1 || $case['op'] == 'revert revision' || $case['op'] == 'delete revision'))) {
            if (!empty($case['account']->is_admin) || $case['account']->hasPermission($this->map[$case['op']])) {
   -          $this->assertTrue($node_revision_access->checkAccess($revision, $case['account'], $case['op']), "{$this->map[$case['op']]} granted.");
   +          $this->assertTrue($revision->access($case['op'], $case['account']), "{$this->map[$case['op']]} granted.");
            }
            else {
   -          $this->assertFalse($node_revision_access->checkAccess($revision, $case['account'], $case['op']), "{$this->map[$case['op']]} not granted.");
   +          $this->assertFalse($revision->access($case['op'], $case['account']), "{$this->map[$case['op']]} not granted.");
            }
   

   wondering a bit why both have existing test coverage that we are converting and a new unit test? do we need both? if the unit test is complete, could we remove the functional test, maybe in a follow-up? The existing test could also explicitly become a legacy test here to test the BC layer in the old access check? Those deprecation messages do not seem to be tested yet, only the constructors of the access handlers.

Agree with @Berdir in comment #139.2, that is hard to understand what is going on, let's keep it readable please.

TLDR; This should address all of #139 except 6.

1. Yay!
2. No I completely agree, your solution is much nicer.
3. Removed
4. I really hope not! I have stripped them back
5. Have tried to fix up any camelCase in the patch
6. I would like to move this to a follow-up. I agree there may be some overlap but the changes here to NodeRevisionPermissionsTest seem to just be to get it passing with the changes. There honestly seems to be a LOT of overlap around revision access testing already. Maybe we don't even need the unit test?

Interesting, I expected test fails due to deprecation messages, but this looks like we are missing permission checks fo revert/delete revision operations?

My guess is on administer nodes, which is a very weird permission with an unclear scope and various usages that are wrong leftovers after the bypass permission was split off. Do the tests actually click on those links? Could that be broken on HEAD? that the links are visible but lead to access denied?

You've already stripped them, but another emphasis on code like this:

$revert_permission = $account->hasPermission("revert $type revisions") || $account->hasPermission('revert all revisions') || $account->hasPermission('administer nodes') || $node->access('revert revision');

Please don't do this, it completely kills access modules' ability to change the outcome of this check. Ideally, everything should be passed on to the code in $node->access() and all permissions should be checked there. If we need to change things up, we can do so by simply implementing hook_entity_access_alter()

The first failure (in LayoutBuilderContentModerationIntegrationTest) sets up a user that has the revert bundle_with_section_field revisionspermission, so yeah it looks like we're missing permission checks in the access control handler. Will be looking into it today.

Nope! We just needed to check the revision itself, not the node. This was breaking things because it was being short circuited by the fact that it was always checking revert/delete access on the default revision.

Ah, that does make sense, nice catch. The previous code was wrong then as well, we just didn't notice it as the hardcoded permission checks were sufficient.

Checking every revision is quite an overhead, but I think there's no way around that. This would allow use cases like preventing delete access on certain revisions which was not possible before.

I think there are only two things left?

1) Decide what to do with the existing and new test coverage (my point 6). Still a bit unsure about that.
2) Test coverage of the BC layers of the old access checks.

I still think that making the existing test methods legacy and check for the deprecation message might solve both problems at once. we would need to make only minimal changes to that existing test and could combine it with a todo/follow-up issue to decide if the test should be rewritten or removed when we remove the BC layer in d10? Not sure if it will be accepted by a core committer if we don't make that decision now though.

Un-checkmarking default issue credit status for #79 and #81 as they are just screenshots of git apply.

This patch reverts the changes to NodeRevisionPermissionsTest, marks it as @legacy and checks for deprecations. Is that all that was needed in #148?

In the last hour I've gone through the code and tested locally. This patch seems clean and finished.

I think #150 does address the 2 things left in #148. And making an issue to follow up that will remove the deprecated code in d10 and decide there how we will handle the old test code. We can make this issue when this lands.

It would be really awesome if we can get this in. And since I see no issues on the code, and there is no open feedback afaik. Settings this RTBC and crossing my fingers :)

Fixes for #152

Thank you everyone for getting it across the finish line.

Just a fun fact Lee created and fixed the issue with zero patches uploaded :D.

Also published https://www.drupal.org/node/3214171 as it is about code changes.

Amazing work!

Great to see this get committed, now onto #2350939: Implement a generic revision UI, hopefully we can get that done in time for 9.4/10.0!

@AaronMcHale 9.3.0?

@nevergone the alpha deadline for 9.3 has just passed which means all new features, like this one, will need to go into 9.4/10.0.

Correct, I'm pretty sure @nevergone was asking about #2350939: Implement a generic revision UI per #164 :)
But yeah, my comment was incredibly unclear, sorry about that.