From c61bf30c7148ea96053c5f63516bde47ef18913b Mon Sep 17 00:00:00 2001 From: Juha Louhiranta Date: Mon, 9 Dec 2024 21:57:24 +0200 Subject: [PATCH] feat: add ProfileNode.availableLoginMethods This extends the information already available from loginMethods field by adding ceatedAt and userLabel fields to the data. At the same time loginMethods field is deprecated since this new fields is meant to replace it. Refs: HP-2530 --- .../snapshots/snap_test_graphql_api_schema.py | 10 +- profiles/keycloak_integration.py | 35 ++++-- profiles/models.py | 1 + profiles/schema.py | 96 ++++++++++++---- profiles/tests/test_gql_my_profile_query.py | 57 ++++++++-- profiles/tests/test_keycloak_integration.py | 104 +++++++++++++++--- 6 files changed, 245 insertions(+), 58 deletions(-) diff --git a/open_city_profile/tests/snapshots/snap_test_graphql_api_schema.py b/open_city_profile/tests/snapshots/snap_test_graphql_api_schema.py index bb74fe34..846d20dc 100644 --- a/open_city_profile/tests/snapshots/snap_test_graphql_api_schema.py +++ b/open_city_profile/tests/snapshots/snap_test_graphql_api_schema.py @@ -4,6 +4,7 @@ from snapshottest import Snapshot + snapshots = Snapshot() snapshots['test_graphql_schema_matches_the_reference 1'] = '''type Query { @@ -122,7 +123,8 @@ phones(offset: Int, before: String, after: String, first: Int, last: Int): PhoneNodeConnection addresses(offset: Int, before: String, after: String, first: Int, last: Int): AddressNodeConnection contactMethod: ContactMethod - loginMethods: [LoginMethodType] + loginMethods: [LoginMethodType] @deprecated(reason: "This field is deprecated, use availableLoginMethods.") + availableLoginMethods: [LoginMethodNode] sensitivedata: SensitiveDataNode serviceConnections(offset: Int, before: String, after: String, first: Int, last: Int): ServiceConnectionTypeConnection verifiedPersonalInformation: VerifiedPersonalInformationNode @@ -222,6 +224,12 @@ SUOMI_FI } +type LoginMethodNode { + method: LoginMethodType! + createdAt: DateTime + userLabel: String +} + type SensitiveDataNode implements Node { id: ID! ssn: String! diff --git a/profiles/keycloak_integration.py b/profiles/keycloak_integration.py index 24e9d2a2..8fd3e490 100644 --- a/profiles/keycloak_integration.py +++ b/profiles/keycloak_integration.py @@ -1,3 +1,5 @@ +import datetime + from django.conf import settings from django.core.signals import setting_changed from django.dispatch import receiver @@ -105,27 +107,40 @@ def send_profile_changes_to_keycloak(instance): pass -def get_user_identity_providers(user_id) -> set[str]: +def get_user_identity_providers(user_id) -> list[dict]: if not _keycloak_admin_client: - return set() + return [] try: user_data = _keycloak_admin_client.get_user_federated_identities(user_id) - return {ip["identityProvider"] for ip in user_data} + return [{"method": user_data["identityProvider"]} for user_data in user_data] except keycloak.UserNotFoundError: - return set() + return [] -def get_user_credential_types(user_id) -> set[str]: +def get_user_credential_types(user_id) -> list[dict]: if not _keycloak_admin_client: - return set() + return [] try: user_data = _keycloak_admin_client.get_user_credentials(user_id) - return {cred["type"] for cred in user_data} + credentials = [] + for c in user_data: + created_at = ( + datetime.datetime.fromtimestamp(c["createdDate"] / 1000, datetime.UTC) + if c.get("createdDate") + else None + ) + credential = { + "method": c["type"], + "created_at": created_at, + "user_label": c.get("userLabel"), + } + credentials.append(credential) + return credentials except keycloak.UserNotFoundError: - return set() + return [] -def get_user_login_methods(user_id) -> set[str]: - return get_user_identity_providers(user_id) | get_user_credential_types(user_id) +def get_user_login_methods(user_id) -> list[dict]: + return get_user_identity_providers(user_id) + get_user_credential_types(user_id) diff --git a/profiles/models.py b/profiles/models.py index b2acfa11..f4c1daf0 100644 --- a/profiles/models.py +++ b/profiles/models.py @@ -121,6 +121,7 @@ class Meta: "language", "contact_method", "login_methods", + "available_login_methods", ] def resolve_profile(self): diff --git a/profiles/schema.py b/profiles/schema.py index 17b619c4..9e1f3e43 100644 --- a/profiles/schema.py +++ b/profiles/schema.py @@ -1,4 +1,5 @@ import logging +from collections.abc import Iterable, Set from itertools import chain import django.dispatch @@ -57,7 +58,10 @@ download_connected_service_data, ) from .enums import AddressType, EmailType, LoginMethodType, PhoneType -from .keycloak_integration import delete_profile_from_keycloak, get_user_login_methods +from .keycloak_integration import ( + delete_profile_from_keycloak, + get_user_login_methods, +) from .models import ( Address, ClaimToken, @@ -92,7 +96,9 @@ AllowedAddressType = graphene.Enum.from_enum( AddressType, description=lambda e: e.label if e else "" ) - +LoginMethodTypeEnum = graphene.Enum.from_enum( + LoginMethodType, description=lambda e: e.label if e else "" +) """Provides the updated Profile instance as a keyword argument called `instance`.""" profile_updated = django.dispatch.Signal() @@ -438,6 +444,16 @@ def __resolve_reference(self, info, **kwargs): ) +class LoginMethodNode(graphene.ObjectType): + method = LoginMethodTypeEnum(required=True, description="The login method used.") + created_at = graphene.DateTime( + description="Time when the login method was created or edited." + ) + user_label = graphene.String( + description="User-friendly label for the login method." + ) + + class VerifiedPersonalInformationAddressNode(graphene.ObjectType): street_address = graphene.String( required=True, description="Street address with possible house number etc." @@ -585,9 +601,13 @@ class Meta: filterset_class = ProfileFilter login_methods = graphene.List( - graphene.Enum.from_enum( - LoginMethodType, description=lambda e: e.label if e else "" - ), + LoginMethodTypeEnum, + description="List of login methods that the profile has used to authenticate. " + "Only visible to the user themselves.", + deprecation_reason="This field is deprecated, use availableLoginMethods.", + ) + available_login_methods = graphene.List( + LoginMethodNode, description="List of login methods that the profile has used to authenticate. " "Only visible to the user themselves.", ) @@ -605,6 +625,42 @@ class Meta: "privileges to access this information.", ) + @staticmethod + def _has_correct_amr_claim(amr: Set): + """ + For future software archeologists: + This field was added to the API to support the front-end's need to know + which login methods the user has used. It's only needed for profiles + with helsinki-tunnus or Suomi.fi, so for other cases, save a couple + API calls and return an empty list. There's no other reasoning for the + logic here. + Can remove this after Tunnistamo is no longer in use. Related ticket: HP-2495 + """ + return amr.intersection({"helsinki_tunnus", "heltunnistussuomifi", "suomi_fi"}) + + @staticmethod + def _filter_loging_methods(user_uuid, extended=False) -> Iterable: + login_methods = get_user_login_methods(user_uuid) + + login_methods_in_enum = [ + val + for val in login_methods + if val["method"] in enum_values(LoginMethodType) + ] + + if unknown_login_methods := set([val["method"] for val in login_methods]) - set( + val["method"] for val in login_methods_in_enum + ): + logger.warning( + "Found login methods which are not part of the LoginMethodType enum: %s", # noqa: E501 + unknown_login_methods, + ) + + if extended: + return login_methods_in_enum + else: + return {val["method"] for val in login_methods_in_enum} + def resolve_login_methods(self: Profile, info, **kwargs): if info.context.user != self.user: raise PermissionDenied( @@ -613,26 +669,20 @@ def resolve_login_methods(self: Profile, info, **kwargs): amr = set(force_list(info.context.user_auth.data.get("amr"))) - # For future software archeologists: - # This field was added to the API to support the front-end's need to know - # which login methods the user has used. It's only needed for profiles - # with helsinki-tunnus or Suomi.fi, so for other cases, save a couple - # API calls and return an empty list. There's no other reasoning for the - # logic here. - # Can remove this after Tunnistamo is no longer in use. Related ticket: HP-2495 - if amr.intersection({"helsinki_tunnus", "heltunnistussuomifi", "suomi_fi"}): - login_methods = get_user_login_methods(self.user.uuid) - login_methods_in_enum = { - val for val in login_methods if val in enum_values(LoginMethodType) - } - if unknown_login_methods := login_methods - login_methods_in_enum: - logger.warning( - "Found login methods which are not part of the LoginMethodType enum: %s", # noqa: E501 - unknown_login_methods, - ) + if ProfileNode._has_correct_amr_claim(amr): + return ProfileNode._filter_loging_methods(self.user.uuid, extended=False) + return [] - return login_methods_in_enum + def resolve_available_login_methods(self: Profile, info, **kwargs): + if info.context.user != self.user: + raise PermissionDenied( + "No permission to read login methods of another user." + ) + + amr = set(force_list(info.context.user_auth.data.get("amr"))) + if ProfileNode._has_correct_amr_claim(amr): + return ProfileNode._filter_loging_methods(self.user.uuid, extended=True) return [] def resolve_service_connections(self: Profile, info, **kwargs): diff --git a/profiles/tests/test_gql_my_profile_query.py b/profiles/tests/test_gql_my_profile_query.py index a43121c7..1563fac4 100644 --- a/profiles/tests/test_gql_my_profile_query.py +++ b/profiles/tests/test_gql_my_profile_query.py @@ -1,3 +1,5 @@ +import datetime + import pytest from open_city_profile.tests import to_graphql_name @@ -620,11 +622,21 @@ def test_my_profile_checks_allowed_data_fields_for_multiple_queries( def test_user_can_see_own_login_methods_with_correct_amr_claim( user_gql_client, profile, group, service, monkeypatch, amr_claim_value ): - def mock_return(*_, **__): - return {"suomi_fi", "password"} - monkeypatch.setattr( - "profiles.keycloak_integration.get_user_identity_providers", mock_return + "profiles.keycloak_integration.get_user_credential_types", + lambda _: [ + { + "created_at": datetime.datetime( + 2024, 12, 5, 11, 54, 21, 491000, tzinfo=datetime.UTC + ), + "method": "password", + "user_label": None, + }, + ], + ) + monkeypatch.setattr( + "profiles.keycloak_integration.get_user_identity_providers", + lambda _: [{"method": "suomi_fi"}], ) profile = ProfileFactory(user=user_gql_client.user) @@ -634,6 +646,11 @@ def mock_return(*_, **__): { myProfile { loginMethods + availableLoginMethods { + method + createdAt + userLabel + } } } """ @@ -641,10 +658,22 @@ def mock_return(*_, **__): query, auth_token_payload={"amr": amr_claim_value}, service=service ) assert "errors" not in executed - assert set(executed["data"]["myProfile"]["loginMethods"]) == { + assert executed["data"]["myProfile"]["loginMethods"] == [ "SUOMI_FI", "PASSWORD", - } + ] + assert executed["data"]["myProfile"]["availableLoginMethods"] == [ + { + "createdAt": None, + "method": "SUOMI_FI", + "userLabel": None, + }, + { + "createdAt": "2024-12-05T11:54:21.491000+00:00", + "method": "PASSWORD", + "userLabel": None, + }, + ] @pytest.mark.parametrize("amr_claim_value", [None, "helsinkiad"]) @@ -652,7 +681,7 @@ def test_user_cannot_see_own_login_methods_with_other_amr_claims( user_gql_client, profile, group, service, monkeypatch, amr_claim_value ): def mock_return(*_, **__): - return {"this should not show up"} + raise Exception("This should not be called") monkeypatch.setattr( "profiles.keycloak_integration.get_user_identity_providers", mock_return @@ -665,6 +694,9 @@ def mock_return(*_, **__): { myProfile { loginMethods + availableLoginMethods { + method + } } } """ @@ -673,13 +705,14 @@ def mock_return(*_, **__): ) assert "errors" not in executed assert executed["data"]["myProfile"]["loginMethods"] == [] + assert executed["data"]["myProfile"]["availableLoginMethods"] == [] def test_user_does_not_see_non_enum_login_methods( user_gql_client, profile, group, service, monkeypatch ): def mock_return(*_, **__): - return {"password", "this should not show up"} + return [{"method": "password"}, {"method": "this should not show up"}] monkeypatch.setattr( "profiles.keycloak_integration.get_user_identity_providers", mock_return @@ -692,6 +725,9 @@ def mock_return(*_, **__): { myProfile { loginMethods + availableLoginMethods { + method + } } } """ @@ -699,4 +735,7 @@ def mock_return(*_, **__): query, auth_token_payload={"amr": "helsinki_tunnus"}, service=service ) assert "errors" not in executed - assert set(executed["data"]["myProfile"]["loginMethods"]) == {"PASSWORD"} + assert executed["data"]["myProfile"]["loginMethods"] == ["PASSWORD"] + assert executed["data"]["myProfile"]["availableLoginMethods"] == [ + {"method": "PASSWORD"} + ] diff --git a/profiles/tests/test_keycloak_integration.py b/profiles/tests/test_keycloak_integration.py index 97506011..ab9b2874 100644 --- a/profiles/tests/test_keycloak_integration.py +++ b/profiles/tests/test_keycloak_integration.py @@ -1,3 +1,4 @@ +import datetime from unittest.mock import MagicMock import pytest @@ -9,6 +10,37 @@ ) from utils.keycloak import UserNotFoundError +RECOVERY_CODE_METHOD = { + "id": "ed913eb9-6243-45bb-b3f0-66eff3e9235e", + "type": "recovery-authn-codes", + "userLabel": "Recovery codes", + "createdDate": 1716894380239, + "credentialData": '{"hashIterations":1,"algorithm":"RS512","totalCodes":12,"remainingCodes":12}', +} +OTP_METHOD = { + "id": "d48ec74d-7c98-4810-b6ad-69022ce94bee", + "type": "otp", + "userLabel": "Phone", + "createdDate": 1716891252633, + "credentialData": '{"subType":"totp","digits":6,"counter":0,"period":30,"algorithm":"HmacSHA1"}', +} +PASSWORD_METHOD = { + "id": "b437745e-d17d-415c-a749-637833e87ff0", + "type": "password", + "createdDate": 1733399661491, + "credentialData": '{"hashIterations":100000,"algorithm":"pbkdf2-sha256","additionalParameters":{}}', +} +SUOMI_FI_PROVIDER = { + "identityProvider": "suomi_fi", + "userId": "e29a380628e06d3e0e903b8fb245f1910bceee063cda47c27df1f976dc60aa9b", + "userName": "e29a380628e06d3e0e903b8fb245f1910bceee063cda47c27df1f976dc60aa9b", +} +HELSINKI_AD_PROVIDER = { + "identityProvider": "helsinkiad", + "userId": "df6c34b9-22e6-49e7-8c2b-211c5267a84e", + "userName": "test@example.com", +} + @pytest.fixture def mock_keycloak_admin_client(monkeypatch): @@ -22,17 +54,17 @@ def test_get_user_identity_providers_no_client(monkeypatch): """Test the function when _keycloak_admin_client is None.""" monkeypatch.setattr("profiles.keycloak_integration._keycloak_admin_client", None) - assert get_user_identity_providers("dummy_user_id") == set() + assert get_user_identity_providers("dummy_user_id") == [] @pytest.mark.parametrize( "mock_return_value, expected_result", [ - ([], set()), - ([{"identityProvider": "helsinkiad"}], {"helsinkiad"}), + ([], []), + ([HELSINKI_AD_PROVIDER], [{"method": "helsinkiad"}]), ( - [{"identityProvider": "helsinkiad"}, {"identityProvider": "suomi_fi"}], - {"helsinkiad", "suomi_fi"}, + [HELSINKI_AD_PROVIDER, SUOMI_FI_PROVIDER], + [{"method": "helsinkiad"}, {"method": "suomi_fi"}], ), ], ) @@ -52,14 +84,14 @@ def test_get_user_identity_providers_user_not_found(mock_keycloak_admin_client): UserNotFoundError ) - assert get_user_identity_providers("dummy_user_id") == set() + assert get_user_identity_providers("dummy_user_id") == [] def test_get_user_federated_identities_no_identities(mock_keycloak_admin_client): """Test the function when the user has no federated identities.""" mock_keycloak_admin_client.get_user_federated_identities.return_value = set() - assert get_user_identity_providers("dummy_user_id") == set() + assert get_user_identity_providers("dummy_user_id") == [] def test_get_user_identity_providers_exception(mock_keycloak_admin_client): @@ -74,15 +106,44 @@ def test_get_user_identity_providers_exception(mock_keycloak_admin_client): def test_get_user_credential_types_no_client(monkeypatch): monkeypatch.setattr("profiles.keycloak_integration._keycloak_admin_client", None) - assert get_user_credential_types("dummy_user_id") == set() + assert get_user_credential_types("dummy_user_id") == [] @pytest.mark.parametrize( "mock_return_value, expected_result", [ - ([], set()), - ([{"type": "password"}], {"password"}), - ([{"type": "password"}, {"type": "otp"}], {"password", "otp"}), + ([], []), + ( + [PASSWORD_METHOD], + [ + { + "created_at": datetime.datetime( + 2024, 12, 5, 11, 54, 21, 491000, tzinfo=datetime.UTC + ), + "method": "password", + "user_label": None, + } + ], + ), + ( + [PASSWORD_METHOD, OTP_METHOD], + [ + { + "created_at": datetime.datetime( + 2024, 12, 5, 11, 54, 21, 491000, tzinfo=datetime.UTC + ), + "method": "password", + "user_label": None, + }, + { + "created_at": datetime.datetime( + 2024, 5, 28, 10, 14, 12, 633000, tzinfo=datetime.UTC + ), + "method": "otp", + "user_label": "Phone", + }, + ], + ), ], ) def test_get_user_credential_types_with_data( @@ -96,7 +157,7 @@ def test_get_user_credential_types_with_data( def test_get_user_credential_types_user_not_found(mock_keycloak_admin_client): mock_keycloak_admin_client.get_user_credentials.side_effect = UserNotFoundError - assert get_user_credential_types("dummy_user_id") == set() + assert get_user_credential_types("dummy_user_id") == [] def test_get_user_credential_types_exception(mock_keycloak_admin_client): @@ -108,13 +169,26 @@ def test_get_user_credential_types_exception(mock_keycloak_admin_client): # Tests for get_user_login_methods def test_get_user_login_methods(monkeypatch): + expected_credentials = [ + { + "created_at": datetime.datetime( + 2024, 12, 5, 11, 54, 21, 491000, tzinfo=datetime.UTC + ), + "method": "password", + "user_label": None, + }, + ] + expected_idps = [{"method": "suomi_fi"}] + monkeypatch.setattr( "profiles.keycloak_integration.get_user_credential_types", - lambda _: {"password"}, + lambda _: expected_credentials, ) monkeypatch.setattr( "profiles.keycloak_integration.get_user_identity_providers", - lambda _: {"provider1"}, + lambda _: expected_idps, ) - assert get_user_login_methods("dummy_user_id") == {"password", "provider1"} + assert ( + get_user_login_methods("dummy_user_id") == expected_idps + expected_credentials + )