Repository navigation
Handle WordPress.com accounts whose token is missing or rejected - #26132
crazytonyli wants to merge 6 commits into
Conversation
Generated by 🚫 Danger |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 34900 | |
| Version | PR #26132 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 275f5bf | |
| Installation URL | 4mshb1bi2jos0 |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 34900 | |
| Version | PR #26132 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 275f5bf | |
| Installation URL | 4mtn73tc2n59g |
|
Version |
jkmassel
left a comment
jkmassel
left a comment
There was a problem hiding this comment.
Reviewed at 275f5bf23f. CI is green.
The core fix looks right:
- A rejected token no longer wipes Reader data or drops the user to the full-screen sign-in.
- Signing in again updates the stored account by user ID.
- A late rejection of the old token can't erase its replacement, and a test covers that case.
The one serious problem is #26228, which is still present on this branch. Its fix, #26301, is open and not merged, so the two need to ship together in 27.5.
1. Reader tab breaks after signing in again (#26228)
What happens: signing in again posts wpAccountDefaultWordPressComAccountChanged with the account attached. WPTabBarController reacts by dropping its Reader presenter, and it never installs a new one.
What users see: on iPhone, the Reader menu does nothing, and links into the Reader only switch tabs. This lasts until the app is relaunched.
How this was checked: two unit tests failed 3 of 3 runs on this PR and passed on trunk. Side-by-side simulator runs confirmed it. Details are in #26228.
2. Low: PingHub keeps retrying with the rejected token
What happens: on trunk, a rejected token posts the account-changed notification with no account, which resets PingHub. This PR removes that post. PingHubManager.accountChanged is PingHub's only way to pick up a new token.
Effect: while the sign-in prompt is open, PingHub keeps reconnecting with the dead token, at most every 30 s. It recovers when the user signs in again, logs out, or relaunches the app.
How this was checked: by reading the code only. It was not run.
3. Low: category and tag syncs never call back when a site has no remote
What happens: for a WordPress.com site whose account has no token, PostCategoryService and PostTagService now return no remote. Their callers then message nil, so neither success nor failure ever runs. BlogService got nil checks for this; these callers didn't.
On trunk, these services built an XML-RPC remote with a missing password instead. That hit an assertion in Debug and called failure in Release.
Effect: a screen waiting on one of these calls, such as the category picker, would spin forever.
Who is affected: probably almost nobody. The re-sign-in alert (Log out / Sign In) is modal, so the user has to pick one before using the app.
Suggested fix: call failure when the remote is nil, as BlogService now does.
CommentService behaves the same as on trunk. There, building the site's WordPressSite already throws without a token, and XML-RPC needs a password, so trunk already ends up with no remote.
4. Nits from AGENTS.md
- The new
BlogServiceRemoteSelectionTests.mis an Objective-C XCTest file. The rule is "Use Swift Testing for new tests". BlogService.m:96adds aDDLogErrorcall. New code should use swift-log.
Checked and ruled out
| Concern | Why it isn't a problem |
|---|---|
performAndWait in the invalid-token handler can deadlock |
The handler runs on main. The new token is written synchronously from the @MainActor syncWPCom, so the code comment saying they can't overlap is accurate. A deadlock would need a background context waiting on main at that same moment. We found no such path. |
| A failed keychain read makes the handler ignore a real rejection | It only delays the prompt. The next request reads a nil token, and wordPressComRestApi then asks for sign-in. |
An empty "" token would reach Core REST in PostTagService |
No real code path stores an empty token. |
| Accounts stored without a user ID or an email | Every account-creation path stores the user ID from /me. Accounts with no email behave as they did before. |
Notifications reset and the defaultAccountChanged event |
They moved from token rejection to re-sign-in, so the overall effect is unchanged. |
| Widgets and migration cleanup | Both only act on logout, so nothing changes for them. |
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.
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.
275f5bf to
269db50
Compare
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
Trunk added the support sign-in context, which skips the account check like the default context. It also removed CocoaLumberjack from Objective-C, so the new log call goes through WPLog.
A rejected token no longer posts the account-changed notification that PingHub relied on. PingHub now also drops its client when the app asks for the WordPress.com sign-in and the default account's token has changed.
Without a remote, these requests messaged nil and never called back, so a screen waiting on one could spin forever. Deleting a tag now also keeps it locally when the server cannot be reached.
|
The first issue about Reader is potentially related to https://linear.app/a8c/issue/CMM-2581. I have leave that out, and will address it properly in a separate PR. |
Description
Losing a WordPress.com token should let the user sign in again with their data intact. Currently, token rejection triggers logout cleanup even though the account still exists. Signing in again can also create a duplicate account after a username change, causing the old account's sites and local drafts to be removed.
Fixes #26158