Skip to content

Conversation

@SaintPatrck
Copy link
Contributor

@SaintPatrck SaintPatrck commented Jan 20, 2026

🎟️ Tracking

PM-30899

📔 Objective

Update the createAccountKeys API call to handle and store the full response object, which includes the accountKeys.

When a new SSO user is created, the accountKeys received from the createAccountKeys endpoint are now stored in authDiskSource.

Additionally, during a soft logout, the user's accountKeys are preserved and restored along with other account data upon re-authentication.

This fixes an issue where an SSO user was forced to re-authenticate after app process death, even when the device is trusted.

Specific changes include:

  • Modified AccountsService and related API definitions to expect and return a CreateAccountKeysResponseJson object instead of Unit.
  • Introduced the CreateAccountKeysResponseJson data class to model the API response.
  • Updated AuthRepositoryImpl to store the accountKeys from the response after creating a new SSO user.
  • Updated UserLogoutManagerImpl to ensure accountKeys are retained during a soft logout.
  • Adjusted associated tests to reflect these changes, including mocking the new response structure and verifying that accountKeys are stored correctly.

⏰ Reminders before review

  • Contributor guidelines followed
  • All formatters and local linters executed and passed
  • Written new unit and / or integration tests where applicable
  • Protected functional changes with optionality (feature flags)
  • Used internationalization (i18n) for all UI strings
  • CI builds passed
  • Communicated to DevOps any deployment requirements
  • Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

  • 👍 (:+1:) or similar for great changes
  • 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
  • ❓ (:question:) for questions
  • 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
  • 🎨 (:art:) for suggestions / improvements
  • ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
  • 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
  • ⛏ (:pick:) for minor or nitpick changes

@github-actions github-actions bot added app:password-manager Bitwarden Password Manager app context app:authenticator Bitwarden Authenticator app context t:misc Change Type - ¯\_(ツ)_/¯ labels Jan 20, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2026

Logo
Checkmarx One – Scan Summary & Detailsccffa67f-f632-47c0-9f88-9af99e7b3996

Great job! No new security vulnerabilities introduced in this pull request

@SaintPatrck SaintPatrck force-pushed the PM-30899/tde-trust-device-fix branch from dfa18a7 to 04f6a64 Compare January 20, 2026 19:21
@codecov
Copy link

codecov bot commented Jan 20, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.56%. Comparing base (afc1ff4) to head (ae1ca95).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #6384   +/-   ##
=======================================
  Coverage   85.56%   85.56%           
=======================================
  Files         764      764           
  Lines       55501    55506    +5     
  Branches     8047     8047           
=======================================
+ Hits        47488    47493    +5     
  Misses       5239     5239           
  Partials     2774     2774           

☔ View full report in Codecov by Sentry.
📢 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.

@SaintPatrck SaintPatrck force-pushed the PM-30899/tde-trust-device-fix branch 2 times, most recently from 7098446 to 17b844d Compare January 21, 2026 15:23
@SaintPatrck SaintPatrck marked this pull request as ready for review January 21, 2026 15:23
@SaintPatrck SaintPatrck requested review from a team and david-livefront as code owners January 21, 2026 15:23
@SaintPatrck SaintPatrck added the ai-review Request a Claude code review label Jan 21, 2026
@claude
Copy link
Contributor

claude bot commented Jan 21, 2026

Claude finished @SaintPatrck's task in 1m 39s —— View job


Code Review Summary

PR: PM-30899 Store account keys upon SSO user creation

Overview

This PR fixes an issue where SSO users were forced to re-authenticate after app process death even when the device is trusted. The fix ensures accountKeys are stored when a new SSO user is created and preserved during soft logout.

Changes Reviewed

File Change
AuthenticatedAccountsApi.kt Updated createAccountKeys return type to CreateAccountKeysResponseJson
CreateAccountKeysResponseJson.kt New response model for account keys API
AccountsService.kt / AccountsServiceImpl.kt Updated service interface and implementation
AuthRepositoryImpl.kt Store accountKeys after successful SSO user creation
UserLogoutManagerImpl.kt Preserve and restore accountKeys during soft logout
Test files Updated to reflect new response structure and verify storage

Analysis

Security - The changes correctly handle sensitive cryptographic material following established patterns in the codebase. Account keys are stored using the same mechanisms as other sensitive data.

Correctness - The implementation is sound:

  • The .onSuccess { } callback properly stores account keys before mapping the result
  • Soft logout preserves accountKeys following the identical pattern used for PIN data and vault timeout settings
  • Nullable accountKeys is correctly handled since storeAccountKeys accepts null values

Test Coverage - Comprehensive:

  • Codecov reports all modified lines are covered
  • Tests verify account keys storage in success scenarios
  • Soft logout tests verify preservation and restoration

Code Quality

  • Appropriate @Suppress("LongMethod") annotation added
  • Clear KDoc documentation on the new response model
  • Follows established patterns in the codebase

Verdict

APPROVED - Clean implementation with no issues found.


Reviewed with Claude Code

authDiskSource
.storeAccountKeys(
userId = userId,
accountKeys = response.accountKeys,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this be done in the onSuccess down below?

If organizationResetPasswordEnroll fails, we will have persisted these keys without completing the full flow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is also this comment down below:

// TDE and SSO user creation still uses crypto-v1. These users are not
// expected to have the AEAD keys so we only store the private key for now.
// See #5682 (comment)
// for more details.

Do we know if this is still true?

Copy link
Contributor Author

@SaintPatrck SaintPatrck Jan 21, 2026

Choose a reason for hiding this comment

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

Should this be done in the onSuccess down below?

Good catch. Yes.

@quexten what's the status of TDE & SSO user creation? Is it safe to remove the crypto-v1 usages yet?

This commit updates the `createAccountKeys` API call to handle and store the full response object, which includes the `accountKeys`.

When a new SSO user is created, the `accountKeys` received from the `createAccountKeys` endpoint are now stored in `authDiskSource`.

Additionally, during a soft logout, the user's `accountKeys` are preserved and restored along with other account data upon re-authentication.

Specific changes include:
- Modified `AccountsService` and related API definitions to expect and return a `CreateAccountKeysResponseJson` object instead of `Unit`.
- Introduced the `CreateAccountKeysResponseJson` data class to model the API response.
- Updated `AuthRepositoryImpl` to store the `accountKeys` from the response after creating a new SSO user.
- Updated `UserLogoutManagerImpl` to ensure `accountKeys` are retained during a soft logout.
- Adjusted associated tests to reflect these changes, including mocking the new response structure and verifying that `accountKeys` are stored correctly.
@SaintPatrck SaintPatrck force-pushed the PM-30899/tde-trust-device-fix branch from 17b844d to ae1ca95 Compare January 21, 2026 16:06
@github-actions github-actions bot removed the ai-review Request a Claude code review label Jan 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

app:authenticator Bitwarden Authenticator app context app:password-manager Bitwarden Password Manager app context t:misc Change Type - ¯\_(ツ)_/¯

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants