diff --git a/CHANGELOG.md b/CHANGELOG.md index 9856ea0..7a35917 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ uses semantic versioning (`MAJOR.MINOR.PATCH`). ### Fixed +- **Two-factor authentication can be turned on again.** "Set Up 2FA" failed + every time, because the server has required the account password to start + enrolment since 0.5.0 and the app never sent one, so there was no way to add a + second factor from the phone at all. Setup now asks for the password first, + and says so if it is wrong instead of opening a sheet with no secret in it. + +- **Active Sessions knows which session is yours, and "Revoke All Other + Sessions" works.** The list marked every session as another device, including + the one you were looking at it from, and the revoke-all button quietly did + nothing at all: it asked the server to work out the current session from a + cookie this app does not send, which the server answered by refusing the whole + request. Your own session is now labelled and left alone, and the others are + signed out one by one. + - **"Delete unused files" no longer deletes files that are in use.** It worked out which uploads were unused by looking at your system avatar and your members' avatars, and nothing else, so banners, any image inside a bio or a diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt index a3dedc8..4523d38 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/SheafApiService.kt @@ -32,7 +32,7 @@ interface SheafApiService { suspend fun logout() @POST("/v1/auth/totp/setup") - suspend fun setupTotp(): TOTPSetupResponse + suspend fun setupTotp(@Body body: TOTPSetupRequest): TOTPSetupResponse @POST("/v1/auth/totp/verify") suspend fun verifyTotp(@Body body: TOTPVerify) @@ -112,8 +112,10 @@ interface SheafApiService { @DELETE("/v1/auth/sessions/{id}") suspend fun revokeSession(@Path("id") id: String) - @POST("/v1/auth/sessions/revoke-others") - suspend fun revokeOtherSessions(@Body body: TokenRefresh) + // POST /v1/auth/sessions/revoke-others is deliberately not declared here. + // It reads the caller's session from the `sheaf_session` cookie, which this + // client never sends, so it can only ever answer 400 "No current session". + // SessionsViewModel revokes each handle individually instead. /** * Mint a child session + independent refresh token for a paired diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt index a64daf3..400df9c 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/api/TokenAuthenticator.kt @@ -46,9 +46,15 @@ class TokenAuthenticator @Inject constructor( if (!originMatches(response.request.url, baseForGuard)) return null val path = response.request.url.encodedPath + // /auth/totp/setup answers 401 for a wrong password, not an expired + // token. Refreshing and retrying there burns a one-shot refresh + // rotation and sends the same wrong password a second time, and the + // server counts each attempt towards the password lockout, so a couple + // of typos could lock the account. if (path.endsWith("/auth/refresh") || path.endsWith("/auth/delete-account") || path.endsWith("/delete-confirmation") || + path.endsWith("/auth/totp/setup") || path.endsWith("/notifications/redeem")) return null val storedAccessToken = runBlocking { prefs.accessToken.firstOrNull() } diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt index c316e0c..fd8b4cb 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/data/model/Models.kt @@ -184,6 +184,19 @@ data class SecondarySessionRequest( @Json(name = "client_name") val clientName: String? = null, ) +/** + * Starting TOTP enrolment needs the account password. + * + * The server has required it since 0.5.0, so that a session-only attacker + * cannot enrol their own second factor and lock the owner out of changing their + * own password or email. This client sent no body at all, so turning 2FA on + * answered 422 every time and was simply impossible. + */ +@JsonClass(generateAdapter = true) +data class TOTPSetupRequest( + val password: String, +) + @JsonClass(generateAdapter = true) data class TOTPVerify( val code: String, diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/OnboardingScreen.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/OnboardingScreen.kt index 6a0841f..d20a213 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/OnboardingScreen.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/auth/OnboardingScreen.kt @@ -117,6 +117,7 @@ fun OnboardingScreen( if (showTotpSheet) { TotpSetupSheet( state = state, + onSubmitPassword = { password -> settingsViewModel.submitTotpPassword(password) }, onAdvanceToVerify = { settingsViewModel.advanceTotpToVerify() }, onVerify = { code -> settingsViewModel.verifyTotp(code) }, onAdvanceToDone = { settingsViewModel.advanceTotpToDone() }, diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/sessions/SessionsViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/sessions/SessionsViewModel.kt index 9100f34..4f6736a 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/sessions/SessionsViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/sessions/SessionsViewModel.kt @@ -5,13 +5,13 @@ import androidx.lifecycle.viewModelScope import systems.lupine.sheaf.data.api.SheafApiService import systems.lupine.sheaf.data.model.SessionRead import systems.lupine.sheaf.data.model.SessionUpdate -import systems.lupine.sheaf.data.model.TokenRefresh import systems.lupine.sheaf.data.repository.PreferencesRepository import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.* import kotlinx.coroutines.launch import retrofit2.HttpException import systems.lupine.sheaf.util.extractSessionId +import systems.lupine.sheaf.util.sessionHandle import systems.lupine.sheaf.util.toUserMessage import javax.inject.Inject @@ -22,6 +22,17 @@ data class SessionsUiState( val isRevoking: Boolean = false, ) +/** + * The sessions "revoke all others" should end: everything the server did not + * mark as the caller's own. + * + * Filtered here rather than leaning on the server's refusal to revoke the + * current session, so the request is never sent in the first place. Revoking + * the current one would sign the user out of the phone they are holding. + */ +internal fun sessionsToRevoke(sessions: List): List = + sessions.filterNot { it.isCurrent } + @HiltViewModel class SessionsViewModel @Inject constructor( private val api: SheafApiService, @@ -38,12 +49,15 @@ class SessionsViewModel @Inject constructor( _state.update { it.copy(isLoading = true, error = null) } // The server's /v1/auth/sessions identifies the current session by // cookie, which our bearer-token client doesn't carry, so it always - // returns is_current=false. Fall back to the JWT's `sid` claim. - val sid = extractSessionId(prefs.accessToken.firstOrNull()) + // returns is_current=false. Fall back to the JWT's `sid` claim, + // hashed into the handle the list addresses sessions by: the raw id + // is the cookie credential and is never returned, so comparing the + // two directly matched nothing and left every row marked "other". + val handle = extractSessionId(prefs.accessToken.firstOrNull())?.let { sessionHandle(it) } runCatching { api.listSessions() } .onSuccess { sessions -> - val tagged = if (sid != null) { - sessions.map { it.copy(isCurrent = it.id == sid) } + val tagged = if (handle != null) { + sessions.map { it.copy(isCurrent = it.id == handle) } } else sessions _state.update { it.copy(isLoading = false, sessions = tagged) } } @@ -76,17 +90,47 @@ class SessionsViewModel @Inject constructor( } } + /** + * Revoke every session except this one, one at a time. + * + * Not `POST /auth/sessions/revoke-others`, which this used to call and + * which can never work from here: it identifies the caller's session from + * the `sheaf_session` cookie, and this client authenticates with a bearer + * token and deliberately keeps only the trusted-device cookie. Every + * attempt came back 400 "No current session", so the button has never + * revoked anything. + * + * Revoking each handle individually is the same outcome by a route that + * works with a bearer token. Which one is ours is already known: the server + * cannot tell either, for the same cookie reason, so [load] re-tags + * `isCurrent` from the access token's own `sid` claim. + */ fun revokeOtherSessions() { viewModelScope.launch { _state.update { it.copy(isRevoking = true, error = null) } - val refreshToken = prefs.refreshToken.firstOrNull() - if (refreshToken == null) { - _state.update { it.copy(isRevoking = false, error = "Not signed in") } + val others = sessionsToRevoke(_state.value.sessions) + if (others.isEmpty()) { + _state.update { it.copy(isRevoking = false) } return@launch } - runCatching { api.revokeOtherSessions(TokenRefresh(refreshToken)) } - .onSuccess { load() } - .onFailure { e -> _state.update { it.copy(isRevoking = false, error = e.toUserMessage("Failed to revoke sessions")) } } + // Not atomic, so a failure part way leaves the rest revoked. That + // is the right way round for this action: the point is to shut + // other sessions out, and a partial result is still progress. The + // reload shows exactly which survived. + val failures = others.count { session -> + runCatching { api.revokeSession(session.id) }.isFailure + } + // After the reload, not before: load() clears `error`, so setting + // it first would report a partial failure to nobody. + load() + if (failures > 0) { + _state.update { + it.copy( + error = "Couldn't revoke $failures of ${others.size} sessions. " + + "The rest were signed out.", + ) + } + } } } diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt index 6325b46..50db534 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsCategoryScreens.kt @@ -1041,6 +1041,7 @@ fun AccountSettingsScreen( if (showTotpSheet) { TotpSetupSheet( state = state, + onSubmitPassword = { password -> viewModel.submitTotpPassword(password) }, onAdvanceToVerify = { viewModel.advanceTotpToVerify() }, onVerify = { code -> viewModel.verifyTotp(code) }, onAdvanceToDone = { viewModel.advanceTotpToDone() }, diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsScreen.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsScreen.kt index 6ef14f1..d47a667 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsScreen.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsScreen.kt @@ -327,6 +327,7 @@ fun SettingsScreen( @Composable internal fun TotpSetupSheet( state: SettingsUiState, + onSubmitPassword: (String) -> Unit, onAdvanceToVerify: () -> Unit, onVerify: (String) -> Unit, onAdvanceToDone: () -> Unit, @@ -336,6 +337,7 @@ internal fun TotpSetupSheet( val scope = rememberCoroutineScope() var code by remember { mutableStateOf("") } var copiedSecret by remember { mutableStateOf(false) } + var totpPassword by remember { mutableStateOf("") } var copiedCodes by remember { mutableStateOf(false) } ModalBottomSheet( @@ -352,6 +354,35 @@ internal fun TotpSetupSheet( horizontalAlignment = Alignment.CenterHorizontally, ) { when (state.totpStep) { + // The server will not mint a secret without the account + // password, so this is where enrolment starts. + TotpStep.PASSWORD -> { + Text("Set Up 2FA", style = MaterialTheme.typography.titleLarge) + Text( + "Confirm your password to start. This stops someone who has got hold " + + "of your signed-in phone from adding their own second factor.", + style = MaterialTheme.typography.bodyMedium, + color = MaterialTheme.colorScheme.onSurfaceVariant, + ) + state.totpError?.let { + Text(it, style = MaterialTheme.typography.bodySmall, color = MaterialTheme.colorScheme.error) + } + OutlinedTextField( + value = totpPassword, + onValueChange = { totpPassword = it }, + label = { Text("Password") }, + singleLine = true, + visualTransformation = PasswordVisualTransformation(), + keyboardOptions = KeyboardOptions(keyboardType = KeyboardType.Password), + modifier = Modifier.fillMaxWidth(), + ) + Button( + onClick = { onSubmitPassword(totpPassword) }, + enabled = totpPassword.isNotBlank(), + modifier = Modifier.fillMaxWidth(), + ) { Text("Continue") } + } + TotpStep.LOADING -> { Text("Set Up 2FA", style = MaterialTheme.typography.titleLarge) CircularProgressIndicator() diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt index c9822d1..c286d5b 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/ui/settings/SettingsViewModel.kt @@ -25,7 +25,7 @@ import retrofit2.HttpException import systems.lupine.sheaf.util.toUserMessage import javax.inject.Inject -enum class TotpStep { LOADING, SECRET, VERIFY, RECOVERY_CODES, DONE } +enum class TotpStep { PASSWORD, LOADING, SECRET, VERIFY, RECOVERY_CODES, DONE } /** Step-up auth and grace-period state for the orphan-files cleanup action. */ data class OrphanFilesDeleteSafety( @@ -44,7 +44,7 @@ data class SettingsUiState( val isLoading: Boolean = false, val error: String? = null, // TOTP - val totpStep: TotpStep = TotpStep.LOADING, + val totpStep: TotpStep = TotpStep.PASSWORD, val totpSetupResponse: TOTPSetupResponse? = null, val totpError: String? = null, val totpIsVerifying: Boolean = false, @@ -347,15 +347,40 @@ class SettingsViewModel @Inject constructor( // ── TOTP ────────────────────────────────────────────────────────────────── + /** + * Open the sheet on the password step. No request yet: the server will not + * mint a secret without the password, so there is nothing to ask for until + * it has been typed. + */ fun startTotpSetup() { - _state.update { it.copy(totpStep = TotpStep.LOADING, totpError = null, totpSetupResponse = null) } + _state.update { + it.copy(totpStep = TotpStep.PASSWORD, totpError = null, totpSetupResponse = null) + } + } + + /** + * Exchange the password for a secret. + * + * A wrong password comes back 401, which stays on this step with the + * message rather than failing forward into a sheet showing no secret. That + * was the old behaviour for every failure, including the 422 this client + * used to get for sending no password at all. + */ + fun submitTotpPassword(password: String) { + if (password.isBlank()) return + _state.update { it.copy(totpStep = TotpStep.LOADING, totpError = null) } viewModelScope.launch { - runCatching { api.setupTotp() } + runCatching { api.setupTotp(systems.lupine.sheaf.data.model.TOTPSetupRequest(password)) } .onSuccess { resp -> _state.update { it.copy(totpSetupResponse = resp, totpStep = TotpStep.SECRET) } } .onFailure { e -> - _state.update { it.copy(totpError = e.toUserMessage(), totpStep = TotpStep.SECRET) } + val message = if (e is HttpException && e.code() == 401) { + "That password is not right." + } else { + e.toUserMessage("Couldn't start two-factor setup") + } + _state.update { it.copy(totpError = message, totpStep = TotpStep.PASSWORD) } } } } diff --git a/sheaf/app/src/main/java/systems/lupine/sheaf/util/Jwt.kt b/sheaf/app/src/main/java/systems/lupine/sheaf/util/Jwt.kt index 82740e6..01c0e02 100644 --- a/sheaf/app/src/main/java/systems/lupine/sheaf/util/Jwt.kt +++ b/sheaf/app/src/main/java/systems/lupine/sheaf/util/Jwt.kt @@ -24,3 +24,24 @@ fun extractSessionId(jwt: String?): String? { if (jwt.isNullOrBlank()) return null return decodeJwtPayload(jwt)?.optString("sid")?.takeIf { it.isNotBlank() } } + +/** + * The opaque handle `/v1/auth/sessions` lists a session under. + * + * The raw session id is itself the `sheaf_session` cookie credential, so no + * read surface returns it; every one returns `sha256(id)` truncated to 32 hex + * characters instead, and the mutation endpoints resolve that back. + * + * Needed because the two values were being compared directly to find the + * current session, which can never match: one is the digest of the other. The + * session list therefore showed no "this device" at all, and anything that + * tried to leave the current session alone was working from a flag that was + * false for every row. + * + * Mirrors `sheaf/auth/sessions.py:session_handle`. + */ +fun sessionHandle(sessionId: String): String = + java.security.MessageDigest.getInstance("SHA-256") + .digest(sessionId.toByteArray(Charsets.UTF_8)) + .joinToString("") { "%02x".format(it) } + .take(32) diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt index d023cc2..7d0d2e3 100644 --- a/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/data/api/TokenAuthenticatorTest.kt @@ -52,4 +52,22 @@ class TokenAuthenticatorTest { assertNull(result) verify(exactly = 0) { prefs.refreshToken } } + + @Test fun `a 401 that means "wrong password" is never retried`() { + // These answer 401 for a credential the user typed, not for an expired + // token. Retrying burns a one-shot refresh rotation and sends the same + // wrong credential again, and the server counts each attempt towards + // the password lockout, so a couple of typos could lock the account. + val passwordPaths = listOf( + "/v1/auth/refresh", + "/v1/auth/delete-account", + "/v1/auth/totp/setup", + "/v1/notifications/redeem", + ) + for (path in passwordPaths) { + val result = authenticator.authenticate(null, response401("https://app.sheaf.sh$path")) + assertNull(result, "$path should not be retried") + } + verify(exactly = 0) { prefs.refreshToken } + } } diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/ui/sessions/RevokeOtherSessionsTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/sessions/RevokeOtherSessionsTest.kt new file mode 100644 index 0000000..8cb8f4a --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/ui/sessions/RevokeOtherSessionsTest.kt @@ -0,0 +1,52 @@ +package systems.lupine.sheaf.ui.sessions + +import systems.lupine.sheaf.data.model.SessionRead +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +/** + * Which sessions "Revoke all other sessions" actually targets. + * + * It used to call `POST /auth/sessions/revoke-others`, which identifies the + * caller's session from the `sheaf_session` cookie. This client authenticates + * with a bearer token and keeps only the trusted-device cookie, so every + * attempt came back 400 "No current session" and the button never revoked + * anything. It now revokes each handle in turn, which makes "which handles" + * the part worth pinning: revoking the current one would sign the user out of + * the phone they are holding. + */ +class RevokeOtherSessionsTest { + + private fun session(id: String, current: Boolean = false) = SessionRead( + id = id, + nickname = null, + clientName = "Sheaf Android", + createdAt = null, + createdIp = null, + lastActiveAt = null, + lastActiveIp = null, + isCurrent = current, + ) + + @Test fun `the current session is never revoked`() { + val sessions = listOf(session("a"), session("b", current = true), session("c")) + assertEquals(listOf("a", "c"), sessionsToRevoke(sessions).map { it.id }) + } + + @Test fun `nothing to do when this is the only session`() { + assertTrue(sessionsToRevoke(listOf(session("a", current = true))).isEmpty()) + } + + @Test fun `an empty list is not an error`() { + assertTrue(sessionsToRevoke(emptyList()).isEmpty()) + } + + @Test fun `a list with no current session still spares nothing by accident`() { + // The server marks the caller, so this should not happen. If it ever + // does, revoking everything is the honest reading of "all others" and + // the worst case is signing this device out, which is recoverable. + val sessions = listOf(session("a"), session("b")) + assertEquals(listOf("a", "b"), sessionsToRevoke(sessions).map { it.id }) + } +} diff --git a/sheaf/app/src/test/java/systems/lupine/sheaf/util/SessionHandleTest.kt b/sheaf/app/src/test/java/systems/lupine/sheaf/util/SessionHandleTest.kt new file mode 100644 index 0000000..69097c0 --- /dev/null +++ b/sheaf/app/src/test/java/systems/lupine/sheaf/util/SessionHandleTest.kt @@ -0,0 +1,44 @@ +package systems.lupine.sheaf.util + +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * The handle the session list addresses a session by. + * + * The raw session id is the `sheaf_session` cookie credential, so no read + * surface returns it; the list returns `sha256(id)` truncated to 32 hex + * characters. This client was comparing the raw `sid` from its own token + * against that digest to find the current session, which cannot match, so the + * list showed no "this device" and every row was marked as another device. + * + * Expected values computed with the server's own algorithm + * (`sheaf/auth/sessions.py:session_handle`). If the two ever drift, the only + * symptom is a flag silently false everywhere, so it is worth pinning to fixed + * digests rather than to a reimplementation of the same formula. + */ +class SessionHandleTest { + + @Test fun `a handle is the first 32 hex characters of the sha-256`() { + assertEquals( + "a765c1df3e91544f89f5555c47390b3c", + sessionHandle("brvY6W1UUbqx_MKeII4CBAYLFk5yA9ubnt4nV3gr10U"), + ) + assertEquals("ba7816bf8f01cfea414140de5dae2223", sessionHandle("abc")) + assertEquals("e3b0c44298fc1c149afbf4c8996fb924", sessionHandle("")) + } + + @Test fun `a handle is 32 characters and lower case hex`() { + val handle = sessionHandle("some-session-id") + assertEquals(32, handle.length) + assertEquals(handle, handle.lowercase()) + assertEquals(true, handle.all { it in "0123456789abcdef" }) + } + + @Test fun `the handle is not the id it came from`() { + // The whole point: returning the raw id would hand out a replayable + // credential, which is why a direct comparison could never work. + val id = "brvY6W1UUbqx_MKeII4CBAYLFk5yA9ubnt4nV3gr10U" + assertEquals(false, sessionHandle(id) == id) + } +}