Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
You don't want to only apply mutliple access checkers via OR, but you also want to have routes which match on all of them.
Suggested syntax in the routing yml
requirements:
_conjunction: 'OR'
_access: 'TRUE'
_permission: 'Foo'
Comment | File | Size | Author |
---|---|---|---|
#53 | access_mode_all_tests-1984528-53.patch | 2.54 KB | Wim Leers |
#44 | access_manager-1984528-44.patch | 20.57 KB | ParisLiakos |
#44 | interdiff.txt | 1.02 KB | ParisLiakos |
#42 | access_manager-1984528-42.patch | 20.56 KB | dawehner |
#37 | drupal-1984528-37.patch | 19.1 KB | dawehner |
Comments
Comment #1
dawehnerStarted with some basic unit testing, though it still fails.
Comment #3
Crell CreditAttribution: Crell commentedIf we're going to do this, should we go ahead and default AND? Since that seems to be the more common case in practice?
I feel like the code would read cleaner if we moved the if statement outside the loop entirely. The nested conditionals here are just really hard to make sense of.
I'm a little uneasy using another requirement property here. Shouldn't it be in options, as, say, _access_conjunction? It's not a requirement per se, and we're already abusing requirements badly enough... :-)
Comment #4
dawehnerLet's go on here and fix the issues.
Comment #5
dawehner.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedi think we always use public for this one
also you miss the getInfo function, UnitTestCase throws a nice exception now
you could use dataproviders for this:) would be much readable i think.
Comment #8
Crell CreditAttribution: Crell commentedThis can be collapsed. It's a tautology. :-)
I think this logic is wrong. A checker can still return FALSE, which even in OR should result in access-deny.
And now that I'm reading it, we should probably split the AND and OR routines off to separate methods on this object to keep everything as simple and contained as possible. (Sorry, I didn't think of that before.)
That's only a PHPUnit feature. Which is a perfectly good reason these tests should be done in PHPUnit. :-)
Comment #9
dawehnerThis is different on PHPUnit
I tried hard but as setUp() is not called before dataproviders you can't setup objects before and yeah then there are all kind of problems.
He!
Comment #11
dawehnerEnsured that NULL does not block access.
Comment #12
damiankloip CreditAttribution: damiankloip commentedDo you think we should just return these directly instead, and not bother assigning the variable.
'AND'
We should not use the $access variable, at the moment there is a mixture of variable assignment and jsut returning the bool.
'OR'
This could just return FALSE instead.
Adds.
Comment #13
dawehnerThank you, fixed your points.
Comment #14
Crell CreditAttribution: Crell commentedI think we need to put this in the options, not in requirements. It's a configuration option of the route behavior, basically.
I don't think this is quite right. Consider 2 checkers, the first returns false and the second true. That should result in a false result, since there is at least one fail. However, in this case the first one would set $access to false, and the second would set it back to true, causing true to be returned.
Instead, $service_access FALSE should return FALSE immediately.
Also, the breaks aren't necessary. It's the end of the loop anyway.
Comment #15
dawehnerComment #16
Crell CreditAttribution: Crell commentedDaniel, chx, and I had a nice detailed discussion about this at the extended sprints. Final result:
AND and OR are replaced by they keywords ALL and ANY, respectively. I recommend an option key of _access_mode: ANY.
Instead of TRUE, NULL, FALSE, we use more semantically-correct constants ALLOW, DENY, and STOP, respectively. (Probably those belong on the access checker interface: AccessCheckerInterface::ALLOW, AccessCheckerInterface::DENY, etc.)
The logic is:
- In ANY mode, access is true iff at least one checker returns ALLOW.
- In ALL mode, access is true iff all checkers return ALLOW.
- If any checker returns STOP, access is false regardless of mode.
That is essentially what I was suggesting above, but the better naming makes it much clearer how the logic works so it's not confusing. :-)
I've attached a screen shot of the pseduo-truth-table we had on the whiteboard.
Comment #17
dawehnerI wasn't sure about our exact termonoligy, so I went with ACCEPT/DENY/KILL but we can discuss about it.
Isn't it just great that there is no problem with existing access checkers...
Comment #18
Crell CreditAttribution: Crell commentedI thought we were going to call this ALLOW, not ACCEPT?
Suggested text:
Allow: Grant access
A Checker should return this value to indicate that it grants access to a route.
Deny: Deny access
A Checker should return this value to indicate that it does not grant access to a route.
Kill: Block access
A Checker should return this value to indicate that it wants to completely block access to this route, regardless of any other access checkers. Most checkers will not need to use this value.
We need to run through all checkers, even if one returns Allow, in case a later one returns Kill. We can't just return TRUE if we find an Accept response.
Given the rename, this comment is now confusing. I suggest instead "Do not allow access if the user does not have the necessary role."
Comment #19
dawehnerThanks for the review.
I rewrote the tests to just map our table.
One possible follow up would be to replace 'TRUE' with 'ALLOW' etc. on the _access access checker.
Comment #21
tim.plunkett#19: drupal-1984528-19.patch queued for re-testing.
Comment #23
Crell CreditAttribution: Crell commentedGood idea!
Side note: I love that the way we're doing this lets the simple static:: syntax work here in all checkers. That will be great for DX.
This looks like a good use case for @dataProvider for PHPUnit.
Looks like testbot is still being janky, so will retry again momentarily.
Comment #24
Crell CreditAttribution: Crell commented#19: drupal-1984528-19.patch queued for re-testing.
Comment #25
dawehnerSee https://drupal.org/node/1984528#comment-7437470
Comment #26
damiankloip CreditAttribution: damiankloip commentedLooking pretty good, and as always, nice test additions!
'FALSE otherwise' sounds better IMO
This does a getOption() twice, wont make a massive difference but would it be better to do something like
$conjuction = $route->getOption('_access_conjuction'); if (empty($conjuction)) {$conjuction = 'AND'}
or something? You get what I mean.Couldn't these two be just be in the same condition?
Agree with Crell, after this whole new world of data providers (new for me :)), this seems like a perfect use case.
I'm fine with it being called little, but I think this might need to be changed to just 'Converts Blah to a string'
Comment #28
dawehnerI checked that with my debugger and that's actually not TRUE. getOption in total is just called twice in the full function.
Used a dataprovider now.
Comment #30
dawehner#28: drupal-1984528-28.patch queued for re-testing.
Comment #32
dawehnerLet's fallback back to OR for now and see whether everything passes with it.
Comment #33
Crell CreditAttribution: Crell commentedWell that's annoying...
Also, I thought we were going to switch to _access_mode: [ANY|ALL], not "conjunction" AND/OR?
Comment #34
dawehnerHopefully I have not done total shit at 1am.
Comment #35
tim.plunkettCurrently you are limited to only using ANY (or) for access checks, which means any route that would normally combine them, would need a custom checker.
This restores some sanity.
Comment #36
damiankloip CreditAttribution: damiankloip commentedSorry..
@dawehner, also, are you going to change conjunction to access_mode?
Comment #37
dawehnerLet's do that.
Comment #38
Crell CreditAttribution: Crell commentedWhew! Thanks, dawehner! I think we finally figured it out. :-)
Comment #39
catchReally like this one but it no longer applies.
Comment #40
catch#37: drupal-1984528-37.patch queued for re-testing.
Comment #42
dawehnerJust another rerole.
Comment #43
ParisLiakos CreditAttribution: ParisLiakos commentedusually we prefix with provider and then use the method name.eg here
providerTestCheckConjuctions
shouldnt hold this patch though
Comment #44
ParisLiakos CreditAttribution: ParisLiakos commentedand just for the sake of consistency
Comment #46
tim.plunkett#44: access_manager-1984528-44.patch queued for re-testing.
Comment #47
dawehnerBack to RTBC
Comment #48
catchCommitted/pushed to 8.x, thanks!
Comment #49
jibranI miss @webchick.
Comment #50
catchOops, thanks!
Comment #51
dawehnerDoes someone has an oppinion whether it should be added to https://drupal.org/node/1851490 or https://drupal.org/node/1800686 or there should be even a new one?
Comment #52
Crell CreditAttribution: Crell commentedUpdate the former I'd say. https://drupal.org/node/1851490 Doesn't need a new one.
Comment #53
Wim LeersChange notice was updated: https://drupal.org/node/1851490/revisions/view/2675322/2725683
However… I've just lost half a day over the changes made here. See below.
ANY
and notALL
? AFAICT nobody intended for this to happen, but the patch got committed *very* fast, without proper review it seems, after there was a reroll to just see if defaulting toANY
would make tests pass. Did we check the consequences of that? I think *everybody* is going to assume these requirements areAND
ed by default…_access_checks: 'ANY'
, not_access_mode: 'ANY'
! And that is the default, not'ALL'
(see point 1), so why is it even an example?WebTestBase
) coverage for this at all? That would also provide useful examples. If there's anything worth integration testing, then it's routing system access checks, is it not?static::(ALLOW|DENY|KILL)
?Attached is a commentless initial patch with basic integration test coverage.
Comment #54
Wim LeersAnd the thing I did not yet even mention: the change from defaulting from
ALL
toANY
has completely broken existing code. I can't believe it is our goal to have integration tests for *every* route, to ensure their declarative access checks are indeed handled correctly by the routing system. But it seems that's what we have to do.E.g. this Edit route is affected by the changed default:
Because the default has changed from
ALL
toANY
, as of this commit it means that a user without the in-place editing permission can edit any content…!This is just one example. There likely are more.
Comment #55
dawehnerJust to be sure, this was not changed, but rather some of the access checkers now implement the API properly by returning NULL/static::DENY instead of FALSE/static::KILL.
Comment #56
dawehnerSo much trouble just because noone had time to write a change notice yet.
Comment #57
Wim Leers#55:
Heh, you're, right … but only in a very subtle way.
As you can see in #2063405-1: Update all access checkers to use static::ALLOW/static::DENY/static::KILL, just about everything in Drupal core was returning
TRUE
orFALSE
. Even the most typical example of all,EntityAccessCheck
returned a boolean and never NULL, yet only now it is being changed (fixed) to returnALLOW
(TRUE
) orDENY
(FALSE
):So… yes, you're right in theory, but in practice just about everything was using
FALSE
, notNULL
, which is now being fixed in #2063405: Update all access checkers to use static::ALLOW/static::DENY/static::KILL. So, it's kind of harsh to say that everybody was just using it wrong, when most of core was wrong, and everybody else was looking at existing core examples, thus perpetuating the existing flaws. Since everybody was usingFALSE
, notNULL
(i.e.KILL
, notDENY
), that effectively meant a default ofALL
for the majority of use cases.Conclusion: when introducing a tri state return value, ensure all of core is using it correctly, or otherwise problems and assumptions will permeate, especially if it *looks* like a boolean.
Comment #58
dawehnerI totally agree with you that it was problematic that all this access checkers went in without proper review,
as from the first days of the access manager this tri-logic existed. Let's concentrate now on fixing all the places.
Comment #59
Wim LeersYes, let's concentrate on fixing :)
I'm fixing it for:
Comment #60
catchHmm this was RTBC for four days before commit excluding re-rolls, it wasn't that fast :(
What needs to happen in this issue? Looks it's a bug rather than task now - would've been better to open new issues.
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedso, should this issue be closed? i agree new issues should be open.
anyway setting to needs work, since the patch needs at least a reroll
Comment #62
jibran53: access_mode_all_tests-1984528-53.patch queued for re-testing.
Comment #65
dawehnerI don't really think that we should do anything here.
Comment #66
dawehnerI am pretty sure we can close this issue now.
Comment #67
Wim LeersYay :)