Make wpApiRestUrl, xmlRpcUrl, and app-password creds single-writer

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")

WordPress/src/test/java/org/wordpress/android/ui/mysite/cards/applicationpassword/ApplicationPasswordViewModelSliceTest.kt

@@ -19,7 +19,6 @@
 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 @@
     @Mock
     lateinit var siteApiRestUrlRecoverer: SiteApiRestUrlRecoverer
 
-    @Mock
-    lateinit var dispatcher: Dispatcher
-
     @Mock
     lateinit var credentialsChangedNotifier: CredentialsChangedNotifier
 
@@ -96,7 +92,6 @@
             selfHostedEndpointFinder,
             siteXMLRPCClient,
             siteApiRestUrlRecoverer,
-            dispatcher,
             credentialsChangedNotifier,
             testDispatcher()
         ).apply {
@@ -380,8 +375,10 @@
     }
 
     @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 @@
                     eq(xmlRpcUrl), any(), any()
                 )
             ).thenReturn(SitesModel(listOf(SiteModel())))
+            whenever(siteStore.persistXmlRpcUrl(any(), any())).thenReturn(mock())
 
             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 @@
             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 @@
 
             verify(selfHostedEndpointFinder)
                 .verifyOrDiscoverXMLRPCEndpoint(TEST_URL)
-            verify(dispatcher, never()).dispatch(any())
+            verify(siteStore, never()).persistXmlRpcUrl(any(), any())
             assert(siteTest.xmlRpcUrl.isNullOrEmpty())
         }
 }

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

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<SiteModel>
         get() = WellSql.select(SiteModel::class.java)
                 .where().beginGroup()
@@ -579,8 +721,11 @@ class SiteSqlUtils
         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)

libs/fluxc/src/main/java/org/wordpress/android/fluxc/persistence/UpdateAllExceptId.java

@@ -8,15 +8,29 @@
 
 class UpdateAllExceptId<T> implements InsertMapper<T> {
     private final SQLiteMapper<T> mMapper;
+    private final String[] mAdditionalColumnsToSkip;
 
     UpdateAllExceptId(Class<T> 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<T> 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;
     }
 }