diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 807ed160a71a..8e805abba4ca 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -14,11 +14,11 @@ * [**] Images and videos shared to the app from the photo picker now upload instead of being silently dropped. * [**] Media shared from apps that generate it on the fly, such as an "Enhanced" photo from Google Photos, now keeps its correct file type instead of always uploading as a JPEG, and sharing several photos at once no longer drops some of them. [https://github.com/wordpress-mobile/WordPress-Android/issues/23047] * [**] Self-hosted sites added with an application password now have their Jetpack status detected, so opening Stats in the Jetpack app no longer asks you to install a plugin your site already has. +* [*] Reworked editor capability detection to be more reliable and prevent a false "Unable to connect to your site" banner on private Atomic sites. 26.9 ----- - 26.8 ----- * [**] Resolved an issue where the editor could become impossible to exit when it failed to load. diff --git a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt index 0fe21d79dcda..685420793de6 100644 --- a/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt +++ b/WordPress/src/main/java/org/wordpress/android/AppInitializer.kt @@ -68,6 +68,7 @@ import org.wordpress.android.networking.NetworkConnectionMonitor import org.wordpress.android.networking.OAuthAuthenticator import org.wordpress.android.networking.RestClientUtils import org.wordpress.android.push.GCMRegistrationScheduler +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.android.support.ZendeskHelper import org.wordpress.android.ui.ActivityId import org.wordpress.android.ui.debug.cookies.DebugCookieManager @@ -228,6 +229,9 @@ class AppInitializer @Inject constructor( @Inject lateinit var wpApiClientProvider: WpApiClientProvider + @Inject + lateinit var siteProvisioningSource: SiteProvisioningSource + @Inject lateinit var openWebLinksWithJetpackHelper: DeepLinkOpenWebLinksWithJetpackHelper @@ -721,6 +725,9 @@ class AppInitializer @Inject constructor( // Clear cached wordpress-rs services and API clients wpServiceProvider.clearAll() wpApiClientProvider.clearAllClients() + + // Drop per-site provisioning + capability state for the signed-out user + siteProvisioningSource.clear() } /* diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt index bce5a2d3dcf4..b46b8c2f22fd 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt @@ -34,17 +34,6 @@ class EditorSettingsRepository @Inject constructor( fun hasCachedCapabilities(site: SiteModel): Boolean = appPrefsWrapper.hasSiteEditorCapabilities(site) - /** - * True when capability detection can't run yet because an Atomic site's - * direct-host probe needs an application password that hasn't been - * provisioned. The password is minted asynchronously on the My Site - * screen (see ApplicationPasswordViewModelSlice), so a first-login fetch - * can fail purely for lack of credentials — callers should treat this as - * pending, not a connection failure. - */ - fun isAwaitingApplicationPassword(site: SiteModel): Boolean = - site.isWPComAtomic && !site.hasApplicationPasswordCredentials() - /** * Returns whether the site is known to support the * `wp-block-editor/v1/settings` endpoint, based on @@ -108,11 +97,12 @@ class EditorSettingsRepository @Inject constructor( private suspend fun fetchRouteSupport( site: SiteModel ): Boolean = try { - // For Atomic sites the editor fetches `wp-block-editor/v1/settings` - // from the direct host — proxy and direct host can advertise - // different route lists, so detection has to probe the direct host - // too. See #22879. - if (site.isWPComAtomic) { + // Atomic and Jetpack-WPCom-REST sites have their own REST host that the editor talks to + // directly — the WP.com proxy and the direct host advertise different route lists, so + // detection has to probe the direct host too. The proxy is only for minting the application + // password. WP.com Simple sites have no direct host (the WP.com REST API *is* their API), + // and self-hosted sites are already direct via the configured client. See #22879. + if (site.isUsingWpComRestApi && !site.isWPComSimpleSite) { fetchRouteSupportViaDirectHostDiscovery(site) } else { fetchRouteSupportViaConfiguredClient(site) diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt new file mode 100644 index 000000000000..029c5b821f95 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/repositories/SiteProvisioningSource.kt @@ -0,0 +1,680 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.Job +import kotlinx.coroutines.async +import kotlinx.coroutines.coroutineScope +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.launch +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler +import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.modules.APPLICATION_SCOPE +import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.ui.accounts.login.ApplicationPasswordReauthNotifier +import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.accounts.login.SiteXmlRpcUrlRecoverer +import org.wordpress.android.ui.accounts.login.XmlRpcRecovery +import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator +import org.wordpress.android.util.AppLog +import org.wordpress.android.util.NetworkUtilsWrapper +import java.util.concurrent.ConcurrentHashMap +import javax.inject.Inject +import javax.inject.Named +import javax.inject.Singleton +import kotlin.coroutines.cancellation.CancellationException + +// How many consecutive heals a site may attempt before we stop honouring its 401s. Two allows one +// retry after a re-mint that didn't take, without letting a self-feeding 401 run unbounded. +private const val MAX_CONSECUTIVE_HEALS = 2 + +/** + * The single source of truth for getting a site ready to use: it provisions + * application-password credentials, recovers the REST API root, recovers the + * XML-RPC endpoint (self-hosted), and detects editor capabilities. + * + * Capability detection can't run until a credential exists (you can't probe a + * private Atomic host's REST API unauthenticated), so **auth is awaited first**; + * after that, the REST-capability branch and the XML-RPC branch are independent + * and run **in parallel**. Routing every consumer (connectivity banner, editor + * preloader, application-password card) through this one pipeline means the + * first-login race is structurally impossible — the probe is downstream of the + * mint — and there's one shared, deduplicated run per site instead of each + * consumer racing the others. + * + * ### No model is held across stages + * Stages take a **`siteLocalId`**, read the `SiteModel` fresh from the store at + * the point of use, and write back **only the one column they changed** + * (`persistApiRootUrl` / `persistXmlRpcUrl`). Nothing keeps a mutated `SiteModel` + * around, so the two parallel branches can't clobber each other and there's no + * stale-model write (see #22905). The passed `SiteModel` is used for its id only. + * + * Per site there is at most one in-flight pipeline (single-flight, keyed by + * [SiteModel.id]); concurrent callers join it — which also subsumes the + * application-password card's old single-flight guard (two concurrent mints hit + * a 409 that destroys the winner's credentials). + * + * ## Entry points + * - [stateFor] — reactive: returns a shared [StateFlow]; first access runs the + * pipeline, later accesses reuse a [SiteReadiness.Ready] result. + * - [await] — one-shot: runs the pipeline (if needed) and returns the result. + * - [isXmlRpcUnavailable] — whether XML-RPC recovery settled on a definitive negative. + * - [invalidate] — forces a re-run (pull-to-refresh, retry). + * - [clear] — cancels all work and drops all state; wire into sign-out. + */ +@Singleton +@Suppress("LongParameterList") +class SiteProvisioningSource @Inject constructor( + private val siteStore: SiteStore, + private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, + private val applicationPasswordValidator: ApplicationPasswordValidator, + private val wpApiClientProvider: WpApiClientProvider, + private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val siteXmlRpcUrlRecoverer: SiteXmlRpcUrlRecoverer, + private val editorSettingsRepository: EditorSettingsRepository, + private val networkUtilsWrapper: NetworkUtilsWrapper, + private val appLogWrapper: AppLogWrapper, + private val wpAppNotifierHandler: WpAppNotifierHandler, + private val applicationPasswordReauthNotifier: ApplicationPasswordReauthNotifier, + @Named(APPLICATION_SCOPE) private val appScope: CoroutineScope, +) : WpAppNotifierHandler.NotifierListener { + private val states = ConcurrentHashMap>() + private val jobs = ConcurrentHashMap() + + // Sites whose pipeline reached Ready this process — the dedup gate. Only a fully-ready site + // latches; auth-needed / unreachable / transient outcomes are left to re-run on the next + // access. Reset by invalidate / clear. + private val ready = ConcurrentHashMap.newKeySet() + + // Sites whose current run was triggered by a 401 (onRequestedWithInvalidAuthentication). If such a + // run settles Unprovisionable(hadCredentials), the headless heal failed, so we escalate to the + // interactive re-auth UI. A routine run (onResume) never sets this, so it never pops re-auth. + private val reauthOnFailure = ConcurrentHashMap.newKeySet() + + // Sites whose XML-RPC discovery reached a *definitive* negative. A missing xmlRpcUrl alone isn't + // evidence XML-RPC is off (discovery also fails transiently, e.g. a 429), so only membership here + // licenses the application-password card to show the XML-RPC-disabled warning. + private val xmlRpcUnavailable = ConcurrentHashMap.newKeySet() + + // Heal budget spent per site. The pipeline's own requests can 401 — validation always goes through + // a notifier-wired client, as does the capability probe on self-hosted sites — so an unbounded + // heal feeds itself. A heal that + // confirmed the stored password and replaced nothing can never be the fix — the 401 came from + // something re-provisioning can't touch — so it spends the whole budget at once; one that + // re-minted might have worked, so it only counts against it, which bounds a host that mints + // happily but never accepts the result. Reset when a run reaches Ready without needing a heal, + // and by invalidate / clear. + private val healBudgetSpent = ConcurrentHashMap() + + // What the most recent run established about each site's credentials. The deferred heal reads it + // to avoid re-running after a run that already re-minted — validation's own 401 fires the notifier + // during the very heal that is fixing it, which would otherwise schedule a redundant second run. + private val lastEvidence = ConcurrentHashMap() + + // Sites already escalated to interactive re-auth for their current Unprovisionable state. Both + // escalation paths funnel through escalateReauth, which uses this for idempotency; a run settling + // anything else clears it, so a later revocation escalates again. + private val escalated = ConcurrentHashMap.newKeySet() + + // Sites that earned an escalation while nothing was listening — the heal settled with the app + // backgrounded. Retried by the next run that settles Unprovisionable, which in practice is the + // onResume run, by which time an activity has registered its listener. + private val escalationPending = ConcurrentHashMap.newKeySet() + + init { + // Re-provision when wordpress-rs reports a request was rejected for invalid auth (the app + // password was revoked / rotated server-side). Without this, a site latched Ready keeps its + // silently-broken credentials until a manual pull-to-refresh; instead force ensureAuth to + // re-validate, which wipes the dead credential and re-mints (or surfaces the re-auth card). + wpAppNotifierHandler.addListener(this) + } + + /** + * The shared readiness state for [site]. The first call starts the pipeline; + * later calls return the same flow without re-running once it reached + * [SiteReadiness.Ready]. Only [SiteModel.id] is read from [site]. + */ + @Synchronized + fun stateFor(site: SiteModel): StateFlow { + val flow = flowFor(site.id) + if (shouldRun(site.id)) launchPipeline(site.id) + return flow + } + + /** + * Runs the pipeline for [site] (if it hasn't reached Ready) and returns the + * settled readiness. Respects the once-per-site gate; call [invalidate] first + * to force a fresh run. + */ + suspend fun await(site: SiteModel): SiteReadiness { + stateFor(site) + jobs[site.id]?.join() + return states[site.id]?.value ?: SiteReadiness.Probing + } + + /** + * Forces a re-run for [site] (pull-to-refresh, banner retry), bypassing the once-per-site gate. + * + * Deliberately a **no-op while a run is already in flight**: cancelling mid-pipeline could + * interrupt an in-progress application-password mint after the server created it but before we + * persisted it, orphaning the credential and tripping a 409 on the next mint. So an explicit + * refresh that lands during an active run is coalesced into that run (the user gets its live + * result) rather than pre-empting it; a refresh once the run is idle starts a genuinely fresh one. + */ + @Synchronized + fun invalidate(site: SiteModel) { + // Clear the suppression verdicts first, above the in-flight guard. Only the *relaunch* must not + // pre-empt a mid-mint run; dropping these sets pre-empts nothing. It has to happen here because + // MySiteViewModel.refresh builds the application-password card before it reaches this call, and + // that card's buildCard starts a run synchronously on Main.immediate — so by the time an + // explicit retry lands, a run is already active for exactly the sites that need clearing. + healBudgetSpent.remove(site.id) + escalated.remove(site.id) + // No-op while running — see KDoc: don't pre-empt an in-flight mint. + if (jobs[site.id]?.isActive == true) return + ready.remove(site.id) + launchPipeline(site.id) + } + + /** Cancels all in-flight pipelines and drops all cached state (sign-out). */ + @Synchronized + fun clear() { + jobs.values.forEach { it.cancel() } + jobs.clear() + states.clear() + ready.clear() + reauthOnFailure.clear() + xmlRpcUnavailable.clear() + healBudgetSpent.clear() + lastEvidence.clear() + escalated.clear() + escalationPending.clear() + } + + /** + * Whether [recoverXmlRpcIfNeeded] concluded that [siteLocalId] genuinely has XML-RPC disabled, as + * opposed to merely failing to recover the endpoint. Only the definitive case may surface the + * XML-RPC-disabled card; a transient discovery failure (e.g. a 429 rate-limit) must not. + */ + fun isXmlRpcUnavailable(siteLocalId: Int): Boolean = siteLocalId in xmlRpcUnavailable + + /** + * [WpAppNotifierHandler.NotifierListener] — wordpress-rs rejected a request for [site] with invalid + * authentication — a revoked application password; bearer clients no longer report here. Re-provision it + * so ensureAuth re-validates and heals: for a WP.com-connected site the headless re-mint succeeds and + * recovery is silent; for one that can't be re-minted the run settles Unprovisionable and + * [settleHealState] escalates to interactive re-auth. + * + * The notifier hands us the exact [SiteModel] whose client raised the 401, so we heal that one row by + * id — no resolving back by URL (which isn't unique: the DB constraint is on SITE_ID+URL, so two rows + * can share a URL). When a run is already in flight, [healForInvalidAuth] defers the heal until it + * finishes rather than pre-empting a possibly mid-mint stage — so a validate that itself 401s can't + * spin this into a loop. + */ + override fun onRequestedWithInvalidAuthentication(site: SiteModel) { + if (canHeal(site)) healForInvalidAuth(site) + } + + /** + * Whether re-provisioning could plausibly fix a 401 for [site]: + * + * - WP.com Simple sites are bearer-only — no application password applies, so there is nothing to + * heal. + * - An already-[SiteAuthState.Unprovisionable] site has its re-auth pending the user; re-running + * would just re-fail the mint on every 401. + * - A site that has spent its heal budget ([healBudgetSpent]) has already had attempts that + * changed nothing, so its 401s come from something re-provisioning can't fix. + */ + private fun canHeal(site: SiteModel): Boolean = + !site.isWPComSimpleSite && hasHealBudget(site.id) + + /** + * Whether [siteLocalId] may still be healed: its re-auth isn't already pending the user, and it + * hasn't spent its heal budget on attempts that changed nothing. + */ + private fun hasHealBudget(siteLocalId: Int): Boolean { + val auth = (states[siteLocalId]?.value as? SiteReadiness.NeedsAuth)?.auth + return auth !is SiteAuthState.Unprovisionable && + (healBudgetSpent[siteLocalId] ?: 0) < MAX_CONSECUTIVE_HEALS + } + + /** + * Drive a heal for a 401. If the pipeline is idle, run it now and arm [reauthOnFailure] so a failed + * heal escalates to interactive re-auth. If a run is already in flight we must not pre-empt a + * possibly mid-mint stage — but the 401 must not be swallowed either: that run may have validated the + * credential *before* it was revoked and will settle Ready, consuming nothing and healing nothing. So + * defer a fresh heal until the active run finishes. The flag is armed only when the heal actually + * launches, so the in-flight run's tail can't consume it for an outcome it never serviced. + * + * Two cases end the deferral instead of relaunching. A run that settled + * [SiteAuthState.Unprovisionable] has already made a terminal mint attempt, so relaunching would + * only re-fail it — escalate to interactive re-auth instead (a *routine* run never armed + * [reauthOnFailure], so it never escalated on its own). And a site that has spent its heal budget + * has already had attempts change nothing, so its 401 is not an application-password problem. + */ + @Synchronized + private fun healForInvalidAuth(site: SiteModel) { + val siteLocalId = site.id + val active = jobs[siteLocalId] + if (active?.isActive != true) { + reauthOnFailure.add(siteLocalId) + ready.remove(siteLocalId) + launchPipeline(siteLocalId) + return + } + active.invokeOnCompletion { cause -> + if (cause != null) return@invokeOnCompletion // cancelled / relaunched — a fresh run is coming + // Decide under the lock, act outside it: escalateReauth reads the DB and starts an + // Activity, and the main thread takes this same monitor on every stateFor / invalidate. + val escalation = synchronized(this@SiteProvisioningSource) { resumeDeferredHeal(siteLocalId) } + escalation?.let { escalateReauth(siteLocalId, it) } + } + } + + /** + * The deferred half of [healForInvalidAuth], run once the in-flight pipeline finishes. Returns the + * [SiteAuthState.Unprovisionable] to escalate for, or `null` when there's nothing to do — the + * caller performs the escalation outside the lock. + */ + private fun resumeDeferredHeal(siteLocalId: Int): SiteAuthState.Unprovisionable? = when { + // Superseded: a newer run is underway, or the run we waited on itself re-minted. Validation's + // own 401 goes through a notifier-wired client, so the heal that fixes a revoked password also + // schedules this deferral — re-running would re-validate the fresh credential for nothing. + jobs[siteLocalId]?.isActive == true || + lastEvidence[siteLocalId] == HealEvidence.Replaced -> null + + // Budget re-checked here, not just at registration time: that run may have spent it. A + // terminal mint failure escalates instead of relaunching — a routine run's tail skipped that + // because it never armed reauthOnFailure. + !hasHealBudget(siteLocalId) -> + (states[siteLocalId]?.value as? SiteReadiness.NeedsAuth)?.auth as? SiteAuthState.Unprovisionable + + else -> { + reauthOnFailure.add(siteLocalId) + ready.remove(siteLocalId) + launchPipeline(siteLocalId) + null + } + } + + /** + * Records what a settled run means for future 401s, and escalates to interactive re-auth when a + * heal couldn't recover a previously-working credential. + * + * [wasHeal] is the consumed [reauthOnFailure] flag: only a 401-triggered run may prompt, so a + * routine run that happens to settle [SiteAuthState.Unprovisionable] leaves the prompt to the + * application-password card. A heal that settled without changing the credentials is recorded in + * [healBudgetSpent] — the 401 came from something re-provisioning can't fix. + */ + private fun settleHealState(siteLocalId: Int, result: PipelineResult, wasHeal: Boolean) { + lastEvidence[siteLocalId] = result.evidence + val auth = (result.readiness as? SiteReadiness.NeedsAuth)?.auth + if (auth is SiteAuthState.Unprovisionable) { + // A routine run normally leaves the prompt to the card, but it must still deliver an + // escalation that an earlier heal earned and couldn't hand to anyone. + if (wasHeal || siteLocalId in escalationPending) escalateReauth(siteLocalId, auth) + return + } + // Not stuck on auth any more, so a future revocation should be able to prompt again. + escalated.remove(siteLocalId) + escalationPending.remove(siteLocalId) + when { + wasHeal -> { + // A heal that only re-confirmed the stored password can never be the fix, so it + // exhausts the budget outright; one that re-minted might have worked, so it costs one. + val cost = if (result.evidence == HealEvidence.ConfirmedUnchanged) { + MAX_CONSECUTIVE_HEALS + } else { + 1 + } + val spent = healBudgetSpent.merge(siteLocalId, cost, Int::plus) + appLogWrapper.w( + AppLog.T.MAIN, + "A_P: Heal for $siteLocalId spent $cost (${spent}/$MAX_CONSECUTIVE_HEALS)" + ) + } + // A run that reached Ready without needing a heal means whatever was failing has cleared, + // so a later revocation gets a full budget again. + result.readiness == SiteReadiness.Ready -> healBudgetSpent.remove(siteLocalId) + } + } + + /** + * Prompts for interactive re-auth at most once per [SiteAuthState.Unprovisionable] episode. + * + * The episode is only recorded once a live listener has taken the prompt. Activities register + * theirs in onResume and drop it in onPause, so a heal that settles while the app is backgrounded + * would otherwise burn the one prompt on an empty listener map — and nothing would ask again, + * because further 401s are blocked by the Unprovisionable state. An undelivered prompt is + * held in [escalationPending] and retried by the next run instead. + */ + private fun escalateReauth(siteLocalId: Int, auth: SiteAuthState.Unprovisionable) { + val alreadyHandled = !auth.hadCredentials || siteLocalId in escalated + val site = if (alreadyHandled) null else siteStore.getSiteByLocalId(siteLocalId) + if (site == null) return + if (applicationPasswordReauthNotifier.notifyReauthRequired(site.url)) { + escalated.add(siteLocalId) + escalationPending.remove(siteLocalId) + } else { + escalationPending.add(siteLocalId) + appLogWrapper.d( + AppLog.T.MAIN, + "A_P: Nobody listening for re-auth on ${site.url} - will retry on the next run" + ) + } + } + + @Synchronized + private fun launchPipeline(siteLocalId: Int) { + jobs[siteLocalId]?.cancel() + val flow = flowFor(siteLocalId) + jobs[siteLocalId] = appScope.launch { + // runPipeline runs on the app-lifetime appScope, a plain (non-supervisor) Job: an escaping + // throw would cancel it and every other app-scoped coroutine. Contain any unexpected failure + // (e.g. a SQLiteException from a stage's DB write) as Unreachable so the flow still settles + // and the next run retries; let cancellation propagate normally. + val result = try { + runPipeline(siteLocalId) + } catch (e: CancellationException) { + throw e + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { + appLogWrapper.e( + AppLog.T.MAIN, + "Provisioning pipeline failed for $siteLocalId: ${e::class.simpleName}: ${e.message}" + ) + // Same offline distinction detectCapabilities makes: with no network this isn't the + // site's fault, and the connectivity banner would just stack on the global + // no-connection one. + PipelineResult(unreachableOrTransient(), latch = false) + } + flow.value = result.readiness + // Latch the dedup gate only on a freshly live-probed Ready; a Ready served from stale cache + // (latch = false) is left to re-probe on the next run instead of sticking for the process. + if (result.latch) ready.add(siteLocalId) + // settleHealState reads the DB and drives the reauth notifier, and runs after the flow has + // already settled. Contain its throws too: on this non-supervisor appScope an escaping throw + // here would cancel the scope and wedge provisioning for every other site. It must run + // before this job completes so the deferred heal handler sees the verdicts it records. + try { + settleHealState(siteLocalId, result, wasHeal = reauthOnFailure.remove(siteLocalId)) + } catch (e: CancellationException) { + throw e + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { + appLogWrapper.e( + AppLog.T.MAIN, + "Reauth escalation failed for $siteLocalId: ${e::class.simpleName}: ${e.message}" + ) + } + } + } + + /** [SiteReadiness.Unreachable] only when the device has a network; otherwise it's just offline. */ + private fun unreachableOrTransient(): SiteReadiness = + if (networkUtilsWrapper.isNetworkAvailable()) { + SiteReadiness.Unreachable + } else { + SiteReadiness.TransientError + } + + private fun flowFor(siteLocalId: Int): MutableStateFlow = + states.getOrPut(siteLocalId) { MutableStateFlow(SiteReadiness.Probing) } + + private fun shouldRun(siteLocalId: Int): Boolean = + jobs[siteLocalId]?.isActive != true && siteLocalId !in ready + + private suspend fun runPipeline(siteLocalId: Int): PipelineResult { + return when (val stage = ensureAuth(siteLocalId)) { + is AuthStage.Proceed -> coroutineScope { + // Post-auth, the REST-capability chain and the XML-RPC recovery are independent — each + // reads the site fresh and writes only its own column — so run them in parallel. + // recoverRestUrlIfNeeded precedes detectCapabilities within its branch because the probe + // needs the recovered REST root. Both branches read the credentials fresh from the store: + // the mint persists them via a single writer that the generic full-row update can no + // longer clobber (#22947), so the re-read is now trustworthy (#22905). + val capabilities = async { + recoverQuietly("REST root recovery") { recoverRestUrlIfNeeded(siteLocalId) } + detectCapabilities(siteLocalId) + } + val xmlRpc = async { recoverQuietly("XML-RPC recovery") { recoverXmlRpcIfNeeded(siteLocalId) } } + xmlRpc.await() + capabilities.await().copy(evidence = stage.evidence) + } + // The site itself couldn't be reached, so nothing downstream can run. This is the + // connectivity banner's case, not the application-password card's — validation never got + // far enough to say anything about the credentials. + AuthStage.SiteUnreachable -> PipelineResult(SiteReadiness.Unreachable, latch = false) + is AuthStage.Stop -> PipelineResult(SiteReadiness.NeedsAuth(stage.auth), latch = false) + } + } + + /** + * Stage 1 — ensure the site has working application-password credentials. Validates stored creds + * with Basic auth against the direct host; on a confirmed rejection wipes them and mints fresh + * ones via the FluxC Jetpack tunnel. The mint persists the credentials (single-writer, #22947), so + * the downstream stages read them back from a fresh [SiteModel] rather than having them threaded. + */ + // Each return is a distinct auth outcome (missing site, valid, unreachable, transient, minted, + // failed); collapsing to one return would thread a result through nested branches and read worse. + @Suppress("ReturnCount") + private suspend fun ensureAuth(siteLocalId: Int): AuthStage { + val site = siteStore.getSiteByLocalId(siteLocalId) + ?: return AuthStage.Stop(SiteAuthState.Unprovisionable(hadCredentials = false)) + // WP.com Simple sites are fully proxied and OAuth-bearer-authed — no application password + // applies (the mint returns NotSupported). Capability detection works through the proxy, so + // treat them as ready instead of blocking detection behind a mint that can never run. No + // password is involved, so there is nothing for a heal to confirm. + if (site.isWPComSimpleSite) return AuthStage.Proceed(HealEvidence.Inconclusive) + val hadCredentials = !applicationPasswordLoginHelper.siteHasBadCredentials(site) + if (hadCredentials) { + when (applicationPasswordValidator.validate(site)) { + ApplicationPasswordValidator.Outcome.Valid -> + return AuthStage.Proceed(HealEvidence.ConfirmedUnchanged) + ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { + // The validator maps everything ambiguous — DNS, timeout, refused, 5xx — to this + // outcome so it never wipes credentials on a guess. That conflates two very + // different situations, and only the device-offline one should stay quiet: the + // global offline banner already covers it. If the device is online, the *site* is + // what we couldn't reach, which is exactly the connectivity banner's case (#22944). + if (networkUtilsWrapper.isNetworkAvailable()) { + appLogWrapper.d(AppLog.T.MAIN, "A_P: Site unreachable during validation: ${site.url}") + return AuthStage.SiteUnreachable + } + appLogWrapper.d(AppLog.T.MAIN, "A_P: Device offline during validation for ${site.url}") + return AuthStage.Stop(SiteAuthState.Provisioning) + } + ApplicationPasswordValidator.Outcome.Invalid -> { + appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${site.url}, clearing") + siteStore.deleteStoredApplicationPasswordCredentials(site) + wpApiClientProvider.clearSelfHostedClient(site.id) + } + } + } + // createApplicationPassword mints and persists the credentials; downstream stages read them + // back from a fresh SiteModel. + val createResult = siteStore.createApplicationPassword(site) + if (!createResult.isError && createResult.credentials != null) { + wpApiClientProvider.clearSelfHostedClient(site.id) + appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${site.url}") + // A fresh mint replaced the credentials, so a 401 that prompted this run may genuinely be + // healed by it — this is the one path that marks the heal as having done something. + return AuthStage.Proceed(HealEvidence.Replaced) + } + appLogWrapper.d( + AppLog.T.MAIN, + "A_P: Headless mint failed for ${site.url} (notSupported=${createResult.error?.notSupported})" + ) + return AuthStage.Stop(SiteAuthState.Unprovisionable(hadCredentials = hadCredentials)) + } + + /** + * Runs a best-effort recovery stage so its failure can't decide the site's readiness. + * + * Both stages heal a column when they can, and both end in an unguarded DB write — `WellSql` + * update calls that can throw. Left to propagate, that throw would cancel the sibling through + * the enclosing `coroutineScope` and settle the whole site `Unreachable`, discarding a + * capability probe that had already succeeded. Cancellation still propagates normally. + */ + private suspend fun recoverQuietly(label: String, recover: suspend () -> Unit) { + try { + recover() + } catch (e: CancellationException) { + throw e + } catch (@Suppress("TooGenericExceptionCaught") e: Exception) { + appLogWrapper.e( + AppLog.T.MAIN, + "A_P: $label failed, continuing: ${e::class.simpleName}: ${e.message}" + ) + } + } + + /** + * Stage 2a — recover the REST API root for Atomic sites minted through the + * Jetpack tunnel (which never runs discovery and leaves `wpApiRestUrl` null). + * Persists the one column; the capability probe (sequenced after it) re-reads it. + */ + private suspend fun recoverRestUrlIfNeeded(siteLocalId: Int) { + val site = siteStore.getSiteByLocalId(siteLocalId) ?: return + // WP.com Simple sites are proxy-served — no direct REST host to recover (their wpApiRestUrl + // is legitimately null), so don't burn a discovery call on them. + if (site.isWPComSimpleSite || !site.wpApiRestUrl.isNullOrEmpty()) return + siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> + siteApiRestUrlRecoverer.persistApiRootUrl(siteLocalId, apiRootUrl) + } + } + + /** + * Stage 2b (parallel) — recover the XML-RPC endpoint for true self-hosted + * sites that don't have one. Discovers + authenticates against it with the + * site's application-password credentials (which work for XML-RPC just as for + * REST), and on success persists the one column; the application-password card + * re-reads it. + */ + private suspend fun recoverXmlRpcIfNeeded(siteLocalId: Int) { + val site = siteStore.getSiteByLocalId(siteLocalId) ?: return + // WP.com / Atomic / Jetpack-WPCom-REST sites talk REST end-to-end and don't use XML-RPC. + if (site.isUsingWpComRestApi || !site.xmlRpcUrl.isNullOrEmpty()) return + when (val recovery = siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(site)) { + is XmlRpcRecovery.Recovered -> { + xmlRpcUnavailable.remove(siteLocalId) + siteXmlRpcUrlRecoverer.persistXmlRpcUrl(siteLocalId, recovery.endpoint) + } + // Definitive negative — license the card. Inconclusive settles nothing, so clear any stale + // verdict and leave the card hidden until a later run reaches a conclusion. + XmlRpcRecovery.Unavailable -> xmlRpcUnavailable.add(siteLocalId) + XmlRpcRecovery.Inconclusive -> xmlRpcUnavailable.remove(siteLocalId) + } + } + + /** + * Stage 3 — probe the REST API for editor-capability support and persist it. + * Reached only once [ensureAuth] returned [AuthStage.Proceed]. + * Reads the site fresh: the mint has already persisted the credentials (single-writer, #22947), + * so a probe failure here is a real transport problem, not a pending mint. + */ + private suspend fun detectCapabilities(siteLocalId: Int): PipelineResult { + val site = siteStore.getSiteByLocalId(siteLocalId) + ?: return PipelineResult(SiteReadiness.Unreachable, latch = false) + val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) + val hasCache = editorSettingsRepository.hasCachedCapabilities(site) + return when { + // Live probe succeeded: latch so we stop re-probing — capabilities rarely change. + ok -> PipelineResult(SiteReadiness.Ready, latch = true) + // Probe failed but stale cache keeps the site usable and the banner hidden; do NOT latch, + // so the next run re-probes and can refresh the cache / surface a genuine failure (#22944). + hasCache -> PipelineResult(SiteReadiness.Ready, latch = false) + !networkUtilsWrapper.isNetworkAvailable() -> + PipelineResult(SiteReadiness.TransientError, latch = false) + else -> PipelineResult(SiteReadiness.Unreachable, latch = false) + } + } + + /** + * A settled pipeline result plus whether it should latch the per-site dedup gate ([ready]). Only a + * freshly live-probed [SiteReadiness.Ready] latches; a Ready served from stale cache does not, so it + * re-probes on the next run. [evidence] is what the run established about the credentials, which is + * how [settleHealState] judges whether a heal accomplished anything. Internal to the pipeline — + * consumers only ever see [readiness]. + */ + private data class PipelineResult( + val readiness: SiteReadiness, + val latch: Boolean, + val evidence: HealEvidence = HealEvidence.Inconclusive, + ) + + /** + * What [ensureAuth] settled on. Only [Stop] escapes the pipeline, as a + * [SiteReadiness.NeedsAuth] the application-password card renders. + */ + private sealed interface AuthStage { + /** The credentials are usable — run the rest of the pipeline. */ + data class Proceed(val evidence: HealEvidence) : AuthStage + + /** The site couldn't be reached while the device was online, so validation says nothing about + * the credentials. Surfaces as [SiteReadiness.Unreachable] for the connectivity banner. */ + data object SiteUnreachable : AuthStage + + /** Stopped at the auth stage; [auth] is what the card should show. */ + data class Stop(val auth: SiteAuthState) : AuthStage + } + + /** + * What a run established about the site's application password — the basis for deciding whether a + * 401-triggered heal did anything, and so whether repeating it could ever help. + */ + private enum class HealEvidence { + /** Nothing was established: the site was unreachable, the device was offline, the mint failed, + * or no application password applies to the site at all. */ + Inconclusive, + + /** The stored credentials were confirmed working and left untouched. A heal that ends here + * cannot be the fix for the 401 that prompted it. */ + ConfirmedUnchanged, + + /** The credentials were replaced by a fresh mint, so the heal may genuinely have healed. */ + Replaced, + } +} + +/** + * Why a site's application password isn't usable yet. Owned by [SiteProvisioningSource] and rendered + * by the application-password card — these are the only two auth outcomes a consumer ever observes. + * Everything else the auth stage can settle on (usable credentials, an unreachable site) stays inside + * the pipeline as `AuthStage`, so a `when` over this type has no unreachable branches. + */ +sealed interface SiteAuthState { + /** Not usable yet, but not a terminal failure — a mint is implied / a transient + * validation error occurred. The card stays hidden; the next run retries. */ + data object Provisioning : SiteAuthState + + /** Terminal: the mint failed. [hadCredentials] distinguishes a re-authentication + * (creds went bad) from a first-time authentication prompt. */ + data class Unprovisionable(val hadCredentials: Boolean) : SiteAuthState +} + +/** + * The combined per-site readiness the [SiteProvisioningSource] exposes. The + * connectivity banner renders [Unreachable], the application-password card + * renders [NeedsAuth], and the editor preloader awaits a non-[Probing] value — + * each a slice of the one state, so they can't disagree. + */ +sealed interface SiteReadiness { + /** The pipeline is running and hasn't produced a result yet. */ + data object Probing : SiteReadiness + + /** Stopped at the auth stage — credentials aren't usable. Carries the + * [SiteAuthState] so the card can pick re-auth vs. first-auth vs. hidden. */ + data class NeedsAuth(val auth: SiteAuthState) : SiteReadiness + + /** Provisioned and editor capabilities are known (detected or cached). */ + data object Ready : SiteReadiness + + /** Provisioned, but the capability probe failed — the site looks unreachable. + * The only state that surfaces the connectivity banner. */ + data object Unreachable : SiteReadiness + + /** Provisioned, but a transient failure (e.g. offline). Retried on the next run. */ + data object TransientError : SiteReadiness +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt index 82bdc70f94c7..8eb250457352 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt @@ -25,6 +25,8 @@ import org.wordpress.android.util.crashlogging.sendReportWithTag import rs.wordpress.api.kotlin.ApiDiscoveryResult import rs.wordpress.api.kotlin.WpLoginClient import uniffi.wp_api.DiscoveredAuthenticationMechanism +import uniffi.wp_api.FetchAndParseApiRootFailure +import uniffi.wp_api.WpErrorCode import uniffi.wp_api.applicationPasswordsUrl import java.net.URI import javax.inject.Inject @@ -36,6 +38,11 @@ private const val REASON_TAG = "reason" private const val SOURCE_TAG = "source" private const val ERROR_TAG = "error" +// WordPress.com returns this error code from the REST root of a site whose Privacy setting is +// Private (or Coming Soon). The gate sits in front of WordPress, so discovery never reaches the +// API — the site's Application Password support is irrelevant to the failure. +private const val PRIVATE_SITE_ERROR_CODE = "private_site" + class ApplicationPasswordLoginHelper @Inject constructor( @param:Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, private val dispatcherWrapper: DispatcherWrapper, @@ -48,19 +55,46 @@ class ApplicationPasswordLoginHelper @Inject constructor( private val discoverSuccessWrapper: DiscoverSuccessWrapper, private val crashLogging: CrashLogging, private val wpApiClientProvider: WpApiClientProvider, - private val credentialsChangedNotifier: CredentialsChangedNotifier, ) { private var processedAppPasswordData: String? = null sealed class DiscoveryResult { data class Authorized(val authorizationUrl: String) : DiscoveryResult() - data class Failed(val userFacingMessage: String) : DiscoveryResult() + + /** + * Discovery couldn't reach or read the site's REST API. [userFacingMessage] is the library's + * description of what went wrong; [reason] narrows it when we can recognise the cause, so the + * UI can explain it rather than guess. + */ + data class Failed( + val userFacingMessage: String, + val reason: FailureReason = FailureReason.Unknown, + ) : DiscoveryResult() /** * The site is hosted on WordPress.com: API discovery reported OAuth2 as the authentication * mechanism, so it can't use Application Passwords and should log in via WordPress.com. */ object WpComSite : DiscoveryResult() + + /** A recognised cause for a [Failed] discovery. */ + enum class FailureReason { + /** Nothing more specific than the library's message. */ + Unknown, + + /** + * The site's Privacy setting blocks anonymous requests, so discovery got a 403 instead of + * the REST root. Nothing is wrong with the site's Application Password support — it just + * has to be publicly reachable for the login flow to read its API. + */ + PrivateSite, + + /** + * Discovery succeeded but the site advertises no application-passwords endpoint, so it + * genuinely can't be logged into this way. + */ + NotSupported, + } } @Suppress("TooGenericExceptionCaught") @@ -84,6 +118,15 @@ class ApplicationPasswordLoginHelper @Inject constructor( } else { val authorizationUrl = discoverSuccessWrapper.getApplicationPasswordsAuthenticationUrl(urlDiscoveryResult) + if (authorizationUrl == null) { + // Discovery worked; the site just doesn't offer application passwords. This + // is the one case the old blanket "not supported" message was right about. + return@withContext handleAuthenticationDiscoveryError( + siteUrl, + "No application-passwords authentication URL advertised", + DiscoveryResult.FailureReason.NotSupported, + ) + } val apiRootUrl = discoverSuccessWrapper.getApiRootUrl(urlDiscoveryResult) if (apiRootUrl.isNotEmpty()) { // Store the ApiRootUrl for use it after the login @@ -105,15 +148,39 @@ class ApplicationPasswordLoginHelper @Inject constructor( is ApiDiscoveryResult.FailureParseSiteUrl -> handleAuthenticationDiscoveryError( siteUrl, - urlDiscoveryResult.userFacingErrorMessage(siteUrl).orEmpty() + urlDiscoveryResult.userFacingErrorMessage(siteUrl).orEmpty(), + urlDiscoveryResult.failureReason(), ) } } - private fun handleAuthenticationDiscoveryError(siteUrl: String, message: String): DiscoveryResult { - appLogWrapper.e(AppLog.T.API, "A_P: Error during API discovery for $siteUrl - $message") + /** + * Recognise causes worth naming to the user. A [FetchAndParseApiRootFailure.WpError] means we + * reached the site and it answered with a REST error envelope, so its `code` is a reliable + * signal — WordPress.com sends `private_site` from a site whose Privacy setting hides it. + */ + private fun ApiDiscoveryResult.failureReason(): DiscoveryResult.FailureReason { + val wpError = (this as? ApiDiscoveryResult.FailureFetchAndParseApiRoot) + ?.fetchAndParseApiRootFailure as? FetchAndParseApiRootFailure.WpError + ?: return DiscoveryResult.FailureReason.Unknown + // `private_site` has no dedicated WpErrorCode, so the library surfaces it as a CustomException + // carrying the raw code string. + val rawCode = (wpError.errorCode as? WpErrorCode.CustomException)?.v1 + return if (rawCode == PRIVATE_SITE_ERROR_CODE) { + DiscoveryResult.FailureReason.PrivateSite + } else { + DiscoveryResult.FailureReason.Unknown + } + } + + private fun handleAuthenticationDiscoveryError( + siteUrl: String, + message: String, + reason: DiscoveryResult.FailureReason = DiscoveryResult.FailureReason.Unknown, + ): DiscoveryResult { + appLogWrapper.e(AppLog.T.API, "A_P: Error during API discovery for $siteUrl - $message ($reason)") AnalyticsTracker.track(Stat.BACKGROUND_REST_AUTODISCOVERY_FAILED) - return DiscoveryResult.Failed(message) + return DiscoveryResult.Failed(message, reason) } sealed class StoreCredentialsResult { @@ -167,7 +234,6 @@ class ApplicationPasswordLoginHelper @Inject constructor( } wpApiClientProvider.clearSelfHostedClient(site.id) dispatcherWrapper.updateApplicationPassword(site) - credentialsChangedNotifier.notifyChanged(site.id) trackSuccessful(effectiveUrlLogin.siteUrl) trackCreated(creationSource, success = true) processedAppPasswordData = effectiveUrlLogin.siteUrl @@ -438,12 +504,9 @@ class ApplicationPasswordLoginHelper @Inject constructor( WPUrlUtils.isWordPressCom(authentication.endpoints.authorizationUrl) } + /** `null` when the site advertises no application-passwords endpoint. */ fun getApplicationPasswordsAuthenticationUrl( successObject: ApiDiscoveryResult.Success - ): String = requireNotNull( - applicationPasswordsUrl(successObject.success.authentication)?.url() - ) { - "Application passwords authentication URL is required" - } + ): String? = applicationPasswordsUrl(successObject.success.authentication)?.url() } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifier.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifier.kt new file mode 100644 index 000000000000..cdfb44da25ab --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifier.kt @@ -0,0 +1,52 @@ +package org.wordpress.android.ui.accounts.login + +import java.lang.ref.WeakReference +import javax.inject.Inject +import javax.inject.Singleton + +/** + * App-scoped relay that asks the UI to start interactive application-password re-authentication for a + * site. SiteProvisioningSource posts here only after a headless heal (validate + re-mint) has failed + * for a site that previously had credentials — i.e. the credential is revoked and can't be recovered + * silently. WPMainActivity / MediaBrowserActivity listen and navigate to the re-auth screen. + * + * The raw wordpress-rs 401 signal (WpAppNotifierHandler) now drives the provisioning pipeline's heal + * instead of the UI directly, so a successful re-mint no longer flashes the re-auth screen. This + * mirrors that handler's weak-listener shape so the activity add/remove lifecycle is unchanged. + */ +@Singleton +class ApplicationPasswordReauthNotifier @Inject constructor() { + private val listeners = mutableMapOf>() + + /** + * Asks any listening UI to navigate to interactive re-auth for [siteUrl], returning whether a + * live listener actually took it. Listeners are registered per activity onResume/onPause, so a + * heal that settles while the app is backgrounded has nobody to tell — the caller needs to know + * that so it doesn't record the prompt as delivered. + */ + @Synchronized + fun notifyReauthRequired(siteUrl: String): Boolean { + cleanupDeadReferences() + val live = listeners.values.mapNotNull { it.get() } + live.forEach { it.onReauthRequired(siteUrl) } + return live.isNotEmpty() + } + + @Synchronized + fun addListener(listener: Listener) { + listeners[listener.toString()] = WeakReference(listener) + } + + @Synchronized + fun removeListener(listener: Listener) { + listeners.remove(listener.toString()) + } + + private fun cleanupDeadReferences() { + listeners.entries.removeAll { it.value.get() == null } + } + + interface Listener { + fun onReauthRequired(siteUrl: String) + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt deleted file mode 100644 index 0046dc400595..000000000000 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/CredentialsChangedNotifier.kt +++ /dev/null @@ -1,34 +0,0 @@ -package org.wordpress.android.ui.accounts.login - -import kotlinx.coroutines.channels.BufferOverflow -import kotlinx.coroutines.flow.MutableSharedFlow -import kotlinx.coroutines.flow.SharedFlow -import kotlinx.coroutines.flow.asSharedFlow -import javax.inject.Inject -import javax.inject.Singleton - -/** - * App-scoped signal that an application password was newly established for a site — either by the - * headless Jetpack-tunnel mint on the My Site screen or by the interactive application-password - * login. Lets credential-dependent work (e.g. editor capability detection) re-run as soon as the - * password exists, instead of waiting for the next My Site resume/refresh. - * - * Emits the site's local id; collectors should re-read a fresh SiteModel so they observe the - * just-persisted credentials rather than a stale in-memory copy. - */ -@Singleton -class CredentialsChangedNotifier @Inject constructor() { - // replay = 1 so a collector that subscribes just after an emit still sees it — closes the - // emit-before-collect race. DROP_OLDEST keeps tryEmit non-suspending without an unbounded buffer. - private val _events = MutableSharedFlow( - replay = 1, - extraBufferCapacity = 1, - onBufferOverflow = BufferOverflow.DROP_OLDEST, - ) - val events: SharedFlow = _events.asSharedFlow() - - /** Signals that [siteLocalId]'s application-password credentials were just established. */ - fun notifyChanged(siteLocalId: Int) { - _events.tryEmit(siteLocalId) - } -} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt new file mode 100644 index 000000000000..ac313a541cf5 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecoverer.kt @@ -0,0 +1,117 @@ +package org.wordpress.android.ui.accounts.login + +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.withContext +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder +import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient +import org.wordpress.android.fluxc.persistence.SiteSqlUtils +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.modules.IO_THREAD +import org.wordpress.android.util.AppLog +import javax.inject.Inject +import javax.inject.Named +import javax.inject.Singleton +import kotlin.coroutines.cancellation.CancellationException + +/** + * Heals [SiteModel.xmlRpcUrl] for true self-hosted sites whose XML-RPC endpoint was never + * discovered (or was previously gated). The REST counterpart is [SiteApiRestUrlRecoverer]; this + * follows the same shape so callers never hold a mutated [SiteModel]: + * + * - [discoverAndVerifyXmlRpcUrl] discovers the endpoint and confirms it works with an authenticated + * call using the site's application-password credentials, returning an [XmlRpcRecovery] that + * separates a verified URL from a *definitive* "XML-RPC is off" from an inconclusive failure. + * - [persistXmlRpcUrl] writes only that one column to the DB row for `localId`. + * + * The three-way outcome matters because a missing `xmlRpcUrl` is not evidence that XML-RPC is + * disabled: discovery can also fail transiently (e.g. a 429 rate-limit). Only a definitive negative + * may surface the "XML-RPC Disabled" card, or throttled sites get a false warning. + */ +@Singleton +class SiteXmlRpcUrlRecoverer @Inject constructor( + private val selfHostedEndpointFinder: SelfHostedEndpointFinder, + private val siteXMLRPCClient: SiteXMLRPCClient, + private val siteSqlUtils: SiteSqlUtils, + private val appLogWrapper: AppLogWrapper, + // IO, not BG: verifyOrDiscoverXMLRPCEndpoint is a blocking chain of HTTP calls that can take + // tens of seconds against a dead host, and BG_THREAD is Dispatchers.Default — the same + // CPU-sized pool APPLICATION_SCOPE runs every site's pipeline on. This is the dispatcher the + // code in ApplicationPasswordViewModelSlice used before the move. + @param:Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, +) { + @Suppress("SwallowedException", "TooGenericExceptionCaught") + suspend fun discoverAndVerifyXmlRpcUrl(site: SiteModel): XmlRpcRecovery = withContext(ioDispatcher) { + try { + val endpoint = selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url) + val result = siteXMLRPCClient.fetchSites( + endpoint, + site.apiRestUsernamePlain, + site.apiRestPasswordPlain, + ) + if (result.isError) { + // Discovery found a working xmlrpc.php, so XML-RPC is *enabled* — the credentials or the + // transport are what failed. Don't persist, and don't claim XML-RPC is off. + appLogWrapper.w(AppLog.T.API, "XML-RPC verification failed for ${site.url}") + XmlRpcRecovery.Inconclusive + } else { + XmlRpcRecovery.Recovered(endpoint) + } + } catch (e: CancellationException) { + throw e + } catch (e: SelfHostedEndpointFinder.DiscoveryException) { + if (e.discoveryError.indicatesXmlRpcUnavailable()) { + appLogWrapper.w(AppLog.T.API, "XML-RPC unavailable for ${site.url} (${e.discoveryError})") + XmlRpcRecovery.Unavailable + } else { + appLogWrapper.w( + AppLog.T.API, + "XML-RPC discovery inconclusive for ${site.url} (${e.discoveryError})" + ) + XmlRpcRecovery.Inconclusive + } + } catch (e: Exception) { + // Best-effort recovery must never let an unexpected throw escape and cancel the + // provisioning pipeline (mirrors SiteApiRestUrlRecoverer). An unexpected throw says nothing + // about the endpoint, so it's inconclusive rather than a negative. + appLogWrapper.e( + AppLog.T.API, + "XML-RPC discovery threw for ${site.url}: ${e::class.simpleName}: ${e.message}" + ) + XmlRpcRecovery.Inconclusive + } + } + + suspend fun persistXmlRpcUrl(localId: Int, xmlRpcUrl: String): Boolean = withContext(ioDispatcher) { + val rowsUpdated = siteSqlUtils.updateXmlRpcUrl(localId, xmlRpcUrl) + if (rowsUpdated == 0) { + appLogWrapper.w(AppLog.T.API, "Cannot persist xmlRpcUrl: no site with localId=$localId") + false + } else { + appLogWrapper.d(AppLog.T.API, "Persisted xmlRpcUrl=$xmlRpcUrl for localId=$localId") + true + } + } + + // A failed discovery is only a genuine "disabled" signal when it reached a definitive conclusion. + // Transient errors (RATE_LIMITED, GENERIC_ERROR) and unrelated conditions (auth/SSL/invalid URL) + // must not surface the warning, to avoid false positives on throttled sites. + private fun SelfHostedEndpointFinder.DiscoveryError.indicatesXmlRpcUnavailable(): Boolean = + this == SelfHostedEndpointFinder.DiscoveryError.NO_SITE_ERROR || + this == SelfHostedEndpointFinder.DiscoveryError.MISSING_XMLRPC_METHOD || + this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_BLOCKED || + this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_FORBIDDEN +} + +/** The outcome of an XML-RPC endpoint recovery attempt. See [SiteXmlRpcUrlRecoverer]. */ +sealed interface XmlRpcRecovery { + /** Discovered and authenticated against [endpoint] — safe to persist. */ + data class Recovered(val endpoint: String) : XmlRpcRecovery + + /** Discovery reached a definitive negative: the site really has XML-RPC off. */ + data object Unavailable : XmlRpcRecovery + + /** The attempt failed without settling the question (transient error, bad credentials, + * unexpected throw). Claim nothing; the next run retries. */ + data object Inconclusive : XmlRpcRecovery +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordFragment.kt index 85e65f552729..bc3631489100 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordFragment.kt @@ -95,12 +95,9 @@ class LoginSiteApplicationPasswordFragment : Fragment() { viewLifecycleOwner.lifecycleScope.launch { viewLifecycleOwner.repeatOnLifecycle(Lifecycle.State.STARTED) { + // Only ever a URL to navigate to; a failed discovery surfaces through errorMessage. viewModel.discoveryURL.collect { url -> - if (url.isEmpty()) { - viewModel.setError(getString(R.string.application_password_not_supported_error)) - } else { - activityNavigator.openApplicationPasswordLogin(requireActivity(), url) - } + activityNavigator.openApplicationPasswordLogin(requireActivity(), url) } } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModel.kt index e334e7886bae..985bb7b2f8ed 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModel.kt @@ -9,12 +9,15 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.asStateFlow import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch +import org.wordpress.android.R import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.viewmodel.ResourceProvider import javax.inject.Inject @HiltViewModel class LoginSiteApplicationPasswordViewModel @Inject constructor( private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, + private val resourceProvider: ResourceProvider, ) : ViewModel() { private val _discoveryURL = Channel(Channel.BUFFERED) val discoveryURL = _discoveryURL.receiveAsFlow() @@ -39,25 +42,38 @@ class LoginSiteApplicationPasswordViewModel @Inject constructor( _discoveryURL.send(result.authorizationUrl) is ApplicationPasswordLoginHelper.DiscoveryResult.WpComSite -> _wpComDetected.send(siteUrl) - is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> { - _errorMessage.value = result.userFacingMessage - _discoveryURL.send("") - } + // Report why discovery failed. Previously this also sent an empty discoveryURL, which + // the fragment read as "unsupported" and used to overwrite this message with a generic + // one — so the real reason never reached the user. Failure is signalled by + // [errorMessage] alone; discoveryURL only ever carries a URL to navigate to. + is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> + _errorMessage.value = messageFor(result) } _loadingStateFlow.value = false } } + /** + * Prefer an explanation the user can act on. The library's message for a private site is + * "failed to read its API configuration", which describes the symptom rather than the cause and + * reads as though the site is broken. + */ + private fun messageFor(result: ApplicationPasswordLoginHelper.DiscoveryResult.Failed): String = + when (result.reason) { + ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.PrivateSite -> + resourceProvider.getString(R.string.application_password_private_site_error) + ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.NotSupported -> + resourceProvider.getString(R.string.application_password_not_supported_error) + ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.Unknown -> + result.userFacingMessage + } + fun cancelDiscovery() { discoveryJob?.cancel() discoveryJob = null _loadingStateFlow.value = false } - fun setError(message: String) { - _errorMessage.value = message - } - fun clearError() { _errorMessage.value = null } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackConnectionHelper.kt b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackConnectionHelper.kt index 5206fed305a1..48f8932034ee 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackConnectionHelper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackConnectionHelper.kt @@ -2,7 +2,6 @@ package org.wordpress.android.ui.jetpackrestconnection import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.network.TrackNetworkRequestsInterceptor -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpNetworkAvailabilityProvider import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.util.AppLog @@ -14,17 +13,50 @@ import uniffi.wp_api.WpApiClientDelegate import uniffi.wp_api.WpApiMiddlewarePipeline import uniffi.wp_api.WpAppNotifier import uniffi.wp_api.WpAuthenticationProvider +import uniffi.wp_api.WpOrgSiteApiUrlResolver import javax.inject.Inject class JetpackConnectionHelper @Inject constructor( - private val wpApiClientProvider: WpApiClientProvider, private val appLogWrapper: AppLogWrapper, private val trackNetworkRequestsInterceptor: TrackNetworkRequestsInterceptor, private val networkAvailabilityProvider: WpNetworkAvailabilityProvider, ) { - fun initWpApiClient(site: SiteModel): WpApiClient { + /** + * Builds a **dedicated** [WpApiClient] for the Jetpack plugin-install step, talking to the site's + * own REST host with its application-password credentials. + * + * Unlike [org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider.getWpApiClient], a + * 401 from this client is handled **locally** via [onInvalidAuth] rather than broadcast on the + * app-wide `WpAppNotifierHandler`. An invalid-auth here is the connection flow's own concern (the + * view model resets and restarts it); broadcasting it would also wake the app-wide + * SiteProvisioningSource, whose concurrent validate / wipe / re-mint of these same credentials can + * race this in-flight connection — e.g. clearing the `apiRest*` columns that [requireRestCredentials] + * needs, mid-install. Keeping the install client off that bus leaves SiteProvisioningSource as the + * single global invalid-auth authority. This mirrors [initJetpackConnectionClient], whose + * [InvalidAuthNotifier] is likewise connection-local. + * + * [onInvalidAuth] fires at most once per client: install issues up to two requests (list, then + * create) and the flow should only restart once. + */ + fun initWpApiClient(site: SiteModel, onInvalidAuth: () -> Unit): WpApiClient { requireRestCredentials(site) - return wpApiClientProvider.getWpApiClient(site) + return WpApiClient( + apiUrlResolver = WpOrgSiteApiUrlResolver(ParsedUrl.parse(resolveRestApiUrl(site))), + authProvider = createRestAuthProvider(site), + requestExecutor = WpRequestExecutor( + interceptors = listOf(trackNetworkRequestsInterceptor), + networkAvailabilityProvider = networkAvailabilityProvider + ), + appNotifier = object : WpAppNotifier { + private var handled = false + override suspend fun requestedWithInvalidAuthentication(requestUrl: String) { + if (handled) return + handled = true + appLogWrapper.d(AppLog.T.API, "$TAG: requestedWithInvalidAuthentication (install client)") + onInvalidAuth() + } + }, + ) } fun initJetpackConnectionClient(site: SiteModel): JetpackConnectionClient { @@ -61,8 +93,11 @@ class JetpackConnectionHelper @Inject constructor( } } + // takeIf: wpApiRestUrl can be an empty string as well as null (the column is cleared, not + // dropped), and ParsedUrl.parse("") throws. Mirrors WpApiClientProvider.buildUrl, which the + // install client used before it got its own notifier-free client. private fun resolveRestApiUrl(site: SiteModel) = - site.wpApiRestUrl ?: "${site.url}/wp-json" + site.wpApiRestUrl?.takeIf { it.isNotEmpty() } ?: "${site.url}/wp-json" private inner class InvalidAuthNotifier : WpAppNotifier { override suspend fun requestedWithInvalidAuthentication(requestUrl: String) { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt index f83612659a90..00e7cd19fbb0 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackInstaller.kt @@ -20,10 +20,15 @@ class JetpackInstaller @Inject constructor( private val jetpackConnectionHelper: JetpackConnectionHelper, private val appLogWrapper: AppLogWrapper, ) { + /** + * Installs/activates the Jetpack plugin on [site]. [onInvalidAuth] is invoked if the site rejects the + * application-password credentials (401) — handled locally by the caller rather than via the app-wide + * notifier, so it can't race a concurrent provisioning heal (see [JetpackConnectionHelper.initWpApiClient]). + */ @Suppress("TooGenericExceptionCaught") - suspend fun installJetpack(site: SiteModel): Result { + suspend fun installJetpack(site: SiteModel, onInvalidAuth: () -> Unit): Result { return try { - val apiClient = jetpackConnectionHelper.initWpApiClient(site) + val apiClient = jetpackConnectionHelper.initWpApiClient(site, onInvalidAuth) val info = getPluginInfo(apiClient) when (info?.status) { PluginStatus.ACTIVE, PluginStatus.NETWORK_ACTIVE -> { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt index 302d54e9e3a7..e60805e78243 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModel.kt @@ -16,7 +16,6 @@ import org.wordpress.android.analytics.AnalyticsTracker.JETPACK_REST_CONNECT_STA import org.wordpress.android.analytics.AnalyticsTracker.JETPACK_REST_CONNECT_STATE_STARTED import org.wordpress.android.analytics.AnalyticsTracker.JETPACK_REST_CONNECT_STATE_STEP_KEY import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.modules.BG_THREAD @@ -41,8 +40,7 @@ class JetpackRestConnectionViewModel @Inject constructor( private val jetpackModuleHelper: JetpackStatsModuleHelper, private val appLogWrapper: AppLogWrapper, private val analyticsTrackerWrapper: AnalyticsTrackerWrapper, - private val wpAppNotifierHandler: WpAppNotifierHandler, -) : ScopedViewModel(mainDispatcher), WpAppNotifierHandler.NotifierListener { +) : ScopedViewModel(mainDispatcher) { // Internal variables that can be overridden for testing internal var uiDelayMs: Long = UI_DELAY_MS internal var stepTimeoutMs: Long = STEP_TIMEOUT_MS @@ -80,7 +78,6 @@ class JetpackRestConnectionViewModel @Inject constructor( _uiEvent.value = null analyticsTrackerWrapper.track(AnalyticsTracker.Stat.JETPACK_REST_CONNECT_STARTED) - wpAppNotifierHandler.addListener(this) startStep(fromStep ?: ConnectionStep.LoginWpCom) } @@ -95,7 +92,6 @@ class JetpackRestConnectionViewModel @Inject constructor( _buttonType.value = ButtonType.Retry } - wpAppNotifierHandler.removeListener(this) _currentStep.value = null } @@ -362,7 +358,7 @@ class JetpackRestConnectionViewModel @Inject constructor( * Step 2: Installs Jetpack to the current site if not already installed */ private suspend fun installJetpack() { - val result = jetpackInstaller.installJetpack(site) + val result = jetpackInstaller.installJetpack(site, onInvalidAuth = ::onInstallAuthFailed) result.fold( onSuccess = { status -> @@ -475,12 +471,14 @@ class JetpackRestConnectionViewModel @Inject constructor( } /** - * Called when auth fails in the WpApiClient created in JetpackConnectionHelper.initWpApiClient, reset the - * access token and restart the connection flow so the user sees the login page + * Invoked when the site rejects the application password during the Jetpack install step. This is + * delivered **locally** by the install client's notifier (see [JetpackConnectionHelper.initWpApiClient]), + * not by the app-wide `WpAppNotifierHandler`, so the app-wide SiteProvisioningSource doesn't also react + * and race this connection (see #22944). Reset the access token and restart the flow so the user sees + * the login page. */ - override fun onRequestedWithInvalidAuthentication(siteUrl: String) { + private fun onInstallAuthFailed() { appLogWrapper.d(AppLog.T.API, "$TAG: Invalid authentication, restarting") - wpAppNotifierHandler.removeListener(this) accountStore.resetAccessToken() clearValues() startConnectionFlow() diff --git a/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java index 32d5244a6316..3c239813e33c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/main/WPMainActivity.java @@ -44,7 +44,6 @@ import org.wordpress.android.fluxc.generated.SiteActionBuilder; import org.wordpress.android.fluxc.model.PostModel; import org.wordpress.android.fluxc.model.SiteModel; -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler; import org.wordpress.android.fluxc.network.rest.wpcom.site.PrivateAtomicCookie; import org.wordpress.android.fluxc.store.AccountStore; import org.wordpress.android.fluxc.store.AccountStore.AuthenticationErrorType; @@ -61,6 +60,7 @@ import org.wordpress.android.fluxc.store.SiteStore.OnSiteRemoved; import org.wordpress.android.inappupdate.IInAppUpdateManager; import org.wordpress.android.inappupdate.InAppUpdateListener; +import org.wordpress.android.ui.accounts.login.ApplicationPasswordReauthNotifier; import org.wordpress.android.ui.accounts.login.LoginAnalyticsListener; import org.wordpress.android.networking.NetworkConnectionMonitor; import org.wordpress.android.push.GCMMessageHandler; @@ -187,7 +187,7 @@ public class WPMainActivity extends BaseAppCompatActivity implements BloggingPromptsReminderSchedulerListener, BloggingPromptsOnboardingListener, UpdateSelectedSiteListener, - WpAppNotifierHandler.NotifierListener { + ApplicationPasswordReauthNotifier.Listener { public static final String ARG_CONTINUE_JETPACK_CONNECT = "ARG_CONTINUE_JETPACK_CONNECT"; public static final String ARG_CREATE_SITE = "ARG_CREATE_SITE"; public static final String ARG_IS_MAGIC_LINK_LOGIN = "ARG_IS_MAGIC_LINK_LOGIN"; @@ -276,7 +276,7 @@ public class WPMainActivity extends BaseAppCompatActivity implements @Inject PerAppLocaleManager mPerAppLocaleManager; - @Inject WpAppNotifierHandler mWpAppNotifierHandler; + @Inject ApplicationPasswordReauthNotifier mReauthNotifier; @Inject NetworkConnectionMonitor mNetworkConnectionMonitor; @@ -1052,7 +1052,7 @@ protected void onResume() { setUpMainView(); - mWpAppNotifierHandler.addListener(this); + mReauthNotifier.addListener(this); // Load selected site initSelectedSite(); @@ -1702,7 +1702,7 @@ public void onSetPromptReminderClick(final int siteId) { protected void onPause() { super.onPause(); - mWpAppNotifierHandler.removeListener(this); + mReauthNotifier.removeListener(this); } private void enableDeepLinkingComponentsIfNeeded() { @@ -1760,7 +1760,7 @@ private void showOpenPageMessageIfNeeded() { } } - @Override public void onRequestedWithInvalidAuthentication(@NonNull String siteUrl) { + @Override public void onReauthRequired(@NonNull String siteUrl) { showApplicationPasswordOffReauthenticateDialog(siteUrl); } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java b/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java index 1c0700b2a82d..bb0fbb94a515 100755 --- a/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/media/MediaBrowserActivity.java @@ -50,8 +50,8 @@ import org.wordpress.android.fluxc.model.MediaModel; import org.wordpress.android.fluxc.model.MediaModel.MediaUploadState; import org.wordpress.android.fluxc.model.SiteModel; -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler; import org.wordpress.android.fluxc.store.MediaStore; +import org.wordpress.android.ui.accounts.login.ApplicationPasswordReauthNotifier; import org.wordpress.android.fluxc.store.MediaStore.CancelMediaPayload; import org.wordpress.android.fluxc.store.MediaStore.OnMediaChanged; import org.wordpress.android.fluxc.store.MediaStore.OnMediaListFetched; @@ -107,7 +107,7 @@ */ public class MediaBrowserActivity extends BaseAppCompatActivity implements MediaGridListener, OnQueryTextListener, OnActionExpandListener, - WPMediaUtils.LaunchCameraCallback, WpAppNotifierHandler.NotifierListener { + WPMediaUtils.LaunchCameraCallback, ApplicationPasswordReauthNotifier.Listener { public static final String ARG_BROWSER_TYPE = "media_browser_type"; public static final String ARG_FILTER = "filter"; public static final String ARG_LAUNCH_PHOTO_PICKER = "launch_photo_picker"; @@ -127,7 +127,7 @@ public class MediaBrowserActivity extends BaseAppCompatActivity implements Media @Inject SelectedSiteRepository mSelectedSiteRepository; @Inject JetpackFeatureRemovalHelper mJetpackFeatureRemovalHelper; @Inject ActivityNavigator mActivityNavigator; - @Inject WpAppNotifierHandler mWpAppNotifierHandler; + @Inject ApplicationPasswordReauthNotifier mReauthNotifier; @Inject LiveData mConnectionStatus; private SiteModel mSite; @@ -312,7 +312,7 @@ private void showQuota(boolean show) { } } - @Override public void onRequestedWithInvalidAuthentication(@NonNull String siteUrl) { + @Override public void onReauthRequired(@NonNull String siteUrl) { showApplicationPasswordReauthenticateDialog(siteUrl); } @@ -438,7 +438,7 @@ private void setFilter(@NonNull MediaFilter filter) { public void onStart() { super.onStart(); - mWpAppNotifierHandler.addListener(this); + mReauthNotifier.addListener(this); mDispatcher.register(this); EventBus.getDefault().register(this); @@ -463,7 +463,7 @@ protected void onResume() { @Override public void onStop() { - mWpAppNotifierHandler.removeListener(this); + mReauthNotifier.removeListener(this); EventBus.getDefault().unregister(this); mDispatcher.unregister(this); super.onStop(); diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteCardAndItem.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteCardAndItem.kt index 93ef3128cabf..5df28c804cf4 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteCardAndItem.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/MySiteCardAndItem.kt @@ -372,12 +372,18 @@ sealed class MySiteCardAndItem(open val type: Type) { ) : MySiteCardAndItem(type) { data class InfoItem(val title: UiString) : Item(INFO_ITEM) + /** + * [centerText] presents the card as a plain notice: the image is hidden and the text is + * centred across the whole card rather than sitting in the column beside the icon. Use it + * when the card only explains something and has nothing to tap. + */ data class SingleActionCard( @StringRes val textResource: Int, @DrawableRes val imageResource: Int, val onActionClick: () -> Unit, val centerImageVertically: Boolean = false, - val showLearnMore: Boolean = true + val showLearnMore: Boolean = true, + val centerText: Boolean = false ) : Item(SINGLE_ACTION_CARD) data class CategoryHeaderItem(val title: UiString) : Item(CATEGORY_HEADER_ITEM) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index 63cd793d395a..2634ab5a8223 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -2,22 +2,17 @@ package org.wordpress.android.ui.mysite.cards.applicationpassword import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData -import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.launch -import kotlinx.coroutines.withContext import org.wordpress.android.R -import androidx.annotation.VisibleForTesting import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder -import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem import org.wordpress.android.ui.mysite.MySiteCardAndItem.Card.QuickLinksItem.QuickLinkItem import org.wordpress.android.ui.mysite.SiteNavigationAction @@ -25,22 +20,26 @@ import org.wordpress.android.ui.pages.SnackbarMessageHolder import org.wordpress.android.ui.utils.ListItemInteraction import org.wordpress.android.ui.utils.UiString.UiStringRes import org.wordpress.android.util.AppLog -import org.wordpress.android.modules.IO_THREAD import org.wordpress.android.viewmodel.Event import javax.inject.Inject -import javax.inject.Named +/** + * Renders the application-password card from the site's readiness. All the + * provisioning mechanics — validate, mint, REST-root recovery, XML-RPC + * recovery — live in [SiteProvisioningSource]; this slice is a thin view over + * the result: + * + * - [SiteAuthState.Unprovisionable] → a re-authentication banner (creds went + * bad) or a first-time "authenticate" card, looked up lazily. + * - provisioned (any non-[SiteReadiness.NeedsAuth] terminal state) → hidden, + * except true self-hosted sites whose XML-RPC endpoint the pipeline couldn't + * recover, which get the XML-RPC-disabled card. + */ class ApplicationPasswordViewModelSlice @Inject constructor( private val applicationPasswordLoginHelper: ApplicationPasswordLoginHelper, private val siteStore: SiteStore, private val appLogWrapper: AppLogWrapper, - private val wpApiClientProvider: WpApiClientProvider, - private val applicationPasswordValidator: ApplicationPasswordValidator, - private val selfHostedEndpointFinder: SelfHostedEndpointFinder, - private val siteXMLRPCClient: SiteXMLRPCClient, - private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, - private val credentialsChangedNotifier: CredentialsChangedNotifier, - @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, + private val siteProvisioningSource: SiteProvisioningSource, ) { lateinit var scope: CoroutineScope @@ -57,101 +56,54 @@ class ApplicationPasswordViewModelSlice @Inject constructor( val uiModelMutable = MutableLiveData() val uiModel: LiveData = uiModelMutable - // Single-flight guard: buildCard is invoked from onResume / refresh / onSitePicked, which can - // fire close together. Without this, two coroutines both pass the "creds missing" check in - // ApplicationPasswordsManager and issue two server-side mints. Worse, the 409 conflict handler - // then deletes-and-recreates the winner's password, so the losing racer destroys working creds. - private var buildJob: Job? = null + private var collectJob: Job? = null + private var currentSite: SiteModel? = null fun buildCard(siteModel: SiteModel) { - if (buildJob?.isActive == true) { - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Skipping buildCard for ${siteModel.url} - previous run still in flight" - ) - return - } - buildJob = scope.launch { - val storedSite = siteStore.sites.firstOrNull { it.id == siteModel.id } ?: siteModel - val hadCreds = !applicationPasswordLoginHelper.siteHasBadCredentials(storedSite) - - // Step 1: if we already have stored creds, validate them with Basic auth against the - // direct host. This actually exercises the application password (unlike - // WpApiClientProvider.getWpApiClient, which routes WPCom-flagged sites through the - // bearer-token path and would not catch a revoked password). - if (hadCreds) { - when (applicationPasswordValidator.validate(storedSite)) { - ApplicationPasswordValidator.Outcome.Valid -> { - // Heal in the background so the card hides immediately on a slow network. - scope.launch { healApiRestUrlIfMissing(storedSite) } - handleValidAuth(storedSite) - return@launch - } - ApplicationPasswordValidator.Outcome.NetworkUnavailable -> { - // Don't punish flaky networks — leave the card hidden and try again next time. - uiModelMutable.postValue(null) - appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation network error for ${storedSite.url}") - return@launch - } - ApplicationPasswordValidator.Outcome.Invalid -> { - // Stored creds are stale (revoked, deleted, etc.) — clear them so the next - // mint creates fresh ones, and invalidate the cached client. - appLogWrapper.d(AppLog.T.MAIN, "A_P: Stored creds invalid for ${storedSite.url}, clearing") - siteStore.deleteStoredApplicationPasswordCredentials(storedSite) - wpApiClientProvider.clearSelfHostedClient(storedSite.id) - } - } - } - - // Step 2: mint a fresh application password via the FluxC Jetpack tunnel. wordpress-rs - // can't do this today — the WP.com REST proxy doesn't expose the application-passwords - // endpoint under /wp/v2/sites/{id}/... (see Automattic/wordpress-rs#1350) — so FluxC's - // Jetpack-tunnel client is the only working path for Atomic / Jetpack-WPCom-REST sites. - val createResult = siteStore.createApplicationPassword(storedSite) - if (!createResult.isError && createResult.credentials != null) { - wpApiClientProvider.clearSelfHostedClient(storedSite.id) - appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${storedSite.url}") - credentialsChangedNotifier.notifyChanged(storedSite.id) - // The mint goes through the Jetpack tunnel and never runs discovery — without this - // step, freshly minted Atomic sites end up with working creds but a NULL - // wpApiRestUrl in the local DB. Run in the background so the card hides immediately. - scope.launch { healApiRestUrlIfMissing(storedSite) } - handleValidAuth(storedSite) - return@launch - } - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Headless mint failed for ${storedSite.url} (notSupported=" + - "${createResult.error?.notSupported})" - ) - - // Step 3: mint failed. If we started with creds, show the reauth banner; otherwise the - // standard "authenticate" card. Either way, discovery is required to populate the URL. - if (hadCreds) { - buildReauthenticationBanner(storedSite) - } else { - buildAuthenticationCard(storedSite) + collectJob?.cancel() + currentSite = siteModel + collectJob = scope.launch { + siteProvisioningSource.stateFor(siteModel).collect { readiness -> + // Bail if the user switched sites while suspended — postValue is not a + // suspension point, so cancellation alone won't catch this. + if (currentSite?.id != siteModel.id) return@collect + renderCard(siteModel, readiness) } } } - private suspend fun healApiRestUrlIfMissing(site: SiteModel) { - if (!site.wpApiRestUrl.isNullOrEmpty()) return - siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)?.let { apiRootUrl -> - site.wpApiRestUrl = apiRootUrl - siteApiRestUrlRecoverer.persistApiRootUrl(site.id, apiRootUrl) + private suspend fun renderCard(site: SiteModel, readiness: SiteReadiness) { + when (readiness) { + is SiteReadiness.NeedsAuth -> when (val auth = readiness.auth) { + is SiteAuthState.Unprovisionable -> + if (auth.hadCredentials) buildReauthenticationBanner(site) else buildAuthenticationCard(site) + SiteAuthState.Provisioning -> { + // Mint in flight / transient validation error — hide and let the next run retry. + uiModelMutable.postValue(null) + appLogWrapper.d(AppLog.T.MAIN, "A_P: Provisioning in progress for ${site.url}") + } + } + // Any terminal provisioned state — the credentials are usable, so the only card left to + // show is the self-hosted XML-RPC fallback. Capability outcome (Ready/Unreachable) is the + // connectivity banner's concern, not this card's. + SiteReadiness.Ready, + SiteReadiness.Unreachable, + SiteReadiness.TransientError -> handleProvisioned(site) + SiteReadiness.Probing -> Unit // leave the card unchanged while the pipeline runs } } - private fun handleValidAuth(site: SiteModel) { - // Only true self-hosted sites need the XML-RPC fallback path — Atomic and Jetpack-WPCom-REST - // sites talk REST end-to-end and don't need XML-RPC. - if (!site.isUsingWpComRestApi && site.xmlRpcUrl.isNullOrEmpty()) { - // A missing xmlRpcUrl doesn't mean XML-RPC is disabled — the login fetch may have fallen back to - // WPAPI on a transient error (e.g. a 429 rate-limit). Keep the card hidden and let rediscovery - // decide; it only surfaces the warning on a definitive negative. - uiModelMutable.postValue(null) - attemptXmlRpcRediscovery(site) + private fun handleProvisioned(site: SiteModel) { + // Read fresh: the pipeline's parallel XML-RPC branch may have just recovered the endpoint. + val storedSite = siteStore.getSiteByLocalId(site.id) ?: site + // Only true self-hosted sites need XML-RPC, and a missing endpoint alone isn't proof it's off — + // recovery also fails transiently (e.g. a 429). Warn only when the pipeline reached a definitive + // negative, so a throttled site doesn't get a false "XML-RPC Disabled". + if (!storedSite.isUsingWpComRestApi && + storedSite.xmlRpcUrl.isNullOrEmpty() && + siteProvisioningSource.isXmlRpcUnavailable(storedSite.id) + ) { + buildXmlRpcDisabledCard(storedSite) } else { uiModelMutable.postValue(null) appLogWrapper.d(AppLog.T.MAIN, "A_P: Hiding card for ${site.url} - authenticated") @@ -177,14 +129,7 @@ class ApplicationPasswordViewModelSlice @Inject constructor( "A_P: Hiding reauthentication card for ${site.url} - WordPress.com site" ) } - is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> { - // TODO follow-up: surface result.userFacingMessage in the card (issue #22884). - uiModelMutable.postValue(null) - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Hiding reauthentication card for ${site.url} - bad discovery: ${result.userFacingMessage}" - ) - } + is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> handleFailedDiscovery(site, result) } } @@ -197,15 +142,40 @@ class ApplicationPasswordViewModelSlice @Inject constructor( uiModelMutable.postValue(null) appLogWrapper.d(AppLog.T.MAIN, "A_P: Hiding card for ${site.url} - WordPress.com site") } - is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> { - // TODO follow-up: surface result.userFacingMessage in the card (issue #22884). - uiModelMutable.postValue(null) - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: Hiding card for ${site.url} - bad discovery: ${result.userFacingMessage}" + is ApplicationPasswordLoginHelper.DiscoveryResult.Failed -> handleFailedDiscovery(site, result) + } + } + + /** + * Discovery couldn't produce an authorization URL, so neither the create nor the re-authenticate + * card can be built. A private site is the one cause we can name: its Privacy gate answers the + * anonymous discovery request with a 403, and without saying so the user sees nothing at all + * while their credentials stay broken. Every other cause still hides the card pending #22884. + */ + private fun handleFailedDiscovery( + site: SiteModel, + result: ApplicationPasswordLoginHelper.DiscoveryResult.Failed, + ) { + if (result.reason == ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.PrivateSite) { + uiModelMutable.postValue( + MySiteCardAndItem.Item.SingleActionCard( + textResource = R.string.application_password_private_site_card, + // Hidden by centerText — the card is a notice with nothing to tap. + imageResource = R.drawable.ic_notice_white_24dp, + onActionClick = { }, + showLearnMore = false, + centerText = true, ) - } + ) + appLogWrapper.d(AppLog.T.MAIN, "A_P: Showing private-site card for ${site.url}") + return } + // TODO follow-up: surface result.userFacingMessage in the card (issue #22884). + uiModelMutable.postValue(null) + appLogWrapper.d( + AppLog.T.MAIN, + "A_P: Hiding card for ${site.url} - bad discovery: ${result.userFacingMessage}" + ) } private fun showApplicationPasswordCreateCard(site: SiteModel, alternativeUrl: String) { @@ -246,60 +216,6 @@ class ApplicationPasswordViewModelSlice @Inject constructor( ) } - @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) - internal fun attemptXmlRpcRediscovery(site: SiteModel) { - scope.launch { - val xmlRpcEndpoint = try { - withContext(ioDispatcher) { - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(site.url) - } - } catch (e: SelfHostedEndpointFinder.DiscoveryException) { - // Only surface the "XML-RPC Disabled" warning on a definitive negative. A transient failure - // (e.g. a 429 rate-limit) leaves the state unknown, so keep the card hidden and re-check on the - // next refresh rather than wrongly claiming XML-RPC is off. - if (e.discoveryError.indicatesXmlRpcUnavailable()) { - buildXmlRpcDisabledCard(site) - } else { - uiModelMutable.postValue(null) - appLogWrapper.d( - AppLog.T.MAIN, - "A_P: XML-RPC rediscovery inconclusive for ${site.url} " + - "(${e.discoveryError}) - hiding card" - ) - } - return@launch - } - - // Discovery verified a working xmlrpc.php, so XML-RPC is enabled. Confirm the credentials with an - // authenticated call before persisting the endpoint, but keep the card hidden either way. - val result = withContext(ioDispatcher) { - siteXMLRPCClient.fetchSites( - xmlRpcEndpoint, - site.apiRestUsernamePlain, - site.apiRestPasswordPlain - ) - } - if (!result.isError) { - site.xmlRpcUrl = xmlRpcEndpoint - // Persist only the rediscovered column — mirrors healApiRestUrlIfMissing. A full-row - // updateSite would rewrite ~80 columns from this in-memory model for a one-field change - // (risking clobbering other out-of-band values), so write just xmlRpcUrl. - siteStore.persistXmlRpcUrl(site.id, xmlRpcEndpoint) - } - uiModelMutable.postValue(null) - } - } - - // A missing/unusable XML-RPC endpoint is only a genuine "disabled" signal when discovery reaches a - // definitive conclusion. Transient errors (RATE_LIMITED, GENERIC_ERROR) and unrelated conditions - // (auth/SSL/invalid URL) must not surface the warning, to avoid false positives on throttled sites. - private fun SelfHostedEndpointFinder.DiscoveryError.indicatesXmlRpcUnavailable(): Boolean = - this == SelfHostedEndpointFinder.DiscoveryError.NO_SITE_ERROR || - this == SelfHostedEndpointFinder.DiscoveryError.MISSING_XMLRPC_METHOD || - this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_BLOCKED || - this == SelfHostedEndpointFinder.DiscoveryError.XMLRPC_FORBIDDEN - private fun onClick(site: SiteModel, alternativeUrl: String) { _onNavigation.postValue( Event( diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt index 5a93cd7df624..cb2a07ba82bd 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt @@ -1,91 +1,80 @@ package org.wordpress.android.ui.mysite.cards.connectivity import androidx.lifecycle.LiveData +import androidx.lifecycle.MediatorLiveData import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job import kotlinx.coroutines.launch import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.mysite.MySiteCardAndItem -import org.wordpress.android.ui.mysite.SelectedSiteRepository import org.wordpress.android.util.NetworkUtilsWrapper +import org.wordpress.android.viewmodel.helpers.ConnectionStatus import javax.inject.Inject class SiteConnectivityBannerViewModelSlice @Inject constructor( - private val editorSettingsRepository: EditorSettingsRepository, + private val siteProvisioningSource: SiteProvisioningSource, private val networkUtilsWrapper: NetworkUtilsWrapper, - private val credentialsChangedNotifier: CredentialsChangedNotifier, - private val selectedSiteRepository: SelectedSiteRepository, + connectionStatus: LiveData, ) { private lateinit var scope: CoroutineScope - private var currentJob: Job? = null + private var collectJob: Job? = null private var currentSite: SiteModel? = null - private val _uiModel = MutableLiveData() + private val readiness = MutableLiveData() + + private val _uiModel = MediatorLiveData() val uiModel: LiveData = _uiModel - /* Site capabilities rarely change, so once we've successfully fetched them for a site we - skip subsequent non-user-initiated fetches in this slice's lifetime. Failed fetches do - not populate this set, so a transient network failure recovers on the next onResume. - User-initiated calls (PTR, banner retry) always bypass this gate. */ - private val fetchedCapabilitiesForSite = mutableSetOf() + init { + _uiModel.addSource(readiness) { render() } + // Losing the network doesn't re-run the pipeline, so the readiness we hold stays Unreachable + // and the banner would sit there stacked on the global "no connection" bar. This source is a + // change trigger only — it emits on transitions and swallows its initial value — so the + // decision reads the live availability rather than the emitted status. + _uiModel.addSource(connectionStatus) { render() } + } fun initialize(scope: CoroutineScope) { this.scope = scope - // Re-run detection the moment an application password is established for the selected site - // (e.g. the headless mint finished after our first fetch lost the race), instead of waiting - // for the next resume/refresh. Re-read the selected site so we see the just-persisted - // credentials; isUserInitiated = false so a replayed event is a no-op once cached. - scope.launch { - credentialsChangedNotifier.events.collect { siteLocalId -> - val site = selectedSiteRepository.getSelectedSite() - if (site != null && site.id == siteLocalId) { - fetchCapabilities(site, isUserInitiated = false) - } - } - } } + /** + * Subscribes the banner to [site]'s readiness. The banner is a thin view over + * that state — it surfaces only on [SiteReadiness.Unreachable], which the pipeline + * reports both when the capability probe failed and when the site couldn't be + * reached at the auth stage while the device was online. Every other state + * (probing, needs auth, offline, ready) leaves it hidden: when credentials are the + * problem the application-password card owns it, and the banner stays out of the + * way. [isUserInitiated] (pull-to-refresh, retry) forces a fresh run. + */ fun fetchCapabilities(site: SiteModel, isUserInitiated: Boolean) { - currentJob?.cancel() + collectJob?.cancel() currentSite = site - currentJob = scope.launch { - if (site.id in fetchedCapabilitiesForSite && !isUserInitiated) { - return@launch + if (isUserInitiated) siteProvisioningSource.invalidate(site) + collectJob = scope.launch { + siteProvisioningSource.stateFor(site).collect { state -> + // Bail if the user switched sites while suspended — postValue is + // not a suspension point, so cancellation alone won't catch this. + if (currentSite?.id != site.id) return@collect + readiness.postValue(state) } - val ok = editorSettingsRepository.fetchEditorCapabilitiesForSite(site) - if (ok) { - fetchedCapabilitiesForSite.add(site.id) - } - val hasCache = editorSettingsRepository.hasCachedCapabilities(site) - // Bail if the user switched sites while we were suspended — postValue - // isn't a suspension point, so cancellation alone won't catch this. - if (currentSite?.id != site.id) return@launch - // Suppress the banner when the device is offline — the global "no - // connection" banner already covers this case, and stacking warnings - // for the same root cause is just noise. - val suppressForOffline = !ok && !networkUtilsWrapper.isNetworkAvailable() - // Atomic sites probe the direct host with an application password that's minted - // asynchronously on this same screen, so a first-login fetch can fail purely because - // the credential isn't ready yet. Treat that as pending, not a connection failure — - // the application-password card owns that state and a later fetch will succeed. - val suppressForPendingAuth = - !ok && editorSettingsRepository.isAwaitingApplicationPassword(site) - // Show the banner only as a last resort — not when detection succeeded, when we have - // cached capabilities, or while a transient non-error state (offline / pending creds) - // already explains the failure. - val suppressBanner = ok || hasCache || suppressForOffline || suppressForPendingAuth - _uiModel.postValue(if (suppressBanner) null else buildBanner()) } } fun clearBanner() { - currentJob?.cancel() + collectJob?.cancel() currentSite = null - _uiModel.postValue(null) + readiness.postValue(null) + } + + private fun render() { + val showBanner = readiness.value is SiteReadiness.Unreachable && + networkUtilsWrapper.isNetworkAvailable() + _uiModel.value = if (showBanner) buildBanner() else null } private fun buildBanner(): MySiteCardAndItem.Item.SingleActionCard = @@ -93,10 +82,7 @@ class SiteConnectivityBannerViewModelSlice @Inject constructor( textResource = R.string.site_connectivity_banner_text, imageResource = R.drawable.ic_cloud_off_themed_24dp, onActionClick = { - val site = currentSite - if (site != null && currentJob?.isActive != true) { - fetchCapabilities(site, isUserInitiated = true) - } + currentSite?.let { fetchCapabilities(it, isUserInitiated = true) } }, showLearnMore = false, centerImageVertically = true, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/items/singleactioncard/SingleActionCardViewHolder.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/items/singleactioncard/SingleActionCardViewHolder.kt index 2bb9d1f45250..77a27a2c6718 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/items/singleactioncard/SingleActionCardViewHolder.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/items/singleactioncard/SingleActionCardViewHolder.kt @@ -1,9 +1,11 @@ package org.wordpress.android.ui.mysite.items.singleactioncard +import android.view.Gravity import android.view.View import android.view.ViewGroup import androidx.constraintlayout.widget.ConstraintLayout import androidx.core.content.ContextCompat +import org.wordpress.android.R import org.wordpress.android.databinding.MySiteSingleActionCardItemBinding import org.wordpress.android.ui.mysite.MySiteCardAndItem.Item.SingleActionCard import org.wordpress.android.ui.mysite.MySiteCardAndItemViewHolder @@ -16,40 +18,48 @@ class SingleActionCardViewHolder( ) { fun bind(singleActionCard: SingleActionCard) = with(binding) { val context = root.context - singleActionCardText.text = - context.getString(singleActionCard.textResource) + singleActionCardText.text = context.getString(singleActionCard.textResource) singleActionCardImage.setImageDrawable( ContextCompat.getDrawable(context, singleActionCard.imageResource) ) - singleActionCardCover.setOnClickListener { - singleActionCard.onActionClick() - } - learnMore.visibility = if (singleActionCard.showLearnMore) { - View.VISIBLE + singleActionCardCover.setOnClickListener { singleActionCard.onActionClick() } + learnMore.visibility = if (singleActionCard.showLearnMore) View.VISIBLE else View.GONE + bindText(singleActionCard) + bindImage(singleActionCard) + } + + /** + * Every branch here is written both ways round: view holders are recycled, so a card bound with + * one presentation must not leave the next card wearing it. + */ + private fun MySiteSingleActionCardItemBinding.bindText(singleActionCard: SingleActionCard) { + val marginExtraLarge = root.context.resources.getDimensionPixelSize(R.dimen.margin_extra_large) + val params = singleActionCardText.layoutParams as ConstraintLayout.LayoutParams + if (singleActionCard.showLearnMore) { + params.bottomToBottom = ConstraintLayout.LayoutParams.UNSET + params.bottomMargin = 0 } else { - View.GONE + params.bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID + params.bottomMargin = marginExtraLarge } - val textParams = singleActionCardText.layoutParams - as ConstraintLayout.LayoutParams - if (!singleActionCard.showLearnMore) { - textParams.bottomToBottom = - ConstraintLayout.LayoutParams.PARENT_ID - textParams.bottomMargin = context.resources - .getDimensionPixelSize( - org.wordpress.android.R.dimen.margin_extra_large - ) + // Centred cards span the whole card rather than the column beside the icon, which + // bindImage hides. + if (singleActionCard.centerText) { + singleActionCardText.gravity = Gravity.CENTER_HORIZONTAL + params.startToEnd = ConstraintLayout.LayoutParams.UNSET + params.startToStart = ConstraintLayout.LayoutParams.PARENT_ID } else { - textParams.bottomToBottom = - ConstraintLayout.LayoutParams.UNSET - textParams.bottomMargin = 0 + singleActionCardText.gravity = Gravity.START + params.startToEnd = singleActionCardImage.id + params.startToStart = ConstraintLayout.LayoutParams.UNSET } - singleActionCardText.layoutParams = textParams - val marginExtraLarge = context.resources - .getDimensionPixelSize( - org.wordpress.android.R.dimen.margin_extra_large - ) - val params = singleActionCardImage.layoutParams - as ConstraintLayout.LayoutParams + singleActionCardText.layoutParams = params + } + + private fun MySiteSingleActionCardItemBinding.bindImage(singleActionCard: SingleActionCard) { + singleActionCardImage.visibility = if (singleActionCard.centerText) View.GONE else View.VISIBLE + val marginExtraLarge = root.context.resources.getDimensionPixelSize(R.dimen.margin_extra_large) + val params = singleActionCardImage.layoutParams as ConstraintLayout.LayoutParams if (singleActionCard.centerImageVertically) { params.topToTop = ConstraintLayout.LayoutParams.PARENT_ID params.bottomToBottom = ConstraintLayout.LayoutParams.PARENT_ID @@ -57,8 +67,7 @@ class SingleActionCardViewHolder( params.bottomMargin = 0 } else { params.topToTop = ConstraintLayout.LayoutParams.UNSET - params.bottomToBottom = - ConstraintLayout.LayoutParams.UNSET + params.bottomToBottom = ConstraintLayout.LayoutParams.UNSET params.topMargin = marginExtraLarge params.bottomMargin = marginExtraLarge } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt index 5a9a116b8769..4d757b6d00cc 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergEditorPreloader.kt @@ -10,9 +10,9 @@ import kotlinx.coroutines.launch import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore +import org.wordpress.android.fluxc.store.SiteStore import org.wordpress.android.modules.BG_THREAD -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.android.util.AppLog import org.wordpress.gutenberg.model.EditorDependencies import java.util.concurrent.ConcurrentHashMap @@ -63,8 +63,8 @@ class GutenbergEditorPreloader @Inject constructor( private val gutenbergKitSettingsBuilder: GutenbergKitSettingsBuilder, private val siteSettingsProvider: SiteSettingsProvider, private val editorServiceProvider: EditorServiceProvider, - private val editorSettingsRepository: EditorSettingsRepository, - private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, + private val siteProvisioningSource: SiteProvisioningSource, + private val siteStore: SiteStore, @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher ) { private sealed class PreloadState { @@ -95,12 +95,13 @@ class GutenbergEditorPreloader @Inject constructor( val siteId = site.id val job = scope.launch(bgDispatcher) { try { - if (site.wpApiRestUrl.isNullOrEmpty()) { - siteApiRestUrlRecoverer.discoverApiRootUrl(site.url) - ?.let { site.wpApiRestUrl = it } - } - editorSettingsRepository - .fetchEditorCapabilitiesForSite(site) + // The provisioning source mints creds, recovers the REST root, and + // detects capabilities — one shared, deduplicated run, so the preloader + // and connectivity banner can't double-probe. We only need it to have + // run; re-read the provisioned site so the config points at the + // recovered REST root. + siteProvisioningSource.await(site) + val provisionedSite = siteStore.getSiteByLocalId(siteId) ?: site // Preloading produces EditorDependencies, which the editor // consumes alongside its own per-launch EditorConfiguration. // Cookies and network-logging are per-launch concerns the @@ -108,7 +109,7 @@ class GutenbergEditorPreloader @Inject constructor( // defaults here. val config = gutenbergKitSettingsBuilder .buildPostConfiguration( - site = site, + site = provisionedSite, accessToken = accountStore.accessToken, cookies = emptyMap(), isNetworkLoggingEnabled = false, @@ -146,6 +147,9 @@ class GutenbergEditorPreloader @Inject constructor( @MainThread fun refreshPreloading(site: SiteModel, scope: CoroutineScope) { clearSite(site) + // Pull-to-refresh: force a fresh provisioning run so the await in + // preloadIfNeeded re-detects instead of returning the cached result. + siteProvisioningSource.invalidate(site) preloadIfNeeded(site, scope) } diff --git a/WordPress/src/main/res/values/strings.xml b/WordPress/src/main/res/values/strings.xml index 90e9c9afba18..e80047d4d379 100644 --- a/WordPress/src/main/res/values/strings.xml +++ b/WordPress/src/main/res/values/strings.xml @@ -4590,6 +4590,8 @@ translators: %s: Select control option value e.g: "Auto, 25%". --> Authorization cancelled Authenticate using Application Password The provided site does not support Application Password authentication. + This site is private, so we can\'t read its settings to sign you in. + This site is private, so we can\'t reconnect to it. Invalid Application Password Your application password no longer exists. Please sign in again to create a new application password. Application Password Required diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt index c4371b8675fb..a3bd6301355c 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt @@ -229,6 +229,36 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { verify(wpApiClientProvider, never()).getWpApiClient(atomicSite) } + @Test + fun `jetpack site probes the direct host, not the WP_com proxy`() = + runTest { + val jetpackSite = SiteModel().apply { + id = 7 + url = "https://jetpack.example.com" + // isUsingWpComRestApi via Jetpack, but not Atomic and not WP.com Simple → direct host. + setIsJetpackConnected(true) + setOrigin(SiteModel.ORIGIN_WPCOM_REST) + } + mockDiscoverySuccess( + siteUrl = jetpackSite.url, + hasEditorSettings = true, + hasEditorAssets = true + ) + whenever(themeRepository.fetchCurrentTheme(jetpackSite)) + .thenReturn(buildTheme(isBlockTheme = false)) + + val result = + repository.fetchEditorCapabilitiesForSite(jetpackSite) + + assertThat(result).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(jetpackSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(jetpackSite, true) + // The proxy is only for minting — capability detection goes direct. + verify(wpApiClientProvider, never()).getWpApiClient(jetpackSite) + } + @Test fun `atomic site returns false when discovery fails`() = runTest { diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt new file mode 100644 index 000000000000..d0a1701f05ac --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/repositories/SiteProvisioningSourceTest.kt @@ -0,0 +1,789 @@ +package org.wordpress.android.repositories + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.StandardTestDispatcher +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.any +import org.mockito.kotlin.atLeast +import org.mockito.kotlin.atMost +import org.mockito.kotlin.eq +import org.mockito.kotlin.mockingDetails +import org.mockito.kotlin.never +import org.mockito.kotlin.times +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError +import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType +import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordCredentials +import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler +import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated +import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.ui.accounts.login.ApplicationPasswordReauthNotifier +import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.ui.accounts.login.SiteXmlRpcUrlRecoverer +import org.wordpress.android.ui.accounts.login.XmlRpcRecovery +import org.wordpress.android.ui.mysite.cards.applicationpassword.ApplicationPasswordValidator +import org.wordpress.android.util.NetworkUtilsWrapper + +private const val TEST_SITE_LOCAL_ID = 7 + +@ExperimentalCoroutinesApi +class SiteProvisioningSourceTest : BaseUnitTest(StandardTestDispatcher()) { + @Mock lateinit var siteStore: SiteStore + @Mock lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper + @Mock lateinit var applicationPasswordValidator: ApplicationPasswordValidator + @Mock lateinit var wpApiClientProvider: WpApiClientProvider + @Mock lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer + @Mock lateinit var siteXmlRpcUrlRecoverer: SiteXmlRpcUrlRecoverer + @Mock lateinit var editorSettingsRepository: EditorSettingsRepository + @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper + @Mock lateinit var appLogWrapper: AppLogWrapper + @Mock lateinit var wpAppNotifierHandler: WpAppNotifierHandler + @Mock lateinit var applicationPasswordReauthNotifier: ApplicationPasswordReauthNotifier + + private lateinit var site: SiteModel + private lateinit var source: SiteProvisioningSource + + @Before + fun setUp() { + site = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://test.example.com" + // Non-null REST root + XML-RPC url so both recovery branches short-circuit unless a + // test clears them — keeping the auth/capability tests focused. + wpApiRestUrl = "https://test.example.com/wp-json" + xmlRpcUrl = "https://test.example.com/xmlrpc.php" + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(site) + source = SiteProvisioningSource( + siteStore, + applicationPasswordLoginHelper, + applicationPasswordValidator, + wpApiClientProvider, + siteApiRestUrlRecoverer, + siteXmlRpcUrlRecoverer, + editorSettingsRepository, + networkUtilsWrapper, + appLogWrapper, + wpAppNotifierHandler, + applicationPasswordReauthNotifier, + testScope(), + ) + } + + private fun stubHasStoredCredentials(value: Boolean) = + whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(!value) + + private suspend fun stubValidate(outcome: ApplicationPasswordValidator.Outcome) = + whenever(applicationPasswordValidator.validate(any())).thenReturn(outcome) + + /** + * The real validator talks through `getApplicationPasswordClient`, whose notifier reports a 401 + * back to the source — so a revoked credential fires onRequestedWithInvalidAuthentication during + * the very heal that is fixing it. Plain `thenReturn` stubs hide that feedback entirely. + */ + private suspend fun stubValidateFiringNotifierOn401(vararg outcomes: ApplicationPasswordValidator.Outcome) { + var call = 0 + whenever(applicationPasswordValidator.validate(any())).thenAnswer { + val outcome = outcomes[minOf(call, outcomes.lastIndex)] + call++ + if (outcome == ApplicationPasswordValidator.Outcome.Invalid) { + source.onRequestedWithInvalidAuthentication(site) + } + outcome + } + } + + private suspend fun stubMintSuccess() = + whenever(siteStore.createApplicationPassword(any())).thenReturn( + OnApplicationPasswordCreated(site, ApplicationPasswordCredentials("user", "pass", uuid = "u")) + ) + + /** A listener is registered (an activity is in the foreground), so prompts are delivered. */ + private fun stubReauthListenerPresent() = + whenever(applicationPasswordReauthNotifier.notifyReauthRequired(any())).thenReturn(true) + + private suspend fun stubMintFailure() = + whenever(siteStore.createApplicationPassword(any())).thenReturn( + OnApplicationPasswordCreated(site, BaseNetworkError(GenericErrorType.UNKNOWN, "fail"), notSupported = false) + ) + + private suspend fun stubCapabilityProbe(ok: Boolean, cached: Boolean = false) { + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(any())).thenReturn(ok) + // `ok || hasCache` short-circuits, so only stub the cache when the live probe failed. + if (!ok) whenever(editorSettingsRepository.hasCachedCapabilities(any())).thenReturn(cached) + } + + // region auth stage + + @Test + fun `given valid stored credentials, then provisioned and ready`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + verify(siteStore, never()).createApplicationPassword(any()) + } + + @Test + fun `given no credentials, then mints and is ready`() = test { + stubHasStoredCredentials(false) + stubMintSuccess() + stubCapabilityProbe(ok = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + verify(siteStore).createApplicationPassword(any()) + } + + @Test + fun `given mint fails with no prior credentials, then needs first-time auth`() = test { + stubHasStoredCredentials(false) + stubMintFailure() + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) + } + + @Test + fun `given stored credentials that fail to mint, then needs re-auth`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + verify(siteStore).deleteStoredApplicationPasswordCredentials(any()) + } + + @Test + fun `given a transient validation error while offline, then provisioning and no mint or probe`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.NetworkUnavailable) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Provisioning)) + verify(siteStore, never()).createApplicationPassword(any()) + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) + } + + @Test + fun `given the site is gone from the store, then unprovisionable`() = test { + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(null) + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + } + + @Test + fun `given a WPCom Simple site, then it detects capabilities without minting`() = test { + val simple = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://simple.wordpress.com" + setIsWPCom(true) // isWPComSimpleSite = isWPCom && !isWPComAtomic + wpApiRestUrl = "https://simple.wordpress.com/wp-json" + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(simple) + stubCapabilityProbe(ok = true) + + assertThat(source.await(simple)).isEqualTo(SiteReadiness.Ready) + // No application password applies — it must not validate or mint, just probe via the proxy. + verify(applicationPasswordValidator, never()).validate(any()) + verify(siteStore, never()).createApplicationPassword(any()) + } + + // endregion + + // region recovery + capability stages + + @Test + fun `given provisioned with a missing REST root, then it recovers and persists the url`() = test { + site.wpApiRestUrl = "" + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(site.url)) + .thenReturn("https://test.example.com/custom-rest") + stubCapabilityProbe(ok = true) + + source.await(site) + + verify(siteApiRestUrlRecoverer).persistApiRootUrl(eq(site.id), eq("https://test.example.com/custom-rest")) + } + + @Test + fun `given a provisioned self-hosted site without XML-RPC, then it recovers and persists xmlRpcUrl`() = test { + val selfHosted = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://selfhosted.example.com" + wpApiRestUrl = "https://selfhosted.example.com/wp-json" // REST branch short-circuits + // not WP.com and no xmlRpcUrl → the XML-RPC branch runs + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(selfHosted) + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + whenever(siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(selfHosted)) + .thenReturn(XmlRpcRecovery.Recovered("https://selfhosted.example.com/xmlrpc.php")) + + source.await(selfHosted) + + verify(siteXmlRpcUrlRecoverer) + .persistXmlRpcUrl(eq(TEST_SITE_LOCAL_ID), eq("https://selfhosted.example.com/xmlrpc.php")) + assertThat(source.isXmlRpcUnavailable(TEST_SITE_LOCAL_ID)).isFalse + } + + @Test + fun `given the XML-RPC column write throws, then a successful probe still settles Ready`() = test { + // Recovery is best effort and ends in an unguarded WellSql update. Left to propagate, that + // throw cancels the sibling capability probe through the enclosing coroutineScope and the + // whole site settles Unreachable — discarding a probe that already succeeded (#22944 review). + val selfHosted = selfHostedWithoutXmlRpc() + whenever(siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(selfHosted)) + .thenReturn(XmlRpcRecovery.Recovered("https://selfhosted.example.com/xmlrpc.php")) + whenever(siteXmlRpcUrlRecoverer.persistXmlRpcUrl(any(), any())) + .thenThrow(RuntimeException("database is locked")) + + assertThat(source.await(selfHosted)).isEqualTo(SiteReadiness.Ready) + } + + @Test + fun `given the REST root write throws, then a successful probe still settles Ready`() = test { + // Same hole on the other branch, where the write runs before detectCapabilities. + val site = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://test.example.com" + xmlRpcUrl = "https://test.example.com/xmlrpc.php" // XML-RPC branch short-circuits + // no wpApiRestUrl -> the REST recovery branch runs + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(site) + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(any())) + .thenReturn("https://test.example.com/custom-rest") + whenever(siteApiRestUrlRecoverer.persistApiRootUrl(any(), any())) + .thenThrow(RuntimeException("database is locked")) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + } + + @Test + fun `given XML-RPC recovery reaches a definitive negative, then the site is flagged unavailable`() = test { + val selfHosted = selfHostedWithoutXmlRpc() + whenever(siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(selfHosted)) + .thenReturn(XmlRpcRecovery.Unavailable) + + source.await(selfHosted) + + verify(siteXmlRpcUrlRecoverer, never()).persistXmlRpcUrl(any(), any()) + assertThat(source.isXmlRpcUnavailable(TEST_SITE_LOCAL_ID)).isTrue + } + + @Test + fun `given XML-RPC recovery is inconclusive, then the site is not flagged unavailable`() = test { + // A transient discovery failure (e.g. a 429) must not license the XML-RPC-disabled card. + val selfHosted = selfHostedWithoutXmlRpc() + whenever(siteXmlRpcUrlRecoverer.discoverAndVerifyXmlRpcUrl(selfHosted)) + .thenReturn(XmlRpcRecovery.Inconclusive) + + source.await(selfHosted) + + verify(siteXmlRpcUrlRecoverer, never()).persistXmlRpcUrl(any(), any()) + assertThat(source.isXmlRpcUnavailable(TEST_SITE_LOCAL_ID)).isFalse + } + + private suspend fun selfHostedWithoutXmlRpc(): SiteModel { + val selfHosted = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://selfhosted.example.com" + wpApiRestUrl = "https://selfhosted.example.com/wp-json" // REST branch short-circuits + // not WP.com and no xmlRpcUrl → the XML-RPC branch runs + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(selfHosted) + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + return selfHosted + } + + @Test + fun `given probe fails while offline, then transient error`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.TransientError) + } + + @Test + fun `given probe fails while online, then unreachable`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Unreachable) + } + + @Test + fun `given probe fails but capabilities are cached, then ready`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false, cached = true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Ready) + } + + // endregion + + // region single-flight / dedup / invalidate / clear + + @Test + fun `given a prior ready run, when awaited again, then it does not re-run`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.await(site) + + verify(applicationPasswordValidator, times(1)).validate(any()) + } + + @Test + fun `given a prior cache-only ready run, when awaited again, then it re-probes`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + // Ready served from stale cache (live probe failing) must NOT latch the dedup gate, so the + // next access re-probes and can refresh / surface a genuine failure (#22944 c3). + stubCapabilityProbe(ok = false, cached = true) + + source.await(site) + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + @Test + fun `given a prior ready run, when invalidated, then it re-runs`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.invalidate(site) + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + @Test + fun `given a ready site, when cleared, then the next run re-runs`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.clear() + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + // endregion + + // region invalid-auth re-provisioning + + @Test + fun `registers as an invalid-auth listener on construction`() { + verify(wpAppNotifierHandler).addListener(source) + } + + @Test + fun `given a ready site, when auth is reported invalid, then it re-runs`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.await(site) + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + @Test + fun `given a 401-triggered heal succeeds, then no reauth is requested`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) // creds revoked -> wiped + stubMintSuccess() // re-mint heals silently (WP.com-connected) + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + + verify(siteStore).createApplicationPassword(any()) + verify(applicationPasswordReauthNotifier, never()).notifyReauthRequired(any()) + } + + @Test + fun `given a 401-triggered heal fails, then reauth is requested`() = test { + stubReauthListenerPresent() + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + + verify(applicationPasswordReauthNotifier).notifyReauthRequired(site.url) + } + + @Test + fun `given the reauth notifier throws, then the pipeline still settles off probing`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + // The post-settle reauth escalation runs on the non-supervisor appScope; a throw here must be + // contained, not allowed to cancel the scope and wedge provisioning for every site (#22944 c1). + whenever(applicationPasswordReauthNotifier.notifyReauthRequired(any())) + .thenThrow(RuntimeException("boom")) + + source.onRequestedWithInvalidAuthentication(site) + val result = source.await(site) + + assertThat(result) + .isEqualTo(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + } + + @Test + fun `given a 401 during an active run, the deferred heal still escalates a dead credential`() = test { + stubReauthListenerPresent() + stubHasStoredCredentials(true) + // The active run validates the not-yet-revoked credential (Valid -> Ready); the deferred re-heal + // then sees it revoked (Invalid) and can't re-mint, so it must escalate rather than be swallowed + // by the run that was already in flight when the 401 arrived (#22944 c2). + whenever(applicationPasswordValidator.validate(any())) + .thenReturn(ApplicationPasswordValidator.Outcome.Valid) + .thenReturn(ApplicationPasswordValidator.Outcome.Invalid) + stubCapabilityProbe(ok = true) + stubMintFailure() + + source.stateFor(site) // launch the run; it stays active (not yet run) + source.onRequestedWithInvalidAuthentication(site) // 401 while the run is active -> defer the heal + testScheduler.advanceUntilIdle() // run the active run, then the deferred heal + + verify(applicationPasswordReauthNotifier).notifyReauthRequired(site.url) + } + + @Test + fun `given no listener for the re-auth prompt, then the episode is not consumed`() = test { + // Activities register their listener in onResume, so a heal settling while the app is + // backgrounded has nobody to tell. Recording it as prompted would burn the single + // per-episode escalation on an empty listener map (#22944 review). + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + whenever(applicationPasswordReauthNotifier.notifyReauthRequired(any())).thenReturn(false) + + source.onRequestedWithInvalidAuthentication(site) // heal settles while nothing is listening + source.await(site) + + // The app comes back to the foreground: an activity registers, and the onResume run retries + // the prompt even though it is a routine run. + stubReauthListenerPresent() + source.invalidate(site) + testScheduler.advanceUntilIdle() + + verify(applicationPasswordReauthNotifier, atLeast(2)).notifyReauthRequired(site.url) + } + + @Test + fun `given the pipeline throws while offline, then transient rather than unreachable`() = test { + // The connectivity banner would otherwise stack on the global no-connection banner. + stubHasStoredCredentials(true) + whenever(applicationPasswordValidator.validate(any())).thenThrow(RuntimeException("boom")) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.TransientError) + } + + @Test + fun `given the pipeline throws while online, then unreachable`() = test { + stubHasStoredCredentials(true) + whenever(applicationPasswordValidator.validate(any())).thenThrow(RuntimeException("boom")) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + assertThat(source.await(site)).isEqualTo(SiteReadiness.Unreachable) + } + + @Test + fun `given a 401 on a WPCom Simple site, then it is ignored`() = test { + val simple = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://simple.wordpress.com" + setIsWPCom(true) // isWPComSimpleSite = isWPCom && !isWPComAtomic -> bearer-only + } + + source.onRequestedWithInvalidAuthentication(simple) + + verify(applicationPasswordValidator, never()).validate(any()) + verify(applicationPasswordReauthNotifier, never()).notifyReauthRequired(any()) + } + + @Test + fun `given an already-unprovisionable site, when 401 arrives, then it does not re-run`() = test { + stubHasStoredCredentials(false) + stubMintFailure() + + source.await(site) // settles NeedsAuth(Unprovisionable) + source.onRequestedWithInvalidAuthentication(site) + + verify(siteStore, times(1)).createApplicationPassword(any()) + } + + @Test + fun `given a routine run settles unprovisionable, then no reauth is requested`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + + source.await(site) // not 401-triggered, so the failure must not pop re-auth + + verify(applicationPasswordReauthNotifier, never()).notifyReauthRequired(any()) + } + + @Test + fun `given the site is unreachable while online, then unreachable rather than provisioning`() = test { + // The validator collapses DNS / timeout / refused / 5xx into NetworkUnavailable so it never + // wipes credentials on a guess. With the device online that means the *site* is down, which is + // the connectivity banner's case — reporting Provisioning here hides the banner entirely and + // loses the one state it was written for (#22944 c2). + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.NetworkUnavailable) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + val result = source.await(site) + + assertThat(result).isEqualTo(SiteReadiness.Unreachable) + verify(siteStore, never()).createApplicationPassword(any()) + verify(editorSettingsRepository, never()).fetchEditorCapabilitiesForSite(any()) + } + + @Test + fun `given the pipeline's own request 401s every run, then healing stops instead of looping`() = test { + // The WP.com bearer client used for capability detection raises the same invalid-auth + // notification as a revoked application password. With a rejected bearer token the app password + // validates fine every run, so an unguarded heal relaunches the pipeline forever (#22944 c1). + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(any())).thenAnswer { + source.onRequestedWithInvalidAuthentication(site) + true + } + + source.await(site) + testScheduler.advanceUntilIdle() + + // One routine run plus exactly one heal; the heal proved re-provisioning changes nothing. + verify(applicationPasswordValidator, times(2)).validate(any()) + verify(siteStore, never()).createApplicationPassword(any()) + } + + @Test + fun `given validation's own 401 during a successful heal, then no redundant re-heal and healing stays enabled`() = + test { + // The heal's validate sees the revoked credential (Invalid) and its notifier fires mid-run; + // the mint then succeeds. The deferred heal must notice the run already re-minted and stand + // down, or it re-validates the fresh password, records "changed nothing", and permanently + // disables healing for the site (#22944 review c2). + val selfHosted = SiteModel().apply { + id = TEST_SITE_LOCAL_ID + url = "https://test.example.com" + wpApiRestUrl = "https://test.example.com/wp-json" + xmlRpcUrl = "https://test.example.com/xmlrpc.php" + } + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)).thenReturn(selfHosted) + stubHasStoredCredentials(true) + stubValidateFiringNotifierOn401(ApplicationPasswordValidator.Outcome.Invalid) + stubMintSuccess() + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + testScheduler.advanceUntilIdle() + + // Exactly one heal: the deferred one stood down because the run had already re-minted. + verify(siteStore, times(1)).createApplicationPassword(any()) + verify(applicationPasswordReauthNotifier, never()).notifyReauthRequired(any()) + } + + @Test + fun `given a mint that never sticks, then heals stop after the budget is spent`() = test { + // A host that mints happily but never accepts the result would otherwise re-mint on every + // 401 forever, accumulating application passwords on the user's account (#22944 review c1). + stubHasStoredCredentials(true) + stubValidateFiringNotifierOn401(ApplicationPasswordValidator.Outcome.Invalid) + stubMintSuccess() + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + repeat(5) { + source.onRequestedWithInvalidAuthentication(site) + testScheduler.advanceUntilIdle() + } + + // Bounded by MAX_CONSECUTIVE_HEALS rather than growing with the number of 401s. + verify(siteStore, atMost(2)).createApplicationPassword(any()) + } + + @Test + fun `given an explicit retry while a run is active, then the heal budget is still cleared`() = test { + // MySiteViewModel.refresh builds the application-password card first, which starts a run + // synchronously on Main.immediate — so invalidate lands mid-run for exactly the sites that + // need clearing. The clears must sit above the in-flight guard (#22944 review c4). + // + // The site must settle Unreachable, not Ready: Ready latches (so stateFor wouldn't start a + // run at all) and also resets the budget on its own, either of which hides the bug. + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = false, cached = false) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + source.onRequestedWithInvalidAuthentication(site) // heal #1: ConfirmedUnchanged -> budget spent + source.await(site) + assertThat(source.await(site)).isEqualTo(SiteReadiness.Unreachable) // never latches + source.stateFor(site) // a run is now in flight, as after buildCard + source.invalidate(site) // pull-to-refresh lands mid-run + testScheduler.advanceUntilIdle() + + val validationsBeforeRetry = mockingDetails(applicationPasswordValidator).invocations + .count { it.method.name == "validate" } + + source.onRequestedWithInvalidAuthentication(site) // must be honoured again + testScheduler.advanceUntilIdle() + + val validationsAfterRetry = mockingDetails(applicationPasswordValidator).invocations + .count { it.method.name == "validate" } + assertThat(validationsAfterRetry) + .describedAs("the 401 after an explicit retry should have started a fresh heal") + .isGreaterThan(validationsBeforeRetry) + } + + @Test + fun `given a heal changed no credentials, then later 401s are ignored`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) // heal #1: validates Valid, changes nothing + source.await(site) + source.onRequestedWithInvalidAuthentication(site) // must be dropped, not healed again + testScheduler.advanceUntilIdle() + + verify(applicationPasswordValidator, times(1)).validate(any()) + } + + @Test + fun `given an explicit retry after a futile heal, then 401s are honoured again`() = test { + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Valid) + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + source.invalidate(site) // pull-to-refresh clears the futile verdict + testScheduler.advanceUntilIdle() + source.onRequestedWithInvalidAuthentication(site) + testScheduler.advanceUntilIdle() + + // Futile heal, then the invalidate run, then the re-enabled heal. + verify(applicationPasswordValidator, times(3)).validate(any()) + } + + @Test + fun `given a heal re-minted the credentials, then later 401s still heal`() = test { + // A heal that actually replaced the password did something, so it must not be recorded as + // futile — otherwise one genuine revocation permanently disables healing for the site. + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintSuccess() + stubCapabilityProbe(ok = true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + source.onRequestedWithInvalidAuthentication(site) + testScheduler.advanceUntilIdle() + + verify(siteStore, times(2)).createApplicationPassword(any()) + } + + @Test + fun `given an unreachable site during a heal, then the heal is not recorded as futile`() = test { + // The heal never got far enough to confirm the credentials, so it's inconclusive rather than + // proof that re-provisioning can't help. + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.NetworkUnavailable) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + + source.onRequestedWithInvalidAuthentication(site) + source.await(site) + source.onRequestedWithInvalidAuthentication(site) + testScheduler.advanceUntilIdle() + + verify(applicationPasswordValidator, times(2)).validate(any()) + } + + @Test + fun `given a routine run settles unprovisionable with a 401 pending, then it escalates once`() = test { + // A routine run never arms the heal flag, so its own tail skips escalation. The deferred handler + // used to return "already escalated" on that outcome and swallow the 401 entirely, leaving no + // interactive re-auth and dropping every later 401 via the Unprovisionable guard (#22944 c3). + stubReauthListenerPresent() + stubHasStoredCredentials(true) + stubValidate(ApplicationPasswordValidator.Outcome.Invalid) + stubMintFailure() + + source.stateFor(site) // routine run, still active + source.onRequestedWithInvalidAuthentication(site) // 401 mid-run -> deferred + testScheduler.advanceUntilIdle() + + verify(applicationPasswordReauthNotifier, times(1)).notifyReauthRequired(site.url) + // The run already made a terminal mint attempt; relaunching would only re-fail it. + verify(siteStore, times(1)).createApplicationPassword(any()) + } + + // endregion + + // region failure containment + + @Test + fun `given a stage throws, when awaited, then it settles off Probing`() = test { + // An unhandled throw inside the pipeline (here getSiteByLocalId failing like a SQLiteException) + // must not escape launchPipeline's appScope.launch: if it does, flow.value is never assigned, so + // every consumer is wedged on Probing and the shared appScope is cancelled. The run has to turn + // an unexpected failure into a terminal readiness instead. + whenever(siteStore.getSiteByLocalId(TEST_SITE_LOCAL_ID)) + .thenThrow(RuntimeException("DB read failed")) + + assertThat(source.await(site)).isNotEqualTo(SiteReadiness.Probing) + } + + // endregion +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt index a8faeabc7868..c10302d26f1d 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt @@ -77,9 +77,6 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { @Mock lateinit var wpApiClientProvider: WpApiClientProvider - @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier - private lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper @Before @@ -96,8 +93,7 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { apiRootUrlCache, discoverSuccessWrapper, crashLogging, - wpApiClientProvider, - credentialsChangedNotifier + wpApiClientProvider ) } @@ -230,7 +226,6 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() { verify(siteStore).sites verify(dispatcherWrapper).updateApplicationPassword(eq(siteModel)) verify(wpApiClientProvider).clearSelfHostedClient(eq(siteModel.id)) - verify(credentialsChangedNotifier).notifyChanged(eq(siteModel.id)) } @Test diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifierTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifierTest.kt new file mode 100644 index 000000000000..b7fd05dcbcfe --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordReauthNotifierTest.kt @@ -0,0 +1,33 @@ +package org.wordpress.android.ui.accounts.login + +import org.junit.Test +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify + +private const val SITE_URL = "https://selfhosted.example.com" + +class ApplicationPasswordReauthNotifierTest { + private val notifier = ApplicationPasswordReauthNotifier() + + @Test + fun `notifies a registered listener`() { + val listener = mock() + notifier.addListener(listener) + + notifier.notifyReauthRequired(SITE_URL) + + verify(listener).onReauthRequired(SITE_URL) + } + + @Test + fun `does not notify a removed listener`() { + val listener = mock() + notifier.addListener(listener) + notifier.removeListener(listener) + + notifier.notifyReauthRequired(SITE_URL) + + verify(listener, never()).onReauthRequired(SITE_URL) + } +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt new file mode 100644 index 000000000000..057a4f9be830 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/SiteXmlRpcUrlRecovererTest.kt @@ -0,0 +1,133 @@ +package org.wordpress.android.ui.accounts.login + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.junit.MockitoJUnitRunner +import org.mockito.kotlin.any +import org.mockito.kotlin.eq +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.model.SitesModel +import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError +import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType +import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder +import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient +import org.wordpress.android.fluxc.persistence.SiteSqlUtils +import org.wordpress.android.fluxc.utils.AppLogWrapper + +private const val ENDPOINT = "https://selfhosted.example.com/xmlrpc.php" +private const val SITE_LOCAL_ID = 5 + +@ExperimentalCoroutinesApi +@RunWith(MockitoJUnitRunner::class) +class SiteXmlRpcUrlRecovererTest : BaseUnitTest() { + @Mock lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder + @Mock lateinit var siteXMLRPCClient: SiteXMLRPCClient + @Mock lateinit var siteSqlUtils: SiteSqlUtils + @Mock lateinit var appLogWrapper: AppLogWrapper + + private lateinit var site: SiteModel + private lateinit var recoverer: SiteXmlRpcUrlRecoverer + + @Before + fun setUp() { + site = SiteModel().apply { + id = SITE_LOCAL_ID + url = "https://selfhosted.example.com" + apiRestUsernamePlain = "user" + apiRestPasswordPlain = "pass" + } + recoverer = SiteXmlRpcUrlRecoverer( + selfHostedEndpointFinder, + siteXMLRPCClient, + siteSqlUtils, + appLogWrapper, + testDispatcher(), + ) + } + + @Test + fun `given discovery and authenticated verify succeed, then returns the endpoint`() = test { + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) + // The site's stored credentials are forwarded to the authenticated verify call. + whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), eq("user"), eq("pass"), any())) + .thenReturn(SitesModel(listOf(SiteModel()))) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)) + .isEqualTo(XmlRpcRecovery.Recovered(ENDPOINT)) + } + + @Test + fun `given discovery fails with a definitive negative, then returns Unavailable`() = test { + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(discoveryException(SelfHostedEndpointFinder.DiscoveryError.XMLRPC_BLOCKED)) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(XmlRpcRecovery.Unavailable) + } + + @Test + fun `given discovery fails transiently with rate limiting, then returns Inconclusive`() = test { + // A 429 says nothing about whether XML-RPC is enabled — reporting Unavailable here would + // surface a false "XML-RPC Disabled" warning on a throttled site. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(discoveryException(SelfHostedEndpointFinder.DiscoveryError.RATE_LIMITED)) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(XmlRpcRecovery.Inconclusive) + } + + @Test + fun `given discovery fails for an unrelated reason, then returns Inconclusive`() = test { + // HTTP auth / SSL / invalid-URL failures are about reaching the site at all, not about XML-RPC. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(discoveryException(SelfHostedEndpointFinder.DiscoveryError.HTTP_AUTH_REQUIRED)) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(XmlRpcRecovery.Inconclusive) + } + + @Test + fun `given discovery throws an unexpected exception, then returns Inconclusive`() = test { + // A non-DiscoveryException (e.g. a RuntimeException from the network/parse path) must be + // contained, not propagated — otherwise it cancels the whole provisioning pipeline. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)) + .thenThrow(RuntimeException("unexpected")) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(XmlRpcRecovery.Inconclusive) + } + + @Test + fun `given the authenticated verify errors, then returns Inconclusive`() = test { + // Discovery already proved xmlrpc.php works, so a failed verify is a credential/transport + // problem — never a reason to claim XML-RPC is off. + whenever(selfHostedEndpointFinder.verifyOrDiscoverXMLRPCEndpoint(site.url)).thenReturn(ENDPOINT) + whenever(siteXMLRPCClient.fetchSites(eq(ENDPOINT), any(), any(), any())) + .thenReturn(SitesModel().apply { error = BaseNetworkError(GenericErrorType.UNKNOWN, "x") }) + + assertThat(recoverer.discoverAndVerifyXmlRpcUrl(site)).isEqualTo(XmlRpcRecovery.Inconclusive) + } + + @Test + fun `given persist updates a row, then returns true`() = test { + whenever(siteSqlUtils.updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT))).thenReturn(1) + + assertThat(recoverer.persistXmlRpcUrl(SITE_LOCAL_ID, ENDPOINT)).isTrue + verify(siteSqlUtils).updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT)) + } + + @Test + fun `given persist matches no row, then returns false`() = test { + whenever(siteSqlUtils.updateXmlRpcUrl(eq(SITE_LOCAL_ID), eq(ENDPOINT))).thenReturn(0) + + assertThat(recoverer.persistXmlRpcUrl(SITE_LOCAL_ID, ENDPOINT)).isFalse + } + + // DiscoveryException exposes discoveryError as a public final field, so a Mockito mock can't + // carry one — construct the real exception. + private fun discoveryException(error: SelfHostedEndpointFinder.DiscoveryError) = + SelfHostedEndpointFinder.DiscoveryException(error, site.url) +} diff --git a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModelTest.kt index 07b1080def1e..92af0c7290eb 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/accounts/login/applicationpassword/LoginSiteApplicationPasswordViewModelTest.kt @@ -14,7 +14,9 @@ import org.mockito.junit.MockitoJUnitRunner import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest +import org.wordpress.android.R import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper +import org.wordpress.android.viewmodel.ResourceProvider import kotlin.test.assertEquals import kotlin.test.assertNull @@ -24,6 +26,9 @@ class LoginSiteApplicationPasswordViewModelTest : BaseUnitTest() { @Mock private lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper + @Mock + private lateinit var resourceProvider: ResourceProvider + private lateinit var viewModel: LoginSiteApplicationPasswordViewModel // Test dispatcher for coroutines @@ -32,7 +37,7 @@ class LoginSiteApplicationPasswordViewModelTest : BaseUnitTest() { @Before fun setUp() { MockitoAnnotations.openMocks(this) - viewModel = LoginSiteApplicationPasswordViewModel(applicationPasswordLoginHelper) + viewModel = LoginSiteApplicationPasswordViewModel(applicationPasswordLoginHelper, resourceProvider) } @Test @@ -93,10 +98,10 @@ class LoginSiteApplicationPasswordViewModelTest : BaseUnitTest() { } @Test - fun `Given discovery fails, when running discovery, then the generic error is shown`() = test { + fun `Given discovery fails, when running discovery, then the library message is shown as-is`() = test { // Given val siteUrl = "https://example.com" - val errorMessage = "not supported" + val errorMessage = "Found a site but failed to read its API configuration." whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(siteUrl)) .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed(errorMessage)) @@ -110,12 +115,59 @@ class LoginSiteApplicationPasswordViewModelTest : BaseUnitTest() { viewModel.runApiDiscovery(siteUrl) advanceUntilIdle() - // Then + // Then — the specific reason survives, and no URL is emitted for the UI to misread as a + // failure signal (that empty emission is what used to overwrite this message). assertEquals(errorMessage, viewModel.errorMessage.value) - assertEquals("", collectedUrl) + assertNull(collectedUrl) assertEquals(false, wpComDetected) wpComJob.cancel() discoveryJob.cancel() } + + @Test + fun `Given a private site, when running discovery, then the private-site explanation is shown`() = test { + // The library reports a private site as a generic "couldn't read the API configuration", + // which reads as though the site is broken. Name the actual cause instead. + val siteUrl = "https://example.com" + val privateSiteMessage = "This site is private, so we can't read its settings to sign you in." + whenever(resourceProvider.getString(R.string.application_password_private_site_error)) + .thenReturn(privateSiteMessage) + whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(siteUrl)) + .thenReturn( + ApplicationPasswordLoginHelper.DiscoveryResult.Failed( + userFacingMessage = "Found a site but failed to read its API configuration.", + reason = ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.PrivateSite, + ) + ) + + viewModel.runApiDiscovery(siteUrl) + advanceUntilIdle() + + assertEquals(privateSiteMessage, viewModel.errorMessage.value) + assertEquals(false, viewModel.loadingStateFlow.value) + } + + @Test + fun `Given a site without application passwords, then the not-supported message is shown`() = test { + // Discovery succeeded but advertised no application-passwords endpoint. Removing the + // fragment's blanket overwrite took this message's only producer with it, leaving the raw + // untranslated exception text on screen (#22944 review c3). + val siteUrl = "https://example.com" + val notSupportedMessage = "The provided site does not support Application Password authentication." + whenever(resourceProvider.getString(R.string.application_password_not_supported_error)) + .thenReturn(notSupportedMessage) + whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(siteUrl)) + .thenReturn( + ApplicationPasswordLoginHelper.DiscoveryResult.Failed( + userFacingMessage = "No application-passwords authentication URL advertised", + reason = ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.NotSupported, + ) + ) + + viewModel.runApiDiscovery(siteUrl) + advanceUntilIdle() + + assertEquals(notSupportedMessage, viewModel.errorMessage.value) + } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModelTest.kt index 8bb91e1a9a30..b48a9406b9c1 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/jetpackrestconnection/JetpackRestConnectionViewModelTest.kt @@ -20,7 +20,6 @@ import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.BuildConfig import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.WpAppNotifierHandler import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.fluxc.utils.AppLogWrapper import org.wordpress.android.ui.jetpackrestconnection.JetpackRestConnectionViewModel.ButtonType @@ -56,9 +55,6 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { @Mock lateinit var analyticsTrackerWrapper: AnalyticsTrackerWrapper - @Mock - lateinit var wpAppNotifierHandler: WpAppNotifierHandler - @Mock lateinit var siteModel: SiteModel @@ -94,7 +90,6 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { jetpackModuleHelper = jetpackModuleHelper, appLogWrapper = appLogWrapper, analyticsTrackerWrapper = analyticsTrackerWrapper, - wpAppNotifierHandler = wpAppNotifierHandler, ) // Override delays for faster tests @@ -147,7 +142,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { @Test fun `onRetryClick retries from failed step`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.failure(Exception("Failed"))) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.failure(Exception("Failed"))) viewModel.onStartClick() advanceTimeBy(TEST_ADVANCE_TIME_MS) @@ -155,7 +150,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { assertThat(viewModel.stepStates.value[ConnectionStep.InstallJetpack]?.status) .isEqualTo(ConnectionStatus.Failed) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) viewModel.onRetryClick() advanceTimeBy(TEST_ADVANCE_TIME_MS) // Need to advance time for retry to complete @@ -213,7 +208,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { @Test fun `installJetpack step succeeds with active plugin`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) viewModel.onStartClick() advanceTimeBy(TEST_ADVANCE_TIME_MS) @@ -225,7 +220,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { @Test fun `installJetpack step fails with inactive plugin`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.INACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.INACTIVE)) viewModel.onStartClick() advanceTimeBy(TEST_ADVANCE_TIME_MS) @@ -237,7 +232,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { @Test fun `connectSite step succeeds and updates site ID`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) viewModel.onStartClick() @@ -253,7 +248,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { whenever(accountStore.hasAccessToken()) .thenReturn(true) // Initial check for LoginWpCom .thenReturn(false) // Check in ConnectUser step - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) viewModel.onStartClick() @@ -266,7 +261,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { fun `connectUser step succeeds with access token`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) whenever(accountStore.accessToken).thenReturn(TEST_ACCESS_TOKEN) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) whenever(jetpackConnector.connectUser(any(), any())).thenReturn(Result.success(TEST_USER_ID)) @@ -282,7 +277,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { fun `finalize step succeeds and completes step`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) whenever(accountStore.accessToken).thenReturn(TEST_ACCESS_TOKEN) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) whenever(jetpackConnector.connectUser(any(), any())).thenReturn(Result.success(TEST_USER_ID)) whenever(jetpackModuleHelper.activateStatsModule(any())).thenReturn(Result.success(Unit)) @@ -297,7 +292,7 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { fun `finalize step fails on exception`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) whenever(accountStore.accessToken).thenReturn(TEST_ACCESS_TOKEN) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) whenever(jetpackConnector.connectUser(any(), any())).thenReturn(Result.success(TEST_USER_ID)) whenever(jetpackModuleHelper.activateStatsModule(any())).thenReturn(Result.failure(Exception("Stats failed"))) @@ -313,18 +308,29 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { } @Test - fun `onRequestedWithInvalidAuthentication resets and restarts flow`() = runTest { - viewModel.onRequestedWithInvalidAuthentication("https://example.com") + fun `install auth failure resets token and restarts the flow`() = runTest { + whenever(accountStore.hasAccessToken()) + .thenReturn(true) // initial LoginWpCom completes so the install step runs + .thenReturn(false) // after the reset, the restarted flow waits at the login step + // The install client signals invalid auth locally via the onInvalidAuth callback (2nd arg), + // not the app-wide WpAppNotifierHandler. Simulate that callback firing on a 401 (see #22944). + whenever(jetpackInstaller.installJetpack(any(), any())).doSuspendableAnswer { + it.getArgument<() -> Unit>(1).invoke() + Result.failure(Exception("Invalid credentials")) + } + + viewModel.onStartClick() + advanceTimeBy(TEST_ADVANCE_TIME_MS) + advanceUntilIdle() - verify(wpAppNotifierHandler).removeListener(viewModel) verify(accountStore).resetAccessToken() - assertThat(viewModel.currentStep.value).isEqualTo(ConnectionStep.LoginWpCom) + assertThat(viewModel.uiEvent.value).isEqualTo(UiEvent.StartWPComLogin) } @Test fun `step timeout triggers timeout error`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).doSuspendableAnswer { + whenever(jetpackInstaller.installJetpack(any(), any())).doSuspendableAnswer { delay(TEST_STEP_TIMEOUT_MS + 10L) // Longer than TEST_STEP_TIMEOUT_MS to trigger timeout Result.success(PluginStatus.ACTIVE) } @@ -410,10 +416,10 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { } @Test - fun `successful flow completion sets Done button and removes listener`() = runTest { + fun `successful flow completion sets Done button`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) whenever(accountStore.accessToken).thenReturn(TEST_ACCESS_TOKEN) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.success(PluginStatus.ACTIVE)) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.success(PluginStatus.ACTIVE)) whenever(jetpackConnector.connectSite(any())).thenReturn(Result.success(TEST_SITE_ID)) whenever(jetpackConnector.connectUser(any(), any())).thenReturn(Result.success(TEST_USER_ID)) whenever(jetpackModuleHelper.activateStatsModule(any())).thenReturn(Result.success(Unit)) @@ -422,13 +428,12 @@ class JetpackRestConnectionViewModelTest : BaseUnitTest() { advanceTimeBy(TEST_ADVANCE_TIME_MS) assertThat(viewModel.buttonType.value).isEqualTo(ButtonType.Done) - verify(wpAppNotifierHandler).removeListener(viewModel) } @Test fun `failed flow completion sets Retry button`() = runTest { whenever(accountStore.hasAccessToken()).thenReturn(true) - whenever(jetpackInstaller.installJetpack(any())).thenReturn(Result.failure(Exception("Failed"))) + whenever(jetpackInstaller.installJetpack(any(), any())).thenReturn(Result.failure(Exception("Failed"))) viewModel.onStartClick() advanceTimeBy(TEST_ADVANCE_TIME_MS) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt index e6e2cad75168..522760372190 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt @@ -1,9 +1,9 @@ package org.wordpress.android.ui.mysite.cards.applicationpassword import junit.framework.TestCase.assertNull -import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.flow.MutableStateFlow +import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test import org.junit.runner.RunWith @@ -11,466 +11,176 @@ import org.mockito.Mock import org.mockito.MockitoAnnotations import org.mockito.junit.MockitoJUnitRunner import org.mockito.kotlin.any -import org.mockito.kotlin.doSuspendableAnswer import org.mockito.kotlin.eq import org.mockito.kotlin.never -import org.mockito.kotlin.times import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest -import org.mockito.kotlin.mock import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.model.SitesModel -import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError -import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType -import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder -import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient -import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordCredentials -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider import org.wordpress.android.fluxc.store.SiteStore -import org.wordpress.android.fluxc.store.SiteStore.OnApplicationPasswordCreated import org.wordpress.android.fluxc.utils.AppLogWrapper +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer import org.wordpress.android.ui.mysite.MySiteCardAndItem -import kotlin.test.assertNotNull private const val TEST_URL = "https://www.test.com" -private const val TEST_SITE_NAME = "My Site" private const val TEST_SITE_ID = 1 -private const val TEST_SITE_ICON = "http://site.com/icon.jpg" -private const val TEST_URL_AUTH = "https://www.test.com/auth" -private const val TEST_URL_AUTH_SUFFIX = "?app_name=android-jetpack-client&success_url=callback://callback" +private const val TEST_AUTH_URL = "https://www.test.com/auth" @ExperimentalCoroutinesApi @RunWith(MockitoJUnitRunner::class) class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { - @Mock - lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper - - @Mock - lateinit var siteStore: SiteStore - - @Mock - lateinit var appLogWrapper: AppLogWrapper - - @Mock - lateinit var wpApiClientProvider: WpApiClientProvider - - @Mock - lateinit var applicationPasswordValidator: ApplicationPasswordValidator - - @Mock - lateinit var selfHostedEndpointFinder: SelfHostedEndpointFinder - - @Mock - lateinit var siteXMLRPCClient: SiteXMLRPCClient - - @Mock - lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer - - @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier + @Mock lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper + @Mock lateinit var siteStore: SiteStore + @Mock lateinit var appLogWrapper: AppLogWrapper + @Mock lateinit var siteProvisioningSource: SiteProvisioningSource private lateinit var siteTest: SiteModel - - private var applicationPasswordCard: MySiteCardAndItem? = null - - private lateinit var applicationPasswordViewModelSlice: ApplicationPasswordViewModelSlice + private var card: MySiteCardAndItem? = null + private lateinit var slice: ApplicationPasswordViewModelSlice @Before fun setUp() { MockitoAnnotations.openMocks(this) - - applicationPasswordViewModelSlice = ApplicationPasswordViewModelSlice( + slice = ApplicationPasswordViewModelSlice( applicationPasswordLoginHelper, siteStore, appLogWrapper, - wpApiClientProvider, - applicationPasswordValidator, - selfHostedEndpointFinder, - siteXMLRPCClient, - siteApiRestUrlRecoverer, - credentialsChangedNotifier, - testDispatcher() - ).apply { - initialize(testScope()) - } + siteProvisioningSource, + ).apply { initialize(testScope()) } siteTest = SiteModel().apply { id = TEST_SITE_ID url = TEST_URL - name = TEST_SITE_NAME - iconUrl = TEST_SITE_ICON - siteId = TEST_SITE_ID.toLong() - apiRestUsernamePlain = "testuser" - apiRestPasswordPlain = "testpass" - // Mark xmlRpcUrl so handleValidAuth's XML-RPC-disabled fallback doesn't fire — that - // path is exercised by the dedicated xmlRpcRediscovery tests below. - xmlRpcUrl = "https://www.test.com/xmlrpc.php" - } - - applicationPasswordCard = null - applicationPasswordViewModelSlice.uiModel.observeForever { card -> - applicationPasswordCard = card } - - // By default, treat the site as having no stored credentials. Tests that exercise the - // validate-stored-creds path override this. - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(true) - } - - private suspend fun stubMintFailure(notSupported: Boolean = false) { - whenever(siteStore.createApplicationPassword(any())).thenReturn( - OnApplicationPasswordCreated( - siteTest, - BaseNetworkError(GenericErrorType.UNKNOWN, "fail"), - notSupported = notSupported, - ) - ) + card = null + slice.uiModel.observeForever { card = it } } - private suspend fun stubMintSuccess() { - whenever(siteStore.createApplicationPassword(any())).thenReturn( - OnApplicationPasswordCreated( - siteTest, - ApplicationPasswordCredentials("user", "pass", uuid = "u") - ) - ) + private fun stubReadiness(readiness: SiteReadiness): MutableStateFlow { + val flow = MutableStateFlow(readiness) + whenever(siteProvisioningSource.stateFor(siteTest)).thenReturn(flow) + return flow } - @Test - fun `given proper site, when api discovery is success, then add the application password card`() = runTest { - stubMintFailure() + private suspend fun stubAuthorized() = whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") - ) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNotNull(applicationPasswordCard) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) - } + .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Authorized(TEST_AUTH_URL)) @Test - fun `given login scenario, when api discovery is empty, then show no card`() = runTest { - stubMintFailure() - whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed("test discovery failure")) + fun `given unprovisionable without prior creds, then show the create card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + stubAuthorized() - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) + assertThat(card).isInstanceOf(MySiteCardAndItem.Card.QuickLinksItem::class.java) } @Test - fun `given headless mint succeeds, then hide card and skip discovery`() = runTest { - stubMintSuccess() + fun `given unprovisionable with prior creds, then show the reauthentication card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + stubAuthorized() - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(siteStore).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper, never()).getAuthorizationUrlComplete(any()) + val banner = card as MySiteCardAndItem.Item.SingleActionCard + assertThat(banner.textResource).isEqualTo(R.string.application_password_reauthentication_banner) } @Test - fun `given headless mint succeeds, then notify credentials changed`() = runTest { - stubMintSuccess() + fun `given unprovisionable but discovery fails, then no card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) + whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) + .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed("bad discovery")) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - verify(credentialsChangedNotifier).notifyChanged(TEST_SITE_ID) + assertNull(card) } @Test - fun `given headless mint succeeds, card hides without waiting for the recoverer`() = runTest { - stubMintSuccess() - val recoverGate = CompletableDeferred() - whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(any())) - .doSuspendableAnswer { recoverGate.await(); null } + fun `given provisioning, then no card`() = test { + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Provisioning)) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - // Card has been hidden even though the recoverer is still suspended on the gate. - assertNull(applicationPasswordCard) - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(siteTest.url) - - // Release the recoverer so the test scope doesn't carry a dangling coroutine. - recoverGate.complete(Unit) + assertNull(card) } @Test - fun `given valid stored creds, card hides without waiting for the recoverer`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "password" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) + fun `given ready on a WPCom-REST site, then no card`() = test { + stubReadiness(SiteReadiness.Ready) + whenever(siteStore.getSiteByLocalId(TEST_SITE_ID)).thenReturn( + SiteModel().apply { id = TEST_SITE_ID; url = TEST_URL; setIsWPCom(true) } ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Valid) - val recoverGate = CompletableDeferred() - whenever(siteApiRestUrlRecoverer.discoverApiRootUrl(any())) - .doSuspendableAnswer { recoverGate.await(); null } - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(TEST_URL) - - recoverGate.complete(Unit) - } - - @Test - fun `given headless mint returns NotSupported, then fall back to discovery`() = runTest { - stubMintFailure(notSupported = true) - whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) - .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") - ) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNotNull(applicationPasswordCard) - verify(siteStore).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper).getAuthorizationUrlComplete(eq(TEST_URL)) - verify(credentialsChangedNotifier, never()).notifyChanged(any()) + assertNull(card) + verify(applicationPasswordLoginHelper, never()).getAuthorizationUrlComplete(any()) } @Test - fun `given site already authenticated and validation succeeds, then show no card`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "password" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) + fun `given ready on a self-hosted site with XML-RPC definitively off, then show the disabled card`() = test { + stubReadiness(SiteReadiness.Ready) + // Not WP.com-REST and still no XML-RPC endpoint after the pipeline's recovery attempt. + whenever(siteStore.getSiteByLocalId(TEST_SITE_ID)).thenReturn( + SiteModel().apply { id = TEST_SITE_ID; url = TEST_URL } ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Valid) + whenever(siteProvisioningSource.isXmlRpcUnavailable(TEST_SITE_ID)).thenReturn(true) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(applicationPasswordValidator).validate(any()) - verify(siteStore, never()).createApplicationPassword(any()) - verify(applicationPasswordLoginHelper, times(0)).getAuthorizationUrlComplete(any()) + val xmlRpcCard = card as MySiteCardAndItem.Item.SingleActionCard + assertThat(xmlRpcCard.textResource).isEqualTo(R.string.xmlrpc_disabled_card_text) } @Test - fun `given stored creds invalid, clear them and try headless mint`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "stale-user" - apiRestPasswordPlain = "stale-pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) + fun `given XML-RPC recovery was inconclusive, then keep the card hidden`() = test { + // A missing xmlRpcUrl is not evidence XML-RPC is off — recovery also fails transiently (e.g. a + // 429), and warning then would be a false positive on a throttled site. + stubReadiness(SiteReadiness.Ready) + whenever(siteStore.getSiteByLocalId(TEST_SITE_ID)).thenReturn( + SiteModel().apply { id = TEST_SITE_ID; url = TEST_URL } ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Invalid) - stubMintSuccess() + whenever(siteProvisioningSource.isXmlRpcUnavailable(TEST_SITE_ID)).thenReturn(false) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNull(applicationPasswordCard) - verify(siteStore).deleteStoredApplicationPasswordCredentials(any()) - // clearSelfHostedClient is invoked twice — once on invalidation, once after the fresh mint - verify(wpApiClientProvider, times(2)).clearSelfHostedClient(siteTest.id) - verify(siteStore).createApplicationPassword(any()) + assertNull(card) } @Test - fun `given stored creds invalid and mint fails, show reauthentication banner`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "stale-user" - apiRestPasswordPlain = "stale-pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.Invalid) - stubMintFailure(notSupported = true) + fun `given a private site with revoked credentials, then the private-site card explains it`() = test { + // Discovery can't produce an authorization URL through the Privacy gate, so the re-auth card + // can't be built. Hiding it leaves the user with broken credentials and no explanation at all. + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) .thenReturn( - ApplicationPasswordLoginHelper.DiscoveryResult.Authorized("$TEST_URL_AUTH$TEST_URL_AUTH_SUFFIX") + ApplicationPasswordLoginHelper.DiscoveryResult.Failed( + userFacingMessage = "Found a site but failed to read its API configuration.", + reason = ApplicationPasswordLoginHelper.DiscoveryResult.FailureReason.PrivateSite, + ) ) - applicationPasswordViewModelSlice.buildCard(siteTest) + slice.buildCard(siteTest) - assertNotNull(applicationPasswordCard) - // Reauth banner uses SingleActionCard (not the QuickLinksItem create card) - assert(applicationPasswordCard is MySiteCardAndItem.Item.SingleActionCard) + val privateCard = card as MySiteCardAndItem.Item.SingleActionCard + assertThat(privateCard.textResource).isEqualTo(R.string.application_password_private_site_card) } @Test - fun `given validation hits a network error, leave the card hidden without re-minting`() = runTest { - whenever(applicationPasswordLoginHelper.siteHasBadCredentials(any())).thenReturn(false) - whenever(siteStore.sites).thenReturn( - listOf( - SiteModel().apply { - id = siteTest.id - url = TEST_URL - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "pass" - xmlRpcUrl = siteTest.xmlRpcUrl - } - ) - ) - whenever(applicationPasswordValidator.validate(any())) - .thenReturn(ApplicationPasswordValidator.Outcome.NetworkUnavailable) - - applicationPasswordViewModelSlice.buildCard(siteTest) - - assertNull(applicationPasswordCard) - verify(siteStore, never()).createApplicationPassword(any()) - verify(siteStore, never()).deleteStoredApplicationPasswordCredentials(any()) - } - - @Test - fun `concurrent buildCard calls coalesce to a single mint`() = runTest { - // Gate the mint so the first buildCard suspends mid-call and the second arrives while it's - // still in flight. Without the single-flight guard, both calls would issue separate - // server-side mints and race the 409 conflict handler in ApplicationPasswordsManager. - val mintGate = CompletableDeferred() - whenever(siteStore.createApplicationPassword(any())).doSuspendableAnswer { - mintGate.await() - OnApplicationPasswordCreated( - siteTest, - ApplicationPasswordCredentials("user", "pass", uuid = "u") - ) - } - - applicationPasswordViewModelSlice.buildCard(siteTest) - applicationPasswordViewModelSlice.buildCard(siteTest) + fun `given discovery fails for any other reason, then the card stays hidden`() = test { + // Only a private site is named; everything else still hides pending #22884. + stubReadiness(SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = true))) + whenever(applicationPasswordLoginHelper.getAuthorizationUrlComplete(eq(TEST_URL))) + .thenReturn(ApplicationPasswordLoginHelper.DiscoveryResult.Failed("connection reset")) - mintGate.complete(Unit) - advanceUntilIdle() + slice.buildCard(siteTest) - verify(siteStore, times(1)).createApplicationPassword(any()) + assertNull(card) } - - @Test - fun `given xmlRpc rediscovery and auth check succeed, then persist the discovered xmlRpcUrl`() = - runTest { - // @Before seeds siteTest.xmlRpcUrl; clear it so the final assertion proves rediscovery set it. - siteTest.xmlRpcUrl = null - val xmlRpcUrl = "https://www.test.com/xmlrpc.php" - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenReturn(xmlRpcUrl) - whenever( - siteXMLRPCClient.fetchSites( - eq(xmlRpcUrl), any(), any(), any() - ) - ).thenReturn(SitesModel(listOf(SiteModel()))) - whenever(siteStore.persistXmlRpcUrl(any(), any())).thenReturn(SiteStore.OnSiteChanged(0)) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(siteStore).persistXmlRpcUrl(siteTest.id, xmlRpcUrl) - assert(siteTest.xmlRpcUrl == xmlRpcUrl) - } - - @Test - fun `given xmlRpc rediscovery succeeds but auth check fails, then do not persist`() = - runTest { - siteTest.xmlRpcUrl = null - val xmlRpcUrl = "https://www.test.com/xmlrpc.php" - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenReturn(xmlRpcUrl) - val errorResult = SitesModel().apply { - error = mock() - } - whenever( - siteXMLRPCClient.fetchSites( - eq(xmlRpcUrl), any(), any(), any() - ) - ).thenReturn(errorResult) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(siteStore, never()).persistXmlRpcUrl(any(), any()) - assert(siteTest.xmlRpcUrl.isNullOrEmpty()) - } - - @Test - fun `given xmlRpc rediscovery fails with a definitive negative, then show the disabled card`() = - runTest { - siteTest.xmlRpcUrl = null - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenThrow( - SelfHostedEndpointFinder.DiscoveryException( - SelfHostedEndpointFinder.DiscoveryError.NO_SITE_ERROR, TEST_URL - ) - ) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(selfHostedEndpointFinder) - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - verify(siteStore, never()).persistXmlRpcUrl(any(), any()) - assert(siteTest.xmlRpcUrl.isNullOrEmpty()) - val card = applicationPasswordCard - assertNotNull(card) - assert(card is MySiteCardAndItem.Item.SingleActionCard) - assert( - (card as MySiteCardAndItem.Item.SingleActionCard).textResource == - R.string.xmlrpc_disabled_card_text - ) - } - - @Test - fun `given xmlRpc rediscovery fails transiently with rate limiting, then keep the card hidden`() = - runTest { - siteTest.xmlRpcUrl = null - whenever( - selfHostedEndpointFinder - .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - ).thenThrow( - SelfHostedEndpointFinder.DiscoveryException( - SelfHostedEndpointFinder.DiscoveryError.RATE_LIMITED, TEST_URL - ) - ) - - applicationPasswordViewModelSlice - .attemptXmlRpcRediscovery(siteTest) - - verify(siteStore, never()).persistXmlRpcUrl(any(), any()) - assert(siteTest.xmlRpcUrl.isNullOrEmpty()) - assertNull(applicationPasswordCard) - } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt index 59ed881f498e..ee3dfd3c28e8 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSliceTest.kt @@ -1,29 +1,28 @@ package org.wordpress.android.ui.mysite.cards.connectivity -import kotlinx.coroutines.CompletableDeferred +import androidx.lifecycle.MutableLiveData import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.advanceUntilIdle import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mock -import org.mockito.Mockito.lenient import org.mockito.junit.MockitoJUnitRunner -import org.mockito.kotlin.doSuspendableAnswer -import org.mockito.kotlin.eq -import org.mockito.kotlin.times +import org.mockito.kotlin.any +import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.R import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier +import org.wordpress.android.repositories.SiteAuthState +import org.wordpress.android.repositories.SiteProvisioningSource +import org.wordpress.android.repositories.SiteReadiness import org.wordpress.android.ui.mysite.MySiteCardAndItem -import org.wordpress.android.ui.mysite.SelectedSiteRepository import org.wordpress.android.util.NetworkUtilsWrapper +import org.wordpress.android.viewmodel.helpers.ConnectionStatus private const val TEST_SITE_LOCAL_ID = 42 @@ -31,18 +30,12 @@ private const val TEST_SITE_LOCAL_ID = 42 @RunWith(MockitoJUnitRunner::class) class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Mock - lateinit var editorSettingsRepository: EditorSettingsRepository + lateinit var siteProvisioningSource: SiteProvisioningSource @Mock lateinit var networkUtilsWrapper: NetworkUtilsWrapper - @Mock - lateinit var credentialsChangedNotifier: CredentialsChangedNotifier - - @Mock - lateinit var selectedSiteRepository: SelectedSiteRepository - - private val credentialsChangedFlow = MutableSharedFlow(extraBufferCapacity = 1) + private val connectionStatus = MutableLiveData() private lateinit var siteTest: SiteModel private lateinit var slice: SiteConnectivityBannerViewModelSlice @@ -51,34 +44,28 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { @Before fun setUp() { siteTest = SiteModel().apply { id = TEST_SITE_LOCAL_ID } - // Default network state is available; tests that need offline override per-test. Lenient - // because tests where the fetch succeeds never reach the network check. - lenient().`when`(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) - whenever(credentialsChangedNotifier.events).thenReturn(credentialsChangedFlow) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) slice = SiteConnectivityBannerViewModelSlice( - editorSettingsRepository, + siteProvisioningSource, networkUtilsWrapper, - credentialsChangedNotifier, - selectedSiteRepository, + connectionStatus, ) slice.initialize(testScope()) slice.uiModel.observeForever { emittedBanners.add(it) } } - @Test - fun `given fetch succeeds, when fetchCapabilities invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - assertThat(emittedBanners.last()).isNull() + private fun stubReadiness( + site: SiteModel, + readiness: SiteReadiness, + ): MutableStateFlow { + val flow = MutableStateFlow(readiness) + whenever(siteProvisioningSource.stateFor(site)).thenReturn(flow) + return flow } @Test - fun `given fetch fails with no cache, when fetchCapabilities invoked, then banner is shown`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given unreachable, when fetchCapabilities invoked, then banner is shown`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -89,48 +76,39 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given fetch fails with no cache but device offline, when fetchCapabilities invoked, then banner is null`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) - - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - - // Global offline indicator covers this case — suppress to avoid stacked warnings. - assertThat(emittedBanners.last()).isNull() - } - - @Test - fun `given fetch fails but app password pending, when fetchCapabilities invoked, then banner is null`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.isAwaitingApplicationPassword(siteTest)).thenReturn(true) + fun `given the banner is showing, when the device goes offline, then it is hidden`() = test { + // Losing the network doesn't re-run the pipeline, so readiness stays Unreachable. Without + // reacting to connectivity the banner sits stacked on the global "no connection" bar. + stubReadiness(siteTest, SiteReadiness.Unreachable) + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() + assertThat(emittedBanners.last()).isNotNull - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + connectionStatus.value = ConnectionStatus.UNAVAILABLE + advanceUntilIdle() - // Credentials are still being minted — pending, not a connection failure. - assertThat(emittedBanners.last()).isNull() - } + assertThat(emittedBanners.last()).isNull() + } @Test - fun `when credentials change for the selected site, then capabilities are re-fetched`() = test { - whenever(selectedSiteRepository.getSelectedSite()).thenReturn(siteTest) - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) + fun `given hidden while offline, when the network returns, then the banner comes back`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(false) + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() + assertThat(emittedBanners.last()).isNull() - credentialsChangedFlow.emit(TEST_SITE_LOCAL_ID) + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + connectionStatus.value = ConnectionStatus.AVAILABLE advanceUntilIdle() - verify(editorSettingsRepository).fetchEditorCapabilitiesForSite(siteTest) + assertThat(emittedBanners.last()).isNotNull } @Test - fun `given fetch fails but cache exists, when fetchCapabilities invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(true) + fun `given ready, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -139,131 +117,100 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { } @Test - fun `given prior successful fetch, when fetchCapabilities invoked again non-user-initiated, then fetch skipped`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) + fun `given needs auth, when fetchCapabilities invoked, then banner is null`() = test { + // Credentials are the problem — the application-password card owns it, banner stays hidden. + stubReadiness(siteTest, SiteReadiness.NeedsAuth(SiteAuthState.Unprovisionable(hadCredentials = false))) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(siteTest) - } + assertThat(emittedBanners.last()).isNull() + } @Test - fun `given prior failed fetch, when fetchCapabilities invoked again non-user-initiated, then fetch retries`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false, true) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given probing, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.Probing) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - assertThat(emittedBanners.last()).isNotNull - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) - assertThat(emittedBanners.last()).isNull() - } + assertThat(emittedBanners.last()).isNull() + } @Test - fun `given prior successful fetch, when user-initiated fetchCapabilities invoked, then fetch runs again`() = - test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) + fun `given transient error, when fetchCapabilities invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.TransientError) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - slice.fetchCapabilities(siteTest, isUserInitiated = true) - advanceUntilIdle() + slice.fetchCapabilities(siteTest, isUserInitiated = false) + advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) - } + assertThat(emittedBanners.last()).isNull() + } @Test - fun `given banner showing, when retry tapped, then fetch runs and bypasses session dedup`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given unreachable then recovered to ready, when state changes, then banner clears`() = test { + val flow = stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard + assertThat(emittedBanners.last()).isNotNull - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - banner.onActionClick() + flow.value = SiteReadiness.Ready advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) assertThat(emittedBanners.last()).isNull() } @Test - fun `when clearBanner invoked, then banner is null`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given user-initiated, when fetchCapabilities invoked, then source is invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) - slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() - assertThat(emittedBanners.last()).isNotNull - slice.clearBanner() + slice.fetchCapabilities(siteTest, isUserInitiated = true) advanceUntilIdle() - assertThat(emittedBanners.last()).isNull() + verify(siteProvisioningSource).invalidate(siteTest) } @Test - fun `given two different sites, when fetched in sequence, then both fetches run`() = test { - val otherSite = SiteModel().apply { id = TEST_SITE_LOCAL_ID + 1 } - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(true) - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(otherSite)).thenReturn(true) + fun `given non-user-initiated, when fetchCapabilities invoked, then source is not invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - slice.fetchCapabilities(otherSite, isUserInitiated = false) - advanceUntilIdle() - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(eq(siteTest)) - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(eq(otherSite)) + verify(siteProvisioningSource, never()).invalidate(any()) } @Test - fun `given fetch in flight, when clearBanner invoked, then banner stays null after fetch completes`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given banner showing, when retry tapped, then source is invalidated`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) - slice.clearBanner() advanceUntilIdle() + val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard - assertThat(emittedBanners.last()).isNull() + banner.onActionClick() + advanceUntilIdle() + + verify(siteProvisioningSource).invalidate(siteTest) } @Test - fun `given retry in flight, when banner tapped again, then second tap is a no-op`() = test { - val gate = CompletableDeferred() - var callCount = 0 - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).doSuspendableAnswer { - callCount++ - if (callCount == 1) false else gate.await() - } - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `when clearBanner invoked, then banner is null`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() - val banner = emittedBanners.last() as MySiteCardAndItem.Item.SingleActionCard - - banner.onActionClick() // first tap — retry suspends on gate - banner.onActionClick() // second tap — should be ignored - gate.complete(true) + assertThat(emittedBanners.last()).isNotNull + slice.clearBanner() advanceUntilIdle() - verify(editorSettingsRepository, times(2)).fetchEditorCapabilitiesForSite(siteTest) + assertThat(emittedBanners.last()).isNull() } @Test - fun `given banner cleared, when retry tapped, then no fetch runs`() = test { - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).thenReturn(false) - whenever(editorSettingsRepository.hasCachedCapabilities(siteTest)).thenReturn(false) + fun `given banner cleared, when retry tapped, then no invalidate runs`() = test { + stubReadiness(siteTest, SiteReadiness.Unreachable) slice.fetchCapabilities(siteTest, isUserInitiated = false) advanceUntilIdle() @@ -271,31 +218,28 @@ class SiteConnectivityBannerViewModelSliceTest : BaseUnitTest() { slice.clearBanner() advanceUntilIdle() - // Simulate a tap that landed before LiveData propagated the null clear. banner.onActionClick() advanceUntilIdle() - verify(editorSettingsRepository, times(1)).fetchEditorCapabilitiesForSite(siteTest) + verify(siteProvisioningSource, never()).invalidate(any()) } @Test - fun `given fetch in flight for site A, when fetch starts for site B, then site A result is discarded`() = test { + fun `given site switched, when old site becomes unreachable, then banner ignores it`() = test { val siteB = SiteModel().apply { id = TEST_SITE_LOCAL_ID + 1 } - val gateA = CompletableDeferred() - // Site A's fetch suspends on a gate so we can interleave site B's call before A completes. - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteTest)).doSuspendableAnswer { - gateA.await() - } - whenever(editorSettingsRepository.fetchEditorCapabilitiesForSite(siteB)).thenReturn(true) + val flowA = stubReadiness(siteTest, SiteReadiness.Ready) + stubReadiness(siteB, SiteReadiness.Ready) slice.fetchCapabilities(siteTest, isUserInitiated = false) - advanceUntilIdle() // siteA suspended in fetch + advanceUntilIdle() slice.fetchCapabilities(siteB, isUserInitiated = false) - advanceUntilIdle() // siteB completes; currentSite is now siteB - gateA.complete(false) // release siteA — its result must NOT post a banner advanceUntilIdle() - // No banner card should ever have been emitted for site A. - assertThat(emittedBanners.filterIsInstance()).isEmpty() + flowA.value = SiteReadiness.Unreachable + advanceUntilIdle() + + assertThat( + emittedBanners.filterIsInstance() + ).isEmpty() } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt index b009b9f3cc2d..c5989af09870 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergEditorPreloaderTest.kt @@ -19,8 +19,8 @@ import org.wordpress.android.BaseUnitTest import org.wordpress.android.datasets.SiteSettingsProvider import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.store.AccountStore -import org.wordpress.android.repositories.EditorSettingsRepository -import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer +import org.wordpress.android.fluxc.store.SiteStore +import org.wordpress.android.repositories.SiteProvisioningSource import org.wordpress.gutenberg.model.EditorAssetBundle import org.wordpress.gutenberg.model.EditorConfiguration import org.wordpress.gutenberg.model.EditorDependencies @@ -49,10 +49,10 @@ class GutenbergEditorPreloaderTest : lateinit var editorServiceProvider: EditorServiceProvider @Mock - lateinit var editorSettingsRepository: EditorSettingsRepository + lateinit var siteProvisioningSource: SiteProvisioningSource @Mock - lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer + lateinit var siteStore: SiteStore private val editorDependencies = EditorDependencies.empty @@ -75,8 +75,8 @@ class GutenbergEditorPreloaderTest : gutenbergKitSettingsBuilder = gutenbergKitSettingsBuilder, siteSettingsProvider = siteSettingsProvider, editorServiceProvider = editorServiceProvider, - editorSettingsRepository = editorSettingsRepository, - siteApiRestUrlRecoverer = siteApiRestUrlRecoverer, + siteProvisioningSource = siteProvisioningSource, + siteStore = siteStore, bgDispatcher = testDispatcher() ) } @@ -194,7 +194,7 @@ class GutenbergEditorPreloaderTest : } @Test - fun `successful preload fetches editor capabilities`() = test { + fun `successful preload runs the provisioning source`() = test { val site = createSite() enablePreloading(site) stubSuccessfulPreload() @@ -203,8 +203,7 @@ class GutenbergEditorPreloaderTest : preloader.preloadIfNeeded(site, this) advanceUntilIdle() - verify(editorSettingsRepository) - .fetchEditorCapabilitiesForSite(site) + verify(siteProvisioningSource).await(site) } @Test @@ -482,22 +481,4 @@ class GutenbergEditorPreloaderTest : } // endregion - - // region wpApiRestUrl recovery - - @Test - fun `successful preload invokes discovery only — slice owns persistence`() = test { - val site = createSite() - enablePreloading(site) - stubSuccessfulPreload() - stubEditorService() - - preloader.preloadIfNeeded(site, this) - advanceUntilIdle() - - verify(siteApiRestUrlRecoverer).discoverApiRootUrl(site.url) - verify(siteApiRestUrlRecoverer, never()).persistApiRootUrl(any(), any()) - } - - // endregion } diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandler.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandler.kt index 58a8871eeb99..6ce9c3fb3eed 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandler.kt +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandler.kt @@ -18,7 +18,10 @@ class WpAppNotifierHandler @Inject constructor() { cleanupDeadReferences() listeners.forEach { val listener = it.value.get() - listener?.onRequestedWithInvalidAuthentication(site.url) + // Hand listeners the whole SiteModel, not just its URL: URL is not a unique key + // (UNIQUE is on SITE_ID+URL), so a listener resolving back by URL can't tell which row + // actually failed. The id pins it to the exact site whose client raised the 401. + listener?.onRequestedWithInvalidAuthentication(site) } } @@ -37,6 +40,6 @@ class WpAppNotifierHandler @Inject constructor() { } interface NotifierListener { - fun onRequestedWithInvalidAuthentication(siteUrl: String) + fun onRequestedWithInvalidAuthentication(site: SiteModel) } } diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandlerTest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandlerTest.kt index f480e591064f..338323602894 100644 --- a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandlerTest.kt +++ b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/WpAppNotifierHandlerTest.kt @@ -38,8 +38,8 @@ class WpAppNotifierHandlerTest { wpAppNotifierHandler.notifyRequestedWithInvalidAuthentication(testSite) // Then - verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSiteUrl) - verify(mockListener2, times(1)).onRequestedWithInvalidAuthentication(testSiteUrl) + verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSite) + verify(mockListener2, times(1)).onRequestedWithInvalidAuthentication(testSite) } @Test @@ -53,7 +53,7 @@ class WpAppNotifierHandlerTest { } @Test - fun `notifyRequestedWithInvalidAuthentication passes correct site URL to listeners`() { + fun `notifyRequestedWithInvalidAuthentication passes the site to listeners`() { // Given val customSiteUrl = "https://custom-site.example.org" val customSite = SiteModel().apply { @@ -66,7 +66,7 @@ class WpAppNotifierHandlerTest { wpAppNotifierHandler.notifyRequestedWithInvalidAuthentication(customSite) // Then - verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(customSiteUrl) + verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(customSite) } @Test @@ -80,8 +80,8 @@ class WpAppNotifierHandlerTest { wpAppNotifierHandler.notifyRequestedWithInvalidAuthentication(testSite) // Then - only remaining listener should be called - verify(mockListener1, never()).onRequestedWithInvalidAuthentication(testSiteUrl) - verify(mockListener2, times(1)).onRequestedWithInvalidAuthentication(testSiteUrl) + verify(mockListener1, never()).onRequestedWithInvalidAuthentication(testSite) + verify(mockListener2, times(1)).onRequestedWithInvalidAuthentication(testSite) } @Test @@ -104,7 +104,7 @@ class WpAppNotifierHandlerTest { wpAppNotifierHandler.notifyRequestedWithInvalidAuthentication(testSite) // Then - original listener should still be called - verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSiteUrl) + verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSite) } @Test @@ -117,6 +117,6 @@ class WpAppNotifierHandlerTest { wpAppNotifierHandler.notifyRequestedWithInvalidAuthentication(testSite) // Then - should only be called once (overwrites previous entry) - verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSiteUrl) + verify(mockListener1, times(1)).onRequestedWithInvalidAuthentication(testSite) } }