From e989463d91dbb74abab63f29479d8aff2adaf291 Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Tue, 5 May 2026 12:47:07 +0100 Subject: [PATCH] Restore the .oidcCallback route for external authentication. (#5391) * Restore the .oidcCallback route (partially reverts #3461) for external authentication. * Make sure OIDC also works for non-http URLs. * Remove oidcAuthentication from the state machine. There isn't a reliable way to detect failure/cancellation when e.g. the user returns from an external app without interacting with the MAS page. --- .../Sources/Application/AppCoordinator.swift | 7 ++ .../Application/Navigation/AppRoutes.swift | 17 ++++- .../Application/Settings/AppSettings.swift | 5 +- .../AuthenticationFlowCoordinator.swift | 46 ++++++------ .../ChatsTabFlowCoordinator.swift | 3 +- .../EncryptionResetFlowCoordinator.swift | 4 + .../EncryptionSettingsFlowCoordinator.swift | 4 +- .../OnboardingFlowCoordinator.swift | 3 + .../RoomFlowCoordinator.swift | 2 +- .../RoomMembersFlowCoordinator.swift | 3 +- .../SettingsFlowCoordinator.swift | 1 + .../UserSessionFlowCoordinator.swift | 4 +- .../OIDCAuthenticationPresenter.swift | 74 +++++++++++++++---- .../SoftLogoutScreenCoordinator.swift | 11 +++ .../OIDCAccountSettingsPresenter.swift | 19 ++++- .../UITests/UITestsAppCoordinator.swift | 1 + .../Sources/AppRouteURLParserTests.swift | 29 ++++++++ 17 files changed, 181 insertions(+), 52 deletions(-) diff --git a/ElementX/Sources/Application/AppCoordinator.swift b/ElementX/Sources/Application/AppCoordinator.swift index 622295c47..212288fab 100644 --- a/ElementX/Sources/Application/AppCoordinator.swift +++ b/ElementX/Sources/Application/AppCoordinator.swift @@ -276,6 +276,12 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg case .accountProvisioningLink: handleAppRoute(route, windowType: windowType) + case .oidcCallback(let url): + if stateMachine.state == .softLogout { + softLogoutCoordinator?.handleOIDCRedirectURL(url) + } else { + authenticationFlowCoordinator?.handleOIDCRedirectURL(url) + } case .userProfile(let userID): if isExternalURL { handleAppRoute(route, @@ -712,6 +718,7 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg let parameters = SoftLogoutScreenCoordinatorParameters(authenticationService: authenticationService, credentials: credentials, keyBackupNeeded: false, + appMediator: appMediator, appSettings: appSettings, userIndicatorController: ServiceLocator.shared.userIndicatorController) let coordinator = SoftLogoutScreenCoordinator(parameters: parameters) diff --git a/ElementX/Sources/Application/Navigation/AppRoutes.swift b/ElementX/Sources/Application/Navigation/AppRoutes.swift index b2e11985c..b9734d720 100644 --- a/ElementX/Sources/Application/Navigation/AppRoutes.swift +++ b/ElementX/Sources/Application/Navigation/AppRoutes.swift @@ -14,6 +14,9 @@ import MatrixRustSDK enum AppRoute: Hashable { /// An account provisioning link generated externally. case accountProvisioningLink(AccountProvisioningParameters) + /// An external callback used to complete login with OIDC. This is only used when authentication + /// requires an external app so cannot be done within the built in web authentication session. + case oidcCallback(url: URL) /// The app's home screen. case roomList @@ -58,6 +61,7 @@ enum AppRoute: Hashable { var isAuthenticationRoute: Bool { switch self { case .accountProvisioningLink: true + case .oidcCallback: true default: false } } @@ -82,7 +86,8 @@ struct AppRouteURLParser { AppGroupURLParser(), MatrixPermalinkParser(), ElementWebURLParser(domains: appSettings.elementWebHosts), - AccountProvisioningURLParser(domain: appSettings.accountProvisioningHost) + AccountProvisioningURLParser(domain: appSettings.accountProvisioningHost), + OIDCCallbackURLParser(redirectURL: appSettings.oidcRedirectURL) ] } @@ -198,3 +203,13 @@ private struct AccountProvisioningURLParser: URLParser { return .accountProvisioningLink(.init(accountProvider: serverName, loginHint: loginHint)) } } + +/// The parser for the OIDC callback URL. This always returns a `.oidcCallback`. +struct OIDCCallbackURLParser: URLParser { + let redirectURL: URL + + func route(from url: URL) -> AppRoute? { + guard url.absoluteString.starts(with: redirectURL.absoluteString) else { return nil } + return .oidcCallback(url: url) + } +} diff --git a/ElementX/Sources/Application/Settings/AppSettings.swift b/ElementX/Sources/Application/Settings/AppSettings.swift index cced885e8..76423c0f5 100644 --- a/ElementX/Sources/Application/Settings/AppSettings.swift +++ b/ElementX/Sources/Application/Settings/AppSettings.swift @@ -251,8 +251,9 @@ final class AppSettings { /// Any pre-defined static client registrations for OIDC issuers. let oidcStaticRegistrations: [URL: String] = ["https://id.thirdroom.io/realms/thirdroom": "elementx"] - /// The redirect URL used for OIDC. This no longer uses universal links so we don't need the bundle ID to avoid conflicts between Element X, Nightly and PR builds. - private(set) var oidcRedirectURL: URL = "https://element.io/oidc/login" + /// The redirect URL used for OIDC. For the normal case we don't actually need the bundle ID as the web authentication session handles the redirect internally. + /// However in the case where MAS sends the user to an external app, we need to make sure that the system will open the correct variant of the app (e.g. Nightly). + private(set) var oidcRedirectURL: URL! = URL(string: "https://element.io/oauth/ios/\(InfoPlistReader.main.bundleIdentifier)") private(set) lazy var oidcConfiguration = OIDCConfiguration(clientName: InfoPlistReader.main.bundleDisplayName, redirectURI: oidcRedirectURL, diff --git a/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift index 1942d469b..75d6e7e16 100644 --- a/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift @@ -40,8 +40,6 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { case serverConfirmationScreen /// The screen to choose a different server. case serverSelectionScreen - /// The web authentication session is being presented. - case oidcAuthentication /// The screen to login with a password. case loginScreen @@ -76,10 +74,6 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { /// The user is no longer selecting a server. case dismissedServerSelection - /// Show the web authentication session for OIDC (using the parameters in the `userInfo`). - case continueWithOIDC - /// The web authentication session was aborted. - case cancelledOIDCAuthentication(previousState: State) /// Show the screen to login with password (with the optional login hint in the `userInfo`). case continueWithPassword /// The password login was aborted. @@ -157,6 +151,8 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { } func clearRoute(animated: Bool) { + oidcPresenter?.cancel() // Handle ongoing OIDC authentication first. + switch stateMachine.state { case .initial, .startScreen: break @@ -168,9 +164,6 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { case .serverSelectionScreen: navigationStackCoordinator.setSheetCoordinator(nil) navigationStackCoordinator.popToRoot(animated: animated) - case .oidcAuthentication: - oidcPresenter?.cancel() - navigationStackCoordinator.popToRoot(animated: animated) case .loginScreen: navigationStackCoordinator.popToRoot(animated: animated) case .bugReportFlow: @@ -182,6 +175,15 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { } } + func handleOIDCRedirectURL(_ url: URL) { + guard let oidcPresenter else { + MXLog.error("Failed to find an OIDC request in progress.") + return + } + + oidcPresenter.handleUniversalLinkCallback(url) + } + // MARK: - Setup private func configureStateMachine() { @@ -220,16 +222,6 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { } stateMachine.addRoutes(event: .dismissedServerSelection, transitions: [.serverSelectionScreen => .serverConfirmationScreen]) - stateMachine.addRoutes(event: .continueWithOIDC, transitions: [.serverConfirmationScreen => .oidcAuthentication, - .startScreen => .oidcAuthentication]) { [weak self] context in - guard let (oidcData, window) = context.userInfo as? (OIDCAuthorizationDataProxy, UIWindow) else { - fatalError("Missing the OIDC data and presentation anchor.") - } - self?.showOIDCAuthentication(oidcData: oidcData, presentationAnchor: window, fromState: context.fromState) - } - stateMachine.addRoutes(event: .cancelledOIDCAuthentication(previousState: .serverConfirmationScreen), transitions: [.oidcAuthentication => .serverConfirmationScreen]) - stateMachine.addRoutes(event: .cancelledOIDCAuthentication(previousState: .startScreen), transitions: [.oidcAuthentication => .startScreen]) - stateMachine.addRoutes(event: .continueWithPassword, transitions: [.serverConfirmationScreen => .loginScreen, .startScreen => .loginScreen]) { [weak self] context in let loginHint = context.userInfo as? String @@ -255,7 +247,8 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { // Completion stateMachine.addRoutes(event: .signedIn, transitions: [.qrCodeLoginScreen => .complete, - .oidcAuthentication => .complete, + .serverConfirmationScreen => .complete, // OIDC authentication + .startScreen => .complete, // Direct OIDC authentication .loginScreen => .complete]) { [weak self] context in guard let userSession = context.userInfo as? UserSessionProtocol else { fatalError("The user session wasn't included in the context") } self?.userHasSignedIn(userSession: userSession) @@ -308,7 +301,7 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { stateMachine.tryEvent(.confirmServer(.register)) case .loginDirectlyWithOIDC(let oidcData, let window): - stateMachine.tryEvent(.continueWithOIDC, userInfo: (oidcData, window)) + showOIDCAuthentication(oidcData: oidcData, presentationAnchor: window) case .loginDirectlyWithPassword(let loginHint): stateMachine.tryEvent(.continueWithPassword, userInfo: loginHint) @@ -382,7 +375,7 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { switch action { case .continueWithOIDC(let oidcData, let window): - stateMachine.tryEvent(.continueWithOIDC, userInfo: (oidcData, window)) + showOIDCAuthentication(oidcData: oidcData, presentationAnchor: window) case .continueWithPassword: stateMachine.tryEvent(.continueWithPassword) case .changeServer: @@ -424,10 +417,14 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { } } - private func showOIDCAuthentication(oidcData: OIDCAuthorizationDataProxy, presentationAnchor: UIWindow, fromState: State) { + /// **Note:** We have intentionally excluded this presentation from the state machine as it doesn't mutate our navigation stack and there + /// isn't a robust way to detect why the user returned to the app when the MAS URL directly opens an external app for authentication without + /// presenting a web authentication session. + private func showOIDCAuthentication(oidcData: OIDCAuthorizationDataProxy, presentationAnchor: UIWindow) { let presenter = OIDCAuthenticationPresenter(authenticationService: authenticationService, oidcRedirectURL: appSettings.oidcRedirectURL, presentationAnchor: presentationAnchor, + appMediator: appMediator, userIndicatorController: userIndicatorController) oidcPresenter = presenter @@ -436,8 +433,7 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { case .success(let userSession): stateMachine.tryEvent(.signedIn, userInfo: userSession) case .failure: - stateMachine.tryEvent(.cancelledOIDCAuthentication(previousState: fromState)) - // Nothing more to do, the alerts are handled by the presenter. + break // Nothing to do, any alerts will be handled by the presenter. } oidcPresenter = nil } diff --git a/ElementX/Sources/FlowCoordinators/ChatsTabFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/ChatsTabFlowCoordinator.swift index 1480e6d18..175c64693 100644 --- a/ElementX/Sources/FlowCoordinators/ChatsTabFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/ChatsTabFlowCoordinator.swift @@ -174,7 +174,7 @@ class ChatsTabFlowCoordinator: FlowCoordinatorProtocol { } case .globalSearch: presentGlobalSearch() - case .accountProvisioningLink, .settings, .chatBackupSettings, .call: + case .accountProvisioningLink, .oidcCallback, .settings, .chatBackupSettings, .call: break // These routes cannot be handled. } } @@ -690,6 +690,7 @@ class ChatsTabFlowCoordinator: FlowCoordinatorProtocol { private func startEncryptionResetFlow(animated: Bool) { let sheetNavigationStackCoordinator = NavigationStackCoordinator() let parameters = EncryptionResetFlowCoordinatorParameters(userSession: userSession, + appMediator: flowParameters.appMediator, appSettings: flowParameters.appSettings, userIndicatorController: flowParameters.userIndicatorController, navigationStackCoordinator: sheetNavigationStackCoordinator, diff --git a/ElementX/Sources/FlowCoordinators/EncryptionResetFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/EncryptionResetFlowCoordinator.swift index 1e9b57112..b3fc7bf93 100644 --- a/ElementX/Sources/FlowCoordinators/EncryptionResetFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/EncryptionResetFlowCoordinator.swift @@ -19,6 +19,7 @@ enum EncryptionResetFlowCoordinatorAction: Equatable { struct EncryptionResetFlowCoordinatorParameters { let userSession: UserSessionProtocol + let appMediator: AppMediatorProtocol let appSettings: AppSettings let userIndicatorController: UserIndicatorControllerProtocol let navigationStackCoordinator: NavigationStackCoordinator @@ -27,6 +28,7 @@ struct EncryptionResetFlowCoordinatorParameters { class EncryptionResetFlowCoordinator: FlowCoordinatorProtocol { private let userSession: UserSessionProtocol + private let appMediator: AppMediatorProtocol private let appSettings: AppSettings private let userIndicatorController: UserIndicatorControllerProtocol @@ -62,6 +64,7 @@ class EncryptionResetFlowCoordinator: FlowCoordinatorProtocol { init(parameters: EncryptionResetFlowCoordinatorParameters) { userSession = parameters.userSession + appMediator = parameters.appMediator appSettings = parameters.appSettings userIndicatorController = parameters.userIndicatorController navigationStackCoordinator = parameters.navigationStackCoordinator @@ -158,6 +161,7 @@ class EncryptionResetFlowCoordinator: FlowCoordinatorProtocol { // As of iOS 16, there is an issue on the simulator with accessing the cookie but it works on a device. 🤷‍♂️ accountSettingsPresenter = OIDCAccountSettingsPresenter(accountURL: url, presentationAnchor: windowManager.mainWindow, + appMediator: appMediator, appSettings: appSettings) accountSettingsPresenter?.start() } diff --git a/ElementX/Sources/FlowCoordinators/EncryptionSettingsFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/EncryptionSettingsFlowCoordinator.swift index e24771e93..889b46688 100644 --- a/ElementX/Sources/FlowCoordinators/EncryptionSettingsFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/EncryptionSettingsFlowCoordinator.swift @@ -80,8 +80,8 @@ class EncryptionSettingsFlowCoordinator: FlowCoordinatorProtocol { MXLog.info("Handling app route: \(appRoute)") switch appRoute { - case .accountProvisioningLink: - break // We always ignore this flow when logged in. + case .accountProvisioningLink, .oidcCallback: + break // We always ignore these flows when logged in. case .roomList, .room, .roomAlias, .childRoom, .childRoomAlias, .roomDetails, .roomMemberDetails, .userProfile, .thread, .event, .eventOnRoomAlias, .childEvent, .childEventOnRoomAlias, diff --git a/ElementX/Sources/FlowCoordinators/OnboardingFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/OnboardingFlowCoordinator.swift index b2fe73e87..2398423aa 100644 --- a/ElementX/Sources/FlowCoordinators/OnboardingFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/OnboardingFlowCoordinator.swift @@ -20,6 +20,7 @@ class OnboardingFlowCoordinator: FlowCoordinatorProtocol { private let userSession: UserSessionProtocol private let appLockService: AppLockServiceProtocol private let analyticsService: AnalyticsService + private let appMediator: AppMediatorProtocol private let appSettings: AppSettings private let notificationManager: NotificationManagerProtocol private let userIndicatorController: UserIndicatorControllerProtocol @@ -66,6 +67,7 @@ class OnboardingFlowCoordinator: FlowCoordinatorProtocol { userSession = flowParameters.userSession self.appLockService = appLockService analyticsService = flowParameters.analytics + appMediator = flowParameters.appMediator appSettings = flowParameters.appSettings notificationManager = flowParameters.notificationManager userIndicatorController = flowParameters.userIndicatorController @@ -315,6 +317,7 @@ class OnboardingFlowCoordinator: FlowCoordinatorProtocol { private func startEncryptionResetFlow() { let resetNavigationStackCoordinator = NavigationStackCoordinator() let coordinator = EncryptionResetFlowCoordinator(parameters: .init(userSession: userSession, + appMediator: appMediator, appSettings: appSettings, userIndicatorController: userIndicatorController, navigationStackCoordinator: resetNavigationStackCoordinator, diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 8c5abaf98..daf5b1f43 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -201,7 +201,7 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { } case .roomAlias, .childRoomAlias, .eventOnRoomAlias, .childEventOnRoomAlias: break // These are converted to a room ID route one level above. - case .accountProvisioningLink, .roomList, .userProfile, .call, .settings, .chatBackupSettings, .globalSearch: + case .accountProvisioningLink, .oidcCallback, .roomList, .userProfile, .call, .settings, .chatBackupSettings, .globalSearch: break // These routes can't be handled. case .transferOwnership(let roomID): guard self.roomID == roomID else { fatalError("Navigation route doesn't belong to this room flow.") } diff --git a/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift index 8156c7e0d..cd240760c 100644 --- a/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomMembersFlowCoordinator.swift @@ -120,7 +120,8 @@ final class RoomMembersFlowCoordinator: FlowCoordinatorProtocol { } case .roomAlias, .childRoomAlias, .eventOnRoomAlias, .childEventOnRoomAlias: break // These are converted to a room ID route one level above. - case .accountProvisioningLink, .roomList, .room, .roomDetails, .event, + case .accountProvisioningLink, .oidcCallback, + .roomList, .room, .roomDetails, .event, .userProfile, .call, .settings, .chatBackupSettings, .share, .transferOwnership, .thread, .globalSearch: break diff --git a/ElementX/Sources/FlowCoordinators/SettingsFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/SettingsFlowCoordinator.swift index c8ee16cb9..3d0a3d94a 100644 --- a/ElementX/Sources/FlowCoordinators/SettingsFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/SettingsFlowCoordinator.swift @@ -303,6 +303,7 @@ class SettingsFlowCoordinator: FlowCoordinatorProtocol { // As of iOS 16, there is an issue on the simulator with accessing the cookie but it works on a device. 🤷‍♂️ accountSettingsPresenter = OIDCAccountSettingsPresenter(accountURL: url, presentationAnchor: flowParameters.windowManager.mainWindow, + appMediator: flowParameters.appMediator, appSettings: flowParameters.appSettings, continuation: continuation) accountSettingsPresenter?.start() diff --git a/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift index bece7ad7e..4cfb22cf4 100644 --- a/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift @@ -121,8 +121,8 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol { MXLog.info("Handling app route: \(appRoute)") switch appRoute { - case .accountProvisioningLink: - break // We always ignore this flow when logged in. + case .accountProvisioningLink, .oidcCallback: + break // We always ignore these flows when logged in. case .settings, .chatBackupSettings: if ProcessInfo.processInfo.isiOSAppOnMac, flowParameters.windowManager.secondaryWindowsEnabled { startSettingsFlow(detached: true) diff --git a/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift b/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift index 3ca623249..9769198ca 100644 --- a/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift +++ b/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift @@ -9,31 +9,57 @@ import AuthenticationServices /// Presents a web authentication session for an OIDC request. +/// +/// In certain instances the URL may require opening an external app instead of using a WAS. Because of this +/// it is recommended to not encode the OIDC authentication within any state machines, as there is no guarantee +/// that any cancellations/failures will be communicated upwards. @MainActor class OIDCAuthenticationPresenter: NSObject { private let authenticationService: AuthenticationServiceProtocol private let oidcRedirectURL: URL private let presentationAnchor: UIWindow + private let appMediator: AppMediatorProtocol private let userIndicatorController: UserIndicatorControllerProtocol - private var activeSession: ASWebAuthenticationSession? + /// The data required to complete a request. + struct Request { + let session: ASWebAuthenticationSession + let continuation: CheckedContinuation + } + + struct Response { + let url: URL? + let isExternal: Bool + let error: Error? + } + + private var activeRequest: Request? init(authenticationService: AuthenticationServiceProtocol, oidcRedirectURL: URL, presentationAnchor: UIWindow, + appMediator: AppMediatorProtocol, userIndicatorController: UserIndicatorControllerProtocol) { self.authenticationService = authenticationService self.oidcRedirectURL = oidcRedirectURL self.presentationAnchor = presentationAnchor + self.appMediator = appMediator self.userIndicatorController = userIndicatorController super.init() } /// Presents a web authentication session for the supplied data. + /// + /// **Note:** The failure case cannot be relied upon as a signal that the authentication has ended. + /// In particular if the authentication URL requires opening an external app, then the user may return + /// to the app without completing (or cancelling) the authentication. func authenticate(using oidcData: OIDCAuthorizationDataProxy) async -> Result { - let (url, error) = await withCheckedContinuation { continuation in - let session = ASWebAuthenticationSession(url: oidcData.url, callback: .oidcRedirectURL(oidcRedirectURL)) { url, error in - continuation.resume(returning: (url, error)) + let response = await withCheckedContinuation { continuation in + let authenticationURL = oidcData.url + + let session = ASWebAuthenticationSession(url: authenticationURL, callback: .oidcRedirectURL(oidcRedirectURL)) { url, error in + MXLog.info("Handling callback from the session.") + continuation.resume(returning: Response(url: url, isExternal: false, error: error)) } session.prefersEphemeralWebBrowserSession = false @@ -42,21 +68,31 @@ class OIDCAuthenticationPresenter: NSObject { "X-Element-User-Agent": UserAgentBuilder.makeASCIIUserAgent() ] - activeSession = session - session.start() + activeRequest = Request(session: session, continuation: continuation) + + if authenticationURL.scheme == "https" || authenticationURL.scheme == "http" { + session.start() + } else { + appMediator.open(authenticationURL) + } } - activeSession = nil + if response.isExternal { + // Manually dismiss the web authentication session if the login was completed outside of the app. + // Note: This doesn't trigger a callback so no need to worry about the continuation being called twice. + activeRequest?.session.cancel() + } + activeRequest = nil - guard let url else { - // Check for user cancellation to avoid showing an alert in that instance. - if error?.isOIDCUserCancellation == true { + guard let url = response.url else { + // Check for user cancellation (on the WAS sheet) to avoid showing an alert in that instance. + if response.error?.isOIDCUserCancellation == true { // No need to show an error here, just abort and return a failure. await authenticationService.abortOIDCLogin(data: oidcData) return .failure(.oidcError(.userCancellation)) } - let errorDescription = error.map(String.init(describing:)) ?? "Unknown error" + let errorDescription = response.error.map(String.init(describing:)) ?? "Unknown error" MXLog.error("Missing callback URL from the web authentication session: \(errorDescription)") showFailureIndicator() @@ -71,7 +107,7 @@ class OIDCAuthenticationPresenter: NSObject { switch await authenticationService.loginWithOIDCCallback(url) { case .success(let userSession): return .success(userSession) - case .failure(.oidcError(.userCancellation)): + case .failure(.oidcError(.userCancellation)): // Check for user cancellation (on the MAS web page) // No need to show an error here, just return the failure. return .failure(.oidcError(.userCancellation)) case .failure(let error): @@ -81,8 +117,20 @@ class OIDCAuthenticationPresenter: NSObject { } } + /// This method will be used when the web authentication session redirects the user to an external + /// authentication app. During normal use the redirect is handled automatically by the session's closure. + func handleUniversalLinkCallback(_ url: URL) { + guard let activeRequest else { + MXLog.error("Failed to handle universal link callback. Missing request.") + return + } + MXLog.info("Handling callback from a universal link.") + activeRequest.continuation.resume(returning: Response(url: url, isExternal: true, error: nil)) + } + func cancel() { - activeSession?.cancel() + activeRequest?.session.cancel() + activeRequest = nil // Programatically cancelling the session doesn't trigger a callback. } private var loadingIndicatorID: String { diff --git a/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift b/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift index d2916eb00..8acafca00 100644 --- a/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift +++ b/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift @@ -13,6 +13,7 @@ struct SoftLogoutScreenCoordinatorParameters { let authenticationService: AuthenticationServiceProtocol let credentials: SoftLogoutScreenCredentials let keyBackupNeeded: Bool + let appMediator: AppMediatorProtocol let appSettings: AppSettings let userIndicatorController: UserIndicatorControllerProtocol } @@ -90,6 +91,15 @@ final class SoftLogoutScreenCoordinator: CoordinatorProtocol { AnyView(SoftLogoutScreen(context: viewModel.context)) } + func handleOIDCRedirectURL(_ url: URL) { + guard let oidcPresenter else { + MXLog.error("Failed to find an OIDC request in progress.") + return + } + + oidcPresenter.handleUniversalLinkCallback(url) + } + // MARK: - Private private static let loadingIndicatorIdentifier = "\(SoftLogoutScreenCoordinator.self)-Loading" @@ -149,6 +159,7 @@ final class SoftLogoutScreenCoordinator: CoordinatorProtocol { let presenter = OIDCAuthenticationPresenter(authenticationService: parameters.authenticationService, oidcRedirectURL: parameters.appSettings.oidcRedirectURL, presentationAnchor: presentationAnchor, + appMediator: parameters.appMediator, userIndicatorController: parameters.userIndicatorController) self.oidcPresenter = presenter switch await presenter.authenticate(using: oidcData) { diff --git a/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift b/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift index e706c27d5..336f5951d 100644 --- a/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift +++ b/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift @@ -17,17 +17,24 @@ import AuthenticationServices @MainActor class OIDCAccountSettingsPresenter: NSObject { private let accountURL: URL - private let presentationAnchor: UIWindow private let oidcRedirectURL: URL + private let presentationAnchor: UIWindow + private let appMediator: AppMediatorProtocol typealias Continuation = AsyncStream>.Continuation private let continuation: Continuation? - init(accountURL: URL, presentationAnchor: UIWindow, appSettings: AppSettings, continuation: Continuation? = nil) { + init(accountURL: URL, + presentationAnchor: UIWindow, + appMediator: AppMediatorProtocol, + appSettings: AppSettings, + continuation: Continuation? = nil) { self.accountURL = accountURL - self.presentationAnchor = presentationAnchor oidcRedirectURL = appSettings.oidcRedirectURL + self.presentationAnchor = presentationAnchor + self.appMediator = appMediator self.continuation = continuation + super.init() } @@ -53,7 +60,11 @@ class OIDCAccountSettingsPresenter: NSObject { "X-Element-User-Agent": UserAgentBuilder.makeASCIIUserAgent() ] - session.start() + if accountURL.scheme == "https" || accountURL.scheme == "http" { + session.start() + } else { + appMediator.open(accountURL) + } } } diff --git a/ElementX/Sources/UITests/UITestsAppCoordinator.swift b/ElementX/Sources/UITests/UITestsAppCoordinator.swift index 51fe5bb43..2cac38e38 100644 --- a/ElementX/Sources/UITests/UITestsAppCoordinator.swift +++ b/ElementX/Sources/UITests/UITestsAppCoordinator.swift @@ -735,6 +735,7 @@ class MockScreen: Identifiable { let navigationStackCoordinator = NavigationStackCoordinator() let coordinator = EncryptionResetFlowCoordinator(parameters: .init(userSession: userSession, + appMediator: AppMediatorMock.default, appSettings: ServiceLocator.shared.settings, userIndicatorController: userIndicatorController, navigationStackCoordinator: navigationStackCoordinator, diff --git a/UnitTests/Sources/AppRouteURLParserTests.swift b/UnitTests/Sources/AppRouteURLParserTests.swift index f90a26de6..7cebea9f3 100644 --- a/UnitTests/Sources/AppRouteURLParserTests.swift +++ b/UnitTests/Sources/AppRouteURLParserTests.swift @@ -20,6 +20,35 @@ struct AppRouteURLParserTests { appRouteURLParser = AppRouteURLParser(appSettings: appSettings) } + @Test + func oidcCallbackRoute() { + // Given an OIDC callback for this app. + let callbackURL = appSettings.oidcRedirectURL.appending(queryItems: [URLQueryItem(name: "state", value: "12345"), + URLQueryItem(name: "code", value: "67890")]) + + // When parsing that route. + let route = appRouteURLParser.route(from: callbackURL) + + // Then it should be considered a valid OIDC callback. + #expect(route == .oidcCallback(url: callbackURL)) + } + + @Test + func oidcCallbackAppVariantRoute() { + // Given an OIDC callback for a different app variant. + let callbackURL = appSettings.oidcRedirectURL + .deletingLastPathComponent() + .appending(component: "io.element.elementz") + .appending(queryItems: [URLQueryItem(name: "state", value: "12345"), + URLQueryItem(name: "code", value: "67890")]) + + // When parsing that route in this app. + let route = appRouteURLParser.route(from: callbackURL) + + // Then the route shouldn't be considered valid and should be ignored. + #expect(route == nil) + } + @Test func matrixUserURL() throws { let userID = "@test:matrix.org"