ShortcutsBlock does not respect the 'access shortcuts' permission

Awesome that you found BlockInterface::access(), I wasn't aware this could be solved so elegantly. I had a look at BlockBase::access() and that basically just delegates to BlockBase::blockAccess() but - even though all core block plugins still do their access logic there - that is a relict of the past as the code comment in BlockBase::access() clearly indicates. See also #2375689: BlockBase::blockAccess() should return AccessResult instead of a bool. Thus, it is correct to override access() directly, and not blockAccess(), awesome!

Also awesome that you figured out the $return_as_object weirdness correctly.

One comment on the actual access checking:

+++ b/core/modules/shortcut/src/Plugin/Block/ShortcutsBlock.php
@@ -29,4 +31,20 @@ public function build() {
+
+    if ($account->hasPermission('access shortcuts')) {
+      $access = AccessResult::allowed();
+    }
+    else {
+      $access = AccessResult::forbidden();
+    }
+
+    $access->setCacheable(FALSE);

This whole hunk can be replaced with

$access = AccessResult::allowedIfHasPermission('access shortcuts');

That is not only a little bit more readble but also has the benefit of setting proper role-based cacheability, so that this check does not need to be repeated each time. (I.e. the setCacheable(FALSE) is unnecessary in this case, but if we use allowedIfHasPermission() we don't need to care about that in the first place because that handles the caching logic for us.)

Regarding tests: I would prefer introducing a new ShortcutsBlockTest which enables the shortcut block (see WebTestBase::drupalPlaceBlock()) and creates two users: one with the access shortcuts permission and one without. And then you can assert that the shortcuts block is only shown for the correct user.

@dawehner: Currently blockAccess() must return a bool so it circumvents the entire AccessResult logic. That's why I think it's currently preferred to implement access() directly, even though none of the other block plugins in core do it (see also #2). But it would be a nice idea to hide away the $return_as_object foo in access() and just let block plugins always return an AccessResult. Would probably need some committer feedback first, whether that's feasible at this point.

@pjonckiere: That $return_as_object foo is a BC-layer. Various access methods in core used to return TRUE/NULL/FALSE and it was deemed to disruptive to have them suddenly return an AccessResult object, which is far superior due to the caching logic. So receiving an AccessResult object is opt-in, which is why we have that argument. This, however, puts the burden on the implementors of the access logic (you, writing the block access in this case), which is rather unfortunate. So the line that was in patch #1 regarding the $return_as_object parameter is still needed.

Awesome, I think we are almost done here. I just have some suggestions to make the test a little bit more concise, although in terms of test coverage it's already perfect:

1. +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -327,4 +327,45 @@ private function verifyAccessShortcutsPermissionForEditPages() {
   +    $block_name = 'shortcuts';
   +    // Create a random title for the block.
   +    $title = $this->randomMachineName(8);
   +    // Get the default theme.
   +    $default_theme = $this->config('system.theme')->get('default');
   +    $edit = array(
   +      'id' => strtolower($this->randomMachineName(8)),
   +      'settings[label]' => $title,
   +    );
   ...
   +    // Assign block to sidebar first region.
   +    $edit['region'] = 'sidebar_first';
   +    $this->drupalPostForm('admin/structure/block/add/' . $block_name . '/' . $default_theme, $edit, t('Save block'));
   +    $this->assertText('The block configuration has been saved.', 'Block was saved');
   

   This code can be replaced with $block = $this->drupalPlaceBlock('shortcuts');.

   That uses the API instead of the UI to place the block, but that is fine, because we do not need to test that placing blocks works as part of this test. The method returns the block entity so you can then do $block->label() in the assertions below.

2. +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -327,4 +327,45 @@ private function verifyAccessShortcutsPermissionForEditPages() {
   +    $this->drupalGet('');
   +    $this->assertText($title, 'Block was displayed on the front page.');
   

   Although it doesn't particularly hurt, I don't think it's necessary to test that an admin user can see the block since we're testing that a "regular" user can see it just below that. So in my opinion these two lines can be removed.

3. +++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
   @@ -327,4 +327,45 @@ private function verifyAccessShortcutsPermissionForEditPages() {
   +    $this->drupalLogout();
   +
   +    // Verify that users without the 'access shortcuts' permission can see the
   +    // shortcut block.
   +    $this->drupalLogin($this->drupalCreateUser(array()));
   

   I think the drupalLogin() is not necessary here, as anonymous users should not have the access shortcuts permission either. In that case the drupalLogout() can just be moved below the comment (and maybe the comment should be adapted). Could you try that?

4. +++ b/core/modules/shortcut/src/Tests/ShortcutTestBase.php
   @@ -83,7 +83,7 @@ protected function setUp() {
   -    $this->adminUser = $this->drupalCreateUser(array('access toolbar', 'administer shortcuts', 'view the administration theme', 'create article content', 'create page content', 'access content overview', 'administer users', 'link to any page', 'edit any article content'));
   +    $this->adminUser = $this->drupalCreateUser(array('access toolbar', 'administer blocks', 'administer shortcuts', 'access shortcuts', 'view the administration theme', 'create article content', 'create page content', 'access content overview', 'administer users', 'link to any page', 'edit any article content'));
   

   If my comments above are implemented I think this change is no longer necessary.

Great work so far, thanks a lot!