PROD-2700 Implement Soft Delete for PrivacyRequests #5321

erosselli · 2024-09-23T20:31:28Z

Description Of Changes

Adds support for soft deleting PrivacyRequests. The deletion is defined by the new column deleted_at, and we also stored who deleted it in the deleted_by column. A new DELETE /privacy-request/id endpoint is added in order to allow soft deleting requests, as well as an endpoint to bulk delete multiple requests at the same time POST /privacy-request/bulk/delete.

Code Changes

Added columns deleted_at and deleted_by to PrivacyRequest , both nullable
Added a new endpoint to allow soft deleting privacy requests, DELETE /privacy-request/id and for bulk deleting them POST /privacy-request/bulk/delete
Added a new optional param to the existing search / list privacy requests endpoints, include_deleted_requests, that is False by default
Added validation to the existing privacy request endpoints that modify privacy request data, to throw a 422 error when attempting to update any data related to a deleted privacy request

Steps to Confirm

Run the admin UI and the privacy center (or alternatively, just run the entire test project)
Create four different privacy requests, each with a different email, by clicking the "Access my data" blob of the privacy center
If you go to the Admin UI, you should see all privacy requests
Open SwaggerUI (or your Postman collection if you prefer) and call the DELETE endpoint with the id of one of the privacy requests
Next, use the bulk delete endpoint to delete two other requests
Go back to Admin UI -- you should now see only one privacy request, the one that hasn't been deleted

Error cases

Trying to do anything that modifies the deleted privacy request should cause a 422 error. Some examples:

approving the request
deleting the request
restarting the request

Pre-Merge Checklist

vercel · 2024-09-23T20:31:32Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment

Name	Status	Preview	Comments	Updated (UTC)
fides-plus-nightly	⬜️ Ignored (Inspect)	Visit Preview		Sep 24, 2024 2:50pm

erosselli · 2024-09-23T20:31:53Z

src/fides/api/alembic/migrations/versions/75bb9ee843f5_add_soft_delete_for_privacy_request.py

+revision = "75bb9ee843f5"
+down_revision = "9de4bb76307a"


note to self: remember to update before merging

erosselli · 2024-09-23T20:32:38Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

 def get_privacy_request_or_error(
-    db: Session, privacy_request_id: str
+    db: Session, privacy_request_id: str, error_if_deleted: Optional[bool] = True


If the privacy request is deleted, we error by default, unless the error_if_deleted argument is set to False

erosselli · 2024-09-23T20:33:06Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

@@ -434,6 +443,7 @@ def _filter_privacy_request_queryset(
    external_id: Optional[str] = None,
    action_type: Optional[ActionType] = None,
    include_consent_webhook_requests: Optional[bool] = False,
+    include_deleted_requests: Optional[bool] = False,


same here, by default we will filter out deleted requests, unless explicitly included

erosselli · 2024-09-23T20:34:04Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

-    get_privacy_request_or_error(db, privacy_request_id)
+    get_privacy_request_or_error(db, privacy_request_id, error_if_deleted=False)


I allow get operations on the deleted privacy request , but not update ones. TBH there weren't any specific requirements regarding this on the ticket, so I used my best judgement. Any feedback / opinions are welcome

This is a great thing to run by Michael early on! There's a case to be made either way.

Maybe I would strive for consistency here:get_request_status_logs, view_uploaded_manual_webhook_data, view_uploaded_erasure_manual_webhook_data, and get_individual_privacy_request_tasks return very specific details about potentially deleted privacy requests by default.

But the search endpoint which is the place to get primary privacy request details does not return deleted privacy requests by default. This latter detail tips me towards wanting to hide everything by default or add query params to the GET ones so the default is "don't show" instead of "show"

erosselli · 2024-09-23T20:35:33Z

src/fides/api/models/privacy_request.py

+        self.save(db)
+        # TODO: do we need to remove cache keys?
+


the question is because the "real" delete method (defined right above) does the following:

cache: FidesopsRedis = get_cache() all_keys = get_all_cache_keys_for_privacy_request(privacy_request_id=self.id) for key in all_keys: cache.delete(key) for provided_identity in self.provided_identities: # type: ignore[attr-defined] provided_identity.delete(db=db)

great question, I wouldn't yet since you're still allowing these privacy requests to come up in search with certain query params.

erosselli · 2024-09-23T20:36:18Z

src/fides/api/service/privacy_request/email_batch_service.py

            .filter(PrivacyRequest.status == PrivacyRequestStatus.awaiting_email_send)
+            .filter(PrivacyRequest.deleted_at.is_(None))
            .order_by(PrivacyRequest.created_at.asc())  # oldest first


again, there were no specfic requirements here, but I think it makes sense not to send emails for deleted requests

erosselli · 2024-09-23T20:37:16Z

src/fides/api/service/privacy_request/request_runner_service.py

+        if privacy_request.deleted_at is not None:
+            logger.info(
+                "Terminating privacy request {}: request deleted.", privacy_request.id
+            )
+            return


same here, I didn't think it made sense to run an access request for a deleted privacy reqyest

erosselli · 2024-09-23T20:38:26Z

tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py

+    # FIXME: don't skip this
+    @pytest.mark.skip("temporary cause it hangs?")


Un related to this PR, see #5301 . I've skipped it just so that this file would run on the CI. I can remove the skip before merging

cypress · 2024-09-23T20:46:39Z

fides Run #10085

Run Properties: Passed #10085 • 306a46123a ℹ️: Merge f465edfc8ebf12d0c2fe4fd18c4ce7d9cfff9c39 into 0a0ea4f400ca6d1e10ad3b154f90...

Project	`fides`
Branch Review	`refs/pull/5321/merge`
Run status	`Passed #10085`
Run duration	`00m 38s`
Commit	`306a46123a ℹ️: Merge f465edfc8ebf12d0c2fe4fd18c4ce7d9cfff9c39 into 0a0ea4f400ca6d1e10ad3b154f90...`
Committer	`erosselli`
View all properties for this run ↗︎

Test results
Failures	`0`
Flaky	`0`
Pending	`0`
Skipped	`0`
Passing	`4`
⚠️ You've recorded test results over your free plan limit. Upgrade your plan to view test results.
View all changes introduced in this branch ↗︎

erosselli

Just wanted to call out that I tried to filter out the deleted requests from the places where it made sense, but also I don't know if I found all the places where we get a list of privacy requests. Wanted to point it out to get some extra eyes around that if possible

pattisdr · 2024-09-24T18:34:32Z

starting review -

pattisdr

This looks pretty good but let's step back for a second and clarify with product about expected behavior around "soft deletes". What should be able to be retrieved about a privacy request once it has been soft deleted?

pattisdr · 2024-09-24T18:35:34Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

@@ -2441,3 +2488,80 @@ def request_task_async_callback(
    queue_request_task(request_task, privacy_request_proceed=True)

    return {"task_queued": True}
+
+
+@router.delete(


Since this isn't a hard DELETE and it shows up under GET requests, I'd consider a different http method, maybe patch?

pattisdr · 2024-09-24T19:26:48Z

src/fides/api/alembic/migrations/versions/75bb9ee843f5_add_soft_delete_for_privacy_request.py

+    op.create_foreign_key(
+        "privacyrequest_deleted_at_key",
+        "privacyrequest",
+        "fidesuser",
+        ["deleted_by"],
+        ["id"],
+        ondelete="SET NULL",
+    )


This isn't that dangerous of an operation, adding a nullable FK, but it could prevent privacy requests from being written while the table is being scanned? So that might be something we need to call out. If we're concerned about it, we could also add the constraint as not valid, and then validate the constraint in the other. The privacy request table can be large https://squawkhq.com/docs/adding-foreign-key-constraint/

Alternatively, we might consider removing the FK constraint. This has an added benefit that we can track if the root user made the change, which is not technically a real user. This would be similar to AuditLog.user_id instead

ah that's a good idea I think, it may be overkill to have the FK constraint anyways -- I was following what was done for the approved_by column (I think), but it could be argued that this is a different use case anyways

pattisdr · 2024-09-24T19:32:29Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+            continue
+
+        privacy_request.soft_delete(db, client.user_id)
+        succeeded.append(privacy_request)


While it's nice to get to re-use the BulkPostPrivacyRequests schema, since the single delete API endpoint returns a 204, this feels a little jarring to return the whole object.

Perhaps we create a new schema, where the "succeeded" list has just the privacy request id, deleted_at, and deleted_by? or similar

pattisdr · 2024-09-24T19:47:17Z

src/fides/api/models/privacy_request.py

+        self.save(db)
+        # TODO: do we need to remove cache keys?
+


great question, I wouldn't yet since you're still allowing these privacy requests to come up in search with certain query params.

pattisdr · 2024-09-24T19:52:35Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

+    privacy_request.soft_delete(db, user_id)
+
+
+@router.post(


maybe patch here as well? what do you think?

pattisdr · 2024-09-24T20:19:42Z

src/fides/api/api/v1/endpoints/privacy_request_endpoints.py

-    get_privacy_request_or_error(db, privacy_request_id)
+    get_privacy_request_or_error(db, privacy_request_id, error_if_deleted=False)


This is a great thing to run by Michael early on! There's a case to be made either way.

Maybe I would strive for consistency here:get_request_status_logs, view_uploaded_manual_webhook_data, view_uploaded_erasure_manual_webhook_data, and get_individual_privacy_request_tasks return very specific details about potentially deleted privacy requests by default.

But the search endpoint which is the place to get primary privacy request details does not return deleted privacy requests by default. This latter detail tips me towards wanting to hide everything by default or add query params to the GET ones so the default is "don't show" instead of "show"

pattisdr · 2024-09-24T20:22:35Z

src/fides/api/service/privacy_request/email_batch_service.py

            .filter(PrivacyRequest.status == PrivacyRequestStatus.awaiting_email_send)
+            .filter(PrivacyRequest.deleted_at.is_(None))
            .order_by(PrivacyRequest.created_at.asc())  # oldest first


erosselli added 2 commits September 23, 2024 16:35

Allow soft deleting privacy requests

d0ba059

Add some validation to other endpoints

f364c0b

erosselli commented Sep 23, 2024

View reviewed changes

erosselli added 3 commits September 23, 2024 17:57

static checks

c0e5686

Add bulk soft delete endpoint

189bb09

static checks

71cf98a

erosselli marked this pull request as ready for review September 24, 2024 14:30

erosselli requested a review from pattisdr September 24, 2024 14:30

erosselli added 2 commits September 24, 2024 11:32

Add changelog entry

91eb6cd

Fix migration

f465edf

erosselli commented Sep 24, 2024

View reviewed changes

pattisdr reviewed Sep 24, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

erosselli commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

erosselli Sep 23, 2024

erosselli Sep 23, 2024

erosselli Sep 23, 2024

erosselli Sep 23, 2024

pattisdr Sep 24, 2024

erosselli Sep 23, 2024

pattisdr Sep 24, 2024

erosselli Sep 23, 2024

pattisdr Sep 24, 2024

erosselli Sep 23, 2024

erosselli Sep 23, 2024

cypress bot commented Sep 23, 2024 •

edited

Loading

erosselli left a comment

pattisdr commented Sep 24, 2024

pattisdr left a comment

pattisdr Sep 24, 2024

pattisdr Sep 24, 2024

erosselli Sep 24, 2024

pattisdr Sep 24, 2024

pattisdr Sep 24, 2024

pattisdr Sep 24, 2024

pattisdr Sep 24, 2024

pattisdr Sep 24, 2024

		get_privacy_request_or_error(db, privacy_request_id)
		get_privacy_request_or_error(db, privacy_request_id, error_if_deleted=False)

		# FIXME: don't skip this
		@pytest.mark.skip("temporary cause it hangs?")

		revision = "75bb9ee843f5"
		down_revision = "9de4bb76307a"

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

Are you sure you want to change the base?

PROD-2700 Implement Soft Delete for PrivacyRequests #5321

Conversation

erosselli commented Sep 23, 2024 • edited Loading

Description Of Changes

Code Changes

Steps to Confirm

Error cases

Pre-Merge Checklist

vercel bot commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cypress bot commented Sep 23, 2024 • edited Loading

erosselli left a comment

Choose a reason for hiding this comment

pattisdr commented Sep 24, 2024

pattisdr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erosselli commented Sep 23, 2024 •

edited

Loading

vercel bot commented Sep 23, 2024 •

edited

Loading

cypress bot commented Sep 23, 2024 •

edited

Loading