From e93a435ca33899c99c86bcd2ca0d3176eb482082 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Mon, 22 Dec 2025 23:51:56 +0100 Subject: [PATCH 1/2] Add clearLogin to LoginStorage to prevent storage of invalid tokens --- Sources/OAuthenticator/Authenticator.swift | 21 +++--- Sources/OAuthenticator/Models.swift | 9 ++- .../AuthenticatorTests.swift | 69 +++++++++++++++++++ 3 files changed, 86 insertions(+), 13 deletions(-) diff --git a/Sources/OAuthenticator/Authenticator.swift b/Sources/OAuthenticator/Authenticator.swift index 3c93c2b..a2af0df 100644 --- a/Sources/OAuthenticator/Authenticator.swift +++ b/Sources/OAuthenticator/Authenticator.swift @@ -219,16 +219,13 @@ extension Authenticator { try await storage.storeLogin(login) } - private func clearLogin() async { - guard let storage = config.loginStorage else { return } - - let invalidLogin = Login(token: "invalid", validUntilDate: .distantPast) - - do { - try await storage.storeLogin(invalidLogin) - } catch { - print("failed to store an invalid login, possibly stuck", error) + private func clearLogin() async throws { + guard let storage = config.loginStorage else { + self.localLogin = nil + return } + + try await storage.clearLogin() } } @@ -267,8 +264,10 @@ extension Authenticator { await self.config.authenticationStatusHandler?(.success(login)) } catch let authenticatorError as AuthenticatorError { - await self.config.authenticationStatusHandler?(.failure(authenticatorError)) + try await clearLogin() + await self.config.authenticationStatusHandler?(.failure(authenticatorError)) + // Rethrow error throw authenticatorError } @@ -356,8 +355,6 @@ extension Authenticator { return login } catch { - await clearLogin() - throw error } } diff --git a/Sources/OAuthenticator/Models.swift b/Sources/OAuthenticator/Models.swift index 71b12fd..2c50251 100644 --- a/Sources/OAuthenticator/Models.swift +++ b/Sources/OAuthenticator/Models.swift @@ -88,13 +88,20 @@ public struct AppCredentials: Codable, Hashable, Sendable { public struct LoginStorage: Sendable { public typealias RetrieveLogin = @Sendable () async throws -> Login? public typealias StoreLogin = @Sendable (Login) async throws -> Void + public typealias ClearLogin = @Sendable () async throws -> Void public let retrieveLogin: RetrieveLogin public let storeLogin: StoreLogin + public let clearLogin: ClearLogin - public init(retrieveLogin: @escaping RetrieveLogin, storeLogin: @escaping StoreLogin) { + public init( + retrieveLogin: @escaping RetrieveLogin, + storeLogin: @escaping StoreLogin, + clearLogin: @escaping ClearLogin + ) { self.retrieveLogin = retrieveLogin self.storeLogin = storeLogin + self.clearLogin = clearLogin } } diff --git a/Tests/OAuthenticatorTests/AuthenticatorTests.swift b/Tests/OAuthenticatorTests/AuthenticatorTests.swift index b14dc10..b9d7a5e 100644 --- a/Tests/OAuthenticatorTests/AuthenticatorTests.swift +++ b/Tests/OAuthenticatorTests/AuthenticatorTests.swift @@ -103,6 +103,8 @@ final class AuthenticatorTests: XCTestCase { XCTAssertEqual($0, Login(token: "TOKEN")) storeTokenExp.fulfill() + } clearLogin: { + XCTFail() } let config = Authenticator.Configuration( @@ -152,6 +154,8 @@ final class AuthenticatorTests: XCTestCase { return Login(token: "TOKEN") } storeLogin: { _ in XCTFail() + } clearLogin: { + XCTFail() } let config = Authenticator.Configuration(appCredentials: Self.mockCredentials, @@ -204,6 +208,8 @@ final class AuthenticatorTests: XCTestCase { storeTokenExp.fulfill() XCTAssertEqual(login.accessToken.value, "REFRESHED") + } clearLogin: { + XCTFail() } let config = Authenticator.Configuration(appCredentials: Self.mockCredentials, @@ -218,6 +224,65 @@ final class AuthenticatorTests: XCTestCase { await fulfillment(of: [retrieveTokenExp, refreshExp, storeTokenExp, authedLoadExp], timeout: 1.0, enforceOrder: true) } + @MainActor + func testExpiredTokenRefreshFailing() async throws { + let mockLoader: URLResponseProvider = { request in + // We should never load the resource, since we failed to refresh the session: + XCTFail() + + return MockURLResponseProvider.dummyResponse + } + + let refreshExp = expectation(description: "refresh") + let refreshProvider: TokenHandling.RefreshProvider = { login, _, _ in + XCTAssertEqual(login.accessToken.value, "EXPIRED") + XCTAssertEqual(login.refreshToken?.value, "REFRESH") + + refreshExp.fulfill() + + // Fail the refresh attempt, e.g., the refresh token has expired: + throw AuthenticatorError.refreshNotPossible + } + + let tokenHandling = TokenHandling(authorizationURLProvider: Self.disabledAuthorizationURLProvider, + loginProvider: Self.disabledLoginProvider, + refreshProvider: refreshProvider, + responseStatusProvider: TokenHandling.allResponsesValid) + + let retrieveTokenExp = expectation(description: "get token") + let clearTokenExp = expectation(description: "clear token") + + let storage = LoginStorage { + retrieveTokenExp.fulfill() + + return Login(accessToken: Token(value: "EXPIRED", expiry: .distantPast), + refreshToken: Token(value: "REFRESH")) + } storeLogin: { login in + XCTFail() + } clearLogin: { + clearTokenExp.fulfill() + } + + let config = Authenticator.Configuration(appCredentials: Self.mockCredentials, + loginStorage: storage, + tokenHandling: tokenHandling, + userAuthenticator: Self.disabledUserAuthenticator) + + let auth = Authenticator(config: config, urlLoader: mockLoader) + + do { + let (_, _) = try await auth.response(for: URLRequest(url: URL(string: "https://example.com")!)) + + XCTFail() + } catch AuthenticatorError.refreshNotPossible { + // we expect this error to be thrown + } catch { + XCTFail() + } + + await fulfillment(of: [retrieveTokenExp, refreshExp, clearTokenExp], timeout: 1.0, enforceOrder: true) + } + @MainActor func testManualAuthentication() async throws { let urlProvider: TokenHandling.AuthorizationURLProvider = { parameters in @@ -418,6 +483,8 @@ final class AuthenticatorTests: XCTestCase { refreshToken: Token(value: "REFRESH")) } storeLogin: { login in XCTAssertEqual(login.accessToken.value, "REFRESHED") + } clearLogin: { + XCTFail() } let config = Authenticator.Configuration(appCredentials: Self.mockCredentials, @@ -479,6 +546,8 @@ final class AuthenticatorTests: XCTestCase { return storedLogin } storeLogin: { @MainActor login in savedLogins.append(login) + } clearLogin: { + XCTFail() } let config = Authenticator.Configuration(appCredentials: Self.mockCredentials, From 529b8f189e5da681c7923c95195db3d0ce73a154 Mon Sep 17 00:00:00 2001 From: Emelia Smith Date: Tue, 23 Dec 2025 00:37:14 +0100 Subject: [PATCH 2/2] Revert move of clearLogin() --- Sources/OAuthenticator/Authenticator.swift | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Sources/OAuthenticator/Authenticator.swift b/Sources/OAuthenticator/Authenticator.swift index a2af0df..90edd81 100644 --- a/Sources/OAuthenticator/Authenticator.swift +++ b/Sources/OAuthenticator/Authenticator.swift @@ -264,10 +264,8 @@ extension Authenticator { await self.config.authenticationStatusHandler?(.success(login)) } catch let authenticatorError as AuthenticatorError { - try await clearLogin() - await self.config.authenticationStatusHandler?(.failure(authenticatorError)) - + // Rethrow error throw authenticatorError } @@ -355,6 +353,8 @@ extension Authenticator { return login } catch { + try await clearLogin() + throw error } }