-
Notifications
You must be signed in to change notification settings - Fork 180
fix: piggycards auth hardening #1496
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -24,8 +24,8 @@ import okhttp3.Request | |||||||||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Response | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Route | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.data.dashspend.piggycards.model.LoginRequest | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.data.dashspend.piggycards.model.LoginResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.network.service.piggycards.PiggyCardsTokenApi | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.repository.CTXSpendException | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.utils.PiggyCardsConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import org.slf4j.LoggerFactory | ||||||||||||||||||||||||||||||||||||||||||||||||||
| import java.time.LocalDateTime | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -39,53 +39,79 @@ class PiggyCardsAuthenticator @Inject constructor( | |||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| companion object { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private val log = LoggerFactory.getLogger(PiggyCardsAuthenticator::class.java) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| // For multiple call to refresh token sync | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private val tokenMutex = Mutex() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Shared across every PiggyCardsAuthenticator instance (the OkHttp retry path plus the | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // factory-built instance used by the repository) so concurrent re-logins are serialized | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // against the single shared token store, instead of racing on separate per-instance locks. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private val tokenMutex = Mutex() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| override fun authenticate(route: Route?, response: Response): Request? { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (response.responseCount >= 2) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return null | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return runBlocking { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| tokenMutex.withLock { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val loginResponse = refreshToken() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (loginResponse != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| handleLoginResponse(loginResponse) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| response.request.newBuilder() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .header("Authorization", "Bearer ${loginResponse.accessToken}") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| null | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error("Failed to refresh token: ${e.message}", e) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| null | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| reLogin()?.let { accessToken -> | ||||||||||||||||||||||||||||||||||||||||||||||||||
| response.request.newBuilder() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .header("Authorization", "Bearer $accessToken") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| private suspend fun refreshToken(): LoginResponse? { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Re-authenticates with the stored credentials under a process-wide lock and persists the new | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * access token, so the OkHttp retry path and [org.dash.wallet.features.exploredash.repository.PiggyCardsRepository] | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * never issue overlapping logins. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * @return the new access token, or null when no credentials are stored or the login failed. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * Stored credentials are preserved on transient failures so the session survives; only a | ||||||||||||||||||||||||||||||||||||||||||||||||||
| * genuine rejection (HTTP 401) clears the cached access token. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| suspend fun reLogin(): String? = tokenMutex.withLock { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val userId = config.getSecuredData(PiggyCardsConfig.PREFS_KEY_USER_ID) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val password = config.getSecuredData(PiggyCardsConfig.PREFS_KEY_PASSWORD) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| return if (!userId.isNullOrBlank() && !password.isNullOrBlank()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| tokenApi.login(LoginRequest(userId = userId, password = password)) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (userId.isNullOrBlank() || password.isNullOrBlank()) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return@withLock null | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| try { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val response = tokenApi.login(LoginRequest(userId = userId, password = password)) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| persistLogin(response.accessToken, response.expiresIn) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| response.accessToken.takeIf { it.isNotBlank() } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } catch (e: Exception) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| log.error("Failed to refresh token: ${e.message}", e) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // Only a genuine credential rejection drops the cached token. The login endpoint | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // returns HTTP 401 for that case, surfaced by ErrorHandlingInterceptor as a | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // CTXSpendException; any other failure is transient, so the token is kept and the | ||||||||||||||||||||||||||||||||||||||||||||||||||
| // next request retries instead of forcing the user to sign in again. | ||||||||||||||||||||||||||||||||||||||||||||||||||
| if (e is CTXSpendException && e.errorCode == 401) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "") | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| null | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+84
to
94
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: cat -n features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.kt | head -120Repository: dashpay/dash-wallet Length of output: 5811 🏁 Script executed: rg "CancellationException" features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/authenticator/PiggyCardsAuthenticator.ktRepository: dashpay/dash-wallet Length of output: 45 🏁 Script executed: rg "import.*CancellationException" features/exploredash/Repository: dashpay/dash-wallet Length of output: 45 🏁 Script executed: cd features/exploredash && find . -name "*.kt" | head -5 && cd - > /dev/nullRepository: dashpay/dash-wallet Length of output: 470 🏁 Script executed: find . -name ".editorconfig" -o -name "ktlint.yml" -o -name "ktlint.yaml" | head -10Repository: dashpay/dash-wallet Length of output: 110 🏁 Script executed: rg "CancellationException" --type kt | head -20Repository: dashpay/dash-wallet Length of output: 90 🏁 Script executed: rg "CancellationException" --type kotlin | head -20Repository: dashpay/dash-wallet Length of output: 1572 🏁 Script executed: rg "CancellationException" | head -20Repository: dashpay/dash-wallet Length of output: 1572 🏁 Script executed: cat wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt | grep -A 5 "catch (_: CancellationException)" | head -20Repository: dashpay/dash-wallet Length of output: 541 🏁 Script executed: cat .editorconfig | head -50Repository: dashpay/dash-wallet Length of output: 569 Rethrow coroutine cancellation before generic failure handling. The Proposed fix+import kotlinx.coroutines.CancellationException
+
...
- } catch (e: Exception) {
+ } catch (e: CancellationException) {
+ throw e
+ } catch (e: Exception) {
log.error("Failed to refresh token: ${e.message}", e)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| private suspend fun handleLoginResponse( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| response: LoginResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, response.accessToken) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| private suspend fun persistLogin(accessToken: String, expiresIn: Int) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, accessToken) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| val expiresAt = LocalDateTime.now().plusSeconds(response.expiresIn.toLong()) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| val expiresAt = LocalDateTime.now().plusSeconds(expiresIn.toLong()) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData( | ||||||||||||||||||||||||||||||||||||||||||||||||||
| PiggyCardsConfig.PREFS_KEY_TOKEN_EXPIRES_AT, | ||||||||||||||||||||||||||||||||||||||||||||||||||
| expiresAt.format(DateTimeFormatter.ISO_LOCAL_DATE_TIME) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||
| private val Response.responseCount: Int | ||||||||||||||||||||||||||||||||||||||||||||||||||
| get() { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| var result = 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||
| var priorResponse = this.priorResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||
| while (priorResponse != null) { | ||||||||||||||||||||||||||||||||||||||||||||||||||
| result++ | ||||||||||||||||||||||||||||||||||||||||||||||||||
| priorResponse = priorResponse.priorResponse | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| return result | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,143 @@ | ||||||||||||||||||||||||||||||||||||||||||
| /* | ||||||||||||||||||||||||||||||||||||||||||
| * Copyright (c) 2026. Dash Core Group. | ||||||||||||||||||||||||||||||||||||||||||
| * This program is free software: you can redistribute it and/or modify | ||||||||||||||||||||||||||||||||||||||||||
| * it under the terms of the GNU General Public License as published by | ||||||||||||||||||||||||||||||||||||||||||
| * the Free Software Foundation, either version 3 of the License, or | ||||||||||||||||||||||||||||||||||||||||||
| * (at your option) any later version. | ||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||
| * This program is distributed in the hope that it will be useful, | ||||||||||||||||||||||||||||||||||||||||||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||||||||||||||||||||||||||||||||||||||||||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||||||||||||||||||||||||||||||||||||||||||
| * GNU General Public License for more details. | ||||||||||||||||||||||||||||||||||||||||||
| * | ||||||||||||||||||||||||||||||||||||||||||
| * You should have received a copy of the GNU General Public License | ||||||||||||||||||||||||||||||||||||||||||
| * along with this program. If not, see <http://www.gnu.org/licenses/>. | ||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| package org.dash.wallet.features.exploredash | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| import android.content.Context | ||||||||||||||||||||||||||||||||||||||||||
| import androidx.test.core.app.ApplicationProvider | ||||||||||||||||||||||||||||||||||||||||||
| import kotlinx.coroutines.runBlocking | ||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Protocol | ||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Request | ||||||||||||||||||||||||||||||||||||||||||
| import okhttp3.Response | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.common.WalletDataProvider | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.common.util.security.EncryptionProvider | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.data.dashspend.piggycards.model.LoginRequest | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.data.dashspend.piggycards.model.LoginResponse | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.network.authenticator.PiggyCardsAuthenticator | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.network.service.piggycards.PiggyCardsTokenApi | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.repository.CTXSpendException | ||||||||||||||||||||||||||||||||||||||||||
| import org.dash.wallet.features.exploredash.utils.PiggyCardsConfig | ||||||||||||||||||||||||||||||||||||||||||
| import org.junit.Assert.assertEquals | ||||||||||||||||||||||||||||||||||||||||||
| import org.junit.Assert.assertNull | ||||||||||||||||||||||||||||||||||||||||||
| import org.junit.Test | ||||||||||||||||||||||||||||||||||||||||||
| import org.junit.runner.RunWith | ||||||||||||||||||||||||||||||||||||||||||
| import org.mockito.kotlin.mock | ||||||||||||||||||||||||||||||||||||||||||
| import org.robolectric.RobolectricTestRunner | ||||||||||||||||||||||||||||||||||||||||||
| import java.io.IOException | ||||||||||||||||||||||||||||||||||||||||||
| import java.util.concurrent.atomic.AtomicInteger | ||||||||||||||||||||||||||||||||||||||||||
| import kotlin.concurrent.thread | ||||||||||||||||||||||||||||||||||||||||||
| import kotlin.math.max | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||
| * Verifies the PiggyCards auth hardening that mirrors PR #1492's CTX fixes: | ||||||||||||||||||||||||||||||||||||||||||
| * - re-logins are serialized by a process-wide (companion) lock across instances, and | ||||||||||||||||||||||||||||||||||||||||||
| * - a transient failure keeps the cached token while a genuine 401 clears it. | ||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||
| @RunWith(RobolectricTestRunner::class) | ||||||||||||||||||||||||||||||||||||||||||
| class PiggyCardsAuthenticatorTest { | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| private val identityEncryption = object : EncryptionProvider { | ||||||||||||||||||||||||||||||||||||||||||
| override fun encrypt(keyAlias: String, textToEncrypt: String): ByteArray = textToEncrypt.toByteArray() | ||||||||||||||||||||||||||||||||||||||||||
| override fun decrypt(keyAlias: String, encryptedData: ByteArray): String = String(encryptedData) | ||||||||||||||||||||||||||||||||||||||||||
| override fun deleteKey(keyAlias: String) {} | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| private fun realConfig(): PiggyCardsConfig { | ||||||||||||||||||||||||||||||||||||||||||
| val context = ApplicationProvider.getApplicationContext<Context>() | ||||||||||||||||||||||||||||||||||||||||||
| return PiggyCardsConfig(context, mock<WalletDataProvider>(), identityEncryption) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| private fun unauthorizedResponse(): Response = | ||||||||||||||||||||||||||||||||||||||||||
| Response.Builder() | ||||||||||||||||||||||||||||||||||||||||||
| .request(Request.Builder().url("https://example.com/").build()) | ||||||||||||||||||||||||||||||||||||||||||
| .protocol(Protocol.HTTP_1_1) | ||||||||||||||||||||||||||||||||||||||||||
| .code(401) | ||||||||||||||||||||||||||||||||||||||||||
| .message("Unauthorized") | ||||||||||||||||||||||||||||||||||||||||||
| .build() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||
| fun reLogin_acrossInstances_isSerialized_byProcessWideLock() { | ||||||||||||||||||||||||||||||||||||||||||
| val config = realConfig() | ||||||||||||||||||||||||||||||||||||||||||
| runBlocking { | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_USER_ID, "user-1") | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_PASSWORD, "pw-1") | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val concurrent = AtomicInteger(0) | ||||||||||||||||||||||||||||||||||||||||||
| val maxConcurrent = AtomicInteger(0) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val tokenApi = object : PiggyCardsTokenApi { | ||||||||||||||||||||||||||||||||||||||||||
| override suspend fun login(loginRequest: LoginRequest): LoginResponse { | ||||||||||||||||||||||||||||||||||||||||||
| val now = concurrent.incrementAndGet() | ||||||||||||||||||||||||||||||||||||||||||
| maxConcurrent.updateAndGet { max(it, now) } | ||||||||||||||||||||||||||||||||||||||||||
| Thread.sleep(200) | ||||||||||||||||||||||||||||||||||||||||||
| concurrent.decrementAndGet() | ||||||||||||||||||||||||||||||||||||||||||
| return LoginResponse(accessToken = "fresh-token", tokenType = "Bearer", expiresIn = 3600) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| val authenticatorA = PiggyCardsAuthenticator(tokenApi, config) | ||||||||||||||||||||||||||||||||||||||||||
| val authenticatorB = PiggyCardsAuthenticator(tokenApi, config) | ||||||||||||||||||||||||||||||||||||||||||
| val response = unauthorizedResponse() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val threadA = thread { authenticatorA.authenticate(null, response) } | ||||||||||||||||||||||||||||||||||||||||||
| val threadB = thread { authenticatorB.authenticate(null, response) } | ||||||||||||||||||||||||||||||||||||||||||
| threadA.join() | ||||||||||||||||||||||||||||||||||||||||||
| threadB.join() | ||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+95
to
+98
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Propagate failures from the worker threads. Exceptions thrown inside Proposed fix import java.util.concurrent.atomic.AtomicInteger
+import java.util.concurrent.atomic.AtomicReference
...
val response = unauthorizedResponse()
+ val failure = AtomicReference<Throwable?>()
- val threadA = thread { authenticatorA.authenticate(null, response) }
- val threadB = thread { authenticatorB.authenticate(null, response) }
+ val threadA = thread {
+ runCatching { authenticatorA.authenticate(null, response) }
+ .exceptionOrNull()
+ ?.let(failure::set)
+ }
+ val threadB = thread {
+ runCatching { authenticatorB.authenticate(null, response) }
+ .exceptionOrNull()
+ ?.let(failure::set)
+ }
threadA.join()
threadB.join()
+ failure.get()?.let { throw it }📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| // The shared companion lock prevents overlapping re-logins across separate instances. | ||||||||||||||||||||||||||||||||||||||||||
| assertEquals(1, maxConcurrent.get()) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||
| fun reLogin_onTransientFailure_preservesToken() = runBlocking { | ||||||||||||||||||||||||||||||||||||||||||
| val config = realConfig() | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_USER_ID, "user-1") | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_PASSWORD, "pw-1") | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "existing-token") | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val tokenApi = object : PiggyCardsTokenApi { | ||||||||||||||||||||||||||||||||||||||||||
| override suspend fun login(loginRequest: LoginRequest): LoginResponse = | ||||||||||||||||||||||||||||||||||||||||||
| throw IOException("transient network failure") // not a 401 | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| val authenticator = PiggyCardsAuthenticator(tokenApi, config) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val result = authenticator.reLogin() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| assertNull(result) | ||||||||||||||||||||||||||||||||||||||||||
| // Transient failure must NOT wipe the cached token: the session survives and retries. | ||||||||||||||||||||||||||||||||||||||||||
| assertEquals("existing-token", config.getSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN)) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| @Test | ||||||||||||||||||||||||||||||||||||||||||
| fun reLogin_onGenuine401_clearsToken() = runBlocking { | ||||||||||||||||||||||||||||||||||||||||||
| val config = realConfig() | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_USER_ID, "user-1") | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_PASSWORD, "pw-1") | ||||||||||||||||||||||||||||||||||||||||||
| config.setSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN, "existing-token") | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val tokenApi = object : PiggyCardsTokenApi { | ||||||||||||||||||||||||||||||||||||||||||
| override suspend fun login(loginRequest: LoginRequest): LoginResponse = | ||||||||||||||||||||||||||||||||||||||||||
| throw CTXSpendException("unauthorized", errorCode = 401) // genuine credential rejection | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| val authenticator = PiggyCardsAuthenticator(tokenApi, config) | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| val result = authenticator.reLogin() | ||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||
| assertNull(result) | ||||||||||||||||||||||||||||||||||||||||||
| // A genuine 401 means the credentials are no longer valid, so the cached token is cleared. | ||||||||||||||||||||||||||||||||||||||||||
| assertEquals("", config.getSecuredData(PiggyCardsConfig.PREFS_KEY_ACCESS_TOKEN)) | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Validate the access token before persisting it.
persistLogin()runs even whenresponse.accessTokenis blank, so a malformed successful response can overwrite the cached token/expiry whilereLogin()returnsnull. Persist only after the token passes the non-blank check.Proposed fix
📝 Committable suggestion
🤖 Prompt for AI Agents