From aee5e57ba8ba1cf2f73f1cd56f78f8b57adb2fb9 Mon Sep 17 00:00:00 2001 From: Jason Ross Date: Fri, 3 Jul 2026 03:18:36 -0500 Subject: [PATCH] fix(onboarding): enforce GPL license gate for upgrade users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AppViewModel.startDestination chose onboarding-vs-mailbox purely by account count, so a user upgrading from a pre-#172 install (accounts present, licenseAccepted=false) went straight to the mailbox and never saw the license screen — the license is only the onboarding graph's start destination. Route to ONBOARDING whenever the license is unaccepted, even when accounts exist; only an accepted user with an account lands on the mailbox. Once such a user accepts, send them straight to their inbox rather than ONBOARDING_WELCOME ("add your first account"), which would strand a user who already has accounts. AppViewModel now also exposes hasAccounts for that post-accept routing decision. Also guard the pendingCompose mailto/share deep-link with the same start != ONBOARDING check pendingOpenMessageId already uses, so a deep-link can't jump past the license gate either. From the post-batch security review, refs #172. Co-Authored-By: Claude Opus 4.8 --- .../kotlin/org/libremail/ui/AppViewModel.kt | 47 +++++++++++----- .../kotlin/org/libremail/ui/LibreMailApp.kt | 53 +++++++++++++------ .../org/libremail/ui/AppViewModelTest.kt | 44 +++++++++++---- 3 files changed, 104 insertions(+), 40 deletions(-) diff --git a/app/src/main/kotlin/org/libremail/ui/AppViewModel.kt b/app/src/main/kotlin/org/libremail/ui/AppViewModel.kt index ace2c7e..6cd2b33 100644 --- a/app/src/main/kotlin/org/libremail/ui/AppViewModel.kt +++ b/app/src/main/kotlin/org/libremail/ui/AppViewModel.kt @@ -6,6 +6,7 @@ import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.map import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.flow.take @@ -15,33 +16,51 @@ import org.libremail.ui.navigation.Routes import javax.inject.Inject /** - * Decides the app's start destination from the stored account count: no accounts → the onboarding - * welcome flow; otherwise the mailbox. + * Decides the app's start destination from two launch-time facts: whether any account is stored and + * whether the GPL-3.0 license has been accepted (#172). The license gate comes first — an un-accepted + * license routes to onboarding (which begins at the license screen) even when accounts already exist, + * so a user upgrading from a pre-#172 install can't skip the license by virtue of already having mail. + * Only a user who has accepted AND has at least one account lands straight on the mailbox. * - * [startDestination] is `null` until the first account snapshot loads — the UI holds (renders - * nothing) during that window so a cold start never flashes the wrong screen. Only the *first* - * determination is used ([take]), so adding the first account mid-onboarding does not later flip the - * start destination and tear down the in-progress flow. + * Each exposed flow is `null` until the first snapshot loads — the UI holds (renders nothing) during + * that window so a cold start never flashes the wrong screen. Only the *first* determination is used + * ([take]), so adding the first account (or accepting the license) mid-onboarding does not later flip + * the start destination and tear down the in-progress flow. */ @HiltViewModel class AppViewModel @Inject constructor(accountRepository: AccountRepository, settingsRepository: SettingsRepository) : ViewModel() { - val startDestination: StateFlow = accountRepository.observeAccounts() - .map { accounts -> if (accounts.isEmpty()) Routes.ONBOARDING else Routes.MAILBOX } + val startDestination: StateFlow = combine( + accountRepository.observeAccounts(), + settingsRepository.settings, + ) { accounts, settings -> + if (settings.licenseAccepted && accounts.isNotEmpty()) Routes.MAILBOX else Routes.ONBOARDING + } .take(1) .stateIn(viewModelScope, SharingStarted.Eagerly, null) /** - * Whether the user has already agreed to the GPL-3.0 license (#172), resolved once at launch - * exactly like [startDestination] (same "hold until known, then never re-decide" contract). - * [LibreMailApp][org.libremail.ui.LibreMailApp] holds off building its `NavHost` until this is - * known too, because the onboarding graph — and therefore its choice of start destination between - * `Routes.ONBOARDING_LICENSE` and `Routes.ONBOARDING_WELCOME` — is registered as part of that same - * `NavHost` regardless of whether [startDestination] itself resolves to onboarding or the mailbox. + * Whether the user has already agreed to the GPL-3.0 license (#172), resolved once at launch. + * [LibreMailApp][org.libremail.ui.LibreMailApp] uses it to pick the onboarding graph's own start + * destination between `Routes.ONBOARDING_LICENSE` and `Routes.ONBOARDING_WELCOME`, and holds off + * building its `NavHost` until this is known — the onboarding graph is registered as part of that + * `NavHost` regardless of whether [startDestination] resolves to onboarding or the mailbox. */ val licenseAccepted: StateFlow = settingsRepository.settings .map { it.licenseAccepted } .take(1) .stateIn(viewModelScope, SharingStarted.Eagerly, null) + + /** + * Whether at least one account already exists, resolved once at launch. Lets + * [LibreMailApp][org.libremail.ui.LibreMailApp] route correctly *after* an upgrade user accepts the + * license: someone who already had accounts goes straight to the mailbox rather than the "add your + * first account" welcome screen (which would be a dead-end for them), while a fresh install with no + * accounts continues into that welcome flow as before. + */ + val hasAccounts: StateFlow = accountRepository.observeAccounts() + .map { it.isNotEmpty() } + .take(1) + .stateIn(viewModelScope, SharingStarted.Eagerly, null) } diff --git a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt index fa6e750..5761662 100644 --- a/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt +++ b/app/src/main/kotlin/org/libremail/ui/LibreMailApp.kt @@ -63,11 +63,13 @@ fun LibreMailApp( ) { val startDestination by appViewModel.startDestination.collectAsStateWithLifecycle() val licenseAccepted by appViewModel.licenseAccepted.collectAsStateWithLifecycle() + val hasAccounts by appViewModel.hasAccounts.collectAsStateWithLifecycle() // Hold (render nothing) until the account count AND the license-acceptance flag are known, so a // cold start never flashes the wrong screen before onboarding-vs-mailbox — and, within onboarding, // license-vs-welcome (#172) — is decided. val start = startDestination ?: return val licenseAlreadyAccepted = licenseAccepted ?: return + val hasExistingAccounts = hasAccounts ?: return val navController = rememberNavController() val pendingCrash by startupViewModel.pendingCrash.collectAsStateWithLifecycle() @@ -75,15 +77,20 @@ fun LibreMailApp( // it fires once per intent (and again for a new intent delivered while the app is alive). LaunchedEffect(pendingCompose) { val prefill = pendingCompose ?: return@LaunchedEffect - navController.navigate( - Routes.compose( - to = prefill.to, - subject = prefill.subject, - cc = prefill.cc, - bcc = prefill.bcc, - body = prefill.body, - ), - ) + // Don't let a deep-link jump past onboarding — including the license gate (#172), which is why + // start == ONBOARDING now also covers an upgrade user who hasn't accepted yet. Consume the + // request either way so it isn't replayed. Mirrors the pendingOpenMessageId guard below. + if (start != Routes.ONBOARDING) { + navController.navigate( + Routes.compose( + to = prefill.to, + subject = prefill.subject, + cc = prefill.cc, + bcc = prefill.bcc, + body = prefill.body, + ), + ) + } onComposeHandled() } @@ -100,7 +107,7 @@ fun LibreMailApp( navController = navController, startDestination = start, ) { - onboardingGraph(navController, licenseAlreadyAccepted) + onboardingGraph(navController, licenseAlreadyAccepted, hasExistingAccounts) composable( route = Routes.MAILBOX_PATTERN, @@ -295,8 +302,15 @@ private fun CrashReportDialog(onReview: () -> Unit, onLater: () -> Unit, onDisca * @param licenseAlreadyAccepted decides the graph's start destination (#172): false routes through * [Routes.ONBOARDING_LICENSE] first; true (the user already agreed on a prior run) skips straight to * [Routes.ONBOARDING_WELCOME], matching this graph's pre-#172 behavior. + * @param hasExistingAccounts where to go once the license is accepted from [Routes.ONBOARDING_LICENSE]: + * true (an upgrade from a pre-#172 install that already has accounts) goes straight to the mailbox; + * false (a fresh install) continues into the welcome/add-account flow. */ -private fun NavGraphBuilder.onboardingGraph(navController: NavHostController, licenseAlreadyAccepted: Boolean) { +private fun NavGraphBuilder.onboardingGraph( + navController: NavHostController, + licenseAlreadyAccepted: Boolean, + hasExistingAccounts: Boolean, +) { val onboardingStart = if (licenseAlreadyAccepted) Routes.ONBOARDING_WELCOME else Routes.ONBOARDING_LICENSE navigation(startDestination = onboardingStart, route = Routes.ONBOARDING) { composable(Routes.ONBOARDING_LICENSE) { entry -> @@ -305,10 +319,19 @@ private fun NavGraphBuilder.onboardingGraph(navController: NavHostController, li LicenseScreen( onAgree = { onboarding.markLicenseAccepted() - // Pop LICENSE off the back stack: it is a one-time gate, so a later back-press - // from the welcome screen must not be able to return to it. - navController.navigate(Routes.ONBOARDING_WELCOME) { - popUpTo(Routes.ONBOARDING_LICENSE) { inclusive = true } + if (hasExistingAccounts) { + // Upgrade path (#172 follow-up): a pre-#172 install with accounts is now forced + // through the license gate too (see AppViewModel.startDestination). Once they + // accept, send them straight to their inbox — ONBOARDING_WELCOME ("add your + // first account") would strand a user who already has accounts. finishOnboarding + // pops the whole onboarding graph, so the one-time license gate is left behind. + navController.finishOnboarding(firstAccountId = null) + } else { + // Fresh install: continue into the add-account flow. Pop LICENSE off the back + // stack so a later back-press from welcome can't return to this one-time gate. + navController.navigate(Routes.ONBOARDING_WELCOME) { + popUpTo(Routes.ONBOARDING_LICENSE) { inclusive = true } + } } }, // MainActivity is this app's only Activity (single-activity Compose app), so finish() diff --git a/app/src/test/kotlin/org/libremail/ui/AppViewModelTest.kt b/app/src/test/kotlin/org/libremail/ui/AppViewModelTest.kt index 4b63a43..e1e9d47 100644 --- a/app/src/test/kotlin/org/libremail/ui/AppViewModelTest.kt +++ b/app/src/test/kotlin/org/libremail/ui/AppViewModelTest.kt @@ -24,12 +24,12 @@ import org.libremail.ui.navigation.Routes import kotlin.test.assertEquals /** - * [AppViewModel] decides two independent things, each resolved once at launch: the top-level start - * destination (onboarding vs. mailbox, from the stored account count) and, since #172, whether the - * user has already agreed to the license (consulted by `onboardingGraph()` in `LibreMailApp.kt` to - * pick between `Routes.ONBOARDING_LICENSE` and `Routes.ONBOARDING_WELCOME`). Both repositories are - * mocked: [AccountRepository] and [SettingsRepository] are interfaces/classes this ViewModel only - * reads from, and `SettingsRepository` itself needs a real `Context` it doesn't get in a JVM test. + * [AppViewModel] resolves, once at launch, the top-level start destination plus the two facts it + * derives from: whether any account exists and whether the license has been accepted (#172). The + * start destination is onboarding unless the license is accepted AND an account exists — so an upgrade + * user with accounts but no acceptance is still routed through the license gate, not the mailbox. + * Both repositories are mocked: [AccountRepository] and [SettingsRepository] are types this ViewModel + * only reads from, and `SettingsRepository` itself needs a real `Context` it doesn't get in a JVM test. */ @OptIn(ExperimentalCoroutinesApi::class) class AppViewModelTest { @@ -60,19 +60,32 @@ class AppViewModelTest { } @Test - fun `no accounts resolves onboarding as the start destination`() = runTest(testDispatcher) { - val vm = AppViewModel(accountRepository(emptyList()), settingsRepository()) + fun `no accounts routes to onboarding regardless of license state`() = runTest(testDispatcher) { + val unaccepted = AppViewModel(accountRepository(emptyList()), settingsRepository(licenseAccepted = false)) + val accepted = AppViewModel(accountRepository(emptyList()), settingsRepository(licenseAccepted = true)) - assertEquals(Routes.ONBOARDING, vm.startDestination.value) + assertEquals(Routes.ONBOARDING, unaccepted.startDestination.value) + assertEquals(Routes.ONBOARDING, accepted.startDestination.value) } @Test - fun `an existing account resolves the mailbox as the start destination`() = runTest(testDispatcher) { - val vm = AppViewModel(accountRepository(listOf(account())), settingsRepository()) + fun `existing accounts route to the mailbox once the license is accepted`() = runTest(testDispatcher) { + val vm = AppViewModel(accountRepository(listOf(account())), settingsRepository(licenseAccepted = true)) assertEquals(Routes.MAILBOX, vm.startDestination.value) } + @Test + fun `existing accounts with an unaccepted license still route to onboarding (upgrade license gate)`() = + runTest(testDispatcher) { + // Regression for the post-batch review finding: a pre-#172 install already has accounts, so + // the old account-count-only rule sent it straight to the mailbox and skipped the license. + // The license gate must win, forcing onboarding (which begins at ONBOARDING_LICENSE). + val vm = AppViewModel(accountRepository(listOf(account())), settingsRepository(licenseAccepted = false)) + + assertEquals(Routes.ONBOARDING, vm.startDestination.value) + } + @Test fun `license acceptance defaults to not accepted`() = runTest(testDispatcher) { val vm = AppViewModel(accountRepository(), settingsRepository(licenseAccepted = false)) @@ -87,4 +100,13 @@ class AppViewModelTest { assertEquals(true, vm.licenseAccepted.value) } + + @Test + fun `hasAccounts reflects whether any account is stored`() = runTest(testDispatcher) { + val empty = AppViewModel(accountRepository(emptyList()), settingsRepository()) + val withAccount = AppViewModel(accountRepository(listOf(account())), settingsRepository()) + + assertEquals(false, empty.hasAccounts.value) + assertEquals(true, withAccount.hasAccounts.value) + } }