From 101c5a0b657b5cc2950dabd5beb58b9c2cef4f2f Mon Sep 17 00:00:00 2001 From: SteveDMurphy Date: Wed, 31 Jul 2024 16:58:35 +0100 Subject: [PATCH 1/5] fix: ignore valid 400 for user not existing --- data/saas/config/marigold_engage_config.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/data/saas/config/marigold_engage_config.yml b/data/saas/config/marigold_engage_config.yml index fc36812baa..d1c2311d7b 100644 --- a/data/saas/config/marigold_engage_config.yml +++ b/data/saas/config/marigold_engage_config.yml @@ -3,7 +3,7 @@ saas_config: name: Marigold Engage by Sailthru type: marigold_engage description: A sample schema representing the Marigold Engage via Sailthru connector for Fides - version: 0.1.0 + version: 0.1.1 connector_params: - name: domain @@ -30,6 +30,7 @@ saas_config: param_values: - name: email identity: email + ignore_errors: [400] delete: request_override: marigold_engage_user_delete param_values: From 754d44a450bbe9959be5ee2c06b3184e203d8476 Mon Sep 17 00:00:00 2001 From: SteveDMurphy Date: Wed, 31 Jul 2024 16:59:52 +0100 Subject: [PATCH 2/5] changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9aa51f2d7b..6276433de8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -47,6 +47,7 @@ The types of changes are: - Fixed Admin UI issue where banner would disappear in Experience Preview with GPC enabled [#5131](https://github.com/ethyca/fides/pull/5131) - Fixed not being able to edit a monitor from scheduled to not scheduled [#5114](https://github.com/ethyca/fides/pull/5114) - Migrating missing Fideslang 2.0 data categories [#5073](https://github.com/ethyca/fides/pull/5073) +- Fixes a Marigold Sailthru error when a user does not exist [#5145](https://github.com/ethyca/fides/pull/5145) ## [2.41.0](https://github.com/ethyca/fides/compare/2.40.0...2.41.0) From 23f0a8e5d8e0549523b51162ef0fa368b4d9b9a3 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Thu, 1 Aug 2024 09:28:04 -0700 Subject: [PATCH 3/5] Moving ignore_errors to request override and adding test --- data/saas/config/marigold_engage_config.yml | 1 - .../marigold_engage_request_overrides.py | 8 +++++--- .../ops/integration_tests/saas/connector_runner.py | 13 ++++++++----- .../saas/test_marigold_engage_task.py | 12 ++++++++++++ 4 files changed, 25 insertions(+), 9 deletions(-) diff --git a/data/saas/config/marigold_engage_config.yml b/data/saas/config/marigold_engage_config.yml index d1c2311d7b..a92ceca563 100644 --- a/data/saas/config/marigold_engage_config.yml +++ b/data/saas/config/marigold_engage_config.yml @@ -30,7 +30,6 @@ saas_config: param_values: - name: email identity: email - ignore_errors: [400] delete: request_override: marigold_engage_user_delete param_values: diff --git a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py index bc8109a905..e13418741d 100644 --- a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py +++ b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py @@ -67,10 +67,12 @@ def marigold_engage_user_read( method=HTTPMethod.GET, path="/user", query_params=signed_payload(secrets, payload), - ) + ), + ignore_errors=[400], ) - user = response.json() - output.append(user) + if response.ok: + user = response.json() + output.append(user) return output diff --git a/tests/ops/integration_tests/saas/connector_runner.py b/tests/ops/integration_tests/saas/connector_runner.py index 83d03a2b94..ebf9104849 100644 --- a/tests/ops/integration_tests/saas/connector_runner.py +++ b/tests/ops/integration_tests/saas/connector_runner.py @@ -100,6 +100,7 @@ async def access_request( access_policy: Policy, identities: Dict[str, Any], privacy_request_id: Optional[str] = None, + skip_collection_verification: Optional[bool] = False, ) -> Dict[str, List[Row]]: """Access request for a given access policy and identities""" @@ -142,11 +143,13 @@ async def access_request( identities, self.db, ) - # verify we returned at least one row for each collection in the dataset - for collection in self.dataset["collections"]: - assert len( - access_results[f"{fides_key}:{collection['name']}"] - ), f"No rows returned for collection '{collection['name']}'" + + if not skip_collection_verification: + # verify we returned at least one row for each collection in the dataset + for collection in self.dataset["collections"]: + assert len( + access_results[f"{fides_key}:{collection['name']}"] + ), f"No rows returned for collection '{collection['name']}'" return access_results async def strict_erasure_request( diff --git a/tests/ops/integration_tests/saas/test_marigold_engage_task.py b/tests/ops/integration_tests/saas/test_marigold_engage_task.py index 17a7cd7fdf..9443387b11 100644 --- a/tests/ops/integration_tests/saas/test_marigold_engage_task.py +++ b/tests/ops/integration_tests/saas/test_marigold_engage_task.py @@ -21,6 +21,18 @@ async def test_access_request( for user in access_results["marigold_engage_instance:user"]: assert user["keys"]["email"] == marigold_engage_identity_email + async def test_access_request_user_not_found( + self, + marigold_engage_runner: ConnectorRunner, + policy, + ): + access_results = await marigold_engage_runner.access_request( + access_policy=policy, + identities={"email": "notfound@example.com"}, + skip_collection_verification=True, + ) + assert access_results == {"marigold_engage_instance:user": []} + async def test_non_strict_erasure_request( self, marigold_engage_runner: ConnectorRunner, From 7e7c1b749e40ec48a6370b1f15806924b58b381e Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Mon, 5 Aug 2024 18:23:21 -0700 Subject: [PATCH 4/5] Adding test for errored response with HTTP 200 status code --- .../marigold_engage_request_overrides.py | 43 +++++++++++++++---- .../saas/test_marigold_engage_task.py | 31 +++++++++++++ 2 files changed, 65 insertions(+), 9 deletions(-) diff --git a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py index e13418741d..a963236389 100644 --- a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py +++ b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py @@ -2,16 +2,22 @@ import json from typing import Any, Dict, List +from loguru import logger + from fides.api.graph.traversal import TraversalNode from fides.api.models.policy import Policy from fides.api.models.privacy_request import PrivacyRequest from fides.api.schemas.saas.shared_schemas import HTTPMethod, SaaSRequestParams -from fides.api.service.connectors.saas.authenticated_client import AuthenticatedClient +from fides.api.service.connectors.saas.authenticated_client import ( + AuthenticatedClient, + RequestFailureResponseException, +) from fides.api.service.saas_request.saas_request_override_factory import ( SaaSRequestType, register, ) from fides.api.util.collection_util import Row +from fides.api.util.logger_context_utils import request_details @register("marigold_engage_test", [SaaSRequestType.TEST]) @@ -62,17 +68,36 @@ def marigold_engage_user_read( "lifetime": 1, }, } + request_params = SaaSRequestParams( + method=HTTPMethod.GET, + path="/user", + query_params=signed_payload(secrets, payload), + ) response = client.send( - SaaSRequestParams( - method=HTTPMethod.GET, - path="/user", - query_params=signed_payload(secrets, payload), - ), + request_params, ignore_errors=[400], ) - if response.ok: - user = response.json() - output.append(user) + + response_json = response.json() + error_msg = response_json.get("errormsg") + + context_logger = logger.bind( + **request_details( + client.get_authenticated_request(request_params), response + ) + ) + + if response.ok and not error_msg: + output.append(response_json) + elif error_msg and error_msg.startswith("User not found with"): + context_logger.info( + f"Connector request successful. Ignoring {error_msg.lower()}" + ) + else: + context_logger.error( + "Connector request failed with status code {}.", response.status_code + ) + raise RequestFailureResponseException(response=response) return output diff --git a/tests/ops/integration_tests/saas/test_marigold_engage_task.py b/tests/ops/integration_tests/saas/test_marigold_engage_task.py index 9443387b11..73e973cdb2 100644 --- a/tests/ops/integration_tests/saas/test_marigold_engage_task.py +++ b/tests/ops/integration_tests/saas/test_marigold_engage_task.py @@ -1,5 +1,11 @@ +import json +from unittest import mock +from unittest.mock import Mock + import pytest +from requests import Response +from fides.api.common_exceptions import FidesopsException from fides.api.models.policy import Policy from tests.ops.integration_tests.saas.connector_runner import ConnectorRunner @@ -33,6 +39,31 @@ async def test_access_request_user_not_found( ) assert access_results == {"marigold_engage_instance:user": []} + @mock.patch("fides.api.service.connectors.saas_connector.AuthenticatedClient.send") + async def test_access_request_errored_request( + self, + mock_send: Mock, + marigold_engage_runner: ConnectorRunner, + policy, + ): + response = Response() + response.status_code = 200 + response._content = json.dumps( + { + "errormsg": "Invalid email: notfound@example.com", + "errorcode": 11, + } + ).encode("utf-8") + + mock_send.return_value = response + + with pytest.raises(FidesopsException): + await marigold_engage_runner.access_request( + access_policy=policy, + identities={"email": "notfound@example.com"}, + skip_collection_verification=True, + ) + async def test_non_strict_erasure_request( self, marigold_engage_runner: ConnectorRunner, From b536dda350ecfe041806366f984dcaadfa160475 Mon Sep 17 00:00:00 2001 From: Adrian Galvan Date: Tue, 6 Aug 2024 10:42:08 -0700 Subject: [PATCH 5/5] Adding code comment --- .../marigold_engage_request_overrides.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py index a963236389..432283530d 100644 --- a/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py +++ b/src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py @@ -3,6 +3,7 @@ from typing import Any, Dict, List from loguru import logger +from starlette.status import HTTP_400_BAD_REQUEST from fides.api.graph.traversal import TraversalNode from fides.api.models.policy import Policy @@ -73,9 +74,12 @@ def marigold_engage_user_read( path="/user", query_params=signed_payload(secrets, payload), ) + + # This API endpoint returns an HTTP 400 for "user not found" and in some cases an HTTP 200 for errored responses. + # We want to ignore these errors and inspect the payload to determine if the request should be considered an error. response = client.send( request_params, - ignore_errors=[400], + ignore_errors=[HTTP_400_BAD_REQUEST], ) response_json = response.json()