Hardening for the Atomic application-password validator and cleanup flow

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

@@ -1,18 +1,28 @@
 package org.wordpress.android.ui.mysite.cards.applicationpassword
 
+import kotlinx.coroutines.CancellationException
 import org.wordpress.android.fluxc.model.SiteModel
 import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider
 import org.wordpress.android.fluxc.utils.AppLogWrapper
 import org.wordpress.android.util.AppLog
 import rs.wordpress.api.kotlin.WpRequestResult
 import uniffi.wp_api.RequestExecutionErrorReason
+import uniffi.wp_api.WpErrorCode
+import java.net.HttpURLConnection
 import javax.inject.Inject
 
 /**
  * Validates that the SiteModel's application-password credentials still work against the site's
  * direct host. Uses [WpApiClientProvider.getApplicationPasswordClient] so the call exercises the
  * application password specifically — `getWpApiClient` would route WPCom-flagged sites through the
  * bearer-token path and would not catch a revoked password.
+ *
+ * Classification is intentionally asymmetric: a return of [Outcome.Invalid] cascades into a
+ * credential wipe + re-mint in the caller, so we only classify as Invalid when we have positive
+ * evidence the server rejected the credential — an auth-specific [WpErrorCode], an auth-specific
+ * [RequestExecutionErrorReason], or a [WpRequestResult.WpError] with a 401/403 status. Everything
+ * ambiguous — 5xx, parse errors, offline, DNS — falls to [Outcome.NetworkUnavailable], which
+ * hides the card and lets the next foreground retry.
  */
 class ApplicationPasswordValidator @Inject constructor(
     private val wpApiClientProvider: WpApiClientProvider,
@@ -27,25 +37,9 @@ class ApplicationPasswordValidator @Inject constructor(
             val client = wpApiClientProvider.getApplicationPasswordClient(site)
             val response = client.request { it.users().retrieveMeWithViewContext() }
             appLogWrapper.d(AppLog.T.MAIN, "A_P: Validation response: ${response::class.simpleName}")
-            when (response) {
-                is WpRequestResult.Success -> {
-                    val user = response.response.data
-                    appLogWrapper.d(
-                        AppLog.T.MAIN,
-                        "A_P: Validation Success returned user id=${user.id}, slug='${user.slug}', name='${user.name}'"
-                    )
-                    Outcome.Valid
-                }
-                is WpRequestResult.WpError -> Outcome.Invalid
-                is WpRequestResult.UnknownError -> Outcome.Invalid
-                is WpRequestResult.RequestExecutionFailed ->
-                    if (response.reason is RequestExecutionErrorReason.HttpTimeoutError) {
-                        Outcome.NetworkUnavailable
-                    } else {
-                        Outcome.Invalid
-                    }
-                else -> Outcome.Invalid
-            }
+            classify(response)
+        } catch (e: CancellationException) {
+            throw e
         } catch (@Suppress("TooGenericExceptionCaught") e: Exception) {
             appLogWrapper.e(
                 AppLog.T.MAIN,
@@ -55,5 +49,53 @@ class ApplicationPasswordValidator @Inject constructor(
         }
     }
 
+    private fun <T> classify(response: WpRequestResult<T>): Outcome = when (response) {
+        is WpRequestResult.Success -> Outcome.Valid
+
+        // A WpError is the server returning a parseable error envelope. Treat any 401/403 as an
+        // auth rejection regardless of the WpErrorCode value — WordPress emits a wide variety of
+        // codes for credential failures (`incorrect_password`, `invalid_username`,
+        // `application_passwords_disabled_for_user`, plugin-defined codes, etc.) and many of them
+        // get parsed as `WpErrorCode.CustomError` via the library's untagged fallback. Status
+        // code is the reliable signal. Additionally, accept a small allowlist of WpErrorCode
+        // names (e.g. `ApplicationPasswordNotFound`, which comes back with 404) that signal an
+        // unusable credential outside the 401/403 status range.
+        is WpRequestResult.WpError -> if (
+            isAuthErrorCode(response.errorCode) || isAuthStatusCode(response.statusCode)
+        ) {
+            Outcome.Invalid
+        } else {
+            // Parseable error envelope without an auth-rejection signal (e.g. a 5xx returned as
+            // a structured WpError). Ambiguous — don't wipe creds.
+            Outcome.NetworkUnavailable
+        }
+
+        is WpRequestResult.RequestExecutionFailed -> if (isAuthErrorReason(response.reason)) {
+            Outcome.Invalid
+        } else {
+            // Timeouts, offline, DNS, SSL, generic transport errors — all non-destructive.
+            Outcome.NetworkUnavailable
+        }
+
+        // UnknownError (4xx/5xx without parseable WP error JSON), parse errors, and any other
+        // variants are all ambiguous. Default to non-destructive.
+        else -> Outcome.NetworkUnavailable
+    }
+
+    private fun isAuthErrorCode(code: WpErrorCode): Boolean =
+        code is WpErrorCode.Unauthorized ||
+            code is WpErrorCode.Forbidden ||
+            code is WpErrorCode.ApplicationPasswordNotFound ||
+            code is WpErrorCode.NoAuthenticatedAppPassword
+
+    private fun isAuthStatusCode(statusCode: UInt): Boolean =
+        statusCode.toInt() == HttpURLConnection.HTTP_UNAUTHORIZED ||
+            statusCode.toInt() == HttpURLConnection.HTTP_FORBIDDEN
+
+    private fun isAuthErrorReason(reason: RequestExecutionErrorReason): Boolean =
+        reason is RequestExecutionErrorReason.HttpAuthenticationRejectedError ||
+            reason is RequestExecutionErrorReason.HttpAuthenticationRequiredError ||
+            reason is RequestExecutionErrorReason.HttpForbiddenError
+
     enum class Outcome { Valid, Invalid, NetworkUnavailable }
 }

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

@@ -0,0 +1,277 @@
+package org.wordpress.android.ui.mysite.cards.applicationpassword
+
+import kotlinx.coroutines.CancellationException
+import kotlinx.coroutines.ExperimentalCoroutinesApi
+import kotlinx.coroutines.test.runTest
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.Before
+import org.junit.Test
+import org.mockito.Mock
+import org.mockito.MockitoAnnotations
+import org.mockito.kotlin.any
+import org.mockito.kotlin.whenever
+import org.wordpress.android.BaseUnitTest
+import org.wordpress.android.fluxc.model.SiteModel
+import org.wordpress.android.fluxc.network.rest.wpapi.rs.WpApiClientProvider
+import org.wordpress.android.fluxc.utils.AppLogWrapper
+import rs.wordpress.api.kotlin.WpApiClient
+import rs.wordpress.api.kotlin.WpRequestResult
+import uniffi.wp_api.RequestExecutionErrorReason
+import uniffi.wp_api.RequestMethod
+import uniffi.wp_api.WpErrorCode
+
+@ExperimentalCoroutinesApi
+class ApplicationPasswordValidatorTest : BaseUnitTest() {
+    @Mock
+    lateinit var wpApiClientProvider: WpApiClientProvider
+
+    @Mock
+    lateinit var appLogWrapper: AppLogWrapper
+
+    @Mock
+    lateinit var wpApiClient: WpApiClient
+
+    private lateinit var validator: ApplicationPasswordValidator
+    private lateinit var site: SiteModel
+
+    @Before
+    fun setUp() {
+        MockitoAnnotations.openMocks(this)
+        validator = ApplicationPasswordValidator(wpApiClientProvider, appLogWrapper)
+        site = SiteModel().apply {
+            id = 1
+            url = "https://example.com"
+            apiRestUsernamePlain = "user"
+            apiRestPasswordPlain = "pass"
+        }
+        whenever(wpApiClientProvider.getApplicationPasswordClient(site)).thenReturn(wpApiClient)
+    }
+
+    @Suppress("UNCHECKED_CAST")
+    private suspend fun stubResponse(response: WpRequestResult<*>) {
+        whenever(wpApiClient.request<Any>(any())).thenReturn(response as WpRequestResult<Any>)
+    }
+
+    // Default to a non-auth status so WpErrorCode-only tests isolate the code path from the
+    // status path. Tests of the status path (401/403) pass an explicit value.
+    private fun wpError(code: WpErrorCode, statusCode: Int = 500) = WpRequestResult.WpError<Any>(
+        errorCode = code,
+        errorMessage = "msg",
+        statusCode = statusCode.toUInt(),
+        response = "",
+        requestUrl = "https://example.com",
+        requestMethod = RequestMethod.GET,
+    )
+
+    private fun requestFailed(reason: RequestExecutionErrorReason) =
+        WpRequestResult.RequestExecutionFailed<Any>(
+            statusCode = null,
+            redirects = null,
+            reason = reason,
+            requestUrl = "https://example.com",
+            requestMethod = RequestMethod.GET,
+        )
+
+    // --- Success ---
+
+    @Test
+    fun `Success maps to Valid`() = runTest {
+        stubResponse(WpRequestResult.Success<Any>(response = Any()))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Valid)
+    }
+
+    // --- WpError: auth-related codes map to Invalid ---
+
+    @Test
+    fun `WpError Unauthorized maps to Invalid`() = runTest {
+        stubResponse(wpError(WpErrorCode.Unauthorized()))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `WpError Forbidden maps to Invalid`() = runTest {
+        stubResponse(wpError(WpErrorCode.Forbidden()))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `WpError ApplicationPasswordNotFound maps to Invalid`() = runTest {
+        stubResponse(wpError(WpErrorCode.ApplicationPasswordNotFound()))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `WpError NoAuthenticatedAppPassword maps to Invalid`() = runTest {
+        stubResponse(wpError(WpErrorCode.NoAuthenticatedAppPassword()))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    // --- WpError: non-auth codes must NOT wipe creds ---
+
+    @Test
+    fun `WpError non-auth code with non-auth status maps to NetworkUnavailable`() = runTest {
+        // Non-401/403 status with an unrelated WpErrorCode: ambiguous, don't wipe creds.
+        stubResponse(wpError(WpErrorCode.InvalidParam(), statusCode = 400))
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `WpError with 401 status maps to Invalid regardless of code`() = runTest {
+        // WordPress emits a wide range of WpErrorCodes for credential rejections (e.g.
+        // `incorrect_password`, `invalid_username`, plugin-defined codes). Many fall through
+        // to wordpress-rs's untagged-string fallback and aren't recognized as auth codes by
+        // name. The status code is the reliable signal — a parseable WpError with 401/403 is
+        // always an auth rejection regardless of which WpErrorCode it carries.
+        stubResponse(wpError(WpErrorCode.InvalidParam(), statusCode = 401))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `WpError with 403 status maps to Invalid regardless of code`() = runTest {
+        stubResponse(wpError(WpErrorCode.InvalidParam(), statusCode = 403))
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `WpError with 500 status maps to NetworkUnavailable`() = runTest {
+        // A server returning a structured WpError on 5xx is unusual but possible. Without an
+        // auth-status signal and without an auth code, treat it as transient.
+        stubResponse(wpError(WpErrorCode.InvalidParam(), statusCode = 500))
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    // --- RequestExecutionFailed: auth-related reasons map to Invalid ---
+
+    @Test
+    fun `RequestExecutionFailed HttpAuthenticationRejectedError maps to Invalid`() = runTest {
+        stubResponse(
+            requestFailed(
+                RequestExecutionErrorReason.HttpAuthenticationRejectedError(
+                    hostname = "example.com",
+                    method = null,
+                )
+            )
+        )
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `RequestExecutionFailed HttpAuthenticationRequiredError maps to Invalid`() = runTest {
+        stubResponse(
+            requestFailed(
+                RequestExecutionErrorReason.HttpAuthenticationRequiredError(
+                    hostname = "example.com",
+                    method = null,
+                )
+            )
+        )
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    @Test
+    fun `RequestExecutionFailed HttpForbiddenError maps to Invalid`() = runTest {
+        stubResponse(
+            requestFailed(RequestExecutionErrorReason.HttpForbiddenError(hostname = "example.com"))
+        )
+        assertThat(validator.validate(site)).isEqualTo(ApplicationPasswordValidator.Outcome.Invalid)
+    }
+
+    // --- RequestExecutionFailed: non-auth reasons must NOT wipe creds ---
+
+    @Test
+    fun `RequestExecutionFailed HttpTimeoutError maps to NetworkUnavailable`() = runTest {
+        stubResponse(requestFailed(RequestExecutionErrorReason.HttpTimeoutError))
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `RequestExecutionFailed DeviceIsOfflineError maps to NetworkUnavailable`() = runTest {
+        stubResponse(
+            requestFailed(RequestExecutionErrorReason.DeviceIsOfflineError(errorMessage = "off"))
+        )
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `RequestExecutionFailed NonExistentSiteError maps to NetworkUnavailable`() = runTest {
+        stubResponse(
+            requestFailed(
+                RequestExecutionErrorReason.NonExistentSiteError(
+                    errorMessage = null,
+                    suggestedAction = null,
+                )
+            )
+        )
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `RequestExecutionFailed HttpError maps to NetworkUnavailable`() = runTest {
+        stubResponse(requestFailed(RequestExecutionErrorReason.HttpError(reason = "boom")))
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    // --- Other variants: all default to NetworkUnavailable ---
+
+    @Test
+    fun `UnknownError (5xx without parseable JSON) maps to NetworkUnavailable`() = runTest {
+        stubResponse(
+            WpRequestResult.UnknownError<Any>(
+                statusCode = 503.toUInt(),
+                response = "<html>Service Unavailable</html>",
+                requestUrl = "https://example.com",
+                requestMethod = RequestMethod.GET,
+            )
+        )
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `InvalidHttpStatusCode maps to NetworkUnavailable`() = runTest {
+        stubResponse(
+            WpRequestResult.InvalidHttpStatusCode<Any>(
+                statusCode = 999.toUInt(),
+                requestUrl = "https://example.com",
+                requestMethod = RequestMethod.GET,
+            )
+        )
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test
+    fun `ResponseParsingError maps to NetworkUnavailable`() = runTest {
+        stubResponse(
+            WpRequestResult.ResponseParsingError<Any>(
+                reason = "bad json",
+                response = "not json",
+                requestUrl = "https://example.com",
+                requestMethod = RequestMethod.GET,
+            )
+        )
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    // --- Exception handling ---
+
+    @Test
+    fun `generic Exception maps to NetworkUnavailable`() = runTest {
+        whenever(wpApiClient.request<Any>(any())).thenThrow(RuntimeException("boom"))
+        assertThat(validator.validate(site))
+            .isEqualTo(ApplicationPasswordValidator.Outcome.NetworkUnavailable)
+    }
+
+    @Test(expected = CancellationException::class)
+    fun `CancellationException is rethrown, not swallowed`() = runTest {
+        whenever(wpApiClient.request<Any>(any())).thenThrow(CancellationException("cancelled"))
+        validator.validate(site)
+    }
+}