From 5b4983db3d3ecebe902af35a354d44e58f4c7a19 Mon Sep 17 00:00:00 2001 From: Dawn Pattison Date: Mon, 29 Apr 2024 09:14:33 -0500 Subject: [PATCH] Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models (#4838) --- CHANGELOG.md | 1 + ...0cc0_adds_webhook_id_column_to_auditlog.py | 1 + ...83545ed9b6_pre_approval_webhook_cleanup.py | 131 ++++++++++++++++++ .../c85a641cc92c_unique_constraint_.py | 1 + .../v1/endpoints/privacy_request_endpoints.py | 35 +++-- src/fides/api/models/pre_approval_webhook.py | 30 +++- tests/conftest.py | 6 +- .../test_privacy_request_endpoints.py | 45 +++++- tests/ops/models/test_pre_approval_webhook.py | 31 +++-- 9 files changed, 249 insertions(+), 32 deletions(-) create mode 100644 src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 02a52e7682..a5a85dae5b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,6 +29,7 @@ The types of changes are: - Added `reinitialize` method to FidesJS SDK [#4812](https://github.com/ethyca/fides/pull/4812) - Added undeclared data category columns to data map report table [#4781](https://github.com/ethyca/fides/pull/4781) - Fully implement pre-approval webhooks [#4822](https://github.com/ethyca/fides/pull/4822) +- Sync models and database for pre-approval webhooks [#4838](https://github.com/ethyca/fides/pull/4838) ### Changed - Removed the Celery startup banner from the Fides worker logs [#4814](https://github.com/ethyca/fides/pull/4814) diff --git a/src/fides/api/alembic/migrations/versions/93af4df40cc0_adds_webhook_id_column_to_auditlog.py b/src/fides/api/alembic/migrations/versions/93af4df40cc0_adds_webhook_id_column_to_auditlog.py index 8981b17978..34fe0e07d1 100644 --- a/src/fides/api/alembic/migrations/versions/93af4df40cc0_adds_webhook_id_column_to_auditlog.py +++ b/src/fides/api/alembic/migrations/versions/93af4df40cc0_adds_webhook_id_column_to_auditlog.py @@ -5,6 +5,7 @@ Create Date: 2024-04-20 13:17:33.928813 """ + import sqlalchemy as sa from alembic import op diff --git a/src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py b/src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py new file mode 100644 index 0000000000..a0390686e9 --- /dev/null +++ b/src/fides/api/alembic/migrations/versions/9e83545ed9b6_pre_approval_webhook_cleanup.py @@ -0,0 +1,131 @@ +"""pre_approval_webhook_cleanup + +Revision ID: 9e83545ed9b6 +Revises: c85a641cc92c +Create Date: 2024-04-26 23:47:27.821607 + +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "9e83545ed9b6" +down_revision = "c85a641cc92c" +branch_labels = None +depends_on = None + + +def upgrade(): + # Index PreApproval Webhook Key + op.create_index( + op.f("ix_preapprovalwebhook_key"), "preapprovalwebhook", ["key"], unique=True + ) + # Create Unique Constraint on Preapproval Webhook Name + op.create_unique_constraint( + "preapprovalwebhook_name_key", "preapprovalwebhook", ["name"] + ) + # Remove PreApprovalWebhook.connection_config_id FK + op.drop_constraint( + "preapprovalwebhook_connection_config_id_fkey", + "preapprovalwebhook", + type_="foreignkey", + ) + # Restore PreApprovalWebhook.connection_config_id FK with cascade delete + op.create_foreign_key( + "preapprovalwebhook_connection_config_id_fkey", + "preapprovalwebhook", + "connectionconfig", + ["connection_config_id"], + ["id"], + ondelete="CASCADE", + ) + # Set Preapproval Webhook Reply to nullable in case the Webhook is deleted + op.alter_column( + "preapprovalwebhookreply", + "webhook_id", + existing_type=sa.VARCHAR(length=255), + nullable=True, + ) + # Index the PreApprovalWebhookReply.privacy_request_id + op.create_index( + op.f("ix_preapprovalwebhookreply_privacy_request_id"), + "preapprovalwebhookreply", + ["privacy_request_id"], + unique=False, + ) + # Index the PreApprovalWebhookReply.webhook_id + op.create_index( + op.f("ix_preapprovalwebhookreply_webhook_id"), + "preapprovalwebhookreply", + ["webhook_id"], + unique=False, + ) + # Remove FK on preapprovalwebhookreply.privacy_request_id + op.drop_constraint( + "preapprovalwebhookreply_privacy_request_id_fkey", + "preapprovalwebhookreply", + type_="foreignkey", + ) + # Add FK on preapprovalwebhookreply.webhook_id with ondelete set null + op.create_foreign_key( + "preapprovalwebhookreply_webhook_id_fkey", + "preapprovalwebhookreply", + "preapprovalwebhook", + ["webhook_id"], + ["id"], + ondelete="SET NULL", + ) + # Restore FK on preapprovalwebhookreply.privacy_request_id with ondelete set null + op.create_foreign_key( + "preapprovalwebhookreply_privacy_request_id_fkey", + "preapprovalwebhookreply", + "privacyrequest", + ["privacy_request_id"], + ["id"], + ondelete="SET NULL", + ) + + +def downgrade(): + op.drop_constraint( + "preapprovalwebhookreply_privacy_request_id_fkey", + "preapprovalwebhookreply", + type_="foreignkey", + ) + op.drop_constraint( + "preapprovalwebhookreply_webhook_id_fkey", + "preapprovalwebhookreply", + type_="foreignkey", + ) + op.create_foreign_key( + "preapprovalwebhookreply_privacy_request_id_fkey", + "preapprovalwebhookreply", + "privacyrequest", + ["privacy_request_id"], + ["id"], + ) + op.drop_index( + op.f("ix_preapprovalwebhookreply_webhook_id"), + table_name="preapprovalwebhookreply", + ) + op.drop_index( + op.f("ix_preapprovalwebhookreply_privacy_request_id"), + table_name="preapprovalwebhookreply", + ) + op.drop_constraint( + "preapprovalwebhook_connection_config_id_fkey", + "preapprovalwebhook", + type_="foreignkey", + ) + op.create_foreign_key( + "preapprovalwebhook_connection_config_id_fkey", + "preapprovalwebhook", + "connectionconfig", + ["connection_config_id"], + ["id"], + ) + op.drop_constraint( + "preapprovalwebhook_name_key", "preapprovalwebhook", type_="unique" + ) + op.drop_index(op.f("ix_preapprovalwebhook_key"), table_name="preapprovalwebhook") diff --git a/src/fides/api/alembic/migrations/versions/c85a641cc92c_unique_constraint_.py b/src/fides/api/alembic/migrations/versions/c85a641cc92c_unique_constraint_.py index 25b1778ffe..67e35a1f39 100644 --- a/src/fides/api/alembic/migrations/versions/c85a641cc92c_unique_constraint_.py +++ b/src/fides/api/alembic/migrations/versions/c85a641cc92c_unique_constraint_.py @@ -5,6 +5,7 @@ Create Date: 2024-04-23 19:47:07.836244 """ + import sqlalchemy as sa from alembic import op diff --git a/src/fides/api/api/v1/endpoints/privacy_request_endpoints.py b/src/fides/api/api/v1/endpoints/privacy_request_endpoints.py index 8f9239a24f..69b615354a 100644 --- a/src/fides/api/api/v1/endpoints/privacy_request_endpoints.py +++ b/src/fides/api/api/v1/endpoints/privacy_request_endpoints.py @@ -142,9 +142,9 @@ PRIVACY_REQUEST_MANUAL_WEBHOOK_ACCESS_INPUT, PRIVACY_REQUEST_MANUAL_WEBHOOK_ERASURE_INPUT, PRIVACY_REQUEST_NOTIFICATIONS, - PRIVACY_REQUEST_REQUEUE, PRIVACY_REQUEST_PRE_APPROVE_ELIGIBLE, PRIVACY_REQUEST_PRE_APPROVE_NOT_ELIGIBLE, + PRIVACY_REQUEST_REQUEUE, PRIVACY_REQUEST_RESUME, PRIVACY_REQUEST_RESUME_FROM_REQUIRES_INPUT, PRIVACY_REQUEST_RETRY, @@ -1160,9 +1160,9 @@ def _approve_request( "action": AuditLogAction.approved, "message": "", "user_id": user_id if user_id else None, - "webhook_id": webhook_id - if webhook_id - else None, # the last webhook reply received is what approves the entire request + "webhook_id": ( + webhook_id if webhook_id else None + ), # the last webhook reply received is what approves the entire request } AuditLog.create( db=db, @@ -1226,11 +1226,11 @@ def deny_privacy_request( user_id = client.user_id def _deny_request( - db: Session, - config_proxy: ConfigProxy, - privacy_request: PrivacyRequest, - webhook_id: Optional[str], - user_id: Optional[str], + db: Session, + config_proxy: ConfigProxy, + privacy_request: PrivacyRequest, + webhook_id: Optional[str], + user_id: Optional[str], ) -> None: """Method for how to process requests - denied""" privacy_request.status = PrivacyRequestStatus.denied @@ -1305,6 +1305,7 @@ def mark_privacy_request_pre_approve_eligible( webhook.key, webhook.connection_config.key, ) + try: PreApprovalWebhookReply.create( db=db, @@ -1343,11 +1344,17 @@ def _reply_exists_for_webhook(webhook_id: str) -> bool: privacy_request_id, ) return - # Check if all replies are true - replies_for_privacy_request = PreApprovalWebhookReply.filter( - db=db, - conditions=(PreApprovalWebhookReply.privacy_request_id == privacy_request_id), - ).all() + + # Check if all replies are true. Reply ignored if its webhook has since been deleted. + replies_for_privacy_request = ( + db.query(PreApprovalWebhookReply) + .filter( + PreApprovalWebhookReply.privacy_request_id == privacy_request_id, + PreApprovalWebhookReply.webhook_id.isnot(None), + ) + .all() + ) + if not all(reply.is_eligible for reply in replies_for_privacy_request): logger.info( "Not all pre-approval webhooks have responded with eligible for privacy request '{}'. Cannot automatically approve request.", diff --git a/src/fides/api/models/pre_approval_webhook.py b/src/fides/api/models/pre_approval_webhook.py index c00f30a206..16eec07dd4 100644 --- a/src/fides/api/models/pre_approval_webhook.py +++ b/src/fides/api/models/pre_approval_webhook.py @@ -1,14 +1,23 @@ -from sqlalchemy import Boolean, Column, ForeignKey, String +from sqlalchemy import Boolean, Column, ForeignKey, String, UniqueConstraint from sqlalchemy.orm import Session, relationship # type: ignore from fides.api.db.base_class import Base class PreApprovalWebhook(Base): + """ + An HTTPS callback that calls an external REST API endpoint as soon as a privacy request + is received (or after user identity verification, if applicable). + """ + name = Column(String, unique=True, nullable=False) key = Column(String, index=True, unique=True, nullable=False) - connection_config_id = Column( - String, ForeignKey("connectionconfig.id"), nullable=False + connection_config_id = ( + Column( # Deleting a Connection Config also deletes the Pre Approval Webhook + String, + ForeignKey("connectionconfig.id", ondelete="CASCADE"), + nullable=False, + ) ) connection_config = relationship( "ConnectionConfig", back_populates="pre_approval_webhooks", uselist=False # type: ignore @@ -24,13 +33,26 @@ def persist_obj( class PreApprovalWebhookReply(Base): + """ + Stores the callback response to the PreApprovalWebhook to determine if the Privacy + Request is eligible to be automatically approved + """ + webhook_id = Column( - String, ForeignKey(PreApprovalWebhook.id_field_path), nullable=False + String, + ForeignKey(PreApprovalWebhook.id_field_path, ondelete="SET NULL"), index=True ) privacy_request_id = Column( String, ForeignKey("privacyrequest.id", ondelete="SET NULL"), index=True ) # Which privacy request this webhook response belongs to is_eligible = Column(Boolean, nullable=False) + privacy_request = relationship( "PrivacyRequest", back_populates="pre_approval_webhook_replies", uselist=False # type: ignore ) + + __table_args__ = ( + UniqueConstraint( + "webhook_id", "privacy_request_id", name="webhook_id_privacy_request_id" + ), + ) diff --git a/tests/conftest.py b/tests/conftest.py index 3038fbc11d..b5d5408ee2 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -12,10 +12,6 @@ import yaml from fastapi import Query from fastapi.testclient import TestClient -from fides.api.models.privacy_request import ( - generate_request_callback_resume_jwe, - generate_request_callback_pre_approval_jwe, -) from fideslang import DEFAULT_TAXONOMY, models from httpx import AsyncClient from loguru import logger @@ -35,6 +31,8 @@ from fides.api.main import app from fides.api.models.privacy_request import ( EXITED_EXECUTION_LOG_STATUSES, + generate_request_callback_pre_approval_jwe, + generate_request_callback_resume_jwe, ) from fides.api.models.sql_models import Cookies, DataUse, PrivacyDeclaration from fides.api.oauth.jwt import generate_jwe diff --git a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py index e2a6974820..383eea2d4f 100644 --- a/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/test_privacy_request_endpoints.py @@ -78,9 +78,9 @@ PRIVACY_REQUEST_MANUAL_WEBHOOK_ACCESS_INPUT, PRIVACY_REQUEST_MANUAL_WEBHOOK_ERASURE_INPUT, PRIVACY_REQUEST_NOTIFICATIONS, - PRIVACY_REQUEST_REQUEUE, PRIVACY_REQUEST_PRE_APPROVE_ELIGIBLE, PRIVACY_REQUEST_PRE_APPROVE_NOT_ELIGIBLE, + PRIVACY_REQUEST_REQUEUE, PRIVACY_REQUEST_RESUME, PRIVACY_REQUEST_RESUME_FROM_REQUIRES_INPUT, PRIVACY_REQUEST_RETRY, @@ -2472,6 +2472,49 @@ def test_mark_eligible_but_not_all_webhook_replies_received( assert not submit_mock.called assert not mock_dispatch_message.called + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.delay" + ) + @mock.patch( + "fides.api.api.v1.endpoints.privacy_request_endpoints.dispatch_message_task.apply_async" + ) + def test_mark_eligible_webhook_deleted( + self, + mock_dispatch_message, + submit_mock, + db, + url, + api_client, + generate_auth_header, + user, + privacy_request_status_pending, + pre_approval_webhooks, + generate_pre_approval_webhook_auth_header, + privacy_request_review_notification_enabled, + ): + """If a webhook is deleted, its eligibility is no longer needed""" + # mock previous webhook reply + PreApprovalWebhookReply.create( + db=db, + data={ + "webhook_id": pre_approval_webhooks[0].id, + "privacy_request_id": privacy_request_status_pending.id, + "is_eligible": False, + }, + ) + pre_approval_webhooks[0].delete(db) + + auth_header = generate_pre_approval_webhook_auth_header( + webhook=pre_approval_webhooks[1] + ) + + # new webhook reply + response = api_client.post(url, headers=auth_header) + + assert response.status_code == 200 + assert submit_mock.called + assert mock_dispatch_message.called + @mock.patch( "fides.api.service.privacy_request.request_runner_service.run_privacy_request.delay" ) diff --git a/tests/ops/models/test_pre_approval_webhook.py b/tests/ops/models/test_pre_approval_webhook.py index 7521f5d258..4511519dd1 100644 --- a/tests/ops/models/test_pre_approval_webhook.py +++ b/tests/ops/models/test_pre_approval_webhook.py @@ -39,12 +39,12 @@ def test_create_pre_approval_webhook( db.add(webhook_2) db.commit() - loaded_webhook = db.query(PreApprovalWebhook).filter_by(name=name_1).first() + db.refresh(webhook_1) - assert loaded_webhook.key == "some_service_webhook" - assert loaded_webhook.name == name_1 - assert loaded_webhook.connection_config_id == https_connection_config.id - assert loaded_webhook.created_at is not None + assert webhook_1.key == "some_service_webhook" + assert webhook_1.name == name_1 + assert webhook_1.connection_config_id == https_connection_config.id + assert webhook_1.created_at is not None # assert relationship with ConnectionConfig all_webhooks_with_specific_connection_config = ( @@ -55,18 +55,23 @@ def test_create_pre_approval_webhook( connection_config = ( db.query(ConnectionConfig).filter_by(id=https_connection_config.id).first() ) - assert loaded_webhook.connection_config == connection_config + assert webhook_1.connection_config == connection_config assert ( connection_config.pre_approval_webhooks == all_webhooks_with_specific_connection_config ) + webhook_1_id = webhook_1.id + webhook_2_id = webhook_2.id connection_config.delete(db=db) - for webhook in all_webhooks_with_specific_connection_config: - webhook.delete(db=db) + # Deleting the Connection Config also deletes any attached PreApproval Webhooks + assert db.query(PreApprovalWebhook).get(webhook_1_id) is None + assert db.query(PreApprovalWebhook).get(webhook_2_id) is None + + @pytest.mark.usefixtures("pre_approval_webhooks") def test_create_connection_config_errors( - self, db: Session, https_connection_config, pre_approval_webhooks + self, db: Session, https_connection_config ): with pytest.raises(KeyValidationError) as exc: new_webhook_no_key = PreApprovalWebhook.create( @@ -130,4 +135,12 @@ def test_create_pre_approval_webhook_reply( assert loaded_reply.privacy_request == privacy_request assert privacy_request.pre_approval_webhook_replies[0] == loaded_reply + # Deleting the webhook or the Privacy Request causes these ids on the Replies to be null + pre_approval_webhooks[0].delete(db) + privacy_request.delete(db) + db.refresh(reply) + + assert reply.privacy_request_id is None + assert reply.webhook_id is None + reply.delete(db=db)