SA-CORE-2017-004 by arshadcn, amateescu, Wim Leers, Berdir, Lendude, …

…dawehner, klausi, pwolanin, xjm, mpdonadio, mlhess, larowlan, milesw

(cherry picked from commit 65cef2e)

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();
   }
 

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;
+  }
+
 }

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.)

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());
+  }
+
 }

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);

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 {
+
+}

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)) {

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);
+  }
+
 }

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];
   }