From 51c09db06337dfd060cc7a83a03f7548cdb627df Mon Sep 17 00:00:00 2001 From: Ryan Johnson Date: Tue, 24 Jan 2023 11:36:53 -0800 Subject: [PATCH] fix update_user method to work without a request (#5375) * fix update_user method to work without a request * adjust based on feedback --- kitsune/users/auth.py | 25 +++++++++------- kitsune/users/tests/test_auth.py | 51 +++++++++++++++++++------------- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/kitsune/users/auth.py b/kitsune/users/auth.py index 44f9a5a37..77ea122d9 100644 --- a/kitsune/users/auth.py +++ b/kitsune/users/auth.py @@ -79,9 +79,11 @@ class FXAAuthBackend(OIDCAuthenticationBackend): def update_contributor_status(self, profile): """Register user as contributor.""" - if contribution_area := self.request.session.get("contributor"): + # The request attribute might not be set. + request = getattr(self, "request", None) + if request and (contribution_area := request.session.get("contributor")): add_to_contributors(profile.user, profile.locale, contribution_area) - del self.request.session["contributor"] + del request.session["contributor"] def create_user(self, claims): """Override create user method to mark the profile as migrated.""" @@ -195,11 +197,11 @@ class FXAAuthBackend(OIDCAuthenticationBackend): # Check if the user has active subscriptions subscriptions = claims.get("subscriptions", []) - if not profile.is_fxa_migrated: + if (request := getattr(self, "request", None)) and not profile.is_fxa_migrated: # Check if there is already a Firefox Account with this ID if Profile.objects.filter(fxa_uid=fxa_uid).exists(): msg = _("This Firefox Account is already used in another profile.") - messages.error(self.request, msg) + messages.error(request, msg) return None # If it's not migrated, we can assume that there isn't an FxA id too @@ -207,18 +209,19 @@ class FXAAuthBackend(OIDCAuthenticationBackend): profile.fxa_uid = fxa_uid # This is the first time an existing user is using FxA. Redirect to profile edit # in case the user wants to update any settings. - self.request.session["oidc_login_next"] = reverse("users.edit_my_profile") - messages.info(self.request, "fxa_notification_updated") + request.session["oidc_login_next"] = reverse("users.edit_my_profile") + messages.info(request, "fxa_notification_updated") # There is a change in the email in Firefox Accounts. Let's update user's email # unless we have a superuser if user.email != email and not user.is_staff: if User.objects.exclude(id=user.id).filter(email=email).exists(): - msg = _( - "The e-mail address used with this Firefox Account is already " - "linked in another profile." - ) - messages.error(self.request, msg) + if request: + msg = _( + "The e-mail address used with this Firefox Account is already " + "linked in another profile." + ) + messages.error(request, msg) return None user.email = email user_attr_changed = True diff --git a/kitsune/users/tests/test_auth.py b/kitsune/users/tests/test_auth.py index c45ad2a72..2e33492c2 100644 --- a/kitsune/users/tests/test_auth.py +++ b/kitsune/users/tests/test_auth.py @@ -130,16 +130,23 @@ class FXAAuthBackendTests(TestCase): claims = { "uid": "my_unique_fxa_id", } + # Test without a request (for example, when called from a Celery task). + with self.subTest("without a request"): + self.backend.update_user(user, claims) + assert not message_mock.error.called + assert not User.objects.get(id=user.id).profile.is_fxa_migrated + assert not User.objects.get(id=user.id).profile.fxa_uid + # Test with a request. request_mock = Mock(spec=HttpRequest) request_mock.session = {} - self.backend.claims = claims - self.backend.request = request_mock - self.backend.update_user(user, claims) - message_mock.error.assert_called_with( - request_mock, "This Firefox Account is already used in another profile." - ) - assert not User.objects.get(id=user.id).profile.is_fxa_migrated - assert not User.objects.get(id=user.id).profile.fxa_uid + with self.subTest("with a request"): + self.backend.request = request_mock + self.backend.update_user(user, claims) + message_mock.error.assert_called_with( + request_mock, "This Firefox Account is already used in another profile." + ) + assert not User.objects.get(id=user.id).profile.is_fxa_migrated + assert not User.objects.get(id=user.id).profile.fxa_uid def test_login_existing_user_by_email(self): """Test user filtering by email.""" @@ -166,10 +173,6 @@ class FXAAuthBackendTests(TestCase): profile__is_fxa_migrated=True, ) claims = {"uid": "my_unique_fxa_id", "email": "bar@example.com", "subscriptions": "[]"} - request_mock = Mock(spec=HttpRequest) - request_mock.session = {} - self.backend.claims = claims - self.backend.request = request_mock self.backend.update_user(user, claims) user = User.objects.get(id=user.id) self.assertEqual(user.email, "bar@example.com") @@ -224,14 +227,20 @@ class FXAAuthBackendTests(TestCase): UserFactory.create(email="foo@example.com") user = UserFactory.create(email="bar@example.com") claims = {"uid": "my_unique_fxa_id", "email": "foo@example.com"} + # Test without a request (for example, when called from a Celery task). + with self.subTest("without a request"): + self.backend.update_user(user, claims) + assert not message_mock.error.called + self.assertEqual(User.objects.get(id=user.id).email, "bar@example.com") + # Test with a request. request_mock = Mock(spec=HttpRequest) request_mock.session = {} - self.backend.claims = claims - self.backend.request = request_mock - self.backend.update_user(user, claims) - message_mock.error.assert_called_with( - request_mock, - "The e-mail address used with this Firefox Account" - " is already linked in another profile.", - ) - self.assertEqual(User.objects.get(id=user.id).email, "bar@example.com") + with self.subTest("with a request"): + self.backend.request = request_mock + self.backend.update_user(user, claims) + message_mock.error.assert_called_with( + request_mock, + "The e-mail address used with this Firefox Account" + " is already linked in another profile.", + ) + self.assertEqual(User.objects.get(id=user.id).email, "bar@example.com")