Problem/Motivation

If an anonymous user tries to submit a comment with the author field set to an existing username, the following error is displayed:

The validation failed because the value conflicts with the value in uid, which you cannot access.

Screenshot (before patch):

Without the patch, a strange message is shown if a username is already in use.

Proposed resolution

Fix bug, add tests, commit

Remaining tasks

add test

User interface changes

"human readable" error message

API changes

no

Data model changes

no

CommentFileSizeAuthor
#49 comment-name-uid-violations-2572553.49.patch2.6 KBlarowlan
#49 interdiff.txt3.68 KBlarowlan
#44 comment-name-uid-violations-2572553.44.patch3.35 KBlarowlan
#44 interdiff.txt674 byteslarowlan
#42 comment-name-uid-violations-2572553.42.patch3.63 KBlarowlan
#42 interdiff.txt2.54 KBlarowlan
#39 comment-name-uid-violations-2572553.39.patch2.92 KBlarowlan
#39 interdiff.txt1.61 KBlarowlan
#35 after_patch_2.JPG99.98 KBTruptti
#35 after_patch.JPG98.37 KBTruptti
#35 patch_applied.JPG88.59 KBTruptti
#34 interdiff.txt1.11 KBLoMo
#32 incomprehensible-2572553-32.patch3.03 KBLoMo
#30 issue-2572553-verify_patch_fixes_issue.png497.23 KBLoMo
#30 issue-2572553-updated_pre_patch_screenshot.png503.89 KBLoMo
#26 comment-name-uid-violations-2572553.26.patch2.79 KBlarowlan
#26 interdiff.txt1.35 KBlarowlan
#22 comment-name-uid-violations-2572553.22.patch2.75 KBlarowlan
#19 comprehensible_message.png40.09 KBfil00dl
#19 incomprehensible_message.png38.65 KBfil00dl
#17 comment_with_existing_user_name_admin.png40.09 KBfil00dl
#17 comment_with_existing_user_name.png32.3 KBfil00dl
#11 2572553-11.patch1 KBUpchuk
#6 Screen Shot 09-22-15 at 02.28 PM.PNG26.88 KBmaartendeblock
#5 incomprehensible-2572553-5-TEST-ONLY.patch1.14 KBznerol
Screenshot from 2015-09-22 12:36:50.png53.43 KBznerol
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

znerol created an issue. See original summary.

maartendeblock’s picture

Status: Active » Closed (duplicate)
znerol’s picture

Status: Closed (duplicate) » Active

Then that is a regression. I can reproduce it in head. Though it is quite strange that this is not exposed by the automated test.

znerol’s picture

Oh, so that only happens if Anonymous commenting is set to Anonymous posters may not enter their contact information.

znerol’s picture

Status: Active » Needs review
FileSize
1.14 KB

The attached test-only patch exposes the problem.

maartendeblock’s picture

I get "The name you used belongs to a registered user." with the latest dev (see screenshot).

Didn't see your other comments.

LoMo’s picture

Status: Needs review » Active

#5 Indicates to me that this does not need a review next, but a (small) bugfix that that test exposes. @znerol, I believe, has correctly identified the situation and created a test, but we still need a fix for this remaining issue that was not corrected by #2403485

Since this issue ticket is not to create a new test to expose this bug, I should say this still "needs work" (or is "active", since the work or the bug-fix has not yet been done here, right?)

Status: Active » Needs work

The last submitted patch, 5: incomprehensible-2572553-5-TEST-ONLY.patch, failed testing.

The last submitted patch, 5: incomprehensible-2572553-5-TEST-ONLY.patch, failed testing.

Upchuk’s picture

Assigned: Unassigned » Upchuk
Upchuk’s picture

Assigned: Upchuk » Unassigned
Status: Needs work » Needs review
FileSize
1 KB

The problem here is that inside CommentAccessControlHandler::checkFieldAccess, the comment name field is considered to be part of the fields that represent Anonymous contact information. Yet, if the comment is set to not allow anons to provide this information, why is the Name field present on the comment form?

Going by the assumption that we need the Name field on the form, the attached patch removes the Name field from being denied access if the anons cannot submit contact info. Otherwise, the access denied causes down the line (see EntityConstraintViolationList::filterByFieldAccess() and ::filterByFields()) some problems with the composite constraint (which includes both name and uid for some reason).

So i'm not sure also why this code is in the violation list class (this adds the extra violation if the Name field gets access denied on edit...)

if ($violation->getConstraint() instanceof CompositeConstraintBase) {
          $covered_fields = $violation->getConstraint()->coversFields();

          // Keep the composite field if it covers some remaining field and put
          // a violation on some other covered field instead.
          if ($remaining_fields = array_diff($covered_fields, $field_names)) {
            $message_params = ['%field_name' => $field_name];
            $violation = new ConstraintViolation(
              $this->t('The validation failed because the value conflicts with the value in %field_name, which you cannot access.', $message_params),
              'The validation failed because the value conflicts with the value in %field_name, which you cannot access.',
              $message_params,
              $violation->getRoot(),
              reset($remaining_fields),
              $violation->getInvalidValue(),
              $violation->getPlural(),
              $violation->getCode(),
              $violation->getConstraint(),
              $violation->getCause()
            );
            $new_violations[] = $violation;
          }
        }

Status: Needs review » Needs work

The last submitted patch, 11: 2572553-11.patch, failed testing.

The last submitted patch, 11: 2572553-11.patch, failed testing.

Upchuk’s picture

Issue tags: +Barcelona2015
Upchuk’s picture

Status: Needs work » Postponed

There's a lot of work happening on the CommentForm (#2571909: CommentForm selects using the user formatted name) and this may either be fixed with that or should be postponed until that is fixed.

Upchuk’s picture

Status: Postponed » Needs work

I don't think it got fixed.

fil00dl’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
32.3 KB
40.09 KB

The 2572553-11.patch seems fix the initial issue -> validation message is now comprehensible : "The name you used (admin) belongs to a registered user", when an anonymous user tries to submit a new comment with an existing username.

In order to test correctly, don't forget to allow the "post comments" permission for anonymous user.

andypost’s picture

fixed IS

patch #11 needs test from #5

fil00dl’s picture

Issue summary: View changes
FileSize
38.65 KB
40.09 KB

the incomprehensible-2572553-5-TEST-ONLY.patch doesn't solve the issue.

The 2572553-11.patch fixes the issue.

Upchuk’s picture

I'm not super excited about my fix in #11. Are we sure that's the way to go? If it is, i think there are other places as well where we need to cut the Name field from the "group" of contact info fields.

larowlan’s picture

Assigned: Unassigned » larowlan

looking

larowlan’s picture

So we've split the fields into two as part of #2403485: Complete conversion of comment form validation to entity validation - this means that we only ever submit one value via the form - not both.

However, we cannot move to separate constraints (not a composite constraint) because it is still possible to submit both uid and name via REST.

So we need to make the comment form behave slightly differently to the other entity forms. Yes we respect that the violation is composite, but if the user cannot edit uid, we special case constraints relating to 'name' field.

Thoughts?

andypost’s picture

Suppose REST should use entity level constraint here, https://www.drupal.org/node/2438011
We already have \Drupal\comment\Plugin\Validation\Constraint\CommentNameConstraintValidator

@larowlan you was involved in this conversion so why this constraint is not propagated to form?

larowlan’s picture

@andypost it is propogated to the form, but if you cannot edit one of the fields covered by the composite constraint, it falls back to the message we're seeing in HEAD. That is the correct behaviour for 90% of the cases.

But comment form knows a bit more about the name/uid field than core's generic validation system so can intervene and modify what the default behaviour is.

larowlan’s picture

Status: Needs review » Needs work
larowlan
timplunkett: question is, is that unholy use of form state?

timplunkett
larowlan: i would probably try and use setTemporaryValue
larowlan’s picture

Status: Needs work » Needs review
FileSize
1.35 KB
2.79 KB

fixes #25

andypost’s picture

  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -323,12 +323,33 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    if (!isset($form['uid']) || $form['uid']['#access'] == FALSE) {
    +      $form_state->setTemporaryValue('name_violations', $violations->getByField('name'));
    ...
    +    if (!$name_violations = $form_state->getTemporaryValue('name_violations')) {
    +      $name_violations = $name_violations->getByField('name');
    

    That needs code comment...

  2. +++ b/core/modules/comment/src/CommentForm.php
    @@ -323,12 +323,33 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    else {
    +      $form_state->setTemporaryValue('name_violations', NULL);
    

    that one too, it's not clear that temporary value preserved within submissions

LoMo’s picture

Assigned: Unassigned » LoMo
Status: Needs review » Needs work

I think @andypost implies that we need proper documentation for some new helper functions, etc, before the patch can be marked RTBC. So I'll take a look at it (and either handle it or remove assignment from myself within the next few hours).

larowlan’s picture

Assigned: LoMo » Unassigned

as per #28 - thanks @LoMo

LoMo’s picture

Issue summary: View changes
Status: Needs work » Reviewed & tested by the community
FileSize
503.89 KB
497.23 KB

I've updated the issue summary with a current screenshot (it doesn't look quite the same as it used to, due to other issues having been resolved). In any case, the incomprehensible message has been verified when an anonymous user attempts to post a comment (on a site configured such that anonymous users are allowed to post comments). If "admin" is already used as a username and is used in the form, the message is certainly not clear.

After applying the patch, we get a much more comprehensible message, one which I agree is reasonably appropriate and not to hard to make sense of:
After the patch, the message makes a lot more sense

I cannot mark this RTBC, since I'm adding a patch... and I still need to create that. Just a moment...

LoMo’s picture

Status: Reviewed & tested by the community » Needs work

Oops... marked it RTBC, should be needs work still (doing the work). Then it will need a review. I've at least got an environment that supports testing it, have verified the patch works, as advertised, and will now look at properly commenting the code and add a new .patch.

LoMo’s picture

Status: Needs work » Needs review
FileSize
3.03 KB

Patch updated to include some comments for the code andypost marked. Needs review, but I think this should suffice. The patch works and has already passed. This only changes code comments (no interdiff created since this is just such a small patch).

geertvd’s picture

Even though it's a small patch an interdiff would still be very helpful to review this change.

LoMo’s picture

FileSize
1.11 KB

(LoMo grumbles to himself about making an interdiff for such a trivial thing... ;P )

And... interdiff uploaded...

Truptti’s picture

FileSize
88.59 KB
98.37 KB
99.98 KB

Verified the patch incomprehensible-2572553-32.patch in #32.Error message displayed now is 'The name you used (admin) belongs to a registered user.' Marking this as RTBC

Truptti’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

Just a couple small points before this goes in.

  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -323,12 +323,36 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    // Check for name violations for anonymous users,
    +    // e.g. using the name of a registered user:
    

    This comment is wrapped too short. Also the trailing colon should be a period.

  2. +++ b/core/modules/comment/src/CommentForm.php
    @@ -323,12 +323,36 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    if (!$name_violations = $form_state->getTemporaryValue('name_violations')) {
    +      $name_violations = $name_violations->getByField('name');
    +    }
    

    This could use a simple comment I think.

  3. +++ b/core/modules/comment/src/CommentForm.php
    @@ -323,12 +323,36 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    else {
    +      // Clear name_violations temp value if no name violation exists now;
    +      // this temporary value may otherwise still have a value.
    +      $form_state->setTemporaryValue('name_violations', NULL);
    +    }
    

    Why do we need to do this? It's a temporary value, it will be discarded appropriately.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

larowlan’s picture

@larowlan applies @LoMo's interdiff, so his branch includes latest changes

So yep, it was useful.

Raising this one from an un-dead state.

Here's a new patch and addresses the points in #37. We must be close now.

larowlan’s picture

Status: Needs work » Needs review
tim.plunkett’s picture

Not sure why I didn't notice this in my first review, but:

  1. +++ b/core/modules/comment/src/CommentForm.php
    @@ -316,12 +316,33 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    $entity = $this->buildEntity($form, $form_state);
    ...
    +    $violations = $entity->validate();
    ...
    +    return parent::validateForm($form, $form_state);
    

    This is now calling $entity->validate() twice. Is that idempotent?

  2. +++ b/core/modules/comment/src/CommentForm.php
    @@ -316,12 +316,33 @@ protected function getEditedFieldNames(FormStateInterface $form_state) {
    +    if (!isset($form['uid']) || $form['uid']['#access'] == FALSE) {
    +      $form_state->setTemporaryValue('name_violations', $violations->getByField('name'));
    +    }
    ...
    -    foreach ($violations->getByField('name') as $violation) {
    +    // If no additional violations were flagged by ::validateForm, get the
    +    // default violations.
    +    if (!$name_violations = $form_state->getTemporaryValue('name_violations')) {
    +      $name_violations = $name_violations->getByField('name');
    +    }
    +    foreach ($name_violations as $violation) {
    

    Why compute, set, and then get elsewhere? Couldn't this just do the check in flagViolations? It has access to the full $form still...

larowlan’s picture

Yeah, lets forget the parent method, we're dancing around it

Status: Needs review » Needs work

The last submitted patch, 42: comment-name-uid-violations-2572553.42.patch, failed testing.

larowlan’s picture

Status: Needs work » Needs review
FileSize
674 bytes
3.35 KB

yeah, naffed that up - too heavy on the deleting - this is better

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

This looks great, thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work

This is a lot of logic for what ends up being a single line form validation error - it could use a high level comment explaining why we have to handle all this here vs. adding an entity-level constraint or similar.

Also I wonder a bit if we couldn't do this in flagViolations() with a bit less logic. Look for a violation on the name field, and add it to the other ones? Although that would probably mean validating the entity twice or something. If that's a terrible idea just say so - CNW for the comment anyway.

larowlan’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
2.6 KB

So I did some digging.... and I think the issue is actually in the access controller.

The name field isn't hidden/shown depending on anonymous contact information settings, only mail and homepage are.

If you think about it, we ask them for the field value on a new comment, so they have to have edit access.

So after fixing that logic flaw, things seem to work. Let's see what bot says.

keesje’s picture

Patch in #49 confimed as working on 8.2.1. You will still be redirected to comment/reply/*, but the warning is much more usefull. Though some might argue dat telling that "this username exists" might be a information disclosure in some cases?

keesje’s picture

Status: Needs review » Reviewed & tested by the community

I'm comfortable with this fix

  • xjm committed 3e58b49 on 8.3.x
    Issue #2572553 by larowlan, LoMo, znerol, Upchuk, fil00dl, Truptti,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Ah wow, I love it when it turns out to be a simple logic fix like that. Nice sleuthing @larowlan!

Though some might argue dat telling that "this username exists" might be a information disclosure in some cases?

I thought about that too, but the latest patch shows that is a behavior that's already established in HEAD, and we are just fixing HEAD to use the right code path in this one case. Also, the security team's policy is that usernames are not considered private data out of the box.

Committed 3e58b49 and pushed to 8.3.x and 8.2.x. Thanks!

  • xjm committed 812f57d on 8.2.x
    Issue #2572553 by larowlan, LoMo, znerol, Upchuk, fil00dl, Truptti,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

/me squints at d.o

Status: Fixed » Needs work

The last submitted patch, 49: comment-name-uid-violations-2572553.49.patch, failed testing.

andypost’s picture

Status: Needs work » Fixed

yay! glad to see this fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.