Skip to content

Commit

Permalink
✅(backend) add test to secure updating user when matched on email
Browse files Browse the repository at this point in the history
We had doubts that the user was correctly updated in the case where
its identity was matched on the email and not on the sub. I added
a test and confirmed that it was working correctly. I still modified
the backend to update the user based on its "id" instead of its "sub"
because it was confusing, but both actually work the same.
  • Loading branch information
sampaccoud committed Jan 10, 2025
1 parent 9f83ea7 commit 945f55f
Show file tree
Hide file tree
Showing 2 changed files with 46 additions and 3 deletions.
2 changes: 1 addition & 1 deletion src/backend/core/authentication/backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -134,4 +134,4 @@ def update_user_if_needed(self, user, claims):
)
if has_changed:
updated_claims = {key: value for key, value in claims.items() if value}
self.UserModel.objects.filter(sub=user.sub).update(**updated_claims)
self.UserModel.objects.filter(id=user.id).update(**updated_claims)
47 changes: 45 additions & 2 deletions src/backend/core/tests/authentication/test_backends.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,12 @@ def get_userinfo_mocked(*args):
("Jack", "Duy", "[email protected]"),
],
)
def test_authentication_getter_existing_user_change_fields(
def test_authentication_getter_existing_user_change_fields_sub(
first_name, last_name, email, django_assert_num_queries, monkeypatch
):
"""
It should update the email or name fields on the user when they change.
It should update the email or name fields on the user when they change
and the user was identified by its "sub".
"""
klass = OIDCAuthenticationBackend()
user = UserFactory(
Expand Down Expand Up @@ -164,6 +165,48 @@ def get_userinfo_mocked(*args):
assert user.short_name == first_name


@pytest.mark.parametrize(
"first_name, last_name, email",
[
("Jack", "Doe", "[email protected]"),
("John", "Duy", "[email protected]"),
],
)
def test_authentication_getter_existing_user_change_fields_email(
first_name, last_name, email, django_assert_num_queries, monkeypatch
):
"""
It should update the name fields on the user when they change
and the user was identified by its "email" as fallback.
"""
klass = OIDCAuthenticationBackend()
user = UserFactory(
full_name="John Doe", short_name="John", email="[email protected]"
)

def get_userinfo_mocked(*args):
return {
"sub": "123",
"email": user.email,
"first_name": first_name,
"last_name": last_name,
}

monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked)

# One and only one additional update query when a field has changed
with django_assert_num_queries(3):
authenticated_user = klass.get_or_create_user(
access_token="test-token", id_token=None, payload=None
)

assert user == authenticated_user
user.refresh_from_db()
assert user.email == email
assert user.full_name == f"{first_name:s} {last_name:s}"
assert user.short_name == first_name


def test_authentication_getter_new_user_no_email(monkeypatch):
"""
If no user matches the user's info sub, a user should be created.
Expand Down

0 comments on commit 945f55f

Please sign in to comment.