feat: add Google OAuth sign-in to FlyFunBrief

[roznet]

Summary

• Add Google OAuth as second auth provider alongside Apple Sign-In
• Uses ASWebAuthenticationSession with shared flyfun-common auth server (forms.flyfun.aero)
• JWT token stored in Keychain, user info fetched from /auth/me
• New "Sign in with Google" button in Settings

Prerequisites

• APPLE_APP_IDS on flyfun-common server needs net.ro-z.FlyFunBrief added for Apple token exchange
• Google OAuth redirect flow (platform=ios&scheme=flyfunbrief) already supported by server-side regex

Test plan

• Test Google sign-in flow on device
• Test Apple sign-in still works
• Test session restore on app restart
• Test sign-out clears both providers
• Verify flyfunbrief://auth/callback deep link works

🤖 Generated with Claude Code

[claude]

Code Review

Three issues worth fixing before merging:

---

1. Token saved to Keychain before fetchUserInfo succeeds (bug)

AuthenticationService.swift, signInWithGoogle()

// Save JWT token  ← saved here
saveToKeychain(service: keychainServiceToken, account: "sessionToken", data: Data(token.utf8))

// Fetch user info from /auth/me  ← if this throws...
let user = try await fetchUserInfo(token: token)
// ...authUser is never written, but sessionToken is already in Keychain

If fetchUserInfo throws, the session token sits in Keychain with no matching authUser entry. On next launch, checkExistingSession loads from authUser (not sessionToken), so the session is silently lost and the orphaned token is never cleared.

Fix: move saveToKeychain for the token to after fetchUserInfo succeeds, or save both atomically.

---

2. Deprecated UIWindowScene.windows API (iOS 16+ deprecated, app requires iOS 18+)

AuthenticationService.swift, presentationAnchor

nonisolated func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor {
    guard let scene = UIApplication.shared.connectedScenes.first as? UIWindowScene,
          let window = scene.windows.first else {   // ← deprecated iOS 16+

UIWindowScene.windows is deprecated since iOS 16. Since this app targets iOS 18+, use scene.keyWindow instead:

let window = scene.keyWindow ?? scene.windows.first

---

3. isLoading spinner on Google button fires during Apple sign-in too

SettingsView.swift

if appState?.auth.isLoading == true {
    ProgressView()

isLoading is a single flag on AuthenticationService set by both handleAppleAuthorization and signInWithGoogle. If the user is mid–Apple sign-in, the Google button will also show a spinner. Consider tracking loading state per provider, or simply removing the spinner from the Google button (the web browser session makes it obvious that something is happening).

---

Minor note

AuthError.cancelled is never thrown by the service — the UI catches ASWebAuthenticationSessionError.canceledLogin directly. The case is unused; either throw it from signInWithGoogle or remove it.

[claude]

Code Review

Bug: handleAuthCallback is unreachable dead code

ASWebAuthenticationSession with .customScheme intercepts the redirect URL and delivers it directly to the session completion handler — it never reaches the app onOpenURL. The url.host == auth block in FlyFunBriefApp.swift is never triggered for this flow, and handleAuthCallback is never called. The full auth response is already handled inside signInWithGoogle() via withCheckedThrowingContinuation. Both the URL routing in FlyFunBriefApp.swift and handleAuthCallback should be removed.

Bug: handleAuthCallback orphans a Keychain token on error

handleAuthCallback saves the JWT to Keychain BEFORE calling fetchUserInfo. If fetchUserInfo throws, the token stays in Keychain while isAuthenticated is false and currentUser is nil — inconsistent state. signInWithGoogle() correctly does fetch-then-persist with an explicit comment explaining why. handleAuthCallback should follow the same pattern.

Bug: fetchUserInfo hardcodes .google provider

fetchUserInfo returns AuthUser with provider .google unconditionally, despite hitting a shared /auth/me endpoint. Provider should be a parameter or derived from the response.

Design issue: authError is now publicly mutable

authError changed from private(set) var to var. SettingsView now sets appState?.auth.authError = error.localizedDescription directly, bypassing the service. Error state should flow through the service — keep authError as private(set) and let signInWithGoogle() throws propagate to callers.

Minor: authSession not cleared after completion

After ASWebAuthenticationSession completes, self.authSession is never set back to nil, retaining a reference to the completed session until the next sign-in. Set self.authSession = nil after the continuation resumes.

[claude]

Code Review

Three issues found, all in AuthenticationService.swift.

---

1. nonisolated presentationAnchor accesses @MainActor-isolated APIs

nonisolated func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor {
    guard let scene = UIApplication.shared.connectedScenes.first as? UIWindowScene,
          let window = scene.keyWindow else { ...

UIApplication.shared.connectedScenes and keyWindow are @MainActor-isolated. Accessing them from a nonisolated context is unsound under Swift 6 strict concurrency and will be a compiler error. The standard fix is MainActor.assumeIsolated { } (safe here since ASWebAuthenticationSession always calls this on the main thread):

nonisolated func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor {
    MainActor.assumeIsolated {
        guard let scene = UIApplication.shared.connectedScenes.first as? UIWindowScene,
              let window = scene.windows.first(where: \.isKeyWindow) else {
            return ASPresentationAnchor()
        }
        return window
    }
}

---

2. catch is ASWebAuthenticationSessionError is too broad

} catch is ASWebAuthenticationSessionError {
    // User cancelled the browser — not an error
    return
}

This silently swallows all ASWebAuthenticationSession errors, including .presentationContextNotProvided (a programmer bug) and .presentationContextInvalid. Only .canceledLogin should be silent:

} catch let error as ASWebAuthenticationSessionError where error.code == .canceledLogin {
    return
}

---

3. Empty userId fallback in fetchUserInfo should throw

let userId = json["id"] as? String ?? ""

If the server omits id (unexpected response shape), an AuthUser with userId: "" is stored in Keychain. Subsequent credential checks and backend calls using this empty ID will silently misbehave. Should throw instead:

guard let userId = json["id"] as? String, !userId.isEmpty else {
    throw AuthError.invalidResponse
}

---

Overall the Google OAuth flow design (OAuth via ASWebAuthenticationSession, JWT stored in Keychain, user info from /auth/me, provider-aware credential state checks) is solid and consistent with the existing Apple Sign-In pattern.

🤖 Generated with Claude Code

[claude]

Code Review

Three issues worth addressing before merge:

---

1. Silent Keychain Migration Break (Bug)

The Keychain account key changed from "appleUser" to "authUser" in checkExistingSession and the save paths. Existing Apple users upgrading will find "authUser" empty, silently fail to restore their session, and be shown as signed out with no explanation.

Either keep loading from "appleUser" as a fallback, or add a one-time migration step that reads the old key and writes to the new one.

// checkExistingSession — existing Apple sessions won't be found
if let userData = loadFromKeychain(service: keychainServiceUser, account: "authUser"), ...

---

2. fetchUserInfo Uses JSONSerialization Instead of Codable (Inconsistency / Maintainability)

AuthUser is already Codable, and the rest of the service uses JSONDecoder/JSONEncoder. Manually parsing via JSONSerialization is fragile and harder to maintain:

guard let json = try? JSONSerialization.jsonObject(with: data) as? [String: Any] else {
    throw AuthError.invalidResponse
}
guard let userId = json["id"] as? String, !userId.isEmpty else { ... }
let email = json["email"] as? String

Define a private Decodable struct for the /auth/me response and decode with JSONDecoder. This is consistent with the existing pattern and gives compile-time field safety.

---

3. No Token Validation on Google Session Restore (Logic Gap)

When restoring a Google session at startup, checkExistingSession sets isAuthenticated = true without verifying the JWT is still valid. Apple users get checkCredentialState(), but Google users get nothing — an expired token won't be detected until the first API call fails.

Consider calling fetchUserInfo(token:) on the restored token for Google sessions (and signing out on failure), to match the intent of checkCredentialState() for Apple.

if user.provider == .apple {
    await checkCredentialState()
}
// Google: no equivalent validation — expired tokens won't be caught here

[claude]

Code Review

Bug: Keychain migration will silently fail for existing Apple Sign-In users

In checkExistingSession, the migration reads legacy appleUser JSON (encoded from AppleUser) and tries to decode it directly as AuthUser:

userData = legacyData
// ...
let user = try? JSONDecoder().decode(AuthUser.self, from: userData)

The old AppleUser JSON has no provider field:

{"userId":"...", "email":"...", "firstName":"...", "lastName":"..."}

But AuthUser.provider: AuthProvider is a required non-optional field with no default value. JSONDecoder will fail silently (via try?), userData won't decode, and all existing Apple Sign-In users will be logged out on first launch after the update.

Fix: decode the legacy data with a transitional struct (or give AuthUser.provider a default of .apple via a custom init(from:)):

private struct LegacyAppleUser: Decodable {
    let userId: String; let email: String?
    let firstName: String?; let lastName: String?
}

// In migration block:
if let legacy = try? JSONDecoder().decode(LegacyAppleUser.self, from: legacyData) {
    let converted = AuthUser(userId: legacy.userId, email: legacy.email,
                             firstName: legacy.firstName, lastName: legacy.lastName, provider: .apple)
    if let reencoded = try? JSONEncoder().encode(converted) {
        userData = reencoded
        saveToKeychain(service: keychainServiceUser, account: "authUser", data: reencoded)
    }
    deleteFromKeychain(service: keychainServiceUser, account: "appleUser")
}

---

Everything else looks solid: the ASWebAuthenticationSession lifetime management (authSession retained on self), the MainActor.assumeIsolated pattern in presentationAnchor, the token-before-persist ordering in signInWithGoogle, and the checkCredentialState guard scoping to Apple-only users are all correct.

🤖 Generated with Claude Code

[claude]

Code Review

Overall this is a clean, well-structured addition. A few issues to address:

---

Bug: force unwrap crash in signInWithGoogle

AuthenticationService.swift -- signInWithGoogle:

If authURL from the secrets file is malformed, URLComponents(string: "\(authURL)/auth/login/google")! crashes instead of gracefully setting authError. Should guard and set authError = AuthError.invalidURL.localizedDescription on failure.

---

Bug: force unwrap in fetchUserInfo

let lastName = nameParts?.count ?? 0 > 1 ? String(nameParts![1]) : nil

The ! force unwrap is logically safe today because of the preceding count check, but it is fragile. Prefer: nameParts.flatMap { $0.count > 1 ? String($0[1]) : nil }

---

Logic gap: handleOpenURL logs error for auth-related deep links

The handler accepts any flyfunbrief:// URL but only understands ?path=. If flyfunbrief://auth/callback?token=... reaches onOpenURL (edge case during ASWebAuthenticationSession teardown or a misconfigured test), it logs "Invalid deep link - missing path parameter" -- a misleading error. The handler should return silently for paths it does not own, e.g. guard url.host == "import" else { return }.

---

Missing field: approved flag not checked from /auth/me

AuthMeResponse decodes id, email, name but not approved. Per AUTH_USAGE.md, /auth/me includes "approved": true. If the server returns 200 with approved: false for a pending user, the iOS app will authenticate them as if they have full access. Either decode and check this field, or confirm the server returns non-200 for unapproved users.

---

No architecture violations. Migration path for legacy appleUser keychain data is correct. The authSession property retention pattern for ASWebAuthenticationSession is right.

[claude]

Code Review

Bug: Google session authenticated before token validation

In checkExistingSession, for Google users isAuthenticated = true and onAuthChanged?(sessionToken) fire before fetchUserInfo validates the JWT:

self.currentUser = user
self.isAuthenticated = true
onAuthChanged?(sessionToken)          // BriefingService starts using the token

// ...only then validated:
_ = try await fetchUserInfo(token: token, provider: .google)
// catch: signOut()

If the stored token is expired, signOut() is called after observers have already received it, potentially triggering network requests with an invalid token. For Google, token liveness is the only validity check (unlike Apple where ASAuthorizationAppleIDProvider checks device-level credential state). The validation should happen before setting isAuthenticated / calling onAuthChanged.

Bug: No concurrent-call guard in signInWithGoogle

signInWithGoogle is async on @MainActor. Two concurrent calls can interleave at the await withCheckedThrowingContinuation suspension point, causing the second call to overwrite self.authSession before the first session receives its callback. The isLoading UI flag is the only protection but is not enforced at the service boundary. A guard at the top (guard !isLoading else { return }) would close this.

Minor: Removed error log silences unexpected deep links

The original Logger.app.error for a flyfunbrief:// URL missing path was removed with no replacement. While the comment explains auth callbacks are intercepted by ASWebAuthenticationSession, a malformed link or unexpected URL variant will now be silently dropped, making it harder to debug.