Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Views needs a access check class, which allows you to check access by role. It's certainly not really helpful
for code-only routes, though if you configure for example a View people really often uses role based access, primarily
because you don't have an UI for adding new permissions, just for adding roles.
Comment | File | Size | Author |
---|---|---|---|
#61 | drupal-1956896-61.patch | 12.68 KB | dawehner |
#61 | interdiff.txt | 1.99 KB | dawehner |
#60 | drupal-1956896-60.patch | 11.89 KB | dawehner |
#60 | interdiff.txt | 7.17 KB | dawehner |
#55 | drupal-1956896-55.patch | 7.01 KB | dawehner |
Comments
Comment #1
dawehnerHere is a first patch with a test, though I would love to not use a WebTest for that.
Comment #2
damiankloip CreditAttribution: damiankloip commentedNice work. I know you have a couple of todo things in there, but hey, reviews are always welcome right? :)
Nice doc, I would just use 'Create' instead of 'Setup'?
I get this is used below. I think it might be better to just do
$all_uids = array_keys($accounts);
after you've created the accounts?I get this is account 1 and 2 kind of thing, but would just account 3 be better?
Needs a docblock.
We are still using the full namespaces?
Not sure if we are bothering, but should we use
$account->get('roles')
?Comment #3
dawehnerWow this was a long route ;)
Additional this is so full of hacks, so I'm wondering whether it's worth to do now, but with #1620010: Move LANGUAGE constants to the Language class this hacks in bootstrap.inc aren't required anymore.
Tried to actually get a working kernel + access manager etc, but then I realized all what's needed is to use the RoleAccessCheck() class directly.
Comment #4
dawehnerI didn't checked the review in #2, sorry
Comment #5
dawehnerI'm waiting for WSCCI to tell whether the hack in bootstrap.inc is okay.
Comment #6
Crell CreditAttribution: Crell commentedPHPUnit has annotations that can let you inject all of this setup code, including getTestRouteCollection(), from other methods. That would keep the test cleaner, IMO.
Comment #7
dawehnerWe could also wait until #1825332: Introduce an AccountInterface to represent the current user is in, all this removes the requirement for creating User Entities.
Comment #8
msonnabaum CreditAttribution: msonnabaum commentedHere's a version that uses @dataProvider like Crell suggested. It's not a *huge* win here because the setup is still a bit complex, but it does make the assertion method much simpler and we could probably use more examples of this in core.
I also removed the bootstrap.inc include. That's verboten in UnitTestCase. Instead, I just defined the one language constant that User needs.
Comment #9
dawehnerOh right this works also. Then please let's also remove this bootstrap.inc hack.
Comment #11
dawehnerThis exceptions happen if you execute these tests via simpletest.
Comment #12
Crell CreditAttribution: Crell commentedWouldn't this be "say which path"? In which case it should be preceeded by a semi-colon, not comma.
Needs docblock.
Other than those nits, this is looking good.
Comment #13
dawehnerFixed the documentation.
Comment #14
Crell CreditAttribution: Crell commentedThis still doesn't make sense grammatically. "Setup expected values; specify which paths can be accessed by which user." would be what you're looking for, I think.
Comment #15
dawehnerRespect, this has been exactly 80 chars :)
Comment #16
damiankloip CreditAttribution: damiankloip commentedLooking pretty good.
Looks like we don't really need to use a $role variable, can just be in the array_diff?
Comment #17
dawehnerYeah we could do it. No idea which of these two alternatives are easier to read.
Comment #18
dawehnerAdding the tag.
Comment #19
tim.plunkettEither use the new {@inheritdoc} or the full \Drupal\... part, but not this :)
If a user doesn't have a role, shouldn't they be denied access? NULL means "no opinion", that doesn't seem to be correct here. So replace with
return empty($diff);
?Missing some trailing commas here.
Is making this public a PHPUnit thing, or can it be protected?
Delete this?
I don't know what this is. Missing param.
Comment #20
dawehnerThanks for your review!
Yeah I totally wasn't sure about that, so I went with the way PermissionAccessCheck works. http://api.drupal.org/api/drupal/core!modules!user!lib!Drupal!user!Acces...
That's pretty cool stuff: http://www.phpunit.de/manual/3.6/en/appendixes.annotations.html#appendix...
Comment #21
Crell CreditAttribution: Crell commentedI disagree. Why should we force-deny if the role doesn't match? Most of the access checkers are TRUE/NULL, and don't return FALSE. Remember, if a checker returns false then NOTHING can grant access after that. FALSE is terminal.
Comment #22
tim.plunkettWell, this comes back to the fact that access checkers aren't ANDed together. So if you want to say "This route must meet both of these access checkers", there is no way to do that. (that I can see).
Comment #23
Crell CreditAttribution: Crell commentedTrue, but that's by design. Individual checkers shouldn't declare themselves the Final Authority(tm) without very good reason. I don't see a reason for generic role checks to be a special snowflake.
Comment #24
dawehnerI agree with crell. If we return FALSE, you can't connect multiple one. I guess in the custom world there seems to be a lot of usecases for that.
Comment #25
klausiwat? hook_permission() does not define roles?
That could use a comment why we return NULL instead of false. Yes, it is already documented elsewhere what NULL means, but I think we need to repeat it more often so that people get it.
What does the entity class call? A constant cannot be called? In what case is this constant not available?
Should be "Test for the role based access checker in the routing system."
So this is not a test method. Then it should at least have a reference or @see to the actual test method that uses it. Is there no PHPUnit doc standard for that?
Comment #26
olli CreditAttribution: olli commentedDataprovider could have a prefix other than "test".
Comment #27
dawehnerThanks for the reviews!
... Everything needs to be autoloadable or not usuable at all. Added a todo and improved the comment.
Adapted all the other comments as well.
Comment #28
olli CreditAttribution: olli commentedIs there an issue on that exception
Incorrect integer value: '' for column 'line' at row 3: INSERT INTO {simpletest}..
?Comment #29
tim.plunkettSorry.
Comment #30
dawehner@Olli
Not that I know of.
Comment #31
klausiLooks RTBC to me now!
Comment #32
olli CreditAttribution: olli commentedFiled #1963022: Problem with PHPUnit test results when using @dataProvider.
Comment #33
tim.plunkett#27: drupal-1956896-27.patch queued for re-testing.
Comment #35
dawehnerNo autocompletion in the bundle anymore :(
Comment #36
Crell CreditAttribution: Crell commentedFrom the testbot log:
I have no idea what it's complaining about...
Comment #37
klausiThat sounds like those special PHP exceptions/warnings that don't have a line number, for example http://stackoverflow.com/questions/9076899/notice-unknown-on-line-0-how-...
Comment #38
klausiMaking simpletest more robust to see the actual error.
Comment #39
dawehnerOlli created an issue for that: #1963022: Problem with PHPUnit test results when using @dataProvider
Comment #40
moshe weitzman CreditAttribution: moshe weitzman commented#38: views-role-access-1956896-38.patch queued for re-testing.
Comment #41
tim.plunkett#38: views-role-access-1956896-38.patch queued for re-testing.
Comment #42
tim.plunkettI'm not sure if that first hunk really belongs, but this whole thing looks pretty great!
Comment #43
dawehnerUploading a recent version + a screenshot from the simpletest page.
Comment #44
dawehnerurgs.
Comment #45
BerdirIs the first hook related to the broken phpunit reporting that got fixed in #1969406: PHPUnit Test asserting logging broken?
Comment #46
dawehner@Berdir
Yes, the patch from klausi has been just for debugging.
Comment #47
dawehner#44: drupal-1956896-41.patch queued for re-testing.
Comment #48
alexpottI don't think data providers should be providing data to the test through a class property... looks weird to me... about this?
Comment #49
alexpottAdding missing @param :(
Comment #50
alexpott@dawehner noticed a spurious blank line
Comment #51
dawehnerI personally think this patch is now much easier to read with the changes of alex.
Comment #52
tim.plunkettNice changes! Still RTBC.
Comment #53
andypostNice example but without usage. Maybe better introduce it's usage within patch?
this could be removed, 'else' not needed
Comment #54
dawehnerThanks for your review.
The problem is that I doubt that there is any usage in core beside views and maybe SCOTCH. You know, as a module developer I think it's bad practice to rely on roles, but it's a useful feature elsewhere, so I can't find an actual usecase.
Regarding the else bit, I think it's helpful to explain why NULL is returned. This also matches the documentation of PermissionAccessCheck.
Comment #55
dawehnerRemoved the else
Comment #56
andypostThanx a lot , RTBC as it was
Comment #57
Crell CreditAttribution: Crell commentedCan we document on the checker class that it is an "all" check, not an "any" check (vis, you can specify multiple roles, but you then need all of them)? I had to read the code 4 times to determine that it was an "all" check... and even then I'm not 100% certain of that. :-)
Probably just something in the class's docblock is sufficient.
Comment #58
dawehnerIt's indeed a good question whether we want to have both behavior? all and any. Does someone has an oppinion about that?
Comment #59
alexpottWell doesn't the answer to #58 depend on what @andypost has been asking for... use cases :)
Comment #60
dawehnerI renamed the test file to match it with the classname. I think that's an improvement.
Additional I implemented some logic, how to connect with OR and AND.
Comment #61
dawehnerLet's extend the test coverage to test for non-trimmed values.
Comment #62
dawehnerAlex asked why #60 changed phpunit_error.xml.
When #1963022: Problem with PHPUnit test results when using @dataProvider was written I used this issue to generate a proper phpunit_error.xml. On that time I had named the PHPUnit Test "RouteRoleTest",
so that also landed in the phpunit_error.xml file. Note: the RouteRoleTest did not get added by that patch, just the example phpunit_error.xml.
On #60 of this issue I realized: Let's just make the test class named as parallel as possible to the actual access checker,
so there is now RoleAccessCheck and RoleAccessCheckTest. Just for sanity, I also changed phpunit_error.xml to point to the actual existing file.
Nothing fancy at the end.
Comment #63
tim.plunkettIn that case, this looks great! (That was the only part I wasn't sure of).
@dawehner++
Comment #64
webchickSo according to #1800998-75: Use route system instead of hook_menu() in Views, this is a blocker for REST stuff, which I think makes this more of a task than a feature.
Seems sensible though, and something other modules outside of views could make good use of. I'm a little concerned about adding more $GLOBALS['user'] to our code, but I guess if that could be refactored out here, it would. Is this blocked on #1825332: Introduce an AccountInterface to represent the current user or..?
When I reviewed earlier with Alex, Jess, and Moshe, Jess had a concern about whether or not "," and "+" were clear enough for OR or AND. Let's spin off a follow-up to discuss whether we can keep that as-is or need to change it. Personally, I like it because it's what taxonomy does/used to do when combining multiple terms into a single URL.
Committed and pushed to 8.x. Thanks!
This will need a change notice to inform people of the new check.
Comment #65
dawehnerThanks for finally getting this in!
I added a small bit to the more or less central route access change notice: http://drupal.org/node/1851490/revisions/view/2461630/2675054
I don't 100% follow the plans to get rid of global user, but I guess the plan is to get the AccountInterface and place it on the request object at some point.
From there the access checkers would have access to it.
Yeah indeed, this let's me always think a bit which one is which.
I opened an issue for that: #1986640: Support AND/OR conjunctions for permission checks
Comment #66
aspilicious CreditAttribution: aspilicious commentedI would add a simple example. :)
Comment #67
dawehnerGood idea.
Comment #68
BerdirLooks good.
The change notice is getting long and somewhat hard to scan, we might want to shorten it down and instead have a documentation page and just link to that to describe the different default access checkers. There' also the entity access one that has a separate change notice. But putting it in there means it is written down and someone who is better a structuring/writing documentation than us developers can do that :) That's what those checkboxes at the bottom are for...
Comment #70
xjmWhatever change notice was updated needs a reference to this issue.
Also please untag when the change notice is complete.
Comment #71
dawehnerAdded
Comment #72
xjmThanks! and removing the tag... :)