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..1daebbe0 100644 --- a/profiles/schema.py +++ b/profiles/schema.py @@ -1,4 +1,5 @@ import logging +from collections.abc import Iterable 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 _get_login_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._get_login_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._get_login_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 + )