From b199c4dec3863586b820aeb8eb767143029c76b1 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 8 Dec 2022 18:35:20 -0500 Subject: [PATCH 1/5] return empty dict when child transfer errors --- .../service/connectors/fides/fides_client.py | 1 + .../connectors/fides/test_fides_client.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/fides/api/ops/service/connectors/fides/fides_client.py b/src/fides/api/ops/service/connectors/fides/fides_client.py index da3fd2b506..82a1eac47f 100644 --- a/src/fides/api/ops/service/connectors/fides/fides_client.py +++ b/src/fides/api/ops/service/connectors/fides/fides_client.py @@ -216,5 +216,6 @@ def retrieve_request_results( privacy_request_id, response.json(), ) + return {} return response.json() diff --git a/tests/ops/service/connectors/fides/test_fides_client.py b/tests/ops/service/connectors/fides/test_fides_client.py index 8f008e2aeb..2496a4e7cc 100644 --- a/tests/ops/service/connectors/fides/test_fides_client.py +++ b/tests/ops/service/connectors/fides/test_fides_client.py @@ -355,3 +355,20 @@ def test_request_status_privacy_request( # or environment-specific issues, # let's not assume anything about the status here. assert statuses[0]["status"] is not None + + def test_retrieve_request_results_nonexistent_request( + self, authenticated_fides_client: FidesClient, policy + ): + """ + Tests that retrieving requests results for a nonexistent request + properly returns an empty dict. + + At this point, a nonexistent request behaves the same as a + legitimate request that does not output data. So we use this to + ensure that these requests are handled well by the client, + and simply return no data. + """ + result = authenticated_fides_client.retrieve_request_results( + "some_nonexistent_request", "some_nonexistent_rule" + ) + assert result == {} From cc546e5cd0cd0f22817e47d15c4a560f5ff0a5e0 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 8 Dec 2022 18:36:01 -0500 Subject: [PATCH 2/5] add and improve logging in fides connector and client --- .../service/connectors/fides/fides_client.py | 41 +++++++++++++++---- .../ops/service/connectors/fides_connector.py | 13 ++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/fides/api/ops/service/connectors/fides/fides_client.py b/src/fides/api/ops/service/connectors/fides/fides_client.py index 82a1eac47f..f052b74aeb 100644 --- a/src/fides/api/ops/service/connectors/fides/fides_client.py +++ b/src/fides/api/ops/service/connectors/fides/fides_client.py @@ -51,6 +51,9 @@ def __init__( def login(self) -> None: ul: UserLogin = UserLogin(username=self.username, password=self.password) + log.info( + f"Logging in to remote fides {self.uri} with username '{self.username}'..." + ) try: response = requests.post( f"{self.uri}{urls.V1_URL_PREFIX}{urls.LOGIN}", json=ul.dict() @@ -61,6 +64,9 @@ def login(self) -> None: if response.ok: self.token = response.json()["token_data"]["access_token"] + log.info( + f"Successfully logged in to remote fides {self.uri} with username '{self.username}'" + ) else: log.error(f"Error logging in on remote Fides {self.uri}") response.raise_for_status() @@ -104,6 +110,9 @@ def create_privacy_request( policy_key=policy_key, ) + log.info( + f"Creating privacy request with external_id {external_id} on remote fides {self.uri}..." + ) request: PreparedRequest = self.authenticated_request( method="POST", path=urls.V1_URL_PREFIX + urls.PRIVACY_REQUEST_AUTHENTICATED, @@ -118,7 +127,12 @@ def create_privacy_request( raise FidesError( f"Failed privacy request creation on remote Fides {self.uri} with failure message: {response.json()['failed'][0]['message']}" ) - return response.json()["succeeded"][0]["id"] + + pr_id = response.json()["succeeded"][0]["id"] + log.info( + f"Successfully created privacy request with id {pr_id} and external_id {external_id} on remote fides {self.uri}" + ) + return pr_id @sync async def poll_for_request_completion( @@ -137,6 +151,9 @@ async def poll_for_request_completion( f"Unable to poll for request completion. No token for Fides connector for server {self.uri}" ) + log.info( + f"Polling remote fides {self.uri} for completion of privacy request with id {privacy_request_id}..." + ) status: PrivacyRequestResponse = await poll_server_for_completion( privacy_request_id=privacy_request_id, server_url=self.uri, @@ -157,7 +174,7 @@ async def poll_for_request_completion( f"Privacy request [{privacy_request_id}] on remote Fides {self.uri} was denied. Look at the remote Fides for more information." ) if status.status == PrivacyRequestStatus.complete: - log.debug( + log.info( f"Privacy request [{privacy_request_id}] is complete on remote Fides {self.uri}!", ) return status @@ -170,6 +187,9 @@ def request_status(self, privacy_request_id: str = None) -> List[Dict[str, Any]] """ Return privacy request object that tracks its status """ + log.info( + f"Retrieving request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}..." + ) request: PreparedRequest = self.authenticated_request( method="GET", path=urls.V1_URL_PREFIX + urls.PRIVACY_REQUESTS, @@ -184,6 +204,9 @@ def request_status(self, privacy_request_id: str = None) -> List[Dict[str, Any]] ) response.raise_for_status() + log.info( + f"Retrieved request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}" + ) return response.json()["items"] def retrieve_request_results( @@ -197,6 +220,9 @@ def retrieve_request_results( Returns the filtered access results as a `Dict[str, List[Row]] """ try: + log.info( + f"Retrieving request results for privacy request {privacy_request_id} on remote fides {self.uri}..." + ) request = self.authenticated_request( method="get", path=f"{urls.V1_URL_PREFIX}{urls.PRIVACY_REQUEST_TRANSFER_TO_PARENT.format(privacy_request_id=privacy_request_id, rule_key=rule_key)}", @@ -205,17 +231,16 @@ def retrieve_request_results( response = self.session.send(request) except requests.exceptions.HTTPError as e: log.error( - "Error retrieving data from child server for privacy request %s: %s", - privacy_request_id, - e, + f"Error retrieving data from child server for privacy request {privacy_request_id}: {e}" ) if response.status_code != 200: log.error( - "Error retrieving data from child server for privacy request %s: %s", - privacy_request_id, - response.json(), + f"Error retrieving data from child server for privacy request {privacy_request_id}: {response.text}" ) return {} + log.info( + f"Retrieved request results for privacy request {privacy_request_id} on remote fides {self.uri}" + ) return response.json() diff --git a/src/fides/api/ops/service/connectors/fides_connector.py b/src/fides/api/ops/service/connectors/fides_connector.py index 405a33e587..c55bfc60c2 100644 --- a/src/fides/api/ops/service/connectors/fides_connector.py +++ b/src/fides/api/ops/service/connectors/fides_connector.py @@ -69,6 +69,7 @@ def test_connection(self) -> Optional[ConnectionTestStatus]: log.error(f"Error testing connection to remote Fides {str(e)}") return ConnectionTestStatus.failed + log.info(f"Successful connection test for {self.configuration.key}") return ConnectionTestStatus.succeeded def retrieve_data( @@ -84,6 +85,9 @@ def retrieve_data( raise FidesError( f"No identity data found for privacy request {privacy_request.id}, cannot execute Fides connector!" ) + log.info( + f"{self.configuration.key} starting retrieve_data for privacy request {privacy_request.id}..." + ) client: FidesClient = self.client() @@ -110,6 +114,9 @@ def retrieve_data( for rule in policy.get_rules_for_action(action_type=ActionType.access) } + log.info( + f"{self.configuration.key} finished retrieve_data for privacy request {privacy_request.id}" + ) return [results] def mask_data( @@ -126,6 +133,9 @@ def mask_data( raise FidesError( f"No identity data found for privacy request {privacy_request.id}, cannot execute Fides connector!" ) + log.info( + f"{self.configuration.key} starting mask_data for privacy request {privacy_request.id}..." + ) client: FidesClient = self.client() update_ct = 0 @@ -142,6 +152,9 @@ def mask_data( ) update_ct += 1 + log.info( + f"{self.configuration.key} finished mask_data for privacy request {privacy_request.id}" + ) return update_ct def close(self) -> None: From 6f2a8e1d93469ecbff3a0ff8fbc564a234f91ae7 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 8 Dec 2022 18:41:00 -0500 Subject: [PATCH 3/5] update changelog --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c3e02daea..d8ee6e95e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,9 @@ The types of changes are: ## [Unreleased](https://github.com/ethyca/fides/compare/2.2.1...main) +### Changed + +* Update Fides connector to better handle children with no access results [#2012](https://github.com/ethyca/fides/pull/2012) ## [2.2.1](https://github.com/ethyca/fides/compare/2.2.0...2.2.1) From 002ef749c288a09d89c06a227d39c35d83d16fc1 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 8 Dec 2022 18:45:02 -0500 Subject: [PATCH 4/5] Add missing changelog entry. This was missed in https://github.com/ethyca/fides/pull/2000 so just sliding it in here. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d8ee6e95e9..26390b6885 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ The types of changes are: ### Changed +* Update connection test endpoint to be effectively non-blocking [#2000](https://github.com/ethyca/fides/pull/2000) * Update Fides connector to better handle children with no access results [#2012](https://github.com/ethyca/fides/pull/2012) ## [2.2.1](https://github.com/ethyca/fides/compare/2.2.0...2.2.1) From a4d604a2ed01b97df2aa1a5cd8daa49939698791 Mon Sep 17 00:00:00 2001 From: Adam Sachs Date: Thu, 8 Dec 2022 22:47:00 -0500 Subject: [PATCH 5/5] clean up logging in request status retrieval --- .../service/connectors/fides/fides_client.py | 23 ++++++++++++++----- 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/src/fides/api/ops/service/connectors/fides/fides_client.py b/src/fides/api/ops/service/connectors/fides/fides_client.py index f052b74aeb..23eda8576a 100644 --- a/src/fides/api/ops/service/connectors/fides/fides_client.py +++ b/src/fides/api/ops/service/connectors/fides/fides_client.py @@ -187,9 +187,15 @@ def request_status(self, privacy_request_id: str = None) -> List[Dict[str, Any]] """ Return privacy request object that tracks its status """ - log.info( - f"Retrieving request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}..." - ) + if privacy_request_id: + log.info( + f"Retrieving request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}..." + ) + else: + log.info( + f"Retrieving request status for all privacy requests on remote fides {self.uri}..." + ) + request: PreparedRequest = self.authenticated_request( method="GET", path=urls.V1_URL_PREFIX + urls.PRIVACY_REQUESTS, @@ -204,9 +210,14 @@ def request_status(self, privacy_request_id: str = None) -> List[Dict[str, Any]] ) response.raise_for_status() - log.info( - f"Retrieved request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}" - ) + if privacy_request_id: + log.info( + f"Retrieved request status for privacy request {privacy_request_id if privacy_request_id else ''} on remote fides {self.uri}" + ) + else: + log.info( + f"Retrieved request status for all privacy requests on remote fides {self.uri}" + ) return response.json()["items"] def retrieve_request_results(