diff --git a/app/src/main/java/app/closer/domain/model/User.kt b/app/src/main/java/app/closer/domain/model/User.kt index c4eef82a..0fcdd012 100644 --- a/app/src/main/java/app/closer/domain/model/User.kt +++ b/app/src/main/java/app/closer/domain/model/User.kt @@ -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 diff --git a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt index a33f62f3..5fde0d84 100644 --- a/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt +++ b/app/src/main/java/app/closer/ui/onboarding/OnboardingViewModel.kt @@ -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 { diff --git a/app/src/test/java/app/closer/data/remote/UserProfileWriteTest.kt b/app/src/test/java/app/closer/data/remote/UserProfileWriteTest.kt index d6346f16..0fd885d3 100644 --- a/app/src/test/java/app/closer/data/remote/UserProfileWriteTest.kt +++ b/app/src/test/java/app/closer/data/remote/UserProfileWriteTest.kt @@ -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) } } diff --git a/docs/Engineering_Reference_Manual.md b/docs/Engineering_Reference_Manual.md index 0960edc4..7748568b 100644 --- a/docs/Engineering_Reference_Manual.md +++ b/docs/Engineering_Reference_Manual.md @@ -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. ---