Skip to content

Tech Spec: Backup code and account recovery - #3843

Open
shubham1g5 wants to merge 8 commits into
masterfrom
ccct-2677-tech_spec_backup_code
Open

Tech Spec: Backup code and account recovery#3843
shubham1g5 wants to merge 8 commits into
masterfrom
ccct-2677-tech_spec_backup_code

Conversation

@shubham1g5

Copy link
Copy Markdown
Contributor

Jira
Design Doc

Reviewer Notes:

  • Please review the design doc first
  • Review spec directly (no commits structuring)
  • Can skip the section on Implementation Details (I have tried separating out the architectural changes from implementation details that Claude will need to take this spec forward)
  • Review in Android Studio for better formatting with markdown preview as the doc contains a flow chart.

shubham1g5 and others added 3 commits July 28, 2026 17:09
Covers four work areas: change backup code, backup code recovery via
email OTP, periodic reminders, and email modification flow changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add source tracking to Area 1, missing recovery-from-reminder event
to Area 2, and a full analytics section for Area 4.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Fragment hierarchy and nav graph flow diagrams for both backup code
and phone verification refactors.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The design document defines PersonalID backup-code management across profile, signup, recovery, reminder, and email-change flows. It adds shared backup-code and phone-verification fragment contracts, email OTP handling, activity-scoped profile state, a backup-code update API, reminder scheduling, recovery behavior, verification gates, analytics events, and updated signup messaging.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested reviewers: orangeandgreen, conroy-ricketts, jignesh-dimagi

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description includes issue and design links but omits the required technical summary, safety story, test coverage, and review checklist sections. Add the required template sections, including rationale, safety assessment, automated test coverage, and completed labels and review checklist items.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the technical specification's main focus: backup code management and account recovery.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ccct-2677-tech_spec_backup_code

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In
`@docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md`:
- Line 15: Update the Markdown in the design document by adding the text
language identifier to the ASCII diagram fences at the referenced locations and
removing the extra space in the heading near the later diagram. Preserve the
diagram contents and heading text while resolving MD040 and MD019.
- Around line 175-180: Update PersonalIdProfileActivityViewModel to define
currentCode lifecycle rules: bind it to the active user or session, clear it
immediately after a successful authenticated operation, and also clear it on
cancellation, lockout, logout, and account changes. Ensure stale backup
credentials cannot be reused across users or sessions.
- Around line 165-173: Expand the PersonalIdSendEmailOtpFragment specification
and its send_email_otp contract to define server-enforced OTP expiration,
single-use consumption, resend throttling, verification-attempt limits, and
binding to the account, email, and EmailWorkFlow. Document these controls as
authoritative server behavior, including the failure responses clients must
handle; do not rely on the client-side resend timer.
- Around line 165-173: Update the PersonalIdSendEmailOtpFragment navigation
contract so EmailWorkFlow is passed to PersonalIdEmailVerificationFragment
alongside the existing OTP navigation. Define the destination argument and
ensure the workflow is restored through the established navigation or
shared-state mechanism after recreation, allowing verification to select the
correct completion path.
- Around line 40-62: Update the email-change flow specification around the
phone-verification branch and the backup-code-gated branch to define and retain
pendingEmail with the newly entered address before navigation begins, ensuring
the no-email path can use it during the later email OTP step.
- Around line 83-94: Update the backup-code fragment hierarchy documentation and
related workflow contracts so each workflow/navigation graph has one unambiguous
authentication contract. Separate credential type from credential source for
changes, profile recovery, and session recovery, avoid using ambiguous
ProvidedAuth(userId, currentCode), and make the documented
/users/set_backup_code contract match the PersonalID API’s TokenAuth behavior
for CONFIRM_RECOVERY.
🪄 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: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c0426239-226a-416a-82c0-bfbe1e3e11a6

📥 Commits

Reviewing files that changed from the base of the PR and between e1c8ba8 and b4fdfd6.

📒 Files selected for processing (1)
  • docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md

Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 26.84%. Comparing base (ca0462c) to head (eeeef49).
⚠️ Report is 216 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3843      +/-   ##
============================================
- Coverage     27.29%   26.84%   -0.46%     
+ Complexity     4773     4704      -69     
============================================
  Files           991      988       -3     
  Lines         59019    58856     -163     
  Branches       7025     7016       -9     
============================================
- Hits          16111    15797     -314     
- Misses        40974    41146     +172     
+ Partials       1934     1913      -21     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…design spec

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@shubham1g5
shubham1g5 marked this pull request as ready for review July 31, 2026 11:31
Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated

@conroy-ricketts conroy-ricketts left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clearly separating out the implementation reference for Claude! That made this a lot easier to review and is a good model for specs going forward

Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
Comment on lines +279 to +280
All CommCare launching activities (e.g. `ConnectActivity`, `LoginActivity`, `StandardHomeActivity`) check for due reminders on resume.
If due, show the reminder dialog.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main concern here is that we keep triggering these dialog prompts directly inside the Activity. This could lead to multiple dialogs popping up at the same time—for example, if a user doesn't have an email set, they get prompted to add one, but they might simultaneously trigger a prompt to update their backup code.

We should consider establishing a priority queue or hierarchy for these dialogs so only one is presented to the user at a time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. can you remind me if we have the email pop-up already on launch screens ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's getting triggered from here.

…ackupCodeWorkflow enum

Both email-change paths (has email and no email) now start with Confirm Backup Code.
For the add-email path, Forgot? routes to Phone OTP rather than blocking the user.
CONFIRM_BACKUP_CODE split into three distinct workflow values to make routing explicit.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OrangeAndGreen
OrangeAndGreen previously approved these changes Aug 4, 2026
conroy-ricketts
conroy-ricketts previously approved these changes Aug 4, 2026
Jignesh-dimagi
Jignesh-dimagi previously approved these changes Aug 5, 2026
@shubham1g5 shubham1g5 closed this Aug 5, 2026

@calellowitz calellowitz left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One real concern about the flow, and a few smaller questions

Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated

### API Changes

#### New: `POST /users/set_backup_code`

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing workflows require confirmation of newly set backup codes to ensure the user entered the backup code they intended (and know what they put in). Are we intentionally removing that step?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are not, the UI on this page presents users with 2 input fields for user to verify the backup code before we make the API call.

Comment thread docs/superpowers/specs/2026-07-28-personalid-backup-code-management-design.md Outdated
│ Email OTP entry ◄ Profile (success)
│ │
│ ▼
│ Set new backup code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we allow setting a new backup code before biometric/PIN unlock? That allows them to change the profile info, before they are fully authenticated. Setting a backup code isn't really part of the authentication process in this flow, but is happening part way through. If we moved it to the end, after the success, we would only need to allow logged in users to change the code. Rather than users part way through a recovery and would therefore only need to support one authentication method (and one view that already exists).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh the flow diagram makes it a bit confusing but the Biometric unlock is the pop on the same page that happens before we make the API call. So the order is more like this -

  1. User enters and confirms backup code on "Set New Backup Code Page"
  2. We show the biometric unlock pop on the same page. If failed, the only option user have is to press back from here.
  3. If success, we make the API call to set the backup code

Rather than users part way through a recovery and would therefore only need to support one authentication method (and one view that already exists).

We do need to support setting a new back up code during Account recovery /configuration flow otherwise how will we let users signing on a device but who have forgotten their backup code recover their accounts ?

@calellowitz calellowitz Aug 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how will we let users signing on a device but who have forgotten their backup code recover their accounts

We will set it after they successfully recover their account. If they have recovered their account by other means, they will have a password for basic auth. If they have not finished recovering, it is not safe to allow them to modify the authentication methods for the account.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

revised spec - f554540 and eeeef49

shubham1g5 and others added 2 commits August 18, 2026 18:07
…gn-in

- Account configuration forgot flow: email OTP now signs user in and
  then prompts to set a new backup code, rather than setting backup
  code without authentication
- set_recovery_pin (existing endpoint, ProvidedAuth only) replaces the
  planned new set_backup_code endpoint; TokenAuth removed from SET_NEW_CODE
- Split EmailWorkFlow.BACKUP_CODE_RECOVERY into BACKUP_CODE_RECOVERY_SIGN_IN
  (account config graph) and BACKUP_CODE_RECOVERY_SET_CODE (profile graph)
- Add explicit server team request: account recovery via email OTP endpoint
  and email surfacing in start_configuration vs check_name

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-integration-tests Skip android tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants