From 11ef44a7f767000930a2588dc4d71fb59fe6ea04 Mon Sep 17 00:00:00 2001
From: xjm <xjm@65776.no-reply.drupal.org>
Date: Wed, 16 Aug 2017 12:10:35 -0500
Subject: [PATCH] SA-CORE-2017-004 by arshadcn, amateescu, Wim Leers, Berdir,
 Lendude, dawehner, klausi, pwolanin, xjm, mpdonadio, mlhess, larowlan, milesw

(cherry picked from commit 65cef2e9f5c37c7b0d1b70627779bd47d060e650)
---
 .../Entity/EntityAccessControlHandler.php     | 16 +++-
 core/modules/comment/src/Entity/Comment.php   | 23 ++++--
 .../tests/src/Unit/Entity/CommentLockTest.php |  4 -
 .../Comment/CommentResourceTestBase.php       | 33 +++++++++
 .../modules/entity_test/entity_test.module    | 10 +++
 .../src/Entity/EntityTestNoUuid.php           | 29 ++++++++
 .../src/Controller/ViewAjaxController.php     |  2 +-
 core/modules/views/src/Tests/ViewAjaxTest.php | 12 ++-
 .../Controller/ViewAjaxControllerTest.php     | 74 ++++++++++++++-----
 .../Entity/EntityAccessControlHandlerTest.php | 70 ++++++++++++++++++
 10 files changed, 238 insertions(+), 35 deletions(-)
 create mode 100644 core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoUuid.php

diff --git a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
index 3b16d1cddbc3..ac364115ebd6 100644
--- a/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -66,7 +66,19 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
       $operation = 'view';
     }
 
-    if (($return = $this->getCache($entity->uuid(), $operation, $langcode, $account)) !== NULL) {
+    // If an entity does not have a UUID, either from not being set or from not
+    // having them, use the 'entity type:ID' pattern as the cache $cid.
+    $cid = $entity->uuid() ?: $entity->getEntityTypeId() . ':' . $entity->id();
+
+    // If the entity is revisionable, then append the revision ID to allow
+    // individual revisions to have specific access control and be cached
+    // separately.
+    if ($entity instanceof RevisionableInterface) {
+      /** @var $entity \Drupal\Core\Entity\RevisionableInterface */
+      $cid .= ':' . $entity->getRevisionId();
+    }
+
+    if (($return = $this->getCache($cid, $operation, $langcode, $account)) !== NULL) {
       // Cache hit, no work necessary.
       return $return_as_object ? $return : $return->isAllowed();
     }
@@ -92,7 +104,7 @@ public function access(EntityInterface $entity, $operation, AccountInterface $ac
     if (!$return->isForbidden()) {
       $return = $return->orIf($this->checkAccess($entity, $operation, $account));
     }
-    $result = $this->setCache($return, $entity->uuid(), $operation, $langcode, $account);
+    $result = $this->setCache($return, $cid, $operation, $langcode, $account);
     return $return_as_object ? $result : $result->isAllowed();
   }
 
diff --git a/core/modules/comment/src/Entity/Comment.php b/core/modules/comment/src/Entity/Comment.php
index 821b30d0bab2..c57e247df96a 100644
--- a/core/modules/comment/src/Entity/Comment.php
+++ b/core/modules/comment/src/Entity/Comment.php
@@ -82,14 +82,6 @@ class Comment extends ContentEntityBase implements CommentInterface {
   public function preSave(EntityStorageInterface $storage) {
     parent::preSave($storage);
 
-    if (is_null($this->get('status')->value)) {
-      if (\Drupal::currentUser()->hasPermission('skip comment approval')) {
-        $this->setPublished();
-      }
-      else {
-        $this->setUnpublished();
-      }
-    }
     if ($this->isNew()) {
       // Add the comment to database. This next section builds the thread field.
       // @see \Drupal\comment\CommentViewBuilder::buildComponents()
@@ -238,6 +230,9 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
 
     $fields['langcode']->setDescription(t('The comment language code.'));
 
+    // Set the default value callback for the status field.
+    $fields['status']->setDefaultValueCallback('Drupal\comment\Entity\Comment::getDefaultStatus');
+
     $fields['pid'] = BaseFieldDefinition::create('entity_reference')
       ->setLabel(t('Parent ID'))
       ->setDescription(t('The parent comment ID if this is a reply to a comment.'))
@@ -559,4 +554,16 @@ public function getTypeId() {
     return $this->bundle();
   }
 
+  /**
+   * Default value callback for 'status' base field definition.
+   *
+   * @see ::baseFieldDefinitions()
+   *
+   * @return bool
+   *  TRUE if the comment should be published, FALSE otherwise.
+   */
+  public static function getDefaultStatus() {
+    return \Drupal::currentUser()->hasPermission('skip comment approval') ? CommentInterface::PUBLISHED : CommentInterface::NOT_PUBLISHED;
+  }
+
 }
diff --git a/core/modules/comment/tests/src/Unit/Entity/CommentLockTest.php b/core/modules/comment/tests/src/Unit/Entity/CommentLockTest.php
index e724dc6b89c5..9dbe6d36eba7 100644
--- a/core/modules/comment/tests/src/Unit/Entity/CommentLockTest.php
+++ b/core/modules/comment/tests/src/Unit/Entity/CommentLockTest.php
@@ -81,10 +81,6 @@ public function testLocks() {
     $comment->expects($this->any())
       ->method('getEntityType')
       ->will($this->returnValue($entity_type));
-    $comment->expects($this->at(1))
-      ->method('get')
-      ->with('status')
-      ->will($this->returnValue((object) ['value' => NULL]));
     $storage = $this->getMock('Drupal\comment\CommentStorageInterface');
 
     // preSave() should acquire the lock. (This is what's really being tested.)
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
index ea705de8a41c..5b5101445a59 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/Comment/CommentResourceTestBase.php
@@ -324,4 +324,37 @@ protected function getExpectedUnauthorizedAccessMessage($method) {
     }
   }
 
+  /**
+   * Tests POSTing a comment with and without 'skip comment approval'
+   */
+  public function testPostSkipCommentApproval() {
+    $this->initAuthentication();
+    $this->provisionEntityResource();
+    $this->setUpAuthorization('POST');
+
+    // Create request.
+    $request_options = [];
+    $request_options[RequestOptions::HEADERS]['Accept'] = static::$mimeType;
+    $request_options[RequestOptions::HEADERS]['Content-Type'] = static::$mimeType;
+    $request_options = array_merge_recursive($request_options, $this->getAuthenticationRequestOptions('POST'));
+    $request_options[RequestOptions::BODY] = $this->serializer->encode($this->getNormalizedPostEntity(), static::$format);
+
+    $url = $this->getEntityResourcePostUrl()->setOption('query', ['_format' => static::$format]);
+
+    // Status should be FALSE when posting as anonymous.
+    $response = $this->request('POST', $url, $request_options);
+    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), static::$format);
+    $this->assertResourceResponse(201, FALSE, $response);
+    $this->assertFalse($unserialized->getStatus());
+
+    // Grant anonymous permission to skip comment approval.
+    $this->grantPermissionsToTestedRole(['skip comment approval']);
+
+    // Status should be TRUE when posting as anonymous and skip comment approval.
+    $response = $this->request('POST', $url, $request_options);
+    $unserialized = $this->serializer->deserialize((string) $response->getBody(), get_class($this->entity), static::$format);
+    $this->assertResourceResponse(201, FALSE, $response);
+    $this->assertTrue($unserialized->getStatus());
+  }
+
 }
diff --git a/core/modules/system/tests/modules/entity_test/entity_test.module b/core/modules/system/tests/modules/entity_test/entity_test.module
index 4d35275db8e9..bcf55f48b270 100644
--- a/core/modules/system/tests/modules/entity_test/entity_test.module
+++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -737,6 +737,16 @@ function entity_test_entity_access(EntityInterface $entity, $operation, AccountI
     return AccessResult::allowed();
   }
 
+  // Create specific labels to allow or deny access based on certain test
+  // conditions.
+  // @see \Drupal\KernelTests\Core\Entity\EntityAccessControlHandlerTest
+  if ($entity->label() == 'Accessible') {
+    return AccessResult::allowed();
+  }
+  if ($entity->label() == 'Inaccessible') {
+    return AccessResult::forbidden();
+  }
+
   // Uncacheable because the access result depends on a State key-value pair and
   // might therefore change at any time.
   $condition = \Drupal::state()->get("entity_test_entity_access.{$operation}." . $entity->id(), FALSE);
diff --git a/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoUuid.php b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoUuid.php
new file mode 100644
index 000000000000..8d0ddac4824c
--- /dev/null
+++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestNoUuid.php
@@ -0,0 +1,29 @@
+<?php
+
+namespace Drupal\entity_test\Entity;
+
+/**
+ * Test entity class with revisions but without UUIDs.
+ *
+ * @ContentEntityType(
+ *   id = "entity_test_no_uuid",
+ *   label = @Translation("Test entity without UUID"),
+ *   handlers = {
+ *     "access" = "Drupal\entity_test\EntityTestAccessControlHandler",
+ *   },
+ *   base_table = "entity_test_no_uuid",
+ *   revision_table = "entity_test_no_uuid_revision",
+ *   admin_permission = "administer entity_test content",
+ *   persistent_cache = FALSE,
+ *   entity_keys = {
+ *     "id" = "id",
+ *     "revision" = "vid",
+ *     "bundle" = "type",
+ *     "label" = "name",
+ *     "langcode" = "langcode",
+ *   },
+ * )
+ */
+class EntityTestNoUuid extends EntityTest {
+
+}
diff --git a/core/modules/views/src/Controller/ViewAjaxController.php b/core/modules/views/src/Controller/ViewAjaxController.php
index c40cf8576607..d204766f28db 100644
--- a/core/modules/views/src/Controller/ViewAjaxController.php
+++ b/core/modules/views/src/Controller/ViewAjaxController.php
@@ -142,7 +142,7 @@ public function ajaxView(Request $request) {
         throw new NotFoundHttpException();
       }
       $view = $this->executableFactory->get($entity);
-      if ($view && $view->access($display_id)) {
+      if ($view && $view->access($display_id) && $view->setDisplay($display_id) && $view->display_handler->getOption('use_ajax')) {
         $response->setView($view);
         // Fix the current path for paging.
         if (!empty($path)) {
diff --git a/core/modules/views/src/Tests/ViewAjaxTest.php b/core/modules/views/src/Tests/ViewAjaxTest.php
index b8b2730e6edc..5a966a8f76ba 100644
--- a/core/modules/views/src/Tests/ViewAjaxTest.php
+++ b/core/modules/views/src/Tests/ViewAjaxTest.php
@@ -17,7 +17,7 @@ class ViewAjaxTest extends ViewTestBase {
    *
    * @var array
    */
-  public static $testViews = ['test_ajax_view'];
+  public static $testViews = ['test_ajax_view', 'test_view'];
 
   protected function setUp() {
     parent::setUp();
@@ -61,4 +61,14 @@ public function testAjaxView() {
     $this->assertEqual(count($result), 2, 'Ensure that two items are rendered in the HTML.');
   }
 
+  /**
+   * Ensures that non-ajax view cannot be accessed via an ajax HTTP request.
+   */
+  public function testNonAjaxViewViaAjax() {
+    $this->drupalPost('views/ajax', '', ['view_name' => 'test_ajax_view', 'view_display_id' => 'default'], ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
+    $this->assertResponse(200);
+    $this->drupalPost('views/ajax', '', ['view_name' => 'test_view', 'view_display_id' => 'default'], ['query' => [MainContentViewSubscriber::WRAPPER_FORMAT => 'drupal_ajax']]);
+    $this->assertResponse(403);
+  }
+
 }
diff --git a/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
index f766975e488b..a02f098cda73 100644
--- a/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
+++ b/core/modules/views/tests/src/Unit/Controller/ViewAjaxControllerTest.php
@@ -18,6 +18,9 @@
  */
 class ViewAjaxControllerTest extends UnitTestCase {
 
+  const USE_AJAX = TRUE;
+  const USE_NO_AJAX = FALSE;
+
   /**
    * The mocked view entity storage.
    *
@@ -186,23 +189,6 @@ public function testAjaxView() {
 
     list($view, $executable) = $this->setupValidMocks();
 
-    $display_handler = $this->getMockBuilder('Drupal\views\Plugin\views\display\DisplayPluginBase')
-      ->disableOriginalConstructor()
-      ->getMock();
-    // Ensure that the pager element is not set.
-    $display_handler->expects($this->never())
-      ->method('setOption');
-
-    $display_collection = $this->getMockBuilder('Drupal\views\DisplayPluginCollection')
-      ->disableOriginalConstructor()
-      ->getMock();
-    $display_collection->expects($this->any())
-      ->method('get')
-      ->with('page_1')
-      ->will($this->returnValue($display_handler));
-
-    $executable->displayHandlers = $display_collection;
-
     $this->redirectDestination->expects($this->atLeastOnce())
       ->method('set')
       ->with('/test-page?type=article');
@@ -215,6 +201,24 @@ public function testAjaxView() {
     $this->assertViewResultCommand($response);
   }
 
+  /**
+   * Tests a valid view without ajax enabled.
+   */
+  public function testAjaxViewWithoutAjax() {
+    $request = new Request();
+    $request->request->set('view_name', 'test_view');
+    $request->request->set('view_display_id', 'page_1');
+    $request->request->set('view_path', '/test-page');
+    $request->request->set('_wrapper_format', 'ajax');
+    $request->request->set('ajax_page_state', 'drupal.settings[]');
+    $request->request->set('type', 'article');
+
+    $this->setupValidMocks(static::USE_NO_AJAX);
+
+    $this->setExpectedException(AccessDeniedHttpException::class);
+    $this->viewAjaxController->ajaxView($request);
+  }
+
   /**
    * Tests a valid view with arguments.
    */
@@ -297,8 +301,15 @@ public function testAjaxViewWithPager() {
 
   /**
    * Sets up a bunch of valid mocks like the view entity and executable.
+   *
+   * @param bool $use_ajax
+   *   Whether the 'use_ajax' option is set on the view display. Defaults to
+   *   using ajax (TRUE).
+   *
+   * @return array
+   *   A pair of view storage entity and executable.
    */
-  protected function setupValidMocks() {
+  protected function setupValidMocks($use_ajax = self::USE_AJAX) {
     $view = $this->getMockBuilder('Drupal\views\Entity\View')
       ->disableOriginalConstructor()
       ->getMock();
@@ -314,7 +325,10 @@ protected function setupValidMocks() {
     $executable->expects($this->once())
       ->method('access')
       ->will($this->returnValue(TRUE));
-    $executable->expects($this->once())
+    $executable->expects($this->any())
+      ->method('setDisplay')
+      ->willReturn(TRUE);
+    $executable->expects($this->atMost(1))
       ->method('preview')
       ->will($this->returnValue(['#markup' => 'View result']));
 
@@ -323,6 +337,28 @@ protected function setupValidMocks() {
       ->with($view)
       ->will($this->returnValue($executable));
 
+    $display_handler = $this->getMockBuilder('Drupal\views\Plugin\views\display\DisplayPluginBase')
+      ->disableOriginalConstructor()
+      ->getMock();
+    // Ensure that the pager element is not set.
+    $display_handler->expects($this->never())
+      ->method('setOption');
+    $display_handler->expects($this->any())
+      ->method('getOption')
+      ->with('use_ajax')
+      ->willReturn($use_ajax);
+
+    $display_collection = $this->getMockBuilder('Drupal\views\DisplayPluginCollection')
+      ->disableOriginalConstructor()
+      ->getMock();
+    $display_collection->expects($this->any())
+      ->method('get')
+      ->with('page_1')
+      ->will($this->returnValue($display_handler));
+
+    $executable->display_handler = $display_handler;
+    $executable->displayHandlers = $display_collection;
+
     return [$view, $executable];
   }
 
diff --git a/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php b/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
index d7644b205954..b3af27fba28b 100644
--- a/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
+++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityAccessControlHandlerTest.php
@@ -9,7 +9,9 @@
 use Drupal\Core\Session\AnonymousUserSession;
 use Drupal\entity_test\Entity\EntityTest;
 use Drupal\entity_test\Entity\EntityTestDefaultAccess;
+use Drupal\entity_test\Entity\EntityTestNoUuid;
 use Drupal\entity_test\Entity\EntityTestLabel;
+use Drupal\entity_test\Entity\EntityTestRev;
 use Drupal\language\Entity\ConfigurableLanguage;
 use Drupal\user\Entity\User;
 
@@ -20,6 +22,16 @@
  */
 class EntityAccessControlHandlerTest extends EntityLanguageTestBase {
 
+  /**
+   * {@inheritdoc}
+   */
+  public function setUp() {
+    parent::setUp();
+
+    $this->installEntitySchema('entity_test_no_uuid');
+    $this->installEntitySchema('entity_test_rev');
+  }
+
   /**
    * Asserts entity access correctly grants or denies access.
    */
@@ -199,6 +211,64 @@ public function testEntityTranslationAccess() {
     ], $translation);
   }
 
+  /**
+   * Ensures the static access cache works correctly in the absence of an UUID.
+   *
+   * @see entity_test_entity_access()
+   */
+  public function testEntityWithoutUuidAccessCache() {
+    $account = $this->createUser();
+
+    $entity1 = EntityTestNoUuid::create([
+      'name' => 'Accessible',
+    ]);
+    $entity1->save();
+
+    $entity2 = EntityTestNoUuid::create([
+      'name' => 'Inaccessible',
+    ]);
+    $entity2->save();
+
+    $this->assertTrue($entity1->access('delete', $account), 'Entity 1 can be deleted.');
+    $this->assertFalse($entity2->access('delete', $account), 'Entity 2 CANNOT be deleted.');
+
+    $entity1
+      ->setName('Inaccessible')
+      ->setNewRevision();
+    $entity1->save();
+
+    $this->assertFalse($entity1->access('delete', $account), 'Entity 1 revision 2 CANNOT be deleted.');
+  }
+
+  /**
+   * Ensures the static access cache works correctly with a UUID and revisions.
+   *
+   * @see entity_test_entity_access()
+   */
+  public function testEntityWithUuidAccessCache() {
+    $account = $this->createUser();
+
+    $entity1 = EntityTestRev::create([
+      'name' => 'Accessible',
+    ]);
+    $entity1->save();
+
+    $entity2 = EntityTestRev::create([
+      'name' => 'Inaccessible',
+    ]);
+    $entity2->save();
+
+    $this->assertTrue($entity1->access('delete', $account), 'Entity 1 can be deleted.');
+    $this->assertFalse($entity2->access('delete', $account), 'Entity 2 CANNOT be deleted.');
+
+    $entity1
+      ->setName('Inaccessible')
+      ->setNewRevision();
+    $entity1->save();
+
+    $this->assertFalse($entity1->access('delete', $account), 'Entity 1 revision 2 CANNOT be deleted.');
+  }
+
   /**
    * Tests hook invocations.
    */
-- 
GitLab