From 402e3c4264256acfad06c7981dbf32a79685ed51 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Fri, 3 Jun 2022 11:03:14 -0500 Subject: [PATCH 01/15] Add the ability to cancel a pending task. The celery task is not actually cancelled yet. - Track cancel reason, datetime cancelled, and add a new cancelled status. --- .../privacy-requests/RequestBadge.tsx | 4 ++ .../privacy-requests/RequestFilters.tsx | 1 + .../src/features/privacy-requests/types.ts | 1 + .../api/v1/endpoints/drp_endpoints.py | 40 +++++++++++- src/fidesops/api/v1/urn_registry.py | 1 + src/fidesops/models/privacy_request.py | 14 +++++ src/fidesops/schemas/drp_privacy_request.py | 7 +++ .../service/drp/drp_fidesops_mapper.py | 1 + .../ed1b00ff963d_cancel_privacy_request.py | 45 +++++++++++++ tests/api/v1/endpoints/test_drp_endpoints.py | 63 +++++++++++++++++++ 10 files changed, 176 insertions(+), 1 deletion(-) create mode 100644 src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py diff --git a/clients/admin-ui/src/features/privacy-requests/RequestBadge.tsx b/clients/admin-ui/src/features/privacy-requests/RequestBadge.tsx index e8e2c7f68..0c3638500 100644 --- a/clients/admin-ui/src/features/privacy-requests/RequestBadge.tsx +++ b/clients/admin-ui/src/features/privacy-requests/RequestBadge.tsx @@ -18,6 +18,10 @@ export const statusPropMap: { bg: 'red.500', label: 'Denied', }, + canceled: { + bg: 'red.600', + label: 'Canceled', + }, error: { bg: 'red.800', label: 'Error', diff --git a/clients/admin-ui/src/features/privacy-requests/RequestFilters.tsx b/clients/admin-ui/src/features/privacy-requests/RequestFilters.tsx index fd369f159..aa8aef09c 100644 --- a/clients/admin-ui/src/features/privacy-requests/RequestFilters.tsx +++ b/clients/admin-ui/src/features/privacy-requests/RequestFilters.tsx @@ -107,6 +107,7 @@ const RequestFilters: React.FC = () => { + diff --git a/clients/admin-ui/src/features/privacy-requests/types.ts b/clients/admin-ui/src/features/privacy-requests/types.ts index 838826bc1..74e5f00ca 100644 --- a/clients/admin-ui/src/features/privacy-requests/types.ts +++ b/clients/admin-ui/src/features/privacy-requests/types.ts @@ -5,6 +5,7 @@ export type PrivacyRequestStatus = | "error" | "in_processing" | "paused" + | "canceled" | "pending"; export interface DenyPrivacyRequest { diff --git a/src/fidesops/api/v1/endpoints/drp_endpoints.py b/src/fidesops/api/v1/endpoints/drp_endpoints.py index ac375e912..4ac057813 100644 --- a/src/fidesops/api/v1/endpoints/drp_endpoints.py +++ b/src/fidesops/api/v1/endpoints/drp_endpoints.py @@ -6,6 +6,7 @@ from sqlalchemy.orm import Session from starlette.status import ( HTTP_200_OK, + HTTP_400_BAD_REQUEST, HTTP_404_NOT_FOUND, HTTP_422_UNPROCESSABLE_ENTITY, HTTP_424_FAILED_DEPENDENCY, @@ -16,14 +17,18 @@ from fidesops.api import deps from fidesops.api.v1 import scope_registry as scopes from fidesops.api.v1 import urn_registry as urls +from fidesops.api.v1.endpoints.privacy_request_endpoints import ( + get_privacy_request_or_error, +) from fidesops.core.config import config from fidesops.models.policy import DrpAction, Policy -from fidesops.models.privacy_request import PrivacyRequest +from fidesops.models.privacy_request import PrivacyRequest, PrivacyRequestStatus from fidesops.schemas.drp_privacy_request import ( DRP_VERSION, DrpDataRightsResponse, DrpIdentity, DrpPrivacyRequestCreate, + DrpRevokeRequest, ) from fidesops.schemas.privacy_request import PrivacyRequestDRPStatusResponse from fidesops.schemas.redis_cache import PrivacyRequestIdentity @@ -177,3 +182,36 @@ def get_drp_data_rights(*, db: Session = Depends(deps.get_db)) -> DrpDataRightsR return DrpDataRightsResponse( version=DRP_VERSION, api_base=None, actions=actions, user_relationships=None ) + + +@router.post( + urls.DRP_REVOKE, + dependencies=[ + Security(verify_oauth_client, scopes=[scopes.PRIVACY_REQUEST_REVIEW]) + ], + response_model=PrivacyRequestDRPStatusResponse, +) +def revoke_request( + *, db: Session = Depends(deps.get_db), data: DrpRevokeRequest +) -> PrivacyRequestDRPStatusResponse: + """ + Revoke a pending privacy request. + """ + privacy_request: PrivacyRequest = get_privacy_request_or_error(db, data.request_id) + + if privacy_request.status != PrivacyRequestStatus.pending: + raise HTTPException( + status_code=HTTP_400_BAD_REQUEST, + detail=f"Invalid revoke request. Can only revoke `pending` requests. " + f"Privacy request '{privacy_request.id}' status = {privacy_request.status.value}.", + ) + + logger.info(f"Canceling privacy request '{privacy_request.id}'") + privacy_request.cancel_processing(db, cancel_reason=data.reason) + + return PrivacyRequestDRPStatusResponse( + request_id=privacy_request.id, + received_at=privacy_request.requested_at, + status=DrpFidesopsMapper.map_status(privacy_request.status), + reason=data.reason, + ) diff --git a/src/fidesops/api/v1/urn_registry.py b/src/fidesops/api/v1/urn_registry.py index 8fc323b82..46f93c141 100644 --- a/src/fidesops/api/v1/urn_registry.py +++ b/src/fidesops/api/v1/urn_registry.py @@ -100,3 +100,4 @@ DRP_EXERCISE = "/drp/exercise" DRP_STATUS = "/drp/status" DRP_DATA_RIGHTS = "/drp/data-rights" +DRP_REVOKE = "/drp/revoke" diff --git a/src/fidesops/models/privacy_request.py b/src/fidesops/models/privacy_request.py index 0ee2062fe..908a20acd 100644 --- a/src/fidesops/models/privacy_request.py +++ b/src/fidesops/models/privacy_request.py @@ -59,6 +59,7 @@ class PrivacyRequestStatus(str, EnumType): in_processing = "in_processing" complete = "complete" paused = "paused" + canceled = "canceled" error = "error" @@ -117,6 +118,9 @@ class PrivacyRequest(Base): backref="privacy_requests", ) + cancel_reason = Column(String) + canceled_at = Column(DateTime(timezone=True), nullable=True) + # passive_deletes="all" prevents execution logs from having their privacy_request_id set to null when # a privacy_request is deleted. We want to retain for record-keeping. execution_logs = relationship( @@ -380,6 +384,16 @@ def start_processing(self, db: Session) -> None: self.started_processing_at = datetime.utcnow() self.save(db=db) + def cancel_processing(self, db: Session, cancel_reason: Optional[str]) -> None: + """Cancels a privacy request. Currently should only cancel 'pending' tasks""" + if self.canceled_at is not None: + self.status = PrivacyRequestStatus.canceled + self.cancel_reason = cancel_reason + self.canceled_at = datetime.utcnow() + self.save(db) + + # TODO cancel celery task PrivacyRequestRunner.submit + def error_processing(self, db: Session) -> None: """Mark privacy request as errored, and note time processing was finished""" self.update( diff --git a/src/fidesops/schemas/drp_privacy_request.py b/src/fidesops/schemas/drp_privacy_request.py index 3967ab5bc..f2e052df4 100644 --- a/src/fidesops/schemas/drp_privacy_request.py +++ b/src/fidesops/schemas/drp_privacy_request.py @@ -66,3 +66,10 @@ class DrpDataRightsResponse(BaseSchema): api_base: Optional[str] actions: List[DrpAction] user_relationships: Optional[List[str]] + + +class DrpRevokeRequest(BaseSchema): + """DRP Data Rights Revoke Request Body""" + + request_id: str + reason: Optional[str] diff --git a/src/fidesops/service/drp/drp_fidesops_mapper.py b/src/fidesops/service/drp/drp_fidesops_mapper.py index 16b45c599..0ba32ab68 100644 --- a/src/fidesops/service/drp/drp_fidesops_mapper.py +++ b/src/fidesops/service/drp/drp_fidesops_mapper.py @@ -50,6 +50,7 @@ def map_status( PrivacyRequestStatus.complete: PrivacyRequestDRPStatus.fulfilled, PrivacyRequestStatus.paused: PrivacyRequestDRPStatus.in_progress, PrivacyRequestStatus.error: PrivacyRequestDRPStatus.expired, + PrivacyRequestStatus.canceled: PrivacyRequestDRPStatus.revoked, } try: return PRIVACY_REQUEST_STATUS_TO_DRP_MAPPING[status] diff --git a/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py b/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py new file mode 100644 index 000000000..7f0500913 --- /dev/null +++ b/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py @@ -0,0 +1,45 @@ +"""cancel privacy request + +Revision ID: ed1b00ff963d +Revises: 3a7c5fb119c9 +Create Date: 2022-06-03 15:45:14.584540 + +""" +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "ed1b00ff963d" +down_revision = "3a7c5fb119c9" +branch_labels = None +depends_on = None + + +def upgrade(): + op.add_column( + "privacyrequest", sa.Column("cancel_reason", sa.String(), nullable=True) + ) + op.add_column( + "privacyrequest", + sa.Column("canceled_at", sa.DateTime(timezone=True), nullable=True), + ) + op.execute("alter type privacyrequeststatus add value 'canceled'") + + +def downgrade(): + op.drop_column("privacyrequest", "canceled_at") + op.drop_column("privacyrequest", "cancel_reason") + + op.execute("delete from privacyrequest where status in ('canceled')") + + op.execute("alter type privacyrequeststatus rename to privacyrequeststatus_old") + op.execute( + "create type privacyrequeststatus as enum('in_processing', 'complete', 'pending', 'error', 'paused', 'approved', 'denied')" + ) + op.execute( + ( + "alter table privacyrequest alter column status type privacyrequeststatus using " + "status::text::privacyrequeststatus" + ) + ) + op.execute("drop type privacyrequeststatus_old") diff --git a/tests/api/v1/endpoints/test_drp_endpoints.py b/tests/api/v1/endpoints/test_drp_endpoints.py index f339c3195..67439aaf8 100644 --- a/tests/api/v1/endpoints/test_drp_endpoints.py +++ b/tests/api/v1/endpoints/test_drp_endpoints.py @@ -9,11 +9,13 @@ from fidesops.api.v1.scope_registry import ( POLICY_READ, PRIVACY_REQUEST_READ, + PRIVACY_REQUEST_REVIEW, STORAGE_CREATE_OR_UPDATE, ) from fidesops.api.v1.urn_registry import ( DRP_DATA_RIGHTS, DRP_EXERCISE, + DRP_REVOKE, DRP_STATUS, V1_URL_PREFIX, ) @@ -424,3 +426,64 @@ def test_get_drp_data_rights_multiple_drp_policies( ) assert 200 == response.status_code assert response.json() == expected_response + + +class TestDrpRevoke: + @pytest.fixture(scope="function") + def url(self) -> str: + return V1_URL_PREFIX + DRP_REVOKE + + def test_revoke_not_authenticated( + self, api_client: TestClient, privacy_request, url + ): + response = api_client.post(url, headers={}) + assert 401 == response.status_code + + def test_revoke_wrong_scope( + self, api_client: TestClient, generate_auth_header, url + ): + auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_READ]) + response = api_client.post(url, headers=auth_header, json={}) + assert 403 == response.status_code + + def test_revoke_wrong_status( + self, db, api_client: TestClient, generate_auth_header, url, privacy_request + ): + auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_REVIEW]) + response = api_client.post( + url, headers=auth_header, json={"request_id": privacy_request.id} + ) + assert 400 == response.status_code + assert response.json()[ + "detail" + ] == "Invalid revoke request. Can only revoke `pending` requests. Privacy request '{}' status = in_processing.".format( + privacy_request.id + ) + db.refresh(privacy_request) + assert privacy_request.status == PrivacyRequestStatus.in_processing + assert privacy_request.canceled_at is None + + def test_revoke( + self, db, api_client: TestClient, generate_auth_header, url, privacy_request + ): + privacy_request.status = PrivacyRequestStatus.pending + privacy_request.save(db) + canceled_reason = "Accidentally submitted" + + auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_REVIEW]) + response = api_client.post( + url, + headers=auth_header, + json={"request_id": privacy_request.id, "reason": canceled_reason}, + ) + assert 200 == response.status_code + db.refresh(privacy_request) + + assert privacy_request.status == PrivacyRequestStatus.canceled + assert privacy_request.cancel_reason == canceled_reason + assert privacy_request.canceled_at is not None + + data = response.json() + assert data["request_id"] == privacy_request.id + assert data["status"] == "revoked" + assert data["reason"] == canceled_reason From 7aab5930586f6d4de9046eccd01f71979cff92a6 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Fri, 3 Jun 2022 11:20:07 -0500 Subject: [PATCH 02/15] Add drp revoke request to postman collection. --- .../postman/Fidesops.postman_collection.json | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/docs/fidesops/docs/postman/Fidesops.postman_collection.json b/docs/fidesops/docs/postman/Fidesops.postman_collection.json index 96b334164..b8c73a6a2 100644 --- a/docs/fidesops/docs/postman/Fidesops.postman_collection.json +++ b/docs/fidesops/docs/postman/Fidesops.postman_collection.json @@ -1,6 +1,6 @@ { "info": { - "_postman_id": "9019796e-9107-41a1-a268-4d2cda117e85", + "_postman_id": "ffdc764a-b9d2-45ec-a106-ba25cb840a73", "name": "Fidesops", "schema": "https://schema.getpostman.com/json/collection/v2.1.0/collection.json" }, @@ -3503,6 +3503,43 @@ } }, "response": [] + }, + { + "name": "Revoke request", + "request": { + "auth": { + "type": "bearer", + "bearer": [ + { + "key": "token", + "value": "{{client_token}}", + "type": "string" + } + ] + }, + "method": "POST", + "header": [], + "body": { + "mode": "raw", + "raw": "{\n \"request_id\": \"{{privacy_request_id}}\", \n \"reason\": \"Accidentally submitted\"\n\n}", + "options": { + "raw": { + "language": "json" + } + } + }, + "url": { + "raw": "{{host}}/drp/revoke", + "host": [ + "{{host}}" + ], + "path": [ + "drp", + "revoke" + ] + } + }, + "response": [] } ] }, From 7be7aeb7bf4e8f8b7b863219d364e33fce2989bd Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Fri, 3 Jun 2022 11:22:57 -0500 Subject: [PATCH 03/15] Add drp revoke docs. --- docs/fidesops/docs/guides/data_rights_protocol.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/fidesops/docs/guides/data_rights_protocol.md b/docs/fidesops/docs/guides/data_rights_protocol.md index 376a21d78..6af7282cb 100644 --- a/docs/fidesops/docs/guides/data_rights_protocol.md +++ b/docs/fidesops/docs/guides/data_rights_protocol.md @@ -85,4 +85,15 @@ All data rights associated with existing policies may be returned via the `/data ], "user_relationships": null } +``` + +### Revoke + +You can revoke a pending privacy request via the `/revoke` endpoint. + +```json title="GET /api/v1/drp/revoke" +{ + "request_id": "c789ff35-7644-4ceb-9981-4b35c264aac3", + "reason": "Accidentally submitted" +} ``` \ No newline at end of file From c4422453038c43426e7af1e68a6ac511a719026c Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Fri, 3 Jun 2022 12:05:10 -0500 Subject: [PATCH 04/15] Update down_rev after rebase. --- .../versions/ed1b00ff963d_cancel_privacy_request.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py b/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py index 7f0500913..fcbe8066e 100644 --- a/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py +++ b/src/migrations/versions/ed1b00ff963d_cancel_privacy_request.py @@ -1,7 +1,7 @@ """cancel privacy request Revision ID: ed1b00ff963d -Revises: 3a7c5fb119c9 +Revises: 1ff88b7bd579 Create Date: 2022-06-03 15:45:14.584540 """ @@ -10,7 +10,7 @@ # revision identifiers, used by Alembic. revision = "ed1b00ff963d" -down_revision = "3a7c5fb119c9" +down_revision = "1ff88b7bd579" branch_labels = None depends_on = None From d17c394d09c1fc9ae6a427cb4f98ad8564cb4c10 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Fri, 3 Jun 2022 12:19:00 -0500 Subject: [PATCH 05/15] Fix incorrect check. --- src/fidesops/models/privacy_request.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/fidesops/models/privacy_request.py b/src/fidesops/models/privacy_request.py index 908a20acd..06b861ada 100644 --- a/src/fidesops/models/privacy_request.py +++ b/src/fidesops/models/privacy_request.py @@ -386,7 +386,7 @@ def start_processing(self, db: Session) -> None: def cancel_processing(self, db: Session, cancel_reason: Optional[str]) -> None: """Cancels a privacy request. Currently should only cancel 'pending' tasks""" - if self.canceled_at is not None: + if self.canceled_at is None: self.status = PrivacyRequestStatus.canceled self.cancel_reason = cancel_reason self.canceled_at = datetime.utcnow() From 7c309a7f0260627930e054608221ebeefc65a86a Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Jun 2022 18:25:58 -0500 Subject: [PATCH 06/15] Restore new canceled state. --- clients/admin-ui/src/features/common/RequestStatusBadge.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/clients/admin-ui/src/features/common/RequestStatusBadge.tsx b/clients/admin-ui/src/features/common/RequestStatusBadge.tsx index c688fd540..2cab512df 100644 --- a/clients/admin-ui/src/features/common/RequestStatusBadge.tsx +++ b/clients/admin-ui/src/features/common/RequestStatusBadge.tsx @@ -18,6 +18,10 @@ export const statusPropMap: { bg: "red.500", label: "Denied", }, + canceled: { + bg: 'red.600', + label: 'Canceled', + }, error: { bg: "red.800", label: "Error", From a54dc0158cc856b65166e5b8ecfcd0d2702b01f2 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Jun 2022 19:02:24 -0500 Subject: [PATCH 07/15] Check that the privacy request is not canceled right before starting execution. This is really our last chance to check before we start executing the graph in dask. The use case here might be it was canceled shortly after it was approved. --- .../service/privacy_request/request_runner_service.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/fidesops/service/privacy_request/request_runner_service.py b/src/fidesops/service/privacy_request/request_runner_service.py index e373987a3..487b8122e 100644 --- a/src/fidesops/service/privacy_request/request_runner_service.py +++ b/src/fidesops/service/privacy_request/request_runner_service.py @@ -173,6 +173,11 @@ def run_privacy_request( with SessionLocal() as session: privacy_request = PrivacyRequest.get(db=session, id=privacy_request_id) + if privacy_request.status == PrivacyRequestStatus.canceled: + logging.info( + f"Terminating privacy request {privacy_request.id}: request canceled." + ) + return logging.info(f"Dispatching privacy request {privacy_request.id}") privacy_request.start_processing(session) From 4e31c4a95cdc5552a400746f275d64a5e6baa6e2 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Jun 2022 19:03:43 -0500 Subject: [PATCH 08/15] Attempt to revoke a queued celery task if we cancel it before it starts executing. --- src/fidesops/models/privacy_request.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/fidesops/models/privacy_request.py b/src/fidesops/models/privacy_request.py index 7f9f47f67..839dbc510 100644 --- a/src/fidesops/models/privacy_request.py +++ b/src/fidesops/models/privacy_request.py @@ -38,6 +38,7 @@ ) from fidesops.schemas.masking.masking_secrets import MaskingSecretCache from fidesops.schemas.redis_cache import PrivacyRequestIdentity +from fidesops.tasks import celery_app from fidesops.util.cache import ( FidesopsRedis, get_all_cache_keys_for_privacy_request, @@ -473,7 +474,11 @@ def cancel_processing(self, db: Session, cancel_reason: Optional[str]) -> None: self.canceled_at = datetime.utcnow() self.save(db) - # TODO cancel celery task PrivacyRequestRunner.submit + task_id = self.get_cached_task_id() + if task_id: + logger.info(f"Revoking task {task_id} for request {self.id}") + # Only revokes if execution is not already in progress + celery_app.control.revoke(task_id, terminate=False) def error_processing(self, db: Session) -> None: """Mark privacy request as errored, and note time processing was finished""" From 82623e534d1364cf3ea4fcf1ca2cb1fd11d25a47 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Jun 2022 22:22:30 -0500 Subject: [PATCH 09/15] Prettier. --- clients/admin-ui/src/features/common/RequestStatusBadge.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/admin-ui/src/features/common/RequestStatusBadge.tsx b/clients/admin-ui/src/features/common/RequestStatusBadge.tsx index 2cab512df..fef7e7715 100644 --- a/clients/admin-ui/src/features/common/RequestStatusBadge.tsx +++ b/clients/admin-ui/src/features/common/RequestStatusBadge.tsx @@ -19,8 +19,8 @@ export const statusPropMap: { label: "Denied", }, canceled: { - bg: 'red.600', - label: 'Canceled', + bg: "red.600", + label: "Canceled", }, error: { bg: "red.800", From 6b28d68d8a267c9884b1d3f629489007b4c4dd63 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Wed, 22 Jun 2022 22:24:21 -0500 Subject: [PATCH 10/15] Changelog updated. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index f366b7284..1dd94d2cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,8 @@ The types of changes are: * Added the ability to delete a datastore from the frontend [#683] https://github.com/ethyca/fidesops/pull/683 * Added the ability to disable/enable a datastore from the frontend [#693] https://github.com/ethyca/fidesops/pull/693 * Adds Postgres and Redis health checks to health endpoint [#690](https://github.com/ethyca/fidesops/pull/690) +* Adds the ability to revoke a pending privacy request [#592](https://github.com/ethyca/fidesops/pull/592/files) + ### Changed From 33357bd9ee59bd411e24aecdfdf398fc7195905d Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Thu, 23 Jun 2022 06:01:52 -0500 Subject: [PATCH 11/15] Add a few unit tests around how triggering the run_privacy_request_task with a cancelled task id doesn't do anything and how you can't approve a canceled privacy request. --- .../test_privacy_request_endpoints.py | 21 +++++++++++++++---- tests/fixtures/application_fixtures.py | 13 ++++++++++++ tests/integration_tests/test_execution.py | 1 - .../request_runner_service_test.py | 16 ++++++++++++++ 4 files changed, 46 insertions(+), 5 deletions(-) diff --git a/tests/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/api/v1/endpoints/test_privacy_request_endpoints.py index 5128b9098..567716ead 100644 --- a/tests/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/api/v1/endpoints/test_privacy_request_endpoints.py @@ -1432,13 +1432,24 @@ def test_approve_privacy_request_does_not_exist( ) assert not submit_mock.called + @pytest.mark.parametrize( + "privacy_request_status", + [PrivacyRequestStatus.complete, PrivacyRequestStatus.canceled], + ) @mock.patch( "fidesops.service.privacy_request.request_runner_service.run_privacy_request.delay" ) - def test_approve_completed_privacy_request( - self, submit_mock, db, url, api_client, generate_auth_header, privacy_request + def test_approve_privacy_request_in_non_pending_state( + self, + submit_mock, + db, + url, + api_client, + generate_auth_header, + privacy_request, + privacy_request_status, ): - privacy_request.status = PrivacyRequestStatus.complete + privacy_request.status = privacy_request_status privacy_request.save(db=db) auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_REVIEW]) @@ -1450,7 +1461,9 @@ def test_approve_completed_privacy_request( assert response_body["succeeded"] == [] assert len(response_body["failed"]) == 1 assert response_body["failed"][0]["message"] == "Cannot transition status" - assert response_body["failed"][0]["data"]["status"] == "complete" + assert ( + response_body["failed"][0]["data"]["status"] == privacy_request_status.value + ) assert not submit_mock.called @mock.patch( diff --git a/tests/fixtures/application_fixtures.py b/tests/fixtures/application_fixtures.py index db16e3f62..ba60c2f18 100644 --- a/tests/fixtures/application_fixtures.py +++ b/tests/fixtures/application_fixtures.py @@ -799,6 +799,19 @@ def privacy_request_status_pending(db: Session, policy: Policy) -> PrivacyReques privacy_request.delete(db) +@pytest.fixture(scope="function") +def privacy_request_status_canceled(db: Session, policy: Policy) -> PrivacyRequest: + privacy_request = _create_privacy_request_for_policy( + db, + policy, + PrivacyRequestStatus.canceled, + ) + privacy_request.started_processing_at = None + privacy_request.save(db) + yield privacy_request + privacy_request.delete(db) + + @pytest.fixture(scope="function") def privacy_request_with_drp_action( db: Session, policy_drp_action: Policy diff --git a/tests/integration_tests/test_execution.py b/tests/integration_tests/test_execution.py index c9864ca7a..a0b2f6fd5 100644 --- a/tests/integration_tests/test_execution.py +++ b/tests/integration_tests/test_execution.py @@ -5,7 +5,6 @@ from pydantic import ValidationError from sqlalchemy.exc import InvalidRequestError -from fidesops.core.config import config from fidesops.db.session import get_db_session from fidesops.graph.config import CollectionAddress from fidesops.graph.graph import DatasetGraph diff --git a/tests/service/privacy_request/request_runner_service_test.py b/tests/service/privacy_request/request_runner_service_test.py index 48beeecf7..7be947ebd 100644 --- a/tests/service/privacy_request/request_runner_service_test.py +++ b/tests/service/privacy_request/request_runner_service_test.py @@ -99,6 +99,22 @@ def test_start_processing_doesnt_overwrite_started_processing_at( assert privacy_request.started_processing_at == before +def test_halts_proceeding_if_cancelled( + db: Session, + privacy_request_status_canceled: PrivacyRequest, + run_privacy_request_task, +) -> None: + assert privacy_request_status_canceled.status == PrivacyRequestStatus.canceled + run_privacy_request_task.delay(privacy_request_status_canceled.id).get( + timeout=PRIVACY_REQUEST_TASK_TIMEOUT + ) + _sessionmaker = get_db_session() + db = _sessionmaker() + reloaded_pr = PrivacyRequest.get(db=db, id=privacy_request_status_canceled.id) + assert reloaded_pr.started_processing_at is None + assert reloaded_pr.status == PrivacyRequestStatus.canceled + + @mock.patch( "fidesops.service.privacy_request.request_runner_service.run_webhooks_and_report_status", ) From ce1e798ba466a01ce1f3e07fa7922b21cbf83029 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Thu, 23 Jun 2022 06:20:29 -0500 Subject: [PATCH 12/15] Fix SQLAlchemy logging to console - logging in migration propagates to the rest of the application. --- .../migrations/versions/55d61eb8ed12_add_default_policies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/fidesops/migrations/versions/55d61eb8ed12_add_default_policies.py b/src/fidesops/migrations/versions/55d61eb8ed12_add_default_policies.py index 74e1eb1d5..1a287456d 100644 --- a/src/fidesops/migrations/versions/55d61eb8ed12_add_default_policies.py +++ b/src/fidesops/migrations/versions/55d61eb8ed12_add_default_policies.py @@ -37,7 +37,6 @@ logging.basicConfig() logger = logging.getLogger(__name__) logger.setLevel(logging.INFO) -logging.getLogger("sqlalchemy.engine").setLevel(logging.INFO) revision = "55d61eb8ed12" down_revision = "b3b68c87c4a0" From 59bb7be8699be6aded62c5b7759b1e691da2a431 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Thu, 23 Jun 2022 09:57:43 -0500 Subject: [PATCH 13/15] Refresh session instead of creating a new one. --- .../request_runner_service_test.py | 28 ++++--------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/tests/service/privacy_request/request_runner_service_test.py b/tests/service/privacy_request/request_runner_service_test.py index 7be947ebd..5cc8c8089 100644 --- a/tests/service/privacy_request/request_runner_service_test.py +++ b/tests/service/privacy_request/request_runner_service_test.py @@ -70,14 +70,8 @@ def test_start_processing_sets_started_processing_at( run_privacy_request_task.delay(privacy_request_status_pending.id).get( timeout=PRIVACY_REQUEST_TASK_TIMEOUT ) - _sessionmaker = get_db_session() - db = _sessionmaker() - assert ( - PrivacyRequest.get( - db=db, id=privacy_request_status_pending.id - ).started_processing_at - is not None - ) + db.refresh(privacy_request_status_pending) + assert privacy_request_status_pending.started_processing_at is not None def test_start_processing_doesnt_overwrite_started_processing_at( @@ -92,10 +86,7 @@ def test_start_processing_doesnt_overwrite_started_processing_at( timeout=PRIVACY_REQUEST_TASK_TIMEOUT ) - _sessionmaker = get_db_session() - db = _sessionmaker() - - privacy_request = PrivacyRequest.get(db=db, id=privacy_request.id) + db.refresh(privacy_request) assert privacy_request.started_processing_at == before @@ -108,8 +99,7 @@ def test_halts_proceeding_if_cancelled( run_privacy_request_task.delay(privacy_request_status_canceled.id).get( timeout=PRIVACY_REQUEST_TASK_TIMEOUT ) - _sessionmaker = get_db_session() - db = _sessionmaker() + db.refresh(privacy_request_status_canceled) reloaded_pr = PrivacyRequest.get(db=db, id=privacy_request_status_canceled.id) assert reloaded_pr.started_processing_at is None assert reloaded_pr.status == PrivacyRequestStatus.canceled @@ -140,10 +130,7 @@ def test_from_graph_resume_does_not_run_pre_webhooks( from_step=PausedStep.access.value, ).get(timeout=PRIVACY_REQUEST_TASK_TIMEOUT) - _sessionmaker = get_db_session() - db = _sessionmaker() - - privacy_request = PrivacyRequest.get(db=db, id=privacy_request.id) + db.refresh(privacy_request) assert privacy_request.started_processing_at is not None # Starting privacy request in the middle of the graph means we don't run pre-webhooks again @@ -179,10 +166,7 @@ def test_resume_privacy_request_from_erasure( from_step=PausedStep.erasure.value, ).get(timeout=PRIVACY_REQUEST_TASK_TIMEOUT) - _sessionmaker = get_db_session() - db = _sessionmaker() - - privacy_request = PrivacyRequest.get(db=db, id=privacy_request.id) + db.refresh(privacy_request) assert privacy_request.started_processing_at is not None # Starting privacy request in the middle of the graph means we don't run pre-webhooks again From bb5b7bfc944ba4d5c89804b58eac14051ca1151f Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Thu, 23 Jun 2022 10:11:51 -0500 Subject: [PATCH 14/15] Add 200 character limit. --- .../migrations/versions/ed1b00ff963d_cancel_privacy_request.py | 2 +- src/fidesops/models/privacy_request.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/fidesops/migrations/versions/ed1b00ff963d_cancel_privacy_request.py b/src/fidesops/migrations/versions/ed1b00ff963d_cancel_privacy_request.py index 16b510388..07c0598b9 100644 --- a/src/fidesops/migrations/versions/ed1b00ff963d_cancel_privacy_request.py +++ b/src/fidesops/migrations/versions/ed1b00ff963d_cancel_privacy_request.py @@ -17,7 +17,7 @@ def upgrade(): op.add_column( - "privacyrequest", sa.Column("cancel_reason", sa.String(), nullable=True) + "privacyrequest", sa.Column("cancel_reason", sa.String(200), nullable=True) ) op.add_column( "privacyrequest", diff --git a/src/fidesops/models/privacy_request.py b/src/fidesops/models/privacy_request.py index 839dbc510..4c4f002f9 100644 --- a/src/fidesops/models/privacy_request.py +++ b/src/fidesops/models/privacy_request.py @@ -160,7 +160,7 @@ class PrivacyRequest(Base): # pylint: disable=R0904 backref="privacy_requests", ) - cancel_reason = Column(String) + cancel_reason = Column(String(200)) canceled_at = Column(DateTime(timezone=True), nullable=True) # passive_deletes="all" prevents execution logs from having their privacy_request_id set to null when From dcc57aaa78ff4e20467f0899a2570dbcc4929a99 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Thu, 23 Jun 2022 11:43:32 -0500 Subject: [PATCH 15/15] Add some assertions that db.refresh is doing what we think it's doing. --- .../request_runner_service_test.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/tests/service/privacy_request/request_runner_service_test.py b/tests/service/privacy_request/request_runner_service_test.py index 5cc8c8089..fdac688fa 100644 --- a/tests/service/privacy_request/request_runner_service_test.py +++ b/tests/service/privacy_request/request_runner_service_test.py @@ -12,7 +12,6 @@ from fidesops.common_exceptions import ClientUnsuccessfulException, PrivacyRequestPaused from fidesops.core.config import config -from fidesops.db.session import get_db_session from fidesops.models.policy import PausedStep, PolicyPostWebhook from fidesops.models.privacy_request import ( ActionType, @@ -66,12 +65,15 @@ def test_start_processing_sets_started_processing_at( privacy_request_status_pending: PrivacyRequest, run_privacy_request_task, ) -> None: + updated_at = privacy_request_status_pending.updated_at assert privacy_request_status_pending.started_processing_at is None run_privacy_request_task.delay(privacy_request_status_pending.id).get( timeout=PRIVACY_REQUEST_TASK_TIMEOUT ) + db.refresh(privacy_request_status_pending) assert privacy_request_status_pending.started_processing_at is not None + assert privacy_request_status_pending.updated_at > updated_at def test_start_processing_doesnt_overwrite_started_processing_at( @@ -81,6 +83,7 @@ def test_start_processing_doesnt_overwrite_started_processing_at( ) -> None: before = privacy_request.started_processing_at assert before is not None + updated_at = privacy_request.updated_at run_privacy_request_task.delay(privacy_request.id).get( timeout=PRIVACY_REQUEST_TASK_TIMEOUT @@ -88,9 +91,14 @@ def test_start_processing_doesnt_overwrite_started_processing_at( db.refresh(privacy_request) assert privacy_request.started_processing_at == before + assert privacy_request.updated_at > updated_at +@mock.patch( + "fidesops.service.privacy_request.request_runner_service.upload_access_results" +) def test_halts_proceeding_if_cancelled( + upload_access_results_mock, db: Session, privacy_request_status_canceled: PrivacyRequest, run_privacy_request_task, @@ -103,6 +111,7 @@ def test_halts_proceeding_if_cancelled( reloaded_pr = PrivacyRequest.get(db=db, id=privacy_request_status_canceled.id) assert reloaded_pr.started_processing_at is None assert reloaded_pr.status == PrivacyRequestStatus.canceled + assert not upload_access_results_mock.called @mock.patch( @@ -124,6 +133,7 @@ def test_from_graph_resume_does_not_run_pre_webhooks( privacy_request.started_processing_at = None privacy_request.policy = erasure_policy privacy_request.save(db) + updated_at = privacy_request.updated_at run_privacy_request_task.delay( privacy_request_id=privacy_request.id, @@ -132,6 +142,7 @@ def test_from_graph_resume_does_not_run_pre_webhooks( db.refresh(privacy_request) assert privacy_request.started_processing_at is not None + assert privacy_request.updated_at > updated_at # Starting privacy request in the middle of the graph means we don't run pre-webhooks again assert run_webhooks.call_count == 1 @@ -160,6 +171,7 @@ def test_resume_privacy_request_from_erasure( privacy_request.started_processing_at = None privacy_request.policy = erasure_policy privacy_request.save(db) + updated_at = privacy_request.updated_at run_privacy_request_task.delay( privacy_request_id=privacy_request.id, @@ -168,6 +180,7 @@ def test_resume_privacy_request_from_erasure( db.refresh(privacy_request) assert privacy_request.started_processing_at is not None + assert privacy_request.updated_at > updated_at # Starting privacy request in the middle of the graph means we don't run pre-webhooks again assert run_webhooks.call_count == 1