diff --git a/Resources/Localizable.xcstrings b/Resources/Localizable.xcstrings index 899ac886f992..846cbf3a612b 100644 --- a/Resources/Localizable.xcstrings +++ b/Resources/Localizable.xcstrings @@ -110152,6 +110152,35 @@ } } }, + "settings.account.error.signInSessionFailed": { + "extractionState": "manual", + "localizations": { + "en": { + "stringUnit": { + "state": "translated", + "value": "Sign-in didn't complete. Please try again." + } + }, + "ja": { + "stringUnit": { + "state": "translated", + "value": "サインインを完了できませんでした。もう一度お試しください。" + } + }, + "uk": { + "stringUnit": { + "state": "translated", + "value": "Не вдалося завершити вхід. Спробуйте ще раз." + } + }, + "ko": { + "stringUnit": { + "state": "translated", + "value": "로그인을 완료하지 못했습니다. 다시 시도해 주세요." + } + } + } + }, "remote.state.connected.vmNoProxy": { "extractionState": "manual", "localizations": { diff --git a/Sources/Auth/AuthManager.swift b/Sources/Auth/AuthManager.swift index e060796c6d08..12b95e92dc5c 100644 --- a/Sources/Auth/AuthManager.swift +++ b/Sources/Auth/AuthManager.swift @@ -31,10 +31,11 @@ private final class AuthPresentationContext: NSObject, ASWebAuthenticationPresen } } -enum AuthManagerError: LocalizedError { +enum AuthManagerError: LocalizedError, Equatable { case invalidCallback case missingAccessToken case missingRefreshToken + case signInSessionFailed var errorDescription: String? { switch self { @@ -53,6 +54,11 @@ enum AuthManagerError: LocalizedError { localized: "settings.account.error.missingRefreshToken", defaultValue: "Account refresh token is unavailable." ) + case .signInSessionFailed: + return String( + localized: "settings.account.error.signInSessionFailed", + defaultValue: "Sign-in didn't complete. Please try again." + ) } } } @@ -128,6 +134,7 @@ final class AuthManager: ObservableObject { @Published private(set) var isLoading = false @Published private(set) var isRestoringSession = false @Published private(set) var didCompleteBrowserSignIn = false + @Published private(set) var lastSignInError: AuthManagerError? @Published var selectedTeamID: String? { didSet { guard selectedTeamID != oldValue else { return } @@ -189,6 +196,7 @@ final class AuthManager: ObservableObject { loginPollTask?.cancel() webAuthSession?.cancel() webAuthSession = nil + lastSignInError = nil isLoading = true let signInURL = AuthEnvironment.signInURL() @@ -205,14 +213,15 @@ final class AuthManager: ObservableObject { self.webAuthSession = nil } if let error { - NSLog("auth.webauth failed: %@", "\(error)") + Self.authLog("webauth failed: \(error)") + self.recordSignInSessionError(error) return } guard let callbackURL else { return } do { try await self.handleCallbackURL(callbackURL) } catch { - NSLog("auth.webauth callback failed: %@", "\(error)") + Self.authLog("webauth callback failed: \(error)") } } } @@ -222,11 +231,22 @@ final class AuthManager: ObservableObject { if session.start() { webAuthSession = session } else { - NSLog("auth.webauth: session.start() returned false") + Self.authLog("webauth: session.start() returned false") + lastSignInError = .signInSessionFailed isLoading = false } } + private func recordSignInSessionError(_ error: Error) { + // canceledLogin = explicit user cancel; surfacing an error there is + // hostile. Other codes (network, sandbox, etc.) surface to fix #3617. + if let asError = error as? ASWebAuthenticationSessionError, + asError.code == .canceledLogin { + return + } + lastSignInError = .signInSessionFailed + } + /// Starts the ASWebAuthenticationSession popup and awaits the user's /// completion by observing isAuthenticated AND isLoading. Resolves when /// authenticated, when the sign-in attempt settles unsuccessfully (popup @@ -395,18 +415,25 @@ final class AuthManager: ObservableObject { func handleCallbackURL(_ url: URL) async throws { guard let payload = AuthCallbackRouter.callbackPayload(from: url) else { + lastSignInError = .invalidCallback throw AuthManagerError.invalidCallback } isLoading = true defer { isLoading = false } - await tokenStore.seed( - accessToken: payload.accessToken, - refreshToken: payload.refreshToken - ) - try await refreshSession() - didCompleteBrowserSignIn = true + do { + await tokenStore.seed( + accessToken: payload.accessToken, + refreshToken: payload.refreshToken + ) + try await refreshSession() + didCompleteBrowserSignIn = true + lastSignInError = nil + } catch { + lastSignInError = (error as? AuthManagerError) ?? .signInSessionFailed + throw error + } } func seedTokensFromCLI(refreshToken: String, accessToken: String?) async { @@ -433,6 +460,7 @@ final class AuthManager: ObservableObject { await tokenStore.setTokens(accessToken: resolvedAccess, refreshToken: refreshToken) do { try await refreshSession() + lastSignInError = nil authLog("seedTokensFromCLI: success user=\(currentUser?.primaryEmail ?? "nil")") } catch { authLog("seedTokensFromCLI: refreshSession failed: \(error)") @@ -505,6 +533,7 @@ final class AuthManager: ObservableObject { isAuthenticated = true selectedTeamID = Self.resolveTeamID(selectedTeamID: selectedTeamID, teams: result.teams) didCompleteBrowserSignIn = true + lastSignInError = nil authLog("applySignInResult: user=\(result.email ?? "nil") teams=\(result.teams.count) teamID=\(selectedTeamID ?? "nil")") } @@ -570,6 +599,7 @@ final class AuthManager: ObservableObject { selectedTeamID = Self.resolveTeamID(selectedTeamID: selectedTeamID, teams: teams) authLog("signInWithCredential: success user=\(user.primaryEmail ?? "nil") teams=\(teams.count) teamID=\(selectedTeamID ?? "nil")") didCompleteBrowserSignIn = true + lastSignInError = nil } func signOut() async { @@ -696,6 +726,7 @@ final class AuthManager: ObservableObject { currentUser = nil isAuthenticated = false didCompleteBrowserSignIn = false + lastSignInError = nil if clearSelectedTeam { selectedTeamID = nil } diff --git a/Sources/cmuxApp.swift b/Sources/cmuxApp.swift index 941d3fa288c6..81a3e238d3f2 100644 --- a/Sources/cmuxApp.swift +++ b/Sources/cmuxApp.swift @@ -7526,6 +7526,12 @@ private struct AuthSettingsRow: View { .font(.system(size: 11)) .foregroundColor(.secondary) } + if let errorMessage = errorText { + Text(errorMessage) + .font(.system(size: 11)) + .foregroundColor(.red) + .fixedSize(horizontal: false, vertical: true) + } } Spacer(minLength: 12) if authManager.isLoading || authManager.isRestoringSession { @@ -7567,6 +7573,10 @@ private struct AuthSettingsRow: View { ) } + private var errorText: String? { + authManager.lastSignInError?.errorDescription + } + private var buttonTitle: String { if authManager.isAuthenticated { return String( diff --git a/cmuxTests/AuthManagerSignInErrorVisibilityTests.swift b/cmuxTests/AuthManagerSignInErrorVisibilityTests.swift new file mode 100644 index 000000000000..6ecb43ddce96 --- /dev/null +++ b/cmuxTests/AuthManagerSignInErrorVisibilityTests.swift @@ -0,0 +1,111 @@ +import CMUXAuthCore +import StackAuth +import XCTest + +#if canImport(cmux_DEV) +@testable import cmux_DEV +#elseif canImport(cmux) +@testable import cmux +#endif + +/// Regression coverage for #3617: failed sign-in callbacks were silently +/// `NSLog`-ed and never surfaced to the UI. The fix exposes the failure as a +/// published `lastSignInError`. Per AGENTS.md regression test policy this +/// failing test ships in its own commit so CI proves it catches the bug. +@MainActor +final class AuthManagerSignInErrorVisibilityTests: XCTestCase { + func testInvalidCallbackURLPublishesLastSignInError() async throws { + let manager = AuthManager( + client: NoopAuthTestClient(), + tokenStore: InMemoryTestTokenStore() + ) + await manager.awaitBootstrapped() + + XCTAssertNil( + manager.lastSignInError, + "AuthManager should not surface an error before any sign-in attempt" + ) + + // No `stack_refresh` / `stack_access` query items: this is the exact + // shape that makes `AuthCallbackRouter.callbackPayload` return nil and + // forces `handleCallbackURL` to throw `.invalidCallback`. Do not + // "fix" the URL — that would defeat this regression. + let badCallbackURL = URL(string: "cmux://auth-callback")! + + do { + try await manager.handleCallbackURL(badCallbackURL) + XCTFail("Expected handleCallbackURL to throw AuthManagerError.invalidCallback") + } catch AuthManagerError.invalidCallback { + } catch let error as AuthManagerError { + XCTFail("Expected .invalidCallback, got AuthManagerError.\(error)") + } catch { + XCTFail("Expected AuthManagerError, got \(type(of: error)): \(error)") + } + + XCTAssertEqual( + manager.lastSignInError, + AuthManagerError.invalidCallback, + "After a failed sign-in callback, AuthManager.lastSignInError must be populated so AuthSettingsRow can render it" + ) + } + + func testSignOutClearsStaleLastSignInError() async throws { + let manager = AuthManager( + client: NoopAuthTestClient(), + tokenStore: InMemoryTestTokenStore() + ) + await manager.awaitBootstrapped() + + // Stale-error invariant: a prior failed sign-in leaves .invalidCallback + // published; signOut → clearSessionState must clear it. + do { + try await manager.handleCallbackURL(URL(string: "cmux://auth-callback")!) + XCTFail("Expected handleCallbackURL to throw") + } catch { + } + XCTAssertEqual(manager.lastSignInError, .invalidCallback) + + await manager.signOut() + + XCTAssertNil( + manager.lastSignInError, + "signOut() routes through clearSessionState which must clear lastSignInError so it doesn't survive across auth-state transitions" + ) + } +} + +// MARK: - Test doubles + +private final class NoopAuthTestClient: AuthClientProtocol { + func currentUser() async throws -> CMUXAuthUser? { nil } + func listTeams() async throws -> [AuthTeamSummary] { [] } +} + +private final actor InMemoryTestTokenStore: StackAuthTokenStoreProtocol { + private var accessToken: String? + private var refreshToken: String? + + func getStoredAccessToken() async -> String? { accessToken } + func getStoredRefreshToken() async -> String? { refreshToken } + + func setTokens(accessToken: String?, refreshToken: String?) async { + self.accessToken = accessToken + self.refreshToken = refreshToken + } + + func clearTokens() async { + accessToken = nil + refreshToken = nil + } + + func compareAndSet( + compareRefreshToken: String, + newRefreshToken: String?, + newAccessToken: String? + ) async { + if refreshToken == compareRefreshToken { + refreshToken = newRefreshToken + accessToken = newAccessToken + } + } +}