fix: Delete the API token, not just the password, when removing an account (#40)

`deleteInstance` cleared only the keychain password, never the token (`logout` deletes both), so swipe-to-delete left a live API token orphaned; this routes both paths through a new `KeychainManager.deleteCredentials` and adds regression tests (186 ForjiTests green).

I grant Stefan Hausotte an irrevocable, worldwide, royalty-free license to use, sublicense, and distribute my contribution, including through Apple's App Store under the project's App Store exception.

Reviewed-on: https://codeberg.org/secana/Forji/pulls/40
This commit is contained in:
systemBlue 2026-06-02 18:20:43 +02:00 committed by secana
parent 89a0cd0bb2
commit 31d9aeea35
4 changed files with 47 additions and 6 deletions

View file

@ -99,11 +99,7 @@ class AuthenticationService {
if let instance = currentInstance {
let normalizedURL = ForgejoClient.normalizeServerURL(instance.serverURL)
do {
try await KeychainManager.shared.deletePassword(
for: normalizedURL,
username: instance.username,
)
try await KeychainManager.shared.deleteToken(
try await KeychainManager.shared.deleteCredentials(
for: normalizedURL,
username: instance.username,
)

View file

@ -40,6 +40,15 @@ actor KeychainManager {
try deleteItem(forKey: key(for: server, username: username, suffix: "_token"))
}
// MARK: - Account (both items)
/// Removes every credential stored for an account: the password and the API token.
/// Use when an account is removed entirely (logout, delete instance) so neither item is orphaned.
func deleteCredentials(for server: String, username: String) throws {
try deletePassword(for: server, username: username)
try deleteToken(for: server, username: username)
}
// MARK: - Synchronous token read (no actor hop)
nonisolated static func getTokenSync(for server: String, username: String) -> String? {

View file

@ -223,7 +223,7 @@ struct InstanceListView: View {
assertionFailure("SwiftData save failed: \(error)")
}
Task {
try? await KeychainManager.shared.deletePassword(for: normalizedURL, username: username)
try? await KeychainManager.shared.deleteCredentials(for: normalizedURL, username: username)
}
}

View file

@ -133,4 +133,40 @@ struct KeychainManagerTests {
// Clean up
try await KeychainManager.shared.deletePassword(for: normalized, username: "user")
}
@Test func keychainDeleteCredentialsRemovesBothPasswordAndToken() async throws {
let server = "https://delete-credentials.example.com"
let username = "user"
try await KeychainManager.shared.savePassword("mypassword", for: server, username: username)
try await KeychainManager.shared.saveToken("mytoken", for: server, username: username)
try await KeychainManager.shared.deleteCredentials(for: server, username: username)
do {
_ = try await KeychainManager.shared.getPassword(for: server, username: username)
Issue.record("Expected KeychainError.notFound for password after deleteCredentials")
} catch is KeychainError {
// Expected
} catch {
Issue.record("Unexpected error: \(error)")
}
do {
_ = try await KeychainManager.shared.getToken(for: server, username: username)
Issue.record("Expected KeychainError.notFound for token after deleteCredentials")
} catch is KeychainError {
// Expected this is the assertion that fails under the old leak
} catch {
Issue.record("Unexpected error: \(error)")
}
}
@Test func keychainDeleteCredentialsNonExistentDoesNotThrow() async throws {
// Deleting credentials for an account with nothing stored must not throw.
try await KeychainManager.shared.deleteCredentials(
for: "https://nonexistent-credentials.example.com",
username: "nobody"
)
}
}