Skip to content

Commit 840c7c0

Browse files
authored
Fix false connectivity banner on private Atomic sites (#22926)
* Fix false connectivity banner on private Atomic sites Editor capability detection on Atomic sites probes the direct host via unauthenticated REST API discovery (added in #22883). Private Atomic hosts gate anonymous requests, so discovery fails and the My Site "Unable to connect to your site" banner appears even though the site is reachable. Fall back to an authenticated app-password probe against the same host when discovery fails and credentials exist. * Log authenticated direct-host probe failures for diagnosability The authenticated app-password fallback previously returned a silent `false` on a non-success response, and the no-credentials path was also silent. Log both (mirroring the discovery-failure log) so a recurrence of the connectivity banner can be diagnosed from AppLog rather than being indistinguishable from "discovery failed, no creds". * Suppress the connectivity banner while the app password is being minted On first login an Atomic site's application password is minted asynchronously on the My Site screen, so the connectivity capability fetch can race ahead of it and fail purely for lack of credentials — flashing a false "unable to connect" banner that clears on the next refresh. Treat a credential-less fetch as pending rather than a connection failure, mirroring the existing offline suppression; the application-password card already owns the no-credentials state. * Re-detect editor capabilities when an app password is established The connectivity banner's capability fetch and the application-password mint both run on the My Site screen with no ordering, so on first login the fetch loses the race and capability flags aren't detected until the next resume/refresh. Add an app-scoped CredentialsChangedNotifier that both credential-establishment paths (headless mint and interactive login) emit to, and have the connectivity slice re-run detection on it — re-reading a fresh site so it observes the just-persisted credentials. This also closes the asymmetry where only the interactive path announced credential changes globally. * Hoist connectivity banner suppression into a named flag The four-way suppression condition tripped detekt's ComplexCondition threshold once the pending-credentials term was added. Lift it into a named `suppressBanner` flag — clearer intent, and out of the if-condition detekt inspects. No behavior change. * Surface the connectivity banner when the app-password mint fails The pending-credentials suppression also hid the banner when the headless mint *terminally* failed — and the application-password card can't render for private Atomic sites either (its authorize-URL discovery hits the same gated host), so the user was left with a broken site and no signal at all. Track terminal mint failure in the renamed ApplicationPasswordMonitor (was CredentialsChangedNotifier) and suppress only while provisioning is genuinely in flight; once the mint fails, surface the banner. The monitor also wakes capability detection on failure so the banner appears without waiting for the next resume/refresh. * Revert "Surface the connectivity banner when the app-password mint fails" This reverts commit 357fd82. The terminal-failure surfacing called onMintFailed on every non-success, including transient failures, so a transient first-login mint failure latched hasMintFailed and showed the false connectivity banner this PR set out to suppress. Revert to the pending-suppression behavior; the durable single-source-of-truth redesign is tracked in #22942.
1 parent ddea2c4 commit 840c7c0

10 files changed

Lines changed: 291 additions & 14 deletions

File tree

WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt

Lines changed: 66 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,17 @@ class EditorSettingsRepository @Inject constructor(
3434
fun hasCachedCapabilities(site: SiteModel): Boolean =
3535
appPrefsWrapper.hasSiteEditorCapabilities(site)
3636

37+
/**
38+
* True when capability detection can't run yet because an Atomic site's
39+
* direct-host probe needs an application password that hasn't been
40+
* provisioned. The password is minted asynchronously on the My Site
41+
* screen (see ApplicationPasswordViewModelSlice), so a first-login fetch
42+
* can fail purely for lack of credentials — callers should treat this as
43+
* pending, not a connection failure.
44+
*/
45+
fun isAwaitingApplicationPassword(site: SiteModel): Boolean =
46+
site.isWPComAtomic && !site.hasApplicationPasswordCredentials()
47+
3748
/**
3849
* Returns whether the site is known to support the
3950
* `wp-block-editor/v1/settings` endpoint, based on
@@ -139,24 +150,66 @@ class EditorSettingsRepository @Inject constructor(
139150
* assume the API lives at `/wp-json` (custom permalink structures or
140151
* REST API paths would break that assumption), then use the routes list
141152
* returned by discovery directly — no second request needed.
153+
*
154+
* Discovery is unauthenticated, so it can't reach a *private* Atomic host
155+
* — the host gates anonymous requests and the API root never loads. When
156+
* the site has application-password credentials, fall back to an
157+
* authenticated probe against the same direct host (Basic auth), which is
158+
* exactly the transport the editor uses there. Without credentials there's
159+
* nothing to authenticate with, so we report failure. See #22883.
142160
*/
143161
private suspend fun fetchRouteSupportViaDirectHostDiscovery(
144162
site: SiteModel
145163
): Boolean {
146164
val discovery = wpLoginClient.apiDiscovery(site.url)
147-
if (discovery !is ApiDiscoveryResult.Success) {
165+
if (discovery is ApiDiscoveryResult.Success) {
166+
val resolver = wpApiClientProvider.urlResolverFor(
167+
discovery.success.apiRootUrl
168+
)
169+
persistRouteSupport(site, discovery.success.apiDetails, resolver)
170+
return true
171+
}
172+
AppLog.w(
173+
T.EDITOR,
174+
"Direct-host API discovery failed for" +
175+
" site=${site.name}: ${discovery::class.simpleName}"
176+
)
177+
return if (site.hasApplicationPasswordCredentials()) {
178+
fetchRouteSupportViaApplicationPasswordClient(site)
179+
} else {
148180
AppLog.w(
149181
T.EDITOR,
150-
"Direct-host API discovery failed for" +
151-
" site=${site.name}: ${discovery::class.simpleName}"
182+
"No application password for site=${site.name};" +
183+
" skipping authenticated direct-host probe"
152184
)
153-
return false
185+
false
186+
}
187+
}
188+
189+
/**
190+
* Authenticated direct-host route probe for Atomic sites whose host
191+
* rejects the anonymous discovery request (e.g. private sites). Uses the
192+
* site's application-password (Basic auth) client and resolves routes
193+
* against the same direct host — mirrors
194+
* [fetchRouteSupportViaConfiguredClient] but bypasses the WP.com proxy.
195+
*/
196+
private suspend fun fetchRouteSupportViaApplicationPasswordClient(
197+
site: SiteModel
198+
): Boolean {
199+
val client = wpApiClientProvider.getApplicationPasswordClient(site)
200+
val resolver = wpApiClientProvider.getDirectHostApiUrlResolver(site)
201+
val response = client.request { it.apiRoot().get() }
202+
return if (response is WpRequestResult.Success) {
203+
persistRouteSupport(site, response.response.data, resolver)
204+
true
205+
} else {
206+
AppLog.w(
207+
T.EDITOR,
208+
"Authenticated direct-host probe failed for" +
209+
" site=${site.name}: ${response::class.simpleName}"
210+
)
211+
false
154212
}
155-
val resolver = wpApiClientProvider.urlResolverFor(
156-
discovery.success.apiRootUrl
157-
)
158-
persistRouteSupport(site, discovery.success.apiDetails, resolver)
159-
return true
160213
}
161214

162215
private fun persistRouteSupport(
@@ -215,3 +268,7 @@ class EditorSettingsRepository @Inject constructor(
215268
false
216269
}
217270
}
271+
272+
private fun SiteModel.hasApplicationPasswordCredentials(): Boolean =
273+
!apiRestUsernamePlain.isNullOrEmpty() &&
274+
!apiRestPasswordPlain.isNullOrEmpty()

WordPress/src/main/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelper.kt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ class ApplicationPasswordLoginHelper @Inject constructor(
4646
private val discoverSuccessWrapper: DiscoverSuccessWrapper,
4747
private val crashLogging: CrashLogging,
4848
private val wpApiClientProvider: WpApiClientProvider,
49+
private val credentialsChangedNotifier: CredentialsChangedNotifier,
4950
) {
5051
private var processedAppPasswordData: String? = null
5152

@@ -148,6 +149,7 @@ class ApplicationPasswordLoginHelper @Inject constructor(
148149
}
149150
wpApiClientProvider.clearSelfHostedClient(site.id)
150151
dispatcherWrapper.updateApplicationPassword(site)
152+
credentialsChangedNotifier.notifyChanged(site.id)
151153
trackSuccessful(effectiveUrlLogin.siteUrl)
152154
trackCreated(creationSource, success = true)
153155
processedAppPasswordData = effectiveUrlLogin.siteUrl
Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,34 @@
1+
package org.wordpress.android.ui.accounts.login
2+
3+
import kotlinx.coroutines.channels.BufferOverflow
4+
import kotlinx.coroutines.flow.MutableSharedFlow
5+
import kotlinx.coroutines.flow.SharedFlow
6+
import kotlinx.coroutines.flow.asSharedFlow
7+
import javax.inject.Inject
8+
import javax.inject.Singleton
9+
10+
/**
11+
* App-scoped signal that an application password was newly established for a site — either by the
12+
* headless Jetpack-tunnel mint on the My Site screen or by the interactive application-password
13+
* login. Lets credential-dependent work (e.g. editor capability detection) re-run as soon as the
14+
* password exists, instead of waiting for the next My Site resume/refresh.
15+
*
16+
* Emits the site's local id; collectors should re-read a fresh SiteModel so they observe the
17+
* just-persisted credentials rather than a stale in-memory copy.
18+
*/
19+
@Singleton
20+
class CredentialsChangedNotifier @Inject constructor() {
21+
// replay = 1 so a collector that subscribes just after an emit still sees it — closes the
22+
// emit-before-collect race. DROP_OLDEST keeps tryEmit non-suspending without an unbounded buffer.
23+
private val _events = MutableSharedFlow<Int>(
24+
replay = 1,
25+
extraBufferCapacity = 1,
26+
onBufferOverflow = BufferOverflow.DROP_OLDEST,
27+
)
28+
val events: SharedFlow<Int> = _events.asSharedFlow()
29+
30+
/** Signals that [siteLocalId]'s application-password credentials were just established. */
31+
fun notifyChanged(siteLocalId: Int) {
32+
_events.tryEmit(siteLocalId)
33+
}
34+
}

WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider
1818
import org.wordpress.android.fluxc.store.SiteStore
1919
import org.wordpress.android.fluxc.utils.AppLogWrapper
2020
import org.wordpress.android.ui.accounts.login.ApplicationPasswordLoginHelper
21+
import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier
2122
import org.wordpress.android.ui.accounts.login.SiteApiRestUrlRecoverer
2223
import org.wordpress.android.ui.mysite.MySiteCardAndItem
2324
import org.wordpress.android.ui.mysite.MySiteCardAndItem.Card.QuickLinksItem.QuickLinkItem
@@ -41,6 +42,7 @@ class ApplicationPasswordViewModelSlice @Inject constructor(
4142
private val siteXMLRPCClient: SiteXMLRPCClient,
4243
private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer,
4344
private val dispatcher: Dispatcher,
45+
private val credentialsChangedNotifier: CredentialsChangedNotifier,
4446
@Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher,
4547
) {
4648
lateinit var scope: CoroutineScope
@@ -112,6 +114,7 @@ class ApplicationPasswordViewModelSlice @Inject constructor(
112114
if (!createResult.isError && createResult.credentials != null) {
113115
wpApiClientProvider.clearSelfHostedClient(storedSite.id)
114116
appLogWrapper.d(AppLog.T.MAIN, "A_P: Headless mint succeeded for ${storedSite.url}")
117+
credentialsChangedNotifier.notifyChanged(storedSite.id)
115118
// The mint goes through the Jetpack tunnel and never runs discovery — without this
116119
// step, freshly minted Atomic sites end up with working creds but a NULL
117120
// wpApiRestUrl in the local DB. Run in the background so the card hides immediately.

WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/connectivity/SiteConnectivityBannerViewModelSlice.kt

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,13 +8,17 @@ import kotlinx.coroutines.launch
88
import org.wordpress.android.R
99
import org.wordpress.android.fluxc.model.SiteModel
1010
import org.wordpress.android.repositories.EditorSettingsRepository
11+
import org.wordpress.android.ui.accounts.login.CredentialsChangedNotifier
1112
import org.wordpress.android.ui.mysite.MySiteCardAndItem
13+
import org.wordpress.android.ui.mysite.SelectedSiteRepository
1214
import org.wordpress.android.util.NetworkUtilsWrapper
1315
import javax.inject.Inject
1416

1517
class SiteConnectivityBannerViewModelSlice @Inject constructor(
1618
private val editorSettingsRepository: EditorSettingsRepository,
1719
private val networkUtilsWrapper: NetworkUtilsWrapper,
20+
private val credentialsChangedNotifier: CredentialsChangedNotifier,
21+
private val selectedSiteRepository: SelectedSiteRepository,
1822
) {
1923
private lateinit var scope: CoroutineScope
2024
private var currentJob: Job? = null
@@ -31,6 +35,18 @@ class SiteConnectivityBannerViewModelSlice @Inject constructor(
3135

3236
fun initialize(scope: CoroutineScope) {
3337
this.scope = scope
38+
// Re-run detection the moment an application password is established for the selected site
39+
// (e.g. the headless mint finished after our first fetch lost the race), instead of waiting
40+
// for the next resume/refresh. Re-read the selected site so we see the just-persisted
41+
// credentials; isUserInitiated = false so a replayed event is a no-op once cached.
42+
scope.launch {
43+
credentialsChangedNotifier.events.collect { siteLocalId ->
44+
val site = selectedSiteRepository.getSelectedSite()
45+
if (site != null && site.id == siteLocalId) {
46+
fetchCapabilities(site, isUserInitiated = false)
47+
}
48+
}
49+
}
3450
}
3551

3652
fun fetchCapabilities(site: SiteModel, isUserInitiated: Boolean) {
@@ -52,7 +68,17 @@ class SiteConnectivityBannerViewModelSlice @Inject constructor(
5268
// connection" banner already covers this case, and stacking warnings
5369
// for the same root cause is just noise.
5470
val suppressForOffline = !ok && !networkUtilsWrapper.isNetworkAvailable()
55-
_uiModel.postValue(if (ok || hasCache || suppressForOffline) null else buildBanner())
71+
// Atomic sites probe the direct host with an application password that's minted
72+
// asynchronously on this same screen, so a first-login fetch can fail purely because
73+
// the credential isn't ready yet. Treat that as pending, not a connection failure —
74+
// the application-password card owns that state and a later fetch will succeed.
75+
val suppressForPendingAuth =
76+
!ok && editorSettingsRepository.isAwaitingApplicationPassword(site)
77+
// Show the banner only as a last resort — not when detection succeeded, when we have
78+
// cached capabilities, or while a transient non-error state (offline / pending creds)
79+
// already explains the failure.
80+
val suppressBanner = ok || hasCache || suppressForOffline || suppressForPendingAuth
81+
_uiModel.postValue(if (suppressBanner) null else buildBanner())
5682
}
5783
}
5884

WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt

Lines changed: 85 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,9 @@ class EditorSettingsRepositoryTest : BaseUnitTest() {
5353
@Mock
5454
lateinit var wpApiClient: WpApiClient
5555

56+
@Mock
57+
lateinit var appPasswordClient: WpApiClient
58+
5659
@Mock
5760
lateinit var apiUrlResolver: ApiUrlResolver
5861

@@ -285,6 +288,84 @@ class EditorSettingsRepositoryTest : BaseUnitTest() {
285288
verify(wpApiClientProvider, never()).getWpApiClient(atomicSite)
286289
}
287290

291+
@Test
292+
fun `atomic site falls back to authenticated probe when discovery fails`() =
293+
runTest {
294+
val atomicSite = SiteModel().apply {
295+
id = 5
296+
url = "https://atomic.example.com"
297+
setIsWPCom(true)
298+
setIsWPComAtomic(true)
299+
apiRestUsernamePlain = "user"
300+
apiRestPasswordPlain = "secret"
301+
}
302+
mockDiscoveryFailure(atomicSite.url)
303+
whenever(
304+
wpApiClientProvider.getApplicationPasswordClient(atomicSite)
305+
).thenReturn(appPasswordClient)
306+
whenever(
307+
wpApiClientProvider.getDirectHostApiUrlResolver(atomicSite)
308+
).thenReturn(directHostResolver)
309+
mockApiRootResponseFor(
310+
client = appPasswordClient,
311+
resolver = directHostResolver,
312+
hasEditorSettings = true,
313+
hasEditorAssets = true,
314+
)
315+
whenever(themeRepository.fetchCurrentTheme(atomicSite))
316+
.thenReturn(buildTheme(isBlockTheme = false))
317+
318+
val result =
319+
repository.fetchEditorCapabilitiesForSite(atomicSite)
320+
321+
assertThat(result).isTrue()
322+
verify(appPrefsWrapper)
323+
.setSiteSupportsEditorSettings(atomicSite, true)
324+
verify(appPrefsWrapper)
325+
.setSiteSupportsEditorAssets(atomicSite, true)
326+
}
327+
328+
@Test
329+
fun `atomic site returns false when authenticated probe also fails`() =
330+
runTest {
331+
val atomicSite = SiteModel().apply {
332+
id = 6
333+
url = "https://atomic.example.com"
334+
setIsWPCom(true)
335+
setIsWPComAtomic(true)
336+
apiRestUsernamePlain = "user"
337+
apiRestPasswordPlain = "secret"
338+
}
339+
mockDiscoveryFailure(atomicSite.url)
340+
whenever(
341+
wpApiClientProvider.getApplicationPasswordClient(atomicSite)
342+
).thenReturn(appPasswordClient)
343+
whenever(
344+
wpApiClientProvider.getDirectHostApiUrlResolver(atomicSite)
345+
).thenReturn(directHostResolver)
346+
mockApiRootErrorFor(appPasswordClient)
347+
whenever(themeRepository.fetchCurrentTheme(atomicSite))
348+
.thenReturn(buildTheme(isBlockTheme = false))
349+
350+
val result =
351+
repository.fetchEditorCapabilitiesForSite(atomicSite)
352+
353+
assertThat(result).isFalse()
354+
verify(appPrefsWrapper, never())
355+
.setSiteSupportsEditorSettings(any(), any())
356+
verify(appPrefsWrapper, never())
357+
.setSiteSupportsEditorAssets(any(), any())
358+
}
359+
360+
private suspend fun mockDiscoveryFailure(siteUrl: String) {
361+
whenever(wpLoginClient.apiDiscovery(siteUrl))
362+
.thenReturn(
363+
ApiDiscoveryResult.FailureParseSiteUrl(
364+
ParseUrlException.Generic("")
365+
)
366+
)
367+
}
368+
288369
private suspend fun mockApiRootResponse(
289370
hasEditorSettings: Boolean,
290371
hasEditorAssets: Boolean
@@ -363,15 +444,17 @@ class EditorSettingsRepositoryTest : BaseUnitTest() {
363444
)
364445
}
365446

447+
private suspend fun mockApiRootError() = mockApiRootErrorFor(wpApiClient)
448+
366449
@Suppress("UNCHECKED_CAST")
367-
private suspend fun mockApiRootError() {
450+
private suspend fun mockApiRootErrorFor(client: WpApiClient) {
368451
val error = WpRequestResult.UnknownError<Any>(
369452
statusCode = 500u,
370453
response = "Internal Server Error",
371454
requestUrl = "https://test.wordpress.com/wp-json",
372455
requestMethod = uniffi.wp_api.RequestMethod.GET
373456
)
374-
whenever(wpApiClient.request<Any>(any()))
457+
whenever(client.request<Any>(any()))
375458
.thenReturn(error)
376459
}
377460

WordPress/src/test/java/org/wordpress/android/ui/accounts/login/ApplicationPasswordLoginHelperTest.kt

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,9 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() {
7676
@Mock
7777
lateinit var wpApiClientProvider: WpApiClientProvider
7878

79+
@Mock
80+
lateinit var credentialsChangedNotifier: CredentialsChangedNotifier
81+
7982
private lateinit var applicationPasswordLoginHelper: ApplicationPasswordLoginHelper
8083

8184
@Before
@@ -92,7 +95,8 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() {
9295
apiRootUrlCache,
9396
discoverSuccessWrapper,
9497
crashLogging,
95-
wpApiClientProvider
98+
wpApiClientProvider,
99+
credentialsChangedNotifier
96100
)
97101
}
98102

@@ -206,6 +210,7 @@ class ApplicationPasswordLoginHelperTest : BaseUnitTest() {
206210
verify(siteStore).sites
207211
verify(dispatcherWrapper).updateApplicationPassword(eq(siteModel))
208212
verify(wpApiClientProvider).clearSelfHostedClient(eq(siteModel.id))
213+
verify(credentialsChangedNotifier).notifyChanged(eq(siteModel.id))
209214
}
210215

211216
@Test

0 commit comments

Comments
 (0)