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):