feat(auth): request YouVersion Platform permissions at sign-in (YPE-3707)#92
feat(auth): request YouVersion Platform permissions at sign-in (YPE-3707)#92Dustin-Kelley wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Cursor on behalf of Cam — review summary
praise: Intentional divergence from the original YPE-3707 AuthScope approach is correct and matches platform-sdk-react.
todo (blocking): Update the AGENTS Auth bullet to mention permissions?.
chore (blocking): Rebase onto main (branch is 1 commit behind — #89 i18n sync).
note (non-blocking): README shows permissions: ['highlights'] but the example app still uses scopes only (intentional per your note). Worth a one-liner in the PR or AGENTS that the example stays scopes-only until C3.
note (non-blocking): Provider-level AuthConfig.permissions (vs web's per-call signIn({ permissions }) / AuthButton prop) is fine for RN — consider one sentence in AGENTS Auth so partners don't hunt for a button prop.
Implementation, tests, changeset, and omit-when-empty for the three-state granted_permissions story all look solid.
| **UI** (`@youversion/platform-react-native-expo-ui`): `YouVersionProvider`, `BibleCard`, `BibleChapterPickerSheet`, `BibleReader`, `BibleReaderSettingsSheet`, `BibleTextView`, `BibleVersionPickerSheet`, `VerseOfTheDay`, and `YouVersionAuthButton` | ||
|
|
||
| **Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth`, `mmkvStorage`, and auth types (`AuthConfig`, `AuthScope`, `YVUserInfo`) | ||
| **Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth`, `mmkvStorage`, and auth types (`AuthConfig`, `AuthPermission`, `AuthScope`, `YVUserInfo`) |
There was a problem hiding this comment.
Cursor on behalf of Cam
todo (blocking): Update the AGENTS Auth bullet to include permissions?.
Exports correctly adds AuthPermission, but Auth still documents only auth: { redirectUri, scopes? }. Please change it to e.g. auth: { redirectUri, scopes?, permissions? } so AGENTS matches the public surface and the README example.
There was a problem hiding this comment.
Cursor on behalf of Cam — alignment review vs platform-sdk-react (permissions already on web + highlights context from youversion/platform-sdk-react#283)
praise: Wire contract is correct and aligned — same requested_permissions[], same permission string values, not OIDC scopes, omit when empty. Diverging from the ticket's AuthScope approach was the right call.
todo (blocking): AGENTS Auth bullet still documents only auth: { redirectUri, scopes? } — add permissions? (see inline).
chore (blocking): Rebase onto main (branch is 1 behind).
suggestion (docs,non-blocking): Document provider-level AuthConfig.permissions (vs web's per-call signIn / AuthButton prop) so partners don't hunt for a button prop.
note (non-blocking): Keep AuthPermission naming (pairs with AuthScope); do not rename to web's SignInWithYouVersionPermissionValues.
thought (non-blocking): For C3 (YPE-3709), reuse web's pure parseGrantedPermissions rather than reinventing.
Implementation, tests, changeset, and the three-state granted_permissions notes all look solid.
| * as being granted it — the user can deny on the consent screen and sign-in | ||
| * still succeeds; reading back what was granted is not yet supported. | ||
| */ | ||
| permissions?: readonly AuthPermission[] |
There was a problem hiding this comment.
Cursor on behalf of Cam
suggestion (docs,non-blocking): Document that RN configures permissions on the provider, not on AuthButton / signIn().
On platform-sdk-react, permissions is per-call — signIn({ permissions }) and a YouVersionAuthButton prop. Here it lives only on provider AuthConfig, and signIn() / AuthButton take none.
That's a reasonable RN choice (single PKCE path through the provider). Please add one sentence to the AGENTS Auth section so partners coming from web don't look for a button prop:
On RN,
permissionsis configured onYouVersionProvider'sauthconfig (not onYouVersionAuthButton/signIn()), unlike web.
No need to add a button prop in this PR unless we want call-site overrides later.
| * silently drops unknown values from `scope`, so passing a permission there grants | ||
| * nothing. | ||
| */ | ||
| export type AuthPermission = 'bibles' | 'highlights' | 'votd' | 'demographics' | 'bible_activity' |
There was a problem hiding this comment.
Cursor on behalf of Cam
note (non-blocking): Keep the AuthPermission type name — do not rename to match web.
Web uses SignInWithYouVersionPermission (const) → SignInWithYouVersionPermissionValues. Renaming RN to match would fight our existing AuthScope / AuthConfig idiom.
Wire string values already match web (bibles | highlights | votd | demographics | bible_activity) — that's the alignment that matters. Optional later: a const map for ergonomics (AuthPermission.highlights) mirroring web; not required for C1.
| // Omit the param entirely when there are none — don't emit an empty one. On the | ||
| // callback, absent `granted_permissions` means "none requested" while an empty | ||
| // value means "requested and denied"; emitting an empty param blurs the two. | ||
| for (const permission of [...new Set(args.permissions ?? [])].sort()) { |
There was a problem hiding this comment.
Cursor on behalf of Cam
praise: Omit-when-empty + the three-state granted_permissions comment is excellent foreshadowing for C3.
thought (non-blocking): For YPE-3709, port web's pure parseGrantedPermissions rather than reinventing.
Web already ships it in platform-sdk-react (packages/core/src/permissions.ts) — order-preserving dedupe, splits on ,/ . Prefer that helper when parsing the app-redirect grant (grant is on the first hop only, per your device notes).
note (non-blocking): Dedupe+sort here is stricter than web (web preserves input order) — prefer the stable URL; no change needed in this PR.
| **UI** (`@youversion/platform-react-native-expo-ui`): `YouVersionProvider`, `BibleCard`, `BibleChapterPickerSheet`, `BibleReader`, `BibleReaderSettingsSheet`, `BibleTextView`, `BibleVersionPickerSheet`, `VerseOfTheDay`, and `YouVersionAuthButton` | ||
|
|
||
| **Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth`, `mmkvStorage`, and auth types (`AuthConfig`, `AuthScope`, `YVUserInfo`) | ||
| **Core** (`@youversion/platform-react-native-expo-core`): `YouVersionProvider` (installation id + optional auth), `useYouVersion`, `useYVAuth`, `mmkvStorage`, and auth types (`AuthConfig`, `AuthPermission`, `AuthScope`, `YVUserInfo`) |
There was a problem hiding this comment.
Cursor on behalf of Cam
todo (blocking): Update the AGENTS Auth bullet to include permissions?.
Exports correctly lists AuthPermission, but Auth still documents only:
auth: { redirectUri, scopes? }
Please update to:
auth: { redirectUri, scopes?, permissions? }
suggestion (docs,non-blocking): While you're there, add a one-liner that RN configures permissions on the provider (not AuthButton) so partners from web don't hunt for a button prop.
| } | ||
| ``` | ||
|
|
||
| `permissions` lists YouVersion Platform permissions (`'bibles'`, `'highlights'`, `'votd'`, `'demographics'`, `'bible_activity'`) to ask for on the consent screen — these are not OIDC scopes, so keep them out of `scopes`. Today this only _requests_ the permission: the SDK does not yet report whether the user granted it, and the highlights APIs and the just-in-time grant flow land in a later release. |
There was a problem hiding this comment.
Cursor on behalf of Cam
note (non-blocking): Keep this caveat for this PR.
Partners otherwise assume grant is guaranteed after successful sign-in. Fine to drop/update when C3 lands.
nitpick (non-blocking): Optional soften if you want it shorter:
Today this only requests the permission; whether it was granted is not exposed yet (coming in a follow-up).
Also worth a quiet note somewhere (PR description or AGENTS) that the example app stays scopes-only until C3, so this README sample doesn't fight the runnable example.
Address review feedback: Auth bullet includes permissions?, notes RN provider config vs web AuthButton, and softens the README grant caveat. Co-authored-by: Cursor <cursoragent@cursor.com>
0db4dd9 to
aabd0d3
Compare
Closes YPE-3707 (C1 in the RN Expo Highlights epic, YPE-2894).
What
Adds an opt-in
permissionsfield to core'sAuthConfig, typed by a new exportedAuthPermissionunion:Configured values are sent on
/auth/authorizeas repeatedrequested_permissions[]params — deduped and sorted, and omitted entirely whenpermissionsis undefined or[].AuthScopeandDEFAULT_SCOPESare untouched, so this stays least-privilege: apps that never ask never receive.Core-only change. The UI package types
auth?: AuthConfigand passes it through, sopermissionsflows structurally with no UI edit and no UI version bump.With permission

With out permission

The ticket (and Notion locked decision #8) specify adding
'highlights'to theAuthScopeunion and sending it in thescopeparam. That does not work.highlightsis a YouVersion Platform permission, not an OIDC scope. The auth server silently drops unknown values fromscope. Verified on device: requesting and granting highlights still returnsscope=email openid profile.Implementing it as written would have type-checked, passed its own acceptance criteria, and granted nothing — surfacing much later as a 403 from
/v1/highlights.platform-sdk-reactalready ships the correct contract via a separatepermissionsargument, with an explicit "intentionally NOT OIDC scopes" comment inSignInWithYouVersionPKCE.ts.The Notion epic plan and the Linear ticket still carry the old premise. Not amended — noting it here so YPE-3709 doesn't inherit it.
Device verification — findings for YPE-3709 (C3)
Unit tests assert we build the URL we intended, not that the server honors it, so this was exercised end-to-end on a device (example-app edit made temporarily and reverted, not committed).
The consent screen does list highlights, and the permission is honored. Two findings C3 depends on:
1.
granted_permissionsarrives only on the app redirect. Our flow reads params twice — the app redirect fromopenAuthSessionAsync(result.url), then re-posts them to/auth/callbackand reads theLocationheader. The grant is present on the first and absent on the second. C3 must parse it before the second hop.2. It is a three-state signal, both branches measured:
null)granted_permissions=)granted_permissions=highlightsThis is why an empty
permissionsarray omits the param rather than emitting an empty one — emitting it would collapse "partner configured nothing" into "user denied".Deliberately deferred to YPE-3709 (C3)
Per locked decisions #5/#8 and C3's own "save the granted permission" AC:
granted_permissionshasPermission()/ granted-permission surface on auth statePOST /data-exchange/token→ hosted consent)Known consequence: this ships the request side only. If a user denies the permission, sign-in still reports success and the first symptom is a 403 downstream. Accepted for C1; C3 closes it.
Testing
Five new cases in
pkce-flow.test.ts: param present when configured; absent when unconfigured; absent when[]; deduped and sorted; and an explicit assertion that no permission value leaks intoscope— the failure mode described above.CI gates pass locally:
lint --max-warnings=0,lint:native-i18n,typecheck,test.🤖 Generated with Claude Code
Greptile Summary
This PR adds opt-in YouVersion Platform permission requests to the core sign-in flow. The main changes are:
AuthPermissiontype.AuthConfigwith optional permissions.requested_permissions[]authorization parameters.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
AuthConfig.permissionsfield.AuthPermissionfrom the authentication barrel.AuthPermissionfrom the core package entry point.Reviews (3): Last reviewed commit: "docs(auth): document provider-level perm..." | Re-trigger Greptile
Context used: