diff --git a/Makefile b/Makefile index 6a9f71b..96c25b1 100644 --- a/Makefile +++ b/Makefile @@ -177,7 +177,7 @@ migrate: ## run django migrations for the people project. superuser: ## Create an admin superuser with password "admin" @echo "$(BOLD)Creating a Django superuser$(RESET)" - @$(MANAGE) createsuperuser --username admin --email admin@example.com --no-input + @$(MANAGE) createsuperuser --email admin@example.com --password admin .PHONY: superuser back-i18n-compile: ## compile the gettext files diff --git a/src/backend/core/factories.py b/src/backend/core/factories.py index cca1d83..d9384a8 100644 --- a/src/backend/core/factories.py +++ b/src/backend/core/factories.py @@ -124,6 +124,7 @@ class UserFactory(factory.django.DjangoModelFactory): class Meta: model = models.User + email = factory.Faker("email") language = factory.fuzzy.FuzzyChoice([lang[0] for lang in settings.LANGUAGES]) password = make_password("password") diff --git a/src/backend/core/migrations/0001_initial.py b/src/backend/core/migrations/0001_initial.py index 8d3af27..7066d6e 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 on 2023-12-31 17:11 +# Generated by Django 5.0 on 2024-01-14 13:41 import core.models import django.core.validators @@ -44,6 +44,7 @@ class Migration(migrations.Migration): ('id', models.UUIDField(default=uuid.uuid4, editable=False, help_text='primary key for the record as UUID', primary_key=True, serialize=False, verbose_name='id')), ('created_at', models.DateTimeField(auto_now_add=True, help_text='date and time at which a record was created', verbose_name='created on')), ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')), + ('email', models.EmailField(blank=True, max_length=254, null=True, unique=True, verbose_name='email address')), ('language', 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')), ('timezone', timezone_field.fields.TimeZoneField(choices_display='WITH_GMT_OFFSET', default='UTC', help_text='The timezone in which the user wants to see times.', use_pytz=False)), ('is_device', models.BooleanField(default=False, help_text='Whether the user is a device or a real user.', verbose_name='device')), @@ -92,7 +93,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 on')), ('updated_at', models.DateTimeField(auto_now=True, help_text='date and time at which a record was last updated', verbose_name='updated on')), ('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(max_length=254, verbose_name='email address')), + ('email', models.EmailField(blank=True, max_length=254, null=True, verbose_name='email address')), ('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 200b1d1..2c4f756 100644 --- a/src/backend/core/models.py +++ b/src/backend/core/models.py @@ -159,6 +159,7 @@ class UserManager(auth_models.UserManager): class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): """User model to work with OIDC only authentication.""" + email = models.EmailField(_("email address"), unique=True, null=True, blank=True) profile_contact = models.OneToOneField( Contact, on_delete=models.SET_NULL, @@ -200,7 +201,7 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): objects = UserManager() - USERNAME_FIELD = "id" + USERNAME_FIELD = "email" REQUIRED_FIELDS = [] class Meta: @@ -209,7 +210,11 @@ class User(AbstractBaseUser, BaseModel, auth_models.PermissionsMixin): verbose_name_plural = _("users") def __str__(self): - return str(self.profile_contact) if self.profile_contact else str(self.id) + return ( + str(self.profile_contact) + if self.profile_contact + else self.email or str(self.id) + ) def clean(self): """Validate fields.""" @@ -257,7 +262,7 @@ class Identity(BaseModel): unique=True, validators=[sub_validator], ) - email = models.EmailField(_("email address")) + email = models.EmailField(_("email address"), null=True, blank=True) is_main = models.BooleanField( _("main"), default=False, @@ -286,7 +291,8 @@ class Identity(BaseModel): def clean(self): """Normalize the email field and clean the 'is_main' field.""" - self.email = User.objects.normalize_email(self.email) + if self.email: + self.email = User.objects.normalize_email(self.email) if not self.user.identities.exclude(pk=self.pk).filter(is_main=True).exists(): if not self.created_at: self.is_main = True @@ -452,20 +458,21 @@ def oidc_user_getter(validated_token): ) from exc try: - user = User.objects.select_related("profile_contact").get( - **{api_settings.USER_ID_FIELD: user_id} - ) - except User.DoesNotExist: - contact = Contact.objects.create() - user = User.objects.create( - **{api_settings.USER_ID_FIELD: user_id}, profile_contact=contact - ) + email_param = {"email": validated_token["email"]} + except KeyError: + email_param = {} - # If the identity in the token is seen for the first time, make it the main email. - # Otherwise, update the email and respect the main identity set by the user - if email := validated_token["email"]: - Identity.objects.update_or_create( - user=user, email=email, create_defaults={"is_main": True} - ) + user = ( + User.objects.filter(identities__sub=user_id) + .annotate(identity_email=models.F("identities__email")) + .distinct() + .first() + ) + + if user is None: + user = User.objects.create(password="!", **email_param) # noqa: S106 + Identity.objects.create(user=user, sub=user_id, **email_param) + elif email_param and validated_token["email"] != user.identity_email: + Identity.objects.filter(sub=user_id).update(email=validated_token["email"]) return user diff --git a/src/backend/core/tests/test_models_identities.py b/src/backend/core/tests/test_models_identities.py index 25108bd..cf155d7 100644 --- a/src/backend/core/tests/test_models_identities.py +++ b/src/backend/core/tests/test_models_identities.py @@ -69,11 +69,10 @@ def test_models_identities_is_main_switch(): assert first_identity.is_main is False -def test_models_identities_email_required(): - """The "email" field is required.""" +def test_models_identities_email_not_required(): + """The "email" field can be blank.""" user = factories.UserFactory() - with pytest.raises(ValidationError, match="This field cannot be null."): - models.Identity.objects.create(user=user, email=None) + models.Identity.objects.create(user=user, sub="123", email=None) def test_models_identities_user_required(): diff --git a/src/backend/core/tests/test_models_oidc_user_getter.py b/src/backend/core/tests/test_models_oidc_user_getter.py new file mode 100644 index 0000000..da3558e --- /dev/null +++ b/src/backend/core/tests/test_models_oidc_user_getter.py @@ -0,0 +1,107 @@ +"""Unit tests for the `oidc_user_getter` function.""" +import pytest +from rest_framework_simplejwt.exceptions import InvalidToken +from rest_framework_simplejwt.tokens import AccessToken + +from core import factories, models + +pytestmark = pytest.mark.django_db + + +def test_models_oidc_user_getter_exsting_user_no_email(django_assert_num_queries): + """ + When a valid token is passed, an existing user matching the token's sub should be returned. + """ + identity = factories.IdentityFactory() + factories.IdentityFactory(user=identity.user) # another identity for the user + token = AccessToken() + token["sub"] = str(identity.sub) + + with django_assert_num_queries(1): + user = models.oidc_user_getter(token) + + identity.refresh_from_db() + assert user == identity.user + + +def test_models_oidc_user_getter_existing_user_with_email(django_assert_num_queries): + """ + When the valid token passed contains an email and targets an existing user, + it should update the email on the identity but not on the user. + """ + identity = factories.IdentityFactory() + factories.IdentityFactory(user=identity.user) # another identity for the user + user_email = identity.user.email + assert models.User.objects.count() == 1 + + token = AccessToken() + token["sub"] = str(identity.sub) + + # Only 1 query if the email has not changed + token["email"] = identity.email + with django_assert_num_queries(1): + user = models.oidc_user_getter(token) + + # Additional update query if the email has changed + new_email = "people@example.com" + token["email"] = new_email + with django_assert_num_queries(2): + user = models.oidc_user_getter(token) + + identity.refresh_from_db() + assert identity.email == new_email + + assert models.User.objects.count() == 1 + assert user == identity.user + assert user.email == user_email + + +def test_models_oidc_user_getter_new_user_no_email(): + """ + When a valid token is passed, a user should be created if the sub + does not match any existing user. + """ + token = AccessToken() + token["sub"] = "123" + + user = models.oidc_user_getter(token) + + identity = user.identities.get() + assert identity.sub == "123" + assert identity.email is None + + assert user.email is None + assert user.password == "!" + assert models.User.objects.count() == 1 + + +def test_models_oidc_user_getter_new_user_with_email(): + """ + When the valid token passed contains an email and a new user is created, + the email should be set on the user and on the identity. + """ + email = "people@example.com" + token = AccessToken() + token["sub"] = "123" + token["email"] = email + + user = models.oidc_user_getter(token) + + identity = user.identities.get() + assert identity.sub == "123" + assert identity.email == email + + assert user.email == email + assert models.User.objects.count() == 1 + + +def test_models_oidc_user_getter_invalid_token(django_assert_num_queries): + """The token passed in argument should contain the configured user id claim.""" + token = AccessToken() + + with django_assert_num_queries(0), pytest.raises( + InvalidToken, match="Token contained no recognizable user identification" + ): + models.oidc_user_getter(token) + + assert models.User.objects.exists() is False diff --git a/src/backend/core/tests/test_models_users.py b/src/backend/core/tests/test_models_users.py index 176a36a..107b32d 100644 --- a/src/backend/core/tests/test_models_users.py +++ b/src/backend/core/tests/test_models_users.py @@ -29,6 +29,23 @@ def test_models_users_id_unique(): factories.UserFactory(id=user.id) +def test_models_users_email_unique(): + """The "email" field should be unique except for the null value.""" + user = factories.UserFactory() + with pytest.raises( + ValidationError, match="User with this Email address already exists." + ): + factories.UserFactory(email=user.email) + + +def test_models_users_email_several_null(): + """Several users with a null value for the "email" field can co-exist.""" + factories.UserFactory(email=None) + factories.UserFactory(email=None) + + assert models.User.objects.filter(email__isnull=True).count() == 2 + + def test_models_users_profile_not_owned(): """A user cannot declare as profile a contact that not is owned.""" user = factories.UserFactory() diff --git a/src/backend/core/tests/utils.py b/src/backend/core/tests/utils.py index 76cc381..cd5ed56 100644 --- a/src/backend/core/tests/utils.py +++ b/src/backend/core/tests/utils.py @@ -7,8 +7,10 @@ class OIDCToken(AccessToken): @classmethod def for_user(cls, user): - token = super().for_user(user) + """Returns an authorization token for the given user for testing.""" identity = user.identities.filter(is_main=True).first() + + token = cls() token["first_name"] = ( user.profile_contact.short_name if user.profile_contact else "David" ) @@ -17,5 +19,7 @@ class OIDCToken(AccessToken): if user.profile_contact else "Bowman" ) - token["email"] = identity.email + token["sub"] = str(identity.sub) + token["email"] = user.email + return token diff --git a/src/backend/core/tokens.py b/src/backend/core/tokens.py new file mode 100644 index 0000000..0867868 --- /dev/null +++ b/src/backend/core/tokens.py @@ -0,0 +1,10 @@ +"""Tokens for Magnify's core app.""" +from rest_framework_simplejwt.settings import api_settings +from rest_framework_simplejwt.tokens import Token + + +class BearerToken(Token): + """Bearer token as emitted by Keycloak OIDC for example.""" + + token_type = "Bearer" # noqa: S105 + lifetime = api_settings.ACCESS_TOKEN_LIFETIME diff --git a/src/backend/people/settings.py b/src/backend/people/settings.py index 91b3a1a..7dfb4a6 100755 --- a/src/backend/people/settings.py +++ b/src/backend/people/settings.py @@ -239,17 +239,28 @@ class Base(Configuration): "REDOC_DIST": "SIDECAR", } + # Simple JWT SIMPLE_JWT = { - "ALGORITHM": values.Value("HS256", environ_name="JWT_ALGORITHM"), - "SIGNING_KEY": values.SecretValue( - environ_name="JWT_PRIVATE_SIGNING_KEY", + "ALGORITHM": values.Value( + "RS256", environ_name="SIMPLE_JWT_ALGORITHM", environ_prefix=None + ), + "JWK_URL": values.Value( + None, environ_name="SIMPLE_JWT_JWK_URL", environ_prefix=None + ), + "SIGNING_KEY": values.Value( + None, environ_name="SIMPLE_JWT_SIGNING_KEY", environ_prefix=None + ), + "VERIFYING_KEY": values.Value( + None, environ_name="SIMPLE_JWT_VERIFYING_KEY", environ_prefix=None ), "AUTH_HEADER_TYPES": ("Bearer",), "AUTH_HEADER_NAME": "HTTP_AUTHORIZATION", - "USER_ID_FIELD": "id", + "TOKEN_TYPE_CLAIM": "typ", + "USER_ID_FIELD": "sub", "USER_ID_CLAIM": "sub", - "AUTH_TOKEN_CLASSES": ("rest_framework_simplejwt.tokens.AccessToken",), + "AUTH_TOKEN_CLASSES": ("core.tokens.BearerToken",), } + JWT_USER_GETTER = values.Value( "core.models.oidc_user_getter", environ_name="PEOPLE_JWT_USER_GETTER", @@ -380,6 +391,10 @@ class Development(Base): class Test(Base): """Test environment settings""" + SIMPLE_JWT = { + "USER_ID_FIELD": "sub", + "USER_ID_CLAIM": "sub", + } LOGGING = values.DictValue( { "version": 1, @@ -413,10 +428,6 @@ class Test(Base): CELERY_TASK_ALWAYS_EAGER = values.BooleanValue(True) - def __init__(self): - # pylint: disable=invalid-name - self.INSTALLED_APPS += ["drf_spectacular_sidecar"] - class ContinuousIntegration(Test): """ diff --git a/src/backend/pyproject.toml b/src/backend/pyproject.toml index 29fde6b..088aece 100644 --- a/src/backend/pyproject.toml +++ b/src/backend/pyproject.toml @@ -35,7 +35,7 @@ dependencies = [ "django-storages==1.14.2", "django-timezone-field>=5.1", "django==5.0", - "djangorestframework-simplejwt==5.3.0", + "djangorestframework-simplejwt[crypto]==5.3.0", "djangorestframework==3.14.0", "drf_spectacular==0.26.5", "dockerflow==2022.8.0",