Skip to content

Split Health Connect import permissions by category - #1509

Open
kavemang wants to merge 1 commit into
ryanbr:mainfrom
kavemang:fix/health-connect-permissions-645
Open

Split Health Connect import permissions by category#1509
kavemang wants to merge 1 commit into
ryanbr:mainfrom
kavemang:fix/health-connect-permissions-645

Conversation

@kavemang

Copy link
Copy Markdown

Closes #645

Summary

  • group Health Connect reads into Recovery and wellness, Activity, and Body composition categories
  • let users select the same categories from onboarding and Data Sources before Android prompts
  • default new installs to Recovery and wellness while preserving all categories for existing users
  • gate imports and foreground step refreshes on both the selected categories and granted permissions
  • preserve partial grants and request only newly selected permissions
  • localize the consent surface across every shipped Android locale

Tests

  • ANDROID_HOME=/home/kaveman/Android/Sdk ./gradlew testFullDebugUnitTest
  • python3 Tools/i18n_audit.py --ci origin/main
  • python3 Tools/doc_comment_lint.py
  • git diff --check

@kavemang
kavemang marked this pull request as ready for review August 21, 2026 06:43
@ryanbr

ryanbr commented Aug 21, 2026

Copy link
Copy Markdown
Owner

Nice piece of work — the decomposition is clean, all seven locales are covered, and every one of the 16
record types survives the split (I diffed the old permission set against the union of the three categories;
nothing dropped). One thing I'd want changed before it lands, because the failure is silent.

The migration misses a real cohort

selectedCategories treats "has a permission signature" as "is an existing user":

preferences.contains(PERMISSION_SIGNATURE_KEY)

That key arrived on 2026-07-30 with #949. HealthConnectImporter.kt has shipped since 2026-06-07. So a
user who connected Health Connect in that ~8-week window, and hasn't been re-prompted since, has no
signature and no stored selection
categoriesFromStoredKeys hands them DEFAULT_CATEGORIES, i.e.
Recovery only.

For them this PR silently stops importing steps, calories, VO₂max, exercise sessions, distance, weight,
body fat and lean mass. Android still shows those permissions as granted, so nothing on screen would say
why. It reads as "my steps just stopped".

The signal that doesn't have this problem

What Android has already granted. Someone holding a granted StepsRecord permission is an Activity user by
definition, whatever version they onboarded under — and it's strictly more accurate than the pref for
everyone else too.

I built and tested the change rather than sketching it, because two details only show up if you compile it.
Full patch below: 86 insertions, Android suite 4,183 passing on top of your branch, doc lint clean.

1. The derivation, and a guarded one-time write

internal fun categoriesFromGrantedPermissions(granted: Set<String>): Set<ImportCategory> =
    ImportCategory.entries.filterTo(linkedSetOf()) { category ->
        permissionsFor(setOf(category)).any { it in granted }
    }

fun migrateSelectionFromGrants(context: Context, granted: Set<String>) {
    if (prefs(context).getStringSet(CATEGORY_SELECTION_KEY, null) != null) return
    val inferred = categoriesFromGrantedPermissions(granted)
    if (inferred.isNotEmpty()) setSelectedCategories(context, inferred)
}

It only writes when nothing is stored, so someone who deliberately narrows to Recovery is never
re-broadened, and it's safe to call from every entry point that might be reached first.

2. import()'s default has to become lazy — this is the detail I'd have missed

-        categories: Set<ImportCategory> = selectedCategories(context),
+        categories: Set<ImportCategory>? = null,
+        migrateSelectionFromGrants(context, granted)
+        val effectiveCategories = categories ?: selectedCategories(context)

A default argument is evaluated at call time, so categories is already bound before granted exists.
Migrating inside import() without this changes nothing at all — it compiles, ships, and quietly does
nothing for the users it was written for.

3. Both selector screens need it too

DataSourcesScreen:122 and OnboardingScreen:782 read the selection synchronously into
mutableStateOf. A user opening Data Sources before any import runs would see Recovery-only pre-ticked,
and saving it locks in the narrowing the migration was meant to prevent. Both already fetch grants, so it's
five lines each:

HealthConnectImporter.migrateSelectionFromGrants(context, granted)
hcReadCategories = HealthConnectImporter.selectedCategories(context)

4. A test that fails against the PR as it stands

@Test
fun grantsFromBeforeTheSelectorRecoverTheirCategories() {
    val granted = setOf(
        HealthPermission.getReadPermission(HeartRateRecord::class),
        HealthPermission.getReadPermission(StepsRecord::class),
    )
    assertEquals(
        setOf(ImportCategory.RECOVERY, ImportCategory.ACTIVITY),
        HealthConnectImporter.categoriesFromGrantedPermissions(granted),
    )
}

Full patch

86 insertions across 4 files — apply with git apply
diff --git a/android/app/src/main/java/com/noop/ingest/HealthConnectImporter.kt b/android/app/src/main/java/com/noop/ingest/HealthConnectImporter.kt
index 89206eaf5..5d1db2db2 100644
--- a/android/app/src/main/java/com/noop/ingest/HealthConnectImporter.kt
+++ b/android/app/src/main/java/com/noop/ingest/HealthConnectImporter.kt
@@ -186,6 +186,34 @@ object HealthConnectImporter {
             .apply()
     }
 
+    /**
+     * The categories implied by grants Android already holds.
+     *
+     * A user who granted Health Connect before #645 existed has no stored selection, and — if they
+     * onboarded before #949 added it — no permission signature either. The importer has shipped since
+     * 2026-06-07 and that key only since 2026-07-30, so there is a real cohort with neither. Falling back
+     * to [DEFAULT_CATEGORIES] for them would silently stop importing Activity and Body composition while
+     * Android still shows those permissions as granted: nothing on screen would say why steps stopped.
+     *
+     * Their grants are the honest record of what they agreed to, so read the scope back off those.
+     */
+    internal fun categoriesFromGrantedPermissions(granted: Set<String>): Set<ImportCategory> =
+        ImportCategory.entries.filterTo(linkedSetOf()) { category ->
+            permissionsFor(setOf(category)).any { it in granted }
+        }
+
+    /**
+     * One-time backfill of the selection for a user who predates it, from what Android has granted.
+     *
+     * Only ever writes when NOTHING is stored, so a user who deliberately narrows to Recovery is never
+     * re-broadened, and it is safe to call from every entry point that can be the first one reached.
+     */
+    fun migrateSelectionFromGrants(context: Context, granted: Set<String>) {
+        if (prefs(context).getStringSet(CATEGORY_SELECTION_KEY, null) != null) return
+        val inferred = categoriesFromGrantedPermissions(granted)
+        if (inferred.isNotEmpty()) setSelectedCategories(context, inferred)
+    }
+
     internal fun categoriesFromStoredKeys(
         storedKeys: Set<String>?,
         hadLegacyPermissionSignature: Boolean,
@@ -261,7 +289,9 @@ object HealthConnectImporter {
         context: Context,
         repo: WhoopRepository,
         heightCm: Double = 0.0,
-        categories: Set<ImportCategory> = selectedCategories(context),
+        // Null means "whatever the user has selected", resolved AFTER the grant-based migration below. A
+        // non-lazy default is evaluated at CALL time, before the migration could widen it.
+        categories: Set<ImportCategory>? = null,
     ): ImportSummary {
         if (sdkStatus(context) != HealthConnectClient.SDK_AVAILABLE) {
             return ImportSummary.failure(SOURCE, "Health Connect is not available on this device.")
@@ -285,12 +315,18 @@ object HealthConnectImporter {
         } catch (e: Exception) {
             return ImportSummary.failure(SOURCE, "Could not read Health Connect permissions: ${e.message}")
         }
+        // #645 follow-up: a user who predates the category selector has nothing stored — recover their
+        // real scope from the grants before deciding what to read, or the first import after the update
+        // would quietly narrow them to Recovery.
+        migrateSelectionFromGrants(context, granted)
+        val effectiveCategories = categories ?: selectedCategories(context)
+
         // Partial permissions are fine (#150): import the record types the user DID grant and skip the
         // rest, instead of refusing the whole import when any single type is missing. Each per-type read
         // below is already independently fault-tolerant — a type whose read permission was revoked throws
         // and is caught/skipped in [readAll] (same path as #34) — so we only need to bail when NOTHING is
         // granted. The user choosing exactly what NOOP can see is the intended behaviour.
-        val selectedPermissions = permissionsFor(categories)
+        val selectedPermissions = permissionsFor(effectiveCategories)
         if (granted.none { it in selectedPermissions }) {
             return ImportSummary.failure(
                 SOURCE,
@@ -304,7 +340,7 @@ object HealthConnectImporter {
         val filter = TimeRangeFilter.between(start, end)
         // #528: skip our own writes on import (see readAll / isSelfWritten).
         val selfPackage = context.packageName
-        val selectedRecordTypes = readableRecordTypes(categories, granted)
+        val selectedRecordTypes = readableRecordTypes(effectiveCategories, granted)
 
         // A granted permission can outlive the category selection that originally requested it. Gate
         // on BOTH here so switching a category off stops its reads immediately without requiring the
diff --git a/android/app/src/main/java/com/noop/ui/DataSourcesScreen.kt b/android/app/src/main/java/com/noop/ui/DataSourcesScreen.kt
index ab0376469..1d9f6dfbd 100644
--- a/android/app/src/main/java/com/noop/ui/DataSourcesScreen.kt
+++ b/android/app/src/main/java/com/noop/ui/DataSourcesScreen.kt
@@ -310,6 +310,11 @@ fun DataSourcesScreen(vm: AppViewModel) {
             val granted = runCatching {
                 HealthConnectImporter.client(context).permissionController.getGrantedPermissions()
             }.getOrDefault(emptySet())
+            // #645: a user who predates the selector has nothing stored. Recover their real scope from
+            // what Android already grants BEFORE the checkboxes are read back, or a first visit would
+            // show Recovery-only and saving it would lock in the narrowing.
+            HealthConnectImporter.migrateSelectionFromGrants(context, granted)
+            hcReadCategories = HealthConnectImporter.selectedCategories(context)
             val selectedPermissions = HealthConnectImporter.permissionsFor(hcReadCategories)
             // `any` (not `all`) is deliberate — partial grants are supported (#150). But that alone
             // would never ASK about a permission added in an update, so a newly-read type would come
diff --git a/android/app/src/main/java/com/noop/ui/OnboardingScreen.kt b/android/app/src/main/java/com/noop/ui/OnboardingScreen.kt
index fb1367884..b65058268 100644
--- a/android/app/src/main/java/com/noop/ui/OnboardingScreen.kt
+++ b/android/app/src/main/java/com/noop/ui/OnboardingScreen.kt
@@ -832,6 +832,11 @@ private fun ImportStep(viewModel: AppViewModel) {
             val granted = runCatching {
                 HealthConnectImporter.client(context).permissionController.getGrantedPermissions()
             }.getOrDefault(emptySet())
+            // #645: a user who predates the selector has nothing stored. Recover their real scope from
+            // what Android already grants BEFORE the checkboxes are read back, or a first visit would
+            // show Recovery-only and saving it would lock in the narrowing.
+            HealthConnectImporter.migrateSelectionFromGrants(context, granted)
+            hcReadCategories = HealthConnectImporter.selectedCategories(context)
             val selectedPermissions = HealthConnectImporter.permissionsFor(hcReadCategories)
             if (granted.any { it in selectedPermissions } &&
                 !HealthConnectImporter.hasUnaskedPermissions(context, hcReadCategories)
diff --git a/android/app/src/test/java/com/noop/ingest/HealthConnectPermissionCategoryTest.kt b/android/app/src/test/java/com/noop/ingest/HealthConnectPermissionCategoryTest.kt
index 072658eb1..8d40667f1 100644
--- a/android/app/src/test/java/com/noop/ingest/HealthConnectPermissionCategoryTest.kt
+++ b/android/app/src/test/java/com/noop/ingest/HealthConnectPermissionCategoryTest.kt
@@ -92,4 +92,41 @@ class HealthConnectPermissionCategoryTest {
 
         assertEquals(setOf(HeartRateRecord::class), readable)
     }
+    /**
+     * #645 migration: a user who granted Health Connect before the selector existed has no stored
+     * selection, and if they onboarded before #949 no permission signature either — the importer has
+     * shipped since 2026-06-07 and that key only since 2026-07-30. Their Android grants are the only
+     * honest record of what they agreed to, so the scope is read back off those rather than defaulted.
+     *
+     * Without this they would silently stop importing Activity and Body composition while Android still
+     * showed those permissions as granted, with nothing on screen saying why steps had stopped.
+     */
+    @Test
+    fun grantsFromBeforeTheSelectorRecoverTheirCategories() {
+        val granted = setOf(
+            HealthPermission.getReadPermission(HeartRateRecord::class),
+            HealthPermission.getReadPermission(StepsRecord::class),
+        )
+        assertEquals(
+            setOf(HealthConnectImporter.ImportCategory.RECOVERY, HealthConnectImporter.ImportCategory.ACTIVITY),
+            HealthConnectImporter.categoriesFromGrantedPermissions(granted),
+        )
+    }
+
+    /** A fresh install grants nothing, so there is nothing to recover and the caller keeps its default. */
+    @Test
+    fun noGrantsRecoversNothing() {
+        assertTrue(HealthConnectImporter.categoriesFromGrantedPermissions(emptySet()).isEmpty())
+    }
+
+    /** One granted type is enough to claim its whole category — partial grants stay supported (#150). */
+    @Test
+    fun oneGrantedTypeClaimsItsCategory() {
+        assertEquals(
+            setOf(HealthConnectImporter.ImportCategory.BODY_COMPOSITION),
+            HealthConnectImporter.categoriesFromGrantedPermissions(
+                setOf(HealthPermission.getReadPermission(WeightRecord::class)),
+            ),
+        )
+    }
 }

Happy for you to take it as-is, adapt it, or push back if you read the cohort differently — it's your PR
and the rest of it is in good shape.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Privacy: split Health Connect import permissions by data category

2 participants