fix(auth): don't mistake the sign-in stub document for a new user (C-AUTH-001)
Verified on a throwaway emulator with a real paired account: signing in on a new
device sent a fully set-up user to CREATE_PROFILE instead of RECOVERY. Not a rare
race — it happened on every run.
The previous commit guessed at the cause and was wrong: it assumed the profile
read *failed*, and retried on exceptions. Instrumenting the decision showed the
read succeeds and returns a document that exists with no fields at all:
DIAG null=false nameBlank=true sexBlank=true couple=false -> create_profile
DIAG null=false nameBlank=true sexBlank=true couple=false -> create_profile
DIAG null=false nameBlank=false sexBlank=false sexLen=33 couple=true -> home
The first two land within a millisecond of sign-in; the real document arrives
215ms later. Sign-in registers the FCM token, and that merge-write materialises
users/{uid} before the profile has synced. Reading from the server does not dodge
it — Source.SERVER returns the same field-less document — so the read has to be
*trusted* before it is acted on, not just sourced differently.
resolveDestination now treats a field-less document as "ask again" rather than
"new user", retrying until one with real identity on it arrives; only that decides.
A read that never succeeds routes Home, which is non-destructive and routes to
Recovery on its own. New users are unaffected: every path that creates a user
writes email and createdAt before onboarding, and a document that genuinely stays
empty still falls through to profile setup.
This mattered because createUser is a whole-document set(), not a merge, and the
CreateProfile submit calls it: tapping through the screen that should never have
been shown overwrites the real displayName with the locked placeholder and drops
coupleId/partnerId, unpairing the couple.
Keeps getUserFromServer for the decision so an offline read throws rather than
answering from cache. Landmine written up as C-AUTH-001 — the general trap is that
exists() == true does not mean populated, and blank fields right after sign-in mean
"not synced yet".
Verified live end to end on the throwaway: sign-in now lands on Recovery, and a
Title-Cased phrase (previously rejected) unlocks to Home, "Connected with Ben".
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null
parent
7ab80013df
commit
6330bf98ca
|
|
@ -7,6 +7,7 @@ import app.closer.domain.model.User
|
|||
import com.google.firebase.firestore.DocumentSnapshot
|
||||
import com.google.firebase.firestore.FirebaseFirestore
|
||||
import com.google.firebase.firestore.SetOptions
|
||||
import com.google.firebase.firestore.Source
|
||||
import kotlinx.coroutines.channels.awaitClose
|
||||
import kotlinx.coroutines.flow.Flow
|
||||
import kotlinx.coroutines.flow.callbackFlow
|
||||
|
|
@ -66,6 +67,23 @@ class FirestoreUserDataSource @Inject constructor(
|
|||
return if (snap.exists()) snapshotToUser(snap.id, snap) else null
|
||||
}
|
||||
|
||||
/**
|
||||
* The user, straight from the server — never the local cache.
|
||||
*
|
||||
* [getUser] is `Source.DEFAULT`, so right after a fresh sign-in it answers from cache in about a
|
||||
* millisecond with a *hollow* document: `exists() == true`, every field null. The real document
|
||||
* lands ~200ms later. That is fine for a screen that re-renders, and wrong for a one-shot
|
||||
* decision made once at startup — a hollow read is indistinguishable from "this user has no
|
||||
* profile", which is how a paired, fully set-up user got routed into new-user profile setup.
|
||||
*
|
||||
* Throws when the server can't be reached (offline), which is the point: callers making a
|
||||
* destructive decision must be able to tell "no profile" from "don't know yet".
|
||||
*/
|
||||
suspend fun getUserFromServer(uid: String): User? {
|
||||
val snap = userRef(uid).get(Source.SERVER).await()
|
||||
return if (snap.exists()) snapshotToUser(snap.id, snap) else null
|
||||
}
|
||||
|
||||
fun observeUser(uid: String): Flow<User?> = callbackFlow {
|
||||
val listener = userRef(uid).addSnapshotListener { snap, error ->
|
||||
if (error != null) {
|
||||
|
|
|
|||
|
|
@ -15,6 +15,8 @@ class UserRepositoryImpl @Inject constructor(
|
|||
|
||||
override suspend fun getUser(uid: String): User? = dataSource.getUser(uid)
|
||||
|
||||
override suspend fun getUserFromServer(uid: String): User? = dataSource.getUserFromServer(uid)
|
||||
|
||||
override fun observeUser(uid: String): Flow<User?> = dataSource.observeUser(uid)
|
||||
|
||||
override suspend fun createUser(user: User) = dataSource.createUser(user)
|
||||
|
|
|
|||
|
|
@ -6,6 +6,9 @@ import kotlinx.coroutines.flow.Flow
|
|||
|
||||
interface UserRepository {
|
||||
suspend fun getUser(uid: String): User?
|
||||
|
||||
/** Server-authoritative read; throws when offline. Use for decisions that must not act on a cache miss. */
|
||||
suspend fun getUserFromServer(uid: String): User?
|
||||
fun observeUser(uid: String): Flow<User?>
|
||||
suspend fun createUser(user: User)
|
||||
suspend fun updateDisplayName(uid: String, displayName: String)
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@ import android.util.Log
|
|||
import androidx.lifecycle.ViewModel
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import app.closer.domain.model.AuthState
|
||||
import app.closer.domain.model.User
|
||||
import app.closer.domain.repository.AuthRepository
|
||||
import app.closer.domain.repository.UserRepository
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
|
|
@ -49,38 +50,65 @@ class OnboardingViewModel @Inject constructor(
|
|||
* Where a signed-in user belongs: the profile step only if they genuinely have no profile yet.
|
||||
*
|
||||
* "Couldn't read the profile" and "has no profile" are NOT the same thing, and treating them the
|
||||
* same was a data-loss trap. A returning user whose profile read failed (slow network, token not
|
||||
* propagated yet — a real race seen right after sign-in) was sent to CREATE_PROFILE, which
|
||||
* greets them with "What should your partner call you?" over a `🔒 Couldn't unlock on this
|
||||
* device` value. Tapping through it re-encrypts and overwrites the very name we had just failed
|
||||
* to read — and they never reach the Recovery screen that would have fixed them.
|
||||
* same was a data-loss trap. Sending a returning user to CREATE_PROFILE greets them with "What
|
||||
* should your partner call you?" over a `🔒 Couldn't unlock on this device` value, and tapping
|
||||
* through it calls `createUser`, which is a whole-document `set()` — it overwrites their real
|
||||
* name with the placeholder and drops `coupleId`/`partnerId`, unpairing the couple. They never
|
||||
* reach the Recovery screen that would have fixed them.
|
||||
*
|
||||
* The repository distinguishes the two cleanly: a missing document is `success(null)`, while a
|
||||
* failed read throws. So only a *successful* read may route to the profile step. If the read
|
||||
* never succeeds we send them Home, which is non-destructive and has its own offline/error
|
||||
* handling — and which routes to Recovery by itself when the couple key is missing.
|
||||
* Reading it once at sign-in is not enough, and this was measured rather than reasoned about. In
|
||||
* the first ~200ms after sign-in the read comes back with a document that *exists* and has no
|
||||
* fields at all — no email, no name, no `coupleId` — because signing in registers the FCM token,
|
||||
* and that merge-write materialises `users/{uid}` before the real profile has synced. The real
|
||||
* document arrives ~200ms later. A returning user on a new phone therefore hit the stub every
|
||||
* time and was sent to profile setup; this was reproduced end to end with a paired account.
|
||||
*
|
||||
* So the read has to be *trusted* before it is acted on: a stub is treated as "ask again", not as
|
||||
* "new user". Only a document with real identity on it decides, and if none ever arrives we send
|
||||
* them Home — non-destructive, and it routes to Recovery by itself when the couple key is missing.
|
||||
* A genuinely new user keeps working: their document is created with an email and `createdAt`
|
||||
* before they ever reach here, and if it truly stays empty we fall through to profile setup.
|
||||
*/
|
||||
private suspend fun resolveDestination(uid: String): String {
|
||||
var sawNoProfile = false
|
||||
repeat(PROFILE_READ_ATTEMPTS) { attempt ->
|
||||
val result = runCatching { userRepository.getUser(uid) }
|
||||
val result = runCatching { userRepository.getUserFromServer(uid) }
|
||||
if (result.isSuccess) {
|
||||
val user = result.getOrNull()
|
||||
val needsProfile = user == null || user.displayName.isBlank() || user.sex.isBlank()
|
||||
return if (needsProfile) "create_profile" else "home"
|
||||
if (user != null && !user.isSignInStub()) {
|
||||
val needsProfile = user.displayName.isBlank() || user.sex.isBlank()
|
||||
return if (needsProfile) "create_profile" else "home"
|
||||
}
|
||||
// Absent or still a stub: too early to tell a new user from an unsynced one.
|
||||
sawNoProfile = true
|
||||
} else {
|
||||
Log.w(
|
||||
TAG,
|
||||
"Could not load user profile during onboarding (attempt ${attempt + 1})",
|
||||
result.exceptionOrNull()
|
||||
)
|
||||
}
|
||||
Log.w(
|
||||
TAG,
|
||||
"Could not load user profile during onboarding (attempt ${attempt + 1})",
|
||||
result.exceptionOrNull()
|
||||
)
|
||||
if (attempt < PROFILE_READ_ATTEMPTS - 1) delay(RETRY_DELAY_MS * (attempt + 1))
|
||||
}
|
||||
// Never read it. They are signed in, so they are not a new user: send them somewhere safe
|
||||
// rather than inviting them to overwrite a profile we simply couldn't see.
|
||||
Log.w(TAG, "Profile unreadable after $PROFILE_READ_ATTEMPTS attempts; routing Home, not to profile setup")
|
||||
return "home"
|
||||
// Out of attempts. A document that stayed empty really is a new user; a read that never
|
||||
// succeeded at all tells us nothing, so don't invite them to overwrite a profile we couldn't see.
|
||||
return if (sawNoProfile) {
|
||||
"create_profile"
|
||||
} else {
|
||||
Log.w(TAG, "Profile unreadable after $PROFILE_READ_ATTEMPTS attempts; routing Home, not to profile setup")
|
||||
"home"
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* True for the field-less `users/{uid}` that FCM-token registration leaves behind at sign-in,
|
||||
* before the profile has synced. Every path that creates a real user writes `email` and
|
||||
* `createdAt` first, so their absence means "nothing has been written here yet" — never "this
|
||||
* person has no profile".
|
||||
*/
|
||||
private fun User.isSignInStub(): Boolean =
|
||||
email.isBlank() && createdAt == 0L && coupleId == null
|
||||
|
||||
fun onNavigated() = _uiState.update { it.copy(navigateTo = null) }
|
||||
|
||||
companion object {
|
||||
|
|
|
|||
|
|
@ -1478,6 +1478,13 @@ OOB code = `truncate6(SHA-256(pubkey ‖ nonce))`.
|
|||
**Fix (R15)**: `scripts/theme-scan.sh` is a mandatory Pass C pre-check that statically detects hardcoded colors, direct `painterResource`/`Image` usage outside `BrandIllustration`, mismatched container/surface colors, and missing `preview`s. Run it before any manual visual sweep; file findings in `ClaudeReport.md` and counts in `ClaudeQACoverage.md`.
|
||||
**Re-introduction risk / still-open**: any new art rendered via a **direct** `painterResource(R.drawable.illustration_*)` (NOT `BrandIllustration`) bypasses theme-correct loading and feathering. Always route opaque rounded-rect illustrations through `BrandIllustration`; keep `MaterialTheme.colorScheme` for surfaces; add Tier 1/2 previews for new screens. A hardcoded `CloserPalette.*` dark value or a raw `Color.White`/`Color.Black` will be flagged by `theme-scan.sh`.
|
||||
|
||||
### C-AUTH-001 - a `users/{uid}` read right after sign-in returns a field-less document
|
||||
**Symptom**: signing in on a *new device* with a fully set-up, paired account routed to `CREATE_PROFILE` ("What should your partner call you?", over a `🔒 Couldn't unlock on this device` value) instead of `RECOVERY`. Reproduced end to end on a throwaway emulator with a real paired account.
|
||||
**Cause**: for roughly the first 200ms after sign-in, `getUser(uid)` returns a document that **exists with no fields at all** - no `email`, no `displayName`, no `coupleId` - and the real document only lands afterwards. Sign-in registers the FCM token, and that `SetOptions.merge()` write materialises `users/{uid}` before the profile has synced. Measured on-device: two reads at t+0ms field-less, the real document at t+215ms. `Source.SERVER` does **not** dodge it, so "read from the server" is not the fix.
|
||||
**Why it was destructive**: `FirestoreUserDataSource.createUser` is a whole-document `set()`, not a merge, and `CreateProfileViewModel` submit calls it. Tapping through that mistakenly-shown screen overwrites the real `displayName` with the locked placeholder and drops `coupleId`/`partnerId`, unpairing the couple - and the user never reaches Recovery.
|
||||
**Fix (R30)**: `OnboardingViewModel.resolveDestination` treats a field-less document as "ask again", not as "new user": it retries (3x, 400ms backoff) until a document with real identity on it arrives, and only that decides. A read that never succeeds routes Home (non-destructive; Home routes to Recovery by itself when the couple key is missing). `UserRepository.getUserFromServer` (`Source.SERVER`) backs the decision so an offline read throws instead of guessing from cache.
|
||||
**Re-introduction risk**: **any** one-shot `users/{uid}` read on a startup path that branches on a field being blank. `exists() == true` does not mean "populated". Blank fields this early mean "not synced yet", and `createUser`'s `set()` semantics make guessing wrong expensive. If you need "does this person have a profile", require identity (`email`/`createdAt`) to be present before believing the answer.
|
||||
|
||||
---
|
||||
|
||||
## Where to look first
|
||||
|
|
|
|||
Loading…
Reference in New Issue