Issue #3231992 by phenaproxima: Make the staged sites/default writable before cleaning

automatic_updates.services.yml

@@ -14,9 +14,11 @@ services:
     arguments:
       ['@automatic_updates.composer_runner', '@automatic_updates.file_system' ]
   automatic_updates.cleaner:
-    class: PhpTuf\ComposerStager\Domain\Cleaner
+    class: Drupal\automatic_updates\ComposerStager\Cleaner
     arguments:
-      ['@automatic_updates.file_system' ]
+      - '@automatic_updates.file_system'
+      - '%site.path%'
+      - '@automatic_updates.path_locator'
   automatic_updates.committer:
     class: PhpTuf\ComposerStager\Domain\Committer
     arguments:
@@ -90,3 +92,4 @@ services:
     class: Drupal\automatic_updates\PathLocator
     arguments:
       - '@config.factory'
+      - '%app.root%'

src/ComposerStager/Cleaner.php

+<?php
+
+namespace Drupal\automatic_updates\ComposerStager;
+
+use Drupal\automatic_updates\PathLocator;
+use PhpTuf\ComposerStager\Domain\Cleaner as ComposerStagerCleaner;
+use PhpTuf\ComposerStager\Domain\CleanerInterface;
+use PhpTuf\ComposerStager\Domain\Output\ProcessOutputCallbackInterface;
+use PhpTuf\ComposerStager\Infrastructure\Filesystem\FilesystemInterface;
+use Symfony\Component\Filesystem\Filesystem;
+
+/**
+ * Defines a cleaner service that makes the staged site directory writable.
+ */
+class Cleaner implements CleanerInterface {
+
+  /**
+   * The decorated cleaner service.
+   *
+   * @var \PhpTuf\ComposerStager\Domain\CleanerInterface
+   */
+  protected $decorated;
+
+  /**
+   * The current site path, without leading or trailing slashes.
+   *
+   * @var string
+   */
+  protected $sitePath;
+
+  /**
+   * The path locator service.
+   *
+   * @var \Drupal\automatic_updates\PathLocator
+   */
+  protected $pathLocator;
+
+  /**
+   * Constructs a Cleaner object.
+   *
+   * @param \PhpTuf\ComposerStager\Infrastructure\Filesystem\FilesystemInterface $file_system
+   *   The Composer Stager file system service.
+   * @param string $site_path
+   *   The current site path (e.g., 'sites/default'), without leading or
+   *   trailing slashes.
+   * @param \Drupal\automatic_updates\PathLocator $locator
+   *   The path locator service.
+   */
+  public function __construct(FilesystemInterface $file_system, string $site_path, PathLocator $locator) {
+    // @todo Inject the decorated cleaner once the Composer Stager wiring has
+    // been moved into a separate module.
+    $this->decorated = new ComposerStagerCleaner($file_system);
+    $this->sitePath = $site_path;
+    $this->pathLocator = $locator;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function clean(string $stagingDir, ?ProcessOutputCallbackInterface $callback = NULL, ?int $timeout = 120): void {
+    // Ensure that the staged site directory is writable so we can delete it.
+    $site_dir = implode(DIRECTORY_SEPARATOR, [
+      $stagingDir,
+      $this->pathLocator->getWebRoot() ?: '.',
+      $this->sitePath,
+    ]);
+
+    if ($this->directoryExists($site_dir)) {
+      (new Filesystem())->chmod($site_dir, 0777);
+    }
+    $this->decorated->clean($stagingDir, $callback, $timeout);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function directoryExists(string $stagingDir): bool {
+    return $this->decorated->directoryExists($stagingDir);
+  }
+
+}

src/PathLocator.php

@@ -18,14 +18,24 @@ class PathLocator {
    */
   protected $configFactory;
 
+  /**
+   * The absolute path of the running Drupal code base.
+   *
+   * @var string
+   */
+  protected $appRoot;
+
   /**
    * Constructs a PathLocator object.
    *
    * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory
    *   The config factory service.
+   * @param string $app_root
+   *   The absolute path of the running Drupal code base.
    */
-  public function __construct(ConfigFactoryInterface $config_factory) {
+  public function __construct(ConfigFactoryInterface $config_factory, string $app_root) {
     $this->configFactory = $config_factory;
+    $this->appRoot = $app_root;
   }
 
   /**
@@ -41,8 +51,13 @@ class PathLocator {
   /**
    * Returns the path of the directory where updates should be staged.
    *
+   * This directory may be made world-writeable for clean-up, so it should be
+   * somewhere that doesn't put the Drupal installation at risk.
+   *
    * @return string
    *   The absolute path of the directory where updates should be staged.
+   *
+   * @see \Drupal\automatic_updates\ComposerStager\Cleaner::clean()
    */
   public function getStageDirectory(): string {
     // Append the site ID to the directory in order to support parallel test
@@ -76,4 +91,17 @@ class PathLocator {
     return dirname($reflector->getFileName(), 2);
   }
 
+  /**
+   * Returns the path of the Drupal installation, relative to the project root.
+   *
+   * @return string
+   *   The path of the Drupal installation, relative to the project root and
+   *   without leading or trailing slashes. Will return an empty string if the
+   *   project root and Drupal root are the same.
+   */
+  public function getWebRoot(): string {
+    $web_root = str_replace($this->getProjectRoot(), NULL, $this->appRoot);
+    return trim($web_root, DIRECTORY_SEPARATOR);
+  }
+
 }

tests/src/Functional/ExclusionsTest.php → tests/src/Functional/FileSystemOperationsTest.php

@@ -2,17 +2,18 @@
 
 namespace Drupal\Tests\automatic_updates\Functional;
 
+use Drupal\automatic_updates\ComposerStager\Cleaner;
 use Drupal\automatic_updates\PathLocator;
 use Drupal\automatic_updates\Updater;
 use Drupal\Core\Site\Settings;
 use Drupal\Tests\BrowserTestBase;
 
 /**
- * Tests exclusion of certain files and directories from the staging area.
+ * Tests handling of files and directories during an update.
  *
  * @group automatic_updates
  */
-class ExclusionsTest extends BrowserTestBase {
+class FileSystemOperationsTest extends BrowserTestBase {
 
   /**
    * {@inheritdoc}
@@ -25,37 +26,69 @@ class ExclusionsTest extends BrowserTestBase {
   protected $defaultTheme = 'stark';
 
   /**
-   * Tests that certain files and directories are not staged.
+   * The updater service under test.
    *
-   * @covers \Drupal\automatic_updates\Updater::getExclusions
+   * @var \Drupal\automatic_updates\Updater
    */
-  public function testExclusions(): void {
-    $stage_dir = "$this->siteDirectory/stage";
+  private $updater;
+
+  /**
+   * The full path of the staging directory.
+   *
+   * @var string
+   */
+  protected $stageDir;
 
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+
+    // Create a mocked path locator that uses the fake site fixture as its
+    // active directory, and has a staging area within the site directory for
+    // this test.
+    $drupal_root = $this->getDrupalRoot();
     /** @var \Drupal\automatic_updates\PathLocator|\Prophecy\Prophecy\ObjectProphecy $locator */
     $locator = $this->prophesize(PathLocator::class);
     $locator->getActiveDirectory()->willReturn(__DIR__ . '/../../fixtures/fake-site');
-    $locator->getStageDirectory()->willReturn($stage_dir);
-    $locator->getProjectRoot()->willReturn($this->getDrupalRoot());
+    $this->stageDir = implode(DIRECTORY_SEPARATOR, [
+      $drupal_root,
+      $this->siteDirectory,
+      'stage',
+    ]);
+    $locator->getStageDirectory()->willReturn($this->stageDir);
+    $locator->getProjectRoot()->willReturn($drupal_root);
+    $locator->getWebRoot()->willReturn('');
+
+    // Create a cleaner that uses 'sites/default' as its site path, since it
+    // will otherwise default to the site path being used for the test site,
+    // which doesn't exist in the fake site fixture.
+    $cleaner = new Cleaner(
+      $this->container->get('automatic_updates.file_system'),
+      'sites/default',
+      $locator->reveal()
+    );
 
-    $updater = new Updater(
+    $this->updater = new Updater(
       $this->container->get('state'),
       $this->container->get('string_translation'),
       $this->container->get('automatic_updates.beginner'),
       $this->container->get('automatic_updates.stager'),
-      $this->container->get('automatic_updates.cleaner'),
+      $cleaner,
       $this->container->get('automatic_updates.committer'),
       $this->container->get('event_dispatcher'),
       $locator->reveal()
     );
 
+    // Use the public and private files directories in the fake site fixture.
     $settings = Settings::getAll();
     $settings['file_public_path'] = 'files/public';
     $settings['file_private_path'] = 'files/private';
     new Settings($settings);
 
     // Updater::begin() will trigger update validators, such as
-    // \Drupal\automatic_updates\Event\UpdateVersionSubscriber, that need to
+    // \Drupal\automatic_updates\Validator\UpdateVersionValidator, that need to
     // fetch release metadata. We need to ensure that those HTTP request(s)
     // succeed, so set them up to point to our fake release metadata.
     $this->config('update_test.settings')
@@ -69,20 +102,43 @@ class ExclusionsTest extends BrowserTestBase {
     $this->config('update_test.settings')
       ->set('system_info.#all.version', '9.8.0')
       ->save();
+  }
 
-    $updater->begin(['drupal' => '9.8.1']);
-    $this->assertFileDoesNotExist("$stage_dir/sites/default/settings.php");
-    $this->assertFileDoesNotExist("$stage_dir/sites/default/settings.local.php");
-    $this->assertFileDoesNotExist("$stage_dir/sites/default/services.yml");
+  /**
+   * Tests that certain files and directories are not staged.
+   *
+   * @covers \Drupal\automatic_updates\Updater::getExclusions
+   */
+  public function testExclusions(): void {
+    $this->updater->begin(['drupal' => '9.8.1']);
+    $this->assertFileDoesNotExist("$this->stageDir/sites/default/settings.php");
+    $this->assertFileDoesNotExist("$this->stageDir/sites/default/settings.local.php");
+    $this->assertFileDoesNotExist("$this->stageDir/sites/default/services.yml");
     // A file in sites/default, that isn't one of the site-specific settings
     // files, should be staged.
-    $this->assertFileExists("$stage_dir/sites/default/staged.txt");
-    $this->assertDirectoryDoesNotExist("$stage_dir/sites/simpletest");
-    $this->assertDirectoryDoesNotExist("$stage_dir/files/public");
-    $this->assertDirectoryDoesNotExist("$stage_dir/files/private");
+    $this->assertFileExists("$this->stageDir/sites/default/staged.txt");
+    $this->assertDirectoryDoesNotExist("$this->stageDir/sites/simpletest");
+    $this->assertDirectoryDoesNotExist("$this->stageDir/files/public");
+    $this->assertDirectoryDoesNotExist("$this->stageDir/files/private");
     // A file that's in the general files directory, but not in the public or
     // private directories, should be staged.
-    $this->assertFileExists("$stage_dir/files/staged.txt");
+    $this->assertFileExists("$this->stageDir/files/staged.txt");
+  }
+
+  /**
+   * Tests that the staging directory is properly cleaned up.
+   *
+   * @covers \Drupal\automatic_updates\Cleaner
+   */
+  public function testClean(): void {
+    $this->updater->begin(['drupal' => '9.8.1']);
+    // Make the staged site directory read-only, so we can test that it will be
+    // made writable on clean-up.
+    $this->assertTrue(chmod("$this->stageDir/sites/default", 0400));
+    $this->assertNotIsWritable("$this->stageDir/sites/default/staged.txt");
+    // If the site directory is not writable, this will throw an exception.
+    $this->updater->clean();
+    $this->assertDirectoryDoesNotExist($this->stageDir);
   }
 
 }