diff --git a/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md b/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md deleted file mode 100644 index ddcfc8c..0000000 --- a/.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md +++ /dev/null @@ -1,54 +0,0 @@ -# TODO: `MinistryPlatformClient` discards `expires_in` and caps every token at 5 minutes - -**Created:** 2026-08-21 -**Severity:** Low โ€” wasteful, and the code contradicts its own comment. -**Status:** Open. - -## Symptom - -`src/lib/providers/ministry-platform/client.ts`: - -```ts -// Token refresh interval - refresh 5 minutes before actual expiration for safety -const TOKEN_LIFE = 5 * 60 * 1000; // 5 minutes -... -const creds = await getClientCredentialsToken(); -this.token = creds.access_token; -// Set expiration time with safety buffer (TOKEN_LIFE before actual expiration) -this.expiresAt = new Date(Date.now() + TOKEN_LIFE); -``` - -The comments describe subtracting a safety buffer from the real expiry. The code instead sets every -token's usable life to exactly 5 minutes, discarding the `expires_in` value that MP returns in the -token response. - -MP client-credentials tokens are typically valid for an hour, so this means roughly 12x more token -requests than necessary. Behavior is correct โ€” just wasteful, and the stated intent and the actual -behavior disagree, which is the kind of gap that bites whoever edits it next. - -## Fix - -```ts -const creds = await getClientCredentialsToken(); -this.token = creds.access_token; -const lifetimeMs = (Number(creds.expires_in) || 3600) * 1000; -const SAFETY_MARGIN = 5 * 60 * 1000; -this.expiresAt = new Date(Date.now() + Math.max(lifetimeMs - SAFETY_MARGIN, 30_000)); -``` - -Rename `TOKEN_LIFE` to `TOKEN_SAFETY_MARGIN` so the constant says what it is. The `max(..., 30s)` -floor keeps a pathologically short `expires_in` from causing a refresh storm. - -## Test to add alongside the fix - -- `expires_in: 3600` -> `expiresAt` is ~55 minutes out -- `expires_in` missing -> falls back to the 1-hour default -- `expires_in: 60` -> clamped to the 30s floor rather than going negative - -Note for whoever writes these: `client.test.ts` already exercises the refresh path, and the current -behavior is not pinned by any assertion on `expiresAt` โ€” so the fix will not break existing tests, -which is precisely the problem. - -## Related - -- `.claude/docs/TestCoverage.md` ยง7.7 diff --git a/.claude/docs/TestCoverage.md b/.claude/docs/TestCoverage.md index 53b9b26..84306de 100644 --- a/.claude/docs/TestCoverage.md +++ b/.claude/docs/TestCoverage.md @@ -258,12 +258,23 @@ remaining callers are one per page load and one per `contact-logs.tsx` mount; ca process-wide singleton would hide a newly added contact log type until restart, for a single-digit request saving. -### 5.7 `client.ts` token lifetime ignores `expires_in` ๐ŸŸก - -โ†’ `.claude/TODO/mp-client-token-lifetime-ignores-expires-in.md` - -The comment says "refresh 5 minutes *before* actual expiration"; the code caps every token at 5 -minutes total, discarding `expires_in`. Roughly 12ร— more token requests than necessary. +### 5.7 `client.ts` token lifetime ignores `expires_in` โœ… FIXED + +Was: the comment said "refresh 5 minutes *before* actual expiration"; the code set +`expiresAt = now + 5min` for every token, discarding the `expires_in` the OAuth endpoint returned. +Since `MinistryPlatformProvider` is a singleton and `ensureValidToken()` runs before every service +call, a 1-hour MP token was thrown away after 5 minutes โ€” roughly 12ร— more token requests than +necessary. Two tests in `client.test.ts` pinned the wrong behavior by advancing timers past the +5-minute mark and asserting a refresh, so the cap looked deliberate. + +Now: the lifetime comes from `expires_in`, minus a `TOKEN_SAFETY_MARGIN` of 5 minutes, floored at 30 +seconds so a pathologically short or negative value cannot drive a refresh storm. A missing or +non-numeric `expires_in` falls back to `DEFAULT_TOKEN_LIFETIME_SECONDS` (3600). +`getClientCredentialsToken()` now declares a `ClientCredentialsToken` return type instead of leaking +`any` out of `response.json()`. The two misleading tests were rewritten against the real boundary and +four cases added (3600s โ†’ 55min, `expires_in` absent, `expires_in: 60` โ†’ 30s floor, non-numeric +โ†’ default). Verified by mutation: restoring the flat 5-minute cap fails all four, and dropping just +the `Math.max` floor fails the clamp test. ### 5.8 Resolved: `auth.test.ts` asserted against a copy of the logic โœ… diff --git a/src/lib/providers/ministry-platform/auth/client-credentials.ts b/src/lib/providers/ministry-platform/auth/client-credentials.ts index 46b2132..7d9334e 100644 --- a/src/lib/providers/ministry-platform/auth/client-credentials.ts +++ b/src/lib/providers/ministry-platform/auth/client-credentials.ts @@ -1,4 +1,18 @@ -export async function getClientCredentialsToken() { +/** + * Shape of the OAuth2 client-credentials token response returned by the + * Ministry Platform `/oauth/connect/token` endpoint. + * + * `expires_in` is the token's lifetime in seconds. It is optional here because + * the endpoint's response is untrusted at the type level โ€” callers must handle + * it being absent or non-numeric. + */ +export interface ClientCredentialsToken { + access_token: string; + token_type: string; + expires_in?: number; +} + +export async function getClientCredentialsToken(): Promise { const mpBaseUrl = process.env.MINISTRY_PLATFORM_BASE_URL!; const mpOauthUrl = `${mpBaseUrl}/oauth`; @@ -21,5 +35,5 @@ export async function getClientCredentialsToken() { throw new Error(`Failed to get client credentials token: ${response.statusText}`); } - return await response.json(); + return (await response.json()) as ClientCredentialsToken; } diff --git a/src/lib/providers/ministry-platform/client.test.ts b/src/lib/providers/ministry-platform/client.test.ts index 540dd4f..a1c273b 100644 --- a/src/lib/providers/ministry-platform/client.test.ts +++ b/src/lib/providers/ministry-platform/client.test.ts @@ -72,7 +72,7 @@ describe('MinistryPlatformClient', () => { await client.ensureValidToken(); expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); - // Advance time by 1 minute (still within 5-minute validity window) + // Advance time by 1 minute (well inside the token's validity window) vi.advanceTimersByTime(60 * 1000); // Second call - should NOT fetch new token @@ -99,8 +99,8 @@ describe('MinistryPlatformClient', () => { await client.ensureValidToken(); expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); - // Advance time by 6 minutes (past the 5-minute token life) - vi.advanceTimersByTime(6 * 60 * 1000); + // Advance time past the token's usable life (3600s - 5min safety margin) + vi.advanceTimersByTime(56 * 60 * 1000); // Second call - should fetch new token await client.ensureValidToken(); @@ -148,7 +148,8 @@ describe('MinistryPlatformClient', () => { }); describe('Token Lifecycle', () => { - it('should use 5-minute safety buffer for token expiration', async () => { + it('should refresh 5 minutes before the reported expiration', async () => { + // expires_in: 3600 minus the 5-minute safety margin => 55 minutes usable mockGetClientCredentialsToken .mockResolvedValueOnce({ access_token: 'token-1', @@ -167,20 +168,94 @@ describe('MinistryPlatformClient', () => { await client.ensureValidToken(); expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); - // Advance time by 4 minutes 59 seconds (just under 5-minute buffer) - vi.advanceTimersByTime(4 * 60 * 1000 + 59 * 1000); + // Advance to 54:59 - just inside the 55-minute window + vi.advanceTimersByTime(54 * 60 * 1000 + 59 * 1000); // Should still be valid await client.ensureValidToken(); expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); - // Advance time by 2 more seconds (past 5-minute buffer) + // Advance 2 more seconds, past 55:00 vi.advanceTimersByTime(2000); // Should refresh now await client.ensureValidToken(); expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(2); }); + + it('should fall back to a 1-hour lifetime when expires_in is missing', async () => { + mockGetClientCredentialsToken + .mockResolvedValueOnce({ + access_token: 'no-expiry-token', + token_type: 'Bearer', + }) + .mockResolvedValueOnce({ + access_token: 'refreshed-token', + token_type: 'Bearer', + }); + + const client = new MinistryPlatformClient(); + + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + + // Same 55-minute boundary as an explicit expires_in: 3600 + vi.advanceTimersByTime(54 * 60 * 1000 + 59 * 1000); + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + + vi.advanceTimersByTime(2000); + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(2); + }); + + it('should clamp a short expires_in to the 30-second floor', async () => { + // 60s - 5min margin is negative, so the floor applies instead + mockGetClientCredentialsToken + .mockResolvedValueOnce({ + access_token: 'short-lived-token', + expires_in: 60, + token_type: 'Bearer', + }) + .mockResolvedValueOnce({ + access_token: 'refreshed-token', + expires_in: 60, + token_type: 'Bearer', + }); + + const client = new MinistryPlatformClient(); + + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + + // Just inside the 30-second floor + vi.advanceTimersByTime(29 * 1000); + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + + // Just past it + vi.advanceTimersByTime(2000); + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(2); + }); + + it('should ignore a non-numeric expires_in and use the default lifetime', async () => { + mockGetClientCredentialsToken.mockResolvedValueOnce({ + access_token: 'bogus-expiry-token', + expires_in: 'not-a-number', + token_type: 'Bearer', + }); + + const client = new MinistryPlatformClient(); + + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + + // Would have refreshed immediately if NaN had reached expiresAt + vi.advanceTimersByTime(54 * 60 * 1000); + await client.ensureValidToken(); + expect(mockGetClientCredentialsToken).toHaveBeenCalledTimes(1); + }); }); describe('HTTP Client', () => { diff --git a/src/lib/providers/ministry-platform/client.ts b/src/lib/providers/ministry-platform/client.ts index a0a23d5..124d11d 100644 --- a/src/lib/providers/ministry-platform/client.ts +++ b/src/lib/providers/ministry-platform/client.ts @@ -1,8 +1,17 @@ import { getClientCredentialsToken } from "./auth/client-credentials"; import { HttpClient } from "./utils/http-client"; -// Token refresh interval - refresh 5 minutes before actual expiration for safety -const TOKEN_LIFE = 5 * 60 * 1000; // 5 minutes +// Refresh this far ahead of the token's real expiration, so a request that is +// already in flight never races the expiry boundary. +const TOKEN_SAFETY_MARGIN = 5 * 60 * 1000; // 5 minutes + +// Used when the token response omits expires_in. MP client-credentials tokens +// are issued with a 1 hour lifetime. +const DEFAULT_TOKEN_LIFETIME_SECONDS = 3600; + +// Floor on usable token life, so a pathologically short expires_in cannot drive +// a refresh storm (or, after subtracting the margin, go negative). +const MIN_TOKEN_LIFETIME = 30 * 1000; // 30 seconds /** * MinistryPlatformClient - Core HTTP client with automatic authentication management @@ -48,8 +57,16 @@ export class MinistryPlatformClient { const creds = await getClientCredentialsToken(); this.token = creds.access_token; - // Set expiration time with safety buffer (TOKEN_LIFE before actual expiration) - this.expiresAt = new Date(Date.now() + TOKEN_LIFE); + // Expire the token TOKEN_SAFETY_MARGIN before the lifetime the + // server reported, never sooner than MIN_TOKEN_LIFETIME from now. + const seconds = Number(creds.expires_in); + const lifetimeMs = + (Number.isFinite(seconds) && seconds > 0 + ? seconds + : DEFAULT_TOKEN_LIFETIME_SECONDS) * 1000; + this.expiresAt = new Date( + Date.now() + Math.max(lifetimeMs - TOKEN_SAFETY_MARGIN, MIN_TOKEN_LIFETIME) + ); console.log("Token refreshed. Expires at: ", this.expiresAt); } catch (error) { @@ -66,4 +83,4 @@ export class MinistryPlatformClient { public getHttpClient(): HttpClient { return this.httpClient; } -} \ No newline at end of file +}