Conversation
Duplicate detection can mark a privacy request as "duplicate" before the user completes identity verification. Previously, verify_identity rejected any status other than identity_unverified, blocking the user. Now it also accepts duplicate status and transitions to pending, so handle_approval can re-evaluate with the fresh identity_verified_at timestamp — the first-verified request in a group becomes canonical. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (84.94%) is below the target coverage (85.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #7971 +/- ##
==========================================
- Coverage 85.04% 84.94% -0.10%
==========================================
Files 631 630 -1
Lines 41213 41082 -131
Branches 4807 4768 -39
==========================================
- Hits 35049 34899 -150
- Misses 5070 5101 +31
+ Partials 1094 1082 -12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/code-review |
There was a problem hiding this comment.
Code Review: Fix duplicate request identity verification
The fix is targeted, minimal, and logically correct. Here's a summary of the analysis.
What the change does
Previously, verify_identity only allowed the identity_unverified status. Any request already marked duplicate would throw IdentityVerificationException, blocking the user from completing verification. The fix adds duplicate to the allowed set so those requests can transition to pending. The endpoint then calls handle_approval → check_for_duplicates → is_duplicate_request, which re-evaluates the request with the fresh identity_verified_at timestamp, potentially promoting it to canonical.
Logic correctness
The re-evaluation path is sound:
- After
verify_identity, status ispending(notduplicate), so the early-return atduplication_detection.py:398(if request.status == PrivacyRequestStatus.duplicate: return True) does not trigger. verified_identity_casescorrectly picks the request with the earliestidentity_verified_atas canonical and re-marks the rest asduplicate. The docstring claim about "first-verified becomes canonical" is accurate.- The verification code is single-use and TTL-bound, so no infinite
duplicate → pending → duplicateloop is possible.
Findings
Three comments are inline; summarized here:
-
Missing assertions in the new test —
mock_dispatch_messageandmock_run_privacy_requestare patched but never asserted. The test doesn't confirm whether a receipt email was sent or whether the request was queued. -
No test for the "stays duplicate" case — only the happy path (duplicate becomes canonical) is tested. The inverse case — duplicate verifies after the canonical already verified, so it should be re-marked as
duplicate— is not covered. -
Receipt email sent before re-evaluation (pre-existing, low severity) —
send_privacy_request_receiptfires beforehandle_approvalre-checks duplication, so in the inverse case above, a receipt email is sent for a request that ends up back induplicate. Not introduced by this PR, but the fix expands the cases where this can happen.
Summary
The core model change is correct and the changelog entry is present. The main gap is test coverage for the duplicate → pending → duplicate cycle. The happy-path fix is ready; the test improvements are recommended before merging.
🔬 Codegraph: connected (47107 nodes)
💡 Write /code-review in a comment to re-run this review.
- Add assertions for mock_dispatch_message and mock_run_privacy_request in test_verify_identity_duplicate_request - Add test_verify_identity_duplicate_request_stays_duplicate covering the duplicate→pending→duplicate cycle Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
erosselli
left a comment
There was a problem hiding this comment.
approved with a question!
|
|
||
| self._verify_identity(provided_code=provided_code) | ||
|
|
||
| self.status = PrivacyRequestStatus.pending |
There was a problem hiding this comment.
if we mark it as pending, is there any other way to flag to the admins that "this might be a duplicate " ?
There was a problem hiding this comment.
It checks when it re-queues and will get marked as a duplicate or if it is the first verified DSR it will get processed as usual and the unverified DSR will get marked as duplicate instead. Tested both paths manually.
Change type from Fixed to Security and link to GHSA-qx5f-ghc2-7g5c.
Ticket ENG-3492
Description Of Changes
When duplicate detection is enabled, a privacy request can be marked as
duplicatebefore the user completes identity verification (via the privacy center flow). Previously,verify_identityonly acceptedidentity_unverifiedstatus, so it rejected duplicate requests with a 400 error — blocking the user from completing verification.This fix allows
verify_identityto also acceptduplicatestatus. The request transitions topendingso thathandle_approvalcan re-evaluate duplicate detection with the freshidentity_verified_attimestamp. This matters because the first-verified request in a duplicate group should become canonical — the duplicate detection logic inverified_identity_casesusesidentity_verified_atordering to determine which request wins.Code Changes
src/fides/api/models/privacy_request/privacy_request.py—verify_identitynow acceptsduplicatein addition toidentity_unverifiedstatus, always transitions topendingtests/.../test_privacy_request_endpoints.py— Addedtest_verify_identity_duplicate_requestto confirm duplicate requests can successfully verifySteps to Confirm — Verified Locally
identity_unverifiedduplicatepending, becomes canonicalduplicateduplicate(second to verify)E2E results:
Pre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works