diff --git a/docs/README.md b/docs/README.md index ef372b441..dedef1544 100644 --- a/docs/README.md +++ b/docs/README.md @@ -457,6 +457,7 @@ location / { - [userinfo](#featuresuserinfo) - [webMessageResponseMode](#featureswebmessageresponsemode) - [acrValues](#acrvalues) +- [allowOmittingSingleRegisteredRedirectUri](#allowomittingsingleregisteredredirecturi) - [audiences](#audiences) - [claims ❗](#claims) - [clientBasedCORS](#clientbasedcors) @@ -1926,6 +1927,16 @@ _**default value**_: [] ``` +### allowOmittingSingleRegisteredRedirectUri + +Allow omitting the redirect_uri parameter when only a single one is registered for a client. + + +_**default value**_: +```js +false +``` + ### audiences Function used to set an audience to issued Access Tokens. The return value should be either: diff --git a/lib/actions/authorization/one_redirect_uri_clients.js b/lib/actions/authorization/one_redirect_uri_clients.js index 2dc675868..11cc00f17 100644 --- a/lib/actions/authorization/one_redirect_uri_clients.js +++ b/lib/actions/authorization/one_redirect_uri_clients.js @@ -1,8 +1,14 @@ +const instance = require('../../helpers/weak_cache'); + /* * If no redirect_uri is provided and client only pre-registered one unique value it is assumed * to be the requested redirect_uri and used as if it was explicitly provided; */ module.exports = function oneRedirectUriClients(ctx, next) { + if (!instance(ctx.oidc.provider).configuration('allowOmittingSingleRegisteredRedirectUri')) { + return next(); + } + const { params, client } = ctx.oidc; if (params.redirect_uri === undefined && client.redirectUris.length === 1) { diff --git a/lib/actions/grants/authorization_code.js b/lib/actions/grants/authorization_code.js index 983ba9d4f..dbc75177a 100644 --- a/lib/actions/grants/authorization_code.js +++ b/lib/actions/grants/authorization_code.js @@ -12,11 +12,12 @@ module.exports.handler = async function authorizationCodeHandler(ctx, next) { const { issueRefreshToken, audiences, + allowOmittingSingleRegisteredRedirectUri, conformIdTokenClaims, features: { userinfo, dPoP: { iatTolerance }, mTLS: { getCertificate } }, } = instance(ctx.oidc.provider).configuration(); - if (ctx.oidc.params.redirect_uri === undefined) { + if (allowOmittingSingleRegisteredRedirectUri && ctx.oidc.params.redirect_uri === undefined) { // It is permitted to omit the redirect_uri if only ONE is registered on the client const { 0: uri, length } = ctx.oidc.client.redirectUris; if (uri && length === 1) { diff --git a/lib/helpers/defaults.js b/lib/helpers/defaults.js index 917b29186..9d1ab3f2c 100644 --- a/lib/helpers/defaults.js +++ b/lib/helpers/defaults.js @@ -640,6 +640,13 @@ function getDefaults() { */ conformIdTokenClaims: true, + /* + * allowOmittingSingleRegisteredRedirectUri + * + * title: Allow omitting the redirect_uri parameter when only a single one is registered for a client. + */ + allowOmittingSingleRegisteredRedirectUri: false, + /* * acceptQueryParamAccessTokens * diff --git a/test/authorization_code/authorization_code.config.js b/test/authorization_code/authorization_code.config.js index ab92af886..dd787e198 100644 --- a/test/authorization_code/authorization_code.config.js +++ b/test/authorization_code/authorization_code.config.js @@ -1,4 +1,8 @@ -const config = require('../default.config'); +const cloneDeep = require('lodash/cloneDeep'); + +const config = cloneDeep(require('../default.config')); + +config.allowOmittingSingleRegisteredRedirectUri = false; module.exports = { config, diff --git a/test/authorization_code/code.grant.test.js b/test/authorization_code/code.grant.test.js index 697df81a9..6663cc3e5 100644 --- a/test/authorization_code/code.grant.test.js +++ b/test/authorization_code/code.grant.test.js @@ -24,7 +24,7 @@ describe('grant_type=authorization_code', () => { this.provider.removeAllListeners('server_error'); }); - context('with real tokens (1/2) - more than two redirect_uris registered', () => { + context('with real tokens (1/3) - more than one redirect_uris registered', () => { before(function () { return this.login(); }); after(function () { return this.logout(); }); @@ -258,6 +258,21 @@ describe('grant_type=authorization_code', () => { }); }); + it('validates redirect_uri presence', function () { + return this.agent.post(route) + .auth('client', 'secret') + .send({ + code: this.ac, + grant_type: 'authorization_code', + }) + .type('form') + .expect(400) + .expect((response) => { + expect(response.body).to.have.property('error', 'invalid_request'); + expect(response.body).to.have.property('error_description', "missing required parameter 'redirect_uri'"); + }); + }); + it('validates account is still there', function () { sinon.stub(this.provider.Account, 'findAccount').callsFake(() => Promise.resolve()); @@ -286,9 +301,52 @@ describe('grant_type=authorization_code', () => { }); }); - context('with real tokens (2/2) - one redirect_uri registered', () => { - before(function () { return this.login(); }); - after(function () { return this.logout(); }); + context('with real tokens (2/3) - one redirect_uri registered with allowOmittingSingleRegisteredRedirectUri=false', () => { + before(async function () { + await this.login(); + return this.agent.get('/auth') + .query({ + client_id: 'client2', + scope: 'openid', + response_type: 'code', + redirect_uri: 'https://client.example.com/cb3', + }) + .expect(302) + .expect((response) => { + const { query: { code } } = parseUrl(response.headers.location, true); + this.ac = code; + }); + }); + + it('validates redirect_uri presence', function () { + const spy = sinon.spy(); + this.provider.on('grant.error', spy); + + return this.agent.post(route) + .auth('client', 'secret') + .send({ + code: this.ac, + grant_type: 'authorization_code', + }) + .type('form') + .expect(400) + .expect((response) => { + expect(response.body).to.have.property('error', 'invalid_request'); + expect(response.body).to.have.property('error_description', "missing required parameter 'redirect_uri'"); + }); + }); + }); + + context('with real tokens (3/3) - one redirect_uri registered with allowOmittingSingleRegisteredRedirectUri=true', () => { + before(function () { + i(this.provider).configuration().allowOmittingSingleRegisteredRedirectUri = true; + return this.login(); + }); + + after(function () { + i(this.provider).configuration().allowOmittingSingleRegisteredRedirectUri = false; + return this.logout(); + }); beforeEach(function () { return this.agent.get('/auth') diff --git a/test/core/basic/basic.config.js b/test/core/basic/basic.config.js index 8a7979fe8..5699f5014 100644 --- a/test/core/basic/basic.config.js +++ b/test/core/basic/basic.config.js @@ -7,6 +7,7 @@ const { Prompt, Check, base } = require('../../../lib/helpers/interaction_policy config.extraParams = ['triggerCustomFail']; merge(config.features, { requestObjects: { requestUri: false } }); config.responseTypes = ['id_token', 'code', 'none']; +config.allowOmittingSingleRegisteredRedirectUri = false; const policy = base(); diff --git a/test/core/basic/code.authorization.test.js b/test/core/basic/code.authorization.test.js index ff7be8d9a..2d4374478 100644 --- a/test/core/basic/code.authorization.test.js +++ b/test/core/basic/code.authorization.test.js @@ -531,6 +531,56 @@ describe('BASIC code', () => { }); }); + context('when client has a single redirect_uri', () => { + after(async function () { + i(this.provider).configuration().allowOmittingSingleRegisteredRedirectUri = false; + await this.logout(); + }); + + it('missing mandatory parameter redirect_uri', function () { + const emitSpy = sinon.spy(); + const renderSpy = sinon.spy(i(this.provider).configuration(), 'renderError'); + this.provider.once('authorization.error', emitSpy); + const auth = new this.AuthorizationRequest({ + response_type, + scope, + }); + delete auth.redirect_uri; + + return this.wrap({ route, verb, auth }) + .expect(() => { + renderSpy.restore(); + }) + .expect(400) + .expect(() => { + expect(emitSpy.calledOnce).to.be.true; + expect(renderSpy.calledOnce).to.be.true; + const renderArgs = renderSpy.args[0]; + expect(renderArgs[1]).to.have.property('error', 'invalid_request'); + expect(renderArgs[1]).to.have.property('error_description', 'missing required parameter \'redirect_uri\''); + expect(renderArgs[2]).to.be.an.instanceof(InvalidRequest); + }); + }); + + it('unless allowOmittingSingleRegisteredRedirectUri is true', async function () { + i(this.provider).configuration().allowOmittingSingleRegisteredRedirectUri = true; + await this.login(); + const auth = new this.AuthorizationRequest({ + response_type, + scope, + client_id: 'client', + }); + + delete auth.redirect_uri; + + return this.wrap({ route, verb, auth }) + .expect(302) + .expect(auth.validatePresence(['code', 'state'])) + .expect(auth.validateState) + .expect(auth.validateClientLocation); + }); + }); + context('when client has more then one redirect_uri', () => { before(async function () { const client = await this.provider.Client.find('client'); diff --git a/test/default.config.js b/test/default.config.js index 5b79dc4f3..900523f3c 100644 --- a/test/default.config.js +++ b/test/default.config.js @@ -53,4 +53,5 @@ module.exports = merge({ ], features: {}, enabledJWA: cloneDeep(JWA), + allowOmittingSingleRegisteredRedirectUri: true, }, global.TEST_CONFIGURATION_DEFAULTS); diff --git a/test/test_helper.js b/test/test_helper.js index 85121ef8f..11fa486a9 100644 --- a/test/test_helper.js +++ b/test/test_helper.js @@ -192,8 +192,14 @@ module.exports = function testHelper(dir, { Object.defineProperty(this, 'validateClientLocation', { value: (response) => { - const expected = parse(this.redirect_uri, true); const actual = parse(response.headers.location, true); + let expected + if (this.redirect_uri) { + expected = parse(this.redirect_uri, true); + } else { + expected = parse(c && c.redirect_uris[0], true); + } + ['protocol', 'host', 'pathname'].forEach((attr) => { expect(actual[attr]).to.equal(expected[attr]); }); diff --git a/types/index.d.ts b/types/index.d.ts index b623845e8..5f5a177f8 100644 --- a/types/index.d.ts +++ b/types/index.d.ts @@ -1040,6 +1040,8 @@ export interface Configuration { renderError?: (ctx: KoaContextWithOIDC, out: ErrorOut, error: errors.OIDCProviderError | Error) => CanBePromise; + allowOmittingSingleRegisteredRedirectUri?: boolean; + interactions?: { policy?: interactionPolicy.Prompt[]; url?: (ctx: KoaContextWithOIDC, interaction: Interaction) => CanBePromise;