diff --git a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt index fcfd237fba85..3ae1fdaa37c0 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSlice.kt @@ -9,8 +9,6 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import org.wordpress.android.R import androidx.annotation.VisibleForTesting -import org.wordpress.android.fluxc.Dispatcher -import org.wordpress.android.fluxc.generated.SiteActionBuilder import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.network.discovery.SelfHostedEndpointFinder import org.wordpress.android.fluxc.network.xmlrpc.site.SiteXMLRPCClient @@ -41,7 +39,6 @@ class ApplicationPasswordViewModelSlice @Inject constructor( private val selfHostedEndpointFinder: SelfHostedEndpointFinder, private val siteXMLRPCClient: SiteXMLRPCClient, private val siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer, - private val dispatcher: Dispatcher, private val credentialsChangedNotifier: CredentialsChangedNotifier, @Named(IO_THREAD) private val ioDispatcher: CoroutineDispatcher, ) { @@ -257,9 +254,10 @@ class ApplicationPasswordViewModelSlice @Inject constructor( } site.xmlRpcUrl = xmlRpcEndpoint - dispatcher.dispatch( - SiteActionBuilder.newUpdateSiteAction(site) - ) + // Persist only the rediscovered column — mirrors healApiRestUrlIfMissing. A full-row + // updateSite would rewrite ~80 columns from this in-memory model for a one-field change + // (risking clobbering other out-of-band values), so write just xmlRpcUrl. + siteStore.persistXmlRpcUrl(site.id, xmlRpcEndpoint) buildCard(site) } catch ( @Suppress("SwallowedException") diff --git a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt index 00bf2c00bbad..6276f50d4a58 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt @@ -19,7 +19,6 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.mockito.kotlin.mock -import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.model.SitesModel import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError @@ -71,9 +70,6 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { @Mock lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer - @Mock - lateinit var dispatcher: Dispatcher - @Mock lateinit var credentialsChangedNotifier: CredentialsChangedNotifier @@ -96,7 +92,6 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { selfHostedEndpointFinder, siteXMLRPCClient, siteApiRestUrlRecoverer, - dispatcher, credentialsChangedNotifier, testDispatcher() ).apply { @@ -380,8 +375,10 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { } @Test - fun `given xmlRpc rediscovery and auth check succeed, then update site and dispatch`() = + fun `given xmlRpc rediscovery and auth check succeed, then persist the discovered xmlRpcUrl`() = runTest { + // @Before seeds siteTest.xmlRpcUrl; clear it so the final assertion proves rediscovery set it. + siteTest.xmlRpcUrl = null val xmlRpcUrl = "https://www.test.com/xmlrpc.php" whenever( selfHostedEndpointFinder @@ -392,16 +389,17 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { eq(xmlRpcUrl), any(), any() ) ).thenReturn(SitesModel(listOf(SiteModel()))) + whenever(siteStore.persistXmlRpcUrl(any(), any())).thenReturn(SiteStore.OnSiteChanged(0)) applicationPasswordViewModelSlice .attemptXmlRpcRediscovery(siteTest) - verify(dispatcher).dispatch(any()) + verify(siteStore).persistXmlRpcUrl(siteTest.id, xmlRpcUrl) assert(siteTest.xmlRpcUrl == xmlRpcUrl) } @Test - fun `given xmlRpc rediscovery succeeds but auth check fails, then do not dispatch`() = + fun `given xmlRpc rediscovery succeeds but auth check fails, then do not persist`() = runTest { siteTest.xmlRpcUrl = null val xmlRpcUrl = "https://www.test.com/xmlrpc.php" @@ -421,12 +419,12 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { applicationPasswordViewModelSlice .attemptXmlRpcRediscovery(siteTest) - verify(dispatcher, never()).dispatch(any()) + verify(siteStore, never()).persistXmlRpcUrl(any(), any()) assert(siteTest.xmlRpcUrl.isNullOrEmpty()) } @Test - fun `given xmlRpc rediscovery fails, then do not dispatch`() = + fun `given xmlRpc rediscovery fails, then do not persist`() = runTest { siteTest.xmlRpcUrl = null whenever( @@ -441,7 +439,7 @@ class ApplicationPasswordViewModelSliceTest : BaseUnitTest() { verify(selfHostedEndpointFinder) .verifyOrDiscoverXMLRPCEndpoint(TEST_URL) - verify(dispatcher, never()).dispatch(any()) + verify(siteStore, never()).persistXmlRpcUrl(any(), any()) assert(siteTest.xmlRpcUrl.isNullOrEmpty()) } } diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/CookieNonceAuthenticator.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/CookieNonceAuthenticator.kt index 031efb5c04d5..b2d8bae663c6 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/CookieNonceAuthenticator.kt +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/CookieNonceAuthenticator.kt @@ -56,8 +56,11 @@ class CookieNonceAuthenticator @Inject constructor( ): T { val usingSavedRestUrl = site.wpApiRestUrl != null if (!usingSavedRestUrl) { - site.wpApiRestUrl = discoverApiEndpoint(site.url) - (siteSqlUtils::insertOrUpdateSite)(site) + val discoveredUrl = discoverApiEndpoint(site.url) + site.wpApiRestUrl = discoveredUrl + // WP_API_REST_URL is excluded from full-row writes (see SiteSqlUtils), so persist the + // freshly discovered value through its dedicated writer. + siteSqlUtils.updateWpApiRestUrl(site.id, discoveredUrl) } val response = makeAuthenticatedWPAPIRequest( @@ -71,9 +74,9 @@ class CookieNonceAuthenticator @Inject constructor( return if (response is WPAPIResponse.Error<*> && response.error.volleyError?.networkResponse?.statusCode == STATUS_CODE_NOT_FOUND) { - // call failed with 'not found' so clear the (failing) rest url + // Reset the in-memory rest url so the retry rediscovers it. The stored value is left intact + // (a 404 may be transient); WP_API_REST_URL is healed only via updateWpApiRestUrl. site.wpApiRestUrl = null - (siteSqlUtils::insertOrUpdateSite)(site) if (usingSavedRestUrl) { // If we did the previous call with a saved rest url, try again by making diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt index 26a0955b5d7c..4be29001db32 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/SiteSqlUtils.kt @@ -105,7 +105,17 @@ class SiteSqlUtils .decryptAPIRestCredentials() /** - * Inserts the given SiteModel into the DB, or updates an existing entry where sites match. + * Inserts or updates [site] and returns the number of rows affected (1 if a row was written, 0 + * otherwise). Use [insertOrUpdateSiteReturningId] when you need the local id of the written row. + */ + @Throws(DuplicateSiteException::class) + fun insertOrUpdateSite(site: SiteModel?): Int = + if (insertOrUpdateSiteReturningId(site) != 0) 1 else 0 + + /** + * Inserts the given SiteModel into the DB, or updates an existing entry where sites match, returning the + * local id of the written row (0 if nothing was written). Returning the id lets callers target that exact + * row with the single-column writers without a second, fragile lookup. * * Possible cases: * 1. Exists in the DB already and matches by local id (simple update) -> UPDATE @@ -117,7 +127,7 @@ class SiteSqlUtils */ @Suppress("LongMethod", "ReturnCount", "ComplexMethod") @Throws(DuplicateSiteException::class) - fun insertOrUpdateSite(site: SiteModel?): Int { + fun insertOrUpdateSiteReturningId(site: SiteModel?): Int { if (site == null) { return 0 } @@ -214,15 +224,39 @@ class SiteSqlUtils return if (siteResult.isEmpty()) { // No site with this local ID, REMOTE_ID + URL, or XMLRPC URL, then insert it AppLog.d(DB, "Inserting site: " + finalSiteModel.url) + // WellSql back-fills the auto-assigned id onto the model on insert (Identifiable.setId). WellSql.insert(finalSiteModel).asSingleTransaction(true).execute() - 1 + finalSiteModel.id } else { // Update old site AppLog.d(DB, "Updating site: " + finalSiteModel.url) val oldId = siteResult[0].id try { + // WP_API_REST_URL and the application-password credential columns are discovered/healed out + // of band — never carried by the general site-sync responses — so they're excluded from the + // generic mapper and written only by their dedicated writers (updateWpApiRestUrl / + // updateApplicationPasswordCredentials); a stale full-row write must not clobber them. + // + // XMLRPC_URL is different: the WP.com REST sync reliably carries it (meta.links.xmlrpc), so + // the generic write persists it — that's how a changed endpoint (e.g. a domain migration) + // lands. But partial writers that don't carry it (the WPAPI fetch builds a model with a null + // xmlRpcUrl) must not clear a stored/rediscovered value, so preserve it on absence. + if (finalSiteModel.xmlRpcUrl.isNullOrEmpty()) { + finalSiteModel.xmlRpcUrl = siteResult[0].xmlRpcUrl + } WellSql.update(SiteModel::class.java).whereId(oldId) - .put(finalSiteModel, UpdateAllExceptId(SiteModel::class.java)).execute() + .put( + finalSiteModel, + UpdateAllExceptId( + SiteModel::class.java, + SiteModelTable.WP_API_REST_URL, + SiteModelTable.API_REST_USERNAME, + SiteModelTable.API_REST_PASSWORD, + SiteModelTable.API_REST_USERNAME_IV, + SiteModelTable.API_REST_PASSWORD_IV + ) + ).execute() + oldId } catch (e: SQLiteConstraintException) { AppLog.e( DB, @@ -260,6 +294,11 @@ class SiteSqlUtils }).execute() } + /** + * Targeted writer for [SiteModel.wpApiRestUrl]. This is the sole writer of WP_API_REST_URL on an + * existing row: the generic full-row update path ([insertOrUpdateSite]) excludes the column so that + * stale in-memory sites can't clobber a value that was healed/discovered out of band. + */ fun updateWpApiRestUrl(localId: Int, wpApiRestUrl: String): Int { return WellSql.update(SiteModel::class.java) .whereId(localId) @@ -270,6 +309,109 @@ class SiteSqlUtils }).execute() } + /** + * Clears [SiteModel.wpApiRestUrl] for the given local id. Use this instead of a full-row update when an + * explicit action (e.g. removing an application password) needs to drop the stored REST URL, since the + * generic update path no longer touches the column. + */ + fun clearWpApiRestUrl(localId: Int): Int = updateWpApiRestUrl(localId, "") + + /** + * Targeted writer for [SiteModel.xmlRpcUrl], used by the XML-RPC rediscovery heal to persist just that + * one column without a full-row write of an in-memory model. Unlike [updateWpApiRestUrl], XMLRPC_URL is + * NOT excluded from the generic mapper — the WP.com sync reliably carries it — but the generic update + * path preserves it on absence, so a partial write can't clobber a rediscovered value. (The recovery flow + * that calls this lands separately.) + */ + fun updateXmlRpcUrl(localId: Int, xmlRpcUrl: String): Int { + return WellSql.update(SiteModel::class.java) + .whereId(localId) + .put(xmlRpcUrl, { value -> + val cv = ContentValues() + cv.put(SiteModelTable.XMLRPC_URL, value) + cv + }).execute() + } + + /** + * Targeted writer for the application-password credential columns: the encrypted username and password + * and their IVs. The four are written as a set — the IVs are required to decrypt the ciphertext, so + * persisting the values without their IVs would break reads. This is the sole writer of these columns on + * an existing row: the generic full-row update path excludes them so a credential-less inbound SiteModel + * (e.g. a /me/sites sync model) can't zero them out. + */ + fun updateApplicationPasswordCredentials(localId: Int, usernamePlain: String, passwordPlain: String): Int { + val username = encryptionUtils.encrypt(usernamePlain) + val password = encryptionUtils.encrypt(passwordPlain) + return writeApplicationPasswordCredentialColumns( + localId, username.first, username.second, password.first, password.second + ) + } + + /** + * Clears the application-password credential columns (encrypted username/password and their IVs) for the + * given local id. Companion to [updateApplicationPasswordCredentials] for the sign-out / revoke paths, + * since the generic update path no longer touches these columns. + */ + fun clearApplicationPasswordCredentials(localId: Int): Int = + writeApplicationPasswordCredentialColumns(localId, "", "", "", "") + + /** + * The single place that maps the four application-password credential columns to their values, so the + * column set lives in one spot. Callers pass already-encrypted values (or empty strings to clear); each + * IV always travels with its ciphertext, so a row can never be left holding one without the other. + */ + private fun writeApplicationPasswordCredentialColumns( + localId: Int, + usernameCipher: String, + usernameIv: String, + passwordCipher: String, + passwordIv: String + ): Int { + return WellSql.update(SiteModel::class.java) + .whereId(localId) + .put(localId, { _ -> + val cv = ContentValues() + cv.put(SiteModelTable.API_REST_USERNAME, usernameCipher) + cv.put(SiteModelTable.API_REST_USERNAME_IV, usernameIv) + cv.put(SiteModelTable.API_REST_PASSWORD, passwordCipher) + cv.put(SiteModelTable.API_REST_PASSWORD_IV, passwordIv) + cv + }).execute() + } + + /** + * URL-keyed variant of [updateWpApiRestUrl] for application-password (ORIGIN_WPAPI) sites, which are + * fetched as fresh models with no local id (see SiteWPAPIRestClient.fetchWPAPISite). Scoped to + * ORIGIN_WPAPI so it can't touch a WP.com/Jetpack row that happens to share the same URL. + */ + fun updateWpApiRestUrlForWPAPISite(siteUrl: String, wpApiRestUrl: String): Int { + val localId = wpApiSiteLocalIdByUrl(siteUrl) ?: return 0 + return updateWpApiRestUrl(localId, wpApiRestUrl) + } + + /** + * URL-keyed variant of [updateApplicationPasswordCredentials] for ORIGIN_WPAPI sites fetched without a + * local id. See [updateWpApiRestUrlForWPAPISite]. + */ + fun updateApplicationPasswordCredentialsForWPAPISite( + siteUrl: String, + usernamePlain: String, + passwordPlain: String + ): Int { + val localId = wpApiSiteLocalIdByUrl(siteUrl) ?: return 0 + return updateApplicationPasswordCredentials(localId, usernamePlain, passwordPlain) + } + + private fun wpApiSiteLocalIdByUrl(siteUrl: String): Int? = + WellSql.select(SiteModel::class.java) + .where().beginGroup() + .equals(SiteModelTable.URL, siteUrl) + .equals(SiteModelTable.ORIGIN, SiteModel.ORIGIN_WPAPI) + .endGroup().endWhere() + .asModel + .firstOrNull()?.id + val wPComSites: SelectQuery get() = WellSql.select(SiteModel::class.java) .where().beginGroup() @@ -573,14 +715,17 @@ class SiteSqlUtils return this.map { it.decryptAPIRestCredentials() } } - @Suppress("ReturnCount") + @Suppress("ReturnCount", "ComplexCondition") private fun SiteModel.decryptAPIRestCredentials(): SiteModel { // If already decrypted, do nothing if (!apiRestUsernamePlain.isNullOrEmpty() && !apiRestPasswordPlain.isNullOrEmpty()) { return this } - // If the encrypted credentials are empty, there's nothing to decrypt - if (apiRestUsernameEncrypted.isNullOrEmpty() || apiRestPasswordEncrypted.isNullOrEmpty()) { + // If the encrypted credentials — or the IVs required to decrypt them — are empty, there's nothing to + // safely decrypt. The ciphertext/IV pairs are always written together, so a row missing any one is + // treated as having no decryptable credentials rather than risking a decrypt failure on a blank IV. + if (apiRestUsernameEncrypted.isNullOrEmpty() || apiRestPasswordEncrypted.isNullOrEmpty() || + apiRestUsernameIV.isNullOrEmpty() || apiRestPasswordIV.isNullOrEmpty()) { return this } apiRestUsernamePlain = encryptionUtils.decrypt(apiRestUsernameEncrypted, apiRestUsernameIV) diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java index 3d3c79e5ead9..f5f41554534f 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java @@ -8,15 +8,29 @@ class UpdateAllExceptId implements InsertMapper { private final SQLiteMapper mMapper; + private final String[] mAdditionalColumnsToSkip; UpdateAllExceptId(Class clazz) { + this(clazz, new String[0]); + } + + /** + * @param additionalColumnsToSkip columns (beyond the primary key) that should be left untouched by the + * resulting UPDATE. Use this when a column has a dedicated writer and must + * not be overwritten by full-row updates built from partial in-memory models. + */ + UpdateAllExceptId(Class clazz, String... additionalColumnsToSkip) { mMapper = WellSql.mapperFor(clazz); + mAdditionalColumnsToSkip = additionalColumnsToSkip; } @Override public ContentValues toCv(T item) { ContentValues cv = mMapper.toCv(item); cv.remove("_id"); + for (String column : mAdditionalColumnsToSkip) { + cv.remove(column); + } return cv; } } diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt index 7e2de0511b7b..90b13ea0d86f 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/ReactNativeStore.kt @@ -15,7 +15,6 @@ import org.wordpress.android.fluxc.network.rest.wpapi.NonceRestClient import org.wordpress.android.fluxc.network.rest.wpapi.reactnative.ReactNativeWPAPIRestClient import org.wordpress.android.fluxc.network.rest.wpcom.reactnative.ReactNativeWPComRestClient import org.wordpress.android.fluxc.persistence.SiteSqlUtils -import org.wordpress.android.fluxc.persistence.SiteSqlUtils.DuplicateSiteException import org.wordpress.android.fluxc.store.ReactNativeFetchResponse.Error import org.wordpress.android.fluxc.store.ReactNativeFetchResponse.Success import org.wordpress.android.fluxc.tools.CoroutineEngine @@ -40,7 +39,6 @@ class ReactNativeStore @VisibleForTesting constructor( private val siteSqlUtils: SiteSqlUtils, private val coroutineEngine: CoroutineEngine, private val currentTimeMillis: () -> Long = System::currentTimeMillis, - private val sitePersistanceFunction: (site: SiteModel) -> Int = siteSqlUtils::insertOrUpdateSite, private val uriParser: (string: String) -> Uri = Uri::parse ) { @Inject constructor( @@ -58,7 +56,6 @@ class ReactNativeStore @VisibleForTesting constructor( siteSqlUtils, coroutineEngine, System::currentTimeMillis, - siteSqlUtils::insertOrUpdateSite, Uri::parse ) @@ -175,10 +172,13 @@ class ReactNativeStore @VisibleForTesting constructor( val usingSavedRestUrl = wpApiRestUrl != null if (!usingSavedRestUrl) { - wpApiRestUrl = discoveryWPAPIRestClient.discoverWPAPIBaseURL(site.url) // discover rest api endpoint + val discoveredUrl = discoveryWPAPIRestClient.discoverWPAPIBaseURL(site.url) // discover rest api endpoint ?: slashJoin(site.url, "wp-json/") // fallback to ".../wp-json/" default if discovery fails - site.wpApiRestUrl = wpApiRestUrl - persistSiteSafely(site) + wpApiRestUrl = discoveredUrl + site.wpApiRestUrl = discoveredUrl + // WP_API_REST_URL is excluded from full-row writes (see SiteSqlUtils), so persist the + // freshly discovered value through its dedicated writer. + siteSqlUtils.updateWpApiRestUrl(site.id, discoveredUrl) } val fullRestUrl = slashJoin(wpApiRestUrl, path) @@ -230,9 +230,9 @@ class ReactNativeStore @VisibleForTesting constructor( } HttpURLConnection.HTTP_NOT_FOUND -> { - // call failed with 'not found' so clear the (failing) rest url + // Reset the in-memory rest url so the retry rediscovers it. The stored value is left + // intact (a 404 may be transient); WP_API_REST_URL is healed only via updateWpApiRestUrl. site.wpApiRestUrl = null - persistSiteSafely(site) if (usingSavedRestUrl) { // If we did the previous call with a saved rest url, try again by making @@ -286,8 +286,8 @@ class ReactNativeStore @VisibleForTesting constructor( is Success -> response is Error -> when (response.statusCode()) { HttpURLConnection.HTTP_NOT_FOUND -> { + // Reset the in-memory rest url so the retry rediscovers it (see executeWPAPIRequest). site.wpApiRestUrl = null - persistSiteSafely(site) if (usingSavedRestUrl) { executeWPAPIRequest( @@ -351,16 +351,6 @@ class ReactNativeStore @VisibleForTesting constructor( return Pair(uri.path, paramMap) } - private fun persistSiteSafely(site: SiteModel) { - try { - sitePersistanceFunction.invoke(site) - } catch (e: DuplicateSiteException) { - // persistance failed, which is not a big deal because it just means we may need to re-discover the - // rest url later. - AppLog.d(AppLog.T.DB, "Error when persisting site: $e") - } - } - private fun getNonce(site: SiteModel) = nonceRestClient.getNonce(site) companion object { diff --git a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt index b73dcaf4dc71..494f8c60fb83 100644 --- a/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt +++ b/libs/fluxc/src/main/java/org/wordpress/android/fluxc/store/SiteStore.kt @@ -1615,7 +1615,21 @@ open class SiteStore @Inject constructor( if (!siteModel.isError) { siteModel.wpApiRestUrl = payload.apiRootUrl } - updateSite(siteModel) + val result = updateSite(siteModel) + // updateSite's full-row write skips WP_API_REST_URL and the credential columns, and this fresh + // site has no local id (it's matched by URL), so persist them via the URL-keyed writers. + if (!siteModel.isError) { + persistAppPasswordColumns( + siteModel, + { username, password -> + siteSqlUtils.updateApplicationPasswordCredentialsForWPAPISite( + siteModel.url, username, password + ) + }, + { siteSqlUtils.updateWpApiRestUrlForWPAPISite(siteModel.url, it) } + ) + } + result } catch (e: Exception) { val errorMsg = e.message ?: e.javaClass.simpleName AppLog.e( @@ -1653,21 +1667,13 @@ open class SiteStore @Inject constructor( OnSiteChanged(SiteErrorUtils.genericToSiteError(siteModel.error)) } else { try { - // The REST API doesn't return info about the editor(s) nor the Application Password. - // Make sure to copy current values available on the DB. - // Otherwise the apps will receive an update site without editor prefs set. - // The apps will dispatch the action to update editor(s) when necessary. + // The REST API doesn't return editor prefs, so copy the current values from the DB to avoid + // emitting an updated site without them. (Credentials and wpApiRestUrl are protected by the + // mapper exclusion in SiteSqlUtils, so they don't need copying here.) val freshSiteFromDB = getSiteByLocalId(siteModel.id) if (freshSiteFromDB != null) { siteModel.mobileEditor = freshSiteFromDB.mobileEditor siteModel.webEditor = freshSiteFromDB.webEditor - if (!freshSiteFromDB.apiRestUsernameEncrypted.isNullOrEmpty()) { - siteModel.apiRestUsernameEncrypted = freshSiteFromDB.apiRestUsernameEncrypted - siteModel.apiRestPasswordEncrypted = freshSiteFromDB.apiRestPasswordEncrypted - siteModel.apiRestUsernameIV = freshSiteFromDB.apiRestUsernameIV - siteModel.apiRestPasswordIV = freshSiteFromDB.apiRestPasswordIV - siteModel.wpApiRestUrl = freshSiteFromDB.wpApiRestUrl - } } OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteModel)) } catch (e: DuplicateSiteException) { @@ -1676,26 +1682,63 @@ open class SiteStore @Inject constructor( } } + /** + * Targeted persist for [SiteModel.xmlRpcUrl] — for callers (e.g. XML-RPC rediscovery) that heal/discover + * only that one column and don't want to full-row write an in-memory model. The generic update path also + * persists xmlRpcUrl (and preserves it on absence), so this is a convenience for the single-column heal, + * not the sole writer. + */ + suspend fun persistXmlRpcUrl(localId: Int, xmlRpcUrl: String): OnSiteChanged = + coroutineEngine.withDefaultContext(T.API, this, "persistXmlRpcUrl") { + OnSiteChanged(siteSqlUtils.updateXmlRpcUrl(localId, xmlRpcUrl)) + } + + /** + * Persists the application-password columns that the generic full-row write excludes — the encrypted + * credentials and, when present, [SiteModel.wpApiRestUrl] — via the supplied targeted writers, returning the + * number of rows written. The three post-write handlers (UPDATE_APPLICATION_PASSWORD, the WPAPI app-password + * fetch, and app-password XML-RPC login in createOrUpdateSites) differ only in targeting the row by local id + * or by URL, so each passes its matching writer pair. + * + * wpApiRestUrl is written only alongside credentials. A credential-less /me/sites model can be a WP.com simple + * site whose getWpApiRestUrl() synthesizes a public-api proxy URL; gating on credentials keeps that synthetic + * value out of the DB (such sites never carry app-password credentials). + */ + private fun persistAppPasswordColumns( + site: SiteModel, + persistCredentials: (username: String, password: String) -> Int, + persistWpApiRestUrl: (wpApiRestUrl: String) -> Int + ): Int { + val username = site.apiRestUsernamePlain + val password = site.apiRestPasswordPlain + if (username.isNullOrEmpty() || password.isNullOrEmpty()) return 0 + var rowsAffected = persistCredentials(username, password) + site.wpApiRestUrl?.takeIf { it.isNotEmpty() }?.let { + rowsAffected += persistWpApiRestUrl(it) + } + return rowsAffected + } + @Suppress("SwallowedException", "TooGenericExceptionCaught") private fun updateApplicationPassword(siteModel: SiteModel): OnSiteChanged { return try { val siteFromDB = getSiteByLocalId(siteModel.id) - // If the site doesn't exists we rely on create a new one - val siteToStore = if (siteFromDB == null) { - siteModel + if (siteFromDB == null) { + // New site: the full insert persists everything, incl. credentials via encrypt-on-insert. + OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteModel)) } else { - siteFromDB.apply { - apiRestUsernamePlain = siteModel.apiRestUsernamePlain - apiRestPasswordPlain = siteModel.apiRestPasswordPlain - apiRestUsernameEncrypted = siteModel.apiRestUsernameEncrypted - apiRestPasswordEncrypted = siteModel.apiRestPasswordEncrypted - apiRestUsernameIV = siteModel.apiRestUsernameIV - apiRestPasswordIV = siteModel.apiRestPasswordIV - wpApiRestUrl = siteModel.wpApiRestUrl - } - siteFromDB + // Existing site: credentials + WP_API_REST_URL are excluded from the full-row write, so + // persist them on the existing row via their targeted writers (nothing else changed). + OnSiteChanged( + persistAppPasswordColumns( + siteModel, + { username, password -> + siteSqlUtils.updateApplicationPasswordCredentials(siteFromDB.id, username, password) + }, + { siteSqlUtils.updateWpApiRestUrl(siteFromDB.id, it) } + ) + ) } - OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteToStore)) } catch (e: DuplicateSiteException) { OnSiteChanged(SiteError(DUPLICATE_SITE)) } catch (e: Exception) { @@ -1717,16 +1760,11 @@ open class SiteStore @Inject constructor( if (siteFromDB == null) { OnSiteChanged(SiteError(SiteErrorType.INVALID_SITE)) } else { - siteFromDB.apply { - apiRestUsernamePlain = "" - apiRestPasswordPlain = "" - apiRestUsernameEncrypted = "" - apiRestPasswordEncrypted = "" - apiRestUsernameIV = "" - apiRestPasswordIV = "" - wpApiRestUrl = "" - } - OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteFromDB)) + // Credentials + WP_API_REST_URL are excluded from the full-row write, so clear them on the + // existing row via their targeted writers now that the application password is gone. + val rowsAffected = siteSqlUtils.clearApplicationPasswordCredentials(siteFromDB.id) + + siteSqlUtils.clearWpApiRestUrl(siteFromDB.id) + OnSiteChanged(rowsAffected) } } catch (e: DuplicateSiteException) { OnSiteChanged(SiteError(DUPLICATE_SITE)) @@ -1776,27 +1814,31 @@ open class SiteStore @Inject constructor( val updatedSites = mutableListOf() for (site in sites.sites) { try { - // The REST API doesn't return info about the editor(s) nor the Application Password. - // Make sure to copy current values available on the DB. - // Otherwise the apps will receive an update site without editor prefs set. - // The apps will dispatch the action to update editor(s) when necessary. + // The REST API doesn't return editor prefs, so copy the current values from the DB to avoid + // emitting an updated site without them. (Credentials and wpApiRestUrl are protected by the + // mapper exclusion in SiteSqlUtils, so they don't need copying here.) val siteFromDB = getSiteBySiteId(site.siteId) if (siteFromDB != null) { site.mobileEditor = siteFromDB.mobileEditor site.webEditor = siteFromDB.webEditor - if (!siteFromDB.apiRestUsernameEncrypted.isNullOrEmpty()) { - site.apiRestUsernameEncrypted = siteFromDB.apiRestUsernameEncrypted - site.apiRestPasswordEncrypted = siteFromDB.apiRestPasswordEncrypted - site.apiRestUsernameIV = siteFromDB.apiRestUsernameIV - site.apiRestPasswordIV = siteFromDB.apiRestPasswordIV - site.wpApiRestUrl = siteFromDB.wpApiRestUrl - } } - val isUpdated = (siteSqlUtils.insertOrUpdateSite(site) == 1) - if (isUpdated) { + val localId = siteSqlUtils.insertOrUpdateSiteReturningId(site) + if (localId != 0) { rowsAffected++ updatedSites.add(site) } + // Credentials + wpApiRestUrl are excluded from the full-row write, so when this site carries + // app-password creds (XML-RPC app-password login), persist them on the row that was just + // written (localId) via the targeted writers. + if (localId != 0) { + persistAppPasswordColumns( + site, + { username, password -> + siteSqlUtils.updateApplicationPasswordCredentials(localId, username, password) + }, + { siteSqlUtils.updateWpApiRestUrl(localId, it) } + ) + } } catch (caughtException: DuplicateSiteException) { duplicateSiteFound = true } @@ -2459,19 +2501,9 @@ open class SiteStore @Inject constructor( return try { val siteFromDB = getSiteByLocalId(siteModel.id) ?: return OnSiteChanged(SiteError(SiteErrorType.INVALID_SITE)) - // Clear both Plain and Encrypted columns. `SiteSqlUtils.encryptAPIRestCredentials` - // short-circuits when the encrypted columns are non-empty, so clearing only the plain - // values would leave the stale ciphertext in place and `decryptAPIRestCredentials` - // would resurrect them on the next read. - siteFromDB.apply { - apiRestUsernamePlain = "" - apiRestPasswordPlain = "" - apiRestUsernameEncrypted = "" - apiRestPasswordEncrypted = "" - apiRestUsernameIV = "" - apiRestPasswordIV = "" - } - OnSiteChanged(siteSqlUtils.insertOrUpdateSite(siteFromDB)) + // Credentials are excluded from the full-row write, so clear them on the existing row via the + // targeted writer. wpApiRestUrl is intentionally preserved here — credential rotation reuses it. + OnSiteChanged(siteSqlUtils.clearApplicationPasswordCredentials(siteFromDB.id)) } catch (e: DuplicateSiteException) { OnSiteChanged(SiteError(DUPLICATE_SITE)) } diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt index 28885f9cdf0e..8a3568c8dc3a 100644 --- a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt +++ b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/persistence/SiteSqlUtilsTest.kt @@ -5,6 +5,8 @@ import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test import org.junit.runner.RunWith +import org.mockito.kotlin.mock +import org.mockito.kotlin.whenever import org.robolectric.RobolectricTestRunner import org.robolectric.RuntimeEnvironment import org.wordpress.android.fluxc.encryption.EncryptionUtils @@ -14,6 +16,13 @@ import org.wordpress.android.fluxc.model.SiteModel class SiteSqlUtilsTest { private val siteSqlUtils = SiteSqlUtils(EncryptionUtils()) + // EncryptionUtils uses the AndroidKeyStore provider, which Robolectric can't load, so the production + // util's encrypt/decrypt throw here. A stubbed util lets us exercise the encrypting writers' own + // ContentValues mapping (which half of each encrypt() Pair lands in which column) against a real DB — + // the SiteStore-level tests can't, since they verify a mocked SiteSqlUtils and never run the body. + private val fakeEncryptionUtils = mock() + private val encryptingSiteSqlUtils = SiteSqlUtils(fakeEncryptionUtils) + @Before fun setUp() { val appContext = RuntimeEnvironment.getApplication().applicationContext @@ -49,4 +58,352 @@ class SiteSqlUtilsTest { assertThat(rowsUpdated).isEqualTo(0) } + + @Test + fun `insertOrUpdateSite update does not clobber wpApiRestUrl from a stale model`() { + val healed = "https://example.test/wp-json/" + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + name = "Example" + wpApiRestUrl = healed + }).execute() + // WellSql auto-assigns the primary key on insert, so read back the id it actually allocated. + val localId = siteSqlUtils.getSites().single().id + + // A later writer (e.g. a FETCH_SITE round-trip) carries a stale, null wpApiRestUrl but a fresh name. + val stale = SiteModel().apply { + id = localId + siteId = 42 + url = "https://example.test" + name = "Updated name" + wpApiRestUrl = null + } + val rows = siteSqlUtils.insertOrUpdateSite(stale) + + assertThat(rows).isEqualTo(1) + val stored = siteSqlUtils.getSites().single() + assertThat(stored.wpApiRestUrl).isEqualTo(healed) // preserved + assertThat(stored.name).isEqualTo("Updated name") // other columns still update + } + + @Test + fun `insertOrUpdateSite insert still persists wpApiRestUrl for a new site`() { + val rows = siteSqlUtils.insertOrUpdateSite(SiteModel().apply { + siteId = 99 + url = "https://newsite.test" + name = "New" + wpApiRestUrl = "https://newsite.test/wp-json/" + }) + + assertThat(rows).isEqualTo(1) + assertThat(siteSqlUtils.getSites().single().wpApiRestUrl) + .isEqualTo("https://newsite.test/wp-json/") + } + + @Test + fun `clearWpApiRestUrl empties the stored url`() { + WellSql.insert(SiteModel().apply { + url = "https://example.test" + wpApiRestUrl = "https://example.test/wp-json/" + }).execute() + val localId = siteSqlUtils.getSites().single().id + + val rows = siteSqlUtils.clearWpApiRestUrl(localId) + + assertThat(rows).isEqualTo(1) + assertThat(siteSqlUtils.getSites().single().wpApiRestUrl).isEmpty() + } + + @Test + fun `updateWpApiRestUrlForWPAPISite updates the matching WPAPI site by url`() { + WellSql.insert(SiteModel().apply { + url = "https://selfhosted.test" + origin = SiteModel.ORIGIN_WPAPI + wpApiRestUrl = null + }).execute() + + val rows = siteSqlUtils.updateWpApiRestUrlForWPAPISite( + siteUrl = "https://selfhosted.test", + wpApiRestUrl = "https://selfhosted.test/wp-json/" + ) + + assertThat(rows).isEqualTo(1) + assertThat(siteSqlUtils.getSites().single().wpApiRestUrl) + .isEqualTo("https://selfhosted.test/wp-json/") + } + + @Test + fun `updateWpApiRestUrlForWPAPISite leaves a non-WPAPI site with the same url untouched`() { + WellSql.insert(SiteModel().apply { + url = "https://shared.test" + origin = SiteModel.ORIGIN_WPCOM_REST + wpApiRestUrl = null + }).execute() + + val rows = siteSqlUtils.updateWpApiRestUrlForWPAPISite( + siteUrl = "https://shared.test", + wpApiRestUrl = "https://shared.test/wp-json/" + ) + + assertThat(rows).isEqualTo(0) + assertThat(siteSqlUtils.getSites().single().wpApiRestUrl).isNull() + } + + @Test + fun `insertOrUpdateSite update does not clobber credential columns from a stale model`() { + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + apiRestUsernameEncrypted = "enc_user" + apiRestUsernameIV = "iv_user" + apiRestPasswordEncrypted = "enc_pass" + apiRestPasswordIV = "iv_pass" + }).execute() + val localId = storedSite().id + + // A credential-less inbound model (e.g. a /me/sites sync) must not zero the credential columns. + val stale = SiteModel().apply { + id = localId + siteId = 42 + url = "https://example.test" + name = "Updated name" + } + siteSqlUtils.insertOrUpdateSite(stale) + + val stored = storedSite() + assertThat(stored.apiRestUsernameEncrypted).isEqualTo("enc_user") + assertThat(stored.apiRestUsernameIV).isEqualTo("iv_user") + assertThat(stored.apiRestPasswordEncrypted).isEqualTo("enc_pass") + assertThat(stored.apiRestPasswordIV).isEqualTo("iv_pass") + assertThat(stored.name).isEqualTo("Updated name") // other columns still update + } + + @Test + fun `clearApplicationPasswordCredentials empties the credential columns`() { + WellSql.insert(SiteModel().apply { + url = "https://example.test" + apiRestUsernameEncrypted = "enc_user" + apiRestUsernameIV = "iv_user" + apiRestPasswordEncrypted = "enc_pass" + apiRestPasswordIV = "iv_pass" + }).execute() + val localId = storedSite().id + + val rows = siteSqlUtils.clearApplicationPasswordCredentials(localId) + + assertThat(rows).isEqualTo(1) + val stored = storedSite() + assertThat(stored.apiRestUsernameEncrypted).isEmpty() + assertThat(stored.apiRestUsernameIV).isEmpty() + assertThat(stored.apiRestPasswordEncrypted).isEmpty() + assertThat(stored.apiRestPasswordIV).isEmpty() + } + + @Test + fun `reading a site with credential ciphertext but a blank IV does not attempt decryption`() { + // A row carrying credential ciphertext without its IV is malformed — decrypting it would fail. The + // read path must treat it as having no decryptable credentials instead of throwing. Both ciphertexts + // are present here so only the blank-IV guard (not the empty-ciphertext one) can short-circuit. + WellSql.insert(SiteModel().apply { + url = "https://example.test" + apiRestUsernameEncrypted = "enc_user" + apiRestUsernameIV = "" + apiRestPasswordEncrypted = "enc_pass" + apiRestPasswordIV = "iv_pass" + }).execute() + + val stored = siteSqlUtils.getSites().single() + + assertThat(stored.apiRestUsernamePlain).isNullOrEmpty() + assertThat(stored.apiRestPasswordPlain).isNullOrEmpty() + } + + @Test + fun `updateApplicationPasswordCredentials writes each encrypted value into its matching column`() { + // ciphertext (encrypt().first) -> *_ENCRYPTED column, IV (.second) -> *_IV column, and username + // must not be cross-wired with password. A .first/.second swap, a wrong column, or a dropped + // cv.put would all read back wrong here — the failure modes the SiteStore mock can't catch. + whenever(fakeEncryptionUtils.encrypt("user")).thenReturn("enc_user" to "iv_user") + whenever(fakeEncryptionUtils.encrypt("pass")).thenReturn("enc_pass" to "iv_pass") + WellSql.insert(SiteModel().apply { url = "https://example.test" }).execute() + val localId = storedSite().id + + val rows = encryptingSiteSqlUtils.updateApplicationPasswordCredentials(localId, "user", "pass") + + assertThat(rows).isEqualTo(1) + val stored = storedSite() + assertThat(stored.apiRestUsernameEncrypted).isEqualTo("enc_user") + assertThat(stored.apiRestUsernameIV).isEqualTo("iv_user") + assertThat(stored.apiRestPasswordEncrypted).isEqualTo("enc_pass") + assertThat(stored.apiRestPasswordIV).isEqualTo("iv_pass") + } + + @Test + fun `updateApplicationPasswordCredentials persists credentials that decrypt back to the originals`() { + whenever(fakeEncryptionUtils.encrypt("user")).thenReturn("enc_user" to "iv_user") + whenever(fakeEncryptionUtils.encrypt("pass")).thenReturn("enc_pass" to "iv_pass") + whenever(fakeEncryptionUtils.decrypt("enc_user", "iv_user")).thenReturn("user") + whenever(fakeEncryptionUtils.decrypt("enc_pass", "iv_pass")).thenReturn("pass") + WellSql.insert(SiteModel().apply { url = "https://example.test" }).execute() + val localId = storedSite().id + + encryptingSiteSqlUtils.updateApplicationPasswordCredentials(localId, "user", "pass") + + // Read back through the decrypting path: the stored ciphertext/IV must round-trip to the inputs. + val stored = encryptingSiteSqlUtils.getSites().single() + assertThat(stored.apiRestUsernamePlain).isEqualTo("user") + assertThat(stored.apiRestPasswordPlain).isEqualTo("pass") + } + + @Test + fun `updateApplicationPasswordCredentialsForWPAPISite writes the matching WPAPI site by url`() { + whenever(fakeEncryptionUtils.encrypt("user")).thenReturn("enc_user" to "iv_user") + whenever(fakeEncryptionUtils.encrypt("pass")).thenReturn("enc_pass" to "iv_pass") + WellSql.insert(SiteModel().apply { + url = "https://selfhosted.test" + origin = SiteModel.ORIGIN_WPAPI + }).execute() + + val rows = encryptingSiteSqlUtils.updateApplicationPasswordCredentialsForWPAPISite( + siteUrl = "https://selfhosted.test", + usernamePlain = "user", + passwordPlain = "pass" + ) + + assertThat(rows).isEqualTo(1) + val stored = storedSite() + assertThat(stored.apiRestUsernameEncrypted).isEqualTo("enc_user") + assertThat(stored.apiRestUsernameIV).isEqualTo("iv_user") + assertThat(stored.apiRestPasswordEncrypted).isEqualTo("enc_pass") + assertThat(stored.apiRestPasswordIV).isEqualTo("iv_pass") + } + + @Test + fun `updateApplicationPasswordCredentialsForWPAPISite leaves a non-WPAPI site with the same url untouched`() { + WellSql.insert(SiteModel().apply { + url = "https://shared.test" + origin = SiteModel.ORIGIN_WPCOM_REST + }).execute() + + val rows = encryptingSiteSqlUtils.updateApplicationPasswordCredentialsForWPAPISite( + siteUrl = "https://shared.test", + usernamePlain = "user", + passwordPlain = "pass" + ) + + assertThat(rows).isEqualTo(0) + val stored = storedSite() + assertThat(stored.apiRestUsernameEncrypted).isNullOrEmpty() + assertThat(stored.apiRestPasswordEncrypted).isNullOrEmpty() + } + + @Test + fun `updateXmlRpcUrl writes the column and leaves other fields alone`() { + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + name = "Example" + xmlRpcUrl = null + }).execute() + val localId = siteSqlUtils.getSites().single().id + + val rowsUpdated = siteSqlUtils.updateXmlRpcUrl(localId, "https://example.test/xmlrpc.php") + + assertThat(rowsUpdated).isEqualTo(1) + val stored = siteSqlUtils.getSites().single() + assertThat(stored.xmlRpcUrl).isEqualTo("https://example.test/xmlrpc.php") + assertThat(stored.url).isEqualTo("https://example.test") + assertThat(stored.name).isEqualTo("Example") + } + + @Test + fun `updateXmlRpcUrl returns 0 when no site row matches the local id`() { + val rowsUpdated = siteSqlUtils.updateXmlRpcUrl(localId = 999, xmlRpcUrl = "https://example.test/xmlrpc.php") + + assertThat(rowsUpdated).isEqualTo(0) + } + + @Test + fun `insertOrUpdateSite update does not clobber xmlRpcUrl from a stale model`() { + // Absence is preserved: a partial writer that doesn't carry xmlRpcUrl (e.g. the WPAPI fetch) must + // not clear a stored/rediscovered value, even though XMLRPC_URL is not excluded from the mapper. + val xmlRpc = "https://example.test/xmlrpc.php" + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + name = "Example" + xmlRpcUrl = xmlRpc + }).execute() + val localId = siteSqlUtils.getSites().single().id + + val stale = SiteModel().apply { + id = localId + siteId = 42 + url = "https://example.test" + name = "Updated name" + xmlRpcUrl = null + } + siteSqlUtils.insertOrUpdateSite(stale) + + val stored = siteSqlUtils.getSites().single() + assertThat(stored.xmlRpcUrl).isEqualTo(xmlRpc) // preserved + assertThat(stored.name).isEqualTo("Updated name") // other columns still update + } + + @Test + fun `insertOrUpdateSite update persists a changed xmlRpcUrl carried by the inbound model`() { + // Presence is authoritative: the WP.com sync reliably carries meta.links.xmlrpc, so a non-empty + // inbound value (e.g. a domain migration) must overwrite the stored one — only absence is preserved. + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + xmlRpcUrl = "https://example.test/xmlrpc.php" + }).execute() + val localId = siteSqlUtils.getSites().single().id + + val migrated = SiteModel().apply { + id = localId + siteId = 42 + url = "https://example.test" + xmlRpcUrl = "https://migrated.test/xmlrpc.php" + } + siteSqlUtils.insertOrUpdateSite(migrated) + + assertThat(siteSqlUtils.getSites().single().xmlRpcUrl) + .isEqualTo("https://migrated.test/xmlrpc.php") + } + + @Test + fun `insertOrUpdateSiteReturningId returns the matched row id on update`() { + WellSql.insert(SiteModel().apply { + siteId = 42 + url = "https://example.test" + }).execute() + val existingId = siteSqlUtils.getSites().single().id + + // A fresh inbound model (id = 0) that matches the existing row by SITE_ID + URL. + val returnedId = siteSqlUtils.insertOrUpdateSiteReturningId(SiteModel().apply { + siteId = 42 + url = "https://example.test" + name = "Updated" + }) + + assertThat(returnedId).isEqualTo(existingId) + } + + @Test + fun `insertOrUpdateSiteReturningId returns the new row id on insert`() { + val returnedId = siteSqlUtils.insertOrUpdateSiteReturningId(SiteModel().apply { + siteId = 99 + url = "https://newsite.test" + }) + + assertThat(returnedId).isNotEqualTo(0) + assertThat(siteSqlUtils.getSites().single().id).isEqualTo(returnedId) + } + + // Raw read that bypasses SiteSqlUtils' decryptAPIRestCredentials, so tests can assert on the stored + // ciphertext columns directly without invoking the AndroidKeyStore-backed EncryptionUtils. + private fun storedSite(): SiteModel = WellSql.select(SiteModel::class.java).asModel.single() } diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/ReactNativeStoreWPAPITest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/ReactNativeStoreWPAPITest.kt index 92b65c7f4d1c..7407019cc9a9 100644 --- a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/ReactNativeStoreWPAPITest.kt +++ b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/ReactNativeStoreWPAPITest.kt @@ -15,8 +15,8 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.robolectric.RobolectricTestRunner import org.robolectric.annotation.Config -import org.wordpress.android.fluxc.TestSiteSqlUtils import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.persistence.SiteSqlUtils import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.UNKNOWN import org.wordpress.android.fluxc.network.discovery.DiscoveryWPAPIRestClient import org.wordpress.android.fluxc.network.rest.wpapi.Nonce @@ -49,9 +49,7 @@ class ReactNativeStoreWPAPITest { private lateinit var store: ReactNativeStore private lateinit var site: SiteModel - private interface SitePersister : (SiteModel) -> Int - - private lateinit var sitePersistenceMock: SitePersister + private val siteSqlUtils = mock() @Before fun setup() { @@ -65,16 +63,14 @@ class ReactNativeStoreWPAPITest { private fun initStore(nonce: Nonce?) { whenever(nonceRestClient.getNonce(any())).thenReturn(nonce) - sitePersistenceMock = mock() store = ReactNativeStore( mock(), wpApiRestClient, nonceRestClient, discoveryWPAPIRestClient, - TestSiteSqlUtils.siteSqlUtils, + siteSqlUtils, initCoroutineEngine(), - { currentTime }, - sitePersistenceMock + { currentTime } ) } @@ -102,9 +98,9 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executeGetRequest(site, restPathWithParams) assertEquals(callWithSuccess, actualResponse) assertEquals(restUrl, site.wpApiRestUrl, "site should be updated with rest endpoint used for successful call") - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient, nonceRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient, nonceRestClient) { verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist site after discovering wpApiRestUrl + verify(siteSqlUtils).updateWpApiRestUrl(site.id, restUrl) // persist discovered wpApiRestUrl verify(nonceRestClient).requestNonce(site) verify(wpApiRestClient).getRequest(fetchUrl, nonce.value) } @@ -134,9 +130,9 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executePostRequest(site, restPathWithParams, bodyMap) assertEquals(callWithSuccess, actualResponse) assertEquals(restUrl, site.wpApiRestUrl, "site should be updated with rest endpoint used for successful call") - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient, nonceRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient, nonceRestClient) { verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist site after discovering wpApiRestUrl + verify(siteSqlUtils).updateWpApiRestUrl(site.id, restUrl) // persist discovered wpApiRestUrl verify(nonceRestClient).requestNonce(site) verify(wpApiRestClient).postRequest(postURL, nonce.value) } @@ -154,7 +150,7 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executeGetRequest(site, restPathWithParams) assertEquals(initialResponseWithSuccess, actualResponse) verify(wpApiRestClient).getRequest(fetchUrl) - verify(sitePersistenceMock, never())(any()) // no wpApiRestUrl updates, so no persistence + verify(siteSqlUtils, never()).updateWpApiRestUrl(any(), any()) // no wpApiRestUrl updates, so no persistence verify(discoveryWPAPIRestClient, never()).discoverWPAPIBaseURL(any()) } @@ -177,9 +173,9 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executeGetRequest(site, restPathWithParams) assertEquals(initialResponseWithSuccess, actualResponse) assertEquals(restUrl, site.wpApiRestUrl, "site should be updated with rest endpoint used for successful call") - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient) { verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist site after discovering wpApiRestUrl + verify(siteSqlUtils).updateWpApiRestUrl(site.id, restUrl) // persist discovered wpApiRestUrl verify(wpApiRestClient).getRequest(fetchUrl) } } @@ -206,9 +202,9 @@ class ReactNativeStoreWPAPITest { fallbackRestUrl, site.wpApiRestUrl, "site should be updated with rest endpoint used for successful call" ) - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient) { verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist default endpoint after failed discovery + verify(siteSqlUtils).updateWpApiRestUrl(site.id, fallbackRestUrl) // persist fallback endpoint verify(wpApiRestClient).getRequest(fetchUrl) } } @@ -239,11 +235,10 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executeGetRequest(site, restPathWithParams) assertEquals(secondResponseWithSuccess, actualResponse) assertEquals(restUrl, site.wpApiRestUrl, "should save rest endpoint used for successful call") - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient) { verify(wpApiRestClient).getRequest(incorrectUrl) - verify(sitePersistenceMock)(site) // persist site after clearing wpApiRestUrl that resulted in 404 failure verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist site after discovering wpApiRestUrl + verify(siteSqlUtils).updateWpApiRestUrl(site.id, restUrl) // persist discovered wpApiRestUrl verify(wpApiRestClient).getRequest(correctUrl) } } @@ -268,11 +263,10 @@ class ReactNativeStoreWPAPITest { val actualResponse = store.executeGetRequest(site, restPathWithParams) assertEquals(responseWithNotFoundError, actualResponse) assertNull(site.wpApiRestUrl, "should not update site wpApiRestEndpoint when call fails") - inOrder(discoveryWPAPIRestClient, sitePersistenceMock, wpApiRestClient) { + inOrder(discoveryWPAPIRestClient, siteSqlUtils, wpApiRestClient) { verify(discoveryWPAPIRestClient).discoverWPAPIBaseURL(site.url) - verify(sitePersistenceMock)(site) // persist site after discovering wpApiRestUrl + verify(siteSqlUtils).updateWpApiRestUrl(site.id, restUrl) // persist discovered wpApiRestUrl verify(wpApiRestClient).getRequest(fetchUrl) - verify(sitePersistenceMock)(site) // persist site after clearing wpApiRestUrl that resulted in 404 failure } } @@ -483,10 +477,9 @@ class ReactNativeStoreWPAPITest { wpApiRestClient, nonceRestClient, discoveryWPAPIRestClient, - TestSiteSqlUtils.siteSqlUtils, + siteSqlUtils, initCoroutineEngine(), { currentTime }, - sitePersistenceMock, uriParser ) diff --git a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt index 68cdc7821aa0..8e173c0d0f97 100644 --- a/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt +++ b/libs/fluxc/src/test/java/org/wordpress/android/fluxc/store/SiteStoreTest.kt @@ -11,11 +11,13 @@ import org.mockito.junit.MockitoJUnitRunner import org.mockito.kotlin.any import org.mockito.kotlin.argWhere import org.mockito.kotlin.inOrder +import org.mockito.kotlin.mock import org.mockito.kotlin.never import org.mockito.kotlin.verify import org.mockito.kotlin.verifyNoInteractions import org.mockito.kotlin.whenever import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.generated.SiteActionBuilder import org.wordpress.android.fluxc.model.PostFormatModel import org.wordpress.android.fluxc.model.SiteModel import org.wordpress.android.fluxc.model.SitesModel @@ -24,6 +26,7 @@ import org.wordpress.android.fluxc.model.jetpacksocial.JetpackSocialMapper import org.wordpress.android.fluxc.network.BaseRequest.BaseNetworkError import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.NETWORK_ERROR import org.wordpress.android.fluxc.network.BaseRequest.GenericErrorType.PARSE_ERROR +import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsManager import org.wordpress.android.fluxc.network.rest.wpapi.site.SiteWPAPIRestClient import org.wordpress.android.fluxc.network.rest.wpcom.WPComGsonRequest.WPComGsonNetworkError import org.wordpress.android.fluxc.network.rest.wpcom.WPComGsonRequestBuilder.Response @@ -61,6 +64,7 @@ import org.wordpress.android.fluxc.store.SiteStore.SiteFilter.WPCOM import org.wordpress.android.fluxc.store.SiteStore.SiteVisibility.PUBLIC import org.wordpress.android.fluxc.test import org.wordpress.android.fluxc.tools.initCoroutineEngine +import javax.inject.Provider import kotlin.test.assertEquals @RunWith(MockitoJUnitRunner::class) @@ -176,16 +180,16 @@ class SiteStoreTest { val siteB = SiteModel() sitesModel.sites = listOf(siteA, siteB) whenever(siteRestClient.fetchSites(payload.filters, false)).thenReturn(sitesModel) - whenever(siteSqlUtils.insertOrUpdateSite(siteA)).thenReturn(1) - whenever(siteSqlUtils.insertOrUpdateSite(siteB)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteA)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteB)).thenReturn(1) val onSiteChanged = siteStore.fetchSites(payload) assertThat(onSiteChanged.rowsAffected).isEqualTo(2) assertThat(onSiteChanged.error).isNull() val inOrder = inOrder(siteSqlUtils) - inOrder.verify(siteSqlUtils).insertOrUpdateSite(siteA) - inOrder.verify(siteSqlUtils).insertOrUpdateSite(siteB) + inOrder.verify(siteSqlUtils).insertOrUpdateSiteReturningId(siteA) + inOrder.verify(siteSqlUtils).insertOrUpdateSiteReturningId(siteB) inOrder.verify(siteSqlUtils).removeWPComRestSitesAbsentFromList(postSqlUtils, sitesModel.sites) } @@ -218,8 +222,8 @@ class SiteStoreTest { sitesModel.sites = listOf(siteA, siteB) sitesModel.jetpackCPSites = listOf(siteC) whenever(siteRestClient.fetchSites(payload.filters, false)).thenReturn(sitesModel) - whenever(siteSqlUtils.insertOrUpdateSite(siteA)).thenReturn(1) - whenever(siteSqlUtils.insertOrUpdateSite(siteB)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteA)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteB)).thenReturn(1) siteStore.fetchSites(payload) @@ -248,8 +252,8 @@ class SiteStoreTest { } sitesModel.sites = listOf(siteA, siteB) whenever(siteRestClient.fetchSites(payload.filters, false)).thenReturn(sitesModel) - whenever(siteSqlUtils.insertOrUpdateSite(siteA)).thenReturn(1) - whenever(siteSqlUtils.insertOrUpdateSite(siteB)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteA)).thenReturn(1) + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(siteB)).thenReturn(1) siteStore.fetchSites(payload) @@ -575,8 +579,96 @@ class SiteStoreTest { .isEqualTo("appPass") assertThat(fetchedSite.wpApiRestUrl) .isEqualTo("https://example.com/wp-json/") + // updateSite's full-row write skips WP_API_REST_URL, so the discovered URL must be persisted + // via the URL-keyed targeted writer (the fetched site has no local id to key on). + verify(siteSqlUtils).updateWpApiRestUrlForWPAPISite( + "https://example.com", + "https://example.com/wp-json/" + ) + verify(siteSqlUtils).updateApplicationPasswordCredentialsForWPAPISite( + "https://example.com", + "appUser", + "appPass" + ) } + @Test + fun `updateApplicationPassword persists credentials and wpApiRestUrl via targeted writers`() { + val existing = SiteModel().apply { + id = 3 + url = "https://selfhosted.test" + } + whenever(siteSqlUtils.getSitesWithLocalId(3)).thenReturn(listOf(existing)) + val incoming = SiteModel().apply { + id = 3 + apiRestUsernamePlain = "user" + apiRestPasswordPlain = "pass" + wpApiRestUrl = "https://selfhosted.test/wp-json/" + } + + siteStore.onAction(SiteActionBuilder.newUpdateApplicationPasswordAction(incoming)) + + verify(siteSqlUtils).updateApplicationPasswordCredentials(3, "user", "pass") + verify(siteSqlUtils).updateWpApiRestUrl(3, "https://selfhosted.test/wp-json/") + } + + @Test + fun `removeApplicationPassword clears credentials and wpApiRestUrl via targeted writers`() { + val existing = SiteModel().apply { + id = 4 + url = "https://selfhosted.test" + wpApiRestUrl = "https://selfhosted.test/wp-json/" + } + whenever(siteSqlUtils.getSitesWithLocalId(4)).thenReturn(listOf(existing)) + siteStore.onAction( + SiteActionBuilder.newRemoveApplicationPasswordAction(SiteModel().apply { id = 4 }) + ) + + verify(siteSqlUtils).clearApplicationPasswordCredentials(4) + verify(siteSqlUtils).clearWpApiRestUrl(4) + } + + @Test + fun `clearApplicationPasswordColumns clears credentials but preserves wpApiRestUrl`() { + val existing = SiteModel().apply { + id = 8 + url = "https://selfhosted.test" + } + whenever(siteSqlUtils.getSitesWithLocalId(8)).thenReturn(listOf(existing)) + siteStore.applicationPasswordsManagerProvider = Provider { mock() } + + siteStore.deleteStoredApplicationPasswordCredentials(SiteModel().apply { id = 8 }) + + verify(siteSqlUtils).clearApplicationPasswordCredentials(8) + verify(siteSqlUtils, never()).clearWpApiRestUrl(8) + } + + @Test + fun `persistXmlRpcUrl writes the column via the targeted writer`() = test { + siteStore.persistXmlRpcUrl(9, "https://selfhosted.test/xmlrpc.php") + + verify(siteSqlUtils).updateXmlRpcUrl(9, "https://selfhosted.test/xmlrpc.php") + } + + @Test + fun `createOrUpdateSites persists app-password credentials on the stamped row`() { + val appPwSite = SiteModel().apply { + selfHostedSiteId = 77L + url = "https://selfhosted.test" + xmlRpcUrl = "https://selfhosted.test/xmlrpc.php" + origin = SiteModel.ORIGIN_XMLRPC + apiRestUsernamePlain = "user" + apiRestPasswordPlain = "pass" + wpApiRestUrl = "https://selfhosted.test/wp-json/" + } + whenever(siteSqlUtils.insertOrUpdateSiteReturningId(any())).thenReturn(9) + + siteStore.onAction(SiteActionBuilder.newUpdateSitesAction(SitesModel(listOf(appPwSite)))) + + verify(siteSqlUtils).updateApplicationPasswordCredentials(9, "user", "pass") + verify(siteSqlUtils).updateWpApiRestUrl(9, "https://selfhosted.test/wp-json/") + } + @Test fun `fetchPostFormats returns empty list for WPAPI site without crashing`() = test {