PM-37879: feat: Add SDK state bridge - #7204
Conversation
david-livefront
added
the
ai-review-vnext
github-actions
Bot
added
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the V2 encryption groundwork: the new Code Review Details
Previously reported, no longer applicable at
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7204 +/- ##
==========================================
+ Coverage 86.10% 86.20% +0.09%
==========================================
Files 932 890 -42
Lines 66448 64873 -1575
Branches 9741 9712 -29
==========================================
- Hits 57218 55924 -1294
+ Misses 5747 5474 -273
+ Partials 3483 3475 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
4ee8300 to
b4ef4d7
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
c28fd0c to
a18aa60
Compare
| } | ||
|
|
||
| override suspend fun setEphemeralPinEnvelope(value: PasswordProtectedKeyEnvelope) { | ||
| authDiskSource.storePinProtectedUserKeyEnvelope( |
There was a problem hiding this comment.
@david-livefront Both ephemeral and persistent must be present at the same time / that is the expectation from the SDK side. How does this work in auth disk source? It seems the key here is the same? 🤔
There was a problem hiding this comment.
That is not how we treat the pin envelope on Android. We always store it in memory and optionally store it to disk based on the inMemoryOnly flag.
override fun storePinProtectedUserKeyEnvelope(
userId: String,
pinProtectedUserKeyEnvelope: String?,
inMemoryOnly: Boolean,
) {
inMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope
if (inMemoryOnly) {
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope)
return
}
putString(
key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(userId),
value = pinProtectedUserKeyEnvelope,
)
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope)
}and then we fetch whatever is available when trying to retrieve the data:
override fun getPinProtectedUserKeyEnvelope(userId: String): String? =
inMemoryPinProtectedUserKeyEnvelopes[userId]
?: getString(key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(userId))
| } | ||
|
|
||
| override suspend fun setMasterpasswordUnlockData(value: MasterPasswordUnlockData) { | ||
| authDiskSource.userState = authDiskSource.userState?.updateMasterPasswordUnlock( |
There was a problem hiding this comment.
If we have a updateMasterPasswordUnlock function here, why not also make a getMasterPasswordUnlock, instead of having the log . chain in the get below?
There was a problem hiding this comment.
Setting the data is a more complicated process that we want abstracted for reusability, fetching the data is not particularly complicated, so we never bothered to abstract it.
For whatever it's worth, I am working on migrating the master password unlock data out of the userState. Once that work is done, they with both be simple get and set functions on the authDiskSource.
a18aa60 to
75e171a
Compare
| override fun getInMemoryPinProtectedUserKeyEnvelope( | ||
| userId: String, | ||
| ): String? = storedPinProtectedUserKeyEnvelopes[userId] | ||
| ?.let { (pinProtectedUserKeyEnvelope, inMemoryOnly) -> | ||
| pinProtectedUserKeyEnvelope.takeIf { inMemoryOnly } | ||
| } | ||
|
|
||
| override fun getPersistentPinProtectedUserKeyEnvelope( | ||
| userId: String, | ||
| ): String? = storedPinProtectedUserKeyEnvelopes[userId] | ||
| ?.let { (pinProtectedUserKeyEnvelope, inMemoryOnly) -> | ||
| pinProtectedUserKeyEnvelope.takeUnless { inMemoryOnly } | ||
| } | ||
|
|
There was a problem hiding this comment.
♻️ DEBT: The fake treats the in-memory and persistent envelopes as mutually exclusive, but the real disk source stores both.
Details and fix
AuthDiskSourceImpl.storePinProtectedUserKeyEnvelope always writes the in-memory map and additionally writes to disk when inMemoryOnly == false:
inMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope
if (inMemoryOnly) { /* emit + */ return }
putString(key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(userId), value = ...)So production and the fake disagree in both directions:
store(v, inMemoryOnly = false)→ realgetInMemoryPinProtectedUserKeyEnvelopereturnsv; the fake returnsnull.store(null, inMemoryOnly = true)→ realgetPersistentPinProtectedUserKeyEnvelopestill returns the previously persisted value; the fake returnsnull.
Since SdkStateBridgeTest validates the new ephemeral/persistent split entirely through this fake, those tests can pass while SdkStateBridge behaves differently at runtime — e.g. clearPersistentPinEnvelope() also drops the ephemeral envelope in production, which the current test does not surface.
Backing the fake with two maps (one always written, one written only when !inMemoryOnly) would keep the double faithful to the real source.
There was a problem hiding this comment.
Claude finished @david-livefront's task in 2m 48s —— View job
Verification ✅
- Check out the latest branch state
- Re-check
storePinProtectedUserKeyEnvelope/clearPinProtectedUserKeyEnvelope - Re-trace the callers that clear the envelope
- Check the remaining open findings on this PR
Confirmed on 183c673 (local HEAD matches origin/PM-37879-sdk-state-bridge, working tree clean). The envelope-clearing findings are addressed:
clearPinProtectedUserKeyEnvelope now clears both stores and emits — AuthDiskSourceImpl.kt:403-410:
override fun clearPinProtectedUserKeyEnvelope(userId: String) {
inMemoryPinProtectedUserKeyEnvelopes.remove(userId)
putString(key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId), value = null)
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null)
}Both clear callers route through it — SettingsRepositoryImpl.clearUnlockPin() (:656) and AuthDiskSourceImpl.clearData() (:184). So the traced path (enable PIN with "require master password on restart" → in-memory-only envelope → disable PIN) now drops the in-memory envelope and emits null, so isUnlockWithPinEnabledFlow settles on false.
The third caller I flagged is a non-issue on closer reading — PinProtectedUserKeyManagerImpl's onFailure still passes inMemoryOnly = false, but both entry points early-return when getPinProtectedUserKeyEnvelope(...) != null (:17, :32), so no stale envelope can exist in either store when that branch runs.
The fake now mirrors the real split — FakeAuthDiskSource backs the two getters with storedInMemoryPinProtectedUserKeyEnvelopes / storedPersistentPinProtectedUserKeyEnvelopes, and AuthDiskSourceTest:946 covers clearing both stores.
Two small leftovers, both non-blocking:
FakeAuthDiskSource.clearPinProtectedUserKeyEnvelope(:381) still has notryEmit(null), so it diverges from the real source again in the other direction. Any fake-backed test ofclearUnlockPin()→isUnlockWithPinEnabledFlowwould still reproduce the old stale-truebehavior that production no longer has. The newAuthDiskSourceTestcase asserts the two stores but not the emission.- The
SdkStateBridge.kt:119-124thread is unchanged —updateMasterPasswordUnlockstill setsforcePasswordResetReason = nullunconditionally (UserStateJsonExtensions.kt:125), so an SDK-driven unlock-data write still drops a pending forced reset. Flagging only so it isn't assumed covered by this fix.
I wasn't able to run ./gradlew app:testStandardDebugUnitTest or git fetch in this environment (both need approval in --allowedTools), so the above is a source-level verification rather than a test run.
• PM-37879-sdk-state-bridge
75e171a to
cae440b
Compare
| override suspend fun setMasterpasswordUnlockData(value: MasterPasswordUnlockData) { | ||
| authDiskSource.userState = authDiskSource.userState?.updateMasterPasswordUnlock( | ||
| userId = userId, | ||
| masterPasswordUnlock = value, | ||
| ) | ||
| } |
There was a problem hiding this comment.
updateMasterPasswordUnlock also clears forcePasswordResetReason, so an SDK-driven unlock-data write silently drops a pending forced password reset.
Details and fix
UserStateJson.updateMasterPasswordUnlock unconditionally sets forcePasswordResetReason = null (UserStateJsonExtensions.kt:125). That is intended for its existing callers in AuthRepositoryImpl (set/change/remove password), but through this bridge the SDK can write unlock data while a reset is still pending — for example when the V2 upgrade re-wraps the user key during unlock for a user carrying ADMIN_FORCE_PASSWORD_RESET or WEAK_MASTER_PASSWORD_ON_LOGIN.
Nothing restores the flag afterwards: on sync, getForcePasswordResetReason only carries the previous value forward (?: previousForcePasswordResetReason), so needsPasswordReset stays false until the next login and the user is never prompted to reset.
Consider a helper that updates only masterPasswordUnlock/hasMasterPassword and leaves forcePasswordResetReason untouched for the bridge path.
Related: clearMasterpasswordUnlockData flips hasMasterPassword to false. Worth confirming the SDK only calls it when the master password is genuinely being removed, since the app uses that flag to route users into the set-password and TDE flows.
cae440b to
417d37e
Compare
| if (inMemoryOnly) { | ||
| getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope) | ||
| return | ||
| inMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope | ||
| } else { | ||
| putString( | ||
| key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId), | ||
| value = pinProtectedUserKeyEnvelope, | ||
| ) | ||
| } |
There was a problem hiding this comment.
❌ CRITICAL: With the stores now split, no app-side code path clears the in-memory envelope, so a disabled PIN stays usable for the rest of the session.
Details and fix
Previously this method always wrote inMemoryPinProtectedUserKeyEnvelopes[userId] and then additionally wrote to disk when inMemoryOnly was false. Now it writes to exactly one store, but every existing caller that clears the envelope passes the default inMemoryOnly = false:
SettingsRepositoryImpl.clearUnlockPin()AuthDiskSourceImpl.clearData()(logout)PinProtectedUserKeyManagerImpl.enrollPinWithEncryptedPin()onFailure
Traceable path:
- User enables PIN unlock with "Require master password on app restart" →
storeUnlockPin(..., shouldRequireMasterPasswordOnRestart = true)→inMemoryOnly = true, so the envelope lives only ininMemoryPinProtectedUserKeyEnvelopes. - User turns PIN unlock off →
clearUnlockPin()→storePinProtectedUserKeyEnvelope(userId, null)withinMemoryOnly = false→ only the (already empty) disk key is cleared; the in-memory envelope survives. getPinProtectedUserKeyEnvelope()still returns the stale envelope (in-memory takes precedence), soUserStateManagerImpl.getVaultUnlockType()reportsPINandVaultRepositoryImpl.unlockVaultWithPin()unlocks the vault with the PIN the user just disabled.
A grep confirms inMemoryPinProtectedUserKeyEnvelopes is now only ever assigned from the inMemoryOnly = true branch — nothing else clears it.
Secondary effect: the flow at the end still emits the single written value, so getPinProtectedUserKeyEnvelopeFlow and getPinProtectedUserKeyEnvelope can now disagree (e.g. the SDK calling clearEphemeralPinEnvelope() emits null while a persistent envelope is still returned by the getter).
Suggested fix — clear both stores when the value is null, since a null write is always a "clear" intent:
override fun storePinProtectedUserKeyEnvelope(
userId: String,
pinProtectedUserKeyEnvelope: String?,
inMemoryOnly: Boolean,
) {
if (pinProtectedUserKeyEnvelope == null) {
inMemoryPinProtectedUserKeyEnvelopes.remove(userId)
putString(
key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId),
value = null,
)
} else if (inMemoryOnly) {
inMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope
} else {
putString(
key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId),
value = pinProtectedUserKeyEnvelope,
)
}
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope)
}Note that the existing clearData test passes only because it stores the envelope with inMemoryOnly = false, so the in-memory path is never exercised.
There was a problem hiding this comment.
Claude finished @david-livefront's task in 1m 2s —— View job
Verification ✅
Confirmed — FakeAuthDiskSource now models the ephemeral/persistent split faithfully. Checked at 183c673 (local matches origin/PM-37879-sdk-state-bridge, working tree clean).
The fake now backs the envelope with two independent maps and routes writes exactly like the real source:
// FakeAuthDiskSource.kt:368
override fun storePinProtectedUserKeyEnvelope(
userId: String,
pinProtectedUserKeyEnvelope: String?,
inMemoryOnly: Boolean,
) {
if (inMemoryOnly) {
storedInMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope
} else {
storedPersistentPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope
}
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope)
}Side-by-side against AuthDiskSourceImpl (lines 355–410):
| Behavior | Real | Fake |
|---|---|---|
store(v, inMemoryOnly = true) |
in-memory only | in-memory only ✅ |
store(v, inMemoryOnly = false) |
persistent only | persistent only ✅ |
getInMemory… / getPersistent… |
reads its own store | reads its own store ✅ |
get… precedence |
inMemory ?: persistent |
inMemory ?: persistent ✅ |
clear… |
removes both | removes both ✅ |
clearData / logout |
clears both | clears both ✅ |
My original comment described the fake as diverging from a real source that wrote both stores; the implementation has since moved to the one-store-per-call form, and the fake matches it in both directions. The two mismatches I called out (store(v, inMemoryOnly = false) losing the in-memory value, store(null, inMemoryOnly = true) not clearing the persistent one) no longer apply — production and the double agree.
I also confirmed the earlier clearPinProtectedUserKeyEnvelope finding landed: AuthDiskSourceImpl.kt:409 now ends with getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null), so isUnlockWithPinEnabledFlow sees the clear.
One small leftover, non-blocking: the fake's clearPinProtectedUserKeyEnvelope (line 381) removes both maps but does not tryEmit(null), while the real one now does. Any future test that drives a flow through clearUnlockPin() won't observe the emission the production code performs. Adding the tryEmit(null) to the fake would close that last gap. Fix this →
Thanks for the fix! 🎉
--- • Branch: PM-37879-sdk-state-bridge
417d37e to
160b93f
Compare
| override fun clearPinProtectedUserKeyEnvelope(userId: String) { | ||
| inMemoryPinProtectedUserKeyEnvelopes.remove(userId) | ||
| putString( | ||
| key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(userId), | ||
| value = pinProtectedUserKeyEnvelope, | ||
| key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId), | ||
| value = null, | ||
| ) | ||
| getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope) | ||
| } |
There was a problem hiding this comment.
clearPinProtectedUserKeyEnvelope never emits on the envelope flow, so isUnlockWithPinEnabledFlow keeps reporting true after the user disables PIN unlock.
Details and fix
SettingsRepositoryImpl.clearUnlockPin() previously called storePinProtectedUserKeyEnvelope(userId, null), which ended with getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null). It now calls clearPinProtectedUserKeyEnvelope, which has no tryEmit.
Trace on the Account Security screen:
AccountSecurityViewModelcollectsisUnlockWithPinEnabledFlow, which isgetPinProtectedUserKeyFlow(...).combine(getPinProtectedUserKeyEnvelopeFlow(...)). The envelope flow (replay = 1) holds the non-null envelope, so the toggle renders ON.- User turns the toggle off →
clearUnlockPin()runs:storeEncryptedPin(null)emits nothing,clearPinProtectedUserKeyEnvelope(...)emits nothing, thenstorePinProtectedUserKey(null)does emit, re-runningcombineagainst the stale non-null envelope value →true. handlePinProtectedLockUpdatewrites that back into state, soisUnlockWithPinEnabledflips back totrueeven though the PIN was just cleared.
Suggested fix:
override fun clearPinProtectedUserKeyEnvelope(userId: String) {
inMemoryPinProtectedUserKeyEnvelopes.remove(userId)
putString(
key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId),
value = null,
)
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null)
}Related: now that the two stores are independent, storePinProtectedUserKeyEnvelope emitting its raw argument (line 400) can also disagree with getPinProtectedUserKeyEnvelope. UserLogoutManagerImpl soft logout restores the in-memory envelope and then writes a null persistent envelope, so the last emission is null while the getter returns the in-memory value. Emitting getPinProtectedUserKeyEnvelope(userId) instead would keep the flow and getter in sync in both cases.
FakeAuthDiskSource.clearPinProtectedUserKeyEnvelope mirrors the missing emission, so no test covers this.
There was a problem hiding this comment.
Claude finished @david-livefront's task in 1m 28s —— View job
Verification ✅
Confirmed — the fake and the real disk source now agree on the ephemeral/persistent split.
FakeAuthDiskSource now backs the envelope with two independent maps, and the real source was changed in the same direction (writing exactly one store per call), so both directions of my original finding are gone:
| Call | AuthDiskSourceImpl |
FakeAuthDiskSource |
|---|---|---|
store(v, inMemoryOnly = false) → getInMemory… |
null (disk only, :395) |
null (:376) |
store(null, inMemoryOnly = true) → getPersistent… |
prior persisted value (untouched, :393) |
prior value in persistent map (:374) |
getPinProtectedUserKeyEnvelope |
inMemory ?: disk (:355) |
inMemory ?: persistent (:356) |
clearData → clearPinProtectedUserKeyEnvelope (AuthDiskSourceImpl.kt:184) is mirrored by the fake clearing both maps, so the logout path is faithful too. Thanks for tightening this up. 🎉
One leftover divergence (minor, not blocking)
FakeAuthDiskSource.clearPinProtectedUserKeyEnvelope (FakeAuthDiskSource.kt:381) doesn't emit, while the real one now does (AuthDiskSourceImpl.kt:409):
override fun clearPinProtectedUserKeyEnvelope(userId: String) {
storedInMemoryPinProtectedUserKeyEnvelopes.remove(userId)
storedPersistentPinProtectedUserKeyEnvelopes.remove(userId)
// missing: getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null)
}This direction is safe (the fake under-emits rather than over-emits), but it means the fake can't reproduce the clearUnlockPin() → isUnlockWithPinEnabledFlow behavior. SettingsRepositoryTest.kt:1365 only drives the deprecated storePinProtectedUserKey, so nothing currently covers the envelope flow after a clear.
Verified against 183c673 (the PR head checked out for this run; git fetch/gh were not permitted in this environment, so I couldn't re-confirm against the remote ref).
· Branch
160b93f to
183c673
Compare
| if (inMemoryOnly) { | ||
| getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope) | ||
| return | ||
| inMemoryPinProtectedUserKeyEnvelopes[userId] = pinProtectedUserKeyEnvelope | ||
| } else { | ||
| putString( | ||
| key = PIN_PROTECTED_USER_KEY_KEY_ENVELOPE.appendIdentifier(identifier = userId), | ||
| value = pinProtectedUserKeyEnvelope, | ||
| ) | ||
| } | ||
| getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(pinProtectedUserKeyEnvelope) |
There was a problem hiding this comment.
getPinProtectedUserKeyEnvelope returns.
Details and fix
getPinProtectedUserKeyEnvelopeFlow seeds subscribers with the effective value (onSubscription { emit(getPinProtectedUserKeyEnvelope(userId)) } = in-memory ?: persistent), but this method emits only the value that was just written to one of the two stores.
Traceable path — UserLogoutManagerImpl.softLogout for a user with PIN + "require master password on restart" (in-memory envelope only, persistent null):
storePinProtectedUserKeyEnvelope(inMemoryOnly = true)→ emits the in-memory envelope.storePinProtectedUserKeyEnvelope(inMemoryOnly = false)withpersistentPinProtectedUserKeyEnvelope == null→ emitsnull.
The last value on the flow is null while getPinProtectedUserKeyEnvelope still returns the envelope, so SettingsRepositoryImpl.isUnlockWithPinEnabledFlow reports false even though PIN unlock is still available. The same contradiction occurs when SdkStateBridge.clearPersistentPinEnvelope() runs while an ephemeral envelope is present.
Emitting the effective value keeps the flow consistent with the getter regardless of which store was written:
getMutablePinProtectedUserKeyEnvelopeFlow(userId)
.tryEmit(getPinProtectedUserKeyEnvelope(userId = userId))
}
| override fun clearPinProtectedUserKeyEnvelope(userId: String) { | ||
| storedInMemoryPinProtectedUserKeyEnvelopes.remove(userId) | ||
| storedPersistentPinProtectedUserKeyEnvelopes.remove(userId) | ||
| } |
There was a problem hiding this comment.
♻️ DEBT: The fake's clearPinProtectedUserKeyEnvelope does not emit on the envelope flow, but AuthDiskSourceImpl does.
Details and fix
AuthDiskSourceImpl.clearPinProtectedUserKeyEnvelope ends with getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null). The fake omits it, so any test covering the new clearPinProtectedUserKeyEnvelope call sites (SettingsRepositoryImpl.clearUnlockPin, AuthDiskSourceImpl.clearData) cannot observe the isUnlockWithPinEnabledFlow transition that production emits.
override fun clearPinProtectedUserKeyEnvelope(userId: String) {
storedInMemoryPinProtectedUserKeyEnvelopes.remove(userId)
storedPersistentPinProtectedUserKeyEnvelopes.remove(userId)
getMutablePinProtectedUserKeyEnvelopeFlow(userId).tryEmit(null)
}2 participants
🎟️ Tracking
PM-37879
📔 Objective
This PR is a first step in supporting V2 encryption.
V2UpgradeTokenis now supported in the SyncResponse and we store that to diskSdkStateBridgeis now in place to support communicating data between the SDK and the main appV2UpgradeTokenis passed into the SDK when unlocking in order to migrate users to V2 encryption