From c6b6c17540ff91a8b37d15e92454790806a85038 Mon Sep 17 00:00:00 2001 From: Rachid Mrad Date: Tue, 30 Apr 2024 13:28:03 -0400 Subject: [PATCH] revise logic a bit to ensure user and contact do not overwrite each other --- src/djangooidc/backends.py | 21 +++--------- src/djangooidc/tests/test_backends.py | 49 +++------------------------ src/registrar/models/contact.py | 23 +++++++++---- 3 files changed, 26 insertions(+), 67 deletions(-) diff --git a/src/djangooidc/backends.py b/src/djangooidc/backends.py index 6556d8d7a..9b500cd3f 100644 --- a/src/djangooidc/backends.py +++ b/src/djangooidc/backends.py @@ -8,8 +8,6 @@ from django.contrib.auth import get_user_model from django.contrib.auth.backends import ModelBackend from django.utils import timezone -from registrar.models.contact import Contact - logger = logging.getLogger(__name__) @@ -79,26 +77,15 @@ class OpenIdConnectBackend(ModelBackend): 'last_name', and 'phone' fields, unless specific conditions are met. - 'phone' field will be updated if it's None or an empty string. - - 'first_name' and 'last_name' will be updated if the provided value is not empty. + - 'first_name', 'last_name' or 'phone' will be updated if the provided value is not empty. """ - contacts = Contact.objects.filter(user=user) - - if len(contacts) == 0: # no matching contact - logger.warning("Could not find a contact when one should've existed.") - - if len(contacts) > 1: # multiple matches - logger.warning("There are multiple Contacts with the same email address.") - # Iterate over fields to update for key, value in kwargs.items(): # Check if the field is not 'first_name', 'last_name', or 'phone', - # or if it's the 'phone' field and 'user.phone' is None or empty, - # or if it's 'first_name' or 'last_name' and the provided value is not empty - if ( - key not in ["first_name", "last_name", "phone"] - or (key == "phone" and not contacts[0].phone) - or (key in ["first_name", "last_name"] and value) + # or if it's 'first_name' or 'last_name' or 'phone' and the provided value is not empty + if key not in ["first_name", "last_name", "phone"] or ( + key in ["first_name", "last_name", "phone"] and value ): # Update the corresponding attribute of the user object setattr(user, key, value) diff --git a/src/djangooidc/tests/test_backends.py b/src/djangooidc/tests/test_backends.py index ffe26e2cb..c15106fa9 100644 --- a/src/djangooidc/tests/test_backends.py +++ b/src/djangooidc/tests/test_backends.py @@ -64,6 +64,7 @@ class OpenIdConnectBackendTestCase(TestCase): # Remove given_name and family_name from the input, self.kwargs self.kwargs.pop("given_name", None) self.kwargs.pop("family_name", None) + self.kwargs.pop("phone", None) # Ensure that the authenticate method updates the existing user # and preserves existing first and last names @@ -78,53 +79,13 @@ class OpenIdConnectBackendTestCase(TestCase): self.assertEqual(user.email, "john.doe@example.com") self.assertEqual(user.phone, "9999999999") - def test_authenticate_with_existing_user_with_no_phone_will_update_phone(self): - """Test that authenticate updates an existing user if it finds one. - For this test, the existing user has an empty string for phone - - The existing user's phone number is overwritten""" - # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user", phone="") - - # Ensure that the authenticate method updates the existing user - # and preserves existing first and last names - user = self.backend.authenticate(request=None, **self.kwargs) - self.assertIsNotNone(user) - self.assertIsInstance(user, User) - self.assertEqual(user, existing_user) # The same user instance should be returned - - # Verify that user fields are correctly updated - self.assertEqual(user.first_name, "John") - self.assertEqual(user.last_name, "Doe") - self.assertEqual(user.email, "john.doe@example.com") - self.assertEqual(user.phone, "123456789") - - def test_authenticate_with_existing_user_with_none_phone_will_update_phone(self): - """Test that authenticate updates an existing user if it finds one. - For this test, the existing user has None for phone - - The existing user's phone number is overwritten""" - # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user") - - # Ensure that the authenticate method updates the existing user - # and preserves existing first and last names - user = self.backend.authenticate(request=None, **self.kwargs) - self.assertIsNotNone(user) - self.assertIsInstance(user, User) - self.assertEqual(user, existing_user) # The same user instance should be returned - - # Verify that user fields are correctly updated - self.assertEqual(user.first_name, "John") - self.assertEqual(user.last_name, "Doe") - self.assertEqual(user.email, "john.doe@example.com") - self.assertEqual(user.phone, "123456789") - - def test_authenticate_with_existing_user_different_name(self): + def test_authenticate_with_existing_user_different_name_phone(self): """Test that authenticate updates an existing user if it finds one. For this test, given_name and family_name are supplied and overwrite""" # Create an existing user with the same username and with first and last names - existing_user = User.objects.create_user(username="test_user", first_name="WillBe", last_name="Replaced") + existing_user = User.objects.create_user( + username="test_user", first_name="WillBe", last_name="Replaced", phone="987654321" + ) # Ensure that the authenticate method updates the existing user # and preserves existing first and last names diff --git a/src/registrar/models/contact.py b/src/registrar/models/contact.py index 185b84ded..3ebd8bc3e 100644 --- a/src/registrar/models/contact.py +++ b/src/registrar/models/contact.py @@ -101,12 +101,23 @@ class Contact(TimeStampedModel): # Call the parent class's save method to perform the actual save super().save(*args, **kwargs) - # Update the related User object's first_name and last_name - if self.user and (not self.user.first_name or not self.user.last_name or not self.user.phone): - self.user.first_name = self.first_name - self.user.last_name = self.last_name - self.user.phone = self.phone - self.user.save() + if self.user: + updated = False + + # Update first name and last name if necessary + if not self.user.first_name or not self.user.last_name: + self.user.first_name = self.first_name + self.user.last_name = self.last_name + updated = True + + # Update phone if necessary + if not self.user.phone: + self.user.phone = self.phone + updated = True + + # Save user if any updates were made + if updated: + self.user.save() def __str__(self): if self.first_name or self.last_name: