diff --git a/.coverage b/.coverage deleted file mode 100644 index 117fc844..00000000 Binary files a/.coverage and /dev/null differ diff --git a/.env.example b/.env.example index 265942af..60f344c1 100644 --- a/.env.example +++ b/.env.example @@ -15,3 +15,4 @@ POSTGRES_PASSWORD=password OIDC_RP_CLIENT_PRIVATE_KEY="MYSUPERSECRETPRIVATEKEY" OIDC_RP_CLIENT_ID="lcrc" OIDC_OP_FQDN="https://example.com" +NHS_LOGIN_SETTINGS_URL="https://settings.example.com" diff --git a/lung_cancer_screening/core/jinja2/layout.jinja b/lung_cancer_screening/core/jinja2/layout.jinja index 0268f944..f26fbdc8 100644 --- a/lung_cancer_screening/core/jinja2/layout.jinja +++ b/lung_cancer_screening/core/jinja2/layout.jinja @@ -18,11 +18,16 @@ }, "account": { "items": [ + { + "text": request.user.full_name, + "href": NHS_LOGIN_SETTINGS_URL, + "icon": True + }, { "text": "Log out", "href": url("questions:logout") } - ] + ] if request.user.is_authenticated else [] } }) }} {% endblock header %} diff --git a/lung_cancer_screening/jinja2_env.py b/lung_cancer_screening/jinja2_env.py index 9df59df2..5f021072 100644 --- a/lung_cancer_screening/jinja2_env.py +++ b/lung_cancer_screening/jinja2_env.py @@ -26,7 +26,12 @@ def environment(**options): ) env.globals.update( - {"static": static, "url": reverse, "STATIC_URL": settings.STATIC_URL} + { + "static": static, + "url": reverse, + "STATIC_URL": settings.STATIC_URL, + "NHS_LOGIN_SETTINGS_URL": settings.NHS_LOGIN_SETTINGS_URL, + } ) env.filters.update( diff --git a/lung_cancer_screening/questions/auth.py b/lung_cancer_screening/questions/auth.py index 9c753ab2..98d29b59 100644 --- a/lung_cancer_screening/questions/auth.py +++ b/lung_cancer_screening/questions/auth.py @@ -39,16 +39,36 @@ def create_user(self, claims): raise ValueError("Missing 'nhs_number' claim in OIDC token") email = claims.get('email') + given_name = claims.get('given_name') + family_name = claims.get('family_name') return user_class.objects.create_user( nhs_number=nhs_number, - email=email + email=email, + given_name=given_name, + family_name=family_name, ) def update_user(self, user, claims): + changed = False email = claims.get('email') + given_name = claims.get("given_name") + family_name = claims.get("family_name") + if email and user.email != email: user.email = email + changed = True + + if given_name and user.given_name != given_name: + user.given_name = given_name + changed = True + + if family_name and user.family_name != family_name: + user.family_name = family_name + changed = True + + if changed: user.save() + return user def _create_client_assertion(self): diff --git a/lung_cancer_screening/questions/migrations/0054_add_given_name_and_family_name_to_user.py b/lung_cancer_screening/questions/migrations/0054_add_given_name_and_family_name_to_user.py new file mode 100644 index 00000000..9cffd303 --- /dev/null +++ b/lung_cancer_screening/questions/migrations/0054_add_given_name_and_family_name_to_user.py @@ -0,0 +1,23 @@ +# Generated by Django 5.2.10 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('questions', '0053_smokedamountresponse'), + ] + + operations = [ + migrations.AddField( + model_name='user', + name='given_name', + field=models.CharField(blank=False, null=False, max_length=255), + ), + migrations.AddField( + model_name='user', + name='family_name', + field=models.CharField(blank=False, null=False, max_length=255), + ), + ] diff --git a/lung_cancer_screening/questions/models/user.py b/lung_cancer_screening/questions/models/user.py index 84b43206..36957f2e 100644 --- a/lung_cancer_screening/questions/models/user.py +++ b/lung_cancer_screening/questions/models/user.py @@ -19,6 +19,8 @@ def create_user(self, nhs_number, **extra_fields): class User(AbstractBaseUser): nhs_number = models.CharField(max_length=10, unique=True) + given_name = models.CharField(max_length=255, blank=False, null=False) + family_name = models.CharField(max_length=255, blank=False, null=False) email = models.EmailField(blank=True, null=True) created_at = models.DateTimeField(auto_now_add=True) updated_at = models.DateTimeField(auto_now=True) @@ -46,3 +48,7 @@ def has_recently_submitted_responses(self, excluding=None): def most_recent_response_set(self): return self.responseset_set.order_by('-submitted_at').first() + + @property + def full_name(self): + return f"{self.given_name} {self.family_name}" diff --git a/lung_cancer_screening/questions/tests/factories/user_factory.py b/lung_cancer_screening/questions/tests/factories/user_factory.py index fa8495c9..57c0c54c 100644 --- a/lung_cancer_screening/questions/tests/factories/user_factory.py +++ b/lung_cancer_screening/questions/tests/factories/user_factory.py @@ -9,3 +9,5 @@ class Meta: nhs_number = factory.Sequence(lambda n: f"9{str(n).zfill(9)}") password = factory.django.Password(None) + given_name = factory.Faker("first_name") + family_name = factory.Faker("last_name") diff --git a/lung_cancer_screening/questions/tests/unit/models/test_user.py b/lung_cancer_screening/questions/tests/unit/models/test_user.py index 7463d47c..b23c03fd 100644 --- a/lung_cancer_screening/questions/tests/unit/models/test_user.py +++ b/lung_cancer_screening/questions/tests/unit/models/test_user.py @@ -79,6 +79,29 @@ def test_raises_a_validation_error_if_nhs_number_is_duplicate(self): ) + def test_is_invalid_without_a_given_name(self): + self.user.given_name = None + + with self.assertRaises(ValidationError) as context: + self.user.full_clean() + + self.assertIn( + "This field cannot be null.", + context.exception.messages + ) + + + def test_is_invalid_without_a_family_name(self): + self.user.family_name = None + + with self.assertRaises(ValidationError) as context: + self.user.full_clean() + + self.assertIn( + "This field cannot be null.", + context.exception.messages[0] + ) + def test_has_recently_submitted_responses_returns_true_if_has_recently_submitted_response_set(self): ResponseSetFactory.create( user=self.user, @@ -93,3 +116,9 @@ def test_has_recently_submitted_responses_returns_false_if_has_no_recently_submi not_recently_submitted=True ) self.assertFalse(self.user.has_recently_submitted_responses()) + + + def test_full_name_returns_the_full_name(self): + self.user.given_name = "John" + self.user.family_name = "Doe" + self.assertEqual(self.user.full_name, "John Doe") diff --git a/lung_cancer_screening/questions/tests/unit/test_auth.py b/lung_cancer_screening/questions/tests/unit/test_auth.py index 7ac42315..ef4f19fe 100644 --- a/lung_cancer_screening/questions/tests/unit/test_auth.py +++ b/lung_cancer_screening/questions/tests/unit/test_auth.py @@ -6,6 +6,7 @@ from cryptography.hazmat.primitives import serialization from ...auth import NHSLoginOIDCBackend +from ...tests.factories.user_factory import UserFactory User = get_user_model() @@ -31,44 +32,45 @@ def setUp(self): encryption_algorithm=serialization.NoEncryption() ).decode('utf-8') + self.claims = { + "nhs_number": "1234567890", + "email": "test@example.com", + "given_name": "Jane", + "family_name": "Smith", + } + def test_filter_users_by_claims_for_existing_user(self): - user = User.objects.create_user(nhs_number='1234567890') + user = User.objects.create_user(**self.claims) - claims = {'nhs_number': '1234567890'} - result = self.backend.filter_users_by_claims(claims) + result = self.backend.filter_users_by_claims(self.claims) self.assertEqual(result.count(), 1) self.assertEqual(result.first(), user) def test_filter_users_by_claims_for_non_existent_user(self): - claims = {'nhs_number': '1111111111'} + claims = {**self.claims, "nhs_number": "1111111111"} result = self.backend.filter_users_by_claims(claims) self.assertEqual(result.count(), 0) def test_filter_users_by_claims_when_no_claim_is_provided(self): - claims = {} - result = self.backend.filter_users_by_claims(claims) + result = self.backend.filter_users_by_claims({}) self.assertEqual(result.count(), 0) def test_create_user_when_nhs_number_claim_is_provided(self): - claims = {'nhs_number': '1234567890'} + user = self.backend.create_user(self.claims) - user = self.backend.create_user(claims) + self.assertEqual(user.nhs_number, self.claims["nhs_number"]) + self.assertEqual(user.email, self.claims["email"]) - self.assertEqual(user.nhs_number, '1234567890') - def test_create_user_with_email_claim(self): - claims = { - 'nhs_number': '1234567890', - 'email': 'test@example.com' - } + def test_create_user_with_name_claims_sets_given_name_and_family_name(self): + user = self.backend.create_user(self.claims) - user = self.backend.create_user(claims) + self.assertEqual(user.given_name, 'Jane') + self.assertEqual(user.family_name, 'Smith') - self.assertEqual(user.nhs_number, '1234567890') - self.assertEqual(user.email, 'test@example.com') def test_create_user_without_nhs_number_raises_error(self): claims = {} @@ -80,7 +82,7 @@ def test_create_user_without_nhs_number_raises_error(self): def test_update_user_returns_user(self): - user = User.objects.create_user(nhs_number='1234567890') + user = UserFactory.create(nhs_number='1234567890') claims = {'nhs_number': '1234567890', 'email': 'test@example.com'} result = self.backend.update_user(user, claims) @@ -89,7 +91,7 @@ def test_update_user_returns_user(self): self.assertEqual(user.email, 'test@example.com') def test_update_user_updates_email_when_provided(self): - user = User.objects.create_user( + user = UserFactory.create( nhs_number='1234567890', email='old@example.com' ) @@ -102,7 +104,7 @@ def test_update_user_updates_email_when_provided(self): self.assertEqual(result, user) def test_update_user_does_not_update_email_when_not_provided(self): - user = User.objects.create_user( + user = UserFactory.create( nhs_number='1234567890', email='existing@example.com' ) @@ -114,6 +116,27 @@ def test_update_user_does_not_update_email_when_not_provided(self): self.assertEqual(user.email, 'existing@example.com') self.assertEqual(result, user) + + def test_update_user_updates_given_name_and_family_name_when_provided(self): + user = UserFactory.create( + nhs_number='1234567890', + given_name='Old', + family_name='Name', + ) + claims = { + 'nhs_number': '1234567890', + 'given_name': 'Jane', + 'family_name': 'Smith', + } + + result = self.backend.update_user(user, claims) + + user.refresh_from_db() + self.assertEqual(user.given_name, 'Jane') + self.assertEqual(user.family_name, 'Smith') + self.assertEqual(result, user) + + @patch('lung_cancer_screening.questions.auth.requests.post') def test_get_token_success(self, mock_post): settings.OIDC_RP_CLIENT_PRIVATE_KEY = self.test_private_key_pem diff --git a/lung_cancer_screening/settings.py b/lung_cancer_screening/settings.py index 59556710..36f1d1dd 100644 --- a/lung_cancer_screening/settings.py +++ b/lung_cancer_screening/settings.py @@ -233,8 +233,9 @@ def list_env(key): # NHS Login requires RS512 for token endpoint authentication # See: https://auth.sandpit.signin.nhs.uk/.well-known/openid-configuration OIDC_RP_SIGN_ALGO = "RS512" -OIDC_RP_SCOPES = "openid profile" +OIDC_RP_SCOPES = "openid profile profile_extended" OIDC_RP_REDIRECT_URI = "/oidc/callback/" +NHS_LOGIN_SETTINGS_URL = environ.get("NHS_LOGIN_SETTINGS_URL") # Authentication backends AUTHENTICATION_BACKENDS = [