Allow users to change their own email address #8671
Conversation
📝 WalkthroughWalkthroughThis change enables users to change their own email addresses through a new UI and backend flow, requiring password verification for the action. It removes the ability for admins to change user emails, updates the database schema to track email change timestamps, and adjusts email verification logic and configuration accordingly. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested labels
Poem
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 (
|
| activated = false | ||
| required = false | ||
| activated = true | ||
| required = true | ||
| gracePeriod = 7 days # time period in which users do not need to verify their email address |
There was a problem hiding this comment.
for testing purposes. REMOVE BEFORE MERGING
…accurate email verification grace period
…knossos into users-can-change-email
There was a problem hiding this comment.
The password verification should now be working. I did not add an explicit route for this and instead made it part of the update route. In case the current user wants to update their email, their password is needed in the request. If an admin changes the email of another user, the password should not be required in the update route.
Moreover, I noticed that the email verification resending is triggered even when the email update call returns an error due to e.g. wrong password. Could you please give this a try @knollengewaechs?
|
Oh and I also slightly changed the frontend to accommodate the newest backend changes to make sure the password entered by the user is sent as part of the update request so that it gets verified properly |
|
@MichaelBuessemeyer thanks for including the password check and adjusting the frontend!
From what I can tell, this shouldnt be possible any more? see slack |
|
I would have done the necessary frontend changes myself of course :) but I think its working now, so apart from the question before, this PR should be ready for review, right? |
Whoopsie, thanks for telling me. I'll remove this option then :)
Yeah sorry for fiddling with it but I needed this to test (and fix 🙈) the backend code. The inital version wasn't working 😅 .
Yeah sounds like it 🚀 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
conf/application.conf (1)
86-88: Verify if enabling email verification by default is intentional.The configuration now enables and requires email verification by default. However, there's a past review comment from knollengewaechs stating these were "for testing purposes. REMOVE BEFORE MERGING."
Please confirm whether enabling email verification by default in production is the intended behavior for this PR, or if these configuration changes should be reverted before merging.
🧹 Nitpick comments (4)
frontend/javascripts/admin/auth/change_email_view.tsx (2)
48-48: Fix parameter type annotation.The parameter
otherEmailFieldKeyis typed asstring[]but the function expects and uses it as a single string value.-function checkEmailsAreMatching(value: string, otherEmailFieldKey: string[]) { +function checkEmailsAreMatching(value: string, otherEmailFieldKey: string) {
176-176: Remove unnecessary router wrapper.The component doesn't appear to use any router props, making the
withRouterwrapper unnecessary.-export default withRouter<RouteComponentProps, any>(ChangeEmailView); +export default ChangeEmailView;Also remove the unused import:
-import { type RouteComponentProps, withRouter } from "react-router-dom";app/controllers/UserController.scala (2)
275-275: Address the TODO comment about guest users.The comment questions whether the super user check should remain given the introduction of guest users. This needs clarification or resolution.
Should I help analyze the impact of guest users on this permission check or create an issue to track this decision?
309-309: Fix comment formatting.The comment has inconsistent formatting.
Apply this diff to fix the comment:
- // properties for team managers dataset manager and admins only: experiences, teams + // Properties for team managers, dataset managers and admins only: experiences, teams
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
MIGRATIONS.unreleased.md(1 hunks)app/controllers/UserController.scala(7 hunks)app/models/user/EmailVerificationService.scala(1 hunks)app/models/user/MultiUser.scala(3 hunks)conf/application.conf(1 hunks)conf/evolutions/135-extra-column-for-email-changed.sql(1 hunks)conf/evolutions/reversions/135-extra-column-for-email-changed.sql(1 hunks)frontend/javascripts/admin/auth/change_email_view.tsx(1 hunks)frontend/javascripts/admin/user/user_list_view.tsx(1 hunks)frontend/javascripts/navbar.tsx(1 hunks)frontend/javascripts/router.tsx(2 hunks)tools/postgres/schema.sql(2 hunks)
🔇 Additional comments (25)
tools/postgres/schema.sql (2)
24-24: LGTM: Schema version increment is correct.The schema version increment from 134 to 135 aligns with the new migration script.
462-462: LGTM: New emailChangeDate column is properly defined.The column definition follows existing patterns with appropriate type, nullability, and default value.
conf/evolutions/135-extra-column-for-email-changed.sql (1)
1-24: LGTM: Well-structured migration script.The migration follows best practices:
- Proper transaction handling for atomicity
- Schema version validation before applying changes
- Correct handling of dependent views
- Sensible initialization of the new column with existing
createdtimestampsMIGRATIONS.unreleased.md (1)
13-13: LGTM: Documentation updated correctly.The new migration file is properly documented in the unreleased migrations list.
frontend/javascripts/navbar.tsx (1)
680-680: LGTM: New email change menu item is well-positioned.The "Change Email" menu item follows existing patterns and is logically placed next to the "Change Password" option.
app/models/user/EmailVerificationService.scala (1)
81-81: LGTM! Logical improvement to grace period calculation.Using
emailChangeDateinstead ofcreatedfor the grace period calculation makes perfect sense - users should get a grace period from when they last changed their email, not from when their account was created.app/models/user/MultiUser.scala (3)
29-29: LGTM! Proper addition of email change tracking.The new
emailChangeDatefield with a sensible default value properly supports tracking when emails are changed.
61-61: LGTM! Correct database field parsing.The parsing correctly reads the
emailchangedatefield from the database row.
96-97: LGTM! Proper email update implementation.Setting both
isEmailVerified = falseandemailChangeDate = NOW()when updating email ensures proper tracking and forces re-verification of the new email address.frontend/javascripts/router.tsx (2)
3-3: LGTM! Correct import addition.Import for the new ChangeEmailView component is properly added.
648-652: LGTM! Properly secured route for email changes.The new route is correctly configured with authentication requirements and follows the established pattern for auth-related routes.
frontend/javascripts/admin/user/user_list_view.tsx (1)
20-20: LGTM! Proper cleanup of unused imports.Removing unused imports after eliminating inline email editing functionality keeps the code clean and reduces bundle size.
frontend/javascripts/admin/auth/change_email_view.tsx (2)
22-28: LGTM! Secure email change implementation.The
changeEmailfunction properly includes password verification by adding it to the user object, ensuring backend validation of the user's identity.
72-79: LGTM! Excellent user experience design.The info alert clearly communicates the logout consequence, setting proper user expectations before they proceed with the email change.
conf/evolutions/reversions/135-extra-column-for-email-changed.sql (4)
1-3: LGTM - Proper transaction setup and version assertion.The transaction boundary and schema version assertion provide good safety measures for the reversion process.
5-8: LGTM - Correct dependency management and column removal.The script properly drops dependent views before removing the column, which prevents constraint violations. The
emailChangeDatecolumn removal is handled correctly.
10-19: LGTM - View recreation matches expected schema.The recreated views properly exclude the
emailChangeDatecolumn and maintain the original filtering and join logic for the pre-135 schema state.
21-23: LGTM - Proper schema version update and transaction commit.The schema version is correctly updated to 134 and the transaction is properly committed.
app/controllers/UserController.scala (7)
18-21: LGTM - Appropriate imports for authentication functionality.The new imports support the password verification feature for email changes.
29-29: LGTM - Dependency injection for credentials provider.The
CredentialsProviderdependency is correctly injected for password verification.
| .then(async () => { | ||
| handleResendVerificationEmail(); | ||
| Toast.success("Email address changed successfully. You will be logged out."); | ||
| await Request.receiveJSON("/api/auth/logout"); | ||
| window.location.href = "/auth/login"; | ||
| Store.dispatch(logoutUserAction()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix potential race condition between redirect and logout action.
There's a potential race condition where window.location.href = "/auth/login" initiates navigation before Store.dispatch(logoutUserAction()) can complete.
Move the logout dispatch before the redirect to ensure proper state cleanup:
Toast.success("Email address changed successfully. You will be logged out.");
await Request.receiveJSON("/api/auth/logout");
+Store.dispatch(logoutUserAction());
window.location.href = "/auth/login";
-Store.dispatch(logoutUserAction());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .then(async () => { | |
| handleResendVerificationEmail(); | |
| Toast.success("Email address changed successfully. You will be logged out."); | |
| await Request.receiveJSON("/api/auth/logout"); | |
| window.location.href = "/auth/login"; | |
| Store.dispatch(logoutUserAction()); | |
| .then(async () => { | |
| handleResendVerificationEmail(); | |
| Toast.success("Email address changed successfully. You will be logged out."); | |
| await Request.receiveJSON("/api/auth/logout"); | |
| Store.dispatch(logoutUserAction()); | |
| window.location.href = "/auth/login"; |
🤖 Prompt for AI Agents
In frontend/javascripts/admin/auth/change_email_view.tsx around lines 34 to 39,
the logout action is dispatched after setting window.location.href, causing a
race condition where navigation may occur before the logout completes. To fix
this, move Store.dispatch(logoutUserAction()) to execute before setting
window.location.href to "/auth/login" to ensure the logout state is properly
updated before redirecting.
|
Sorry, backend CI doesn't run due to some download server issues / outage |
|
@knollengewaechs I requested @daniel-wer for the frontend review. Hope thats ok :) |
…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
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/8671.md (1)
5-5: Fix grammatical error: missing apostrophe.The phrase "a users email address" should be "a user's email address" to indicate the possessive form.
-- A administator of the organization can no longer update a users email address. This has to be done by the user themselves. +- An administrator of the organization can no longer update a user's email address. This has to be done by the user themselves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/db/multiusers.csvis excluded by!**/*.csv
📒 Files selected for processing (5)
app/controllers/UserController.scala(6 hunks)app/models/user/UserService.scala(1 hunks)conf/application.conf(1 hunks)conf/messages(1 hunks)unreleased_changes/8671.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- conf/messages
🚧 Files skipped from review as they are similar to previous changes (2)
- conf/application.conf
- app/controllers/UserController.scala
🧰 Additional context used
🪛 LanguageTool
unreleased_changes/8671.md
[uncategorized] ~5-~5: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...the organization can no longer update a users email address. This has to be done by t...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
🔇 Additional comments (1)
app/models/user/UserService.scala (1)
201-204: LGTM! Good addition of audit logging for email changes.The for-comprehension properly sequences the email update with logging, providing valuable audit trail for email changes. The implementation correctly maintains existing error handling while adding transparency.
There was a problem hiding this comment.
Frontend code lgtm 👍
Notes from testing:
- When entering the wrong password, the user is still logged out without any indication that the email update did not succeed. It would be great if the user is only logged out if the email update succeeded and sees an error that the password is wrong otherwise.
- It feels a bit weird to be logged out and then after a second of waiting have the page reload. Do you also observe this or is it specific to my setup? Could we avoid reloading the page or is that tied to being logged out?
Co-authored-by: Daniel <daniel.werner@scalableminds.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unreleased_changes/8671.md (1)
2-6: Tighten wording & fix missing possessive apostropheMinor copy-editing to improve clarity and correct grammar.
- A user can now update the email address of their account by themselves. + Users can now update their own email addresses. - An administrator of the organization can no longer update a users email address. This has to be done by the user themselves. + Organization administrators can no longer update a user's email address; users must make this change themselves.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/models/user/UserService.scala(1 hunks)unreleased_changes/8671.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/models/user/UserService.scala
🧰 Additional context used
🪛 LanguageTool
unreleased_changes/8671.md
[uncategorized] ~5-~5: It looks like there might be a possessive apostrophe missing. Consider inserting it.
Context: ...the organization can no longer update a users email address. This has to be done by t...
(AI_EN_LECTOR_MISSING_NOUN_POSSESSIVE)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-smoketest-push
- GitHub Check: frontend-tests
- GitHub Check: backend-tests
This PR adds a new "Account Settings" page to host various other sub-pages for: - changing email (soon) - changing password - appearance/theme - passkeys (future) - API Auth Token <img width="1269" alt="Screenshot 2025-06-25 at 10 45 45" src="https://github.com/user-attachments/assets/848cc79b-b636-4de1-b495-e15059e7d452" /> <img width="1268" alt="Screenshot 2025-06-25 at 10 45 54" src="https://github.com/user-attachments/assets/fcfb3c08-8a58-450e-b50d-96bd58ed9f1a" /> <img width="1269" alt="Screenshot 2025-06-25 at 10 46 01" src="https://github.com/user-attachments/assets/4a7da90e-a2ed-4a60-ac9b-ebff65d8e140" /> ### URL of deployed dev instance (used for testing): - https://accountsettingspage.webknossos.xyz/ ### Steps to test: - Go to new Account settings page - Change PW - Try dark/light mode ### Issues: - Blocked by #8671 - Blocked by #8679 - fixes #5408 ------ (Please delete unneeded items, merge only when none are left open) - [x] Updated [changelog](../blob/master/CHANGELOG.unreleased.md#unreleased) - [ ] Updated [migration guide](../blob/master/MIGRATIONS.unreleased.md#unreleased) if applicable - [ ] Updated [documentation](../blob/master/docs) if applicable - [ ] Adapted [wk-libs python client](https://github.com/scalableminds/webknossos-libs/tree/master/webknossos/webknossos/client) if relevant API parts change - [ ] Removed dev-only changes like prints and application.conf edits - [x] Considered [common edge cases](../blob/master/.github/common_edge_cases.md) - [ ] Needs datastore update after deployment --------- Co-authored-by: Philipp Otto <philippotto@users.noreply.github.com> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> Co-authored-by: Philipp Otto <philipp.4096@gmail.com>
URL of deployed dev instance (used for testing):
more context in slack:
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)