Issue #3323788: Revert and Delete actions should not be available for the current revision
Merge request reports
Activity
added 1 commit
- 1b00cdaa - created EntityAccessControlHandlerWithRevision helper class
added 2 commits
added 1 commit
- 99f2123d - Removed helper class and added fix to existing code
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 } I tried implementing this, but we are overriding
checkAccess()
method, so we need to call the parentcheckAccess()
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 Rodechanged this line in version 4 of the diff
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
Toggle commit list-
ca860898...9028e6c0 - 14 commits from branch
added 1 commit
- 1ea60dc8 - removed force delete and force revert and respective tests
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) || 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()) { 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 GrossenbacherReverted the change and created a follow-up issue for missing test coverage: https://www.drupal.org/project/drupal/issues/3375168
Edited by Yash Rodechanged this line in version 7 of the diff
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
Toggle commit list-
1ea60dc8...5c126061 - 5 commits from branch
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
Toggle commit list-
a1d301a9...3d4e3fc7 - 8 commits from branch
added 2 commits
- Resolved by Yash Rode
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)); added 15 commits
-
c945cd59...5ea65c99 - 13 commits from branch
project:11.x
- b0591153 - Merge branch '11.x' into 3323788-revert-and-delete
- 39d8b9c0 - Fix for when $return_as_object is FALSE
-
c945cd59...5ea65c99 - 13 commits from branch
added 16 commits
- 39d8b9c0...52ca1bac - 6 earlier commits
- dcf105f2 - removed force delete and force revert and respective tests
- 46bda7fc - Reverted data integrity change
- 6940dd49 - removed latest
- a705f5bb - removed the operations array
- 11ad5221 - changed test
- 188eb786 - phpcs fix
- b3fdc3fc - cspell fix
- a710da7f - removed cachePerPermissions
- 74983399 - Fix for when $return_as_object is FALSE
- bce582b0 - Added test coverage for not returing as object
Toggle commit list