Problem/Motivation

The following issues all permit access to view something using the 'access content' permission.

Is this correct? Is this correct for configuration entities? For public files?

For more context around access and REST - in some of the other issues we are adding access handlers and new permissions to deal with admin access see:

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Issue summary: View changes
vaplas’s picture

In my worldview, the files are part of the content, hence when the user does not have 'access content' this should apply to the files too. But I'm also not against the appearance of new specialized permission like "access files". And of course, I'm absolutely ready to admit my mistakes after listening to other opinions.

dawehner’s picture

The file module already uses this specific permission during the file upload:


file.ajax_progress:
  path: '/file/progress/{key}'
  defaults:
    _controller: '\Drupal\file\Controller\FileWidgetAjaxController::progress'
  requirements:
    _permission: 'access content'
~
~
Wim Leers’s picture

Priority: Normal » Major
Issue tags: +blocker

The problem is that many (most) config entity types do not specify an access control handler, nor an admin_permission.

The current status
Entity types not specifying an access control handler nor an admin_permission automatically use the default entity access control handler (\Drupal\Core\Entity\EntityAccessControlHandler). For checking access, they do this:
  protected function checkAccess(EntityInterface $entity, $operation, AccountInterface $account) {
    if ($operation == 'delete' && $entity->isNew()) {
      return AccessResult::forbidden()->addCacheableDependency($entity);
    }
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $this->entityType->getAdminPermission());
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

and:

  protected function checkCreateAccess(AccountInterface $account, array $context, $entity_bundle = NULL) {
    if ($admin_permission = $this->entityType->getAdminPermission()) {
      return AccessResult::allowedIfHasPermission($account, $admin_permission);
    }
    else {
      // No opinion.
      return AccessResult::neutral();
    }
  }

The latter is only for creating entities of that type. The former is for all other operations.

First necessity: admin_permission for every entity type
The above code shows that entities of a certain type can only be created if they define an admin_permission. If they don't do that, then no operations on these entities are permitted at all.

Conclusion: specifying an admin_permission is a necessity for virtually all of them.

Second necessity: a separate permission for the view operation
However, the mere existence of that admin_permission is almost never sufficient. Most if not all config entity types are safe to be viewed by many users. There's often only risk in allowing users to modify entities.

A clear example: the Vocabulary config entity. Without the ability to view these, decoupled applications cannot retrieve the label or description of a vocabulary. This is a problem.
The only solution (at least right now, and this has been the case since 8.0.0) is to give them the 'administer taxonomy' permission. But this permission then also allows them to modify and delete vocabularies (and terms!). That's far too much!
In other words: this is a clear example for the need to have a separate view permission.

The 'access content' permission.
In Drupal 8.0.0, the 'access content' permission is already being used to grant access to nodes, node_types and terms. Besides those entity access control handlers, it's used for many routes across many modules. Many of them not node-related at all. Quoting myself from #2808217-27: To be able to view Vocabulary config entities via REST, one should not have to grant the 'administer taxonomy' permission, from December 1, 2016 (>4 months ago):

I wonder if we don't want to have some permission to obey? The Block, Comment, File and System modules all use the access content permission to do this, despite not depending on the node module.

So I propose we do the same here. If we fix that at some point, we'll need to move the access content permission to the System module and provide an upgrade path anyway.

IOW: 'access content' already is the "bare minimum permission you need to access pretty much anything on a Drupal site". So… let's continue to apply that elsewhere too: non-sensitive configuration entities such as vocabulary entities would then be easily made accessible.

Wim Leers’s picture

#3:

Linking some other committed issues that made access changes under test coverage auspices.

All of these are only making minor adjustments to existing entity access control handlers. They're adding support for the view operation — the existing code only supported other operations.

Wim Leers’s picture

Also, I forgot to make one very important point in #6.

The reason this went unnoticed all this time is rather simple: Drupal 8 is not API-first, largely because our Entity/Field API has some questionable design decisions.

We're creating config entities in forms (in their submit callbacks). We're not validating those config entities. We're not checking access to those config entities. We're only checking access to the form. Hence for config entities, we are bypassing our validation and access APIs.
This works … but as soon as you want to be able to perform those same tasks via a REST API (or GraphQL or JSON API or something else), you're then forced to finally define the hitherto undefined access control. (And for validation we have #2300677: Create/Update/Delete (POST/PATCH/DELETE) ConfigEntity via REST.)

Wim Leers’s picture

I bumped this to "major" in #6 because this is blocking 7 issues that are effectively RTBC, but they're all implementing a change that we need to agree on in this plan issue. Alternatively, this plan issue must specify a different approach.

Wim Leers’s picture

Status: Active » Needs review

This is actually the appropriate state.

dawehner’s picture

First necessity: admin_permission for every entity type
The above code shows that entities of a certain type can only be created if they define an admin_permission. If they don't do that, then no operations on these entities are permitted at all.
Conclusion: specifying an admin_permission is a necessity for virtually all of them.

Well, the admin permission seems to be more of a shortcut ... it feels better to define a custom access control handler on the longrun, given that people want specific permissions anyway.

Wim Leers’s picture

#11: Agreed that you want fine-grained, well-thought-through access control handlers for every entity type … but an admin_permission is still useful for the coarse "be able to do anything with this entity type", which is also a valid need.

dawehner’s picture

That's fair, well to be honest I totally agree with your for core for now. The problem is once we add this admin permission we need to carry it around potentially, so let's better think about it before we add it.

Wim Leers’s picture

The problem is once we add this admin permission we need to carry it around potentially,

What do you mean? It's up to every entity type individually to specify an admin_permission. So the impact is always limited to that one entity type.

dawehner’s picture

What do you mean? It's up to every entity type individually to specify an admin_permission. So the impact is always limited to that one entity type.

Right. Let's assume though we adapt the entity type in the future to have more granular permissions. In that case we still need to provide support admin_permission.

Wim Leers’s picture

True. But for most of these entity types, there already is a module-level administrative permission!

I agree though we need to consider that long-term cost. Do you see an alternative approach to avoid that?

dawehner’s picture

Now that I think about it, config entities, unlike content entities, are created/changed by the same group of people (site builders).
These people use an admin role anyway, from my point of view, so given that, I think it is actually fine to add administrative permissions to every of those entity types.

What are your thoughts about that?

Wim Leers’s picture

My thoughts exactly! :)

Wim Leers’s picture

Issue tags: +Baltimore2017

Just discussed this with @catch at DrupalCon Baltimore 2017.

Outcome:

  1. He agrees that access content is the sensible permission to use for this; because it is already being used outside the node module anyway.
  2. He is concerned that we are allowing people to view view config entities with just the access content permission, but he agrees that doing so is a step forward compared to today (because today we grant permission always, no permissions needed). So he wants to see a major follow-up tagged Security improvement to review that.

I think that the next step is for Alex Pott to agree or disagree. Then:

  • if he agrees, he can RTBC it. Then I can unblock all the blocked issues and get them moving forward again.
  • if he disagrees, we need further discussion.
alexpott’s picture

I'm okay we the decision - I have a mental dissonance with access content granting access to view configuration - but having this controlled by a permission definitely makes sense. I'm right in thinking that you can configure more granular permissions on the rest.resource config entity if you want?

Wim Leers’s picture

but having this controlled by a permission definitely makes sense

Yep, it's not controlled by any permission right now.

I'm right in thinking that you can configure more granular permissions on the rest.resource config entity if you want?

No; you only get the extra permissions if you enable the BC layer that #2664780: Remove REST's resource- and verb-specific permissions for EntityResource, but provide BC and document why it's necessary for other resources left in place. What do you mean exactly when you say more granular permissions? I could see it be valuable to require the administer views to just view a view (heh), but most others seem 100% harmless to expose to just about anybody: DateFormat, RdfMapping, Menu, and so on.
So, can you please clarify?

dawehner’s picture

For me viewing a config entity as a thing is tricky outside of REST.

alexpott’s picture

@dawehner yep that's why this is such an odd issue. Thinking about this some more I really think that viewing raw configuration should be a separate permission from access content. Taking the view example you might not want to expose your business configuration to anyone that can access content on your site. Whilst there might no harm there certainly is disclosure and you might view your view as intellectual property that you only want certain users to be be able to access.