Missing access check for subscriber mail field

Yeah, a lot of contrib didn't bother with field-based access as that's a new thing and wasn't really required to get things working. Core is very inconsistent too, e.g. media has a bunch of open issues around this.

Happy to improve that, that said, I'm not quite sure how that applies to subscribers, as they as a whole should be equally restricted? Field access applies when different fields have different access, e.g. user name being public and user e-mail private. So field access is generally on top of entity access, so field access is only checked when you have access to the entity.

There doesn't seem to be a reason to create a view of subscribers where some of the data should be public? So the bigger issue there is that views defaults to no access checks unless you have a custom wizard that sets a default permission?

Entity access defaults to no access, unlike fields which default to being accessible. I don't think views checks entity access like you described it, or what you did would not have been possible if the only thing we have atm is an admin permission :)

So this patch does not restrict access, it opens it up based on a new permission.

1. +++ b/src/Entity/Subscriber.php
   @@ -356,18 +357,16 @@ class Subscriber extends ContentEntityBase implements SubscriberInterface {
            'weight' => 5,
   -      ))
   -      ->setDisplayConfigurable('form', TRUE);
   +      ));
   

   yeah, this doesn't make sense to be configurable, was there since the first 8.x commit basically and probably copy & pasted from somewhere.

2. +++ b/src/Form/SubscriberForm.php
   @@ -28,13 +28,14 @@ class SubscriberForm extends ContentEntityForm {
          '#title' => t('Active'),
          '#default_value' => $subscriber->getStatus(),
   +      '#disabled' => !$subscriber->status->access('edit'),
        );
   

   I prefer using get('status') especially in cases like this when you actually use methods on the FieldItemList object and benefit from autocomplete and code inspection.

3. +++ b/src/Form/SubscriptionsFormBase.php
   @@ -132,7 +132,6 @@ abstract class SubscriptionsFormBase extends ContentEntityForm {
    
        // Modify UI texts.
        if ($mail = $this->entity->getMail()) {
   -      $form['mail']['#access'] = FALSE;
          $form['subscriptions']['widget']['#title'] = t('Subscriptions for %mail', array('%mail' => $mail));
          $form['subscriptions']['widget']['#description'] = t('Check the newsletters you want to subscribe to. Uncheck the ones you want to unsubscribe from.');
        }
   diff --git a/src/SubscriberAccessControlHandler.php b/src/SubscriberAccessControlHandler.php
   

   the title was changed here because the e-mail wasn't visible anymore I think, so maybe that part isn't necessary anymore? not 100% related, so possibly better left for some other UI improvement issue.

4. +++ b/src/SubscriberAccessControlHandler.php
   @@ -0,0 +1,89 @@
   +    // keep a record of the subscription history.
   +    $is_own_account = $account->isAuthenticated() && ($entity->getUserId() == $account->id());
   +    if ($is_own_account && ($operation != 'delete') && $account->hasPermission('subscribe to newsletters')) {
   +      return AccessResult::allowed()->addCacheableDependency($entity)->cachePerPermissions();
   +    }
   +
   

   this access check does also vary on the user not just the entity. if either the user or the entity is different, then the access result may be different.

   Also, the cache contexts need to be outside of at least some of these conditions, imagine this:

   * user A who is not the user connected to subscription 1 tries to edit it, he is not owner, => access denied.
   * user B who is the connected user tries to access it but the previous check was cached without considering the current user, so access is denied.

   What you can do to optimize it is to check the permission and operation first, then add the cache contexts, then check if it is his subscription.

   Allowing this certainly makes sense for e.g. decoupled scenarios but the question is how the user finds "his" subscription in the first place, without being allowed to list them :) Because if you compare that with the default UI, that is always exposed through local tasks/a page/blocks where we look up the entity for the user. So to be able to actually make this work for decoupled sites, we'd need some kind of API that allows users to either get the ID of their subscription or a special route that works similar to the page and looks up the right subscription.

   Just saying that implementing conceptually correct but not very practical ye.

Other than that stuff, this looks good, I'd just like to point out one last time that this still does not lock down access as the issue summary claims except denying edit access for the mail/langcode field if connected to a user. Everything actually adds cases where we allow more access than before. Currently we rely on "admin_permission = "administer simplenews subscriptions",", which means *anything* is only possible with that permission, now we also respect own access as well as viewing subscriptions.

Will check again later, to be clear, my last comment was simply about the issue summary being out of date and incorrect and we should update it because it is confusing. It does not and can not fix the mentioned views problem, the only thing that would help with that is if we'd provide a views wizard thing that sets a default access check. I agree with the changes here, but don't think it fixes any security issues. That's all I'm trying to say. That this is more a task/refactoring than a (major) bugfix.

Feel free to go ahead.

> Views makes a call to checkFieldAccess with items = NULL and if that fails it hides the email column.

True. My point is that having a subscriber view exposed to anyone makes no sense in the first place. Yes, it does hide the e-mail field, but what I was trying to get at is the the view as a whole should default to a sensible permission. Certain columns might still remain visible and configurable fields would too. But the only way to have that is a custom views wizard thingy that sets a default permission, an access control handler can't fix that.