From ff7c4c705df123cacb06d4c00bbc8e890894faa2 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 22:44:19 +0800 Subject: [PATCH 01/15] add _reAuthenticateModalToggle property to control closed/reopen re-authenticate modal --- ghost/admin/app/controllers/editor.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/ghost/admin/app/controllers/editor.js b/ghost/admin/app/controllers/editor.js index 13a7c6d50d7..71df4f0a002 100644 --- a/ghost/admin/app/controllers/editor.js +++ b/ghost/admin/app/controllers/editor.js @@ -124,6 +124,7 @@ export default class EditorController extends Controller { _leaveConfirmed = false; _previousTagNames = null; // set by setPost and _postSaved, used in hasDirtyAttributes + _reAuthenticateModalToggle = false; /* computed properties ---------------------------------------------------*/ @@ -476,6 +477,9 @@ export default class EditorController extends Controller { post.set('statusScratch', null); + // Clear any error notification (if any) + this.notifications.clearAll(); + if (!options.silent) { this._showSaveNotification(prevStatus, post.get('status'), isNew ? true : false); } @@ -490,6 +494,11 @@ export default class EditorController extends Controller { return post; } catch (error) { + if (!this.session.isAuthenticated && !this._reAuthenticateModalToggle) { + this.toggleProperty('showReAuthenticateModal'); + } + + this._reAuthenticateModalToggle = false; if (this.showReAuthenticateModal) { this._reauthSave = true; this._reauthSaveOptions = options; From 7ad58930e308ca03bd1a83f19f3287ec8f136add Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 22:47:55 +0800 Subject: [PATCH 02/15] re-enable editor test. --- .../tests/acceptance/authentication-test.js | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/ghost/admin/tests/acceptance/authentication-test.js b/ghost/admin/tests/acceptance/authentication-test.js index 36bf6910ab8..d7d5240017a 100644 --- a/ghost/admin/tests/acceptance/authentication-test.js +++ b/ghost/admin/tests/acceptance/authentication-test.js @@ -1,8 +1,9 @@ +import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd'; import windowProxy from 'ghost-admin/utils/window-proxy'; import {Response} from 'miragejs'; import {afterEach, beforeEach, describe, it} from 'mocha'; import {authenticateSession, invalidateSession} from 'ember-simple-auth/test-support'; -import {click, currentRouteName, currentURL, fillIn, findAll, visit} from '@ember/test-helpers'; +import {currentRouteName, currentURL, fillIn, findAll, triggerKeyEvent, visit} from '@ember/test-helpers'; import {expect} from 'chai'; import {run} from '@ember/runloop'; import {setupApplicationTest} from 'ember-mocha'; @@ -94,7 +95,7 @@ describe('Acceptance: Authentication', function () { }); // TODO: re-enable once modal reappears correctly - describe.skip('editor', function () { + describe('editor', function () { let origDebounce = run.debounce; let origThrottle = run.throttle; @@ -107,13 +108,14 @@ describe('Acceptance: Authentication', function () { it('displays re-auth modal attempting to save with invalid session', async function () { let role = this.server.create('role', {name: 'Administrator'}); this.server.create('user', {roles: [role]}); + let testOn = 'save'; // use marker for different type of server.put result // simulate an invalid session when saving the edited post - this.server.put('/posts/:id/', function ({posts}, {params}) { + this.server.put('/posts/:id/', function ({posts, db}, {params}) { let post = posts.find(params.id); - let attrs = this.normalizedRequestAttrs(); + let attrs = db.posts.find(params.id); // use attribute from db.posts to avoid hasInverseFor error - if (attrs.mobiledoc.cards[0][1].markdown === 'Edited post body') { + if (testOn === 'edit') { return new Response(401, {}, { errors: [ {message: 'Access denied.', type: 'UnauthorizedError'} @@ -129,9 +131,12 @@ describe('Acceptance: Authentication', function () { await visit('/editor'); // create the post - await fillIn('#entry-title', 'Test Post'); + await fillIn('.gh-editor-title', 'Test Post'); await fillIn('.__mobiledoc-editor', 'Test post body'); - await click('.js-publish-button'); + await triggerKeyEvent('.gh-editor-title', 'keydown', 83, { + metaKey: ctrlOrCmd === 'command', + ctrlKey: ctrlOrCmd === 'ctrl' + }); // we shouldn't have a modal at this point expect(findAll('.modal-container #login').length, 'modal exists').to.equal(0); @@ -140,7 +145,10 @@ describe('Acceptance: Authentication', function () { // update the post await fillIn('.__mobiledoc-editor', 'Edited post body'); - await click('.js-publish-button'); + await triggerKeyEvent('.gh-editor-title', 'keydown', 83, { + metaKey: ctrlOrCmd === 'command', + ctrlKey: ctrlOrCmd === 'ctrl' + }); // we should see a re-auth modal expect(findAll('.fullscreen-modal #login').length, 'modal exists').to.equal(1); From 53e53953b75e1fa208f10ed41630d2bfab697ea4 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 22:48:48 +0800 Subject: [PATCH 03/15] add 403 error coverage test --- .../tests/acceptance/authentication-test.js | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/ghost/admin/tests/acceptance/authentication-test.js b/ghost/admin/tests/acceptance/authentication-test.js index d7d5240017a..1bfabc13a7b 100644 --- a/ghost/admin/tests/acceptance/authentication-test.js +++ b/ghost/admin/tests/acceptance/authentication-test.js @@ -69,6 +69,27 @@ describe('Acceptance: Authentication', function () { expect(currentURL(), 'url after 401').to.equal('/signin'); }); + it('invalidates session on 403 API response', async function () { + // return a 401 when attempting to retrieve users + this.server.get('/users/', () => new Response(403, {}, { + errors: [ + {message: 'Authorization failed', type: 'NoPermissionError'} + ] + })); + + await authenticateSession(); + await visit('/settings/staff'); + + // running `visit(url)` inside windowProxy.replaceLocation breaks + // the async behaviour so we need to run `visit` here to simulate + // the browser visiting the new page + if (newLocation) { + await visit(newLocation); + } + + expect(currentURL(), 'url after 403').to.equal('/signin'); + }); + it('doesn\'t show navigation menu on invalid url when not authenticated', async function () { await invalidateSession(); From 9cabdb81b0fcfeddb1aaa2236cd40c114c176073 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 22:56:58 +0800 Subject: [PATCH 04/15] fix toggleReAuthenticateModal --- ghost/admin/app/controllers/editor.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ghost/admin/app/controllers/editor.js b/ghost/admin/app/controllers/editor.js index 71df4f0a002..9a07018d891 100644 --- a/ghost/admin/app/controllers/editor.js +++ b/ghost/admin/app/controllers/editor.js @@ -265,6 +265,8 @@ export default class EditorController extends Controller { @action toggleReAuthenticateModal() { + this._reAuthenticateModalToggle = true; + if (this.showReAuthenticateModal) { // closing, re-attempt save if needed if (this._reauthSave) { From 49425484d2220b4567a49322b8bbde9686be741f Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 22:58:40 +0800 Subject: [PATCH 05/15] re-setup session if user data is available in session store (authenticated) so that redirection will flow to desired location instead of sign in page --- ghost/admin/app/services/session.js | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index a62c5608529..d869fa1730a 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -22,6 +22,7 @@ export default class SessionService extends ESASessionService { @tracked user = null; skipAuthSuccessHandler = false; + forceTransition = false; async populateUser(options = {}) { if (this.user) { @@ -75,6 +76,30 @@ export default class SessionService extends ESASessionService { }); } + async requireAuthentication(transition, route) { + if (!this.isAuthenticated) { + // try to re-setup session if user data is still available + await this.reSetupSession(transition); + } + + super.requireAuthentication(transition, route); + } + + // TODO: feels a bit hacky to re-setup session if user data is available + async reSetupSession(transition) { + if (this.user) { + await this.setup(); + this.forceTransition = true; + this.notifications.clearAll(); + } + + // retry previous transition if there is active session + if (this.forceTransition) { + transition.retry(); + return; + } + } + handleInvalidation() { let transition = this.appLoadTransition; From 6596d9acc3e96a0af96217d52f117b94eac24750 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 23:06:38 +0800 Subject: [PATCH 06/15] fix authentication-test.js --- ghost/admin/tests/acceptance/authentication-test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ghost/admin/tests/acceptance/authentication-test.js b/ghost/admin/tests/acceptance/authentication-test.js index 1bfabc13a7b..cde32146fe6 100644 --- a/ghost/admin/tests/acceptance/authentication-test.js +++ b/ghost/admin/tests/acceptance/authentication-test.js @@ -165,6 +165,7 @@ describe('Acceptance: Authentication', function () { expect(findAll('.gh-alert').length, 'no of alerts').to.equal(0); // update the post + testOn = 'edit'; await fillIn('.__mobiledoc-editor', 'Edited post body'); await triggerKeyEvent('.gh-editor-title', 'keydown', 83, { metaKey: ctrlOrCmd === 'command', From 1cacfb3c45ecb1a889bf32fa0802727b61171241 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 23:31:17 +0800 Subject: [PATCH 07/15] apply same changes to lexical-editor.js --- ghost/admin/app/controllers/lexical-editor.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ghost/admin/app/controllers/lexical-editor.js b/ghost/admin/app/controllers/lexical-editor.js index 0f14fdb88fb..918b15be2e1 100644 --- a/ghost/admin/app/controllers/lexical-editor.js +++ b/ghost/admin/app/controllers/lexical-editor.js @@ -120,6 +120,7 @@ export default class LexicalEditorController extends Controller { _leaveConfirmed = false; _previousTagNames = null; // set by setPost and _postSaved, used in hasDirtyAttributes + _reAuthenticateModalToggle = false; /* computed properties ---------------------------------------------------*/ @@ -260,6 +261,8 @@ export default class LexicalEditorController extends Controller { @action toggleReAuthenticateModal() { + this._reAuthenticateModalToggle = true; + if (this.showReAuthenticateModal) { // closing, re-attempt save if needed if (this._reauthSave) { @@ -473,6 +476,9 @@ export default class LexicalEditorController extends Controller { post.set('statusScratch', null); + // Clear any error notification (if any) + this.notifications.clearAll(); + if (!options.silent) { this._showSaveNotification(prevStatus, post.get('status'), isNew ? true : false); } @@ -487,6 +493,11 @@ export default class LexicalEditorController extends Controller { return post; } catch (error) { + if (!this.session.isAuthenticated && !this._reAuthenticateModalToggle) { + this.toggleProperty('showReAuthenticateModal'); + } + + this._reAuthenticateModalToggle = false; if (this.showReAuthenticateModal) { this._reauthSave = true; this._reauthSaveOptions = options; From 33c2926e3917e63006d4834f0e4e20662ee44a25 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Mon, 3 Oct 2022 23:46:07 +0800 Subject: [PATCH 08/15] set forceTransition back to false --- ghost/admin/app/services/session.js | 1 + 1 file changed, 1 insertion(+) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index d869fa1730a..340833dd0e1 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -95,6 +95,7 @@ export default class SessionService extends ESASessionService { // retry previous transition if there is active session if (this.forceTransition) { + this.forceTransition = false; transition.retry(); return; } From 4cfbfeee2ffbba471a315ce406492b6c9e8ab3f3 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Tue, 4 Oct 2022 00:13:36 +0800 Subject: [PATCH 09/15] add comments --- ghost/admin/app/services/session.js | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index 340833dd0e1..3c8384b48bd 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -78,14 +78,19 @@ export default class SessionService extends ESASessionService { async requireAuthentication(transition, route) { if (!this.isAuthenticated) { - // try to re-setup session if user data is still available + /** + * Always try to re-setup session if user data is still available + * although the session is invalid and retry the original transition. + * If success, it will retry the original transition. + * If failed, it will be handled by the redirect to sign in. + */ await this.reSetupSession(transition); } super.requireAuthentication(transition, route); } - // TODO: feels a bit hacky to re-setup session if user data is available + // TODO: feels a bit hacky, maybe got a better way to handle this async reSetupSession(transition) { if (this.user) { await this.setup(); From 757a32d1efd48abad3771338d909dee681b16238 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Tue, 4 Oct 2022 00:54:11 +0800 Subject: [PATCH 10/15] change authentication-test.js to call /users/me instead since it is loaded on every new visit --- ghost/admin/tests/acceptance/authentication-test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ghost/admin/tests/acceptance/authentication-test.js b/ghost/admin/tests/acceptance/authentication-test.js index cde32146fe6..918f0a9ba02 100644 --- a/ghost/admin/tests/acceptance/authentication-test.js +++ b/ghost/admin/tests/acceptance/authentication-test.js @@ -50,7 +50,7 @@ describe('Acceptance: Authentication', function () { it('invalidates session on 401 API response', async function () { // return a 401 when attempting to retrieve users - this.server.get('/users/', () => new Response(401, {}, { + this.server.get('/users/me', () => new Response(401, {}, { errors: [ {message: 'Access denied.', type: 'UnauthorizedError'} ] @@ -71,7 +71,7 @@ describe('Acceptance: Authentication', function () { it('invalidates session on 403 API response', async function () { // return a 401 when attempting to retrieve users - this.server.get('/users/', () => new Response(403, {}, { + this.server.get('/users/me', () => new Response(403, {}, { errors: [ {message: 'Authorization failed', type: 'NoPermissionError'} ] From 465237418e80c0ae60b10661dc6920ced8e80555 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Tue, 4 Oct 2022 00:55:00 +0800 Subject: [PATCH 11/15] refactor to fix uncaught TypeError when running lexical-test.js --- ghost/admin/app/routes/authenticated.js | 22 +++++++++++++++++++++- ghost/admin/app/services/session.js | 24 ++---------------------- 2 files changed, 23 insertions(+), 23 deletions(-) diff --git a/ghost/admin/app/routes/authenticated.js b/ghost/admin/app/routes/authenticated.js index c073cb85785..d912a9193cf 100644 --- a/ghost/admin/app/routes/authenticated.js +++ b/ghost/admin/app/routes/authenticated.js @@ -3,8 +3,28 @@ import {inject as service} from '@ember/service'; export default class AuthenticatedRoute extends Route { @service session; + @service notifications; - beforeModel(transition) { + async beforeModel(transition) { + await this.checkForActiveSession(); this.session.requireAuthentication(transition, 'signin'); } + + /** + * Always try to re-setup session & retry the original transition + * if user data is still available in session store although the + * session unauthenticated. + * + * If success, it will retry the original transition. + * If failed, it will be handled by the redirect to sign in. + */ + async checkForActiveSession() { + if (!this.session.isAuthenticated) { + if (this.session.user) { + await this.session.setup(); + this.session.forceTransition = true; + this.notifications.clearAll(); + } + } + } } diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index 3c8384b48bd..ebcef0573ce 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -77,33 +77,13 @@ export default class SessionService extends ESASessionService { } async requireAuthentication(transition, route) { - if (!this.isAuthenticated) { - /** - * Always try to re-setup session if user data is still available - * although the session is invalid and retry the original transition. - * If success, it will retry the original transition. - * If failed, it will be handled by the redirect to sign in. - */ - await this.reSetupSession(transition); - } - - super.requireAuthentication(transition, route); - } - - // TODO: feels a bit hacky, maybe got a better way to handle this - async reSetupSession(transition) { - if (this.user) { - await this.setup(); - this.forceTransition = true; - this.notifications.clearAll(); - } - - // retry previous transition if there is active session if (this.forceTransition) { this.forceTransition = false; transition.retry(); return; } + + super.requireAuthentication(transition, route); } handleInvalidation() { From 66d922c7a309b222455c4122a31bb6cd3e29f97c Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Fri, 7 Oct 2022 07:05:17 +0800 Subject: [PATCH 12/15] revert back to original authenticated.js move checking logic to session.js --- ghost/admin/app/routes/authenticated.js | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/ghost/admin/app/routes/authenticated.js b/ghost/admin/app/routes/authenticated.js index d912a9193cf..7688de57df6 100644 --- a/ghost/admin/app/routes/authenticated.js +++ b/ghost/admin/app/routes/authenticated.js @@ -3,28 +3,8 @@ import {inject as service} from '@ember/service'; export default class AuthenticatedRoute extends Route { @service session; - @service notifications; async beforeModel(transition) { - await this.checkForActiveSession(); this.session.requireAuthentication(transition, 'signin'); } - - /** - * Always try to re-setup session & retry the original transition - * if user data is still available in session store although the - * session unauthenticated. - * - * If success, it will retry the original transition. - * If failed, it will be handled by the redirect to sign in. - */ - async checkForActiveSession() { - if (!this.session.isAuthenticated) { - if (this.session.user) { - await this.session.setup(); - this.session.forceTransition = true; - this.notifications.clearAll(); - } - } - } } From f2fb80c67ea43ae316edfdcdcafbe2b4090355a9 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Fri, 7 Oct 2022 07:11:34 +0800 Subject: [PATCH 13/15] move checking and session setup to session.js --- ghost/admin/app/services/session.js | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index ebcef0573ce..a5cf6f46e3b 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -76,11 +76,26 @@ export default class SessionService extends ESASessionService { }); } + /** + * Always try to re-setup session & retry the original transition + * if user data is still available in session store although the + * ember-session is unauthenticated. + * + * If success, it will retry the original transition. + * If failed, it will be handled by the redirect to sign in. + */ async requireAuthentication(transition, route) { - if (this.forceTransition) { - this.forceTransition = false; - transition.retry(); - return; + // Only when ember session invalidated + if (!this.isAuthenticated) { + transition.abort(); + + if (this.user) { + await this.setup(); + this.forceTransition = true; + this.notifications.clearAll(); + + transition.retry(); + } } super.requireAuthentication(transition, route); From 7205ab557e44f53364bcbf9bb0b37c1cac52f0e0 Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Fri, 7 Oct 2022 10:04:44 +0800 Subject: [PATCH 14/15] remove unrelevent lines --- ghost/admin/app/services/session.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index a5cf6f46e3b..b6316b59ae4 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -91,9 +91,7 @@ export default class SessionService extends ESASessionService { if (this.user) { await this.setup(); - this.forceTransition = true; this.notifications.clearAll(); - transition.retry(); } } From 049950c737e1612f24bfa170ad6f16bbcba855ad Mon Sep 17 00:00:00 2001 From: Hakim Razalan Date: Fri, 7 Oct 2022 10:30:04 +0800 Subject: [PATCH 15/15] remove unused property --- ghost/admin/app/services/session.js | 1 - 1 file changed, 1 deletion(-) diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index b6316b59ae4..4be3019b212 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -22,7 +22,6 @@ export default class SessionService extends ESASessionService { @tracked user = null; skipAuthSuccessHandler = false; - forceTransition = false; async populateUser(options = {}) { if (this.user) {