From 18b6874ffe9be2e784fdfeaafa9c8cd16fafb40e Mon Sep 17 00:00:00 2001 From: Tony Li Date: Thu, 20 Mar 2025 10:40:30 +1300 Subject: [PATCH 1/3] Support WordPress.com magic link login --- .../Login/WordPressDotComAuthenticator.swift | 109 ++++++++++++------ .../System/WordPressAppDelegate+openURL.swift | 3 + 2 files changed, 79 insertions(+), 33 deletions(-) diff --git a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift index fddead0549f8..31ae640d1fca 100644 --- a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift +++ b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift @@ -4,6 +4,8 @@ import Foundation import UIKit import WordPressData import WordPressShared +import BuildSettingsKit +@preconcurrency import Combine /// Log in or sign up a WordPress.com account via web. /// @@ -47,6 +49,9 @@ struct WordPressDotComAuthenticator { case loadingSites(Error) } + static let redirectURI = URL(string: BuildSettings.current.appURLScheme + "://oauth2-callback")! + static let callbackNotification = Foundation.Notification.Name(rawValue: "WordPressDotComAuthenticatorCallbackURL") + private let coreDataStack: CoreDataStackSwift private let authenticator: ((URL) throws(AuthenticationError) -> URL)? @@ -172,11 +177,10 @@ struct WordPressDotComAuthenticator { ) async throws(AuthenticationError) -> String { let clientId = ApiCredentials.client let clientSecret = ApiCredentials.secret - let redirectURI = "x-wordpress-app://oauth2-callback" var queries: [String: Any] = [ "client_id": clientId, - "redirect_uri": redirectURI, + "redirect_uri": Self.redirectURI, "response_type": "code", "scope": "global", ] @@ -192,31 +196,83 @@ struct WordPressDotComAuthenticator { let callbackURL = try await authorize(from: viewController, url: authorizeURL, prefersEphemeralWebBrowserSession: prefersEphemeralWebBrowserSession) - return try await handleAuthorizeCallbackURL(callbackURL, clientId: clientId, clientSecret: clientSecret, redirectURI: redirectURI) + return try await handleAuthorizeCallbackURL(callbackURL, clientId: clientId, clientSecret: clientSecret, redirectURI: Self.redirectURI) } + @MainActor private func authorize(from viewController: UIViewController, url authorizeURL: URL, prefersEphemeralWebBrowserSession: Bool) async throws(AuthenticationError) -> URL { if let authenticator { return try authenticator(authorizeURL) } - return try await withCheckedTypedThrowingContinuation { continuation in - DispatchQueue.main.async { - let provider = WebAuthenticationPresentationAnchorProvider(anchor: viewController.view.window ?? UIWindow()) - let session = ASWebAuthenticationSession(url: authorizeURL, callbackURLScheme: "x-wordpress-app") { url, error in - let result: Result - if let url { - result = .success(url) - } else { - DDLogWarn("Error from authentication session: \(String(describing: error))") - result = .failure(.cancelled) - } - continuation(result) + class CancellableHolder: Cancellable, @unchecked Sendable { + var cancellable: AnyCancellable? + + func cancel() { + cancellable?.cancel() + cancellable = nil + } + } + let cancellable = CancellableHolder() + + // When the login account is unverified, the user receives an email with a login link. Opening the login link takes + // the user to Safari, where they can authorize/deny the OAuth request. After authorization/denial, the website + // opens the app via the OAuth callback URL. In this scenario, the callback URL is received by the open URL + // delegate method instead of the ASWebAuthenticationSession instance. + let callbackURLViaOpenAppURL = NotificationCenter.default + .publisher(for: Self.callbackNotification) + .compactMap { + if let url = $0.object as? URL { + return url + } + return nil + } + .filter { (url: URL) in + url.absoluteString.hasPrefix(Self.redirectURI.absoluteString) + } + .setFailureType(to: AuthenticationError.self) + .first() + + let callbackURLViaWebAuthenticationSession = PassthroughSubject() + let provider = WebAuthenticationPresentationAnchorProvider(anchor: viewController.view.window ?? UIWindow()) + let session = ASWebAuthenticationSession(url: authorizeURL, callbackURLScheme: Self.redirectURI.scheme!) { url, error in + if let url { + callbackURLViaWebAuthenticationSession.send(url) + callbackURLViaWebAuthenticationSession.send(completion: .finished) + } else { + DDLogWarn("Error from authentication session: \(String(describing: error))") + callbackURLViaWebAuthenticationSession.send(completion: .failure(.cancelled)) + } + } + session.prefersEphemeralWebBrowserSession = prefersEphemeralWebBrowserSession + session.presentationContextProvider = provider + session.start() + + do { + return try await withTaskCancellationHandler { + try await withCheckedThrowingContinuation { continuation in + cancellable.cancellable = Publishers.Merge(callbackURLViaOpenAppURL, callbackURLViaWebAuthenticationSession) + .first() + .sink(receiveCompletion: { [session] in + session.cancel() + + if case let .failure(error) = $0 { + continuation.resume(throwing: error) + } + }, receiveValue: { url in + continuation.resume(returning: url) + }) } - session.prefersEphemeralWebBrowserSession = prefersEphemeralWebBrowserSession - session.presentationContextProvider = provider - session.start() + } onCancel: { [session] in + session.cancel() + cancellable.cancel() } + } catch { + // We can't get the `AuthenticationError` type from the syntax level, because + // the `withTaskCancellationHandler` and `withCheckedThrowingContinuation` functions are not updated to + // support typed throw. But the error can only be `AuthenticationError` at runtime. + wpAssert(error is AuthenticationError) + throw (error as? AuthenticationError) ?? .cancelled } } @@ -224,7 +280,7 @@ struct WordPressDotComAuthenticator { _ url: URL, clientId: String, clientSecret: String, - redirectURI: String + redirectURI: URL ) async throws(AuthenticationError) -> String { guard let query = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems else { throw .invalidCallbackURL @@ -246,7 +302,7 @@ struct WordPressDotComAuthenticator { "grant_type": "authorization_code", "client_id": clientId, "client_secret": clientSecret, - "redirect_uri": redirectURI, + "redirect_uri": redirectURI.absoluteString, "code": code, ] @@ -281,19 +337,6 @@ struct WordPressDotComAuthenticator { } } -/// typed-throw version of `withCheckedThrowingContinuation` -private func withCheckedTypedThrowingContinuation(body: (@escaping ((Result) -> Void)) -> Void) async throws(E) -> T { - do { - return try await withCheckedThrowingContinuation { continuation in - body { - continuation.resume(with: $0) - } - } - } catch { - throw (error as! E) - } -} - private extension WordPressDotComAuthenticator.SignInError { var alertMessage: String? { switch self { diff --git a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift index be0b4020c3e5..ca1d694d85eb 100644 --- a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift +++ b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift @@ -48,6 +48,9 @@ import BuildSettingsKit return handleViewStats(url: url) case "debugging": return handleDebugging(url: url) + case WordPressDotComAuthenticator.redirectURI.host(): + NotificationCenter.default.post(name: WordPressDotComAuthenticator.callbackNotification, object: url) + return true default: return false } From 9df3ad6189b3c91d7493e8b623fbfb89e0389bd4 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Thu, 20 Mar 2025 11:37:48 +1300 Subject: [PATCH 2/3] Fix unit tests --- .../Login/WordPressDotComAuthenticator.swift | 26 ++++++++++++------- .../System/WordPressAppDelegate+openURL.swift | 2 +- .../WordPressDotComAuthenticatorTests.swift | 14 +++++----- 3 files changed, 24 insertions(+), 18 deletions(-) diff --git a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift index 31ae640d1fca..9462e1c7f8f8 100644 --- a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift +++ b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift @@ -49,18 +49,23 @@ struct WordPressDotComAuthenticator { case loadingSites(Error) } - static let redirectURI = URL(string: BuildSettings.current.appURLScheme + "://oauth2-callback")! static let callbackNotification = Foundation.Notification.Name(rawValue: "WordPressDotComAuthenticatorCallbackURL") + static func redirectURI(for scheme: String) -> String { + "\(scheme)://oauth2-callback" + } + let redirectURIScheme: String private let coreDataStack: CoreDataStackSwift private let authenticator: ((URL) throws(AuthenticationError) -> URL)? init( coreDataStack: CoreDataStackSwift = ContextManager.shared, - authenticator: ((URL) throws(AuthenticationError) -> URL)? = nil + authenticator: ((URL) throws(AuthenticationError) -> URL)? = nil, + redirectURIScheme: String = BuildSettings.current.appURLScheme ) { self.coreDataStack = coreDataStack self.authenticator = authenticator + self.redirectURIScheme = redirectURIScheme } /// Sign in WP.com account. @@ -177,10 +182,11 @@ struct WordPressDotComAuthenticator { ) async throws(AuthenticationError) -> String { let clientId = ApiCredentials.client let clientSecret = ApiCredentials.secret + let redirectURI = Self.redirectURI(for: redirectURIScheme) var queries: [String: Any] = [ "client_id": clientId, - "redirect_uri": Self.redirectURI, + "redirect_uri": redirectURI, "response_type": "code", "scope": "global", ] @@ -194,13 +200,13 @@ struct WordPressDotComAuthenticator { let authorizeURL = try? URLEncoding.queryString.encode(URLRequest(url: URL(string: "https://public-api.wordpress.com/oauth2/authorize")!), with: queries).url guard let authorizeURL else { throw .urlError(URLError(.badURL)) } - let callbackURL = try await authorize(from: viewController, url: authorizeURL, prefersEphemeralWebBrowserSession: prefersEphemeralWebBrowserSession) + let callbackURL = try await authorize(from: viewController, url: authorizeURL, prefersEphemeralWebBrowserSession: prefersEphemeralWebBrowserSession, redirectURI: redirectURI) - return try await handleAuthorizeCallbackURL(callbackURL, clientId: clientId, clientSecret: clientSecret, redirectURI: Self.redirectURI) + return try await handleAuthorizeCallbackURL(callbackURL, clientId: clientId, clientSecret: clientSecret, redirectURI: redirectURI) } @MainActor - private func authorize(from viewController: UIViewController, url authorizeURL: URL, prefersEphemeralWebBrowserSession: Bool) async throws(AuthenticationError) -> URL { + private func authorize(from viewController: UIViewController, url authorizeURL: URL, prefersEphemeralWebBrowserSession: Bool, redirectURI: String) async throws(AuthenticationError) -> URL { if let authenticator { return try authenticator(authorizeURL) } @@ -228,14 +234,14 @@ struct WordPressDotComAuthenticator { return nil } .filter { (url: URL) in - url.absoluteString.hasPrefix(Self.redirectURI.absoluteString) + url.absoluteString.hasPrefix(redirectURI) } .setFailureType(to: AuthenticationError.self) .first() let callbackURLViaWebAuthenticationSession = PassthroughSubject() let provider = WebAuthenticationPresentationAnchorProvider(anchor: viewController.view.window ?? UIWindow()) - let session = ASWebAuthenticationSession(url: authorizeURL, callbackURLScheme: Self.redirectURI.scheme!) { url, error in + let session = ASWebAuthenticationSession(url: authorizeURL, callbackURLScheme: redirectURIScheme) { url, error in if let url { callbackURLViaWebAuthenticationSession.send(url) callbackURLViaWebAuthenticationSession.send(completion: .finished) @@ -280,7 +286,7 @@ struct WordPressDotComAuthenticator { _ url: URL, clientId: String, clientSecret: String, - redirectURI: URL + redirectURI: String ) async throws(AuthenticationError) -> String { guard let query = URLComponents(url: url, resolvingAgainstBaseURL: true)?.queryItems else { throw .invalidCallbackURL @@ -302,7 +308,7 @@ struct WordPressDotComAuthenticator { "grant_type": "authorization_code", "client_id": clientId, "client_secret": clientSecret, - "redirect_uri": redirectURI.absoluteString, + "redirect_uri": redirectURI, "code": code, ] diff --git a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift index ca1d694d85eb..6a6fc49411de 100644 --- a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift +++ b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift @@ -48,7 +48,7 @@ import BuildSettingsKit return handleViewStats(url: url) case "debugging": return handleDebugging(url: url) - case WordPressDotComAuthenticator.redirectURI.host(): + case BuildSettings.current.appURLScheme where url.absoluteString.hasPrefix(WordPressDotComAuthenticator.redirectURI(for: BuildSettings.current.appURLScheme)): NotificationCenter.default.post(name: WordPressDotComAuthenticator.callbackNotification, object: url) return true default: diff --git a/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift b/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift index d46a99808c52..a598a79f7f65 100644 --- a/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift +++ b/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift @@ -16,7 +16,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { func testAuthenticateSuccess() async { stubTokenExchange() - let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["code": "random"])) + let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.authenticate(from: UIViewController(), prefersEphemeralWebBrowserSession: false) } catch { @@ -25,7 +25,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { } func testAuthenticateWithInvalidCallbackURL() async { - let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["empty": "yes"])) + let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["empty": "yes"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.authenticate(from: .init(), prefersEphemeralWebBrowserSession: false) XCTFail("Unexpected successful result") @@ -37,7 +37,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { } func testAuthenticateWithAccessDenied() async { - let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["error": "access_denied"])) + let authenticator = WordPressDotComAuthenticator(authenticator: fakeAuthenticator(callback: ["error": "access_denied"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.authenticate(from: .init(), prefersEphemeralWebBrowserSession: false) XCTFail("Unexpected successful result") @@ -58,7 +58,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { try XCTAssertNil(WPAccount.lookupDefaultWordPressComAccount(in: mainContext)) // When signing in with a WP.com account - let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"])) + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") let accountID = await authenticator.signIn(from: UIViewController(), context: .default) XCTAssertNotNil(accountID) @@ -79,7 +79,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { AccountService(coreDataStack: contextManager).setDefaultWordPressComAccount(account) // When signing in with another WP.com account. - let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"])) + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.attemptSignIn(from: UIViewController(), context: .jetpackSite(accountEmail: nil)) XCTFail("Unexpected successful result") @@ -95,7 +95,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { stubGetAccountDetails() stubGetSitesError() - let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"])) + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.attemptSignIn(from: UIViewController(), context: .default) XCTFail("Unexpected successful result") @@ -117,7 +117,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { stubGetAccountDetails() // Then `mismatchedEmail` error should be returned - let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"])) + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") do { let _ = try await authenticator.attemptSignIn(from: UIViewController(), context: .reauthentication(accountEmail: account.email)) XCTFail("Unexpected successful result") From e47815502507fb97024259201317d795b8277274 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Fri, 21 Mar 2025 11:38:24 +1300 Subject: [PATCH 3/3] Fix login link URL check --- .../Login/WordPressDotComAuthenticator.swift | 12 +++++++- .../System/WordPressAppDelegate+openURL.swift | 7 +++-- .../WordPressDotComAuthenticatorTests.swift | 29 +++++++++++++++++++ 3 files changed, 44 insertions(+), 4 deletions(-) diff --git a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift index 9462e1c7f8f8..6e3661d04deb 100644 --- a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift +++ b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift @@ -49,11 +49,21 @@ struct WordPressDotComAuthenticator { case loadingSites(Error) } - static let callbackNotification = Foundation.Notification.Name(rawValue: "WordPressDotComAuthenticatorCallbackURL") + private static let callbackNotification = Foundation.Notification.Name(rawValue: "WordPressDotComAuthenticatorCallbackURL") + static func redirectURI(for scheme: String) -> String { "\(scheme)://oauth2-callback" } + static func handleAppOpeningURL(_ url: URL, appURLScheme: String = BuildSettings.current.appURLScheme) -> Bool { + guard url.scheme == appURLScheme, url.absoluteString.hasPrefix(redirectURI(for: appURLScheme)) else { + return false + } + + NotificationCenter.default.post(name: callbackNotification, object: url) + return true + } + let redirectURIScheme: String private let coreDataStack: CoreDataStackSwift private let authenticator: ((URL) throws(AuthenticationError) -> URL)? diff --git a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift index 6a6fc49411de..5bda2f67e6d6 100644 --- a/WordPress/Classes/System/WordPressAppDelegate+openURL.swift +++ b/WordPress/Classes/System/WordPressAppDelegate+openURL.swift @@ -30,6 +30,10 @@ import BuildSettingsKit return JetpackNotificationMigrationService.shared.handleNotificationMigrationOnWordPress() } + if WordPressDotComAuthenticator.handleAppOpeningURL(url) { + return true + } + guard url.scheme == BuildSettings.current.appURLScheme else { return false } @@ -48,9 +52,6 @@ import BuildSettingsKit return handleViewStats(url: url) case "debugging": return handleDebugging(url: url) - case BuildSettings.current.appURLScheme where url.absoluteString.hasPrefix(WordPressDotComAuthenticator.redirectURI(for: BuildSettings.current.appURLScheme)): - NotificationCenter.default.post(name: WordPressDotComAuthenticator.callbackNotification, object: url) - return true default: return false } diff --git a/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift b/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift index a598a79f7f65..fe1f6f6f9e3a 100644 --- a/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift +++ b/WordPress/WordPressTest/Login/WordPressDotComAuthenticatorTests.swift @@ -67,6 +67,35 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { XCTAssertTrue(isDefaultAccount) } + @MainActor + func testSignInViaMagicLink() async throws { + stubTokenExchange() + stubGetAccountDetails() + stubGetSites() + + // Given the app is not signed in with a WP.com account + try XCTAssertNil(WPAccount.lookupDefaultWordPressComAccount(in: mainContext)) + + // When signing in with a WP.com account via web authentication session using a magic link + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, redirectURIScheme: "testapp") + // Here we post a notification to simulate this scenario in the production app: + // - user taps the magic link in their email + // - the link opens Safari and then redirects to the app + // - the app posts a notification. + Task.detached { @MainActor in + try await Task.sleep(for: .milliseconds(100)) + let handled = WordPressDotComAuthenticator.handleAppOpeningURL(URL(string: "testapp://oauth2-callback?code=random")!, appURLScheme: "testapp") + XCTAssertTrue(handled) + } + + let accountID = await authenticator.signIn(from: UIViewController(), context: .default) + XCTAssertNotNil(accountID) + + // The new WP.com acount should be set as the default account. + let isDefaultAccount = try mainContext.existingObject(with: XCTUnwrap(accountID)).isDefaultWordPressComAccount + XCTAssertTrue(isDefaultAccount) + } + @MainActor func testSignInAnotherAccount() async throws { stubTokenExchange()