From 589df7d76edaebdd69958e49430c2fbe3369841b Mon Sep 17 00:00:00 2001 From: Doug <6060466+pixlwave@users.noreply.github.com> Date: Tue, 29 Oct 2024 11:28:05 +0000 Subject: [PATCH] Use an https callback for OIDC once again. (#3461) * Use the new WAS callback type and return back to the https callback for OIDC. * Simplify OIDCAuthenticationPresenter now it doesn't need to handle universal links. * Remove old unit tests. --- .../Sources/Application/AppCoordinator.swift | 6 -- .../Sources/Application/AppSettings.swift | 10 +- .../Application/Navigation/AppRoutes.swift | 13 --- .../AuthenticationFlowCoordinator.swift | 9 -- .../RoomFlowCoordinator.swift | 2 +- .../UserSessionFlowCoordinator.swift | 2 - .../OIDCAuthenticationPresenter.swift | 97 ++++++------------- .../SoftLogoutScreenCoordinator.swift | 9 -- .../OIDCAccountSettingsPresenter.swift | 2 +- .../Sources/AppRouteURLParserTests.swift | 27 ------ 10 files changed, 34 insertions(+), 143 deletions(-) diff --git a/ElementX/Sources/Application/AppCoordinator.swift b/ElementX/Sources/Application/AppCoordinator.swift index 1b878c785..b46a3aad5 100644 --- a/ElementX/Sources/Application/AppCoordinator.swift +++ b/ElementX/Sources/Application/AppCoordinator.swift @@ -195,12 +195,6 @@ class AppCoordinator: AppCoordinatorProtocol, AuthenticationFlowCoordinatorDeleg if let route = appRouteURLParser.route(from: url) { switch route { - case .oidcCallback(let url): - if stateMachine.state == .softLogout { - softLogoutCoordinator?.handleOIDCRedirectURL(url) - } else { - authenticationFlowCoordinator?.handleOIDCRedirectURL(url) - } case .genericCallLink(let url): if let userSessionFlowCoordinator { userSessionFlowCoordinator.handleAppRoute(route, animated: true) diff --git a/ElementX/Sources/Application/AppSettings.swift b/ElementX/Sources/Application/AppSettings.swift index 476244923..c759358e2 100644 --- a/ElementX/Sources/Application/AppSettings.swift +++ b/ElementX/Sources/Application/AppSettings.swift @@ -153,14 +153,8 @@ 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. - let oidcRedirectURL = { - guard let url = URL(string: "\(InfoPlistReader.main.appScheme):/callback") else { - fatalError("Invalid OIDC redirect URL") - } - - return url - }() + /// 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. + let oidcRedirectURL: URL = "https://element.io/oidc/login" private(set) lazy var oidcConfiguration = OIDCConfigurationProxy(clientName: InfoPlistReader.main.bundleDisplayName, redirectURI: oidcRedirectURL, diff --git a/ElementX/Sources/Application/Navigation/AppRoutes.swift b/ElementX/Sources/Application/Navigation/AppRoutes.swift index 507e77b4b..d90b9282a 100644 --- a/ElementX/Sources/Application/Navigation/AppRoutes.swift +++ b/ElementX/Sources/Application/Navigation/AppRoutes.swift @@ -9,8 +9,6 @@ import Foundation import MatrixRustSDK enum AppRoute: Equatable { - /// The callback used to complete login with OIDC. - case oidcCallback(url: URL) /// The app's home screen. case roomList /// A room, shown as the root of the stack (popping any child rooms). @@ -52,7 +50,6 @@ struct AppRouteURLParser { urlParsers = [ MatrixPermalinkParser(), ElementWebURLParser(domains: appSettings.elementWebHosts), - OIDCCallbackURLParser(appSettings: appSettings), ElementCallURLParser() ] } @@ -76,16 +73,6 @@ protocol URLParser { func route(from url: URL) -> AppRoute? } -/// The parser for the OIDC callback URL. This always returns a `.oidcCallback`. -struct OIDCCallbackURLParser: URLParser { - let appSettings: AppSettings - - func route(from url: URL) -> AppRoute? { - guard url.absoluteString.starts(with: appSettings.oidcRedirectURL.absoluteString) else { return nil } - return .oidcCallback(url: url) - } -} - /// The parser for Element Call links. This always returns a `.genericCallLink`. struct ElementCallURLParser: URLParser { private let knownHosts = ["call.element.io"] diff --git a/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift index 5edc7967d..7a93de1a0 100644 --- a/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/AuthenticationFlowCoordinator.swift @@ -65,15 +65,6 @@ class AuthenticationFlowCoordinator: FlowCoordinatorProtocol { fatalError() } - 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 func showStartScreen() { diff --git a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift index 89faaa1c8..fd8420848 100644 --- a/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/RoomFlowCoordinator.swift @@ -163,7 +163,7 @@ class RoomFlowCoordinator: FlowCoordinatorProtocol { } case .roomAlias, .childRoomAlias, .eventOnRoomAlias, .childEventOnRoomAlias: break // These are converted to a room ID route one level above. - case .roomList, .userProfile, .call, .genericCallLink, .oidcCallback, .settings, .chatBackupSettings: + case .roomList, .userProfile, .call, .genericCallLink, .settings, .chatBackupSettings: break // These routes can't be handled. } } diff --git a/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift b/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift index c5b64b220..37a960b68 100644 --- a/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift +++ b/ElementX/Sources/FlowCoordinators/UserSessionFlowCoordinator.swift @@ -282,8 +282,6 @@ class UserSessionFlowCoordinator: FlowCoordinatorProtocol { presentCallScreen(genericCallLink: url) case .settings, .chatBackupSettings: settingsFlowCoordinator.handleAppRoute(appRoute, animated: animated) - case .oidcCallback: - break } } diff --git a/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift b/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift index 03b20e502..d4f85f83e 100644 --- a/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift +++ b/ElementX/Sources/Screens/Authentication/OIDCAuthenticationPresenter.swift @@ -14,16 +14,6 @@ class OIDCAuthenticationPresenter: NSObject { private let oidcRedirectURL: URL private let presentationAnchor: UIWindow - /// The data required to complete a request. - struct Request { - let session: ASWebAuthenticationSession - let oidcData: OIDCAuthorizationDataProxy - let continuation: CheckedContinuation, Never> - } - - /// The current request in progress. This is a single use value and will be moved on access. - @Consumable private var request: Request? - init(authenticationService: AuthenticationServiceProtocol, oidcRedirectURL: URL, presentationAnchor: UIWindow) { self.authenticationService = authenticationService self.oidcRedirectURL = oidcRedirectURL @@ -33,74 +23,35 @@ class OIDCAuthenticationPresenter: NSObject { /// Presents a web authentication session for the supplied data. func authenticate(using oidcData: OIDCAuthorizationDataProxy) async -> Result { - await withCheckedContinuation { continuation in - let session = ASWebAuthenticationSession(url: oidcData.url, - callbackURLScheme: oidcRedirectURL.scheme) { [weak self] url, error in - // This closure won't be called if the scheme is https, see handleUniversalLinkCallback for more info. - guard let self else { return } - - guard let url else { - // Check for user cancellation to avoid showing an alert in that instance. - if let nsError = error as? NSError, - nsError.domain == ASWebAuthenticationSessionErrorDomain, - nsError.code == ASWebAuthenticationSessionError.canceledLogin.rawValue { - self.completeAuthentication(throwing: .oidcError(.userCancellation)) - return - } - - self.completeAuthentication(throwing: .oidcError(.unknown)) - return - } - - completeAuthentication(callbackURL: url) + let (url, error) = await withCheckedContinuation { continuation in + let session = ASWebAuthenticationSession(url: oidcData.url, callback: .oidcRedirectURL(oidcRedirectURL)) { url, error in + continuation.resume(returning: (url, error)) } session.prefersEphemeralWebBrowserSession = false session.presentationContextProvider = self - request = Request(session: session, oidcData: oidcData, continuation: continuation) - session.start() } - } - - /// This method will be used if the `appSettings.oidcRedirectURL`'s scheme is `https`. - /// When using a custom scheme, the redirect will be handled by the web auth session's closure. - func handleUniversalLinkCallback(_ url: URL) { - completeAuthentication(callbackURL: url) - } - - /// Completes the authentication by exchanging the callback URL for a user session. - private func completeAuthentication(callbackURL: URL) { - guard let request else { - MXLog.error("Failed to complete authentication. Missing request.") - return - } - if callbackURL.scheme?.starts(with: "http") == true { - request.session.cancel() - } - - Task { - switch await authenticationService.loginWithOIDCCallback(callbackURL, data: request.oidcData) { - case .success(let userSession): - request.continuation.resume(returning: .success(userSession)) - case .failure(let error): - request.continuation.resume(returning: .failure(error)) + guard let url else { + // Check for user cancellation to avoid showing an alert in that instance. + if let nsError = error as? NSError, + nsError.domain == ASWebAuthenticationSessionErrorDomain, + nsError.code == ASWebAuthenticationSessionError.canceledLogin.rawValue { + await authenticationService.abortOIDCLogin(data: oidcData) + return .failure(.oidcError(.userCancellation)) } - } - } - - /// Aborts the authentication with the supplied error. - private func completeAuthentication(throwing error: AuthenticationServiceError) { - guard let request else { - MXLog.error("Failed to throw authentication error. Missing request.") - return + + await authenticationService.abortOIDCLogin(data: oidcData) + return .failure(.oidcError(.unknown)) } - Task { - await authenticationService.abortOIDCLogin(data: request.oidcData) - request.continuation.resume(returning: .failure(error)) + switch await authenticationService.loginWithOIDCCallback(url, data: oidcData) { + case .success(let userSession): + return .success(userSession) + case .failure(let error): + return .failure(error) } } } @@ -110,3 +61,15 @@ class OIDCAuthenticationPresenter: NSObject { extension OIDCAuthenticationPresenter: ASWebAuthenticationPresentationContextProviding { func presentationAnchor(for session: ASWebAuthenticationSession) -> ASPresentationAnchor { presentationAnchor } } + +extension ASWebAuthenticationSession.Callback { + static func oidcRedirectURL(_ url: URL) -> Self { + if url.scheme == "https", let host = url.host() { + .https(host: host, path: url.path()) + } else if let scheme = url.scheme { + .customScheme(scheme) + } else { + fatalError("Invalid OIDC redirect URL: \(url)") + } + } +} diff --git a/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift b/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift index db748aa4f..e306d2c35 100644 --- a/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift +++ b/ElementX/Sources/Screens/Authentication/SoftLogoutScreen/SoftLogoutScreenCoordinator.swift @@ -85,15 +85,6 @@ 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" diff --git a/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift b/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift index c6980de8e..3be0d1a2a 100644 --- a/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift +++ b/ElementX/Sources/Screens/Settings/AccountSettings/OIDCAccountSettingsPresenter.swift @@ -28,7 +28,7 @@ class OIDCAccountSettingsPresenter: NSObject { /// Presents a web authentication session for the supplied data. func start() { - let session = ASWebAuthenticationSession(url: accountURL, callbackURLScheme: oidcRedirectURL.scheme) { _, _ in } + let session = ASWebAuthenticationSession(url: accountURL, callback: .oidcRedirectURL(oidcRedirectURL)) { _, _ in } session.prefersEphemeralWebBrowserSession = false session.presentationContextProvider = self session.start() diff --git a/UnitTests/Sources/AppRouteURLParserTests.swift b/UnitTests/Sources/AppRouteURLParserTests.swift index 952e249f8..c24c4c8c2 100644 --- a/UnitTests/Sources/AppRouteURLParserTests.swift +++ b/UnitTests/Sources/AppRouteURLParserTests.swift @@ -73,33 +73,6 @@ class AppRouteURLParserTests: XCTestCase { XCTAssertEqual(appRouteURLParser.route(from: customSchemeURL), nil) } - func testOIDCCallbackRoute() { - // 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. - XCTAssertEqual(route, AppRoute.oidcCallback(url: callbackURL)) - } - - func testOIDCCallbackAppVariantRoute() { - // Given an OIDC callback for a different app variant. - let callbackURL = appSettings.oidcRedirectURL - .deletingLastPathComponent() - .appending(component: "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. - XCTAssertEqual(route, nil) - } - func testMatrixUserURL() { let userID = "@test:matrix.org" guard let url = URL(string: "https://matrix.to/#/\(userID)") else {