Allow headless application-password creation on Atomic sites#22885
Conversation
Generated by 🚫 Danger |
|
|
|
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #22885 +/- ##
==========================================
+ Coverage 37.32% 37.34% +0.01%
==========================================
Files 2319 2320 +1
Lines 124585 124653 +68
Branches 16928 16941 +13
==========================================
+ Hits 46506 46549 +43
- Misses 74318 74339 +21
- Partials 3761 3765 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
jkmassel
force-pushed
the
jkmassel/issue-22884
branch
2 times, most recently
from
1888111 to
912dd2e
Compare
jkmassel
force-pushed
the
jkmassel/issue-22884
branch
from
912dd2e to
a123523
Compare
jkmassel
changed the base branch from
trunk
to
jkmassel/issue-22884-list-screen-direct-host
jkmassel
force-pushed
the
jkmassel/issue-22884
branch
from
dc9ff28 to
cc93500
Compare
jkmassel
force-pushed
the
jkmassel/issue-22884
branch
from
cc93500 to
80b1ebf
Compare
There was a problem hiding this comment.
I left a few comments, but none of them are blockers.
I've tested what I could, and almost everything worked as expected. I found one issue, where the Invalid Application Password dialogue couldn't be dismissed.
invalid_password.webm
I think this ^ issue was introduced in this PR. Here is how I produced it:
- Add a self-hosted site with application password
- Revoke the application password online
- Pull the My Site screen to refresh
Note that I couldn't test 2 things:
- For atomic site, I couldn't check the token server side, but the flow worked as expected. (I can't login to the site on the web for some reason)
- I couldn't test tapping the banner because I couldn't dismiss the
Invalid Application Passworddialogue.
| private var processedAppPasswordData: String? = null | ||
|
|
||
| sealed class DiscoveryResult { | ||
| data class Authorized(val authorizationUrl: String) : DiscoveryResult() |
There was a problem hiding this comment.
I had to do a double take while reviewing the diff in WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/ApplicationPasswordAutoAuthDialogViewModel.kt, specifically this part:
when (val result = applicationPasswordLoginHelper.getAuthorizationUrlComplete(siteUrl)) {
is ApplicationPasswordLoginHelper.DiscoveryResult.Authorized ->
_navigationEvent.emit(NavigationEvent.FallbackToManualLogin(result.authorizationUrl))
Authorized > Fallback confused me a little bit. That Authorized just means the discovery succeeded, so the implementation looks correct to me. However, if we change the name to Success or Discovered, it might reduce some of the friction at call sites. Wdyt?
There was a problem hiding this comment.
Yeah, I think this could be improved in the library for sure.
There was a problem hiding this comment.
I actually meant maybe we wanted to rename DiscoveryResult::Authorized sealed class variant, but either way it was a small issue.
| } | ||
| } | ||
|
|
||
| enum class Outcome { Valid, Invalid, NetworkUnavailable } |
There was a problem hiding this comment.
I believe this is being improved on in a subsequent PR, so I am just noting my thoughts. There might be a few cases where we return Outcome.Invalid when it's really Outcome.Unreliable (for lack of a better term) and I wonder if it'd be better if we were strict about when something is Invalid and go with Unreliable/NeedsRetry type of outcome for others.
Maybe NetworkUnavailable covers everything we need, and expanding on that in the subsequent PR will get us exactly where we want to be. I used a different term above, because I wasn't sure if network availability is the only case where we'd want to retry vs invalidate.
| } | ||
|
|
||
| // Validate credentials by making a simple API call | ||
| scope.launch { |
There was a problem hiding this comment.
This is an existing issue, but we are not handling cancellations so it results in double logs like so:
A_P: Headless mint succeeded for https://ouzgutenbergkitatomicsite.wpcomstaging.com
A_P: Hiding card for https://ouzgutenbergkitatomicsite.wpcomstaging.com - authenticated
A_P: Headless mint succeeded for https://ouzgutenbergkitatomicsite.wpcomstaging.com
A_P: Hiding card for https://ouzgutenbergkitatomicsite.wpcomstaging.com - authenticated
I think this might mean that we are creating multiple application passwords on the server side. For some reason I can't login to the site with my WordPress.com account to confirm.
ApplicationPasswordViewModelSlice.buildCard fires from MySiteViewModel's onResume / refresh / onSitePicked. Without job tracking, two close-together calls race: both pass ApplicationPasswordsManager's `existingPassword == null` check and issue separate server-side mints. The 409 conflict handler then deletes-and-recreates the winner's password, so the losing racer destroys the credential it just helped create. Pre-PR the slice raced on validate-only, which had no server-side side effect. The new mint step in #22885 makes the race visible and harmful. Fix: track the in-flight Job and drop subsequent calls while it's active. The running call posts its result regardless, so the user-visible state still updates. Test gates `createApplicationPassword` on a `CompletableDeferred`, fires buildCard twice, and verifies a single mint invocation reaches the store.
|
The dialog issue should also be addressed by #22893 |
Fixes #22884. `ApplicationPasswordsManager.getApplicationCredentials` returned `NotSupported` for any `site.isWPCom` site. The guard was correct for Simple sites but blocked Atomic sites, which are also `isWPCom`-flagged and do support REST application-password creation. Users on Atomic saw the "Authenticate using Application Password" card on My Site and had to authorize through a Chrome Custom Tab even though the app could mint the credential on their behalf. Relax the FluxC guard from `site.isWPCom` to `site.isWPComSimpleSite` and add a uniform validate-then-mint pipeline to `ApplicationPasswordViewModelSlice`: validate stored creds via wordpress-rs Basic auth against the direct host (new `ApplicationPasswordValidator`, using `WpApiClientProvider.getApplicationPasswordClient` from #22894); on Invalid, clear them via a new `SiteStore.deleteStoredApplicationPasswordCredentials` and fall through to mint via a new `SiteStore.createApplicationPassword` (FluxC Jetpack tunnel); on mint failure, the existing discovery + card path takes over. The XML-RPC-disabled card path is now gated on `!isUsingWpComRestApi` so it only fires for true self-hosted sites. Also provides the missing `@ApplicationPasswordsClientId` Dagger binding — without it any call into `ApplicationPasswordsStore` threw `NoSuchElementException`. The path was latent on these apps until the auto-mint above started routing My Site through it.
ApplicationPasswordViewModelSlice.buildCard fires from MySiteViewModel's onResume / refresh / onSitePicked. Without job tracking, two close-together calls race: both pass ApplicationPasswordsManager's `existingPassword == null` check and issue separate server-side mints. The 409 conflict handler then deletes-and-recreates the winner's password, so the losing racer destroys the credential it just helped create. Pre-PR the slice raced on validate-only, which had no server-side side effect. The new mint step in #22885 makes the race visible and harmful. Fix: track the in-flight Job and drop subsequent calls while it's active. The running call posts its result regardless, so the user-visible state still updates. Test gates `createApplicationPassword` on a `CompletableDeferred`, fires buildCard twice, and verifies a single mint invocation reaches the store.
jkmassel
force-pushed
the
jkmassel/issue-22884
branch
from
06231eb to
5bfe308
Compare
oguzkocer
left a comment
oguzkocer
left a comment
There was a problem hiding this comment.
The fix for the double token creation looks good to me. I've tested it by:
- Revoking the application password online (I was previously using the wrong WordPress.com account to login 🤦, so this time I was able to test it properly)
- Pulled to refresh
- Verified that only one token was created from web
- Verified through the application logs
Looks good to me. 
4 participants
Description
Fixes #22884.
Problem
ApplicationPasswordsManager.getApplicationCredentialsreturnedNotSupportedfor anysite.isWPComsite. The guard was correct for Simple sites but blocked Atomic sites, which are alsoisWPCom-flagged and do support REST application-password creation. Users on Atomic saw the "Authenticate using Application Password" card on My Site and had to authorize through a Chrome Custom Tab even though the app could mint the credential on their behalf.What changed
Relax the FluxC guard (
ApplicationPasswordsManager.kt:45):site.isWPCom→site.isWPComSimpleSite. Atomic falls through to the existingorigin == ORIGIN_WPCOM_RESTbranches and the Jetpack-tunnelJetpackApplicationPasswordsRestClient.Auto-mint before showing the card (
ApplicationPasswordViewModelSlice+ newSiteStore.createApplicationPassword): before rendering the "Authenticate" card, attempt headless creation via the FluxC manager. On success, persist credentials onto the SiteModel (sositeHasBadCredentialsflips false) and skip the card. On failure, fall back to the existing discovery + Custom Tab card.Fix the launch crash (
ApplicationPasswordsClientIdModule+SiteStoreguard): the WordPress / Jetpack apps had never bound@ApplicationPasswordsClientIdin their Dagger graphs, so any call intoApplicationPasswordsStorethrewNoSuchElementException. This was latent — no production caller hit the path on these apps — until step 2 above started routing My Site through it and crashed the app on launch. Provide the binding with the same device-interpolated name the Custom Tab flow already uses ("Jetpack Android App on <device>"/"WordPress Android App on …"); also short-circuitSiteStore.createApplicationPasswordtoNotSupportedwhen the configuration is disabled, so we degrade gracefully if the binding goes missing again.Validate-then-mint pipeline (new
ApplicationPasswordValidator, slice restructure): collapse the previousisUsingSelfHostedRestApi → validatevselse → discovery+cardsplit into a single uniformvalidate → mint → cardflow that works for every site type.The validator hits
users().retrieveMeWithViewContext()viaWpApiClientProvider.getApplicationPasswordClient(site)(introduced in Route the application-passwords list screen through Basic auth #22894) — always Basic auth against the direct host. The previous validation went throughgetWpApiClient, which for Atomic returns the WPCom bearer client and never actually exercised the application password, so revoked passwords on Atomic were undetectable. OnInvalid, the slice clears stored creds via a newSiteStore.deleteStoredApplicationPasswordCredentialsand falls through to mint; onNetworkUnavailable, the card stays hidden.The mint step calls
SiteStore.createApplicationPasswordfrom item 2. The card step shows the reauth banner if we started with creds, the "authenticate" card otherwise — either way discovery still populates the Custom Tab URL.The XML-RPC-disabled card path is now gated on
!isUsingWpComRestApiso it only fires for true self-hosted sites — Atomic and Jetpack-WPCom-REST sites talk REST end-to-end.Site type coverage
trunk)The "revoke and recover" Atomic case is covered by #22893.
On-device verification
Confirmed against
jeremyseriousbusinesstesting.wpcomstaging.com(Atomic):Tests
Adds two FluxC manager cases (simple-WPCom
NotSupported; Atomic Jetpack-tunnel mint) inApplicationPasswordManagerTests, six slice cases inApplicationPasswordViewModelSliceTestcovering the new validate → mint → card flow, and a small standaloneOnApplicationPasswordCreateErrorTestfor the FluxC error class.Testing instructions
Atomic happy path — first foreground
adb logcat -s WordPress-MAIN | grep A_P:) showsHeadless mint succeeded.Jetpack Android App on <your device>.Atomic — subsequent loads with valid creds
A_P: Validation Success returned user id=...followed byHiding card for ... - authenticated. No re-mint attempt.Simple WPCom regression
NotSupportedand discovery returning empty.Self-hosted regression (true self-hosted, not Atomic)
Out of scope here (covered by #22893)
These scenarios will not behave correctly against PR2 alone — the validator's classification still has the issues found during the initial on-device testing of this PR:
NetworkUnavailableforincorrect_passwordand the card stays silently hidden instead of triggering a re-mint or reauth banner.Invalidand wipes stored credentials.DeviceIsOfflineErrorasInvalidand wipes stored credentials.If you want to exercise the full revoke-and-recover flow on-device, build from the
jkmassel/issue-22884-validator-hardeningbranch (or check out #22893's head).Related
application-passwordscreation routes, so wordpress-rs can't replace FluxC for this mint today. Filed during this PR.