Issue #2453341 by Wim Leers: Contact forms don't have necessary cache contexts...

Issue #2453341 by Wim Leers: Contact forms don't have necessary cache contexts & tags; flood control should work on validate, not on view

core/modules/contact/src/Access/ContactPageAccess.php

@@ -62,7 +62,7 @@ public function access(UserInterface $user, AccountInterface $account) {
 
     // Anonymous users cannot have contact forms.
     if ($contact_account->isAnonymous()) {
-      return AccessResult::forbidden();
+      return AccessResult::forbidden()->cachePerPermissions();
     }
 
     // Users may not contact themselves.

core/modules/contact/src/Controller/ContactController.php

@@ -7,16 +7,12 @@
 
 namespace Drupal\contact\Controller;
 
-use Drupal\Core\Cache\Cache;
 use Drupal\Core\Controller\ControllerBase;
-use Drupal\Core\Datetime\DateFormatter;
-use Drupal\Core\Flood\FloodInterface;
 use Drupal\contact\ContactFormInterface;
 use Drupal\Core\Render\RendererInterface;
 use Drupal\user\UserInterface;
 use Drupal\Component\Utility\SafeMarkup;
 use Symfony\Component\DependencyInjection\ContainerInterface;
-use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
 use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;
 
 /**
@@ -24,20 +20,6 @@
  */
 class ContactController extends ControllerBase {
 
-  /**
-   * The flood service.
-   *
-   * @var \Drupal\Core\Flood\FloodInterface
-   */
-  protected $flood;
-
-  /**
-   * The date formatter service.
-   *
-   * @var \Drupal\Core\Datetime\DateFormatter
-   */
-  protected $dateFormatter;
-
   /**
    * The renderer.
    *
@@ -48,16 +30,10 @@ class ContactController extends ControllerBase {
   /**
    * Constructs a ContactController object.
    *
-   * @param \Drupal\Core\Flood\FloodInterface $flood
-   *   The flood service.
-   * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
-   *   The date service.
    * @param \Drupal\Core\Render\RendererInterface $renderer
    *   The renderer.
    */
-  public function __construct(FloodInterface $flood, DateFormatter $date_formatter, RendererInterface $renderer) {
-    $this->flood = $flood;
-    $this->dateFormatter = $date_formatter;
+  public function __construct(RendererInterface $renderer) {
     $this->renderer = $renderer;
   }
 
@@ -66,8 +42,6 @@ public function __construct(FloodInterface $flood, DateFormatter $date_formatter
    */
   public static function create(ContainerInterface $container) {
     return new static(
-      $container->get('flood'),
-      $container->get('date.formatter'),
       $container->get('renderer')
     );
   }
@@ -86,10 +60,6 @@ public static function create(ContainerInterface $container) {
    *   contact form.
    */
   public function contactSitePage(ContactFormInterface $contact_form = NULL) {
-    // Check if flood control has been activated for sending emails.
-    if (!$this->currentUser()->hasPermission('administer contact forms')) {
-      $this->contactFloodControl();
-    }
     $config = $this->config('contact.settings');
 
     // Use the default form if no form has been passed.
@@ -118,6 +88,7 @@ public function contactSitePage(ContactFormInterface $contact_form = NULL) {
 
     $form = $this->entityFormBuilder()->getForm($message);
     $form['#title'] = SafeMarkup::checkPlain($contact_form->label());
+    $form['#cache']['contexts'][] = 'user.permissions';
     $this->renderer->addDependency($form, $config);
     return $form;
   }
@@ -141,11 +112,6 @@ public function contactPersonalPage(UserInterface $user) {
       throw new NotFoundHttpException();
     }
 
-    // Check if flood control has been activated for sending emails.
-    if (!$this->currentUser()->hasPermission('administer contact forms') && !$this->currentUser()->hasPermission('administer users')) {
-      $this->contactFloodControl();
-    }
-
     $message = $this->entityManager()->getStorage('contact_message')->create(array(
       'contact_form' => 'personal',
       'recipient' => $user->id(),
@@ -153,24 +119,8 @@ public function contactPersonalPage(UserInterface $user) {
 
     $form = $this->entityFormBuilder()->getForm($message);
     $form['#title'] = $this->t('Contact @username', array('@username' => $user->getUsername()));
+    $form['#cache']['contexts'][] = 'user.permissions';
     return $form;
   }
 
-  /**
-   * Throws an exception if the current user triggers flood control.
-   *
-   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
-   */
-  protected function contactFloodControl() {
-    $limit = $this->config('contact.settings')->get('flood.limit');
-    $interval = $this->config('contact.settings')->get('flood.interval');
-    if (!$this->flood->isAllowed('contact', $limit, $interval)) {
-      drupal_set_message($this->t('You cannot send more than %limit messages in @interval. Try again later.', array(
-        '%limit' => $limit,
-        '@interval' => $this->dateFormatter->formatInterval($interval),
-      )), 'error');
-      throw new AccessDeniedHttpException();
-    }
-  }
-
 }

core/modules/contact/src/MessageForm.php

@@ -8,6 +8,7 @@
 namespace Drupal\contact;
 
 use Drupal\Component\Utility\SafeMarkup;
+use Drupal\Core\Datetime\DateFormatter;
 use Drupal\Core\Entity\ContentEntityForm;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Flood\FloodInterface;
@@ -49,6 +50,13 @@ class MessageForm extends ContentEntityForm {
    */
   protected $mailHandler;
 
+  /**
+   * The date formatter service.
+   *
+   * @var \Drupal\Core\Datetime\DateFormatter
+   */
+  protected $dateFormatter;
+
   /**
    * Constructs a MessageForm object.
    *
@@ -60,12 +68,15 @@ class MessageForm extends ContentEntityForm {
    *   The language manager service.
    * @param \Drupal\contact\MailHandlerInterface $mail_handler
    *   The contact mail handler service.
+   * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
+   *   The date service.
    */
-  public function __construct(EntityManagerInterface $entity_manager, FloodInterface $flood, LanguageManagerInterface $language_manager, MailHandlerInterface $mail_handler) {
+  public function __construct(EntityManagerInterface $entity_manager, FloodInterface $flood, LanguageManagerInterface $language_manager, MailHandlerInterface $mail_handler, DateFormatter $date_formatter) {
     parent::__construct($entity_manager);
     $this->flood = $flood;
     $this->languageManager = $language_manager;
     $this->mailHandler = $mail_handler;
+    $this->dateFormatter = $date_formatter;
   }
 
   /**
@@ -76,7 +87,8 @@ public static function create(ContainerInterface $container) {
       $container->get('entity.manager'),
       $container->get('flood'),
       $container->get('language_manager'),
-      $container->get('contact.mail_handler')
+      $container->get('contact.mail_handler'),
+      $container->get('date.formatter')
     );
   }
 
@@ -172,6 +184,28 @@ public function preview(array $form, FormStateInterface $form_state) {
     $form_state->setRebuild();
   }
 
+  /**
+   * {@inheritdoc}
+   */
+  public function validate(array $form, FormStateInterface $form_state) {
+    parent::validate($form, $form_state);
+
+    $message = $this->entity;
+
+    // Check if flood control has been activated for sending emails.
+    if (!$this->currentUser()->hasPermission('administer contact forms') && (!$message->isPersonal() || !$this->currentUser()->hasPermission('administer users'))) {
+      $limit = $this->config('contact.settings')->get('flood.limit');
+      $interval = $this->config('contact.settings')->get('flood.interval');
+
+      if (!$this->flood->isAllowed('contact', $limit, $interval)) {
+        $form_state->setErrorByName('', $this->t('You cannot send more than %limit messages in @interval. Try again later.', array(
+          '%limit' => $limit,
+          '@interval' => $this->dateFormatter->formatInterval($interval),
+        )));
+      }
+    }
+  }
+
   /**
    * {@inheritdoc}
    */

core/modules/contact/src/Tests/ContactPersonalTest.php

@@ -152,11 +152,13 @@ function testPersonalContactAccess() {
     // Test that anonymous users can access admin user's contact form.
     $this->drupalGet('user/' . $this->adminUser->id() . '/contact');
     $this->assertResponse(200);
+    $this->assertCacheContext('user.permissions');
 
     // Revoke the personal contact permission for the anonymous user.
     user_role_revoke_permissions(RoleInterface::ANONYMOUS_ID, array('access user contact forms'));
     $this->drupalGet('user/' . $this->contactUser->id() . '/contact');
     $this->assertResponse(403);
+    $this->assertCacheContext('user.permissions');
     $this->drupalGet('user/' . $this->adminUser->id() . '/contact');
     $this->assertResponse(403);
 
@@ -235,7 +237,7 @@ function testPersonalContactFlood() {
     }
 
     // Submit contact form one over limit.
-    $this->drupalGet('user/' . $this->contactUser->id(). '/contact');
+    $this->submitPersonalContact($this->contactUser);
     $this->assertRaw(t('You cannot send more than %number messages in @interval. Try again later.', array('%number' => $flood_limit, '@interval' => \Drupal::service('date.formatter')->formatInterval($this->config('contact.settings')->get('flood.interval')))), 'Normal user denied access to flooded contact form.');
 
     // Test that the admin user can still access the contact form even though

core/modules/contact/src/Tests/ContactSitewideTest.php

@@ -230,8 +230,7 @@ function testSiteWideContact() {
       $this->assertText(t('Your message has been sent.'));
     }
     // Submit contact form one over limit.
-    $this->drupalGet('contact');
-    $this->assertResponse(403);
+    $this->submitContact($this->randomMachineName(16), $recipients[0], $this->randomMachineName(16), $id, $this->randomMachineName(64));
     $this->assertRaw(t('You cannot send more than %number messages in @interval. Try again later.', array('%number' => $this->config('contact.settings')->get('flood.limit'), '@interval' => \Drupal::service('date.formatter')->formatInterval(600))));
 
     // Test listing controller.

core/modules/simpletest/src/WebTestBase.php

@@ -2740,6 +2740,17 @@ protected function buildUrl($path, array $options = array()) {
     }
   }
 
+  /**
+   * Asserts whether an expected cache context was present in the last response.
+   *
+   * @param string $expected_cache_context
+   *   The expected cache context.
+   */
+  protected function assertCacheContext($expected_cache_context) {
+    $cache_contexts = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Contexts'));
+    $this->assertTrue(in_array($expected_cache_context, $cache_contexts), "'" . $expected_cache_context . "' is present in the X-Drupal-Cache-Contexts header.");
+  }
+
   /**
    * Asserts whether an expected cache tag was present in the last response.
    *
@@ -2748,7 +2759,7 @@ protected function buildUrl($path, array $options = array()) {
    */
   protected function assertCacheTag($expected_cache_tag) {
     $cache_tags = explode(' ', $this->drupalGetHeader('X-Drupal-Cache-Tags'));
-    $this->assertTrue(in_array($expected_cache_tag, $cache_tags));
+    $this->assertTrue(in_array($expected_cache_tag, $cache_tags), "'" . $expected_cache_tag . "' is present in the X-Drupal-Cache-Tags header.");
   }
 
 }