Use comment access handler instead of hardcoding permissions

This is a significant architectural tidyup with possible BC implications, would be good to have maintainer input early.

I agree that having this done would be amazing, I have a question to kick off the discussion.

Here's a piece of code from comment.module:

/**
 * Implements hook_node_search_result().
 *
 * Formats a comment count string and returns it, for display with search
 * results.
 */
function comment_node_search_result(EntityInterface $node) {
  $comment_fields = \Drupal::service('comment.manager')->getFields('node');
  $comments = 0;
  $open = FALSE;
  foreach ($comment_fields as $field_name => $info) {
    // Skip fields that entity does not have.
    if (!$node->hasField($field_name)) {
      continue;
    }
    // Do not make a string if comments are hidden.
    $status = $node->get($field_name)->status;
    if (\Drupal::currentUser()->hasPermission('access comments') && $status != CommentItemInterface::HIDDEN) {
      if ($status == CommentItemInterface::OPEN) {
        // At least one comment field is open.
        $open = TRUE;
      }
      $comments += $node->get($field_name)->comment_count;
    }
  }
  // Do not make a string if there are no comment fields, or no comments exist
  // or all comment fields are hidden.
  if ($comments > 0 || $open) {
    return ['comment' => \Drupal::translation()->formatPlural($comments, '1 comment', '@count comments')];
  }
}

The comment entity is never loaded in this hook, which seems fair enough because the only requirement (as it currently stands) is for the search result to show the number of comments on the node.

I *think* that if we wanted to use Entity::access here, we would need to load all of the comments on the node and iterate through them, checking to see if any of them were viewable by the user.

So what do we do?

Perhaps put the access logic into the field handler somehow and alter the output of $node->get($field_name)->comment_count?

Gah, if only that code could die. Does adding a comment count to a search result really add any value :(

It's not the only place where comment_count is used to imply access. Here's another example from CommentDefaultFormatter:

  /**
   * {@inheritdoc}
   */
  public function viewElements(FieldItemListInterface $items, $langcode) {
    $elements = [];
    $output = [];

    $field_name = $this->fieldDefinition->getName();
    $entity = $items->getEntity();

    $status = $items->status;

    if ($status != CommentItemInterface::HIDDEN && empty($entity->in_preview) &&
      // Comments are added to the search results and search index by
      // comment_node_update_index() instead of by this formatter, so don't
      // return anything if the view mode is search_index or search_result.
      !in_array($this->viewMode, ['search_result', 'search_index'])) {
      $comment_settings = $this->getFieldSettings();

      // Only attempt to render comments if the entity has visible comments.
      // Unpublished comments are not included in
      // $entity->get($field_name)->comment_count, but unpublished comments
      // should display if the user is an administrator.
      $elements['#cache']['contexts'][] = 'user.permissions';
      if ($this->currentUser->hasPermission('access comments') || $this->currentUser->hasPermission('administer comments')) {
        $output['comments'] = [];

        if ($entity->get($field_name)->comment_count || $this->currentUser->hasPermission('administer comments')) {

// and so on...

...and I suspect that Drupal.org is using that comment count on our dashboards:

And on search too.

Does it add to the usability of search results? Anecdotally I would say that I have picked search results that have more comments on them from time to time as it can indicate that a more robust conversation can be found on that particular post in a blog or forum. It would be nice if we could kill it but I suspect that would make a number of people rather unhappy.

Seems to me that hasPermission('access comments') is right in the case of #7.

We want to say nothing if the user can't access comments but "0 comments" if they could access (at least some) comments in principle but it just happens that there aren't any yet. So we can't use $entity->access('view') because we want to return true even if there's no specific comment entity to inquire about.

The example does make we wonder if there's some missing "CommentAccess" service(s) that could centralise and DRY some of these esoteric logic checks. Like "does this entity have any accessible comments" which is surely a useful question in more cases than just adding comment counts to node search results. Such a service would also make it much easier for contrib to add more fine-grained comment access control.

Maybe the first thing to do would be to create such a service, then as a second step modify it to use entity access where possible.

Seems to me that hasPermission('access comments') is right in the case of #7.

I would agree if the comment_count contained all possible comments, including unpublished ones. Which would of course change a couple of things about how that count is used. By only containing published comments the count is automatically proxying access rights.

"does this entity have any accessible comments" which is surely a useful question in more cases than just adding comment counts to node search results.

Yes. A thousand times yes.

A use-case I am working on for a client right now is that they want more granular comment permissions such as:

• Reply to comments on own entity
• Publish/Unpublish comments on own entity

I've had to override several routes, some methods on the CommentController, the default field formatter, and the storage just to implement the new permissions (I'm not including the new code for additional actions such as Unpublish). All of this could be contained in CommentAccessHandler or the service you've proposed.

For my education - why a service rather than inclusion in the access handler? Is it because it doesn't relate directly to the comment entity?

For my education - why a service rather than inclusion in the access handler?

Beyond the scope of my knowledge - education needed for me too!

I think this issue should go more far, than initially proposed. AccessControlHandler would be needed on different levels: e.g.:

• global level
• comments-field level (?)
• comment-type level according to #1903138: Move global comment permissions to comment-type level
• single comment level according to #2858157: CommentDefaultFormatter does not respect custom access control
• comments on special node types, e.g. pluggable access control for articles

CommentAccessControlHandler will need to respect all these levels. Or there should be different pluggable AccessControlHandler, which can cooperate.

First of all, I think, that we should not force usage of $comment->access creating a comment entity Comment if it does not exist in given context, hence it is an expensive procedure.

Secondly, we need to use access method of comments-field CommentFieldItemList when known to proxy to access method of comment entities:

• 'view' access on comments-field might mean 'view' access to at least one comment inside of this field
• 'create' access on comments-field might mean appending new comment to list, so would equal to 'create' access on comment entity

So there are results of my scanning and replacing for hasPermission() calls in comment module:

1. Permission 'edit own comments':

Have not appeared in any parts of code accept in access control handler CommentAccessControlHandler, which is fine.

2. Permission 'post comments':

Case A1: When used in context of comment-entity, like in CommentForm::save, replace by:

Check for 'create' access on comment-entity:

$comment->access('create', $account)

Case A2: When used in context of comments-field, but whithout knowing comment-entity, like in CommentLinkBuilder::buildCommentedEntityLinks, replace by:

Check for 'create' access on access control handler without using comment-entity:

\Drupal::entityTypeManager()->getAccessControlHandler('comment')->createAccess($bundle, $account);

Or alternatively:

Check for 'create' access on comments-field, which acts in sense of appending new comment-entity to the list:

$field->access('create', $account)

Special Case A2.1: Checking 'create' access for authenticeted user role in CommentManageer::forbiddenMessage. Since EntityAccessControlHandler (as parent class of CommentAccessControlHandler) will cache access per uid, it is not possible to load some authenticated user and abuse it for check... So I tried to simulate an general authenticated user by creating a dummy user (UserSession) with uid = '-1', and to check access using that. Thus actually violates uid definition of user, but works fine. Needs review please!

$account = new UserSession(['uid' => -1, 'roles' => ['authenticated']]);
$field->access('create', $account);

Case A3: When used globally without knowing comments-field nor comment-entity, like in CommentController::replyFormAccess, replace by:

Check for 'create' access on access control handler without using comment-entity:

\Drupal::entityTypeManager()->getAccessControlHandler('comment')->createAccess($bundle, $account);

Special Case A3.1: Checking 'create' access for anonymous user role in CommentItem::fieldSettingsForm. I coped with it creating an anonymous user, which much more stable than Special Case A2.1:

$account = new AnonymousUserSession();
\Drupal::entityTypeManager()->getAccessControlHandler('comment')->createAccess($bundle, $account);

3. Permission 'access comments':

Case B1: When used in context of comment-entity, like in CommentController::replyFormAccess, replace by:

Check 'view' access on comment-entity:

$comment->access('view', $account);

Case B2: When used in context of comments-field, but without comment-entity (like in #7), replace by:

Check 'view' access on comments-field, which equals to 'view' comments

$field->access('view', $account);

Case B3: When used globally without having comments-field or comment-entity in context...

Well, there is no real replacement, since 'view' access check needs comment-entity to run correctly.
Thus is used once in comment.module::comment_node_update_index to restrict indexing of comments:

  if ($index_comments === NULL) {
    // Do not index in the following three cases:
    // 1. 'Authenticated user' can search content but can't access comments.
    // 2. 'Anonymous user' can search content but can't access comments.
    // 3. Any role can search content but can't access comments and access
    // comments is not granted by the 'authenticated user' role. In this case
    // all users might have both permissions from various roles but it is also
    // possible to set up a user to have only search content and so a user
    // edit could change the security situation so it is not safe to index the
    // comments.
    $index_comments = TRUE;
    $roles = \Drupal::entityManager()->getStorage('user_role')->loadMultiple();
    $authenticated_can_access = $roles[RoleInterface::AUTHENTICATED_ID]->hasPermission('access comments');
    foreach ($roles as $rid => $role) {
      if ($role->hasPermission('search content') && !$role->hasPermission('access comments')) {
        if ($rid == RoleInterface::AUTHENTICATED_ID || $rid == RoleInterface::ANONYMOUS_ID || !$authenticated_can_access) {
          $index_comments = FALSE;
          break;
        }
      }
    }
  }

IMHO it is basicly wrong way to deny indexing on all comments, only when some users may not be allowed to access them at all.
I think, that all comments should be indexed in general, and then access should be checked later on 'view' oder 'view label' access of node.
For exapmple, do not show such node in search results, when search matches a comment, but there is no access to it.

This will need a general change in displaying search results and should not be treated by this issue.

Providing new patch with corrected pathes

Correcting patch due to test results #22.

Same procedure as every year...

Correcting patch due to testing results of #25:

• CommentAnonymousTest => Bug fix
• CommentNonNodeTest => Bug fix
• CommentLinkBuilderTest => Adapt test to new requirements

This is impressive work. The basic approach seems sound.

The problems touched on in the discussion about the access control for lists of comments are hard, but it's right to ignore them for now. Hopefully something like #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() will give us ways to improve this in the future.

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,73 @@ public function offsetExists($offset) {
   +        // Allow if ‘view’ access on comment is allowed
   +        $result = $comment->access($operation, $account, $return_as_object);
   

   $operation defaults to 'view' but isn't necessarily 'view'.

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -372,7 +372,7 @@ public function save(array $form, FormStateInterface $form_state) {
   +    if ($comment->access('create', $this->currentUser) && ($this->currentUser->hasPermission('administer comments') || $entity->{$field_name}->status == CommentItemInterface::OPEN)) {
   

   Can we centralise the 'administer comments' check into a checkAdministerAccess method on the CommentAccessHandler? That would increase consistency, and make it easier to override (e.g. to help with more granular per-bundle access control in future).

3. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -203,7 +206,7 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -              ->getNewCommentPageNumber($entity->{$field_name}->comment_count, $new_comments, $entity, $field_name);
   +              ->getNewCommentPageNumber($field->getValue('comment_count'), $new_comments, $entity, $field_name);
   

   Isn't this change unrelated and unnecessary?

4. +++ b/core/modules/comment/src/CommentManager.php
   @@ -133,19 +133,22 @@ public function addBodyField($comment_type_id) {
   -      if ($entity->get($field_name)->getFieldDefinition()->getSetting('form_location') == CommentItemInterface::FORM_SEPARATE_PAGE) {
   +      if ($field->getFieldDefinition()->getSetting('form_location') == CommentItemInterface::FORM_SEPARATE_PAGE) {
   

   Again unrelated. This is a complex patch and getting it reviewed will be really hard; it's important to keep the changed hunks to a minimum.

5. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -276,8 +276,11 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +    // Check if the user has the proper creapermissions.
   

   'create permission'?

$operation defaults to 'view' but isn't necessarily 'view'.

Indeed. I would change comment in this case. Method might be used for 'update' operation, too.

Can we centralise the 'administer comments' check into a checkAdministerAccess method on the CommentAccessHandler? That would increase consistency, and make it easier to override (e.g. to help with more granular per-bundle access control in future).

Good idea!

Isn't this change unrelated and unnecessary?

Conversion from $entity->{$field_name} to $field is not necessary, but reasonable. $entity->{$field_name} will involve get() method on entity each time. Where $field just reference an object in local scope.

But conversion from ->comment_count to ->getValue('comment_count') was indeed necessary to pass CommentLinkBuilderTest test. This test case was designed to pass sdtClass instead of FieldItemListInterface. Hence it was not possible to call access() method on it, which lead to failure. I had to adapt this test with mocking all necessary methods. Mocking magic mathod __get() to keep access to property via ->comment_count is not possible IMHO.

Again unrelated. This is a complex patch and getting it reviewed will be really hard; it's important to keep the changed hunks to a minimum.

Again, just a small optimization to avoid calling get() method multiple times.

'create permission'?

Yes.

@a_grizzli interesting approch, quickly skimed patch and it looks promissing

Not clear how access handlers can replace permissions but I suggest to file new issue to define and document access control on comments, for example formatter doing access checks on comment entity which could not exists (field ui when editing the field default value)
This case and views needs to be covered and not clear how comments could get benefit from #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter()

Can we centralise the 'administer comments' check into a checkAdministerAccess method on the CommentAccessHandler?

Moving implementation makes no sense to me because it will replicate "account->hasPermission()" call for no reason

Can we centralise the 'administer comments' check into a checkAdministerAccess method on the CommentAccessHandler?

Moving implementation makes no sense to me because it will replicate "account->hasPermission()" call for no reason

Comment permissions logic is complex and dispersed; centralising it in one place makes it easier to swap it out to meet the needs of a use case. This would also help issues like #1903138: Move global comment permissions to comment-type level which needs to change what permissions are involved in comments.

I like the approach here, thanks for keeping this moving.

We need some additional docs around 'view only' because it is a special case, and I agree we should probably make 'view' the lowest operation, given its the default. So that would mean view would be used instead of view only, and we'd need a new operation to signify 'view and comment'.

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,73 @@ public function offsetExists($offset) {
   +      // necessery to show comment form, so in case of 'create' access too.
   

   nit: necessary

2. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,73 @@ public function offsetExists($offset) {
   +      if ($cid != 0) {
   

   nit: !==

But conversion from ->comment_count to ->getValue('comment_count') was indeed necessary to pass CommentLinkBuilderTest test.

@jonathanshaw You were right! It was bad idea to use getValue(), since it actually does a completely wrong thing. Seems I was more concentrated on passing test...

I agree we should probably make 'view' the lowest operation, given its the default.

It might become a little bit more complicated, since EntityViewDisplay::buildMultiple uses access('view', ...); by default instead of 'view and comment' (better 'view or create'). We would need to add an instanceof check especially for CommentFieldItemList here.

Patch passed Drupal tests.

It might become a little bit more complicated, since EntityViewDisplay::buildMultiple uses access('view', ...); by default instead of 'view and comment' (better 'view or create'). We would need to add an instanceof check especially for CommentFieldItemList here.

I gave it a try. Attached a new alternative patch. And it seems that there were not many changes necessary. Only in:

• EntityViewDisplay::buildMultiple
• CommentDefaultFormatter::viewElements
• CommentFieldItemList::access

Furthurmore, old 'view' checks using $field->access('view', ...); have been unnecessary far-reaching and only have not produced any troubles because of context used in, which is the same reason for little changes to new 'view' operation.

However, this change affects a core entity class beyond comments-only scope.

We need some additional docs around 'view only' because it is a special case [...]

Docs revised in new patch.

As for #30:

Comment permissions logic is complex and dispersed; centralising it in one place makes it easier to swap it out to meet the needs of a use case. This would also help issues like #1903138: Move global comment permissions to comment-type level which needs to change what permissions are involved in comments.

At the moment changing 'administer comments' is not within the scope of #1903138: Move global comment permissions to comment-type level. But 'skip comment approval' is considered. However, it is used only once. Centralising might be a topic for future development in this case maybe (?).

What I am trying to say is, that if we start centralising 'administer comments' we should consider all permissions in accordance with comments in general. Issue summary have to be updated also.

Examining occurencies of 'administer comments'... Like in case of other permissions there are occurencies in all contextes. Please refer to #18:

1. used in context of comment-entity
2. used in context of comments-field, but without knowing comment-entity
3. used in context of user account or role only, but without knowing comments-field nor comment-entity

The solution might be similar to calling CommentAccessControlHandler::createAccess for 'create' access:

// For checking depending from an account
\Drupal::entityTypeManager()->getAccessControlHandler('comment')->checkAdministerAccess($bundle, $account);
// For checking depending from a role
\Drupal::entityTypeManager()->getAccessControlHandler('comment')->checkAdministerAccess($bundle, $role);

Indeed, CommentAccessControlHandler already use 'administer account' checks itself, so bundling it in a public method would be a simple.

Same could be applied to 'skip comment approval', e.g. checkApproveAccess

Furthermore, same could be applied for yet not solved (not patched) problem within comment.module::comment_node_update_index() with 'access comments' permission check, e.g. checkViewAccess

+++ b/core/lib/Drupal/Core/Entity/Entity/EntityViewDisplay.php
@@ -246,8 +248,15 @@ public function buildMultiple(array $entities) {
+          $operation = 'view';
+          if ($items instanceof CommentFieldItemList) {
+            // Comments field needs a special permission check, since viewing it
+            // might also mean just to view comment form, which needs 'create'
+            // access on comment entity. @see CommentFieldItemList::access()
+            $operation = 'view or create';
+          }

No way should we do this. Hardcoding comment specific logic into EntityViewDisplay is not ok.

I've no idea what to propose though.

Centralising might be a topic for future development in this case maybe

That's fine. It's good to keep each patch small and simple rather than doing too much at once. Let's leave 'administer comments' out of this patch.

No way should we do this. Hardcoding comment specific logic into EntityViewDisplay is not ok.

D'accord.

Another possibility would be, to define a new method, e.g. getLowestOperation() somewhere in scope of FieldItemList object (?). Such method might return 'view' by default and could be overwritten by CommentFieldItemList to return 'view only'. And then to call it in EntityViewDisplay:

$field_access = $items->access($items->getLowestOperation(), NULL, TRUE);

Maybe a method to explore possible operations would be nice also, like getOperations().

I don't now yet, where it might be placed. Whether in FieldItemListInterface or AccessibleInterface. In any case thus should not be solved in this issue.

I propose, to get along with 'view only' operations first and make new issues for operations exploration (in core) and correcting 'view only'.

In this case, I would provide a new patch, which also corrects this:

Furthermore, old 'view' checks using $field->access('view', ...); have been unnecessary far-reaching and only have not produced any troubles because of context used in, which is the same reason for little changes to new 'view' operation.

That's an interesting idea, but I think it might be good to see what @larowlan says.

Anything that goes beyond comment is massively increasing the scope of this issue so it may be best left to a follow-up issue in order to keep this one tight.

Ok.

Nevertheless, here the promised patch, which uses 'view only' consistently as the lowest operation. This will help to change it later by just looking for this term.

Ups... Overseen to adapt CommentLinkBuilderTest for 'view only'. Next try.

What's novice-friendly about this issue?

Patch failed to apply and needs to be re-rolled.

So we should be tagging it 'Needs reroll' I think rather than the more general 'Novice'

Here's a reroll of 42.

Sorry, that patch was wrong.

Some review comments. Many of these are coding standard related nits which you could prevent by installing an automatic coding standard checker. See https://www.drupal.org/docs/develop/standards/coding-standards and possibly also https://www.drupal.org/docs/develop/development-tools

1. -    if (\Drupal::currentUser()->hasPermission('access comments') && $status != CommentItemInterface::HIDDEN) {
   +    // Check 'view' access on CommentFieldItemList, which equals to 'view' comments
   +    $access = $node->{$field_name}->access('view only', \Drupal::currentUser());
   +    if ($access && $status != CommentItemInterface::HIDDEN) {
   

   The comment is too long for the line and needs a period at the end.

2.    public function access($operation = 'view', AccountInterface $account = NULL, $return_as_object = FALSE) {
   +    // Decide to choose account
   +    $user = $account ?: \Drupal::currentUser();
   

   1. The comment needs a period. Also, I don't really understand it. What is it meant to clearify?
   2. Shouldn't we be injecting the current user instead of calling \Drupal?

3. +  /**
   +   * Check access on last comment
   +   *
   +   * @param string $operation
   +   * @param \Drupal\Core\Session\AccountInterface $account
   +   * @param bool $return_as_object
   +   *
   +   * @return bool|AccessResult
   +   */
   +  public function lastCommentAccess($operation = 'view', AccountInterface $account, $return_as_object = FALSE) {
   +    // If there are no comments, then access does not matter
   +    $result = AccessResult::allowed();
   +    // Load last comment in thread
   +    $last_comment_id = $this->first()->getValue();
   +    if (isset($last_comment_id['cid'])) {
   +      $cid = intval($last_comment_id['cid']);
   +      if ($cid !== 0) {
   +        $comment = Comment::load($cid);
   +        // Allow if access on comment is allowed
   +        $result = $comment->access($operation, $account, $return_as_object);
   +      }
   +    }
   +
   +    return $result;
   +  }
   +
   

   1. The docblock needs a period at the end of the short description.
   2. The @param entries need descriptions
   3. Arguments with default values must be at the end of the argument list. IOW, $account should not be the middle argument, but the first.
   4. All inline comments need closing periods.

4.    public function forbiddenMessage(EntityInterface $entity, $field_name) {
   +    // Get comments-field
   +    $field = $entity->get($field_name);
   

   Comment needs a period.

5. +      // permission to post comments by logging in,
   +      // so check 'create' access for authenticated user using CommentAccessControlHandler (proxying over field).
   +      // Simulate authenticated user by creating a dummy user session with uid = '-1',
   +      // which is actually not valid, but avoid real user access cache
   +      $dummy_authenticated_user = new UserSession(['uid' => -1, 'roles' => ['authenticated']]);
   +      $this->authenticatedCanPostComments = $field->access('create', $dummy_authenticated_user);
   

   Some lines in this comment are longer than 80 characters and a period is missing at the end.

6. +      // Check if the user has the proper permissions to view parent comment.
   +      // If there is no valid comment, than do not allow action
   +      $access = AccessResult::forbidden();
   +      if (!empty($comment)) {
   +        $access = $comment->access('view', $account, TRUE);
   +      }
   

   The second comment line... you guessed it... needs a period.

7. +    // Check access for anonymous user
        $anonymous_user = new AnonymousUserSession();
   +    $anonymous_user_access = \Drupal::entityTypeManager()
   +      ->getAccessControlHandler('comment')
   +      ->createAccess($this->getSetting('comment_type'), $anonymous_user);
   

   Same.

Only patch reroll

Sorry, I forgot to remove the tag. I am curious what patch you rerolled and based on what, exactly...

Latest patch didn't apply. I reworked the patch.

im trying to find the right hook to prevent some comments from loading, will this patch help?

webberly, this patch should allow you to control who some comments load for.

Thank you Darren,
im still not sure the best way to deny access to a individual comment and stop it from being rendered from a module.
There should be a mymodule_comment_access hook.

Fixing feedbacks from #52.

About dependency injection, I don't think it is possible for typed data, there is an issue for it here:
#2053415: Allow typed data plugins to receive injected dependencies
I think it is outside the scope of this issue to fix it.

I also found another related issue, CommentLazyBuilders builds links always checking access on the default language instead of the specific translation. I've made a new issue and patch for it here: #3134223: CommentLazyBuilders should be language aware

I'm using the latest patch and it seems good enough.

However, sometimes for some reason I have orphan comments that brings the following error:

Error: Call to a member function access() on null in Drupal\comment\CommentFieldItemList->lastCommentAccess() (line 109 of /var/www/codebase/web/core/modules/comment/src/CommentFieldItemList.php)

Here is a patch with one line changed (interdiff attached).

@Matroskeen this fix is out of scope, see related

Add back to NR for #61

This is looking good, some observations

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,72 @@ public function offsetExists($offset) {
   +  public function lastCommentAccess(AccountInterface $account, $operation = 'view', $return_as_object = FALSE) {
   

   We don't use this other than for 'view' operation. So do we need to support the $operation argument here?

   Also, do we really want this method to be public? (I think we should keep it internal to the class)

2. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,72 @@ public function offsetExists($offset) {
   +        $result = $comment->access($operation, $account, $return_as_object);
   

   in theory there could be N comments for a particular entity, should we be checking that at least one of them returns access allowed?

   e.g. in theory the last comment could be unpublished, which would mean the whole thread was not visible. If that's the case, we're missing some test coverage here

3. +++ b/core/modules/comment/src/CommentManager.php
   @@ -167,19 +167,23 @@ public function addBodyField($comment_type_id) {
   +      $dummy_authenticated_user = new UserSession(['uid' => -1, 'roles' => ['authenticated']]);
   

   this should use the constant instead of 'authenticated'

   is there a perfomance cost for this?

4. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -314,11 +315,14 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   -      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'access comments'));
   ...
   +        $access = $comment->access('view', $account, TRUE);
   

   shouldn't this be 'andIf' as per above?

5. +++ b/core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
   @@ -176,13 +176,14 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   -      if ($this->currentUser->hasPermission('access comments') || $this->currentUser->hasPermission('administer comments')) {
   +      if ($items->access('view only', $this->currentUser) || $this->currentUser->hasPermission('administer comments')) {
   ...
   -        if ($entity->get($field_name)->comment_count || $this->currentUser->hasPermission('administer comments')) {
   +        if ($items->comment_count || $this->currentUser->hasPermission('administer comments')) {
   

   should the administer comments permission also be moved to behind the field access method?

Thanks for all the work here!

Removed tags:

• Needs subsystem maintainer review: The subsystem maintainer reviewed and accepted the approach in #31 and, again, in #70 but for some reasons the tag has been re-added in #41. The subsystem maintainer agrees with this architectural change, so we just need to RTBCable patch.
• Needs tests: The patch has tests.

Here's my review and I'm also assigning to fix the issues from #70 and here:

1. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,11 +294,12 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +    // Check if the user has the proper 'create' permissions.
   +    $field = $entity->{$field_name};
   +    $access = $field->access('create', $account, TRUE);
   

   The comment doesn't reflect the code. Maybe

   // Check that the user is able to create comments.
   

   We shouldn't talk about permission, that's an implementation detail of the access handler.

2. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -306,11 +307,14 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +      // Check if the user has the proper permissions to view parent comment.
   

   Same here, let's avoid "permission", we're now using the entity access handler and that could be swapped to ignore the permissions and use its custom logic.

3. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +44,71 @@ public function offsetExists($offset) {
   +      // This operation is only used by EntityViewDisplay::buildMultiple().
   +      // In this case check both: 'view' or 'create' access, since this field
   +      // is considered as composition of listed comments and comment form.
   +      // Decision about to show only comments or only comment form or both
   +      // will be made by CommentDefaultFormatter::viewElements() later.
   +      // Uses recursive calls on same method invoking lower operations.
   ...
   +      // In contrast to 'view', this operation is used as the lowest
   +      // operation by various methods to check only single permission on last
   +      // comment.
   ...
   +      // In contrast to 'view', this operation is used as the lowest
   +      // operation by various methods to check only single 'create' permission
   +      // on comment entity.
   
   +++ b/core/modules/comment/src/CommentManager.php
   @@ -151,19 +151,23 @@ public function addBodyField($comment_type_id) {
   +      // permission to post comments by logging in, so check 'create' access
   +      // for authenticated user using CommentAccessControlHandler (proxying
   +      // over field).
   +      // Simulate authenticated user by creating a dummy user session with uid = '-1',
   +      // which is actually not valid, but avoid real user access cache.
   

   Should be rearranged according to https://www.drupal.org/docs/develop/standards/api-documentation-and-comm...

   Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over (...)

4. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,11 +294,12 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +    $access = $field->access('create', $account, TRUE);
   

   If this check returns forbidden or neutral, does it make sense to continue, or should we exit early? If we continue, regardless of future checks, the final access cannot be allowed.

5. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,11 +294,12 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +    $status = $status = $field->status;
   

   Duplicate assignment to $status var.

6. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,11 +294,12 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
        $access = $access->andIf(AccessResult::allowedIf($status == CommentItemInterface::OPEN)
          ->addCacheableDependency($entity))
          // And if user has access to the host entity.
   

   If we, previously, exit on forbidden/neutral, in this point $access is allowed. So the first ->andIf() makes no sense. And the $entity cacheability should be applied to the result of both checks as both are depending on $entity cacheability.

   Also, does it make sense to continue if this check results as forbidden/neutral (i.e. either the comments are closed or the user cannot access the host entity)? The following part is about replies but I don't think you can reply to a comment when the comments are closed or you cannot view the host entity. So, I would exit here if this check is not allowed.

7. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -306,11 +307,14 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +        $access = $comment->access('view', $account, TRUE);
   

   Would it make sense to continue if the parent comment doesn't exist? Also, if we exit here we don't need to perform, later, 2 additional checks for $comment.

8. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -306,11 +307,14 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
          // Check if the parent comment is published and belongs to the entity.
          $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
          if ($comment) {
   

   All these checks can be simplified if we exit early when $comment doesn't exist. Also we can process the boolean result of $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id() && $comment->access('view', $account) and use only one AccessResult::allowedIf(). That would add more clarity.

9. +++ b/core/modules/comment/comment.module
   @@ -428,7 +428,8 @@ function comment_node_search_result(EntityInterface $node) {
   -    if (\Drupal::currentUser()->hasPermission('access comments') && $status != CommentItemInterface::HIDDEN) {
   +    $access = $node->{$field_name}->access('view only', \Drupal::currentUser());
   +    if ($access && $status != CommentItemInterface::HIDDEN) {
   

   Isn't the $status check faster?

10. +++ b/core/modules/comment/src/CommentLinkBuilder.php
    @@ -157,10 +158,12 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
    +              $separate_form_location = $comment_form_location == CommentItemInterface::FORM_SEPARATE_PAGE;
    

    Let's be strict.

• #70.3:
  

  is there a performance cost for this?

  I cannot see why this change would bring a performance cost.

• #70.5:
  

  should the administer comments permission also be moved to behind the field access method?

  I think the approach from the patch is correct. A user granted with such a permission cannot be denied by an access handler. It's kind of "superuser for comments".

• #70.4:
  

  shouldn't this be 'andIf' as per above?

  I have more remarks on the whole method, in #71:4, 5, 6, 7, 8. I'm proposing a different approach.

I've fixed all remarks except #70.2. I need more time on that.

Needs subsystem maintainer review: The subsystem maintainer reviewed and accepted the approach in #31 and, again, in #70 but for some reasons the tag has been re-added in #41.

I added in #41 to get a verdict on #37 / #38, which #70 did not give. Rereading now, it looks like we can ignore it for now, it's possible follow-up material.

The patch in #65 is green, so if #71.4 and #71.6 are valid is there a hole in the test coverage?

Lastly, not sure that this is a bug.

This issue has plenty of tricky considerations. I'm concerned we might be hindering getting it committed by changing things that are not strictly in scope. In case a committer declines to commit because of this, I'll list here the valid minor changes that could be removed:

1. +++ b/core/modules/comment/comment.module
   @@ -428,7 +428,7 @@ function comment_node_search_result(EntityInterface $node) {
   -    if (\Drupal::currentUser()->hasPermission('access comments') && $status != CommentItemInterface::HIDDEN) {
   +    if ($status !== CommentItemInterface::HIDDEN && $node->{$field_name}->access('view only', \Drupal::currentUser())) {
   

   Scope creep: Change to strict comparison

2. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -93,7 +93,8 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -      $commenting_status = $entity->get($field_name)->status;
   +      $field = $entity->get($field_name);
   +      $commenting_status = $field->status;
   
   @@ -101,10 +102,10 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -            if (!empty($entity->get($field_name)->comment_count)) {
   ...
   +            if ($field->comment_count > 0) {
   ...
   -                'title' => $this->formatPlural($entity->get($field_name)->comment_count, '1 comment', '@count comments'),
   +                'title' => $this->formatPlural($field->comment_count, '1 comment', '@count comments'),
   
   @@ -116,7 +117,7 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -                    'data-history-node-last-comment-timestamp' => $entity->get($field_name)->last_comment_timestamp,
   +                    'data-history-node-last-comment-timestamp' => $field->last_comment_timestamp,
   

   All the changes from $entity->get() to $field are technically scope creep.

3. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -157,10 +158,12 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -              if ($comment_form_location == CommentItemInterface::FORM_SEPARATE_PAGE || (!empty($entity->get($field_name)->comment_count) && $this->currentUser->hasPermission('access comments'))) {
   +              $separate_form_location = $comment_form_location === CommentItemInterface::FORM_SEPARATE_PAGE;
   +              $existing_comments = $field->comment_count > 0 && $field->access('view only', $this->currentUser);
   +              if ($separate_form_location || $existing_comments) {
   
   @@ -203,7 +206,7 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
   -              ->getNewCommentPageNumber($entity->{$field_name}->comment_count, $new_comments, $entity, $field_name);
   +              ->getNewCommentPageNumber($field->comment_count, $new_comments, $entity, $field_name);
   

   More scope creep

4. +++ b/core/modules/comment/src/CommentManager.php
   @@ -151,19 +151,25 @@ public function addBodyField($comment_type_id) {
   -      if ($entity->get($field_name)->getFieldDefinition()->getSetting('form_location') == CommentItemInterface::FORM_SEPARATE_PAGE) {
   +      if ($field->getFieldDefinition()->getSetting('form_location') == CommentItemInterface::FORM_SEPARATE_PAGE) {
   

   Again

5. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,29 +295,47 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   +    $access = AccessResult::allowedIf($status === CommentItemInterface::OPEN)
          // And if user has access to the host entity.
   -      ->andIf(AccessResult::allowedIf($entity->access('view')));
   +      ->andIf(AccessResult::allowedIf($entity->access('view')))
   +      // Both checks cacheability are depending on the host entity.
   +      ->addCacheableDependency($entity);
   +    if (!$access->isAllowed()) {
   +      // Exit if commenting is not open or the user is not allowed to access the
   +      // host entity for view.
   +      return $access;
   +    }
    
        // $pid indicates that this is a reply to a comment.
        if ($pid) {
   -      // Check if the user has the proper permissions.
   -      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'access comments'));
   -
          // Load the parent comment.
          $comment = $this->entityTypeManager()->getStorage('comment')->load($pid);
   -      // Check if the parent comment is published and belongs to the entity.
   -      $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
   -      if ($comment) {
   -        $access->addCacheableDependency($comment);
   +      if (!$comment) {
   +        // Cannot reply to a non-existing comment.
   +        return AccessResult::forbidden('Cannot reply to a non-existing comment');
   ...
   +
   +      $access = AccessResult::allowedIf(
   +        // The parent comment is published.
   +        $comment->isPublished()
   +        // And the parent comment host belongs to the entity.
   +        && $comment->getCommentedEntityId() == $entity->id()
   +        // And the user is allowed to view the parent comment.
   +        && $comment->access('view', $account)
   +      )->addCacheableDependency($comment);
        }
   

   This refactoring may be right, but isn't it scope creep?

+++ b/core/modules/comment/src/Controller/CommentController.php
@@ -294,29 +295,47 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
+    // Allow if commenting is open on the entity.
+    $access = AccessResult::allowedIf($status === CommentItemInterface::OPEN)
       // And if user has access to the host entity.
...
+      ->andIf(AccessResult::allowedIf($entity->access('view')))
+      // Both checks cacheability are depending on the host entity.
+      ->addCacheableDependency($entity);

This strict check made the tests to fail, because $status is a string (e.g. "2") while CommentItemInterface::OPEN is an integer (i.e. 2). Also the code has unneeded complexity. Like in the other place, we need a boolean evaluation of $status == CommentItemInterface::OPEN && $entity->access('view', $account) and passing it to AccessResult::allowedIf().

@jonathanshaw,

The patch in #65 is green, so if #71.4 and #71.6 are valid is there a hole in the test coverage?

Not sure I get your question. My changes in the ::replyFormAccess() method are only optimisations. For instance, if $field->access('create', ...) returns forbidden, there's no reason to check other aspects, few lines below. We can just exit. Same for the next checks.

As now we rely on access check handlers, rather than permissions, I think that CommentAccessControlHandler::checkCreateAccess() needs additional context, more specific, the commented (host) entity. I'm facing this in the project we're working, where we extend CommentAccessControlHandler::checkCreateAccess() and we add custom logic that should rely on the commented entity. I've enriched the $context parameter with a new commented_entity key.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -520,6 +521,22 @@ public function getOwner() {
+  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    if ($operation === 'create') {
+      // The 'create access' handler should be aware of the commented entity
+      // because that might contain valuable information when making the
+      // decision to allow comment creation.
+      $context = ['commented_entity' => $this->getCommentedEntity()];

Makes sense. I'm not very familiar with access handlers, but might other operations (e.g. 'view') also like to have this context? Would it make sense to add it whenever getCommentedEntity() is not null, instead of special-casing 'create'?

My changes in the ::replyFormAccess() method are only optimisations.

OK, thanks for explaining that.

@jonathanshaw

#75.1: We already touch that line as we switch from permission to access check. It's not out-of-scope to improve the whole line in such cases. There are 2 changes there: (1) the order of the checks was swapped and (2) one of checks was made strict. The 2nd, indeed is wrong because $status might be a string in some circumstances while the constant is always an integer. For (1) see #71.9.

#75.2, 3, 4: I'm not the author of these changes but I think I would have done it in the same way. So, we need the $field variable because now we check $field->access() in more than one place, so it makes sense to capture it into a variable. The other changes, such as $entity->get($field_name)->comment_count to $field->comment_count are just coming from this change. But no strong opinion.

#75.5: This started from the @larowlan's question, in #70.4. In fact, almost all the method implementation is affected by the changes in the scope of this issue, see the patch from #65. What I did, I tried to fix #70.4 but than I realised a lot of issues with the proposed implementation and with the current code as well. As we are making an important change in this issue and we touch that method, I think it make sense to optimise it properly.

#77:

(...) might other operations (e.g. 'view') also like to have this context? Would it make sense to add it whenever getCommentedEntity() is not null, instead of special-casing 'create'?

Indeed, create is a special case. That's why EntityAccessControlHandlerInterface distinguish between normal "access", when the entity to be checked already exists, and "create access", when we do a check but the entity doesn't exist yet. That's why EntityAccessControlHandlerInterface::createAccess() was split out of EntityAccessControlHandlerInterface::access(). In the case of other operations, the handler method (::access()) receives the whole comment entity as parameter, so is able to access the commented entity $comment->getCommentedEntity().

Fixed also the coding standards complains.

@jonathanshaw the test failure from #76 proved that you were right in #75.1.

+++ b/core/modules/comment/src/Entity/Comment.php
@@ -520,6 +521,22 @@ public function getOwner() {
+  public function access($operation, AccountInterface $account = NULL, $return_as_object = FALSE) {
+    if ($operation === 'create') {
+      // The 'create access' handler should be aware of the commented entity
+      // because that might contain valuable information when making the
+      // decision to allow comment creation.
+      $context = ['commented_entity' => $this->getCommentedEntity()];

Would parent comment also be relevant context sometimes?

I've reread the issue comments in preparation for RTBC ...

1. From the IS:
   

   If possible, the commenting status of the parent ID should be taken into account in the access control handler checks. As this status flag affects the entity access for certain operations, its checks should also be centralized in the access control handler

   AFAIK we are not doing this and this should be removed from the IS?

2. #31 - #37 have a discussion about 'view', 'view only' and 'view and comment' that I don't understand but which might require a follow-up. Can anyone comment?
3. I'm not aware of any other follow-ups that are needed, nor that a change record is needed.
4. The IS says we are replacing direct usage of 'access comments', 'post comments', and 'edit own comments'. It's been 2 years since the patch was first created, we should do a final check to ensure no additional uses have crept into the codebase that are not yet covered in this patch. AFAIK the one exception is comment.module::comment_node_update_index which according to #18 B3 we are explicitly not fixing here.

So, it's green. However, it still Needs work because...

• #70.2 is still not addressed.
• #81: Add parent in context: This is a valid requirement but I don't see any solution. Let's try to find a way.
• Now I'm less confident that adding the Comment::access() is really fixing the issue. We need to test that. Tagging with Needs tests.
• As per #81, it needs CR and the IS should be revisited.

Now I'm less confident that adding the Comment::access() is really fixing the issue. We need to test that.

Do you plan to work on this? If not, a comment explaining would be really useful as its meaning is not clear to me.

@jonathanshaw, yes, I plan to work on this in the next days.

This issue blocks #2786587: [PP-1] Add thread depth configuration to comments.

Is looking good! Some small things that popped up during review:

1. diff --git a/core/modules/comment/src/CommentFieldItemList.php b/core/modules/comment/src/CommentFieldItemList.php
   index 73215e8698..f207826270 100644
   --- a/core/modules/comment/src/CommentFieldItemList.php
   +++ b/core/modules/comment/src/CommentFieldItemList.php
   +  protected function lastCommentAccess(AccountInterface $account, bool $return_as_object) {
   +    // Load last comment in thread.
   +    $last_comment_id = $this->first()->getValue();
   

   There is a potential fatal error here, since $this->first() can return NULL.

2. --- a/core/modules/comment/src/Controller/CommentController.php
   +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -294,29 +294,47 @@ public function 
   +    if (!$access->isAllowed()) {
   +      // Exit if commenting is not open or the user is not allowed to access the
   +      // host entity for view.
   +      return $access;
   

   This documentation can be improved, how about:

   +      // Exit if comments are closed or the user cannot view the host entity.
   

Working on it

Fixing the context when creating comments, either as first-tier comments or as replies. This still needs testing.

Forgot the patch.

Well... The problem now is that different create access results should be statically cached separately in EntityAccessControlHandler::$accessCache. But it's easy to see, looking at EntityAccessControlHandler::createAccess(), that every time the create access result will get the same cid. There is already #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context dealing with this issue and I've posted a patch there. Unfortunately, this is postponed on #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context.

Unassigning.

This patch adds tests for #89.

Just to advance here, for testing purposed, I've created a patch on top of #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context. Partially moving to Needs review to see the bot.

Removing out-of-scope changes in CommentTestBase

On #70.2:

@larowlan

in theory there could be N comments for a particular entity, should we be checking that at least one of them returns access allowed? e.g. in theory the last comment could be unpublished, which would mean the whole thread was not visible. If that's the case, we're missing some test coverage here

Well, not quite. The new method is using the comment field to retrieve the latest comment. But the field only records the latest published comment, see CommentStatistics::update(). That said, the implementation is conceptually correct. In this patch I've only improved a little bit the method, fix method and var naming and, I think, the function should return neutral if there are no published comments. To prove this, I've added a regression test.

Remaning tasks:

• Review if all changes have test coverage and add missing tests.
• Update IS
• Create CR
• Find someone to review #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context.
• Fix #85.

Changes:

• Added a comprehensive Kernel test that checks every possible scenario of CommentController. I wonder if we cannot drop \Drupal\Tests\comment\Functional\CommentAccessTest.
• Re. #85.1: @pfrenssen that is highly unlikely, see \Drupal\comment\CommentFieldItemList::get(), and if occurs then is something wrong in other place.
• Re. #85.2: Good suggestion. Fixed.
• Consolidated the output of CommentAccessControlHandler::buildCreateAccessCid(), so that every time we get a consistent $cid.
• Returned to strict check, where possible. It's healthier.
• ::lastPublishedCommentAccess() should, indeed, return "allowed" when there's no comment. In some circumstances we still want to show 0 comments.
• Fixed failures from #94.

Still to be done:

• Investigate more in deep the @larowlan's suggestion from #70.5. I think there were some proposals in the old comments.
• (note to myself) Need to understand in what circumstances CommentFieldItemList::access() receives editas $operation.
• Update IS
• Create CR
• Find someone to review #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context.

In #95, I've reverted the return of ::lastPublishedCommentAccess() but I forgot to adapt the test.

Great to see this coming along. #28-38 has some discussion around 'administer comments'.

@jonathanshaw, thank you for pointing me to those comments. Reading them again and reading the code, I see some value in refactoring that too. At least, I can imagine that such a change could allow to implement custom access logic on the comment field when editing a node. A custom access handler could decide, for instance, that a user can close commenting on article nodes, while they cannot on page nodes. And so on...

However, looking to code occurrences, I think this will make this patch un-reviewable, it's already approaching 50KB. For this practical reason, I've considered @a_grizzli's suggestion from #35 and deferred this task to #3180177: [PP-2] Replaces usages of 'administer comments' permission w/ access handler checks follow-up.

Fixes:

• Added #3180177: [PP-2] Replaces usages of 'administer comments' permission w/ access handler checks followup.
• Added CR: https://www.drupal.org/node/3180183.
• I've been checked all occurrences of permissions mentioned in #81.4 and found no new instances. The exception is comment_node_update_index(), which, according to #18.3.B3 will not be fixed here.
• #81.2: It was requested by @larowlan to add some more code inline documentation, in order to make clear what are 'view' & 'view only' about. This was already fixed in #34.

Remaining tasks:

• Review, approve, merge #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context.
• Final review of this issue.

This is looking great, the only things I could find here were nits

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,86 @@ public function offsetExists($offset) {
   +      // considered as composition of listed comments and comment form. Decision
   

   nit: this should be 'a composition', not 'as'. 'Decision about' should be 'The decision'

2. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,86 @@ public function offsetExists($offset) {
   +      // by various methods to check only single permission on last comment.
   

   'a single permission on the last comment'

3. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,86 @@ public function offsetExists($offset) {
   +      // by various methods to check only single 'create' permission on comment
   

   'the single' on 'the comment entity'

4. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,86 @@ public function offsetExists($offset) {
   +      $access_control_handler = \Drupal::entityTypeManager()->getAccessControlHandler('comment');
   ...
   +          'parent_comment' => \Drupal::entityTypeManager()->getStorage('comment')->load($parent_comment_id),
   

   let's store this in a variable to avoid two container calls

5. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,86 @@ public function offsetExists($offset) {
   +    // Load the last published comment in thread.
   

   the thread?

6. +++ b/core/modules/comment/tests/src/Kernel/CommentControllerTest.php
   @@ -0,0 +1,175 @@
   +    $this->{$assertMethod}($this->controller->replyFormAccess($node, 'comment')->isAllowed());
   

   could we not just use $this->assertEquals($expectation, $this->controller...) instead

7. +++ b/core/modules/comment/tests/src/Kernel/CommentFieldAccessTest.php
   @@ -314,4 +315,48 @@ public function testAccessToAdministrativeFields() {
   +    // Check that the 'view only' access to the field not is permitted.
   

   not is > is not

Thanks so much for moving this along.

For context, most of this existing logic around permissions was the same in D7, so it never really got to use the handler concept like other entity-types.

Glad to see this coming together

Assigning to clear nits.

#99.6: I agree. I double checked that using assertTrue or assertFalse doesn't provide better messaging in the test results and it does not. I reckon assertEquals($expectation, ...) is more readable.

I came with a trivial idea in #2886800-9: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context to fix the problem of entity access control handler internal cache. But that issue is receiving now some inputs that probably will end in a more complex solution. As the solution that I've proposed in #2886800 - #21 is a so simple and with a minimal footprint I'll move it here in order to unblock this issue. Then, #2886800, could be relieved from the pressure of being a blocker here and could be improved accordingly. Inserting here that code snippet doesn't affect in any way the outcome of the other issue.

I'm OK with integrating the simple fix from #2886800: EntityAccessControlHandler::createAccess() returns false positive cache hits because it ignores context in here as proposed in #2879087-102: Use comment access handler instead of hardcoding permissions . It unblocks this issue in a non-disruptive way, and this is introducing the same protected method as is being worked on right now in the other issue. It will be trivial to adapt either this issue or the other, no matter which will be completed first.

1. +  protected function lastPublishedCommentAccess(AccountInterface $account, bool $return_as_object) {
   +    // ...
   +    // If there are no comments, then access does not matter.
   +    return $return_as_object ? AccessResult::allowed() : TRUE;
   +  }

   Question: wouldn't AccessResult::neutral() be more correct if access doesn't matter?
   edit: I checked this and it would forbid access.

2. --- a/core/modules/comment/src/Controller/CommentController.php
   +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -292,31 +292,51 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   -    // If commenting is open on the entity.
   -    $status = $entity->{$field_name}->status;
   -    $access = $access->andIf(AccessResult::allowedIf($status == CommentItemInterface::OPEN)
   -      ->addCacheableDependency($entity))
   -      // And if user has access to the host entity.
   -      ->andIf(AccessResult::allowedIf($entity->access('view')));
   +      // Commenting is open on this entity.
   +      $status === CommentItemInterface::OPEN
   +      // And user is allowed to access to the host entity.

   The original documentation was better IMO, how about "And the user has access to the host entity."?

3. +  /**
   +  public function testFieldViewOnlyOperationAccess(): void {
   ...
   +    // Delete the published comment. From now on the field contains only
   +    // unpublished comments.
   +    $enabled->delete();
   +
   +    // Reload the entity.
   +    $this->host = EntityTest::load($this->host->id());
   +    /** @var \Drupal\comment\CommentFieldItemList $comment_field */
   +    $comment_field = $this->host->get('comment');
   +    // Check that the 'view only' access to the field is not permitted.
   +    $this->assertTrue($comment_field->access('view only', $account));
   +  }

   This test indicates that the functionality doesn't work as expected. It is checking that "view only" access is NOT permitted, but uses assertTrue() which seems to prove that the access IS permitted.

Fixed #103.2.

Ref. #103.1 & 3, I've reflected more on the fallback return value of CommentFieldItemList::lastPublishedCommentAccess(). Actually, @pfrenssen is right in #103.1, there's no doubt that, when there are no comments, we should return neutral, which reads as make no opinion. That said, I've fixed comment_node_search_result() accordingly, because, even with no published comments, when a field has OPEN status, it should print the comment count.

Moved the $open flag block code but not in the right place.

Thanks for addressing the remarks! I did not connect the dots in my previous comment that the access check being inverted in the test was related to my other remark about the neutral access result :)

This is looking good to me now.

Random test failure, moving back to RTBC.

1. +++ b/core/modules/comment/src/CommentFieldItemList.php
   @@ -43,24 +43,88 @@ public function offsetExists($offset) {
       * {@inheritdoc}
       */
      public function access($operation = 'view', AccountInterface $account = NULL, $return_as_object = FALSE) {
   

   Given we're using non-standard operations I think we need to document them somewhere and what they all mean. In a list. Here's as good a place as any. Or maybe in a new documentation section in comment.api.php - yeah there because it is easier for a developer to find.

2. +++ b/core/modules/comment/src/CommentLinkBuilder.php
   @@ -101,10 +102,10 @@ public function buildCommentedEntityLinks(FieldableEntityInterface $entity, arra
              // Teaser view: display the number of comments that have been posted,
   ...
   +            if ($field->comment_count > 0) {
   

   The call to $field->access() can only return TRUE if there are comments - AFAICS. So this can be removed. Or we can add this to the previous line and make it first in the if - perhaps it is more performant.

3. +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -292,31 +292,51 @@ public function replyFormAccess(EntityInterface $entity, $field_name, $pid = NUL
   -    // If commenting is open on the entity.
   -    $status = $entity->{$field_name}->status;
   -    $access = $access->andIf(AccessResult::allowedIf($status == CommentItemInterface::OPEN)
   -      ->addCacheableDependency($entity))
   -      // And if user has access to the host entity.
   -      ->andIf(AccessResult::allowedIf($entity->access('view')));
   +    // Check if the user is able to create comments.
   +    $access = $field->access($create_operation, NULL, TRUE);
   +    if (!$access->isAllowed()) {
   +      // Exit if the user is not allowed to create new comments.
   +      return $access;
   +    }
    
   -    // $pid indicates that this is a reply to a comment.
   +    $status = (int) $field->status;
   +    $access = AccessResult::allowedIf(
   +      // Commenting is open on this entity.
   +      $status === CommentItemInterface::OPEN
   +      // And the user has access to the host entity.
   +      && $entity->access('view')
   +    )->addCacheableDependency($entity);
   +    if (!$access->isAllowed()) {
   +      // Exit if comments are closed or the user cannot view the host entity.
   +      return $access;
   +    }
   +
   +    // Comment replies require some additional checks.
        if ($pid) {
   -      // Check if the user has the proper permissions.
   -      $access = $access->andIf(AccessResult::allowedIfHasPermission($account, 'access comments'));
   -
   -      // Load the parent comment.
   -      $comment = $this->entityTypeManager()->getStorage('comment')->load($pid);
   -      // Check if the parent comment is published and belongs to the entity.
   -      $access = $access->andIf(AccessResult::allowedIf($comment && $comment->isPublished() && $comment->getCommentedEntityId() == $entity->id()));
   -      if ($comment) {
   -        $access->addCacheableDependency($comment);
   -      }
   +      return AccessResult::allowedIf(
   +        // The parent comment is published.
   +        $parent_comment->isPublished()
   +        // And the parent comment host belongs to the entity.
   +        && $parent_comment->getCommentedEntityId() === $entity->id()
   +        // And the user is allowed to view the parent comment.
   +        && $parent_comment->access('view')
   +      )->addCacheableDependency($parent_comment);
        }
   +
        return $access;
   

   I'm not convinced that the early returns and not merging in the results of the previous access checks here correctly maintains meta-data on the returned access result. For example if the PID is checked the access result does not have $entity added as a cacheable dependency. That does not feel right.

4. create is an interesting choice of operation to mean "post comments". I'm undecided if it is correct. Pairing it with field access is interesting. On one hand it is ties up nicely with the "create comment" permission and "create" operation for the comment entity. On the other it's not quite the same...
5. view vs view only is unfortunate.

@alexpott, fixed #111.1, 2, 3.

About operation naming: There are two operations, view and edit that are coming from Drupal core and we cannot change. I can see the problem with view, but view is checked in EntityViewDisplay::buildMultiple() and I don't see how we can avoid that. On comments this is special as comment field on view display has also a form. I've documented all these aspects in comment.api.php. Regarding view only (#111.5), again, I agree. But how the name the view part of the "view"?

Regarding the create op (#111.4), I have no idea. Open to any suggestion.

Regarding view only (#111.5), again, I agree. But how the name the view part of the "view"?

Having read the docs you've just written on this, it seems that basically view == view_only && create.

If that's right, I think "view" and "view_comments" would be clearer names for the comments field operations. It sort of makes sense that 'view' would mean view the field including the form, and 'view_comments' would be a narrower aspect of the generic 'view'.

Alternatively 'read' or 'read_comments' would make some sense instead of 'view_only', as you can view a form but you can't really read a form.

I've fixed all the remarks from #111.

@jonathanshaw, @alexpott, @larowlan and me, we had a chat on Slack and decided to rename the view only operation to view comments only.

This is now ready for RTBC.

Couple of nits and proper question on the MR.

Setting back to RTBC as I only fixed the docs typos and a straight rename.

Slightly concerned about the "reply to $pid" access operation - is that absolutely necessary to do here or could it be moved to a follow-up?

I've applied only a small docs fix.

@claudiu.cristea do you have a thought about @catch's actual question:
"is that absolutely necessary to do here or could it be moved to a follow-up?"

'reply to' makes sense to me with comment entity access. i.e. <? $parent_comment->access('reply', $account) ?>

So could the comment controller do an && of field access 'create' and parent comment 'reply', or something along those lines.

So could the comment controller do an && of field access 'create' and parent comment 'reply', or something along those lines.

If i'm getting right what you’re saying, you suggest to replace in controller:

    // Check if the user is able to create comments.
    $access = $field->access($create_operation, NULL, TRUE);
    if ($access->isForbidden()) {
      return $access;
    }

with

    // Check if the user is able to create comments.
    $access = $field->access('create', NULL, TRUE) && $parent_comment->access('WHAT OP?');
    if ($access->isForbidden()) {
      return $access;
    }

There are 2 issues with this: There's no 'reply' operation on entity level. We can add it but how can we decide if a comment is "reply-able"? The second, and most important, is that, in field access method, we ask the comment access handler to decide if the new comment can be created. But, with this change, we'll not pass the parent ID as context to the handler. It's not enough to check, upstream, the reply access against parent. The parent may contain other valuable data (fields) that are critical to make the decision if a reply can be created.

EDIT: So, all the reply to {$pid} stuff is to provide valuable context to CommentAccessControlHandler::createAccess() and subsequently to hook_comment_create_access().

Another possible way to do it would be that when there's a parent comment, delegate everything to $parent_comment->('reply') - and in that method, call the field access 'create' (which in core would be the only thing we check). That would mean that customising the behaviour would happen at the entity access level.

IMO it's OK to add an extra entity access operation.

I do think it would be easier to split this bit out to a follow-up, but if we need to address it here would prefer to be absolutely sure there's not an alternative.

...delegate everything to $parent_comment->('reply') - and in that method, call the field access 'create' (which in core would be the only thing we check). That would mean that customising the behaviour would happen at the entity access level.

But inside $parent_comment->access('reply') we don't have the field context. How to call the field access?

EDIT: We can probably get the field item list instance from the parent comment entity, via ::getCommentedEntity() and ::getFieldName()

Yeah that information is all available. Moving to NW let's see what that looks like.

@catch, @jonathanshaw, Re. #123: I think, apart of adding reply as entity access new operation, this one should defer the logic to CommentAccessControlHandler::checkAccess(), which now will have to also address a new reply operation. The reason is that I want to have some consistency and honour the entity access check architecture, which defers to EntityAccessControlHandlerInterface handler the logic. This will allow also hook_comment_access() to receive reply as operation.

I haven't read all this thread, thus maybe my comment is not very useful but here I go:
I end up here because I could not understand why NodeAccessControlHandler was never called when I am trying to display or add a comment.
More specifically, If I have a comment field attached to a Node type, I would expect that the checkFieldAccess() method of NodeAccessControlHandler will be called (ex: when a comment is displayed) like for any other field attached to the node.
Why such an exception?

I have rebased this for 9.4.x.

In order to preserve the work done previously I have:

1. Made a copy of the original branch (which was against 9.2.x) available as 2879087-comment-access-handler-9.2.x.
2. Created a new branch 2879087-comment-access-handler-9.4.x to perform the rebase
3. Force-pushed the original branch to be identical to the 9.4.x branch

So the current "working branch" remains the same as before: 2879087-comment-access-handler. This is also the only one which has an associated merge request.

I'm uploading patches for older D9 versions because the dynamically generated patches will now only apply on 9.4.x.

Is the patch still needed with Drupal 10?
As it is mentioned as 'required' by the https://www.drupal.org/project/group_comment module, it would be great to have some update.
Thanks for any feedback.

Attached a patch for 10.1. Patch is based on 01ed69c2. Slightly adjusted for D10.

It seems I introduced a type error in the last patch when the account is not set. Updated patch. (Only addition
$account = $account ?: \Drupal::currentUser(); in CommentFieldItemList.php).

This patch is identical to #136, only I removed one change introduced by this commit. I'm not sure if that permission check is needed (as view access on the parent comment is checked), and feels like it goes against the idea of the patch itself (to use access checks instead of permission checks). We use this patch together with Group Comment module, and that check would only allow replying to comments if we'd use the 'View comments' global permission, and we'd like to avoid that in favor of group permissions.

this is 137 rerolled for 10.3.x

My patch in #139 was missing a use statement, this should work for 10.3.x

I have put the patch in #141 into the merge request. The person who opened the merge request, @claudiu.cristea, will need to update the merge request so that it targets branch 11.x.

Done

But why did you squash? We lost the commit history :(

@claudiu.cristea if you want to rebuild you can try looking at the fork, and looking at the tags with `previous-` prefix.

• https://git.drupalcode.org/issue/drupal-2879087/-/network/9.2.x?ref_type...
• https://git.drupalcode.org/issue/drupal-2879087/-/tags

Update: I recreated the history from previous/2879087-comment-access-handler/2022-01-31 branch
https://git.drupalcode.org/issue/drupal-2879087/-/tags/previous%2F287908...
Merged 11.x, fixed conflicts, phpcs, phpstan and the tests touched by the patch.

5 tests are failing at the moment, among those these are concerning to me and relate to the changes in CommentController::replyFormAccess:

- core/modules/comment/tests/src/Functional/CommentNonNodeTest.php
- core/modules/comment/tests/src/Functional/CommentAnonymousTest.php

I believe this commit is wrong, we want to deny access if the parent access is not explicitely returning AccessResult::allowed. Such as when the user doesn't have the "access comments" permission, we get AccessResult::neutral and we should deny access to reply.
Reverting it fixed the above tests, I also added back null check protection after loading the parent comment which got lost.

But it caused CommentLinksTest.php:185 to fail.
This revealed an issue in that class/test as it doesn't actually create the comment type. Because of this, calling $comment->getCommentedEntity at CommentAccessControlHandler.php:35 was retrieving a user entity and denying access to view the reply link! fun one :)
This relates to #3165822: [PP-1] Creating comment field w/o a comment type is allowed

----

There are 3 remaining tests failing, 2 about cache tags and 1 performance? (quite cryptic):

core/modules/page_cache/tests/src/Functional/PageCacheTagsIntegrationTest.php
core/modules/comment/tests/src/Kernel/CommentDefaultFormatterCacheTagsTest.php
core/profiles/standard/tests/src/FunctionalJavascript/StandardPerformanceTest.php

Also, #137: The branch was force pushed before so not sure what happened with that commit... but I don't see it in the current PR.

For now here are patches of the current MR23 commit f2ac23e6.

Important fix for cacheability:
The CommentDefaultFormatterCacheTagsTest was failing because the CommentDefaultFormatter was not applying cacheability to the render array.

---

On that note, we have a need in our project where the create comment permission will have a very high cache variance (per user).
This means that currently, it would kill the Dynamic Page Cache when CommentDefaultFormatter checks for the create permission and adds the lazy_builder and cacheability to the render array.
I propose to move the access check inside the lazy_builder callback, in CommentLazyBuilders::renderForm.

I'd like to keep the scope to a minimum but this touches on the same lines and lots of projects may be in a similar scenario so we should try to preserve the page cache as much as possible. Viewing comment is less likely to have such requirements.
The change is very small, but this could also be reverted and a separate issue or follow up instead if anyone objects.

I pushed an attempt now to address #118 to #125 (reply to {$pid}).
By calling $parent_comment->access('reply') in CommentController::replyFormAccess and doing the actual check in CommentAccessControlHandler::checkAccess

Also, I had a look at the other issue #2786587: [PP-1] Add thread depth configuration to comments which requires this issue. It seems the approach there was to rely only on the create operation in CommentAccessControlHandler::checkCreateAccess to limit the thread depth. Since we now have the reply access operation, I'm not even sure we need all the changes to $context (parent_comment, commented_entity).
I'll sync with @claudiu.cristea on Monday.

---

PS: still the same 2 (less concerning) test failures which I left out for now since I don't quite understand them yet.

Issue summary needs to be flushed out.

1. This is adding a new trait but not sure I see that mentioned.
2. All the changes outside the comment module, to me seem out of scope.
3. Adding a test EntityCreateAccessCustomStaticCidTest outside of comment not sure I see the why.

Hiding all branches.

Hide patches

Updating IS

Re #161:

1. Updated the IS. Mentioned the trait.
2. There are only 2 changes outside the comment module. Both are test expectations changes because of the comment module changes.
3. Removed EntityCreateAccessCustomStaticCidTest because in the meantime, core added a similar test.

Ready for review

I’ve been following this issue and testing the patches for a while. Overall, things are working well, but I did notice a couple of cases where the behavior isn’t what I would expect:

1. When a comment is denied access by returning AccessResult::forbidden() in a hook_comment_access() implementation, it is still visible in the thread. CommentDefaultFormatter::viewElements() doesn’t currently filter out inaccessible comments. It just calls viewMultiple(), which doesn’t perform access checks.

I think viewElements needs to do something along the lines of

$accessible_comments = [];
foreach ($comments as $comment) {
  if ($comment->access('view', $this->currentUser)) {
    $accessible_comments[] = $comment;
  }
}
// Only render if we have accessible comments
if ($accessible_comments) {
  $build = $this->viewBuilder->viewMultiple($accessible_comments, $this->getSetting('view_mode'));
}

2. The comment field is hidden when the last comment is inaccessible.
CommentFieldItemList::lastPublishedCommentAccess() is used by CommentFieldItemList::access() to determine whether the entire field/thread is accessible. If the last comment in a thread is denied access, the entire field, including the comment form, is hidden. In this case, CommentDefaultFormatter::viewElements() is never called.

Im not sure what the rationale for the lastPublishedCommentAccess() method is here, couldn't we just check $account->hasPermission('access comments') here?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

OK, I've addressed the remarks from @larowlan (after #168 comment)

I tried to create a (11.3.x) patch from the latest contents of the MR but I couldn't figure out what the actual commit / branch of core those changes are actually written against. So all these changes are missing .. sorry.

My patch only includes the patch from #149 re-rolled against 11.3.x.

#167.1 I think should be a seperate issue, it's a pre-existing matter.
#167.2 touches upon something discussed in earlier MR comments, it's clear that the implemention here has the potential to be surprising and probably there's room to better document or cross-reference the explanations around this. However, I don't think this concern is strong enough to justify blocking the issue - docs can always be improved.

This has had a lot of eyes over a long period of time, so I suggest RTBC.

Given how esoteric the access considerations are, it's hard to be certain of BC here. This may be one of those potentially disruptive issues where it's good to commit to a branch a long time before a release.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Since #171 there are only straight re-rolls.