Skip to content

feat(token-handler): distinguish public and confidential clients #81 #457

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
dsschiramm wants to merge 5 commits into
base: master
Choose a base branch
from
Open

feat(token-handler): distinguish public and confidential clients #81 #457

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c8e51ac
feat(token-handler): distinguish public and confidential clients #81
dsschiramm Jul 3, 2026
4137d45
Merge branch 'master' into feat/public-confidential-client-distinction
jankapunkt Jul 3, 2026
a0447b6
Potential fix for pull request finding
dsschiramm Jul 4, 2026
e0b27c6
refactor(token-handler): remove redundant PKCE check for client_id re…
dsschiramm Jul 4, 2026
b90e3cf
Merge branch 'master' into feat/public-confidential-client-distinction
jankapunkt Jul 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,7 @@ declare namespace OAuth2Server {
grants: string | string[];
accessTokenLifetime?: number;
refreshTokenLifetime?: number;
type?: 'public' | 'confidential';
[key: string]: any;
}

Expand Down
26 changes: 10 additions & 16 deletions lib/handlers/token-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,6 @@ class TokenHandler {
throw new InvalidRequestError('Missing parameter: `client_id`');
}

if (this.isClientAuthenticationRequired(grantType) && !credentials.clientSecret && !isPkce) {
throw new InvalidRequestError('Missing parameter: `client_secret`');
}

if (!isFormat.vschar(credentials.clientId)) {
throw new InvalidRequestError('Invalid parameter: `client_id`');
}
Expand All @@ -140,7 +136,7 @@ class TokenHandler {
}

try {
const client = await this.model.getClient(credentials.clientId, credentials.clientSecret);
const client = await this.model.getClient(credentials.clientId, credentials.clientSecret || null);

if (!client) {
throw new InvalidClientError('Invalid client: client is invalid');
Expand All @@ -154,6 +150,10 @@ class TokenHandler {
throw new ServerError('Server error: `grants` must be an array');
}

if (this.isClientAuthenticationRequired(grantType, client) && !credentials.clientSecret && !isPkce) {
throw new InvalidClientError('Invalid client: client is invalid');
}

return client;
} catch (e) {
// Include the "WWW-Authenticate" response header field if the client
Expand Down Expand Up @@ -194,16 +194,8 @@ class TokenHandler {
};
}

if (pkce.isPKCERequest({ grantType, codeVerifier })) {

@jankapunkt jankapunkt Jul 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please elaborate why this is now fully omitted? The role of PKCE is especially to protect public clients that cannot safely keep a secret.

@dsschiramm dsschiramm Jul 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The !isPkce condition has been relocated. Previously, getClientCredentials had two early-return flows (one for PKCE requests and another for !isClientAuthenticationRequired) that both returned { clientId } without the secret. I consolidated both into a single return, because the decision regarding whether the secret is required or if the PKCE/public-client status excuse it, now take place in getClient after model.getClient() executes, since that's the only place client.type (public vs confidential) is known. Therefore, the check has moved but has not disappeared: getClient still evaluates isClientAuthenticationRequired(grantType, client) && !credentials.clientSecret && !isPkce before accepting a client without a secret, and isClientAuthenticationRequired checks if client is public.

@dsschiramm dsschiramm Jul 20, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change in the validation location is also the reason why I unified the error with the !client case: both throw the same InvalidClientError('Invalid client: client is invalid'). Otherwise a client_id that exists but lacks its required secret would be distinguishable from one that doesn't exist.

if (request.body.client_id) {
return { clientId: request.body.client_id };
}
}

if (!this.isClientAuthenticationRequired(grantType)) {
if (request.body.client_id) {
return { clientId: request.body.client_id };
}
if (request.body.client_id) {
return { clientId: request.body.client_id };
}

throw new InvalidClientError('Invalid client: cannot retrieve client credentials');
Expand Down Expand Up @@ -311,7 +303,9 @@ class TokenHandler {
/**
* Given a grant type, check if client authentication is required
*/
isClientAuthenticationRequired(grantType) {
isClientAuthenticationRequired(grantType, client) {
if (client && client.type === 'public') return false;

if (Object.keys(this.requireClientAuthentication).length > 0) {
return typeof this.requireClientAuthentication[grantType] !== 'undefined'
? this.requireClientAuthentication[grantType]
Expand Down
1 change: 1 addition & 0 deletions lib/model.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const ServerError = require('./errors/server-error');
* @property grants {string[]} Grant types allowed for the client.
* @property accessTokenLifetime {number} Client-specific lifetime of generated access tokens in seconds.
* @property refreshTokenLifetime {number} Client-specific lifetime of generated refresh tokens in seconds.
* @property type {string} The client type: `public` or `confidential`. Defaults to 'confidential' when not set.
*/

/**
Expand Down
104 changes: 95 additions & 9 deletions test/integration/handlers/token-handler_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -816,6 +816,98 @@ describe('TokenHandler integration', function () {
.catch(should.fail);
});

describe('with a public client and no `client_secret`', function () {
it('should return the client without requiring a secret', function () {
const client = { id: 'foo', grants: ['authorization_code'], type: 'public' };
const model = Model.from({
getClient: function () {
return client;
},
saveToken: function () {},
});
const handler = new TokenHandler({
accessTokenLifetime: 120,
model: model,
refreshTokenLifetime: 120,
});
const request = new Request({
body: { client_id: 'foo', grant_type: 'authorization_code' },
headers: {},
method: {},
query: {},
});

return handler
.getClient(request)
.then(function (data) {
data.should.equal(client);
})
.catch(should.fail);
});
});

describe('with a confidential client (explicit type) and no `client_secret`', function () {
it('should throw an error', function () {
const client = { id: 'foo', grants: ['authorization_code'], type: 'confidential' };
const model = Model.from({
getClient: function () {
return client;
},
saveToken: function () {},
});
const handler = new TokenHandler({
accessTokenLifetime: 120,
model: model,
refreshTokenLifetime: 120,
});
const request = new Request({
body: { client_id: 'foo', grant_type: 'authorization_code' },
headers: {},
method: {},
query: {},
});

return handler
.getClient(request)
.then(should.fail)
.catch(function (e) {
e.should.be.an.instanceOf(InvalidClientError);
e.message.should.equal('Invalid client: client is invalid');
});
});
});

describe('with a confidential client (no type, default) and no `client_secret`', function () {
it('should throw an error', function () {
const client = { id: 'foo', grants: ['password'] };
const model = Model.from({
getClient: function () {
return client;
},
saveToken: function () {},
});
const handler = new TokenHandler({
accessTokenLifetime: 120,
model: model,
refreshTokenLifetime: 120,
});
const request = new Request({
body: { client_id: 'foo', grant_type: 'password' },
headers: {},
method: {},
query: {},
});

return handler
.getClient(request)
.then(should.fail)
.catch(function (e) {
e.should.be.an.instanceOf(InvalidClientError);
e.message.should.equal('Invalid client: client is invalid');
});
});
});

describe('with `password` grant type and `requireClientAuthentication` is false', function () {
it('should return a client ', function () {
const client = { id: 12345, grants: [] };
Expand Down Expand Up @@ -959,7 +1051,7 @@ describe('TokenHandler integration', function () {
}
});

it('should throw an error if `client_secret` is missing', async function () {
it('should return credentials with only `clientId` when `client_secret` is missing', function () {
const model = Model.from({
getClient: function () {},
saveToken: function () {},
Expand All @@ -976,14 +1068,8 @@ describe('TokenHandler integration', function () {
query: {},
});

try {
await handler.getClientCredentials(request);

should.fail();
} catch (e) {
e.should.be.an.instanceOf(InvalidClientError);
e.message.should.equal('Invalid client: cannot retrieve client credentials');
}
const credentials = handler.getClientCredentials(request);
credentials.should.eql({ clientId: 'foo' });
});

describe('with `client_id` and grant type is `password` and `requireClientAuthentication` is false', function () {
Expand Down
29 changes: 28 additions & 1 deletion test/unit/handlers/token-handler_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ const should = require('chai').should();

describe('TokenHandler', function () {
describe('getClient()', function () {
it('should call `model.getClient()`', function () {
it('should call `model.getClient()` with the provided secret', function () {
const model = Model.from({
getClient: sinon.stub().returns({ grants: ['password'] }),
saveToken: function () {},
Expand Down Expand Up @@ -44,5 +44,32 @@ describe('TokenHandler', function () {
})
.catch(should.fail);
});

it('should call `model.getClient()` when no client secret is provided (public client)', function () {
const model = Model.from({
getClient: sinon.stub().returns({ grants: ['authorization_code'], type: 'public' }),
saveToken: function () {},
});
const handler = new TokenHandler({
accessTokenLifetime: 120,
model: model,
refreshTokenLifetime: 120,
});
const request = new Request({
body: { client_id: 'foo', grant_type: 'authorization_code' },
headers: {},
method: {},
query: {},
});

return handler
.getClient(request)
.then(function () {
model.getClient.callCount.should.equal(1);
model.getClient.firstCall.args[0].should.equal('foo');
should.equal(model.getClient.firstCall.args[1], null);
})
.catch(should.fail);
});
});
});
Loading