diff --git a/lib/actions/authorization/fetch_request_uri.js b/lib/actions/authorization/fetch_request_uri.js index a8c264c0b..8ddea63db 100644 --- a/lib/actions/authorization/fetch_request_uri.js +++ b/lib/actions/authorization/fetch_request_uri.js @@ -1,7 +1,9 @@ const { URL } = require('url'); const assert = require('assert'); -const { InvalidRequestUri } = require('../../helpers/errors'); +const { + InvalidRequest, InvalidRequestUri, RequestNotSupported, RequestUriNotSupported, +} = require('../../helpers/errors'); const instance = require('../../helpers/weak_cache'); const allowedSchemes = new Set(['http:', 'https:', 'urn:']); @@ -12,12 +14,35 @@ const allowedSchemes = new Set(['http:', 'https:', 'urn:']); * uses the response body as a value for the request parameter to be validated by a downstream * middleware * + * + * @throws: invalid_request * @throws: invalid_request_uri - * @see: RequestUriCache - * @see: decodeRequest + * @throws: request_not_supported + * @throws: request_uri_not_supported */ module.exports = async function fetchRequestUri(ctx, next) { - const { params } = ctx.oidc; + const { request, requestUri } = instance(ctx.oidc.provider).configuration('features'); + const { client, params } = ctx.oidc; + + if (!request.enabled && params.request !== undefined) { + throw new RequestNotSupported(); + } + + if (!requestUri.enabled && params.request_uri !== undefined) { + throw new RequestUriNotSupported(); + } + + if (params.request !== undefined && params.request_uri !== undefined) { + throw new InvalidRequest('request and request_uri parameters MUST NOT be used together'); + } + + if ( + client.requestObjectSigningAlg + && params.request === undefined + && params.request_uri === undefined + ) { + throw new InvalidRequest('request or request_uri must be provided for this client'); + } if (params.request_uri !== undefined) { let protocol; diff --git a/lib/actions/authorization/index.js b/lib/actions/authorization/index.js index bcda680a2..13d661fc7 100644 --- a/lib/actions/authorization/index.js +++ b/lib/actions/authorization/index.js @@ -10,11 +10,11 @@ const getTokenAuth = require('../../shared/token_auth'); const checkClient = require('./check_client'); const checkResponseMode = require('./check_response_mode'); -const throwNotSupported = require('./throw_not_supported'); +const rejectRegistration = require('./reject_registration'); const oauthRequired = require('./oauth_required'); const oneRedirectUriClients = require('./one_redirect_uri_clients'); const fetchRequestUri = require('./fetch_request_uri'); -const decodeRequest = require('./decode_request'); +const processRequestObject = require('./process_request_object'); const oidcRequired = require('./oidc_required'); const checkPrompt = require('./check_prompt'); const checkMaxAge = require('./check_max_age'); @@ -119,10 +119,10 @@ module.exports = function authorizationAction(provider, endpoint) { use(() => rejectDupesMiddleware, A, DA ); use(() => checkClientGrantType, DA ); use(() => checkResponseMode, A ); - use(() => throwNotSupported, A, DA ); use(() => oauthRequired, A ); use(() => fetchRequestUri, A, DA ); - use(() => decodeRequest.bind(_, whitelist), A, DA ); + use(() => processRequestObject.bind(_, whitelist), A, DA ); + use(() => rejectRegistration, A, DA ); use(() => oidcRequired, A ); use(() => assignDefaults, A, DA ); use(() => checkOpenidScope.bind(_, whitelist), A, DA ); diff --git a/lib/actions/authorization/decode_request.js b/lib/actions/authorization/process_request_object.js similarity index 79% rename from lib/actions/authorization/decode_request.js rename to lib/actions/authorization/process_request_object.js index 3eb817253..99829d7a9 100644 --- a/lib/actions/authorization/decode_request.js +++ b/lib/actions/authorization/process_request_object.js @@ -2,7 +2,9 @@ const { isPlainObject } = require('lodash'); const JWT = require('../../helpers/jwt'); const instance = require('../../helpers/weak_cache'); -const { InvalidRequest, InvalidRequestObject } = require('../../helpers/errors'); +const { InvalidRequestObject } = require('../../helpers/errors'); + +const checkResponseMode = require('./check_response_mode'); /* * Decrypts and validates the content of provided request parameter and replaces the parameters @@ -10,18 +12,15 @@ const { InvalidRequest, InvalidRequestObject } = require('../../helpers/errors') * * @throws: invalid_request_object */ -module.exports = async function decodeRequest(PARAM_LIST, ctx, next) { - const { keystore, configuration: conf } = instance(ctx.oidc.provider); +module.exports = async function processRequestObject(PARAM_LIST, ctx, next) { const { params, client } = ctx.oidc; - let trusted = false; // signed or encrypted by client confidential material - if (params.request === undefined) { - if (client.requestObjectSigningAlg) { - throw new InvalidRequest('request or request_uri must be provided for this client'); - } return next(); } + const { keystore, configuration: conf } = instance(ctx.oidc.provider); + let trusted = false; // signed or encrypted by client confidential material + if (conf('features.encryption.enabled') && params.request.split('.').length === 5) { try { const header = JWT.header(params.request); @@ -57,15 +56,40 @@ module.exports = async function decodeRequest(PARAM_LIST, ctx, next) { const { payload, header: { alg } } = decoded; - if (payload.request !== undefined || payload.request_uri !== undefined) { + const request = Object.entries(payload).reduce((acc, [key, value]) => { + if (PARAM_LIST.has(key)) { + if (key === 'claims' && isPlainObject(value)) { + acc[key] = JSON.stringify(value); + } else if (key === 'resource' && Array.isArray(value) && conf('features.resourceIndicators.enabled')) { + acc[key] = value; + } else if (typeof value !== 'string') { + acc[key] = String(value); + } else { + acc[key] = value; + } + } + + return acc; + }, {}); + + if (request.state !== undefined) { + params.state = request.state; + } + + if (request.response_mode !== undefined) { + params.response_mode = request.response_mode; + checkResponseMode(ctx, () => {}); + } + + if (request.request !== undefined || request.request_uri !== undefined) { throw new InvalidRequestObject('request object must not contain request or request_uri properties'); } - if (PARAM_LIST.has('response_type') && payload.response_type !== undefined && payload.response_type !== params.response_type) { + if (request.response_type !== undefined && request.response_type !== params.response_type) { throw new InvalidRequestObject('request response_type must equal the one in request parameters'); } - if (payload.client_id !== undefined && payload.client_id !== params.client_id) { + if (request.client_id !== undefined && request.client_id !== params.client_id) { throw new InvalidRequestObject('request client_id must equal the one in request parameters'); } @@ -77,14 +101,17 @@ module.exports = async function decodeRequest(PARAM_LIST, ctx, next) { throw new InvalidRequestObject('unsupported signed request alg'); } - if (alg !== 'none') { + const opts = { + issuer: payload.iss ? client.clientId : undefined, + audience: payload.aud ? ctx.oidc.issuer : undefined, + clockTolerance: conf('clockTolerance'), + ignoreAzp: true, + }; + + if (alg === 'none') { + JWT.assertPayload(payload, opts); + } else { try { - const opts = { - issuer: payload.iss ? client.clientId : undefined, - audience: payload.aud ? ctx.oidc.issuer : undefined, - clockTolerance: conf('clockTolerance'), - ignoreAzp: true, - }; await JWT.verify(params.request, client.keystore, opts); trusted = true; } catch (err) { @@ -102,27 +129,12 @@ module.exports = async function decodeRequest(PARAM_LIST, ctx, next) { } } - const request = Object.entries(payload).reduce((acc, [key, value]) => { - if (PARAM_LIST.has(key)) { - if (key === 'claims' && isPlainObject(value)) { - acc[key] = JSON.stringify(value); - } else if (key === 'resource' && Array.isArray(value) && conf('features.resourceIndicators')) { - acc[key] = value; - } else if (typeof value !== 'string') { - acc[key] = String(value); - } else { - acc[key] = value; - } - } - - return acc; - }, {}); - if (trusted) { ctx.oidc.signed = Object.keys(request); } else if (ctx.oidc.insecureRequestUri) { throw new InvalidRequestObject('request object from unsecure request_uri must be signed and/or symmetrically encrypted'); } + Object.assign(params, request); params.request = undefined; diff --git a/lib/actions/authorization/reject_registration.js b/lib/actions/authorization/reject_registration.js new file mode 100644 index 000000000..31cbc361d --- /dev/null +++ b/lib/actions/authorization/reject_registration.js @@ -0,0 +1,14 @@ +const { RegistrationNotSupported } = require('../../helpers/errors'); + +/* + * Rejects registration parameter as not supported. + * + * @throws: registration_not_supported + */ +module.exports = function rejectRegistration(ctx, next) { + if (ctx.oidc.params.registration !== undefined) { + throw new RegistrationNotSupported(); + } + + return next(); +}; diff --git a/lib/actions/authorization/throw_not_supported.js b/lib/actions/authorization/throw_not_supported.js deleted file mode 100644 index f358c4ba0..000000000 --- a/lib/actions/authorization/throw_not_supported.js +++ /dev/null @@ -1,39 +0,0 @@ -const { - InvalidRequest, - RegistrationNotSupported, - RequestNotSupported, - RequestUriNotSupported, -} = require('../../helpers/errors'); -const instance = require('../../helpers/weak_cache'); - -/* - * Validates parameters that rely features that are not supported by the provider configuration - * are not provided and that request and request_uri are not used in conjunction. - * - * @throws: invalid_request - * @throws: request_not_supported - * @throws: request_uri_not_supported - * @throws: registration_not_supported - */ -module.exports = async function throwNotSupported(ctx, next) { - const { params } = ctx.oidc; - const features = instance(ctx.oidc.provider).configuration('features'); - - if (!features.request.enabled && params.request !== undefined) { - throw new RequestNotSupported(); - } - - if (!features.requestUri.enabled && params.request_uri !== undefined) { - throw new RequestUriNotSupported(); - } - - if (params.registration !== undefined) { - throw new RegistrationNotSupported(); - } - - if (params.request !== undefined && params.request_uri !== undefined) { - throw new InvalidRequest('request and request_uri parameters MUST NOT be used together'); - } - - await next(); -}; diff --git a/test/encryption/encryption.test.js b/test/encryption/encryption.test.js index b2097c0c9..26dee0f6e 100644 --- a/test/encryption/encryption.test.js +++ b/test/encryption/encryption.test.js @@ -214,7 +214,7 @@ const route = '/auth'; response_type: 'id_token', nonce: 'foobar', redirect_uri: 'https://client.example.com/cb', - }, null, 'none', { issuer: 'client', audience: this.provider.issuer }).then(signed => JWT.encrypt(signed, client.keystore.get({ alg: 'A128KW' }), { enc: 'A128CBC-HS256', alg: 'A128KW' })).then(encrypted => this.wrap({ + }, null, 'none', { issuer: 'clientSymmetric', audience: this.provider.issuer }).then(signed => JWT.encrypt(signed, client.keystore.get({ alg: 'A128KW' }), { enc: 'A128CBC-HS256', alg: 'A128KW' })).then(encrypted => this.wrap({ route, verb, auth: { diff --git a/test/request/jwt_request.test.js b/test/request/jwt_request.test.js index c28dfcccf..3f836d4b4 100644 --- a/test/request/jwt_request.test.js +++ b/test/request/jwt_request.test.js @@ -338,16 +338,41 @@ describe('request parameter features', () => { })); }); - if (route !== '/device/auth') { - it('can contain resource parameter as an Array', async function () { - const spy = sinon.spy(); - this.provider.once(successEvt, spy); + it('can contain resource parameter as an Array', async function () { + const spy = sinon.spy(); + this.provider.once(successEvt, spy); - await JWT.sign({ + await JWT.sign({ + client_id: 'client', + response_type: 'code', + redirect_uri: 'https://client.example.com/cb', + resource: ['https://rp.example.com/api'], + }, null, 'none', { issuer: 'client', audience: this.provider.issuer }).then(request => this.wrap({ + agent: this.agent, + route, + verb, + auth: { + request, + scope: 'openid', client_id: 'client', response_type: 'code', + }, + }) + .expect(successCode) + .expect(successFnCheck)); + + expect( + spy.calledWithMatch({ oidc: { params: { resource: ['https://rp.example.com/api'] } } }), + ).to.be.true; + }); + + if (route !== '/device/auth') { + it('may contain a response_mode and it will be honoured', function () { + return JWT.sign({ + client_id: 'client', + response_type: 'code', + response_mode: 'fragment', redirect_uri: 'https://client.example.com/cb', - resource: ['https://rp.example.com/api'], }, null, 'none', { issuer: 'client', audience: this.provider.issuer }).then(request => this.wrap({ agent: this.agent, route, @@ -359,12 +384,40 @@ describe('request parameter features', () => { response_type: 'code', }, }) + .expect(this.AuthorizationRequest.prototype.validateFragment) .expect(successCode) .expect(successFnCheck)); + }); - expect( - spy.calledWithMatch({ oidc: { params: { resource: ['https://rp.example.com/api'] } } }), - ).to.be.true; + it('re-checks the response mode from the request', function () { + const spy = sinon.spy(); + this.provider.once(errorEvt, spy); + + return JWT.sign({ + client_id: 'client2', + response_type: 'code', + redirect_uri: 'https://client.example.com/cb', + response_mode: 'foo', + }, null, 'none', { issuer: 'client2', audience: this.provider.issuer }).then(request => this.wrap({ + agent: this.agent, + route, + verb, + auth: { + request, + scope: 'openid', + client_id: 'client', + response_type: 'code', + }, + }) + .expect(errorCode) + .expect(() => { + expect(spy.calledOnce).to.be.true; + expect(spy.args[0][1]).to.have.property('message', 'unsupported_response_mode'); + expect(spy.args[0][1]).to.have.property( + 'error_description', + 'unsupported response_mode requested', + ); + })); }); it('doesnt allow response_type to differ', function () { @@ -396,6 +449,38 @@ describe('request parameter features', () => { ); })); }); + + it('uses the state from the request even if its validations will fail', function () { + const spy = sinon.spy(); + this.provider.once(errorEvt, spy); + + return JWT.sign({ + client_id: 'client2', + response_type: 'code', + redirect_uri: 'https://client.example.com/cb', + state: 'foobar', + }, null, 'none', { issuer: 'client2', audience: this.provider.issuer }).then(request => this.wrap({ + agent: this.agent, + route, + verb, + auth: { + request, + scope: 'openid', + client_id: 'client', + response_type: 'code', + }, + }) + .expect(this.AuthorizationRequest.prototype.validateResponseParameter.call({}, 'state', 'foobar')) + .expect(errorCode) + .expect(() => { + expect(spy.calledOnce).to.be.true; + expect(spy.args[0][1]).to.have.property('message', 'invalid_request_object'); + expect(spy.args[0][1]).to.have.property( + 'error_description', + 'request client_id must equal the one in request parameters', + ); + })); + }); } it('doesnt allow client_id to differ', function () { @@ -540,6 +625,37 @@ describe('request parameter features', () => { })); }); + it('rejects "registration" parameter part of the request object', function () { + const spy = sinon.spy(); + this.provider.once(errorEvt, spy); + + return JWT.sign({ + client_id: 'client', + response_type: 'code', + redirect_uri: 'https://client.example.com/cb', + registration: 'foo', + }, null, 'none', { issuer: 'client', audience: this.provider.issuer }).then(request => this.wrap({ + agent: this.agent, + route, + verb, + auth: { + request, + scope: 'openid', + client_id: 'client', + response_type: 'code', + }, + }) + .expect(errorCode) + .expect(() => { + expect(spy.calledOnce).to.be.true; + expect(spy.args[0][1]).to.have.property('message', 'registration_not_supported'); + expect(spy.args[0][1]).to.have.property( + 'error_description', + 'registration parameter provided but not supported', + ); + })); + }); + it('handles unrecognized parameters', async function () { const key = (await this.provider.Client.find('client-with-HS-sig')).keystore.get({ alg: 'HS256',