Problem/Motivation

The Drupal\block\BlockPluginInterface::access() method (and it's implementation in Drupal\block\BlockBase)
does not allow to check access for a specific account, but just for the global user.

This may prevent BlockAccessController from working correctly, if access for an account is checked.
I stumbled over this in the refactoring of the AccessControllerInterface in #1947892: Improve DX with EntityAccessControllerInterface

Proposed resolution

Add \Drupal\Core\Session\AccountInterface argument to BlockPluginInterface::access() method
And change implementation of BlockBase::access()

Remaining tasks

Extend interface
Alter implementations
Update BlockAccessController ( #1947892: Improve DX with EntityAccessControllerInterface )

User interface changes

None

API changes

Change BlockPluginInterface::access() to

public function access(\Drupal\Core\Session\AccountInterface $account);

#1947892: Improve DX with EntityAccessControllerInterface

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

theduke’s picture

Status: Needs work » Needs review
FileSize
1.92 KB

This patch extends the interface and updates the implementation in BlockBase.

jibran’s picture

Issue tags: +Blocks-Layouts

Tagging

theduke’s picture

I forgot the @param phpdoc...

EclipseGc’s picture

Status: Needs review » Postponed

I'd prefer we solve this by embracing Condition plugins for access and visibility. We'll know what, if any, changes are necessary in the interface/base classes at that point, and not before. :-(

Eclipse

tstoeckler’s picture

Status: Postponed » Reviewed & tested by the community

Well, this is a bug fix, and pretty straightforward one on top of that. So I would suggest to just go ahead and fix it now. I don't feel strongly about this, though, so if you want to set it back to postponed, I won't fight it.

EclipseGc’s picture

Status: Reviewed & tested by the community » Postponed

I would really prefer we focus our energies on the conditions stuff. I'm not saying that this doesn't need to happen, I'm saying we need to see the implementation first because it may in fact need to be a very different approach since conditions will need more than just the user passed in.

Eclipse

tstoeckler’s picture

Right. The point is that this simply makes it consistent with other access controllers and access functions.

EclipseGc’s picture

Yes, but it doesn't make it consistent with Condition Plugins, which is why I'm making this point.

Eclipse

sun’s picture

Status: Postponed » Needs work

@tstoeckler is right, this patch just simply follows what we're doing in all other access APIs already.

We should move forward with this patch as-is. In case condition plugins will introduce a different pattern, then we can still rethink. But until then, the proposed change seems to be 100% accurate and required for D8 to be released.

That said: Never use $user as function argument. It is always $account. Marking needs work for that.

EclipseGc’s picture

Status: Needs work » Postponed

I'm re-postponing this on #1927608: Remove the tight coupling between Block Plugins and Block Entities. That HAS to go in before we go changing any other interactions of the plugin/entity stuff here. Once that's in I think we can come back and look at this and justifying it better in the light of where things are going after that patch.

Eclipse

tstoeckler’s picture

Status: Postponed » Active
benjy’s picture

Issue tags: +Needs reroll
benjy’s picture

Status: Active » Needs review
Issue tags: -Needs reroll
FileSize
1.97 KB

I just re-did this patch because it seems a lot of stuff has changed around this part of the codebase.

Status: Needs review » Needs work

The last submitted patch, blockinterface-1951386-13.patch, failed testing.

tim.plunkett’s picture

Title: Extend BlockInterface::access to allow passing in an account » Extend BlockPluginInterface::access to allow passing in an account
@@ -11,6 +11,7 @@
+use Drupal\user\Plugin\Core\Entity\User;

@@ -75,7 +76,7 @@ public function setConfigurationValue($key, $value) {
+  public function access(User $user = null) {

Should be AccountInterface, and $account

benjy’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Blocks-Layouts
FileSize
15.05 KB

@timplunkett, yeah I did think that but I was trying to make the patch the same as the previous one.

I've updated it to AccountInterface, I've also updated the prototypes of all classes that were extending BlockBase and anything that was using the global user object now uses $account.

There are a few errors with the User tests that still need to be resolved but it's a start.

benjy’s picture

Issue tags: +Needs tests, +Blocks-Layouts

Not sure what happened to the tags...

benjy’s picture

This patch also replaces user_access() with $account->hasPermission() inside access()

tim.plunkett’s picture

@@ -39,12 +40,14 @@ public function settings();
+   *   (optional) User entity to check access for.

It's not optional and its not necessarily a user, its the account

But you might as well wait for the test results, just in case anything broke.

Status: Needs review » Needs work

The last submitted patch, blockinterface-1951386-18.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
17.3 KB

Fixed the two I missed which broke the tests and the comment pointed out by @tim.plunkett.

tim.plunkett’s picture

  1. @@ -75,7 +76,7 @@ public function setConfigurationValue($key, $value) {
    +  public function access(AccountInterface $account = null) {
    

    = NULL should go away

  2. @@ -39,12 +40,14 @@ public function settings();
    +   *   The Account Interface.
    

    The user session for which to check access.
    is the comment used elsewhere.

benjy’s picture

Fixed feedback in 22.

tim.plunkett’s picture

@@ -32,7 +33,7 @@ class SystemHelpBlock extends BlockBase {
+  public function access(AccountInterface $account = null) {

I swear this wasn't in the last patch. Anyway, one more and then RTBC. And you should be including interdiffs

benjy’s picture

And again. I must be blind today! Sorry, completely forgot about the interdiffs, when you triggered the factoid in IRC I almost knew this was coming ;-)

Status: Needs review » Needs work

The last submitted patch, blockinterface-1951386-25.patch, failed testing.

andypost’s picture

andypost’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
17.9 KB
9.01 KB

Re-rolled and updated doc-blocks
We have great testing for blocks so no reason for tests

andypost’s picture

Issue summary: View changes

updated summary

catch’s picture

Issue tags: +Approved API change
EclipseGc’s picture

This seems pretty sane to me, I'd like to do one more pass on it this week with sdboyer and make sure it won't impact princess stuff at all, but I think this ok.

Eclipse

andypost’s picture

andypost’s picture

re-roll

jibran’s picture

It is RTBC for me. Let's wait for sdboyer.

dawehner’s picture

Now that we have currentUser() as part of the base plugin class we technically don't need it that urgent anymore.
Oh the other hand though this adds a lot of semanticness.

sdboyer’s picture

Status: Needs review » Reviewed & tested by the community

heated agreement. this has always been what i've wanted to see. the account being used to verify permissions should NOT be mixed with formalized notions of context, whatever those might be - those have to do with producing output. they should never be conflated with the responsibility for asserting access.

even in a situation where, say, a block is showing a user's own private dashboard - the block should expect an AccountInterface to be injected as context via a separate mechanism, then in these access checks another AccountInterface is injected, and the block performs a check that returns true IFF the context-injected AccountInterface === the one injected for the access check.

so, yes. RTBC this hot momma.

Xano’s picture

#32: drupal8.block-module.1951386-32.patch queued for re-testing.

I wonder if it would be worth adding an interface similar to AccessibleInterface, but that doesn't take an operation so we can ensure consistency for access checking.

tim.plunkett’s picture

This is only called from one place, and BlockPluginInterface is enough as is.

Xano’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

tim.plunkett’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
17.63 KB
alexpott’s picture

Priority: Normal » Major
Status: Reviewed & tested by the community » Active

Committed 49ac630 and pushed to 8.x. Thanks!

We probably need to update some change notices

andypost’s picture

The only change notice I found is https://drupal.org/node/1880620
But the one is outdated too somehow, Drupal\block\BlockBase::blockAccess() is not access() the we have now

andypost’s picture

Issue summary: View changes

Updated issue summary.

benjy’s picture

Title: Extend BlockPluginInterface::access to allow passing in an account » Change notice: Extend BlockPluginInterface::access to allow passing in an account
Issue summary: View changes
Issue tags: +Needs change record
xjm’s picture

Category: Bug report » Task
xjm’s picture

Issue tags: +Missing change record

The patch for this issue was committed on October 23, 2013, meaning that the change record updates for this issue have been outstanding for more than three months. Let's update the main blocks-as-plugins change record for this change, and check for other change records that need updating.

WRT to stuff being out of date: Blocks have more oustanding change records than pretty much anything, so that's not surprising. Eventually, the bulk of the change records around the D8 block system should be moved to https://drupal.org/node/2168137... but that's a much larger undertaking.

dawehner’s picture

xjm’s picture

Thanks! It should probably update the existing blocks change record, though, not exist as a separate one.

tim.plunkett’s picture

xjm’s picture

Title: Change notice: Extend BlockPluginInterface::access to allow passing in an account » Extend BlockPluginInterface::access to allow passing in an account
Category: Task » Bug report
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record, -Missing change record

Thanks!

Status: Fixed » Closed (fixed)

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