Skip to content

Commit e6f795f

Browse files
authored
CMM-2304: Detect Jetpack status on the application-password path (#23262)
* CMM-2304: Detect Jetpack status on the application-password path * Address review findings on the Jetpack detection Resolve the stored row before dispatching a site fetch, carry it into every WPAPI fetch as previousSite (one carry-forward mechanism with fresh-over-stale precedence), yield a blog id another row already owns before writing instead of retrying after the constraint fires, treat non-null detection results as definitive, and reuse one WpRequestExecutor in the connection fetcher. * Derive the Jetpack connection state from the blog id Collapse the three identical JetpackConnectionState constructions into one and derive isConnected from wpComSiteId, making the null-iff- disconnected invariant structural instead of documented. * Restructure the connection-state guard to satisfy detekt's ReturnCount * Fix unit-test failures from the review fixes Replace the UrlUtils scheme handling in storedWPAPISite with plain string checks (UrlUtils touches unmocked Android APIs under plain JUnit), and let the fetchSite error tests tolerate the intended stored-row read while still asserting nothing is written. * Treat an absent namespaces field as unknown, not as no Jetpack An absent namespaces list now carries the stored Jetpack state forward instead of definitively clearing it, and the stored-row lookup tries both schemes even when the caller's URL already has one.
1 parent b9bfaf6 commit e6f795f

6 files changed

Lines changed: 300 additions & 13 deletions

File tree

RELEASE-NOTES.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313
* [*] Sharing content to the app now lists self-hosted sites connected with an application password.
1414
* [**] Images and videos shared to the app from the photo picker now upload instead of being silently dropped.
1515
* [**] 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]
16+
* [**] 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.
1617

1718
26.9
1819
-----
Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,91 @@
1+
package org.wordpress.android.fluxc.network.rest.wpapi.jetpack
2+
3+
import okhttp3.Interceptor
4+
import org.wordpress.android.fluxc.module.OkHttpClientQualifiers
5+
import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpNetworkAvailabilityProvider
6+
import org.wordpress.android.fluxc.utils.AppLogWrapper
7+
import org.wordpress.android.util.AppLog
8+
import rs.wordpress.api.kotlin.EmptyAppNotifier
9+
import rs.wordpress.api.kotlin.WpRequestExecutor
10+
import uniffi.wp_api.JetpackConnectionClient
11+
import uniffi.wp_api.JetpackConnectionStatus
12+
import uniffi.wp_api.ParsedUrl
13+
import uniffi.wp_api.WpApiClientDelegate
14+
import uniffi.wp_api.WpApiMiddlewarePipeline
15+
import uniffi.wp_api.WpAuthenticationProvider
16+
import javax.inject.Inject
17+
import javax.inject.Named
18+
import javax.inject.Singleton
19+
20+
/**
21+
* A self-hosted site's Jetpack connection to WordPress.com. [wpComSiteId] is the blog ID WordPress.com
22+
* assigned the site when it connected, and is null when it isn't connected.
23+
*/
24+
data class JetpackConnectionState(val wpComSiteId: Long?) {
25+
val isConnected: Boolean get() = wpComSiteId != null
26+
}
27+
28+
/**
29+
* Reads a self-hosted site's Jetpack connection state over wordpress-rs.
30+
*
31+
* Only the Jetpack plugin knows whether a site is connected to WordPress.com and which blog ID it was
32+
* given. Sites added with an application password have no other source for either: the WP.com REST site
33+
* payload that populates those fields for Jetpack sites is never fetched for them.
34+
*/
35+
@Singleton
36+
class JetpackConnectionStatusFetcher @Inject constructor(
37+
@Named(OkHttpClientQualifiers.INTERCEPTORS) interceptors: Set<@JvmSuppressWildcards Interceptor>,
38+
networkAvailabilityProvider: WpNetworkAvailabilityProvider,
39+
private val appLogWrapper: AppLogWrapper
40+
) {
41+
// One executor for the singleton: each WpRequestExecutor builds its own OkHttpClient (with its own
42+
// connection pool), and the executor is site-independent -- auth lives in the per-call delegate.
43+
private val requestExecutor = WpRequestExecutor(
44+
interceptors = interceptors.toList(),
45+
networkAvailabilityProvider = networkAvailabilityProvider
46+
)
47+
48+
/**
49+
* Returns the site's Jetpack connection state, or null when it can't be determined — the site is
50+
* unreachable, the endpoint isn't there, or Jetpack is older than 14.2, which is where these
51+
* endpoints were added. Callers should read null as "unchanged", not as "not connected".
52+
*/
53+
@Suppress("TooGenericExceptionCaught")
54+
suspend fun fetch(
55+
apiRootUrl: String,
56+
username: String,
57+
password: String
58+
): JetpackConnectionState? = try {
59+
buildClient(apiRootUrl, username, password).use { client ->
60+
val blogId = when (val status = client.status()) {
61+
is JetpackConnectionStatus.NotConnected -> null
62+
is JetpackConnectionStatus.Site -> status.blogId
63+
is JetpackConnectionStatus.User -> status.blogId
64+
}
65+
JetpackConnectionState(wpComSiteId = blogId?.toLong())
66+
}
67+
} catch (e: Exception) {
68+
appLogWrapper.d(AppLog.T.API, "$TAG: couldn't read the Jetpack connection status: ${e.message}")
69+
null
70+
}
71+
72+
private fun buildClient(
73+
apiRootUrl: String,
74+
username: String,
75+
password: String
76+
) = JetpackConnectionClient(
77+
apiRootUrl = ParsedUrl.parse(apiRootUrl),
78+
delegate = WpApiClientDelegate(
79+
authProvider = WpAuthenticationProvider.staticWithUsernameAndPassword(username, password),
80+
requestExecutor = requestExecutor,
81+
middlewarePipeline = WpApiMiddlewarePipeline(emptyList()),
82+
// Reading the status is a passive check made during a site refresh, so a rejected
83+
// credential shouldn't raise the app-wide "re-authenticate" prompt on its own.
84+
appNotifier = EmptyAppNotifier()
85+
)
86+
)
87+
88+
companion object {
89+
private const val TAG = "JetpackConnectionStatusFetcher"
90+
}
91+
}

libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/site/SiteWPAPIRestClient.kt

Lines changed: 152 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,8 @@
11
package org.wordpress.android.fluxc.network.rest.wpapi.site
22

33
import com.android.volley.RequestQueue
4+
import com.google.gson.reflect.TypeToken
5+
import okhttp3.Credentials
46
import okhttp3.HttpUrl.Companion.toHttpUrl
57
import org.wordpress.android.fluxc.Dispatcher
68
import org.wordpress.android.fluxc.model.SiteModel
@@ -13,6 +15,9 @@ import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIDiscoveryUtils
1315
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIGsonRequestBuilder
1416
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIResponse.Error
1517
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIResponse.Success
18+
import org.wordpress.android.fluxc.network.rest.wpapi.jetpack.JetpackConnectionState
19+
import org.wordpress.android.fluxc.network.rest.wpapi.jetpack.JetpackConnectionStatusFetcher
20+
import org.wordpress.android.fluxc.network.rest.wpapi.plugin.PluginResponseModel
1621
import org.wordpress.android.fluxc.store.SiteStore.FetchWPAPISitePayload
1722
import org.wordpress.android.fluxc.utils.extensions.getPasswordProcessed
1823
import org.wordpress.android.fluxc.utils.extensions.getUserNameProcessed
@@ -26,19 +31,33 @@ import javax.inject.Singleton
2631
class SiteWPAPIRestClient @Inject constructor(
2732
private val wpapiGsonRequestBuilder: WPAPIGsonRequestBuilder,
2833
private val discoveryWPAPIRestClient: DiscoveryWPAPIRestClient,
34+
private val jetpackConnectionStatusFetcher: JetpackConnectionStatusFetcher,
2935
dispatcher: Dispatcher,
3036
@Named(OkHttpClientQualifiers.CUSTOM_SSL) requestQueue: RequestQueue,
3137
userAgent: UserAgent
3238
) : BaseWPAPIRestClient(dispatcher, requestQueue, userAgent) {
3339
companion object {
3440
private const val WOO_API_NAMESPACE_PREFIX = "wc/"
41+
private const val JETPACK_API_NAMESPACE_PREFIX = "jetpack/"
3542
private const val FETCH_API_CALL_FIELDS =
3643
"name,description,gmt_offset,url,authentication,namespaces"
3744
private const val APPLICATION_PASSWORDS_URL_SUFFIX = "authorize-application.php"
45+
private const val PLUGINS_PATH = "wp/v2/plugins"
46+
private const val PLUGINS_FIELDS = "plugin,status,version"
47+
private const val JETPACK_PLUGIN_SEARCH = "jetpack"
48+
private const val JETPACK_PLUGIN_ID = "jetpack/jetpack"
49+
private const val PLUGIN_STATUS_ACTIVE = "active"
50+
private const val PLUGIN_STATUS_NETWORK_ACTIVE = "network-active"
3851
}
3952

53+
/**
54+
* @param previousSite the site this fetch is refreshing, when there is one. The model returned here
55+
* replaces the stored row wholesale, so fields this fetch can't determine are carried forward from it
56+
* rather than reset.
57+
*/
4058
suspend fun fetchWPAPISite(
41-
payload: FetchWPAPISitePayload
59+
payload: FetchWPAPISitePayload,
60+
previousSite: SiteModel? = null
4261
): SiteModel {
4362
val cleanedUrl = UrlUtils.addUrlSchemeIfNeeded(payload.url, false).let { urlWithScheme ->
4463
DiscoveryUtils.stripKnownPaths(urlWithScheme)
@@ -57,14 +76,22 @@ class SiteWPAPIRestClient @Inject constructor(
5776
return when (result) {
5877
is Success -> {
5978
val response = result.data
79+
val jetpackPlugin = fetchJetpackPluginState(response?.namespaces, discoveredWpApiUrl, payload)
80+
val jetpackConnection = fetchJetpackConnectionState(jetpackPlugin, discoveredWpApiUrl, payload)
6081
SiteModel().apply {
82+
// Carry the local id so SiteStore.updateSite finds the stored row and preserves the
83+
// editor preferences, and so SiteSqlUtils matches the row by local id rather than by
84+
// SITE_ID + URL -- that match misses, and inserts a duplicate site, as soon as a real
85+
// WP.com blog id is stored.
86+
id = previousSite?.id ?: 0
6187
name = response?.name
6288
description = response?.description
6389
timezone = response?.gmtOffset
6490
origin = SiteModel.ORIGIN_WPAPI
6591
hasWooCommerce = response?.namespaces?.any {
6692
it.startsWith(WOO_API_NAMESPACE_PREFIX)
6793
} ?: false
94+
applyJetpackState(jetpackPlugin, jetpackConnection, previousSite)
6895

6996
applicationPasswordsAuthorizeUrl = response?.authentication?.applicationPasswords
7097
?.endpoints?.authorization
@@ -104,10 +131,133 @@ class SiteWPAPIRestClient @Inject constructor(
104131
password = site.getPasswordProcessed(),
105132
isApplicationPassword =
106133
site.hasApplicationPassword(),
107-
)
134+
),
135+
previousSite = site
108136
)
109137
}
110138

139+
/**
140+
* The Jetpack plugin, as reported by the site itself. [isActive] is what
141+
* [SiteModel.isJetpackInstalled] documents -- installed *and* activated.
142+
*/
143+
private data class JetpackPluginState(val isActive: Boolean, val version: String?)
144+
145+
/**
146+
* Determines whether the site is running the Jetpack plugin, or null when it can't be determined.
147+
*
148+
* The `jetpack/` REST namespace is only a first filter: it's registered by the shared
149+
* `automattic/jetpack-connection` package, which also ships inside Jetpack Boost, Protect, Social and
150+
* VaultPress Backup, so its presence does *not* mean the Jetpack plugin is installed. What it does give
151+
* us for free is a reliable negative -- a namespace list without `jetpack/` means no active Jetpack --
152+
* which keeps the plugin lookup off the refresh path for the sites that have nothing to do with Jetpack.
153+
* That only holds when the list is present: an absent field is "couldn't read", not "no Jetpack".
154+
*
155+
* Reading the plugin list needs credentials and the `activate_plugins` capability, so it returns null for
156+
* sites without an application password and for users who aren't administrators. Callers must read null
157+
* as "unchanged", not as "not installed".
158+
*/
159+
private suspend fun fetchJetpackPluginState(
160+
namespaces: List<String>?,
161+
apiRootUrl: String,
162+
payload: FetchWPAPISitePayload
163+
): JetpackPluginState? = when {
164+
namespaces == null -> null
165+
namespaces.none { it.startsWith(JETPACK_API_NAMESPACE_PREFIX) } ->
166+
JetpackPluginState(isActive = false, version = null)
167+
payload.isApplicationPassword &&
168+
!payload.username.isNullOrEmpty() && !payload.password.isNullOrEmpty() ->
169+
requestJetpackPlugin(apiRootUrl, payload.username.orEmpty(), payload.password.orEmpty())
170+
else -> null
171+
}
172+
173+
private suspend fun requestJetpackPlugin(
174+
apiRootUrl: String,
175+
username: String,
176+
password: String
177+
): JetpackPluginState? {
178+
val result = wpapiGsonRequestBuilder.syncGetRequest<List<PluginResponseModel>>(
179+
restClient = this,
180+
url = apiRootUrl.trimEnd('/') + "/" + PLUGINS_PATH,
181+
params = mapOf("search" to JETPACK_PLUGIN_SEARCH, "_fields" to PLUGINS_FIELDS),
182+
type = object : TypeToken<List<PluginResponseModel>>() {}.type,
183+
headers = mapOf("Authorization" to Credentials.basic(username, password))
184+
)
185+
186+
return when (result) {
187+
is Success -> {
188+
val jetpack = result.data?.firstOrNull { it.plugin == JETPACK_PLUGIN_ID }
189+
JetpackPluginState(
190+
isActive = jetpack?.status == PLUGIN_STATUS_ACTIVE ||
191+
jetpack?.status == PLUGIN_STATUS_NETWORK_ACTIVE,
192+
version = jetpack?.version
193+
)
194+
}
195+
196+
is Error -> null
197+
}
198+
}
199+
200+
/**
201+
* Reads the site's Jetpack connection to WordPress.com, or null when it can't be determined. Only the
202+
* Jetpack plugin knows the blog ID WordPress.com assigned the site, and without it the row keeps
203+
* SITE_ID = 0 -- which is what makes a later /me/sites sync insert a duplicate site instead of matching
204+
* and upgrading this one. Skipped entirely when the plugin isn't there to ask.
205+
*/
206+
private suspend fun fetchJetpackConnectionState(
207+
plugin: JetpackPluginState?,
208+
apiRootUrl: String,
209+
payload: FetchWPAPISitePayload
210+
): JetpackConnectionState? {
211+
// isActive = true is only ever produced by requestJetpackPlugin, which already required the
212+
// application-password credentials, so no separate credential check is needed here.
213+
if (plugin?.isActive != true) return null
214+
val username = payload.username
215+
val password = payload.password
216+
return if (username != null && password != null) {
217+
jetpackConnectionStatusFetcher.fetch(apiRootUrl, username, password)
218+
} else {
219+
null
220+
}
221+
}
222+
223+
/**
224+
* A null [plugin] or [connection] means that state couldn't be read on this run, in which case the
225+
* stored values are carried forward rather than reset. A non-null result is definitive, including
226+
* its null fields -- a disconnected site clears the blog id rather than keeping a stale one.
227+
*
228+
* Note that [SiteModel.isJetpackConnected] only says the site is connected to *some* WordPress.com
229+
* account, not necessarily the one signed in here -- callers that need the stronger claim have to check
230+
* account access separately. See CMM-2344.
231+
*/
232+
private fun SiteModel.applyJetpackState(
233+
plugin: JetpackPluginState?,
234+
connection: JetpackConnectionState?,
235+
previousSite: SiteModel?
236+
) {
237+
if (plugin != null) {
238+
setIsJetpackInstalled(plugin.isActive)
239+
jetpackVersion = plugin.version
240+
} else {
241+
setIsJetpackInstalled(previousSite?.isJetpackInstalled ?: false)
242+
jetpackVersion = previousSite?.jetpackVersion
243+
}
244+
when {
245+
// The plugin is definitively gone, so its WordPress.com connection is too.
246+
plugin != null && !plugin.isActive -> {
247+
setIsJetpackConnected(false)
248+
siteId = 0
249+
}
250+
connection != null -> {
251+
setIsJetpackConnected(connection.isConnected)
252+
siteId = connection.wpComSiteId ?: 0L
253+
}
254+
else -> {
255+
setIsJetpackConnected(previousSite?.isJetpackConnected ?: false)
256+
siteId = previousSite?.siteId ?: 0L
257+
}
258+
}
259+
}
260+
111261
private fun discoverApiEndpoint(
112262
url: String
113263
): String {

libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -415,14 +415,21 @@ class SiteSqlUtils
415415
return updateXmlRpcUrl(localId, xmlRpcUrl)
416416
}
417417

418-
private fun wpApiSiteLocalIdByUrl(siteUrl: String): Int? =
418+
/**
419+
* The stored ORIGIN_WPAPI row for [siteUrl], for fetches that build a fresh model with no local id and
420+
* need the identity and the columns the response can't carry. Keyed by URL for the same reason as
421+
* [updateWpApiRestUrlForWPAPISite].
422+
*/
423+
fun getWPAPISiteByUrl(siteUrl: String): SiteModel? =
419424
WellSql.select(SiteModel::class.java)
420425
.where().beginGroup()
421426
.equals(SiteModelTable.URL, siteUrl)
422427
.equals(SiteModelTable.ORIGIN, SiteModel.ORIGIN_WPAPI)
423428
.endGroup().endWhere()
424429
.asModel
425-
.firstOrNull()?.id
430+
.firstOrNull()
431+
432+
private fun wpApiSiteLocalIdByUrl(siteUrl: String): Int? = getWPAPISiteByUrl(siteUrl)?.id
426433

427434
val wPComSites: SelectQuery<SiteModel>
428435
get() = WellSql.select(SiteModel::class.java)

0 commit comments

Comments
 (0)