Updated: Comment 0
Problem/Motivation
Most access checkers need the current account, as access is most of the time bound to the actual user.
Proposed resolution
Add an $account parameters to the access method, so the code would look like this:
Before
public function access(Route $route, Request $request) {
$account = $request->attributes->get('_account');
$permission = $route->getRequirement('_permission');
// @todo Replace user_access() with a correctly injected and session-using
// alternative.
// If user_access() fails, return NULL to give other checks a chance.
return $account->hasPermission($permission) ? static::ALLOW : static::DENY;
}
After
public function access(Route $route, Request $request, AccountInterface $account) {
$permission = $route->getRequirement('_permission');
// @todo Replace user_access() with a correctly injected and session-using
// alternative.
// If user_access() fails, return NULL to give other checks a chance.
return $account->hasPermission($permission) ? static::ALLOW : static::DENY;
}
Remaining tasks
User interface changes
API changes
Related Issues
Original report by @fubhy
This makes it much easier for our route access checks to properly use the currently logged in account. Now that we have it on the request we can easily forward it to our access checks.
Comment | File | Size | Author |
---|---|---|---|
#91 | access-2048223-91.patch | 90.21 KB | tim.plunkett |
#89 | access-2048223-89.patch | 90.75 KB | dawehner |
#89 | interdiff.txt | 680 bytes | dawehner |
#88 | access-2048223-88.patch | 91.41 KB | dawehner |
#88 | interdiff.txt | 7.28 KB | dawehner |
Comments
Comment #1
fubhy CreditAttribution: fubhy commentedComment #2
fubhy CreditAttribution: fubhy commentedHahaha, I guess I was trying to write on IRC when I wrote that patch :D
Comment #3
fubhy CreditAttribution: fubhy commentedRe-uploading without the reference to chx :)
Comment #5
Crell CreditAttribution: Crell commentedLet's put the account parameter as the 3rd argument for the method, and make it optional (default NULL). I think that will make this change backward-compatible. (I agree it's a good chance, but we need to minimize breakage.)
Comment #6
damiankloip CreditAttribution: damiankloip commentedI think I like that idea better, let's move that param.
Comment #8
damiankloip CreditAttribution: damiankloip commentedComment #10
dawehnerYou just missed some places.
Comment #12
dawehnerFor now we need this fix.
Comment #14
dawehnerI explicitly removed the empty AccessInterface from the interface, so it is not an api change.
Comment #16
dawehnerRerolled.
Comment #18
vijaycs85Re-roll + updating interface with $account.
Comment #20
vijaycs85seems this swap in arguments position fix most of the fails.
Comment #22
dawehnerThe problem with the patch in #20 is that this is sadly an api change.
Comment #23
dawehnerLet's move the issue a bit.
After #2062151: Create a current user service to ensure that current account is always available we have a way to properly inject the account into the access manager.
Comment #25
dawehnerMh ...
Comment #26
dawehnerThere we go.
Comment #28
dawehnerThis should fix most of the failures.
Comment #30
dawehnerSorry for all the noise, not sure whether this is nice, but I figured out why it actually fails:
So the test code triggers are route rebuild, which gets the access subscriber, so the access manager which then relies on the current user.
Potentially splitting up the access manager or just make the current user optional might be a solution.
Comment #32
dawehnerSplitted up the access subscriber from being dependent on the current user/request and one just handling the rebuilding.
Added the current user to the request aware event subscriber and pass it further to the access manager.
Comment #34
dawehnerMissing file.
Comment #36
dawehnerJust a rerole.
Comment #38
dawehnerLet's see.
Comment #40
tim.plunkettComment #41
dawehnerHere is a rerole.
------------
45e492dc05f38d697e9833fcef6bb53580aa7b94
Comment #43
dawehnerLet's upload the views fix.
Comment #45
tim.plunkettIn EntityAccessController:
This becomes strange, because $account is now always passed.
Right now we have to do
$account = $this->prepareUser($account);
at the beginning of each, which is justComment #46
YesCT CreditAttribution: YesCT commentedthis issue might provide a more generic solution to what we need to get config_translation into core. #2064697: Remove user_access from ConfigNameCheck
Comment #47
dawehnerGRRR.
Comment #48
catchBlocks #2073531: Use current user service instead of _account, remove _account from the request object, bumping to critical.
Comment #49
ParisLiakos CreditAttribution: ParisLiakos commentedok so TaxonomyTermCreateAccess moved to EntityCreateAccessCheck see #2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck
lets see how much i messed up the reroll
Comment #51
ParisLiakos CreditAttribution: ParisLiakos commentedjust a reroll
core/modules/block/lib/Drupal/block/Access/BlockThemeAccessCheck is dead thats why this is smaller
Comment #53
ParisLiakos CreditAttribution: ParisLiakos commentedOf course we have new access checkers. the drop moves!
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedComment #57
ParisLiakos CreditAttribution: ParisLiakos commentedComment #59
ParisLiakos CreditAttribution: ParisLiakos commentedthe
HandlerFilterUserNameTest
test passes locally:/now..i believe the route enhancers we have for authentication...hmm dunno, they come pretty late obviously..
i ll check tomorrow
this should fix at least 1 test
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedsynchronized FTW
Comment #63
ParisLiakos CreditAttribution: ParisLiakos commentedthe remaining failures, pass locally, so should be php5.3/enviroment specific errors
Comment #64
dawehnerwow! Just wondering why the same tests failed before but we need all the synchronized services for this issue. I totally agree that it is AWESOME that you found
all these bugs
Comment #65
ParisLiakos CreditAttribution: ParisLiakos commentedyes i agree its awesome:D
#59 was failing in access tests
in #61 those are fixed cause of the changes in the access subscriber, but we got two new in
Drupal\block\Tests\Views\DisplayBlockTest
i ll ask for a re-test in case some of them where random.
about synced: i think we should do the same for the request, but in a different issue;)
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commented#61: access_manager-2048223-61.patch queued for re-testing.
Comment #68
dawehnerUrgs, I would really like to avoid this if possible.
Comment #69
ParisLiakos CreditAttribution: ParisLiakos commentedit just says the truth:) i dont see whats bad about it. it depends on the request, so it is in request scope
Comment #70
dawehnerThis test is pretty dump, so maybe this works ... but well
Comment #72
dawehner#70: access-2048223-70.patch queued for re-testing.
Comment #74
dawehnerThis is blocked on doing something sane for #2095125: Use access constants in every access control context
Comment #75
joelpittetRe-rolled #70
Comment #76
joelpittetadding tags back, wtf
Comment #78
herom CreditAttribution: herom commentedget #75 past the install fails. also fix some Drupal → \Drupal cases.
Comment #80
herom CreditAttribution: herom commentedpatch update.
Comment #82
dawehnerSomes fixes here and there.
Comment #84
herom CreditAttribution: herom commentedanother update. hoping for green.
Comment #85
ParisLiakos CreditAttribution: ParisLiakos commentedhey its green!
lets remove this comment too
So lets revert those chnages now..unless the test ones are actually needed?
So i see we got a getAccountStub method, although its more of a mock:P
I am not sure it makes much sense to have a method here?
But anyway we should update those calls, to call the method instead
Comment #86
tim.plunkettI don't see how getAccountStub is useful. It's a one liner, just mock the interface you want.
Comment #87
dawehnerWell, the reason I added it was because this is potentially needed in a hell lot of places.
Comment #88
dawehnerIt is needed to fix the failures. If you have a look at the existing code you really wonder how this could have ever passed.
It did not worked on purpose and got broken somehow by that patch.
Fixed the other stuff.
Comment #89
dawehnerNo debug().
Comment #90
tim.plunkettNice!
This looks ready to go.
Comment #91
tim.plunkett_search_menu_access() was removed, so this needed a reroll.
Comment #92
catchWhy is the request scope added here, when we had to remove it from the current user service in #2076411: Remove the request scope from the current user service? We have a critical to add that back, would just like to know why it breaks without it or whether this is a nicety.
Otherwise looks great.
Comment #93
ParisLiakos CreditAttribution: ParisLiakos commentedi added it before #2076411: Remove the request scope from the current user service went in. now its not needed, but we wiill have to add it back when current_user get under request scope again. so i think just leaving it there its ok
Comment #94
catchHmm OK. We have that critical to add it back, so it'll all make sense again once that happens.
Patch doesn't apply any more unfortunately.
Comment #95
catch#91: access-2048223-91.patch queued for re-testing.
Comment #96
joelpittetPatch seemed to apply here still.
Comment #97
catchHmm yes my mistake locally..
Committed/pushed to 8.x, thanks! Will need a change notice.
Comment #98
dawehnerA couple of changes everywhere: https://drupal.org/node/2117181/revisions/view/2885323/2896891
https://drupal.org/node/1851490/revisions/view/2870225/2896897
Comment #98.0
dawehnertodo
Comment #99
effulgentsia CreditAttribution: effulgentsia commentedComment #100
xjm@dawehner, I guess this is "Needs review" for the change record updates you list? Do we need to add anything else?
Comment #101
dawehnerI think we already cover now all in the existing change notifications.
Comment #102
Berdir