From 18e6cf7b3027cb7a231f2938cd666181a41531cd Mon Sep 17 00:00:00 2001 From: null Date: Wed, 15 Jul 2026 01:37:16 -0500 Subject: [PATCH] fix(auth): GoogleProfileMerger must not be able to kill a successful sign-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../domain/usecase/GoogleProfileMerger.kt | 28 ++-- .../app/closer/ui/pairing/RecoveryScreen.kt | 3 +- .../crypto/RecoveryPhraseNormalizationTest.kt | 5 +- .../domain/usecase/GoogleProfileMergerTest.kt | 130 ++++++++++++++++++ 4 files changed, 152 insertions(+), 14 deletions(-) create mode 100644 app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt diff --git a/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt b/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt index fb334471..c80b129e 100644 --- a/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt +++ b/app/src/main/java/app/closer/domain/usecase/GoogleProfileMerger.kt @@ -1,6 +1,6 @@ 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.User 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. * - * 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. + * Best-effort by design, and [merge] never throws: by the time it runs, authentication has already + * succeeded, so a cosmetic seed failing must cost nothing — the caller sets its success state on the + * 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 * 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 class GoogleProfileMerger @Inject constructor( - private val userRepository: UserRepository + private val userRepository: UserRepository, + private val crashReporter: CrashReporter ) { suspend fun merge(result: GoogleSignInResult) { + runCatching { seedProfile(result) } + .onFailure { crashReporter.recordException(it) } + } + + private suspend fun seedProfile(result: GoogleSignInResult) { val uid = result.uid if (uid.isBlank()) return @@ -32,7 +42,7 @@ class GoogleProfileMerger @Inject constructor( 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()) + read.exceptionOrNull()?.let { crashReporter.recordException(it) } return } 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 // 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") + crashReporter.log("GoogleProfileMerger: profile not synced yet at sign-in; not seeding over it") return } @@ -66,8 +76,4 @@ class GoogleProfileMerger @Inject constructor( userRepository.updatePhotoUrl(uid, result.photoUrl) } } - - private companion object { - const val TAG = "GoogleProfileMerger" - } } diff --git a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt index 75ac1db5..f6cd5d8e 100644 --- a/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt +++ b/app/src/main/java/app/closer/ui/pairing/RecoveryScreen.kt @@ -20,6 +20,7 @@ import androidx.compose.material3.Button import androidx.compose.material3.ButtonDefaults import app.closer.ui.components.CloserHeartLoader import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.OutlinedButton import androidx.compose.material3.OutlinedTextField import androidx.compose.material3.OutlinedTextFieldDefaults 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 // (pasted from their partner's message) is unlocked instantly and offline, whereas this // path has to wait on the partner tapping consent. - androidx.compose.material3.OutlinedButton( + OutlinedButton( onClick = onPartnerRestore, enabled = !state.isLoading, modifier = Modifier.fillMaxWidth().heightIn(min = 56.dp), diff --git a/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt index 7cab821d..c065ef65 100644 --- a/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt +++ b/app/src/test/java/app/closer/crypto/RecoveryPhraseNormalizationTest.kt @@ -1,6 +1,7 @@ package app.closer.crypto import org.junit.Assert.assertEquals +import org.junit.Assert.assertNotEquals import org.junit.Test /** @@ -52,8 +53,8 @@ class RecoveryPhraseNormalizationTest { @Test fun aGenuinelyWrongPhraseStillDiffers() { // 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) - assert(norm("hunt down gear east lead over drop live more") != canonical) + assertNotEquals(canonical, norm("hunt down gear east lead over drop live more cat")) + assertNotEquals(canonical, norm("hunt down gear east lead over drop live more")) } @Test diff --git a/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt b/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt new file mode 100644 index 00000000..362a0086 --- /dev/null +++ b/app/src/test/java/app/closer/domain/usecase/GoogleProfileMergerTest.kt @@ -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() + 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() + 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()) } + } +}