fix(auth): GoogleProfileMerger must not be able to kill a successful sign-in
Self-review of the last four commits against the codebase's own error-handling conventions. Two real defects found, both in GoogleProfileMerger, both mine: 1. Its writes could crash sign-in. The class doc said "best-effort by design", but createUser/updateDisplayName/updatePhotoUrl were unwrapped, and merge() runs inside the view model's onSuccess block in viewModelScope — a throw there is an unhandled coroutine exception, after authentication has already succeeded. The words claimed a contract the code didn't deliver. merge() now never throws: the seed runs under runCatching and failures are recorded. (The pre-rework copies had the same unwrapped createUser, so this is older than my refactor — but I rewrote the class and kept the hole, and the doc claiming best-effort made it worse than inherited.) 2. It was the only file in domain/ using android.util.Log. The house pattern for use cases is an injected CrashReporter (DailyQuestionResolver is the model): recordException for failures, log() for breadcrumbs. Swapped over; a swallowed failure now actually surfaces in Crashlytics instead of dying in logcat on a device we'll never see. Also aligned: the two Kotlin assert() calls in RecoveryPhraseNormalizationTest were the only ones in the suite and silently depend on the JVM -ea flag — now assertNotEquals like everything else; RecoveryScreen's OutlinedButton is imported rather than fully qualified (the file's lone qualified call was the anomaly, not the rule). GoogleProfileMergerTest pins the contract: a failed read writes nothing and is recorded (C-AUTH-001's conflation), a failed write is recorded not thrown, a stub is not seeded over, a complete profile is untouched, and only blank fields are filled. Mutation-checked by removing the runCatching — and that check also caught a bug in the test itself: the "complete profile" fixture had defaulted photoUrl to "", so the merger legitimately tried to fill it and the strict mock's throw was swallowed by the very runCatching under test — a test passing for the wrong reason. Fixture fixed; the mutation now kills exactly the one test that asserts the contract. Hilt graph validated via assembleDebug; full unit suite green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
parent
0955276024
commit
18e6cf7b30
|
|
@ -1,6 +1,6 @@
|
||||||
package app.closer.domain.usecase
|
package app.closer.domain.usecase
|
||||||
|
|
||||||
import android.util.Log
|
import app.closer.core.crash.CrashReporter
|
||||||
import app.closer.domain.model.GoogleSignInResult
|
import app.closer.domain.model.GoogleSignInResult
|
||||||
import app.closer.domain.model.User
|
import app.closer.domain.model.User
|
||||||
import app.closer.domain.model.isSignInStub
|
import app.closer.domain.model.isSignInStub
|
||||||
|
|
@ -11,9 +11,13 @@ import javax.inject.Singleton
|
||||||
/**
|
/**
|
||||||
* Seeds a user's profile from what Google already knows (name, photo) at sign-in.
|
* 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
|
* Best-effort by design, and [merge] never throws: by the time it runs, authentication has already
|
||||||
* as identical copies inside both `LoginViewModel` and `SignUpViewModel`, which is how the same bug
|
* succeeded, so a cosmetic seed failing must cost nothing — the caller sets its success state on the
|
||||||
* came to exist twice.
|
* next line, and an escaping exception here would kill a sign-in that actually worked. Failures are
|
||||||
|
* recorded to [CrashReporter] instead.
|
||||||
|
*
|
||||||
|
* 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
|
* 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
|
* returns null both when the user genuinely has no document *and* when the read simply failed. On the
|
||||||
|
|
@ -22,9 +26,15 @@ import javax.inject.Singleton
|
||||||
*/
|
*/
|
||||||
@Singleton
|
@Singleton
|
||||||
class GoogleProfileMerger @Inject constructor(
|
class GoogleProfileMerger @Inject constructor(
|
||||||
private val userRepository: UserRepository
|
private val userRepository: UserRepository,
|
||||||
|
private val crashReporter: CrashReporter
|
||||||
) {
|
) {
|
||||||
suspend fun merge(result: GoogleSignInResult) {
|
suspend fun merge(result: GoogleSignInResult) {
|
||||||
|
runCatching { seedProfile(result) }
|
||||||
|
.onFailure { crashReporter.recordException(it) }
|
||||||
|
}
|
||||||
|
|
||||||
|
private suspend fun seedProfile(result: GoogleSignInResult) {
|
||||||
val uid = result.uid
|
val uid = result.uid
|
||||||
if (uid.isBlank()) return
|
if (uid.isBlank()) return
|
||||||
|
|
||||||
|
|
@ -32,7 +42,7 @@ class GoogleProfileMerger @Inject constructor(
|
||||||
if (read.isFailure) {
|
if (read.isFailure) {
|
||||||
// Can't tell a new account from an unreadable one. Writing either way risks overwriting a
|
// 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.
|
// 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())
|
read.exceptionOrNull()?.let { crashReporter.recordException(it) }
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
val existing = read.getOrNull()
|
val existing = read.getOrNull()
|
||||||
|
|
@ -55,7 +65,7 @@ class GoogleProfileMerger @Inject constructor(
|
||||||
// field blank. Filling it in from Google would overwrite the real name of a returning user
|
// 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.
|
// whose profile simply hadn't synced yet. Their name is already set — leave it alone.
|
||||||
if (existing.isSignInStub) {
|
if (existing.isSignInStub) {
|
||||||
Log.w(TAG, "Profile not synced yet at Google sign-in; not seeding name/photo over it")
|
crashReporter.log("GoogleProfileMerger: profile not synced yet at sign-in; not seeding over it")
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|
@ -66,8 +76,4 @@ class GoogleProfileMerger @Inject constructor(
|
||||||
userRepository.updatePhotoUrl(uid, result.photoUrl)
|
userRepository.updatePhotoUrl(uid, result.photoUrl)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
private companion object {
|
|
||||||
const val TAG = "GoogleProfileMerger"
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -20,6 +20,7 @@ import androidx.compose.material3.Button
|
||||||
import androidx.compose.material3.ButtonDefaults
|
import androidx.compose.material3.ButtonDefaults
|
||||||
import app.closer.ui.components.CloserHeartLoader
|
import app.closer.ui.components.CloserHeartLoader
|
||||||
import androidx.compose.material3.MaterialTheme
|
import androidx.compose.material3.MaterialTheme
|
||||||
|
import androidx.compose.material3.OutlinedButton
|
||||||
import androidx.compose.material3.OutlinedTextField
|
import androidx.compose.material3.OutlinedTextField
|
||||||
import androidx.compose.material3.OutlinedTextFieldDefaults
|
import androidx.compose.material3.OutlinedTextFieldDefaults
|
||||||
import androidx.compose.material3.Scaffold
|
import androidx.compose.material3.Scaffold
|
||||||
|
|
@ -162,7 +163,7 @@ fun RecoveryScreen(
|
||||||
// It stays *below* the field rather than replacing it: someone who does have the phrase
|
// It stays *below* the field rather than replacing it: someone who does have the phrase
|
||||||
// (pasted from their partner's message) is unlocked instantly and offline, whereas this
|
// (pasted from their partner's message) is unlocked instantly and offline, whereas this
|
||||||
// path has to wait on the partner tapping consent.
|
// path has to wait on the partner tapping consent.
|
||||||
androidx.compose.material3.OutlinedButton(
|
OutlinedButton(
|
||||||
onClick = onPartnerRestore,
|
onClick = onPartnerRestore,
|
||||||
enabled = !state.isLoading,
|
enabled = !state.isLoading,
|
||||||
modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp),
|
modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp),
|
||||||
|
|
|
||||||
|
|
@ -1,6 +1,7 @@
|
||||||
package app.closer.crypto
|
package app.closer.crypto
|
||||||
|
|
||||||
import org.junit.Assert.assertEquals
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Assert.assertNotEquals
|
||||||
import org.junit.Test
|
import org.junit.Test
|
||||||
|
|
||||||
/**
|
/**
|
||||||
|
|
@ -52,8 +53,8 @@ class RecoveryPhraseNormalizationTest {
|
||||||
@Test
|
@Test
|
||||||
fun aGenuinelyWrongPhraseStillDiffers() {
|
fun aGenuinelyWrongPhraseStillDiffers() {
|
||||||
// Normalisation must not make wrong phrases pass: only the *presentation* is folded.
|
// Normalisation must not make wrong phrases pass: only the *presentation* is folded.
|
||||||
assert(norm("hunt down gear east lead over drop live more cat") != canonical)
|
assertNotEquals(canonical, norm("hunt down gear east lead over drop live more cat"))
|
||||||
assert(norm("hunt down gear east lead over drop live more") != canonical)
|
assertNotEquals(canonical, norm("hunt down gear east lead over drop live more"))
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
|
|
|
||||||
|
|
@ -0,0 +1,130 @@
|
||||||
|
package app.closer.domain.usecase
|
||||||
|
|
||||||
|
import app.closer.core.crash.CrashReporter
|
||||||
|
import app.closer.domain.model.GoogleSignInResult
|
||||||
|
import app.closer.domain.model.User
|
||||||
|
import app.closer.domain.repository.UserRepository
|
||||||
|
import io.mockk.Runs
|
||||||
|
import io.mockk.coEvery
|
||||||
|
import io.mockk.coVerify
|
||||||
|
import io.mockk.every
|
||||||
|
import io.mockk.just
|
||||||
|
import io.mockk.mockk
|
||||||
|
import io.mockk.slot
|
||||||
|
import kotlinx.coroutines.test.runTest
|
||||||
|
import org.junit.Assert.assertEquals
|
||||||
|
import org.junit.Test
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The merger runs right after a *successful* Google sign-in, from inside the view model's
|
||||||
|
* `onSuccess` block — so its one hard contract is that it never throws: an escaping exception there
|
||||||
|
* is an unhandled coroutine exception that kills a sign-in that actually worked. Everything else it
|
||||||
|
* does is optional polish; failures go to [CrashReporter] and the sign-in proceeds.
|
||||||
|
*
|
||||||
|
* The write-safety half (a failed read must not be mistaken for "new account" — C-AUTH-001's
|
||||||
|
* conflation) is asserted here too, since this class is where that decision now lives.
|
||||||
|
*/
|
||||||
|
class GoogleProfileMergerTest {
|
||||||
|
|
||||||
|
private val userRepository: UserRepository = mockk()
|
||||||
|
private val crashReporter: CrashReporter = mockk(relaxUnitFun = true)
|
||||||
|
private val merger = GoogleProfileMerger(userRepository, crashReporter)
|
||||||
|
|
||||||
|
private val googleResult = GoogleSignInResult(
|
||||||
|
uid = "u1",
|
||||||
|
email = "ava@example.com",
|
||||||
|
displayName = "Ava",
|
||||||
|
photoUrl = "https://example.com/a.jpg"
|
||||||
|
)
|
||||||
|
|
||||||
|
// Genuinely complete — including the photo. Leaving photoUrl at its "" default here would make
|
||||||
|
// the merger legitimately try to fill it, and the strict mock's throw would then be swallowed by
|
||||||
|
// the very runCatching under test: a test that passes for the wrong reason. The mutation check
|
||||||
|
// caught exactly that.
|
||||||
|
private val pairedUser = User(
|
||||||
|
id = "u1",
|
||||||
|
email = "ava@example.com",
|
||||||
|
displayName = "enc:v1:name",
|
||||||
|
photoUrl = "https://example.com/existing.jpg",
|
||||||
|
sex = "enc:v1:sex",
|
||||||
|
coupleId = "couple1",
|
||||||
|
partnerId = "ben",
|
||||||
|
createdAt = 1_700_000_000_000L
|
||||||
|
)
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aFailedReadWritesNothingAndIsRecordedNotThrown() = runTest {
|
||||||
|
// The C-AUTH-001 conflation: read failure must not read as "new account".
|
||||||
|
val boom = RuntimeException("firestore unavailable")
|
||||||
|
coEvery { userRepository.getUser("u1") } throws boom
|
||||||
|
|
||||||
|
merger.merge(googleResult) // must not throw
|
||||||
|
|
||||||
|
coVerify(exactly = 0) { userRepository.createUser(any()) }
|
||||||
|
coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) }
|
||||||
|
val recorded = slot<Throwable>()
|
||||||
|
coVerify { crashReporter.recordException(capture(recorded)) }
|
||||||
|
assertEquals(boom, recorded.captured)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aFailedWriteIsRecordedNotThrown() = runTest {
|
||||||
|
// Auth already succeeded; a cosmetic seed failing must not kill the sign-in.
|
||||||
|
coEvery { userRepository.getUser("u1") } returns null
|
||||||
|
coEvery { userRepository.createUser(any()) } throws RuntimeException("write refused")
|
||||||
|
|
||||||
|
merger.merge(googleResult) // must not throw
|
||||||
|
|
||||||
|
coVerify { crashReporter.recordException(any()) }
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aMissingDocumentSeedsANewProfile() = runTest {
|
||||||
|
coEvery { userRepository.getUser("u1") } returns null
|
||||||
|
coEvery { userRepository.createUser(any()) } just Runs
|
||||||
|
|
||||||
|
merger.merge(googleResult)
|
||||||
|
|
||||||
|
val created = slot<User>()
|
||||||
|
coVerify { userRepository.createUser(capture(created)) }
|
||||||
|
assertEquals("Ava", created.captured.displayName)
|
||||||
|
assertEquals("ava@example.com", created.captured.email)
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aSignInStubIsNotSeededOver() = runTest {
|
||||||
|
// The FCM-materialised document: exists, nothing on it. Seeding Google's name over it would
|
||||||
|
// overwrite the real name of a returning user whose profile simply hadn't synced yet.
|
||||||
|
coEvery { userRepository.getUser("u1") } returns User(id = "u1", createdAt = 0L, lastActiveAt = 0L)
|
||||||
|
every { crashReporter.log(any()) } just Runs
|
||||||
|
|
||||||
|
merger.merge(googleResult)
|
||||||
|
|
||||||
|
coVerify(exactly = 0) { userRepository.createUser(any()) }
|
||||||
|
coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) }
|
||||||
|
coVerify(exactly = 0) { userRepository.updatePhotoUrl(any(), any()) }
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun aCompleteProfileIsLeftAlone() = runTest {
|
||||||
|
coEvery { userRepository.getUser("u1") } returns pairedUser
|
||||||
|
|
||||||
|
merger.merge(googleResult)
|
||||||
|
|
||||||
|
coVerify(exactly = 0) { userRepository.createUser(any()) }
|
||||||
|
coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) }
|
||||||
|
coVerify(exactly = 0) { userRepository.updatePhotoUrl(any(), any()) }
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
fun onlyTheBlankFieldsAreFilledIn() = runTest {
|
||||||
|
// A real (non-stub) profile missing just the photo: fill the photo, leave the name.
|
||||||
|
coEvery { userRepository.getUser("u1") } returns pairedUser.copy(photoUrl = "")
|
||||||
|
coEvery { userRepository.updatePhotoUrl(any(), any()) } just Runs
|
||||||
|
|
||||||
|
merger.merge(googleResult)
|
||||||
|
|
||||||
|
coVerify { userRepository.updatePhotoUrl("u1", "https://example.com/a.jpg") }
|
||||||
|
coVerify(exactly = 0) { userRepository.updateDisplayName(any(), any()) }
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in New Issue