Sync up Database and PreApprovalWebhook/PreApprovalWebhookReply Models

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)

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
 

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")

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
 

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.",

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"
+        ),
+    )

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

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"
     )