feat(accounts): add platform credential slots - #436
Conversation
Obiente previewNC Native · Obiente updates this comment as the preview changes. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5e6014e2e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0bb0e52ff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dabe16e75
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5849f1719f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
d6d0017 to
4daca37
Compare
4daca37 to
f7931ce
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7931ce047
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| withContext(Dispatchers.IO) { | ||
| accountOperationGuard.serialize operation@{ | ||
| if (activeAccountId() == accountId) return@operation loadSession(accountId) | ||
| if (hasLiveAccountResources()) { |
There was a problem hiding this comment.
Block new range sessions during account selection
Although virtual-provider activation is now serialized, openFileRangeSession is fresh evidence of a remaining race: it registers resources only under fileRangeSessionLock and never takes accountOperationGuard. If a file or media range opens after this snapshot reports no live resources but before the selected credential is persisted, the switch succeeds while that old-account session remains open and can continue authenticated reads after the new identity is published. Serialize range-session registration with account transitions or mark the session as switching before checking resources.
Useful? React with 👍 / 👎.
| if (!json.has(KEY_VERSION)) { | ||
| restoreLegacyAndroidAccountCredentialState(json) | ||
| } else { | ||
| require(json.getInt(KEY_VERSION) == ANDROID_ACCOUNT_CREDENTIAL_STORE_VERSION) |
There was a problem hiding this comment.
Preserve future credential stores as unsupported
When a user downgrades after a newer app has written a version greater than 2, this check sends the store through the generic malformed path. saveSession then replaces KEY_SESSION with a one-account v2 store; the original is never read from quarantine on a later upgrade, and it is discarded outright when an older quarantine already exists. The legacy-registry branch now preserves unsupported versions as read-only, but this top-level version check remains fresh evidence of the same downgrade risk; return a distinct unsupported state and reject mutations instead.
AGENTS.md reference: AGENTS.md:L325-L326
Useful? React with 👍 / 👎.
| } else { | ||
| persistState(current.remove(accountId)) |
There was a problem hiding this comment.
Clear previews when removing an inactive account
When the requested account is retained but inactive, this branch removes only its credential slot. That account can still have private entries in AndroidNativeMediaPreviewCache from when it was previously selected, but only the active-account removal path calls clearPreviewAccount; after this mutation the normal cleanup path no longer has the removed session available to derive its cache identity. Clear the removed account's preview cache as part of this branch so account removal does not leave its cached content on the device.
Useful? React with 👍 / 👎.
| (readStore() as? AndroidAccountCredentialStoreRead.Available)?.state?.registry?.accounts.orEmpty() | ||
|
|
There was a problem hiding this comment.
Keep account listing credential-free
NextcloudAccountCredentialServices.listAccounts() promises to list local records without loading their secrets, but this implementation calls readStore(), whose decoder constructs a NextcloudSession containing the app password for every stored account. Consequently every account-picker refresh decrypts and materializes all credentials even though it needs only registry metadata, unnecessarily expanding secret exposure and allowing one damaged credential slot to suppress the whole account list. Store or read the credential-free registry independently and load only the specifically requested account secret.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1e4cb658
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| commitPreferences(editor) | ||
| }, | ||
| cancelAll = scheduler::cancelAll, |
There was a problem hiding this comment.
Serialize Android sync runs before switching accounts
When a periodic or manually triggered sync is already running, this callback only initiates asynchronous WorkManager cancellation, and manual AndroidFileSyncEngine.runPair calls are not covered at all. Because replaceSession persists and publishes the replacement before invoking this callback, the old account can continue issuing remote mutations after selectAccount has returned with the new identity; make sync runs acquire the account operation lease and wait for them before committing the transition.
AGENTS.md reference: AGENTS.md:L373-L376
Useful? React with 👍 / 👎.
| saveSecret(session) | ||
| persistAccountState(encodedRegistry, updatedRegistry.activeAccount) |
There was a problem hiding this comment.
Roll back newly saved secrets when registry persistence fails
When saving a brand-new account, the keychain write succeeds before persistAccountState; if Preferences.flush() then fails, that method restores the old registry and throws but leaves the new app password in its account-specific secret slot. The account is absent from listAccounts, so removeAccount cannot clean up the valid orphaned credential unless the user happens to retry the same login; add transactional rollback or a durable recovery record for this failure path.
AGENTS.md reference: AGENTS.md:L457-L459
Useful? React with 👍 / 👎.
| if (current.registry.activeAccountId == accountId) { | ||
| clearSession(current) | ||
| } else { | ||
| persistState(current.remove(accountId)) |
There was a problem hiding this comment.
Resolve deferred uploads when deleting a retained account
If a durable multipart worker has already observed this account as inactive, it returns success while leaving the job Queued, relying exclusively on a future account selection to call resumeQueuedForAccount. Removing that retained account here only deletes its credential slot, so no worker or future selection will ever transition or remove the parked job, release its persisted source permission, or free its active-queue capacity; cancel and clean up these deferred uploads as part of account removal.
AGENTS.md reference: AGENTS.md:L457-L459
Useful? React with 👍 / 👎.
|
|
||
| fun upsertAndSelect(session: NextcloudSession): AndroidAccountCredentialState { | ||
| requireMutationsAllowed() | ||
| return copy( |
There was a problem hiding this comment.
Keep Android work keys stable for the canonical account
This state keys and replaces sessions by canonical NextcloudAccountId, but Android sync, offline, and upload records use NextcloudDocumentIds.accountKey, which hashes the raw server URL after only trimming trailing slashes. Reauthenticating the same account with a canonically equivalent URL spelling, such as different host case or an explicit default port, therefore replaces this credential slot while changing its work identity: persisted sync pairs are filtered out and retained jobs can no longer resolve their session. Derive Android work keys from the canonical account ID or migrate the old keys when the stored URL representation changes.
AGENTS.md reference: AGENTS.md:L410-L413
Useful? React with 👍 / 👎.
Outcome
Advances #172. This PR depends on #429.
Verification
Build-host commands:
The full source and packaging validation ran on exact #429 parent
78eb576fc8881a246feb1fac8b0b443d4ed1b903. Before publication, #429 advanced tof0696b474a625bd52dfffb29d5695a0a8f996265with only a one-line automated marketing capture-manifest update. The child source was unchanged. The final child commit only links this PR number in the already-validated changelog fragment.Compatibility and risk
Visual changes
Not applicable.