Skip to content

Privacy: split HealthKit permissions into staged categories - #959

Open
tigercraft4 wants to merge 1 commit into
ryanbr:mainfrom
tigercraft4:privacy/staged-health-permission-consent
Open

tigercraft4 wants to merge 1 commit into
ryanbr:mainfrom
tigercraft4:privacy/staged-health-permission-consent

Conversation

@tigercraft4

@tigercraft4 tigercraft4 commented Jul 30, 2026

Copy link
Copy Markdown

Fixes #645
Fixes #653

What this PR does

Splits Apple Health authorization into staged, explicit consent instead of asking for every read and write capability in one system sheet.

This branch now contains only the Apple/HealthKit half. The Android category flow landed separately in #1509 and all redundant Android changes were dropped during the rebase requested in review.

The primary Enable Apple Health action now requests only:

  • core reads used by NOOP's local scoring and Apple Health views; and
  • nightly-vitals write-back for resting HR, HRV, blood oxygen, respiratory rate, and sleep.

Once connected, three narrower extras are exposed separately, with explanatory copy before each HealthKit request:

  • Body composition — read-only weight, body fat, lean mass, and BMI.
  • Continuous heart rate — write-back of NOOP's minute-by-minute heart rate.
  • Workouts — workout write-back with its energy and distance samples.

Existing per-feature authorization guards, source-scoped deletion, deduplication, and chunked heart-rate writes remain unchanged.

Rebase decisions

  • Reconciles iOS: write to Apple Health when an offload lands, not only on foreground entry (#1021) #1024 by retaining its status-only refreshAuthIfPreviouslyGranted() path, so background/offload write-back can restore the bridge's authorization state.
  • Removes the old automatic re-request of extra share types from that resume path. Optional write-back stages are now requested only after an explicit user action.
  • Keeps the foreground-only core-read signature refresh for genuinely new read types; it cannot request body composition or either optional write-back stage.
  • Existing users retain previously granted HealthKit access. The body-composition read flag migrates on when the legacy nightly-vitals grant proves the old bundled flow was completed; new installs default it off.

Type of change

  • Bug fix / privacy improvement

How it was tested

  • python3 Tools/i18n_audit.py --ci origin/main — passed.
  • python3 -m json.tool Strand/Resources/Localizable.xcstrings — passed.
  • xcodebuild -project Strand.xcodeproj -scheme NOOPiOS -destination "platform=iOS Simulator,name=iPhone 17 Pro" CODE_SIGNING_ALLOWED=NO build — passed locally.
  • GitHub Actions on exact head 22970a49: 5/5 checks passed, including the NOOPiOS and macOS builds, Android build/tests, i18n Coverage, and Source Hygiene.

No real-device HealthKit authorization sheet was exercised in this pass. This change does not touch BLE collection or protocol behavior.

Checklist

  • Current main rebased cleanly
  • Apple and Android CI pass
  • New UI copy is localized
  • No BLE protocol bytes changed
  • No generated project output or secrets committed

@ryanbr

ryanbr commented Aug 2, 2026

Copy link
Copy Markdown
Owner

This has gone CONFLICTING, and it is worth flagging because it is not just a textual conflict#1024 landed on main and the two changes disagree.

Your #653 work removes the silent auto-reprompt for newly-added share types. #1024 went the other way: it kept refreshAuthIfPreviouslyGranted() and added a call to it from the offload write-back path, because a backfill completing in a backgrounded or BLE-relaunched process left auth at .unknown and silently dropped every write. I foreground-gated the re-request inside it so it cannot spend the one authorization request where no sheet can present — but the reprompt still exists, which is what your comment says it should not.

So resolving the HealthKitBridge.swift hunks means deciding which behaviour wins, not merging text. I did not attempt it: guessing there would quietly overturn a privacy decision.

My read is they are reconcilable — the write-back needs auth resumed, which is the status read, not the reprompt; the reprompt is a separate branch inside the same function and could go without breaking #1024. But that is your call, and worth stating explicitly in the PR either way.

@tigercraft4
tigercraft4 force-pushed the privacy/staged-health-permission-consent branch 2 times, most recently from 387b7eb to b45af10 Compare August 29, 2026 21:10
ryanbr pushed a commit that referenced this pull request Sep 1, 2026
Health Connect asked for everything at once. It now asks per category - Recovery
and wellness, Activity, Body composition - chosen in onboarding or Data Sources
BEFORE Android prompts, so the system dialog explains what is being granted
rather than presenting one undifferentiated list.

The parts most likely to be wrong are right.

Existing users keep everything. categoriesFromStoredKeys falls back to
`hadLegacyPermissionSignature ? ALL_CATEGORIES : DEFAULT_CATEGORIES`, so someone
prompted under the old all-at-once flow is unchanged and only new installs get
the narrow default. A privacy split that silently narrowed existing users would
be data loss wearing a privacy hat.

It fails safe twice over. migrateSelectionFromGrants guards
`if (inferred.isNotEmpty())`, so an empty grant set cannot reach
setSelectedCategories' non-empty require and crash. And when stored keys exist
but none are recognised, `if (restored.isNotEmpty())` falls through to the
legacy path rather than returning an empty selection.

Re-prompting is handled with care: hasUnaskedPermissions stores the union of
permissions previously PRESENTED, not the last selection, so narrowing never
re-prompts and a decline stays remembered.

Verified rather than assumed, because this branch is 286 commits behind main and
its green checks predate that drift. Merged it locally against current main:
clean, and #1735's recordImportRun stamps survive on both "ran" paths - the new
early exit is a failure(), consistent with main's other failure paths, so the
"never ran" versus "ran and found nothing" distinction is preserved. The merged
tree compiles and passes 5039 unit tests, 0 failures, including the 10 new
permission-category tests. Every new string is present in all seven locale
files; i18n audit exits 0.

Issue 645 is deliberately NOT closed here. PR #959 targets the same issue and
carries the HealthKit half, and closing it on the Android half alone would leave
that PR pointing at a closed issue and reading as redundant when it is the only
one covering Apple. The closing keyword was removed from this PR's description
before merge for that reason.

Thanks @kavemang.
@ryanbr

ryanbr commented Sep 1, 2026

Copy link
Copy Markdown
Owner

Heads up @tigercraft4#1509 (@kavemang) just landed the Android half of this, so this PR will need a rebase and will conflict in HealthConnectImporter.kt, DataSourcesScreen.kt and the values*/strings.xml files.

Two things I did deliberately so this PR isn't left stranded:

#645 is still open. #1509's description carried Closes #645, which would have closed your issue on the Android half alone and left this PR pointing at a closed issue, reading as redundant when it's the only one covering Apple. I removed the closing keyword before merging, and verified the link is gone — #645 stays open until HealthKit has it too.

The Apple half is the part that's now unique to this PR, and it's the reason the feature isn't finished. Android now asks per category; HealthKit still requests everything at once, which is the more visible gap of the two on a privacy feature.

So the smaller ask: rebase this down to the Apple sideHealthKitBridge.swift, AppleHealthView.swift, Strand/Resources/Localizable.xcstrings — and drop the Android files, which are now redundant. That should shrink it considerably from +1005/−80 and remove the conflicts entirely.

Worth knowing what landed on the Android side, in case you want the Apple half to mirror it: three categories (Recovery and wellness / Activity / Body composition); selection happens before the OS prompt, so the system dialog is scoped; existing users are detected by their legacy permission signature and keep all categories, while only new installs get the narrow default; and re-prompt state stores the union of permissions previously presented, so narrowing never re-prompts and a decline stays remembered.

Sorry for the collision — your issue, and you got there first on scope. This was the sequencing that kept both pieces of work alive rather than one closing the other out.

…ly rebase)

Rebases ryanbr#959 down to the Apple side per ryanbr's request on the PR:
Android is dropped because ryanbr#1509 already shipped the category split there
(HealthConnectImporter.kt, DataSourcesScreen.kt, values*/strings.xml are
byte-identical to upstream/main again). ryanbr#645 (Android) stays closed by
that PR; this is scoped to ryanbr#653 (iOS) only.

Also resolves the HealthKitBridge.swift conflict with ryanbr#1024 the way
ryanbr suggested in the same comment: refreshAuthIfPreviouslyGranted()
keeps the nightlyVitalsWriteTypesGranted() status read that ryanbr#1024's
writeBackAfterNewData() depends on to resume `auth` for a backgrounded
offload, but drops the silent auto-reprompt branch for newly-added
share types -- the one behavior ryanbr#653's own review objected to. That
was the only actual behavioral disagreement between the two PRs;
everything else conflicted only textually.

requestNewReadTypesIfNeeded()/readTypeSignature (ryanbr#949) is rewired from
the pre-staging quantityReadIds/writeTypes/readTypes symbols (removed
by ryanbr#653) to the new .coreRead-scoped accessors, keeping it scoped to
core reads only so it can never re-request body composition or either
write-back stage -- those already have their own explicit opt-in paths.

Not done here: an actual Xcode build. No macOS toolchain in this
environment, matching the limitation ryanbr#959's own description already
discloses.

Verified instead:
- cherry-picked ryanbr#959's single commit, resolved conflicts by hand,
  reviewed line by line
- grepped this file and every other .swift file in the repo for
  dangling references to symbols ryanbr#653 renamed/removed
  (quantityReadIds/quantityWriteIds/highResQuantityWriteIds/bare
  writeTypes/readTypes) -- none found
- no leftover git conflict markers anywhere in the tree
- Localizable.xcstrings re-validated as parseable JSON with no
  duplicate keys after the merge
- Android files (HealthConnectImporter.kt, DataSourcesScreen.kt,
  values*/strings.xml) diffed byte-for-byte against upstream/main --
  zero drift, confirming the Android half was fully dropped and not
  silently reintroduced
@tigercraft4
tigercraft4 force-pushed the privacy/staged-health-permission-consent branch from b45af10 to 22970a4 Compare September 14, 2026 13:12
@tigercraft4 tigercraft4 changed the title Privacy: split Health Connect and HealthKit permissions into staged categories Privacy: split HealthKit permissions into staged categories Sep 14, 2026
@tigercraft4

Copy link
Copy Markdown
Author

@ryanbr the requested rebase is complete: the redundant Android half is gone, this PR is now Apple/HealthKit-only, and #1024 is reconciled by keeping its status-resume path without silently re-requesting optional share scopes. Title/body are updated. The exact head 22970a49 is conflict-free and all 5 CI checks are green, including iOS, macOS, and Android. Ready for your merge review.

@ryanbr

ryanbr commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Reviewed at 22970a4. The rebase I asked for is done and the shape is right: Apple-only, staged consent, #1024 reconciled the way I guessed it could be. One thing needs changing before this can go in, and one piece of housekeeping is my fault rather than yours.

First, a correction I owe you

On 2026-09-01 I edited the description of #1509 so it would not auto-retire #645 on merge, kept that issue open "until HealthKit has it too", and called it "your issue". That was wrong. #645 is the Health Connect issue: its body is entirely HealthConnectImporter.kt and DataSourcesScreen.kt, with nothing about HealthKit. #1509 was the Health Connect half, so #645 should have been retired then. Your issue is #653.

The practical effect is that #645 has sat open for three weeks on my misreading, and the rationale I gave you for why this PR mattered was muddled. The Apple half genuinely is the remaining gap, and that part stands, but it tracks #653 and always did.

The change this needs

nightlyVitalsWriteTypesGranted gates on allSatisfy: every one of resting HR, HRV, blood oxygen, respiratory rate AND sleep must read .sharingAuthorized. It replaces this:

let granted = writeTypes.contains { store.authorizationStatus(for: $0) == .sharingAuthorized }

whose comment, removed in the same hunk, records why it was deliberately broad:

Resume when at least one write type is granted so a person who intentionally declined (for example) workouts still gets sleep/vitals exported after a relaunch. Requiring every legacy type made partial grants look wholly disconnected.

So the new predicate is the thing that comment says was tried and rejected. This matters more than it looks, because under the OLD bundled sheet a person could untick individual types: partial grants are the ordinary legacy state, not a corner. Someone who declined blood oxygen and kept everything else is a completely normal pre-#653 install.

One helper, two callers, and it bites in both:

  1. refreshAuthIfPreviouslyGranted. That person never resumes auth out of .unknown in a backgrounded or BLE-relaunched process, so every write-back is silently dropped. That is exactly the iOS: write to Apple Health when an offload lands, not only on foreground entry (#1021) #1024 symptom, in the path that exists to prevent it, and it is invisible: no error, just nothing written.
  2. The init migration. The same person does not get body composition migrated on, so their weight and body-fat history disappears behind an unset toggle. That is the specific outcome your own migration comment says the migration exists to avoid.

contains rather than allSatisfy restores both. If you want the resume to stay broader than the migration, they can take different predicates; they only share one today because the shapes happened to match.

Checked and fine

These all looked like problems on a first pass and are not, so recording them rather than leaving you to guess what I looked at:

  • The migration respects an explicit choice. It reads the stored flag when the key exists and only computes a migrated value in the else, so someone who turns body composition off keeps it off. My first skim of the hunk missed the else and I had this wrong for a minute.
  • The read/write asymmetry is correctly reasoned. HealthKit reports share status but never read status, which is precisely why a read stage needs a persisted flag while the write-back stages can check live. That is a genuinely non-obvious distinction and the comment earns its length.
  • dietaryReadIds is not a new ask. .dietaryWater and .dietaryCaffeine are already in the read set on main; this renames them into a named constant. No new category enters the sheet, which matters on a PR whose whole argument is narrowing consent.
  • The Skin temperature is decoded and scored but never written to HealthKit #1366/fix(health): guard the HealthKit share set against read-only types (a launch-crash footgun) #1371 denylist survives and still filters every stage's write set, so sharing an Apple-reserved type stays structurally impossible rather than a launch crash waiting for a careless edit.
  • The rest of the iOS: write to Apple Health when an offload lands, not only on foreground entry (#1021) #1024 reconciliation is the shape I suggested back in August: keep the status read, drop the silent re-request.

Housekeeping

The description opens with two closing-verb lines. The first names the Health Connect issue this PR no longer touches, left from before the Apple-only rebase. Both want rewriting to plain references before merge, since a body auto-closes on merge whatever the squash message says. I will handle that at merge time if you would rather not.

The branch has gone dirty since your 09-14 push: 130 commits behind, and HealthKitBridge.swift has moved twenty times, including the open-night hold that landed today. The good news is the conflict is confined to that ONE file. AppleHealthView.swift and the 523-line catalogue both merge clean, so this is a smaller rebase than the distance suggests.

One sequencing note: #2298 rewrites much of the same file. Whichever of you goes second will have a real merge, so they should not both be taken in parallel.

Approving once allSatisfy becomes contains and the branch is rebased. This is careful work, and the staged-consent shape is right.

@ryanbr

ryanbr commented Sep 21, 2026

Copy link
Copy Markdown
Owner

Sequencing decided: this goes in first, ahead of #2298, which rewrites much of the same file.

So the rebase you do is the smaller one, and the next PR absorbs the merge rather than you doing it twice.

Recapping what it needs, unchanged from my review:

  1. allSatisfy to contains in nightlyVitalsWriteTypesGranted. This is the one that blocks. Requiring every nightly-vitals write type is what the comment you removed records as tried and rejected, and it silently drops write-back for anyone who declined a single type under the old bundled sheet.
  2. A rebase. 130 behind, but the conflict is confined to HealthKitBridge.swift; AppleHealthView.swift and the catalogue both merge clean, so it is a smaller job than the distance suggests. Note the open-night hold landed in that file today.
  3. The two linking lines at the top of the description. The first names the Health Connect issue. I have now retired that one on Split Health Connect import permissions by category #1509's merit, so the line points at something already settled and no longer describes this PR at all. The second is right in intent but wants to be a plain reference, since a description acts on merge whatever the squash message says. Say the word if you would rather I handle both at merge time.

Nothing else outstanding. The staged-consent shape is right and the read-versus-write reasoning in it is the part I would have got wrong writing this myself.

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.

iOS: split broad HealthKit permissions into staged consent toggles Privacy: split Health Connect import permissions by data category

2 participants