From 0853b5724470bc7df3f2090fbc4d97d0626049e2 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Mon, 10 Jul 2017 11:18:19 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20prevent=20session=20sync=20issue?= =?UTF-8?q?s=20with=20multiple=20tabs/refreshes=20(#772)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refs https://github.com/TryGhost/Ghost/issues/8616 - only refresh tokens on boot if we last refreshed more than 24hrs ago - this should prevent rapidly changing access/refresh tokens when opening new admin tabs or refreshing whilst other tabs are open - fix token refresh test which was testing it's own behaviour instead of the applications 🙈 This may not be the full solution to the session issues but it closes one potential culprit and should at least reduce token churn which can only help track down the real cause. --- ghost/admin/app/routes/application.js | 22 +++++--- ghost/admin/mirage/config/authentication.js | 12 ++--- .../tests/acceptance/authentication-test.js | 53 ++++++++++++++++--- .../tests/acceptance/settings/tags-test.js | 6 +++ ghost/admin/tests/acceptance/signin-test.js | 4 +- 5 files changed, 74 insertions(+), 23 deletions(-) diff --git a/ghost/admin/app/routes/application.js b/ghost/admin/app/routes/application.js index a0b0057356..f7f954fca0 100644 --- a/ghost/admin/app/routes/application.js +++ b/ghost/admin/app/routes/application.js @@ -6,6 +6,7 @@ import Route from 'ember-route'; import ShortcutsRoute from 'ghost-admin/mixins/shortcuts-route'; import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd'; import injectService from 'ember-service/inject'; +import moment from 'moment'; import observer from 'ember-metal/observer'; import run from 'ember-runloop'; import windowProxy from 'ghost-admin/utils/window-proxy'; @@ -48,14 +49,21 @@ export default Route.extend(ApplicationRouteMixin, ShortcutsRoute, { transition.send('loadServerNotifications'); transition.send('checkForOutdatedDesktopApp'); - // trigger a background refresh of the access token to enable - // "infinite" sessions. We also trigger a logout if the refresh - // token is invalid to prevent attackers with only the access token - // from loading the admin + // trigger a background token refresh to enable "infinite" sessions + // NOTE: we only do this if the last refresh was > 1 day ago to avoid + // potential issues with multiple tabs and concurrent admin loads/refreshes. + // see https://github.com/TryGhost/Ghost/issues/8616 let session = this.get('session.session'); - let authenticator = session._lookupAuthenticator(session.authenticator); - if (authenticator && authenticator.onOnline) { - authenticator.onOnline(); + let expiresIn = session.get('authenticated.expires_in') * 1000; + let expiresAt = session.get('authenticated.expires_at'); + let lastRefresh = moment(expiresAt - expiresIn); + let oneDayAgo = moment().subtract(1, 'day'); + + if (lastRefresh.isBefore(oneDayAgo)) { + let authenticator = session._lookupAuthenticator(session.authenticator); + if (authenticator && authenticator.onOnline) { + authenticator.onOnline(); + } } let featurePromise = this.get('feature').fetch().then(() => { diff --git a/ghost/admin/mirage/config/authentication.js b/ghost/admin/mirage/config/authentication.js index 4a5011c707..36631b43ae 100644 --- a/ghost/admin/mirage/config/authentication.js +++ b/ghost/admin/mirage/config/authentication.js @@ -15,16 +15,16 @@ export default function mockAuthentication(server) { } return { - access_token: '5JhTdKI7PpoZv4ROsFoERc6wCHALKFH5jxozwOOAErmUzWrFNARuH1q01TYTKeZkPW7FmV5MJ2fU00pg9sm4jtH3Z1LjCf8D6nNqLYCfFb2YEKyuvG7zHj4jZqSYVodN2YTCkcHv6k8oJ54QXzNTLIDMlCevkOebm5OjxGiJpafMxncm043q9u1QhdU9eee3zouGRMVVp8zkKVoo5zlGMi3zvS2XDpx7xsfk8hKHpUgd7EDDQxmMueifWv7hv6n', - expires_in: 3600, - refresh_token: 'XP13eDjwV5mxOcrq1jkIY9idhdvN3R1Br5vxYpYIub2P5Hdc8pdWMOGmwFyoUshiEB62JWHTl8H1kACJR18Z8aMXbnk5orG28br2kmVgtVZKqOSoiiWrQoeKTqrRV0t7ua8uY5HdDUaKpnYKyOdpagsSPn3WEj8op4vHctGL3svOWOjZhq6F2XeVPMR7YsbiwBE8fjT3VhTB3KRlBtWZd1rE0Qo2EtSplWyjGKv1liAEiL0ndQoLeeSOCH4rTP7' + access_token: 'MirageAccessToken', + expires_in: 172800, + refresh_token: 'MirageRefreshToken' }; } else { // Password sign-in return { - access_token: '5JhTdKI7PpoZv4ROsFoERc6wCHALKFH5jxozwOOAErmUzWrFNARuH1q01TYTKeZkPW7FmV5MJ2fU00pg9sm4jtH3Z1LjCf8D6nNqLYCfFb2YEKyuvG7zHj4jZqSYVodN2YTCkcHv6k8oJ54QXzNTLIDMlCevkOebm5OjxGiJpafMxncm043q9u1QhdU9eee3zouGRMVVp8zkKVoo5zlGMi3zvS2XDpx7xsfk8hKHpUgd7EDDQxmMueifWv7hv6n', - expires_in: 3600, - refresh_token: 'XP13eDjwV5mxOcrq1jkIY9idhdvN3R1Br5vxYpYIub2P5Hdc8pdWMOGmwFyoUshiEB62JWHTl8H1kACJR18Z8aMXbnk5orG28br2kmVgtVZKqOSoiiWrQoeKTqrRV0t7ua8uY5HdDUaKpnYKyOdpagsSPn3WEj8op4vHctGL3svOWOjZhq6F2XeVPMR7YsbiwBE8fjT3VhTB3KRlBtWZd1rE0Qo2EtSplWyjGKv1liAEiL0ndQoLeeSOCH4rTP7', + access_token: 'MirageAccessToken', + expires_in: 172800, + refresh_token: 'MirageRefreshToken', token_type: 'Bearer' }; } diff --git a/ghost/admin/tests/acceptance/authentication-test.js b/ghost/admin/tests/acceptance/authentication-test.js index 51b692c764..be31645db0 100644 --- a/ghost/admin/tests/acceptance/authentication-test.js +++ b/ghost/admin/tests/acceptance/authentication-test.js @@ -48,12 +48,26 @@ describe('Acceptance: Authentication', function () { server.create('user', {roles: [role], slug: 'test-user'}); }); - it('refreshes app tokens on boot', async function () { + it('refreshes tokens on boot if last refreshed > 24hrs ago', async function () { /* eslint-disable camelcase */ - authenticateSession(application, { - access_token: 'testAccessToken', - refresh_token: 'refreshAccessToken' + // the tokens here don't matter, we're using the actual oauth + // authenticator so we get the tokens back from the mirage endpoint + await authenticateSession(application, { + access_token: 'access_token', + refresh_token: 'refresh_token' }); + + // authenticating the session above will trigger a token refresh + // request so we need to clear it to ensure we aren't testing the + // test behaviour instead of application behaviour + server.pretender.handledRequests = []; + + // fake a longer session so it appears that we last refreshed > 24hrs ago + let {__container__: container} = application; + let {session} = container.lookup('service:session'); + let newSession = session.get('content'); + newSession.authenticated.expires_in = 172800 * 2; + session.get('store').persist(newSession); /* eslint-enable camelcase */ await visit('/'); @@ -61,13 +75,36 @@ describe('Acceptance: Authentication', function () { let requests = server.pretender.handledRequests; let refreshRequest = requests.findBy('url', '/ghost/api/v0.1/authentication/token'); - expect(refreshRequest).to.exist; + expect(refreshRequest, 'token refresh request').to.exist; expect(refreshRequest.method, 'method').to.equal('POST'); let requestBody = $.deparam(refreshRequest.requestBody); - expect(requestBody.grant_type, 'grant_type').to.equal('password'); - expect(requestBody.username.access_token, 'access_token').to.equal('testAccessToken'); - expect(requestBody.username.refresh_token, 'refresh_token').to.equal('refreshAccessToken'); + expect(requestBody.grant_type, 'grant_type').to.equal('refresh_token'); + expect(requestBody.refresh_token, 'refresh_token').to.equal('MirageRefreshToken'); + }); + + it('doesn\'t refresh tokens on boot if last refreshed < 24hrs ago', async function () { + /* eslint-disable camelcase */ + // the tokens here don't matter, we're using the actual oauth + // authenticator so we get the tokens back from the mirage endpoint + await authenticateSession(application, { + access_token: 'access_token', + refresh_token: 'refresh_token' + }); + /* eslint-enable camelcase */ + + // authenticating the session above will trigger a token refresh + // request so we need to clear it to ensure we aren't testing the + // test behaviour instead of application behaviour + server.pretender.handledRequests = []; + + // we've only just refreshed tokens above so we should always be < 24hrs + await visit('/'); + + let requests = server.pretender.handledRequests; + let refreshRequest = requests.findBy('url', '/ghost/api/v0.1/authentication/token'); + + expect(refreshRequest, 'refresh request').to.not.exist; }); }); diff --git a/ghost/admin/tests/acceptance/settings/tags-test.js b/ghost/admin/tests/acceptance/settings/tags-test.js index c420d23bef..96b070b9f7 100644 --- a/ghost/admin/tests/acceptance/settings/tags-test.js +++ b/ghost/admin/tests/acceptance/settings/tags-test.js @@ -10,6 +10,7 @@ import {afterEach, beforeEach, describe, it} from 'mocha'; import {authenticateSession, invalidateSession} from 'ghost-admin/tests/helpers/ember-simple-auth'; import {errorOverride, errorReset} from 'ghost-admin/tests/helpers/adapter-error'; import {expect} from 'chai'; +import {timeout} from 'ember-concurrency'; // Grabbed from keymaster's testing code because Ember's `keyEvent` helper // is for some reason not triggering the events in a way that keymaster detects: @@ -183,6 +184,11 @@ describe('Acceptance: Settings - Tags', function () { await fillIn('.tag-settings-pane input[name="name"]', 'New Tag'); await triggerEvent('.tag-settings-pane input[name="name"]', 'blur'); + // extra timeout needed for FF on Linux - sometimes it doesn't update + // quick enough, especially on Travis, and an extra wait() call + // doesn't help + await timeout(100); + // it redirects to the new tag's URL expect(currentURL(), 'URL after tag creation').to.equal('/settings/tags/new-tag'); diff --git a/ghost/admin/tests/acceptance/signin-test.js b/ghost/admin/tests/acceptance/signin-test.js index 166a201479..d036d09e38 100644 --- a/ghost/admin/tests/acceptance/signin-test.js +++ b/ghost/admin/tests/acceptance/signin-test.js @@ -58,9 +58,9 @@ describe('Acceptance: Signin', function() { if (password === 'testpass') { return { - access_token: '5JhTdKI7PpoZv4ROsFoERc6wCHALKFH5jxozwOOAErmUzWrFNARuH1q01TYTKeZkPW7FmV5MJ2fU00pg9sm4jtH3Z1LjCf8D6nNqLYCfFb2YEKyuvG7zHj4jZqSYVodN2YTCkcHv6k8oJ54QXzNTLIDMlCevkOebm5OjxGiJpafMxncm043q9u1QhdU9eee3zouGRMVVp8zkKVoo5zlGMi3zvS2XDpx7xsfk8hKHpUgd7EDDQxmMueifWv7hv6n', + access_token: 'MirageAccessToken', expires_in: 3600, - refresh_token: 'XP13eDjwV5mxOcrq1jkIY9idhdvN3R1Br5vxYpYIub2P5Hdc8pdWMOGmwFyoUshiEB62JWHTl8H1kACJR18Z8aMXbnk5orG28br2kmVgtVZKqOSoiiWrQoeKTqrRV0t7ua8uY5HdDUaKpnYKyOdpagsSPn3WEj8op4vHctGL3svOWOjZhq6F2XeVPMR7YsbiwBE8fjT3VhTB3KRlBtWZd1rE0Qo2EtSplWyjGKv1liAEiL0ndQoLeeSOCH4rTP7', + refresh_token: 'MirageRefreshToken', token_type: 'Bearer' }; } else {