From ae759d39ea5a503b3869b024fedb31593587dee7 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 16 Apr 2018 17:55:21 +0100 Subject: [PATCH] Fixed multiple 401s caused by token revocation requests no issue - disabled Ember Simple Auth's default token revocation - we trigger session invalidation on a 401 which means our token isn't valid so the revoke requests will also fail - renamed application route's `invalidateSession` to `logout` in order to distinguish it from any ESA methods - added the token revocation requests to this action, we can be fairly sure at this point that the current tokens will be valid so the requests will succeed - added check to `ajax.handleResponse` so that we don't invalidate the session for requests to external services - removed pointless assertion from the ajax integration test --- ghost/admin/app/authenticators/oauth2.js | 7 ++-- ghost/admin/app/routes/application.js | 33 ++++++++++++++++--- ghost/admin/app/routes/signout.js | 4 +-- ghost/admin/app/services/ajax.js | 20 ++++++----- .../tests/integration/services/ajax-test.js | 3 -- 5 files changed, 47 insertions(+), 20 deletions(-) diff --git a/ghost/admin/app/authenticators/oauth2.js b/ghost/admin/app/authenticators/oauth2.js index 5b0a0b82ef..a0c8cbce60 100644 --- a/ghost/admin/app/authenticators/oauth2.js +++ b/ghost/admin/app/authenticators/oauth2.js @@ -26,9 +26,10 @@ export default Authenticator.extend({ return `${this.get('ghostPaths.apiRoot')}/authentication/token`; }), - serverTokenRevocationEndpoint: computed('ghostPaths.apiRoot', function () { - return `${this.get('ghostPaths.apiRoot')}/authentication/revoke`; - }), + // disable general token revocation because the requests will always 401 + // (revocation is triggered by invalid access token so it's already invalid) + // we have a separate logout procedure that sends revocation requests + serverTokenRevocationEndpoint: null, makeRequest(url, data) { /* eslint-disable camelcase */ diff --git a/ghost/admin/app/routes/application.js b/ghost/admin/app/routes/application.js index 997e78e32f..652f0b648c 100644 --- a/ghost/admin/app/routes/application.js +++ b/ghost/admin/app/routes/application.js @@ -30,8 +30,10 @@ shortcuts.esc = {action: 'closeMenus', scope: 'default'}; shortcuts[`${ctrlOrCmd}+s`] = {action: 'save', scope: 'all'}; export default Route.extend(ApplicationRouteMixin, ShortcutsRoute, { + ajax: service(), config: service(), feature: service(), + ghostPaths: service(), notifications: service(), settings: service(), tour: service(), @@ -106,10 +108,33 @@ export default Route.extend(ApplicationRouteMixin, ShortcutsRoute, { this.send('loadServerNotifications', true); }, - invalidateSession() { - this.get('session').invalidate().catch((error) => { - this.get('notifications').showAlert(error.message, {type: 'error', key: 'session.invalidate.failed'}); - }); + // this is only called by the `signout` route at present. + // it's separate to the normal ESA session invalidadition because it will + // actually send the token revocation requests whereas we have to avoid + // those most of the time because they will fail if we have invalid tokens + logout() { + let session = this.get('session'); + // revoke keys on the server + if (session.get('isAuthenticated')) { + let auth = session.get('data.authenticated'); + let revokeEndpoint = `${this.get('ghostPaths.apiRoot')}/authentication/revoke`; + let authenticator = session.get('session')._lookupAuthenticator(session.get('session.authenticator')); + let requests = []; + ['refresh_token', 'access_token'].forEach((tokenType) => { + let data = { + token_type_hint: tokenType, + token: auth[tokenType] + }; + authenticator.makeRequest(revokeEndpoint, data); + }); + RSVP.all(requests).finally(() => { + // remove local keys and refresh + session.invalidate(); + }); + } else { + // remove local keys and refresh + session.invalidate(); + } }, authorizationFailed() { diff --git a/ghost/admin/app/routes/signout.js b/ghost/admin/app/routes/signout.js index fa76d3795c..50d5fece73 100644 --- a/ghost/admin/app/routes/signout.js +++ b/ghost/admin/app/routes/signout.js @@ -16,9 +16,9 @@ export default AuthenticatedRoute.extend(styleBody, { afterModel(model, transition) { this.get('notifications').clearAll(); if (canInvoke(transition, 'send')) { - transition.send('invalidateSession'); + transition.send('logout'); } else { - this.send('invalidateSession'); + this.send('logout'); } } }); diff --git a/ghost/admin/app/services/ajax.js b/ghost/admin/app/services/ajax.js index e47db7af66..2661657811 100644 --- a/ghost/admin/app/services/ajax.js +++ b/ghost/admin/app/services/ajax.js @@ -7,13 +7,15 @@ import {isArray as isEmberArray} from '@ember/array'; import {isNone} from '@ember/utils'; import {inject as service} from '@ember/service'; -const JSONContentType = 'application/json'; +const JSON_CONTENT_TYPE = 'application/json'; +const GHOST_REQUEST = /\/ghost\/api\//; +const TOKEN_REQUEST = /authentication\/(?:token|ghost|revoke)/; function isJSONContentType(header) { if (!header || isNone(header)) { return false; } - return header.indexOf(JSONContentType) === 0; + return header.indexOf(JSON_CONTENT_TYPE) === 0; } /* Version mismatch error */ @@ -135,8 +137,8 @@ let ajaxService = AjaxService.extend({ // and formats appropriately, we want to handle `application/json` the same _makeRequest(hash) { let isAuthenticated = this.get('session.isAuthenticated'); - let isGhostRequest = hash.url.indexOf('/ghost/api/') !== -1; - let isTokenRequest = isGhostRequest && hash.url.match(/authentication\/(?:token|ghost)/); + let isGhostRequest = GHOST_REQUEST.test(hash.url); + let isTokenRequest = isGhostRequest && TOKEN_REQUEST.test(hash.url); let tokenExpiry = this.get('session.authenticated.expires_at'); let isTokenExpired = tokenExpiry < (new Date()).getTime(); @@ -167,7 +169,7 @@ let ajaxService = AjaxService.extend({ return this._super(...arguments); }, - handleResponse(status, headers, payload) { + handleResponse(status, headers, payload, request) { if (this.isVersionMismatchError(status, headers, payload)) { return new VersionMismatchError(payload); } else if (this.isServerUnreachableError(status, headers, payload)) { @@ -182,9 +184,11 @@ let ajaxService = AjaxService.extend({ return new ThemeValidationError(payload); } - // TODO: we may want to check that we are hitting our own API before - // logging the user out due to a 401 response - if (this.isUnauthorizedError(status, headers, payload) && this.get('session.isAuthenticated')) { + let isGhostRequest = GHOST_REQUEST.test(request.url); + let isAuthenticated = this.get('session.isAuthenticated'); + let isUnauthorized = this.isUnauthorizedError(status, headers, payload); + + if (isAuthenticated && isGhostRequest && isUnauthorized) { this.get('session').invalidate(); } diff --git a/ghost/admin/tests/integration/services/ajax-test.js b/ghost/admin/tests/integration/services/ajax-test.js index b265676343..3803752e7c 100644 --- a/ghost/admin/tests/integration/services/ajax-test.js +++ b/ghost/admin/tests/integration/services/ajax-test.js @@ -177,8 +177,6 @@ describe('Integration: Service: ajax', function () { /* eslint-disable camelcase */ describe('session handling', function () { - let successfulRequest = false; - let sessionStub = Service.extend({ isAuthenticated: true, restoreCalled: false, @@ -261,7 +259,6 @@ describe('Integration: Service: ajax', function () { // TODO: fix the error return when a session restore fails // expect(isUnauthorizedError(error)).to.be.true; expect(ajax.get('session.restoreCalled'), 'restoreCalled').to.be.true; - expect(successfulRequest, 'successfulRequest').to.be.false; expect(invalidateCalled, 'invalidateCalled').to.be.true; done(); });