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.
Followup of #1984528: Follow-up: Allow for more robust access checking, we have to replace ANY with ALL
Problem/Motivation
If a route needs several access checkers it currently uses an OR/ANY logic to concat them.
Developers though expect it to be ALL/AND by default
Proposed resolution
Switch the default to ALL, and check potential issues.
API changes
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#47 | 2063401-47.patch | 12.35 KB | damiankloip |
#41 | 2063401-41.patch | 12.88 KB | damiankloip |
#41 | interdiff-2063401-41.txt | 794 bytes | damiankloip |
#37 | access-2063401-37.patch | 12.59 KB | dawehner |
#32 | routing-2063401-32.patch | 12.59 KB | dawehner |
Comments
Comment #1
dawehnerLet's see how much breaks if ANY/ALL is switched.
Comment #3
dawehnerThis should fix the failures.
Comment #4
Wim LeersIs this really correct?
Shouldn't this be
ALL
and be only dependent on_permission
?I'm not familiar with the REST module code, but I see there's access checks happening on
_method
as well. Though it shouldn't apply here because_access_rest_csrf
isn't set. I'm not aware of the full scope of what's happening, but in any case,ANY
seems wrong.If I'm wrong, care to explain why? :)
Comment #5
Wim LeersDidn't mean to mark this NW.
Comment #6
dawehnerlinking because of pure lazyness: #2063405: Update all access checkers to use static::ALLOW/static::DENY/static::KILL
Comment #7
Crell CreditAttribution: Crell commented_method is not enforced by an access checker but by UrlMatcher. _access_mode is entirely irrelevant for that.
Comment #8
dawehnerTried to debug that problem with more depth:
First note:
It feels wrong to have rest.csrf twice on there.
We are doing a POST request here, so the CSRFAccessChecker will return NULL (as they don't make sense on POST). As NULL is considered to break on the ALL case, we seem to need ANY here?
Comment #9
dawehner#3: access-2063401-3.patch queued for re-testing.
Comment #10
Wim Leers#3: access-2063401-3.patch queued for re-testing.
Comment #11
dawehner#3: access-2063401-3.patch queued for re-testing.
Comment #12
dawehner#3: access-2063401-3.patch queued for re-testing.
Comment #13
Wim LeersHow do we move this forward? Who is the most appropriate person to review this?
Comment #14
dawehnerMaybe damian well react to the message left on druplicon :) Crell is out of chicago for quite a long time, as far as I remember
Comment #15
Crell CreditAttribution: Crell commentedFortunately I brought my laptop with me to Minneapolis. :-)
Comment #16
damiankloip CreditAttribution: damiankloip commentedI was confused about Daniel's comment in #8. I guess that's not actually an issue anymore then? :)
Otherwise, +1 on this.
Comment #17
dawehnerNot that I think of. The problem is that you would expect it to be all, maybe, but change the beaviour to 'ALL' is out of scope of this issue.
Comment #18
tim.plunkett#3: access-2063401-3.patch queued for re-testing.
Comment #19
webchickPatch no longer applies.
Comment #20
dawehnerHere is a rerole.
Comment #21
damiankloip CreditAttribution: damiankloip commentedAnd back again.
Comment #22
Wim Leersd.o tag monster
Comment #23
tim.plunkett#20: access-2063401-20.patch queued for re-testing.
Comment #25
dawehner#20: access-2063401-20.patch queued for re-testing.
Comment #27
tim.plunkettTwo of the recent additions needed ANY.
Comment #29
webchickI would've expected this patch to also kill all of the unnecessary "ALL"s. Is that possible?
Comment #30
dawehnerThis could be it.
Comment #32
dawehnerUps.
Comment #33
jibran#29 is addressed so back to RTBC.
Comment #34
dawehner#32: routing-2063401-32.patch queued for re-testing.
Comment #36
jibranneeds reroll after #2089671: Convert non-test form callbacks to new form controller
Comment #37
dawehnerJust an ordinary reroll.
Comment #38
webchickMoving back to RTBC, since that's where it was before testbot failed it.
Comment #39
dawehnerCool!
Comment #41
damiankloip CreditAttribution: damiankloip commentedJust one missing in the ContentTranslationRouteSubscriber I think. This is when you have to love test coverage.
Comment #42
damiankloip CreditAttribution: damiankloip commentedThis should go back to RTBC if/when this passes.
Comment #43
dawehnerGood catch!
Comment #44
tim.plunkett#41: 2063401-41.patch queued for re-testing.
Comment #45
pwolanin CreditAttribution: pwolanin commented+1 from me too.
Comment #46
alexpottPatch no longer applies.
Comment #47
damiankloip CreditAttribution: damiankloip commentedrerolled. The hunk from router_test.routing.yml is already in HEAD:
Comment #48
dawehnerThank you for the reroll!
Comment #49
webchickAwesome, thank you SO much for this patch!
Committed and pushed to 8.x. Thanks!
Needs a change notice.
Comment #50
dawehnerhttps://drupal.org/node/1851490/revisions/view/2868379/2870151
https://drupal.org/node/2107991
Comment #51
jibranFixed the change notice title and added some examples.
Comment #52
Wim Leersjibran++
Comment #54
klausiOops, this change caused a security issue in rest.module: #2420559: REST permissions are not working as expected..
Comment #55
dawehner@klausi
Common misconception though of these patches.