From 519cfc1a903f552aea10fa8e5e434c8e3000f459 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 29 May 2026 13:37:14 -0600 Subject: [PATCH 1/2] Probe editor capability endpoints instead of trusting the route list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the editor failing to launch on sites where the WP.com REST proxy advertises `wp-block-editor/v1/settings` in its route list but the actual call 404s — most visibly on JPC sandboxes like `protective-artifact-leopard.jurassic.ninja`. The old detection asked `apiRoot().get()` whether the route was registered; the route was there, capability said "supported," then the editor's settings fetch 404'd. Replace the route-advertisement check with direct endpoint probes via OkHttp: - 2xx / 401 / 403 → Supported (route exists; auth required is fine) - 404 → Unsupported - network / 5xx → Inconclusive, leave the cached pref alone Probes are cancellable (`suspendCancellableCoroutine` + `Call.cancel` via `invokeOnCancellation`) so dangling preloads don't keep sockets open after the scope is torn down. Preserves the Atomic-direct-host fix from #22879/#22883: GutenbergKit fetches `wp-block-editor/v1/settings` from the direct host on Atomic sites (bypassing `siteApiRoot`), so capability detection must too — pinned with a MUST-NOT-REMOVE test. `getSupportsEditorSettingsForSite` now prefers the probe answer when it has run, falling back to the legacy `EditorSettingsTable` row as the first-launch fast-path. Auth-header construction factored into `EditorAuthHeaderBuilder` so the probe and `GutenbergKitSettingsBuilder` cannot drift on scheme selection. --- .../repositories/EditorAuthHeaderBuilder.kt | 90 ++ .../repositories/EditorSettingsRepository.kt | 310 ++++--- .../ui/posts/GutenbergKitSettingsBuilder.kt | 51 +- .../EditorAuthHeaderBuilderTest.kt | 179 ++++ .../EditorSettingsRepositoryTest.kt | 779 +++++++++++------- .../posts/GutenbergKitSettingsBuilderTest.kt | 131 +-- 6 files changed, 970 insertions(+), 570 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/repositories/EditorAuthHeaderBuilder.kt create mode 100644 WordPress/src/test/java/org/wordpress/android/repositories/EditorAuthHeaderBuilderTest.kt diff --git a/WordPress/src/main/java/org/wordpress/android/repositories/EditorAuthHeaderBuilder.kt b/WordPress/src/main/java/org/wordpress/android/repositories/EditorAuthHeaderBuilder.kt new file mode 100644 index 000000000000..c35400f0202c --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorAuthHeaderBuilder.kt @@ -0,0 +1,90 @@ +package org.wordpress.android.repositories + +import android.util.Base64 +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.util.AppLog +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Builds the `Authorization` header that the editor — and editor + * capability probes — use when talking to a site. Centralised so the + * probe in [EditorSettingsRepository] cannot drift from what + * [org.wordpress.android.ui.posts.GutenbergKitSettingsBuilder] hands + * to GutenbergKit at launch. + * + * - WP.com-routed sites (JPC, Atomic, WPCom) → `Bearer ` + * - Self-hosted sites with an application password → `Basic ` + * + * Returns `null` when the required credentials are missing. + */ +@Singleton +class EditorAuthHeaderBuilder @Inject constructor() { + /** + * Convenience overload that derives [shouldUseWPComRestApi] and + * credentials from [site]. + */ + fun build(site: SiteModel, accessToken: String?): String? { + val applicationPassword = site.apiRestPasswordPlain + val shouldUseWPComRestApi = + applicationPassword.isNullOrEmpty() && site.isUsingWpComRestApi + return build( + shouldUseWPComRestApi = shouldUseWPComRestApi, + accessToken = accessToken, + username = site.apiRestUsernamePlain, + password = applicationPassword, + ) + } + + fun build( + shouldUseWPComRestApi: Boolean, + accessToken: String?, + username: String?, + password: String?, + ): String? = if (shouldUseWPComRestApi) { + buildBearer(accessToken) + } else { + buildBasic(username, password) + } + + private fun buildBearer(accessToken: String?): String? = + if (!accessToken.isNullOrEmpty()) { + "$AUTH_BEARER_PREFIX$accessToken" + } else { + AppLog.w( + AppLog.T.EDITOR, + "Missing access token for WP.com REST API authentication" + ) + null + } + + private fun buildBasic(username: String?, password: String?): String? { + if (username.isNullOrEmpty() || password.isNullOrEmpty()) { + AppLog.w( + AppLog.T.EDITOR, + "Incomplete credentials for Basic authentication" + ) + return null + } + return try { + val credentials = "$username:$password" + val encoded = Base64.encodeToString( + credentials.toByteArray(Charsets.UTF_8), + Base64.NO_WRAP + ) + "$AUTH_BASIC_PREFIX$encoded" + } catch (e: IllegalArgumentException) { + AppLog.e( + AppLog.T.EDITOR, + "Failed to encode Basic auth credentials", + e + ) + null + } + } + + companion object { + private const val AUTH_BEARER_PREFIX = "Bearer " + private const val AUTH_BASIC_PREFIX = "Basic " + } +} 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 a63d68dcecd4..fc804cf7f58b 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt @@ -4,30 +4,37 @@ import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll +import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.withContext +import okhttp3.Call +import okhttp3.Callback +import okhttp3.OkHttpClient +import okhttp3.Request +import okhttp3.Response +import kotlin.coroutines.resume +import kotlin.coroutines.resumeWithException import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.module.OkHttpClientQualifiers +import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.ui.prefs.AppPrefsWrapper import org.wordpress.android.fluxc.persistence.EditorSettingsSqlUtils import org.wordpress.android.modules.IO_THREAD import org.wordpress.android.util.AppLog import org.wordpress.android.util.AppLog.T -import rs.wordpress.api.kotlin.ApiDiscoveryResult -import rs.wordpress.api.kotlin.WpLoginClient -import rs.wordpress.api.kotlin.WpRequestResult -import uniffi.wp_api.ApiUrlResolver -import uniffi.wp_api.WpApiDetails +import java.io.IOException +import java.net.HttpURLConnection import javax.inject.Inject import javax.inject.Named import javax.inject.Singleton @Singleton class EditorSettingsRepository @Inject constructor( - private val wpApiClientProvider: WpApiClientProvider, - private val wpLoginClient: WpLoginClient, - private val appPrefsWrapper: AppPrefsWrapper, private val themeRepository: ThemeRepository, - @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher + private val appPrefsWrapper: AppPrefsWrapper, + private val accountStore: AccountStore, + private val editorAuthHeaderBuilder: EditorAuthHeaderBuilder, + @Named(OkHttpClientQualifiers.REGULAR) private val okHttpClient: OkHttpClient, + @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, ) { private val editorSettingsSqlUtils = EditorSettingsSqlUtils() @@ -36,17 +43,24 @@ class EditorSettingsRepository @Inject constructor( /** * Returns whether the site is known to support the - * `wp-block-editor/v1/settings` endpoint, based on - * cached editor settings or a previously persisted - * result from [fetchEditorCapabilitiesForSite]. + * `wp-block-editor/v1/settings` endpoint. + * + * Once [fetchEditorCapabilitiesForSite] has probed the + * endpoint and persisted an answer, that result is + * authoritative — it correctly reflects sites where the + * endpoint was once available but no longer is. + * + * Before the probe has answered (e.g. on first launch), + * a non-null row in the legacy [EditorSettingsSqlUtils] + * cache stands in as a fast-path "we successfully fetched + * settings here before." */ - fun getSupportsEditorSettingsForSite(site: SiteModel): Boolean { - val hasExisting = - editorSettingsSqlUtils.getEditorSettingsForSite(site) != null - val cachedPref = + fun getSupportsEditorSettingsForSite(site: SiteModel): Boolean = + if (appPrefsWrapper.hasSiteEditorCapabilities(site)) { appPrefsWrapper.getSiteSupportsEditorSettings(site) - return hasExisting || cachedPref - } + } else { + editorSettingsSqlUtils.getEditorSettingsForSite(site) != null + } /** * Returns whether the site is known to support the @@ -70,116 +84,192 @@ class EditorSettingsRepository @Inject constructor( appPrefsWrapper.getSiteThemeIsBlockTheme(site) /** - * Queries the site's REST API to check whether the - * `wp-block-editor/v1/settings` and - * `wpcom/v2/editor-assets` routes are available, - * and fetches the current theme to determine if it - * is a block theme. All results are persisted so - * that [getSupportsEditorSettingsForSite], + * Probes the editor endpoints the GutenbergKit editor will hit + * (`wp-block-editor/v1/settings` and `wpcom/v2/editor-assets`) + * and fetches the current theme to determine if it is a block + * theme. All results are persisted so that + * [getSupportsEditorSettingsForSite], * [getSupportsEditorAssetsForSite], and - * [getThemeSupportsBlockStyles] return them - * synchronously on future calls. + * [getThemeSupportsBlockStyles] return them synchronously on + * future calls. * - * @return `true` when both checks complete without - * transport-level failures. + * Direct endpoint probing — rather than reading the API root's + * route advertisement — is required because the WP.com proxy + * advertises editor routes (e.g. + * `/wp-block-editor/v1/sites//settings`) even when the + * underlying site has not registered them and the actual endpoint + * returns 404. The probe also routes Atomic sites' settings probe + * to the direct host (see the MUST-NOT-REMOVE block below and + * #22879/#22883). + * + * @return `true` when every probe completes with a definitive + * answer (2xx/401/403 → Supported; 404 → Unsupported; theme + * fetched). `false` if any check failed with a transport error, + * so callers know to retry later. */ suspend fun fetchEditorCapabilitiesForSite( site: SiteModel ): Boolean = withContext(ioDispatcher) { val results = awaitAll( - async { fetchRouteSupport(site) }, + async { + probeAndPersist( + site, + namespace = "wp-block-editor/v1", + endpoint = "settings", + // MUST NOT REMOVE — see #22879 / #22883. + // + // GutenbergKit's `wp-block-editor/v1/settings` fetch goes + // to the DIRECT host (it bypasses the configured + // `siteApiRoot` for this one endpoint). On Atomic the + // WP.com proxy can advertise/serve this route while the + // direct host 404s it — probing the proxy here would + // false-positive and the editor would then 404 on the + // direct host (the original bug). Capability detection + // for this endpoint MUST follow the editor to the direct + // host on Atomic. Do not unify with the editor-assets + // probe below — editor-assets does go through the + // configured root, which is the proxy for WPCom-routed + // sites. + forceDirectHost = site.isWPComAtomic, + persist = appPrefsWrapper::setSiteSupportsEditorSettings, + ) + }, + async { + probeAndPersist( + site, + namespace = "wpcom/v2", + endpoint = "editor-assets", + forceDirectHost = false, + persist = appPrefsWrapper::setSiteSupportsEditorAssets, + ) + }, async { fetchThemeBlockStyleSupport(site) } ) results.all { it } } - @Suppress("TooGenericExceptionCaught") - 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) { - fetchRouteSupportViaDirectHostDiscovery(site) - } else { - fetchRouteSupportViaConfiguredClient(site) + private suspend fun probeAndPersist( + site: SiteModel, + namespace: String, + endpoint: String, + forceDirectHost: Boolean, + persist: (SiteModel, Boolean) -> Unit, + ): Boolean = when (probeEndpoint(site, namespace, endpoint, forceDirectHost)) { + ProbeResult.Supported -> { + persist(site, true) + true } - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - AppLog.e( - T.EDITOR, - "Failed to fetch route support" + - " for site=${site.name}", - e - ) - false - } - - private suspend fun fetchRouteSupportViaConfiguredClient( - site: SiteModel - ): Boolean { - val client = wpApiClientProvider.getWpApiClient(site) - val resolver = wpApiClientProvider.getApiUrlResolver(site) - val response = client.request { it.apiRoot().get() } - return if (response is WpRequestResult.Success) { - persistRouteSupport(site, response.response.data, resolver) + ProbeResult.Unsupported -> { + persist(site, false) true - } else { - false } + ProbeResult.Inconclusive -> false } - /** - * On WP.com Atomic sites the editor fetches `wp-block-editor/v1/settings` - * from the direct host — not the WP.com proxy — so detection has to - * match. Run REST API autodiscovery on the site URL so we don't have to - * assume the API lives at `/wp-json` (custom permalink structures or - * REST API paths would break that assumption), then use the routes list - * returned by discovery directly — no second request needed. - */ - private suspend fun fetchRouteSupportViaDirectHostDiscovery( - site: SiteModel - ): Boolean { - val discovery = wpLoginClient.apiDiscovery(site.url) - if (discovery !is ApiDiscoveryResult.Success) { - AppLog.w( + @Suppress("TooGenericExceptionCaught") + private suspend fun probeEndpoint( + site: SiteModel, + namespace: String, + endpoint: String, + forceDirectHost: Boolean, + ): ProbeResult { + val url = buildProbeUrl(site, namespace, endpoint, forceDirectHost) + val authHeader = editorAuthHeaderBuilder.build( + site, accountStore.accessToken + ) ?: return ProbeResult.Inconclusive + val request = Request.Builder() + .url(url) + .header(AUTHORIZATION_HEADER, authHeader) + .get() + .build() + return try { + executeProbe(request).use { response -> + when { + response.isSuccessful -> ProbeResult.Supported + // 401/403 mean the route exists but auth was rejected / + // insufficient — still "registered." Critical for the + // Atomic direct-host probe where the WPCom bearer token + // is not necessarily honored by the direct host. + response.code == HttpURLConnection.HTTP_UNAUTHORIZED || + response.code == HttpURLConnection.HTTP_FORBIDDEN -> + ProbeResult.Supported + response.code == HttpURLConnection.HTTP_NOT_FOUND -> + ProbeResult.Unsupported + else -> ProbeResult.Inconclusive + } + } + } catch (e: IOException) { + AppLog.e( + T.EDITOR, + "Probe failed for $namespace/$endpoint" + + " on site=${site.name}", + e + ) + ProbeResult.Inconclusive + } catch (e: IllegalArgumentException) { + AppLog.e( T.EDITOR, - "Direct-host API discovery failed for" + - " site=${site.name}: ${discovery::class.simpleName}" + "Probe URL invalid for $namespace/$endpoint" + + " on site=${site.name}: $url", + e ) - return false + ProbeResult.Inconclusive } - val resolver = wpApiClientProvider.urlResolverFor( - discovery.success.apiRootUrl - ) - persistRouteSupport(site, discovery.success.apiDetails, resolver) - return true } - private fun persistRouteSupport( + /** + * Wraps OkHttp's async [Call.enqueue] in a suspending function so + * that coroutine cancellation propagates to the in-flight HTTP + * call. Without this, a cancelled preload would leave the socket + * open until OkHttp's read timeout fires (up to ~60s). + */ + private suspend fun executeProbe(request: Request): Response = + suspendCancellableCoroutine { cont -> + val call = okHttpClient.newCall(request) + cont.invokeOnCancellation { call.cancel() } + call.enqueue(object : Callback { + override fun onResponse(call: Call, response: Response) { + if (cont.isActive) cont.resume(response) + else response.close() + } + override fun onFailure(call: Call, e: IOException) { + if (cont.isActive) cont.resumeWithException(e) + } + }) + } + + /** + * Mirrors the URL the editor would hit: WP.com-routed sites go + * through the proxy with a `/sites//` prefix; self-hosted + * sites go to the direct REST root. + * + * [forceDirectHost] overrides the proxy routing for endpoints + * where GutenbergKit bypasses `siteApiRoot` (currently only + * `wp-block-editor/v1/settings` on Atomic). See the + * MUST-NOT-REMOVE block in [fetchEditorCapabilitiesForSite]. + */ + private fun buildProbeUrl( site: SiteModel, - data: WpApiDetails, - resolver: ApiUrlResolver, - ) { - appPrefsWrapper.setSiteSupportsEditorSettings( - site, - data.hasRouteForEndpoint( - resolver, - "/wp-block-editor/v1", - "settings" - ) - ) - appPrefsWrapper.setSiteSupportsEditorAssets( - site, - data.hasRouteForEndpoint( - resolver, - "/wpcom/v2", - "editor-assets" - ) - ) + namespace: String, + endpoint: String, + forceDirectHost: Boolean, + ): String { + val applicationPassword = site.apiRestPasswordPlain + val shouldUseProxy = !forceDirectHost && + applicationPassword.isNullOrEmpty() && + site.isUsingWpComRestApi + val ns = namespace.trim('/') + val ep = endpoint.trim('/') + return if (shouldUseProxy) { + "$WPCOM_API_ROOT$ns/sites/${site.siteId}/$ep" + } else { + val apiRoot = site.wpApiRestUrl + ?.takeIf { it.isNotEmpty() } + ?: "${site.url}/wp-json/" + val withSlash = + if (apiRoot.endsWith('/')) apiRoot else "$apiRoot/" + "$withSlash$ns/$ep" + } } @Suppress("TooGenericExceptionCaught") @@ -214,4 +304,12 @@ class EditorSettingsRepository @Inject constructor( ) false } + + private enum class ProbeResult { Supported, Unsupported, Inconclusive } + + companion object { + private const val WPCOM_API_ROOT = + "https://public-api.wordpress.com/" + private const val AUTHORIZATION_HEADER = "Authorization" + } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilder.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilder.kt index 9399ddfe3f68..fd6c41cd0bba 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilder.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilder.kt @@ -1,9 +1,8 @@ package org.wordpress.android.ui.posts -import android.util.Base64 import org.wordpress.android.fluxc.model.PostImmutableModel import org.wordpress.android.fluxc.model.SiteModel -import org.wordpress.android.util.AppLog +import org.wordpress.android.repositories.EditorAuthHeaderBuilder import org.wordpress.android.util.PerAppLocaleManager import org.wordpress.gutenberg.model.EditorConfiguration import org.wordpress.gutenberg.model.PostTypeDetails @@ -15,6 +14,7 @@ import javax.inject.Singleton class GutenbergKitSettingsBuilder @Inject constructor( private val editorCapabilityResolver: EditorCapabilityResolver, private val perAppLocaleManager: PerAppLocaleManager, + private val editorAuthHeaderBuilder: EditorAuthHeaderBuilder, ) { fun buildPostConfiguration( site: SiteModel, @@ -33,7 +33,7 @@ class GutenbergKitSettingsBuilder @Inject constructor( site.wpApiRestUrl ?: "${site.url}/wp-json/" } - val authHeader = buildAuthHeader( + val authHeader = editorAuthHeaderBuilder.build( shouldUseWPComRestApi = shouldUseWPComRestApi, accessToken = accessToken, username = site.apiRestUsernamePlain, @@ -88,49 +88,6 @@ class GutenbergKitSettingsBuilder @Inject constructor( }.build() } - internal fun buildAuthHeader( - shouldUseWPComRestApi: Boolean, - accessToken: String?, - username: String?, - password: String? - ): String? { - return if (shouldUseWPComRestApi) { - if (!accessToken.isNullOrEmpty()) { - "$AUTH_BEARER_PREFIX$accessToken" - } else { - AppLog.w( - AppLog.T.EDITOR, - "Missing access token for WP.com REST API authentication" - ) - null - } - } else { - if (!username.isNullOrEmpty() && !password.isNullOrEmpty()) { - try { - val credentials = "$username:$password" - val encodedCredentials = Base64.encodeToString( - credentials.toByteArray(Charsets.UTF_8), - Base64.NO_WRAP - ) - "$AUTH_BASIC_PREFIX$encodedCredentials" - } catch (e: IllegalArgumentException) { - AppLog.e( - AppLog.T.EDITOR, - "Failed to encode Basic auth credentials", - e - ) - null - } - } else { - AppLog.w( - AppLog.T.EDITOR, - "Incomplete credentials for Basic authentication" - ) - null - } - } - } - internal fun buildSiteApiNamespace( shouldUseWPComRestApi: Boolean, siteId: Long, @@ -174,8 +131,6 @@ class GutenbergKitSettingsBuilder @Inject constructor( } companion object { - private const val AUTH_BEARER_PREFIX = "Bearer " - private const val AUTH_BASIC_PREFIX = "Basic " private const val WPCOM_API_ROOT = "https://public-api.wordpress.com/" } diff --git a/WordPress/src/test/java/org/wordpress/android/repositories/EditorAuthHeaderBuilderTest.kt b/WordPress/src/test/java/org/wordpress/android/repositories/EditorAuthHeaderBuilderTest.kt new file mode 100644 index 000000000000..be46dbd3608a --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorAuthHeaderBuilderTest.kt @@ -0,0 +1,179 @@ +package org.wordpress.android.repositories + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.junit.MockitoJUnitRunner +import org.wordpress.android.fluxc.model.SiteModel + +@RunWith(MockitoJUnitRunner::class) +class EditorAuthHeaderBuilderTest { + private val builder = EditorAuthHeaderBuilder() + + @Test + fun `WPCom site returns Bearer token header`() { + val header = builder.build( + shouldUseWPComRestApi = true, + accessToken = "my_token", + username = null, + password = null + ) + + assertThat(header).isEqualTo("Bearer my_token") + } + + @Test + fun `WPCom site with null token returns null`() { + val header = builder.build( + shouldUseWPComRestApi = true, + accessToken = null, + username = null, + password = null + ) + + assertThat(header).isNull() + } + + @Test + fun `WPCom site with empty token returns null`() { + val header = builder.build( + shouldUseWPComRestApi = true, + accessToken = "", + username = null, + password = null + ) + + assertThat(header).isNull() + } + + @Test + fun `self-hosted site returns Basic auth header`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "testuser", + password = "testpass" + ) + + assertThat(header).isNotNull() + assertThat(header).startsWith("Basic ") + } + + @Test + fun `Basic auth with null username returns null`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = null, + password = "password123" + ) + + assertThat(header).isNull() + } + + @Test + fun `Basic auth with empty username returns null`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "", + password = "password123" + ) + + assertThat(header).isNull() + } + + @Test + fun `Basic auth with null password returns null`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "username", + password = null + ) + + assertThat(header).isNull() + } + + @Test + fun `Basic auth with empty password returns null`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "username", + password = "" + ) + + assertThat(header).isNull() + } + + @Test + fun `Basic auth with both empty returns null`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "", + password = "" + ) + + assertThat(header).isNull() + } + + @Test + fun `special characters in Basic auth are encoded`() { + val header = builder.build( + shouldUseWPComRestApi = false, + accessToken = null, + username = "user@example.com", + password = "p@ss:word!123" + ) + + assertThat(header).isNotNull() + assertThat(header).startsWith("Basic ") + } + + // ===== Site-convenience overload ===== + + @Test + fun `site overload uses Bearer for WPCom-routed sites without app password`() { + val site = SiteModel().apply { + setIsWPCom(true) + setIsJetpackConnected(false) + origin = SiteModel.ORIGIN_WPCOM_REST + } + + val header = builder.build(site, accessToken = "wpcom_token") + + assertThat(header).isEqualTo("Bearer wpcom_token") + } + + @Test + fun `site overload uses Basic when application password is set`() { + val site = SiteModel().apply { + setIsWPCom(false) + setIsJetpackConnected(true) + origin = SiteModel.ORIGIN_WPCOM_REST + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" + } + + val header = builder.build(site, accessToken = "ignored") + + assertThat(header).startsWith("Basic ") + } + + @Test + fun `site overload prefers Basic over Bearer when app password is set on a WPCom-routed site`() { + val site = SiteModel().apply { + setIsWPCom(false) + setIsJetpackConnected(true) + origin = SiteModel.ORIGIN_WPCOM_REST + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" + } + + val header = builder.build(site, accessToken = "wpcom_token") + + assertThat(header).startsWith("Basic ") + } +} 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 96082cf8df82..9603a54ab3c0 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt @@ -1,29 +1,31 @@ package org.wordpress.android.repositories import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest +import okhttp3.Call +import okhttp3.Callback +import okhttp3.OkHttpClient +import okhttp3.Protocol +import okhttp3.Request +import okhttp3.Response +import okhttp3.ResponseBody.Companion.toResponseBody 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.argumentCaptor import org.mockito.kotlin.mock import org.mockito.kotlin.never 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.rest.wpapi.rs.WpApiClientProvider +import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.ui.prefs.AppPrefsWrapper -import rs.wordpress.api.kotlin.ApiDiscoveryResult -import rs.wordpress.api.kotlin.WpApiClient -import rs.wordpress.api.kotlin.WpLoginClient -import rs.wordpress.api.kotlin.WpRequestResult -import uniffi.wp_api.ApiRootRequestGetResponse -import uniffi.wp_api.ApiUrlResolver -import uniffi.wp_api.AutoDiscoveryAttemptSuccess -import uniffi.wp_api.DiscoveredAuthenticationMechanism -import uniffi.wp_api.ParseUrlException import uniffi.wp_api.ThemeAuthor import uniffi.wp_api.ThemeAuthorUri import uniffi.wp_api.ThemeDescription @@ -33,17 +35,10 @@ import uniffi.wp_api.ThemeStylesheet import uniffi.wp_api.ThemeTags import uniffi.wp_api.ThemeUri import uniffi.wp_api.ThemeWithEditContext -import uniffi.wp_api.WpApiDetails -import uniffi.wp_api.WpNetworkHeaderMap +import java.io.IOException @ExperimentalCoroutinesApi class EditorSettingsRepositoryTest : BaseUnitTest() { - @Mock - lateinit var wpApiClientProvider: WpApiClientProvider - - @Mock - lateinit var wpLoginClient: WpLoginClient - @Mock lateinit var appPrefsWrapper: AppPrefsWrapper @@ -51,65 +46,121 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { lateinit var themeRepository: ThemeRepository @Mock - lateinit var wpApiClient: WpApiClient + lateinit var accountStore: AccountStore @Mock - lateinit var apiUrlResolver: ApiUrlResolver - - @Mock - lateinit var directHostResolver: ApiUrlResolver + lateinit var okHttpClient: OkHttpClient private lateinit var repository: EditorSettingsRepository - private val testSite = SiteModel().apply { + private val wpComSite = SiteModel().apply { id = 1 - url = "https://test.wordpress.com" + siteId = 42L + url = "https://example.wordpress.com" + setIsWPCom(true) + setIsJetpackConnected(false) + origin = SiteModel.ORIGIN_WPCOM_REST + } + + private val atomicSite = SiteModel().apply { + id = 5 + siteId = 777L + url = "https://atomic.example.com" + setIsWPCom(true) + setIsWPComAtomic(true) + setIsJetpackConnected(false) + origin = SiteModel.ORIGIN_WPCOM_REST + wpApiRestUrl = "https://atomic.example.com/wp-json/" + } + + private val selfHostedSite = SiteModel().apply { + id = 2 + siteId = 0L + url = "https://mysite.com" + setIsWPCom(false) + setIsJetpackConnected(false) + wpApiRestUrl = "https://mysite.com/wp-json/" + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" } @Before fun setUp() { - whenever(wpApiClientProvider.getWpApiClient(testSite)) - .thenReturn(wpApiClient) - whenever(wpApiClientProvider.getApiUrlResolver(testSite)) - .thenReturn(apiUrlResolver) - + whenever(accountStore.accessToken).thenReturn("wpcom_token") repository = EditorSettingsRepository( - wpApiClientProvider = wpApiClientProvider, - wpLoginClient = wpLoginClient, - appPrefsWrapper = appPrefsWrapper, themeRepository = themeRepository, - ioDispatcher = testDispatcher() + appPrefsWrapper = appPrefsWrapper, + accountStore = accountStore, + editorAuthHeaderBuilder = EditorAuthHeaderBuilder(), + okHttpClient = okHttpClient, + ioDispatcher = testDispatcher(), ) } + // ===== Probe outcomes ===== + @Test - fun `fetch persists true when routes are present`() = - runTest { - mockApiRootResponse( - hasEditorSettings = true, - hasEditorAssets = true - ) - mockThemeResponse(isBlockTheme = false) - - val result = - repository.fetchEditorCapabilitiesForSite( - testSite - ) - - assertThat(result).isTrue() - verify(appPrefsWrapper) - .setSiteSupportsEditorSettings(testSite, true) - verify(appPrefsWrapper) - .setSiteSupportsEditorAssets(testSite, true) - } + fun `2xx on both endpoints persists true and returns true`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, true) + } + + @Test + fun `404 persists false but still returns true (definitive answer)`() = runTest { + stubResponses( + "wp-block-editor/v1" to 404, + "wpcom/v2" to 404, + ) + mockTheme(isBlockTheme = false) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, false) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, false) + } + + @Test + fun `mixed 200 and 404 persists per-endpoint`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 404, + ) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, false) + } @Test - fun `fetch does not persist routes on API error`() = runTest { - mockApiRootError() - mockThemeResponse(isBlockTheme = false) + fun `5xx response leaves pref untouched and returns false`() = runTest { + stubResponses( + "wp-block-editor/v1" to 500, + "wpcom/v2" to 500, + ) + mockTheme(isBlockTheme = true) - repository.fetchEditorCapabilitiesForSite(testSite) + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + assertThat(ok).isFalse() verify(appPrefsWrapper, never()) .setSiteSupportsEditorSettings(any(), any()) verify(appPrefsWrapper, never()) @@ -117,270 +168,416 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { } @Test - fun `fetch does not persist block theme when theme is null`() = - runTest { - mockApiRootResponse( - hasEditorSettings = true, - hasEditorAssets = true - ) - whenever( - themeRepository.fetchCurrentTheme(testSite) - ).thenReturn(null) - - repository.fetchEditorCapabilitiesForSite( - testSite - ) - - verify(appPrefsWrapper, never()) - .setSiteThemeIsBlockTheme(any(), any()) + fun `network IOException leaves pref untouched and returns false`() = runTest { + whenever(okHttpClient.newCall(any())).thenAnswer { + mock().also { call -> + whenever(call.enqueue(any())).thenAnswer { callInvocation -> + val callback = callInvocation.arguments[0] as Callback + callback.onFailure(call, IOException("offline")) + } + } } + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isFalse() + verify(appPrefsWrapper, never()) + .setSiteSupportsEditorSettings(any(), any()) + verify(appPrefsWrapper, never()) + .setSiteSupportsEditorAssets(any(), any()) + } + + @Test + fun `missing access token on WPCom site marks probe inconclusive`() = runTest { + whenever(accountStore.accessToken).thenReturn(null) + mockTheme(isBlockTheme = false) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isFalse() + verify(okHttpClient, never()).newCall(any()) + verify(appPrefsWrapper, never()) + .setSiteSupportsEditorSettings(any(), any()) + verify(appPrefsWrapper, never()) + .setSiteSupportsEditorAssets(any(), any()) + } + + // ===== Probe URL and headers ===== @Test - fun `route failure does not prevent theme update`() = - runTest { - whenever(wpApiClient.request(any())) - .thenThrow(RuntimeException("network error")) - mockThemeResponse(isBlockTheme = true) - - val result = - repository.fetchEditorCapabilitiesForSite( - testSite - ) - - assertThat(result).isFalse() - verify(appPrefsWrapper) - .setSiteThemeIsBlockTheme(testSite, true) + fun `WPCom site probes through proxy with sites prefix and Bearer auth`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(wpComSite) + + val requests = captureRequests() + val urls = requests.map { it.url.toString() } + assertThat(urls).contains( + "https://public-api.wordpress.com/wp-block-editor/v1/sites/42/settings", + "https://public-api.wordpress.com/wpcom/v2/sites/42/editor-assets", + ) + requests.forEach { + assertThat(it.header("Authorization")) + .isEqualTo("Bearer wpcom_token") } + } @Test - fun `theme failure does not prevent route update`() = - runTest { - mockApiRootResponse( - hasEditorSettings = true, - hasEditorAssets = true - ) - whenever( - themeRepository.fetchCurrentTheme(testSite) - ).thenThrow(RuntimeException("network error")) - - val result = - repository.fetchEditorCapabilitiesForSite( - testSite - ) - - assertThat(result).isFalse() - verify(appPrefsWrapper) - .setSiteSupportsEditorSettings(testSite, true) - verify(appPrefsWrapper) - .setSiteSupportsEditorAssets(testSite, true) + fun `self-hosted site probes wpApiRestUrl directly with Basic auth`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(selfHostedSite) + + val requests = captureRequests() + val urls = requests.map { it.url.toString() } + assertThat(urls).contains( + "https://mysite.com/wp-json/wp-block-editor/v1/settings", + "https://mysite.com/wp-json/wpcom/v2/editor-assets", + ) + requests.forEach { + assertThat(it.header("Authorization")) + .startsWith("Basic ") } + } @Test - fun `both failures returns false without writing prefs`() = - runTest { - whenever(wpApiClient.request(any())) - .thenThrow(RuntimeException("network error")) - whenever( - themeRepository.fetchCurrentTheme(testSite) - ).thenThrow(RuntimeException("network error")) - - val result = - repository.fetchEditorCapabilitiesForSite( - testSite - ) - - assertThat(result).isFalse() - verify(appPrefsWrapper, never()) - .setSiteSupportsEditorSettings(any(), any()) - verify(appPrefsWrapper, never()) - .setSiteSupportsEditorAssets(any(), any()) - verify(appPrefsWrapper, never()) - .setSiteThemeIsBlockTheme(any(), any()) + fun `self-hosted site without wpApiRestUrl falls back to siteUrl wp-json`() = runTest { + val site = SiteModel().apply { + id = 3 + url = "https://fallback.example" + setIsWPCom(false) + setIsJetpackConnected(false) + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" + // no wpApiRestUrl set } + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(site) + + val urls = captureRequests().map { it.url.toString() } + assertThat(urls).contains( + "https://fallback.example/wp-json/wp-block-editor/v1/settings", + "https://fallback.example/wp-json/wpcom/v2/editor-assets", + ) + } @Test - fun `atomic site probes via api discovery`() = - runTest { - val atomicSite = SiteModel().apply { - id = 2 - url = "https://atomic.example.com" - setIsWPCom(true) - setIsWPComAtomic(true) - } - mockDiscoverySuccess( - siteUrl = atomicSite.url, - hasEditorSettings = false, - hasEditorAssets = false - ) - whenever(themeRepository.fetchCurrentTheme(atomicSite)) - .thenReturn(buildTheme(isBlockTheme = false)) - - val result = - repository.fetchEditorCapabilitiesForSite(atomicSite) - - assertThat(result).isTrue() - verify(appPrefsWrapper) - .setSiteSupportsEditorSettings(atomicSite, false) - verify(appPrefsWrapper) - .setSiteSupportsEditorAssets(atomicSite, false) - verify(wpApiClientProvider, never()).getWpApiClient(atomicSite) + fun `app password on WPCom-routed site forces direct probe with Basic auth`() = runTest { + val site = SiteModel().apply { + id = 4 + siteId = 99L + url = "https://atomic.example" + setIsWPCom(false) + setIsJetpackConnected(true) + origin = SiteModel.ORIGIN_WPCOM_REST + wpApiRestUrl = "https://atomic.example/wp-json/" + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" + } + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(site) + + val requests = captureRequests() + val urls = requests.map { it.url.toString() } + assertThat(urls).contains( + "https://atomic.example/wp-json/wp-block-editor/v1/settings", + "https://atomic.example/wp-json/wpcom/v2/editor-assets", + ) + requests.forEach { + assertThat(it.header("Authorization")) + .startsWith("Basic ") } + } + // ===== Atomic direct-host routing (MUST NOT REMOVE — see #22879/#22883) ===== + + /** + * Atomic sites: GutenbergKit fetches `wp-block-editor/v1/settings` from + * the DIRECT host, bypassing the configured `siteApiRoot`. Capability + * detection for this endpoint MUST hit the same host. Probing the WP.com + * proxy here would false-positive on Atomic sites where the proxy + * advertises a route the direct host doesn't actually serve — the editor + * would then 404 trying to fetch from the direct host. + * + * If this test fails, the production fix from #22883 has regressed. + * Restore the direct-host routing rather than weakening the assertion. + */ @Test - fun `atomic site returns false when discovery fails`() = - runTest { - val atomicSite = SiteModel().apply { - id = 4 - url = "https://atomic.example.com" - setIsWPCom(true) - setIsWPComAtomic(true) - } - whenever(wpLoginClient.apiDiscovery(atomicSite.url)) - .thenReturn( - ApiDiscoveryResult.FailureParseSiteUrl( - ParseUrlException.Generic("") - ) - ) - whenever(themeRepository.fetchCurrentTheme(atomicSite)) - .thenReturn(buildTheme(isBlockTheme = false)) - - val result = - repository.fetchEditorCapabilitiesForSite(atomicSite) - - assertThat(result).isFalse() - verify(appPrefsWrapper, never()) - .setSiteSupportsEditorSettings(any(), any()) - verify(appPrefsWrapper, never()) - .setSiteSupportsEditorAssets(any(), any()) - verify(wpApiClientProvider, never()).getWpApiClient(atomicSite) + fun `MUST_NOT_REMOVE - atomic settings probe goes to the direct host, not the proxy`() = runTest { + stubResponses( + "atomic.example.com" to 200, + "public-api.wordpress.com" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(atomicSite) + + val settingsRequests = captureRequests().filter { + it.url.toString().endsWith("wp-block-editor/v1/settings") || + it.url.toString().endsWith("/settings") } + assertThat(settingsRequests).isNotEmpty + settingsRequests.forEach { + assertThat(it.url.toString()) + .`as`("Atomic settings probe must hit the direct host") + .isEqualTo("https://atomic.example.com/wp-json/wp-block-editor/v1/settings") + assertThat(it.url.host) + .`as`("Atomic settings probe must NOT go through the WP.com proxy") + .isNotEqualTo("public-api.wordpress.com") + } + } @Test - fun `atomic site with app password also probes via api discovery`() = - runTest { - val atomicSite = SiteModel().apply { - id = 3 - url = "https://atomic.example.com" - setIsWPCom(true) - setIsWPComAtomic(true) - apiRestUsernamePlain = "user" - apiRestPasswordPlain = "secret" - } - mockDiscoverySuccess( - siteUrl = atomicSite.url, - hasEditorSettings = true, - hasEditorAssets = true - ) - whenever(themeRepository.fetchCurrentTheme(atomicSite)) - .thenReturn(buildTheme(isBlockTheme = false)) - - val result = - repository.fetchEditorCapabilitiesForSite(atomicSite) - - assertThat(result).isTrue() - verify(appPrefsWrapper) - .setSiteSupportsEditorSettings(atomicSite, true) - verify(appPrefsWrapper) - .setSiteSupportsEditorAssets(atomicSite, true) - verify(wpApiClientProvider, never()).getWpApiClient(atomicSite) + fun `atomic editor-assets probe still goes through the proxy (only settings is special)`() = runTest { + stubResponses( + "atomic.example.com" to 200, + "public-api.wordpress.com" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(atomicSite) + + val assetsRequests = captureRequests().filter { + it.url.toString().contains("editor-assets") + } + assertThat(assetsRequests).isNotEmpty + assetsRequests.forEach { + assertThat(it.url.toString()) + .isEqualTo("https://public-api.wordpress.com/wpcom/v2/sites/777/editor-assets") } + } - private suspend fun mockApiRootResponse( - hasEditorSettings: Boolean, - hasEditorAssets: Boolean - ) = mockApiRootResponseFor( - client = wpApiClient, - resolver = apiUrlResolver, - hasEditorSettings = hasEditorSettings, - hasEditorAssets = hasEditorAssets, - ) + @Test + fun `non-atomic WPCom site keeps proxy routing for settings`() = runTest { + // Regression check: only Atomic gets the direct-host override. + stubResponses( + "public-api.wordpress.com" to 200, + ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(wpComSite) + + val urls = captureRequests().map { it.url.toString() } + assertThat(urls).contains( + "https://public-api.wordpress.com/wp-block-editor/v1/sites/42/settings", + ) + } + + // ===== 401/403 handling (route exists, auth rejected) ===== + + @Test + fun `401 response is treated as Supported (route exists, auth required)`() = runTest { + stubResponses( + "wp-block-editor/v1" to 401, + "wpcom/v2" to 401, + ) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, true) + } - private suspend fun mockDiscoverySuccess( - siteUrl: String, - hasEditorSettings: Boolean, - hasEditorAssets: Boolean, - ) { - val apiDetails = mock() - whenever( - apiDetails.hasRouteForEndpoint( - directHostResolver, - "/wp-block-editor/v1", - "settings" - ) - ).thenReturn(hasEditorSettings) - whenever( - apiDetails.hasRouteForEndpoint( - directHostResolver, - "/wpcom/v2", - "editor-assets" - ) - ).thenReturn(hasEditorAssets) - val apiRootUrl = mock() - whenever(wpApiClientProvider.urlResolverFor(apiRootUrl)) - .thenReturn(directHostResolver) - val success = AutoDiscoveryAttemptSuccess( - parsedSiteUrl = mock(), - apiRootUrl = apiRootUrl, - apiDetails = apiDetails, - authentication = DiscoveredAuthenticationMechanism - .ApplicationPasswords(mock()), + @Test + fun `403 response is treated as Supported (route exists, lacks permission)`() = runTest { + stubResponses( + "wp-block-editor/v1" to 403, + "wpcom/v2" to 403, ) - whenever(wpLoginClient.apiDiscovery(siteUrl)) - .thenReturn(ApiDiscoveryResult.Success(success)) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isTrue() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, true) } - @Suppress("UNCHECKED_CAST") - private suspend fun mockApiRootResponseFor( - client: WpApiClient, - resolver: ApiUrlResolver, - hasEditorSettings: Boolean, - hasEditorAssets: Boolean - ) { - val apiDetails = mock() - whenever( - apiDetails.hasRouteForEndpoint( - resolver, - "/wp-block-editor/v1", - "settings" - ) - ).thenReturn(hasEditorSettings) - whenever( - apiDetails.hasRouteForEndpoint( - resolver, - "/wpcom/v2", - "editor-assets" - ) - ).thenReturn(hasEditorAssets) - - val response = ApiRootRequestGetResponse( - data = apiDetails, - headerMap = mock() + // ===== Theme block-style support ===== + + @Test + fun `theme is fetched and persisted independently of probes`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, ) - whenever(client.request(any())) - .thenReturn( - WpRequestResult.Success(response) - as WpRequestResult - ) + mockTheme(isBlockTheme = true) + + repository.fetchEditorCapabilitiesForSite(wpComSite) + + verify(appPrefsWrapper) + .setSiteThemeIsBlockTheme(wpComSite, true) } - @Suppress("UNCHECKED_CAST") - private suspend fun mockApiRootError() { - val error = WpRequestResult.UnknownError( - statusCode = 500u, - response = "Internal Server Error", - requestUrl = "https://test.wordpress.com/wp-json", - requestMethod = uniffi.wp_api.RequestMethod.GET + @Test + fun `null theme is not persisted`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, ) - whenever(wpApiClient.request(any())) - .thenReturn(error) + whenever(themeRepository.fetchCurrentTheme(wpComSite)) + .thenReturn(null) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isFalse() + verify(appPrefsWrapper, never()) + .setSiteThemeIsBlockTheme(any(), any()) + } + + @Test + fun `theme failure does not block probe persistence`() = runTest { + stubResponses( + "wp-block-editor/v1" to 200, + "wpcom/v2" to 200, + ) + whenever(themeRepository.fetchCurrentTheme(wpComSite)) + .thenThrow(RuntimeException("network error")) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isFalse() + verify(appPrefsWrapper) + .setSiteSupportsEditorSettings(wpComSite, true) + verify(appPrefsWrapper) + .setSiteSupportsEditorAssets(wpComSite, true) + } + + @Test + fun `probe failure does not block theme persistence`() = runTest { + stubResponses( + "wp-block-editor/v1" to 500, + "wpcom/v2" to 500, + ) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(wpComSite) + + assertThat(ok).isFalse() + verify(appPrefsWrapper) + .setSiteThemeIsBlockTheme(wpComSite, true) + } + + // ===== Cancellation ===== + + @Test + fun `cancelling the probe coroutine cancels in-flight OkHttp calls`() = runTest { + val capturedCalls = mutableListOf() + whenever(okHttpClient.newCall(any())).thenAnswer { + mock().also { call -> + // enqueue() never invokes the callback — the call sits "in flight" + // until something cancels it. + whenever(call.enqueue(any())).then { /* no-op */ } + capturedCalls.add(call) + } + } + mockTheme(isBlockTheme = true) + + val job = launch { + repository.fetchEditorCapabilitiesForSite(wpComSite) + } + advanceUntilIdle() + job.cancelAndJoin() + + // Cancellation propagated to OkHttp. + assertThat(capturedCalls).hasSize(2) + capturedCalls.forEach { verify(it).cancel() } + + // Coroutine actually ended in a cancelled state — not still suspended + // (a buggy invokeOnCancellation could leak the continuation) and not + // completed normally (the suspension must have observed cancellation). + assertThat(job.isCancelled).isTrue + assertThat(job.isCompleted).isTrue + } + + // ===== getSupportsEditorSettingsForSite gating ===== + + @Test + fun `getSupports honours probe result of true once probe has run`() { + whenever(appPrefsWrapper.hasSiteEditorCapabilities(wpComSite)) + .thenReturn(true) + whenever(appPrefsWrapper.getSiteSupportsEditorSettings(wpComSite)) + .thenReturn(true) + + assertThat(repository.getSupportsEditorSettingsForSite(wpComSite)) + .isTrue() + } + + @Test + fun `getSupports honours probe result of false once probe has run`() { + whenever(appPrefsWrapper.hasSiteEditorCapabilities(wpComSite)) + .thenReturn(true) + whenever(appPrefsWrapper.getSiteSupportsEditorSettings(wpComSite)) + .thenReturn(false) + + // Probe is authoritative — even if a legacy SQL row would otherwise + // suggest supported, the probe's `false` wins. This is the fix for + // sites where the endpoint was once available but no longer is. + assertThat(repository.getSupportsEditorSettingsForSite(wpComSite)) + .isFalse() + } + + // ===== Helpers ===== + + private fun stubResponses(vararg matches: Pair) { + whenever(okHttpClient.newCall(any())).thenAnswer { invocation -> + val request = invocation.arguments[0] as Request + val url = request.url.toString() + val code = matches.firstOrNull { url.contains(it.first) }?.second + ?: error("No stubbed response for URL: $url") + mock().also { call -> + whenever(call.enqueue(any())).thenAnswer { callInvocation -> + val callback = callInvocation.arguments[0] as Callback + callback.onResponse(call, buildResponse(request, code)) + } + } + } + } + + private fun buildResponse(request: Request, code: Int): Response = + Response.Builder() + .request(request) + .protocol(Protocol.HTTP_1_1) + .code(code) + .message(if (code in 200..299) "OK" else "Error") + .body("".toResponseBody(null)) + .build() + + private fun captureRequests(): List { + val captor = argumentCaptor() + verify(okHttpClient, org.mockito.kotlin.atLeastOnce()) + .newCall(captor.capture()) + return captor.allValues } - private suspend fun mockThemeResponse( - isBlockTheme: Boolean - ) { - whenever( - themeRepository.fetchCurrentTheme(testSite) - ).thenReturn(buildTheme(isBlockTheme = isBlockTheme)) + private suspend fun mockTheme(isBlockTheme: Boolean) { + whenever(themeRepository.fetchCurrentTheme(any())) + .thenReturn(buildTheme(isBlockTheme = isBlockTheme)) } private fun buildTheme( diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilderTest.kt index 36beaf052211..288569b4178c 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/GutenbergKitSettingsBuilderTest.kt @@ -10,6 +10,7 @@ import org.mockito.kotlin.any import org.mockito.kotlin.whenever import org.wordpress.android.fluxc.model.PostModel import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.repositories.EditorAuthHeaderBuilder import org.wordpress.android.util.PerAppLocaleManager import org.wordpress.gutenberg.model.PostTypeDetails import java.util.Locale @@ -23,7 +24,11 @@ class GutenbergKitSettingsBuilderTest { lateinit var perAppLocaleManager: PerAppLocaleManager private val builder by lazy { - GutenbergKitSettingsBuilder(editorCapabilityResolver, perAppLocaleManager) + GutenbergKitSettingsBuilder( + editorCapabilityResolver, + perAppLocaleManager, + EditorAuthHeaderBuilder(), + ) } @Before @@ -35,130 +40,6 @@ class GutenbergKitSettingsBuilderTest { whenever(perAppLocaleManager.getCurrentLocale()).thenReturn(Locale.ENGLISH) } - // ===== Auth Header Tests ===== - - @Test - fun `WPCom site returns Bearer token header`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = true, - accessToken = "my_token", - username = null, - password = null - ) - - assertThat(header).isEqualTo("Bearer my_token") - } - - @Test - fun `WPCom site with null token returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = true, - accessToken = null, - username = null, - password = null - ) - - assertThat(header).isNull() - } - - @Test - fun `WPCom site with empty token returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = true, - accessToken = "", - username = null, - password = null - ) - - assertThat(header).isNull() - } - - @Test - fun `self-hosted site returns Basic auth header`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "testuser", - password = "testpass" - ) - - assertThat(header).isNotNull() - assertThat(header).startsWith("Basic ") - } - - @Test - fun `Basic auth with null username returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = null, - password = "password123" - ) - - assertThat(header).isNull() - } - - @Test - fun `Basic auth with empty username returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "", - password = "password123" - ) - - assertThat(header).isNull() - } - - @Test - fun `Basic auth with null password returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "username", - password = null - ) - - assertThat(header).isNull() - } - - @Test - fun `Basic auth with empty password returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "username", - password = "" - ) - - assertThat(header).isNull() - } - - @Test - fun `Basic auth with both empty returns null`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "", - password = "" - ) - - assertThat(header).isNull() - } - - @Test - fun `special characters in Basic auth are encoded`() { - val header = builder.buildAuthHeader( - shouldUseWPComRestApi = false, - accessToken = null, - username = "user@example.com", - password = "p@ss:word!123" - ) - - assertThat(header).isNotNull() - assertThat(header).startsWith("Basic ") - } - // ===== Site API Namespace Tests ===== @Test From 6591c7db85f5110d3c88ccf91c8bf4df9bd80841 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Mon, 1 Jun 2026 11:39:40 -0600 Subject: [PATCH 2/2] Harden capability probes: gate Atomic on app password, add timeout + retry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the endpoint-probe approach: - Atomic settings probe now requires an application password. The direct host doesn't honor the WP.com bearer, and we don't want to send WP.com credentials to whatever host `wpApiRestUrl` points at. Atomic sites that haven't minted an app password yet (e.g. before the headless mint completes) return Inconclusive and leave the cached pref alone until the next preload. - Each probe attempt is bounded by a 5s timeout (instead of inheriting the REGULAR client's 60s read timeout) and retried once on a transport-level Inconclusive — capability detection gates editor preload, so a single blip would otherwise leave the editor degraded until the next preload runs. - Comment why only `sites//` is probed, not the `sites//` fallback GutenbergKit also emits: the WP.com proxy resolves both forms through the same backend, so a 404 on one is a 404 on the other. Tests: MUST_NOT_REMOVE now uses an Atomic site with an app password and asserts Basic (not Bearer) on the direct-host probe; new test pins the no-app-password skip; cancellation test switched to runCurrent() so the new timeout doesn't fire on virtual-time advance. --- .../repositories/EditorSettingsRepository.kt | 104 +++++++++++++----- .../EditorSettingsRepositoryTest.kt | 50 ++++++++- 2 files changed, 123 insertions(+), 31 deletions(-) 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 fc804cf7f58b..7354135e2764 100644 --- a/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/repositories/EditorSettingsRepository.kt @@ -6,6 +6,7 @@ import kotlinx.coroutines.async import kotlinx.coroutines.awaitAll import kotlinx.coroutines.suspendCancellableCoroutine import kotlinx.coroutines.withContext +import kotlinx.coroutines.withTimeoutOrNull import okhttp3.Call import okhttp3.Callback import okhttp3.OkHttpClient @@ -130,6 +131,11 @@ class EditorSettingsRepository @Inject constructor( // probe below — editor-assets does go through the // configured root, which is the proxy for WPCom-routed // sites. + // + // `probeEndpoint` additionally requires an application + // password before sending a direct-host request, so this + // probe is skipped (Inconclusive) on Atomic sites that + // haven't minted one yet. forceDirectHost = site.isWPComAtomic, persist = appPrefsWrapper::setSiteSupportsEditorSettings, ) @@ -166,13 +172,22 @@ class EditorSettingsRepository @Inject constructor( ProbeResult.Inconclusive -> false } - @Suppress("TooGenericExceptionCaught") private suspend fun probeEndpoint( site: SiteModel, namespace: String, endpoint: String, forceDirectHost: Boolean, ): ProbeResult { + // Direct-host probes (Atomic settings) require Basic auth via + // the application password. We don't fall back to the WPCom + // bearer: the direct host doesn't honor it, and we don't want + // to send WPCom credentials to whatever host `wpApiRestUrl` + // points at. Atomic sites without an app password yet (e.g. + // before the headless mint completes) leave the cached pref + // alone until the next preload. + if (forceDirectHost && site.apiRestPasswordPlain.isNullOrEmpty()) { + return ProbeResult.Inconclusive + } val url = buildProbeUrl(site, namespace, endpoint, forceDirectHost) val authHeader = editorAuthHeaderBuilder.build( site, accountStore.accessToken @@ -182,39 +197,67 @@ class EditorSettingsRepository @Inject constructor( .header(AUTHORIZATION_HEADER, authHeader) .get() .build() - return try { - executeProbe(request).use { response -> + // One cheap retry on transport failure. Capability detection + // gates editor preload, and a single blip would otherwise + // leave the editor degraded until the next preload runs. + val first = attemptProbe(request, namespace, endpoint, site) + return if (first == ProbeResult.Inconclusive) { + attemptProbe(request, namespace, endpoint, site) + } else { + first + } + } + + @Suppress("TooGenericExceptionCaught") + private suspend fun attemptProbe( + request: Request, + namespace: String, + endpoint: String, + site: SiteModel, + ): ProbeResult = try { + val response = withTimeoutOrNull(PROBE_TIMEOUT_MS) { + executeProbe(request) + } + if (response == null) { + AppLog.w( + T.EDITOR, + "Probe timed out after ${PROBE_TIMEOUT_MS}ms for" + + " $namespace/$endpoint on site=${site.name}" + ) + ProbeResult.Inconclusive + } else { + response.use { when { - response.isSuccessful -> ProbeResult.Supported - // 401/403 mean the route exists but auth was rejected / - // insufficient — still "registered." Critical for the - // Atomic direct-host probe where the WPCom bearer token - // is not necessarily honored by the direct host. - response.code == HttpURLConnection.HTTP_UNAUTHORIZED || - response.code == HttpURLConnection.HTTP_FORBIDDEN -> + it.isSuccessful -> ProbeResult.Supported + // 401/403 mean the route exists but auth was rejected + // or insufficient — still "registered." On Atomic this + // covers a stale or rotated application password, so + // capability detection stays definitive. + it.code == HttpURLConnection.HTTP_UNAUTHORIZED || + it.code == HttpURLConnection.HTTP_FORBIDDEN -> ProbeResult.Supported - response.code == HttpURLConnection.HTTP_NOT_FOUND -> + it.code == HttpURLConnection.HTTP_NOT_FOUND -> ProbeResult.Unsupported else -> ProbeResult.Inconclusive } } - } catch (e: IOException) { - AppLog.e( - T.EDITOR, - "Probe failed for $namespace/$endpoint" + - " on site=${site.name}", - e - ) - ProbeResult.Inconclusive - } catch (e: IllegalArgumentException) { - AppLog.e( - T.EDITOR, - "Probe URL invalid for $namespace/$endpoint" + - " on site=${site.name}: $url", - e - ) - ProbeResult.Inconclusive } + } catch (e: IOException) { + AppLog.e( + T.EDITOR, + "Probe failed for $namespace/$endpoint" + + " on site=${site.name}", + e + ) + ProbeResult.Inconclusive + } catch (e: IllegalArgumentException) { + AppLog.e( + T.EDITOR, + "Probe URL invalid for $namespace/$endpoint" + + " on site=${site.name}: ${request.url}", + e + ) + ProbeResult.Inconclusive } /** @@ -261,6 +304,12 @@ class EditorSettingsRepository @Inject constructor( val ns = namespace.trim('/') val ep = endpoint.trim('/') return if (shouldUseProxy) { + // `GutenbergKitSettingsBuilder` hands GutenbergKit two + // namespace prefixes for WPCom-routed sites — `sites//` + // and `sites//` — so the editor can fall back if one + // form doesn't resolve. We probe only `sites//` because + // the WP.com proxy resolves both forms through the same + // backend code path; a 404 on one is a 404 on the other. "$WPCOM_API_ROOT$ns/sites/${site.siteId}/$ep" } else { val apiRoot = site.wpApiRestUrl @@ -311,5 +360,6 @@ class EditorSettingsRepository @Inject constructor( private const val WPCOM_API_ROOT = "https://public-api.wordpress.com/" private const val AUTHORIZATION_HEADER = "Authorization" + private const val PROBE_TIMEOUT_MS = 5_000L } } 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 9603a54ab3c0..897e9a95c02c 100644 --- a/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/repositories/EditorSettingsRepositoryTest.kt @@ -3,7 +3,7 @@ package org.wordpress.android.repositories import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.cancelAndJoin import kotlinx.coroutines.launch -import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import okhttp3.Call import okhttp3.Callback @@ -322,14 +322,26 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { * Restore the direct-host routing rather than weakening the assertion. */ @Test - fun `MUST_NOT_REMOVE - atomic settings probe goes to the direct host, not the proxy`() = runTest { + fun `MUST_NOT_REMOVE - atomic settings probe goes to the direct host with Basic auth`() = runTest { + val atomicWithAppPassword = SiteModel().apply { + id = 5 + siteId = 777L + url = "https://atomic.example.com" + setIsWPCom(true) + setIsWPComAtomic(true) + setIsJetpackConnected(false) + origin = SiteModel.ORIGIN_WPCOM_REST + wpApiRestUrl = "https://atomic.example.com/wp-json/" + apiRestUsernamePlain = "admin" + apiRestPasswordPlain = "app_pass" + } stubResponses( "atomic.example.com" to 200, "public-api.wordpress.com" to 200, ) mockTheme(isBlockTheme = true) - repository.fetchEditorCapabilitiesForSite(atomicSite) + repository.fetchEditorCapabilitiesForSite(atomicWithAppPassword) val settingsRequests = captureRequests().filter { it.url.toString().endsWith("wp-block-editor/v1/settings") || @@ -343,7 +355,33 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { assertThat(it.url.host) .`as`("Atomic settings probe must NOT go through the WP.com proxy") .isNotEqualTo("public-api.wordpress.com") + assertThat(it.header("Authorization")) + .`as`("Atomic direct-host probe must use Basic, not the WPCom bearer") + .startsWith("Basic ") + } + } + + @Test + fun `atomic site without application password skips the settings probe`() = runTest { + // The direct-host probe requires Basic auth. We don't fall back to + // the WPCom bearer (it isn't honored by the direct host anyway, and + // we don't want to send WPCom credentials to whatever host + // `wpApiRestUrl` points at). Until the application password mints, + // the probe is Inconclusive and the cached pref is left alone. + stubResponses( + "public-api.wordpress.com" to 200, + ) + mockTheme(isBlockTheme = true) + + val ok = repository.fetchEditorCapabilitiesForSite(atomicSite) + + assertThat(ok).isFalse() + val settingsRequests = captureRequests().filter { + it.url.toString().contains("wp-block-editor/v1/settings") } + assertThat(settingsRequests).isEmpty() + verify(appPrefsWrapper, never()) + .setSiteSupportsEditorSettings(any(), any()) } @Test @@ -501,7 +539,11 @@ class EditorSettingsRepositoryTest : BaseUnitTest() { val job = launch { repository.fetchEditorCapabilitiesForSite(wpComSite) } - advanceUntilIdle() + // `runCurrent` lets the probes start and suspend at their HTTP + // calls without advancing virtual time — otherwise the + // `withTimeoutOrNull` inside the probe would fire and complete + // the job naturally before we got a chance to cancel it. + runCurrent() job.cancelAndJoin() // Cancellation propagated to OkHttp.