diff --git a/Chorus/Views/WebView/WebViewCoordinator.swift b/Chorus/Views/WebView/WebViewCoordinator.swift index de21d9b..df6a80e 100644 --- a/Chorus/Views/WebView/WebViewCoordinator.swift +++ b/Chorus/Views/WebView/WebViewCoordinator.swift @@ -13,6 +13,11 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK /// reload it once the sign-in popup closes (see reloadOpenerAfterPopup). private weak var openerWebView: WKWebView? + /// Whether the current popup was *opened at* a known sign-in gateway. Set + /// once, from the URL that opened it — see `shouldReloadOpener` for why the + /// rest of the navigation chain is deliberately not consulted. + private var popupOpenedAtAuthHost = false + /// Fallback URL to load if the WebContent process crashes before any /// navigation has committed (so `webView.reload()` has nothing to retry). var fallbackURL: URL? @@ -438,6 +443,11 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK // needs to reload to leave its signed-out page. openerWebView = webView + // The sign-in signal, taken from the URL that opened the popup and not + // touched again: a service asking the user to sign in again opens + // straight at its provider. See `shouldReloadOpener`. + popupOpenedAtAuthHost = navigationAction.request.url?.host.map(Self.isAuthHost) ?? false + // CRITICAL: Use the configuration passed in — it inherits the parent's data store let popup = WKWebView(frame: .zero, configuration: configuration) popup.navigationDelegate = self @@ -500,14 +510,56 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK @objc private func popupWindowWillClose(_ notification: Notification) { guard let window = notification.object as? NSWindow, window === popupWindow else { return } - reloadOpenerAfterPopup() + // Closed by the user (red button / ⌘W), not by the page. + reloadOpenerAfterPopup(selfClosed: false) cleanupPopup() } - /// After a sign-in / OAuth popup closes, the shared data store already holds - /// the new session cookies, but the service's main web view is still on its - /// signed-out page. Reload it so the user lands on the authenticated app. - private func reloadOpenerAfterPopup() { + /// Whether closing a popup should reload the service that opened it. + /// + /// Reloading exists for sign-in: the popup shares the service's data store, + /// so once sign-in finishes the session cookies are already here and the + /// main view only needs a reload to leave its signed-out page. + /// + /// Reloading *unconditionally* is wrong, because a popup is also how an + /// ordinary link opens. Glance at a link from a chat service, close the + /// window, and the service reloads underneath you — losing scroll position, + /// a half-typed message, and whatever else the page held but never sent. + /// That is a steady, visible cost paid for a case that comes up rarely. + /// + /// Two signals separate them, and either one is enough: + /// + /// - **The page closed itself.** OAuth popups finish by calling + /// `window.close()`; a link window is closed by the user. This is the + /// signal that carries flows through identity providers we don't list, + /// such as a company's own Okta or Keycloak. + /// - **The popup was opened at a known sign-in gateway.** A service asking + /// the user to sign in again opens straight at its provider, so the very + /// first URL is the gateway. This covers a flow the user closes by hand + /// once it is done, which some providers leave to them. + /// + /// The second signal reads the *opening* URL only, never the rest of the + /// navigation chain, and that distinction is the whole point. Plenty of + /// ordinary links pass through a sign-in gateway on their way somewhere + /// else: opening an Azure portal link from Teams starts at + /// `portal.azure.com` and redirects through `login.microsoftonline.com` for + /// SSO. Watching the chain counts that as a sign-in and reloads the service + /// — the exact bug this function exists to fix, measured happening. + /// + /// When neither signal holds, the popup was a link, and the service is left + /// alone. The failure mode this trades into is mild and recoverable: a + /// sign-in that neither starts at a listed gateway nor closes itself leaves + /// the service on its signed-out page until the user hits reload, once. + nonisolated static func shouldReloadOpener(selfClosed: Bool, openedAtAuthHost: Bool) -> Bool { + selfClosed || openedAtAuthHost + } + + /// Reloads the service that opened the popup, when the rule above says to. + private func reloadOpenerAfterPopup(selfClosed: Bool) { + guard Self.shouldReloadOpener( + selfClosed: selfClosed, + openedAtAuthHost: popupOpenedAtAuthHost + ) else { return } guard let opener = openerWebView else { return } if opener.url != nil { opener.reload() @@ -534,11 +586,16 @@ final class WebViewCoordinator: NSObject, WKNavigationDelegate, WKUIDelegate, WK // Give the next popup its own crash budget — a prior popup's tally must // not shorten the backoff for an unrelated sign-in opened soon after. popupCrashTimestamps = [] + // Likewise the sign-in signal: a link popup opened after a sign-in must + // not inherit its predecessor's reason to reload the service. + popupOpenedAtAuthHost = false } func webViewDidClose(_ webView: WKWebView) { if webView === popupWebView { - reloadOpenerAfterPopup() + // The page called window.close() on itself — the shape an OAuth + // popup takes when it finishes. + reloadOpenerAfterPopup(selfClosed: true) cleanupPopup() } } diff --git a/ChorusTests/ChorusTests.swift b/ChorusTests/ChorusTests.swift index 213e152..a4a24a5 100644 --- a/ChorusTests/ChorusTests.swift +++ b/ChorusTests/ChorusTests.swift @@ -1562,6 +1562,53 @@ final class ChorusTests: XCTestCase { XCTAssertFalse(ServiceInstance(label: "X", url: "https://x.test", forceDarkMode: false).isForceDarkModeEnabled) } + // MARK: - Reloading the opener after a popup closes + + func testUserClosingALinkPopupLeavesTheServiceAlone() { + // The regression this guards: glance at a link opened from a chat + // service, close the window, and the service used to reload underneath + // you — losing scroll position and anything typed but not sent. + XCTAssertFalse(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: false)) + } + + func testSelfClosingPopupReloadsTheService() { + // An OAuth popup finishes by calling window.close(). This is what + // carries sign-in through providers we don't list by name — a company's + // own Okta or Keycloak. + XCTAssertTrue(WebViewCoordinator.shouldReloadOpener( + selfClosed: true, openedAtAuthHost: false)) + } + + func testHandClosedSignInStillReloadsTheService() { + // A service asking the user to sign in again opens straight at its + // provider, so the opening URL is the gateway. Some providers leave the + // last click to the user, and that flow must still reload. + XCTAssertTrue(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: true)) + } + + func testLinkThatMerelyRedirectsThroughSSOLeavesTheServiceAlone() { + // Measured on a real machine: opening an Azure portal link from Teams + // starts at portal.azure.com and redirects through + // login.microsoftonline.com for SSO. Judging by the navigation chain + // counted that as a sign-in and reloaded Teams on close. Only the + // opening URL is consulted, so a link like this stays a link. + XCTAssertFalse(WebViewCoordinator.isAuthHost("portal.azure.com")) + XCTAssertFalse(WebViewCoordinator.shouldReloadOpener( + selfClosed: false, openedAtAuthHost: false)) + } + + func testKnownAuthGatewaysAreRecognisedIncludingSubdomains() { + XCTAssertTrue(WebViewCoordinator.isAuthHost("login.microsoftonline.com")) + XCTAssertTrue(WebViewCoordinator.isAuthHost("accounts.google.com")) + // A subdomain of a gateway still counts. + XCTAssertTrue(WebViewCoordinator.isAuthHost("eu.login.microsoftonline.com")) + // An ordinary link target does not. + XCTAssertFalse(WebViewCoordinator.isAuthHost("teams.cloud.microsoft")) + XCTAssertFalse(WebViewCoordinator.isAuthHost("example.com")) + } + // MARK: - Link routing (belongsToService) func testBelongsToServiceKeepsSlackWorkspacesInApp() {