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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
4.42 KB

Let's see how much breaks if ANY/ALL is switched.

Status: Needs review » Needs work

The last submitted patch, access-2063401-1.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.11 KB
5.53 KB

This should fix the failures.

Wim Leers’s picture

Status: Needs review » Needs work
Issue tags: +DrupalWTF, +WSCCI
+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.php
@@ -92,6 +92,8 @@ public function routes() {
         // The HTTP method is a requirement for this route.
         '_method' => $method,
         '_permission' => "restful $lower_method $this->pluginId",
+      ), array(
+        '_access_mode' => 'ANY',
       ));

Is 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? :)

Wim Leers’s picture

Status: Needs work » Needs review

Didn't mean to mark this NW.

dawehner’s picture

Crell’s picture

_method is not enforced by an access checker but by UrlMatcher. _access_mode is entirely irrelevant for that.

dawehner’s picture

Tried to debug that problem with more depth:

First note:

[3]
access_check.rest.csrf
access_check.permission
access_check.rest.csrf

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?

dawehner’s picture

Issue tags: -DrupalWTF, -WSCCI

#3: access-2063401-3.patch queued for re-testing.

Wim Leers’s picture

#3: access-2063401-3.patch queued for re-testing.

dawehner’s picture

#3: access-2063401-3.patch queued for re-testing.

dawehner’s picture

Issue tags: +DrupalWTF, +WSCCI

#3: access-2063401-3.patch queued for re-testing.

Wim Leers’s picture

How do we move this forward? Who is the most appropriate person to review this?

dawehner’s picture

Maybe damian well react to the message left on druplicon :) Crell is out of chicago for quite a long time, as far as I remember

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Fortunately I brought my laptop with me to Minneapolis. :-)

damiankloip’s picture

I was confused about Daniel's comment in #8. I guess that's not actually an issue anymore then? :)

Otherwise, +1 on this.

dawehner’s picture

I was confused about Daniel's comment in #8. I guess that's not actually an issue anymore then? :)

Not 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.

tim.plunkett’s picture

#3: access-2063401-3.patch queued for re-testing.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.56 KB

Here is a rerole.

damiankloip’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs reroll

And back again.

Wim Leers’s picture

Issue tags: -Needs reroll

d.o tag monster

tim.plunkett’s picture

Issue tags: -DrupalWTF, -WSCCI

#20: access-2063401-20.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, access-2063401-20.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#20: access-2063401-20.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +DrupalWTF, +WSCCI, +Needs reroll

The last submitted patch, access-2063401-20.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
1.25 KB
6.81 KB

Two of the recent additions needed ANY.

Status: Needs review » Needs work

The last submitted patch, access_mode-2063401-27.patch, failed testing.

webchick’s picture

I would've expected this patch to also kill all of the unnecessary "ALL"s. Is that possible?

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
12.53 KB

This could be it.

Status: Needs review » Needs work

The last submitted patch, access_module-2063401-30.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
6.2 KB
12.59 KB

Ups.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

#29 is addressed so back to RTBC.

dawehner’s picture

Issue tags: -DrupalWTF, -WSCCI

#32: routing-2063401-32.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: +DrupalWTF, +WSCCI

The last submitted patch, routing-2063401-32.patch, failed testing.

jibran’s picture

dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.59 KB

Just an ordinary reroll.

webchick’s picture

Status: Needs review » Reviewed & tested by the community

Moving back to RTBC, since that's where it was before testbot failed it.

dawehner’s picture

Cool!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, access-2063401-37.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
794 bytes
12.88 KB

Just one missing in the ContentTranslationRouteSubscriber I think. This is when you have to love test coverage.

damiankloip’s picture

This should go back to RTBC if/when this passes.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Good catch!

tim.plunkett’s picture

#41: 2063401-41.patch queued for re-testing.

pwolanin’s picture

+1 from me too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Patch no longer applies.

damiankloip’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.35 KB

rerolled. The hunk from router_test.routing.yml is already in HEAD:

+  options:
+    _access_mode: 'ANY'
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the reroll!

webchick’s picture

Title: Replace the default _access_checks(access mode) with ALL instead of ANY » Change notice: Replace the default _access_checks(access mode) with ALL instead of ANY
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome, thank you SO much for this patch!

Committed and pushed to 8.x. Thanks!

Needs a change notice.

dawehner’s picture

jibran’s picture

Title: Change notice: Replace the default _access_checks(access mode) with ALL instead of ANY » Replace the default _access_checks(access mode) with ALL instead of ANY
Priority: Major » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Fixed the change notice title and added some examples.

Wim Leers’s picture

jibran++

Automatically closed -- issue fixed for 2 weeks with no activity.

klausi’s picture

Issue summary: View changes

Oops, this change caused a security issue in rest.module: #2420559: REST permissions are not working as expected..

dawehner’s picture

@klausi
Common misconception though of these patches.