Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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: []
4 changes: 4 additions & 0 deletions changelog/7972-block-approve-unverified-duplicate.yaml
Original file line number Diff line number Diff line change
@@ -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: []
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -17,19 +18,28 @@ 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 =
privacyRequest.status === PrivacyRequestStatus.PRE_APPROVAL_NOT_ELIGIBLE;

const showAction =
isPendingStatus ||
isDuplicateStatus ||
(isDuplicateStatus && !(action === "approve" && isUnverifiedDuplicate)) ||
isAwaitingPreApproval ||
isPreApprovalNotEligible;
const modal = useDisclosure();
Expand Down
2 changes: 1 addition & 1 deletion clients/admin-ui/src/features/privacy-requests/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
};
Expand Down
14 changes: 11 additions & 3 deletions src/fides/api/models/privacy_request/privacy_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Expand Down
20 changes: 20 additions & 0 deletions src/fides/service/privacy_request/privacy_request_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {
Expand Down Expand Up @@ -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}
)
Expand Down Expand Up @@ -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"
)
Expand Down Expand Up @@ -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"
)
Expand Down
Loading