Backend: Updating or Deleting Access Webhooks [#1388][#1389] #1394
Conversation
…maining active manual webhooks configured. If not, queue any Privacy Requests stuck in "requires_input" for processing.
…for a privacy request in strict mode. If it fails (no data saved, extra field saved, field missing), return checked=True, so the user knows they need to reupload data for this webhook before it can be submitted. Return the data in non-strict mode, so we just show the overlap between the data saved and the fields defined.
…this is the only module it's being called - both where you update and delete a connection config.
| ) | ||
| checked = False |
There was a problem hiding this comment.
Previously, "checked=False" was only returned if there was no data in the cache, so Chris's UI would show that that webhook still needed to be reviewed and the submit DSR button would be grayed out.
In addition to this use case, we now show checked=False if the webhook definition has changed, which prompts the user to have to re-review.
The variable name checked might need to be updated in the future, but I left it like this for now, because it's the field Chris is using in the FE.
| data = privacy_request.get_manual_webhook_input_non_strict( | ||
| manual_webhook=access_manual_webhook |
There was a problem hiding this comment.
This is the endpoint that returns data that was already uploaded for a webhook if applicable.
I load the webhook data in non-strict mode here if there's an issue, which really just returns the overlap between the saved fields and the new webhook field definitions.
Note that I don't update the data that is actually cached, I just return this "safe mode version". This keeps this endpoint idempotent in case it is hit multiple times or by different people. Regardless, until the webhook fields are updated by a user to match the latest webhook definition, the privacy request itself will not be able to be submitted, because it will also try to access the webhook data in "strict mode" and fail.
| @@ -505,27 +506,50 @@ def cache_manual_webhook_input( | |||
| parsed_data.dict(), | |||
| ) | |||
|
|
|||
| def get_manual_webhook_input( | |||
| def get_manual_webhook_input_strict( | |||
There was a problem hiding this comment.
Updated this existing method to "strict mode" which used to just look for extra fields and/or no data cached, but now it also looks for missing fields.
| raise NoCachedManualWebhookEntry( | ||
| f"No data cached for privacy_request_id '{self.id}' for connection config '{manual_webhook.connection_config.key}'" | ||
| ) | ||
|
|
||
| def get_manual_webhook_input_non_strict( |
There was a problem hiding this comment.
Added a "non strict" mode to load webhook data where we really just preserve the overlap.
| # Access Manual Webhooks are cascade deleted if their ConnectionConfig is deleted, | ||
| # so we queue any privacy requests that are no longer blocked by webhooks | ||
| if connection_type == ConnectionType.manual_webhook: | ||
| requeue_requires_input_requests(db) |
There was a problem hiding this comment.
As a first step, I am just re-queuing applicable privacy requests if a connection is disabled or deleted. I omitted adding a recurring task to also check if any of these fell through the cracks in the interest of time. I think this kind of thing could be added as a piece of a broader work where we look for privacy requests of multiple statuses that are stuck.
| @@ -859,6 +913,17 @@ def test_delete_manual_webhook_connection_config( | |||
| .first() | |||
| is None | |||
| ) | |||
| assert ( | |||
| mock_queue.called | |||
| ), "Deleting this last webhook caused 'requires_input' privacy requests to be queued" | |||
There was a problem hiding this comment.
Just to confirm this is deleting the last webhook, so Fidesops is sure there's no more input required from the user before dispatch?
There was a problem hiding this comment.
Yes, the last webhook is deleted, so the privacy request just needs to be re-queued. Data is no longer required for the webhook. If any data was previously uploaded, it would be ignored.
| assert response.json() == { | ||
| "checked": False, | ||
| "fields": { | ||
| "id_number": None, |
There was a problem hiding this comment.
Using the user preferred naming for this field in the response is a nice touch
| def requeue_requires_input_requests(db: Session) -> None: | ||
| """ | ||
| Queue privacy requests with request status "requires_input" if they are no longer blocked by | ||
| access manual webhooks. |
There was a problem hiding this comment.
It'd be better to say explicitly this is if all access manual webhooks are disabled or deleted.
There was a problem hiding this comment.
Thanks Sean, I've tried to improve this docstring
seanpreston
deleted the
fidesops_1388_1389_webhooks_updated_or_deleted
branch
Purpose
Address some edge cases that surfaced in testing #1377
requires_inputstate with no way to move them forwardChanges
view_uploaded_manual_webhook_dataendpoint, load cached webhook data for a privacy request in strict mode. If it fails (no data saved, extra field saved, field missing), returnchecked=False, so the user knows they need to re-upload data for this webhook before it can be submitted.checked=Truewill prevent the privacy request from being submittedChecklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #1388
Fixes #1389