Repository navigation
iOS sign-in: bound every phase, cancellable OAuth waits, Cancel escape hatch #5728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2d7fb36
3d779d1
0a00bd7
2f8bbf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,19 @@ extension AuthError { | |
| self = authError | ||
| return | ||
| } | ||
| if error is CancellationError || Task.isCancelled { | ||
| // A cancelled flow (the user backed out, or the sheet's task was | ||
| // torn down) is not a failure to render. Mapping it to the generic | ||
| // server error would flash "Something went wrong" after every | ||
| // deliberate cancel. The `Task.isCancelled` arm matters because | ||
| // URLSession-backed phases surface task cancellation as | ||
| // `URLError(.cancelled)` (or a Stack transport error wrapping it), | ||
| // not as `CancellationError`: this mapping always runs in the | ||
| // cancelled flow's own catch, so any error caught on a cancelled | ||
| // task is the cancellation, not an independent failure. | ||
| self = .cancelled | ||
|
Comment on lines
18
to
+30
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The mapping Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| return | ||
| } | ||
| if let stackError = error as? any StackAuthErrorProtocol { | ||
| switch stackError.code.uppercased() { | ||
| case "OAUTH_CANCELLED": | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import Foundation | ||
|
|
||
| /// The sign-in/session phase a deadline applies to. Used as the stable label | ||
| /// in timeout diagnostics so a stuck-at-sign-in report names the exact phase. | ||
| enum AuthPhase: String, Sendable { | ||
| case sendCode = "send_code" | ||
| case verifyCode = "verify_code" | ||
| case passwordSignIn = "password_sign_in" | ||
| case oauth = "oauth" | ||
| case fetchUser = "fetch_user" | ||
| case validateSession = "validate_session" | ||
| case listTeams = "list_teams" | ||
| case postSignIn = "post_sign_in" | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| import Foundation | ||
|
|
||
| /// Internal marker distinguishing "the deadline child fired" from errors the | ||
| /// operation itself threw (including `CancellationError` when the caller's | ||
| /// task is cancelled, which must surface as a cancellation, not a timeout). | ||
| private struct AuthPhaseDeadlineExceeded: Error {} | ||
|
|
||
| /// Race `operation` against a `duration` deadline on `clock`. | ||
| /// | ||
| /// Structured: whichever side finishes first cancels the other, and the task | ||
| /// group joins both children before returning, so neither can outlive the | ||
| /// call. The operation must be cancellation-responsive for the bound to be | ||
| /// real (URLSession calls are; the interactive OAuth waits are made so by the | ||
| /// cancellation gates in the vendored Stack SDK). | ||
| /// | ||
| /// - Parameters: | ||
| /// - phase: The phase label for timeout diagnostics. | ||
| /// - duration: The deadline. | ||
| /// - clock: The clock the deadline sleeps on (virtual in tests). | ||
| /// - log: Redacted diagnostics sink; timeouts log the phase and duration. | ||
| /// - operation: The bounded work. | ||
| /// - Returns: The operation's value when it beats the deadline. | ||
| /// - Throws: ``AuthError/timedOut`` at the deadline; otherwise rethrows the | ||
| /// operation's error (including `CancellationError` on outer cancellation). | ||
| func withAuthPhaseTimeout<T: Sendable>( | ||
| _ phase: AuthPhase, | ||
| duration: Duration, | ||
| clock: any Clock<Duration>, | ||
| log: AuthDebugLog, | ||
| operation: @escaping @Sendable () async throws -> T | ||
| ) async throws -> T { | ||
| try await withThrowingTaskGroup(of: T.self) { group in | ||
| group.addTask { | ||
| try await operation() | ||
| } | ||
| group.addTask { | ||
| try await clock.sleep(for: duration, tolerance: nil) | ||
| throw AuthPhaseDeadlineExceeded() | ||
| } | ||
| // Cancel the loser on every exit path; the group then joins it. | ||
| defer { group.cancelAll() } | ||
| do { | ||
| guard let first = try await group.next() else { | ||
| throw AuthError.timedOut | ||
| } | ||
|
Comment on lines
+40
to
+45
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With exactly two child tasks added (the operation and the deadline), the first call to Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time! |
||
| return first | ||
| } catch is AuthPhaseDeadlineExceeded { | ||
| log.log("auth.phase=\(phase.rawValue) timed out after \(duration)") | ||
| throw AuthError.timedOut | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import Foundation | ||
|
|
||
| /// Per-phase deadlines for the sign-in and session flows the | ||
| /// ``AuthCoordinator`` drives. | ||
| /// | ||
| /// Every phase that holds a user-visible loading state is raced against one of | ||
| /// these deadlines on the coordinator's injected clock, so no flow can leave | ||
| /// the UI spinning forever: a phase that neither completes nor fails by its | ||
| /// deadline ends as ``AuthError/timedOut`` (localized, retryable). Tests | ||
| /// exercise the deadlines with a virtual clock instead of real waiting. | ||
| public struct AuthTimeouts: Sendable, Equatable { | ||
| /// Deadline for flows that include interactive system auth UI | ||
| /// (`ASAuthorizationController` / `ASWebAuthenticationSession`). Generous, | ||
| /// because the user may legitimately be typing a password or completing | ||
| /// 2FA inside the sheet; it exists to terminate the "callback never fires" | ||
| /// hang, not to rush the user. The sign-in UI offers a cancel affordance | ||
| /// for bailing out earlier. | ||
| public var interactiveFlow: Duration | ||
| /// Deadline for non-interactive backend calls (magic-link send/verify, | ||
| /// credential sign-in, user fetch, session validation, team list, the | ||
| /// post-sign-in hook). Sized above the Stack SDK's 30s per-request timeout | ||
| /// so it only fires when retries stack up or a call wedges. | ||
| public var network: Duration | ||
|
|
||
| /// The production defaults: 300s interactive, 60s network. | ||
| public static let `default` = AuthTimeouts( | ||
| interactiveFlow: .seconds(300), | ||
| network: .seconds(60) | ||
| ) | ||
|
|
||
| /// Creates a timeout table. | ||
| /// - Parameters: | ||
| /// - interactiveFlow: Deadline for flows with interactive system UI. | ||
| /// - network: Deadline for non-interactive backend calls. | ||
| public init(interactiveFlow: Duration, network: Duration) { | ||
| self.interactiveFlow = interactiveFlow | ||
| self.network = network | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import CMUXAuthCore | ||
| import Foundation | ||
| import Testing | ||
| @testable import CmuxAuthRuntime | ||
|
|
||
| @MainActor | ||
| @Suite struct AuthCoordinatorCancellationTests { | ||
| private func makeCoordinator(client: any AuthClient) -> AuthCoordinator { | ||
| let store = FakeKeyValueStore() | ||
| return AuthCoordinator( | ||
| client: client, | ||
| sessionCache: CMUXAuthSessionCache(keyValueStore: store, key: "has_tokens"), | ||
| userCache: CMUXAuthIdentityStore(keyValueStore: store, key: "cached_user"), | ||
| teamSelection: CMUXAuthTeamSelectionStore(keyValueStore: store, key: "selected_team"), | ||
| anchor: FakeAnchor(), | ||
| config: .test, | ||
| launch: .plain() | ||
| ) | ||
| } | ||
|
|
||
| @Test func taskCancellationMapsToCancelledNotGenericError() { | ||
| // Backing out of a stuck sign-in is not a failure: it must map to | ||
| // .cancelled (which the UI silently ignores), not the generic | ||
| // "Something went wrong" server error. | ||
| #expect(AuthError(displaySafe: CancellationError()) == .cancelled) | ||
| } | ||
|
|
||
| @Test func cancellingInFlightOAuthSignInThrowsCancelledAndStopsLoading() async { | ||
| let client = HangingOAuthAuthClient() | ||
| let coordinator = makeCoordinator(client: client) | ||
|
|
||
| let signIn = Task { try await coordinator.signInWithApple() } | ||
| await client.oauthDidStart() | ||
| signIn.cancel() | ||
|
|
||
| await #expect(throws: AuthError.cancelled) { try await signIn.value } | ||
| #expect(coordinator.isLoading == false) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.publicprivacy viaAuthDebugLoglog.log("auth.oauth provider=\(provider) failed: \(error)")routes throughAuthDebugLog, which passes the redacted string tologger.log(…, "\(redactedMessage, privacy: .public)"). The redaction patterns strip tokens, JWTs, and email addresses, but not themessagefield ofStackAuthErrororOAuthError— those can include server-controlled strings or the verbose Apple SIWA diagnostic messages hardcoded inAppleSignInDelegate(up to ~300 characters of Xcode/developer.apple.com instructions). Using.publicmeans this content is visible to any log collector without the usualprivacy: .privatescrubbing. The existing error logs invalidateCachedSessionandtryAutoLoginuseauthLog.error(…, privacy: .private)for the same reason. Either route this throughauthLogwithprivacy: .private, or extendAuthDebugLog.redactedto strip themessagefield from Stack SDK error representations before the.publicsink sees them.