Problem/Motivation

The ANY access mode makes it impossible to reuse access checkers in other contexts. The problem is that an access checker has no means to say I don't care, other checkers should decide in a way which works in ANY as well as in ALL mode. If you return allowed() then you allow too much in ANY mode and if you return neutral() than that evaluates to forbidden() in ALL mode.

According to @dawehner in IRC:

dawehner: znerol: historically #2157541: Views sets access to ANY on routes - could result in information disclosure was one usecase why it got introduced

But that use case has been found to be a misconception.

Proposed resolution

Drop support for _access_mode routes and always assume ALL.

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug, because the design of ANY is broken
Issue priority Major, because it solves potential security problems
Disruption Modules using the ANY access mode would have to write some custom access checkers, but ALL is certainly the common case anyway.
CommentFileSizeAuthor
#12 interdiff.txt463 bytesdawehner
#12 2431281-12.patch21.38 KBdawehner
#8 interdiff.txt1.66 KBdawehner
#8 2431281-8.patch21.4 KBdawehner
#2 2431281-2.patch20.41 KBdawehner

Comments

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

_access_mode has always bothered me when I used it and when I have taught others Drupal 8 but I could never really pin-point why. After reading the issue summary, I now know why! :-) Thanks.

I totally agree. So long as we stick to the allowed()/neutral()/forbidden() trifecta - which is not going to change at this point - _access_mode is just conceptually wrong.

dawehner’s picture

dawehner
Primary language German
commented
Status: Active » Needs review
StatusFileSize
new 2431281-2.patch20.41 KB

Just an experiment. I don't say that there aren't usecases for ANY, BUT

fabianx’s picture

fabianx commented

+1 to that! I don't know the code good enough to be able to sensible RTBC it, though.

catch’s picture

catch
he/him
Primary language English
commented

Yes I've only seen this cause security issues and not actually help anything.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

+1

dawehner’s picture

dawehner
Primary language German
commented
Priority: Normal » Major
Issue summary: View changes

Bumping to major, as it is security hardening.

Added a beta evaluation.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

Why keep the constants? I would have expected this patch to remove them.

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new 2431281-8.patch21.4 KB
new interdiff.txt1.66 KB

Good point tim!

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Yep, this caused the security issue in #2420559: REST permissions are not working as expected., +1 on the removal.

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community

*sniff* I'll miss you, _access_mode.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/update/update.routing.yml
@@ -49,8 +45,7 @@ update.module_install:
-  options:
-    _access_mode: 'ALL'
+  requirements:
   requirements:

now we have "requirements:" twice here?

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Needs review
StatusFileSize
new 2431281-12.patch21.38 KB
new interdiff.txt463 bytes

It's not the first time when I think that the YAML parser is not strict enough.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 9c2b19c and pushed to 8.0.x. Thanks!

  • alexpott committed 9c2b19c on 8.0.x 
    Issue #2431281 by dawehner: Drop support for _access_mode routes and...
jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Fixed » Needs work
Issue tags: +Needs change record updates

We have a change notice which needs update or deletion

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs work » Fixed

Unpublished https://www.drupal.org/node/2107991 updated https://www.drupal.org/node/1851490

Status: Fixed » Closed (fixed)

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