Pause manual erasure requests #4115
Conversation
Passing run #4348 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
||||||||||||||||||||
galvana
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4115 +/- ##
==========================================
+ Coverage 87.50% 87.53% +0.02%
==========================================
Files 326 326
Lines 20324 20417 +93
Branches 2640 2654 +14
==========================================
+ Hits 17784 17871 +87
- Misses 2081 2083 +2
- Partials 459 463 +4
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
nice work @galvana , looks good for a first pass! like you said, def some code consolidation opportunities but that's fine to defer, especially since i think this is still pretty straightforward to follow - though there's one i think it'd be worth addressing for now.
there are a couple other points that i'd just like to make sure we've considered and/or are being communicated to product (and potentially documented in release notes/user guides), especially the one about persisting the "confirmation" signal.
| parsed_data.dict(), | ||
| ) | ||
|
|
||
| def get_manual_webhook_input_strict( | ||
| def cache_manual_webhook_erasure_input( |
There was a problem hiding this comment.
i know we're following the example from the access input as a first pass, and i think that's probably fine. but i'd want to make sure we flag this, since i could see it leading to problems if users (or our product team) doesn't realize the implication here.
specifically, there's no sort of persisted record of the confirmation that the records have been deleted from the manual integration. there'd be no way of tracking that admin X confirmed at time Y that person Z's records were deleted from system A (whoops ran out of letters) -- at least, not one that we can rely on since we have to treat the cache as ephemeral. i think it's true that we don't persist directly in our db the manual input data for access requests (though maybe we should?) but it's a bit different there because that input is packaged up as part of the access result package and that package is put into storage and/or sent back to the end user -- so it is persisted somewhere.
this is certainly not listed out as an explicit requirement here, but it feels like a reporting/audit requirement that could potentially have legal implications if/when this gets implemented. not saying we need to solve it here in a first pass, i'd just want this to be flagged before we release!
There was a problem hiding this comment.
We do not need a persisted record of the confirmation since a successfully completed deletion requires the confirmation we can infer that a confirmation was made. CC @rsilvery
There was a problem hiding this comment.
OK, that's fine for me in terms of an MVP, but there's some tension with the other piece of this implementation as noted here.
maybe we can ticket for a follow up, just to track it at least?
There was a problem hiding this comment.
Placeholder ticket #4164
| data: Dict[str, Any] = manual_webhook.erasure_fields_schema.parse_obj( | ||
| cached_results | ||
| ).dict(exclude_unset=True) | ||
| if set(data.keys()) != set( |
There was a problem hiding this comment.
i could be following wrong, but is there a narrow (and perhaps too contrived) edge case where the value has been set to False for a given field (i.e. someone actually checks and unchecks a field) and we essentially judge this as OK, we're ready to proceed, when in reality they still have one or more fields outstanding? maybe that's a desired behavior, i.e. if an admin has explicitly unchecked a box then its up to their discretion to allow the request to proceed...
anyway, sorta just curious whether that's a legitimate possibility and if so whethere you'd thought thru it at all...
There was a problem hiding this comment.
I think this is ok, but that's only because it's hard for us to determine when it's acceptable to proceed with a manual erasure. There are cases when certain fields might not exist in the manual data source so we'll still want to proceed even if no fields are checked.
There was a problem hiding this comment.
OK fair enough, i mean i think there's a bit of tension between that and what we're saying here, i.e. that completed deletion infers a confirmation was made. if we're allowing deletion to proceed without requiring confirmation of all fields, someone who is auditing things really has no way of determining what was deleted on a particular DSR request.
totally acknowledge that this is an edge case and the requirement generally is perhaps beyond MVP scope. i just think that a robust/long term implementation probably includes some sort of persisted tracking of what was confirmed on a particular DSR 👍
| dependencies=[Security(verify_oauth_client, scopes=[PRIVACY_REQUEST_UPLOAD_DATA])], | ||
| response_model=None, | ||
| ) | ||
| def upload_manual_webhook_erasure_data( |
There was a problem hiding this comment.
i know you specifically mentioned not DRYing this code up, and that's fine, but this is basically identical to the above endpoint besides cache_manual_webhook_erasure_input instead of cache_manual_webhook_access_input and then one word differences in log messages. IMO it's worth some minimal effort on DRYing this up for readability and so that we can be sure not to have any drift if/when any of this logic needs to be updated.
There was a problem hiding this comment.
Good point about DRYing this portion up to clearly show these two endpoints are related. I extracted the shared logic into a function so now we can just do this for each endpoint
_handle_manual_webhook_input(
action="access",
connection_config=connection_config,
privacy_request_id=privacy_request_id,
db=db,
input_data=input_data,
)
There was a problem hiding this comment.
awesome, thank you @galvana!
Description Of Changes
Updated the privacy request workflow to pause any erasure requests with manual webhook connection configs before completing the request. The privacy request is flagged as
requires inputand waits for a user to mark which fields have been manually deleted before the request can be completed.I kept the changes very light, I just made enough changes to pause the request in the UI and give the user a way to track which fields have been deleted. The manual erasure inputs are not used for any request execution. The changes are also not very DRY, often just erasure duplicates of existing manual access functionality. The requirements around this ticket were a bit loose (originally scoped as a defect) so I didn't want to integrate this too closely with the existing manual access workflow.
Code Changes
run_privacy_requestfunction ofrequest_runner_service.pyto pause erasure privacy requests if there are any manual webhook connection configs enabled.Steps to Confirm
Manual ProcessintegrationCustomize DSRand add some fieldsRequires Inputstate, either immediately of after clickingapprove(based on your test environment's settings).Pre-Merge Checklist
CHANGELOG.md