Issue #3363725 by tedbow, kunal.sachdev, omkar.podey, phenaproxima, Wim Leers:...

Issue #3363725 by tedbow, kunal.sachdev, omkar.podey, phenaproxima, Wim Leers: Disallow unattended updates if performed by automated_cron

automatic_updates.module

@@ -9,6 +9,7 @@ declare(strict_types = 1);
 
 use Drupal\automatic_updates\BatchProcessor;
 use Drupal\automatic_updates\CronUpdateStage;
+use Drupal\automatic_updates\Validator\AutomatedCronDisabledValidator;
 use Drupal\Core\Routing\RouteMatchInterface;
 use Drupal\automatic_updates\Validation\AdminStatusCheckMessages;
 use Drupal\Core\Url;
@@ -172,6 +173,18 @@ function automatic_updates_cron() {
     return;
   }
 
+  /** @var \Drupal\automatic_updates\Validator\AutomatedCronDisabledValidator $automated_cron_validator */
+  $automated_cron_validator = \Drupal::service(AutomatedCronDisabledValidator::class);
+  if ($automated_cron_validator->hasTerminateBeenCalled()) {
+    // Running updates and status checks will not work during kernel
+    // termination.
+    \Drupal::logger('automatic_updates')
+      ->info('Unattended automatic updates were triggered by Automated Cron, which is not supported. No update was performed. See the <a href=":url">status report</a> for more information.', [
+        ':url' => Url::fromRoute('system.status')->toString(),
+      ]);
+    return;
+  }
+
   /** @var \Drupal\automatic_updates\Validation\StatusChecker $status_checker */
   $status_checker = \Drupal::service('automatic_updates.status_checker');
   $last_results = $status_checker->getResults();

automatic_updates.services.yml

@@ -67,6 +67,9 @@ services:
       $lock: '@lock'
     tags:
       - { name: event_subscriber }
+  Drupal\automatic_updates\Validator\AutomatedCronDisabledValidator:
+    tags:
+      - { name: event_subscriber }
   automatic_updates.validator.staged_database_updates:
     class: Drupal\automatic_updates\Validator\StagedDatabaseUpdateValidator
     tags:

src/Validator/AutomatedCronDisabledValidator.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\automatic_updates\Validator;
+
+use Drupal\automatic_updates\CronUpdateStage;
+use Drupal\Core\Extension\ModuleHandlerInterface;
+use Drupal\Core\StringTranslation\StringTranslationTrait;
+use Drupal\package_manager\Event\StatusCheckEvent;
+use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpKernel\KernelEvents;
+
+/**
+ * Ensures that updates cannot be triggered by Automated Cron.
+ *
+ * @internal
+ *   This is an internal part of Automatic Updates and may be changed or removed
+ *   at any time without warning. External code should not interact with this
+ *   class.
+ */
+final class AutomatedCronDisabledValidator implements EventSubscriberInterface {
+
+  use StringTranslationTrait;
+
+  /**
+   * Flags whether the KernelEvents::TERMINATE event has been dispatched.
+   *
+   * @var bool
+   */
+  private bool $terminateCalled = FALSE;
+
+  /**
+   * AutomatedCronDisabledValidator constructor.
+   *
+   * @param \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler
+   *   The module handler.
+   */
+  public function __construct(
+    private readonly ModuleHandlerInterface $moduleHandler,
+  ) {}
+
+  /**
+   * Checks that Automated Cron is not going to trigger unattended updates.
+   *
+   * @param \Drupal\package_manager\Event\StatusCheckEvent $event
+   *   The event being handled.
+   */
+  public function validateStatusCheck(StatusCheckEvent $event): void {
+    if ($event->stage instanceof CronUpdateStage && $this->moduleHandler->moduleExists('automated_cron')) {
+      $event->addWarning([
+        $this->t('This site has the Automated Cron module installed. To use unattended automatic updates, please configure cron manually on your hosting environment. The Automatic Updates module will not do anything if it is triggered by Automated Cron. See the <a href=":url">Automated Cron documentation</a> for information.', [
+          ':url' => 'https://www.drupal.org/docs/administering-a-drupal-site/cron-automated-tasks/cron-automated-tasks-overview#s-more-reliable-enable-cron-using-external-trigger',
+        ]),
+      ]);
+    }
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public static function getSubscribedEvents(): array {
+    return [
+      StatusCheckEvent::class => 'validateStatusCheck',
+      // Ensure this runs before
+      // \Drupal\automated_cron\EventSubscriber\AutomatedCron::onTerminate().
+      KernelEvents::TERMINATE => ['setTerminateCalled', PHP_INT_MAX],
+    ];
+  }
+
+  /**
+   * Sets a flag is when the kernel terminates.
+   */
+  public function setTerminateCalled(): void {
+    $this->terminateCalled = TRUE;
+  }
+
+  /**
+   * Determines whether the kernel has terminated.
+   *
+   * @return bool
+   *   TRUE if the kernel has terminated (i.e., KernelEvents::TERMINATE has been
+   *   handled), otherwise FALSE.
+   */
+  public function hasTerminateBeenCalled(): bool {
+    return $this->terminateCalled;
+  }
+
+}

src/Validator/CronFrequencyValidator.php

@@ -7,11 +7,9 @@ namespace Drupal\automatic_updates\Validator;
 use Drupal\automatic_updates\CronUpdateStage;
 use Drupal\Component\Datetime\TimeInterface;
 use Drupal\Core\Config\ConfigFactoryInterface;
-use Drupal\Core\Extension\ModuleHandlerInterface;
 use Drupal\Core\Lock\LockBackendInterface;
 use Drupal\Core\State\StateInterface;
 use Drupal\Core\StringTranslation\StringTranslationTrait;
-use Drupal\Core\Url;
 use Drupal\package_manager\Event\StatusCheckEvent;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
 
@@ -23,7 +21,7 @@ use Symfony\Component\EventDispatcher\EventSubscriberInterface;
  *   at any time without warning. External code should not interact with this
  *   class.
  */
-class CronFrequencyValidator implements EventSubscriberInterface {
+final class CronFrequencyValidator implements EventSubscriberInterface {
 
   use StringTranslationTrait;
 
@@ -59,8 +57,6 @@ class CronFrequencyValidator implements EventSubscriberInterface {
    *
    * @param \Drupal\Core\Config\ConfigFactoryInterface $configFactory
    *   The config factory service.
-   * @param \Drupal\Core\Extension\ModuleHandlerInterface $moduleHandler
-   *   The module handler.
    * @param \Drupal\Core\State\StateInterface $state
    *   The state service.
    * @param \Drupal\Component\Datetime\TimeInterface $time
@@ -70,19 +66,18 @@ class CronFrequencyValidator implements EventSubscriberInterface {
    */
   public function __construct(
     private readonly ConfigFactoryInterface $configFactory,
-    private readonly ModuleHandlerInterface $moduleHandler,
     private readonly StateInterface $state,
     private readonly TimeInterface $time,
     private readonly LockBackendInterface $lock,
   ) {}
 
   /**
-   * Validates that cron runs frequently enough to perform automatic updates.
+   * Validates the cron frequency according to the last cron run time.
    *
    * @param \Drupal\package_manager\Event\StatusCheckEvent $event
    *   The event object.
    */
-  public function checkCronFrequency(StatusCheckEvent $event): void {
+  public function validateLastCronRun(StatusCheckEvent $event): void {
     // We only want to do this check if the stage belongs to Automatic Updates.
     if (!$event->stage instanceof CronUpdateStage) {
       return;
@@ -92,43 +87,6 @@ class CronFrequencyValidator implements EventSubscriberInterface {
     if ($event->stage->getMode() === CronUpdateStage::DISABLED) {
       return;
     }
-    elseif ($this->moduleHandler->moduleExists('automated_cron')) {
-      $this->validateAutomatedCron($event);
-    }
-    else {
-      $this->validateLastCronRun($event);
-    }
-  }
-
-  /**
-   * Validates the cron frequency according to Automated Cron settings.
-   *
-   * @param \Drupal\package_manager\Event\StatusCheckEvent $event
-   *   The event object.
-   */
-  protected function validateAutomatedCron(StatusCheckEvent $event): void {
-    $message = $this->t('Cron is not set to run frequently enough. <a href=":configure">Configure it</a> to run at least every @frequency hours or disable automated cron and run it via an external scheduling system.', [
-      ':configure' => Url::fromRoute('system.cron_settings')->toString(),
-      '@frequency' => static::SUGGESTED_INTERVAL,
-    ]);
-
-    $interval = $this->configFactory->get('automated_cron.settings')->get('interval');
-
-    if ($interval > static::ERROR_INTERVAL) {
-      $event->addError([$message]);
-    }
-    elseif ($interval > static::WARNING_INTERVAL) {
-      $event->addWarning([$message]);
-    }
-  }
-
-  /**
-   * Validates the cron frequency according to the last cron run time.
-   *
-   * @param \Drupal\package_manager\Event\StatusCheckEvent $event
-   *   The event object.
-   */
-  protected function validateLastCronRun(StatusCheckEvent $event): void {
     // If cron is running right now, cron is clearly being run recently enough!
     if (!$this->lock->lockMayBeAvailable('cron')) {
       return;
@@ -152,7 +110,7 @@ class CronFrequencyValidator implements EventSubscriberInterface {
    */
   public static function getSubscribedEvents(): array {
     return [
-      StatusCheckEvent::class => 'checkCronFrequency',
+      StatusCheckEvent::class => 'validateLastCronRun',
     ];
   }
 

tests/src/Functional/AutomatedCronDisabledValidatorTest.php

+<?php
+
+namespace Drupal\Tests\automatic_updates\Functional;
+
+use Drupal\automatic_updates\CronUpdateStage;
+
+/**
+ * Tests that updates are not run by Automated Cron.
+ *
+ * @covers \Drupal\automatic_updates\Validator\AutomatedCronDisabledValidator
+ * @group automatic_updates
+ */
+class AutomatedCronDisabledValidatorTest extends AutomaticUpdatesFunctionalTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['dblog', 'automated_cron'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+    $this->drupalLogin($this->createUser(['access site reports']));
+  }
+
+  /**
+   * Tests that automatic updates are not triggered by Automated Cron.
+   */
+  public function testAutomatedCronUpdate() {
+    // Delete the last cron run time, to ensure that Automated Cron will run.
+    $this->container->get('state')->delete('system.cron_last');
+    $this->config('automatic_updates.settings')
+      ->set('unattended.level', CronUpdateStage::ALL)
+      ->save();
+
+    $this->drupalGet('user');
+    // The `drupalGet()` will not wait for the HTTP kernel to terminate (i.e.,
+    // the `KernelEvents::TERMINATE` event) to complete. Although this event
+    // will likely already be completed, wait 1 second to avoid random test
+    // failures.
+    sleep(1);
+    $this->drupalGet('admin/reports/dblog');
+    $this->assertSession()->elementAttributeContains('css', 'a[title^="Unattended"]', 'title', 'Unattended automatic updates were triggered by Automated Cron, which is not supported. No update was performed. See the status report for more information.');
+  }
+
+}

tests/src/Functional/StatusCheckTest.php

@@ -128,11 +128,6 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
   public function testStatusChecksOnStatusReport(): void {
     $assert = $this->assertSession();
     $page = $this->getSession()->getPage();
-
-    // Ensure automated_cron is disabled before installing automatic_updates.
-    // This ensures we are testing that automatic_updates runs the checkers when
-    // the module itself is installed and they weren't run on cron.
-    $this->assertFalse($this->container->get('module_handler')->moduleExists('automated_cron'));
     $this->container->get('module_installer')->install(['automatic_updates', 'automatic_updates_test']);
 
     // If the site is ready for updates, the users will see the same output
@@ -263,10 +258,6 @@ class StatusCheckTest extends AutomaticUpdatesFunctionalTestBase {
     $assert = $this->assertSession();
     $messages_section_selector = '[data-drupal-messages]';
 
-    // Ensure automated_cron is disabled before installing automatic_updates. This
-    // ensures we are testing that automatic_updates runs the checkers when the
-    // module itself is installed and they weren't run on cron.
-    $this->assertFalse($this->container->get('module_handler')->moduleExists('automated_cron'));
     $this->container->get('module_installer')->install(['automatic_updates', 'automatic_updates_test']);
 
     // If site is ready for updates no message will be displayed on admin pages.

tests/src/Kernel/StatusCheck/AutomatedCronDisabledValidatorTest.php

+<?php
+
+declare(strict_types = 1);
+
+namespace Drupal\Tests\automatic_updates\Kernel\StatusCheck;
+
+use Drupal\package_manager\ValidationResult;
+use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
+
+/**
+ * @covers \Drupal\automatic_updates\Validator\AutomatedCronDisabledValidator
+ * @group automatic_updates
+ * @internal
+ */
+class AutomatedCronDisabledValidatorTest extends AutomaticUpdatesKernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['automatic_updates', 'automated_cron'];
+
+  /**
+   * Tests that cron updates are not allowed if Automated Cron is enabled.
+   */
+  public function testCronUpdateNotAllowed(): void {
+    $expected_results = [
+      ValidationResult::createWarning([
+        t('This site has the Automated Cron module installed. To use unattended automatic updates, please configure cron manually on your hosting environment. The Automatic Updates module will not do anything if it is triggered by Automated Cron. See the <a href=":url">Automated Cron documentation</a> for information.', [
+          ':url' => 'https://www.drupal.org/docs/administering-a-drupal-site/cron-automated-tasks/cron-automated-tasks-overview#s-more-reliable-enable-cron-using-external-trigger',
+        ]),
+      ]),
+    ];
+    $this->assertCheckerResultsFromManager($expected_results, TRUE);
+
+    // Even after a cron run, we should have the same results.
+    $this->container->get('cron')->run();
+    $this->assertCheckerResultsFromManager($expected_results);
+  }
+
+}

tests/src/Kernel/StatusCheck/CronFrequencyValidatorTest.php

@@ -5,10 +5,8 @@ declare(strict_types = 1);
 namespace Drupal\Tests\automatic_updates\Kernel\StatusCheck;
 
 use Drupal\automatic_updates\CronUpdateStage;
-use Drupal\automatic_updates\Validator\CronFrequencyValidator;
 use Drupal\package_manager\ValidationResult;
 use Drupal\Tests\automatic_updates\Kernel\AutomaticUpdatesKernelTestBase;
-use PHPUnit\Framework\AssertionFailedError;
 
 /**
  * @covers \Drupal\automatic_updates\Validator\CronFrequencyValidator
@@ -43,32 +41,17 @@ class CronFrequencyValidatorTest extends AutomaticUpdatesKernelTestBase {
     $this->config('automatic_updates.settings')
       ->set('unattended.level', CronUpdateStage::DISABLED)
       ->save();
-
-    $validator = new class (
-      $this->container->get('config.factory'),
-      $this->container->get('module_handler'),
-      $this->container->get('state'),
-      $this->container->get('datetime.time'),
-      $this->container->get('lock')
-    ) extends CronFrequencyValidator {
-
-      /**
-       * {@inheritdoc}
-       */
-      protected function validateAutomatedCron($event): void {
-        throw new AssertionFailedError(__METHOD__ . '() should not have been called.');
-      }
-
-      /**
-       * {@inheritdoc}
-       */
-      protected function validateLastCronRun($event): void {
-        throw new AssertionFailedError(__METHOD__ . '() should not have been called.');
-      }
-
-    };
-    $this->container->set('automatic_updates.cron_frequency_validator', $validator);
+    $state = $this->container->get('state');
+    $state->delete('system.cron_last');
+    $state->delete('install_time');
     $this->assertCheckerResultsFromManager([], TRUE);
+    $this->config('automatic_updates.settings')
+      ->set('unattended.level', CronUpdateStage::ALL)
+      ->save();
+    $error = ValidationResult::createError([
+      t('Cron has not run recently. For more information, see the online handbook entry for <a href="https://www.drupal.org/cron">configuring cron jobs</a> to run at least every 3 hours.'),
+    ]);
+    $this->assertCheckerResultsFromManager([$error], TRUE);
   }
 
   /**
@@ -117,62 +100,4 @@ class CronFrequencyValidatorTest extends AutomaticUpdatesKernelTestBase {
     $this->assertCheckerResultsFromManager([], TRUE);
   }
 
-  /**
-   * Data provider for testAutomatedCronValidation().
-   *
-   * @return mixed[][]
-   *   The test cases.
-   */
-  public function providerAutomatedCronValidation(): array {
-    return [
-      'default configuration' => [
-        NULL,
-        [],
-      ],
-      'every 6 hours' => [
-        21600,
-        [
-          ValidationResult::createWarning([
-            t('Cron is not set to run frequently enough. <a href="/admin/config/system/cron">Configure it</a> to run at least every 3 hours or disable automated cron and run it via an external scheduling system.'),
-          ]),
-        ],
-      ],
-      'every 25 hours' => [
-        90000,
-        [
-          ValidationResult::createError([
-            t('Cron is not set to run frequently enough. <a href="/admin/config/system/cron">Configure it</a> to run at least every 3 hours or disable automated cron and run it via an external scheduling system.'),
-          ]),
-        ],
-      ],
-    ];
-  }
-
-  /**
-   * Tests validation based on Automated Cron settings.
-   *
-   * @param int|null $interval
-   *   The configured interval for Automated Cron. If NULL, the default value
-   *   will be used.
-   * @param \Drupal\package_manager\ValidationResult[] $expected_results
-   *   The expected validation results.
-   *
-   * @dataProvider providerAutomatedCronValidation
-   */
-  public function testAutomatedCronValidation(?int $interval, array $expected_results): void {
-    $this->enableModules(['automated_cron']);
-    $this->installConfig('automated_cron');
-
-    if (isset($interval)) {
-      $this->config('automated_cron.settings')
-        ->set('interval', $interval)
-        ->save();
-    }
-    $this->assertCheckerResultsFromManager($expected_results, TRUE);
-
-    // Even after running cron, we should have the same results.
-    $this->container->get('cron')->run();
-    $this->assertCheckerResultsFromManager($expected_results);
-  }
-
 }