refactor: rust session - #97
Merged
Merged
Conversation
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Pins PASSWORD_DOMAIN and the Argon2 profile behind it with a known-answer test, documents the deliberate verify/unlock error asymmetry and with_ark's non-reentrancy, moves KEK derivation in rewrap_for_new_password ahead of the session lock so it no longer stalls other lock holders, adds AAD-mismatch and failed-unlock- retains-ark coverage, tightens the wrong-length-key assertion, and drops a redundant test import. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Replaces BackupCredential::Ark { key: AccountRootKey } with
BackupCredential::Session { session: Arc<ArkSession> } so JsonBackupManager
derives the backup key from ArkSession::with_ark instead of receiving raw ARK
bytes across the FFI boundary. uniffi 0.32 accepted Arc<ArkSession> inside
#[derive(uniffi::Enum)] without issue, so the enum shape landed; the documented
export_with_session/import_with_session fallback was not needed. The generated
Kotlin session field is the concrete ArkSession class, not ArkSessionInterface.
Fixed a type mismatch in the brief's Step 1/2 sample: with_ark on
session.session (CoreArkSession) returns keygo_core::ark_session::ArkSessionError,
not the bindings-level crate::ark_session::ArkSessionError the brief imported -
two distinct types with identical variant names. Import and convert from the
core type instead.
Threaded the temporary arkSession(ark) bridge (Task 5 deletes it) through
ExportBackupUseCase and ImportBackupUseCase. Ignored the seven tests that now
construct a real native ArkSession and need the native library:
- ExportBackupUseCaseTest: "ark json job seals with the session ark",
"ark json job on a locked provisioned device uses the recovered ark"
- ImportBackupUseCaseTest: "ark-sealed json imports with the session ark"
- ImportWizardViewModelTest: "Continue on selected JSON runs import and
surfaces the summary", "terminal import error surfaces as failure",
"seeding an ARK sealed JSON imports without asking anything", "seeding a
different file after backing out of a mapping does not carry over the old
file's state"
Also fixed FakeJsonBackupManager (rust testFixtures), which still referenced
the deleted Ark variant and blocked feature:backup:test from compiling.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Renames the old Session interface to LegacySession so the new class can take the name. Call sites move over in the next commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Sync isActive inside the dispatched block, in a finally. The JNI calls behind createAccount and unlockWithPassword are blocking and run to completion whether or not the caller is cancelled, so a sync placed after withContext never ran on cancellation: Rust took custody of the ARK while the flow still read false, the lock screen showed, and nothing called endSession. Stop casting every throwable to ArkSessionException. InternalException and the destroyed-handle IllegalStateException are both reachable, and a ClassCastException raised inside fold's onFailure escaped the Result contract and skipped the isActive sync. Only ArkSessionException is an expected failure now; anything else is a bug and propagates with its own type. Read KeyWrapException's fields instead of its generated message, which prefixes the Other variant with "v1=". Other is the catch-all arm of From<CryptoError>, so that prefix could reach a real SessionError.KeyWrap payload. Make FakeArkSession unwrap across instances. wrapRecord was instance-scoped and unwrap was a memo lookup rather than a decryption, so a blob wrapped by one fake never unwrapped in another even under an identical ARK. That is exactly the backup shape, where BackupArkUnlocker recovers the escrowed ARK into a throwaway session and unwraps vault keys the app session wrapped. XOR is its own inverse, so unwrap re-derives the stream and a short appended tag still rejects a wrong outer key or id. This also stops unwrap ignoring the nonce. The wrong-password test now asserts the error value, which showed it expected WrongPassword where both Rust and the fake return the unwrap failure itself. unlock_with_password propagates KeyWrap and only verify_password collapses to WrongPassword; that asymmetry is shipped behaviour, so the test and SessionError's KDoc now pin it rather than contradict it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
The re-review of the Task 4 fixes approved them but noted that the two behaviours the fixes exist for had no coverage. describe() only had one branch exercised, and not the one that mattered. UnwrapFailed's generated message is empty, so asserting it proved nothing about the "v1=" prefix, which only Other carries. A new test drives all five KeyWrapException variants through a session that throws them, and pins Other's payload as bare text. It fails against the old v1.message.orEmpty() mapping. Nothing wrapped a key in one session and unwrapped it in another, which is the entire reason the fake stopped memoising. A new test does that in the shape backup uses: wrap, export the ARK, unlock a second session with it, unwrap, and refuse the wrong vault id. It fails against the old instance-scoped wrapRecord. Also from the re-review: isActive() can throw on a destroyed handle, so syncing it from a finally could replace a good return or discard an in-flight exception. BackupCredential is Disposable and the next task hands a session into it, so this stops being unreachable soon. syncIsActive() now swallows the throw and keeps the last known value, and all four sync sites go through it. The fake rejects a short nonce as UnwrapFailed rather than letting IndexOutOfBoundsException escape the Result contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
The ARK now lives in Rust for the whole session. Change password requires an active session, which the screen already guarantees. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
The ARK crosses into the JVM at exactly two doors, exportArk and unlockWithArk, because the Keystore ciphers that seal the biometric copy and the backup escrow only run on this side of the FFI. Those doors are the one key-residency guarantee this refactor did not hand to Rust, and nothing tested them: FakeArkSession copies on both, so a caller's finally could stop wiping and every test would stay green. RecordingArkSession, now a shared fixture in :rust, delegates to a FakeArkSession but hands out the arrays themselves. All three exportArk callers are covered on the success path and on a failure after the export. BiometricEnrollmentAdapterImpl had no test class at all and now has one. BackupArkUnlockerTest drops its private copy of the recorder for the shared one. CreateAccessUseCase left the session unlocked when account or vault persistence failed, holding an ARK with nothing persisted to unwrap. It now ends the session on every failure. The vault branch returns non-locally out of the resultBinding lambda, so the guard lives in invoke around a private create rather than in an .also that that return would skip. BackupArkUnlocker created the throwaway session between recovering the ARK and entering the try that wipes it, so a throwing factory leaked it. Creating the session now sits inside the wipe guard, with ending it under a guard of its own. ChangePasswordUseCase mapped SessionError.Locked to ActiveAccountNotFound on the verifyPassword call, which cannot report it: verify_password unwraps the stored blob without reading session state. Locked is reachable from rewrapForNewPassword, which needs the live ARK, so the arm moves there and the locked-session test asserts that error instead of merely isFailure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
The session guard in CreateAccessUseCase only checked the returned value, so a repository throwing after both persists left the ARK resident. Make it a `finally`, and pin it with a repository stand-in that throws on the one write reached after everything else has succeeded. Three assertions were not carrying their weight: a locked session asserted to be inactive when it was already inactive, an escrow failure whose runCatching result was discarded, and a wrapping failure checked only for being a failure. Also finishes two import reorders from the previous commit that inserted blank lines instead, and moves two helpers out of the middle of the test runs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Key derivation and account creation live inside ArkSession now, so KeyDeriver, AccountManager and the ARK-level KeyWrapper functions have no callers. No ARK-shaped type remains in the generated bindings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
Fix the three cargo fmt regressions the previous commit introduced in lib.rs, key_wrap.rs and ark_session.rs, narrow core ArkSession::unlock to private now that the FFI wrapper that was its only external caller is gone, correct CLAUDE.md's Rust fakes section to stop naming the deleted KeyDeriverInterface/AccountManagerInterface and to document the ArkSession(NoHandle) test-constructor exception, rewrite FakeKeyWrapper's stale KDoc to describe the item-level wrong-key path it actually exercises now, and add a module doc to types.rs explaining why an unimported file full of side-effecting macros is not dead code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
The first commit on this branch moved these two files from `lib/` to `core/` and re-indented a trailing `.unwrap();` in each test while rewriting their imports. v2 is clean under `cargo fmt --check --all`, so the branch was the source of all five diffs, not the tree it came from. CI's rust-lint job gates rust-test and android-test on that check, so the branch could not go green without this. Whitespace only: `git diff -w` over this commit is empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
with_ark held the session mutex for the whole caller closure, and sealing a backup passes an entire serialization plus one AEAD pass over the vault as that closure. Every other session operation takes the same lock, end() included, and the lock observer calls end() on the main thread. A screen-off during an ARK-encrypted export therefore blocked the main thread for as long as the export ran and deferred auto-lock for that window. Clone the ARK under the lock and release it before the closure runs; the clone is an AccountRootKey, so it is zeroized on drop and never leaves Rust. BackupException.Locked mapped to ExportError.SerializationFailed, which is terminal, so a session locking mid-export recorded the job as failed and released the escrowed credentials the retry needed. Map it to the ExportError.SessionLocked variant that already existed for this, which is retryable and carries no persistable reason, so backup_jobs.pb is untouched. The import mapper gets the same arm: a lock between the isActive guard and the call it guards was reported to the user as a parse failure. Also cover the one ARK wipe that had no test, and stop the Session KDoc claiming more than it can deliver: it omitted verifyArk, and the biometric paths take their key from javax.crypto, which keeps a copy no fill(0) reaches. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Resolves conflicts across the test suites for BiometricEnrollmentAdapterImpl, BiometricUnlockAdapterImpl, AuthViewModel, and the backup escrow/export path. Both branches independently evolved the same test fixtures: v2 added KeyStoreManager-Result handling and new edge-case coverage on the old Session/FakeSession API, while refactor/rust-session replaced that API with the Rust-backed Session. Production code merged automatically; the test files are hand-merged here to keep both sets of coverage against the now-combined production API, including two semantic gaps auto-merge couldn't catch (BiometricUnlockAdapterImplTest.adapterOver missing the biometricEnrollmentAdapter param, and FinishExportWizardUseCaseTest asserting on a thrown exception that the Result-based KeyStoreManager no longer throws). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QxzAPGJ5YmGntBE4xGAdYM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.