[Bug 1963773][Harmony] login-names separate from email addresses - #147
[Bug 1963773][Harmony] login-names separate from email addresses#147topunix wants to merge 131 commits into
Conversation
So, 2025 is ending, and I have not noticed a 5.9.2 tag. But - I have noticed the merge of this PR. Could you update your time estimates from September? |
It is impossible to make time estimates with only volunteers available as everything depends on when people can make time for Bugzilla outside of their primary income work. This is why the Project is trying to raise money to hire someone. That said, there is now less work remaining to do than there was in the last statement above, but work still remains, and I'd still like to get 5.9.2 out in the next week or two. But that depends on having time available to do it. Which hopefully will happen, but I'm not in a position to make any guarantees. |
|
Just to warn those watching this PR, I'm going to kick off a code review by GitHub CoPilot. It's not meant to replace the human review, and I can pretty much guarantee it'll make suggestions that I'm going to override and say no to (based on experience). But it will also find things that I won't, and with a patch this large it can be more thorough on some things than I can, and it's an extra pair of (virtual) eyeballs that we're lacking from volunteers at the moment. |
There was a problem hiding this comment.
Pull request overview
This PR is a draft implementing Bug 1963773 to decouple user login names from email addresses by introducing a dedicated profiles_emails table and updating account creation, login, password reset, admin user management, and related templates/docs.
Changes:
- Adds
profiles_emailsschema + newBugzilla::User::Emailobject and migrates existing emails. - Updates account creation/email-change/password-reset flows to use separate login + email values.
- Updates admin/user-facing templates and tooling (admin UI, install answers, docs) to expose and handle the new email field.
Reviewed changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| userprefs.cgi | Adds separate new_login_name vs new_email handling for account prefs. |
| token.cgi | Updates password reset + email change + account confirmation token flows. |
| template/en/default/global/user-error.html.tmpl | Adds/adjusts error messages for new login/email behaviors. |
| template/en/default/global/messages.html.tmpl | Adds new install prompt message tag + email-changed message. |
| template/en/default/admin/users/userdata.html.tmpl | Adds email field to admin user edit UI and adjusts other fields. |
| template/en/default/admin/users/search.html.tmpl | Adds “email address” to admin user search criteria. |
| template/en/default/admin/users/list.html.tmpl | Adds “Email address” column to admin user list. |
| template/en/default/account/prefs/account.html.tmpl | Adds login-name editing + updates email-change field name/value. |
| template/en/default/account/created.html.tmpl | Updates account-created page to display both login and email. |
| template/en/default/account/create.html.tmpl | Replaces create-account UI to collect login + email. |
| template/en/default/account/cancel-token.txt.tmpl | Adjusts cancel-token email output to show login. |
| template/en/default/account/auth/login.html.tmpl | Updates login form to accept “email or login” with pattern. |
| template/en/default/account/auth/login-small.html.tmpl | Updates mini-login field to accept “email or login”. |
| scripts/entrypoint.pl | Adjusts dev startup admin credentials display/seed behavior. |
| qa/config/checksetup_answers.txt | Adds ADMIN_LOGIN_NAME for QA install answers. |
| extensions/BMO/Extension.pm | Hardens audit log output when remote IP is missing. |
| editusers.cgi | Joins profiles_emails for listing/searching users by email. |
| docs/en/rst/administering/users.rst | Updates admin docs to distinguish login name vs email address. |
| createaccount.cgi | Passes login + email through confirmation flow and template vars. |
| conf/checksetup_answers.txt | Adds ADMIN_LOGIN_NAME for local install answers. |
| Bugzilla/User/Email.pm | New object model for profiles_emails with validators/helpers. |
| Bugzilla/User.pm | Routes email via profiles_emails, updates creation + confirmation logic. |
| Bugzilla/Token.pm | Stores account tokens with email:login eventdata and throttling changes. |
| Bugzilla/Install/DB.pm | Adds migration helper to populate profiles_emails. |
| Bugzilla/Install.pm | Prompts for admin login name + email separately during install. |
| Bugzilla/DB/Schema.pm | Adds profiles_emails table schema and indexes. |
| Bugzilla/Constants.pm | Introduces ACCOUNT_CHANGE_INTERVAL. |
| Bugzilla/Config/Common.pm | Adjusts email param validation behavior. |
| Bugzilla/Auth/Verify.pm | Updates external auth user creation/update to accept separate email. |
| Bugzilla/Auth.pm | Adds can_change_login capability method/docs. |
| .github/checksetup_answers.txt | Adds ADMIN_LOGIN_NAME for GitHub workflows. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
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.
| 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.
Details
This PR is a draft of current work
Additional info