Missing default access for all comment fields

having a crack

Something like so?

valid fails, access controls++

Partial review, but for the majority of the patch.

1. index 7ea9b4c..7672470 100644
   --- a/core/lib/Drupal/Core/Template/Attribute.php
   
   --- a/core/lib/Drupal/Core/Template/Attribute.php
   +++ b/core/lib/Drupal/Core/Template/Attribute.php
   

   Are these changes really necessary/in scope?

2. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +53,46 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    // Only users with the administer comments permission can edit
   

   Double quotes around the permission name would increase legibility.

3. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +53,46 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    // No user can change read only fields.
   

   s/read only/read-only/

4. +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -78,7 +78,7 @@ public function getDisplayOrdinal(CommentInterface $comment, $comment_mode, $div
   -   * @param \Drupal\comment\CommentInterface[] $comments
   +   * @param array $comments
   

   Why this change?

5. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -0,0 +1,209 @@
   + * @group comment
   

   Please also tag with @group Access.

6. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -0,0 +1,209 @@
   +  protected function setUp() {
   

   Missing @inheritdoc.

7. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -0,0 +1,209 @@
   +  public function testAccessToAdministrativeFields() {
   +
   

   Unnecessary leading newline.

8. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -0,0 +1,209 @@
   +    // Create a comment against a test entity..
   

   Two periods, should be one.

9. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
   @@ -0,0 +1,209 @@
   +    $comment_enabled_user = $this->createUser(['name' => 'enabled'], array(
   ...
   +    $comment_no_edit_user = $this->createUser(['name' => 'no edit'], array(
   ...
   +    $comment_disabled_user = $this->createUser(['name' => 'disabled'], array('access content'));
   

   You use the shorthand array syntax for the first entry, but not for the second. Let's make that consistent? :)

10. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
    @@ -0,0 +1,209 @@
    +      // Checks on view operations.
    +      foreach ($test_users as $account) {
    +        $may_view = $comment1->{$field}->access('view', $account);
    +        $this->assertEqual($may_view, $account->hasPermission('access comments'), String::format('Any user may view the field @name.', array('@name' => $field)));
    +      }
    

    This does not test an unpublished comment.

11. +++ b/core/modules/comment/src/Tests/CommentFieldAccessTest.php
    @@ -0,0 +1,209 @@
    +      $may_update = $comment1->{$field}->access('edit', $comment_enabled_user);
    +      $this->assertFalse($may_update, String::format('Users with permission "edit own comments" is not allowed to the field @name.', array('@name' => $field)));
    +      $may_update = $comment2->{$field}->access('edit', $comment_enabled_user);
    +      $this->assertFalse($may_update, String::format('Users with permission "edit own comments" is not allowed to the field @name.', array('@name' => $field)));
    +      $may_update = $comment3->{$field}->access('edit', $comment_no_edit_user);
    +      $this->assertFalse($may_update, String::format('Users with permission "post comments" is not allowed to the field @name.', array('@name' => $field)));
    +      $may_update = $comment1->{$field}->access('edit', $comment_disabled_user);
    +      $this->assertFalse($may_update, String::format('Users with no comment permissions is not allowed to edit the field @name.', array('@name' => $field)));
    +      $may_update = $comment2->{$field}->access('edit', $comment_disabled_user);
    +      $this->assertFalse($may_update, String::format('Users not having permission "edit any page content" is not allowed to the field @name.', array('@name' => $field)));
    +      $may_update = $comment1->{$field}->access('edit', $comment_admin_user) && $comment3->status->access('edit', $comment_admin_user);
    +      $this->assertTrue($may_update, String::format('Users with permission "administer comments" may edit @name fields on all comments.', array('@name' => $field)));
    

    This is rather tricky to follow. You look at comments 1, 2, 3, 1, 2, 1 respectively. Wouldn't it be simpler to look at all combinations of all three comments and all three users, and set expectations for those combinations in advance, in an array?

12. +++ b/core/modules/comment/src/Tests/CommentLanguageTest.php
    @@ -112,14 +112,17 @@ function testCommentLanguage() {
    -        $cids = \Drupal::entityQuery('comment')
    +        $cid = db_select('comment_field_data', 'c')
    

    This feels like a regression…?

Yep unrelated hunks - here it is without them.
Will address other reviews tomorrow.
Fixes 7.12, 7.4, 7.1
Thanks for the review.

I started to do this as well in #2227503: Apply formatters and widgets to Comment base fields.

Note that the access logic needs to be in sync with the current #access logic on the form elements to be able to actually rely on this, I don't think this is currently the case, because it means that it depends on field settings and is different for editing and adding comments.

If we can't or don't want to do that, we need to find a different solution there (separate form modes? Not sure yet..)

Fixes #7
Using generatePermutations to cover all combinations actually found some missing parts - so great idea!
Not sure if I need to do anything here because of #9?

Tipping those fails pertain to #9

One effect/goal of this is to make the #access in form elements of the comment form unecessary, because switching to widgets also means that field access will automatically be respected.

However, if you look at the #access checks there, they are much more complicated. They depend on the comment field settings (if anon users can not, can or must fill out the fields, and it also depends on whether the comment is new or not (admins are not "allowed" to set the name for a new comment, but they can edit the name of a comment by an anomyous user).

So if we want to make the custom #access checks there unnecessary (note that something like quickedit will not be able to respect custom #access logic in the form, although not really problematic here I think), we need to implement that logic.

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -51,4 +53,49 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+    $entity = $items->getEntity();
+    if ($operation == 'view' && !$account->hasPermission('administer comments') && (!$account->hasPermission('access comments') || !$entity->isPublished())) {
+      return FALSE;
+    }
+    if ($operation == 'edit' && !$account->hasPermission('administer comments') && (!$account->hasPermission('edit own comments') || $entity->getOwnerId() != $account->id())) {
+      return FALSE;
+    }

In this case, editing the entity isn't possible right?

You don't need to check this. This is additionally to view/edit permission of the whole entity. Callers of field access() are expected to check that first if they are not already in a context where they can safely assume that this is possible (like an edit form).

Not sure what to do with the 'date' field - there is no 'date' field on Comment entity, so for now it stays as is?
Conversion to something else part of using a widget?

Yeah, date is changed to 'created' in my issue, just like we did for the node form.

Oh, you either need to keep the #access logic for now or check it yourself using $comment->field->access('edit'). Automatic checks only work for actual widgets. As I am converting that in my issue, I'd suggest to just leave it.

I think "date" should go away, maybe file a critical to fix it?

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -51,4 +53,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
+      'homepage',

homepage and subject could be changed by user, so homepage is not admin-field

#16 fixed
#17 fixed
Re 'date' » see #15

Anything more to do here?

Now looks good

Same mistake as znerol's patch a few days ago just even harder to see.

+    if ($operation == 'edit' && in_array($field_definition->getName(), $administrative_fields)) {

you wanted

+    if ($operation == 'edit' && in_array($field_definition->getName(), $administrative_fields, TRUE)) {

Good catch, opened #2337227: NodeAccessControlHandler performs non-strict in_array check too

Thanks!

Could you please delay committing this patch until #2287071: Add cacheability metadata to access checks is committed? That ~300K patch is very, very painful to reroll and this one will conflict. Sorry, and thanks for your consideration. I will help with rerolling this patch if you like.

re-roll

trivial re-roll, back to rtbc - #2287071: Add cacheability metadata to access checks still to go in

#2287071: Add cacheability metadata to access checks is in

fixed for #2287071: Add cacheability metadata to access checks and took out references to non-existent domains in test urls/email addresses

Back to RTBC

thanks @jibran
@Wim Leers can you eyeball my usage of the new api?

1. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    // Only admins or anonymous users can edit name and mail.
   +    if ($operation == 'edit' && in_array($field_definition->getName(), ['name', 'mail', 'homepage'], TRUE)) {
   

   The comment lists "name" and "mail", the code lists "name", "mail" and "homepage".

2. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +      return AccessResult::allowedIf($account->hasPermission('administer comments'))->cachePerRole()
   

   The combination of allowedIf() and cachePerRole() has a short hand: allowedIfHasPermission().

3. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +        ->orIf(AccessResult::allowedIf($entity->isNew() && $account->isAnonymous() && $anonymous_contact != COMMENT_ANONYMOUS_MAYNOT_CONTACT && $account->hasPermission('post comments'))
   +        ->cachePerRole()
   +        ->cacheUntilEntityChanges($entity)
   +        ->cacheUntilEntityChanges($field_definition->getConfig($commented_entity->bundle()))
   +        ->cacheUntilEntityChanges($commented_entity));
   

   This works, but it feels a little bit strange. You say "first access result OR second access result", which is good. But then you apply all these cache metadata manipulations to the OR result. But they actually belong *on* the second access result.

   So I'd change this code block to:

   $admin_access = AR::allowedIfHaspermission(…);
   $anon_access = AR::allowedIf()->cachePerRole()->cacheUntilEntityChanges()->cacheUntil…();
   return $admin_access->orIf($anon_access);
   

   That makes it easier to follow :) And reduces risk of introducing mistakes in the future!

4. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    if ($operation == 'view' && !$account->hasPermission('administer comments') && (!$account->hasPermission('access comments') || !$entity->isPublished())) {
   

   No comment for this one, even though there's a comment for everything else?

5. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +      return AccessResult::forbiddenIf(!$account->hasPermission('administer comments'))->cachePerRole()
   +        ->orIf(AccessResult::forbiddenIf(!$account->hasPermission('access comments') || !$entity->isPublished())
   +        ->cacheUntilEntityChanges($entity));
   

   Analogously here.

6. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +    return parent::checkFieldAccess($operation, $field_definition, $account, $items);
   

   The way this code falls back to the parent method is a bit problematic. It isn't in and of itself, but it is in combination with the above if-tests for the "view" and "edit" operations.

   Let's take a look at the "view" case.

   The "view" operation if-test above always takes permissions into account. That means that this is actually the "else" case for that same if-test, and therefore whatever the parent method returns should also be cached per role (->cachePerRole()), because it means the user *does* have a certain permission, which has affected why this access result (the one from the parent method) was chosen!

   As a general rule, it's better to write access checking logic as follows:

   if ($operation == 'view) {
     if ($some_condition && $permission_check) {
       return AccessResult::something()->cachePerRole();
     }
     else {
       return parent::access()->cachePerRole();
     }
   }
   
   return parent::access();
   

   rather than:

   if ($operation == 'view) {
     if ($some_condition && $permission_check) {
       return AccessResult::something()->cachePerRole();
     }
   }
   
   return parent::access();
   

   Because then it's much more obvious why the call to the parent method for a certain operation should associate certain cacheability metadata and not in another case.

   I hope that was sufficiently clear.

32.1 fixed
32.2 fixed
32.3 fixed
32.4 fixed
32.5 fixed
32.6 fixed in part need advise - see attached patch

Looks great, with one exception:

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -58,55 +58,61 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
-    if ($operation == 'view' && !$account->hasPermission('administer comments') && (!$account->hasPermission('access comments') || !$entity->isPublished())) {
-      return AccessResult::forbiddenIf(!$account->hasPermission('administer comments'))->cachePerRole()
-        ->orIf(AccessResult::forbiddenIf(!$account->hasPermission('access comments') || !$entity->isPublished())
-        ->cacheUntilEntityChanges($entity));
+
+    if ($operation == 'view') {
+      // Admins can view any fields, other users need both the "access comments"
+      // permission and for the comment to be published.
+      $admin_access = AccessResult::allowedIfHasPermission($account, 'administer comments');
+      $anonymous_access = AccessResult::allowedIf($account->hasPermission('access comments') && $entity->isPublished())
+        ->cacheUntilEntityChanges($entity);
+      return $admin_access->orIf($anonymous_access);
     }
     return parent::checkFieldAccess($operation, $field_definition, $account, $items);

Isn't this a functional change?

Before, when the if-test evaluated to FALSE, the parent's checkFieldAcces() method was called. That's no longer the case.

Same patch as at #34 discussed with @Wim Leers on irc

Question: should we be blocking access to mail and host name to other than admim

#35 was answered; it's not a functional change, it was a bug in earlier patches. From an AccessResult POV, this is solid & RTBC.

#37: Do you mean "homepage" instead of "hostname"? Nobody can edit the hostname. I'm also not sure what you mean, because anon users can edit their own mail and homepage in the current patch. Who else should be able to edit it?

Oh yes you are right w.r.t host-name, no issue there√
With mail, I'm talking about 'view' access, not edit access.
Pretty sure we say 'private' on the email field for anonymous users.
So I think from an access point of view, we should alter the 'view' logic to make that field only visible to admins.
Thoughts?

That sounds right to me, but I'm no Comment module expert.

Ok, fixes that.

don't forget to merge

1. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,68 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +      // Only admins or anonymous users can edit name, homepage and mail.
   

   This is an oddity of comment module, but anonymous users can't really edit name/homepage/mail - they get to fill in those fields on creation. Could we make the comment a bit clearer?

   Also can admin users really edit those fields even if the field isn't configured to allow them? That seems OK but also pointless.

2. +++ b/core/modules/comment/src/CommentAccessControlHandler.php
   @@ -51,4 +54,68 @@ protected function checkCreateAccess(AccountInterface $account, array $context,
   +      // Admins can view any fields, other users need both the "access comments"
   

   I don't think we should allow admins to view hostname by default. Currently we dump that in the database (and there's an issue to stop doing that at all), then never display it anywhere.

#45.2: I think this only makes it so that if there were a formatter enabled for it by default, it would only be visible for admin users. But since there isn't, there's no actual change in the UI. @larowlan, please confirm.

45.1 Fixed comment
45.2 Fixed

+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -107,11 +107,12 @@ protected function checkFieldAccess($operation, FieldDefinitionInterface $field_
+      $anonymous_access = AccessResult::allowedIf($account->hasPermission('access comments') && $entity->isPublished() && !in_array($field_definition->getName(), array('mail', 'hostname'), TRUE))
         ->cacheUntilEntityChanges($entity);

This also needs ->cachePerRole().

fixed - thanks

Committed ee083e0 and pushed to 8.0.x. Thanks!

diff --git a/core/modules/comment/src/CommentAccessControlHandler.php b/core/modules/comment/src/CommentAccessControlHandler.php
index 670bd8c..d22a0e4 100644
--- a/core/modules/comment/src/CommentAccessControlHandler.php
+++ b/core/modules/comment/src/CommentAccessControlHandler.php
@@ -13,7 +13,6 @@
 use Drupal\Core\Field\FieldDefinitionInterface;
 use Drupal\Core\Field\FieldItemListInterface;
 use Drupal\Core\Session\AccountInterface;
-use Drupal\field\Entity\FieldInstanceConfig;
 
 /**
  * Defines the access control handler for the comment entity type.

Fixed on commit - this class no longer exists and is not used :)