fix: recover safely from OAuth callback failures
This commit is contained in:
parent
31b43c6466
commit
5778dc9b2a
@ -200,7 +200,6 @@ class MainActivity : ComponentActivity() {
|
|||||||
onRetryLastPushedBranch = viewModel::retryRowLastPushedBranch,
|
onRetryLastPushedBranch = viewModel::retryRowLastPushedBranch,
|
||||||
onShowAllRepositories = viewModel::showAllRepositories,
|
onShowAllRepositories = viewModel::showAllRepositories,
|
||||||
onShowFavorites = viewModel::showFavorites,
|
onShowFavorites = viewModel::showFavorites,
|
||||||
onToggleFavorite = viewModel::toggleFavorite,
|
|
||||||
onViewWorkItems = { repository ->
|
onViewWorkItems = { repository ->
|
||||||
push(Route.WorkItems(repository.coordinates.owner, repository.coordinates.name))
|
push(Route.WorkItems(repository.coordinates.owner, repository.coordinates.name))
|
||||||
},
|
},
|
||||||
|
|||||||
@ -13,9 +13,7 @@ data class OAuthClientConfig(
|
|||||||
require(clientId.isNotBlank()) { "OAuth client ID cannot be blank" }
|
require(clientId.isNotBlank()) { "OAuth client ID cannot be blank" }
|
||||||
require(redirectUri.isSecureRedirect()) { "OAuth redirect URI must be HTTPS without query or fragment" }
|
require(redirectUri.isSecureRedirect()) { "OAuth redirect URI must be HTTPS without query or fragment" }
|
||||||
require(scopes.isNotEmpty()) { "At least one OAuth scope is required" }
|
require(scopes.isNotEmpty()) { "At least one OAuth scope is required" }
|
||||||
require(scopes.all { it.isReadOnly || it in EXPLICITLY_APPROVED_WRITE_SCOPES }) {
|
require(scopes.all { it.isReadOnly }) { "Only read-only OAuth scopes are allowed" }
|
||||||
"OAuth scope requires explicit human approval and an entry in EXPLICITLY_APPROVED_WRITE_SCOPES"
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
val authorizationEndpoint: URI
|
val authorizationEndpoint: URI
|
||||||
@ -36,11 +34,6 @@ data class OAuthClientConfig(
|
|||||||
query == null &&
|
query == null &&
|
||||||
fragment == null
|
fragment == null
|
||||||
|
|
||||||
companion object {
|
|
||||||
// Every entry here is a deliberate, one-at-a-time human product-owner decision
|
|
||||||
// (see docs/decisions and TASK-018), not a general write-scope allowance.
|
|
||||||
val EXPLICITLY_APPROVED_WRITE_SCOPES: Set<OAuthScope> = setOf(OAuthScope.WRITE_USER)
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
enum class OAuthScope(val wireValue: String, val isReadOnly: Boolean) {
|
enum class OAuthScope(val wireValue: String, val isReadOnly: Boolean) {
|
||||||
@ -63,7 +56,6 @@ object UBGiteaOAuthDefaults {
|
|||||||
OAuthScope.READ_ORGANIZATION,
|
OAuthScope.READ_ORGANIZATION,
|
||||||
OAuthScope.READ_ISSUE,
|
OAuthScope.READ_ISSUE,
|
||||||
OAuthScope.READ_NOTIFICATION,
|
OAuthScope.READ_NOTIFICATION,
|
||||||
OAuthScope.WRITE_USER,
|
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
}
|
}
|
||||||
|
|||||||
@ -8,7 +8,11 @@ import java.net.URISyntaxException
|
|||||||
|
|
||||||
sealed interface HandledOAuthCallback {
|
sealed interface HandledOAuthCallback {
|
||||||
data class Success(val authorizationCode: String, val verifier: String) : HandledOAuthCallback
|
data class Success(val authorizationCode: String, val verifier: String) : HandledOAuthCallback
|
||||||
data object Rejected : HandledOAuthCallback
|
|
||||||
|
// `reason` is a stable, non-secret diagnostic tag (no code/token/state values), always
|
||||||
|
// populated so a debug build can surface it; production UI ignores it and shows one generic
|
||||||
|
// message regardless of reason, so this carries no user-facing behavior change.
|
||||||
|
data class Rejected(val reason: String) : HandledOAuthCallback
|
||||||
}
|
}
|
||||||
|
|
||||||
class OAuthCallbackHandler(
|
class OAuthCallbackHandler(
|
||||||
@ -18,20 +22,19 @@ class OAuthCallbackHandler(
|
|||||||
) {
|
) {
|
||||||
suspend fun handle(rawUri: String): HandledOAuthCallback {
|
suspend fun handle(rawUri: String): HandledOAuthCallback {
|
||||||
val transaction = pendingStore.consume()
|
val transaction = pendingStore.consume()
|
||||||
?: return HandledOAuthCallback.Rejected
|
?: return HandledOAuthCallback.Rejected("no_pending_transaction")
|
||||||
val uri = try {
|
val uri = try {
|
||||||
URI(rawUri)
|
URI(rawUri)
|
||||||
} catch (_: URISyntaxException) {
|
} catch (_: URISyntaxException) {
|
||||||
return HandledOAuthCallback.Rejected
|
return HandledOAuthCallback.Rejected("malformed_uri")
|
||||||
}
|
}
|
||||||
return when (val result = validator.validate(uri, transaction, now())) {
|
return when (val result = validator.validate(uri, transaction, now())) {
|
||||||
is OAuthCallbackResult.Success -> HandledOAuthCallback.Success(
|
is OAuthCallbackResult.Success -> HandledOAuthCallback.Success(
|
||||||
result.authorizationCode,
|
result.authorizationCode,
|
||||||
transaction.verifier,
|
transaction.verifier,
|
||||||
)
|
)
|
||||||
is OAuthCallbackResult.Denied,
|
is OAuthCallbackResult.Denied -> HandledOAuthCallback.Rejected("denied:${result.error}")
|
||||||
is OAuthCallbackResult.Invalid,
|
is OAuthCallbackResult.Invalid -> HandledOAuthCallback.Rejected("invalid:${result.reason}")
|
||||||
-> HandledOAuthCallback.Rejected
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@ -7,7 +7,6 @@ import androidx.compose.foundation.layout.Spacer
|
|||||||
import androidx.compose.foundation.layout.fillMaxSize
|
import androidx.compose.foundation.layout.fillMaxSize
|
||||||
import androidx.compose.foundation.layout.fillMaxWidth
|
import androidx.compose.foundation.layout.fillMaxWidth
|
||||||
import androidx.compose.foundation.layout.padding
|
import androidx.compose.foundation.layout.padding
|
||||||
import androidx.compose.foundation.layout.size
|
|
||||||
import androidx.compose.foundation.clickable
|
import androidx.compose.foundation.clickable
|
||||||
import androidx.compose.foundation.horizontalScroll
|
import androidx.compose.foundation.horizontalScroll
|
||||||
import androidx.compose.foundation.rememberScrollState
|
import androidx.compose.foundation.rememberScrollState
|
||||||
@ -52,7 +51,6 @@ fun RepositoryListScreen(
|
|||||||
onRetryLastPushedBranch: (RepositoryCoordinates) -> Unit,
|
onRetryLastPushedBranch: (RepositoryCoordinates) -> Unit,
|
||||||
onShowAllRepositories: () -> Unit,
|
onShowAllRepositories: () -> Unit,
|
||||||
onShowFavorites: () -> Unit,
|
onShowFavorites: () -> Unit,
|
||||||
onToggleFavorite: (RepositoryRowUiModel) -> Unit,
|
|
||||||
onViewWorkItems: (RepositoryRowUiModel) -> Unit,
|
onViewWorkItems: (RepositoryRowUiModel) -> Unit,
|
||||||
onSelectOrganization: (String?) -> Unit,
|
onSelectOrganization: (String?) -> Unit,
|
||||||
onViewNotifications: () -> Unit,
|
onViewNotifications: () -> Unit,
|
||||||
@ -92,10 +90,11 @@ fun RepositoryListScreen(
|
|||||||
is RepositoryListUiState.Error -> {
|
is RepositoryListUiState.Error -> {
|
||||||
val needsLogin = state.type == RepositoryListError.AUTHENTICATION_EXPIRED ||
|
val needsLogin = state.type == RepositoryListError.AUTHENTICATION_EXPIRED ||
|
||||||
state.type == RepositoryListError.INVALID_CALLBACK
|
state.type == RepositoryListError.INVALID_CALLBACK
|
||||||
|
val baseMessage = state.accountName?.let {
|
||||||
|
stringResource(R.string.repositories_error_account, it, stringResource(state.type.messageResource))
|
||||||
|
} ?: stringResource(state.type.messageResource)
|
||||||
MessageState(
|
MessageState(
|
||||||
state.accountName?.let {
|
state.debugDetail?.let { "$baseMessage\n\n[debug] $it" } ?: baseMessage,
|
||||||
stringResource(R.string.repositories_error_account, it, stringResource(state.type.messageResource))
|
|
||||||
} ?: stringResource(state.type.messageResource),
|
|
||||||
stringResource(if (needsLogin) R.string.repositories_sign_in else R.string.repositories_retry),
|
stringResource(if (needsLogin) R.string.repositories_sign_in else R.string.repositories_retry),
|
||||||
if (needsLogin) onSignIn else onRetry,
|
if (needsLogin) onSignIn else onRetry,
|
||||||
Modifier.padding(padding),
|
Modifier.padding(padding),
|
||||||
@ -113,7 +112,6 @@ fun RepositoryListScreen(
|
|||||||
onRetryLastPushedBranch,
|
onRetryLastPushedBranch,
|
||||||
onShowAllRepositories,
|
onShowAllRepositories,
|
||||||
onShowFavorites,
|
onShowFavorites,
|
||||||
onToggleFavorite,
|
|
||||||
onSelectOrganization,
|
onSelectOrganization,
|
||||||
onViewNotifications,
|
onViewNotifications,
|
||||||
Modifier.padding(padding),
|
Modifier.padding(padding),
|
||||||
@ -197,7 +195,6 @@ private fun RepositoryContent(
|
|||||||
onRetryLastPushedBranch: (RepositoryCoordinates) -> Unit,
|
onRetryLastPushedBranch: (RepositoryCoordinates) -> Unit,
|
||||||
onShowAllRepositories: () -> Unit,
|
onShowAllRepositories: () -> Unit,
|
||||||
onShowFavorites: () -> Unit,
|
onShowFavorites: () -> Unit,
|
||||||
onToggleFavorite: (RepositoryRowUiModel) -> Unit,
|
|
||||||
onSelectOrganization: (String?) -> Unit,
|
onSelectOrganization: (String?) -> Unit,
|
||||||
onViewNotifications: () -> Unit,
|
onViewNotifications: () -> Unit,
|
||||||
modifier: Modifier = Modifier,
|
modifier: Modifier = Modifier,
|
||||||
@ -227,13 +224,12 @@ private fun RepositoryContent(
|
|||||||
item { OrganizationFilterRow(state.organization, state.organizations, onSelectOrganization) }
|
item { OrganizationFilterRow(state.organization, state.organizations, onSelectOrganization) }
|
||||||
}
|
}
|
||||||
items(state.repositories, key = { it.coordinates }) { repository ->
|
items(state.repositories, key = { it.coordinates }) { repository ->
|
||||||
Row(Modifier.fillMaxWidth()) {
|
Column(
|
||||||
Column(
|
Modifier
|
||||||
Modifier
|
.fillMaxWidth()
|
||||||
.weight(1f)
|
.clickable(role = Role.Button) { onRepositorySelected(repository) }
|
||||||
.clickable(role = Role.Button) { onRepositorySelected(repository) }
|
.padding(horizontal = 16.dp, vertical = 12.dp),
|
||||||
.padding(horizontal = 16.dp, vertical = 12.dp),
|
) {
|
||||||
) {
|
|
||||||
Text("${repository.owner}/${repository.name}", style = MaterialTheme.typography.titleMedium)
|
Text("${repository.owner}/${repository.name}", style = MaterialTheme.typography.titleMedium)
|
||||||
Text(
|
Text(
|
||||||
stringResource(
|
stringResource(
|
||||||
@ -248,8 +244,6 @@ private fun RepositoryContent(
|
|||||||
style = MaterialTheme.typography.bodySmall,
|
style = MaterialTheme.typography.bodySmall,
|
||||||
)
|
)
|
||||||
LastPushedBranchRow(repository, onRetryLastPushedBranch)
|
LastPushedBranchRow(repository, onRetryLastPushedBranch)
|
||||||
}
|
|
||||||
FavoriteButton(state.mode, repository, onToggleFavorite)
|
|
||||||
}
|
}
|
||||||
HorizontalDivider()
|
HorizontalDivider()
|
||||||
}
|
}
|
||||||
@ -308,37 +302,6 @@ private fun ModeTabs(mode: RepositoryListMode, onShowAllRepositories: () -> Unit
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@Composable
|
|
||||||
private fun FavoriteButton(
|
|
||||||
mode: RepositoryListMode,
|
|
||||||
repository: RepositoryRowUiModel,
|
|
||||||
onToggle: (RepositoryRowUiModel) -> Unit,
|
|
||||||
) {
|
|
||||||
Column(Modifier.padding(end = 16.dp, top = 12.dp)) {
|
|
||||||
when (repository.favoriteAction) {
|
|
||||||
FavoriteActionUiState.InProgress -> CircularProgressIndicator(Modifier.size(20.dp))
|
|
||||||
FavoriteActionUiState.Failed -> OutlinedButton(onClick = { onToggle(repository) }) {
|
|
||||||
Text(stringResource(R.string.repositories_retry))
|
|
||||||
}
|
|
||||||
FavoriteActionUiState.Starred -> Text(
|
|
||||||
stringResource(R.string.repositories_favorited),
|
|
||||||
style = MaterialTheme.typography.bodySmall,
|
|
||||||
)
|
|
||||||
FavoriteActionUiState.Idle -> OutlinedButton(onClick = { onToggle(repository) }) {
|
|
||||||
Text(
|
|
||||||
stringResource(
|
|
||||||
if (mode == RepositoryListMode.FAVORITES) {
|
|
||||||
R.string.repositories_unfavorite
|
|
||||||
} else {
|
|
||||||
R.string.repositories_favorite
|
|
||||||
},
|
|
||||||
),
|
|
||||||
)
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
}
|
|
||||||
|
|
||||||
@Composable
|
@Composable
|
||||||
private fun LastPushedBranchRow(
|
private fun LastPushedBranchRow(
|
||||||
repository: RepositoryRowUiModel,
|
repository: RepositoryRowUiModel,
|
||||||
|
|||||||
@ -1,6 +1,7 @@
|
|||||||
package com.ubisam.ubgitea.feature.repositories
|
package com.ubisam.ubgitea.feature.repositories
|
||||||
|
|
||||||
import android.app.Application
|
import android.app.Application
|
||||||
|
import com.ubisam.ubgitea.BuildConfig
|
||||||
import androidx.lifecycle.ViewModel
|
import androidx.lifecycle.ViewModel
|
||||||
import androidx.lifecycle.ViewModelProvider
|
import androidx.lifecycle.ViewModelProvider
|
||||||
import androidx.lifecycle.viewModelScope
|
import androidx.lifecycle.viewModelScope
|
||||||
@ -64,6 +65,10 @@ sealed interface RepositoryListUiState {
|
|||||||
val type: RepositoryListError,
|
val type: RepositoryListError,
|
||||||
val accountName: String? = null,
|
val accountName: String? = null,
|
||||||
val canLogout: Boolean = false,
|
val canLogout: Boolean = false,
|
||||||
|
// Debug-build-only diagnostic tag (non-secret: no code/token/state values), null in
|
||||||
|
// release builds regardless of what caused the error. Never localized or user-facing
|
||||||
|
// copy — purely so a debug build can show which internal branch was taken.
|
||||||
|
val debugDetail: String? = null,
|
||||||
) : RepositoryListUiState
|
) : RepositoryListUiState
|
||||||
data class ReferencesLoading(
|
data class ReferencesLoading(
|
||||||
val accountName: String,
|
val accountName: String,
|
||||||
@ -168,17 +173,28 @@ class RepositoryListViewModel(
|
|||||||
|
|
||||||
fun handleCallback(uri: String) {
|
fun handleCallback(uri: String) {
|
||||||
startOperation {
|
startOperation {
|
||||||
when (val result = callbackHandler.handle(uri)) {
|
try {
|
||||||
is HandledOAuthCallback.Success -> authenticate(result.authorizationCode, result.verifier)
|
when (val result = callbackHandler.handle(uri)) {
|
||||||
HandledOAuthCallback.Rejected -> mutableState.value = RepositoryListUiState.Error(
|
is HandledOAuthCallback.Success -> authenticate(result.authorizationCode, result.verifier)
|
||||||
RepositoryListError.INVALID_CALLBACK,
|
is HandledOAuthCallback.Rejected -> showCallbackError(result.reason)
|
||||||
activeAccountName,
|
}
|
||||||
hasAuthenticatedSession,
|
} catch (failure: Throwable) {
|
||||||
)
|
if (failure is CancellationException) throw failure
|
||||||
|
runCatching { pendingAuthStore.clear() }
|
||||||
|
showCallbackError("callback_processing_failed")
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private fun showCallbackError(reason: String) {
|
||||||
|
mutableState.value = RepositoryListUiState.Error(
|
||||||
|
RepositoryListError.INVALID_CALLBACK,
|
||||||
|
activeAccountName,
|
||||||
|
hasAuthenticatedSession,
|
||||||
|
debugDetail = if (BuildConfig.DEBUG) reason else null,
|
||||||
|
)
|
||||||
|
}
|
||||||
|
|
||||||
fun retry() {
|
fun retry() {
|
||||||
val current = mutableState.value
|
val current = mutableState.value
|
||||||
if (current is RepositoryListUiState.ReferencesError) {
|
if (current is RepositoryListUiState.ReferencesError) {
|
||||||
|
|||||||
@ -13,7 +13,10 @@ import androidx.compose.runtime.saveable.listSaver
|
|||||||
*/
|
*/
|
||||||
val RouteStackSaver: Saver<List<Route>, Any> = listSaver(
|
val RouteStackSaver: Saver<List<Route>, Any> = listSaver(
|
||||||
save = { stack: List<Route> -> stack.map { route -> RouteEncoding.encode(route) } },
|
save = { stack: List<Route> -> stack.map { route -> RouteEncoding.encode(route) } },
|
||||||
restore = { encoded: List<String> -> encoded.map { value -> RouteEncoding.decode(value) } },
|
restore = { encoded: List<String> ->
|
||||||
|
encoded.mapNotNull(RouteEncoding::decodeOrNull)
|
||||||
|
.ifEmpty { listOf(Route.Repositories) }
|
||||||
|
},
|
||||||
)
|
)
|
||||||
|
|
||||||
internal object RouteEncoding {
|
internal object RouteEncoding {
|
||||||
@ -32,14 +35,23 @@ internal object RouteEncoding {
|
|||||||
Route.Notifications -> TYPE_NOTIFICATIONS
|
Route.Notifications -> TYPE_NOTIFICATIONS
|
||||||
}
|
}
|
||||||
|
|
||||||
fun decode(encoded: String): Route {
|
fun decode(encoded: String): Route = decodeOrNull(encoded) ?: Route.Repositories
|
||||||
|
|
||||||
|
fun decodeOrNull(encoded: String): Route? {
|
||||||
val parts = encoded.split(FIELD_SEPARATOR)
|
val parts = encoded.split(FIELD_SEPARATOR)
|
||||||
return when (parts.first()) {
|
return when (parts.first()) {
|
||||||
TYPE_CODE_BROWSER -> Route.CodeBrowser(owner = parts[1], name = parts[2], refKind = parts[3], ref = parts[4])
|
TYPE_CODE_BROWSER -> parts.takeIf { it.size == 5 }?.let {
|
||||||
TYPE_HISTORY -> Route.History(owner = parts[1], name = parts[2], ref = parts[3])
|
Route.CodeBrowser(owner = it[1], name = it[2], refKind = it[3], ref = it[4])
|
||||||
TYPE_WORK_ITEMS -> Route.WorkItems(owner = parts[1], name = parts[2])
|
}
|
||||||
|
TYPE_HISTORY -> parts.takeIf { it.size == 4 }?.let {
|
||||||
|
Route.History(owner = it[1], name = it[2], ref = it[3])
|
||||||
|
}
|
||||||
|
TYPE_WORK_ITEMS -> parts.takeIf { it.size == 3 }?.let {
|
||||||
|
Route.WorkItems(owner = it[1], name = it[2])
|
||||||
|
}
|
||||||
TYPE_NOTIFICATIONS -> Route.Notifications
|
TYPE_NOTIFICATIONS -> Route.Notifications
|
||||||
else -> Route.Repositories
|
TYPE_REPOSITORIES -> Route.Repositories
|
||||||
|
else -> null
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -1,22 +1,15 @@
|
|||||||
package com.ubisam.ubgitea.core.auth.protocol
|
package com.ubisam.ubgitea.core.auth.protocol
|
||||||
|
|
||||||
import java.net.URI
|
import java.net.URI
|
||||||
import org.junit.Assert.assertEquals
|
|
||||||
import org.junit.Assert.assertThrows
|
import org.junit.Assert.assertThrows
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
|
|
||||||
class OAuthClientConfigTest {
|
class OAuthClientConfigTest {
|
||||||
@Test
|
@Test
|
||||||
fun everyNonReadOnlyScopeIsExplicitlyApproved() {
|
fun rejectsWriteScopeInTheProductionOAuthContract() {
|
||||||
// Guards against a future write scope being added to the enum without also being
|
assertThrows(IllegalArgumentException::class.java) {
|
||||||
// added (deliberately, by a human decision) to OAuthClientConfig's approval list.
|
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_USER))
|
||||||
val writeScopes = OAuthScope.entries.filterNot { it.isReadOnly }.toSet()
|
}
|
||||||
assertEquals(writeScopes, OAuthClientConfig.EXPLICITLY_APPROVED_WRITE_SCOPES)
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
|
||||||
fun acceptsExplicitlyApprovedWriteScopeAlongsideReadScopes() {
|
|
||||||
config(setOf(OAuthScope.READ_USER, OAuthScope.WRITE_USER))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|||||||
@ -6,7 +6,6 @@ import com.ubisam.ubgitea.core.auth.protocol.OAuthTransaction
|
|||||||
import com.ubisam.ubgitea.core.auth.protocol.UBGiteaOAuthDefaults
|
import com.ubisam.ubgitea.core.auth.protocol.UBGiteaOAuthDefaults
|
||||||
import kotlinx.coroutines.test.runTest
|
import kotlinx.coroutines.test.runTest
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
import org.junit.Assert.assertSame
|
|
||||||
import org.junit.Assert.assertTrue
|
import org.junit.Assert.assertTrue
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
|
|
||||||
@ -27,19 +26,31 @@ class OAuthCallbackHandlerTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `denied and malformed callbacks still consume pending state`() = runTest {
|
fun `denied and malformed callbacks still consume pending state and carry a diagnostic reason`() = runTest {
|
||||||
listOf(
|
listOf(
|
||||||
"${UBGiteaOAuthDefaults.config.redirectUri}?state=expected-state&error=access_denied",
|
"${UBGiteaOAuthDefaults.config.redirectUri}?state=expected-state&error=access_denied" to "denied:access_denied",
|
||||||
"not a uri %",
|
"not a uri %" to "malformed_uri",
|
||||||
).forEach { callback ->
|
).forEach { (callback, expectedReason) ->
|
||||||
val store = FakePendingStore(transaction)
|
val store = FakePendingStore(transaction)
|
||||||
|
|
||||||
assertSame(HandledOAuthCallback.Rejected, handler(store).handle(callback))
|
val result = handler(store).handle(callback)
|
||||||
|
|
||||||
|
assertTrue(result is HandledOAuthCallback.Rejected)
|
||||||
|
assertEquals(expectedReason, (result as HandledOAuthCallback.Rejected).reason)
|
||||||
assertEquals(1, store.consumeCount)
|
assertEquals(1, store.consumeCount)
|
||||||
assertEquals(null, store.pending)
|
assertEquals(null, store.pending)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `no pending transaction is rejected with its own reason and never touches the validator`() = runTest {
|
||||||
|
val store = FakePendingStore(pending = null)
|
||||||
|
|
||||||
|
val result = handler(store).handle("${UBGiteaOAuthDefaults.config.redirectUri}?state=x&code=y")
|
||||||
|
|
||||||
|
assertEquals(HandledOAuthCallback.Rejected("no_pending_transaction"), result)
|
||||||
|
}
|
||||||
|
|
||||||
private fun handler(store: FakePendingStore) = OAuthCallbackHandler(
|
private fun handler(store: FakePendingStore) = OAuthCallbackHandler(
|
||||||
store,
|
store,
|
||||||
OAuthCallbackValidator(UBGiteaOAuthDefaults.config),
|
OAuthCallbackValidator(UBGiteaOAuthDefaults.config),
|
||||||
|
|||||||
@ -98,6 +98,24 @@ class RepositoryListViewModelTest {
|
|||||||
assertEquals("repo", error.repository.name)
|
assertEquals("repo", error.repository.name)
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `callback storage failure becomes recoverable error instead of crashing app`() = runTest(dispatcher.scheduler) {
|
||||||
|
val failingStore = object : PendingAuthTransactionStore {
|
||||||
|
override suspend fun save(transaction: OAuthTransaction) = Unit
|
||||||
|
override suspend fun consume(): OAuthTransaction? = error("corrupt pending state")
|
||||||
|
override suspend fun clear() = Unit
|
||||||
|
}
|
||||||
|
val viewModel = viewModel(FakeBrowser(), failingStore)
|
||||||
|
advanceUntilIdle()
|
||||||
|
|
||||||
|
viewModel.handleCallback("https://gitea.ubisampaju.synology.me/ubisampaju/oauth/callback?state=x&code=y")
|
||||||
|
advanceUntilIdle()
|
||||||
|
|
||||||
|
val error = viewModel.state.value as RepositoryListUiState.Error
|
||||||
|
assertEquals(RepositoryListError.INVALID_CALLBACK, error.type)
|
||||||
|
assertEquals("callback_processing_failed", error.debugDetail)
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `repository list requests recent-push order and pages five at a time`() = runTest(dispatcher.scheduler) {
|
fun `repository list requests recent-push order and pages five at a time`() = runTest(dispatcher.scheduler) {
|
||||||
val browser = FakeBrowser(
|
val browser = FakeBrowser(
|
||||||
@ -397,9 +415,12 @@ class RepositoryListViewModelTest {
|
|||||||
updatedAtEpochMillis = 1_000L,
|
updatedAtEpochMillis = 1_000L,
|
||||||
)
|
)
|
||||||
|
|
||||||
private fun viewModel(browser: FakeBrowser): RepositoryListViewModel = RepositoryListViewModel(
|
private fun viewModel(
|
||||||
|
browser: FakeBrowser,
|
||||||
|
pendingStore: PendingAuthTransactionStore = FakePendingStore(),
|
||||||
|
): RepositoryListViewModel = RepositoryListViewModel(
|
||||||
tokenVault = FakeTokenVault(TokenSet("access", null, null)),
|
tokenVault = FakeTokenVault(TokenSet("access", null, null)),
|
||||||
pendingAuthStore = FakePendingStore(),
|
pendingAuthStore = pendingStore,
|
||||||
authApi = FakeAuthGateway(),
|
authApi = FakeAuthGateway(),
|
||||||
browserApi = browser,
|
browserApi = browser,
|
||||||
favoritesApi = browser,
|
favoritesApi = browser,
|
||||||
|
|||||||
@ -33,6 +33,13 @@ class RouteEncodingTest {
|
|||||||
assertEquals(Route.Repositories, RouteEncoding.decode("not-a-known-route-type"))
|
assertEquals(Route.Repositories, RouteEncoding.decode("not-a-known-route-type"))
|
||||||
}
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun `truncated persisted routes fall back instead of indexing missing fields`() {
|
||||||
|
assertEquals(Route.Repositories, RouteEncoding.decode("codeBrowser\u001fowner"))
|
||||||
|
assertEquals(Route.Repositories, RouteEncoding.decode("history\u001fowner\u001frepo"))
|
||||||
|
assertEquals(Route.Repositories, RouteEncoding.decode("workItems\u001fowner"))
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
fun `a full back stack round-trips through the saver in order`() {
|
fun `a full back stack round-trips through the saver in order`() {
|
||||||
val stack = listOf(
|
val stack = listOf(
|
||||||
|
|||||||
@ -1,5 +1,7 @@
|
|||||||
# ADR-004: Favorites use Gitea star via a new `write:user` scope
|
# ADR-004: Favorites use Gitea star via a new `write:user` scope
|
||||||
|
|
||||||
|
Status: suspended by TASK-019 after target-server authorization failure. Production defaults have returned to read-only scopes; this decision must not be used as release authorization.
|
||||||
|
|
||||||
## Status
|
## Status
|
||||||
|
|
||||||
Accepted — 2026-08-26
|
Accepted — 2026-08-26
|
||||||
|
|||||||
@ -294,3 +294,14 @@ This is the append-only implementation and handoff record shared by Codex and Cl
|
|||||||
- Known limitations (both recorded as explicitly blocked acceptance criteria, not silently skipped): (1) R8/ProGuard minification was not enabled — left as an open decision for whoever has real device + login access to verify safely. (2) On-device regression of every authenticated screen built this session (repository list/favorites/organizations, code browser, history, work items, notifications, navigation back-stack, logout) could not be performed — structurally blocked on this agent never entering a password, confirmed with the human product owner rather than assumed.
|
- Known limitations (both recorded as explicitly blocked acceptance criteria, not silently skipped): (1) R8/ProGuard minification was not enabled — left as an open decision for whoever has real device + login access to verify safely. (2) On-device regression of every authenticated screen built this session (repository list/favorites/organizations, code browser, history, work items, notifications, navigation back-stack, logout) could not be performed — structurally blocked on this agent never entering a password, confirmed with the human product owner rather than assumed.
|
||||||
- Next recommended task: This closes the originally-proposed roadmap (TASK-007 through TASK-016). Remaining work is exactly the two blocked items above, both requiring a human (or an agent with real login access) at a real device: complete the authenticated-screen regression pass, then make an informed call on enabling minification with real verification against a build that has actually exercised the JSON-heavy screens.
|
- Next recommended task: This closes the originally-proposed roadmap (TASK-007 through TASK-016). Remaining work is exactly the two blocked items above, both requiring a human (or an agent with real login access) at a real device: complete the authenticated-screen regression pass, then make an informed call on enabling minification with real verification against a build that has actually exercised the JSON-heavy screens.
|
||||||
- Next owner: Human product owner or Codex, for the device-and-login-dependent items above; otherwise no further roadmap items remain unassigned.
|
- Next owner: Human product owner or Codex, for the device-and-login-dependent items above; otherwise no further roadmap items remain unassigned.
|
||||||
|
## 2026-08-26 — TASK-019 OAuth callback crash recovery
|
||||||
|
|
||||||
|
- Agent and role: Codex, incident resolver; Claude diagnostic changes preserved.
|
||||||
|
- Commit/diff: working diff on `claude/task-015-notifications` pending commit.
|
||||||
|
- Features implemented: Restored the production OAuth request to its previously device-verified read-only scopes; added safe callback exception recovery, non-secret debug reason tags, and defensive saved-route decoding.
|
||||||
|
- How it was implemented: `write:user` is no longer part of `UBGiteaOAuthDefaults`; callback storage/validation failures are caught at the ViewModel coroutine boundary and become `INVALID_CALLBACK`; malformed or truncated route records are discarded and fall back to `Repositories`.
|
||||||
|
- Checks and results: Focused OAuth/repository/navigation tests passed; debug build, 184 unit tests across 30 suites, and lint passed; diff and focused secret checks passed.
|
||||||
|
- Review findings and resolutions: P1 target-server authorization incompatibility from TASK-018 scope expansion was resolved by suspending write scope. P1 repeat-launch crash risk from unchecked saved-route indexes was fixed with shape validation. Callback exception regression coverage was added.
|
||||||
|
- Known limitations: Physical-device callback verification is still required. Gitea star/unstar mutation is intentionally unavailable with the restored read-only token.
|
||||||
|
- Next recommended task: Install the rebuilt debug APK, clear UBGitea app data once, complete login approval, and verify repository loading.
|
||||||
|
- Next owner: Human product owner for device verification, then Claude for review.
|
||||||
|
|||||||
@ -1,6 +1,6 @@
|
|||||||
# TASK-018: Favorites tab backed by Gitea star
|
# TASK-018: Favorites tab backed by Gitea star
|
||||||
|
|
||||||
Status: implemented; all required checks pass; pending Codex review and device verification
|
Status: write operation suspended after production OAuth incompatibility; read-only favorites listing remains
|
||||||
Owner: Claude (human product owner priority-1 request, 2026-08-26)
|
Owner: Claude (human product owner priority-1 request, 2026-08-26)
|
||||||
Reviewer: Codex
|
Reviewer: Codex
|
||||||
|
|
||||||
@ -99,4 +99,4 @@ No open P0-P3 findings from self-review against the acceptance criteria. Request
|
|||||||
|
|
||||||
## Review findings
|
## Review findings
|
||||||
|
|
||||||
None yet.
|
- P1 production finding: changing the remembered OAuth grant to include `write:user` caused Gitea to return `error=server_error` during authorization on the target device. TASK-019 restores the previously verified read-only scope set. Star/unstar UI must not be considered releasable until a separate compatible authorization flow passes device verification.
|
||||||
|
|||||||
33
tasks/019-oauth-callback-crash-recovery.md
Normal file
33
tasks/019-oauth-callback-crash-recovery.md
Normal file
@ -0,0 +1,33 @@
|
|||||||
|
# TASK-019: Recover OAuth callback without an app crash
|
||||||
|
|
||||||
|
Status: implemented; automated verification complete; device login pending
|
||||||
|
Owner: Codex
|
||||||
|
Reviewer: Claude
|
||||||
|
|
||||||
|
## Outcome
|
||||||
|
|
||||||
|
OAuth approval returns safely to UBGitea. A rejected or malformed callback, corrupt pending transaction, and stale saved route produce a recoverable screen instead of terminating the process or creating a launch-crash loop.
|
||||||
|
|
||||||
|
## Writable paths
|
||||||
|
|
||||||
|
- `app/src/main/java/com/ubisam/ubgitea/core/auth/protocol/OAuthClientConfig.kt`
|
||||||
|
- `app/src/main/java/com/ubisam/ubgitea/feature/repositories/`
|
||||||
|
- `app/src/main/java/com/ubisam/ubgitea/ui/navigation/RouteStackSaver.kt`
|
||||||
|
- matching unit tests
|
||||||
|
- TASK-018/019, ADR-004, and development-log incident evidence
|
||||||
|
|
||||||
|
## Acceptance criteria
|
||||||
|
|
||||||
|
- [x] Production defaults request only the previously device-verified read-only scopes.
|
||||||
|
- [x] A callback-processing exception maps to a recoverable invalid-callback state.
|
||||||
|
- [x] Debug diagnostics contain only stable reason tags and no code, token, verifier, or state.
|
||||||
|
- [x] Malformed/truncated saved routes restore to the repository root without indexing missing fields.
|
||||||
|
- [x] Focused OAuth, repository, and navigation tests pass.
|
||||||
|
- [ ] Physical device login and callback pass with the rebuilt APK.
|
||||||
|
|
||||||
|
## Evidence
|
||||||
|
|
||||||
|
- Incident reproduction: after TASK-018 added `write:user`, Gitea returned `error=server_error`; after app data was cleared, callback approval was followed by process termination and stale-state relaunch failure.
|
||||||
|
- The default scope set is restored to the previously verified read-only set. Gitea-native star mutation remains disabled by lack of write scope until a separate server-compatible authorization design is approved and verified.
|
||||||
|
- Callback and route restoration now fail closed into UI state. No production credential or callback value is surfaced.
|
||||||
|
- `assembleDebug testDebugUnitTest lintDebug` passed: 184 tests across 30 suites, zero failures; lint and diff checks passed.
|
||||||
Loading…
x
Reference in New Issue
Block a user