Make 2FA setup and revoke-all-sessions work

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

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

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() }

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,

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() },

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<SessionRead>): List<SessionRead> =
+    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.",
+                    )
+                }
+            }
         }
     }
 

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() },

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()

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) }
                 }
         }
     }

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)

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 }
+    }
 }