Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Sailthru Integration: ignore valid 400 for user not existing #5145

Merged
merged 8 commits into from
Aug 6, 2024
Merged

Sailthru Integration: ignore valid 400 for user not existing #5145

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
101c5a0
fix: ignore valid 400 for user not existing
SteveDMurphy Jul 31, 2024
754d44a
changelog
SteveDMurphy Jul 31, 2024
23f0a8e
Moving ignore_errors to request override and adding test
galvana Aug 1, 2024
7e7c1b7
Adding test for errored response with HTTP 200 status code
galvana Aug 6, 2024
726db61
Merge branch 'main' into SteveDMurphy-fides-1082-ignore-400-sailthru
galvana Aug 6, 2024
b536dda
Adding code comment
galvana Aug 6, 2024
ded38d8
Merge branch 'main' into SteveDMurphy-fides-1082-ignore-400-sailthru
galvana Aug 6, 2024
743cd34
Merge branch 'main' into SteveDMurphy-fides-1082-ignore-400-sailthru
galvana Aug 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
2 changes: 1 addition & 1 deletion data/saas/config/marigold_engage_config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 5 additions & 3 deletions ...es/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,12 @@
method=HTTPMethod.GET,
path="/user",
query_params=signed_payload(secrets, payload),
)
),
ignore_errors=[400],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we're using a request_override for the user read, we can't define ignore_errors on the SaaS config, it needs to be defined on the client.send inside the request override. It's possible for a request override function to make multiple client.send calls so if we define ignore_errors at the SaaS config level, we couldn't programmatically determine which client.send to apply that to.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could define a variable that makes explicit for future programmers that we are ignoring the 400 error because the user might not exist, and also remove a magic number/list, by defining something like

error400ForUserNotFound = [400]

Copy link
Contributor

@Vagoasdf Vagoasdf Aug 1, 2024 •
edited by SteveDMurphy
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, Do we have an explanation from their docs on why an UserNotFound is a 400 error instead of a 404 ? Usually NotFound errors are 404.
Nevermind, found the reference on the Docs. I'ts odd that they override the usual 404 error.
Maybe we should check for the specific error message "User not found with {KEY}", just so we dont blank accept any 400 error

It did feel odd to see a 400 for this instead of the 404. Happy to see if we can dig in a bit more to ensure other errors aren't also being ignored. Sounds like that might be the case with the custom override we need to account for based on what @galvana was saying 👍🏽

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its better now. But i still think we should rename the variable, just so we have more clarity on what type of "400" error are we ignoring

)
user = response.json()
output.append(user)
if response.ok:
user = response.json()
output.append(user)

Check warning on line 75 in src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py

View check run for this annotation

Codecov / codecov/patch

src/fides/api/service/saas_request/override_implementations/marigold_engage_request_overrides.py#L74-L75 

Added lines #L74 - L75 were not covered by tests

galvana marked this conversation as resolved.
Show resolved Hide resolved
return output

Expand Down
13 changes: 8 additions & 5 deletions tests/ops/integration_tests/saas/connector_runner.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"""

Expand Down Expand Up @@ -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:
galvana marked this conversation as resolved.
Show resolved Hide resolved
# 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(
Expand Down
12 changes: 12 additions & 0 deletions tests/ops/integration_tests/saas/test_marigold_engage_task.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": "[email protected]"},
skip_collection_verification=True,
)
assert access_results == {"marigold_engage_instance:user": []}
galvana marked this conversation as resolved.
Show resolved Hide resolved

async def test_non_strict_erasure_request(
self,
marigold_engage_runner: ConnectorRunner,
Expand Down