From dfb7f7b55726344b564db6daf44a432db63392ca Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 12 Oct 2026 22:25:47 +1300 Subject: [PATCH 1/2] Return no remote for blogs whose WordPress.com account has no token A WordPress.com blog, or a blog without a stored XML-RPC password, can only be reached with its account's token. Without the token, the remote factories fell through to Core REST and XML-RPC with empty credentials, which sent useless requests in Release and hit assertions in Debug. Those factories now return no remote, and blog syncs that wait on remote callbacks finish with an error instead of hanging. Jetpack sites with stored XML-RPC credentials keep using XML-RPC. --- .../Swift/Blog+WordPressComRestAPI.swift | 18 +++ .../Swift/WordPressOrgRestApi+WordPress.swift | 4 +- .../BlogTokenlessAccountTests.swift | 59 ++++++++++ .../Helpers/RemoteSelectionBlogFixture.swift | 60 ++++++++++ .../BlogServiceRemoteSelectionTests.m | 105 ++++++++++++++++++ .../Tests/Services/PostCategoryServiceTests.m | 36 ++++++ .../Tests/Services/PostTagServiceTests.m | 36 ++++++ .../TokenlessAccountRemoteFactoryTests.swift | 59 ++++++++++ WordPress/Classes/Services/BlogService.h | 6 + WordPress/Classes/Services/BlogService.m | 32 ++++++ .../CommentServiceRemoteFactory.swift | 4 + .../Classes/Services/MediaRepository.swift | 4 + .../Classes/Services/PostCategoryService.m | 28 ++--- WordPress/Classes/Services/PostTagService.m | 12 +- 14 files changed, 443 insertions(+), 20 deletions(-) create mode 100644 Modules/Tests/WordPressDataTests/BlogTokenlessAccountTests.swift create mode 100644 Tests/KeystoneTests/Helpers/RemoteSelectionBlogFixture.swift create mode 100644 Tests/KeystoneTests/Tests/Services/BlogServiceRemoteSelectionTests.m create mode 100644 Tests/KeystoneTests/Tests/Services/TokenlessAccountRemoteFactoryTests.swift 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/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/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/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 b2fa70746ff6..5d89dadd630a 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 11cb125ced74..dcd85cbcc767 100644 --- a/WordPress/Classes/Services/PostTagService.m +++ b/WordPress/Classes/Services/PostTagService.m @@ -172,11 +172,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; From 275f5bf23ffaf0bea89aa0d03fe4e89d40f315d3 Mon Sep 17 00:00:00 2001 From: Tony Li Date: Mon, 12 Oct 2026 22:32:35 +1300 Subject: [PATCH 2/2] Stop treating a rejected WordPress.com token as a logout The invalid-token handler no longer posts the account-changed notification, which made the app wipe Reader data and extension configuration while the account still existed. It also ignores a rejection of a token that is no longer current, comparing against the keychain, so a late failure cannot erase a new token. Signing in again finds the stored account by user ID, updates it in place, and announces the new token so observers such as the wordpress-rs client use it. Re-authentication checks the user ID instead of the email. --- .../Swift/WPAccount+RestApi.swift | 50 ++++--- .../WordPressData/Swift/WPAccount.swift | 25 +++- .../WordPressDotComAuthenticatorTests.swift | 49 +++++- .../Tests/Models/WPAccount+Fixture.swift | 5 +- .../Tests/Models/WPAccount+RestApiTests.swift | 140 ++++++++++++++++++ .../Tests/Services/AccountServiceTests.swift | 74 +++++++++ .../Login/WordPressDotComAuthenticator.swift | 48 ++++-- .../Services/AccountService+Swift.swift | 53 +++++-- WordPress/Classes/Services/AccountService.m | 11 +- .../WordPressAuthenticationManager.swift | 4 +- 10 files changed, 403 insertions(+), 56 deletions(-) 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/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/WordPress/Classes/Login/WordPressDotComAuthenticator.swift b/WordPress/Classes/Login/WordPressDotComAuthenticator.swift index 93aefe36cfbb..abbcc1e84ada 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?) @@ -27,7 +29,7 @@ struct WordPressDotComAuthenticator { switch self { case .default: return nil - case let .reauthentication(email), let .jetpackSite(email): + case let .reauthentication(_, email), let .jetpackSite(email): return email } } @@ -47,7 +49,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) } @@ -171,12 +175,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) } @@ -211,6 +227,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 } @@ -476,8 +501,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: @@ -540,6 +565,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 8a669e174e30..b6714b4023b1 100644 --- a/WordPress/Classes/Services/AccountService.m +++ b/WordPress/Classes/Services/AccountService.m @@ -62,16 +62,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]; @@ -79,6 +84,10 @@ - (NSManagedObjectID *)createOrUpdateAccountWithUserDetails:(RemoteUser *)remote [self updateAccountWithID:accountObjectID withUserDetails:remoteUser]; + if (replacedDefaultAccountToken) { + [self postDefaultAccountTokenReplacedForAccountID:accountObjectID]; + } + return accountObjectID; } 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 )