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);
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#40 | block-access-1951386-40.patch | 17.63 KB | tim.plunkett |
#32 | drupal8.block-module.1951386-32.patch | 17.9 KB | andypost |
#28 | interdiff.txt | 9.01 KB | andypost |
#28 | drupal8.block-module.1951386-28.patch | 17.9 KB | andypost |
#25 | interdiff.txt | 663 bytes | benjy |
Comments
Comment #1
theduke CreditAttribution: theduke commentedThis patch extends the interface and updates the implementation in BlockBase.
Comment #2
jibranTagging
Comment #3
theduke CreditAttribution: theduke commentedI forgot the @param phpdoc...
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedI'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
Comment #5
tstoecklerWell, 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.
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedI 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
Comment #7
tstoecklerRight. The point is that this simply makes it consistent with other access controllers and access functions.
Comment #8
EclipseGc CreditAttribution: EclipseGc commentedYes, but it doesn't make it consistent with Condition Plugins, which is why I'm making this point.
Eclipse
Comment #9
sun@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.Comment #10
EclipseGc CreditAttribution: EclipseGc commentedI'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
Comment #11
tstoecklerComment #12
benjy CreditAttribution: benjy commentedComment #13
benjy CreditAttribution: benjy commentedI just re-did this patch because it seems a lot of stuff has changed around this part of the codebase.
Comment #15
tim.plunkettShould be AccountInterface, and $account
Comment #16
benjy CreditAttribution: benjy commented@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.
Comment #17
benjy CreditAttribution: benjy commentedNot sure what happened to the tags...
Comment #18
benjy CreditAttribution: benjy commentedThis patch also replaces user_access() with $account->hasPermission() inside access()
Comment #19
tim.plunkettIt'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.
Comment #21
benjy CreditAttribution: benjy commentedFixed the two I missed which broke the tests and the comment pointed out by @tim.plunkett.
Comment #22
tim.plunkett= NULL should go away
The user session for which to check access.
is the comment used elsewhere.
Comment #23
benjy CreditAttribution: benjy commentedFixed feedback in 22.
Comment #24
tim.plunkettI swear this wasn't in the last patch. Anyway, one more and then RTBC. And you should be including interdiffs
Comment #25
benjy CreditAttribution: benjy commentedAnd 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 ;-)
Comment #27
andypostWill need a re-roll after #2062023: Replace user_access() calls with $account->hasPermission() in statistics module
#1831846: Help block is broken with language path prefixes
Comment #28
andypostRe-rolled and updated doc-blocks
We have great testing for blocks so no reason for tests
Comment #28.0
andypostupdated summary
Comment #29
catchComment #30
EclipseGc CreditAttribution: EclipseGc commentedThis 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
Comment #31
andypost#28: drupal8.block-module.1951386-28.patch queued for re-testing.
Comment #32
andypostre-roll
Comment #33
jibranIt is RTBC for me. Let's wait for sdboyer.
Comment #34
dawehnerNow 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.
Comment #35
sdboyer CreditAttribution: sdboyer commentedheated 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.
Comment #36
Xano#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.
Comment #37
tim.plunkettThis is only called from one place, and BlockPluginInterface is enough as is.
Comment #38
XanoYes and no. See #2112871: Add an AccessibleInterface without $operation parameter.
Comment #39
alexpottPatch no longer applies.
Comment #40
tim.plunkettStraight reroll, use statements conflicted with #2112711: Provide an easier mechanism for using drupal_get_form() directly
Comment #41
alexpottCommitted 49ac630 and pushed to 8.x. Thanks!
We probably need to update some change notices
Comment #42
andypostThe only change notice I found is https://drupal.org/node/1880620
But the one is outdated too somehow,
Drupal\block\BlockBase::blockAccess()
is notaccess()
the we have nowComment #42.0
andypostUpdated issue summary.
Comment #43
benjy CreditAttribution: benjy commentedComment #44
xjmComment #45
xjmThe 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.
Comment #46
dawehnerhttps://drupal.org/node/2185953
Comment #47
xjmThanks! It should probably update the existing blocks change record, though, not exist as a separate one.
Comment #48
tim.plunkettI updated https://drupal.org/node/1880620/revisions/view/6690745/6889893
Comment #49
xjmThanks!