From da711f1205c51a20e8849fd29e8d9c05f9d9c00a Mon Sep 17 00:00:00 2001 From: Nico Virkki Date: Mon, 29 Apr 2024 09:10:17 +0300 Subject: [PATCH 1/3] feat: add FieldNotAllowedError Refs HP-2319 --- open_city_profile/consts.py | 1 + open_city_profile/exceptions.py | 14 ++++++++++++++ open_city_profile/views.py | 8 ++++++++ 3 files changed, 23 insertions(+) diff --git a/open_city_profile/consts.py b/open_city_profile/consts.py index ff535cf3..89ed4411 100644 --- a/open_city_profile/consts.py +++ b/open_city_profile/consts.py @@ -23,6 +23,7 @@ PROFILE_ALREADY_EXISTS_FOR_USER_ERROR = "PROFILE_ALREADY_EXISTS_FOR_USER_ERROR" PROFILE_MUST_HAVE_PRIMARY_EMAIL = "PROFILE_MUST_HAVE_PRIMARY_EMAIL" INSUFFICIENT_LOA_ERROR = "INSUFFICIENT_LOA_ERROR" +FIELD_NOT_ALLOWED_ERROR = "FIELD_NOT_ALLOWED_ERROR" # Service specific GDPR errors SERVICE_GDPR_API_REQUEST_ERROR = "SERVICE_GDPR_API_REQUEST_ERROR" diff --git a/open_city_profile/exceptions.py b/open_city_profile/exceptions.py index 766209d9..420e70ca 100644 --- a/open_city_profile/exceptions.py +++ b/open_city_profile/exceptions.py @@ -73,3 +73,17 @@ class TokenExchangeError(Exception): class InsufficientLoaError(ProfileGraphQLError): """The requester has insufficient level of authentication to retrieve this data""" + + +class FieldNotAllowedError(ProfileGraphQLError): + """The field is not allowed for the service. + + Field does not exist in the service's allowed data fields (Service.allowed_data_fields). + """ + + field_name: str = None + + def __init__(self, *args, field_name=None, **kwargs): + super().__init__(*args, **kwargs) + + self.field_name = field_name diff --git a/open_city_profile/views.py b/open_city_profile/views.py index e68398d4..d9682cfa 100644 --- a/open_city_profile/views.py +++ b/open_city_profile/views.py @@ -13,6 +13,7 @@ CONNECTED_SERVICE_DELETION_FAILED_ERROR, CONNECTED_SERVICE_DELETION_NOT_ALLOWED_ERROR, DATA_CONFLICT_ERROR, + FIELD_NOT_ALLOWED_ERROR, GENERAL_ERROR, INSUFFICIENT_LOA_ERROR, INVALID_EMAIL_FORMAT_ERROR, @@ -35,6 +36,7 @@ ConnectedServiceDeletionFailedError, ConnectedServiceDeletionNotAllowedError, DataConflictError, + FieldNotAllowedError, InsufficientLoaError, InvalidEmailFormatError, MissingGDPRApiTokenError, @@ -75,6 +77,7 @@ ServiceConnectionDoesNotExist: SERVICE_CONNECTION_DOES_NOT_EXIST_ERROR, ServiceNotIdentifiedError: SERVICE_NOT_IDENTIFIED_ERROR, InsufficientLoaError: INSUFFICIENT_LOA_ERROR, + FieldNotAllowedError: FIELD_NOT_ALLOWED_ERROR, } sentry_ignored_errors = ( @@ -177,4 +180,9 @@ def format_error(error): if "code" not in formatted_error["extensions"]: formatted_error["extensions"]["code"] = error_code + if error_code == FIELD_NOT_ALLOWED_ERROR: + formatted_error["extensions"]["field_name"] = getattr( + error.original_error, "field_name", "" + ) + return formatted_error From 457a90c1e4282009b0cc7205ee539b9a69d0bfa3 Mon Sep 17 00:00:00 2001 From: Nico Virkki Date: Mon, 29 Apr 2024 09:12:21 +0300 Subject: [PATCH 2/3] feat: add middleware for checking allowed data fields When calling profile any kind there should be checked to which fields the service has access rights by using the allowed data fields. This adds middleware and mixin class for Profile model and VerifiedPersonalInfo models for checking that the queried fields are allowed for the service. Refs HP-2319 --- open_city_profile/graphene.py | 19 ++ open_city_profile/settings.py | 1 + open_city_profile/tests/conftest.py | 15 +- .../tests/graphql_test_helpers.py | 20 +- profiles/models.py | 76 +++++- .../tests/test_gql_claim_profile_mutation.py | 56 +++- .../tests/test_gql_claimable_profile_query.py | 2 +- .../test_gql_create_my_profile_mutation.py | 42 ++- ..._verified_personal_information_mutation.py | 2 +- ..._create_or_update_user_profile_mutation.py | 4 +- .../tests/test_gql_create_profile_mutation.py | 9 +- profiles/tests/test_gql_federation.py | 7 +- profiles/tests/test_gql_my_profile_query.py | 245 ++++++++++++++---- profiles/tests/test_gql_profile_query.py | 97 ++++++- ...est_gql_profile_with_access_token_query.py | 4 +- profiles/tests/test_gql_profiles_query.py | 53 +++- profiles/tests/test_gql_relay_ordering.py | 30 ++- .../test_gql_update_my_profile_mutation.py | 101 +++++++- .../tests/test_gql_update_profile_mutation.py | 29 ++- .../test_profiles_graphql_authentication.py | 3 + 20 files changed, 721 insertions(+), 94 deletions(-) diff --git a/open_city_profile/graphene.py b/open_city_profile/graphene.py index 6bad7d7a..27b4c2dc 100644 --- a/open_city_profile/graphene.py +++ b/open_city_profile/graphene.py @@ -5,12 +5,14 @@ from django.conf import settings from django.forms import MultipleChoiceField from django_filters import MultipleChoiceFilter +from graphene.utils.str_converters import to_snake_case from graphene_django import DjangoObjectType from graphene_django.forms.converter import convert_form_field from graphene_django.types import ALL_FIELDS from graphql_sync_dataloaders import SyncDataLoader from parler.models import TranslatableModel +from open_city_profile.exceptions import FieldNotAllowedError, ServiceNotIdentifiedError from profiles.loaders import ( addresses_by_profile_id_loader, emails_by_profile_id_loader, @@ -178,3 +180,20 @@ def __init_subclass_with_meta__( _meta=_meta, **options, ) + + +class AllowedDataFieldsMiddleware: + + def resolve(self, next, root, info, **kwargs): + if getattr(root, "check_allowed_data_fields", False): + field_name = to_snake_case(getattr(info, "field_name", "")) + + if not getattr(info.context, "service", False): + raise ServiceNotIdentifiedError("Service not identified") + + if not root.is_field_allowed_for_service(field_name, info.context.service): + raise FieldNotAllowedError( + "Field is not allowed for service.", field_name=field_name + ) + + return next(root, info, **kwargs) diff --git a/open_city_profile/settings.py b/open_city_profile/settings.py index b71615e2..96f7cbc5 100644 --- a/open_city_profile/settings.py +++ b/open_city_profile/settings.py @@ -288,6 +288,7 @@ "SCHEMA": "open_city_profile.schema.schema", "MIDDLEWARE": [ # NOTE: Graphene runs its middlewares in reverse order! + "open_city_profile.graphene.AllowedDataFieldsMiddleware", "open_city_profile.graphene.JWTMiddleware", "open_city_profile.graphene.GQLDataLoaders", ], diff --git a/open_city_profile/tests/conftest.py b/open_city_profile/tests/conftest.py index b3849328..d7f81563 100644 --- a/open_city_profile/tests/conftest.py +++ b/open_city_profile/tests/conftest.py @@ -22,7 +22,8 @@ UserFactory, ) from open_city_profile.views import GraphQLView -from services.tests.factories import ServiceFactory +from services.models import Service +from services.tests.factories import AllowedDataFieldFactory, ServiceFactory _not_provided = object() @@ -34,6 +35,7 @@ def execute( auth_token_payload=None, service=_not_provided, context=None, + allowed_data_fields: list[str] = None, **kwargs, ): """ @@ -63,9 +65,18 @@ def execute( context.service = None if service is _not_provided: - context.service = ServiceFactory(name="profile", is_profile_service=True) + service = Service.objects.filter(is_profile_service=True).first() + if not service: + service = ServiceFactory(name="profile", is_profile_service=True) + + context.service = service elif service: context.service = service + if allowed_data_fields: + for field_name in allowed_data_fields: + context.service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name=field_name) + ) return super().execute( *args, diff --git a/open_city_profile/tests/graphql_test_helpers.py b/open_city_profile/tests/graphql_test_helpers.py index 1dc99890..42eb59f6 100644 --- a/open_city_profile/tests/graphql_test_helpers.py +++ b/open_city_profile/tests/graphql_test_helpers.py @@ -6,13 +6,19 @@ from django.utils.crypto import get_random_string from jose import jwt -from services.tests.factories import ServiceClientIdFactory +from services.tests.factories import ( + AllowedDataFieldFactory, + ServiceClientIdFactory, + ServiceConnectionFactory, +) from .conftest import get_unix_timestamp_now from .keys import rsa_key AUDIENCE = getattr(settings, "OIDC_API_TOKEN_AUTH")["AUDIENCE"] ISSUER = getattr(settings, "OIDC_API_TOKEN_AUTH")["ISSUER"] +if isinstance(ISSUER, list): + ISSUER = ISSUER[0] CONFIG_URL = f"{ISSUER}/.well-known/openid-configuration" JWKS_URL = f"{ISSUER}/jwks" @@ -129,6 +135,18 @@ def do_graphql_call_as_user( service_client_id = ServiceClientIdFactory( service__service_type=None, service__is_profile_service=True ) + if getattr(user, "profile", None): + ServiceConnectionFactory( + profile=user.profile, service=service_client_id.service + ) + service_client_id.service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="name"), + AllowedDataFieldFactory(field_name="address"), + AllowedDataFieldFactory(field_name="email"), + AllowedDataFieldFactory(field_name="phone"), + AllowedDataFieldFactory(field_name="personalidentitycode"), + ) + elif service: service_client_id = service.client_ids.first() diff --git a/profiles/models.py b/profiles/models.py index 5a451497..ba0f55ac 100644 --- a/profiles/models.py +++ b/profiles/models.py @@ -8,7 +8,7 @@ from encrypted_fields import fields from enumfields import EnumField -from services.models import ServiceConnection +from services.models import Service, ServiceConnection from users.models import User from utils.fields import ( NullToEmptyCharField, @@ -28,7 +28,47 @@ ) -class Profile(UUIDModel, SerializableMixin): +class AllowedDataFieldsMixin: + """ + Mixin class for checking allowed data fields per service. + + `allowed_data_fields_map` is a dictionary where the key is the `field_name` of the allowed data field + `allowed_data_fields.json` and the value is an iterable of django model's field names that the `field_name` + describes. For example, if the `field_name` is `name`, the value could be `("first_name", "last_name")`. + e.g: + allowed_data_fields_map = { + "name": ("first_name", "last_name", "nickname"), + "personalidentitycode": ("national_identification_number",), + "address": ("address", "postal_code", "city", "country_code") + } + + `always_allow_fields`: Since connections are not defined in `allowed_data_fields.json` they should be + defined here. If the field is connection and the node does not inherit this mixin the data will be available + to all services. + """ + + allowed_data_fields_map = {} + always_allow_fields = ["id", "service_connections"] + check_allowed_data_fields = True + + @classmethod + def is_field_allowed_for_service(cls, field_name: str, service: Service): + if not service: + raise ValueError("No service identified") + + if field_name in cls.always_allow_fields: + return True + + allowed_data_fields = service.allowed_data_fields.values_list( + "field_name", flat=True + ) + return any( + field_name in cls.allowed_data_fields_map.get(allowed_data_field, []) + for allowed_data_field in allowed_data_fields + ) + + +class Profile(UUIDModel, SerializableMixin, AllowedDataFieldsMixin): user = models.OneToOneField(User, on_delete=models.PROTECT, null=True, blank=True) first_name = NullToEmptyCharField(max_length=150, blank=True, db_index=True) last_name = NullToEmptyCharField(max_length=150, blank=True, db_index=True) @@ -63,6 +103,24 @@ class Meta: ) audit_log = True + # AllowedDataField configs + allowed_data_fields_map = { + "name": ( + "first_name", + "last_name", + "nickname", + ), + "email": ("emails", "primary_email"), + "phone": ("phones", "primary_phone"), + "address": ("addresses", "primary_address"), + "personalidentitycode": ("sensitivedata",), + } + always_allow_fields = AllowedDataFieldsMixin.always_allow_fields + [ + "verified_personal_information", + "language", + "contact_method", + ] + def resolve_profile(self): return self @@ -178,7 +236,7 @@ def get_national_identification_number_hash_key(): return settings.SALT_NATIONAL_IDENTIFICATION_NUMBER -class VerifiedPersonalInformation(SerializableMixin): +class VerifiedPersonalInformation(SerializableMixin, AllowedDataFieldsMixin): profile = models.OneToOneField( Profile, on_delete=models.CASCADE, related_name="verified_personal_information" ) @@ -237,6 +295,18 @@ class VerifiedPersonalInformation(SerializableMixin): ) audit_log = True + allowed_data_fields_map = { + "name": ("first_name", "last_name", "given_name"), + "personalidentitycode": ("national_identification_number",), + "address": ( + "municipality_of_residence", + "municipality_of_residence_number", + "permanent_address", + "temporary_address", + "permanent_foreign_address", + ), + } + class Meta: permissions = [ ( diff --git a/profiles/tests/test_gql_claim_profile_mutation.py b/profiles/tests/test_gql_claim_profile_mutation.py index 93c89706..d040e98e 100644 --- a/profiles/tests/test_gql_claim_profile_mutation.py +++ b/profiles/tests/test_gql_claim_profile_mutation.py @@ -61,7 +61,7 @@ def test_user_can_claim_claimable_profile_without_existing_profile( } } } - executed = user_gql_client.execute(query) + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) assert "errors" not in executed assert dict(executed["data"]) == expected_data @@ -98,6 +98,42 @@ def test_user_can_claim_claimable_profile_without_existing_profile( """ +def test_user_cant_get_fields_not_allowed_when_claiming_a_profile(user_gql_client): + profile = ProfileWithPrimaryEmailFactory(user=None) + claim_token = ClaimTokenFactory(profile=profile) + + t = Template( + """ + mutation { + claimProfile( + input: { + token: "${claimToken}", + profile: { + firstName: "Joe", + nickname: "Joey" + } + } + ) { + profile { + id + firstName + lastName + nickname + sensitivedata { + ssn + } + } + } + } + """ + ) + query = t.substitute(claimToken=claim_token.token) + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) + + assert "errors" in executed + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + + def test_can_not_change_primary_email_to_non_primary(user_gql_client): profile = ProfileFactory(user=None) email = EmailFactory(profile=profile, primary=True) @@ -112,7 +148,9 @@ def test_can_not_change_primary_email_to_non_primary(user_gql_client): }, } - executed = user_gql_client.execute(CLAIM_PROFILE_MUTATION, variables=variables) + executed = user_gql_client.execute( + CLAIM_PROFILE_MUTATION, variables=variables, allowed_data_fields=["email"] + ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -128,6 +166,7 @@ def test_can_not_delete_primary_email(user_gql_client): "token": str(claim_token.token), "profileInput": {"removeEmails": email_deletes}, }, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -172,6 +211,7 @@ def test_changing_an_email_address_marks_it_unverified( CLAIM_PROFILE_MUTATION, variables=variables, execution_context_class=execution_context_class, + allowed_data_fields=["email"], ) assert "errors" not in executed assert executed["data"] == expected_data @@ -189,7 +229,9 @@ def execute_query(self, user_gql_client, profile_input): "profileInput": profile_input, } - return user_gql_client.execute(CLAIM_PROFILE_MUTATION, variables=variables) + return user_gql_client.execute( + CLAIM_PROFILE_MUTATION, variables=variables, allowed_data_fields=["email"] + ) def test_user_cannot_claim_claimable_profile_if_token_expired(user_gql_client): @@ -223,7 +265,7 @@ def test_user_cannot_claim_claimable_profile_if_token_expired(user_gql_client): """ ) query = t.substitute(claimToken=expired_claim_token.token) - executed = user_gql_client.execute(query) + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) assert "errors" in executed assert executed["errors"][0]["extensions"]["code"] == TOKEN_EXPIRED_ERROR @@ -237,7 +279,9 @@ def test_using_non_existing_token_produces_an_object_does_not_exist_error( variables = { "token": non_existing_token, } - executed = user_gql_client.execute(CLAIM_PROFILE_MUTATION, variables=variables) + executed = user_gql_client.execute( + CLAIM_PROFILE_MUTATION, variables=variables, allowed_data_fields=["email"] + ) assert_match_error_code(executed, "OBJECT_DOES_NOT_EXIST_ERROR") @@ -270,7 +314,7 @@ def test_user_cannot_claim_claimable_profile_with_existing_profile(user_gql_clie """ ) query = t.substitute(claimToken=claim_token.token) - executed = user_gql_client.execute(query) + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) assert "errors" in executed assert ( diff --git a/profiles/tests/test_gql_claimable_profile_query.py b/profiles/tests/test_gql_claimable_profile_query.py index 059d7037..5ac0358e 100644 --- a/profiles/tests/test_gql_claimable_profile_query.py +++ b/profiles/tests/test_gql_claimable_profile_query.py @@ -33,7 +33,7 @@ def test_can_query_claimable_profile_with_token(user_gql_client): "lastName": profile.last_name, } } - executed = user_gql_client.execute(query) + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) assert "errors" not in executed assert dict(executed["data"]) == expected_data diff --git a/profiles/tests/test_gql_create_my_profile_mutation.py b/profiles/tests/test_gql_create_my_profile_mutation.py index 8dc7358c..c25a2e19 100644 --- a/profiles/tests/test_gql_create_my_profile_mutation.py +++ b/profiles/tests/test_gql_create_my_profile_mutation.py @@ -84,7 +84,9 @@ def test_normal_user_can_create_profile( primary=str(email_is_primary).lower(), ssn=ssn, ) - executed = user_gql_client.execute(mutation) + executed = user_gql_client.execute( + mutation, allowed_data_fields=["email", "name", "personalidentitycode"] + ) assert "errors" not in executed assert executed["data"] == expected_data @@ -115,10 +117,46 @@ def test_normal_user_can_create_profile_with_no_email(user_gql_client, email_dat expected_data = {"createMyProfile": {"profile": {"emails": {"edges": []}}}} - executed = user_gql_client.execute(mutation) + executed = user_gql_client.execute(mutation, allowed_data_fields=["email"]) assert executed["data"] == expected_data +def test_cant_query_fields_not_allowed_in_create_mutation(user_gql_client, email_data): + mutation = """ + mutation { + createMyProfile( + input: { + profile: {} + } + ) { + profile { + firstName + emails { + edges { + node { + email, + emailType, + primary, + verified + } + } + } + } + } + } + """ + + expected_data = { + "profile": {"firstName": user_gql_client.user.first_name, "emails": None} + } + + executed = user_gql_client.execute(mutation, allowed_data_fields=["name"]) + + assert "errors" in executed + assert expected_data == executed["data"]["createMyProfile"] + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + + class TestProfileInputValidation(ProfileInputValidationBase): def execute_query(self, user_gql_client, profile_input): mutation = """ diff --git a/profiles/tests/test_gql_create_or_update_profile_with_verified_personal_information_mutation.py b/profiles/tests/test_gql_create_or_update_profile_with_verified_personal_information_mutation.py index ac5156c7..08eb9895 100644 --- a/profiles/tests/test_gql_create_or_update_profile_with_verified_personal_information_mutation.py +++ b/profiles/tests/test_gql_create_or_update_profile_with_verified_personal_information_mutation.py @@ -41,7 +41,7 @@ def execute_mutation(input_data, gql_client): } """ - return gql_client.execute(query, variables={"input": input_data}, service=None) + return gql_client.execute(query, variables={"input": input_data}) def execute_successful_mutation(input_data, gql_client): diff --git a/profiles/tests/test_gql_create_or_update_user_profile_mutation.py b/profiles/tests/test_gql_create_or_update_user_profile_mutation.py index 9db71829..04051646 100644 --- a/profiles/tests/test_gql_create_or_update_user_profile_mutation.py +++ b/profiles/tests/test_gql_create_or_update_user_profile_mutation.py @@ -35,13 +35,13 @@ def execute_mutation(input_data, gql_client): input: $input, ) { profile { - id, + id } } } """ - return gql_client.execute(query, variables={"input": input_data}, service=None) + return gql_client.execute(query, variables={"input": input_data}) def execute_successful_mutation(input_data, gql_client): diff --git a/profiles/tests/test_gql_create_profile_mutation.py b/profiles/tests/test_gql_create_profile_mutation.py index d75e69ac..67b7bc09 100644 --- a/profiles/tests/test_gql_create_profile_mutation.py +++ b/profiles/tests/test_gql_create_profile_mutation.py @@ -146,7 +146,9 @@ def test_staff_user_can_create_a_profile( } } } - executed = user_gql_client.execute(query, service=service) + executed = user_gql_client.execute( + query, service=service, allowed_data_fields=["name", "phone", "email"] + ) assert executed["data"] == expected_data @@ -229,7 +231,9 @@ def test_staff_user_with_sensitive_data_service_accesss_can_create_a_profile_wit "profile": {"firstName": "John", "sensitivedata": {"ssn": "121282-123E"}} } } - executed = user_gql_client.execute(query, service=service) + executed = user_gql_client.execute( + query, service=service, allowed_data_fields=["name", "personalidentitycode"] + ) assert executed["data"] == expected_data @@ -280,6 +284,7 @@ def test_staff_user_cannot_create_a_profile_with_sensitive_data_without_sensitiv assert executed["errors"][0]["message"] == _( "You do not have permission to perform this action." ) + assert executed["data"]["createProfile"] is None class TestProfileInputValidation(ProfileInputValidationBase): diff --git a/profiles/tests/test_gql_federation.py b/profiles/tests/test_gql_federation.py index e2c1f500..eeb76698 100644 --- a/profiles/tests/test_gql_federation.py +++ b/profiles/tests/test_gql_federation.py @@ -149,7 +149,10 @@ def test_owner_can_resolve_profile_entity( "_entities": [{"id": profile._global_id, "firstName": profile.first_name}] } executed = user_gql_client.execute( - ENTITY_QUERY, variables=variables, service=service if with_service else None + ENTITY_QUERY, + variables=variables, + service=service if with_service else None, + allowed_data_fields=["name"] if with_service else None, ) if with_service and with_serviceconnection: @@ -208,7 +211,7 @@ def test_staff_user_can_resolve_profile_entity( "_entities": [{"id": profile._global_id, "firstName": profile.first_name}] } executed = user_gql_client.execute( - ENTITY_QUERY, variables=variables, service=service + ENTITY_QUERY, variables=variables, service=service, allowed_data_fields=["name"] ) if with_serviceconnection: diff --git a/profiles/tests/test_gql_my_profile_query.py b/profiles/tests/test_gql_my_profile_query.py index 2b79339c..cf1fcf50 100644 --- a/profiles/tests/test_gql_my_profile_query.py +++ b/profiles/tests/test_gql_my_profile_query.py @@ -2,7 +2,11 @@ from open_city_profile.tests import to_graphql_name from open_city_profile.tests.asserts import assert_match_error_code -from services.tests.factories import ServiceConnectionFactory +from services.tests.factories import ( + AllowedDataFieldFactory, + ServiceConnectionFactory, + ServiceFactory, +) from .conftest import VERIFIED_PERSONAL_INFORMATION_ADDRESS_TYPES from .factories import ( @@ -16,7 +20,7 @@ ) -def test_normal_user_can_query_emails(user_gql_client): +def test_normal_user_can_query_emails(user_gql_client, service): profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user) email = profile.emails.first() @@ -50,11 +54,14 @@ def test_normal_user_can_query_emails(user_gql_client): } } } - executed = user_gql_client.execute(query) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="email")) + ServiceConnectionFactory(profile=profile, service=service) + + executed = user_gql_client.execute(query, service=service) assert dict(executed["data"]) == expected_data -def test_normal_user_can_query_phones(user_gql_client): +def test_normal_user_can_query_phones(user_gql_client, service): profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user) phone = PhoneFactory(profile=profile) @@ -88,11 +95,14 @@ def test_normal_user_can_query_phones(user_gql_client): } } } - executed = user_gql_client.execute(query) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="phone")) + ServiceConnectionFactory(profile=profile, service=service) + + executed = user_gql_client.execute(query, service=service) assert dict(executed["data"]) == expected_data -def test_normal_user_can_query_addresses(user_gql_client): +def test_normal_user_can_query_addresses(user_gql_client, service): profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user) address = AddressFactory(profile=profile) @@ -126,12 +136,15 @@ def test_normal_user_can_query_addresses(user_gql_client): } } } - executed = user_gql_client.execute(query) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="address")) + ServiceConnectionFactory(profile=profile, service=service) + + executed = user_gql_client.execute(query, service=service) assert dict(executed["data"]) == expected_data def test_normal_user_can_query_primary_contact_details( - user_gql_client, execution_context_class + user_gql_client, execution_context_class, service ): profile = ProfileFactory(user=user_gql_client.user) phone = PhoneFactory(profile=profile, primary=True) @@ -181,8 +194,15 @@ def test_normal_user_can_query_primary_contact_details( }, } } + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="phone"), + AllowedDataFieldFactory(field_name="email"), + AllowedDataFieldFactory(field_name="address"), + ) + ServiceConnectionFactory(profile=profile, service=service) + executed = user_gql_client.execute( - query, execution_context_class=execution_context_class + query, execution_context_class=execution_context_class, service=service ) assert dict(executed["data"]) == expected_data @@ -218,31 +238,56 @@ class TestProfileWithVerifiedPersonalInformation: } """ - @staticmethod - def _execute_query(gql_client, loa="substantial"): + @pytest.fixture(autouse=True) + def setup_data(self, db, user_gql_client): + self.user = user_gql_client.user + self.client = user_gql_client + self.service = ServiceFactory(is_profile_service=True) + self.profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=self.profile, service=self.service) + self._add_allowed_data_fields_to_service(self.service) + + def _create_allowed_data_fields(self): + self.allowed_name = AllowedDataFieldFactory(field_name="name") + self.allowed_address = AllowedDataFieldFactory(field_name="address") + self.allowed_personal_identity_code = AllowedDataFieldFactory( + field_name="personalidentitycode" + ) + + def _add_allowed_data_fields_to_service(self, service): + if not getattr(self, "allowed_name", None): + self._create_allowed_data_fields() + + service.allowed_data_fields.add( + self.allowed_name, + self.allowed_address, + self.allowed_personal_identity_code, + ) + + def _execute_query(self, loa="substantial", service=None): token_payload = { "loa": loa, } - return gql_client.execute( + kwargs = {"service": self.service} + if service: + kwargs["service"] = service + + return self.client.execute( TestProfileWithVerifiedPersonalInformation.QUERY, auth_token_payload=token_payload, + **kwargs, ) - def test_when_verified_personal_infomation_does_not_exist_returns_null( - self, user_gql_client - ): - ProfileFactory(user=user_gql_client.user) - - executed = self._execute_query(user_gql_client) + def test_when_verified_personal_information_does_not_exist_returns_null(self): + executed = self._execute_query() assert "errors" not in executed assert executed["data"]["myProfile"]["verifiedPersonalInformation"] is None - def test_normal_user_can_query_verified_personal_information(self, user_gql_client): - profile = ProfileFactory(user=user_gql_client.user) + def test_normal_user_can_query_verified_personal_information(self): verified_personal_information = VerifiedPersonalInformationFactory( - profile=profile + profile=self.profile ) permanent_address = verified_personal_information.permanent_address @@ -279,20 +324,17 @@ def test_normal_user_can_query_verified_personal_information(self, user_gql_clie } } - executed = self._execute_query(user_gql_client) + executed = self._execute_query() assert executed["data"] == expected_data @pytest.mark.parametrize( "address_type", VERIFIED_PERSONAL_INFORMATION_ADDRESS_TYPES ) - def test_when_address_does_not_exist_returns_null( - self, address_type, user_gql_client - ): - profile = ProfileFactory(user=user_gql_client.user) - VerifiedPersonalInformationFactory(profile=profile, **{address_type: None}) + def test_when_address_does_not_exist_returns_null(self, address_type): + VerifiedPersonalInformationFactory(profile=self.profile, **{address_type: None}) - executed = self._execute_query(user_gql_client) + executed = self._execute_query() assert "errors" not in executed @@ -305,11 +347,10 @@ def test_when_address_does_not_exist_returns_null( assert isinstance(received_address, dict) @pytest.mark.parametrize("loa", ["substantial", "high"]) - def test_high_enough_level_of_assurance_gains_access(self, loa, user_gql_client): - profile = ProfileFactory(user=user_gql_client.user) - VerifiedPersonalInformationFactory(profile=profile) + def test_high_enough_level_of_assurance_gains_access(self, loa): + VerifiedPersonalInformationFactory(profile=self.profile) - executed = self._execute_query(user_gql_client, loa) + executed = self._execute_query(loa) assert not hasattr(executed, "errors") assert isinstance( @@ -317,26 +358,24 @@ def test_high_enough_level_of_assurance_gains_access(self, loa, user_gql_client) ) @pytest.mark.parametrize("loa", [None, "low", "unknown"]) - def test_too_low_level_of_assurance_denies_access(self, loa, user_gql_client): - profile = ProfileFactory(user=user_gql_client.user) - VerifiedPersonalInformationFactory(profile=profile) + def test_too_low_level_of_assurance_denies_access(self, loa): + VerifiedPersonalInformationFactory(profile=self.profile) - executed = self._execute_query(user_gql_client, loa) + executed = self._execute_query(loa) assert_match_error_code(executed, "PERMISSION_DENIED_ERROR") assert executed["data"]["myProfile"]["verifiedPersonalInformation"] is None @pytest.mark.parametrize("with_serviceconnection", (True, False)) - def test_service_connection_required( - self, user_gql_client, service, with_serviceconnection - ): - profile = ProfileFactory(user=user_gql_client.user) - VerifiedPersonalInformationFactory(profile=profile) + def test_service_connection_required(self, with_serviceconnection): + service = ServiceFactory() + self._add_allowed_data_fields_to_service(service) + VerifiedPersonalInformationFactory(profile=self.profile) if with_serviceconnection: - ServiceConnectionFactory(profile=profile, service=service) + ServiceConnectionFactory(profile=self.profile, service=service) - executed = user_gql_client.execute( + executed = self.client.execute( TestProfileWithVerifiedPersonalInformation.QUERY, auth_token_payload={"loa": "substantial"}, service=service, @@ -372,6 +411,7 @@ def test_querying_non_existent_profile_doesnt_return_errors(user_gql_client, ser def test_normal_user_can_query_their_own_profile( user_gql_client, service, with_service, with_serviceconnection ): + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) profile = ProfileFactory(user=user_gql_client.user) if with_serviceconnection: ServiceConnectionFactory(profile=profile, service=service) @@ -399,8 +439,15 @@ def test_normal_user_can_query_their_own_profile( assert executed["data"]["myProfile"] is None -def test_normal_user_can_query_their_own_profiles_sensitivedata(user_gql_client): +def test_normal_user_can_query_their_own_profiles_sensitivedata( + user_gql_client, service +): + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="name"), + AllowedDataFieldFactory(field_name="personalidentitycode"), + ) profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) sensitive_data = SensitiveDataFactory(profile=profile) query = """ @@ -419,5 +466,113 @@ def test_normal_user_can_query_their_own_profiles_sensitivedata(user_gql_client) "sensitivedata": {"ssn": sensitive_data.ssn}, } } - executed = user_gql_client.execute(query) + executed = user_gql_client.execute(query, service=service) assert dict(executed["data"]) == expected_data + + +def test_my_profile_results_error_if_querying_fields_not_allowed( + user_gql_client, service +): + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + + query = """ + { + myProfile { + firstName + lastName + sensitivedata { + ssn + } + } + } + """ + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["myProfile"] is None + + +def test_my_profile_results_error_if_querying_fields_not_allowed_and_shows_allowed( + user_gql_client, service +): + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + myProfile { + firstName + lastName + sensitivedata { + ssn + } + } + } + """ + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["myProfile"]["firstName"] == profile.first_name + assert executed["data"]["myProfile"]["lastName"] == profile.last_name + assert executed["data"]["myProfile"]["sensitivedata"] is None + + +def test_my_profile_checks_allowed_data_fields_for_single_query( + user_gql_client, service +): + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + myProfile { + firstName + lastName + sensitivedata { + ssn + } + } + } + """ + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["myProfile"]["firstName"] == profile.first_name + assert executed["data"]["myProfile"]["lastName"] == profile.last_name + assert executed["data"]["myProfile"]["sensitivedata"] is None + + +def test_my_profile_checks_allowed_data_fields_for_multiple_queries( + user_gql_client, service +): + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + _service { + __typename + } + myProfile { + firstName + lastName + sensitivedata { + ssn + } + } + services { + edges { + node { + name + } + } + } + } + """ + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["myProfile"]["firstName"] == profile.first_name + assert executed["data"]["myProfile"]["lastName"] == profile.last_name + assert executed["data"]["myProfile"]["sensitivedata"] is None + assert executed["data"]["services"] is None diff --git a/profiles/tests/test_gql_profile_query.py b/profiles/tests/test_gql_profile_query.py index 75efbf72..3b157fde 100644 --- a/profiles/tests/test_gql_profile_query.py +++ b/profiles/tests/test_gql_profile_query.py @@ -7,7 +7,7 @@ from open_city_profile.consts import OBJECT_DOES_NOT_EXIST_ERROR from open_city_profile.tests.asserts import assert_match_error_code -from services.tests.factories import ServiceConnectionFactory +from services.tests.factories import AllowedDataFieldFactory, ServiceConnectionFactory from ..schema import ProfileNode from .factories import SensitiveDataFactory @@ -44,6 +44,7 @@ def test_staff_user_can_query_a_profile_connected_to_service_he_is_admin_of( user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) # serviceType is included in query just to ensure that it has NO affect t = Template( @@ -74,6 +75,7 @@ def test_staff_user_cannot_query_a_profile_without_id( user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) query = """ { @@ -98,6 +100,9 @@ def test_staff_user_cannot_query_a_profile_without_a_connection_to_their_service other_service = service_factory() ServiceConnectionFactory(profile=profile, service=other_service) + adf_name = AllowedDataFieldFactory(field_name="name") + other_service.allowed_data_fields.add(adf_name) + staff_user_service.allowed_data_fields.add(adf_name) t = Template( """ @@ -127,6 +132,9 @@ def test_staff_user_cannot_query_sensitive_data_with_only_profile_permissions( user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="personalidentitycode") + ) t = Template( """ @@ -158,6 +166,9 @@ def test_staff_user_can_query_sensitive_data_with_given_permissions( user.groups.add(group) assign_perm("can_view_profiles", group, service) assign_perm("can_view_sensitivedata", group, service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="personalidentitycode") + ) t = Template( """ @@ -188,6 +199,9 @@ def test_staff_receives_null_sensitive_data_if_it_does_not_exist( user.groups.add(group) assign_perm("can_view_profiles", group, service) assign_perm("can_view_sensitivedata", group, service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="personalidentitycode") + ) t = Template( """ @@ -229,6 +243,7 @@ def test_staff_user_needs_required_permission_to_access_verified_personal_inform ServiceConnectionFactory( profile=profile_with_verified_personal_information, service=service ) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) @@ -273,3 +288,83 @@ def test_staff_user_needs_required_permission_to_access_verified_personal_inform else: assert_match_error_code(executed, "PERMISSION_DENIED_ERROR") assert executed["data"] == {"profile": {"verifiedPersonalInformation": None}} + + +def test_profile_checks_allowed_data_fields_for_single_query( + user_gql_client, service, profile, group +): + user = user_gql_client.user + user.groups.add(group) + assign_perm("can_view_profiles", group, service) + assign_perm("can_view_sensitivedata", group, service) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + profile(id: "%s") { + firstName + lastName + sensitivedata { + ssn + } + } + } + """ % relay.Node.to_global_id( + ProfileNode._meta.name, profile.id + ) + + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["profile"]["firstName"] == profile.first_name + assert executed["data"]["profile"]["lastName"] == profile.last_name + assert executed["data"]["profile"]["sensitivedata"] is None + + +def test_my_profile_checks_allowed_data_fields_for_multiple_queries( + user_gql_client, service, profile, group +): + user = user_gql_client.user + user.groups.add(group) + assign_perm("can_view_profiles", group, service) + assign_perm("can_view_sensitivedata", group, service) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + myProfile { + firstName + lastName + sensitivedata { + ssn + } + } + _service { + __typename + } + profile(id: "%s") { + firstName + lastName + sensitivedata { + ssn + } + } + services { + edges { + node { + name + } + } + } + } + """ % relay.Node.to_global_id( + ProfileNode._meta.name, profile.id + ) + + executed = user_gql_client.execute(query, service=service) + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + assert executed["data"]["profile"]["firstName"] == profile.first_name + assert executed["data"]["profile"]["lastName"] == profile.last_name + assert executed["data"]["profile"]["sensitivedata"] is None + assert executed["data"]["services"] is None diff --git a/profiles/tests/test_gql_profile_with_access_token_query.py b/profiles/tests/test_gql_profile_with_access_token_query.py index 1df87249..ea714874 100644 --- a/profiles/tests/test_gql_profile_with_access_token_query.py +++ b/profiles/tests/test_gql_profile_with_access_token_query.py @@ -26,7 +26,9 @@ def test_anonymous_user_can_retrieve_a_profile_with_temporary_read_access_token( profile = ProfileFactory() token = TemporaryReadAccessTokenFactory(profile=profile) - executed = anon_user_gql_client.execute(self.query(token.token)) + executed = anon_user_gql_client.execute( + self.query(token.token), allowed_data_fields=["name"] + ) assert "errors" not in executed actual_profile = executed["data"]["profileWithAccessToken"] diff --git a/profiles/tests/test_gql_profiles_query.py b/profiles/tests/test_gql_profiles_query.py index bf87f65d..4ebc956c 100644 --- a/profiles/tests/test_gql_profiles_query.py +++ b/profiles/tests/test_gql_profiles_query.py @@ -8,7 +8,7 @@ from open_city_profile.tests import to_graphql_name from open_city_profile.tests.asserts import assert_match_error_code from profiles.enums import AddressType, EmailType, PhoneType -from services.tests.factories import ServiceConnectionFactory +from services.tests.factories import AllowedDataFieldFactory, ServiceConnectionFactory from .factories import ( AddressFactory, @@ -41,6 +41,7 @@ def test_normal_user_can_not_query_profiles(user_gql_client, service): def test_admin_user_can_query_profiles(superuser_gql_client, profile, service): ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) query = """ { @@ -73,6 +74,47 @@ def test_admin_user_can_query_profiles(superuser_gql_client, profile, service): assert executed["data"] == expected_data +def test_admin_user_cant_query_fields_that_are_not_allowed( + superuser_gql_client, profile, service +): + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + + query = """ + { + profiles { + edges { + node { + firstName + lastName + nickname + emails { edges { node { email } } } + } + } + } + } + """ + + expected_data = { + "profiles": { + "edges": [ + { + "node": { + "firstName": profile.first_name, + "lastName": profile.last_name, + "nickname": profile.nickname, + "emails": None, + } + } + ] + } + } + executed = superuser_gql_client.execute(query, service=service) + assert executed["data"] == expected_data + assert "errors" in executed + assert_match_error_code(executed, "FIELD_NOT_ALLOWED_ERROR") + + def test_staff_user_with_group_access_can_query_profiles( user_gql_client, profile, group, service ): @@ -80,6 +122,7 @@ def test_staff_user_with_group_access_can_query_profiles( user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) # serviceType is included in query just to ensure that it has NO affect query = """ @@ -154,6 +197,7 @@ def test_staff_user_can_filter_profiles_by_a_field( profile_1, profile_2 = ProfileFactory.create_batch(2) ServiceConnectionFactory(profile=profile_1, service=service) ServiceConnectionFactory(profile=profile_2, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) @@ -189,6 +233,7 @@ def test_staff_user_filter_profiles_by_verified_personal_information_permissions vpi = VerifiedPersonalInformationFactory() ServiceConnectionFactory(profile=vpi.profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) @@ -235,6 +280,7 @@ def test_staff_user_can_sort_profiles(user_gql_client, group, service): ) ServiceConnectionFactory(profile=profile_1, service=service) ServiceConnectionFactory(profile=profile_2, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) @@ -298,6 +344,7 @@ def test_staff_user_can_sort_profiles_by_custom_fields( EmailFactory(profile=profile_2, email="bryan.tester@example.com", primary=True) ServiceConnectionFactory(profile=profile_1, service=service) ServiceConnectionFactory(profile=profile_2, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) @@ -658,6 +705,7 @@ def test_staff_user_can_paginate_profiles( ): ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user = user_gql_client.user user.groups.add(group) assign_perm("can_view_profiles", group, service) @@ -699,12 +747,15 @@ def test_staff_user_with_group_access_can_query_only_profiles_he_has_access_to( entitled_profile = ProfileFactory() entitled_service = service_factory() + adf_name = AllowedDataFieldFactory(field_name="name") ServiceConnectionFactory(profile=entitled_profile, service=entitled_service) assign_perm("can_view_profiles", group, entitled_service) + entitled_service.allowed_data_fields.add(adf_name) unentitled_profile = ProfileFactory() unentitled_service = service_factory() ServiceConnectionFactory(profile=unentitled_profile, service=unentitled_service) + unentitled_service.allowed_data_fields.add(adf_name) query = """ { diff --git a/profiles/tests/test_gql_relay_ordering.py b/profiles/tests/test_gql_relay_ordering.py index c1d55523..616be10e 100644 --- a/profiles/tests/test_gql_relay_ordering.py +++ b/profiles/tests/test_gql_relay_ordering.py @@ -61,8 +61,14 @@ """ -def test_addresses_are_ordered_first_by_primary_then_by_id(user_gql_client): +def test_addresses_are_ordered_first_by_primary_then_by_id(user_gql_client, service): profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="address"), + AllowedDataFieldFactory(field_name="email"), + AllowedDataFieldFactory(field_name="phone"), + ) first_address = AddressFactory(profile=profile, primary=False) primary_address = AddressFactory(profile=profile, primary=True) second_address = AddressFactory(profile=profile, primary=False) @@ -74,12 +80,18 @@ def test_addresses_are_ordered_first_by_primary_then_by_id(user_gql_client): ) ) - executed = user_gql_client.execute(QUERY) + executed = user_gql_client.execute(QUERY, service=service) assert executed["data"]["myProfile"]["addresses"]["edges"] == expected_edges -def test_emails_are_ordered_first_by_primary_then_by_id(user_gql_client): +def test_emails_are_ordered_first_by_primary_then_by_id(user_gql_client, service): profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="address"), + AllowedDataFieldFactory(field_name="email"), + AllowedDataFieldFactory(field_name="phone"), + ) first_email = EmailFactory(profile=profile, primary=False) primary_email = EmailFactory(profile=profile, primary=True) second_email = EmailFactory(profile=profile, primary=False) @@ -91,12 +103,18 @@ def test_emails_are_ordered_first_by_primary_then_by_id(user_gql_client): ) ) - executed = user_gql_client.execute(QUERY) + executed = user_gql_client.execute(QUERY, service=service) assert executed["data"]["myProfile"]["emails"]["edges"] == expected_edges -def test_phones_are_ordered_first_by_primary_then_by_id(user_gql_client): +def test_phones_are_ordered_first_by_primary_then_by_id(user_gql_client, service): profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add( + AllowedDataFieldFactory(field_name="address"), + AllowedDataFieldFactory(field_name="email"), + AllowedDataFieldFactory(field_name="phone"), + ) first_phone = PhoneFactory(profile=profile, primary=False) primary_phone = PhoneFactory(profile=profile, primary=True) second_phone = PhoneFactory(profile=profile, primary=False) @@ -108,7 +126,7 @@ def test_phones_are_ordered_first_by_primary_then_by_id(user_gql_client): ) ) - executed = user_gql_client.execute(QUERY) + executed = user_gql_client.execute(QUERY, service=service) assert executed["data"]["myProfile"]["phones"]["edges"] == expected_edges diff --git a/profiles/tests/test_gql_update_my_profile_mutation.py b/profiles/tests/test_gql_update_my_profile_mutation.py index 8a5d4a7a..a8fd189d 100644 --- a/profiles/tests/test_gql_update_my_profile_mutation.py +++ b/profiles/tests/test_gql_update_my_profile_mutation.py @@ -100,7 +100,9 @@ def test_update_profile( email_type=email_data["email_type"], primary=str(email_data["primary"]).lower(), ) - executed = user_gql_client.execute(mutation, service=service) + executed = user_gql_client.execute( + mutation, service=service, allowed_data_fields=["name", "email"] + ) if with_serviceconnection: assert executed["data"] == expected_data @@ -152,7 +154,7 @@ def test_update_profile_without_email(user_gql_client, profile_data): } mutation = t.substitute(nickname=profile_data["nickname"]) - executed = user_gql_client.execute(mutation) + executed = user_gql_client.execute(mutation, allowed_data_fields=["name", "email"]) assert executed["data"] == expected_data @@ -299,7 +301,7 @@ def test_add_email(user_gql_client, email_data): email_type=email_data["email_type"], primary=str(not email_data["primary"]).lower(), ) - executed = user_gql_client.execute(mutation) + executed = user_gql_client.execute(mutation, allowed_data_fields=["email"]) assert dict(executed["data"]) == expected_data @@ -358,7 +360,9 @@ def test_update_email( ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}} + EMAILS_MUTATION, + variables={"profileInput": {"updateEmails": email_updates}}, + allowed_data_fields=["email"], ) if succeeds: @@ -399,7 +403,9 @@ def test_can_not_update_email_of_another_profile(user_gql_client): } ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}} + EMAILS_MUTATION, + variables={"profileInput": {"updateEmails": email_updates}}, + allowed_data_fields=["email"], ) assert executed["data"]["updateMyProfile"] is None @@ -445,7 +451,9 @@ def test_change_primary_email_to_another_one(user_gql_client): {"id": to_global_id(type="EmailNode", id=email.id), "primary": True} ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}} + EMAILS_MUTATION, + variables={"profileInput": {"updateEmails": email_updates}}, + allowed_data_fields=["email"], ) assert dict(executed["data"]) == expected_data @@ -458,7 +466,9 @@ def test_can_not_change_primary_email_to_non_primary(user_gql_client): {"id": to_global_id(type="EmailNode", id=email.id), "primary": False} ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}} + EMAILS_MUTATION, + variables={"profileInput": {"updateEmails": email_updates}}, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -469,7 +479,9 @@ def test_can_not_delete_primary_email(user_gql_client): email_deletes = [to_global_id(type="EmailNode", id=email.id)] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"removeEmails": email_deletes}} + EMAILS_MUTATION, + variables={"profileInput": {"removeEmails": email_deletes}}, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -502,6 +514,7 @@ def test_can_replace_a_primary_email_with_a_newly_created_one( "updateEmails": email_updates, } }, + allowed_data_fields=["email"], ) new_primary_email = Email.objects.get(email=email_data["email"]) @@ -571,7 +584,9 @@ def test_changing_an_email_address_marks_it_unverified(user_gql_client): {"id": to_global_id("EmailNode", email.id), "email": new_email_value} ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"updateEmails": email_updates}} + EMAILS_MUTATION, + variables={"profileInput": {"updateEmails": email_updates}}, + allowed_data_fields=["email"], ) assert dict(executed["data"]) == expected_data @@ -599,7 +614,9 @@ def test_remove_email(global_id_type, global_id_id, succeeds, user_gql_client): to_global_id(type=global_id_type, id=global_id_id), ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"removeEmails": email_deletes}} + EMAILS_MUTATION, + variables={"profileInput": {"removeEmails": email_deletes}}, + allowed_data_fields=["email"], ) if succeeds: @@ -659,7 +676,9 @@ def test_remove_all_emails_if_they_are_not_primary(user_gql_client): to_global_id(type="EmailNode", id=email2.id), ] executed = user_gql_client.execute( - EMAILS_MUTATION, variables={"profileInput": {"removeEmails": email_deletes}} + EMAILS_MUTATION, + variables={"profileInput": {"removeEmails": email_deletes}}, + allowed_data_fields=["email"], ) assert executed["data"] == expected_data @@ -740,6 +759,7 @@ def test_add_phone(user_gql_client, phone_data): ] } }, + allowed_data_fields=["phone"], ) phone = profile.phones.get() @@ -800,6 +820,7 @@ def test_update_phone( ] } }, + allowed_data_fields=["phone"], ) if succeeds: @@ -844,6 +865,7 @@ def test_can_not_update_phone_of_another_profile(user_gql_client): ] } }, + allowed_data_fields=["phone"], ) assert executed["data"]["updateMyProfile"] is None @@ -876,6 +898,7 @@ def test_remove_phone(global_id_type, global_id_id, succeeds, user_gql_client): "removePhones": [to_global_id(type=global_id_type, id=global_id_id)] } }, + allowed_data_fields=["phone"], ) if succeeds: @@ -911,7 +934,9 @@ def create_profile(self, user): def execute_query(self, user_gql_client, profile_input): return user_gql_client.execute( - PHONES_MUTATION, variables={"profileInput": profile_input} + PHONES_MUTATION, + variables={"profileInput": profile_input}, + allowed_data_fields=["phone"], ) @@ -961,6 +986,7 @@ def test_add_address(user_gql_client, address_data): ] } }, + allowed_data_fields=["address"], ) address = profile.addresses.get() @@ -1027,6 +1053,7 @@ def test_update_address( ] } }, + allowed_data_fields=["address"], ) if succeeds: @@ -1108,6 +1135,7 @@ def test_remove_address(global_id_type, global_id_id, succeeds, user_gql_client) "removeAddresses": [to_global_id(type=global_id_type, id=global_id_id)] } }, + allowed_data_fields=["address"], ) if succeeds: @@ -1239,7 +1267,9 @@ def test_change_primary_contact_details( primary="true", ) executed = user_gql_client.execute( - mutation, execution_context_class=execution_context_class + mutation, + execution_context_class=execution_context_class, + allowed_data_fields=["email", "phone", "address"], ) assert "errors" not in executed assert dict(executed["data"]) == expected_data @@ -1285,5 +1315,48 @@ def test_update_sensitive_data(user_gql_client): } } } - executed = user_gql_client.execute(query) + executed = user_gql_client.execute( + query, allowed_data_fields=["name", "personalidentitycode"] + ) + assert dict(executed["data"]) == expected_data + + +def test_update_profile_does_not_reveal_fields_not_allowed(user_gql_client): + profile = ProfileWithPrimaryEmailFactory(user=user_gql_client.user) + SensitiveDataFactory(profile=profile, ssn="010199-1234") + + t = Template( + """ + mutation { + updateMyProfile( + input: { + profile: { + nickname: "${nickname}" + } + } + ) { + profile { + nickname + sensitivedata { + ssn + } + } + } + } + """ + ) + + data = {"nickname": "Larry"} + + query = t.substitute(**data) + + expected_data = { + "updateMyProfile": { + "profile": { + "nickname": data["nickname"], + "sensitivedata": None, + } + } + } + executed = user_gql_client.execute(query, allowed_data_fields=["name"]) assert dict(executed["data"]) == expected_data diff --git a/profiles/tests/test_gql_update_profile_mutation.py b/profiles/tests/test_gql_update_profile_mutation.py index c32854a9..811c901d 100644 --- a/profiles/tests/test_gql_update_profile_mutation.py +++ b/profiles/tests/test_gql_update_profile_mutation.py @@ -152,7 +152,17 @@ def test_staff_user_can_update_a_profile( } } } - executed = user_gql_client.execute(query, service=service) + executed = user_gql_client.execute( + query, + service=service, + allowed_data_fields=[ + "name", + "email", + "phone", + "personalidentitycode", + "address", + ], + ) if with_serviceconnection: assert executed["data"] == expected_data @@ -207,6 +217,7 @@ def test_can_not_change_primary_email_to_non_primary(user_gql_client, service): "updateEmails": email_updates, } }, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -227,6 +238,7 @@ def test_can_not_delete_primary_email(user_gql_client, service): "removeEmails": email_deletes, } }, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "PROFILE_MUST_HAVE_PRIMARY_EMAIL") @@ -267,7 +279,10 @@ def test_changing_an_email_address_marks_it_unverified(user_gql_client, service) } executed = user_gql_client.execute( - EMAILS_MUTATION, service=service, variables=variables + EMAILS_MUTATION, + service=service, + variables=variables, + allowed_data_fields=["email"], ) assert executed["data"] == expected_data @@ -311,7 +326,10 @@ def test_when_keycloak_returns_conflict_on_update_changes_are_reverted( } } executed = user_gql_client.execute( - EMAILS_MUTATION, service=service, variables=variables + EMAILS_MUTATION, + service=service, + variables=variables, + allowed_data_fields=["email"], ) assert_match_error_code(executed, "DATA_CONFLICT_ERROR") @@ -333,7 +351,10 @@ def execute_query(self, user_gql_client, profile_input): variables = {"profileInput": profile_input} return user_gql_client.execute( - EMAILS_MUTATION, service=service, variables=variables + EMAILS_MUTATION, + service=service, + variables=variables, + allowed_data_fields=["email"], ) diff --git a/profiles/tests/test_profiles_graphql_authentication.py b/profiles/tests/test_profiles_graphql_authentication.py index dbe073a1..a24a7b0b 100644 --- a/profiles/tests/test_profiles_graphql_authentication.py +++ b/profiles/tests/test_profiles_graphql_authentication.py @@ -6,6 +6,7 @@ do_graphql_call, do_graphql_call_as_user, ) +from services.tests.factories import AllowedDataFieldFactory def test_presenting_a_valid_access_token_grants_access(profile, live_server): @@ -28,6 +29,7 @@ def test_jwt_claims_are_usable_in_field_resolvers( service_connection_factory( profile=profile_with_verified_personal_information, service=service ) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) user_uuid = profile_with_verified_personal_information.user.uuid claims = {"sub": str(user_uuid), "loa": loa, "azp": service_client_id.client_id} @@ -55,6 +57,7 @@ def test_determine_service_from_the_azp_claim( user = profile.user user.groups.add(group) assign_perm("can_view_profiles", group, service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) claims = {"sub": str(user.uuid), "azp": service_client_id.client_id} query = """ From 95193fe7c42575b25b81c8723a24e4b30314ed29 Mon Sep 17 00:00:00 2001 From: Nico Virkki Date: Tue, 7 May 2024 14:08:32 +0300 Subject: [PATCH 3/3] feat: add feature flag for checking allowed data fields Adds setting for allowed data field check named as: ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION The setting defaults False. When False, app logs a warning message when there is a query trying to access a field which is not in service's allowed_data_fields Refs HP-2319 --- open_city_profile/graphene.py | 20 ++++++- open_city_profile/settings.py | 3 + open_city_profile/tests/conftest.py | 5 ++ ...est_allowed_data_field_restriction_flag.py | 59 +++++++++++++++++++ 4 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 profiles/tests/test_allowed_data_field_restriction_flag.py diff --git a/open_city_profile/graphene.py b/open_city_profile/graphene.py index 27b4c2dc..05dd65f8 100644 --- a/open_city_profile/graphene.py +++ b/open_city_profile/graphene.py @@ -1,3 +1,4 @@ +import logging import uuid from functools import partial @@ -189,11 +190,24 @@ def resolve(self, next, root, info, **kwargs): field_name = to_snake_case(getattr(info, "field_name", "")) if not getattr(info.context, "service", False): - raise ServiceNotIdentifiedError("Service not identified") + if settings.ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION: + raise ServiceNotIdentifiedError("Service not identified") + + logging.warning( + "Allowed data field exception would occur: Service not identified. Field name: %s", + field_name, + ) if not root.is_field_allowed_for_service(field_name, info.context.service): - raise FieldNotAllowedError( - "Field is not allowed for service.", field_name=field_name + if settings.ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION: + raise FieldNotAllowedError( + "Field is not allowed for service.", field_name=field_name + ) + + logging.warning( + "Allowed data field exception would occur. Field (%s) is not allowed for service %s.", + field_name, + info.context.service, ) return next(root, info, **kwargs) diff --git a/open_city_profile/settings.py b/open_city_profile/settings.py index 96f7cbc5..8786baf4 100644 --- a/open_city_profile/settings.py +++ b/open_city_profile/settings.py @@ -51,6 +51,7 @@ AUDIT_LOG_LOGGER_FILENAME=(str, ""), AUDIT_LOG_TO_DB_ENABLED=(bool, False), OPEN_CITY_PROFILE_LOG_LEVEL=(str, None), + ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION=(bool, False), ENABLE_GRAPHIQL=(bool, False), ENABLE_GRAPHQL_INTROSPECTION=(bool, False), GRAPHQL_QUERY_DEPTH_LIMIT=(int, 12), @@ -174,6 +175,8 @@ GRAPHQL_QUERY_DEPTH_LIMIT = env("GRAPHQL_QUERY_DEPTH_LIMIT") +ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION = env("ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION") + INSTALLED_APPS = [ "helusers.apps.HelusersConfig", "open_city_profile.apps.OpenCityProfileAdminConfig", diff --git a/open_city_profile/tests/conftest.py b/open_city_profile/tests/conftest.py index d7f81563..644a0b4e 100644 --- a/open_city_profile/tests/conftest.py +++ b/open_city_profile/tests/conftest.py @@ -220,3 +220,8 @@ def unix_timestamp_now(): @pytest.fixture(params=[None, ""]) def empty_string_value(request): return request.param + + +@pytest.fixture(autouse=True) +def enable_allowed_data_fields_restriction(settings): + settings.ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION = True diff --git a/profiles/tests/test_allowed_data_field_restriction_flag.py b/profiles/tests/test_allowed_data_field_restriction_flag.py new file mode 100644 index 00000000..4cc1fe5d --- /dev/null +++ b/profiles/tests/test_allowed_data_field_restriction_flag.py @@ -0,0 +1,59 @@ +from unittest import mock + +from profiles.tests.factories import ProfileFactory, SensitiveDataFactory +from services.tests.factories import AllowedDataFieldFactory, ServiceConnectionFactory + + +def test_enable_allowed_data_fields_restriction_flag_false_shows_data( + user_gql_client, service, settings +): + settings.ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION = False + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + SensitiveDataFactory(profile=profile) + + query = """ + { + myProfile { + firstName + sensitivedata { + ssn + } + } + } + """ + + executed = user_gql_client.execute(query, service=service) + + assert executed["data"]["myProfile"]["firstName"] == profile.first_name + assert ( + executed["data"]["myProfile"]["sensitivedata"]["ssn"] + == profile.sensitivedata.ssn + ) + + +@mock.patch("logging.warning") +def test_enable_allowed_data_fields_restriction_flag_logs_warning_if_access_to_restricted_field( + mock_log, user_gql_client, service, settings +): + settings.ENABLE_ALLOWED_DATA_FIELDS_RESTRICTION = False + profile = ProfileFactory(user=user_gql_client.user) + ServiceConnectionFactory(profile=profile, service=service) + service.allowed_data_fields.add(AllowedDataFieldFactory(field_name="name")) + SensitiveDataFactory(profile=profile) + + query = """ + { + myProfile { + firstName + sensitivedata { + ssn + } + } + } + """ + + user_gql_client.execute(query, service=service) + + assert mock_log.call_count == 1