From 847acd02bb2d6d17e757ab2b44a837fa0c3ba669 Mon Sep 17 00:00:00 2001 From: Gustavo Valverde Date: Sun, 11 Oct 2026 18:29:52 +0000 Subject: [PATCH] feat(oauth-provider): pass client ID to privilege checks (#11658) --- ...th-provider-client-privileges-client-id.md | 5 + docs/content/docs/plugins/oauth-provider.mdx | 24 ++ .../oauthClient/endpoints-privileges.test.ts | 246 +++++++++++++++++- .../src/oauthClient/endpoints.ts | 28 +- .../src/oauthClient/privileges.ts | 2 + .../oauth-provider/src/public-types.test.ts | 6 + packages/oauth-provider/src/types/index.ts | 10 + 7 files changed, 316 insertions(+), 5 deletions(-) create mode 100644 .changeset/oauth-provider-client-privileges-client-id.md diff --git a/.changeset/oauth-provider-client-privileges-client-id.md b/.changeset/oauth-provider-client-privileges-client-id.md new file mode 100644 index 0000000000..62dc29c8f8 --- /dev/null +++ b/.changeset/oauth-provider-client-privileges-client-id.md @@ -0,0 +1,5 @@ +--- +"@better-auth/oauth-provider": patch +--- + +If you configure `clientPrivileges`, the callback now receives `clientId` when a user reads, updates, deletes, rotates the secret of, or configures scopes on an existing client. You can use it to allow or deny actions per client among users who share a `clientReference`. Before, the callback could not tell which client the request targeted. `clientId` is absent for creating and listing clients, and the callback still cannot grant access to a client owned by someone else, apart from the existing scope-only updates through `adminUpdateOAuthClient`. Existing callbacks keep working unchanged. diff --git a/docs/content/docs/plugins/oauth-provider.mdx b/docs/content/docs/plugins/oauth-provider.mdx index 814a78b707..0965561489 100644 --- a/docs/content/docs/plugins/oauth-provider.mdx +++ b/docs/content/docs/plugins/oauth-provider.mdx @@ -1706,6 +1706,8 @@ To set user-specific permissions and roles on tokens see [Claims](#claims). To determine whether a logged in user has the ability to perform specific actions in client creation, you can utilize the `clientPrivileges` configuration setting. By default, CRUD actions are allowed for users with matching `userId` or `clientReference`. +The callback can only deny an action. Client CRUD still checks ownership after the callback, so returning `true` cannot give a user access to a client owned by another user or another `clientReference`. The one exception is scope configuration on another owner's client through `adminUpdateOAuthClient`: when the update changes only `client_credentials_scopes`, the callback's `update` and `configure-client-credentials-scopes` decisions authorize it without an ownership check. + The following is a basic example that allows all OAuth Client CRUD actions for organization owners assuming ordinary users cannot create clients: ```ts title="auth.ts" @@ -1720,6 +1722,28 @@ oauthProvider({ }) ``` +For actions on an existing client (`read`, `update`, `delete`, `rotate`, and `configure-client-credentials-scopes`), the callback also receives `clientId`. It is absent for `create`, `list`, and scope configuration during creation. `clientId` is the `client_id` from the request, so the client may not exist and the value proves nothing about ownership. + +Use `clientId` to restrict individual clients among users who share a `clientReference`, such as the members of an organization. In this example, every member can create, list, and read the organization's clients, but only a client's maintainers can change it or configure its scopes. `getClientMaintainers` stands for your own lookup: + +```ts title="auth.ts" +oauthProvider({ + clientReference: ({ session }) => { + return (session?.activeOrganizationId as string | undefined) ?? undefined; + }, + clientPrivileges: async ({ action, user, clientId }) => { + if (!user) return false; + if (action === "create" || action === "list" || action === "read") { + return true; + } + // Scope configuration during creation has no client ID, so deny it here. + if (!clientId) return false; + const maintainers = await getClientMaintainers(clientId); + return maintainers.includes(user.id); + }, +}) +``` + ### Storage By default all secrets are `hashed` by default on the database. This helps protect the `client_secret` in case of a database leak. diff --git a/packages/oauth-provider/src/oauthClient/endpoints-privileges.test.ts b/packages/oauth-provider/src/oauthClient/endpoints-privileges.test.ts index b2693792b3..91cad4fe77 100644 --- a/packages/oauth-provider/src/oauthClient/endpoints-privileges.test.ts +++ b/packages/oauth-provider/src/oauthClient/endpoints-privileges.test.ts @@ -5,8 +5,13 @@ import { getTestInstance } from "better-auth/test"; import { beforeEach, describe, expect, it, vi } from "vitest"; import { oauthProviderClient } from "../client"; import { oauthProvider } from "../oauth"; +import type { OAuthOptions } from "../types"; import type { OAuthClient } from "../types/oauth"; +type ClientPrivilegeContext = Parameters< + NonNullable +>[0]; + type TestOAuthClientUiMetadata = Pick< OAuthClient, | "client_name" @@ -32,7 +37,7 @@ describe("oauthClient", async () => { password: "test123456", name: "forbidden user", }; - const clientPrivileges = vi.fn(({ user }) => { + const clientPrivileges = vi.fn(({ user }: ClientPrivilegeContext) => { if (user?.email === allowedUser.email) { return true; } @@ -144,6 +149,7 @@ describe("oauthClient", async () => { expect(client?.data?.client_id_issued_at).toBeDefined(); expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges.mock.calls[0]?.[0]?.clientId).toBeUndefined(); }); it("should not create client with forbidden user via admin api", async () => { @@ -219,6 +225,30 @@ describe("oauthClient", async () => { }), ); expect(clientPrivileges).toHaveBeenCalledTimes(2); + expect(clientPrivileges.mock.calls[1]?.[0]?.clientId).toBeUndefined(); + + clientPrivileges.mockClear(); + await auth.api.adminUpdateOAuthClient({ + headers: allowedUserHeaders, + body: { + client_id: adminClient.client_id, + update: { client_credentials_scopes: ["m2m:read"] }, + }, + }); + expect(clientPrivileges).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + action: "update", + clientId: adminClient.client_id, + }), + ); + expect(clientPrivileges).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ + action: "configure-client-credentials-scopes", + clientId: adminClient.client_id, + }), + ); await allowedAuthClient.oauth2.deleteClient({ client_id: adminClient.client_id, }); @@ -309,6 +339,12 @@ describe("oauthClient", async () => { expect(check).toMatchObject(expected); expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "read", + clientId: oauthClient.client_id, + }), + ); }); it("should get public-only information about a client with any user", async () => { @@ -356,6 +392,7 @@ describe("oauthClient", async () => { expect(check).toMatchObject(expected); expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges.mock.calls[0]?.[0]?.clientId).toBeUndefined(); }); it("should not update the client with forbidden user", async () => { @@ -393,6 +430,12 @@ describe("oauthClient", async () => { oauthClient = client.data!; expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "update", + clientId: oauthClient.client_id, + }), + ); }); it("should not rotate the client secret with forbidden user", async () => { @@ -420,6 +463,12 @@ describe("oauthClient", async () => { oauthClient = client.data!; expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "rotate", + clientId: oauthClient.client_id, + }), + ); }); it("should not delete the client with forbidden user", async () => { @@ -442,6 +491,12 @@ describe("oauthClient", async () => { expect(client.data).toBeNull(); expect(clientPrivileges).toHaveBeenCalledTimes(1); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "delete", + clientId: oauthClient.client_id, + }), + ); }); it("should not create client via admin api without a session", async () => { @@ -583,3 +638,192 @@ describe("oauthClient dynamic registration privileges", async () => { expect(clientPrivileges).not.toHaveBeenCalled(); }); }); + +/** + * @see https://github.com/better-auth/better-auth/issues/11589 + */ +describe("clientPrivileges client ID", async () => { + const redirectUri = "https://rp.example.com/callback"; + const owner = { + email: "owner@test.com", + password: "test123456", + name: "owner", + }; + const collaborator = { + email: "collaborator@test.com", + password: "test123456", + name: "collaborator", + }; + let restrictedClientId = ""; + const clientPrivileges = vi.fn( + ({ user, action, clientId }: ClientPrivilegeContext) => + user?.email !== collaborator.email || + action === "read" || + clientId !== restrictedClientId, + ); + const { auth, signInWithUser } = await getTestInstance({ + plugins: [ + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + clientReference: () => "shared-organization", + clientPrivileges, + }), + jwt(), + ], + }); + await auth.api.signUpEmail({ body: owner }); + await auth.api.signUpEmail({ body: collaborator }); + const { headers: ownerHeaders } = await signInWithUser( + owner.email, + owner.password, + ); + const { headers: collaboratorHeaders } = await signInWithUser( + collaborator.email, + collaborator.password, + ); + const restrictedClient = await auth.api.adminCreateOAuthClient({ + headers: ownerHeaders, + body: { redirect_uris: [redirectUri] }, + }); + restrictedClientId = restrictedClient.client_id; + + beforeEach(() => { + clientPrivileges.mockClear(); + }); + + it("should let a collaborator read but not change a restricted shared client", async () => { + const client = await auth.api.getOAuthClient({ + headers: collaboratorHeaders, + query: { client_id: restrictedClientId }, + }); + expect(client.client_id).toBe(restrictedClientId); + + await expect( + auth.api.updateOAuthClient({ + headers: collaboratorHeaders, + body: { + client_id: restrictedClientId, + update: { client_name: "renamed" }, + }, + }), + ).rejects.toMatchObject({ statusCode: 401 }); + await expect( + auth.api.rotateClientSecret({ + headers: collaboratorHeaders, + body: { client_id: restrictedClientId }, + }), + ).rejects.toMatchObject({ statusCode: 401 }); + await expect( + auth.api.deleteOAuthClient({ + headers: collaboratorHeaders, + body: { client_id: restrictedClientId }, + }), + ).rejects.toMatchObject({ statusCode: 401 }); + + for (const action of ["read", "update", "rotate", "delete"]) { + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ action, clientId: restrictedClientId }), + ); + } + const stored = await auth.api.getOAuthClient({ + headers: ownerHeaders, + query: { client_id: restrictedClientId }, + }); + expect(stored.client_name).toBeUndefined(); + }); + + it("should let a collaborator change another shared client", async () => { + const otherClient = await auth.api.adminCreateOAuthClient({ + headers: ownerHeaders, + body: { redirect_uris: [redirectUri] }, + }); + + const updated = await auth.api.updateOAuthClient({ + headers: collaboratorHeaders, + body: { + client_id: otherClient.client_id, + update: { client_name: "renamed" }, + }, + }); + + expect(updated.client_name).toBe("renamed"); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "update", + clientId: otherClient.client_id, + }), + ); + }); + + it("should pass an unknown client ID to the callback before the lookup", async () => { + await expect( + auth.api.getOAuthClient({ + headers: ownerHeaders, + query: { client_id: "unknown-client" }, + }), + ).rejects.toMatchObject({ statusCode: 404 }); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ action: "read", clientId: "unknown-client" }), + ); + }); +}); + +describe("clientPrivileges client ID across owners", async () => { + const clientPrivileges = vi.fn((_context: ClientPrivilegeContext) => true); + const { auth, signInWithUser } = await getTestInstance({ + plugins: [ + oauthProvider({ + loginPage: "/login", + consentPage: "/consent", + scopes: ["m2m:read"], + clientPrivileges, + }), + jwt(), + ], + }); + const owner = { + email: "client-owner@test.com", + password: "test123456", + name: "client owner", + }; + const administrator = { + email: "administrator@test.com", + password: "test123456", + name: "administrator", + }; + await auth.api.signUpEmail({ body: owner }); + await auth.api.signUpEmail({ body: administrator }); + const { headers: ownerHeaders } = await signInWithUser( + owner.email, + owner.password, + ); + const { headers: administratorHeaders } = await signInWithUser( + administrator.email, + administrator.password, + ); + + it("should pass the client ID when configuring scopes on another owner's client", async () => { + const client = await auth.api.adminCreateOAuthClient({ + headers: ownerHeaders, + body: { grant_types: ["client_credentials"] }, + }); + clientPrivileges.mockClear(); + + const updated = await auth.api.adminUpdateOAuthClient({ + headers: administratorHeaders, + body: { + client_id: client.client_id, + update: { client_credentials_scopes: ["m2m:read"] }, + }, + }); + + expect(updated.client_credentials_scopes).toEqual(["m2m:read"]); + expect(clientPrivileges).toHaveBeenCalledWith( + expect.objectContaining({ + action: "configure-client-credentials-scopes", + clientId: client.client_id, + }), + ); + }); +}); diff --git a/packages/oauth-provider/src/oauthClient/endpoints.ts b/packages/oauth-provider/src/oauthClient/endpoints.ts index 70d6646e99..505b97cf82 100644 --- a/packages/oauth-provider/src/oauthClient/endpoints.ts +++ b/packages/oauth-provider/src/oauthClient/endpoints.ts @@ -31,7 +31,7 @@ export async function getClientEndpoint( opts: OAuthOptions, ) { const session = await getSessionFromCtx(ctx); - await assertClientPrivileges(ctx, session, opts, "read"); + await assertClientPrivileges(ctx, session, opts, "read", ctx.query.client_id); if (!session) throw new APIError("UNAUTHORIZED"); const client = await getClient(ctx, opts, ctx.query.client_id); @@ -143,7 +143,13 @@ export async function deleteClientEndpoint( opts: OAuthOptions, ) { const session = await getSessionFromCtx(ctx); - await assertClientPrivileges(ctx, session, opts, "delete"); + await assertClientPrivileges( + ctx, + session, + opts, + "delete", + ctx.body.client_id, + ); if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; @@ -216,7 +222,13 @@ export async function updateClientEndpoint( OAuthClientAdministrativeResponse | OAuthClientRegistrationResponse > { const session = await getSessionFromCtx(ctx); - await assertClientPrivileges(ctx, session, opts, "update"); + await assertClientPrivileges( + ctx, + session, + opts, + "update", + ctx.body.client_id, + ); if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; @@ -257,6 +269,7 @@ export async function updateClientEndpoint( session, opts, "configure-client-credentials-scopes", + clientId, ); } @@ -298,6 +311,7 @@ export async function updateClientEndpoint( session, opts, "configure-client-credentials-scopes", + clientId, ); } } @@ -386,7 +400,13 @@ export async function rotateClientSecretEndpoint( opts: OAuthOptions, ) { const session = await getSessionFromCtx(ctx); - await assertClientPrivileges(ctx, session, opts, "rotate"); + await assertClientPrivileges( + ctx, + session, + opts, + "rotate", + ctx.body.client_id, + ); if (!session) throw new APIError("UNAUTHORIZED"); const clientId = ctx.body.client_id; diff --git a/packages/oauth-provider/src/oauthClient/privileges.ts b/packages/oauth-provider/src/oauthClient/privileges.ts index 58d538ddab..6e9e54637b 100644 --- a/packages/oauth-provider/src/oauthClient/privileges.ts +++ b/packages/oauth-provider/src/oauthClient/privileges.ts @@ -25,6 +25,7 @@ export async function assertClientPrivileges( | "list" | "rotate" | "configure-client-credentials-scopes", + clientId?: string, ) { if (!session) throw new APIError("UNAUTHORIZED"); if (!ctx.headers) throw new APIError("BAD_REQUEST"); @@ -41,6 +42,7 @@ export async function assertClientPrivileges( action, session: session.session, user: session.user, + clientId, })) ) { throw new APIError("UNAUTHORIZED"); diff --git a/packages/oauth-provider/src/public-types.test.ts b/packages/oauth-provider/src/public-types.test.ts index d4674caf19..76edb0a8da 100644 --- a/packages/oauth-provider/src/public-types.test.ts +++ b/packages/oauth-provider/src/public-types.test.ts @@ -15,6 +15,9 @@ import { describe, expectTypeOf, it } from "vitest"; type ClientPrivilegeAction = Parameters< NonNullable >[0]["action"]; +type ClientPrivilegeContext = Parameters< + NonNullable +>[0]; describe("public oauth-provider types", () => { it("exports the signed-query verifier with a server-side secret", () => { @@ -75,5 +78,8 @@ describe("public oauth-provider types", () => { expectTypeOf().not.toHaveProperty("type"); expectTypeOf<"configure-client-credentials-scopes">().toMatchTypeOf(); expectTypeOf<"configure-client-credentials">().not.toMatchTypeOf(); + expectTypeOf().toEqualTypeOf< + string | undefined + >(); }); }); diff --git a/packages/oauth-provider/src/types/index.ts b/packages/oauth-provider/src/types/index.ts index 28e0787ac6..65ef15ea79 100644 --- a/packages/oauth-provider/src/types/index.ts +++ b/packages/oauth-provider/src/types/index.ts @@ -943,6 +943,16 @@ export interface OAuthOptions< | "configure-client-credentials-scopes"; user?: User & Record; session?: Session & Record; + /** + * The `client_id` from the request, for actions on an existing client. + * Absent for `create`, `list`, and scope configuration during creation. + * The client may not exist, and this value does not establish ownership. + * Client CRUD enforces ownership except when `adminUpdateOAuthClient` + * changes only `client_credentials_scopes`: the `update` and + * `configure-client-credentials-scopes` decisions can authorize access + * to another owner's client. + */ + clientId?: string; }) => Awaitable; /** * Grant types supported by the token endpoint