Admin should not be able to edit email of authenticated commenters

Is this what you had in mind @larowlan?

Hi,

I also came up with a solution for this.

Changed status to need review.

Reworked the patch a bit.

Why admins should not be able to edit?

Hi,

I used SimplyTest to test the patch at #7.

First, I created a Basic Page node with the admin user. Then, I created an "authenticated user" called "test". As the "test" user, I commented on the node. Then, I got into the comment edition form using the admin user as shown in the following screenshot.

This screenshot depicts the email field disabled. Therefore, I tried to force an email variation by directly modifying the HTML (as shown in the next screenshot) but it wasn't modified in database after submit.

Finally, I changed the author of the comment using the "Authored by" autocomplete field and the email did change this time to the email of the new author (shown in the following screenshots).

@piggito see comment #8. I don't think the patch you're testing is the right one. Thanks.

@larowlan I will indeed be in Melbourne :)

this is pinger's patch from #2. Just re-enqueueing it here at the top of the file list so it's clear which one to test with.
I applied it, went through the test steps, and the email field is gone, see screenshot.

Would it make sense to disable the field, so the admin can still view the email of the user?

Created a patch..please review..

If we disable email(#disabled) and still allow an administrator to change the comment owner, the email address does not change. You will see the previous comment owner's email address when editing the comment owner. This feels wrong to me. On these grounds, I think we should go with my original patch and hide the email address for authenticated comments (via #access).

Currently, the original commenter's email is still stored if you only change the user name (in comment_field_data.mail). But, at least if we hide the email from UI for authenticated comment, changing the owner of a comment makes sense in the UI.

@Swarnendu-Dutta

1. +++ b/core/modules/comment/src/CommentForm.php
   @@ -149,6 +149,7 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $form['author']['name']['#attributes']['disabled'] = TRUE;
   

   Using #attributes to disable a field means the value can still be updated via Javascript and will be saved. Should use Form API #disabled.

   Changing name is out of scope of this issue.

2. +++ b/core/modules/comment/src/CommentForm.php
   @@ -171,6 +172,9 @@ public function form(array $form, FormStateInterface $form_state) {
   +      $form['author']['mail']['#attributes']['disabled'] = TRUE;
   

   This is wrong for the same reason as above, should use Form API #disabled.

   Also, disabling the email field for administrators under all conditions is wrong. We only want to prevent changing the email of comments owned by authenticated users.

@pingers, So can we use readonly here ??? i know it can be exploited through firefug but atleast the admin wont be able to edit through the UI..

@Swarnendu-Dutta "Admin should not be able to edit email of authenticated commenters".

Read-only very much applies in this case... or not at all. This issue is creeping in scope and making a simple change complex.

Let's close it out... patch from comment 2 still stands in my mind. Just needs review.

#2 looks good to me. Has passed all the manual testing I threw at it. Also seems to make the most sense UIX-wise. If the email field remains in the UI, it leads to a user expectation that it could be changed here. I don't think there's a good argument for leaving the field there as non-editible, since the author is still there, identified by username, adding a re-identification by email doesn't seem to add anything to the UI, and just clutters it unnecessarily, in my opinion.

We should have a test for this and also if #2 is the patch to go for then is needs to be re-uploaded since the rtbc retest only tests the last patch on each issue.

@alexpott,
Re-uploaded and setting this to RTBC.

Still needs a test.

Updated comment email field access settings based on settings specified in content type settings.

Thanks for adding that.

I guess we can just set that setting with $this->setCommentAnonymous('2'); since we really need that contact information for this test.

Then these lines can be removed:

+++ b/core/modules/comment/src/Tests/CommentAdminTest.php
@@ -168,4 +168,43 @@ class CommentAdminTest extends CommentTestBase {
+    $field_name = $comment->getFieldName();
+    $field_definition = $this->entityManager->getFieldDefinitions($entity->getEntityTypeId(), $entity->bundle())[$comment->getFieldName()];
+    $anonymous_contact = $field_definition->getSetting('anonymous');
...
+    if($anonymous_contact == COMMENT_ANONYMOUS_MUST_CONTACT) {
...
+    }

Looks like you forgot to add the updated test in the complete patch also

added the updated test from #32 and the idea from #35.

I think
$this->setCommentAnonymous('2');
should be above
$anonymous_comment = $this->postComment($this->node, $this->randomMachineName());

$this->setCommentAnonymous('2'); should be above $anonymous_comment = $this->postComment($this->node, $this->randomMachineName()); like #38 is saying.

+++ b/core/modules/comment/src/Tests/CommentAdminTest.php
@@ -168,4 +168,39 @@ public function testCommentAdmin() {
+    $entity = $this->entityManager->getStorage($comment->getCommentedEntityTypeId())->load($comment->getCommentedEntityId());

What's happening here? I don't see this variable being used anywhere.

Best to always add a failing test and the complete patch.

Updating the patch as per the comment and moving as per #38

Changing patch and adding admin login for setting comment anonymous setting.

Just missed adding the contact info while posting the anonymous comment. This should fix it.

I followed the steps.
Looks good (e-mail of logged in user can't be changed by admin - for an anon user it still can be changed).

So there is no argumentation in this issue? Other than an opinion, that it shouldn't. Is it legally their property? Are people actually not allowed to do this?

+1 rtbc
@Bojhan the admin can change the author here, and they can go into the author's account and change the email address there. I think the point is you don't want the author's email editable here cause it makes it ambiguous. If you edit it here, is it editing it just here, or will that change also show in the author's account? From UIX, it's cleaner, with a single connection, comment-to-author(by name), rather than 2 redundant connections, comment-to-name and comment-to-email.

This issue addresses a major bug and is allowed per https://www.drupal.org/core/beta-changes. Committed 5430ac9 and pushed to 8.0.x. Thanks!