diff --git a/changelog/7971-fix-duplicate-request-identity-verification.yaml b/changelog/7971-fix-duplicate-request-identity-verification.yaml new file mode 100644 index 00000000000..dab3e80f5d9 --- /dev/null +++ b/changelog/7971-fix-duplicate-request-identity-verification.yaml @@ -0,0 +1,4 @@ +type: Security +description: Restored identity verification path for privacy requests classified as duplicates to prevent denial of service [GHSA-qx5f-ghc2-7g5c](https://github.com/ethyca/fides/security/advisories/GHSA-qx5f-ghc2-7g5c) +pr: 7971 +labels: [] diff --git a/changelog/7972-block-approve-unverified-duplicate.yaml b/changelog/7972-block-approve-unverified-duplicate.yaml new file mode 100644 index 00000000000..ad42d9d8764 --- /dev/null +++ b/changelog/7972-block-approve-unverified-duplicate.yaml @@ -0,0 +1,4 @@ +type: Security +description: Added identity verification check before administrator approval of duplicate privacy requests [GHSA-qx5f-ghc2-7g5c](https://github.com/ethyca/fides/security/advisories/GHSA-qx5f-ghc2-7g5c) +pr: 7972 +labels: [] diff --git a/clients/admin-ui/src/features/privacy-requests/RequestTableActions.tsx b/clients/admin-ui/src/features/privacy-requests/RequestTableActions.tsx index 5c635a71d5c..65c79c821b2 100644 --- a/clients/admin-ui/src/features/privacy-requests/RequestTableActions.tsx +++ b/clients/admin-ui/src/features/privacy-requests/RequestTableActions.tsx @@ -13,7 +13,11 @@ import Restrict from "~/features/common/Restrict"; import { useGetConfigurationSettingsQuery } from "~/features/config-settings/config-settings.slice"; import { useGetActiveMessagingProviderQuery } from "~/features/messaging/messaging.slice"; import DenyPrivacyRequestModal from "~/features/privacy-requests/DenyPrivacyRequestModal"; -import { PrivacyRequestResponse, ScopeRegistryEnum } from "~/types/api"; +import { + PrivacyRequestResponse, + PrivacyRequestStatus, + ScopeRegistryEnum, +} from "~/types/api"; import ApprovePrivacyRequestModal from "./ApprovePrivacyRequestModal"; import { getButtonVisibility } from "./helpers"; @@ -48,8 +52,17 @@ export const RequestTableActions = ({ useGetActiveMessagingProviderQuery(); const sendRequestCompletionNotification = config?.notifications?.send_request_completion_notification; - - const buttonVisibility = getButtonVisibility(subjectRequest.status); + const identityVerificationRequired = + config?.execution?.subject_identity_verification_required ?? false; + + const isUnverifiedDuplicate = + subjectRequest.status === PrivacyRequestStatus.DUPLICATE && + !subjectRequest.identity_verified_at && + identityVerificationRequired; + const buttonVisibility = { + ...getButtonVisibility(subjectRequest.status), + ...(isUnverifiedDuplicate && { approve: false }), + }; const renderApproveButton = () => { if (!buttonVisibility.approve) { diff --git a/clients/admin-ui/src/features/privacy-requests/hooks/useApproveDenyPrivacyRequest.ts b/clients/admin-ui/src/features/privacy-requests/hooks/useApproveDenyPrivacyRequest.ts index 48679a6b7c5..1355c831af3 100644 --- a/clients/admin-ui/src/features/privacy-requests/hooks/useApproveDenyPrivacyRequest.ts +++ b/clients/admin-ui/src/features/privacy-requests/hooks/useApproveDenyPrivacyRequest.ts @@ -1,5 +1,6 @@ import { useChakraDisclosure as useDisclosure, useMessage } from "fidesui"; +import { useGetConfigurationSettingsQuery } from "~/features/config-settings/config-settings.slice"; import { PrivacyRequestStatus } from "~/types/api"; import { PrivacyRequestEntity } from "../types"; @@ -17,11 +18,20 @@ const useApproveDenyPrivacyRequest = ({ }); const message = useMessage(); + const { data: config } = useGetConfigurationSettingsQuery({ + api_set: false, + }); + const identityVerificationRequired = + config?.execution?.subject_identity_verification_required ?? false; const isPendingStatus = privacyRequest.status === PrivacyRequestStatus.PENDING; const isDuplicateStatus = privacyRequest.status === PrivacyRequestStatus.DUPLICATE; + const isUnverifiedDuplicate = + isDuplicateStatus && + !privacyRequest.identity_verified_at && + identityVerificationRequired; const isAwaitingPreApproval = privacyRequest.status === PrivacyRequestStatus.AWAITING_PRE_APPROVAL; const isPreApprovalNotEligible = @@ -29,7 +39,7 @@ const useApproveDenyPrivacyRequest = ({ const showAction = isPendingStatus || - isDuplicateStatus || + (isDuplicateStatus && !(action === "approve" && isUnverifiedDuplicate)) || isAwaitingPreApproval || isPreApprovalNotEligible; const modal = useDisclosure(); diff --git a/clients/admin-ui/src/features/privacy-requests/types.ts b/clients/admin-ui/src/features/privacy-requests/types.ts index fb713d716a3..d86d5a18307 100644 --- a/clients/admin-ui/src/features/privacy-requests/types.ts +++ b/clients/admin-ui/src/features/privacy-requests/types.ts @@ -105,7 +105,7 @@ export interface PrivacyRequestEntity { identity: { [key: string]: { label: string; value: string | null }; }; - identity_verified_at?: string; + identity_verified_at?: string | null; custom_privacy_request_fields?: { [key: string]: { label: string; value: any }; }; diff --git a/src/fides/api/models/privacy_request/privacy_request.py b/src/fides/api/models/privacy_request/privacy_request.py index f3ed9f5e619..4ab07f2a25d 100644 --- a/src/fides/api/models/privacy_request/privacy_request.py +++ b/src/fides/api/models/privacy_request/privacy_request.py @@ -661,10 +661,18 @@ def get_persisted_custom_privacy_request_fields(self) -> Dict[str, Any]: } def verify_identity(self, db: Session, provided_code: str) -> "PrivacyRequest": - """Verify the identification code supplied by the user - If verified, change the status of the request to "pending", and set the datetime the identity was verified. + """Verify the identification code supplied by the user. + + If verified, change the status to "pending" and set identity_verified_at. + Duplicate requests are allowed to verify — they transition to "pending" + so that handle_approval can re-evaluate duplicate detection with the + fresh identity_verified_at timestamp (the first-verified request in a + duplicate group becomes canonical). """ - if self.status != PrivacyRequestStatus.identity_unverified: + if self.status not in ( + PrivacyRequestStatus.identity_unverified, + PrivacyRequestStatus.duplicate, + ): raise IdentityVerificationException( f"Invalid identity verification request. Privacy request '{self.id}' status = {self.status.value}." # type: ignore # pylint: disable=no-member ) diff --git a/src/fides/service/privacy_request/privacy_request_service.py b/src/fides/service/privacy_request/privacy_request_service.py index 4e3e81ea80c..4263073f8cc 100644 --- a/src/fides/service/privacy_request/privacy_request_service.py +++ b/src/fides/service/privacy_request/privacy_request_service.py @@ -652,6 +652,26 @@ def approve_privacy_requests( ) continue + if ( + privacy_request.status == PrivacyRequestStatus.duplicate + and privacy_request.identity_verified_at is None + and self.config_proxy.execution.subject_identity_verification_required + ): + logger.error( + "Blocked approval of unverified duplicate privacy request {} by user {}", + privacy_request.id, + reviewed_by, + ) + failed.append( + BulkUpdateFailed( + message="Cannot approve unverified duplicate request", + data=PrivacyRequestResponse.model_validate( + privacy_request + ).model_dump(mode="json"), + ) + ) + continue + try: now = datetime.utcnow() privacy_request.status = PrivacyRequestStatus.approved diff --git a/tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.py b/tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.py index 87a4b07d55f..33e5974df17 100644 --- a/tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.py +++ b/tests/ops/api/v1/endpoints/privacy_request/test_privacy_request_endpoints.py @@ -4264,6 +4264,9 @@ def test_approve_privacy_request( privacy_request_status, ): privacy_request.status = privacy_request_status + if privacy_request_status == PrivacyRequestStatus.duplicate: + # Set verified so test passes with or without verification fixture + privacy_request.identity_verified_at = datetime.utcnow() privacy_request.save(db=db) payload = { @@ -4319,13 +4322,13 @@ def test_bulk_approve_privacy_requests_with_duplicates( user, privacy_requests, ): - # Set first request to pending/duplicate, second to duplicate/pending, third to complete (should fail) - privacy_requests[0].update( - db=db, data={"status": PrivacyRequestStatus.duplicate} - ) - privacy_requests[1].update( - db=db, data={"status": PrivacyRequestStatus.duplicate} - ) + # Set first two to verified duplicates (approvable), third to complete (should fail) + privacy_requests[0].status = PrivacyRequestStatus.duplicate + privacy_requests[0].identity_verified_at = datetime.utcnow() + privacy_requests[0].save(db=db) + privacy_requests[1].status = PrivacyRequestStatus.duplicate + privacy_requests[1].identity_verified_at = datetime.utcnow() + privacy_requests[1].save(db=db) privacy_requests[2].update( db=db, data={"status": PrivacyRequestStatus.complete} ) @@ -4369,6 +4372,120 @@ def test_bulk_approve_privacy_requests_with_duplicates( assert submit_mock.call_count == 2 # Called for each successful approval assert not mock_dispatch_message.called + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" + ) + @pytest.mark.usefixtures("subject_identity_verification_required") + def test_bulk_approve_mixed_verified_unverified_duplicates( + self, + submit_mock, + db, + url, + api_client, + generate_auth_header, + user, + privacy_requests, + ): + """Bulk approve with a mix of verified and unverified duplicates should + approve only the verified ones and report failures for the unverified.""" + # Verified duplicate — should succeed + privacy_requests[0].status = PrivacyRequestStatus.duplicate + privacy_requests[0].identity_verified_at = datetime.utcnow() + privacy_requests[0].save(db=db) + # Unverified duplicate — should fail + privacy_requests[1].status = PrivacyRequestStatus.duplicate + privacy_requests[1].identity_verified_at = None + privacy_requests[1].save(db=db) + + payload = { + JWE_PAYLOAD_ROLES: user.client.roles, + JWE_PAYLOAD_CLIENT_ID: user.client.id, + JWE_ISSUED_AT: datetime.now().isoformat(), + } + auth_header = { + "Authorization": "Bearer " + + generate_jwe(json.dumps(payload), CONFIG.security.app_encryption_key) + } + + body = {"request_ids": [privacy_requests[0].id, privacy_requests[1].id]} + response = api_client.patch(url, headers=auth_header, json=body) + + assert response.status_code == 200 + response_body = response.json() + assert len(response_body["succeeded"]) == 1 + assert len(response_body["failed"]) == 1 + assert response_body["succeeded"][0]["id"] == privacy_requests[0].id + assert response_body["succeeded"][0]["status"] == "approved" + assert response_body["failed"][0]["data"]["id"] == privacy_requests[1].id + assert ( + response_body["failed"][0]["message"] + == "Cannot approve unverified duplicate request" + ) + assert submit_mock.call_count == 1 + + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" + ) + @pytest.mark.usefixtures("subject_identity_verification_required") + def test_approve_unverified_duplicate_rejected( + self, + submit_mock, + db, + url, + api_client, + generate_auth_header, + privacy_request, + ): + """A duplicate request whose identity was never verified should not be + approvable — processing it would bypass identity verification.""" + privacy_request.status = PrivacyRequestStatus.duplicate + privacy_request.identity_verified_at = None + privacy_request.save(db=db) + + auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_REVIEW]) + body = {"request_ids": [privacy_request.id]} + response = api_client.patch(url, headers=auth_header, json=body) + + assert response.status_code == 200 + response_body = response.json() + assert len(response_body["succeeded"]) == 0 + assert len(response_body["failed"]) == 1 + assert ( + response_body["failed"][0]["message"] + == "Cannot approve unverified duplicate request" + ) + assert not submit_mock.called + + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" + ) + @pytest.mark.usefixtures("subject_identity_verification_not_required") + def test_approve_unverified_duplicate_allowed_when_verification_disabled( + self, + submit_mock, + db, + url, + api_client, + generate_auth_header, + privacy_request, + ): + """When identity verification is not required, unverified duplicates + should be approvable — there is no verification gate to bypass.""" + privacy_request.status = PrivacyRequestStatus.duplicate + privacy_request.identity_verified_at = None + privacy_request.save(db=db) + + auth_header = generate_auth_header(scopes=[PRIVACY_REQUEST_REVIEW]) + body = {"request_ids": [privacy_request.id]} + response = api_client.patch(url, headers=auth_header, json=body) + + assert response.status_code == 200 + response_body = response.json() + assert len(response_body["succeeded"]) == 1 + assert len(response_body["failed"]) == 0 + assert response_body["succeeded"][0]["status"] == "approved" + assert submit_mock.called + @mock.patch( "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" ) @@ -6277,6 +6394,86 @@ def test_incorrect_privacy_request_status(self, api_client, url, privacy_request == f"Invalid identity verification request. Privacy request '{privacy_request.id}' status = in_processing." ) + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" + ) + @mock.patch( + "fides.service.messaging.messaging_service.dispatch_message_task.apply_async" + ) + def test_verify_identity_duplicate_request( + self, + mock_dispatch_message, + mock_run_privacy_request, + db, + api_client, + url, + privacy_request, + privacy_request_receipt_notification_enabled, + ): + """A request marked as duplicate before identity verification should + still allow the user to verify their identity. The request transitions + to pending so that handle_approval can re-evaluate duplicate detection + with the fresh identity_verified_at timestamp.""" + privacy_request.status = PrivacyRequestStatus.duplicate + privacy_request.save(db) + privacy_request.cache_identity_verification_code(self.code) + + request_body = {"code": self.code} + resp = api_client.post(url, headers={}, json=request_body) + assert resp.status_code == 200 + + resp_body = resp.json() + assert resp_body["identity_verified_at"] is not None + + db.refresh(privacy_request) + assert privacy_request.identity_verified_at is not None + assert mock_dispatch_message.called # receipt email sent + assert mock_run_privacy_request.called # request queued after re-eval + + @mock.patch( + "fides.api.service.privacy_request.request_runner_service.run_privacy_request.apply_async" + ) + @mock.patch( + "fides.service.messaging.messaging_service.dispatch_message_task.apply_async" + ) + @mock.patch( + "fides.service.privacy_request.privacy_request_service.check_for_duplicates" + ) + def test_verify_identity_duplicate_request_stays_duplicate( + self, + mock_check_for_duplicates, + mock_dispatch_message, + mock_run_privacy_request, + db, + api_client, + url, + privacy_request, + privacy_request_receipt_notification_enabled, + ): + """When a duplicate request verifies identity but another request already + verified first, check_for_duplicates re-marks it as duplicate. The request + should not be queued for processing.""" + + def remark_as_duplicate(db, privacy_request): + privacy_request.status = PrivacyRequestStatus.duplicate + privacy_request.save(db) + + mock_check_for_duplicates.side_effect = remark_as_duplicate + + privacy_request.status = PrivacyRequestStatus.duplicate + privacy_request.save(db) + privacy_request.cache_identity_verification_code(self.code) + + request_body = {"code": self.code} + resp = api_client.post(url, headers={}, json=request_body) + assert resp.status_code == 200 + + db.refresh(privacy_request) + assert privacy_request.identity_verified_at is not None + assert privacy_request.status == PrivacyRequestStatus.duplicate + assert mock_dispatch_message.called # receipt sent before re-eval + assert not mock_run_privacy_request.called # not queued + @mock.patch( "fides.service.messaging.messaging_service.dispatch_message_task.apply_async" )