diff --git a/package-lock.json b/package-lock.json index 216bb11..44aceff 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,6 +1,6 @@ { "name": "@solid/oidc-op", - "version": "0.3.2", + "version": "0.4.0", "lockfileVersion": 1, "requires": true, "dependencies": { @@ -113,6 +113,15 @@ "to-fast-properties": "^2.0.0" } }, + "@sinonjs/commons": { + "version": "1.0.2", + "resolved": "https://registry.npmjs.org/@sinonjs/commons/-/commons-1.0.2.tgz", + "integrity": "sha512-WR3dlgqJP4QNrLC4iXN/5/2WaLQQ0VijOOkmflqFGVJ6wLEpbSjo7c0ZeGIdtY8Crk7xBBp87sM6+Mkerz7alw==", + "dev": true, + "requires": { + "type-detect": "4.0.8" + } + }, "@sinonjs/formatio": { "version": "2.0.0", "resolved": "https://registry.npmjs.org/@sinonjs/formatio/-/formatio-2.0.0.tgz", @@ -122,6 +131,12 @@ "samsam": "1.3.0" } }, + "@sinonjs/samsam": { + "version": "2.0.0", + "resolved": "https://registry.npmjs.org/@sinonjs/samsam/-/samsam-2.0.0.tgz", + "integrity": "sha512-D7VxhADdZbDJ0HjUTMnSQ5xIGb4H2yWpg8k9Sf1T08zfFiQYlaxM8LZydpR4FQ2E6LZJX8IlabNZ5io4vdChwg==", + "dev": true + }, "@solid/jose": { "version": "0.1.8", "resolved": "https://registry.npmjs.org/@solid/jose/-/jose-0.1.8.tgz", @@ -1643,9 +1658,9 @@ } }, "just-extend": { - "version": "1.1.27", - "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-1.1.27.tgz", - "integrity": "sha512-mJVp13Ix6gFo3SBAy9U/kL+oeZqzlYYYLQBwXVBlVzIsZwBqGREnOro24oC/8s8aox+rJhtZ2DiQof++IrkA+g==", + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/just-extend/-/just-extend-3.0.0.tgz", + "integrity": "sha512-Fu3T6pKBuxjWT/p4DkqGHFRsysc8OauWr4ZRTY9dIx07Y9O0RkoR5jcv28aeD1vuAwhm3nLkDurwLXoALp4DpQ==", "dev": true }, "levn": { @@ -1706,9 +1721,9 @@ "integrity": "sha1-7dFMgk4sycHgsKG0K7UhBRakJDg=" }, "lolex": { - "version": "2.7.0", - "resolved": "https://registry.npmjs.org/lolex/-/lolex-2.7.0.tgz", - "integrity": "sha512-uJkH2e0BVfU5KOJUevbTOtpDduooSarH5PopO+LfM/vZf8Z9sJzODqKev804JYM2i++ktJfUmC1le4LwFQ1VMg==", + "version": "2.7.4", + "resolved": "https://registry.npmjs.org/lolex/-/lolex-2.7.4.tgz", + "integrity": "sha512-Gh6Vffq/piTeHwunLNFR1jFVaqlwK9GMNUxFcsO1cwHyvbRKHwX8UDkxmrDnbcPdHNmpv7z2kxtkkSx5xkNpMw==", "dev": true }, "loose-envify": { @@ -1859,13 +1874,13 @@ "dev": true }, "nise": { - "version": "1.4.1", - "resolved": "https://registry.npmjs.org/nise/-/nise-1.4.1.tgz", - "integrity": "sha512-9JX3YwoIt3kS237scmSSOpEv7vCukVzLfwK0I0XhocDSHUANid8ZHnLEULbbSkfeMn98B2y5kphIWzZUylESRQ==", + "version": "1.4.4", + "resolved": "https://registry.npmjs.org/nise/-/nise-1.4.4.tgz", + "integrity": "sha512-pxE0c9PzgrUTyhfv5p+5eMIdfU2bLEsq8VQEuE0kxM4zP7SujSar7rk9wpI2F7RyyCEvLyj5O7Is3RER5F36Fg==", "dev": true, "requires": { "@sinonjs/formatio": "^2.0.0", - "just-extend": "^1.1.27", + "just-extend": "^3.0.0", "lolex": "^2.3.2", "path-to-regexp": "^1.7.0", "text-encoding": "^0.6.4" @@ -4515,24 +4530,37 @@ "dev": true }, "sinon": { - "version": "5.1.0", - "resolved": "https://registry.npmjs.org/sinon/-/sinon-5.1.0.tgz", - "integrity": "sha512-mKJCMMwRKAPEbM48tgT7rea9+Dos5xugEpVXGwMDCSRtPxF6ZjIBO+WrbixxJ0xcYJ6ZMS3/veTAI7XTW010yA==", + "version": "6.2.0", + "resolved": "https://registry.npmjs.org/sinon/-/sinon-6.2.0.tgz", + "integrity": "sha512-gLFZz5UYvOhYzQ+DBzw/OCkmWaLAHlAyQiE2wxUOmAGVdasP9Yw93E+OwZ0UuhW3ReMu1FKniuNsL6VukvC77w==", "dev": true, "requires": { + "@sinonjs/commons": "^1.0.2", "@sinonjs/formatio": "^2.0.0", + "@sinonjs/samsam": "^2.0.0", "diff": "^3.5.0", "lodash.get": "^4.4.2", - "lolex": "^2.4.2", - "nise": "^1.3.3", - "supports-color": "^5.4.0", + "lolex": "^2.7.2", + "nise": "^1.4.4", + "supports-color": "^5.5.0", "type-detect": "^4.0.8" + }, + "dependencies": { + "supports-color": { + "version": "5.5.0", + "resolved": "https://registry.npmjs.org/supports-color/-/supports-color-5.5.0.tgz", + "integrity": "sha512-QjVjwdXIt408MIiAqCX4oUKsgU2EqAGzs2Ppkm4aQYbjm+ZEWEcW4SfFNTr4uMNZma0ey4f5lgLrkB0aX0QMow==", + "dev": true, + "requires": { + "has-flag": "^3.0.0" + } + } } }, "sinon-chai": { - "version": "3.1.0", - "resolved": "https://registry.npmjs.org/sinon-chai/-/sinon-chai-3.1.0.tgz", - "integrity": "sha512-gKcpH0GpDwEDWq6DXzdKf2PCvK4MgB2x6w1hSaNVQKnGF7vDY+uM7RK15pHqRuleypDKFpLUVDPTvlpyBVV1Mw==", + "version": "3.2.0", + "resolved": "https://registry.npmjs.org/sinon-chai/-/sinon-chai-3.2.0.tgz", + "integrity": "sha512-Z72B4a0l0IQe5uWi9yzcqX/Ml6K9e1Hp03NmkjJnRG3gDsKTX7KvLFZsVUmCaz0eqeXLLK089mwTsP1P1W+DUQ==", "dev": true }, "slice-ansi": { @@ -4886,9 +4914,9 @@ "dev": true }, "whatwg-url": { - "version": "6.4.1", - "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-6.4.1.tgz", - "integrity": "sha512-FwygsxsXx27x6XXuExA/ox3Ktwcbf+OAvrKmLulotDAiO1Q6ixchPFaHYsis2zZBZSJTR0+dR+JVtf7MlbqZjw==", + "version": "6.5.0", + "resolved": "https://registry.npmjs.org/whatwg-url/-/whatwg-url-6.5.0.tgz", + "integrity": "sha512-rhRZRqx/TLJQWUpQ6bmrt2UV4f0HCQ463yQuONJqC6fO2VoEb1pTYddbe59SkYq87aoM5A3bdhMZiUiVws+fzQ==", "requires": { "lodash.sortby": "^4.7.0", "tr46": "^1.0.1", diff --git a/package.json b/package.json index 74b8fcc..cf51ce0 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@solid/oidc-op", - "version": "0.3.2", + "version": "0.4.0", "description": "OpenID Connect Provider", "main": "src/index.js", "directories": { @@ -48,7 +48,7 @@ "base64url": "^3.0.0", "pem-jwk": "^1.5.1", "qs": "^6.5.2", - "whatwg-url": "^6.4.1" + "whatwg-url": "^6.5.0" }, "devDependencies": { "chai": "^4.1.2", @@ -58,8 +58,8 @@ "mocha": "^5.2.0", "node-mocks-http": "^1.7.0", "nyc": "^12.0.2", - "sinon": "^5.1.0", - "sinon-chai": "^3.1.0", + "sinon": "^6.1.0", + "sinon-chai": "^3.2.0", "standard": "^11.0.1" }, "nyc": { @@ -68,5 +68,15 @@ "text-summary" ], "cache": true + }, + "standard": { + "globals": [ + "after", + "afterEach", + "before", + "beforeEach", + "describe", + "it" + ] } } diff --git a/src/handlers/RPInitiatedLogoutRequest.js b/src/handlers/RPInitiatedLogoutRequest.js index 0aa887f..3428779 100644 --- a/src/handlers/RPInitiatedLogoutRequest.js +++ b/src/handlers/RPInitiatedLogoutRequest.js @@ -6,23 +6,42 @@ */ const qs = require('qs') const BaseRequest = require('./BaseRequest') +const IDToken = require('../IDToken') +const DEFAULT_POST_LOGOUT_URI = '/' + +/** + * Session spec defines the following params to the RP Initiated Logout request: + * - `id_token_hint` + * - `post_logout_redirect_uri` + * - `state` + * + * @see https://openid.net/specs/openid-connect-session-1_0.html#RPLogout + * @see https://openid.net/specs/openid-connect-frontchannel-1_0.html#RPInitiated + * @see https://openid.net/specs/openid-connect-backchannel-1_0.html#RPInitiated + */ class RPInitiatedLogoutRequest extends BaseRequest { + /** + * @param req {IncomingRequest} + * @param res {ServerResponse} + * @param provider {Provider} + */ + constructor (req, res, provider) { + super(req, res, provider) + this.params = RPInitiatedLogoutRequest.getParams(this) + } + /** * RP Initiated Logout Request Handler * - * @see https://openid.net/specs/openid-connect-session-1_0.html#RPLogout - * @see https://openid.net/specs/openid-connect-frontchannel-1_0.html#RPInitiated - * @see https://openid.net/specs/openid-connect-backchannel-1_0.html#RPInitiated - * * @param req {HTTPRequest} * @param res {HTTPResponse} * @param provider {Provider} * @returns {Promise} */ static handle (req, res, provider) { - let { host } = provider - let request = new RPInitiatedLogoutRequest(req, res, provider) + const { host } = provider + const request = new RPInitiatedLogoutRequest(req, res, provider) return Promise .resolve(request) @@ -34,25 +53,110 @@ class RPInitiatedLogoutRequest extends BaseRequest { // MUST log out the End-User. .then(host.logout) - .then(request.redirectOrRespond.bind(request)) + .then(request.redirectToGoodbye.bind(request)) .catch(request.error.bind(request)) } /** - * Constructor + * validateIdTokenHint * - * Session spec defines the following params to the RP Initiated Logout request: - * - `id_token_hint` - * - `post_logout_redirect_uri` - * - `state` + * Validates the `id_token_hint` parameter * - * @param req {HTTPRequest} - * @param res {HTTPResponse} - * @param provider {Provider} + * RECOMMENDED. Previously issued ID Token passed to the logout endpoint as + * a hint about the End-User's current authenticated session with the Client. + * This is used as an indication of the identity of the End-User that the RP + * is requesting be logged out by the OP. The OP *need not* be listed as an + * audience of the ID Token when it is used as an `id_token_hint` value. + * + * @param request {RPInitiatedLogoutRequest} + * + * @throws {Error} 400 Bad Request if ID Token hint can't be decoded + * or verified + * + * @returns {Promise} Chainable */ - constructor (req, res, provider) { - super(req, res, provider) - this.params = RPInitiatedLogoutRequest.getParams(this) + async validateIdTokenHint (request) { + const { provider, params } = request + const idTokenHint = params['id_token_hint'] + let decodedHint + + if (!idTokenHint) { + return request + } + + try { + decodedHint = await IDToken.decode(idTokenHint) + } catch (error) { + console.error('Error decoding ID Token hint (', idTokenHint, '):', error) + this.badRequest({error_description: 'Error decoding ID Token hint'}) + } + + if (!decodedHint.resolveKeys(provider.keys.jwks)) { + this.badRequest({ + error_description: 'ID Token hint signing keys cannot be resolved' + }) + } + + try { + await decodedHint.verify() + } catch (cause) { + console.error('Could not verify ID Token hint:', decodedHint) + this.badRequest({error_description: 'Could not verify ID Token hint'}) + } + + request.params.decodedHint = decodedHint + + return request + } + + /** + * Validates that `post_logout_redirect_uri` has been registered + * + * The value MUST have been previously registered with the OP, either using + * the `post_logout_redirect_uris` Registration parameter + * or via another mechanism. + * + * @param request {RPInitiatedLogoutRequest} + * + * @throws {Error} + * + * @returns {Promise} Chainable + */ + async validatePostLogoutUri (request) { + const { provider, params } = request + const { post_logout_redirect_uri: uri } = params + const { decodedHint } = params + + if (!uri) { + return request + } + + if (!decodedHint) { + return this.badRequest({ + error_description: 'post_logout_redirect_uri requires id_token_hint' + }) + } + + // Get the client from the ID Token Hint to validate that the + // post logout redirect URI has been pre-registered + const clientId = decodedHint.payload.azp || decodedHint.payload.aud + + const client = await provider.backend.get('clients', clientId) + if (!client) { + return this.badRequest({ + error_description: 'Invalid ID Token hint (client not found)' + }) + } + + // Check that the post logout uri has been registered + if (!client['post_logout_redirect_uris'].includes(uri)) { + return this.badRequest({ + error_description: 'post_logout_redirect_uri must be pre-registered' + }) + } + + // Valid + return request } /** @@ -61,37 +165,24 @@ class RPInitiatedLogoutRequest extends BaseRequest { * @see https://openid.net/specs/openid-connect-session-1_0.html#RPLogout * * @param request {RPInitiatedLogoutRequest} + * + * @returns {Promise} Chainable */ validate (request) { /** - * `state` parameter - no validation need it. Will be passed back to the RP + * `state` parameter - no validation needed. Will be passed back to the RP * in the `redirectToRP()` step. */ - - /** - * TODO: Validate `id_token_hint` (validate as ID Token *except* for `aud`) - * - * RECOMMENDED. Previously issued ID Token passed to the logout endpoint as - * a hint about the End-User's current authenticated session with the Client. - * This is used as an indication of the identity of the End-User that the RP - * is requesting be logged out by the OP. The OP *need not* be listed as an - * audience of the ID Token when it is used as an `id_token_hint` value. - */ - - /** - * TODO: Validate that `post_logout_redirect_uri` has been registered - * - * The value MUST have been previously registered with the OP, either using - * the `post_logout_redirect_uris` Registration parameter - * or via another mechanism. - * - * Question: what's this about 'another mechanism'? - */ + return Promise.resolve(request) + .then(request.validateIdTokenHint) + .then(request.validatePostLogoutUri) } /** - * Redirect to RP or Respond + * Redirects the user-agent to a post logout URI. Also passes through the + * `state` parameter, if supplied by the RP. * + * From the spec: * In some cases, the RP will request that the End-User's User Agent to be * redirected back to the RP after a logout has been performed. Post-logout * redirection is only done when the logout is RP-initiated, in which case the @@ -100,54 +191,29 @@ class RPInitiatedLogoutRequest extends BaseRequest { * * @see https://openid.net/specs/openid-connect-session-1_0.html#RedirectionAfterLogout * - * @returns {null} + * Implementor's notes: + * For usability reasons, the user should still be redirected somewhere after + * logout, even if no redirect uri was passed in by the RP client (we can't + * just show them a 204/no content). For that reason, we allow the OP host + * config to provide a default `post_logout_redirect_uri` in case none is + * provided. Since this is controlled by the host/OP (and not by the RP), + * this is still respectful of the OAuth2 threat model, and mitigates the + * risk of rogue redirects. */ - redirectOrRespond () { - let { params: { post_logout_redirect_uri: postLogoutRedirectUri } } = this - if (postLogoutRedirectUri) { - this.redirectToRP() - } else { - this.respond() - } - } + redirectToGoodbye () { + const { host, res, params } = this + const { state } = params - /** - * Redirect To RP - * - * Redirects the user-agent back to the RP, if requested (by the RP providing - * a `post_logout_redirect_uri` parameter). Also passes through the `state` - * parameter, if supplied by the RP. - * - * @returns {null} - */ - redirectToRP () { - let { res, params: { post_logout_redirect_uri: uri, state } } = this + let uri = params['post_logout_redirect_uri'] || + host.defaults['post_logout_redirect_uri'] || + DEFAULT_POST_LOGOUT_URI if (state) { - let response = qs.stringify({ state }) - uri = `${uri}?${response}` + const queryString = qs.stringify({ state }) + uri = `${uri}?${queryString}` } - res.redirect(uri) // 302 redirect - } - - /** - * Respond - * - * Responds to the RP Initiated Logout request with a 204 No Content, if the - * RP did not supply a `post_logout_redirect_uri` parameter. - * - * @returns {null} - */ - respond () { - let { res } = this - - res.set({ - 'Cache-Control': 'no-store', - 'Pragma': 'no-cache' - }) - - res.status(204).send() + res.redirect(uri) // 302 redirect } } diff --git a/test/AccessTokenSpec.js b/test/AccessTokenSpec.js index 1dbf549..6062224 100644 --- a/test/AccessTokenSpec.js +++ b/test/AccessTokenSpec.js @@ -32,8 +32,6 @@ describe('AccessToken', () => { var provider before(function () { - this.timeout(10000) - let configPath = path.join(__dirname, 'config', 'provider.json') let storedConfig = JSON.parse(fs.readFileSync(configPath, 'utf8')) diff --git a/test/IDTokenSpec.js b/test/IDTokenSpec.js index b672331..2962b9b 100644 --- a/test/IDTokenSpec.js +++ b/test/IDTokenSpec.js @@ -31,8 +31,6 @@ describe('IDToken', () => { var provider before(function () { - this.timeout(25000) - let configPath = path.join(__dirname, 'config', 'provider.json') let storedConfig = JSON.parse(fs.readFileSync(configPath, 'utf8')) diff --git a/test/handlers/AuthenticationRequestSpec.js b/test/handlers/AuthenticationRequestSpec.js index 9a80b93..e4b4768 100644 --- a/test/handlers/AuthenticationRequestSpec.js +++ b/test/handlers/AuthenticationRequestSpec.js @@ -45,8 +45,6 @@ describe('AuthenticationRequest', () => { let provider, params, req, res, request before(function () { - this.timeout(25000) - let configPath = path.join(__dirname, '..', 'config', 'provider.json') let storedConfig = JSON.parse(fs.readFileSync(configPath, 'utf8')) diff --git a/test/handlers/DynamicRegistrationRequestSpec.js b/test/handlers/DynamicRegistrationRequestSpec.js index 8dcdcd6..1ff7a2c 100644 --- a/test/handlers/DynamicRegistrationRequestSpec.js +++ b/test/handlers/DynamicRegistrationRequestSpec.js @@ -34,8 +34,6 @@ describe('DynamicRegistrationRequest', () => { let request before(function () { - this.timeout(25000) - let configPath = path.join(__dirname, '..', 'config', 'provider.json') let storedConfig = JSON.parse(fs.readFileSync(configPath, 'utf8')) diff --git a/test/handlers/JWKSetRequestSpec.js b/test/handlers/JWKSetRequestSpec.js index 5dc176f..9d36d0d 100644 --- a/test/handlers/JWKSetRequestSpec.js +++ b/test/handlers/JWKSetRequestSpec.js @@ -27,8 +27,6 @@ describe('JWKSetRequest', () => { let req, res, provider before(function () { - this.timeout(25000) - req = HttpMocks.createRequest() res = HttpMocks.createResponse() diff --git a/test/handlers/RPInitiatedLogoutRequestSpec.js b/test/handlers/RPInitiatedLogoutRequestSpec.js index a4ef28d..05de46a 100644 --- a/test/handlers/RPInitiatedLogoutRequestSpec.js +++ b/test/handlers/RPInitiatedLogoutRequestSpec.js @@ -10,13 +10,27 @@ chai.should() const HttpMocks = require('node-mocks-http') const RPInitiatedLogoutRequest = require('../../src/handlers/RPInitiatedLogoutRequest') +const IDToken = require('../../src/IDToken') const provider = { host: { - logout: () => {} + logout: () => {}, + defaults: { + post_logout_redirect_uri: '/goodbye' + } } } +async function issueIdToken (oidcProvider) { + const jwt = IDToken.issue(oidcProvider, { + sub: 'user123', + aud: 'https://op.example.com', + azp: 'client123' + }) + + return jwt.encode() +} + const postLogoutRedirectUri = 'https://rp.example.com/goodbye' const reqNoParams = HttpMocks.createRequest({ method: 'GET', params: {} }) const reqWithParams = HttpMocks.createRequest({ @@ -30,27 +44,25 @@ const reqWithParams = HttpMocks.createRequest({ describe('RPInitiatedLogoutRequest', () => { describe('handle()', () => { - it('should invoke injected host.logout', done => { + it('should invoke injected host.logout', () => { let res = HttpMocks.createResponse() let logoutSpy = sinon.stub(provider.host, 'logout').resolves() - RPInitiatedLogoutRequest.handle(reqNoParams, res, provider) + return RPInitiatedLogoutRequest.handle(reqNoParams, res, provider) .then(() => { expect(logoutSpy).to.have.been.called() - done() }) }) }) describe('constructor()', () => { - it('should parse the incoming request params', done => { + it('should parse the incoming request params', () => { let res = {} let request = new RPInitiatedLogoutRequest(reqWithParams, res, provider) expect(request).to.have.property('params') expect(Object.keys(request.params).length).to.equal(3) expect(request.params.state).to.equal('abc123') - done() }) }) @@ -59,41 +71,34 @@ describe('RPInitiatedLogoutRequest', () => { it('should validate that `post_logout_redirect_uri` has been registered') }) - describe('redirectOrRespond()', () => { - it('should redirect to RP if logout uri provided', done => { + describe('redirectToGoodbye()', () => { + it('should redirect to RP if logout uri provided', () => { let res = HttpMocks.createResponse() let req = HttpMocks.createRequest({ method: 'GET', query: { - 'post_logout_redirect_uri': postLogoutRedirectUri + 'post_logout_redirect_uri': postLogoutRedirectUri, + 'state': '$tate' } }) let request = new RPInitiatedLogoutRequest(req, res, provider) - request.respond = sinon.stub().throws() - request.redirectOrRespond() + request.redirectToGoodbye() - expect(request.respond).to.not.have.been.called() expect(res.statusCode).to.equal(302) - expect(res._getRedirectUrl()).to.equal(postLogoutRedirectUri) - done() + expect(res._getRedirectUrl()) + .to.equal(postLogoutRedirectUri + '?state=%24tate') }) - it('should respond with a 204 if no logout uri provided', done => { + it('should redirect to host default if no RP logout uri provided', () => { let res = HttpMocks.createResponse() let request = new RPInitiatedLogoutRequest(reqNoParams, res, provider) - request.redirectToRP = sinon.stub().throws() - request.redirectOrRespond() + request.redirectToGoodbye() - expect(request.redirectToRP).to.not.have.been.called() - expect(res.statusCode).to.equal(204) - done() + expect(res.statusCode).to.equal(302) + expect(res._getRedirectUrl()) + .to.equal(provider.host.defaults.post_logout_redirect_uri) }) }) - - describe('redirectToRP()', () => { - it('should redirect to provided URI') - it('should pass through the state param') - }) }) diff --git a/test/mocha.opts b/test/mocha.opts index 4a52320..974a9cc 100644 --- a/test/mocha.opts +++ b/test/mocha.opts @@ -1 +1,2 @@ --recursive +--timeout 20000