Contrib.social

Use comment access handler instead of hardcoding permissions

Open on Drupal.org →
Created on 17 May 2017, about 7 years ago
Updated 9 June 2024, 15 days ago

Problem/Motivation

The comment module comes with a CommentAccessControlHandler which is not really used on crucial places. Instead of using this handler, there are dozens of places, where the permissions provided by the comment module are checked directly (e.g. post comment permission checks instead of $entity->access('create') or access comments permission checks instead of $entity->access('view')).

For example, the $entity->access('create') access check is only used during comment link generation, but not for the comment form itself.

Due to this, it is currently impossible to create a custom access control handler for comment entities, as it would not be used where necessary. When reading through the code, you can also see there is a lot of access-related duplicate code (because the permission checks duplicate the access checks of the access control handler).

These changes will affect other issues as well, when those deal with comment access/permissions, but they are very important to get the comment entity access handling correct.

Proposed resolution

  • Scan through the code and eliminate all permission checks with proper $entity->access() checks. Search for the following permissions:
    • access comments
    • post comments
    • edit own comments

    The <em>administer comments</em> permission has been split in #3180177: [PP-2] Replaces usages of 'administer comments' permission w/ access handler checks → in order to limit the scope of this issue.

  • 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
  • On "create access" the commented entity and, on comment replying, the parent comment should be passed as context as they might contain valuable information for a custom comment entity access control handler when making the decision.
  • Comment-related tests should still work as before, because only the access checking behavior is changed, but not the access requirements themselves

Remaining tasks

None.

User interface changes

None.

API changes

The current comments access checks behaviour can be altered by swapping the comment access control handler and implementing a custom access logic.

Data model changes

None.

📌 Task
Status

Needs work

Version

11.0 🔥

Component
Comment  →

Last updated 7 days ago

Created by

🇩🇪Germany hctom

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment 8 months ago →
    🇰🇬Kyrgyzstan sahaj

    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.

  • Comment 7 months ago →
    🇳🇱Netherlands jaapjan

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

  • Comment 7 months ago →
    🇳🇱Netherlands jaapjan

    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).

  • Comment 7 months ago →
    🇧🇪Belgium keszthelyi Brussels

    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.

  • Comment 15 days ago →
    🇬🇧United Kingdom jaydenpearly

    Tested for version 10.3
    #136 and #137 didn't apply...

contrib.social Blog FAQ Discussions
Production build 0.69.0 2024