[Bug 1963773][Harmony] login-names separate from email addresses - #147
[Bug 1963773][Harmony] login-names separate from email addresses#147topunix wants to merge 132 commits into
Conversation
|
Triaged the Copilot pass. Accepting 13 as real and pushing two through now since they block the Milestone 1 branch cut: the SHOW COLUMNS migration (MySQL only, breaks Postgres and SQLite) and the account creation form defaulting to GET (login, email and token end up in the URL). Also fixing the set_email duplicate primary insert, the regexp built from an unescaped email, the editusers INNER JOIN that duplicates and drops users, the Config::Common undef guard, the reqpw variable shadowing, the changeEmail new() call, and the otheruser vs user template regressions. One false positive: the "missing semicolon" is not a syntax error, Perl treats it as a separator and perl -c passes, but I added it for consistency. Three I want to verify against code paths outside this diff (eventdata consumers, the dev seed script, and the editusers update branch for persisting email). Will push a fixup commit. |
…tems - Auth/Verify.pm: report addr => $email instead of $login in auth_invalid_email error - user-error.html.tmpl: account_exists now checks IF login instead of IF email - Email.pm: add missing trailing semicolon after ThrowUserError in check_email_for_creation
…validation guard - user-error.html.tmpl: reword invalid_email as not registered, not invalid - token.cgi: fix Bugzilla::User::Email->new() call to use hashref (was silently dropping old_email) - Config/Common.pm: fix defined guard so unparseable input is rejected, not silently passed
- Token.pm: quotemeta($email) before building regexp to prevent metacharacter injection - userdata.html.tmpl: bounce link now uses otheruser.id instead of user.id - userdata.html.tmpl: disabledtext now defaults from otheruser instead of user
Change INNER JOIN to LEFT JOIN on profiles_emails so users without an email row still appear. Add is_primary_email = 1 to the join condition to prevent duplicate rows per user.
The admin user form posts an email field but editusers.cgi never read it, so the value was silently discarded on submit. Pass email through on account creation, and on update persist the primary address in profiles_emails: update the existing primary row if one exists, otherwise create it. Validates only when the value actually changed so unchanged resubmits do not trip email_exists. Uses Bugzilla::User::Email directly rather than User->set_email, which still blind-inserts and would violate the unique index on update.
|
Last I looked at this I found a few issues and many overlapped with the comments already above from CoPilot. I stopped as I felt there was still work going on and my review would just conflict. Do let me know when it's ready for review again or if any help is needed. |
The changed_fields loop in the account_updated message is a fixed IF/ELSIF chain. Persisting the email address in editusers.cgi adds an email key to the changes hash, which rendered as an empty list item because no branch matched. Add a branch reporting the new address.
The new module predates the 5.14.0 sweep in bug 2052697 and still declared 5.10.1, which fails t/002goodperl.t.
The throttling pattern is built with quotemeta and cannot be invalid, so bz_check_regexp only costs a round-trip query, and it validates the SQL-quoted string rather than the pattern itself. Pass nocheck.
Fixes t/005whitespace.t.
set_email always created a new profiles_emails row marked primary, so any update path violated the unique index on email or left two rows flagged primary. Update the existing primary row when one is present, create it otherwise, and return early when the address is unchanged.
issue_new_user_account_token now stores eventdata as "email:login", but request_create_account and cancel_create_account still treated it as a single value, so the confirmation page showed the raw combined string as the email address. Also stop substituting a CGI-supplied login when the token login is already taken. The confirmation form has no login field, so the substitution always yielded undef. Cancel the token and report the conflict instead. Show the login name on the confirmation page, which previously only displayed the email address.
|
All twenty Copilot items are addressed and pushed. Each fix is its own commit referencing the item, and I have replied on every thread with the commit SHA and resolved it. Three of them turned out not to need code changes: the missing semicolon in Email.pm was not a syntax error, the generate_bmo_data.pl login argument works because User::new falls back to login_name when no email row matches, and the two comments on userdata line 133 were about the same fix. Two things I noticed but left alone as out of scope for this review: the account_exists branch in cancel-token.txt.tmpl reads a variable that Token::Cancel does not set, and the email write in changeEmail happens above bz_start_transaction rather than inside it. Happy to take either if you want them in this PR. |
justdave
left a comment
justdave
left a comment
There was a problem hiding this comment.
In addition to the code-referenced review comments, something jumped out at me as I was looking this over:
A username can be set to an email-looking string that belongs to someone else’s account, because the login uniqueness check only looks at profiles.login_name via login_to_id() and does not compare against profiles_emails at all. That means the underlying check in User.pm will not reject someone else’s email unless it is also already a login name. The self-service UI in userprefs.cgi blocks @ in the login field, so regular users can’t do it there, but admin/backend paths still can. Perhaps this part can be dealt with by also blocking @ in login names on the back end (for any changes made, not existing ones grandfathered in) so that admins can't do it either. (or let an admin do it with a warning?)
Likewise, adding an email address that matches someone else’s username is allowed in the current logic, because Bugzilla::User::Email->check_email_for_creation() only checks email syntax plus email uniqueness in profiles_emails. It does not check whether that value is already used as a login name anywhere. This one is probably worth checking to make sure someone can't add an email address that's in someone else's grandfathered-in login name.
| my $old_email = $user->login; | ||
|
|
||
| my ($token, $token_ts) | ||
| = _create_token($user->id, 'emailold', $old_email . ":" . $new_email); | ||
|
|
||
| my $newtoken | ||
| = _create_token($user->id, 'emailnew', $old_email . ":" . $new_email); |
There was a problem hiding this comment.
IssueEmailChangeToken() sets $old_email from $user->login and sends the tokens to that value, and the account prefs path calls it directly when saving a new email (userprefs.cgi:166). With login names now separated from email addresses, this will mis-send confirmation mail and generate tokens that cannot be confirmed for any account whose login is not also its email.
There was a problem hiding this comment.
I think I agree: The Self-service "change my email" flow crashes for the exact users this PR is meant to support. We're storing the token's "old email" as $user->login. But Bugzilla::User::Email has NAME_FIELD => 'email' (Bugzilla/User/Email.pm), so token.cgi's Bugzilla::User::Email->new({name => $old_email}) looks up by the email column using a login value. For any user whose login differs from their email — the whole point of this feature — that lookup returns undef, and the next line ($user_email->set_email(...)) crashes on an undefined value. Bonus bug in the same function: the "confirm your current email" check in token.cgi actually compares the user's input against their login, not their email.
| elsif (exists $param->{name}) { | ||
| my $email = $param->{name}; | ||
|
|
||
| my $user_id = Bugzilla::User::Email->get_user_by_email($email); | ||
| if ($user_id) { | ||
| $param->{id} = $user_id; | ||
| delete $param->{name}; | ||
| } | ||
| } |
There was a problem hiding this comment.
Bugzilla::User->new({ name => ... }) now prefers an email-row match over the login-name match. Good idea in theory, but it changes the meaning of a very widely used constructor and makes lookups ambiguous whenever a login happens to equal some other account’s email. Existing login-based callers will silently get the wrong account in that case, so this needs either a separate email lookup path or preserved login-first semantics. Personally I think this'll be safer to be explicit about which one you're trying to look up from all callers. The idea that someone could possibly set their username to an email address that belongs to someone else bothers me, too. More on that in a general review comment.
| my $old_email = $user->login; | ||
|
|
||
| my ($token, $token_ts) | ||
| = _create_token($user->id, 'emailold', $old_email . ":" . $new_email); | ||
|
|
||
| my $newtoken | ||
| = _create_token($user->id, 'emailnew', $old_email . ":" . $new_email); |
There was a problem hiding this comment.
I think I agree: The Self-service "change my email" flow crashes for the exact users this PR is meant to support. We're storing the token's "old email" as $user->login. But Bugzilla::User::Email has NAME_FIELD => 'email' (Bugzilla/User/Email.pm), so token.cgi's Bugzilla::User::Email->new({name => $old_email}) looks up by the email column using a login value. For any user whose login differs from their email — the whole point of this feature — that lookup returns undef, and the next line ($user_email->set_email(...)) crashes on an undefined value. Bonus bug in the same function: the "confirm your current email" check in token.cgi actually compares the user's input against their login, not their email.
There was a problem hiding this comment.
Pre-PR Bugzilla/Auth/Verify.pm synced $user->set_login($username) when an external auth source (LDAP/SAML/etc.) reported a changed username for an existing account. I diffed against the pre-PR commit and confirmed this branch was deleted and replaced with email-only sync. Sites relying on external auth to keep login names current will silently stop getting that update.
| } | ||
| } | ||
|
|
||
| sub _copy_valid_emails_to_profiles_emails { |
There was a problem hiding this comment.
Not 100% sure if this is an issue or not as I'm not sure how good we've been in the past about validating emails and whether we could have invalid emails in the database already that are also valid accounts but:
Migration silently drops users with no valid email or login-as-email - if neither profiles.email nor login_name passes validate_email_syntax(), the loop does next unless defined $valid_email — that user ends up with zero rows in profiles_emails, no warning logged. This PR's own history explicitly adds support for non-email logins, meaning the users most likely to trigger this are exactly the ones the feature targets. Downstream, User::email() (User.pm:652) silently falls back to returning the login string in place of an email with no indication anything's wrong (notifications, password-reset lookups, etc. would then act on a non-email login string). Separately, individual insert failures in this same function are caught and only warn()ed — a one-time schema migration should fail loudly, not swallow errors.
|
Still reviewing here but wanted to get my initial findings up |
Good catch on both directions. Blocking @ in login names at the backend covers the first one, and the reverse case is a small addition to check_email_for_creation so an email can't collide with someone's grandfathered-in login. For the backend block, do you want a hard reject on any new or changed login containing @, or an admin override with a warning? Happy either way, just want to match what you'd prefer before I write it. Existing logins stay grandfathered in either case. |
Let's hard block it on new or changed. Just make sure it gets documented. |
New and changed login names are rejected if they contain "@". Logins created before this restriction are grandfathered in and continue to work unchanged. The check lives in the login_name validator, so it covers account creation, user preferences, admin editusers and external auth from one place. The redundant inline check in userprefs.cgi is removed. Because a login can no longer look like an email address, a new email address is now also rejected if it collides with an existing grandfathered login. Documented in the administration guide. Also aligns the email label with the login label on the account creation form and adds a matching client side pattern to the login field, so the restriction is visible before submit.
Done in db20fa2. Hard block on new or changed logins, existing ones grandfathered, rule documented in the admin guide. Also added the reverse check so a new email can't collide with a grandfathered login. Same commit adds a client side pattern on the login field and lines the two form labels up. |
|
The
That means component creation and component rename are broken on any install running the extension, so this is not just a test fixture issue. The five failing test files are all downstream of the line 172 create. Options as I see them:
Which direction do you want? I'd rather get your call on the shape before going further, since option 1 is substantially larger than the other two. |
Details
This PR is a draft of current work
Additional info