Problem/Motivation
The work being done in #731724: Convert comment settings into a field to make them work with CMI and non-node entities will result in that comments can be used on any entity just by creating a comment field in the same way as adding a text field. It will also be possible to tailor each comment form used with other fields, making them perfectly adapted for each entity type used on.
But, there is one very big bottleneck that will severely block the usefulness of this new flexibility. That is that permissions are still globally set for all comment forms throughout the same site. This means that it isn't possible to:
- Allow anonymous commenting on blog posts
- Only allow comments for logged in users on articles
- That comments to product must be approved before being visible
To take it further. A site might even need to have different commenting permissions for two different blog content types on the same site.
This permission control isn't just limited to the entity type itself. It should also be possible to override on individual entity items too. For example:
- A blog post offering a giveaway for the right answer could then set the comments to that post to be approved before publishing.
- The writer want to disable comments to a post about a sensitive topic.
This kind of detailed configuration simply isn't possible with global permissions.
Implementing the proposed solution below will not only make the comment module a lot more flexible, it will also give site-owners much more control over their site and who can do what and where.
Proposed resolution
First:
- Remove the global comment viewing/posting permissions completely.
- Split the "Administer comments and comment settings " into two:
- Global comment administration
- Configure comment settings
Then move all the comment viewing/posting permissions to the field configuration. Permissions needed for:
- View
- Post
- Reply (new)
- Skip approval
- Approve comments (new)
- Edit own
- Edit all (new)
Anonymous, authenticated roles will be statically available. All other roles can be manually added to the field. If not added, the permissions for the authenticated is used unless the two new global comment permissions kicks in.
It should also be possible to set the default permissions in the global comment settings. Including adding more roles that then automatically are used for new comment fields.
There will also be a new setting for allowing overriding certain settings on individual items. Such as described in the examples above.
When enabled options are needed for:
- What options can be overridden
- Turn on/off skip approval
- Disable/Enable commenting
- Close/open commenting
- Who can do it
- Item author (overrides the need of the "Approve comments" perm)
- Roles with the "Approve comments" perm
When for example disabling commenting or turning on approval, a custom, overrideable, message should be displayed instead. For example "Comments to this post has been disabled due to ....".
Here too, the global comment admin permission overrides all custom settings.
Remaining tasks
- Create mockups with proposed UI changes
- Implement the changes
User interface changes
TBD
API changes
TBD
Related issues
#10426: Make comment permission more granular. Assigned to: killes@www.drop.org
Comment | File | Size | Author |
---|---|---|---|
#98 | move_global_comment_permissions_to_comment_type_level-1903138-98.patch | 28.51 KB | hannakras |
#97 | move_global_comment_permissions_to_comment_type_level-15216974-97.patch | 28.66 KB | ovanes |
#95 | interdiff_94-95.txt | 5.46 KB | vsujeetkumar |
#95 | 1903138-95.patch | 27.98 KB | vsujeetkumar |
#94 | 1903138-94.patch | 28.42 KB | vsujeetkumar |
Comments
Comment #1
plachPeople working on this may be interested in http://drupal.org/node/1903124 and http://drupal.org/node/1903128.
An (apparently) similar discussion to the one reported in the OP happened in #1807776: Support both simple and editorial workflows for translating entities.
Comment #2
andypost@plach This awesome change! Is there any suggestions/ideas about UI for that? Following #1807776: Support both simple and editorial workflows for translating entities I found that UI part is planned for contrib to not mess a permissions page.
I we make #731724-291: Convert comment settings into a field to make them work with CMI and non-node entities real so this permissions will be very useful. But I not sure exactly how to apply them to comment-field on per entity/bundle basis
Comment #3
tstoecklerSort of related #366416: Field Permissions UI
I think if that gets in this may be obsolete, but I don't consider that likely at this point.
Comment #4
webchickI don't see anything in the OP that you can do in core right now, so don't understand why this is a major task. It sounds like a feature request to me, and most likely one for D9.
Comment #5
andypostThis would be a task when #731724: Convert comment settings into a field to make them work with CMI and non-node entities lands
Comment #6
andypost@tsvenson once we have agreement on settings for comment form. We can continue here.
Anyway having this global settings currently enough, contrib have field permission and only posting an action that questionable.
I see no reason to introduce this permissions in global scope.
Global
View comments - granularity defined by field
Skip approval - same as now, detailed about anonymous in Form settings
Administer - skip entity access and allows to configure forms.
Approve comments (new) - great idea!
Entity Access
Edit own - entity level
Edit all (new) - entity level
Post comments - global + granularity defined by entity
Reply (new) - useless, It's just a link in formatter (when user allowed to post for the entity type)
Comment #7
andypostHere's a new way to extend access for fields introduced in #1883152: Field level access for EntityNG
Comment #8
fubhy CreditAttribution: fubhy commentedWhy do we need to introduce a new concept for this? If we don't fix generic field level access for D8 we should not build a crippled comment-specific field-level access solution here. Instead, just provide dynamic permissions for each comment field instance in use. If you want/need a different permission model for commenting on nodes vs. commenting on users just make the two two separate comment setting fields. Done.
Comment #9
tsvenson CreditAttribution: tsvenson commented@fubhy: Say what? As things are now both those comment settings fields with be governed by the global comment permissions as described in the OP.
Comment #10
fubhy CreditAttribution: fubhy commentedFrom the issue summary:
I disagree with that. We should not split up access permissions to fields and global unless done generically. Let's not do this specifially for comments. If we can solve field level access for D8 generally fine, let's do it, if we can't let's not introduce a crippled subset of that functionality specifically for comment module.
Permissions should be controlled via the global permission configuration page. If that is your intent, both the issue summary and title are not correct.
However, if you actually plan to move permission configuration into the field instance configuration (as the issue summary and title suggest) you are introducing a huge comment-module specific UX wtf.
Comment #11
tsvenson CreditAttribution: tsvenson commentedI would like to turn that around and say that - As long as we have global permissions for things, such as fields, we are severely crippling the usefulness and flexibility of those features. It forces sites to make a global permission compromise instead of being able to tailor it exactly to how they need it for each individual feature on the site.
This crippling is painfully exposed in this case.
Also see my Drupal Limitations and Bottlenecks and Don’t Make Drupal the New ‘Microsoft Office’! posts that goes into much more detail about this.
Comment #12
fubhy CreditAttribution: fubhy commentedI am not saying we should not allow per-comment-field-instance permissions. But we should NOT move them into the field settings. We should keep them on the global permission page. Otherwise people will have serious issues trying to find out where they can set permissions for what. Let's keep it in one spot. That's all.
Comment #13
tsvenson CreditAttribution: tsvenson commentedHow about making them available in both places?
From a UX perspective it would make a massive improvement, and avoid pogo-sticking, if I would be able to get a full overview of both the configuration and the permissions at field level too.
Then on the big permissions page the same are shown, grouped per field and also with a convenient link to the field settings page too.
Comment #14
andypostThere's global permissions anyway - for comment entity (for 80% of sites enough)
I think better to implement permissions per-comment form as "bundle".
If someone will need a per-field settings - it's ok with contrib module a-la field_permissions for comment-field.
Permissions on field-instance level seems absurd but could be implemented with custom module.
Another case is per-entity permissions or more granular for example the workflow module when commenting should be enabled in some states or roles.
Comment #15
andypostThe related discussion about access to field/field-item #2018707: Do we still need to support hook_field_prepare_translation() ?
Comment #16
andypostComment #17
larowlangrabbing this one, not sure if you're actively working on it, if so, please assign back and I'll hold-off.
Comment #18
larowlanhere's a wip, still lots of calls to user_access|hasPermission(access comments|post comments|skip comment approval|edit own comments) to update to the new method as follows:
before
after
But before that thoughts on approach.
Offline for a few weeks so feel free to keep kicking this along.
Comment #19.0
andypostrelated
Comment #20
larowlanI'm wondering if we might be better to refactor comment permission to make one set of permissions per comment bundle regardless.
For sites upgrading from D7 with stacks of different node-types (which map to bundles on upgrade) this would mean a lot of permissions, but no more than node spits out. And if they're not using said bundle, they can remove the field.
Thoughts?
Comment #21
larowlanPostponed on #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form
Comment #22
larowlan#2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form is in
Comment #23
larowlanWorking on this, but deferring most decisions to the CommentAccessController to allow it to be swapped out instead of hard-coded.
Comment #24
tsvenson CreditAttribution: tsvenson commentedAwesome news @larowlan. I am in the process of getting back to Drupal after a longer, much needed, break. Will take a little time to set everything up, but I plan to devote time to help testing D8 stuff.
Comment #26
andypostComment #28
AntonnaviHi, all!
I ported patch from comment #18 to use it with 8.2.x version.
Currently it is alpha version (seems to stable). I need help with:
On adding new comment field You can check checkbox "Create custom permissions for this comment field" and for this field appears personal permissions (access/post/skip approval/edit own) on permissions list page.
Comment #29
AntonnaviComment #30
andypostThe center of authority is CommentItem so code should work on top of field permission.
This perfectly fits into field access model and will allow control UI at widget|formatter level
Comment #14 still open - do we really wanna build access around comment types - read that like "read|answer to threaded comments" or "view only plain comments"?
About UI I'd suggest to add tab to field edit page to manage perwissions
This all over changes...
If we gonna go this way better to add this methods *check|build*FieldPerssions() to field itself
Comment #32
AntonnaviThank You for review, andypost!
Agree with You. Patch was re-done to use comment bundles for access restriction.
Comment #34
andypostOverall this looks great! @Anton Thanx a lot!
300 failed tests now shows we need proper defaults and upgrade path for existing sites.
@larowlan I'm sure that serious change and needs your opinion)
I'd better convert that to private function (prefixed "_")
otoh static cache needs to be refactored to some class
Comment #35
Antonnavicomment_node_update_index_check_access() function name changed to _comment_node_update_index_check_access(), patch was updated.
Comment #36
AntonnaviComment #38
larowlanThanks for moving this along, this is a good feature.
If we're going to extract this, let's move it to a service that can be swapped and unit tested
It feels like we could almost use a helper class here too, just to centralize all the logic, in case we want to tweak it or change it again?
Comment #39
AntonnaviThank You for review larowlan!
1. _comment_node_update_index_check_access() was moved to CommentManager service.
2. Do You mean add new method to CommentManager service that will obtain 2 params:
$permission
(like "access comments")$comment_type
(comment type id)Will concatenate it and return as result:
$permission . ' ' . $comment_type
.This method will be used instead of code like
'access comments ' . $comment_type
right?Comment #40
AntonnaviComment #42
andypostLooks good step forward!
It's really interesting where to put this code knowing that comment manager should get split.
Maybe better to introduce new service?!
#38.2 is exactly about permission so maybe better to add new method to
\Drupal\comment\CommentAccessControlHandler
this brings state to service
so you need to use
DependencySerializationTrait
Comment #43
jefuri CreditAttribution: jefuri as a volunteer commented@andypost
I do not agree. We have a use case where we only want non hierarchy comments. So a reply on an existing comment is not wanted in our use case. But some users are allowed to post. Adding the reply permission as apposed to using the post permission in generals would solve this.
Also I see a lot of direct validation through the hasPermission method in the current user in parts like:
- CommentLinkBuilder
or only use the entity access layer:
- CommentLazyBuilder
Why wouldn't we validate access on the URL object of the links we are trying to render? In theory you would ignore any route subscriber access check alterations on the routes of comments when showing a link. When clicking they would get an access denied.
Would we not prefer that the same access validation is done on the rendering of an link as when we go to the url of the link?
Comment #44
jefuri CreditAttribution: jefuri as a volunteer commentedBased on my previous comment I added the permission to differentiate between a post and a reply.
I also altered the lazybuilder and linkbuilder classes to use the url object to validate access where we could.
Also changed the reply form access callback to reflect this. Please note that I also see the post and reply as a separate access validation which are not dependent on each other. Meaning that one user with a role could post a message, but might not be allowed to reply and vice versa.
Comment #46
jefuri CreditAttribution: jefuri as a volunteer commentedSorry, uploaded a PHPStorm patch. Fixed it!
Comment #47
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedPatch from #46 has an bug or something. Recreated the patch.
Comment #49
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedPatch from #47 was missing the CommentPermissions.php.
Comment #50
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedComment #54
jonathanshawAgreed, flexibility in permissions here would be good. I can see a use case where I would want sort-of-admin users to be able to reply to comments, but other users could only reply to their own comments (or reply to replies on their own comments), but not reply to other people's comments. It would enable a public conversation between a visitor and a website.
Comment #55
hctomI just posted another issue which might be interesting for this issue: #2879087: Use comment access handler instead of hardcoding permissions . As you can see, the entity access checks should be done via the access control handler and not with direct permission checks. This would also have a massive impact on this issue.
Comment #58
jefuri CreditAttribution: jefuri as a volunteer and at Synetic commentedHere a reroll for 8.4 and 8.5
Comment #59
drummondf CreditAttribution: drummondf commentedI have not read all of the above, but had a thought:
If Comments are now wrapped into fields, couldn't Field Permissions be extended to work with the Comment field? My thought would be that I could install Field Permissions, and that would just add the appropriate permissions controls to the field, and thereby the commenting inherently.
Comment #60
Anas_maw CreditAttribution: Anas_maw as a volunteer commentedThe patch in #58 works fine for me.
Comment #61
Anas_maw CreditAttribution: Anas_maw as a volunteer commented@drummondf
I think field permission can't do the same feature, because the last patch give the option to skip the comment approval for each comment type also reply on same comment.
The patch in #58 is a great solution
Comment #64
a_grizzli CreditAttribution: a_grizzli commentedPatch #58 works visually fine! Tested on Drupal 8.5.6. Just added some CodeSniffer suggestions from #62 in new patch.
I think, this patch is a good solution to granulate permissions on comment-type level. However, should be adapted to results of #2879087: Use comment access handler instead of hardcoding permissions . Using AccessControlHandler for comment-type level will bring a nice improvement to this issue also.
Comment #65
a_grizzli CreditAttribution: a_grizzli commented@jefuri: Patch from #58 (so now also from #64) seems to miss removing some code in function
comment_node_update_index
. Seems to be not used code, since it has moved toCommentManager::isIndexingAvailable
:Comment #66
a_grizzli CreditAttribution: a_grizzli as a volunteer commentedRemove of unused code from #65 and adapt for 8.6.x and 8.7.x (trivial change in
CommentAccessControlHandler::checkAccess
needed)Comment #67
a_grizzli CreditAttribution: a_grizzli as a volunteer commentedPatch from #66 (and before) removes all permissions' checks:
...in favour of dedicated comment-type sensitive permissions.
To keep backward compatibility (and pass tests), we should keep old permissions, too. These would stay in sense of:
Furthermore, postpone until results of #2879087: Use comment access handler instead of hardcoding permissions
Comment #70
hgeshev CreditAttribution: hgeshev commentedI had problems applying the patch for 8.7.x, because of the following errors:
I fixed the errors in the following patch, tested it and it is applying and working:
https://www.drupal.org/files/issues/2019-12-16/comment_bundle_permission...
Comment #71
hgeshev CreditAttribution: hgeshev commentedComment #72
yivanov CreditAttribution: yivanov commentedHello, the patch was not applying for 8.8.x, so I rerolled it.
Comment #73
yivanov CreditAttribution: yivanov commentedSorry the previous one was not clean. I am adding the rerolled 8.8.x clean version now.
Comment #74
watson.sm#73
Perfect, exactly what I needed. Thx.
*Edit 2020-06-12
I think I've found an edge case.
Situation: A set of users are allowed to post top level comments & replies(Managers). Another set are NOT allowed to post top level comments, however they can post replies(employees).
Permissions:
- Managers
-- (comment type) Post Comments
-- (comment type) Edit Own Comments
-- (comment type) Reply Comments
-- (comment type) Skip Comment Approval
-- (comment type) View Comments
-- (Global) Edit Own Comments
-- (Global) Post Comments
-- (Global) Reply Comments
-- (Global) Skip comment approval
-- (Global) View comments
- Employees
--
(comment type) Post Comments-- (comment type) Edit Own Comments
-- (comment type) Reply Comments
-- (comment type) Skip Comment Approval
-- (comment type) View Comments
-- (Global) Edit Own Comments
-- (Global) Post Comments
-- (Global) Reply Comments
-- (Global) Skip comment approval
-- (Global) View comments
Since Employees have the permission for (comment type) Reply Comments they are presented with the comment form, however since they are lacking the (comment type) Post Comments permission their comments are being blocked.
Shouldn't their comments be allowed if they have the permission for (comment type) Reply Comments?
Side Question: Are the Global comment permissions still necessary with the more fine-tuned permissions based on type?
Comment #75
SkinI`ve just applied #73 patch to Drupal 8.8.x, , I have to test it a bit, but it seems ok. When can we see this option embedded in Drupal's core? I find this patch really useful.
Comment #78
DuneBLI have installed this patch on 9.1.4 (with few offset) and it is working as expected except that in
CommentLazyBuilders.php
we must replace twice$entity->urlInfo(xxx)
by$entity->toUrl(xxx)
.[Sorry, right now, I don't have time to reroll and correct this patch]
I change the status of this patch (no more postponed) because I don't think we need to wait #2879087: Use comment access handler instead of hardcoding permissions .
We don't need to wait because this patch can work out of the box and when #2879087 will land, it will use the new permissions created here.
Comment #79
jonathanshawNeeds reroll for #78
Comment #80
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll patch created. And replaced $entity->urlInfo(xxx) by $entity->toUrl(xxx) as mentioned in #78.
Comment #83
DuneBLHere is a re-roll for 9.3
Same as #80 expect in
core/modules/comment/src/CommentLazyBuilders.php
:What is following the
&&
was not in #80 but was in 9.3... Thus I keep it with the&&
part as I felt it was good.Comment #84
jonathanshaw#67 suggested postponing this issue on #2879087: Use comment access handler instead of hardcoding permissions . Is that necessary?
#67
#74
We need to decide on a backwards-compatability and possibly deprecation strategy for the global comment permissions.
#74:
Sounds like a bug in the patch.
Comment #89
timohuismanThe patch from #83 fails against 10.1.x and 11.x.
Comment #90
Shura80 CreditAttribution: Shura80 commentedHello, I upload a patch that may be applied after the following one in this issue:
https://www.drupal.org/project/drupal/issues/2879087
https://www.drupal.org/files/issues/2022-01-17/2879087-130-9.3.x.patch
Tested on a Drupal 9.5.10 installation.
Comment #91
Shura80 CreditAttribution: Shura80 commentedComment #92
Shura80 CreditAttribution: Shura80 commentedReuploaded the patch but it will not pass the test as it has to be applied after this one here:
https://www.drupal.org/files/issues/2022-01-17/2879087-130-9.3.x.patch
Comment #93
Shura80 CreditAttribution: Shura80 commentedComment #94
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedNew Patch created for 11.x, Given patch #92 is "Failed to apply", Please have a look.
Comment #95
vsujeetkumar CreditAttribution: vsujeetkumar at Srijan | A Material+ Company for Drupal India Association commentedPatch created, Fixed CCF issue, Please have a look.
Comment #96
smustgrave CreditAttribution: smustgrave at Mobomo commentedSeems to have caused test failures.
Comment #97
ovanes CreditAttribution: ovanes as a volunteer and at FFW commentedHello there,
I've re-rolled the patch mentioned in comment #83 for the Drupal 10.1.x version.
Thank you.
Comment #98
hannakras CreditAttribution: hannakras commentedI re-rolled the patch from #97 for Drupal 10.2. There was one tiny change that broke the patch for me.