From a019fc9208dd47eac944e8d77de9daf4346cb9bd Mon Sep 17 00:00:00 2001 From: Filip Skokan Date: Sun, 18 Aug 2019 13:52:21 +0200 Subject: [PATCH] feat: basic and post client auth methods are now interchangeable Background: Many developers struggle to understand that this is a configuration on the client and their SDKs rarely surface this, combined with the fact that many SDKs default to POST while the defined default should be basic this wastes developer's time. This makes the two client auth methods interchangeable, so if the client is configured with basic but post is provided, it works and vice versa. --- lib/shared/load_client.js | 15 -------- lib/shared/token_auth.js | 52 ++++++++++++++++------------ lib/shared/token_credential_auth.js | 2 +- test/client_auth/client_auth.test.js | 40 +++++++++++++++------ 4 files changed, 60 insertions(+), 49 deletions(-) delete mode 100644 lib/shared/load_client.js diff --git a/lib/shared/load_client.js b/lib/shared/load_client.js deleted file mode 100644 index 226be6420..000000000 --- a/lib/shared/load_client.js +++ /dev/null @@ -1,15 +0,0 @@ -const { InvalidClientAuth } = require('../helpers/errors'); - -module.exports = function getLoadClient(provider) { - return async function loadClient(ctx, next) { - const client = await provider.Client.find(ctx.oidc.authorization.clientId); - - if (!client) { - throw new InvalidClientAuth('client not found'); - } - - ctx.oidc.entity('Client', client); - - await next(); - }; -}; diff --git a/lib/shared/token_auth.js b/lib/shared/token_auth.js index 8d0fc18ab..b25c29db1 100644 --- a/lib/shared/token_auth.js +++ b/lib/shared/token_auth.js @@ -5,7 +5,6 @@ const instance = require('../helpers/weak_cache'); const { 'x5t#S256': thumbprint } = require('../helpers/calculate_thumbprint'); const rejectDupes = require('./reject_dupes'); -const loadClient = require('./load_client'); const tokenCredentialAuth = require('./token_credential_auth'); const getJWTAuthMiddleware = require('./token_jwt_auth'); @@ -83,15 +82,25 @@ module.exports = function tokenAuth(provider, endpoint, jwtAuthEndpointIdentifie } catch (err) { throw new InvalidRequest('client_id and client_secret in the authorization header are not properly encoded'); } - ctx.oidc.authorization.methods = ['client_secret_basic']; if (clientId !== undefined && ctx.oidc.authorization.clientId !== clientId) { throw new InvalidRequest('mismatch in body and authorization client ids'); } - if (clientSecret !== undefined) throw new InvalidRequest('client authentication must only be provided using one mechanism'); + + if (!ctx.oidc.authorization.clientSecret) { + throw new InvalidRequest('client_secret must be provided in the Authorization header'); + } + + if (clientSecret !== undefined) { + throw new InvalidRequest('client authentication must only be provided using one mechanism'); + } + + ctx.oidc.authorization.methods = ['client_secret_basic', 'client_secret_post']; } else if (clientId !== undefined) { ctx.oidc.authorization.clientId = clientId; - ctx.oidc.authorization.methods = ['client_secret_post', 'none', 'tls_client_auth', 'self_signed_tls_client_auth']; + ctx.oidc.authorization.methods = clientSecret + ? ['client_secret_basic', 'client_secret_post'] + : ['none', 'tls_client_auth', 'self_signed_tls_client_auth']; } if (clientAssertion !== undefined) { @@ -128,7 +137,17 @@ module.exports = function tokenAuth(provider, endpoint, jwtAuthEndpointIdentifie return next(); }, - loadClient(provider), + async function loadClient(ctx, next) { + const client = await provider.Client.find(ctx.oidc.authorization.clientId); + + if (!client) { + throw new InvalidClientAuth('client not found'); + } + + ctx.oidc.entity('Client', client); + + await next(); + }, async function auth(ctx, next) { const { params, @@ -148,18 +167,16 @@ module.exports = function tokenAuth(provider, endpoint, jwtAuthEndpointIdentifie switch (clientMethod) { // eslint-disable-line default-case case 'none': - if (params.client_secret) { - throw new InvalidClientAuth(`unexpected client_secret provided for ${endpoint}_endpoint_auth_method=none client request`); - } break; - case 'client_secret_post': - if (!params.client_secret) { - throw new InvalidClientAuth('client_secret must be provided in the body'); - } - tokenCredentialAuth(ctx, ctx.oidc.client.clientSecret, params.client_secret); + case 'client_secret_basic': + case 'client_secret_post': { + const expected = ctx.oidc.client.clientSecret; + const actual = params.client_secret || clientSecret; + tokenCredentialAuth(ctx, actual, expected); break; + } case 'client_secret_jwt': await tokenJwtAuth( @@ -177,15 +194,6 @@ module.exports = function tokenAuth(provider, endpoint, jwtAuthEndpointIdentifie break; - case 'client_secret_basic': - if (!clientSecret) { - throw new InvalidClientAuth('client_secret must be provided in the Authorization header'); - } - - tokenCredentialAuth(ctx, ctx.oidc.client.clientSecret, clientSecret); - - break; - case 'tls_client_auth': { const { mTLS: { getCertificate, certificateAuthorized, certificateSubjectMatches } } = instance(provider).configuration('features'); diff --git a/lib/shared/token_credential_auth.js b/lib/shared/token_credential_auth.js index 78793c35d..9c220f036 100644 --- a/lib/shared/token_credential_auth.js +++ b/lib/shared/token_credential_auth.js @@ -1,7 +1,7 @@ const constantEquals = require('../helpers/constant_equals'); const { InvalidClientAuth } = require('../helpers/errors'); -module.exports = function tokenCredentialAuth(ctx, expected, actual) { +module.exports = function tokenCredentialAuth(ctx, actual, expected) { if (!constantEquals(expected, actual, 1000)) { throw new InvalidClientAuth('invalid secret provided'); } diff --git a/test/client_auth/client_auth.test.js b/test/client_auth/client_auth.test.js index 5988bea3a..fc46e4856 100644 --- a/test/client_auth/client_auth.test.js +++ b/test/client_auth/client_auth.test.js @@ -189,7 +189,7 @@ describe('client authentication options', () => { .type('form') .expect(() => { expect(spy.calledOnce).to.be.true; - expect(errorDetail(spy)).to.equal('unexpected client_secret provided for token_endpoint_auth_method=none client request'); + expect(errorDetail(spy)).to.equal('the registered client token_endpoint_auth_method does not match the provided auth mechanism'); }) .expect(401) .expect(tokenAuthRejected); @@ -207,6 +207,16 @@ describe('client authentication options', () => { .expect(tokenAuthSucceeded); }); + it('accepts the auth (but client configured with post)', function () { + return this.agent.post(route) + .send({ + grant_type: 'implicit', + }) + .type('form') + .auth('client-post', 'secret') + .expect(tokenAuthSucceeded); + }); + it('accepts the auth even with id in the body', function () { return this.agent.post(route) .send({ @@ -253,7 +263,7 @@ describe('client authentication options', () => { .expect(tokenAuthSucceeded); }); - it('accepts the auth (https://tools.ietf.org/html/rfc6749#appendix-B)', function () { + it('rejects improperly encoded headers', function () { return this.agent.post(route) .send({ grant_type: 'implicit', @@ -357,20 +367,17 @@ describe('client authentication options', () => { }); it('requires the client_secret to be sent', function () { - const spy = sinon.spy(); - this.provider.once('grant.error', spy); return this.agent.post(route) .send({ grant_type: 'implicit', }) .type('form') .auth('client-basic', '') - .expect(() => { - expect(spy.calledOnce).to.be.true; - expect(errorDetail(spy)).to.equal('client_secret must be provided in the Authorization header'); - }) - .expect(401) - .expect(tokenAuthRejected); + .expect(400) + .expect({ + error: 'invalid_request', + error_description: 'client_secret must be provided in the Authorization header', + }); }); }); @@ -386,6 +393,17 @@ describe('client authentication options', () => { .expect(tokenAuthSucceeded); }); + it('accepts the auth (but client configured with basic)', function () { + return this.agent.post(route) + .send({ + grant_type: 'implicit', + client_id: 'client-basic', + client_secret: 'secret', + }) + .type('form') + .expect(tokenAuthSucceeded); + }); + it('rejects invalid secrets', function () { const spy = sinon.spy(); this.provider.once('grant.error', spy); @@ -416,7 +434,7 @@ describe('client authentication options', () => { .type('form') .expect(() => { expect(spy.calledOnce).to.be.true; - expect(errorDetail(spy)).to.equal('client_secret must be provided in the body'); + expect(errorDetail(spy)).to.equal('the registered client token_endpoint_auth_method does not match the provided auth mechanism'); }) .expect(401) .expect(tokenAuthRejected);