Issue #2470489 by jidrone, chx, ankithashetty, smustgrave, quietone, xjm:...

Issue #2470489 by jidrone, chx, ankithashetty, smustgrave, quietone, xjm: NodeRevisionDeleteForm::submitForm() has a SQL query and is not checking access on redirect

core/modules/node/src/Form/NodeRevisionDeleteForm.php

@@ -3,6 +3,7 @@
 namespace Drupal\node\Form;
 
 use Drupal\Core\Database\Connection;
+use Drupal\Core\Access\AccessManagerInterface;
 use Drupal\Core\Datetime\DateFormatterInterface;
 use Drupal\Core\Entity\EntityStorageInterface;
 use Drupal\Core\Form\ConfirmFormBase;
@@ -39,6 +40,13 @@ class NodeRevisionDeleteForm extends ConfirmFormBase {
    */
   protected $nodeTypeStorage;
 
+  /**
+   * The access manager.
+   *
+   * @var \Drupal\Core\Access\AccessManagerInterface
+   */
+  protected $accessManager;
+
   /**
    * The database connection.
    *
@@ -60,16 +68,21 @@ class NodeRevisionDeleteForm extends ConfirmFormBase {
    *   The node storage.
    * @param \Drupal\Core\Entity\EntityStorageInterface $node_type_storage
    *   The node type storage.
-   * @param \Drupal\Core\Database\Connection $connection
-   *   The database connection.
+   * @param \Drupal\Core\Access\AccessManagerInterface|\Drupal\Core\Database\Connection $access_manager
+   *   The access manager.
    * @param \Drupal\Core\Datetime\DateFormatterInterface $date_formatter
    *   The date formatter service.
    */
-  public function __construct(EntityStorageInterface $node_storage, EntityStorageInterface $node_type_storage, Connection $connection, DateFormatterInterface $date_formatter) {
+  public function __construct(EntityStorageInterface $node_storage, EntityStorageInterface $node_type_storage, AccessManagerInterface|Connection $access_manager, DateFormatterInterface $date_formatter) {
     $this->nodeStorage = $node_storage;
     $this->nodeTypeStorage = $node_type_storage;
-    $this->connection = $connection;
+    $this->accessManager = $access_manager;
     $this->dateFormatter = $date_formatter;
+    if ($access_manager instanceof Connection) {
+      $this->connection = $access_manager;
+      $this->accessManager = func_get_arg(3);
+      @trigger_error('Calling ' . __CLASS__ . '::_construct() with the $connection argument is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3343754', E_USER_DEPRECATED);
+    }
   }
 
   /**
@@ -80,7 +93,7 @@ public static function create(ContainerInterface $container) {
     return new static(
       $entity_type_manager->getStorage('node'),
       $entity_type_manager->getStorage('node_type'),
-      $container->get('database'),
+      $container->get('access_manager'),
       $container->get('date.formatter')
     );
   }
@@ -139,16 +152,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
         '@type' => $node_type,
         '%title' => $this->revision->label(),
       ]));
-    $form_state->setRedirect(
-      'entity.node.canonical',
-      ['node' => $this->revision->id()]
-    );
-    if ($this->connection->query('SELECT COUNT(DISTINCT [vid]) FROM {node_field_revision} WHERE [nid] = :nid', [':nid' => $this->revision->id()])->fetchField() > 1) {
-      $form_state->setRedirect(
-        'entity.node.version_history',
-        ['node' => $this->revision->id()]
-      );
+    // Set redirect to the revisions history page.
+    $route_name = 'entity.node.version_history';
+    $parameters = ['node' => $this->revision->id()];
+    // If no revisions found, or the user does not have access to the revisions
+    // page, then redirect to the canonical node page instead.
+    if (!$this->accessManager->checkNamedRoute($route_name, $parameters) || count($this->nodeStorage->revisionIds($this->revision)) === 1) {
+      $route_name = 'entity.node.canonical';
     }
+    $form_state->setRedirect($route_name, $parameters);
   }
 
 }

core/modules/node/tests/src/Functional/NodeRevisionsTest.php

@@ -216,6 +216,29 @@ public function testRevisions() {
     $this->submitForm([], 'Revert');
     $this->assertSession()->pageTextContains("Basic page {$nodes[2]->label()} has been reverted to the revision from {$this->container->get('date.formatter')->format($old_revision_date)}.");
 
+    // Confirm user is redirected depending on the remaining revisions,
+    // when a revision is deleted.
+    $existing_revision_ids = $node_storage->revisionIds($node);
+    // Delete all revision except last 3.
+    $remaining_revision_ids = array_slice($existing_revision_ids, -3, 3);
+    foreach ($existing_revision_ids as $revision_id) {
+      if (!in_array($revision_id, $remaining_revision_ids)) {
+        $node_storage->deleteRevision($revision_id);
+      }
+    }
+
+    // Confirm user was redirected to revisions history page.
+    $this->drupalGet("node/" . $node->id() . "/revisions/" . $remaining_revision_ids[0] . "/delete");
+    $this->submitForm([], 'Delete');
+    $this->assertSession()->pageTextContains("Revisions for {$nodes[2]->label()}");
+    $this->assertSession()->pageTextNotContains($nodes[2]->body->value);
+
+    // Confirm user was redirected to the node page.
+    $this->drupalGet("node/" . $node->id() . "/revisions/" . $remaining_revision_ids[1] . "/delete");
+    $this->submitForm([], 'Delete');
+    $this->assertSession()->pageTextNotContains("Revisions for {$nodes[2]->label()}");
+    $this->assertSession()->pageTextContains($nodes[2]->body->value);
+
     // Make a new revision and set it to not be default.
     // This will create a new revision that is not "front facing".
     $new_node_revision = clone $node;

core/modules/node/tests/src/Kernel/NodeDeprecationTest.php

@@ -3,6 +3,10 @@
 namespace Drupal\Tests\node\Kernel;
 
 use Drupal\KernelTests\KernelTestBase;
+use Drupal\Core\Database\Connection;
+use Drupal\Core\Datetime\DateFormatterInterface;
+use Drupal\Core\Entity\EntityStorageInterface;
+use Drupal\node\Form\NodeRevisionDeleteForm;
 
 /**
  * Tests the deprecations in the node.module file.
@@ -50,4 +54,17 @@ public function testNodeTypeUpdateNodesDeprecation(): void {
     node_type_update_nodes(1, 2);
   }
 
+  /**
+   * Tests the deprecation of NodeRevisionDeleteForm constructor.
+   */
+  public function testNodeRevisionDeleteFormConstructorDeprecation(): void {
+    $this->expectDeprecation('Calling Drupal\node\Form\NodeRevisionDeleteForm::_construct() with the $connection argument is deprecated in drupal:10.1.0 and is removed from drupal:11.0.0. See https://www.drupal.org/node/3343754');
+    new NodeRevisionDeleteForm(
+      $this->createMock(EntityStorageInterface::class),
+      $this->createMock(EntityStorageInterface::class),
+      $this->createMock(Connection::class),
+      $this->createMock(DateFormatterInterface::class),
+    );
+  }
+
 }