Problem/Motivation

The scope of the bypass entity access own workspace permission is to allow any users that have access to switch to a workspace to also be allowed to add and edit any content on that workspace. At the moment that permission actually don't grant the user to add and edit content on a workspace where the user is not the owner.

Proposed resolution

Fix user access with the bypass entity access own workspace permission.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Note the unnecessary use of he in this comment is being tackled in #2286655: Gender neutral language

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jeqq’s picture

Status: Active » Needs review
Issue tags: +Workflow Initiative, +DevDaysTransylvania, +DevDaysCluj
FileSize
1.61 KB

This issue reveals that the bypass permission doesn't work correctly.

jeqq’s picture

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks for figuring this out! :)

The last submitted patch, 5: 3021247-5-test-only.patch, failed testing. View results

jeqq’s picture

Title: Comment in WorkspaceBypassTest appears to be incorrect » Bypass workspace access permission is not working as expected

.

jeqq’s picture

Issue summary: View changes
jeqq’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 6253899 and pushed to 8.8.x. Thanks!
Committed a54ef47 and pushed to 8.7.x. Thanks!

I discussed this a bit with @Wim Leers and we agreed that since this is access it's important the code is clear whats happening so something like

diff --git a/core/modules/workspaces/src/EntityAccess.php b/core/modules/workspaces/src/EntityAccess.php
index 96b32472be..2fb7762b86 100644
--- a/core/modules/workspaces/src/EntityAccess.php
+++ b/core/modules/workspaces/src/EntityAccess.php
@@ -124,8 +124,10 @@ protected function bypassAccessResult(AccountInterface $account) {
     // to ALL THE THINGS! That's why this is a dangerous permission.
     $active_workspace = $this->workspaceManager->getActiveWorkspace();
 
-    return AccessResult::allowedIf($active_workspace->getOwnerId() == $account->id())->cachePerUser()->addCacheableDependency($active_workspace)
-      ->orIf(AccessResult::allowedIfHasPermission($account, 'bypass entity access own workspace'));
+    $owner_has_access = AccessResult::allowedIf($active_workspace->getOwnerId() == $account->id())
+      ->cachePerUser()->addCacheableDependency($active_workspace);
+    $access_bypass = AccessResult::allowedIfHasPermission($account, 'bypass entity access own workspace');
+    return $owner_has_access->orIf($access_bypass);
   }
 
 }

is preferable because it makes it clear where the or is happening and that cacheability is dealt with correctly. I've made this change on commit.

Also I think we should add a test case that access is denied - so something like

    // Create a new user that should be able to edit anything in the Bears
    // workspace.
    $a_n_other = $this->drupalCreateUser(['view any workspace']);
    $this->drupalLogin($a_n_other);
    $this->switchToWorkspace($bears);

    // Editor 3 should be able view but not create and edit.
    $this->drupalGet('/node/' . $ditka_bears_node_id . '/edit');
    $this->assertSession()->statusCodeEquals(403);
    $this->drupalGet('/node/' . $ditka_bears_node_id);
    $this->assertSession()->statusCodeEquals(200);

Ahhh!!! We already have coverage of this in \Drupal\Tests\workspaces\Functional\WorkspacePermissionsTest::testEditOwnWorkspace. So let's not do that here.

I think we should open a follow-up to consider merging the test with \Drupal\Tests\workspaces\Functional\WorkspaceViewTest and \Drupal\Tests\workspaces\Functional\WorkspacePermissionsTest as then it easier to spot holes in our coverage.

  • alexpott committed 6253899 on 8.8.x
    Issue #3021247 by jeqq, alexpott, amateescu: Bypass workspace access...

  • alexpott committed a54ef47 on 8.7.x
    Issue #3021247 by jeqq, alexpott, amateescu: Bypass workspace access...
alexpott’s picture

Version: 8.8.x-dev » 8.7.x-dev
rosinegrean’s picture

Issue tags: -DevDaysCluj

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.