diff --git a/Modules/Sources/WordPressData/Swift/Blog+WordPressComRestAPI.swift b/Modules/Sources/WordPressData/Swift/Blog+WordPressComRestAPI.swift index f887705c27df..0ab04b40e145 100644 --- a/Modules/Sources/WordPressData/Swift/Blog+WordPressComRestAPI.swift +++ b/Modules/Sources/WordPressData/Swift/Blog+WordPressComRestAPI.swift @@ -17,4 +17,22 @@ extension Blog { public var isAccessibleThroughWPCom: Bool { wordPressComRestApi != nil } + + /// Whether the blog has no usable remote because its WordPress.com account has no token. + /// + /// A blog hosted at WordPress.com, or one without a stored XML-RPC password, can only be reached with the + /// account's token. Remote factories return no remote for such a blog instead of falling back to Core REST + /// or XML-RPC with empty credentials. A Jetpack-connected self-hosted blog with a stored password keeps + /// using XML-RPC. + /// + /// Reading this property does not trigger the WordPress.com sign-in, unlike `wordPressComRestApi`. + @objc public var isMissingRequiredWordPressComToken: Bool { + guard let account else { + return false + } + if let token = account.authToken, !token.isEmpty { + return false + } + return isHostedAtWPcom || password == nil + } } diff --git a/Modules/Sources/WordPressData/Swift/WPAccount+RestApi.swift b/Modules/Sources/WordPressData/Swift/WPAccount+RestApi.swift index 95f990c90b32..8c7d8fd7c9e0 100644 --- a/Modules/Sources/WordPressData/Swift/WPAccount+RestApi.swift +++ b/Modules/Sources/WordPressData/Swift/WPAccount+RestApi.swift @@ -43,8 +43,12 @@ extension WPAccount { // We use a static function here because it's not safe to access `self` in this closure. // The `WPAccount` instance can be bound to any context object. There is no guarantee that the thread // from which this closure is called is the same as the one in the context object. - context?.perform { - WPAccount.handleInvalidToken(accountID: accountID, context: context) + // + // `WordPressComRestApi` calls this handler on the main queue. Waiting for the context blocks the main + // queue, where sign-in stores replacement tokens, so a replacement cannot be written between the + // comparison and the clear. + context?.performAndWait { + WPAccount.handleInvalidToken(accountID: accountID, rejectedToken: authToken, context: context) } } @@ -69,30 +73,34 @@ extension WPAccount { return account?.wordPressComRestApi } - static func handleInvalidToken(accountID: TaggedManagedObjectID, context: NSManagedObjectContext?) { - let account = try? context?.existingObject(with: accountID) - account?.authToken = nil + /// Clears the account's token after the server rejected it, and asks for the WordPress.com sign-in. + /// + /// This does not post `wpAccountDefaultWordPressComAccountChanged`: the account and its sites still exist, and + /// observers treat that notification without an object as a logout. + static func handleInvalidToken( + accountID: TaggedManagedObjectID, + rejectedToken: String, + context: NSManagedObjectContext? + ) { + guard let account = try? context?.existingObject(with: accountID) else { + return + } + + // Storing a new token does not cancel requests made with the previous one. A late rejection of the + // previous token must not erase its replacement. This instance may still cache the previous token, if the + // replacement was stored through another context, so make it read the replacement next time. + guard account.authTokenInKeychain == rejectedToken else { + WPLogInfo("Ignoring a rejection of a WordPress.com token that is no longer current") + account.discardCachedToken() + return + } + + account.authToken = nil NotificationCenter.default.post( name: .wpAccountRequiresShowingSigninForWPComFixingAuthToken, object: account ) - - if account?.isDefaultWordPressComAccount == true { - // At the time of writing, there is an implicit assumption on what the object parameter value means. - // For example, the WordPressAppDelegate.handleDefaultAccountChangedNotification(_:) subscriber inspects the object parameter to decide whether the notification was sent as a result of a login. - // If the object is non-nil, then the method considers the source a login. - // - // The code path in which we are is that of an invalid token, and that's neither a login nor a logout, it's more appropriate to consider it a logout. - // That's because if the token is invalid the app will soon received errors from the API and it's therefore better to force the user to login again. - - DispatchQueue.main.async { - NotificationCenter.default.post( - name: .wpAccountDefaultWordPressComAccountChanged, - object: nil - ) - } - } } } diff --git a/Modules/Sources/WordPressData/Swift/WPAccount.swift b/Modules/Sources/WordPressData/Swift/WPAccount.swift index 15fcd266137f..812c6230591a 100644 --- a/Modules/Sources/WordPressData/Swift/WPAccount.swift +++ b/Modules/Sources/WordPressData/Swift/WPAccount.swift @@ -104,8 +104,7 @@ public class WPAccount: NSManagedObject { public override func didTurnIntoFault() { super.didTurnIntoFault() - _private_wordPressComRestApi = nil - cachedToken = nil + discardCachedToken() } // MARK: - Auth Token (Keychain) @@ -120,21 +119,33 @@ public class WPAccount: NSManagedObject { if let cachedToken { return cachedToken } - let token = try? Self.token( + let token = authTokenInKeychain + cachedToken = token + return token + } + + /// The token stored in the keychain, bypassing `authToken`'s cache. + /// + /// The cache belongs to this managed object instance. A token written through an instance in another + /// context does not invalidate it, so only the keychain tells which token is current. + var authTokenInKeychain: String? { + try? Self.token( forUsername: username, serviceName: keychainServiceName, migration: keychainMigration, keychain: keychain ) - cachedToken = token - return token } - private func _setAuthToken(_ authToken: String?) { + /// Drops this instance's cached token and REST API, so the next access reads the token from the keychain. + func discardCachedToken() { cachedToken = nil + _private_wordPressComRestApi = nil + } + private func _setAuthToken(_ authToken: String?) { // Make sure to release any RestAPI alloc'ed, since it might have an invalid token - _private_wordPressComRestApi = nil + discardCachedToken() do { try keychain.setPassword(for: username, to: authToken, serviceName: keychainServiceName) diff --git a/Modules/Sources/WordPressData/Swift/WordPressOrgRestApi+WordPress.swift b/Modules/Sources/WordPressData/Swift/WordPressOrgRestApi+WordPress.swift index a08bdb03334e..315f9982076a 100644 --- a/Modules/Sources/WordPressData/Swift/WordPressOrgRestApi+WordPress.swift +++ b/Modules/Sources/WordPressData/Swift/WordPressOrgRestApi+WordPress.swift @@ -3,8 +3,10 @@ import WordPressShared import WordPressKit private func apiBase(blog: Blog) -> URL? { + // A blog with an account is reached through WordPress.com, never through its own REST API. It reaches this + // point when the account has no token, an expected state (the app shows the WordPress.com re-authentication + // gate), or when the blog has no WordPress.com ID. guard blog.account == nil else { - assertionFailure(".com support has not been implemented yet") return nil } diff --git a/Modules/Tests/WordPressDataTests/BlogTokenlessAccountTests.swift b/Modules/Tests/WordPressDataTests/BlogTokenlessAccountTests.swift new file mode 100644 index 000000000000..5c539d74f262 --- /dev/null +++ b/Modules/Tests/WordPressDataTests/BlogTokenlessAccountTests.swift @@ -0,0 +1,59 @@ +import CoreData +import Testing +import WordPressKit +@testable import WordPressData + +@MainActor +struct BlogTokenlessAccountTests { + private let contextManager = ContextManager.forTesting() + + private func makeBlog(hostedAtWPCom: Bool, authToken: String?, xmlrpcPassword: String? = nil) -> Blog { + let blog = BlogBuilder(contextManager.mainContext) + .with(isHostedAtWPCom: hostedAtWPCom) + .build() + let account = WPAccount.fixture(context: contextManager.mainContext) + account.authToken = authToken + blog.account = account + if let xmlrpcPassword { + blog.username = "xmlrpc-user" + blog.password = xmlrpcPassword + } + return blog + } + + @Test func dotComBlogWithTokenHasRemote() { + let blog = makeBlog(hostedAtWPCom: true, authToken: "token") + #expect(!blog.isMissingRequiredWordPressComToken) + } + + @Test func dotComBlogWithoutTokenHasNoRemote() { + #expect(makeBlog(hostedAtWPCom: true, authToken: nil).isMissingRequiredWordPressComToken) + #expect(makeBlog(hostedAtWPCom: true, authToken: "").isMissingRequiredWordPressComToken) + } + + @Test func dotComBlogWithoutTokenHasNoRemoteEvenWithXMLRPCPassword() { + let blog = makeBlog(hostedAtWPCom: true, authToken: nil, xmlrpcPassword: "password") + #expect(blog.isMissingRequiredWordPressComToken) + } + + @Test func jetpackBlogWithoutTokenKeepsStoredXMLRPCCredentials() { + let blog = makeBlog(hostedAtWPCom: false, authToken: nil, xmlrpcPassword: "password") + #expect(!blog.isMissingRequiredWordPressComToken) + } + + @Test func jetpackBlogWithoutTokenOrXMLRPCPasswordHasNoRemote() { + let blog = makeBlog(hostedAtWPCom: false, authToken: nil) + #expect(blog.isMissingRequiredWordPressComToken) + } + + @Test func selfHostedBlogIsUnaffected() { + let blog = BlogBuilder(contextManager.mainContext, dotComID: nil).build() + blog.account = nil + #expect(!blog.isMissingRequiredWordPressComToken) + } + + @Test func noSelfHostedRestApiForDotComBlogWithoutToken() { + let blog = makeBlog(hostedAtWPCom: true, authToken: nil) + #expect(WordPressOrgRestApi(blog: blog) == nil) + } +} diff --git a/Tests/KeystoneTests/Helpers/RemoteSelectionBlogFixture.swift b/Tests/KeystoneTests/Helpers/RemoteSelectionBlogFixture.swift new file mode 100644 index 000000000000..e7efa4b6d014 --- /dev/null +++ b/Tests/KeystoneTests/Helpers/RemoteSelectionBlogFixture.swift @@ -0,0 +1,60 @@ +import CoreData +import Foundation +@testable import WordPressData + +/// Blogs for testing which remote a factory selects, with and without a WordPress.com token. +/// +/// Keychain access is isolated, so these blogs never read or write the simulator keychain. +@objc public final class RemoteSelectionBlogFixture: NSObject { + + /// A blog hosted at WordPress.com whose account has the given token. + @objc public static func dotComBlog(authToken: String?, context: NSManagedObjectContext) -> Blog { + let blog = makeBlog(host: "example.wordpress.com", context: context) + blog.isHostedAtWPcom = true + blog.dotComID = 1 + blog.account = makeAccount(authToken: authToken, context: context) + return blog + } + + /// A Jetpack-connected self-hosted blog whose account has the given token and that may store XML-RPC credentials. + @objc public static func jetpackBlog( + authToken: String?, + xmlrpcPassword: String?, + context: NSManagedObjectContext + ) -> Blog { + let blog = makeBlog(host: "jetpack.example.com", context: context) + blog.isHostedAtWPcom = false + blog.dotComID = 2 + blog.account = makeAccount(authToken: authToken, context: context) + if let xmlrpcPassword { + blog.username = "xmlrpc-\(UUID().uuidString)" + blog.password = xmlrpcPassword + } + return blog + } + + /// A self-hosted blog without an account that stores XML-RPC credentials. + @objc public static func selfHostedBlog(context: NSManagedObjectContext) -> Blog { + let blog = makeBlog(host: "self-hosted.example.com", context: context) + blog.username = "xmlrpc-\(UUID().uuidString)" + blog.password = "password" + return blog + } + + private static func makeBlog(host: String, context: NSManagedObjectContext) -> Blog { + let blog = Blog(context: context) + blog.keychain = TestKeychain() + blog.url = "https://\(host)/" + blog.xmlrpc = "https://\(host)/xmlrpc.php" + return blog + } + + private static func makeAccount(authToken: String?, context: NSManagedObjectContext) -> WPAccount { + let account = WPAccount(context: context) + account.mockKeychain() + // A unique username keeps the real keychain, which `WordPressSite(blog:)` falls back to, from matching. + account.username = "account-\(UUID().uuidString)" + account.authToken = authToken + return account + } +} diff --git a/Tests/KeystoneTests/Tests/Login/WordPressDotComAuthenticatorTests.swift b/Tests/KeystoneTests/Tests/Login/WordPressDotComAuthenticatorTests.swift index c5482e9f0752..09611a0a7c2f 100644 --- a/Tests/KeystoneTests/Tests/Login/WordPressDotComAuthenticatorTests.swift +++ b/Tests/KeystoneTests/Tests/Login/WordPressDotComAuthenticatorTests.swift @@ -180,7 +180,7 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { func testReAuthenticationWithUnexpectedAccount() async throws { // Given the app is already signed in with a WP.com account. - let account = AccountBuilder(mainContext).with(email: "test@example.com").with(username: "default_account").with(authToken: "token").build() + let account = AccountBuilder(mainContext).with(id: 1).with(email: "test@example.com").with(username: "default_account").with(authToken: "token").build() try mainContext.save() AccountService(coreDataStack: contextManager).setDefaultWordPressComAccount(account) @@ -188,17 +188,58 @@ class WordPressDotComAuthenticatorTests: CoreDataTestCase { stubTokenExchange() stubGetAccountDetails() - // Then `mismatchedEmail` error should be returned + // Then `mismatchedAccount` error should be returned 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)) + let _ = try await authenticator.attemptSignIn(from: UIViewController(), context: .reauthentication(userID: account.userID, emailHint: account.email)) XCTFail("Unexpected successful result") - } catch let .mismatchedEmail(expectedEmail) { + } catch let .mismatchedAccount(expectedEmail) { XCTAssertEqual(expectedEmail, "test@example.com") } catch { XCTFail("Unexpected error: \(error)") } } + + @MainActor + func testReAuthenticationAfterEmailChangeSucceeds() async throws { + // Given the default account's stored email is outdated. + let account = AccountBuilder(mainContext).with(id: 55511).with(email: "old@example.com").with(username: "jimthetester").with(authToken: "old-token").build() + try mainContext.save() + AccountService(coreDataStack: contextManager).setDefaultWordPressComAccount(account) + + stubTokenExchange() + stubGetAccountDetails() + stubGetSites() + + // When the same user signs in again. + let authenticator = WordPressDotComAuthenticator(coreDataStack: contextManager, authenticator: fakeAuthenticator(callback: ["code": "random"]), redirectURIScheme: "testapp") + let accountID = try await authenticator.attemptSignIn(from: UIViewController(), context: .reauthentication(userID: account.userID, emailHint: account.email)) + + // Then the stored account is updated. + XCTAssertEqual(accountID.objectID, account.objectID) + XCTAssertEqual(account.email, "jim@wptestaccounts.com") + XCTAssertEqual(try WPAccount.token(forUsername: "jimthetester"), "token") + } + + func testSignInWithAnotherUserIDIsRejectedEvenWithTheSameEmail() async throws { + // Given the app is already signed in with a WP.com account that has the same email as the stubbed user. + let account = AccountBuilder(mainContext).with(id: 1).with(email: "jim@wptestaccounts.com").with(username: "default_account").with(authToken: "token").build() + try mainContext.save() + AccountService(coreDataStack: contextManager).setDefaultWordPressComAccount(account) + + stubTokenExchange() + stubGetAccountDetails() + + 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") + } catch .alreadySignedIn { + // Do nothing + } catch { + XCTFail("Unexpected error: \(error)") + } + } } private extension WordPressDotComAuthenticatorTests { diff --git a/Tests/KeystoneTests/Tests/Models/WPAccount+Fixture.swift b/Tests/KeystoneTests/Tests/Models/WPAccount+Fixture.swift index 17247ed169aa..d5c493859e6e 100644 --- a/Tests/KeystoneTests/Tests/Models/WPAccount+Fixture.swift +++ b/Tests/KeystoneTests/Tests/Models/WPAccount+Fixture.swift @@ -25,9 +25,10 @@ extension WPAccount { return account } + /// Pass the same `keychain` to instances in different contexts to make them share it, like the real keychain. @discardableResult - func mockKeychain() -> WPAccount { - keychain = TestKeychain() + func mockKeychain(_ keychain: TestKeychain = TestKeychain()) -> WPAccount { + self.keychain = keychain keychainServiceName = "test-service" keychainMigration = MockAuthKeyMigration() return self diff --git a/Tests/KeystoneTests/Tests/Models/WPAccount+RestApiTests.swift b/Tests/KeystoneTests/Tests/Models/WPAccount+RestApiTests.swift index 2bb324ee87b1..fbc7dcae53bc 100644 --- a/Tests/KeystoneTests/Tests/Models/WPAccount+RestApiTests.swift +++ b/Tests/KeystoneTests/Tests/Models/WPAccount+RestApiTests.swift @@ -1,9 +1,18 @@ import CoreData +import OHHTTPStubs +import OHHTTPStubsSwift import XCTest // Wanted to use Testing, but don't yet have CoreData utils there @testable import WordPress +@testable import WordPressData class WPAccountRestApiTests: CoreDataTestCase { + override func tearDown() { + HTTPStubs.removeAllStubs() + UserSettings.defaultDotComUUID = nil + super.tearDown() + } + func testAccessingAPIWithNoTokenPostsNotification() async { let account = WPAccount.fixture(context: mainContext, authToken: "") @@ -32,4 +41,135 @@ class WPAccountRestApiTests: CoreDataTestCase { await fulfillment(of: [notificationExpectation], timeout: 0.1) } + + // MARK: - Invalid token + + func testRejectedTokenIsClearedAndSignInIsRequested() async throws { + let account = try makeDefaultAccount(authToken: "a-token") + + let signInExpectation = XCTNSNotificationExpectation( + name: .wpAccountRequiresShowingSigninForWPComFixingAuthToken, + object: account, + notificationCenter: .default + ) + + WPAccount.handleInvalidToken( + accountID: TaggedManagedObjectID(account), + rejectedToken: "a-token", + context: mainContext + ) + + await fulfillment(of: [signInExpectation], timeout: 1) + XCTAssertNil(account.authToken) + } + + func testRejectedTokenDoesNotPostAccountChangedNotification() async throws { + let account = try makeDefaultAccount(authToken: "a-token") + + // Observers treat this notification without an object as a logout. + let accountChangedExpectation = XCTNSNotificationExpectation( + name: .wpAccountDefaultWordPressComAccountChanged, + object: nil, + notificationCenter: .default + ) + accountChangedExpectation.isInverted = true + + WPAccount.handleInvalidToken( + accountID: TaggedManagedObjectID(account), + rejectedToken: "a-token", + context: mainContext + ) + + await fulfillment(of: [accountChangedExpectation], timeout: 0.5) + } + + func testRejectionOfTokenThatIsNoLongerCurrentChangesNothing() async throws { + let account = try makeDefaultAccount(authToken: "new-token") + + let signInExpectation = XCTNSNotificationExpectation( + name: .wpAccountRequiresShowingSigninForWPComFixingAuthToken, + object: nil, + notificationCenter: .default + ) + signInExpectation.isInverted = true + + WPAccount.handleInvalidToken( + accountID: TaggedManagedObjectID(account), + rejectedToken: "old-token", + context: mainContext + ) + + await fulfillment(of: [signInExpectation], timeout: 0.5) + XCTAssertEqual(account.authToken, "new-token") + } + + func testLateRejectionDoesNotEraseTokenStoredThroughAnotherContext() async throws { + let keychain = TestKeychain() + let account = try makeDefaultAccount(authToken: "token-a") + account.mockKeychain(keychain) + account.authToken = "token-a" + // Fill this instance's token and REST API caches with token A. + XCTAssertEqual(account.authToken, "token-a") + XCTAssertNotNil(account.wordPressComRestApi) + + let accountID = TaggedManagedObjectID(account) + try await contextManager.performAndSave { context in + let otherInstance = try context.existingObject(with: accountID) + otherInstance.mockKeychain(keychain) + otherInstance.authToken = "token-b" + } + + let signInExpectation = XCTNSNotificationExpectation( + name: .wpAccountRequiresShowingSigninForWPComFixingAuthToken, + object: nil, + notificationCenter: .default + ) + signInExpectation.isInverted = true + + WPAccount.handleInvalidToken(accountID: accountID, rejectedToken: "token-a", context: mainContext) + + await fulfillment(of: [signInExpectation], timeout: 0.5) + XCTAssertEqual(try keychain.getPassword(for: account.username, serviceName: "test-service"), "token-b") + XCTAssertEqual(account.authToken, "token-b") + + // Later requests through this instance use token B. + var authorization: String? + stub(condition: isHost("public-api.wordpress.com")) { request in + authorization = request.value(forHTTPHeaderField: "Authorization") + return HTTPStubsResponse(jsonObject: [String: String](), statusCode: 200, headers: nil) + } + let api = try XCTUnwrap(account.wordPressComRestApi) + _ = await api.perform(.get, URLString: "rest/v1.1/me") + XCTAssertEqual(authorization, "Bearer token-b") + } + + func testInvalidTokenResponseClearsTheTokenOfTheRequest() async throws { + let account = try makeDefaultAccount(authToken: "a-token") + stub(condition: isHost("public-api.wordpress.com")) { _ in + HTTPStubsResponse( + jsonObject: ["error": "invalid_token", "message": "The OAuth2 token is invalid."], + statusCode: 400, + headers: nil + ) + } + + let signInExpectation = XCTNSNotificationExpectation( + name: .wpAccountRequiresShowingSigninForWPComFixingAuthToken, + object: account, + notificationCenter: .default + ) + + let api = try XCTUnwrap(account.wordPressComRestApi) + _ = await api.perform(.get, URLString: "rest/v1.1/me") + + await fulfillment(of: [signInExpectation], timeout: 1) + XCTAssertNil(account.authToken) + } + + private func makeDefaultAccount(authToken: String) throws -> WPAccount { + let account = WPAccount.fixture(context: mainContext, authToken: authToken) + try mainContext.save() + UserSettings.defaultDotComUUID = account.uuid + return account + } } diff --git a/Tests/KeystoneTests/Tests/Services/AccountServiceTests.swift b/Tests/KeystoneTests/Tests/Services/AccountServiceTests.swift index f14605d64dbf..cc193120ec9b 100644 --- a/Tests/KeystoneTests/Tests/Services/AccountServiceTests.swift +++ b/Tests/KeystoneTests/Tests/Services/AccountServiceTests.swift @@ -270,4 +270,78 @@ class AccountServiceTests: CoreDataTestCase { XCTAssertEqual(account.username, "jimthetester") XCTAssertEqual(account.email, "jim@wptestaccounts.com") } + + // MARK: - Signing in to a stored account + + func testNewTokenForDefaultAccountPostsAccountChangedWithTheAccount() throws { + let user = makeRemoteUser(userID: 1001) + let accountID = accountService.createOrUpdateAccount(withUserDetails: user, authToken: "old-token") + defer { clearToken(of: accountID) } + + let accountChanged = XCTNSNotificationExpectation( + name: .wpAccountDefaultWordPressComAccountChanged, + object: nil, + notificationCenter: .default + ) + accountChanged.handler = { ($0.object as? WPAccount)?.objectID == accountID } + + _ = accountService.createOrUpdateAccount(withUserDetails: user, authToken: "new-token") + + wait(for: [accountChanged], timeout: 1) + } + + func testWordPressComClientCreatedBeforeNewTokenUsesTheNewToken() throws { + let user = makeRemoteUser(userID: 1002) + let accountID = accountService.createOrUpdateAccount(withUserDetails: user, authToken: "old-token") + defer { clearToken(of: accountID) } + let url = try XCTUnwrap(URL(string: "https://public-api.wordpress.com/rest/v1.1/me")) + let provider = AutoUpdatingWPComAuthenticationProvider(coreDataStack: contextManager) + XCTAssertEqual(provider.authorize(URLRequest(url: url)).value(forHTTPHeaderField: "Authorization"), "Bearer old-token") + + _ = accountService.createOrUpdateAccount(withUserDetails: user, authToken: "new-token") + + XCTAssertEqual(provider.authorize(URLRequest(url: url)).value(forHTTPHeaderField: "Authorization"), "Bearer new-token") + } + + func testSigningInAfterUsernameAndEmailChangeUpdatesTheStoredAccount() throws { + let oldUser = makeRemoteUser(userID: 1003, email: "old@example.com") + let accountID = accountService.createOrUpdateAccount(withUserDetails: oldUser, authToken: "old-token") + defer { clearToken(of: accountID) } + let account = try XCTUnwrap(mainContext.existingObject(with: accountID) as? WPAccount) + account.addBlogs(createMockBlogs(withIDs: [1], in: mainContext)) + contextManager.saveContextAndWait(mainContext) + + let accountRemoved = XCTNSNotificationExpectation( + name: .wpAccountDefaultWordPressComAccountChanged, + object: nil, + notificationCenter: .default + ) + accountRemoved.handler = { $0.object == nil } + accountRemoved.isInverted = true + + let newUser = makeRemoteUser(userID: 1003, email: "new@example.com") + let updatedAccountID = accountService.createOrUpdateAccount(withUserDetails: newUser, authToken: "new-token") + + wait(for: [accountRemoved], timeout: 0.5) + XCTAssertEqual(updatedAccountID, accountID) + try XCTAssertEqual(WPAccount.lookupNumberOfAccounts(in: mainContext), 1) + XCTAssertEqual(account.username, newUser.username) + XCTAssertEqual(account.email, "new@example.com") + XCTAssertEqual(account.blogs?.count, 1) + XCTAssertEqual(try WPAccount.token(forUsername: try XCTUnwrap(newUser.username)), "new-token") + XCTAssertThrowsError(try WPAccount.token(forUsername: try XCTUnwrap(oldUser.username))) + } + + /// A user with a unique username, so tests don't share tokens in the simulator keychain. + private func makeRemoteUser(userID: Int, email: String = "user@example.com") -> RemoteUser { + let user = RemoteUser() + user.userID = NSNumber(value: userID) + user.username = "user-\(UUID().uuidString)" + user.email = email + return user + } + + private func clearToken(of accountID: NSManagedObjectID) { + (try? mainContext.existingObject(with: accountID) as? WPAccount)?.authToken = nil + } } diff --git a/Tests/KeystoneTests/Tests/Services/BlogServiceRemoteSelectionTests.m b/Tests/KeystoneTests/Tests/Services/BlogServiceRemoteSelectionTests.m new file mode 100644 index 000000000000..2c7db3b564f9 --- /dev/null +++ b/Tests/KeystoneTests/Tests/Services/BlogServiceRemoteSelectionTests.m @@ -0,0 +1,105 @@ +#import +#import "BlogService.h" +#import "WordPressTest-Swift.h" + +@import WordPressData; +@import WordPressKit; + +@interface BlogService (RemoteSelection) + +- (nullable id)remoteForBlog:(Blog *)blog; + +@end + +@interface BlogServiceRemoteSelectionTests : XCTestCase + +@property (nonatomic, strong) id coreDataStack; +@property (nonatomic, strong) BlogService *service; + +@end + +@implementation BlogServiceRemoteSelectionTests + +- (void)setUp +{ + [super setUp]; + + self.coreDataStack = [self coreDataStackForTesting]; + self.service = [[BlogService alloc] initWithCoreDataStack:self.coreDataStack]; +} + +- (void)tearDown +{ + self.service = nil; + self.coreDataStack = nil; + + [super tearDown]; +} + +- (void)testRemoteForDotComBlogWithTokenUsesREST +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:@"token" context:self.coreDataStack.mainContext]; + XCTAssertTrue([[self.service remoteForBlog:blog] isKindOfClass:[BlogServiceRemoteREST class]]); +} + +- (void)testNoRemoteForDotComBlogWithoutToken +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:nil context:self.coreDataStack.mainContext]; + XCTAssertNil([self.service remoteForBlog:blog]); +} + +- (void)testNoRemoteForDotComBlogWithEmptyToken +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:@"" context:self.coreDataStack.mainContext]; + XCTAssertNil([self.service remoteForBlog:blog]); +} + +- (void)testRemoteForJetpackBlogWithoutTokenUsesStoredXMLRPCCredentials +{ + Blog *blog = [RemoteSelectionBlogFixture jetpackBlogWithAuthToken:nil xmlrpcPassword:@"password" context:self.coreDataStack.mainContext]; + XCTAssertTrue([[self.service remoteForBlog:blog] isKindOfClass:[BlogServiceRemoteXMLRPC class]]); +} + +- (void)testNoRemoteForJetpackBlogWithoutTokenOrXMLRPCCredentials +{ + Blog *blog = [RemoteSelectionBlogFixture jetpackBlogWithAuthToken:nil xmlrpcPassword:nil context:self.coreDataStack.mainContext]; + XCTAssertNil([self.service remoteForBlog:blog]); +} + +- (void)testRemoteForSelfHostedBlogIsUnaffected +{ + Blog *blog = [RemoteSelectionBlogFixture selfHostedBlogWithContext:self.coreDataStack.mainContext]; + XCTAssertTrue([[self.service remoteForBlog:blog] isKindOfClass:[BlogServiceRemoteXMLRPC class]]); +} + +#pragma mark - Syncing without a remote + +- (void)testSyncBlogAndAllMetadataCompletesWithoutRemote +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:nil context:self.coreDataStack.mainContext]; + XCTestExpectation *completion = [self expectationWithDescription:@"completion"]; + + [self.service syncBlogAndAllMetadata:blog completionHandler:^{ + [completion fulfill]; + }]; + + [self waitForExpectations:@[completion] timeout:1]; +} + +- (void)testSyncPostFormatsFailsWithoutRemote +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:nil context:self.coreDataStack.mainContext]; + XCTestExpectation *failure = [self expectationWithDescription:@"failure"]; + + [self.service syncPostFormatsForBlog:blog success:^{ + XCTFail(@"Syncing without a remote must not succeed"); + } failure:^(NSError *error) { + XCTAssertEqualObjects(error.domain, BlogServiceErrorDomain); + XCTAssertEqual(error.code, BlogServiceErrorNoRemoteForBlog); + [failure fulfill]; + }]; + + [self waitForExpectations:@[failure] timeout:1]; +} + +@end diff --git a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m index c869009d64cc..f90cca2e289b 100644 --- a/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostCategoryServiceTests.m @@ -7,6 +7,12 @@ @import WordPressKit; @import OCMock; +@interface PostCategoryService (RemoteSelection) + +- (nullable id)remoteForBlog:(Blog *)blog; + +@end + @interface PostCategoryServiceForStubbing : PostCategoryService @property (nonatomic, strong) TaxonomyServiceRemoteREST *remoteForStubbing; @@ -226,4 +232,34 @@ - (void)testThatCreateCategoryCallsSuccessWithTheCreatedCategory [self waitForExpectations:@[succeeded, failureNotCalled] timeout:1]; } +#pragma mark - Remote selection + +- (void)testRemoteForDotComBlogWithTokenUsesREST +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:@"token" context:self.manager.mainContext]; + PostCategoryService *service = [[PostCategoryService alloc] initWithCoreDataStack:self.manager]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteREST class]]); +} + +- (void)testNoRemoteForDotComBlogWithoutToken +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:nil context:self.manager.mainContext]; + PostCategoryService *service = [[PostCategoryService alloc] initWithCoreDataStack:self.manager]; + XCTAssertNil([service remoteForBlog:blog]); +} + +- (void)testRemoteForJetpackBlogWithoutTokenUsesStoredXMLRPCCredentials +{ + Blog *blog = [RemoteSelectionBlogFixture jetpackBlogWithAuthToken:nil xmlrpcPassword:@"password" context:self.manager.mainContext]; + PostCategoryService *service = [[PostCategoryService alloc] initWithCoreDataStack:self.manager]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]); +} + +- (void)testRemoteForSelfHostedBlogIsUnaffected +{ + Blog *blog = [RemoteSelectionBlogFixture selfHostedBlogWithContext:self.manager.mainContext]; + PostCategoryService *service = [[PostCategoryService alloc] initWithCoreDataStack:self.manager]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]); +} + @end diff --git a/Tests/KeystoneTests/Tests/Services/PostTagServiceTests.m b/Tests/KeystoneTests/Tests/Services/PostTagServiceTests.m index 47c75be49a0b..36980143f9b3 100644 --- a/Tests/KeystoneTests/Tests/Services/PostTagServiceTests.m +++ b/Tests/KeystoneTests/Tests/Services/PostTagServiceTests.m @@ -7,6 +7,12 @@ @import WordPressKit; @import OCMock; +@interface PostTagService (RemoteSelection) + +- (nullable id)remoteForBlog:(Blog *)blog; + +@end + @interface PostTagServiceForStubbing : PostTagService @property (nonatomic, strong) TaxonomyServiceRemoteREST *remoteForStubbing; @@ -116,4 +122,34 @@ - (void)testThatSearchTagsWithNameWorks failure:^(NSError * _Nonnull __unused error) {}]; } +#pragma mark - Remote selection + +- (void)testRemoteForDotComBlogWithTokenUsesCoreREST +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:@"token" context:self.manager.mainContext]; + PostTagService *service = [[PostTagService alloc] initWithManagedObjectContext:self.manager.mainContext]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteCoreREST class]]); +} + +- (void)testNoRemoteForDotComBlogWithoutToken +{ + Blog *blog = [RemoteSelectionBlogFixture dotComBlogWithAuthToken:nil context:self.manager.mainContext]; + PostTagService *service = [[PostTagService alloc] initWithManagedObjectContext:self.manager.mainContext]; + XCTAssertNil([service remoteForBlog:blog]); +} + +- (void)testRemoteForJetpackBlogWithoutTokenUsesStoredXMLRPCCredentials +{ + Blog *blog = [RemoteSelectionBlogFixture jetpackBlogWithAuthToken:nil xmlrpcPassword:@"password" context:self.manager.mainContext]; + PostTagService *service = [[PostTagService alloc] initWithManagedObjectContext:self.manager.mainContext]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]); +} + +- (void)testRemoteForSelfHostedBlogIsUnaffected +{ + Blog *blog = [RemoteSelectionBlogFixture selfHostedBlogWithContext:self.manager.mainContext]; + PostTagService *service = [[PostTagService alloc] initWithManagedObjectContext:self.manager.mainContext]; + XCTAssertTrue([[service remoteForBlog:blog] isKindOfClass:[TaxonomyServiceRemoteXMLRPC class]]); +} + @end diff --git a/Tests/KeystoneTests/Tests/Services/TokenlessAccountRemoteFactoryTests.swift b/Tests/KeystoneTests/Tests/Services/TokenlessAccountRemoteFactoryTests.swift new file mode 100644 index 000000000000..14816d54c090 --- /dev/null +++ b/Tests/KeystoneTests/Tests/Services/TokenlessAccountRemoteFactoryTests.swift @@ -0,0 +1,59 @@ +import Foundation +import Testing +import WordPressData +import WordPressKit + +@testable import WordPress + +@MainActor +struct TokenlessAccountRemoteFactoryTests { + private let context = ContextManager.forTesting().mainContext + + // MARK: - Media + + @Test func mediaRemoteForDotComBlogWithTokenUsesREST() throws { + let blog = RemoteSelectionBlogFixture.dotComBlog(authToken: "token", context: context) + #expect(try MediaServiceRemoteFactory().remote(for: blog) is MediaServiceRemoteREST) + } + + @Test func noMediaRemoteForDotComBlogWithoutToken() { + let blog = RemoteSelectionBlogFixture.dotComBlog(authToken: nil, context: context) + #expect(throws: MediaRepository.Error.remoteAPIUnavailable) { + try MediaServiceRemoteFactory().remote(for: blog) + } + } + + @Test func mediaRemoteForJetpackBlogWithoutTokenUsesStoredXMLRPCCredentials() throws { + let blog = RemoteSelectionBlogFixture.jetpackBlog(authToken: nil, xmlrpcPassword: "password", context: context) + #expect(try MediaServiceRemoteFactory().remote(for: blog) is MediaServiceRemoteXMLRPC) + } + + @Test func noMediaRemoteForJetpackBlogWithoutTokenOrXMLRPCCredentials() { + let blog = RemoteSelectionBlogFixture.jetpackBlog(authToken: nil, xmlrpcPassword: nil, context: context) + #expect(throws: MediaRepository.Error.remoteAPIUnavailable) { + try MediaServiceRemoteFactory().remote(for: blog) + } + } + + // MARK: - Comments + + @Test func commentRemoteForDotComBlogWithTokenUsesREST() { + let blog = RemoteSelectionBlogFixture.dotComBlog(authToken: "token", context: context) + #expect(CommentServiceRemoteFactory().remote(blog: blog) is CommentServiceRemoteREST) + } + + @Test func noCommentRemoteForDotComBlogWithoutToken() { + let blog = RemoteSelectionBlogFixture.dotComBlog(authToken: nil, context: context) + #expect(CommentServiceRemoteFactory().remote(blog: blog) == nil) + } + + @Test func commentRemoteForJetpackBlogWithoutTokenUsesStoredXMLRPCCredentials() { + let blog = RemoteSelectionBlogFixture.jetpackBlog(authToken: nil, xmlrpcPassword: "password", context: context) + #expect(CommentServiceRemoteFactory().remote(blog: blog) is CommentServiceRemoteXMLRPC) + } + + @Test func commentRemoteForSelfHostedBlogIsUnaffected() { + let blog = RemoteSelectionBlogFixture.selfHostedBlog(context: context) + #expect(CommentServiceRemoteFactory().remote(blog: blog) is CommentServiceRemoteXMLRPC) + } +} diff --git a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift index 3f1a8c8645b6..fcf868a35fd8 100644 --- a/WordPress/Classes/Login/WordPressDotComAuthenticator.swift +++ b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift @@ -19,7 +19,9 @@ struct WordPressDotComAuthenticator { case `default` // Sign in to an existing account. // This is typically used when the app needs to get a new OAuth token because the current one becomes invalid. - case reauthentication(accountEmail: String?) + // The account is identified by its WordPress.com user ID. The email only prefills the login form, because + // the stored email can be outdated: the user may have changed it after the app last synced account details. + case reauthentication(userID: NSNumber?, emailHint: String?) // Connect a site to Jetpack or sign in to an already connected site. case jetpackSite(accountEmail: String?) // Sign in from the support screen, which needs a WordPress.com account. @@ -30,7 +32,7 @@ struct WordPressDotComAuthenticator { switch self { case .default, .support: return nil - case let .reauthentication(email), let .jetpackSite(email): + case let .reauthentication(_, email), let .jetpackSite(email): return email } } @@ -50,7 +52,9 @@ struct WordPressDotComAuthenticator { enum SignInError: Error { case authentication(AuthenticationError) case fetchUser(Error) - case mismatchedEmail(expectedEmail: String) + /// The user signed in to a different account than the one the context expects. `expectedEmail` is the + /// expected account's email, if known. + case mismatchedAccount(expectedEmail: String?) case alreadySignedIn(signedInAccountEmail: String) case loadingSites(Error) } @@ -174,12 +178,24 @@ struct WordPressDotComAuthenticator { throw .fetchUser(error) } - // Make sure the signed-in account matches the given `accountEmail` argument. - if let email = context.accountEmail(in: coreDataStack.mainContext), user.email != email { - throw .mismatchedEmail(expectedEmail: email) + // Make sure the signed-in account is the expected one. + switch context { + case .default: + break + case let .reauthentication(userID, emailHint): + if let userID, user.userID != userID { + throw .mismatchedAccount(expectedEmail: emailHint) + } + case let .jetpackSite(email): + if let email, user.email != email { + throw .mismatchedAccount(expectedEmail: email) + } } - if let defaultAccountEmail = defaultAccount?.email, defaultAccountEmail != user.email { + if let defaultAccount, + !Self.isSameUser(user, as: defaultAccount), + let defaultAccountEmail = defaultAccount.email + { throw .alreadySignedIn(signedInAccountEmail: defaultAccountEmail) } @@ -214,6 +230,15 @@ struct WordPressDotComAuthenticator { return accountID } + /// Compares user IDs, because the email of an account can change on WordPress.com. Falls back to the email for + /// an account stored without a user ID. + private static func isSameUser(_ user: RemoteUser, as account: WPAccount) -> Bool { + if let userID = account.userID { + return user.userID == userID + } + return user.email == account.email + } + func present(_ error: SignInError, from viewController: UIViewController) { guard let alertMessage = error.alertMessage else { return } @@ -479,8 +504,8 @@ private extension WordPressDotComAuthenticator.SignInError { return error.alertMessage case .fetchUser: return Strings.fetchUserError - case let .mismatchedEmail(expectedEmail): - return Strings.mismatchedEmail(expectedEmail) + case let .mismatchedAccount(expectedEmail): + return expectedEmail.map(Strings.mismatchedEmail) ?? Strings.mismatchedAccount case let .alreadySignedIn(signedInAccountEmail): return Strings.alreadySignedIn(signedInAccountEmail) case .loadingSites: @@ -543,6 +568,11 @@ private enum Strings { comment: "Error message when user signs in with an unexpected email address. The first argument is the expected email address" ) + static let mismatchedAccount = NSLocalizedString( + "wpComLogin.error.mismatchedAccount", + value: "Please sign in with the WordPress.com account you used before.", + comment: "Error message when user signs in with a different WordPress.com account than the one the app expects" + ) static let alreadySignedIn = NSLocalizedString( "wpComLogin.error.alreadySignedIn", value: "You have already signed in with email address %@. Please sign out try again.", diff --git a/WordPress/Classes/Services/AccountService+Swift.swift b/WordPress/Classes/Services/AccountService+Swift.swift index 9161d68ea4d9..cd46141c62aa 100644 --- a/WordPress/Classes/Services/AccountService+Swift.swift +++ b/WordPress/Classes/Services/AccountService+Swift.swift @@ -21,27 +21,60 @@ extension AccountService { UserPersistentStoreFactory.instance() .set(account.uuid, forKey: AccountService.defaultDotcomAccountUUIDDefaultsKey) - let objectID = TaggedManagedObjectID(account) - let notifyAccountChange = { - let context = self.coreDataStack.mainContext - let account = try? context.existingObject(with: objectID) - NotificationCenter - .default - .post(name: .wpAccountDefaultWordPressComAccountChanged, object: account) - + postDefaultAccountChanged(accountID: account.objectID) { PushNotificationsManager.shared.setupRemoteNotifications() } + } + + /// Posts `wpAccountDefaultWordPressComAccountChanged` on the main thread with the account as the object, then + /// runs `completion`. + /// + /// Nothing is posted if the account is gone: observers treat the notification without an object as a logout. + private func postDefaultAccountChanged(accountID: NSManagedObjectID, completion: (() -> Void)? = nil) { + let notify = { + if let account = try? self.coreDataStack.mainContext.existingObject(with: accountID) { + NotificationCenter.default.post(name: .wpAccountDefaultWordPressComAccountChanged, object: account) + } + completion?() + } if Thread.isMainThread { // This is meant to help with testing account observers. // Short version: dispatch_async and XCTest asynchronous helpers don't play nice with each other // Long version: see the comment in https://github.com/wordpress-mobile/WordPress-iOS/blob/2f9a2100ca69d8f455acec47a1bbd6cbc5084546/WordPress/WordPressTest/AccountServiceRxTests.swift#L7 - notifyAccountChange() + notify() } else { - DispatchQueue.main.async(execute: notifyAccountChange) + DispatchQueue.main.async(execute: notify) } } + /// The stored account of a WordPress.com user: the one with the user's ID, or else the one with the username. + /// + /// The username can change on WordPress.com. A lookup by username alone would add a second account for the + /// same user, and the next blog sync would remove the first one together with its sites and local drafts. + @objc(existingAccountForRemoteUser:inContext:) + func existingAccount(for remoteUser: RemoteUser, in context: NSManagedObjectContext) -> WPAccount? { + if let userID = remoteUser.userID?.int64Value, + let account = try? WPAccount.lookup(withUserID: userID, in: context) + { + return account + } + guard let username = remoteUser.username else { + return nil + } + return try? WPAccount.lookup(withUsername: username, in: context) + } + + /// Tells observers that the default account has a new token. + /// + /// `setDefaultWordPressComAccount` does not post for an account that is already the default, so without this + /// a re-authentication of the default account would leave observers, such as the wordpress-rs authentication + /// provider, with the rejected token. + @objc(postDefaultAccountTokenReplacedForAccountID:) + func postDefaultAccountTokenReplaced(accountID: NSManagedObjectID) { + postDefaultAccountChanged(accountID: accountID) + } + func removeDefaultWordPressComAccount() { wpAssert(Thread.isMainThread, "Must be called from main thread") diff --git a/WordPress/Classes/Services/AccountService.m b/WordPress/Classes/Services/AccountService.m index 9a8fe6df5f16..b8386e9ce79d 100644 --- a/WordPress/Classes/Services/AccountService.m +++ b/WordPress/Classes/Services/AccountService.m @@ -61,16 +61,21 @@ - (NSManagedObjectID *)createOrUpdateAccountWithUserDetails:(RemoteUser *)remote { NSManagedObjectID * __block accountObjectID = nil; [self.coreDataStack.mainContext performBlockAndWait:^{ - accountObjectID = [[WPAccount lookupWithUsername:remoteUser.username context:self.coreDataStack.mainContext] objectID]; + accountObjectID = [[self existingAccountForRemoteUser:remoteUser inContext:self.coreDataStack.mainContext] objectID]; }]; + BOOL __block replacedDefaultAccountToken = NO; if (accountObjectID) { [self.coreDataStack performAndSaveUsingBlock:^(NSManagedObjectContext *context) { WPAccount *account = [context existingObjectWithID:accountObjectID error:nil]; + // Setting a changed username moves the stored token to it, so do it before replacing the token. + account.username = remoteUser.username; + NSString *previousToken = account.authToken; // Even if we find an account via its userID we should still update // its authtoken, otherwise the Authenticator's authtoken fixer won't // work. account.authToken = authToken; + replacedDefaultAccountToken = account.isDefaultWordPressComAccount && ![previousToken isEqualToString:authToken]; }]; } else { accountObjectID = [self createOrUpdateAccountWithUsername:remoteUser.username authToken:authToken]; @@ -78,6 +83,10 @@ - (NSManagedObjectID *)createOrUpdateAccountWithUserDetails:(RemoteUser *)remote [self updateAccountWithID:accountObjectID withUserDetails:remoteUser]; + if (replacedDefaultAccountToken) { + [self postDefaultAccountTokenReplacedForAccountID:accountObjectID]; + } + return accountObjectID; } diff --git a/WordPress/Classes/Services/BlogService.h b/WordPress/Classes/Services/BlogService.h index c47f8ab1f802..5b7b22bb6f61 100644 --- a/WordPress/Classes/Services/BlogService.h +++ b/WordPress/Classes/Services/BlogService.h @@ -6,6 +6,12 @@ extern NSString *const WordPressMinimumVersion; extern NSString *const WPBlogUpdatedNotification; extern NSString *const WPBlogSettingsUpdatedNotification; +extern NSErrorDomain const BlogServiceErrorDomain; +typedef NS_ERROR_ENUM(BlogServiceErrorDomain, BlogServiceError) { + /// The blog has no remote, for example because its WordPress.com account has no token. + BlogServiceErrorNoRemoteForBlog = 1, +}; + @class Blog; @class WPAccount; @class SiteInfo; diff --git a/WordPress/Classes/Services/BlogService.m b/WordPress/Classes/Services/BlogService.m index af964fce39df..a329d155fb3f 100644 --- a/WordPress/Classes/Services/BlogService.m +++ b/WordPress/Classes/Services/BlogService.m @@ -15,6 +15,7 @@ NSString *const WordPressMinimumVersion = @"4.0"; NSString *const WPBlogUpdatedNotification = @"WPBlogUpdatedNotification"; NSString *const WPBlogSettingsUpdatedNotification = @"WPBlogSettingsUpdatedNotification"; +NSErrorDomain const BlogServiceErrorDomain = @"BlogServiceErrorDomain"; @implementation BlogService @@ -90,6 +91,14 @@ - (void)syncBlogAndAllMetadata:(Blog *)blog completionHandler:(void (^)(void))co NSManagedObjectID *blogObjectID = blog.objectID; id remote = [self remoteForBlog:blog]; + if (!remote) { + // The group below waits for callbacks that a nil remote never calls. + DDLogError(@"Failed syncing blog %@: no remote", blog.url); + if (completionHandler) { + completionHandler(); + } + return; + } dispatch_group_enter(syncGroup); [self fetchAndPersistSettingsForBlog:blog completion:^(NSError *error) { @@ -244,6 +253,10 @@ - (void)syncAuthorsForBlog:(Blog *)blog { NSManagedObjectID *blogObjectID = blog.objectID; id remote = [self remoteForBlog:blog]; + if (!remote) { + failure([self noRemoteErrorForBlog:blog]); + return; + } [remote getAllAuthorsWithSuccess:^(NSArray *users) { [self updateMultiAuthor:users forBlog:blogObjectID completionHandler:success]; @@ -259,6 +272,10 @@ - (void)syncPostTypesForBlog:(Blog *)blog { NSManagedObjectID *blogObjectID = blog.objectID; id remote = [self remoteForBlog:blog]; + if (!remote) { + failure([self noRemoteErrorForBlog:blog]); + return; + } [remote syncPostTypesWithSuccess:^(NSArray *remotePostTypes) { [self.coreDataStack performAndSaveUsingBlock:^(NSManagedObjectContext *context) { NSError *blogError; @@ -294,6 +311,10 @@ - (void)syncPostFormatsForBlog:(Blog *)blog failure:(void (^)(NSError *error))failure { id remote = [self remoteForBlog:blog]; + if (!remote) { + failure([self noRemoteErrorForBlog:blog]); + return; + } [remote syncPostFormatsWithSuccess:[self postFormatsHandlerWithBlogObjectID:blog.objectID completionHandler:success] failure:failure]; @@ -518,6 +539,10 @@ - (Blog *)migrateRemoteJetpackBlog:(RemoteBlog *)remoteBlog } } + if (blog.isMissingRequiredWordPressComToken) { + return nil; + } + BlogServiceRemoteCoreREST *restApi = [[BlogServiceRemoteCoreREST alloc] initWithBlog:blog]; if (restApi != nil) { return restApi; @@ -530,6 +555,13 @@ - (Blog *)migrateRemoteJetpackBlog:(RemoteBlog *)remoteBlog return nil; } +- (NSError *)noRemoteErrorForBlog:(Blog *)blog +{ + return [NSError errorWithDomain:BlogServiceErrorDomain + code:BlogServiceErrorNoRemoteForBlog + userInfo:@{NSDebugDescriptionErrorKey: [NSString stringWithFormat:@"No remote for blog %@", blog.url]}]; +} + - (id)remoteForAccount:(WPAccount *)account { if (account.wordPressComRestApi == nil) { diff --git a/WordPress/Classes/Services/CommentServiceRemoteFactory.swift b/WordPress/Classes/Services/CommentServiceRemoteFactory.swift index eb7dfe85d57e..7b4ad95291cb 100644 --- a/WordPress/Classes/Services/CommentServiceRemoteFactory.swift +++ b/WordPress/Classes/Services/CommentServiceRemoteFactory.swift @@ -23,6 +23,10 @@ import WordPressShared return CommentServiceRemoteREST(wordPressComRestApi: api, siteID: dotComID) } + if blog.isMissingRequiredWordPressComToken { + return nil + } + // The REST API does not have information about comment "likes". We'll continue to use WordPress.com API for now. if let site = try? WordPressSite(blog: blog) { return CommentServiceRemoteCoreRESTAPI(client: WordPressClientFactory.shared.instance(for: site)) diff --git a/WordPress/Classes/Services/MediaRepository.swift b/WordPress/Classes/Services/MediaRepository.swift index 97c84e7a9262..95a168a47079 100644 --- a/WordPress/Classes/Services/MediaRepository.swift +++ b/WordPress/Classes/Services/MediaRepository.swift @@ -94,6 +94,10 @@ private extension MediaRepository { return MediaServiceRemoteREST(wordPressComRestApi: api, siteID: dotComID) } + if blog.isMissingRequiredWordPressComToken { + throw MediaRepository.Error.remoteAPIUnavailable + } + // We use WordPress.com API for media management instead of WordPress core REST API to ensure // compatibility with WordPress.com-specific features such as video upload restrictions // and storage limits based on the site's plan. diff --git a/WordPress/Classes/Services/PostCategoryService.m b/WordPress/Classes/Services/PostCategoryService.m index 104103a39d90..579e6c00f548 100644 --- a/WordPress/Classes/Services/PostCategoryService.m +++ b/WordPress/Classes/Services/PostCategoryService.m @@ -198,20 +198,20 @@ - (void)mergeCategories:(NSArray *)remoteCategories forBl } - (nullable id)remoteForBlog:(Blog *)blog { - if ([blog supports:BlogFeatureWpComRESTAPI]) { - if (blog.wordPressComRestApi) { - return [[TaxonomyServiceRemoteREST alloc] initWithWordPressComRestApi:blog.wordPressComRestApi siteID:blog.dotComID]; - } - } else { - // Prefer the core REST API (wp/v2) for self-hosted sites. Some hosts - // block the XML-RPC term methods, which breaks categories (#25758). - TaxonomyServiceRemoteCoreREST *coreREST = [[TaxonomyServiceRemoteCoreREST alloc] initWithBlog:blog]; - if (coreREST) { - return coreREST; - } - if (blog.xmlrpcApi) { - return [[TaxonomyServiceRemoteXMLRPC alloc] initWithApi:blog.xmlrpcApi username:blog.username password:blog.password]; - } + if ([blog supports:BlogFeatureWpComRESTAPI] && blog.wordPressComRestApi) { + return [[TaxonomyServiceRemoteREST alloc] initWithWordPressComRestApi:blog.wordPressComRestApi siteID:blog.dotComID]; + } + if (blog.isMissingRequiredWordPressComToken) { + return nil; + } + // Prefer the core REST API (wp/v2) for self-hosted sites. Some hosts + // block the XML-RPC term methods, which breaks categories (#25758). + TaxonomyServiceRemoteCoreREST *coreREST = [[TaxonomyServiceRemoteCoreREST alloc] initWithBlog:blog]; + if (coreREST) { + return coreREST; + } + if (blog.xmlrpcApi) { + return [[TaxonomyServiceRemoteXMLRPC alloc] initWithApi:blog.xmlrpcApi username:blog.username password:blog.password]; } return nil; } diff --git a/WordPress/Classes/Services/PostTagService.m b/WordPress/Classes/Services/PostTagService.m index 8e25ae06de31..232ad0a80286 100644 --- a/WordPress/Classes/Services/PostTagService.m +++ b/WordPress/Classes/Services/PostTagService.m @@ -173,11 +173,13 @@ - (void)saveTag:(PostTag*)tag return instance; } - if ([blog supports:BlogFeatureWpComRESTAPI]) { - if (blog.wordPressComRestApi) { - return [[TaxonomyServiceRemoteREST alloc] initWithWordPressComRestApi:blog.wordPressComRestApi siteID:blog.dotComID]; - } - } else if (blog.xmlrpcApi) { + if ([blog supports:BlogFeatureWpComRESTAPI] && blog.wordPressComRestApi) { + return [[TaxonomyServiceRemoteREST alloc] initWithWordPressComRestApi:blog.wordPressComRestApi siteID:blog.dotComID]; + } + if (blog.isMissingRequiredWordPressComToken) { + return nil; + } + if (blog.xmlrpcApi) { return [[TaxonomyServiceRemoteXMLRPC alloc] initWithApi:blog.xmlrpcApi username:blog.username password:blog.password]; } return nil; diff --git a/WordPress/Classes/ViewRelated/NUX/Helpers/WordPressAuthenticationManager.swift b/WordPress/Classes/ViewRelated/NUX/Helpers/WordPressAuthenticationManager.swift index f55b5659d39a..ecbf2293781b 100644 --- a/WordPress/Classes/ViewRelated/NUX/Helpers/WordPressAuthenticationManager.swift +++ b/WordPress/Classes/ViewRelated/NUX/Helpers/WordPressAuthenticationManager.swift @@ -101,8 +101,8 @@ extension WordPressAuthenticationManager { let account = await WordPressDotComAuthenticator() .signIn( from: presenter, - context: signedInAccount?.email - .flatMap { .reauthentication(accountEmail: $0) } + context: signedInAccount + .map { .reauthentication(userID: $0.userID, emailHint: $0.email) } ?? .default )