feat: omitting redirect_uri for clients with a single one is now opti…

…onal BREAKING CHANGE: Allowing to omit a redirect_uri parameter for clients with a single one registered is now disabled by default. You can re-enable this using the `allowOmittingSingleRegisteredRedirectUri` configuration option.
panva · Dec 3, 2020 · 329c577 · 329c577
1 parent afcb375
commit 329c577
Show file tree

Hide file tree

Showing 11 changed files with 154 additions and 7 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -1040,6 +1040,8 @@ export interface Configuration {
 
   renderError?: (ctx: KoaContextWithOIDC, out: ErrorOut, error: errors.OIDCProviderError | Error) => CanBePromise<void | undefined>;
 
+  allowOmittingSingleRegisteredRedirectUri?: boolean;
+
   interactions?: {
     policy?: interactionPolicy.Prompt[];
     url?: (ctx: KoaContextWithOIDC, interaction: Interaction) => CanBePromise<string>;