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

Files: 
CommentFileSizeAuthor
#47 2063401-47.patch12.35 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,614 pass(es).
[ View ]
#41 2063401-41.patch12.88 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,889 pass(es).
[ View ]
#41 interdiff-2063401-41.txt794 bytesdamiankloip
#37 access-2063401-37.patch12.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,717 pass(es), 16 fail(s), and 0 exception(s).
[ View ]
#32 routing-2063401-32.patch12.59 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,517 pass(es), 19 fail(s), and 0 exception(s).
[ View ]
#32 interdiff.txt6.2 KBdawehner
#30 access_module-2063401-30.patch12.53 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 59,323 pass(es), 31 fail(s), and 5,010 exception(s).
[ View ]
#30 interdiff.txt6.2 KBdawehner
#27 access_mode-2063401-27.patch6.81 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,510 pass(es), 19 fail(s), and 4 exception(s).
[ View ]
#27 interdiff.txt1.25 KBtim.plunkett
#20 access-2063401-20.patch5.56 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 58,923 pass(es), 362 fail(s), and 50 exception(s).
[ View ]
#3 access-2063401-3.patch5.53 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]
#3 interdiff.txt1.11 KBdawehner
#1 access-2063401-1.patch4.42 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,624 pass(es), 26 fail(s), and 0 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new4.42 KB
FAILED: [[SimpleTest]]: [MySQL] 57,624 pass(es), 26 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.11 KB
new5.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,928 pass(es).
[ View ]

This should fix the failures.

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

Status:Needs work» Needs review

Didn't mean to mark this NW.

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

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?

Issue tags:-DrupalWTF, -WSCCI

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

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

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

Issue tags:+DrupalWTF, +WSCCI

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

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

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

Status:Needs review» Reviewed & tested by the community

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

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

Otherwise, +1 on this.

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.

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

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new5.56 KB
FAILED: [[SimpleTest]]: [MySQL] 58,923 pass(es), 362 fail(s), and 50 exception(s).
[ View ]

Here is a rerole.

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

And back again.

Issue tags:-Needs reroll

d.o tag monster

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.

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.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new1.25 KB
new6.81 KB
FAILED: [[SimpleTest]]: [MySQL] 59,510 pass(es), 19 fail(s), and 4 exception(s).
[ View ]

Two of the recent additions needed ANY.

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new6.2 KB
new12.53 KB
FAILED: [[SimpleTest]]: [MySQL] 59,323 pass(es), 31 fail(s), and 5,010 exception(s).
[ View ]

This could be it.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new6.2 KB
new12.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,517 pass(es), 19 fail(s), and 0 exception(s).
[ View ]

Ups.

Status:Needs review» Reviewed & tested by the community

#29 is addressed so back to RTBC.

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.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new12.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,717 pass(es), 16 fail(s), and 0 exception(s).
[ View ]

Just an ordinary reroll.

Status:Needs review» Reviewed & tested by the community

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

Cool!

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new794 bytes
new12.88 KB
PASSED: [[SimpleTest]]: [MySQL] 58,889 pass(es).
[ View ]

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

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

Status:Needs review» Reviewed & tested by the community

Good catch!

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

+1 from me too.

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

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new12.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,614 pass(es).
[ View ]

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

+  options:
+    _access_mode: 'ANY'

Status:Needs review» Reviewed & tested by the community

Thank you for the reroll!

Title:Replace the default _access_checks(access mode) with ALL instead of ANYChange 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.

Title:Change notice: Replace the default _access_checks(access mode) with ALL instead of ANYReplace 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.

jibran++

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