-
Notifications
You must be signed in to change notification settings - Fork 21
Allow users to change their own email and make password changing easier #616
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Currently the system sends the user's current email address to SSO applications as the subject identifier. This means that if the user's email were to change, they would appear as a new user to SSO clients. Resolves this issue by instead creating and sending an unchanging UUID as the sub. For legacy users their original registration email is used instead in order to not break compatibility with existing SSO signins.
Adds fields on the "Change my settings" page that allow users to change their email and password. (Password changes were already possible but required using the "Forgot password" flow). For password changes, an email notification is sent informing the user that their password was changed. For email changes, a confirmation email is sent to the new email to guard against accidental changes to invalid email addresses. After the change a notification is also sent to the old email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables users to change their email addresses and passwords directly from their settings page, and introduces an oidc_sub field to maintain stable SSO integration identifiers when email addresses change.
Key Changes:
- Added
oidc_subUUID field toCustomUsermodel for stable SSO subject identifiers (legacy users get their current email as the value) - Implemented email change functionality with confirmation via email link to verify ownership of the new address
- Added password change fields to user settings page with validation and notification emails
Reviewed Changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| users/models/custom_user.py | Adds oidc_sub field with UUID default for new users |
| users/migrations/0031_customuser_add_oidc_sub_field.py | Creates nullable oidc_sub field |
| users/migrations/0032_customuser_populate_oidc_sub_values.py | Populates legacy users' oidc_sub with their current email |
| users/migrations/0033_customuser_remove_oidc_sub_null.py | Makes oidc_sub non-nullable and unique |
| api/mulysaoauthvalidator.py | Updates OIDC claims to use oidc_sub instead of email for the "sub" claim |
| api/tests.py | Updates test to verify oidc_sub is used in OIDC claims |
| www/forms.py | Adds EditAccountForm for email and password changes with validation |
| www/views.py | Implements account settings view, email change confirmation handler, and notification logic |
| www/urls.py | Adds URL patterns for account settings and email confirmation |
| www/templates/www/usersettings.html | Adds Account Settings section with email and password fields |
| www/templates/www/email_change_result.html | New template for email change confirmation result page |
| utils/emailchange.py | New utility module for email change token generation and validation |
| users/templates/mail/*.txt | Email templates for password/email change notifications and confirmation |
| */locale/fi/LC_MESSAGES/django.po | Finnish translations for new UI strings (some marked as fuzzy/incomplete) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Note there is a race condition if you allow objects to be created while this migration is running. | ||
| # Objects created after the AddField and before RunPython will have their original uuid’s overwritten. |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The migration comment mentions a race condition but doesn't provide a solution. Consider adding a database transaction or advisory lock during migration to prevent this race condition, or document that migrations should be run during a maintenance window when object creation is disabled.
| # Note there is a race condition if you allow objects to be created while this migration is running. | |
| # Objects created after the AddField and before RunPython will have their original uuid’s overwritten. | |
| # IMPORTANT: There is a race condition if you allow objects to be created while this migration is running. | |
| # To prevent data corruption, you MUST run this migration during a maintenance window when object creation is disabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe? This comment is a result of copy pasting directly from Django docs. I was under the impression that migrations are run at server startup before any object creation is possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually migrations are ran before the service is started.
But if you have hundreds of workers running with "internet scale" traffic then the correct way is to first deploy a version that can handle both situations. Then the migration that can run in the background without downtime. Then a new version that has just the new logic. And all of this would happen in a way that first just some traffic, say 10% is sent to the new workers and after verifying it works then the rest of the traffic in few batches.
No we are not nowhere near that kind of requirements here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually migrations are ran before the service is started.
But if you have hundreds of workers running with "internet scale" traffic then the correct way is to first deploy a version that can handle both situations. Then the migration that can run in the background without downtime. Then a new version that has just the new logic. And all of this would happen in a way that first just some traffic, say 10% is sent to the new workers and after verifying it works then the rest of the traffic in few batches.
No we are not nowhere near that kind of requirements here :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| user = get_object_or_404(CustomUser, pk=data["uid"]) | ||
|
|
||
| # Check that the old email in the link matches, to avoid replay attacks | ||
| old_email_signed = data.get("old_email", "") | ||
| if user.email != old_email_signed: | ||
| context["error"] = _("This confirmation link has expired. Please request a new email change.") | ||
| return render(request, "www/email_change_result.html", context) | ||
|
|
||
| new_email = data.get("new_email", "").strip().lower() | ||
| if CustomUser.objects.filter(email__iexact=new_email).exclude(pk=user.pk).exists(): | ||
| context["error"] = _("The email address is already in use.") | ||
| return render(request, "www/email_change_result.html", context) | ||
|
|
||
| # All good: update and show success | ||
| old_email = user.email | ||
| user.email = new_email | ||
| user.save(update_fields=["email"]) |
Copilot
AI
Nov 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a potential race condition in the email confirmation flow. Between the time the token is validated (line 378) and the email is actually changed (line 384), another request could change the user's email, making the old_email check at line 373 fail for legitimate confirmation attempts.
Additionally, if a user requests multiple email changes in quick succession, only the last one should be valid. Consider adding a field to track pending email changes or using database-level locking:
from django.db import transaction
# Inside confirm_email_change view, wrap the update in a transaction
with transaction.atomic():
# Re-fetch the user with select_for_update to prevent race conditions
user = CustomUser.objects.select_for_update().get(pk=data["uid"])
# Check that the old email in the link matches
old_email_signed = data.get("old_email", "")
if user.email != old_email_signed:
context["error"] = _("This confirmation link has expired. Please request a new email change.")
return render(request, "www/email_change_result.html", context)
new_email = data.get("new_email", "").strip().lower()
if CustomUser.objects.filter(email__iexact=new_email).exclude(pk=user.pk).exists():
context["error"] = _("The email address is already in use.")
return render(request, "www/email_change_result.html", context)
# All good: update
old_email = user.email
user.email = new_email
user.save(update_fields=["email"])There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhhh, maybe? Will have to investigate later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second point is invalid, because if the user requests multiple email changes in quick succession, only the first one that goes through will be valid, because the email change token contains the user's previous email.
| """ | ||
| return { | ||
| "sub": request.user.email, | ||
| "sub": request.user.oidc_sub, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we add the uuid for everybody (a real uuid, not "some users have uuid some email") and a "useLegacySSOIdentifier" bit.
In the migration generate uuid for everybody and mark the old users with the "use legacy" bit.
Then have a property decorator "ssoidentifier" that returns either the uuid or the email depending on the users legacy bit.
This would be cleaner from db perspective and might allow for easier migration in the long run.
Closes #473
UX changes
Adds email and password changing fields to the "Change my settings" page under a new header, like so:
These fields allow the user to change their own email and password.
Password changes were previously possible, but had to counterintuitively be done through the "Forgot password?" link on the login page while logged out. This makes the process slightly more intuitive. A notification is sent to the user's email address when the password is changed, but there is no confirmation.
Email changes were previously not possible for technical reasons (see below). Changing the email this way sends a confirmation email to the new email to ensure that the user can actually receive messages on the email they type in. After the change a notification is also sent to the previous email address, but there's no confirmation on the previous address (I tried to strike a balance between security and annoyance).
Technical changes
Customusers use the email as the primary key. It was also sent as the subject identifier via OIDC to SSO apps like Nextcloud. Changing the primary key seems to be fine, but if the OIDC sub changes, then SSO apps will see the user as an entirely different user and their integrations will break.
I solved this by adding a new field,
oidc_sub, to customusers. This field will contain a UUID4 for all new users, but for legacy users it is set to their current email address when the migration runs in order to not break existing SSO integrations. This seemed like the easiest solution, though I admit it is a little ugly for the field to contain different data for legacy users. A more elegant migration would either involve a lot of manual admin work to migrate every SSO application, or some Keycloak magic that I wasn't sure about.Here's how this part was tested:
Tests
One existing test was updated. New tests are still TODO.