[Bug 1963773][Harmony] login-names separate from email addresses - #147
[Bug 1963773][Harmony] login-names separate from email addresses#147topunix wants to merge 128 commits into
Conversation
|
Or I didn't get to the spot where it was added again yet, which is more likely for a lot of the major features. If it was in the mainline commit log after BMO branched I considered it removed until I found where BMO/Harmony had a matching commit, and I didn't get through it all yet. |
So, I noticed the merge... would you say you are on track for a release in 2025? Many people are waiting holding their fingers crossed :-) |
|
We should have 5.9.2 in the next week or two, with this in it if I don't find any problems with it. As this was the main remaining blocker to releasing Bugzilla 6, a release candidate should follow not long after. |
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.
| # Be careful! Some logins may contain ":" in them. | ||
| my ($email, $login) = split(':', $data, 2); | ||
| $login = $cgi->param('login') if login_to_id($login); | ||
|
|
| sub set_email { | ||
| my ($self, $email) = @_; | ||
| # Create a user email account | ||
| my $email_data = { | ||
| user_id => $self->id, | ||
| email => $email, | ||
| is_primary_email => 1 | ||
| }; | ||
|
|
||
| my $user_email = Bugzilla::User::Email->create($email_data); | ||
| } |
|
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.
Details
This PR is a draft of current work
Additional info