- Updated email handler to not mutate user object since we cannot deduce what user profile to update based on the given event data.

elipe17 · elipe17 · commit dd76e3cef2d5 · 2026-05-11T13:56:10.000-05:00
diff --git a/tdrs-backend/tdpservice/security/event_handler.py b/tdrs-backend/tdpservice/security/event_handler.py
@@ -4,8 +4,6 @@
 from datetime import datetime
 from datetime import timezone as dt_timezone
 
-from django.core.exceptions import ValidationError
-from django.core.validators import validate_email
 from django.utils import timezone
 
 from tdpservice.security.models import SecurityEventToken, SecurityEventType
@@ -65,34 +63,17 @@ def _get_emails(security_event):
         new_email = event_data.get("new-value")
         return new_email, old_email
 
-    def _is_valid_email(email):
-        """Return whether the value is formatted as an email address."""
-        try:
-            validate_email(email)
-        except ValidationError:
-            return False
-        return True
-
     def _handle_email_changed(security_event):
-        """Handle email-changed event."""
+        """Handle email-changed event without mutating the user record."""
         new_email, old_email = SecurityEventHandler._get_emails(security_event)
         user = security_event.user
-
-        if not new_email:
-            logger.info(
-                f"User {user.username} identifier changed for email {old_email}; no new email provided"
-            )
-            return
-
-        if not SecurityEventHandler._is_valid_email(new_email):
-            raise ValueError(
-                f"Invalid new email value in identifier-changed event: {new_email}"
-            )
-
-        user.email = new_email
-        user.username = new_email
-        user.save()
-        logger.info(f"User changed email from {old_email} to {new_email}")
+        logger.info(
+            "User %s identifier changed for email %s; SET new-value=%s. "
+            "User email is synced from OIDC claims on login.",
+            user.username,
+            old_email,
+            new_email,
+        )
 
     def _handle_email_recycled(security_event):
         """Handle email-recycled event."""
@@ -133,6 +114,56 @@ def _handle_reproof_complete(security_event):
         SecurityEventType.REPROOF_COMPLETE: _handle_reproof_complete,
     }
 
+    user_mutation_event_types = {
+        SecurityEventType.ACCOUNT_DISABLED,
+        SecurityEventType.ACCOUNT_ENABLED,
+        SecurityEventType.ACCOUNT_PURGED,
+    }
+
+    @classmethod
+    def _get_issued_at(cls, decoded_jwt):
+        """Convert the SET issued-at timestamp to a datetime."""
+        iat_timestamp = decoded_jwt.get("iat")
+        if not iat_timestamp:
+            return None
+
+        try:
+            return datetime.fromtimestamp(iat_timestamp, tz=dt_timezone.utc)
+        except (ValueError, TypeError) as e:
+            logger.warning(f"Error converting timestamp {iat_timestamp}: {e}")
+            return None
+
+    @classmethod
+    def _create_security_event(cls, user, event_type, event_data, decoded_jwt):
+        """Create a SecurityEventToken for a known or unmatched event subject."""
+        subject = event_data.get("subject", {})
+        return SecurityEventToken.objects.create(
+            user=user,
+            email=user.email if user else subject.get("email"),
+            event_type=event_type,
+            event_data=event_data,
+            jwt_id=decoded_jwt.get("jti"),
+            issuer=decoded_jwt.get("iss"),
+            issued_at=cls._get_issued_at(decoded_jwt),
+        )
+
+    @classmethod
+    def _mark_processed(cls, security_event):
+        """Mark a security event as processed."""
+        security_event.processed = True
+        security_event.processed_at = timezone.now()
+        security_event.save()
+
+    @classmethod
+    def _has_subject_identifier(cls, subject):
+        """Return whether the event subject contains an identifier we understand."""
+        return bool(subject.get("sub") or subject.get("email"))
+
+    @classmethod
+    def _event_mutates_user(cls, event_type):
+        """Return whether handling the event is allowed to update the user model."""
+        return event_type in cls.user_mutation_event_types
+
     @classmethod
     def _get_user(cls, subject):
         """Get User model from email or UUID."""
@@ -165,36 +196,36 @@ def handle_event(cls, event_type, event_data, decoded_jwt):
         """Handle specific event types."""
         try:
             subject = event_data.get("subject", {})
-            user = cls._get_user(subject)
-
-            # Convert Unix timestamp to datetime if present
-            iat_timestamp = decoded_jwt.get("iat")
-            issued_at = None
-            if iat_timestamp:
-                try:
-                    issued_at = datetime.fromtimestamp(
-                        iat_timestamp, tz=dt_timezone.utc
-                    )
-                except (ValueError, TypeError) as e:
-                    logger.warning(f"Error converting timestamp {iat_timestamp}: {e}")
-
-            security_event = SecurityEventToken.objects.create(
-                user=user,
-                email=user.email,
-                event_type=event_type,
-                event_data=event_data,
-                jwt_id=decoded_jwt.get("jti"),
-                issuer=decoded_jwt.get("iss"),
-                issued_at=issued_at,
+            try:
+                user = cls._get_user(subject)
+            except ValueError:
+                if cls._event_mutates_user(
+                    event_type
+                ) or not cls._has_subject_identifier(subject):
+                    raise
+
+                security_event = cls._create_security_event(
+                    None, event_type, event_data, decoded_jwt
+                )
+                cls._mark_processed(security_event)
+                logger.warning(
+                    "Recorded %s event with unmatched subject %s. "
+                    "No user mutation was applied; email is synced from OIDC "
+                    "claims on login.",
+                    event_type,
+                    subject,
+                )
+                return
+
+            security_event = cls._create_security_event(
+                user, event_type, event_data, decoded_jwt
             )
 
             # Call the appropriate handler
             handler = cls.handler_map.get(event_type, cls._handle_unknown_event)
             handler(security_event)
 
-            security_event.processed = True
-            security_event.processed_at = timezone.now()
-            security_event.save()
+            cls._mark_processed(security_event)
 
         except Exception as e:
             logger.exception(f"Error handling event {event_type}: {e}")
diff --git a/tdrs-backend/tdpservice/security/test/test_event_handler.py b/tdrs-backend/tdpservice/security/test/test_event_handler.py
@@ -230,6 +230,55 @@ def test_recovery_information_changed(
             decoded_jwt["iat"], tz=timezone.utc
         )
 
+    @pytest.mark.django_db
+    @pytest.mark.parametrize(
+        "event_type",
+        [
+            SecurityEventType.MFA_LOCKED,
+            SecurityEventType.EMAIL_CHANGED,
+            SecurityEventType.EMAIL_RECYCLED,
+            SecurityEventType.PASSWORD_RESET,
+            SecurityEventType.RECOVERY_ACTIVATED,
+            SecurityEventType.RECOVERY_INFORMATION_CHANGED,
+            SecurityEventType.REPROOF_COMPLETE,
+        ],
+    )
+    def test_non_mutating_events_do_not_update_user(
+        self, stt_data_analyst, event_type, decoded_jwt
+    ):
+        """Test only account status and purge events mutate the user model."""
+        prev_email = stt_data_analyst.email
+        prev_username = stt_data_analyst.username
+        prev_active = stt_data_analyst.is_active
+        prev_login_gov_uuid = str(stt_data_analyst.login_gov_uuid)
+        prev_status = stt_data_analyst.account_approval_status
+        event_data = {"subject": {"sub": str(stt_data_analyst.login_gov_uuid)}}
+
+        if event_type in {
+            SecurityEventType.EMAIL_CHANGED,
+            SecurityEventType.EMAIL_RECYCLED,
+        }:
+            event_data = {
+                "subject": {
+                    "email": stt_data_analyst.email,
+                    "subject_type": "email",
+                }
+            }
+
+        SecurityEventHandler.handle_event(event_type, event_data, decoded_jwt)
+
+        stt_data_analyst.refresh_from_db()
+
+        assert stt_data_analyst.email == prev_email
+        assert stt_data_analyst.username == prev_username
+        assert stt_data_analyst.is_active == prev_active
+        assert str(stt_data_analyst.login_gov_uuid) == prev_login_gov_uuid
+        assert stt_data_analyst.account_approval_status == prev_status
+
+        token = SecurityEventToken.objects.first()
+        assert token.processed is True
+        assert token.event_type == event_type
+
     @pytest.mark.django_db
     def test_mfa_locked(self, stt_data_analyst, event_data, decoded_jwt):
         """Test handling of mfa-locked event."""
@@ -262,17 +311,19 @@ def test_mfa_locked(self, stt_data_analyst, event_data, decoded_jwt):
     def test_email_changed(
         self, stt_data_analyst, email_changed_event_data, decoded_jwt
     ):
-        """Test handling email-changed event updates user's email and username."""
+        """Test handling email-changed event does not update user's email and username."""
+        prev_email = stt_data_analyst.email
+        prev_username = stt_data_analyst.username
+
         event_type = SecurityEventType.EMAIL_CHANGED
         SecurityEventHandler.handle_event(
             event_type, email_changed_event_data, decoded_jwt
         )
 
         stt_data_analyst.refresh_from_db()
 
-        new_email = email_changed_event_data["new-value"]
-        assert stt_data_analyst.email == new_email
-        assert stt_data_analyst.username == new_email
+        assert stt_data_analyst.email == prev_email
+        assert stt_data_analyst.username == prev_username
 
         assert SecurityEventToken.objects.count() == 1
         token = SecurityEventToken.objects.first()
@@ -311,6 +362,59 @@ def test_email_changed_without_new_value_does_not_update_user(
         assert token.event_type == event_type
         assert token.event_data == email_changed_event_data_without_new_value
 
+    @pytest.mark.django_db
+    def test_email_changed_for_unknown_email_records_processed_event(
+        self, caplog, decoded_jwt
+    ):
+        """Test Login.gov identifier-changed can arrive with a new unknown email."""
+        event_type = SecurityEventType.EMAIL_CHANGED
+        event_data = {
+            "subject": {
+                "email": "new_login_email@example.com",
+                "subject_type": "email",
+            }
+        }
+
+        with caplog.at_level(logging.WARNING):
+            SecurityEventHandler.handle_event(event_type, event_data, decoded_jwt)
+
+        assert "No user found with the provided 'email'" not in caplog.text
+        assert "unmatched subject" in caplog.text
+
+        assert SecurityEventToken.objects.count() == 1
+        token = SecurityEventToken.objects.first()
+        assert token.user is None
+        assert token.email == "new_login_email@example.com"
+        assert token.processed is True
+        assert token.processed_at is not None
+        assert token.event_type == event_type
+        assert token.event_data == event_data
+        assert token.jwt_id == decoded_jwt["jti"]
+        assert token.issuer == decoded_jwt["iss"]
+
+    @pytest.mark.django_db
+    def test_non_mutating_event_for_unknown_sub_records_processed_event(
+        self, caplog, decoded_jwt
+    ):
+        """Test non-mutating events with an unknown sub are recorded for audit."""
+        event_type = SecurityEventType.PASSWORD_RESET
+        event_data = {"subject": {"sub": str(uuid.uuid4())}}
+
+        with caplog.at_level(logging.WARNING):
+            SecurityEventHandler.handle_event(event_type, event_data, decoded_jwt)
+
+        assert "No user found with login_gov_uuid" not in caplog.text
+        assert "unmatched subject" in caplog.text
+
+        assert SecurityEventToken.objects.count() == 1
+        token = SecurityEventToken.objects.first()
+        assert token.user is None
+        assert token.email is None
+        assert token.processed is True
+        assert token.processed_at is not None
+        assert token.event_type == event_type
+        assert token.event_data == event_data
+
     @pytest.mark.django_db
     def test_email_recycled(
         self, stt_data_analyst, email_recycled_event_data, decoded_jwt
@@ -384,9 +488,9 @@ def test_handle_event_no_sub(self, caplog):
 
     @pytest.mark.django_db
     def test_handle_event_user_not_found(self, caplog):
-        """Test handling event when user is not found."""
+        """Test handling user-mutating event when user is not found."""
 
-        event_type = SecurityEventType.UNKNOWN_EVENT
+        event_type = SecurityEventType.ACCOUNT_DISABLED
         login_gov_uuid = uuid.uuid4()
         event_data = {"subject": {"sub": str(login_gov_uuid)}}
         decoded_jwt = {}
diff --git a/tdrs-backend/tdpservice/users/api/login.py b/tdrs-backend/tdpservice/users/api/login.py
@@ -121,6 +121,16 @@ def verify_email(self, email):
         """Handle user email exceptions."""
         pass
 
+    def _sync_user_email(self, user, email):
+        """Sync the stored user email from trusted OIDC claims."""
+        if not email or (user.email == email and user.username == email):
+            return
+
+        user.email = email
+        user.username = email
+        user.save(update_fields=["email", "username"])
+        logger.info("Updated user email from OIDC claims: %s", user.username)
+
     def _handle_user(self, email, sub, auth_options):
         """Handle user."""
         User = get_user_model()
@@ -217,14 +227,17 @@ def handle_user(self, request, id_token, decoded_token_data):
         # corresponding emails externally.
         user = CustomAuthentication.authenticate(**auth_options)
         logging.debug("user obj:{}".format(user))
-        if user and (
-            not user.is_active
-            or user.account_approval_status == AccountApprovalStatusChoices.DEACTIVATED
-        ):
-            raise InactiveUser(
-                f"Login failed, user account is inactive: {user.username}"
-            )
-        elif not user:
+        if user:
+            self._sync_user_email(user, email)
+            if (
+                not user.is_active
+                or user.account_approval_status
+                == AccountApprovalStatusChoices.DEACTIVATED
+            ):
+                raise InactiveUser(
+                    f"Login failed, user account is inactive: {user.username}"
+                )
+        else:
             user, login_msg = self._handle_user(email, sub, auth_options)
 
         self.verify_email(user)
diff --git a/tdrs-backend/tdpservice/users/oidc.py b/tdrs-backend/tdpservice/users/oidc.py
@@ -80,6 +80,7 @@ def update_user(self, user: User, claims: dict) -> User:
         """Update existing user attributes from Keycloak claims on each login."""
         login_gov_uuid = claims.get("login_gov_uuid")
         hhs_id = claims.get("hhs_id")
+        email = claims.get("email", "").lower()
         changed = False
 
         if login_gov_uuid and str(user.login_gov_uuid) != login_gov_uuid:
@@ -90,6 +91,11 @@ def update_user(self, user: User, claims: dict) -> User:
             user.hhs_id = hhs_id
             changed = True
 
+        if email and (user.email != email or user.username != email):
+            user.email = email
+            user.username = email
+            changed = True
+
         if changed:
             user.save()
             logger.info(
diff --git a/tdrs-backend/tdpservice/users/test/test_oidc.py b/tdrs-backend/tdpservice/users/test/test_oidc.py
diff --git a/tdrs-backend/tdpservice/users/test/test_primary_login_gov_security_events.py b/tdrs-backend/tdpservice/users/test/test_primary_login_gov_security_events.py
