fix(auth): make a profile write unable to unpair a couple (C-AUTH-001 hardening)
The previous commit fixed the path that reached the hazard. This removes the
hazard: createUser was a whole-document set() of every field, so any caller
holding a partially-loaded User wrote coupleId = null over a paired user and
dropped coupleId/partnerId. The User that does this is not exotic — for ~200ms
after every sign-in, users/{uid} exists with no fields on it at all. Getting a
read slightly wrong should cost a no-op, not a relationship's history.
createUser now merges, and UserProfileWrite omits every null or blank value, so a
field is only ever written with a real value and a hollow User produces an empty
map. For a document that doesn't exist yet — the only case callers actually intend
— the result is byte-identical to before. Clearing a field stays the job of the
targeted update* methods, where the call site says so. It also carries the same
locked-placeholder require()s the targeted writers already had.
plan is deliberately no longer written: User.plan defaults to "free", so merging it
would silently downgrade a paying user whose document we read while it was hollow.
The server owns that field (RevenueCat), reads already default a missing plan to
"free", and no gate reads it — entitlements live in the server-only subdoc. Checked
against firestore.rules: create requires no fields, and update's hasOnly allowlist
still covers everything written.
Correcting my last commit message while I'm here: it claimed CreateProfile submit
calls createUser and overwrites the name. Only half true — it uses targeted merges
when it sees an existing doc, and only calls createUser when it reads null. The
path that actually reached the sharp edge was Google sign-in: mergeGoogleProfile
decided "new account" from runCatching { getUser(uid) }.getOrNull(), which is null
for a failed read as much as for a missing document — C-AUTH-001's conflation, in a
place that then wrote a whole document. It also existed as two verbatim copies, in
LoginViewModel and SignUpViewModel, so the same bug was there twice. Now one
GoogleProfileMerger: it does nothing on a failed read, and won't seed a name over a
stub (which would overwrite a returning user's real name with their Google one).
Both view models drop userRepository entirely as a result.
User.isSignInStub is now shared rather than private to OnboardingViewModel, since
two callers need the same question answered.
UserProfileWriteTest pins the rule; mutation-checked by reverting the omit rule,
which fails 3 of them. Full unit suite green. Sign-in verified live on the throwaway.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
null
parent
6330bf98ca
commit
9448537b6b
|
|
@ -95,21 +95,35 @@ class FirestoreUserDataSource @Inject constructor(
|
|||
awaitClose { listener.remove() }
|
||||
}
|
||||
|
||||
/**
|
||||
* Write a user's profile without ever erasing what is already there.
|
||||
*
|
||||
* This was a whole-document `set()` of every field, which made it destructive by construction:
|
||||
* any caller holding a partially-loaded [User] — or the field-less document that exists for
|
||||
* ~200ms after sign-in (C-AUTH-001) — would write `coupleId = null` over a paired user, dropping
|
||||
* `coupleId`/`partnerId` and unpairing the couple, plus overwriting their name. That is far too
|
||||
* sharp an edge for a function every auth path calls: getting a read slightly wrong should cost
|
||||
* a no-op, not a relationship's history.
|
||||
*
|
||||
* So it merges, and [UserProfileWrite] omits anything null or blank — a field is only ever
|
||||
* written with a real value. For a document that doesn't exist yet (the only case callers
|
||||
* actually intend) the result is identical to before. Clearing a field stays the job of the
|
||||
* targeted `update*` methods, which say so at the call site.
|
||||
*/
|
||||
suspend fun createUser(user: User) {
|
||||
userRef(user.id).set(
|
||||
mapOf(
|
||||
"email" to user.email,
|
||||
"displayName" to encryptProfileField(user.displayName, user.coupleId),
|
||||
"photoUrl" to user.photoUrl,
|
||||
"sex" to encryptProfileField(user.sex, user.coupleId),
|
||||
"partnerId" to user.partnerId,
|
||||
"coupleId" to user.coupleId,
|
||||
"plan" to user.plan,
|
||||
"birthDate" to user.birthDate,
|
||||
"createdAt" to user.createdAt,
|
||||
"lastActiveAt" to user.lastActiveAt
|
||||
)
|
||||
).await()
|
||||
// Same guard the targeted writers carry: never persist the "locked" UI string as a value.
|
||||
require(user.displayName != FieldEncryptor.LOCKED_PLACEHOLDER) {
|
||||
"Refusing to persist the locked placeholder as displayName"
|
||||
}
|
||||
require(user.sex != FieldEncryptor.LOCKED_PLACEHOLDER) {
|
||||
"Refusing to persist the locked placeholder as sex"
|
||||
}
|
||||
val fields = UserProfileWrite.fields(
|
||||
user,
|
||||
displayName = encryptProfileField(user.displayName, user.coupleId),
|
||||
sex = encryptProfileField(user.sex, user.coupleId)
|
||||
)
|
||||
userRef(user.id).set(fields, SetOptions.merge()).await()
|
||||
}
|
||||
|
||||
/** Age-gate DOB (O-AGE-001). Set once when a Google/legacy user has no birthDate yet. */
|
||||
|
|
|
|||
|
|
@ -0,0 +1,49 @@
|
|||
package app.closer.data.remote
|
||||
|
||||
import app.closer.domain.model.User
|
||||
|
||||
/**
|
||||
* The field map [FirestoreUserDataSource.createUser] writes.
|
||||
*
|
||||
* Split out from the data source so the one rule that matters can be pinned by a JVM test, with no
|
||||
* Firestore in the way: **a null or blank value is never written.** The map is merged into a document
|
||||
* that may already hold a real, paired profile, so anything hollow in the [User] has to be left alone
|
||||
* rather than written as an erasure.
|
||||
*
|
||||
* This exists because the write used to be a whole-document `set()`, which put every caller one bad
|
||||
* read away from unpairing a couple: a partially-loaded `User` — or the field-less document that
|
||||
* exists for ~200ms after sign-in (C-AUTH-001) — carries `coupleId = null`, and writing that over a
|
||||
* paired user drops `coupleId`/`partnerId` and overwrites their name. Clearing a field is the job of
|
||||
* the targeted `update*` methods, where the call site says so out loud.
|
||||
*/
|
||||
internal object UserProfileWrite {
|
||||
|
||||
/**
|
||||
* [displayName] and [sex] are passed in already encrypted — the data source owns the couple key.
|
||||
*
|
||||
* `plan` is deliberately absent: it is a billing fact the server owns (RevenueCat writes it), and
|
||||
* [User.plan] defaults to `"free"`, so merging it would silently downgrade a paying user whose
|
||||
* document we read while it was still hollow. Reads already treat a missing `plan` as `"free"`,
|
||||
* so not writing it is the same thing for a new user and harmless for everyone else.
|
||||
*/
|
||||
fun fields(user: User, displayName: String, sex: String): Map<String, Any> = buildMap {
|
||||
putIfReal("email", user.email)
|
||||
putIfReal("displayName", displayName)
|
||||
putIfReal("photoUrl", user.photoUrl)
|
||||
putIfReal("sex", sex)
|
||||
putIfReal("partnerId", user.partnerId)
|
||||
putIfReal("coupleId", user.coupleId)
|
||||
putIfReal("birthDate", user.birthDate)
|
||||
// Epoch millis: 0 is "unset" (the hollow document), never a real timestamp.
|
||||
putIfReal("createdAt", user.createdAt.takeIf { it > 0L })
|
||||
putIfReal("lastActiveAt", user.lastActiveAt.takeIf { it > 0L })
|
||||
}
|
||||
|
||||
private fun MutableMap<String, Any>.putIfReal(key: String, value: Any?) {
|
||||
when (value) {
|
||||
null -> Unit
|
||||
is String -> if (value.isNotBlank()) put(key, value)
|
||||
else -> put(key, value)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -16,3 +16,15 @@ data class User(
|
|||
val createdAt: Long = System.currentTimeMillis(),
|
||||
val lastActiveAt: Long = System.currentTimeMillis()
|
||||
)
|
||||
|
||||
/**
|
||||
* 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).
|
||||
*
|
||||
* 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.
|
||||
*/
|
||||
val User.isSignInStub: Boolean
|
||||
get() = email.isBlank() && createdAt == 0L && coupleId == null
|
||||
|
|
|
|||
|
|
@ -0,0 +1,73 @@
|
|||
package app.closer.domain.usecase
|
||||
|
||||
import android.util.Log
|
||||
import app.closer.domain.model.GoogleSignInResult
|
||||
import app.closer.domain.model.User
|
||||
import app.closer.domain.model.isSignInStub
|
||||
import app.closer.domain.repository.UserRepository
|
||||
import javax.inject.Inject
|
||||
import javax.inject.Singleton
|
||||
|
||||
/**
|
||||
* Seeds a user's profile from what Google already knows (name, photo) at sign-in.
|
||||
*
|
||||
* Best-effort by design: it fills gaps for a new account and otherwise stays out of the way. It ran
|
||||
* as identical copies inside both `LoginViewModel` and `SignUpViewModel`, which is how the same bug
|
||||
* came to exist twice.
|
||||
*
|
||||
* The bug: it decided "this is a new account" from `runCatching { getUser(uid) }.getOrNull()`, which
|
||||
* returns null both when the user genuinely has no document *and* when the read simply failed. On the
|
||||
* second reading it created the profile from scratch, overwriting a real one. That is the same
|
||||
* conflation as C-AUTH-001, and the same rule applies — a read you don't trust decides nothing.
|
||||
*/
|
||||
@Singleton
|
||||
class GoogleProfileMerger @Inject constructor(
|
||||
private val userRepository: UserRepository
|
||||
) {
|
||||
suspend fun merge(result: GoogleSignInResult) {
|
||||
val uid = result.uid
|
||||
if (uid.isBlank()) return
|
||||
|
||||
val read = runCatching { userRepository.getUser(uid) }
|
||||
if (read.isFailure) {
|
||||
// Can't tell a new account from an unreadable one. Writing either way risks overwriting a
|
||||
// real profile, and the only thing at stake here is a pre-filled name — so do nothing.
|
||||
Log.w(TAG, "Couldn't read the profile at Google sign-in; leaving it untouched", read.exceptionOrNull())
|
||||
return
|
||||
}
|
||||
val existing = read.getOrNull()
|
||||
|
||||
if (existing == null) {
|
||||
userRepository.createUser(
|
||||
User(
|
||||
id = uid,
|
||||
email = result.email,
|
||||
displayName = result.displayName,
|
||||
photoUrl = result.photoUrl,
|
||||
createdAt = System.currentTimeMillis(),
|
||||
lastActiveAt = System.currentTimeMillis()
|
||||
)
|
||||
)
|
||||
return
|
||||
}
|
||||
|
||||
// A document with nothing written to it yet looks exactly like a brand-new account: every
|
||||
// field blank. Filling it in from Google would overwrite the real name of a returning user
|
||||
// whose profile simply hadn't synced yet. Their name is already set — leave it alone.
|
||||
if (existing.isSignInStub) {
|
||||
Log.w(TAG, "Profile not synced yet at Google sign-in; not seeding name/photo over it")
|
||||
return
|
||||
}
|
||||
|
||||
if (existing.displayName.isBlank() && result.displayName.isNotBlank()) {
|
||||
userRepository.updateDisplayName(uid, result.displayName)
|
||||
}
|
||||
if (existing.photoUrl.isBlank() && result.photoUrl.isNotBlank()) {
|
||||
userRepository.updatePhotoUrl(uid, result.photoUrl)
|
||||
}
|
||||
}
|
||||
|
||||
private companion object {
|
||||
const val TAG = "GoogleProfileMerger"
|
||||
}
|
||||
}
|
||||
|
|
@ -2,10 +2,8 @@ package app.closer.ui.auth
|
|||
|
||||
import androidx.lifecycle.ViewModel
|
||||
import androidx.lifecycle.viewModelScope
|
||||
import app.closer.domain.model.GoogleSignInResult
|
||||
import app.closer.domain.model.User
|
||||
import app.closer.domain.repository.AuthRepository
|
||||
import app.closer.domain.repository.UserRepository
|
||||
import app.closer.domain.usecase.GoogleProfileMerger
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
import kotlinx.coroutines.flow.StateFlow
|
||||
|
|
@ -26,7 +24,7 @@ data class LoginUiState(
|
|||
@HiltViewModel
|
||||
class LoginViewModel @Inject constructor(
|
||||
private val authRepository: AuthRepository,
|
||||
private val userRepository: UserRepository
|
||||
private val googleProfileMerger: GoogleProfileMerger
|
||||
) : ViewModel() {
|
||||
|
||||
private val _uiState = MutableStateFlow(LoginUiState())
|
||||
|
|
@ -51,7 +49,7 @@ class LoginViewModel @Inject constructor(
|
|||
viewModelScope.launch {
|
||||
authRepository.signInWithGoogle(idToken)
|
||||
.onSuccess { result ->
|
||||
mergeGoogleProfile(result)
|
||||
googleProfileMerger.merge(result)
|
||||
_uiState.update { it.copy(isLoading = false, success = true) }
|
||||
}
|
||||
.onFailure { e -> _uiState.update { it.copy(isLoading = false, error = friendlyError(e)) } }
|
||||
|
|
@ -69,30 +67,6 @@ class LoginViewModel @Inject constructor(
|
|||
}
|
||||
}
|
||||
|
||||
private suspend fun mergeGoogleProfile(result: GoogleSignInResult) {
|
||||
val uid = result.uid
|
||||
if (uid.isBlank()) return
|
||||
val existing = runCatching { userRepository.getUser(uid) }.getOrNull()
|
||||
if (existing == null) {
|
||||
userRepository.createUser(
|
||||
User(
|
||||
id = uid,
|
||||
email = result.email,
|
||||
displayName = result.displayName,
|
||||
photoUrl = result.photoUrl,
|
||||
createdAt = System.currentTimeMillis(),
|
||||
lastActiveAt = System.currentTimeMillis()
|
||||
)
|
||||
)
|
||||
} else {
|
||||
if (existing.displayName.isBlank() && result.displayName.isNotBlank()) {
|
||||
userRepository.updateDisplayName(uid, result.displayName)
|
||||
}
|
||||
if (existing.photoUrl.isBlank() && result.photoUrl.isNotBlank()) {
|
||||
userRepository.updatePhotoUrl(uid, result.photoUrl)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private fun friendlyError(e: Throwable): String = when {
|
||||
e.message?.contains("no user record") == true -> "No account found with that email."
|
||||
|
|
|
|||
|
|
@ -5,10 +5,8 @@ import androidx.lifecycle.viewModelScope
|
|||
import app.closer.domain.AgeGate
|
||||
import app.closer.domain.SignupHandoff
|
||||
import app.closer.ui.brand.CloserCopy
|
||||
import app.closer.domain.model.GoogleSignInResult
|
||||
import app.closer.domain.model.User
|
||||
import app.closer.domain.repository.AuthRepository
|
||||
import app.closer.domain.repository.UserRepository
|
||||
import app.closer.domain.usecase.GoogleProfileMerger
|
||||
import dagger.hilt.android.lifecycle.HiltViewModel
|
||||
import kotlinx.coroutines.flow.MutableStateFlow
|
||||
import kotlinx.coroutines.flow.StateFlow
|
||||
|
|
@ -35,7 +33,7 @@ data class SignUpUiState(
|
|||
@HiltViewModel
|
||||
class SignUpViewModel @Inject constructor(
|
||||
private val authRepository: AuthRepository,
|
||||
private val userRepository: UserRepository,
|
||||
private val googleProfileMerger: GoogleProfileMerger,
|
||||
private val signupHandoff: SignupHandoff
|
||||
) : ViewModel() {
|
||||
|
||||
|
|
@ -89,7 +87,7 @@ class SignUpViewModel @Inject constructor(
|
|||
viewModelScope.launch {
|
||||
authRepository.signInWithGoogle(idToken)
|
||||
.onSuccess { result ->
|
||||
mergeGoogleProfile(result)
|
||||
googleProfileMerger.merge(result)
|
||||
_uiState.update { it.copy(isLoading = false, googleSuccess = true) }
|
||||
}
|
||||
.onFailure { e -> _uiState.update { it.copy(isLoading = false, error = friendlyError(e)) } }
|
||||
|
|
@ -98,30 +96,6 @@ class SignUpViewModel @Inject constructor(
|
|||
|
||||
fun reportError(message: String) = _uiState.update { it.copy(error = message) }
|
||||
|
||||
private suspend fun mergeGoogleProfile(result: GoogleSignInResult) {
|
||||
val uid = result.uid
|
||||
if (uid.isBlank()) return
|
||||
val existing = runCatching { userRepository.getUser(uid) }.getOrNull()
|
||||
if (existing == null) {
|
||||
userRepository.createUser(
|
||||
User(
|
||||
id = uid,
|
||||
email = result.email,
|
||||
displayName = result.displayName,
|
||||
photoUrl = result.photoUrl,
|
||||
createdAt = System.currentTimeMillis(),
|
||||
lastActiveAt = System.currentTimeMillis()
|
||||
)
|
||||
)
|
||||
} else {
|
||||
if (existing.displayName.isBlank() && result.displayName.isNotBlank()) {
|
||||
userRepository.updateDisplayName(uid, result.displayName)
|
||||
}
|
||||
if (existing.photoUrl.isBlank() && result.photoUrl.isNotBlank()) {
|
||||
userRepository.updatePhotoUrl(uid, result.photoUrl)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
private fun friendlyError(e: Throwable): String = when {
|
||||
e.message?.contains("email address is already") == true -> "An account with this email already exists."
|
||||
|
|
|
|||
|
|
@ -0,0 +1,105 @@
|
|||
package app.closer.data.remote
|
||||
|
||||
import app.closer.domain.model.User
|
||||
import app.closer.domain.model.isSignInStub
|
||||
import org.junit.Assert.assertEquals
|
||||
import org.junit.Assert.assertFalse
|
||||
import org.junit.Assert.assertTrue
|
||||
import org.junit.Test
|
||||
|
||||
/**
|
||||
* These are about one thing: a profile write must never be able to erase a couple.
|
||||
*
|
||||
* `createUser` used to `set()` the whole document, so a caller holding a partially-loaded [User]
|
||||
* would write `coupleId = null` over a paired user and unpair them (C-AUTH-001). The read that
|
||||
* produces such a User is not exotic — for ~200ms after every sign-in, `users/{uid}` exists with no
|
||||
* fields on it at all — so the write itself has to be incapable of the damage.
|
||||
*/
|
||||
class UserProfileWriteTest {
|
||||
|
||||
/** Exactly what a read returns in the first ~200ms after sign-in. */
|
||||
private val signInStub = User(id = "u1", createdAt = 0L, lastActiveAt = 0L)
|
||||
|
||||
private val pairedUser = User(
|
||||
id = "u1",
|
||||
email = "ava@example.com",
|
||||
displayName = "enc:v1:name",
|
||||
photoUrl = "https://example.com/a.jpg",
|
||||
sex = "enc:v1:sex",
|
||||
partnerId = "ben",
|
||||
coupleId = "couple1",
|
||||
birthDate = 946_684_800_000L,
|
||||
createdAt = 1_700_000_000_000L,
|
||||
lastActiveAt = 1_700_000_001_000L
|
||||
)
|
||||
|
||||
private fun fieldsOf(user: User) =
|
||||
UserProfileWrite.fields(user, displayName = user.displayName, sex = user.sex)
|
||||
|
||||
@Test
|
||||
fun aHollowUserCannotUnpairACouple() {
|
||||
val fields = fieldsOf(signInStub)
|
||||
// The whole bug in one assertion: merging this must not touch the pairing.
|
||||
assertFalse("coupleId must not be written from a hollow user", fields.containsKey("coupleId"))
|
||||
assertFalse("partnerId must not be written from a hollow user", fields.containsKey("partnerId"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aHollowUserWritesNothingAtAll() {
|
||||
// Not just the pairing: there is nothing real on this document, so nothing should be said
|
||||
// about it. A merge of an empty map is a no-op, which is the correct outcome.
|
||||
assertEquals(emptyMap<String, Any>(), fieldsOf(signInStub))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun aRealUserStillWritesEveryField() {
|
||||
// The non-destructive rule must not cost a genuine create anything.
|
||||
val fields = fieldsOf(pairedUser)
|
||||
assertEquals("ava@example.com", fields["email"])
|
||||
assertEquals("enc:v1:name", fields["displayName"])
|
||||
assertEquals("https://example.com/a.jpg", fields["photoUrl"])
|
||||
assertEquals("enc:v1:sex", fields["sex"])
|
||||
assertEquals("ben", fields["partnerId"])
|
||||
assertEquals("couple1", fields["coupleId"])
|
||||
assertEquals(946_684_800_000L, fields["birthDate"])
|
||||
assertEquals(1_700_000_000_000L, fields["createdAt"])
|
||||
assertEquals(1_700_000_001_000L, fields["lastActiveAt"])
|
||||
}
|
||||
|
||||
@Test
|
||||
fun blanksAreOmittedRatherThanWrittenAsErasures() {
|
||||
val partial = pairedUser.copy(photoUrl = "", sex = "", birthDate = null)
|
||||
val fields = fieldsOf(partial)
|
||||
assertFalse("a blank photoUrl must not clear an existing one", fields.containsKey("photoUrl"))
|
||||
assertFalse("a blank sex must not clear an existing one", fields.containsKey("sex"))
|
||||
assertFalse("a null birthDate must not clear an existing one", fields.containsKey("birthDate"))
|
||||
// ...while everything real about the same user is still written.
|
||||
assertEquals("couple1", fields["coupleId"])
|
||||
}
|
||||
|
||||
@Test
|
||||
fun planIsNeverWritten() {
|
||||
// User.plan defaults to "free", so writing it would downgrade a paying user whose document
|
||||
// we happened to read while it was hollow. The server owns this field.
|
||||
assertFalse(fieldsOf(pairedUser).containsKey("plan"))
|
||||
assertFalse(fieldsOf(pairedUser.copy(plan = "premium")).containsKey("plan"))
|
||||
}
|
||||
|
||||
@Test
|
||||
fun unsetTimestampsAreNotWrittenAsZero() {
|
||||
// 0 is "unset", and writing it would date a real account to 1970.
|
||||
val fields = fieldsOf(pairedUser.copy(createdAt = 0L, lastActiveAt = 0L))
|
||||
assertFalse(fields.containsKey("createdAt"))
|
||||
assertFalse(fields.containsKey("lastActiveAt"))
|
||||
}
|
||||
|
||||
@Test
|
||||
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)
|
||||
}
|
||||
}
|
||||
|
|
@ -1481,8 +1481,9 @@ OOB code = `truncate6(SHA-256(pubkey ‖ nonce))`.
|
|||
### 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.
|
||||
**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.
|
||||
|
||||
---
|
||||
|
|
|
|||
Loading…
Reference in New Issue