From e8b421937a8b327314edb1cb94bb793cb6cf2427 Mon Sep 17 00:00:00 2001 From: Chris Kehayias Date: Fri, 21 Aug 2026 08:42:09 -0400 Subject: [PATCH] fix(mp-client): honor expires_in instead of capping every token at 5 minutes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ensureValidToken()` set `expiresAt = now + 5min` for every token, discarding the `expires_in` the OAuth endpoint returned — while the comment claimed it was subtracting a safety buffer from the real expiration. Since the provider is a singleton and `ensureValidToken()` runs before every service call, a 1-hour MP token was thrown away after 5 minutes, roughly 12x more token requests than necessary. The lifetime now comes from `expires_in`, minus a 5-minute `TOKEN_SAFETY_MARGIN`, floored at 30 seconds so a pathologically short or negative value cannot drive a refresh storm. Missing or non-numeric values fall back to 3600s. `getClientCredentialsToken()` declares a `ClientCredentialsToken` return type instead of leaking `any` out of `response.json()`. Two existing tests pinned the wrong behavior by advancing timers past the 5-minute mark and asserting a refresh; both are rewritten against the real boundary, and the Token Lifecycle block now covers 3600s -> 55min, absent `expires_in`, `expires_in: 60` -> 30s floor, and a non-numeric value. Verified by mutation: restoring the flat cap fails all four, and dropping just the `Math.max` floor fails the clamp test. Closes the TODO; TestCoverage.md 5.7 marked fixed. Co-Authored-By: Claude Opus 5 (1M context) --- ...lient-token-lifetime-ignores-expires-in.md | 54 ----------- .claude/docs/TestCoverage.md | 23 +++-- .../auth/client-credentials.ts | 18 +++- .../ministry-platform/client.test.ts | 89 +++++++++++++++++-- src/lib/providers/ministry-platform/client.ts | 27 ++++-- 5 files changed, 137 insertions(+), 74 deletions(-) delete mode 100644 .claude/TODO/mp-client-token-lifetime-ignores-expires-in.md 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 ddcfc8c9..00000000 --- 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 53b9b260..84306def 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 46b21326..7d9334e9 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 540dd4f9..a1c273b8 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 a0a23d50..124d11d1 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 +}