Decouple access handlers for entity_view_mode and entity_form_mode from field_ui.module

I'm going to assign this to myself and have a stab at it. I hope everyone's happy with that - please let me know if not. I've been helping to work through some of the issues at Write EntityResourceTestBase subclasses for every other entity type, and I was just about to have a go at test coverage for EntityFormMode when I saw that @vaplas has linked this ticket.

I'm trying to step my way through this. The first part of the requirement is reasonably clear - ViewModeAccessCheck and FormModeAccessCheck both have an access method (in fact, bar the constructor, that's the only method they have).

The bit I'm less clear on is where they need to go. I've stepped through the code and it looks like entity access is governed by EntityAccessCheck and EntityAccessControlHandler.

They both have their own access methods. I'm sort of assuming one of them gets called by the access method on Entity itself - though I can't quite see how.

So is the task here to adapt one of access methods that already exist in the Entity code, so that the method recognises when the call is for an entity_view_form or an entity_view_mode, and then applies the logic in the access checker classes in field_ui? And if so, can anyone help me out with which access method needs to be adapted?

Or am I just waaay off the mark?

I guess that we need is to convert those into subclasses of EntityAccessControlHandler, similar to other entity types and then put it into the annotation of those entity classes. possibly even the same class/code if it's generic enough. And then update \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes() to use them.

But I'm not sure if we can really replace that, as view/form displays are special in that they are auto-created, at least the default display. So that might not actually exist, but we still want to allow access.

So likely we can't actually replace them completely, just model the new code based on them.

So you'd end up with something like below (obviously this wouldn't work, as the variables in the function signature don't match the variables in the function body, as I've just cut and pasted this together).

I've put this in the core/config/Entity directory. Is that what you have in mind?

I assume there would need to be some logic somewhere that would match the entity type to the AccessControlHandler - but I don't really understand where that would happen.

<?php

namespace Drupal\Core\Config;

use Drupal\Core\Access\AccessResult;
use Drupal\Core\Entity\EntityAccessControlHandler;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Session\AccountInterface;

/**
 * Defines the access control handler for the comment entity type.
 *
 * @see \Drupal\comment\Entity\Comment
 */
class EntityViewModeAcccessControlHandler extends EntityAccessControlHandler {

  /**
   * {@inheritdoc}
   */
  public function access(EntityInterface $entity, $operation, AccountInterface $account = NULL, $return_as_object = FALSE) {

    $access = AccessResult::neutral();
    if ($entity_type_id = $route->getDefault('entity_type_id')) {
      if (empty($bundle)) {
        $entity_type = $this->entityManager->getDefinition($entity_type_id);
        $bundle = $route_match->getRawParameter($entity_type->getBundleEntityType());
      }

      $visibility = FALSE;
      if ($form_mode_name == 'default') {
        $visibility = TRUE;
      }
      elseif ($entity_display = $this->entityManager->getStorage('entity_form_display')->load($entity_type_id . '.' . $bundle . '.' . $form_mode_name)) {
        $visibility = $entity_display->status();
      }

      if ($form_mode_name != 'default' && $entity_display) {
        $access->addCacheableDependency($entity_display);
      }

      if ($visibility) {
        $permission = $route->getRequirement('_field_ui_form_mode_access');
        $access = $access->orIf(AccessResult::allowedIfHasPermission($account, $permission));
      }
    }
    return $access;
  }
}

OK, so progress on this is slooooow because I don't really completely understand what I'm doing. I think I'm halfway there - if anyone wants to jump in and make suggestions, please do. I'll put this into a patch later - even though it doesn't work I guess it's a good way to kick things off.

So, I've added an access method to the ConfigEntityBase class:

namespace Drupal\Core\Config\Entity;

...

/**
 * Defines a base configuration entity class.
 *
 * @ingroup entity_api
 */
abstract class ConfigEntityBase extends Entity implements ConfigEntityInterface {
...
  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {

    $access = parent::access($operation, $account, $return_as_object);

    if ($operation === 'view') {

      if ($this->entityTypeId === 'entity_form_display') {

        $entityType = new ContentEntityType([$this->entityTypeId]);

        $handler = new ConfigEntityAccessControlHandler($entityType);

        $userHasAccess = $handler->access($this, $operation, $account);

        if ($userHasAccess) {
          return new AccessResultAllowed();
        }

        else {
          return new AccessResultForbidden();
        }
      }
    }
    return $access;
  }
}

This gets called when a user hits the entity_form_display endpoint. In turn, it creates a new ConfigEntityAccessControlHandler, which just has an access method on it. I figure this is all it needs since the only thing to check is whether the user has the export content permission.

The ConfigEntityAccessControlHandler looks like this:

namespace Drupal\Core\Config\Entity;

use Drupal\Core\Entity\EntityAccessControlHandler;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Field\FieldItemListInterface;
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Language\LanguageInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\user\Entity\Role;

/**
 * Class ConfigEntityAccessControlHandler
 */
class ConfigEntityAccessControlHandler extends EntityAccessControlHandler {

  /**
   * {@inheritdoc}
   */
  public function __construct(\Drupal\Core\Entity\EntityTypeInterface $entity_type) {
    parent::__construct($entity_type);
  }

  /**
   * {@inheritdoc}
   */
  public function access(EntityInterface $entity, $operation, AccountInterface $account = NULL, $return_as_object = FALSE) {

    $user = \Drupal::currentUser();

    if ($user->hasPermission('export configuration')) {
      return TRUE;
    }
    return FALSE;
  }
}

The main problem I'm having at the moment seems to be cacheing - if I clear the cache then hit the endpoint with an anonymous user, I get an access denied message. If I do the same thing with an admin user, I can access the configuration. If I then go back to the anonymous user and I can get to the config.

I don't really understand where the response gets cached. Can anyone help?

Hm, looking at the changes in #11, I wonder why we can't do this instead?

I have the feeling that I'm missing something very obvious…

@vaplas - nice job! I was waaaay off the mark!

Yes, I feel like it would be a good idea to introduce that additional permission. But weren't the REST permissions heavily simplified between 8.2 and 8.3? Would this not be a step back?

@vaplas I'm asking why #12 is not functionally identical to #11, but just much simpler?

At the very least field_ui_entity_type_build() needs to be updated to remove the (now-)duplicated setting of the admin permission.

So the permission we're referencing here is provided by Field UI, so I guess we would also have to move the permission, but the question is where to move it to?

At the very least field_ui_entity_type_build() needs to be updated to remove the (now-)duplicated setting of the admin permission.

Indeed.

So the permission we're referencing here is provided by Field UI, so I guess we would also have to move the permission, but the question is where to move it to?

I think the answer is: field.permissions.yml? Nope, that does not work, because EntityFormMode & EntityViewMode live in lib/Drupal/Core… ugh, this makes total sense given this issue's title (which I hadn't read in some time). It also explains why today's code is what it is.

Re-reading the issue brings me back to @Berdir's comment in #7:

I guess that we need is to convert those into subclasses of EntityAccessControlHandler, similar to other entity types and then put it into the annotation of those entity classes. possibly even the same class/code if it's generic enough. And then update \Drupal\field_ui\Routing\RouteSubscriber::alterRoutes() to use them.

But I'm not sure if we can really replace that, as view/form displays are special in that they are auto-created, at least the default display. So that might not actually exist, but we still want to allow access.

So likely we can't actually replace them completely, just model the new code based on them.

@tstoeckler, do you agree with @Berdir's recommendations/thoughts?

I think we are talking about different things here. I just now read the issue summary and that talks about \Drupal\field_ui\Access\ViewModeAccessCheck (and the form mode equivalent). Despite their name, those actually control access to EntityViewDisplay entities, however, not EntityViewMode (and the form mode equivalent). We already discussed how to make those use the entity access system at #2866666: Add proper access handlers for view and form displays and decided to punt that to #2882273: Make (Form|View)ModeAccessCheck use the entity display access control handler. That last issue also seems to be similar to what @Berdir suggested in #7, although to be honest, I don't grok that comment 100%.

The latest patch, however, does actually patch the EntityViewMode (and form) entity type, not the display one(s).

So not sure what this issue is actually about. Having written this, I'm pretty confused, to be honest.

This is blocking 4 issues. All 4 of those are part of the 6 remaining blockers to #2824572: Write EntityResourceTestBase subclasses for every other entity type..

Bumping to major.

So not sure what this issue is actually about. Having written this, I'm pretty confused, to be honest.

Assigning to me to address that.

As an unexpected side effect, #2907654: No URL can be generated for the 'add-form' route of EntityViewMode and EntityFormMode entities unblocked #2843780 — see #2843780-32: EntityResource: Provide comprehensive test coverage for EntityFormMode entity. In that issue, we (or at least I) didn't realize there was a bug in the entity class for Entity(Form|View)Mode; now that that is fixed, that issue is also unblocked, hurray! And so is #2843781: EntityResource: Provide comprehensive test coverage for EntityViewMode entity!

We should still do this though, since it's nonsensical that the access logic for an entity type lives in a different module than the entity type.

This is now just not a blocker anymore. Also, #21 said this was blocking 4 issues, but it was blocking only 2, my bad!

Thank you for creating this issue to improve Drupal.

We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.

Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.

Thanks!