Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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):
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
Comment | File | Size | Author |
---|---|---|---|
#49 | comment-name-uid-violations-2572553.49.patch | 2.6 KB | larowlan |
#49 | interdiff.txt | 3.68 KB | larowlan |
#44 | comment-name-uid-violations-2572553.44.patch | 3.35 KB | larowlan |
#44 | interdiff.txt | 674 bytes | larowlan |
Comments
Comment #2
maartendeblock CreditAttribution: maartendeblock at EntityOne commentedThis has been fixed in #2403485: Complete conversion of comment form validation to entity validation
Comment #3
znerol CreditAttribution: znerol commentedThen that is a regression. I can reproduce it in head. Though it is quite strange that this is not exposed by the automated test.
Comment #4
znerol CreditAttribution: znerol commentedOh, so that only happens if Anonymous commenting is set to Anonymous posters may not enter their contact information.
Comment #5
znerol CreditAttribution: znerol commentedThe attached test-only patch exposes the problem.
Comment #6
maartendeblock CreditAttribution: maartendeblock at EntityOne commentedI get "The name you used belongs to a registered user." with the latest dev (see screenshot).Didn't see your other comments.
Comment #7
LoMo CreditAttribution: LoMo as a volunteer commented#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?)
Comment #10
Upchuk CreditAttribution: Upchuk at Wunder commentedComment #11
Upchuk CreditAttribution: Upchuk at Wunder commentedThe problem here is that inside CommentAccessControlHandler::checkFieldAccess, the comment
name
field is considered to be part of the fields that representAnonymous 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...)
Comment #14
Upchuk CreditAttribution: Upchuk at Wunder commentedComment #15
Upchuk CreditAttribution: Upchuk at Wunder commentedThere'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.
Comment #16
Upchuk CreditAttribution: Upchuk at Wunder commentedI don't think it got fixed.
Comment #17
fil00dl CreditAttribution: fil00dl at Skilld commentedThe 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.
Comment #18
andypostfixed IS
patch #11 needs test from #5
Comment #19
fil00dl CreditAttribution: fil00dl at Skilld commentedthe incomprehensible-2572553-5-TEST-ONLY.patch doesn't solve the issue.
The 2572553-11.patch fixes the issue.
Comment #20
Upchuk CreditAttribution: Upchuk at Wunder commentedI'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.
Comment #21
larowlanlooking
Comment #22
larowlanSo 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?
Comment #23
andypostSuppose 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?
Comment #24
larowlan@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.
Comment #25
larowlanComment #26
larowlanfixes #25
Comment #27
andypostThat needs code comment...
that one too, it's not clear that temporary value preserved within submissions
Comment #28
LoMo CreditAttribution: LoMo as a volunteer commentedI 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).
Comment #29
larowlanas per #28 - thanks @LoMo
Comment #30
LoMo CreditAttribution: LoMo as a volunteer commentedI'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:
I cannot mark this RTBC, since I'm adding a patch... and I still need to create that. Just a moment...
Comment #31
LoMo CreditAttribution: LoMo as a volunteer commentedOops... 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.
Comment #32
LoMo CreditAttribution: LoMo as a volunteer commentedPatch 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).
Comment #33
geertvd CreditAttribution: geertvd at XIO commentedEven though it's a small patch an interdiff would still be very helpful to review this change.
Comment #34
LoMo CreditAttribution: LoMo as a volunteer commentedAnd... interdiff uploaded...
Comment #35
Truptti CreditAttribution: Truptti at Axelerant commentedVerified 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
Comment #36
Truptti CreditAttribution: Truptti at Axelerant commentedComment #37
tim.plunkettJust a couple small points before this goes in.
This comment is wrapped too short. Also the trailing colon should be a period.
This could use a simple comment I think.
Why do we need to do this? It's a temporary value, it will be discarded appropriately.
Comment #39
larowlanSo 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.
Comment #40
larowlanComment #41
tim.plunkettNot sure why I didn't notice this in my first review, but:
This is now calling $entity->validate() twice. Is that idempotent?
Why compute, set, and then get elsewhere? Couldn't this just do the check in flagViolations? It has access to the full $form still...
Comment #42
larowlanYeah, lets forget the parent method, we're dancing around it
Comment #44
larowlanyeah, naffed that up - too heavy on the deleting - this is better
Comment #46
tim.plunkettThis looks great, thanks!
Comment #47
andypost+1 rtbc
Comment #48
catchThis 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.
Comment #49
larowlanSo 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.
Comment #50
keesje CreditAttribution: keesje commentedPatch 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?
Comment #51
keesje CreditAttribution: keesje commentedI'm comfortable with this fix
Comment #53
xjmAh wow, I love it when it turns out to be a simple logic fix like that. Nice sleuthing @larowlan!
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!
Comment #55
xjm/me squints at d.o
Comment #57
andypostyay! glad to see this fixed