mirror of
https://codeberg.org/secana/Forji.git
synced 2026-08-15 14:43:29 -07:00
fix: don't delete account credentials on a transient session-restore failure
A single network failure during automatic session restore (e.g. VPN down) was unconditionally treated as a confirmed token rejection, wiping the instance and its Keychain credentials and forcing the user to re-add the account with a new token even after connectivity returned. Only a definite tokenExpired/tokenPermissionDenied error now triggers logout. Fixes https://codeberg.org/secana/Forji/issues/89
This commit is contained in:
parent
3946db4f21
commit
6e6055f12d
4 changed files with 80 additions and 6 deletions
|
|
@ -83,7 +83,9 @@ struct ContentView: View {
|
||||||
instance.lastUsed = Date()
|
instance.lastUsed = Date()
|
||||||
try? modelContext.save()
|
try? modelContext.save()
|
||||||
} catch {
|
} catch {
|
||||||
await authService.logout(modelContext: modelContext)
|
if Self.shouldLogout(after: error) {
|
||||||
|
await authService.logout(modelContext: modelContext)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
sessionFullyRestored = true
|
sessionFullyRestored = true
|
||||||
}
|
}
|
||||||
|
|
@ -140,7 +142,11 @@ struct ContentView: View {
|
||||||
try? modelContext.save()
|
try? modelContext.save()
|
||||||
sessionFullyRestored = true
|
sessionFullyRestored = true
|
||||||
} catch {
|
} catch {
|
||||||
await authService.logout(modelContext: modelContext)
|
// A transient/network failure leaves the bootstrapped session in place so the
|
||||||
|
// app can retry (via completeSessionRestore) instead of wiping the account.
|
||||||
|
if Self.shouldLogout(after: error) {
|
||||||
|
await authService.logout(modelContext: modelContext)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
@ -184,6 +190,13 @@ struct ContentView: View {
|
||||||
}
|
}
|
||||||
#endif
|
#endif
|
||||||
|
|
||||||
|
/// Only a confirmed credential rejection justifies deleting the stored instance and
|
||||||
|
/// credentials. Transient failures (network, server, certificate, ambiguous responses)
|
||||||
|
/// should leave the account intact so the app can retry on the next launch.
|
||||||
|
static func shouldLogout(after error: Error) -> Bool {
|
||||||
|
(error as? SessionRestoreError)?.isDefiniteCredentialFailure ?? false
|
||||||
|
}
|
||||||
|
|
||||||
private func switchInstance(to pending: PendingNavigation) async {
|
private func switchInstance(to pending: PendingNavigation) async {
|
||||||
let pendingURL = ForgejoClient.normalizeServerURL(pending.serverURL)
|
let pendingURL = ForgejoClient.normalizeServerURL(pending.serverURL)
|
||||||
let pendingUsername = pending.username
|
let pendingUsername = pending.username
|
||||||
|
|
|
||||||
|
|
@ -184,10 +184,14 @@ class AuthenticationService {
|
||||||
throw SessionRestoreError.tokenExpired
|
throw SessionRestoreError.tokenExpired
|
||||||
}
|
}
|
||||||
let password = try await KeychainManager.shared.getPassword(for: serverURL, username: username)
|
let password = try await KeychainManager.shared.getPassword(for: serverURL, username: username)
|
||||||
try await login(
|
do {
|
||||||
serverURL: serverURL, username: username,
|
try await login(
|
||||||
password: password, allowSelfSigned: allowSelfSigned,
|
serverURL: serverURL, username: username,
|
||||||
)
|
password: password, allowSelfSigned: allowSelfSigned,
|
||||||
|
)
|
||||||
|
} catch {
|
||||||
|
throw SessionRestoreError.fromTokenValidationError(error)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Stub factories for SwiftUI previews
|
// Stub factories for SwiftUI previews
|
||||||
|
|
@ -245,6 +249,19 @@ enum SessionRestoreError: LocalizedError, Equatable {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Whether the server has definitively rejected the stored credential, as opposed to a
|
||||||
|
/// transient/network/server-side failure that may resolve on its own. Only this case
|
||||||
|
/// justifies deleting the stored instance and credentials.
|
||||||
|
var isDefiniteCredentialFailure: Bool {
|
||||||
|
switch self {
|
||||||
|
case .tokenExpired, .tokenPermissionDenied:
|
||||||
|
true
|
||||||
|
case .serverUnavailable, .serverNotFound, .networkUnavailable, .certificateError,
|
||||||
|
.invalidServerResponse, .tokenValidationFailedHTTPStatus, .tokenValidationFailed:
|
||||||
|
false
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
static func fromTokenValidationError(_ error: Error) -> SessionRestoreError {
|
static func fromTokenValidationError(_ error: Error) -> SessionRestoreError {
|
||||||
if let sessionError = error as? SessionRestoreError {
|
if let sessionError = error as? SessionRestoreError {
|
||||||
return sessionError
|
return sessionError
|
||||||
|
|
|
||||||
31
Forji/ForjiTests/ContentViewTests.swift
Normal file
31
Forji/ForjiTests/ContentViewTests.swift
Normal file
|
|
@ -0,0 +1,31 @@
|
||||||
|
import ForgejoKit
|
||||||
|
import Foundation
|
||||||
|
import Testing
|
||||||
|
@testable import Forji
|
||||||
|
|
||||||
|
struct ContentViewTests {
|
||||||
|
|
||||||
|
// MARK: - shouldLogout
|
||||||
|
|
||||||
|
@Test func logsOutOnConfirmedTokenExpired() {
|
||||||
|
#expect(ContentView.shouldLogout(after: SessionRestoreError.tokenExpired))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test func logsOutOnConfirmedPermissionDenied() {
|
||||||
|
#expect(ContentView.shouldLogout(after: SessionRestoreError.tokenPermissionDenied))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test func doesNotLogOutOnNetworkUnavailable() {
|
||||||
|
#expect(!ContentView.shouldLogout(after: SessionRestoreError.networkUnavailable))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test func doesNotLogOutOnServerUnavailable() {
|
||||||
|
#expect(!ContentView.shouldLogout(after: SessionRestoreError.serverUnavailable))
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test func doesNotLogOutOnRawURLError() {
|
||||||
|
// Errors that never made it through SessionRestoreError classification
|
||||||
|
// must not be treated as a confirmed credential rejection.
|
||||||
|
#expect(!ContentView.shouldLogout(after: URLError(.notConnectedToInternet)))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
@ -64,4 +64,17 @@ struct SessionRestoreErrorTests {
|
||||||
.contains("expired") == false
|
.contains("expired") == false
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test func onlyConfirmedCredentialRejectionsAreDefiniteFailures() {
|
||||||
|
#expect(SessionRestoreError.tokenExpired.isDefiniteCredentialFailure)
|
||||||
|
#expect(SessionRestoreError.tokenPermissionDenied.isDefiniteCredentialFailure)
|
||||||
|
|
||||||
|
#expect(!SessionRestoreError.networkUnavailable.isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.serverUnavailable.isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.serverNotFound.isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.certificateError.isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.invalidServerResponse.isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.tokenValidationFailedHTTPStatus(500).isDefiniteCredentialFailure)
|
||||||
|
#expect(!SessionRestoreError.tokenValidationFailed.isDefiniteCredentialFailure)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue