diff --git a/.changeset/auth-signed-in-requires-user-id.md b/.changeset/auth-signed-in-requires-user-id.md new file mode 100644 index 0000000..2ab86c8 --- /dev/null +++ b/.changeset/auth-signed-in-requires-user-id.md @@ -0,0 +1,8 @@ +--- +'@youversion/platform-react-native-expo-core': major +--- + +Signed-in sessions now always carry a non-empty YouVersion user id. + +- **`YVUserInfo.id`** is required (was optional). A session without a valid non-empty `sub` in the id_token is rejected at sign-in and cleared on cold start, including malformed stored id_tokens. +- **`getAccessToken()`** `{ status: 'ok', userId }` is now `string` (was `string | null`). When status is `'ok'`, the token and user id were read in the same synchronous block and both belong to the signed-in user. diff --git a/AGENTS.md b/AGENTS.md index 52d6866..c475786 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -183,7 +183,7 @@ UI `YouVersionProvider` wraps core and adds theme context + `NativeSheetProvider - The grant rides only on the **app redirect** — the `/auth/callback` `Location` hop drops it — so `pkce-flow.ts` parses it from `result.url` before that hop, and a test in `__tests__/pkce-flow.test.ts` pins the ordering. It is then cached per user in MMKV (redirect parsing in `auth/granted-permissions.ts`, the cache in `auth/granted-permissions-cache.ts`), seeded synchronously in a `useState` initializer so it is correct on the first render, and purged in `clearAuthState`. `AuthPermission` is an open union and cached values are kept verbatim, not filtered — filtering would turn a server-side addition into a silent denial. - `useYVAuth().requestPermissions(permissions)` is the **just-in-time grant** (data exchange): a signed-in user grants a permission on the spot, no sign-out. Mint (`POST /data-exchange/token`, 201) → hosted consent in an auth session → parse the return → merge into the grant cache. Resolves to a `DataExchangeOutcome` (`granted` / `cancel` / `failure` with `reason: 'not-signed-in' | 'not-permitted' | 'user-changed' | 'in-progress' | 'transient'`) and never throws. Permission-generic — nothing highlights-specific lives in `auth/data-exchange.ts`. - The grant **merges**, never replaces: a `highlights`-only consent must not erase a previously granted `votd`. `cancel` and `failure` never touch the cache. - - An **initiator guard** fails closed: an `AuthIdentity` (`{ sessionId, userId }`) is captured before minting and re-read after the browser returns; any difference discards the grant (`reason: 'user-changed'`). `sessionId` is a local counter compared only for equality, not a server-issued value; it moves only in `setIdentity` (sign-in and sign-out), so a token-only `setAuthState` leaves it alone and a mid-flow refresh passes. `userId` alone cannot carry the guard because `null` means both "signed out" and "signed in with no `sub`". A same-session id-less user passes deliberately — failing closed there locks those users out of the flow entirely. + - An **initiator guard** fails closed: an `AuthIdentity` (`{ sessionId, userId }`) is captured before minting and re-read after the browser returns; any difference discards the grant (`reason: 'user-changed'`). `sessionId` is a local counter compared only for equality, not a server-issued value; it moves only in `setIdentity` (sign-in and sign-out), so a token-only `setAuthState` leaves it alone and a mid-flow refresh passes. A signed-in session always carries a non-empty string `userId` from the id_token's `sub` — sign-in and cold-start bootstrap both reject tokens without one and clear orphan storage — so `userId: null` on the guard means signed out, not an id-less signed-in user. - **The guard is a backstop, not a defence against user action** — worth knowing before you either delete it as dead weight or trust it as a security boundary. Neither platform lets the user reach the app while the consent page is up (iOS is a modal sheet; on Android foregrounding resolves the auth session as `dismiss` first, ending the flow). The paths that _can_ land mid-flow are not user-driven — a revoked token tripping `clearAuthState`, or app code calling `signOut` from async work — and all of them end signed out, where `saveGrantedPermissions` already refuses the null `userId`. What the guard actually buys: a truthful **outcome** (never `granted` for a user who has left, which is what consumers branch on) and a `requestDataExchange` that is correct on its own terms instead of depending on a null check in `granted-permissions-cache.ts` that nothing links to it. - `status: 'granted'` reports what the server granted, which may not be everything asked for. Check the returned list (or `hasPermission`) for the permission you needed. - **Never throws is load-bearing and easy to break.** Every doc for this flow tells consumers not to `try`/`catch`, so each `await` that can reject needs a guard returning a `transient` failure: the mint (in `data-exchange-api.ts`), `WebBrowser.openAuthSessionAsync` (which rejects on a session already open, a missing native module, or no Android activity for the intent), and `getOrSetInstallationId()` in the provider. Tests pin all three. diff --git a/packages/core/src/auth/__tests__/auth-provider.test.tsx b/packages/core/src/auth/__tests__/auth-provider.test.tsx index 6fd5272..bc93493 100644 --- a/packages/core/src/auth/__tests__/auth-provider.test.tsx +++ b/packages/core/src/auth/__tests__/auth-provider.test.tsx @@ -89,9 +89,34 @@ function makeJwt(payload: unknown): string { const noStoredTokens = { accessToken: null, refreshToken: null, + idToken: null, expiryDate: null, } +const clearedTokens = { accessToken: null, refreshToken: null, idToken: null, expiryDate: null } + +function storedSessionTokens( + overrides: Partial<{ + accessToken: string + refreshToken: string + idToken: string | null + expiryDate: Date + }> = {}, +) { + return { + accessToken: 'stored-access', + refreshToken: 'stored-refresh', + idToken: validTokens.id_token, + expiryDate: new Date(Date.now() + 60 * 60 * 1000), + ...overrides, + } +} + +function seedSignedInColdStart(userInfo = adaUserInfo) { + mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify(userInfo)) + mockLoadTokens.mockResolvedValue(storedSessionTokens()) +} + const validTokens = { access_token: 'new-access', refresh_token: 'new-refresh', @@ -240,12 +265,7 @@ describe('AuthProvider — mount', () => { }) it('hydrates state from stored tokens and skips refresh when not near expiry', async () => { - mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify(adaUserInfo)) - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + seedSignedInColdStart() render( @@ -265,11 +285,7 @@ describe('AuthProvider — mount', () => { MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 'u1', name: 'Ada', avatarUrl: 'https://none/' }), ) - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens()) render( @@ -281,16 +297,12 @@ describe('AuthProvider — mount', () => { expect(JSON.parse(getText('userInfo')).avatarUrl).toBeUndefined() }) - it('drops wrong-typed fields from a tampered/corrupt cached userInfo instead of trusting them', async () => { + it('drops cached userInfo without a string id and clears the session when no id_token can repair it', async () => { mockMmkv.set( MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 42, name: { first: 'Ada' }, email: 'ada@example.com' }), ) - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ idToken: null })) render( @@ -299,21 +311,15 @@ describe('AuthProvider — mount', () => { ) await waitFor(() => expect(getText('isLoading')).toBe('false')) - expect(JSON.parse(getText('userInfo'))).toEqual({ - id: undefined, - name: undefined, - email: 'ada@example.com', - avatarUrl: undefined, - }) + expect(getText('isAuthenticated')).toBe('false') + expect(getText('userInfo')).toBe('null') + expect(getText('accessToken')).toBe('null') + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) }) - it('returns null userInfo when cached JSON is a non-object (e.g. "null")', async () => { + it('returns null userInfo when cached JSON is a non-object and clears tokens without an id', async () => { mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify(null)) - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ idToken: null })) render( @@ -323,14 +329,14 @@ describe('AuthProvider — mount', () => { await waitFor(() => expect(getText('isLoading')).toBe('false')) expect(getText('userInfo')).toBe('null') + expect(getText('isAuthenticated')).toBe('false') }) it('triggers a refresh when the stored token is expired and applies the new tokens', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stale-access', - refreshToken: 'stale-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) mockRefreshTokens.mockResolvedValue(validTokens) render( @@ -344,17 +350,16 @@ describe('AuthProvider — mount', () => { expect(mockRefreshTokens).toHaveBeenCalledWith({ apiHost: 'api.example.com', appKey: 'appkey', - refreshToken: 'stale-refresh', + refreshToken: 'stored-refresh', }) expect(getText('accessToken')).toBe('new-access') }) it('sets error when refreshing an expired stored token fails', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stale-access', - refreshToken: 'stale-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) mockRefreshTokens.mockRejectedValue(new Error('refresh failed')) render( @@ -383,6 +388,117 @@ describe('AuthProvider — mount', () => { }) }) +describe('AuthProvider — signed-in user id invariant', () => { + it('clears the session on cold start when tokens exist but neither cache nor id_token yields an id', async () => { + mockLoadTokens.mockResolvedValue(storedSessionTokens({ idToken: null })) + + render( + + + , + ) + + await waitFor(() => expect(getText('isLoading')).toBe('false')) + expect(getText('isAuthenticated')).toBe('false') + expect(getText('accessToken')).toBe('null') + expect(getText('userInfo')).toBe('null') + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) + expect(mockRefreshTokens).not.toHaveBeenCalled() + }) + + it('repairs the session user from a stored id_token sub when cached userInfo is missing', async () => { + mockLoadTokens.mockResolvedValue(storedSessionTokens()) + + render( + + + , + ) + + await waitFor(() => expect(getText('isLoading')).toBe('false')) + expect(getText('isAuthenticated')).toBe('true') + expect(JSON.parse(getText('userInfo'))).toEqual({ + id: 'u1', + name: 'Ada', + email: undefined, + avatarUrl: undefined, + }) + }) + + it('clears the session on cold start when the stored id_token is malformed', async () => { + mockLoadTokens.mockResolvedValue(storedSessionTokens({ idToken: 'not-a-jwt' })) + + render( + + + , + ) + + await waitFor(() => expect(getText('isLoading')).toBe('false')) + expect(getText('isAuthenticated')).toBe('false') + expect(getText('userInfo')).toBe('null') + expect(getText('accessToken')).toBe('null') + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) + expect(mockRefreshTokens).not.toHaveBeenCalled() + }) + + it('clears the session on cold start when the stored id_token has an empty sub', async () => { + mockLoadTokens.mockResolvedValue(storedSessionTokens({ idToken: makeJwt({ sub: '' }) })) + + render( + + + , + ) + + await waitFor(() => expect(getText('isLoading')).toBe('false')) + expect(getText('isAuthenticated')).toBe('false') + expect(getText('userInfo')).toBe('null') + expect(getText('accessToken')).toBe('null') + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) + expect(mockRefreshTokens).not.toHaveBeenCalled() + }) + + it('reports isAuthenticated false when a token is present in memory but userInfo has no id', async () => { + mockLoadTokens.mockResolvedValue(noStoredTokens) + + render( + + + , + ) + await waitFor(() => expect(getText('isLoading')).toBe('false')) + + mockSignInWithPKCE.mockResolvedValue({ + kind: 'success', + tokens: validTokens, + userInfo: { id: undefined as unknown as string }, + grantedPermissions: null, + }) + fireEvent.press(screen.getByTestId('signIn')) + await waitFor(() => + expect(getText('signInOutcome')).toBe('rejected: Sign-in did not return a user id'), + ) + + expect(getText('accessToken')).toBe('null') + expect(getText('isAuthenticated')).toBe('false') + }) + + it('never exposes userInfo without a string id', async () => { + seedSignedInColdStart() + + render( + + + , + ) + + await waitFor(() => expect(getText('isLoading')).toBe('false')) + const parsed = JSON.parse(getText('userInfo')) as { id: string } + expect(typeof parsed.id).toBe('string') + }) +}) + describe('AuthProvider — signIn', () => { beforeEach(() => { mockLoadTokens.mockResolvedValue(noStoredTokens) @@ -447,6 +563,32 @@ describe('AuthProvider — signIn', () => { expect(getText('error')).toBe('PKCE blew up') expect(getText('isAuthenticated')).toBe('false') }) + + it('does not commit when sign-in returns user info without an id', async () => { + mockSignInWithPKCE.mockResolvedValue({ + kind: 'success', + tokens: validTokens, + userInfo: { id: undefined as unknown as string, name: 'Ada' }, + grantedPermissions: null, + }) + + render( + + + , + ) + await waitFor(() => expect(getText('isLoading')).toBe('false')) + mockSaveTokens.mockClear() + + fireEvent.press(screen.getByTestId('signIn')) + + await waitFor(() => + expect(getText('signInOutcome')).toBe('rejected: Sign-in did not return a user id'), + ) + expect(getText('isAuthenticated')).toBe('false') + expect(getText('accessToken')).toBe('null') + expect(mockSaveTokens).not.toHaveBeenCalled() + }) }) describe('AuthProvider — signOut', () => { @@ -454,11 +596,7 @@ describe('AuthProvider — signOut', () => { mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 'u1' })) setCachedHighlights('u1', JHN3, [{ version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }]) parkWrite('u1', JHN3) - mockLoadTokens.mockResolvedValue({ - accessToken: 'a', - refreshToken: 'r', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ accessToken: 'a', refreshToken: 'r' })) render( @@ -472,11 +610,7 @@ describe('AuthProvider — signOut', () => { await waitFor(() => expect(getText('isAuthenticated')).toBe('false')) expect(getText('accessToken')).toBe('null') expect(getText('userInfo')).toBe('null') - expect(mockSaveTokens).toHaveBeenCalledWith({ - accessToken: null, - refreshToken: null, - expiryDate: null, - }) + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) expect(mockMmkv.has(MMKV_AUTH_KEYS.cachedUserInfo)).toBe(false) expect(getCachedHighlights('u1', JHN3)).toBeNull() expect(listQueuedScopes('u1')).toEqual([]) @@ -488,11 +622,8 @@ describe('AuthProvider — signOut', () => { parkWrite('u1', JHN3) parkWrite('u1', GEN1) parkWrite('u2', PSA23) - mockLoadTokens.mockResolvedValue({ - accessToken: 'a', - refreshToken: 'r', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 'u1' })) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ accessToken: 'a', refreshToken: 'r' })) render( @@ -513,11 +644,7 @@ describe('AuthProvider — signOut', () => { it('still signs out when the cache purges cannot reach the store', async () => { mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 'u1' })) parkWrite('u1', JHN3) - mockLoadTokens.mockResolvedValue({ - accessToken: 'a', - refreshToken: 'r', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ accessToken: 'a', refreshToken: 'r' })) render( @@ -532,23 +659,15 @@ describe('AuthProvider — signOut', () => { await waitFor(() => expect(getText('isAuthenticated')).toBe('false')) expect(getText('accessToken')).toBe('null') expect(getText('userInfo')).toBe('null') - expect(mockSaveTokens).toHaveBeenCalledWith({ - accessToken: null, - refreshToken: null, - expiryDate: null, - }) + expect(mockSaveTokens).toHaveBeenCalledWith(clearedTokens) }) }) describe('AuthProvider — refresh failure policy', () => { - const expiredStored = { - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() - 1000), - } - const clearedTokens = { accessToken: null, refreshToken: null, expiryDate: null } + const expiredStored = storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }) it('keeps tokens on a transient error (e.g. network failure) so the user can retry', async () => { + seedSignedInColdStart() mockLoadTokens.mockResolvedValue(expiredStored) mockRefreshTokens.mockRejectedValue(new Error('Network request failed')) @@ -566,6 +685,7 @@ describe('AuthProvider — refresh failure policy', () => { }) it('clears tokens when the refresh token is revoked (TokenEndpointError 401)', async () => { + mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify({ id: 'user-1' })) setCachedHighlights('user-1', JHN3, [ { version_id: 111, passage_id: 'JHN.3.16', color: 'fffe00' }, ]) @@ -591,11 +711,14 @@ describe('AuthProvider — refresh failure policy', () => { describe('AuthProvider — refresh lock', () => { it('prevents concurrent refresh calls (only one HTTP call while one is in flight)', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'a', - refreshToken: 'r', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ + accessToken: 'a', + refreshToken: 'r', + expiryDate: new Date(Date.now() - 1000), + }), + ) let resolveRefresh: (v: TokenResponse) => void = () => {} mockRefreshTokens.mockReturnValue( @@ -625,11 +748,14 @@ describe('AuthProvider — refresh lock', () => { }) it('joins an in-flight refresh instead of resolving on the stale token', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'expired-access', - refreshToken: 'r', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ + accessToken: 'expired-access', + refreshToken: 'r', + expiryDate: new Date(Date.now() - 1000), + }), + ) let resolveRefresh: (v: TokenResponse) => void = () => {} mockRefreshTokens.mockReturnValue( @@ -678,8 +804,6 @@ describe('AuthProvider — refresh lock', () => { }) describe('AuthProvider — getAccessToken', () => { - const clearedTokens = { accessToken: null, refreshToken: null, expiryDate: null } - function renderProvider() { return render( @@ -689,27 +813,22 @@ describe('AuthProvider — getAccessToken', () => { } it('resolves ok with the current token, with no refresh call, when it is beyond the leeway', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + seedSignedInColdStart() renderProvider() await waitFor(() => expect(getText('isLoading')).toBe('false')) const result = await act(async () => latestAuth!.getAccessToken()) - expect(result).toEqual({ status: 'ok', token: 'stored-access', userId: null }) + expect(result).toEqual({ status: 'ok', token: 'stored-access', userId: 'u1' }) expect(mockRefreshTokens).not.toHaveBeenCalled() }) it('refreshes an expired token and resolves ok with the new one', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) // Bootstrap's refresh lands a token that is *still* at expiry, so the // accessor's own leeway check genuinely triggers the second refresh. mockRefreshTokens.mockResolvedValueOnce({ @@ -724,16 +843,15 @@ describe('AuthProvider — getAccessToken', () => { mockRefreshTokens.mockResolvedValueOnce({ ...validTokens, access_token: 'fresh-access' }) const result = await act(async () => latestAuth!.getAccessToken()) - expect(result).toEqual({ status: 'ok', token: 'fresh-access', userId: null }) + expect(result).toEqual({ status: 'ok', token: 'fresh-access', userId: 'u1' }) expect(mockRefreshTokens).toHaveBeenCalledTimes(2) }) it('reports refresh-failed on a transient refresh error, keeping tokens and the session', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) mockRefreshTokens.mockRejectedValue(new Error('Network request failed')) renderProvider() @@ -752,11 +870,10 @@ describe('AuthProvider — getAccessToken', () => { // The leeway triggers the refresh; it does not decide usable. A token 30s from // expiry still works, so a failed refresh must hand it over, not refuse it. it('resolves ok with a token inside the leeway window that the refresh could not replace', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() + 30 * 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() + 30 * 1000) }), + ) mockRefreshTokens.mockRejectedValue(new Error('Network request failed')) renderProvider() @@ -764,17 +881,16 @@ describe('AuthProvider — getAccessToken', () => { const result = await act(async () => latestAuth!.getAccessToken()) - expect(result).toEqual({ status: 'ok', token: 'stored-access', userId: null }) + expect(result).toEqual({ status: 'ok', token: 'stored-access', userId: 'u1' }) // Pins the failed-refresh path, not the fresh-token shortcut. expect(mockRefreshTokens).toHaveBeenCalled() }) it('reports signed-out when the refresh finds the token revoked and clears the session', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) // Bootstrap keeps the token stale; the accessor's refresh hits the revocation. mockRefreshTokens .mockResolvedValueOnce({ ...validTokens, access_token: 'still-stale', expires_in: '0' }) @@ -807,11 +923,12 @@ describe('AuthProvider — getAccessToken', () => { // the caller is awaiting cannot hand it a token attributed to the old user. it('reports the signed-in user alongside the token, updated by a sign-in as somebody else', async () => { mockMmkv.set(MMKV_AUTH_KEYS.cachedUserInfo, JSON.stringify(adaUserInfo)) - mockLoadTokens.mockResolvedValue({ - accessToken: 'ada-access', - refreshToken: 'ada-refresh', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ + accessToken: 'ada-access', + refreshToken: 'ada-refresh', + }), + ) renderProvider() await waitFor(() => expect(getText('isLoading')).toBe('false')) @@ -839,11 +956,14 @@ describe('AuthProvider — getAccessToken', () => { }) it('joins an in-flight refresh: concurrent callers share one HTTP call and get the new token', async () => { - mockLoadTokens.mockResolvedValue({ - accessToken: 'expired-access', - refreshToken: 'r', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ + accessToken: 'expired-access', + refreshToken: 'r', + expiryDate: new Date(Date.now() - 1000), + }), + ) let resolveRefresh: (v: TokenResponse) => void = () => {} mockRefreshTokens.mockReturnValue( @@ -867,8 +987,8 @@ describe('AuthProvider — getAccessToken', () => { await Promise.all([first, second]) }) - expect(await first).toEqual({ status: 'ok', token: 'new-access', userId: null }) - expect(await second).toEqual({ status: 'ok', token: 'new-access', userId: null }) + expect(await first).toEqual({ status: 'ok', token: 'new-access', userId: 'u1' }) + expect(await second).toEqual({ status: 'ok', token: 'new-access', userId: 'u1' }) expect(mockRefreshTokens).toHaveBeenCalledTimes(1) }) }) @@ -955,11 +1075,7 @@ describe('AuthProvider — granted permissions', () => { MMKV_AUTH_KEYS.grantedPermissions, JSON.stringify({ userId: 'u1', permissions: ['highlights'] }), ) - mockLoadTokens.mockResolvedValue({ - accessToken: 'a', - refreshToken: 'r', - expiryDate: new Date(Date.now() + 60 * 60 * 1000), - }) + mockLoadTokens.mockResolvedValue(storedSessionTokens({ accessToken: 'a', refreshToken: 'r' })) render( @@ -1355,11 +1471,10 @@ describe('AuthProvider — requestPermissions', () => { * short-circuiting on the leeway check. */ async function signInWithStaleToken() { - mockLoadTokens.mockResolvedValue({ - accessToken: 'stored-access', - refreshToken: 'stored-refresh', - expiryDate: new Date(Date.now() - 1000), - }) + seedSignedInColdStart() + mockLoadTokens.mockResolvedValue( + storedSessionTokens({ expiryDate: new Date(Date.now() - 1000) }), + ) mockRefreshTokens.mockResolvedValueOnce({ ...validTokens, access_token: 'stale-access', diff --git a/packages/core/src/auth/__tests__/id-token.test.ts b/packages/core/src/auth/__tests__/id-token.test.ts index 5c635d4..185c089 100644 --- a/packages/core/src/auth/__tests__/id-token.test.ts +++ b/packages/core/src/auth/__tests__/id-token.test.ts @@ -69,14 +69,10 @@ describe('deriveUserInfo', () => { }) }) - it('returns undefined for non-string field values', () => { - const jwt = makeJwt({ sub: 123, name: null, email: { x: 1 }, profile_picture: false }) - expect(deriveUserInfo(jwt)).toEqual({ - id: undefined, - name: undefined, - email: undefined, - avatarUrl: undefined, - }) + it('returns null when sub is missing, not a string, or empty', () => { + expect(deriveUserInfo(makeJwt({ name: 'Ada' }))).toBeNull() + expect(deriveUserInfo(makeJwt({ sub: 123, name: null, email: { x: 1 } }))).toBeNull() + expect(deriveUserInfo(makeJwt({ sub: '' }))).toBeNull() }) it('ignores extra claims', () => { @@ -91,7 +87,7 @@ describe('deriveUserInfo', () => { it('drops a sentinel profile_picture URL (backend "no photo" placeholder)', () => { const jwt = makeJwt({ sub: 'u1', profile_picture: 'https://none/' }) - expect(deriveUserInfo(jwt).avatarUrl).toBeUndefined() + expect(deriveUserInfo(jwt)?.avatarUrl).toBeUndefined() }) }) diff --git a/packages/core/src/auth/__tests__/pkce-flow.test.ts b/packages/core/src/auth/__tests__/pkce-flow.test.ts index ebeeecb..c8c2f8d 100644 --- a/packages/core/src/auth/__tests__/pkce-flow.test.ts +++ b/packages/core/src/auth/__tests__/pkce-flow.test.ts @@ -299,6 +299,30 @@ describe('signInWithPKCE — id_token validation', () => { 'Nonce mismatch - possible id_token replay', ) }) + + it('throws when the id_token has no string sub claim', async () => { + arrangeHappyPath() + mockExchange.mockResolvedValue({ + access_token: 'a', + refresh_token: 'r', + id_token: makeJwt({ nonce: 'NONCE', sub: 123 }), + expires_in: '3600', + token_type: 'Bearer', + }) + await expect(signInWithPKCE(defaultProps())).rejects.toThrow('id_token missing sub claim') + }) + + it('throws when the id_token has an empty sub claim', async () => { + arrangeHappyPath() + mockExchange.mockResolvedValue({ + access_token: 'a', + refresh_token: 'r', + id_token: makeJwt({ nonce: 'NONCE', sub: '' }), + expires_in: '3600', + token_type: 'Bearer', + }) + await expect(signInWithPKCE(defaultProps())).rejects.toThrow('id_token missing sub claim') + }) }) describe('signInWithPKCE — happy path', () => { diff --git a/packages/core/src/auth/__tests__/token-storage.test.ts b/packages/core/src/auth/__tests__/token-storage.test.ts index 70dd648..3824fe9 100644 --- a/packages/core/src/auth/__tests__/token-storage.test.ts +++ b/packages/core/src/auth/__tests__/token-storage.test.ts @@ -33,6 +33,7 @@ jest.mock('../../storage/mmkv-storage', () => ({ const fullTokens: StoredTokens = { accessToken: 'access', refreshToken: 'refresh', + idToken: 'id.jwt.token', expiryDate: new Date('2030-01-01T00:00:00.000Z'), } @@ -47,6 +48,7 @@ describe('saveTokens', () => { await saveTokens(fullTokens) expect(secureStorage.set).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.accessToken, 'access') expect(secureStorage.set).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.refreshToken, 'refresh') + expect(secureStorage.set).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.idToken, 'id.jwt.token') }) it('writes expiryDate as an ISO string under the MMKV_AUTH key', async () => { @@ -58,9 +60,10 @@ describe('saveTokens', () => { }) it('removes each secure value when its token is null', async () => { - await saveTokens({ accessToken: null, refreshToken: null, expiryDate: null }) + await saveTokens({ accessToken: null, refreshToken: null, idToken: null, expiryDate: null }) expect(secureStorage.remove).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.accessToken) expect(secureStorage.remove).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.refreshToken) + expect(secureStorage.remove).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.idToken) expect(secureStorage.set).not.toHaveBeenCalled() }) @@ -78,7 +81,7 @@ describe('saveTokens', () => { }) await expect( - saveTokens({ accessToken: null, refreshToken: null, expiryDate: null }), + saveTokens({ accessToken: null, refreshToken: null, idToken: null, expiryDate: null }), ).resolves.toBeUndefined() expect(secureStorage.remove).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.accessToken) expect(secureStorage.remove).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.refreshToken) @@ -98,6 +101,7 @@ describe('saveTokens', () => { await saveTokens({ accessToken: 'a', refreshToken: null, + idToken: null, expiryDate: new Date('2030-01-01T00:00:00.000Z'), }) expect(secureStorage.set).toHaveBeenCalledWith(SECURE_STORAGE_KEYS.accessToken, 'a') @@ -115,6 +119,7 @@ describe('loadTokens', () => { expect(await loadTokens()).toEqual({ accessToken: null, refreshToken: null, + idToken: null, expiryDate: null, }) }) diff --git a/packages/core/src/auth/auth-context.tsx b/packages/core/src/auth/auth-context.tsx index b12d905..b140f25 100644 --- a/packages/core/src/auth/auth-context.tsx +++ b/packages/core/src/auth/auth-context.tsx @@ -15,7 +15,7 @@ import type { AuthPermission, YVUserInfo } from './types' * lags the token by a render and can pass an owner check it should have failed. */ export type AccessTokenResult = - | { status: 'ok'; token: string; userId: string | null } + | { status: 'ok'; token: string; userId: string } | { status: 'unavailable'; reason: 'signed-out' | 'refresh-failed' } export type AuthContextValue = { diff --git a/packages/core/src/auth/auth-provider.tsx b/packages/core/src/auth/auth-provider.tsx index 8bde54c..5cfff22 100644 --- a/packages/core/src/auth/auth-provider.tsx +++ b/packages/core/src/auth/auth-provider.tsx @@ -16,7 +16,7 @@ import { saveGrantedPermissions, } from './granted-permissions-cache' import { refreshTokens, TokenEndpointError } from './http' -import { sanitizeAvatarUrl } from './id-token' +import { deriveUserInfo, sanitizeAvatarUrl } from './id-token' import { signInWithPKCE } from './pkce-flow' import { loadTokens, saveTokens, type StoredTokens } from './token-storage' import type { AuthConfig, AuthPermission, YVUserInfo } from './types' @@ -59,16 +59,18 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // token it must send is the one that refresh just wrote, not the one this // render captured. const accessTokenRef = useRef(null) + const idTokenRef = useRef(null) // Latest identity, for a read that has to outlive a render: the data-exchange // initiator guard needs who is signed in *now*, once the browser comes back, // not who was captured in the closure when the flow started. // // The session id counts identity transitions — sign-in and sign-out, never a - // token refresh — so the guard can tell "signed out" from "signed in without - // an id", which a null `userInfo.id` alone cannot. Both are written together - // by `setIdentity` so they can never disagree; an effect would leave a window - // where the session id has moved and the id has not. + // token refresh — for the data-exchange initiator guard. A signed-in session + // always carries a non-empty `userId`; `userId: null` on the guard means + // signed out. Both are written together by `setIdentity` so they can never + // disagree; an effect would leave a window where the session id has moved + // and the id has not. const userInfoRef = useRef(userInfo) const sessionIdRef = useRef(0) @@ -99,10 +101,15 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth const setAuthState = useCallback( async (tokens: StoredTokens, user?: YVUserInfo) => { + if (user !== undefined && typeof user.id !== 'string') { + throw new Error('Cannot commit session without user id') + } + await saveTokens(tokens) expiryRef.current = tokens.expiryDate refreshTokenRef.current = tokens.refreshToken accessTokenRef.current = tokens.accessToken + idTokenRef.current = tokens.idToken setAccessToken(tokens.accessToken) if (user) { @@ -139,10 +146,11 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth expiryRef.current = null refreshTokenRef.current = null accessTokenRef.current = null + idTokenRef.current = null setAccessToken(null) setIdentity(null) setError(null) - await saveTokens({ accessToken: null, refreshToken: null, expiryDate: null }) + await saveTokens({ accessToken: null, refreshToken: null, idToken: null, expiryDate: null }) }, [invalidatePermissions, setIdentity]) const refreshToken = useCallback( @@ -181,6 +189,7 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth accessToken: response.access_token, refreshToken: response.refresh_token, expiryDate: new Date(Date.now() + Number(response.expires_in) * 1000), + idToken: typeof response.id_token === 'string' ? response.id_token : idTokenRef.current, }) } catch (e) { if (e instanceof TokenEndpointError && e.isRevoked) { @@ -232,8 +241,13 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth const { setAuthState, refreshToken, clearAuthState } = authActionsRef.current if (stored.refreshToken) { - await setAuthState(stored) - await refreshToken() + const user = resolveSessionUser(loadCachedUserInfo(), stored.idToken) + if (user === null) { + await clearAuthState() + } else { + await setAuthState(stored, user) + await refreshToken() + } } else { await clearAuthState() } @@ -277,16 +291,20 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth if (result.kind === 'cancel') { return } + if (typeof result.userInfo.id !== 'string') { + throw new Error('Sign-in did not return a user id') + } await setAuthState( { accessToken: result.tokens.access_token, refreshToken: result.tokens.refresh_token, + idToken: result.tokens.id_token ?? null, expiryDate: new Date(Date.now() + Number(result.tokens.expires_in) * 1000), }, result.userInfo, ) - const nextUserId = result.userInfo.id ?? null + const nextUserId = result.userInfo.id if (result.grantedPermissions != null) { saveGrantedPermissions(nextUserId, result.grantedPermissions) setGrantedPermissions(result.grantedPermissions) @@ -344,8 +362,8 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth // write both, so a caller checking the owner it captured cannot be handed a // token from the other side of a sign-in. const token = accessTokenRef.current - const userId = userInfoRef.current?.id ?? null - if (refreshTokenRef.current === null || token === null) { + const userId = userInfoRef.current?.id + if (refreshTokenRef.current === null || token === null || typeof userId !== 'string') { return { status: 'unavailable', reason: 'signed-out' } } @@ -501,7 +519,7 @@ export default function AuthProvider({ config, appKey, apiHost, children }: Auth const value: AuthContextValue = useMemo( () => ({ - isAuthenticated: accessToken !== null, + isAuthenticated: accessToken !== null && typeof userInfo?.id === 'string', accessToken, userInfo, error, @@ -548,19 +566,29 @@ function permissionKey(permissions: readonly AuthPermission[]): string { } // Validate untrusted cached JSON instead of blindly casting it to YVUserInfo. -// The cache can predate the current schema, be hand-tampered, or be corrupt, so -// each identity field falls back to undefined if it isn't a string rather than -// trusting `as` — a corrupt `id` won't discard a valid `email`. avatarUrl is -// left unknown here and run through sanitizeAvatarUrl below: it not only enforces -// the type but also drops placeholders (e.g. "https://none/") persisted by a -// build predating sanitizeAvatarUrl, since deriveUserInfo only runs at sign-in. +// The cache can predate the current schema, be hand-tampered, or be corrupt. +// Records without a string `id` are dropped — a signed-in session always has one. const cachedUserInfoSchema = z.object({ - id: z.string().optional().catch(undefined), + id: z.string().min(1), name: z.string().optional().catch(undefined), email: z.string().optional().catch(undefined), avatarUrl: z.unknown().optional(), }) +function resolveSessionUser(cached: YVUserInfo | null, idToken: string | null): YVUserInfo | null { + if (cached !== null) { + return cached + } + if (idToken === null) { + return null + } + try { + return deriveUserInfo(idToken) + } catch { + return null + } +} + function loadCachedUserInfo(): YVUserInfo | null { try { const userJson = mmkvStorage.getString(MMKV_AUTH_KEYS.cachedUserInfo) diff --git a/packages/core/src/auth/constants.ts b/packages/core/src/auth/constants.ts index d6fdb67..495824b 100644 --- a/packages/core/src/auth/constants.ts +++ b/packages/core/src/auth/constants.ts @@ -1,6 +1,7 @@ export const SECURE_STORAGE_KEYS = { accessToken: 'yvp.accessToken', refreshToken: 'yvp.refreshToken', + idToken: 'yvp.idToken', } as const export const MMKV_AUTH_KEYS = { diff --git a/packages/core/src/auth/id-token.ts b/packages/core/src/auth/id-token.ts index 0f913de..043ff09 100644 --- a/packages/core/src/auth/id-token.ts +++ b/packages/core/src/auth/id-token.ts @@ -20,11 +20,15 @@ export function decodeIdToken(jwt: string): IdTokenPayload { return JSON.parse(payloadJson) } -// Convenience: produce the YVUserInfo shape our hook returns. -export function deriveUserInfo(idToken: string): YVUserInfo { +// Convenience: produce the YVUserInfo shape our hook returns. Returns null when +// the id_token carries no non-empty string `sub` — a session must not commit without one. +export function deriveUserInfo(idToken: string): YVUserInfo | null { const p = decodeIdToken(idToken) + if (typeof p.sub !== 'string' || p.sub.length === 0) { + return null + } return { - id: typeof p.sub === 'string' ? p.sub : undefined, + id: p.sub, name: typeof p.name === 'string' ? p.name : undefined, email: typeof p.email === 'string' ? p.email : undefined, avatarUrl: sanitizeAvatarUrl(p.profile_picture), diff --git a/packages/core/src/auth/pkce-flow.ts b/packages/core/src/auth/pkce-flow.ts index 808924b..d0033be 100644 --- a/packages/core/src/auth/pkce-flow.ts +++ b/packages/core/src/auth/pkce-flow.ts @@ -109,7 +109,12 @@ export async function signInWithPKCE({ throw new Error('Nonce mismatch - possible id_token replay') } - return { kind: 'success', tokens, userInfo: deriveUserInfo(tokens.id_token), grantedPermissions } + const userInfo = deriveUserInfo(tokens.id_token) + if (userInfo === null) { + throw new Error('id_token missing sub claim') + } + + return { kind: 'success', tokens, userInfo, grantedPermissions } } async function obtainCodeFromCallback({ diff --git a/packages/core/src/auth/token-storage.ts b/packages/core/src/auth/token-storage.ts index d2a3241..ecf0f7b 100644 --- a/packages/core/src/auth/token-storage.ts +++ b/packages/core/src/auth/token-storage.ts @@ -5,6 +5,7 @@ import { MMKV_AUTH_KEYS, SECURE_STORAGE_KEYS } from './constants' export type StoredTokens = { accessToken: string | null refreshToken: string | null + idToken: string | null expiryDate: Date | null } @@ -14,6 +15,7 @@ export async function saveTokens(tokens: StoredTokens): Promise { await Promise.all([ writeSecureValue(SECURE_STORAGE_KEYS.accessToken, tokens.accessToken), writeSecureValue(SECURE_STORAGE_KEYS.refreshToken, tokens.refreshToken), + writeSecureValue(SECURE_STORAGE_KEYS.idToken, tokens.idToken), ]) writeExpiry(tokens.expiryDate) } @@ -35,15 +37,17 @@ function writeExpiry(expiryDate: Date | null): void { } export async function loadTokens(): Promise { - const [accessToken, refreshToken] = await Promise.all([ + const [accessToken, refreshToken, idToken] = await Promise.all([ secureStorage.get(SECURE_STORAGE_KEYS.accessToken), secureStorage.get(SECURE_STORAGE_KEYS.refreshToken), + secureStorage.get(SECURE_STORAGE_KEYS.idToken), ]) const expiryISO = mmkvStorage.getString(MMKV_AUTH_KEYS.expiryDateISO) return { accessToken, refreshToken, + idToken, expiryDate: expiryISO ? new Date(expiryISO) : null, } } diff --git a/packages/core/src/auth/types.ts b/packages/core/src/auth/types.ts index 9cef268..dbe5d39 100644 --- a/packages/core/src/auth/types.ts +++ b/packages/core/src/auth/types.ts @@ -38,7 +38,7 @@ export type AuthConfig = { } export type YVUserInfo = { - id?: string + id: string name?: string email?: string avatarUrl?: string // resolved URL, not the {width} template the web SDK exposes diff --git a/packages/core/src/highlights/__tests__/use-highlights.test.tsx b/packages/core/src/highlights/__tests__/use-highlights.test.tsx index 4c9fa84..a95c821 100644 --- a/packages/core/src/highlights/__tests__/use-highlights.test.tsx +++ b/packages/core/src/highlights/__tests__/use-highlights.test.tsx @@ -4,6 +4,7 @@ import { AppState, Text, type AppStateStatus } from 'react-native' import type { ReactNode } from 'react' import { AuthContext, type AccessTokenResult, type AuthContextValue } from '../../auth/auth-context' +import type { YVUserInfo } from '../../auth/types' import { YouVersionContext } from '../../youversion-context' import type { Result } from '../../result' import type { HighlightsApiError } from '../api' @@ -130,11 +131,11 @@ const getAccessToken = jest.fn, []>() function defaultGetAccessToken(): Promise { const token = currentAuth?.accessToken ?? null - return Promise.resolve( - token === null - ? { status: 'unavailable', reason: 'signed-out' } - : { status: 'ok', token, userId: currentAuth?.userInfo?.id ?? null }, - ) + const userId = currentAuth?.userInfo?.id + if (token === null || typeof userId !== 'string') { + return Promise.resolve({ status: 'unavailable', reason: 'signed-out' }) + } + return Promise.resolve({ status: 'ok', token, userId }) } function authValue(overrides: Partial): AuthContextValue { @@ -1538,7 +1539,7 @@ describe('missing user id', () => { const noUserId: AuthShape = { isAuthenticated: true, accessToken: 'token-1', - userInfo: { name: 'Someone' }, + userInfo: { name: 'Someone' } as YVUserInfo, isLoading: false, } mockGetHighlights.mockResolvedValue(collection([highlight('JHN.3.16', YELLOW)])) @@ -1569,7 +1570,7 @@ describe('missing user id', () => { const { result } = renderUseHighlights({ isAuthenticated: true, accessToken: 'token-1', - userInfo: { name: 'Someone' }, + userInfo: { name: 'Someone' } as YVUserInfo, isLoading: false, }) diff --git a/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx b/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx index b095d07..450b39c 100644 --- a/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx +++ b/packages/ui/src/native/__tests__/bible-reader-highlights-prompts.test.tsx @@ -104,7 +104,7 @@ function stubAuth(isAuthenticated: boolean) { const value: AuthValue = { isAuthenticated, accessToken: isAuthenticated ? 'test-token' : null, - userInfo: null, + userInfo: isAuthenticated ? { id: 'test-user' } : null, error: null, signIn: jest.fn(async () => undefined), signOut: jest.fn(async () => undefined), @@ -112,7 +112,7 @@ function stubAuth(isAuthenticated: boolean) { ensureFreshToken: jest.fn(async () => undefined), getAccessToken: jest.fn(async () => isAuthenticated - ? ({ status: 'ok', token: 'test-token', userId: null } as const) + ? ({ status: 'ok', token: 'test-token', userId: 'test-user' } as const) : ({ status: 'unavailable', reason: 'signed-out' } as const), ), isLoading: false,