Skip to content
Snippets Groups Projects

Issue #3323788: Revert and Delete actions should not be available for the current revision

Closed Issue #3323788: Revert and Delete actions should not be available for the current revision
Closed Yash Rode requested to merge issue/drupal-3323788:3323788-revert-and-delete into 11.x

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
76 76 if ($entity instanceof RevisionableInterface) {
77 77 /** @var \Drupal\Core\Entity\RevisionableInterface $entity */
78 78 $cid .= ':' . $entity->getRevisionId();
79 $restricted_operations = ['delete revision', 'revert'];
80 // It is not possible to delete or revert the default and latest revision.
81 if ($entity->isLatestRevision() && $entity->isDefaultRevision() && in_array($operation, $restricted_operations, TRUE)) {
82 return AccessResult::forbidden()->cachePerPermissions();
83 }
  • Comment on lines +79 to +83

    This makes sense, but I think we should move this to the checkAccess() method. This method, access(), ensures the result is cached correctly, and this early return breaks that.

  • I tried implementing this, but we are overriding checkAccess() method, so we need to call the parent checkAccess() method and check if it returns forbidden at the start of the overwritten method. Which does not make sense as we want to deal with latest and default in parent.

    Edited by Yash Rode
  • Yash Rode changed this line in version 4 of the diff

    changed this line in version 4 of the diff

  • Please register or sign in to reply
  • Yash Rode added 1 commit

    added 1 commit

    Compare with previous version

  • Yash Rode added 20 commits

    added 20 commits

    • ca860898...9028e6c0 - 14 commits from branch project:11.x
    • 21d5ab97 - created EntityAccessControlHandlerWithRevision helper class
    • a6329890 - added doc comment
    • 8a1bf63d - Used in block content
    • 8c85ac3c - Removed helper class and added fix to existing code
    • 60584186 - moved to checkAccess
    • 4019e802 - changed exception condition in storage and reverted back to access

    Compare with previous version

  • Yash Rode added 1 commit

    added 1 commit

    • 1ea60dc8 - removed force delete and force revert and respective tests

    Compare with previous version

  • Yash Rode
  • 33 33 }
    34 34 elseif ($operation === 'revert') {
    35 35 return AccessResult::allowedIf(
    36 // Allow revert even if latest.
    37 in_array('force allow revert', $labels, TRUE) ||
    • Comment on lines -36 to -37

      Force revert won't be possible now, as we are checking the revert condition in \Drupal\Core\Entity\EntityAccessControlHandler::access which runs before this.

    • Please register or sign in to reply
  • Yash Rode
  • 33 33 }
    34 34 elseif ($operation === 'revert') {
    35 35 return AccessResult::allowedIf(
    36 // Allow revert even if latest.
    37 in_array('force allow revert', $labels, TRUE) ||
    38 36 // Disallow reverting to latest.
    39 37 (!$entity->isDefaultRevision() && !$entity->isLatestRevision() && in_array('revert', $labels, TRUE))
    40 38 );
    41 39 }
    42 40 elseif ($operation === 'delete revision') {
    43 41 return AccessResult::allowedIf(
    44 // Allow revision deletion even if latest.
    45 in_array('force allow delete revision', $labels, TRUE) ||
  • 812 812 public function deleteRevision($revision_id) {
    813 813 /** @var \Drupal\Core\Entity\ContentEntityInterface $revision */
    814 814 if ($revision = $this->loadRevision($revision_id)) {
    815 // Prevent deletion if this is the default revision.
    816 if ($revision->isDefaultRevision()) {
    817 throw new EntityStorageException('Default revision can not be deleted');
    815 // Prevent deletion if this is the default and latest revision.
    816 if ($revision->isDefaultRevision() && $revision->isLatestRevision()) {
    • This is a question for Berdir, but won't this break Content Moderation, since it has the ability to differentiate between "default" and "latest"?

    • That said...it doesn't break any existing tests in core, but that being the case, I'm not clear on how Content Moderation works.

    • Hm, this is interesting. So we already have the check for the default revision for this.

      In HEAD, creating a new pending draft of a node with content moderation shows me both the revert and delete operations on the latest revision. But I don't think there are any tests for this, in content moderation or elsewhere. But the logic here doesn't actually do that. It checks for default AND latest, which is in fact incorrect. You must never be allowed to delete the default revision, even if it's not the latest revision.

      The closest for tests is \Drupal\Tests\node\Functional\NodeRevisionsTest::testRevisions(), but that only checks that that the default revision delete/revert routes are inaccessible but this node has no forward revision, it also seems to have bogus code about an another non-default revision it creates, it does $new_node_revision->isDefaultRevision = FALSE; but that does nothing, this property is protected, this calls the magic __set() method which stores it in $this->values. The reason it works is that above, $node is actually loaded as a revision earlier, so it is already a non-default revision and resaving defaults to keep it the non-default. That line could be removed or it should use isDefaultRevision(TRUE) to make it explicit.

      The same test does show that isDefaultRevision() does update as expected when loading said old revision:

      // Confirm that this is the default revision.
          $this->assertTrue($node->isDefaultRevision(), 'Third node revision is the default one.');

      Then it reverts to a previous revision, which creates the fourth and new default revision of that node.

      After that, it reloads revision 3 and verifies that this is no longer the default.

      // Confirm that this is not the default version.
      $node = $node_storage->loadRevision($node->getRevisionId());
      $this->assertFalse($node->isDefaultRevision(), 'Third node revision is not the default one.');
      
      So the extra checks being done here are not correct, and that it doesn't fail shows our much incomplete test coverage for this. There is none for this exception message, it only exists once in core. What the test coverage should do is create a new entity, try to delete the current  revision id, except an exception. then create a new non-default revision, try to delete the default revision, still get an exception (the MR  would fail on this) and then create a new revision as default revision, and then verify that the old revision can not be deleted.
      
      The same can also easily be done in the UI on the MR, and you get the delete/revert operations next to the current revision message.
      Edited by Sascha Grossenbacher
    • Reverted the change and created a follow-up issue for missing test coverage: https://www.drupal.org/project/drupal/issues/3375168

      Edited by Yash Rode
    • Yash Rode changed this line in version 7 of the diff

      changed this line in version 7 of the diff

    • Please register or sign in to reply
  • Yash Rode added 13 commits

    added 13 commits

    • 1ea60dc8...5c126061 - 5 commits from branch project:11.x
    • 42fa610c - created EntityAccessControlHandlerWithRevision helper class
    • 297fa41f - added doc comment
    • 275eaa2c - Used in block content
    • fee4986f - Removed helper class and added fix to existing code
    • 8d56cc51 - moved to checkAccess
    • 57eabfbe - changed exception condition in storage and reverted back to access
    • 8ce433a9 - removed force delete and force revert and respective tests
    • a1d301a9 - Reverted data integrity change

    Compare with previous version

  • Yash Rode added 17 commits

    added 17 commits

    • a1d301a9...3d4e3fc7 - 8 commits from branch project:11.x
    • 5a1d0c3a - created EntityAccessControlHandlerWithRevision helper class
    • c545077b - added doc comment
    • e661be16 - Used in block content
    • e9e21508 - Removed helper class and added fix to existing code
    • e76065ad - moved to checkAccess
    • f222aa76 - changed exception condition in storage and reverted back to access
    • 4ae70951 - removed force delete and force revert and respective tests
    • 414c9891 - Reverted data integrity change
    • 3bc7a640 - removed latest

    Compare with previous version

  • Yash Rode added 2 commits

    added 2 commits

    Compare with previous version

  • Yash Rode added 1 commit

    added 1 commit

    Compare with previous version

  • Yash Rode added 1 commit

    added 1 commit

    Compare with previous version

  • Yash Rode added 1 commit

    added 1 commit

    Compare with previous version

  • 57 57 protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    58 58 assert($entity instanceof BlockContentInterface);
    59 59 $bundle = $entity->bundle();
    60 $forbidIfNotDefaultAndLatest = fn (): AccessResultInterface => AccessResult::forbiddenIf($entity->isDefaultRevision() && $entity->isLatestRevision());
    61 60 $forbidIfNotReusable = fn (): AccessResultInterface => AccessResult::forbiddenIf($entity->isReusable() === FALSE, sprintf('Block content must be reusable to use `%s` operation', $operation));
  • Adam Bramley added 15 commits

    added 15 commits

    Compare with previous version

  • Yash Rode added 16 commits

    added 16 commits

    Compare with previous version

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading