Skip to content

Commit 370a6d4

Browse files
hichamboushabahichamboushaba
authored
Merge pull request #14552 from woocommerce/issue/WOOMOB-1180-app-passwords-error-handling
[Jetpack Performance] App Passwords error handling
2 parents 3d6edae + c977e88 commit 370a6d4

File tree

9 files changed

+274
-63
lines changed

9 files changed

+274
-63
lines changed

‎libs/fluxc-plugin/src/main/kotlin/org/wordpress/android/fluxc/network/rest/wpcom/wc/WooNetwork.kt‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,15 @@
11
package org.wordpress.android.fluxc.network.rest.wpcom.wc
22

3-
import kotlinx.coroutines.Dispatchers
4-
import kotlinx.coroutines.withContext
53
import org.wordpress.android.fluxc.model.SiteModel
64
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPINetwork
75
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPINetworkError
86
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPINetworkingMode
97
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIResponse
108
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsConfiguration
119
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsNetwork
10+
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.JetpackApplicationPasswordsErrorHandler
11+
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.JetpackApplicationPasswordsSupport
1212
import org.wordpress.android.fluxc.network.rest.wpcom.JetpackTunnelWPAPINetwork
13-
import org.wordpress.android.fluxc.persistence.SiteSqlUtils
1413
import org.wordpress.android.util.AppLog
1514
import javax.inject.Inject
1615
import javax.inject.Singleton
@@ -30,7 +29,8 @@ class WooNetwork @Inject constructor(
3029
private val applicationPasswordsConfiguration: ApplicationPasswordsConfiguration,
3130
private val applicationPasswordsNetwork: ApplicationPasswordsNetwork,
3231
private val jetpackTunnelWPAPINetwork: JetpackTunnelWPAPINetwork,
33-
private val siteSqlUtils: SiteSqlUtils
32+
private val jetpackApplicationPasswordsSupport: JetpackApplicationPasswordsSupport,
33+
private val jetpackApplicationPasswordsErrorHandler: JetpackApplicationPasswordsErrorHandler
3434
) : WPAPINetwork {
3535
override suspend fun <T : Any> executeGetGsonRequest(
3636
site: SiteModel,
@@ -128,7 +128,7 @@ class WooNetwork @Inject constructor(
128128
request: suspend WPAPINetwork.() -> WPAPIResponse<T>
129129
): WPAPIResponse<T> {
130130
val appPasswordsEnabled = applicationPasswordsConfiguration.isEnabledForJetpackAccess()
131-
if (!appPasswordsEnabled || !site.isApplicationPasswordsSupported) {
131+
if (!appPasswordsEnabled || !jetpackApplicationPasswordsSupport.supportsAppPasswords(site)) {
132132
if (appPasswordsEnabled) {
133133
AppLog.v(
134134
AppLog.T.API,
@@ -150,15 +150,12 @@ class WooNetwork @Inject constructor(
150150
is WPAPIResponse.Error<*> -> {
151151
logMessageForFailedRequest(requestContext, site.url, appPasswordsResponse.error)
152152

153-
if (appPasswordsResponse.error.errorCode ==
154-
ApplicationPasswordsNetwork.APPLICATION_PASSWORDS_NOT_SUPPORT_ERROR_CODE
155-
) {
156-
withContext(Dispatchers.IO) {
157-
site.applicationPasswordsAuthorizeUrl = null
158-
siteSqlUtils.insertOrUpdateSite(site)
153+
jetpackTunnelWPAPINetwork.request().also {
154+
if (it is WPAPIResponse.Success) {
155+
// when the fallback to Jetpack Tunnel succeeds, notify the error handler
156+
jetpackApplicationPasswordsErrorHandler.handleError(site, appPasswordsResponse.error)
159157
}
160-
}
161-
jetpackTunnelWPAPINetwork.request().copyWith(
158+
}.copyWith(
162159
networkingMode = WPAPINetworkingMode.JetpackTunnel(
163160
isFallback = true,
164161
applicationPasswordsError = appPasswordsResponse.error

‎libs/fluxc-tests/src/test/java/org/wordpress/android/fluxc/network/rest/wpcom/wc/WooNetworkTest.kt‎

Lines changed: 25 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -7,38 +7,42 @@ import org.mockito.kotlin.given
77
import org.mockito.kotlin.mock
88
import org.mockito.kotlin.never
99
import org.mockito.kotlin.verify
10+
import org.mockito.kotlin.whenever
1011
import org.robolectric.RobolectricTestRunner
1112
import org.wordpress.android.fluxc.model.SiteModel
1213
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPINetworkError
1314
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPIResponse
1415
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsConfiguration
1516
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.ApplicationPasswordsNetwork
17+
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.JetpackApplicationPasswordsErrorHandler
18+
import org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords.JetpackApplicationPasswordsSupport
1619
import org.wordpress.android.fluxc.network.rest.wpcom.JetpackTunnelWPAPINetwork
17-
import org.wordpress.android.fluxc.persistence.SiteSqlUtils
1820
import org.wordpress.android.fluxc.test
1921

2022
@RunWith(RobolectricTestRunner::class)
2123
class WooNetworkTest {
2224
private val testSite = SiteModel().apply {
25+
origin = SiteModel.ORIGIN_WPCOM_REST
2326
url = "https://example.com"
2427
}
2528
private val testPath = "path"
2629
private val applicationPasswordsConfiguration = FakeApplicationPasswordsConfiguration()
2730
private val applicationPasswordsNetwork: ApplicationPasswordsNetwork = mock()
2831
private val jetpackTunnelWPAPINetwork: JetpackTunnelWPAPINetwork = mock()
29-
private val siteSqlUtils: SiteSqlUtils = mock()
32+
private val jetpackApplicationPasswordsErrorHandler: JetpackApplicationPasswordsErrorHandler = mock()
33+
private val jetpackApplicationPasswordsSupport: JetpackApplicationPasswordsSupport = mock()
3034

3135
private val sut = WooNetwork(
3236
applicationPasswordsConfiguration = applicationPasswordsConfiguration,
3337
applicationPasswordsNetwork = applicationPasswordsNetwork,
3438
jetpackTunnelWPAPINetwork = jetpackTunnelWPAPINetwork,
35-
siteSqlUtils = siteSqlUtils
39+
jetpackApplicationPasswordsSupport = jetpackApplicationPasswordsSupport,
40+
jetpackApplicationPasswordsErrorHandler = jetpackApplicationPasswordsErrorHandler
3641
)
3742

3843
@Test
3944
fun `given Jetpack site supports app passwords, when making request, then use app passwords network`() = test {
40-
testSite.origin = SiteModel.ORIGIN_WPCOM_REST
41-
testSite.applicationPasswordsAuthorizeUrl = "authorize_url"
45+
whenever(jetpackApplicationPasswordsSupport.supportsAppPasswords(testSite)).thenReturn(true)
4246
val sampleResponse = SampleResponse("value")
4347
givenAppPasswordsResponse(WPAPIResponse.Success(SampleResponse("value"), emptyList()))
4448

@@ -59,8 +63,7 @@ class WooNetworkTest {
5963
@Test
6064
fun `given jetpack site that supports app passwords, when request fails, then fall back to jetpack tunnel`() =
6165
test {
62-
testSite.origin = SiteModel.ORIGIN_WPCOM_REST
63-
testSite.applicationPasswordsAuthorizeUrl = "authorize_url"
66+
whenever(jetpackApplicationPasswordsSupport.supportsAppPasswords(testSite)).thenReturn(true)
6467
givenAppPasswordsResponse(
6568
WPAPIResponse.Error(
6669
WPAPINetworkError(
@@ -84,8 +87,7 @@ class WooNetworkTest {
8487
@Test
8588
fun `given jetpack site that does not support app passwords, when making request, then use jetpack tunnel`() =
8689
test {
87-
testSite.origin = SiteModel.ORIGIN_WPCOM_REST
88-
testSite.applicationPasswordsAuthorizeUrl = null
90+
whenever(jetpackApplicationPasswordsSupport.supportsAppPasswords(testSite)).thenReturn(false)
8991
val sampleResponse = SampleResponse("value")
9092
givenJetpackTunnelResponse(WPAPIResponse.Success(sampleResponse, emptyList()))
9193

@@ -101,8 +103,7 @@ class WooNetworkTest {
101103
@Test
102104
fun `when app passwords are disabled for jetpack access, then always use jetpack tunnel`() = test {
103105
applicationPasswordsConfiguration.isEnabledForJetpackAccessValue = false
104-
testSite.origin = SiteModel.ORIGIN_WPCOM_REST
105-
testSite.applicationPasswordsAuthorizeUrl = "authorize_url"
106+
whenever(jetpackApplicationPasswordsSupport.supportsAppPasswords(testSite)).thenReturn(true)
106107
val sampleResponse = SampleResponse("value")
107108
givenJetpackTunnelResponse(WPAPIResponse.Success(sampleResponse, emptyList()))
108109

@@ -121,33 +122,21 @@ class WooNetworkTest {
121122
}
122123

123124
@Test
124-
fun `when detecting that a site doesn't support app passwords, then update cached site with correct status`() =
125-
test {
126-
testSite.origin = SiteModel.ORIGIN_WPCOM_REST
127-
testSite.applicationPasswordsAuthorizeUrl = "authorize_url"
128-
givenAppPasswordsResponse(
129-
WPAPIResponse.Error(
130-
WPAPINetworkError(
131-
mock(),
132-
errorCode = ApplicationPasswordsNetwork.APPLICATION_PASSWORDS_NOT_SUPPORT_ERROR_CODE
133-
)
134-
)
135-
)
136-
givenJetpackTunnelResponse(
137-
WPAPIResponse.Success(SampleResponse("value"), emptyList())
138-
)
125+
fun `when jetpack tunnel fallback succeeds after app passwords failure, then notify error handler`() = test {
126+
whenever(jetpackApplicationPasswordsSupport.supportsAppPasswords(testSite)).thenReturn(true)
127+
val appPasswordsNetworkError = WPAPINetworkError(mock(), "error")
128+
givenAppPasswordsResponse(WPAPIResponse.Error(appPasswordsNetworkError))
129+
val sampleResponse = SampleResponse("value")
130+
givenJetpackTunnelResponse(WPAPIResponse.Success(sampleResponse, emptyList()))
139131

140-
sut.executeGetGsonRequest(
141-
site = testSite,
142-
path = testPath,
143-
clazz = SampleResponse::class.java
144-
)
132+
sut.executeGetGsonRequest(
133+
site = testSite,
134+
path = testPath,
135+
clazz = SampleResponse::class.java
136+
)
145137

146-
val expectedSite = testSite.apply {
147-
applicationPasswordsAuthorizeUrl = null
148-
}
149-
verify(siteSqlUtils).insertOrUpdateSite(expectedSite)
150-
}
138+
verify(jetpackApplicationPasswordsErrorHandler).handleError(testSite, appPasswordsNetworkError)
139+
}
151140

152141
private suspend fun givenAppPasswordsResponse(response: WPAPIResponse<SampleResponse>) {
153142
given(

‎libs/fluxc/build.gradle‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ android.buildTypes.all { buildType ->
6565
dependencies {
6666
implementation libs.androidx.exifinterface
6767
implementation libs.androidx.security.crypto
68+
implementation libs.androidx.core.ktx
6869

6970
implementation(libs.wordpress.utils) {
7071
// Using official volley package

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/ApplicationPasswordsManager.kt‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ private const val UNAUTHORIZED = 401
1919
private const val CONFLICT = 409
2020
private const val NOT_FOUND = 404
2121
private const val APPLICATION_PASSWORDS_DISABLED_ERROR_CODE = "application_passwords_disabled"
22+
private const val APPLICATION_PASSWORDS_DISABLED_USER_ERROR_CODE = "application_passwords_disabled_for_user"
2223

2324
@Singleton
2425
internal class ApplicationPasswordsManager @Inject constructor(
@@ -162,7 +163,8 @@ internal class ApplicationPasswordsManager @Inject constructor(
162163
}
163164

164165
statusCode == NOT_FOUND ||
165-
errorCode == APPLICATION_PASSWORDS_DISABLED_ERROR_CODE -> {
166+
errorCode == APPLICATION_PASSWORDS_DISABLED_ERROR_CODE ||
167+
errorCode == APPLICATION_PASSWORDS_DISABLED_USER_ERROR_CODE -> {
166168
appLogWrapper.w(
167169
MAIN,
168170
"Application Password feature not supported, " +
@@ -236,8 +238,8 @@ internal class ApplicationPasswordsManager @Inject constructor(
236238
val error = payload.error
237239
appLogWrapper.w(
238240
AppLog.T.MAIN, "Application password deletion failed, error: " +
239-
"${error.type} ${error.message}\n" +
240-
"${error.volleyError?.toString()}"
241+
"${error.type} ${error.message}\n" +
242+
"${error.volleyError?.toString()}"
241243
)
242244
ApplicationPasswordDeletionResult.Failure(error)
243245
}

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/ApplicationPasswordsNetwork.kt‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -105,15 +105,11 @@ class ApplicationPasswordsNetwork @Inject constructor(
105105
}
106106

107107
is ApplicationPasswordCreationResult.NotSupported -> {
108+
val networkError = credentialsResult.originalError.toWPAPINetworkError()
108109
if (listener.isPresent) {
109-
listener.get().onFeatureUnavailable(site, credentialsResult.originalError.toWPAPINetworkError())
110+
listener.get().onFeatureUnavailable(site, networkError)
110111
}
111-
return WPAPIResponse.Error(
112-
WPAPINetworkError(
113-
baseError = credentialsResult.originalError.toWPAPINetworkError(),
114-
errorCode = APPLICATION_PASSWORDS_NOT_SUPPORT_ERROR_CODE
115-
)
116-
)
112+
return WPAPIResponse.Error(networkError)
117113
}
118114
}
119115

@@ -187,10 +183,6 @@ class ApplicationPasswordsNetwork @Inject constructor(
187183
clazz: Class<T>,
188184
params: Map<String, String>
189185
) = executeGsonRequest(site, HttpMethod.DELETE, path, clazz, params)
190-
191-
companion object {
192-
const val APPLICATION_PASSWORDS_NOT_SUPPORT_ERROR_CODE = "application_passwords_not_supported"
193-
}
194186
}
195187

196188
private fun BaseNetworkError.toWPAPINetworkError(): WPAPINetworkError {

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/JetpackApplicationPasswordsErrorHandler.kt‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
package org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords
2+
3+
import org.wordpress.android.fluxc.model.SiteModel
4+
import org.wordpress.android.fluxc.network.rest.wpapi.WPAPINetworkError
5+
import org.wordpress.android.fluxc.utils.AppLogWrapper
6+
import org.wordpress.android.util.AppLog
7+
import javax.inject.Inject
8+
import javax.inject.Singleton
9+
10+
@Singleton
11+
class JetpackApplicationPasswordsErrorHandler @Inject constructor(
12+
private val jetpackApplicationPasswordsSupport: JetpackApplicationPasswordsSupport,
13+
private val appLogWrapper: AppLogWrapper
14+
) {
15+
private var failuresCount: MutableMap<Long, Int> = mutableMapOf()
16+
17+
@Suppress("ComplexCondition")
18+
fun handleError(siteModel: SiteModel, error: WPAPINetworkError) {
19+
val httpStatusCode = error.volleyError?.networkResponse?.statusCode
20+
val apiErrorCode = error.errorCode
21+
22+
if (httpStatusCode == UNAUTHORIZED ||
23+
httpStatusCode == FORBIDDEN ||
24+
httpStatusCode == TOO_MANY_REQUESTS ||
25+
apiErrorCode == "incorrect_password" ||
26+
apiErrorCode == "application_passwords_disabled_for_user"
27+
) {
28+
appLogWrapper.w(
29+
AppLog.T.API,
30+
"Disabling Jetpack Application Passwords support for site ${siteModel.siteId} " +
31+
"due to error: $httpStatusCode / $apiErrorCode"
32+
)
33+
jetpackApplicationPasswordsSupport.flagAsUnsupported(siteModel)
34+
} else {
35+
val siteFailuresCount = (failuresCount[siteModel.siteId] ?: 0) + 1
36+
failuresCount[siteModel.siteId] = siteFailuresCount
37+
38+
if (siteFailuresCount >= FAILURES_THRESHOLD) {
39+
appLogWrapper.w(
40+
AppLog.T.API,
41+
"Disabling Jetpack Application Passwords support for site ${siteModel.siteId} " +
42+
"after $siteFailuresCount failures"
43+
)
44+
jetpackApplicationPasswordsSupport.flagAsUnsupported(siteModel)
45+
}
46+
}
47+
}
48+
49+
companion object {
50+
private const val UNAUTHORIZED = 401
51+
private const val FORBIDDEN = 403
52+
private const val TOO_MANY_REQUESTS = 429
53+
private const val FAILURES_THRESHOLD = 10
54+
}
55+
}

‎libs/fluxc/src/main/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/JetpackApplicationPasswordsSupport.kt‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
package org.wordpress.android.fluxc.network.rest.wpapi.applicationpasswords
2+
3+
import android.content.Context
4+
import androidx.core.content.edit
5+
import org.wordpress.android.fluxc.model.SiteModel
6+
import org.wordpress.android.fluxc.utils.PreferenceUtils
7+
import javax.inject.Inject
8+
9+
class JetpackApplicationPasswordsSupport @Inject constructor(context: Context) {
10+
private val fluxCPreferences by lazy { PreferenceUtils.getFluxCPreferences(context) }
11+
12+
fun supportsAppPasswords(siteModel: SiteModel): Boolean {
13+
return siteModel.isApplicationPasswordsSupported &&
14+
fluxCPreferences.getStringSet(UNSUPPORTED_JETPACK_APP_PASSWORDS_SITES, null)
15+
?.contains(siteModel.siteId.toString()) != true
16+
}
17+
18+
fun flagAsUnsupported(siteModel: SiteModel) {
19+
val unsupportedSites = fluxCPreferences.getStringSet(UNSUPPORTED_JETPACK_APP_PASSWORDS_SITES, null) ?: setOf()
20+
fluxCPreferences.edit {
21+
putStringSet(UNSUPPORTED_JETPACK_APP_PASSWORDS_SITES, unsupportedSites + siteModel.siteId.toString())
22+
}
23+
}
24+
25+
companion object {
26+
private const val UNSUPPORTED_JETPACK_APP_PASSWORDS_SITES = "unsupported_jetpack_app_passwords_sites"
27+
}
28+
}

‎libs/fluxc/src/test/java/org/wordpress/android/fluxc/network/rest/wpapi/applicationpasswords/ApplicationPasswordManagerTests.kt‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,29 @@ class ApplicationPasswordManagerTests {
166166
assertEquals(ApplicationPasswordCreationResult.NotSupported(networkError), result)
167167
}
168168

169+
@Test
170+
fun `when a jetpack site returns application_passwords_disabled_for_user, then return feature not available`() =
171+
runTest {
172+
val site = testSite.apply {
173+
origin = SiteModel.ORIGIN_WPCOM_REST
174+
}
175+
val networkError = WPComGsonNetworkError(BaseNetworkError(GenericErrorType.SERVER_ERROR)).apply {
176+
apiError = "application_passwords_disabled_for_user"
177+
}
178+
179+
whenever(applicationPasswordsStore.getCredentials(testSite)).thenReturn(null)
180+
whenever(mJetpackApplicationPasswordsRestClient.fetchWPAdminUsername(site))
181+
.thenReturn(UsernameFetchPayload(testCredentials.userName))
182+
whenever(mJetpackApplicationPasswordsRestClient.createApplicationPassword(site, applicationName))
183+
.thenReturn(ApplicationPasswordCreationPayload(networkError))
184+
185+
val result = mApplicationPasswordsManager.getApplicationCredentials(
186+
testSite
187+
)
188+
189+
assertEquals(ApplicationPasswordCreationResult.NotSupported(networkError), result)
190+
}
191+
169192
@Test
170193
fun `when a non-jetpack site returns 404, then return feature not available`() =
171194
runTest {

0 commit comments

Comments
 (0)