feat(payments-next): add OAuth strategy guard#20352
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an FxA OAuth JWT authentication strategy to payments-next (NestJS) so endpoints can be protected with bearer tokens when the API starts serving private/PII-adjacent data.
Changes:
- Introduces an
FxaOAuthJwtStrategy(JWKS-backed) plusFxaOAuthJwtAuthGuardfor@UseGuards(...). - Extends the payments API typed config (
RootConfig) and.envto include FxA OAuth JWT validation settings. - Adds unit tests for the strategy/guard and wires the new
AuthModuleinto the app module.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Adds jwks-rsa dependency used for JWKS-backed JWT verification. |
| apps/payments/api/src/config/index.ts | Extends RootConfig with fxaOAuthConfig so env-driven config is available. |
| apps/payments/api/src/auth/index.ts | Exposes auth module, guard, claims interface, and config exports. |
| apps/payments/api/src/auth/fxa-oauth.config.ts | Defines typed config for FxA OAuth JWT validation (JWKS URI/issuer/scope). |
| apps/payments/api/src/auth/fxa-oauth-jwt.strategy.ts | Implements Passport JWT strategy using JWKS and required-scope enforcement. |
| apps/payments/api/src/auth/fxa-oauth-jwt.strategy.spec.ts | Unit-tests scope enforcement in the strategy. |
| apps/payments/api/src/auth/fxa-oauth-jwt-auth.guard.ts | Adds a guard wrapper for the fxa-oauth-jwt strategy. |
| apps/payments/api/src/auth/fxa-oauth-jwt-auth.guard.spec.ts | Basic instantiation test for the guard. |
| apps/payments/api/src/auth/fxa-access-token.interface.ts | Declares expected FxA OAuth access token claims shape. |
| apps/payments/api/src/auth/auth.module.ts | Registers the new JWT strategy provider. |
| apps/payments/api/src/app/app.module.ts | Imports AuthModule so the strategy is registered in the app. |
| apps/payments/api/.env | Adds new FxA OAuth config env vars for local/dev configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| jwtFromRequest: ExtractJwt.fromAuthHeaderAsBearerToken(), | ||
| issuer: config.fxaOAuthIssuer, | ||
| algorithms: ['RS256'], | ||
| }); |
| @IsString() | ||
| public readonly fxaOAuthRequiredScope!: string; | ||
| } | ||
|
|
||
| export const MockFxaOAuthConfig = { | ||
| fxaOAuthJwksUri: faker.internet.url(), | ||
| fxaOAuthIssuer: faker.internet.url(), |
| export interface FxaAccessTokenClaims { | ||
| sub: string; | ||
| client_id: string; | ||
| scope: string; |
| FXA_O_AUTH_CONFIG__FXA_O_AUTH_JWKS_URI=http://localhost:9000/v1/jwks | ||
| FXA_O_AUTH_CONFIG__FXA_O_AUTH_ISSUER=http://localhost:9000 |
| /** | ||
| * JWT claims from an FXA OAuth access token. | ||
| */ | ||
| export interface FxaAccessTokenClaims { |
There was a problem hiding this comment.
suggestion: could we use zod to verify and then infer the type?
StaberindeZA
left a comment
There was a problem hiding this comment.
This looks good. Just one inline comment to move the code into libs/*.
Could you also please provide some testing instructions? I tried testing this locally myself by adding a test endpoint using the guard, but got back errors saying the jwt was malformed. (I added some additional logging to console log that the jwt was malformed. If it helps I could push my test branch)
f906362 to
313bbac
Compare
StaberindeZA
left a comment
There was a problem hiding this comment.
Just a few more small comments, but otherwise this looks good.
| if (token) { | ||
| const headerSegment = token.split('.')[0]; | ||
| const header = JSON.parse( | ||
| Buffer.from(headerSegment, 'base64url').toString() | ||
| ); | ||
| if (normalizeTyp(header.typ) !== EXPECTED_TYP) { | ||
| throw new UnauthorizedException('Invalid token type'); | ||
| } | ||
| } |
There was a problem hiding this comment.
question: Is this necessary?
| } as any; | ||
| } | ||
|
|
||
| function makeVerifyResponse(overrides = {}) { |
There was a problem hiding this comment.
issue: please add some factories for these types
| })); | ||
|
|
||
| /** Build a fake JWT with the given header typ. */ | ||
| function makeJwt(typ?: string): string { |
There was a problem hiding this comment.
issue: please add a factory function for this instead
| describe('FxaOAuthJwtStrategy', () => { | ||
| let strategy: FxaOAuthJwtStrategy; | ||
|
|
||
| const baseClaims: FxaAccessTokenClaims = { |
There was a problem hiding this comment.
issue: please add a factory function for this instead
| interface VerifyResponse { | ||
| user: string; | ||
| client_id: string; | ||
| scope: string[]; | ||
| generation?: number; | ||
| profile_changed_at?: number; | ||
| } |
There was a problem hiding this comment.
issue: could you please add a zod schema for this and infer the type
| jti: '', | ||
| 'fxa-generation': body.generation, | ||
| 'fxa-profileChangedAt': body.profile_changed_at, | ||
| }; |
There was a problem hiding this comment.
suggestion: rather limit the returned value of both strategies to common fields, "user", "client_id" and "scope".
Instead of adding falsey/dummy data for keys in type FxaAccessTokenClaims that are not available in the response from GET /verify, rather limit the returned value from both strategies to fields that common to both.
This way it's clear for consumers of this data which values are actually available.
There was a problem hiding this comment.
Sounds good. I've pulled out the schema to a shared type (FxaOAuthUser) and removed the fields that were only present on one (jwt) strategy
Because: * We are going to be using payments-api to serve private data, such as subscriptions, that could contain PII * Currently we have no proper endpoint authentication tools available to payments-api This commit: * Pulls in JWT strategy and a sessionId strategy for use with FXA-provided credentials via the nestjs PassportStrategy * Provides FxaOAuthJwtAuthGuard for use with @UseGuards(guard) decorator on endpoints Closes #PAY-3612
StaberindeZA
left a comment
There was a problem hiding this comment.
lgtm. Ty for the changes
Because:
This commit:
Closes #PAY-3612
Checklist
Put an
xin the boxes that applyOther Notes
This is not currently in use on any endpoints. An example would look akin to: