From 6e6055f12d875d5edcba51de0180f491ded754c8 Mon Sep 17 00:00:00 2001 From: Stefan Hausotte Date: Sat, 25 Jul 2026 19:33:53 +0200 Subject: [PATCH] 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 --- Forji/Forji/App/ContentView.swift | 17 ++++++++-- .../Services/AuthenticationService.swift | 25 ++++++++++++--- Forji/ForjiTests/ContentViewTests.swift | 31 +++++++++++++++++++ .../ForjiTests/SessionRestoreErrorTests.swift | 13 ++++++++ 4 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 Forji/ForjiTests/ContentViewTests.swift diff --git a/Forji/Forji/App/ContentView.swift b/Forji/Forji/App/ContentView.swift index 16e5bbc..94c3cd3 100644 --- a/Forji/Forji/App/ContentView.swift +++ b/Forji/Forji/App/ContentView.swift @@ -83,7 +83,9 @@ struct ContentView: View { instance.lastUsed = Date() try? modelContext.save() } catch { - await authService.logout(modelContext: modelContext) + if Self.shouldLogout(after: error) { + await authService.logout(modelContext: modelContext) + } } sessionFullyRestored = true } @@ -140,7 +142,11 @@ struct ContentView: View { try? modelContext.save() sessionFullyRestored = true } 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 } @@ -184,6 +190,13 @@ struct ContentView: View { } #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 { let pendingURL = ForgejoClient.normalizeServerURL(pending.serverURL) let pendingUsername = pending.username diff --git a/Forji/Forji/Services/AuthenticationService.swift b/Forji/Forji/Services/AuthenticationService.swift index 6089481..66df955 100644 --- a/Forji/Forji/Services/AuthenticationService.swift +++ b/Forji/Forji/Services/AuthenticationService.swift @@ -184,10 +184,14 @@ class AuthenticationService { throw SessionRestoreError.tokenExpired } let password = try await KeychainManager.shared.getPassword(for: serverURL, username: username) - try await login( - serverURL: serverURL, username: username, - password: password, allowSelfSigned: allowSelfSigned, - ) + do { + try await login( + serverURL: serverURL, username: username, + password: password, allowSelfSigned: allowSelfSigned, + ) + } catch { + throw SessionRestoreError.fromTokenValidationError(error) + } } // 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 { if let sessionError = error as? SessionRestoreError { return sessionError diff --git a/Forji/ForjiTests/ContentViewTests.swift b/Forji/ForjiTests/ContentViewTests.swift new file mode 100644 index 0000000..54d0a89 --- /dev/null +++ b/Forji/ForjiTests/ContentViewTests.swift @@ -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))) + } +} diff --git a/Forji/ForjiTests/SessionRestoreErrorTests.swift b/Forji/ForjiTests/SessionRestoreErrorTests.swift index 34bc1ad..589ca04 100644 --- a/Forji/ForjiTests/SessionRestoreErrorTests.swift +++ b/Forji/ForjiTests/SessionRestoreErrorTests.swift @@ -64,4 +64,17 @@ struct SessionRestoreErrorTests { .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) + } }