From eee20033aecec518ce129985e79e5f247ee3cce0 Mon Sep 17 00:00:00 2001 From: Samuel Paccoud - DINUM Date: Mon, 30 Sep 2024 15:13:42 +0200 Subject: [PATCH] =?UTF-8?q?=E2=9C=A8(backend)=20add=20full=5Fname=20and=20?= =?UTF-8?q?short=5Fname=20to=20user=20model=20and=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The full_name and short_name field are synchronized with the OIDC token upon each login. --- CHANGELOG.md | 1 + src/backend/.pylintrc | 4 +- src/backend/core/admin.py | 36 ++++++- src/backend/core/api/serializers.py | 4 +- src/backend/core/authentication/backends.py | 46 +++++---- src/backend/core/factories.py | 2 + .../0006_add_user_full_name_and_short_name.py | 28 ++++++ src/backend/core/models.py | 4 + .../tests/authentication/test_backends.py | 93 ++++++++++++++++++- src/backend/core/tests/test_api_users.py | 2 + .../demo/management/commands/create_demo.py | 7 ++ src/backend/impress/settings.py | 11 +++ 12 files changed, 206 insertions(+), 32 deletions(-) create mode 100644 src/backend/core/migrations/0006_add_user_full_name_and_short_name.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 9e1f3311..1c180151 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ and this project adheres to ## Added +- ✨(backend) add name fields to the user synchronized with OIDC #301 - ✨(ci) add security scan #291 - ✨(frontend) Activate versions feature #240 - ✨(frontend) one-click document creation #275 diff --git a/src/backend/.pylintrc b/src/backend/.pylintrc index d7490918..9807de7d 100644 --- a/src/backend/.pylintrc +++ b/src/backend/.pylintrc @@ -447,10 +447,10 @@ max-bool-expr=5 max-branches=12 # Maximum number of locals for function / method body -max-locals=15 +max-locals=20 # Maximum number of parents for a class (see R0901). -max-parents=7 +max-parents=10 # Maximum number of public methods for a class (see R0904). max-public-methods=20 diff --git a/src/backend/core/admin.py b/src/backend/core/admin.py index 2df1c279..25dab177 100644 --- a/src/backend/core/admin.py +++ b/src/backend/core/admin.py @@ -29,7 +29,19 @@ class UserAdmin(auth_admin.UserAdmin): ) }, ), - (_("Personal info"), {"fields": ("sub", "email", "language", "timezone")}), + ( + _("Personal info"), + { + "fields": ( + "sub", + "email", + "full_name", + "short_name", + "language", + "timezone", + ) + }, + ), ( _("Permissions"), { @@ -58,6 +70,7 @@ class UserAdmin(auth_admin.UserAdmin): list_display = ( "id", "sub", + "full_name", "admin_email", "email", "is_active", @@ -68,9 +81,24 @@ class UserAdmin(auth_admin.UserAdmin): "updated_at", ) list_filter = ("is_staff", "is_superuser", "is_device", "is_active") - ordering = ("is_active", "-is_superuser", "-is_staff", "-is_device", "-updated_at") - readonly_fields = ("id", "sub", "email", "created_at", "updated_at") - search_fields = ("id", "sub", "admin_email", "email") + ordering = ( + "is_active", + "-is_superuser", + "-is_staff", + "-is_device", + "-updated_at", + "full_name", + ) + readonly_fields = ( + "id", + "sub", + "email", + "full_name", + "short_name", + "created_at", + "updated_at", + ) + search_fields = ("id", "sub", "admin_email", "email", "full_name") @admin.register(models.Template) diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index e09bfef4..6154ced3 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -16,8 +16,8 @@ class UserSerializer(serializers.ModelSerializer): class Meta: model = models.User - fields = ["id", "email"] - read_only_fields = ["id", "email"] + fields = ["id", "email", "full_name", "short_name"] + read_only_fields = ["id", "email", "full_name", "short_name"] class BaseAccessSerializer(serializers.ModelSerializer): diff --git a/src/backend/core/authentication/backends.py b/src/backend/core/authentication/backends.py index efada966..77d4142c 100644 --- a/src/backend/core/authentication/backends.py +++ b/src/backend/core/authentication/backends.py @@ -1,5 +1,6 @@ """Authentication Backends for the Impress core app.""" +from django.conf import settings from django.core.exceptions import SuspiciousOperation from django.utils.translation import gettext_lazy as _ @@ -74,37 +75,42 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): """ user_info = self.get_userinfo(access_token, id_token, payload) - sub = user_info.get("sub") + # Compute user full name from OIDC name fields as defined in settings + names_list = [ + user_info[field] + for field in settings.USER_OIDC_FIELDS_TO_FULLNAME + if user_info.get(field) + ] + claims = { + "email": user_info.get("email"), + "full_name": " ".join(names_list) or None, + "short_name": user_info.get(settings.USER_OIDC_FIELD_TO_SHORTNAME), + } + + sub = user_info.get("sub") if sub is None: raise SuspiciousOperation( _("User info contained no recognizable user identification") ) try: - user = User.objects.get(sub=sub) + user = User.objects.get(sub=sub, is_active=True) except User.DoesNotExist: if self.get_settings("OIDC_CREATE_USER", True): - user = self.create_user(user_info) + user = User.objects.create( + sub=sub, + password="!", # noqa: S106 + **claims, + ) else: user = None - - return user - - def create_user(self, claims): - """Return a newly created User instance.""" - - sub = claims.get("sub") - - if sub is None: - raise SuspiciousOperation( - _("Claims contained no recognizable user identification") + else: + has_changed = any( + value and value != getattr(user, key) for key, value in claims.items() ) - - user = User.objects.create( - sub=sub, - email=claims.get("email"), - password="!", # noqa: S106 - ) + if has_changed: + updated_claims = {key: value for key, value in claims.items() if value} + self.UserModel.objects.filter(sub=sub).update(**updated_claims) return user diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index 54ee9f8e..c410615f 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -22,6 +22,8 @@ class UserFactory(factory.django.DjangoModelFactory): sub = factory.Sequence(lambda n: f"user{n!s}") email = factory.Faker("email") + full_name = factory.Faker("name") + short_name = factory.Faker("first_name") language = factory.fuzzy.FuzzyChoice([lang[0] for lang in settings.LANGUAGES]) password = make_password("password") diff --git a/src/backend/core/migrations/0006_add_user_full_name_and_short_name.py b/src/backend/core/migrations/0006_add_user_full_name_and_short_name.py new file mode 100644 index 00000000..dd5e3b7c --- /dev/null +++ b/src/backend/core/migrations/0006_add_user_full_name_and_short_name.py @@ -0,0 +1,28 @@ +# Generated by Django 5.1.1 on 2024-09-29 03:47 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0005_remove_document_is_public_alter_document_link_reach_and_more'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='full_name', + field=models.CharField(blank=True, max_length=100, null=True, verbose_name='full name'), + ), + migrations.AddField( + model_name='user', + name='short_name', + field=models.CharField(blank=True, max_length=20, null=True, verbose_name='short name'), + ), + migrations.AlterField( + model_name='user', + name='language', + field=models.CharField(choices="(('en-us', 'English'), ('fr-fr', 'French'))", default='en-us', help_text='The language in which the user wants to see the interface.', max_length=10, verbose_name='language'), + ), + ] diff --git a/src/backend/core/models.py b/src/backend/core/models.py index c4390f97..bea27799 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -145,6 +145,10 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): blank=True, null=True, ) + + full_name = models.CharField(_("full name"), max_length=100, null=True, blank=True) + short_name = models.CharField(_("short name"), max_length=20, null=True, blank=True) + email = models.EmailField(_("identity email address"), blank=True, null=True) # Unlike the "email" field which stores the email coming from the OIDC token, this field diff --git a/src/backend/core/tests/authentication/test_backends.py b/src/backend/core/tests/authentication/test_backends.py index f3b9e093..fa4d082c 100644 --- a/src/backend/core/tests/authentication/test_backends.py +++ b/src/backend/core/tests/authentication/test_backends.py @@ -38,6 +38,77 @@ def test_authentication_getter_existing_user_no_email( assert user == db_user +def test_authentication_getter_existing_user_with_email( + django_assert_num_queries, monkeypatch +): + """ + When the user's info contains an email and targets an existing user, + """ + klass = OIDCAuthenticationBackend() + user = UserFactory(full_name="John Doe", short_name="John") + + def get_userinfo_mocked(*args): + return { + "sub": user.sub, + "email": user.email, + "first_name": "John", + "last_name": "Doe", + } + + monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) + + # Only 1 query because email and names have not changed + with django_assert_num_queries(1): + authenticated_user = klass.get_or_create_user( + access_token="test-token", id_token=None, payload=None + ) + + assert user == authenticated_user + + +@pytest.mark.parametrize( + "first_name, last_name, email", + [ + ("Jack", "Doe", "john.doe@example.com"), + ("John", "Duy", "john.doe@example.com"), + ("John", "Doe", "jack.duy@example.com"), + ("Jack", "Duy", "jack.duy@example.com"), + ], +) +def test_authentication_getter_existing_user_change_fields( + first_name, last_name, email, django_assert_num_queries, monkeypatch +): + """ + It should update the email or name fields on the user when they change. + """ + klass = OIDCAuthenticationBackend() + user = UserFactory( + full_name="John Doe", short_name="John", email="john.doe@example.com" + ) + + def get_userinfo_mocked(*args): + return { + "sub": user.sub, + "email": 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(2): + 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. @@ -56,6 +127,8 @@ def test_authentication_getter_new_user_no_email(monkeypatch): assert user.sub == "123" assert user.email is None + assert user.full_name is None + assert user.short_name is None assert user.password == "!" assert models.User.objects.count() == 1 @@ -81,6 +154,8 @@ def test_authentication_getter_new_user_with_email(monkeypatch): assert user.sub == "123" assert user.email == email + assert user.full_name == "John Doe" + assert user.short_name == "John" assert user.password == "!" assert models.User.objects.count() == 1 @@ -116,14 +191,19 @@ def test_authentication_get_userinfo_json_response(): responses.add( responses.GET, re.compile(r".*/userinfo"), - json={"name": "John Doe", "email": "john.doe@example.com"}, + json={ + "first_name": "John", + "last_name": "Doe", + "email": "john.doe@example.com", + }, status=200, ) oidc_backend = OIDCAuthenticationBackend() result = oidc_backend.get_userinfo("fake_access_token", None, None) - assert result["name"] == "John Doe" + assert result["first_name"] == "John" + assert result["last_name"] == "Doe" assert result["email"] == "john.doe@example.com" @@ -137,14 +217,19 @@ def test_authentication_get_userinfo_token_response(monkeypatch): ) def mock_verify_token(self, token): # pylint: disable=unused-argument - return {"name": "Jane Doe", "email": "jane.doe@example.com"} + return { + "first_name": "Jane", + "last_name": "Doe", + "email": "jane.doe@example.com", + } monkeypatch.setattr(OIDCAuthenticationBackend, "verify_token", mock_verify_token) oidc_backend = OIDCAuthenticationBackend() result = oidc_backend.get_userinfo("fake_access_token", None, None) - assert result["name"] == "Jane Doe" + assert result["first_name"] == "Jane" + assert result["last_name"] == "Doe" assert result["email"] == "jane.doe@example.com" diff --git a/src/backend/core/tests/test_api_users.py b/src/backend/core/tests/test_api_users.py index 79eda932..d95153f2 100644 --- a/src/backend/core/tests/test_api_users.py +++ b/src/backend/core/tests/test_api_users.py @@ -120,6 +120,8 @@ def test_api_users_retrieve_me_authenticated(): assert response.json() == { "id": str(user.id), "email": user.email, + "full_name": user.full_name, + "short_name": user.short_name, } diff --git a/src/backend/demo/management/commands/create_demo.py b/src/backend/demo/management/commands/create_demo.py index 8b5b97c1..49cde524 100644 --- a/src/backend/demo/management/commands/create_demo.py +++ b/src/backend/demo/management/commands/create_demo.py @@ -2,6 +2,7 @@ """create_demo management command""" import logging +import math import random import time from collections import defaultdict @@ -111,7 +112,11 @@ def create_demo(stdout): queue = BulkQueue(stdout) with Timeit(stdout, "Creating users"): + name_size = int(math.sqrt(defaults.NB_OBJECTS["users"])) + first_names = [fake.first_name() for _ in range(name_size)] + last_names = [fake.last_name() for _ in range(name_size)] for i in range(defaults.NB_OBJECTS["users"]): + first_name = random.choice(first_names) queue.push( models.User( admin_email=f"user{i:d}@example.com", @@ -120,6 +125,8 @@ def create_demo(stdout): is_superuser=False, is_active=True, is_staff=False, + short_name=first_name, + full_name=f"{first_name:s} {random.choice(last_names):s}", language=random.choice(settings.LANGUAGES)[0], ) ) diff --git a/src/backend/impress/settings.py b/src/backend/impress/settings.py index 5f1137e0..3912fe82 100755 --- a/src/backend/impress/settings.py +++ b/src/backend/impress/settings.py @@ -388,6 +388,17 @@ class Base(Configuration): default=True, environ_name="ALLOW_LOGOUT_GET_METHOD", environ_prefix=None ) + USER_OIDC_FIELDS_TO_FULLNAME = values.ListValue( + default=["first_name", "last_name"], + environ_name="USER_OIDC_FIELDS_TO_FULLNAME", + environ_prefix=None, + ) + USER_OIDC_FIELD_TO_SHORTNAME = values.Value( + default="first_name", + environ_name="USER_OIDC_FIELD_TO_SHORTNAME", + environ_prefix=None, + ) + # pylint: disable=invalid-name @property def ENVIRONMENT(self):