-
Notifications
You must be signed in to change notification settings - Fork 2.9k
[WEB-5430] feat: allow users to change email #8120
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
Changes from all commits
97c8901
4bf6622
13f87cd
ce6c47a
77c0454
abee7f6
15550a1
a0c18ec
4e07c9d
55cfb78
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,10 +1,20 @@ | ||||||
| # Python imports | ||||||
| import uuid | ||||||
| import json | ||||||
| import logging | ||||||
| import random | ||||||
| import string | ||||||
| import secrets | ||||||
|
|
||||||
| # Django imports | ||||||
| from django.db.models import Case, Count, IntegerField, Q, When | ||||||
| from django.contrib.auth import logout | ||||||
| from django.utils import timezone | ||||||
| from django.utils.decorators import method_decorator | ||||||
| from django.views.decorators.cache import cache_control | ||||||
| from django.views.decorators.vary import vary_on_cookie | ||||||
| from django.core.validators import validate_email | ||||||
| from django.core.cache import cache | ||||||
|
|
||||||
| # Third party imports | ||||||
| from rest_framework import status | ||||||
|
|
@@ -36,9 +46,11 @@ | |||||
| from plane.authentication.utils.host import user_ip | ||||||
| from plane.bgtasks.user_deactivation_email_task import user_deactivation_email | ||||||
| from plane.utils.host import base_host | ||||||
| from django.utils.decorators import method_decorator | ||||||
| from django.views.decorators.cache import cache_control | ||||||
| from django.views.decorators.vary import vary_on_cookie | ||||||
| from plane.bgtasks.user_email_update_task import send_email_update_magic_code, send_email_update_confirmation | ||||||
| from plane.authentication.rate_limit import EmailVerificationThrottle | ||||||
|
|
||||||
|
|
||||||
| logger = logging.getLogger("plane") | ||||||
|
|
||||||
|
|
||||||
| class UserEndpoint(BaseViewSet): | ||||||
|
|
@@ -49,6 +61,14 @@ class UserEndpoint(BaseViewSet): | |||||
| def get_object(self): | ||||||
| return self.request.user | ||||||
|
|
||||||
| def get_throttles(self): | ||||||
| """ | ||||||
| Apply rate limiting to specific endpoints. | ||||||
| """ | ||||||
| if self.action == "generate_email_verification_code": | ||||||
| return [EmailVerificationThrottle()] | ||||||
| return super().get_throttles() | ||||||
|
|
||||||
| @method_decorator(cache_control(private=True, max_age=12)) | ||||||
| @method_decorator(vary_on_cookie) | ||||||
| def retrieve(self, request): | ||||||
|
|
@@ -69,6 +89,169 @@ def retrieve_instance_admin(self, request): | |||||
| def partial_update(self, request, *args, **kwargs): | ||||||
| return super().partial_update(request, *args, **kwargs) | ||||||
|
|
||||||
| def _validate_new_email(self, user, new_email): | ||||||
| """ | ||||||
| Validate the new email address. | ||||||
|
|
||||||
| Args: | ||||||
| user: The User instance | ||||||
| new_email: The new email address to validate | ||||||
|
|
||||||
| Returns: | ||||||
| Response object with error if validation fails, None if validation passes | ||||||
| """ | ||||||
| if not new_email: | ||||||
| return Response( | ||||||
| {"error": "Email is required"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| # Validate email format | ||||||
| try: | ||||||
| validate_email(new_email) | ||||||
| except Exception: | ||||||
| return Response( | ||||||
| {"error": "Invalid email format"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| # Check if email is the same as current email | ||||||
| if new_email == user.email: | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Email case sensitivity causes validation inconsistencyThe comparison |
||||||
| return Response( | ||||||
| {"error": "New email must be different from current email"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| # Check if email already exists in the User model | ||||||
| if User.objects.filter(email=new_email).exclude(id=user.id).exists(): | ||||||
| return Response( | ||||||
| {"error": "An account with this email already exists"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| return None | ||||||
|
|
||||||
| def generate_email_verification_code(self, request): | ||||||
NarayanBavisetti marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| """ | ||||||
| Generate and send a magic code to the new email address for verification. | ||||||
| Rate limited to 3 requests per hour per user (enforced by EmailVerificationThrottle). | ||||||
| Additional per-email cooldown of 60 seconds prevents rapid repeated requests. | ||||||
| """ | ||||||
| user = self.get_object() | ||||||
| new_email = request.data.get("email", "").strip().lower() | ||||||
|
|
||||||
| # Validate the new email | ||||||
| validation_error = self._validate_new_email(user, new_email) | ||||||
| if validation_error: | ||||||
| return validation_error | ||||||
|
|
||||||
| try: | ||||||
| # Generate magic code for email verification | ||||||
| # Use a special key prefix to distinguish from regular magic signin | ||||||
| # Include user ID to bind the code to the specific user | ||||||
| cache_key = f"magic_email_update_{user.id}_{new_email}" | ||||||
| ## Generate a random token | ||||||
| token = ( | ||||||
| "".join(secrets.choice(string.ascii_lowercase) for _ in range(4)) | ||||||
| + "-" | ||||||
| + "".join(secrets.choice(string.ascii_lowercase) for _ in range(4)) | ||||||
| + "-" | ||||||
| + "".join(secrets.choice(string.ascii_lowercase) for _ in range(4)) | ||||||
| ) | ||||||
| # Store in cache with 10 minute expiration | ||||||
| cache_data = json.dumps({"token": token}) | ||||||
| cache.set(cache_key, cache_data, timeout=600) | ||||||
|
|
||||||
| # Send magic code to the new email | ||||||
| send_email_update_magic_code.delay(new_email, token) | ||||||
|
|
||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: Cache entry persists after email sending failureIf |
||||||
| return Response( | ||||||
| {"message": "Verification code sent to email"}, | ||||||
| status=status.HTTP_200_OK, | ||||||
| ) | ||||||
| except Exception as e: | ||||||
| logger.error("Failed to generate verification code: %s", str(e), exc_info=True) | ||||||
| return Response( | ||||||
| {"error": "Failed to generate verification code. Please try again."}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| def update_email(self, request): | ||||||
| """ | ||||||
| Verify the magic code and update the user's email address. | ||||||
| This endpoint verifies the code and updates the existing user record | ||||||
| without creating a new user, ensuring the user ID remains unchanged. | ||||||
| """ | ||||||
| user = self.get_object() | ||||||
| new_email = request.data.get("email", "").strip().lower() | ||||||
| code = request.data.get("code", "").strip() | ||||||
|
|
||||||
| # Validate the new email | ||||||
| validation_error = self._validate_new_email(user, new_email) | ||||||
| if validation_error: | ||||||
| return validation_error | ||||||
|
|
||||||
| if not code: | ||||||
| return Response( | ||||||
| {"error": "Verification code is required"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| # Verify the magic code | ||||||
| try: | ||||||
| cache_key = f"magic_email_update_{user.id}_{new_email}" | ||||||
| cached_data = cache.get(cache_key) | ||||||
|
|
||||||
| if not cached_data: | ||||||
| logger.warning("Cache key not found: %s. Code may have expired or was never generated.", cache_key) | ||||||
| return Response( | ||||||
| {"error": "Verification code has expired or is invalid"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| data = json.loads(cached_data) | ||||||
| stored_token = data.get("token") | ||||||
|
|
||||||
| if str(stored_token) != str(code): | ||||||
| return Response( | ||||||
| {"error": "Invalid verification code"}, | ||||||
| status=status.HTTP_400_BAD_REQUEST, | ||||||
| ) | ||||||
|
|
||||||
| except Exception as e: | ||||||
| return Response( | ||||||
| {"error": "Failed to verify code. Please try again."}, | ||||||
|
||||||
| {"error": "Failed to verify code. Please try again."}, | |
| {"error": "A technical error occurred during code verification. Please try again later or contact support if the problem persists."}, |
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.
Bug: Exception swallows error details during code verification
The exception handler catches all exceptions during code verification but doesn't log the actual error or provide specific feedback. This makes debugging difficult when legitimate errors occur, such as JSON parsing failures or cache connection issues. The caught exception e is never logged or used.
NarayanBavisetti marked this conversation as resolved.
Show resolved
Hide resolved
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.
Bug: Race condition in email update causes database error
A race condition exists between checking email availability and saving the user. After the final User.objects.filter(email=new_email).exclude(id=user.id).exists() check at line 228, another user could claim the same email before user.save() executes at line 238. This creates a window where two concurrent requests could pass the validation check but one would fail with a database integrity error when attempting to save a duplicate email.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,110 @@ | ||
| # Python imports | ||
| import logging | ||
|
|
||
| # Third party imports | ||
| from celery import shared_task | ||
|
|
||
| # Django imports | ||
| from django.core.mail import EmailMultiAlternatives, get_connection | ||
| from django.template.loader import render_to_string | ||
| from django.utils.html import strip_tags | ||
|
|
||
| # Module imports | ||
| from plane.license.utils.instance_value import get_email_configuration | ||
| from plane.utils.exception_logger import log_exception | ||
|
|
||
|
|
||
| @shared_task | ||
| def send_email_update_magic_code(email, token): | ||
| try: | ||
| ( | ||
| EMAIL_HOST, | ||
| EMAIL_HOST_USER, | ||
| EMAIL_HOST_PASSWORD, | ||
| EMAIL_PORT, | ||
| EMAIL_USE_TLS, | ||
| EMAIL_USE_SSL, | ||
| EMAIL_FROM, | ||
| ) = get_email_configuration() | ||
|
|
||
| # Send the mail | ||
| subject = "Verify your new email address" | ||
| context = {"code": token, "email": email} | ||
|
|
||
| html_content = render_to_string("emails/auth/magic_signin.html", context) | ||
| text_content = strip_tags(html_content) | ||
|
|
||
| connection = get_connection( | ||
| host=EMAIL_HOST, | ||
| port=int(EMAIL_PORT), | ||
| username=EMAIL_HOST_USER, | ||
| password=EMAIL_HOST_PASSWORD, | ||
| use_tls=EMAIL_USE_TLS == "1", | ||
| use_ssl=EMAIL_USE_SSL == "1", | ||
| ) | ||
|
|
||
| msg = EmailMultiAlternatives( | ||
| subject=subject, | ||
| body=text_content, | ||
| from_email=EMAIL_FROM, | ||
| to=[email], | ||
| connection=connection, | ||
| ) | ||
| msg.attach_alternative(html_content, "text/html") | ||
| msg.send() | ||
| logging.getLogger("plane.worker").info("Email sent successfully.") | ||
| return | ||
| except Exception as e: | ||
| log_exception(e) | ||
| return | ||
|
|
||
|
|
||
| @shared_task | ||
| def send_email_update_confirmation(email): | ||
| """ | ||
| Send a confirmation email to the user after their email address has been successfully updated. | ||
|
|
||
| Args: | ||
| email: The new email address that was successfully updated | ||
| """ | ||
| try: | ||
| ( | ||
| EMAIL_HOST, | ||
| EMAIL_HOST_USER, | ||
| EMAIL_HOST_PASSWORD, | ||
| EMAIL_PORT, | ||
| EMAIL_USE_TLS, | ||
| EMAIL_USE_SSL, | ||
| EMAIL_FROM, | ||
| ) = get_email_configuration() | ||
|
|
||
| # Send the confirmation email | ||
| subject = "Plane email address successfully updated" | ||
| context = {"email": email} | ||
|
|
||
| html_content = render_to_string("emails/user/email_updated.html", context) | ||
| text_content = strip_tags(html_content) | ||
|
|
||
| connection = get_connection( | ||
| host=EMAIL_HOST, | ||
| port=int(EMAIL_PORT), | ||
| username=EMAIL_HOST_USER, | ||
| password=EMAIL_HOST_PASSWORD, | ||
| use_tls=EMAIL_USE_TLS == "1", | ||
| use_ssl=EMAIL_USE_SSL == "1", | ||
| ) | ||
|
|
||
| msg = EmailMultiAlternatives( | ||
| subject=subject, | ||
| body=text_content, | ||
| from_email=EMAIL_FROM, | ||
| to=[email], | ||
| connection=connection, | ||
| ) | ||
| msg.attach_alternative(html_content, "text/html") | ||
| msg.send() | ||
| logging.getLogger("plane.worker").info(f"Email update confirmation sent successfully to {email}.") | ||
| return | ||
| except Exception as e: | ||
| log_exception(e) | ||
| return |
Uh oh!
There was an error while loading. Please reload this page.