feat(erasure): Phase 8C — RA 10173 Erasure & Anonymization#82
Conversation
…nding_review gate
…ization, dead-letter
…, active erasure skip
…e_active sentinel, auditLog
…eSweep, retentionSweep
…ete, pending_review gate
…+ signOut orchestration
… erasure_active sentinel, auditLog
Reviewer's GuideImplements the RA 10173 data erasure execution loop end‑to‑end: citizen erasure request flow (backend callable + PWA UX), legal hold controls, scheduled erasure and retention sweeps (including SMS + Storage handling), updated Firestore/Storage rules, and tests for callables, triggers, and security rules, plus a bugfix to the erasure approval deny path. Sequence diagram for citizen data erasure request and sign-outsequenceDiagram
actor Citizen
participant DeleteAccountFlow
participant ErasureService as requestDataErasureAndSignOut
participant FirebaseFunctions as requestDataErasureCallable
participant Firestore
participant Auth
participant Router as AppRouter
Citizen->>DeleteAccountFlow: click Delete my account
DeleteAccountFlow->>DeleteAccountFlow: step = warn
Citizen->>DeleteAccountFlow: confirm warning
DeleteAccountFlow->>DeleteAccountFlow: step = confirm
Citizen->>DeleteAccountFlow: type DELETE and submit
DeleteAccountFlow->>ErasureService: requestDataErasureAndSignOut()
ErasureService->>FirebaseFunctions: call requestDataErasure
FirebaseFunctions->>Firestore: runTransaction(create erasure_active, erasure_requests)
Firestore-->>FirebaseFunctions: transaction committed
FirebaseFunctions->>Auth: updateUser(uid, disabled true)
Auth-->>FirebaseFunctions: success
FirebaseFunctions-->>ErasureService: success
ErasureService->>Auth: signOut()
Auth-->>ErasureService: success
ErasureService-->>DeleteAccountFlow: resolved
DeleteAccountFlow->>Router: onGoodbye()
Router->>Citizen: navigate to GoodbyeScreen
rect rgb(255,230,230)
FirebaseFunctions->>Firestore: if sentinel exists, abort
FirebaseFunctions-->>ErasureService: HttpsError already_exists
ErasureService-->>DeleteAccountFlow: reject
DeleteAccountFlow->>DeleteAccountFlow: show error message
end
rect rgb(255,230,230)
FirebaseFunctions->>Auth: updateUser disabled true
Auth-->>FirebaseFunctions: failure
FirebaseFunctions->>Firestore: delete erasure_requests, erasure_active
Firestore-->>FirebaseFunctions: rollback complete
FirebaseFunctions-->>ErasureService: HttpsError internal auth_disable_failed
ErasureService-->>DeleteAccountFlow: reject
DeleteAccountFlow->>DeleteAccountFlow: show error message
end
Sequence diagram for scheduled erasure sweep executionsequenceDiagram
participant Scheduler as CloudScheduler
participant ErasureSweep as erasureSweep
participant Firestore
participant Auth
participant Storage
participant Audit as AuditStream
Scheduler->>ErasureSweep: trigger every 15 minutes
ErasureSweep->>Firestore: query erasure_requests status approved_pending_anonymization and legalHold not true limit 1
Firestore-->>ErasureSweep: readySnap
ErasureSweep->>Firestore: query erasure_requests status executing and stale limit 1
Firestore-->>ErasureSweep: staleSnap
ErasureSweep->>Firestore: query erasure_requests status approved_pending_anonymization and legalHold true
Firestore-->>ErasureSweep: heldSnap
ErasureSweep->>ErasureSweep: pick candidate request
alt no candidate
ErasureSweep-->>Scheduler: return processed 0
else candidate found
ErasureSweep->>Firestore: update candidate status executing, set sweepRunId, executionStartedAt
ErasureSweep->>Firestore: query reports where submittedBy equals citizenUid
Firestore-->>ErasureSweep: reportsSnap
loop for each reportId
ErasureSweep->>Firestore: get report_private for reportId
Firestore-->>ErasureSweep: privateSnap
ErasureSweep->>ErasureSweep: collect senderMsisdnHash
end
loop anonymize reports
ErasureSweep->>Firestore: update reports submittedBy citizen_deleted, mediaRedacted true
ErasureSweep->>Firestore: update report_private PII fields null
ErasureSweep->>Firestore: get report_contacts
Firestore-->>ErasureSweep: contactSnap
ErasureSweep->>Firestore: update report_contacts all fields except reportId null
end
loop null sms_sessions and sms_inbox
ErasureSweep->>Firestore: query sms_sessions by senderMsisdnHash
Firestore-->>ErasureSweep: sessSnap
ErasureSweep->>Firestore: update sms_sessions senderMsisdnHash null, msisdn null
ErasureSweep->>Firestore: query sms_inbox by senderMsisdnHash
Firestore-->>ErasureSweep: inboxSnap
ErasureSweep->>Firestore: update sms_inbox senderMsisdnHash null, msisdn null, rawBody null
end
loop delete Storage media
ErasureSweep->>Storage: bucket getFiles prefix report_media/
Storage-->>ErasureSweep: files
ErasureSweep->>Storage: delete files matching reportId
end
ErasureSweep->>Auth: deleteUser citizenUid
Auth-->>ErasureSweep: success
ErasureSweep->>Firestore: delete erasure_active sentinel
ErasureSweep->>Firestore: update erasure_request status completed, completedAt
ErasureSweep->>Audit: streamAuditEvent erasure_completed
ErasureSweep-->>Scheduler: result processed 1
end
rect rgb(255,230,230)
ErasureSweep->>ErasureSweep: executeErasure throws error
ErasureSweep->>Auth: updateUser citizenUid disabled false
Auth-->>ErasureSweep: success or failure logged
ErasureSweep->>Firestore: update erasure_request status dead_lettered, deadLetterReason, deadLetteredAt
ErasureSweep->>Audit: streamAuditEvent erasure_request_dead_lettered_with_auth_unblocked
end
Sequence diagram for superadmin approval and denial of erasure requestssequenceDiagram
actor Superadmin
participant ApproveCallable as approveErasureRequest
participant Firestore
participant Auth
participant Audit as AuditStream
Superadmin->>ApproveCallable: approveErasureRequest approved true
ApproveCallable->>Firestore: runTransaction get erasure_requests doc
Firestore-->>ApproveCallable: snap
alt status not pending_review
ApproveCallable-->>Superadmin: HttpsError failed_precondition erasure_already_reviewed
else status pending_review
ApproveCallable->>Firestore: tx update status approved_pending_anonymization, reviewedBy, reviewedAt, reviewReason
Firestore-->>ApproveCallable: transaction committed
ApproveCallable->>Audit: streamAuditEvent erasure_request_reviewed metadata approved true
ApproveCallable-->>Superadmin: success
end
Superadmin->>ApproveCallable: approveErasureRequest approved false
ApproveCallable->>Firestore: get erasure_requests doc
Firestore-->>ApproveCallable: snap
alt status not pending_review
ApproveCallable-->>Superadmin: HttpsError failed_precondition erasure_already_reviewed
else status pending_review
ApproveCallable->>Auth: updateUser citizenUid disabled false
Auth-->>ApproveCallable: success
ApproveCallable->>Firestore: runTransaction update request status denied and delete erasure_active sentinel
alt transaction success
Firestore-->>ApproveCallable: committed
ApproveCallable->>Audit: streamAuditEvent erasure_request_reviewed metadata approved false
ApproveCallable-->>Superadmin: success
else transaction fails
Firestore-->>ApproveCallable: error
ApproveCallable->>Auth: updateUser citizenUid disabled true
Auth-->>ApproveCallable: best effort
ApproveCallable-->>Superadmin: HttpsError internal deny_write_failed
end
end
ER diagram for erasure and retention related collectionserDiagram
ERASURE_REQUESTS {
string id
string citizenUid
string status
boolean legalHold
number requestedAt
number reviewedAt
string reviewedBy
string reviewReason
string sweepRunId
number executionStartedAt
number completedAt
string deadLetterReason
number deadLetteredAt
}
ERASURE_ACTIVE {
string citizenUid
number createdAt
}
REPORTS {
string id
string submittedBy
boolean verified
number submittedAt
boolean mediaRedacted
number retentionAnonymizedAt
number retentionHardDeleteEligibleAt
}
REPORT_PRIVATE {
string reportId
string citizenName
string rawPhone
string gpsExact
string addressText
string senderMsisdnHash
}
REPORT_CONTACTS {
string reportId
}
SMS_SESSIONS {
string id
string senderMsisdnHash
string msisdn
}
SMS_INBOX {
string id
string senderMsisdnHash
string msisdn
string rawBody
}
RETENTION_AUDIT_LOG {
string id
string reportId
number retentionDeletedAt
string reason
}
ERASURE_ACTIVE ||--|| ERASURE_REQUESTS : citizenUid
ERASURE_REQUESTS ||--o{ REPORTS : anonymizes_reports_of
ERASURE_REQUESTS ||--o{ SMS_SESSIONS : nulls_sessions_for
ERASURE_REQUESTS ||--o{ SMS_INBOX : nulls_messages_for
REPORTS ||--|| REPORT_PRIVATE : has_private
REPORTS ||--|| REPORT_CONTACTS : has_contacts
REPORT_PRIVATE }o--o{ SMS_SESSIONS : linked_by_senderMsisdnHash
REPORT_PRIVATE }o--o{ SMS_INBOX : linked_by_senderMsisdnHash
REPORTS ||--o{ RETENTION_AUDIT_LOG : deletion_logged_in
Class diagram for erasure and retention core functions and PWA componentsclassDiagram
class RequestDataErasureCore {
+requestDataErasureCore(db Firestore, auth Auth, actor Actor) void
}
class ApproveErasureRequestCore {
+approveErasureRequestCore(db Firestore, auth Auth, input unknown, actor Actor) void
}
class SetErasureLegalHoldCore {
+setErasureLegalHoldCore(db Firestore, input unknown, actor Actor) void
}
class ErasureSweepCore {
+erasureSweepCore(input ErasureSweepInput) ErasureSweepResult
+executeErasure(input ErasureSweepInput, citizenUid string, requestId string) void
}
class RetentionSweepCore {
+retentionSweepCore(input RetentionSweepInput) RetentionSweepResult
}
class ErasureSweepInput {
+db Firestore
+auth Auth
+storage Storage
+now function
}
class ErasureSweepResult {
+processed number
+skippedHeld number
+deadLettered number
}
class RetentionSweepInput {
+db Firestore
+storage Storage
+now function
}
class RetentionSweepResult {
+anonymized number
+hardDeleted number
}
class Actor {
+uid string
}
class DeleteAccountFlowComponent {
+step Step
+typed string
+error string
+DeleteAccountFlow(onGoodbye function) void
-handleConfirm() void
}
class GoodbyeScreenComponent {
+GoodbyeScreen() void
}
class ErasureService {
+requestDataErasureAndSignOut() void
}
class Step {
}
RequestDataErasureCore --> Actor
ApproveErasureRequestCore --> Actor
SetErasureLegalHoldCore --> Actor
ErasureSweepCore --> ErasureSweepInput
ErasureSweepCore --> ErasureSweepResult
RetentionSweepCore --> RetentionSweepInput
RetentionSweepCore --> RetentionSweepResult
DeleteAccountFlowComponent --> Step
DeleteAccountFlowComponent --> ErasureService
ErasureService --> RequestDataErasureCore
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughThis PR implements Phase 8C (RA 10173 compliance) with complete data erasure and anonymization. It adds user-facing account deletion UI, Firebase callables for requesting and managing erasure requests, scheduled sweeps that anonymize and hard-delete citizen data, and corresponding Firestore/Storage security rules with comprehensive test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Citizen
participant UI as DeleteAccountFlow
participant Service as erasure Service
participant Callable as requestDataErasure<br/>(Callable)
participant Firestore as Firestore
participant Auth as Firebase Auth
participant ErasureSweep as erasureSweep<br/>(Trigger)
participant Storage as Cloud Storage
User->>UI: Click "Delete my account"
UI->>UI: Show warning modal
User->>UI: Confirm & type "DELETE"
UI->>Service: requestDataErasureAndSignOut()
Service->>Callable: Invoke requestDataErasure({})
Callable->>Firestore: Create erasure_requests (pending_review)
Callable->>Firestore: Create erasure_active sentinel
Callable->>Auth: Disable citizen account
Callable-->>Service: Success
Service->>Auth: signOut()
Service-->>UI: Complete
UI->>UI: Call onGoodbye(), navigate to /goodbye
Note over ErasureSweep: Scheduled every 15 minutes
ErasureSweep->>Firestore: Select eligible erasure_requests
ErasureSweep->>Firestore: Set status=executing
ErasureSweep->>Firestore: Anonymize reports/contacts
ErasureSweep->>Firestore: Null PII fields in report_private
ErasureSweep->>Storage: Delete report_media blobs
ErasureSweep->>Auth: Hard delete citizen account
ErasureSweep->>Firestore: Set status=completed
ErasureSweep->>Firestore: Delete erasure_active sentinel
ErasureSweep-->>Firestore: Emit audit event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 35 minutes and 42 seconds.Comment |
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- In
approveErasureRequestCorethe deny path reads the request and checksstatus !== 'pending_review'outside of the transaction that later updates the doc and deletes the sentinel, so a concurrent approve/deny can still both succeed; consider moving the read + status check into the same transaction that performs the update/delete, similar to the approve path. - Both
executeErasureandretentionSweepCoredelete Storage blobs by callingbucket().getFiles({ prefix: 'report_media/' })and then filteringincludes(reportId)for each report, which will become very inefficient as the bucket grows; it would be more scalable to either structure file paths so you can query with a more specific prefix per report, or fetch the file list once and group by report ID instead of per-report scans. - In
erasureSweepCorethe claim step performs a blindcandidate.ref.update({ status: 'executing', ... })without verifying the prior status orsweepRunId, so two sweep invocations could race and both consider the same request claimed; using a transaction or an update with preconditions on the expected status/timestamps would make the claim truly exclusive.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `approveErasureRequestCore` the deny path reads the request and checks `status !== 'pending_review'` outside of the transaction that later updates the doc and deletes the sentinel, so a concurrent approve/deny can still both succeed; consider moving the read + status check into the same transaction that performs the update/delete, similar to the approve path.
- Both `executeErasure` and `retentionSweepCore` delete Storage blobs by calling `bucket().getFiles({ prefix: 'report_media/' })` and then filtering `includes(reportId)` for each report, which will become very inefficient as the bucket grows; it would be more scalable to either structure file paths so you can query with a more specific prefix per report, or fetch the file list once and group by report ID instead of per-report scans.
- In `erasureSweepCore` the claim step performs a blind `candidate.ref.update({ status: 'executing', ... })` without verifying the prior status or `sweepRunId`, so two sweep invocations could race and both consider the same request claimed; using a transaction or an update with preconditions on the expected status/timestamps would make the claim truly exclusive.
## Individual Comments
### Comment 1
<location path="functions/src/callables/approve-erasure-request.ts" line_range="55-32" />
<code_context>
+ return
+ }
+
+ // Deny path: re-enable Auth → update doc + delete sentinel → rollback on failure.
+ const snap = await requestRef.get()
+ if (!snap.exists) throw new HttpsError('not-found', 'erasure_request_not_found')
+ if (snap.data()?.status !== 'pending_review') {
+ throw new HttpsError('failed-precondition', 'erasure_already_reviewed')
+ }
+ const citizenUid = snap.data()?.citizenUid as string
+
+ await auth.updateUser(citizenUid, { disabled: false })
+
+ try {
+ // eslint-disable-next-line @typescript-eslint/require-await
+ await db.runTransaction(async (tx) => {
+ const sentinelRef = db.collection('erasure_active').doc(citizenUid)
+ tx.update(requestRef, {
</code_context>
<issue_to_address>
**issue (bug_risk):** Re-check request status inside the deny transaction to avoid races with concurrent approvals.
The deny flow checks `status !== 'pending_review'` before `auth.updateUser`, but the transaction then calls `tx.update(requestRef, { status: 'denied', ... })` without re-reading or validating `status`. If an approval transaction commits between the initial `get()` and this transaction, a legitimate status like `approved_pending_anonymization` or `executing` can be overwritten to `denied`. Please move the status check into the transaction (re-read the doc and verify `status === 'pending_review'`) so the write is conditional and aborts if the status has changed.
</issue_to_address>
### Comment 2
<location path="functions/src/triggers/erasure-sweep.ts" line_range="28-37" />
<code_context>
+ // Sequential claim: fetch one ready record (or one stale executing record).
</code_context>
<issue_to_address>
**issue (bug_risk):** Claiming a request via plain update can lead to double-processing and inconsistent final status.
Two sweep runs can read the same `approved_pending_anonymization` doc before either updates it and both call `update({ status: 'executing', ... })` on the same `candidate.ref`, then both run `executeErasure`. The second run will typically fail at `auth.deleteUser`, hit the catch block, attempt to re-enable Auth for an already-deleted user, and overwrite the status to `dead_lettered` even though erasure actually succeeded. To prevent this double-processing and status corruption, claim the record in a transaction that (1) re-reads the doc, (2) confirms it is still `approved_pending_anonymization` (or a stale `executing`), and (3) updates to `executing` only under that precondition. When recovering stale `executing` records, also verify the existing `status` and `sweepRunId`.
</issue_to_address>
### Comment 3
<location path="functions/src/triggers/erasure-sweep.ts" line_range="185-187" />
<code_context>
+ }
+ }
+
+ // Step 8: Delete Storage blobs for all citizen reports (verified and unverified)
+ for (const reportId of reportIds) {
+ const [files] = await input.storage.bucket().getFiles({ prefix: `report_media/` })
+ for (const file of files.filter((f) => f.name.includes(reportId))) {
+ await file.delete()
</code_context>
<issue_to_address>
**suggestion (performance):** Storage deletion loops repeatedly list the entire `report_media/` prefix, which will not scale.
For each `reportId`, this re-runs `getFiles({ prefix: 'report_media/' })` and then filters client-side by `reportId`, so the total work is O(N²) and will scale poorly as the bucket grows. If possible, either: (a) structure storage so each reportId has its own prefix (e.g. `report_media/${reportId}/`) and list by that, or (b) list once outside the loop and group files by `reportId` before deleting.
Suggested implementation:
```typescript
// Step 8: Delete Storage blobs for all citizen reports (verified and unverified)
for (const reportId of reportIds) {
// List only blobs under the per-report prefix to avoid repeatedly scanning the entire bucket
const [files] = await input.storage
.bucket()
.getFiles({ prefix: `report_media/${reportId}/` })
for (const file of files) {
await file.delete()
}
}
```
To fully implement this optimization you should ensure that all report media is written using a per-report prefix that matches this deletion pattern, e.g. `report_media/${reportId}/...`. If the current upload paths differ (for example, `report_media/${reportId}-foo.jpg` or another naming convention), update the `prefix` passed to `getFiles` here to match that structure and/or adjust the upload paths in the relevant write code so each report's media lives under its own prefix.
</issue_to_address>
### Comment 4
<location path="functions/src/triggers/retention-sweep.ts" line_range="78-82" />
<code_context>
+ await input.db.collection('report_contacts').doc(doc.id).update(nulled)
+ }
+
+ // Delete Storage blobs
+ const [files] = await input.storage.bucket().getFiles({ prefix: 'report_media/' })
+ for (const file of files.filter((f) => f.name.includes(doc.id))) {
+ await file.delete()
</code_context>
<issue_to_address>
**suggestion (performance):** Retention sweep also re-lists all media per report, leading to quadratic storage listing cost.
This currently lists all `report_media/` files for every report and then filters by `doc.id`, which yields quadratic listing cost on large buckets and can hit storage/listing limits. Prefer using a report-specific prefix so you can list only that report’s files, or refactor to perform one listing per run and reuse the result across deletions.
```suggestion
// Delete Storage blobs for this report only
const [files] = await input.storage.bucket().getFiles({
prefix: `report_media/${doc.id}`,
})
for (const file of files) {
await file.delete()
}
```
</issue_to_address>
### Comment 5
<location path="apps/citizen-pwa/src/components/DeleteAccountFlow.tsx" line_range="44" />
<code_context>
+
+ if (step === 'warn') {
+ return (
+ <div role="dialog" aria-modal="true">
+ <h2>Delete your account?</h2>
+ <p>This will permanently:</p>
+ <ul>
+ <li>Remove your name, contact info, and account</li>
+ <li>Anonymize your reports (they remain as public record)</li>
+ <li>Sign you out immediately</li>
+ </ul>
+ <p>This cannot be undone. Your request will be reviewed before deletion is complete.</p>
+ <button
+ onClick={() => {
+ setStep('idle')
+ }}
+ >
+ Cancel
+ </button>
+ <button
+ onClick={() => {
+ setStep('confirm')
+ }}
+ >
+ Yes, delete my account →
+ </button>
+ </div>
+ )
+ }
+
+ return (
+ <div role="dialog" aria-modal="true">
+ <h2>Are you sure?</h2>
+ <label htmlFor="delete-confirm">Type DELETE to confirm</label>
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Dialogs lack accessible labelling, which may make them hard to use with assistive tech.
These dialogs set `role="dialog"` and `aria-modal="true"` but lack an accessible name (`aria-labelledby` or `aria-label`), so screen readers may treat them as unnamed. Since you already render headings (e.g. "Delete your account?", "Are you sure?"), give those headings `id`s and reference them from the dialog via `aria-labelledby` so the modal’s purpose is announced to assistive technology.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- Update firestore.rules.template with erasure_requests/erasure_active rules - Move deny-path status check inside transaction (race condition fix) - Wrap erasureSweep claim in transaction with status precondition - Use per-report storage prefix for blob deletion (performance) - Add aria-labelledby to DeleteAccountFlow dialogs (a11y) - Preserve HttpsError re-throw in deny catch block
There was a problem hiding this comment.
Actionable comments posted: 21
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/citizen-pwa/src/components/DeleteAccountFlow.test.tsx`:
- Around line 6-9: Hoist the erasure mock by wrapping its declaration in
vi.hoisted so each test gets isolated control over
mockResolvedValueOnce/mockRejectedValueOnce; specifically, move the current
mockErasure = vi.fn() into a vi.hoisted block and return it from the vi.mock
factory used for requestDataErasureAndSignOut so the module factory references
the hoisted mockErasure (not an uninitialized variable) when mocking
requestDataErasureAndSignOut.
In `@apps/citizen-pwa/src/components/DeleteAccountFlow.tsx`:
- Around line 61-63: The handlers that change the flow step (e.g., the onClick
currently calling setStep('confirm') and the other handler at the second
location) leave previous confirmation state (typed and error) intact; add logic
to reset the confirmation state by clearing typed and error whenever you exit or
reopen the dialog—either call setTyped('') and setError(null) directly in those
onClick handlers or introduce a small helper like resetConfirmationState() and
call it alongside setStep(...) in the relevant handlers (references: setStep,
setTyped, setError, and the onClick handlers that set the step).
- Around line 22-25: The catch block in DeleteAccountFlow.tsx currently swallows
all errors and sets a generic failure via setError and setStep('confirm');
change the catch to capture the error object (catch (err)) and branch on the
erasure-conflict sentinel (e.g. err.code === 'already-exists' or
err.message.includes('already-exists')) so that when an idempotent
"already-exists" erasure response occurs you treat it as a recoverable/expected
state (advance the flow or show a success/info message) instead of marking it as
a hard failure; keep the existing generic setError/setStep('confirm') behavior
for other errors.
In `@apps/citizen-pwa/src/services/erasure.ts`:
- Around line 4-7: The requestDataErasureAndSignOut function must treat a
backend error code 'already-exists' as success and always complete sign-out;
wrap the httpsCallable(fns(), 'requestDataErasure')({}) call in a try/catch, if
the caught error has code === 'already-exists' swallow it (or log and continue),
otherwise rethrow or surface it, and ensure signOut(auth()) is called in a
finally or after handling so the session is always terminated; refer to
requestDataErasureAndSignOut, httpsCallable, 'requestDataErasure', signOut,
fns() and auth() to locate where to apply this change.
In `@functions/src/__tests__/callables/approve-erasure-request.test.ts`:
- Around line 97-109: The test is a placeholder and must actually invoke the
code path to validate rollback: call approveErasureRequestCore (or the exported
function that triggers the two-step update flow) inside the
env!.withSecurityRulesDisabled block after seeding the request with
seedRequest('req-4', 'pending_review'), configure mockUpdateUser to succeed for
the first call and throw/reject on the second to simulate a write failure, then
assert that the re-disable call was invoked (e.g., check mockUpdateUser mock
calls or the specific re-disable helper was called) and that an error was
thrown/handled; update the test to remove the expect(true).toBe(true)
placeholder and replace it with these actual assertions so the deny rollback
path is exercised and verified.
In `@functions/src/__tests__/callables/request-data-erasure.test.ts`:
- Around line 15-16: Reset the mock state before configuring return values so
call history assertions are deterministic: in the test file's beforeEach, call
mockUpdateUser.mockClear() (or mockUpdateUser.mockReset()) before
mockUpdateUser.mockResolvedValue(undefined), and do the same for any other mock
functions used in this file so not.toHaveBeenCalled() checks are reliable.
- Around line 6-9: Replace the current non-hoisted mockUpdateUser with a hoisted
factory so per-test alterations like mockRejectedValueOnce don't leak across
tests: use vi.hoisted to create and export mockUpdateUser (e.g., const {
mockUpdateUser } = vi.hoisted(() => ({ mockUpdateUser: vi.fn() }))) and keep the
vi.mock('firebase-admin/auth', ...) implementation returning getAuth: () => ({
updateUser: mockUpdateUser }); also ensure tests that call
mockUpdateUser.mockRejectedValueOnce(...) rely on this hoisted mock so each test
gets isolated behavior.
In `@functions/src/__tests__/callables/set-erasure-legal-hold.test.ts`:
- Around line 82-89: The test title claims it covers "completed or denied" but
only sets up a 'completed' request; either rename the test to reflect only the
completed case or add the denied variant: inside the same it-block (or a new
it-block) create another document in the erasure_requests collection (e.g. doc
'req-4') with status: 'denied' (mirror the existing await
db.collection('erasure_requests').doc('req-3').set(...) pattern), then invoke
the same callable/expect assertion used for req-3 so the test asserts the
function throws the same failed-precondition for both statuses; update the
it(...) string if you choose to only test completed.
In `@functions/src/__tests__/rules/erasure-requests.rules.test.ts`:
- Around line 92-101: The test "superadmin can read any request" currently seeds
only the erasure_requests doc but omits the active_accounts precondition
required by the rule (isSuperadmin() && isActivePrivileged()); update the test
to, inside the env!.withSecurityRulesDisabled block before creating the
erasure_requests doc, also setDoc(doc(ctx.firestore(), 'active_accounts',
'uid-admin'), { accountStatus: 'active' }) so the
authenticatedContext('uid-admin', superadminToken) meets both isSuperadmin() and
isActivePrivileged() checks and the assertSucceeds(getDoc(...)) remains valid.
In `@functions/src/__tests__/triggers/erasure-sweep.test.ts`:
- Around line 66-70: Reset the shared mock call history at the start of each
test in the beforeEach block by calling the appropriate Jest reset method (e.g.,
mockClear() or mockReset()) on mockUpdateUser, mockDeleteUser, and mockGetFiles
so prior test calls don't affect later assertions; locate the beforeEach setup
and add mockUpdateUser.mockClear(), mockDeleteUser.mockClear(), and
mockGetFiles.mockClear() (or mockReset() if you also need to clear
implementations) before setting mockResolvedValue.
In `@functions/src/callables/approve-erasure-request.ts`:
- Around line 79-81: The empty catch on auth.updateUser(citizenUid, { disabled:
true }) swallows rollback failures; replace it with a catch that captures the
error (e.g., assign to a local rollbackError variable), log the failure with
context (include citizenUid and the error) and preserve original-error
precedence by not masking the primary error — instead attach the rollbackError
to the primary error object (or store it for inclusion in the response) so
recovery evidence is recorded; reference auth.updateUser and citizenUid and add
a rollbackError variable and a logger call in the catch block.
- Around line 55-76: The deny path checks requestRef.status outside the
transaction and then updates it inside db.runTransaction, which allows a race
where a concurrent review can overwrite this deny; to fix, perform the status
gating inside the transaction by using tx.get(requestRef) at the start of the
db.runTransaction callback, verify the doc exists and its status is still
'pending_review' (throw HttpsError('failed-precondition', ... ) from inside the
tx if not), then call tx.update(requestRef, {...}) and tx.delete(sentinelRef);
also move the auth.updateUser(citizenUid, { disabled: false }) call to after the
transaction completes successfully so you don't re-enable the user if the
transaction rolls back (use requestRef, db.runTransaction, tx.get, tx.update,
tx.delete, sentinelRef and auth.updateUser to locate the changes).
In `@functions/src/callables/request-data-erasure.ts`:
- Around line 34-36: The catch block that currently calls
Promise.allSettled([requestRef.delete(), sentinelRef.delete()]) and then throws
new HttpsError('internal','auth_disable_failed') must inspect the allSettled
results and handle failures explicitly: after awaiting Promise.allSettled, check
each result (for requestRef.delete() and sentinelRef.delete()) and if any
settled as "rejected" log the error and throw a distinct HttpsError (or include
details) so callers know rollback failed; ensure you reference the specific
operations (requestRef.delete, sentinelRef.delete) and do not swallow their
rejection—fail fast with a clear message and preserve the original auth-disable
error context when rethrowing.
In `@functions/src/callables/set-erasure-legal-hold.ts`:
- Around line 26-39: The read-then-update must be done inside a Firestore
transaction to make the legal-hold state transition atomic: replace the separate
ref.get() and ref.update() with db.runTransaction(async (tx) => { const snap =
await tx.get(ref); if (!snap.exists) throw ...; const status =
snap.data()?.status as string; if (TERMINAL_STATUSES.has(status)) throw ...;
tx.update(ref, { legalHold: data.hold, legalHoldReason: data.reason,
legalHoldSetBy: actor.uid }); }); so the existence/status check and the update
on the 'erasure_requests' doc referenced by ref are performed atomically.
In `@functions/src/triggers/erasure-sweep.ts`:
- Around line 28-59: The current two-step "query then update" for claiming an
erasure request is race-prone; change it to an atomic transactional claim:
inside a Firestore transaction, re-read the chosen document (candidate.ref) and
verify its status is still either 'approved_pending_anonymization' (and
legalHold !== true) or stale 'executing' with executionStartedAt < now() -
STALE_EXECUTING_MS, then set status to 'executing' and write sweepRunId and
executionStartedAt; if the verification fails abort the transaction and let the
function return without claiming. Use the same symbols (readySnap, staleSnap,
candidate, sweepRunId, executionStartedAt, STALE_EXECUTING_MS) so the logic
locates the document and updates it only within the transaction.
- Around line 185-190: The deletion loop in erasure-sweep.ts currently lists the
entire report_media/ bucket for each reportId and uses
file.name.includes(reportId), which risks substring collisions and inefficiency;
change the logic in the for (const reportId of reportIds) loop to list only
files for that specific report by using a report-scoped prefix (e.g., derive a
prefix like `report_media/${reportId}/` or otherwise construct the exact object
key pattern) when calling input.storage.bucket().getFiles, and match filenames
with exact equality or startsWith rather than includes(reportId) before calling
file.delete(); update the code paths using getFiles, file.delete, and the
reportId variable accordingly to avoid full-bucket scans and accidental
deletions.
In `@functions/src/triggers/retention-sweep.ts`:
- Around line 79-82: The current code uses substring matching (files.filter((f)
=> f.name.includes(doc.id))) which can remove unrelated files and is
inefficient; instead construct deterministic storage paths and list/delete by
exact prefix for that document (e.g., use getFiles({ prefix:
`report_media/${doc.id}` }) or delete known file paths via
bucket.file(path).delete()), replace the files.filter usage with a narrow
getFiles call and call file.delete only on the returned files; update references
to input.storage.bucket(), getFiles, file.delete and doc.id accordingly.
- Around line 120-146: The hard-delete loop (iterating over toDelete from the
retentionHardDeleteEligibleAt query) must skip reports belonging to citizens
with active erasure requests: before deleting, derive the report owner UID (use
the same field your anonymization code uses, e.g. doc.get('citizenUid') ??
doc.get('ownerUid')) and if activeErasureUids.has(uid) return/continue without
deleting; keep the rest of the logic (deleting report_private, report_contacts,
doc.ref.delete, and writing retention_audit_log) unchanged and log/skipping
counts accordingly. Ensure you reference activeErasureUids, toDelete, and the
retentionHardDeleteEligibleAt query when implementing the check.
In `@infra/firebase/firestore.rules`:
- Around line 514-515: The create rule currently only checks ownership; update
the allow create rule for this collection to validate the incoming document
schema and that the payload's citizenUid matches the path UID: check
request.resource.data.citizenUid == citizenUid, restrict allowed field names
(e.g., only ["citizenUid","erasure_active","createdAt", ...] as appropriate)
using request.resource.data.keys().hasOnly(...), and validate types/format for
erasure_active (e.g., boolean or timestamp) and any required fields via
request.resource.data.field is <type> checks so malicious or malformed create
payloads are rejected.
- Around line 499-501: The create rule for erasure_requests currently only
checks isAuthed(), request.resource.data.citizenUid and status ==
'pending_review', so tighten it by explicitly validating the full request shape:
require the exact set of keys (e.g., citizenUid, status, createdAt, reason or
whatever fields your model mandates) using request.resource.data.keys() and
ensure the keys.size() equals the expected number, validate each field's type
and content (e.g., request.resource.data.citizenUid == request.auth.uid, status
== 'pending_review', createdAt is a timestamp, reason is a string of acceptable
length), and reject any extra fields by asserting keys().size() matches the
required list; reference the isAuthed() helper and the existing
request.resource.data.citizenUid/status checks when adding these validations in
the erasure_requests create rule.
In `@infra/firebase/storage.rules`:
- Around line 22-26: The current rule grants any authenticated citizen broad
access because of the unscoped isCitizen() fallback in the
/report_media/{municipalityId}/{reportId}/{filename} match; remove that unscoped
isCitizen() clause and replace it with an ownership-scoped check (for example,
require isCitizen() && resource.data.ownerUid == request.auth.uid or call a
helper like isReportOwner(municipalityId, reportId) that verifies the requesting
uid matches the report’s owner). Ensure the new predicate references the
existing path variables (municipalityId, reportId, filename) or resource
metadata so citizens can only read media they own, leaving the isMuniAdmin(),
myMunicipality(), isSuperadmin() && municipalityId in permittedMunis() checks
intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b15c8e43-dfba-4aaf-ad5e-a53dd0b15353
📒 Files selected for processing (22)
apps/citizen-pwa/src/__tests__/erasure.test.tsapps/citizen-pwa/src/components/DeleteAccountFlow.test.tsxapps/citizen-pwa/src/components/DeleteAccountFlow.tsxapps/citizen-pwa/src/components/GoodbyeScreen.tsxapps/citizen-pwa/src/routes.tsxapps/citizen-pwa/src/services/erasure.tsdocs/learnings.mddocs/progress.mdfunctions/src/__tests__/callables/approve-erasure-request.test.tsfunctions/src/__tests__/callables/request-data-erasure.test.tsfunctions/src/__tests__/callables/set-erasure-legal-hold.test.tsfunctions/src/__tests__/rules/erasure-requests.rules.test.tsfunctions/src/__tests__/triggers/erasure-sweep.test.tsfunctions/src/__tests__/triggers/retention-sweep.test.tsfunctions/src/callables/approve-erasure-request.tsfunctions/src/callables/request-data-erasure.tsfunctions/src/callables/set-erasure-legal-hold.tsfunctions/src/index.tsfunctions/src/triggers/erasure-sweep.tsfunctions/src/triggers/retention-sweep.tsinfra/firebase/firestore.rulesinfra/firebase/storage.rules
| } catch { | ||
| setError('Something went wrong. Your account has not been deleted. Please try again.') | ||
| setStep('confirm') | ||
| } |
There was a problem hiding this comment.
Handle idempotent erasure conflicts separately from generic failures.
All failures are collapsed to one generic error, so an already-exists erasure request is treated as hard failure instead of a recoverable/expected state.
Suggested fix
- } catch {
- setError('Something went wrong. Your account has not been deleted. Please try again.')
- setStep('confirm')
+ } catch (err: unknown) {
+ const code =
+ typeof err === 'object' && err !== null && 'code' in err
+ ? String((err as { code?: unknown }).code)
+ : ''
+ if (code === 'already-exists') {
+ onGoodbye()
+ return
+ }
+ setError('Something went wrong. Your account has not been deleted. Please try again.')
+ setStep('confirm')
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch { | |
| setError('Something went wrong. Your account has not been deleted. Please try again.') | |
| setStep('confirm') | |
| } | |
| } catch (err: unknown) { | |
| const code = | |
| typeof err === 'object' && err !== null && 'code' in err | |
| ? String((err as { code?: unknown }).code) | |
| : '' | |
| if (code === 'already-exists') { | |
| onGoodbye() | |
| return | |
| } | |
| setError('Something went wrong. Your account has not been deleted. Please try again.') | |
| setStep('confirm') | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/citizen-pwa/src/components/DeleteAccountFlow.tsx` around lines 22 - 25,
The catch block in DeleteAccountFlow.tsx currently swallows all errors and sets
a generic failure via setError and setStep('confirm'); change the catch to
capture the error object (catch (err)) and branch on the erasure-conflict
sentinel (e.g. err.code === 'already-exists' or
err.message.includes('already-exists')) so that when an idempotent
"already-exists" erasure response occurs you treat it as a recoverable/expected
state (advance the flow or show a success/info message) instead of marking it as
a hard failure; keep the existing generic setError/setStep('confirm') behavior
for other errors.
| const toDelete = await input.db | ||
| .collection('reports') | ||
| .where('retentionHardDeleteEligibleAt', '<', now()) | ||
| .get() | ||
|
|
||
| for (const doc of toDelete.docs) { | ||
| try { | ||
| await input.db.collection('report_private').doc(doc.id).delete() | ||
| await input.db.collection('report_contacts').doc(doc.id).delete() | ||
| await doc.ref.delete() | ||
|
|
||
| // Write audit log outside the deleted document | ||
| await input.db.collection('retention_audit_log').add({ | ||
| reportId: doc.id, | ||
| retentionDeletedAt: now(), | ||
| reason: 'retention_policy', | ||
| }) | ||
| result.hardDeleted++ | ||
| } catch (err: unknown) { | ||
| log({ | ||
| severity: 'ERROR', | ||
| code: 'RETENTION_DELETE_FAILED', | ||
| message: `retention delete failed for ${doc.id}: ${String(err)}`, | ||
| data: { reportId: doc.id, error: String(err) }, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Hard-delete path should also skip citizens with active erasure requests.
Right now only anonymization respects activeErasureUids; hard-delete can still run for actively erasing users and conflict with the erasure sweep.
Proposed fix
for (const doc of toDelete.docs) {
+ const submittedBy = doc.data().submittedBy as string | undefined
+ if (submittedBy && activeErasureUids.has(submittedBy)) continue
try {
await input.db.collection('report_private').doc(doc.id).delete()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@functions/src/triggers/retention-sweep.ts` around lines 120 - 146, The
hard-delete loop (iterating over toDelete from the retentionHardDeleteEligibleAt
query) must skip reports belonging to citizens with active erasure requests:
before deleting, derive the report owner UID (use the same field your
anonymization code uses, e.g. doc.get('citizenUid') ?? doc.get('ownerUid')) and
if activeErasureUids.has(uid) return/continue without deleting; keep the rest of
the logic (deleting report_private, report_contacts, doc.ref.delete, and writing
retention_audit_log) unchanged and log/skipping counts accordingly. Ensure you
reference activeErasureUids, toDelete, and the retentionHardDeleteEligibleAt
query when implementing the check.
| allow create: if isAuthed() && request.auth.uid == citizenUid; | ||
| allow read: if (isAuthed() && request.auth.uid == citizenUid) |
There was a problem hiding this comment.
Constrain erasure_active create payload to the path UID and expected schema.
allow create checks auth/path ownership only; it does not validate document structure or citizenUid consistency.
Suggested fix
- allow create: if isAuthed() && request.auth.uid == citizenUid;
+ allow create: if isAuthed()
+ && request.auth.uid == citizenUid
+ && request.resource.data.keys().hasOnly(['citizenUid', 'createdAt'])
+ && request.resource.data.citizenUid == citizenUid
+ && request.resource.data.createdAt is number;As per coding guidelines: "Validate external input at boundaries and never swallow errors with empty catch blocks; assume external input is malicious or broken".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| allow create: if isAuthed() && request.auth.uid == citizenUid; | |
| allow read: if (isAuthed() && request.auth.uid == citizenUid) | |
| allow create: if isAuthed() | |
| && request.auth.uid == citizenUid | |
| && request.resource.data.keys().hasOnly(['citizenUid', 'createdAt']) | |
| && request.resource.data.citizenUid == citizenUid | |
| && request.resource.data.createdAt is number; | |
| allow read: if (isAuthed() && request.auth.uid == citizenUid) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/firebase/firestore.rules` around lines 514 - 515, The create rule
currently only checks ownership; update the allow create rule for this
collection to validate the incoming document schema and that the payload's
citizenUid matches the path UID: check request.resource.data.citizenUid ==
citizenUid, restrict allowed field names (e.g., only
["citizenUid","erasure_active","createdAt", ...] as appropriate) using
request.resource.data.keys().hasOnly(...), and validate types/format for
erasure_active (e.g., boolean or timestamp) and any required fields via
request.resource.data.field is <type> checks so malicious or malformed create
payloads are rejected.
| match /report_media/{municipalityId}/{reportId}/{filename} { | ||
| allow read: if (isMuniAdmin() && myMunicipality() == municipalityId) | ||
| || (isSuperadmin() && municipalityId in permittedMunis()); | ||
| || (isSuperadmin() && municipalityId in permittedMunis()) | ||
| || isCitizen(); // citizens read their own report media | ||
| allow write: if false; |
There was a problem hiding this comment.
Remove the unscoped citizen read fallback.
isCitizen() here grants every authenticated citizen read access to every report_media object. That is a cross-account privacy leak unless you have an ownership predicate tied to the object path or metadata.
🔒 Suggested fix
match /report_media/{municipalityId}/{reportId}/{filename} {
allow read: if (isMuniAdmin() && myMunicipality() == municipalityId)
|| (isSuperadmin() && municipalityId in permittedMunis())
- || isCitizen(); // citizens read their own report media
allow write: if false;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@infra/firebase/storage.rules` around lines 22 - 26, The current rule grants
any authenticated citizen broad access because of the unscoped isCitizen()
fallback in the /report_media/{municipalityId}/{reportId}/{filename} match;
remove that unscoped isCitizen() clause and replace it with an ownership-scoped
check (for example, require isCitizen() && resource.data.ownerUid ==
request.auth.uid or call a helper like isReportOwner(municipalityId, reportId)
that verifies the requesting uid matches the report’s owner). Ensure the new
predicate references the existing path variables (municipalityId, reportId,
filename) or resource metadata so citizens can only read media they own, leaving
the isMuniAdmin(), myMunicipality(), isSuperadmin() && municipalityId in
permittedMunis() checks intact.
- PWA erasure.ts: treat already-exists as success, always sign out - DeleteAccountFlow: reset typed/error on dialog dismiss, add already-exists test - Test mock hygiene: vi.hoisted + mockReset in beforeEach across test files - approve-erasure-request: log rollback failures instead of swallowing - request-data-erasure: inspect allSettled results and log rollback failures - set-erasure-legal-hold: wrap read-update in runTransaction for atomicity - retention-sweep: skip hard-delete for reports with active erasure UIDs - firestore.rules: add field allowlists + type checks for erasure_requests/erasure_active create - storage.rules: add architectural note on unscoped citizen read - approve-erasure-request.test: replace placeholder with real rollback test
Summary
Implements the full RA 10173 (Data Privacy Act) erasure execution loop: citizen deletion request submission, anonymization sweep, retention sweep, legal hold, and Firestore/Storage rules.
Changes
Backend — 4 new Cloud Functions + 1 fix
requestDataErasuresetErasureLegalHolderasureSweepretentionSweepapproveErasureRequestpending_reviewRules
erasure_requestsanderasure_activesentinel; superadmins have full read; service accounts handle transitions.report_media(architectural limitation: cannot scope to "own" media without Firestore cross-read).Frontend — Citizen PWA
DeleteAccountFlowtwo-step confirmation modal withDELETEtyping gateGoodbyeScreenstatic post-submission page at/goodbyeProfileTabwired with privacy section containing delete-account entry pointTests
Known Limitations / Open Items
sms_inbox/sms_sessionsdata. Requires UID-linkage at registration time before SMS onboarding can go to production.Rollback
Verification
pnpm exec tsc --noEmitpasses infunctionsandapps/citizen-pwafirebase emulators:exec)Summary by Sourcery
Implement RA 10173-compliant citizen data erasure and retention flows across backend, security rules, and the citizen PWA.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Release Notes
New Features
Security
Documentation
Tests