null
/
Closer
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(auth): stop isSignInStub from looping every new signup into profile setup 

Caught on-device before it shipped, by doing the thing I'd been putting off:
creating a real account and signing in again. My own isSignInStub — added two
commits ago to fix C-AUTH-001 — sent every brand-new email/password user straight
back into profile setup on their next sign-in, forever. Name filled in, profile
finished, still asked "What should your partner call you?" every launch.

It tested email.isBlank() && createdAt == 0L && coupleId == null, on the reasoning
that every path creating a real user writes email and createdAt first, so their
absence must mean nothing is written. Both halves are false:

  - an email/password signup never writes `email` at all — CreateProfileViewModel
    builds its User without one;
  - it only writes `createdAt` through createUser, which it calls only when it
    reads no document — a race it normally LOSES to FCM registration, so it takes
    the targeted-update branch (updateDisplayName/updateSex) and writes neither.

So a finished profile — displayName and sex set, nothing else — read as a stub
forever, and the retry loop I added then dutifully classified it as a new user.
The predicate now asks "is anything set at all?", which is what "nothing has been
written here yet" actually means. Any one field is enough to trust the document.

Two things I got wrong and am correcting rather than leaving:

  - the previous commit claimed isSignInStub was "now shared rather than private to
    OnboardingViewModel". It wasn't. The private copy stayed and shadowed the new
    shared one, so the domain-model version was dead code and the narrow local one
    was what actually ran — which is why the first attempt at this fix changed
    nothing on device. The private copy is gone; there is one definition now. The
    duplicated-logic trap I'd just written a commit message about, walked into.
  - the manual's C-AUTH-001 entry advised requiring email/createdAt before believing
    a read. That advice would reproduce this exact bug, so it now says the opposite,
    with the reason.

Verified live on the throwaway, both directions, because they fail in opposite ways
and fixing one blindly breaks the other: a real signup (account created, profile
completed, re-signed-in) now lands on Home; the paired fixture still lands on
Recovery. The disposable account was deleted through the app's own flow afterwards.

Tests: aFinishedProfileWithNoEmailOrCreatedAtIsNotAStub pins the regression;
anyOneIdentifyingFieldIsEnoughToTrustTheDocument pins the rule. Suite green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null 2026-07-15 01:25:25 -05:00
parent 9448537b6b
commit 0955276024
4 changed files with 62 additions and 26 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

26
app/src/main/java/app/closer/domain/model/User.kt
View File

@ -18,13 +18,27 @@ data class User(
)
/**
* True when nothing has been written to this person's document yet — as opposed to "this person has
* no profile", which looks identical field-by-field and means the opposite (C-AUTH-001).
* True when there is nothing identifying on this document at all — which is what a read returns for
* a moment after sign-in, and is NOT the same as "this person has no profile" even though the two
* look identical field by field (C-AUTH-001).
*
* Signing in registers an FCM token, and that merge-write materialises `users/{uid}` before the
* profile has synced: for roughly the first 200ms every read returns a document that exists and has
* no identity on it. Every path that creates a real user writes [email] and [createdAt] first, so
* their absence means "too early to tell", never "new user". Read it again rather than acting on it.
* profile has synced: for roughly the first 200ms every read comes back empty. Callers should read
* again rather than act on it.
*
* Deliberately checks *every* field a person could be recognised by, not just [email]/[createdAt].
* Those two are not the safety net they look like: an email/password signup never writes `email`,
* and it only writes `createdAt` when `CreateProfileViewModel` finds no document — which loses the
* race with FCM registration, so it usually takes the targeted-update branch and writes neither.
* Keying on them alone made a real, finished profile read as a stub and looped its owner back into
* profile setup on every sign-in. If any field is set, something has been written here: trust it.
*/
val User.isSignInStub: Boolean
get() = email.isBlank() && createdAt == 0L && coupleId == null
get() = email.isBlank() &&
displayName.isBlank() &&
sex.isBlank() &&
photoUrl.isBlank() &&
coupleId == null &&
partnerId == null &&
birthDate == null &&
createdAt == 0L

22
app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt
View File

@ -5,6 +5,7 @@ import androidx.lifecycle.ViewModel
import androidx.lifecycle.viewModelScope
import app.closer.domain.model.AuthState
import app.closer.domain.model.User
import app.closer.domain.model.isSignInStub
import app.closer.domain.repository.AuthRepository
import app.closer.domain.repository.UserRepository
import dagger.hilt.android.lifecycle.HiltViewModel
@ -63,11 +64,11 @@ class OnboardingViewModel @Inject constructor(
* 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.
* So the read has to be *trusted* before it is acted on: an empty document means "ask again", not
* "new user" ([User.isSignInStub]). Only a document with something on it decides. A genuinely new
* user is unaffected — theirs stays empty through every attempt, so they still fall through to
* profile setup, just ~1s later. If the read never succeeds at all we send them Home, which is
* non-destructive and routes to Recovery by itself when the couple key is missing.
*/
private suspend fun resolveDestination(uid: String): String {
var sawNoProfile = false
@ -75,7 +76,7 @@ class OnboardingViewModel @Inject constructor(
val result = runCatching { userRepository.getUserFromServer(uid) }
if (result.isSuccess) {
val user = result.getOrNull()
if (user != null && !user.isSignInStub()) {
if (user != null && !user.isSignInStub) {
val needsProfile = user.displayName.isBlank() || user.sex.isBlank()
return if (needsProfile) "create_profile" else "home"
}
@ -100,15 +101,6 @@ class OnboardingViewModel @Inject constructor(
}
}
/**
* 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 {

37
app/src/test/java/app/closer/data/remote/UserProfileWriteTest.kt
View File

@ -97,9 +97,38 @@ class UserProfileWriteTest {
fun theStubIsRecognisedAndARealUserIsNot() {
assertTrue("the post-sign-in document must read as a stub", signInStub.isSignInStub)
assertFalse("a paired user must never read as a stub", pairedUser.isSignInStub)
// A genuinely new account is NOT a stub: every create writes email + createdAt first, which
// is what keeps new users flowing to profile setup instead of being retried forever.
val brandNew = User(id = "u2", email = "new@example.com", createdAt = 1_700_000_000_000L)
assertFalse("a new account with identity on it is not a stub", brandNew.isSignInStub)
}
@Test
fun aFinishedProfileWithNoEmailOrCreatedAtIsNotAStub() {
// The regression this predicate caused, caught on-device before it shipped. An email/password
// signup never writes `email`, and only writes `createdAt` if CreateProfile finds no document
// — a race it normally loses to FCM registration, so it takes the targeted-update branch and
// writes neither. This is what a real, finished, unpaired profile looks like on disk:
val finishedSignup = User(
id = "u3",
displayName = "enc:v1:zed",
sex = "non-binary",
createdAt = 0L,
lastActiveAt = 0L
)
assertFalse(
"a profile with a name and sex has plainly been written to — looping its owner back " +
"into profile setup on every sign-in is the bug",
finishedSignup.isSignInStub
)
}
@Test
fun anyOneIdentifyingFieldIsEnoughToTrustTheDocument() {
// Whatever arrives first, "something is here" beats "nothing is here".
assertFalse(signInStub.copy(displayName = "enc:v1:x").isSignInStub)
assertFalse(signInStub.copy(sex = "female").isSignInStub)
assertFalse(signInStub.copy(email = "a@b.com").isSignInStub)
assertFalse(signInStub.copy(coupleId = "c1").isSignInStub)
assertFalse(signInStub.copy(partnerId = "p1").isSignInStub)
assertFalse(signInStub.copy(photoUrl = "https://x/y.jpg").isSignInStub)
assertFalse(signInStub.copy(birthDate = 1L).isSignInStub)
assertFalse(signInStub.copy(createdAt = 1_700_000_000_000L).isSignInStub)
}
}

3
docs/Engineering_Reference_Manual.md
View File

@ -1484,7 +1484,8 @@ OOB code = `truncate6(SHA-256(pubkey ‖ nonce))`.
**Why it was destructive**: the user never reaches Recovery, and the screen they get instead invites them to overwrite the profile it couldn't read. `CreateProfileViewModel` submit is only *partly* protected - it uses targeted merges when it sees an existing doc (and `updateDisplayName`/`updateSex` `require` the value isn't the locked placeholder), but it calls `createUser` when it reads `null`, and `createUser` was a whole-document `set()` of every field. Any caller holding a partially-loaded `User` therefore wrote `coupleId = null` over a paired user, dropping `coupleId`/`partnerId`. The Google path reached exactly that: `mergeGoogleProfile` decided "new account" from `runCatching { getUser(uid) }.getOrNull()`, which is `null` for a *failed read* as much as for a missing doc.
**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. `User.isSignInStub` is the shared predicate.
**Hardening (R30)**: the primitive can no longer do the damage. `createUser` merges, and `UserProfileWrite` omits every null/blank value, so a hollow `User` produces an empty map - a no-op - instead of an erasure; it also carries the placeholder `require`s, and deliberately never writes `plan` (client default `"free"` would downgrade a paying user; the server owns it, reads default it). `mergeGoogleProfile` was duplicated verbatim in `LoginViewModel` and `SignUpViewModel` - the same bug twice - and is now one `GoogleProfileMerger` that does nothing on a failed read and won't seed over a stub. Pinned by `UserProfileWriteTest` (mutation-checked: reverting the omit rule fails 3 of them).
**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.
**Do NOT use `email`/`createdAt` as the "is this document real" test.** The first version of `isSignInStub` did exactly that and it looked obviously right - every create writes them, so their absence must mean nothing is written. Both halves are false, and a full signup on a throwaway emulator caught it before it shipped: an email/password signup **never writes `email`** (`CreateProfileViewModel` builds its `User` without one), and it only writes `createdAt` via `createUser`, which it calls **only when it reads no document** - a race it normally *loses* to FCM registration, so it takes the targeted-update branch and writes neither. The result was a finished profile that read as a stub forever: every new signup looped back into profile setup on every sign-in. `isSignInStub` therefore checks that **every** identifying field is empty; if anything is set, something has been written - trust it.
**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", and blank fields this early mean "not synced yet". Prefer "is *anything* set?" over naming particular fields, and check the answer against both a paired account and a fresh signup - the two failure directions are opposite, so fixing one blindly breaks the other.
---