Problem/Motivation
My use case is that my usernames and email addresses match exactly - they have been imported from a previous system where users logged in with their email address and had no username. This works for almost all users, except where an email address contains the + character. In those cases my users are unable to edit their profiles because drupal throws a validation error on save:
The username contains an illegal character.
Proposed resolution
Add + character to the regex.
Remaining tasks
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Comment | File | Size | Author |
---|---|---|---|
#193 | 2157963-3-193.patch | 3.53 KB | alexpott |
#188 | 2157963-3-188.patch | 3.58 KB | alexpott |
#188 | 183-188-interdiff.txt | 1.75 KB | alexpott |
#183 | 2157963-3-183.patch | 2.71 KB | alexpott |
#180 | 2157963-3-180.patch | 1.8 KB | alexpott |
Comments
Comment #1
Sloshed Cause CreditAttribution: Sloshed Cause commentedThanks for this. I agree that this should be added to allow usernames and emails to match providing it won't produce any unwanted side effects.
Comment #2
themic8 CreditAttribution: themic8 commentedI will test the Drupal 7 patch.
Comment #3
themic8 CreditAttribution: themic8 commentedThe that has been provided works.
Comment #4
themic8 CreditAttribution: themic8 commentedUpdate file for testing.
Comment #5
themic8 CreditAttribution: themic8 commentedComment #7
themic8 CreditAttribution: themic8 commentedTested Drupal 7 patch only.
Comment #9
jenlamptonI'm still using this D7 patch, but we'll need some work done on this in order for it to get in to D8.
Comment #10
dcam CreditAttribution: dcam commentedClosing #1821974: Allow plus sign in user_validate_name to be able to use email as login as a duplicate. It's an older issue, but this one has a slightly more recent Drupal 8 patch than the other.
Comment #13
dcam CreditAttribution: dcam commentedThe Drupal 8 patch in the original post needs to be rerolled.
Comment #14
droplet CreditAttribution: droplet commentedComment #15
kpv CreditAttribution: kpv commentedworking on it
Comment #16
kpv CreditAttribution: kpv commentedComment #17
kpv CreditAttribution: kpv commentedComment #18
star-szrWe should probably add/expand upon automated test coverage for this functionality. Thanks for the reroll/repatch @kpv!
Comment #19
benjf CreditAttribution: benjf commentedupdated patch to include a test
Comment #20
benjf CreditAttribution: benjf commentedAdding a test-only patch.
Comment #21
star-szrThanks @kpv and @benjf! This seems backportable to me.
Comment #23
dcam CreditAttribution: dcam commentedSetting to Needs review since #20 was a tests-only patch and was supposed to fail.
Comment #24
star-szr#19 looks good and has tests.
Comment #25
alexpottThis seems sane since more and more sites just use an email address as a username - also see #2014185: Check usernames that are email addresses more rigidly, only allow if matches email - a plus is a valid character an email address.
Let's see if the attached patch breaks anything.
Comment #27
thijsvdanker CreditAttribution: thijsvdanker commentedI'm having a look as to why the test is failing.
Comment #28
thijsvdanker CreditAttribution: thijsvdanker commentedTest fails because of two reasons.
Regarding 1. there are two ways to go:
The second url issue probably needs to be fixed either at test or at code level.
Comment #29
thijsvdanker CreditAttribution: thijsvdanker commentedI've created a separate issue here #2349527: testUrlAlter fails for usernames with a + for the second issue (urlAlter).
Comment #30
lokapujyaThe full dblog text is shown in the hover text, so assertRaw() should work.
Comment #32
alexpottSo I don't think making all the random test usernames over twice as long is a good idea - I just did this to test. The problem we have is that we never test usernames with spaces, @, +, . or other allowed characters. Which means we write poor tests like UrlAlterFunctionalTest.
We need to somehow generate random strings with only these allowed characters.
Comment #33
alexpottPatch attached ensures that usernames generated by web test base might contain all types of valid characters. We no longer need to fix ContactPersonalTest since username length is back to normal. I've included the patch in #2349527: testUrlAlter fails for usernames with a + since I don't think this should be a separate issue - it is a result of ensuring we have proper test coverage for this change so let's fix it here.
Below is a sample of the code simulating how the patch generates usernames and the possible output - it's going to be interesting to see if this introduces any random error.
Outputs...
Comment #34
alexpottHmmm.... usernames starting or ending with a space or having double spaces will be problematic. We can leverage TestBaseTest::randomStringValidate().
Comment #37
lokapujyaShould we have a testBase method like randomUsername() ?
Comment #38
alexpottSo the last patch generates invalid email addresses :)
re #37 @lokapujya I don't think so - that is what drupalCreateUser is for.
Patch attached fixes this with the benefit that we now also start testing having + and . in email addresses before the @ sign.
Below are sample values generated whilst repeating Drupal\system\Tests\Datetime\DrupalDateTimeTest
Comment #40
alexpottForget to rebase
Comment #42
alexpottThis will fix
Drupal\user\Tests\UserPasswordResetTest
- that was asserting on mail subject without mime encoding it.The test fail in
Drupal\config_translation\Tests\ConfigTranslationOverviewTest
is not worrying since this means it has tried over 100 times to generate a valid username and has failed. We might need to look at how Random::string() chooses possible characters.Comment #44
lokapujyaReroll.
Comment #46
lokapujyafunction used in #42 is deprecated.
Comment #48
lokapujyaregarding: choose valid characters -- attaching some local code I was messing around with.
Comment #49
lokapujyaComment #51
thtas CreditAttribution: thtas commentedI had a go at re-rolling this patch as it wasn't applying
Also changed the tests a bit to stop sanitizing the username and site in the testUserPasswordReset
This is because these values aren't santized in user_mail because "replacement is intended for an email message, not a web browser" and the matching against escaped characters was failing in some cases depending on the random string chosen for the test.
Comment #53
thtas CreditAttribution: thtas commentedSeems to be causing continued issues with other tests where there are username comparisons are in assertions.
Because usernames are generated randomly in tests, tests will only fail if there happens to be these these extra characters in the usernames, so the failure stats in the above patch are probably way higher.
Not sure the best way forward, maybe all the calls to assertText() should all be change to assertEscaped() if they are only checking a username?
Comment #54
lokapujyaCan we find out which characters are not passing? I ran the UserSearchTest test a few times locally but couldn't reproduce a failing test. Then, if it's reasonable that those characters would be escaped, then try the assertEscaped()?
Comment #55
thtas CreditAttribution: thtas commentedAttached is the verbose output of an example where a test failed (locally) because of an encoded character
This is from line 101 of UserSearchTest
It's failing because it's looking for the text
so'MoCaY
but the text in the source isso'MoCaY
Comment #58
Noe_ CreditAttribution: Noe_ at Devhouse Spindle commentedRerolled from comment #51.
It should test ok now.
Comment #62
lokapujyaThanks for rerolling! I think we now need to fix the issue with assertions described in 53-55, possibly by using assertEscaped().
Comment #63
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI run the test to fail locally and all are correct except "UrlAlterFunctionalTest".
To resolve this error I used the function urldecode in assertUrlOutboundAlter function. (See interdiff)
Hope it works.
Comment #65
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedUpdate the issue.
I think the error in the test UrlAlterFunctionalTest is because the url is generated differently.
Once you use the same method to get the url, the error disappears, I think.
It would be correct? Any other ideas?
On the other hand the error in the test BulkFormTest not get replicate.
Hope it works!
Comment #68
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedIn the function drupalCreateUser user name is generated randomly.
This may cause the attached patch is right or maybe not.
I tested locally and it works, but as you can see, on the first try the patch failed.
Needs work?
Comment #69
lokapujyaWhat happens if you make the test use
so'MoC@Y
instead of a random for the username on that test that failed?Comment #70
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedI tested with the user "
so'MoC@Y
" and the result can be seen in the attached imageComment #73
lokapujyaThe second failed test contained a single quote.
Comment #74
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedThe test failed is "Fastest.php" no "UrlAlterFunctionalTest.php" which was failing before, I think.
Comment #75
lokapujyaWe need to make the test consistently pass. Seems like it still fails randomly?
dimaro: Are you still trying to get it to work?
Comment #76
thtas CreditAttribution: thtas commentedIt might be worth temporarily changing the random username generation code to always include a failure character (quote or w/e), just for the purpose of tracking down all the different failure cases. The last thing we want is for this to pass tests based on lucky random names...
Comment #77
jaydee1818 CreditAttribution: jaydee1818 commentedI have a use case where user accounts in Drupal are created via an API to an external app's usernames. This app allows the '%' symbol to be used in usernames; Drupal does not. Is it possible to have something in the UI that allows choosing of extra symbols that can be allowed in a username?
Comment #80
lokapujyaComment #81
lokapujyaRerolled.
Comment #83
fuzzy76 CreditAttribution: fuzzy76 commented@jaydee1818 I'm pretty sure that a) that should be a separate issue and b) the percentage-sign (%) would require a huge amount of testing and bugfixing to support since it has special meanings both in string-formatting / placeholders and in url encoding. There are a lot of potential side-effects / secondary bugs.
Comment #84
Nikolay ShapovalovComment #87
Nikolay ShapovalovRerolled.
Comment #88
Nikolay ShapovalovComment #89
kekkisUpdated status to make the last patch testable.
Comment #91
subhojit777Comment #92
subhojit777I guess some tests are randomly failing, because I tested them on local and it passed.
Comment #94
subhojit777These tests are passing in my local. Have tested them with 5.5.x and 5.6.x. Any idea why they are failing here.
Comment #95
subhojit777Comment #96
Anonymous (not verified) CreditAttribution: Anonymous at XIO commentedAfter applying the patch, I tried UsersBlockTest locally. First time it gave me a failure, but a different one than for testbot (user 2 instead of user 1), the second and third time it was green.
I didn't look into the patch too much, but it sure does look random to me.
Comment #97
serundeputy CreditAttribution: serundeputy at Common Media commented@pjonckiere @subhojit777
see #70 and #76; the tests randomly fail since the user names being tested are generated at random. sometimes tests have user names with a single quote and sometimes not.
Comment #100
nlisgo CreditAttribution: nlisgo commentedGoing to see if I can help out on this issue. The first thing I will want to address is ensuring that the test run is consistent.
Comment #101
lokapujyaThank You nlisgo! That would be very helpful if the data that causes the failure was used every test run.
Comment #102
Nikolay ShapovalovUse asserEscaped instead of assertText when searching for email or username in UserSearchTest.php.
Run UserSearchTest 100 times and 12801 (100%) test passes, 0 fails.
Comment #103
nlisgo CreditAttribution: nlisgo commentedComment #104
Nikolay ShapovalovUpdated status to make the last patch testable.
Comment #106
Nikolay ShapovalovI want check all tests with username having apostrophes.
Just for test purposes, my local tests is very very slow.
Please use #102.
Comment #108
Nikolay ShapovalovComment #109
Nikolay ShapovalovThere are some failing tests with apostrophes in a username
So FastTest falls because /user/autocomplete is removed.
BreadcrumbTest falls because UserController::userTitle(), title (username) of user edit page doesn't escaped, and breadcrumb doesn't escaped either.
But AssertBreadcrumbTrait::assertBreadcrumbParts is searching escaped breadcrumb. As I found, try to escape title isn't a good idea, so we should do something with breadcrumb.
Comment #110
Nikolay ShapovalovComment #111
Nikolay ShapovalovComment #114
jenlamptonPatch in #4 still applies cleanly to 7.39
Comment #117
lokapujyaAdding the single quote back until all the fails are gone. Fixing one fail.
Comment #120
biguzis CreditAttribution: biguzis commentedFixed some fails. For some was expected user name with apostrophe, but output was with
'
so I decode those values and i got equal values.Comment #121
biguzis CreditAttribution: biguzis commentedInterdiff
Comment #123
biguzis CreditAttribution: biguzis commented#120 has wrong file
Comment #124
biguzis CreditAttribution: biguzis commentedComment #126
biguzis CreditAttribution: biguzis at Wunder commentedonce again missed correct file. :(
___
okey, some of tests now are passing and new test are failing. Right now failing tests are:
Drupal\search\Tests\SearchConfigSettingsFormTest
Drupal\system\Tests\Menu\BreadcrumbTest
Drupal\simpletest\Tests\SimpleTestTest
Drupal\system\Tests\Theme\FastTest
Drupal\tracker\Tests\TrackerTest
Drupal\user\Tests\UserCancelTest
Drupal\user\Tests\UserPictureTest
Drupal\user\Tests\Views\BulkFormAccessTest
Comment #132
Sir-Arturio CreditAttribution: Sir-Arturio at Druid commentedWorking on this at DrupalCon Barcelona.
Comment #133
Sir-Arturio CreditAttribution: Sir-Arturio at Druid commentedReleased for someone else.
Comment #134
lokapujyaworking on an update.
Comment #135
lokapujyaComment #138
lokapujyaKeeping change in HEAD, although something might need to be changed later.
Comment #143
lokapujyaComment #146
biguzis CreditAttribution: biguzis at Wunder commentedFixed some tests
Comment #149
lokapujyaComment #152
lokapujyaComment #155
lokapujyaI'm not really sure that we really want to support some of these characters in usernames and whether I should continue patching tests like this.
Comment #158
biguzis CreditAttribution: biguzis at Wunder commentedAs reporter said, there can be situations, when e-mails and usernames are the same and can be imported from some other place (for example using LDAP). And who knows is there some of these character or not, but if there is, then someone wont be able log in. Yes, these emails with apostrophe, plus sign and all other special characters looks weird and there is not much services, that allows some of them. Personally haven't seen any e-mail with apostrophe character.
Don't know if its worth to deal with apostrophe thing.
Comment #159
lokapujyaComment #160
lokapujyadelete this comment.So, even if we support some characters, we might still not want to allow them by default.Comment #161
lokapujyanot tested. But, why does the test need to run HTML:escape() here.
Comment #164
lokapujyaThe first change at least is not outputted to a browser. The second one, not sure.
Comment #165
cilefen CreditAttribution: cilefen commentedThis issue needs a summary summary update to explain its exact goals. This issue isn't a Task. I contend this is a feature and therefore must be moved to 8.1.x because 8.0 is in RC. If someone wants to make the argument this is a bug, please do.
Comment #166
lokapujyaAgreed that it's a feature.
Comment #167
subhojit777Comment #168
lokapujyaRight now the issue summary says allow + symbol. But the actual patch is allowing more characters, so adding back the tag.
Comment #169
Nikolay ShapovalovTotally agree.
Comment #170
AaronBaumanShould this be postponed on #2014185: Check usernames that are email addresses more rigidly, only allow if matches email ?
Comment #172
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedComment #173
biguzis CreditAttribution: biguzis at Wunder commentedRerolled
Comment #174
biguzis CreditAttribution: biguzis at Wunder commentedComment #175
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedComment #176
lokapujyaApparently , Drupal already
supportsallows apostrophes. So if we have written some test cases that are failing due to apostrophes then they might be existing bugs.Comment #179
dimaro CreditAttribution: dimaro at La Drupalera by Emergya commentedComment #180
alexpottSo having + signs in a user name is not a feature. A best this is a task and for some sites they might consider this a bug.
I made a scoping mistake 2 years ago. I shouldn't have expanded this out to include testing things like apostrophe's and more randomness in the names. That should be in a separate issue. If I had kept the scope in check this would have made it into D8. alexpott-- My intentions to have better test coverage were right but my implementation in this issue was very wrong.
Here's a patch that does exactly what we need it to.
Comment #181
BerdirWe also have sites that hide the username and use the e-mail for that and I did run into the exact same problem when testing with my usual +something test email.
Patch looks good to me and agree with alex pot that this is a task. Also removing the now unnecessary remaining tasks.
Comment #183
alexpottRerolled and improved test coverage.
Comment #184
BerdirHuh, what is that with the alias thing exactly, how is that related?
Maybe provide a test-only patch if that indeed fails without the fix?
Comment #185
alexpott@Berdir since username aliases are common and we're now permitting + in user names which are url encoded it just seems prescient to have a user with a plus sign in the alias test. Nothing should be broken but it just ensures that no assumptions are being made.
Comment #186
BerdirI see. That test has nothing to do with url aliases, it's url altering :) It doesn't create/store aliases, it does runtime inbound/outbound processing.
Can we improve the comment there a bit to explain what's actually going to happen in the test?
Comment #187
jhodgdonIt might be a good idea to make sure this doesn't break anything with the User search plugin too. There are tests for this plugin in \Drupal\user\Tests\UserSearchTest.
Comment #188
alexpottEnsured this is tested for UserSearch... no problems with the plus sign. assertText has a problem with single quotes - the dom manipulator converts all of these :( but as that is not in scope left it out.
Comment #189
jhodgdonGreat! The revised comment looks good, and assuming the tests pass, I think this should be RTBC again.
Comment #190
lokapujyaAllowing the + simplifies manual testing of sites that automatically use the email as the username. RTBC++
Comment #192
jhodgdonHm. The Postgres failure on the last patch seems like an unrelated problem. But the patch apparently needs a reroll now. We should probably test it against SQLite and PostgreSQL...
Comment #193
alexpottSimple rebase fixed it.
Comment #195
catchCommitted/pushed to 8.2.x, thanks!
Comment #196
Wim LeersWhat a wonderfully simple patch :)
Comment #197
chrowe CreditAttribution: chrowe at Isovera commentedComment #199
jenlamptonPatch in #4 still applies cleanly to 7.50.
Comment #200
jenlamptonNot sure if the D7 patch needs additional tests or not, but setting to RTBC for maintainers to decide.
Comment #201
naveenvalecha@jenlampton
I've created the d7 issue https://www.drupal.org/node/2790923 so that issue credits gets preserved here and there as well. Please do the followup there