From 7bba787b8c3add852059fa6e40a0add0c9d57b04 Mon Sep 17 00:00:00 2001 From: Brandon Corbett Date: Sat, 31 Oct 2026 12:59:46 -0400 Subject: [PATCH] feat(oidc): consume provider records atomically and report the winner --- .changeset/oidc-atomic-consume.md | 5 +++++ openapi.json | 15 ++++++++++++++- src/controllers/oidcEdge.ts | 4 ++-- src/generated/api.ts | 18 +++++++++++++++++- src/routes/oidc.routes.ts | 8 ++++++-- src/services/oidcPayloadStore.ts | 17 +++++++++++++++-- tests/integration/oidc/oidcEdge.spec.ts | 8 +++++++- tests/postgres/oidcProvider.spec.ts | 11 +++++++++++ tests/unit/services/oidcPayloadStore.spec.ts | 18 +++++++++++++++--- 9 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 .changeset/oidc-atomic-consume.md diff --git a/.changeset/oidc-atomic-consume.md b/.changeset/oidc-atomic-consume.md new file mode 100644 index 00000000..9b983a3a --- /dev/null +++ b/.changeset/oidc-atomic-consume.md @@ -0,0 +1,5 @@ +--- +'seamless-auth-api': minor +--- + +`POST /oidc/payloads/:model/:id/consume` now answers `200 { consumed }` instead of `204` (`@seamless-auth/types` 0.34.0-next.4). It sets `consumedAt` in one conditional statement, and `consumed` is true only for the call that set it. Two concurrent refreshes with the same refresh token used to both pass the `consumedAt ??=` check and both rotate. Now exactly one wins, and the provider service treats the other as reuse. An expired record is never consumed. diff --git a/openapi.json b/openapi.json index 9db8acb5..22237599 100644 --- a/openapi.json +++ b/openapi.json @@ -12109,7 +12109,7 @@ }, "/oidc/payloads/{model}/{id}/consume": { "post": { - "summary": "Mark a provider record consumed", + "summary": "Mark a provider record consumed, and say whether this call did", "tags": ["OpenID Provider service"], "security": [{ "serviceToken": [] }], "parameters": [ @@ -12141,6 +12141,19 @@ } ], "responses": { + "200": { + "description": "HTTP 200", + "content": { + "application/json": { + "example": { "consumed": true }, + "schema": { + "type": "object", + "properties": { "consumed": { "type": "boolean" } }, + "required": ["consumed"] + } + } + } + }, "400": { "description": "HTTP 400", "content": { diff --git a/src/controllers/oidcEdge.ts b/src/controllers/oidcEdge.ts index b9c2ca90..405931c1 100644 --- a/src/controllers/oidcEdge.ts +++ b/src/controllers/oidcEdge.ts @@ -131,8 +131,8 @@ export async function getOidcPayloadByUid(req: Req, res: R } export async function postOidcPayloadConsume(req: Req, res: Response) { - await consumeOidcPayload(req.params.model, req.params.id); - return res.status(204).end(); + const consumed = await consumeOidcPayload(req.params.model, req.params.id); + return res.status(200).json({ consumed }); } export async function deleteOidcPayload(req: Req, res: Response) { diff --git a/src/generated/api.ts b/src/generated/api.ts index fceccf27..0a405fc3 100644 --- a/src/generated/api.ts +++ b/src/generated/api.ts @@ -11938,7 +11938,7 @@ export interface paths { }; get?: never; put?: never; - /** Mark a provider record consumed */ + /** Mark a provider record consumed, and say whether this call did */ post: { parameters: { query?: never; @@ -11961,6 +11961,22 @@ export interface paths { }; requestBody?: never; responses: { + /** @description HTTP 200 */ + 200: { + headers: { + [name: string]: unknown; + }; + content: { + /** + * @example { + * "consumed": true + * } + */ + 'application/json': { + consumed: boolean; + }; + }; + }; /** @description HTTP 400 */ 400: { headers: { diff --git a/src/routes/oidc.routes.ts b/src/routes/oidc.routes.ts index 15ac2ef2..9b515c74 100644 --- a/src/routes/oidc.routes.ts +++ b/src/routes/oidc.routes.ts @@ -15,6 +15,7 @@ import { OidcEdgeConfigResponseSchema, OidcEdgeEventSchema, OidcJwksResponseSchema, + OidcPayloadConsumeResponseSchema, OidcPayloadGrantParamsSchema, OidcPayloadParamsSchema, OidcPayloadRecordSchema, @@ -142,8 +143,11 @@ oidcRouter.post( '/payloads/:model/:id/consume', { ...edge, - summary: 'Mark a provider record consumed', - schemas: { params: OidcPayloadParamsSchema, response: guarded }, + summary: 'Mark a provider record consumed, and say whether this call did', + schemas: { + params: OidcPayloadParamsSchema, + response: { 200: OidcPayloadConsumeResponseSchema, ...guarded }, + }, }, postOidcPayloadConsume, ); diff --git a/src/services/oidcPayloadStore.ts b/src/services/oidcPayloadStore.ts index 2acec548..49e8a5ce 100644 --- a/src/services/oidcPayloadStore.ts +++ b/src/services/oidcPayloadStore.ts @@ -204,11 +204,24 @@ export function findOidcPayloadByUid(model: OidcPayloadModel, uid: string) { return findOne({ model, uid }); } +/** + * True only for the caller whose update set `consumedAt`. The condition is in the one + * statement, so of two refreshes racing with the same token exactly one wins, and the edge + * treats the other as reuse rather than issuing a second rotation. + */ export async function consumeOidcPayload(model: OidcPayloadModel, id: string) { - await OidcPayload.update( + const [affected] = await OidcPayload.update( { consumedAt: new Date() }, - { where: { model, id, consumedAt: { [Op.is]: null } } as WhereOptions }, + { + where: { + model, + id, + consumedAt: { [Op.is]: null }, + expiresAt: { [Op.gt]: new Date() }, + } as WhereOptions, + }, ); + return affected > 0; } export async function destroyOidcPayload(model: OidcPayloadModel, id: string) { diff --git a/tests/integration/oidc/oidcEdge.spec.ts b/tests/integration/oidc/oidcEdge.spec.ts index 39876985..8fd99462 100644 --- a/tests/integration/oidc/oidcEdge.spec.ts +++ b/tests/integration/oidc/oidcEdge.spec.ts @@ -270,7 +270,13 @@ describe('/oidc/payloads', () => { }); it('consumes, destroys and revokes by grant', async () => { - expect((await edge('post', '/oidc/payloads/AuthorizationCode/c1/consume')).status).toBe(204); + vi.mocked(consumeOidcPayload).mockResolvedValueOnce(true).mockResolvedValueOnce(false); + const won = await edge('post', '/oidc/payloads/AuthorizationCode/c1/consume'); + expect(won.status).toBe(200); + expect(won.body).toEqual({ consumed: true }); + expect((await edge('post', '/oidc/payloads/AuthorizationCode/c1/consume')).body).toEqual({ + consumed: false, + }); expect((await edge('delete', '/oidc/payloads/Session/s1')).status).toBe(204); expect((await edge('delete', '/oidc/payloads/RefreshToken/by-grant/g1')).status).toBe(204); diff --git a/tests/postgres/oidcProvider.spec.ts b/tests/postgres/oidcProvider.spec.ts index b2c691ba..0b5dd260 100644 --- a/tests/postgres/oidcProvider.spec.ts +++ b/tests/postgres/oidcProvider.spec.ts @@ -51,6 +51,17 @@ describe('oidc_payloads', () => { expect(await findOidcPayload('AuthorizationCode', id)).toBeNull(); }); + it('lets exactly one of many concurrent consumes win', async () => { + const id = randomUUID(); + await upsertOidcPayload('RefreshToken', id, { payload: { grantId: id }, expiresIn: 600 }); + + const results = await Promise.all( + Array.from({ length: 8 }, () => consumeOidcPayload('RefreshToken', id)), + ); + + expect(results.filter(Boolean)).toHaveLength(1); + }); + it('overwrites a record on upsert without losing its consumed state', async () => { const id = randomUUID(); diff --git a/tests/unit/services/oidcPayloadStore.spec.ts b/tests/unit/services/oidcPayloadStore.spec.ts index aca4b595..57715c8a 100644 --- a/tests/unit/services/oidcPayloadStore.spec.ts +++ b/tests/unit/services/oidcPayloadStore.spec.ts @@ -267,12 +267,24 @@ describe('findGrantBoundSession', () => { }); describe('changing records', () => { - it('consumes once', async () => { - await consumeOidcPayload('AuthorizationCode', 'c1'); + it('consumes once, in one conditional statement, and says whether this call did', async () => { + vi.mocked(OidcPayload.update).mockResolvedValueOnce([1] as never); + expect(await consumeOidcPayload('RefreshToken', 'r1')).toBe(true); expect(OidcPayload.update).toHaveBeenCalledWith( { consumedAt: expect.any(Date) }, - { where: { model: 'AuthorizationCode', id: 'c1', consumedAt: { [Op.is]: null } } }, + { + where: { + model: 'RefreshToken', + id: 'r1', + consumedAt: { [Op.is]: null }, + expiresAt: { [Op.gt]: expect.any(Date) }, + }, + }, ); + + // The second of two racing refreshes: the row already had consumedAt. + vi.mocked(OidcPayload.update).mockResolvedValueOnce([0] as never); + expect(await consumeOidcPayload('RefreshToken', 'r1')).toBe(false); }); it('destroys one record, or every record of a grant', async () => {