Allow users to change their email #8781
Conversation
…accurate email verification grace period
…knossos into users-can-change-email
…knossos into users-can-change-email
…knossos into users-can-change-email
user e2e test csv data
…rs-can-change-email - plus add changelog entry
…knossos into users-can-change-email
Co-authored-by: Daniel <daniel.werner@scalableminds.com>
📝 WalkthroughWalkthroughThis change enables users to update their own email addresses by introducing a new self-service UI and backend logic, while removing the ability for administrators to change user emails. It includes password verification for email changes, updates the database schema to track email change timestamps, and enforces email verification requirements. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)frontend/javascripts/navbar.tsx (1)
🔇 Additional comments (2)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
review from @daniel-wer "Notes from testing:
|
|
backend review see #8671 (review) |
|
@daniel-wer thank you for the feedback!
@MichaelBuessemeyer I think this is because the request does not fail if admins enter the wrong password. The bahaviour is as expected for non-admins users. Could this be a remnant of the old implementation that admins dont have to provide their password? I understand that the email update is part of the user update route, and I understand that admins arent supposed to need their PW every time. But when changing the email, I thought this would be useful to prevent abuse? I thought of two solutions (there are probably many more):
Let me know what you think! :) |
|
IMO
should be the way to go. Admins should also be only able to change their email address, in case they provide their password correctly. It should be the same as for normal users.
That sounds very fishy. Thanks for investigating. IMO it should be failing in that case!. Likely me that messed up here 🙃. I'll give it a check once I have time for it in the next days :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/controllers/UserController.scala(6 hunks)conf/evolutions/136-extra-column-for-email-changed.sql(1 hunks)conf/evolutions/reversions/136-extra-column-for-email-changed.sql(1 hunks)tools/postgres/schema.sql(2 hunks)unreleased_changes/8671.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- unreleased_changes/8671.md
- tools/postgres/schema.sql
- app/controllers/UserController.scala
🔇 Additional comments (2)
conf/evolutions/reversions/136-extra-column-for-email-changed.sql (2)
5-7: Mirror theCASCADEchange in the down migration
If you adoptCASCADEin the forward script you should do the same here to guarantee the rollback path always works, even when additional objects reference these views.-DROP VIEW IF EXISTS webknossos.userInfos; -DROP VIEW IF EXISTS webknossos.multiUsers_; +DROP VIEW IF EXISTS webknossos.userInfos CASCADE; +DROP VIEW IF EXISTS webknossos.multiUsers_ CASCADE;
10-20: Symmetry check – confirm the recreated views match version 135 exactly
After removingemailChangeDate, be sure the column order and aliases ofuserInfosmatch the original v135 definition; otherwise clients relying onSELECT *will break during rollback. Please compare against the historical definition or run a diff in CI to prevent subtle regressions.
| DROP VIEW IF EXISTS webknossos.userInfos; | ||
| DROP VIEW IF EXISTS webknossos.multiUsers_; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using CASCADE when dropping views to avoid dependency errors
If other objects depend on userInfos or multiUsers_, the DROP VIEW will fail and abort the whole migration. Adding CASCADE is safer in complex schemas.
-DROP VIEW IF EXISTS webknossos.userInfos;
-DROP VIEW IF EXISTS webknossos.multiUsers_;
+DROP VIEW IF EXISTS webknossos.userInfos CASCADE;
+DROP VIEW IF EXISTS webknossos.multiUsers_ CASCADE;🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 5 to 7,
the DROP VIEW statements for userInfos and multiUsers_ do not use CASCADE, which
can cause failures if other objects depend on these views. Modify the DROP VIEW
statements to include CASCADE (e.g., DROP VIEW IF EXISTS webknossos.userInfos
CASCADE;) to ensure dependent objects are also dropped and avoid migration
aborts.
| ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ NOT NULL DEFAULT NOW(); | ||
| UPDATE webknossos.multiUsers SET emailChangeDate = created; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heavy rewrite risk – split the NOT NULL DEFAULT change into two steps
ADD COLUMN … NOT NULL DEFAULT NOW() forces a full-table rewrite because NOW() is volatile. On large multiUsers tables this can lock the table for a long time.
A less disruptive pattern:
-ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ NOT NULL DEFAULT NOW();
+-- 1. add nullable column without default (no rewrite)
+ALTER TABLE webknossos.multiUsers ADD COLUMN emailChangeDate TIMESTAMPTZ;
+
+-- 2. back-fill existing rows
+UPDATE webknossos.multiUsers SET emailChangeDate = created;
+
+-- 3. make it NOT NULL and set the default for new rows
+ALTER TABLE webknossos.multiUsers
+ ALTER COLUMN emailChangeDate SET NOT NULL,
+ ALTER COLUMN emailChangeDate SET DEFAULT NOW();This avoids the rewrite while achieving the same outcome.
🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 8 to 10,
avoid adding the new column with NOT NULL DEFAULT NOW() in one step because it
causes a full-table rewrite and long locks. Instead, first add the column as
nullable without a default, then update all rows to set the column value to
created, and finally alter the column to set NOT NULL and add the default NOW().
This staged approach prevents heavy table locking on large tables.
| CREATE VIEW webknossos.multiUsers_ AS SELECT * FROM webknossos.multiUsers WHERE NOT isDeleted; | ||
| CREATE VIEW webknossos.userInfos AS | ||
| SELECT | ||
| u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name, | ||
| u.isDeactivated, u.isDatasetManager, u.isAdmin, m.isSuperUser, | ||
| u._organization, o._id AS organization_id, u.created AS user_created, | ||
| m.created AS multiuser_created, u._multiUser, m._lastLoggedInIdentity, u.lastActivity, m.isEmailVerified | ||
| FROM webknossos.users_ u | ||
| JOIN webknossos.organizations_ o ON u._organization = o._id | ||
| JOIN webknossos.multiUsers_ m on u._multiUser = m._id; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing emailChangeDate in userInfos view
The purpose of adding the column is to expose it for downstream logic (e.g. grace-period checks). The recreated userInfos view still omits it, forcing every consumer to query multiUsers directly.
Add the column so callers get it “for free”:
-SELECT
-u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
+SELECT
+u._id AS _user, m.email, u.firstName, u.lastname, o.name AS organization_name,
+m.emailChangeDate,Update all select lists and keep the column order consistent with the forward-migration philosophy that views reflect relevant business data.
🤖 Prompt for AI Agents
In conf/evolutions/136-extra-column-for-email-changed.sql around lines 11 to 20,
the userInfos view definition is missing the emailChangeDate column from the
multiUsers table. To fix this, add the emailChangeDate column to the SELECT list
in the userInfos view, placing it in a logical position consistent with the
existing column order to ensure downstream consumers can access it without
querying multiUsers directly.
I only experience this when activating throttling or paying very close attention to my browser, but nonetheless, it would be nice to omit this! From what I can tell, this behaviour is tied to being logged out. The user needs to be logged out and then navigated to the login page, otherwise you would see the login view with another route in the browsers address bar (at least thats my explanation). Thats why the last step of the log-out procedure is to set the location to |
URL of deployed dev instance (used for testing):
Newer version of #8671!
see https://scm.slack.com/archives/C5AKLAV0B/p1752504188608299
more context in slack:
Steps to test:
http://localhost:9000/auth/changeEmailTODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)