From 8d7c545d1a928dd0bf47595a317753eec65792a5 Mon Sep 17 00:00:00 2001 From: Anthony LC Date: Thu, 15 Feb 2024 12:55:00 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=97=83=EF=B8=8F(backend)=20add=20name=20f?= =?UTF-8?q?ield=20to=20identity?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need a name for the user when we display the members in the frontend. This commit adds the name column to the identity model. We sync the Keycloak user with the identity model when the user logs in to fill and udpate the name automatically. --- docker-compose.yml | 2 +- docker/auth/realm.json | 11 +-- src/backend/core/authentication.py | 38 ++++++---- src/backend/core/factories.py | 1 + src/backend/core/migrations/0001_initial.py | 8 ++- src/backend/core/models.py | 1 + .../test_authentication_get_or_create_user.py | 71 ++++++++++++++----- .../demo/management/commands/create_demo.py | 5 ++ src/backend/people/settings.py | 6 ++ 9 files changed, 107 insertions(+), 36 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index d102911..4bd41d7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -92,8 +92,8 @@ services: image: nginx:1.25 ports: - "8082:8082" - - "8088:8088" - "8083:8083" + - "8088:8088" volumes: - ./docker/files/etc/nginx/conf.d:/etc/nginx/conf.d:ro - ./src/frontend/apps/desk/out:/home/desk diff --git a/docker/auth/realm.json b/docker/auth/realm.json index 295ae2c..05defbb 100644 --- a/docker/auth/realm.json +++ b/docker/auth/realm.json @@ -46,6 +46,9 @@ "users": [ { "username": "people", + "email": "people@people.world", + "firstName": "John", + "lastName": "Doe", "enabled": true, "credentials": [ { @@ -1098,7 +1101,7 @@ }, { "id": "79712bcf-b7f7-4ca3-b97c-418f48fded9b", - "name": "given name", + "name": "first name", "protocol": "openid-connect", "protocolMapper": "oidc-usermodel-property-mapper", "consentRequired": false, @@ -1107,7 +1110,7 @@ "user.attribute": "firstName", "id.token.claim": "true", "access.token.claim": "true", - "claim.name": "given_name", + "claim.name": "first_name", "jsonType.label": "String" } }, @@ -1128,7 +1131,7 @@ }, { "id": "7f741e96-41fe-4021-bbfd-506e7eb94e69", - "name": "family name", + "name": "last name", "protocol": "openid-connect", "protocolMapper": "oidc-usermodel-property-mapper", "consentRequired": false, @@ -1137,7 +1140,7 @@ "user.attribute": "lastName", "id.token.claim": "true", "access.token.claim": "true", - "claim.name": "family_name", + "claim.name": "last_name", "jsonType.label": "String" } }, diff --git a/src/backend/core/authentication.py b/src/backend/core/authentication.py index 2c16142..b868b32 100644 --- a/src/backend/core/authentication.py +++ b/src/backend/core/authentication.py @@ -1,5 +1,5 @@ """Authentication for the People core app.""" - +from django.conf import settings from django.core.exceptions import SuspiciousOperation from django.db import models from django.utils.translation import gettext_lazy as _ @@ -66,9 +66,15 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): user_info = self.get_userinfo(access_token, id_token, payload) - email = user_info.get("email") - sub = user_info.get("sub") + # Compute user name from OIDC name fields as defined in settings + names_list = [ + user_info[field] + for field in settings.USER_OIDC_FIELDS_TO_NAME + if user_info.get(field) + ] + user_info["name"] = " ".join(names_list) or None + sub = user_info.get("sub") if sub is None: raise SuspiciousOperation( _("User info contained no recognizable user identification") @@ -76,14 +82,23 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): user = ( self.UserModel.objects.filter(identities__sub=sub) - .annotate(identity_email=models.F("identities__email")) + .annotate( + identity_email=models.F("identities__email"), + identity_name=models.F("identities__name"), + ) .distinct() .first() ) - if user: - if email and email != user.identity_email: - Identity.objects.filter(sub=sub).update(email=email) + email = user_info.get("email") + name = user_info.get("name") + if ( + email + and email != user.identity_email + or name + and name != user.identity_name + ): + Identity.objects.filter(sub=sub).update(email=email, name=name) elif self.get_settings("OIDC_CREATE_USER", True): user = self.create_user(user_info) @@ -92,16 +107,15 @@ class OIDCAuthenticationBackend(MozillaOIDCAuthenticationBackend): def create_user(self, claims): """Return a newly created User instance.""" - - email = claims.get("email") sub = claims.get("sub") - if sub is None: raise SuspiciousOperation( _("Claims contained no recognizable user identification") ) - user = self.UserModel.objects.create(password="!", email=email) # noqa: S106 - Identity.objects.create(user=user, sub=sub, email=email) + user = self.UserModel.objects.create(password="!") # noqa: S106 + Identity.objects.create( + user=user, sub=sub, email=claims.get("email"), name=claims.get("name") + ) return user diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index c8cd0e2..33ca2a7 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -139,6 +139,7 @@ class IdentityFactory(factory.django.DjangoModelFactory): user = factory.SubFactory(UserFactory) sub = factory.Sequence(lambda n: f"user{n!s}") email = factory.Faker("email") + name = factory.Faker("name") class TeamFactory(factory.django.DjangoModelFactory): diff --git a/src/backend/core/migrations/0001_initial.py b/src/backend/core/migrations/0001_initial.py index 76c4726..1cec853 100644 --- a/src/backend/core/migrations/0001_initial.py +++ b/src/backend/core/migrations/0001_initial.py @@ -1,4 +1,4 @@ -# Generated by Django 5.0.1 on 2024-02-21 12:34 +# Generated by Django 5.0.2 on 2024-02-23 06:46 import django.contrib.auth.models import django.core.validators @@ -27,7 +27,7 @@ class Migration(migrations.Migration): ('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created at')), ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated at')), ('name', models.CharField(max_length=100)), - ('slug', models.SlugField(max_length=100, unique=True)), + ('slug', models.SlugField(editable=False, max_length=100, unique=True)), ], options={ 'verbose_name': 'Team', @@ -59,6 +59,9 @@ class Migration(migrations.Migration): 'verbose_name_plural': 'users', 'db_table': 'people_user', }, + managers=[ + ('objects', django.contrib.auth.models.UserManager()), + ], ), migrations.CreateModel( name='Contact', @@ -92,6 +95,7 @@ class Migration(migrations.Migration): ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated at')), ('sub', models.CharField(help_text='Required. 255 characters or fewer. Letters, numbers, and @/./+/-/_ characters only.', max_length=255, unique=True, validators=[django.core.validators.RegexValidator(message='Enter a valid sub. This value may contain only letters, numbers, and @/./+/-/_ characters.', regex='^[\\w.@+-]+\\Z')], verbose_name='sub')), ('email', models.EmailField(blank=True, max_length=254, null=True, verbose_name='email address')), + ('name', models.CharField(blank=True, max_length=100, null=True, verbose_name='name')), ('is_main', models.BooleanField(default=False, help_text='Designates whether the email is the main one.', verbose_name='main')), ('user', models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='identities', to=settings.AUTH_USER_MODEL)), ], diff --git a/src/backend/core/models.py b/src/backend/core/models.py index dc19738..e73fff3 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -254,6 +254,7 @@ class Identity(BaseModel): validators=[sub_validator], ) email = models.EmailField(_("email address"), null=True, blank=True) + name = models.CharField(_("name"), max_length=100, null=True, blank=True) is_main = models.BooleanField( _("main"), default=False, diff --git a/src/backend/core/tests/test_authentication_get_or_create_user.py b/src/backend/core/tests/test_authentication_get_or_create_user.py index 7f877bb..2d30e25 100644 --- a/src/backend/core/tests/test_authentication_get_or_create_user.py +++ b/src/backend/core/tests/test_authentication_get_or_create_user.py @@ -21,7 +21,7 @@ def test_authentication_getter_existing_user_no_email( klass = OIDCAuthenticationBackend() # Create a user and its identity - identity = IdentityFactory() + identity = IdentityFactory(name=None) # Create multiple identities for a user for _ in range(5): @@ -46,47 +46,82 @@ def test_authentication_getter_existing_user_with_email( ): """ When the user's info contains an email and targets an existing user, - it should update the email on the identity but not on the user. """ klass = OIDCAuthenticationBackend() - identity = IdentityFactory() + identity = IdentityFactory(name="John Doe") # Create multiple identities for a user for _ in range(5): IdentityFactory(user=identity.user) - user_email = identity.user.email assert models.User.objects.count() == 1 def get_userinfo_mocked(*args): - return {"sub": identity.sub, "email": identity.email} + return { + "sub": identity.sub, + "email": identity.email, + "first_name": "John", + "last_name": "Doe", + } monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) - # Only 1 query if the email has not changed + # Only 1 query because email and names have not changed with django_assert_num_queries(1): user = klass.get_or_create_user( access_token="test-token", id_token=None, payload=None ) - new_email = "test@fooo.com" + assert models.User.objects.get() == user - def get_userinfo_mocked_new_email(*args): - return {"sub": identity.sub, "email": new_email} - monkeypatch.setattr( - OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked_new_email - ) +@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 identity when they change. + The email on the user should not be changed. + """ + klass = OIDCAuthenticationBackend() - # Additional update query if the email has changed + identity = IdentityFactory(name="John Doe", email="john.doe@example.com") + user_email = identity.user.email + + # Create multiple identities for a user + for _ in range(5): + IdentityFactory(user=identity.user) + + assert models.User.objects.count() == 1 + + def get_userinfo_mocked(*args): + return { + "sub": identity.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): user = klass.get_or_create_user( access_token="test-token", id_token=None, payload=None ) identity.refresh_from_db() - assert identity.email == new_email + assert identity.email == email + assert identity.name == f"{first_name:s} {last_name:s}" assert models.User.objects.count() == 1 assert user == identity.user @@ -121,14 +156,15 @@ def test_authentication_getter_new_user_no_email(monkeypatch): def test_authentication_getter_new_user_with_email(monkeypatch): """ If no user matches the user's info sub, a user should be created. - User's info contains an email, created user's email should be set. + User's email and name should be set on the identity. + The "email" field on the User model should not be set as it is reserved for staff users. """ klass = OIDCAuthenticationBackend() email = "people@example.com" def get_userinfo_mocked(*args): - return {"sub": "123", "email": email} + return {"sub": "123", "email": email, "first_name": "John", "last_name": "Doe"} monkeypatch.setattr(OIDCAuthenticationBackend, "get_userinfo", get_userinfo_mocked) @@ -139,8 +175,9 @@ def test_authentication_getter_new_user_with_email(monkeypatch): identity = user.identities.get() assert identity.sub == "123" assert identity.email == email + assert identity.name == "John Doe" - assert user.email == email + assert user.email is None assert models.User.objects.count() == 1 diff --git a/src/backend/demo/management/commands/create_demo.py b/src/backend/demo/management/commands/create_demo.py index 17b6828..3ec1440 100755 --- a/src/backend/demo/management/commands/create_demo.py +++ b/src/backend/demo/management/commands/create_demo.py @@ -11,10 +11,14 @@ from django import db from django.conf import settings from django.core.management.base import BaseCommand, CommandError +from faker import Faker + from core import models from demo import defaults +fake = Faker() + logger = logging.getLogger("people.commands.demo.create_demo") @@ -134,6 +138,7 @@ def create_demo(stdout): sub=uuid4(), email=f"identity{i:d}{user_email:s}", is_main=(i == 0), + name=fake.name(), ) ) queue.flush() diff --git a/src/backend/people/settings.py b/src/backend/people/settings.py index 2bd5588..b5505ed 100755 --- a/src/backend/people/settings.py +++ b/src/backend/people/settings.py @@ -334,6 +334,12 @@ class Base(Configuration): default=[], environ_name="OIDC_REDIRECT_ALLOWED_HOSTS", environ_prefix=None ) + USER_OIDC_FIELDS_TO_NAME = values.ListValue( + default=["first_name", "last_name"], + environ_name="USER_OIDC_FIELDS_TO_NAME", + environ_prefix=None, + ) + # pylint: disable=invalid-name @property def ENVIRONMENT(self):