680538 - AMO usernames are synced to forum in all lower caps

Status: RESOLVED WONTFIX

(addons.mozilla.org Graveyard :: Forums, defect, P2)

Description

[Jorge Villalobos [:jorgev] (he/him)]

According to the linked forum topic, the usernames in the forum are all lower case even if the username on AMO is has upper case characters. Since phpBB supports cased usernames correctly, this appears to be a bug in the synchronization code. Casing should be respected if possible.

Comment 1

[Jorge Villalobos [:jorgev] (he/him)]

Since the reported apparently refuses to create a Bugzilla account, here's his proposed fix, from the forum.


The Problem code - uses utf8_clean_string()

  Code:
    // Check if they've changed their name or email on the AMO side.   If they have, update them in phpbb.
    if (($row['username'] != $amouser['username']) || ($row['user_email'] != $amouser['email'])) {
       $sql =' UPDATE '.USERS_TABLE.'
             SET username="'.$db->sql_escape(utf8_clean_string($amouser['username'])).'"
             , user_email="'.$db->sql_escape(utf8_clean_string($amouser['email'])).'"
             WHERE user_id = "'.$db->sql_escape(utf8_clean_string($amouser['id'])).'"';
       $db->sql_query($sql);
    }


...it seems a side-effect (or intended effect?) of "utf8_clean_string()" is it lowercases the string too (instead of just "utf8_clean"ing it, whatever that means).

The utf8_clean_string() function call needs removed...or replaced with a similar function, that will "utf8_clean_string" without also lowercasing it. The Username is already sql_escape()'d, so I wonder if any utf8_clean_string() is even necessary?...is the Username unsafe for insertion without utf8_clean_string()?...actually, these values are directly from the AMO db, so shouldn't they already be safe?

The Fixed code - utf8_clean_string() removed

  Code:
    // Check if they've changed their name or email on the AMO side.   If they have, update them in phpbb.
    if (($row['username'] != $amouser['username']) || ($row['user_email'] != $amouser['email'])) {
       $sql =' UPDATE '.USERS_TABLE.'
             SET username="'.$db->sql_escape($amouser['username']).'"
             , user_email="'.$db->sql_escape($amouser['email']).'"
             WHERE user_id = "'.$db->sql_escape($amouser['id']).'"';
       $db->sql_query($sql);
    }


...there is one other utf8_clean_string() call in that file (not in the code above) & it should probably be removed too. It's used on $amouser['id'], which is already only a number, but leaving that one in at least won't cause this lowercase Username problem.

Comment 2

[Jorge Villalobos [:jorgev] (he/him)]

We're getting more and more complaints about this now. Possible solutions, in order of preference:

1) Fix the sync code, as in comment 1.
2) Convert the usernames to lowercase in the log in form.
3) Add a message indicating that the username has to be entered in all lowercase.

Comment 3

[Wil Clouser [:clouserw]]

0) Use browserid and not have to deal with phpbb's ridiculousness again

Comment 4

[Justin Scott [:fligtar]]

How is BrowserID going to help here? it doesn't have usernames

Comment 5

[Bugzilla]

The fix is already here. Someone give me write access & I'll do it...or someone with write access push it.

(In reply to Jorge Villalobos [:jorgev] from comment #2)
The solution should NOT be to force usernames to lowercase, that is annoying & unnecessary. Frankly, I think AMO (the non-forum part) should Login with Usernames, not the E-mail Address (or support both). E-mail addresses are for E-mail, not logging in with.

(In reply to Wil Clouser [:clouserw] from comment #3)
> 0) Use browserid and not have to deal with phpbb's ridiculousness again
...this is not "phpbb's ridiculousness"...this is the code YOU wrote. Using utf8_clean_string() may have had an unwanted side effect, but it's a QUICK fix to ALLOW non-lowercase names.

Comment 6

[Wil Clouser [:clouserw]]

(In reply to Justin Scott [:fligtar] from comment #4)
> How is BrowserID going to help here? it doesn't have usernames

It'd have to be a switch to email addresses.  Perhaps something we don't want to undertake soon.

Comment 7

[Bugzilla]

What are we waiting for?...

*** Is there ANY reason this simple patch can't be applied immediately? ***

...instead of gutting the whole system for "something better" (aka worse)...just apply the patch as-is right here. A 5 second push to the live site.

(In reply to Wil Clouser [:clouserw] from comment #6)
> It'd have to be a switch to email addresses.

Switching to logging in with E-mail addresses is entirely the wrong approach. I just said AMO should get away from it. E-mail addresses are long & hard to type...especially anti-spam ones ([e-mail]), that's why most sites have "Usernames".

I have to say BrowserID is "good" in that once you are "known" to it you don't have to re-type the Email address, which is good. BUT...I wouldn't wanna have my "Email Address" visible as my "Username" on the Forum. That would be so bad it's insane. Spam much? I use a different e-mail address on EVERY site to prevent spam. I never give multiple sites the same E-mail address, so if I start getting spam, I can see who "leaked" my E-mail address...& then block all e-mail "To" that address.

But still, E-mail addresses are for sending/receiving E-mail (& getting spam)...they SHOULD NOT be used for logging in!...why does Mozilla do this?...they like spam?...& long, hard to type E-mail addresses?

Why can't BrowserID be changed, so you "verify" your E-mail & then assign a "Username" to that E-mail? The BrowserID-enabled site never gets your real E-mail address...& if it needs one, you provide that directly to that site, without giving it to BrowserID...or even better, BrowserID would generate a unique E-mail address to pass to the site ([e-mail]) & BrowserID would be the proxy for that sites E-mail/Spam. When they start spamming, you unlink them from your BrowserID account.

(In reply to Wil Clouser [:clouserw] from comment #6)
> Perhaps something we don't want to undertake soon.

Yes, that is something we DON'T want to undertake EVER.

Someone just apply this patch right here (already written for you!). However, it's easier to read/understand with the formatting on the Forum, than it is, as displayed here, in Bugzilla.

Comment 8

[Wil Clouser [:clouserw]]

Does anyone remember where our -dev forums are?  This is r94882

Comment 9

[Wil Clouser [:clouserw]]

If the patch works, this is fixed.  Push bug is bug 659926

Comment 10

[Jorge Villalobos [:jorgev] (he/him)]

Either this wasn't fixed, or it was reverted somehow. I have just verified with a forum user that entering the username with mixed casing prevents him from logging in.

Comment 12

[Bugzilla]

(In reply to Jorge Villalobos [:jorgev] from comment #10)
> Either this wasn't fixed, or it was reverted somehow.

No, this fix works as intended...but it revealed another limitation in the code.

> I have just verified with a forum user that entering the username with
> mixed casing prevents him from logging in.

I noticed this too after the fix when in, but it didn't bother me much, I prefer to type my name with the correct case.

The problem is this line...

$_sql = 'SELECT id, email, password, username, confirmationcode FROM users WHERE username=?';

...it's SELECTing WHERE username = ?. According to a little research, this should work & it should NOT be case-sensitive (by default), but apparently, it is.

Can someone confirm that...

SELECT id, email, password, username, confirmationcode FROM users WHERE username=UsernameWithCase

...works...& that...

SELECT id, email, password, username, confirmationcode FROM users WHERE username=usernamewithcase

...will fail?...I want to confirm this line is failing & not some other line.

Even tho it should work by default, with MYSQL there is a COLLATE option/command. Can someone tell me what the COLLATE option is set to on the DB? I'm not sure if it's on the users table or the username column (or both). If COLLATE is set (to certain values) on the table/column, that might be causing the case sensitivity.

If all of the above is true, then either of these should fix it...

1. Using LOWER()...

$_sql = 'SELECT id, email, password, username, confirmationcode FROM users WHERE LOWER(username)=LOWER(?)';

...that uses LOWER(), but ONLY on the SELECT, I don't believe LOWER()'ing here will make the username really be lowercase (which was the original problem).

2. Using COLLATE (one possible syntax)...

$_sql = 'SELECT id, email, password, username, confirmationcode FROM users WHERE username COLLATE utf8_general_ci=?';

...I don't know if I used COLLATE properly...& "utf8_general_ci" is just one of the "_ci" values that might work.

3. Using COLLATE (another possible syntax)...

$_sql = 'SELECT id, email, password, username, confirmationcode FROM users WHERE username=? COLLATE utf8_general_ci';

Comment 13

[Kris Maglione [:kmag]]

We can't specify the collation in the login query, since the query won't be able to use an index. It also won't prevent conflicts between users with the same name but different capitalization. If we want mixed case usernames and case-insensitive logins, we need to change the collation function of the column.

As it happens, most of the work for this is already done in bug 858452. We already have the code to cleanup username clashes, and there's code in prod to manually protect against duplicates, so all we need to do is run the `deduplicate_users` management command on prod and add a migration to change the collation of that column.

Wil, can we get this done sometime in the next week?

Comment 14

[Wil Clouser [:clouserw]]

Logins on the forum have been disabled and we expect to have the forums turned off completely by the end of the year.  -> closing this bug.