From c646e78fff85893af9fb5cb49797d579f8377c5c Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Thu, 8 Jul 2021 14:37:31 +0100 Subject: [PATCH] Made `session.user` a synchronous property rather than a promise no issue Having `session.user` return a promise made dealing with it in components difficult because you always had to remember it returned a promise rather than a model and had to handle the async behaviour. It also meant that you couldn't use any current user properties directly inside getters which made refactors to Glimmer/Octane idioms harder to reason about. `session.user` was a cached computed property so it really made no sense for it to be a promise - it was loaded on first access and then always returned instantly but with a fulfilled promise rather than the underlying model. Refactoring to a synchronous property that is loaded as part of the authentication flows (we load the current user to check that we're logged in - we may as well make use of that!) means one less thing to be aware of/remember and provides a nicer migration process to Glimmer components. As part of the refactor, the auth flows and pre-load of required data across other services was also simplified to make it easier to find and follow. - refactored app setup and `session.user` - added `session.populateUser()` that fetches a user model from the current user endpoint and sets it on `session.user` - removed knowledge of app setup from the `cookie` authenticator and moved it into = `session.postAuthPreparation()`, this means we have the same post-authentication setup no matter which authenticator is used so we have more consistent behaviour in tests which don't use the `cookie` authenticator - switched `session` service to native class syntax to get the expected `super()` behaviour - updated `handleAuthentication()` so it populate's `session.user` and performs post-auth setup before transitioning (handles sign-in after app load) - updated `application` route to remove duplicated knowledge of app preload behaviour that now lives in `session.postAuthPreparation()` (handles already-authed app load) - removed out-of-date attempt at pre-loading data from setup controller as that's now handled automatically via `session.handleAuthentication` - updated app code to not treat `session.user` as a promise - predominant usage was router `beforeModel` hooks that transitioned users without valid permissions, this sets us up for an easier removal of the `current-user-settings` mixin in the future --- ghost/admin/app/authenticators/cookie.js | 22 +--- .../components/modal-post-preview/email.hbs | 2 +- .../components/modal-post-preview/email.js | 8 +- ghost/admin/app/controllers/application.js | 6 +- ghost/admin/app/controllers/settings/labs.js | 2 +- ghost/admin/app/controllers/setup/two.js | 16 +-- ghost/admin/app/controllers/signup.js | 2 +- .../admin/app/mixins/current-user-settings.js | 24 ++-- ghost/admin/app/routes/application.js | 74 ++++------- ghost/admin/app/routes/dashboard.js | 8 +- ghost/admin/app/routes/editor/edit.js | 19 ++- ghost/admin/app/routes/editor/new.js | 4 +- ghost/admin/app/routes/integration.js | 5 +- ghost/admin/app/routes/integrations.js | 5 +- ghost/admin/app/routes/integrations/amp.js | 8 +- .../app/routes/integrations/firstpromoter.js | 8 +- ghost/admin/app/routes/integrations/slack.js | 8 +- .../admin/app/routes/integrations/unsplash.js | 8 +- ghost/admin/app/routes/integrations/zapier.js | 7 +- ghost/admin/app/routes/launch.js | 8 +- ghost/admin/app/routes/member.js | 8 +- ghost/admin/app/routes/members.js | 8 +- ghost/admin/app/routes/posts.js | 75 ++++++----- ghost/admin/app/routes/pro.js | 10 +- ghost/admin/app/routes/settings.js | 7 +- .../app/routes/settings/code-injection.js | 5 +- ghost/admin/app/routes/settings/general.js | 5 +- ghost/admin/app/routes/settings/labs.js | 5 +- .../app/routes/settings/members-email.js | 31 +++-- ghost/admin/app/routes/settings/navigation.js | 3 +- ghost/admin/app/routes/settings/product.js | 8 +- ghost/admin/app/routes/settings/theme.js | 3 +- ghost/admin/app/routes/staff/user.js | 32 ++--- ghost/admin/app/routes/tag.js | 3 +- ghost/admin/app/routes/tags.js | 3 +- ghost/admin/app/services/custom-views.js | 4 +- ghost/admin/app/services/feature.js | 8 +- .../admin/app/services/members-count-cache.js | 2 +- ghost/admin/app/services/navigation.js | 9 +- ghost/admin/app/services/session.js | 116 +++++++++++------- ghost/admin/app/session-stores/application.js | 5 +- .../integration/services/feature-test.js | 60 +++++++-- .../tests/unit/authenticators/cookie-test.js | 9 -- 43 files changed, 311 insertions(+), 352 deletions(-) diff --git a/ghost/admin/app/authenticators/cookie.js b/ghost/admin/app/authenticators/cookie.js index dfc709a4e9..32b9370809 100644 --- a/ghost/admin/app/authenticators/cookie.js +++ b/ghost/admin/app/authenticators/cookie.js @@ -5,11 +5,7 @@ import {inject as service} from '@ember/service'; export default Authenticator.extend({ ajax: service(), - config: service(), - feature: service(), ghostPaths: service(), - settings: service(), - whatsNew: service(), sessionEndpoint: computed('ghostPaths.apiRoot', function () { return `${this.ghostPaths.apiRoot}/session`; @@ -28,23 +24,7 @@ export default Authenticator.extend({ dataType: 'text' }; - return this.ajax.post(this.sessionEndpoint, options).then((authResult) => { - // TODO: remove duplication with application.afterModel - let preloadPromises = [ - this.config.fetchAuthenticated(), - this.feature.fetch(), - this.settings.fetch() - ]; - - // kick off background update of "whats new" - // - we don't want to block the router for this - // - we need the user details to know what the user has seen - this.whatsNew.fetchLatest.perform(); - - return RSVP.all(preloadPromises).then(() => { - return authResult; - }); - }); + return this.ajax.post(this.sessionEndpoint, options); }, invalidate() { diff --git a/ghost/admin/app/components/modal-post-preview/email.hbs b/ghost/admin/app/components/modal-post-preview/email.hbs index d54e2c4304..77969c8307 100644 --- a/ghost/admin/app/components/modal-post-preview/email.hbs +++ b/ghost/admin/app/components/modal-post-preview/email.hbs @@ -12,7 +12,7 @@
Send a test newsletter -
+
{ // Reload currentUser and set session - this.set('session.user', store.findRecord('user', currentUserId)); + this.session.populateUser({id: currentUserId}); // TODO: keep as notification, add link to view content notifications.showNotification('Import successful', {key: 'import.upload.success'}); diff --git a/ghost/admin/app/controllers/setup/two.js b/ghost/admin/app/controllers/setup/two.js index d54e8d2717..c400a7dabb 100644 --- a/ghost/admin/app/controllers/setup/two.js +++ b/ghost/admin/app/controllers/setup/two.js @@ -1,6 +1,5 @@ /* eslint-disable camelcase, ghost/ember/alias-model-in-controller */ import Controller, {inject as controller} from '@ember/controller'; -import RSVP from 'rsvp'; import ValidationEngine from 'ghost-admin/mixins/validation-engine'; import {get} from '@ember/object'; import {isInvalidError} from 'ember-ajax/errors'; @@ -15,7 +14,6 @@ export default Controller.extend(ValidationEngine, { ghostPaths: service(), notifications: service(), session: service(), - settings: service(), // ValidationEngine settings validationType: 'setup', @@ -51,7 +49,7 @@ export default Controller.extend(ValidationEngine, { authenticate: task(function* (authStrategy, authentication) { // we don't want to redirect after sign-in during setup - this.set('session.skipAuthSuccessHandler', true); + this.session.skipAuthSuccessHandler = true; try { let authResult = yield this.session @@ -141,15 +139,13 @@ export default Controller.extend(ValidationEngine, { } // Don't call the success handler, otherwise we will be redirected to admin - this.set('session.skipAuthSuccessHandler', true); + this.session.skipAuthSuccessHandler = true; return this.session.authenticate('authenticator:cookie', data.email, data.password).then(() => { this.set('blogCreated', true); return this._afterAuthentication(result); }).catch((error) => { this._handleAuthenticationError(error); - }).finally(() => { - this.set('session.skipAuthSuccessHandler', undefined); }); }).catch((error) => { this._handleSaveError(error); @@ -179,20 +175,14 @@ export default Controller.extend(ValidationEngine, { }, _afterAuthentication(result) { - // fetch settings and private config for synchronous access before transitioning - let fetchSettingsAndConfig = RSVP.all([ - this.settings.fetch() - ]); - if (this.profileImage) { return this._sendImage(result.users[0]) - .then(() => (fetchSettingsAndConfig)) .then(() => (this.transitionToRoute('setup.three'))) .catch((resp) => { this.notifications.showAPIError(resp, {key: 'setup.blog-details'}); }); } else { - return fetchSettingsAndConfig.then(() => this.transitionToRoute('setup.three')); + return this.transitionToRoute('setup.three'); } } }); diff --git a/ghost/admin/app/controllers/signup.js b/ghost/admin/app/controllers/signup.js index 4c0752ef9e..5378bb6917 100644 --- a/ghost/admin/app/controllers/signup.js +++ b/ghost/admin/app/controllers/signup.js @@ -105,7 +105,7 @@ export default Controller.extend({ formData.append('file', imageFile, imageFile.name); formData.append('purpose', 'profile_image'); - let user = yield this.get('session.user'); + let user = this.session.user; let response = yield this.ajax.post(uploadUrl, { data: formData, processData: false, diff --git a/ghost/admin/app/mixins/current-user-settings.js b/ghost/admin/app/mixins/current-user-settings.js index 3d2a9ed371..e5aa5576dd 100644 --- a/ghost/admin/app/mixins/current-user-settings.js +++ b/ghost/admin/app/mixins/current-user-settings.js @@ -1,23 +1,15 @@ import Mixin from '@ember/object/mixin'; export default Mixin.create({ - transitionAuthor() { - return (user) => { - if (user.get('isAuthorOrContributor')) { - return this.transitionTo('staff.user', user); - } - - return user; - }; + transitionAuthor(user) { + if (user.isAuthorOrContributor) { + return this.transitionTo('staff.user', user); + } }, - transitionEditor() { - return (user) => { - if (user.get('isEditor')) { - return this.transitionTo('staff'); - } - - return user; - }; + transitionEditor(user) { + if (user.isEditor) { + return this.transitionTo('staff'); + } } }); diff --git a/ghost/admin/app/routes/application.js b/ghost/admin/app/routes/application.js index 2e724b8909..94981c1fef 100644 --- a/ghost/admin/app/routes/application.js +++ b/ghost/admin/app/routes/application.js @@ -1,11 +1,9 @@ import AuthConfiguration from 'ember-simple-auth/configuration'; -import RSVP from 'rsvp'; import Route from '@ember/routing/route'; import ShortcutsRoute from 'ghost-admin/mixins/shortcuts-route'; import ctrlOrCmd from 'ghost-admin/utils/ctrl-or-cmd'; import windowProxy from 'ghost-admin/utils/window-proxy'; import {InitSentryForEmber} from '@sentry/ember'; -import {configureScope} from '@sentry/browser'; import { isAjaxError, isNotFoundError, @@ -51,57 +49,14 @@ export default Route.extend(ShortcutsRoute, { }, beforeModel() { - return this.config.fetchUnauthenticated() - .then(() => { - // init Sentry here rather than app.js so that we can use API-supplied - // sentry_dsn and sentry_env rather than building it into release assets - if (this.config.get('sentry_dsn')) { - InitSentryForEmber({ - dsn: this.config.get('sentry_dsn'), - environment: this.config.get('sentry_env'), - release: `ghost@${this.config.get('version')}` - }); - } - }); + return this.prepareApp(); }, - afterModel(model, transition) { + async afterModel(model, transition) { this._super(...arguments); if (this.get('session.isAuthenticated')) { this.session.appLoadTransition = transition; - this.session.loadServerNotifications(); - - // return the feature/settings load promises so that we block until - // they are loaded to enable synchronous access everywhere - return RSVP.all([ - this.config.fetchAuthenticated(), - this.feature.fetch(), - this.settings.fetch() - ]).then((results) => { - this._appLoaded = true; - - // update Sentry with the full Ghost version which we only get after authentication - if (this.config.get('sentry_dsn')) { - configureScope((scope) => { - scope.addEventProcessor((event) => { - return new Promise((resolve) => { - resolve({ - ...event, - release: `ghost@${this.config.get('version')}` - }); - }); - }); - }); - } - - // kick off background update of "whats new" - // - we don't want to block the router for this - // - we need the user details to know what the user has seen - this.whatsNew.fetchLatest.perform(); - - return results; - }); } this._appLoaded = true; @@ -186,5 +141,30 @@ export default Route.extend(ShortcutsRoute, { // fallback to 500 error page return true; } + }, + + async prepareApp() { + await this.config.fetchUnauthenticated(); + + // init Sentry here rather than app.js so that we can use API-supplied + // sentry_dsn and sentry_env rather than building it into release assets + if (this.config.get('sentry_dsn')) { + InitSentryForEmber({ + dsn: this.config.get('sentry_dsn'), + environment: this.config.get('sentry_env'), + release: `ghost@${this.config.get('version')}` + }); + } + + if (this.session.isAuthenticated) { + try { + await this.session.populateUser(); + } catch (e) { + await this.session.invalidate(); + } + + await this.session.postAuthPreparation(); + } } + }); diff --git a/ghost/admin/app/routes/dashboard.js b/ghost/admin/app/routes/dashboard.js index 3b8324fd25..929e2d0233 100644 --- a/ghost/admin/app/routes/dashboard.js +++ b/ghost/admin/app/routes/dashboard.js @@ -3,11 +3,9 @@ import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; export default class DashboardRoute extends AuthenticatedRoute { beforeModel() { super.beforeModel(...arguments); - return this.session.user.then((user) => { - if (!user.isOwnerOrAdmin) { - return this.transitionTo('site'); - } - }); + if (!this.session.user.isOwnerOrAdmin) { + return this.transitionTo('site'); + } } buildRouteInfoMetadata() { diff --git a/ghost/admin/app/routes/editor/edit.js b/ghost/admin/app/routes/editor/edit.js index 34985de5a9..eaee895bb1 100644 --- a/ghost/admin/app/routes/editor/edit.js +++ b/ghost/admin/app/routes/editor/edit.js @@ -37,18 +37,17 @@ export default AuthenticatedRoute.extend({ afterModel(post) { this._super(...arguments); - return this.get('session.user').then((user) => { - let returnRoute = pluralize(post.constructor.modelName); + const user = this.session.user; + const returnRoute = pluralize(post.constructor.modelName); - if (user.get('isAuthorOrContributor') && !post.isAuthoredByUser(user)) { - return this.replaceWith(returnRoute); - } + if (user.isAuthorOrContributor && !post.isAuthoredByUser(user)) { + return this.replaceWith(returnRoute); + } - // If the post is not a draft and user is contributor, redirect to index - if (user.get('isContributor') && !post.get('isDraft')) { - return this.replaceWith(returnRoute); - } - }); + // If the post is not a draft and user is contributor, redirect to index + if (user.isContributor && !post.isDraft) { + return this.replaceWith(returnRoute); + } }, serialize(model) { diff --git a/ghost/admin/app/routes/editor/new.js b/ghost/admin/app/routes/editor/new.js index c29587dae3..8af68680d8 100644 --- a/ghost/admin/app/routes/editor/new.js +++ b/ghost/admin/app/routes/editor/new.js @@ -9,9 +9,7 @@ export default AuthenticatedRoute.extend({ return this.replaceWith('error404', {path, status: 404}); } - return this.get('session.user').then(user => ( - this.store.createRecord(modelName, {authors: [user]}) - )); + this.store.createRecord(modelName, {authors: [this.session.user]}); }, // there's no specific controller for this route, instead all editor diff --git a/ghost/admin/app/routes/integration.js b/ghost/admin/app/routes/integration.js index 2475c45aa7..3c536c9da0 100644 --- a/ghost/admin/app/routes/integration.js +++ b/ghost/admin/app/routes/integration.js @@ -18,9 +18,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, model(params, transition) { diff --git a/ghost/admin/app/routes/integrations.js b/ghost/admin/app/routes/integrations.js index eee532aa2b..9c766e983f 100644 --- a/ghost/admin/app/routes/integrations.js +++ b/ghost/admin/app/routes/integrations.js @@ -7,9 +7,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, setupController(controller) { diff --git a/ghost/admin/app/routes/integrations/amp.js b/ghost/admin/app/routes/integrations/amp.js index 2bf0290ebd..ed7c9e246f 100644 --- a/ghost/admin/app/routes/integrations/amp.js +++ b/ghost/admin/app/routes/integrations/amp.js @@ -7,10 +7,10 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()) - .then(this.settings.reload()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); + + return this.settings.reload(); }, actions: { diff --git a/ghost/admin/app/routes/integrations/firstpromoter.js b/ghost/admin/app/routes/integrations/firstpromoter.js index 675b174c39..492b6bb01d 100644 --- a/ghost/admin/app/routes/integrations/firstpromoter.js +++ b/ghost/admin/app/routes/integrations/firstpromoter.js @@ -7,10 +7,10 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()) - .then(this.settings.reload()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); + + return this.settings.reload(); }, actions: { diff --git a/ghost/admin/app/routes/integrations/slack.js b/ghost/admin/app/routes/integrations/slack.js index d156067cc8..3c1ed138b0 100644 --- a/ghost/admin/app/routes/integrations/slack.js +++ b/ghost/admin/app/routes/integrations/slack.js @@ -7,10 +7,10 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()) - .then(this.settings.reload()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); + + return this.settings.reload(); }, actions: { diff --git a/ghost/admin/app/routes/integrations/unsplash.js b/ghost/admin/app/routes/integrations/unsplash.js index f1aac1d4ae..9581c28e8c 100644 --- a/ghost/admin/app/routes/integrations/unsplash.js +++ b/ghost/admin/app/routes/integrations/unsplash.js @@ -7,10 +7,10 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()) - .then(this.settings.reload()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); + + return this.settings.reload(); }, actions: { diff --git a/ghost/admin/app/routes/integrations/zapier.js b/ghost/admin/app/routes/integrations/zapier.js index b01aafb935..fa64d009c9 100644 --- a/ghost/admin/app/routes/integrations/zapier.js +++ b/ghost/admin/app/routes/integrations/zapier.js @@ -24,10 +24,9 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionDisabled()) - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionDisabled(); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, model(params, transition) { diff --git a/ghost/admin/app/routes/launch.js b/ghost/admin/app/routes/launch.js index d2802cc24b..5fcde50601 100644 --- a/ghost/admin/app/routes/launch.js +++ b/ghost/admin/app/routes/launch.js @@ -6,10 +6,8 @@ export default class LaunchRoute extends AuthenticatedRoute { beforeModel() { super.beforeModel(...arguments); - return this.session.user.then((user) => { - if (!user.isOwner) { - return this.transitionTo('home'); - } - }); + if (!this.session.user.isOwner) { + return this.transitionTo('home'); + } } } diff --git a/ghost/admin/app/routes/member.js b/ghost/admin/app/routes/member.js index f6e4492c72..25b07141f5 100644 --- a/ghost/admin/app/routes/member.js +++ b/ghost/admin/app/routes/member.js @@ -18,11 +18,9 @@ export default class MembersRoute extends AuthenticatedRoute { beforeModel() { super.beforeModel(...arguments); - return this.session.user.then((user) => { - if (!user.isOwnerOrAdmin) { - return this.transitionTo('home'); - } - }); + if (!this.session.user.isOwnerOrAdmin) { + return this.transitionTo('home'); + } } model(params) { diff --git a/ghost/admin/app/routes/members.js b/ghost/admin/app/routes/members.js index e7bd35e095..1bd22086f1 100644 --- a/ghost/admin/app/routes/members.js +++ b/ghost/admin/app/routes/members.js @@ -16,11 +16,9 @@ export default class MembersRoute extends AuthenticatedRoute { // - logged in user isn't owner/admin beforeModel() { super.beforeModel(...arguments); - return this.session.user.then((user) => { - if (!user.isOwnerOrAdmin) { - return this.transitionTo('home'); - } - }); + if (!this.session.user.isOwnerOrAdmin) { + return this.transitionTo('home'); + } } model(params) { diff --git a/ghost/admin/app/routes/posts.js b/ghost/admin/app/routes/posts.js index 40a439a546..4512fbd8cc 100644 --- a/ghost/admin/app/routes/posts.js +++ b/ghost/admin/app/routes/posts.js @@ -38,45 +38,44 @@ export default AuthenticatedRoute.extend({ }, model(params) { - return this.session.user.then((user) => { - let queryParams = {}; - let filterParams = {tag: params.tag, visibility: params.visibility}; - let paginationParams = { - perPageParam: 'limit', - totalPagesParam: 'meta.pagination.pages' - }; + const user = this.session.user; + let queryParams = {}; + let filterParams = {tag: params.tag, visibility: params.visibility}; + let paginationParams = { + perPageParam: 'limit', + totalPagesParam: 'meta.pagination.pages' + }; - assign(filterParams, this._getTypeFilters(params.type)); + assign(filterParams, this._getTypeFilters(params.type)); - if (params.type === 'featured') { - filterParams.featured = true; - } + if (params.type === 'featured') { + filterParams.featured = true; + } - if (user.isAuthor) { - // authors can only view their own posts - filterParams.authors = user.slug; - } else if (user.isContributor) { - // Contributors can only view their own draft posts - filterParams.authors = user.slug; - filterParams.status = 'draft'; - } else if (params.author) { - filterParams.authors = params.author; - } + if (user.isAuthor) { + // authors can only view their own posts + filterParams.authors = user.slug; + } else if (user.isContributor) { + // Contributors can only view their own draft posts + filterParams.authors = user.slug; + filterParams.status = 'draft'; + } else if (params.author) { + filterParams.authors = params.author; + } - let filter = this._filterString(filterParams); - if (!isBlank(filter)) { - queryParams.filter = filter; - } + let filter = this._filterString(filterParams); + if (!isBlank(filter)) { + queryParams.filter = filter; + } - if (!isBlank(params.order)) { - queryParams.order = params.order; - } + if (!isBlank(params.order)) { + queryParams.order = params.order; + } - let perPage = this.perPage; - let paginationSettings = assign({perPage, startingPage: 1}, paginationParams, queryParams); + let perPage = this.perPage; + let paginationSettings = assign({perPage, startingPage: 1}, paginationParams, queryParams); - return this.infinity.model(this.modelName, paginationSettings); - }); + return this.infinity.model(this.modelName, paginationSettings); }, // trigger a background load of all tags, authors, and snipps for use in filter dropdowns and card menu @@ -89,13 +88,11 @@ export default AuthenticatedRoute.extend({ }); } - this.session.user.then((user) => { - if (!user.isAuthorOrContributor && !controller._hasLoadedAuthors) { - this.store.query('user', {limit: 'all'}).then(() => { - controller._hasLoadedAuthors = true; - }); - } - }); + if (!this.session.user.isAuthorOrContributor && !controller._hasLoadedAuthors) { + this.store.query('user', {limit: 'all'}).then(() => { + controller._hasLoadedAuthors = true; + }); + } if (!controller._hasLoadedSnippets) { this.store.query('snippet', {limit: 'all'}).then(() => { diff --git a/ghost/admin/app/routes/pro.js b/ghost/admin/app/routes/pro.js index 27b59096ef..c35c6d459e 100644 --- a/ghost/admin/app/routes/pro.js +++ b/ghost/admin/app/routes/pro.js @@ -13,13 +13,11 @@ export default AuthenticatedRoute.extend({ beforeModel(transition) { this._super(...arguments); - return this.session.user.then((user) => { - if (!user.isOwner) { - return this.transitionTo('home'); - } + if (!this.session.user.isOwner) { + return this.transitionTo('home'); + } - this.billing.set('previousTransition', transition); - }); + this.billing.set('previousTransition', transition); }, model(params) { diff --git a/ghost/admin/app/routes/settings.js b/ghost/admin/app/routes/settings.js index b1f5ff4680..8992130de5 100644 --- a/ghost/admin/app/routes/settings.js +++ b/ghost/admin/app/routes/settings.js @@ -4,8 +4,7 @@ import CurrentUserSettings from 'ghost-admin/mixins/current-user-settings'; export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); } -}); \ No newline at end of file +}); diff --git a/ghost/admin/app/routes/settings/code-injection.js b/ghost/admin/app/routes/settings/code-injection.js index 5b16ec547e..281628b922 100644 --- a/ghost/admin/app/routes/settings/code-injection.js +++ b/ghost/admin/app/routes/settings/code-injection.js @@ -7,9 +7,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, model() { diff --git a/ghost/admin/app/routes/settings/general.js b/ghost/admin/app/routes/settings/general.js index d7c295ad8b..8ae625e6f1 100644 --- a/ghost/admin/app/routes/settings/general.js +++ b/ghost/admin/app/routes/settings/general.js @@ -9,9 +9,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, model() { diff --git a/ghost/admin/app/routes/settings/labs.js b/ghost/admin/app/routes/settings/labs.js index 84beb8f7c2..d22970ec86 100644 --- a/ghost/admin/app/routes/settings/labs.js +++ b/ghost/admin/app/routes/settings/labs.js @@ -8,9 +8,8 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()); + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); }, model() { diff --git a/ghost/admin/app/routes/settings/members-email.js b/ghost/admin/app/routes/settings/members-email.js index 74b9e9b307..97144ce783 100644 --- a/ghost/admin/app/routes/settings/members-email.js +++ b/ghost/admin/app/routes/settings/members-email.js @@ -8,22 +8,21 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel(transition) { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()) - .then(this.transitionEditor()) - .then(() => { - if (transition.to.queryParams?.fromAddressUpdate === 'success') { - this.notifications.showAlert( - `Newsletter email address has been updated`.htmlSafe(), - {type: 'success', key: 'members.settings.from-address.updated'} - ); - } else if (transition.to.queryParams?.supportAddressUpdate === 'success') { - this.notifications.showAlert( - `Support email address has been updated`.htmlSafe(), - {type: 'success', key: 'members.settings.support-address.updated'} - ); - } - }); + + this.transitionAuthor(this.session.user); + this.transitionEditor(this.session.user); + + if (transition.to.queryParams?.fromAddressUpdate === 'success') { + this.notifications.showAlert( + `Newsletter email address has been updated`.htmlSafe(), + {type: 'success', key: 'members.settings.from-address.updated'} + ); + } else if (transition.to.queryParams?.supportAddressUpdate === 'success') { + this.notifications.showAlert( + `Support email address has been updated`.htmlSafe(), + {type: 'success', key: 'members.settings.support-address.updated'} + ); + } }, model() { diff --git a/ghost/admin/app/routes/settings/navigation.js b/ghost/admin/app/routes/settings/navigation.js index 36f39982e4..c42271bad2 100644 --- a/ghost/admin/app/routes/settings/navigation.js +++ b/ghost/admin/app/routes/settings/navigation.js @@ -9,8 +9,7 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()); + this.transitionAuthor(this.session.user); }, model() { diff --git a/ghost/admin/app/routes/settings/product.js b/ghost/admin/app/routes/settings/product.js index 360171b346..d6ad517e17 100644 --- a/ghost/admin/app/routes/settings/product.js +++ b/ghost/admin/app/routes/settings/product.js @@ -25,11 +25,9 @@ export default class ProductRoute extends AuthenticatedRoute { beforeModel() { super.beforeModel(...arguments); - return this.session.user.then((user) => { - if (!user.isOwnerOrAdmin) { - return this.transitionTo('home'); - } - }); + if (!this.session.user.isOwnerOrAdmin) { + return this.transitionTo('home'); + } } setupController(controller, product) { diff --git a/ghost/admin/app/routes/settings/theme.js b/ghost/admin/app/routes/settings/theme.js index 4245dc6d7d..1a2572a15d 100644 --- a/ghost/admin/app/routes/settings/theme.js +++ b/ghost/admin/app/routes/settings/theme.js @@ -8,8 +8,7 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()); + this.transitionAuthor(this.session.user); }, model() { diff --git a/ghost/admin/app/routes/staff/user.js b/ghost/admin/app/routes/staff/user.js index 74bb656690..6fae15bb33 100644 --- a/ghost/admin/app/routes/staff/user.js +++ b/ghost/admin/app/routes/staff/user.js @@ -10,24 +10,24 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { afterModel(user) { this._super(...arguments); - return this.get('session.user').then((currentUser) => { - let isOwnProfile = user.get('id') === currentUser.get('id'); - let isAuthorOrContributor = currentUser.get('isAuthorOrContributor'); - let isEditor = currentUser.get('isEditor'); + const currentUser = this.session.user; - if (isAuthorOrContributor && !isOwnProfile) { - this.transitionTo('staff.user', currentUser); - } else if (isEditor && !isOwnProfile && !user.get('isAuthorOrContributor')) { - this.transitionTo('staff'); - } + let isOwnProfile = user.get('id') === currentUser.get('id'); + let isAuthorOrContributor = currentUser.get('isAuthorOrContributor'); + let isEditor = currentUser.get('isEditor'); - if (isOwnProfile) { - this.store.queryRecord('api-key', {id: 'me'}).then((apiKey) => { - this.controller.set('personalToken', apiKey.id + ':' + apiKey.secret); - this.controller.set('personalTokenRegenerated', false); - }); - } - }); + if (isAuthorOrContributor && !isOwnProfile) { + this.transitionTo('staff.user', currentUser); + } else if (isEditor && !isOwnProfile && !user.get('isAuthorOrContributor')) { + this.transitionTo('staff'); + } + + if (isOwnProfile) { + this.store.queryRecord('api-key', {id: 'me'}).then((apiKey) => { + this.controller.set('personalToken', apiKey.id + ':' + apiKey.secret); + this.controller.set('personalTokenRegenerated', false); + }); + } }, serialize(model) { diff --git a/ghost/admin/app/routes/tag.js b/ghost/admin/app/routes/tag.js index aca92a8d11..46f5341457 100644 --- a/ghost/admin/app/routes/tag.js +++ b/ghost/admin/app/routes/tag.js @@ -17,8 +17,7 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()); + this.transitionAuthor(this.session.user); }, model(params) { diff --git a/ghost/admin/app/routes/tags.js b/ghost/admin/app/routes/tags.js index 2e40e5a60c..ba68eede94 100644 --- a/ghost/admin/app/routes/tags.js +++ b/ghost/admin/app/routes/tags.js @@ -23,8 +23,7 @@ export default AuthenticatedRoute.extend(CurrentUserSettings, ShortcutsRoute, { beforeModel() { this._super(...arguments); - return this.get('session.user') - .then(this.transitionAuthor()); + this.transitionAuthor(this.session.user); }, // set model to a live array so all tags are shown and created/deleted tags diff --git a/ghost/admin/app/services/custom-views.js b/ghost/admin/app/services/custom-views.js index bcab6919ad..f76ccff5b7 100644 --- a/ghost/admin/app/services/custom-views.js +++ b/ghost/admin/app/services/custom-views.js @@ -116,13 +116,13 @@ export default class CustomViewsService extends Service { } // eslint-disable-next-line ghost/ember/no-observers - @observes('settings.sharedViews', 'session.isAuthenticated') + @observes('settings.sharedViews', 'session.{isAuthenticated,user}') async updateViewList() { let {settings, session} = this; // avoid fetching user before authenticated otherwise the 403 can fire // during authentication and cause errors during setup/signin - if (!session.isAuthenticated) { + if (!session.isAuthenticated || !session.user) { return; } diff --git a/ghost/admin/app/services/feature.js b/ghost/admin/app/services/feature.js index ac0ec8375e..9adf4ea008 100644 --- a/ghost/admin/app/services/feature.js +++ b/ghost/admin/app/services/feature.js @@ -1,7 +1,6 @@ import $ from 'jquery'; import Ember from 'ember'; import EmberError from '@ember/error'; -import RSVP from 'rsvp'; import Service, {inject as service} from '@ember/service'; import {computed} from '@ember/object'; import {set} from '@ember/object'; @@ -81,11 +80,8 @@ export default Service.extend({ }), fetch() { - return RSVP.hash({ - settings: this.settings.fetch(), - user: this.get('session.user') - }).then(({user}) => { - this.set('_user', user); + return this.settings.fetch().then(() => { + this.set('_user', this.session.user); return this._setAdminTheme().then(() => true); }); }, diff --git a/ghost/admin/app/services/members-count-cache.js b/ghost/admin/app/services/members-count-cache.js index 6b211feb0f..abd0398e4e 100644 --- a/ghost/admin/app/services/members-count-cache.js +++ b/ghost/admin/app/services/members-count-cache.js @@ -25,7 +25,7 @@ export default class MembersCountCacheService extends Service { } async countString(filter = '', {knownCount} = {}) { - const user = await this.session.user; + const user = this.session.user; const basicFilter = filter.replace(/^subscribed:true\+\((.*)\)$/, '$1'); const filterParts = basicFilter.split(','); diff --git a/ghost/admin/app/services/navigation.js b/ghost/admin/app/services/navigation.js index 7478f394dc..66aaeedd1f 100644 --- a/ghost/admin/app/services/navigation.js +++ b/ghost/admin/app/services/navigation.js @@ -23,16 +23,15 @@ export default class NavigationService extends Service { } // eslint-disable-next-line ghost/ember/no-observers - @observes('session.isAuthenticated', 'session.user.accessibility') + @observes('session.{isAuthenticated,user}', 'session.user.accessibility') async updateSettings() { // avoid fetching user before authenticated otherwise the 403 can fire // during authentication and cause errors during setup/signin - if (!this.session.isAuthenticated) { + if (!this.session.isAuthenticated || !this.session.user) { return; } - let user = await this.session.user; - let userSettings = JSON.parse(user.get('accessibility')) || {}; + let userSettings = JSON.parse(this.session.user.accessibility || '{}') || {}; this.settings = userSettings.navigation || Object.assign({}, DEFAULT_SETTINGS); } @@ -51,7 +50,7 @@ export default class NavigationService extends Service { } async _saveNavigationSettings() { - let user = await this.session.user; + let user = this.session.user; let userSettings = JSON.parse(user.get('accessibility')) || {}; userSettings.navigation = this.settings; user.set('accessibility', JSON.stringify(userSettings)); diff --git a/ghost/admin/app/services/session.js b/ghost/admin/app/services/session.js index bd08e42f78..eb9b5506d2 100644 --- a/ghost/admin/app/services/session.js +++ b/ghost/admin/app/services/session.js @@ -1,40 +1,76 @@ -import SessionService from 'ember-simple-auth/services/session'; -import {computed} from '@ember/object'; +import ESASessionService from 'ember-simple-auth/services/session'; +import RSVP from 'rsvp'; +import {configureScope} from '@sentry/browser'; import {getOwner} from '@ember/application'; import {run} from '@ember/runloop'; import {inject as service} from '@ember/service'; +import {tracked} from '@glimmer/tracking'; -export default SessionService.extend({ - dataStore: service('store'), // SessionService.store already exists - notifications: service(), - router: service(), - upgradeStatus: service(), +export default class SessionService extends ESASessionService { + @service config; + @service('store') dataStore; + @service feature; + @service notifications; + @service router; + @service settings; + @service upgradeStatus; + @service whatsNew; - user: computed(function () { - return this.dataStore.queryRecord('user', {id: 'me'}); - }), + @tracked user = null; - authenticate() { - // ensure any cached this.user value is removed and re-fetched - this.notifyPropertyChange('user'); + skipAuthSuccessHandler = false; - return this._super(...arguments); - }, - - handleAuthentication() { - if (this.skipAuthSuccessHandler) { + async populateUser(options = {}) { + if (this.user) { return; } - // standard ESA post-sign-in redirect - this._super('home'); + const id = options.id || 'me'; + const user = await this.dataStore.queryRecord('user', {id}); + this.user = user; + } - // trigger post-sign-in background behaviour - this.user.then(() => { - this.notifications.clearAll(); - this.loadServerNotifications(); - }); - }, + async postAuthPreparation() { + await RSVP.all([ + this.config.fetchAuthenticated(), + this.feature.fetch(), + this.settings.fetch() + ]); + + // update Sentry with the full Ghost version which we only get after authentication + if (this.config.get('sentry_dsn')) { + configureScope((scope) => { + scope.addEventProcessor((event) => { + return new Promise((resolve) => { + resolve({ + ...event, + release: `ghost@${this.config.get('version')}` + }); + }); + }); + }); + } + + this.loadServerNotifications(); + this.whatsNew.fetchLatest.perform(); + } + + async handleAuthentication() { + try { + await this.populateUser(); + } catch (err) { + await this.invalidate(); + } + + await this.postAuthPreparation(); + + if (this.skipAuthSuccessHandler) { + this.skipAuthSuccessHandler = false; + return; + } + + super.handleAuthentication('home'); + } handleInvalidation() { let transition = this.appLoadTransition; @@ -44,28 +80,26 @@ export default SessionService.extend({ } else { run.scheduleOnce('routerTransitions', this, 'triggerAuthorizationFailed'); } - }, + } // TODO: this feels hacky, find a better way than using .send triggerAuthorizationFailed() { getOwner(this).lookup(`route:${this.router.currentRouteName}`).send('authorizationFailed'); - }, + } loadServerNotifications() { if (this.isAuthenticated) { - this.user.then((user) => { - if (!user.isAuthorOrContributor) { - this.dataStore.findAll('notification', {reload: true}).then((serverNotifications) => { - serverNotifications.forEach((notification) => { - if (notification.top || notification.custom) { - this.notifications.handleNotification(notification); - } else { - this.upgradeStatus.handleUpgradeNotification(notification); - } - }); + if (!this.user.isAuthorOrContributor) { + this.dataStore.findAll('notification', {reload: true}).then((serverNotifications) => { + serverNotifications.forEach((notification) => { + if (notification.top || notification.custom) { + this.notifications.handleNotification(notification); + } else { + this.upgradeStatus.handleUpgradeNotification(notification); + } }); - } - }); + }); + } } } -}); +} diff --git a/ghost/admin/app/session-stores/application.js b/ghost/admin/app/session-stores/application.js index cbad2ed3e5..b059fe76c3 100644 --- a/ghost/admin/app/session-stores/application.js +++ b/ghost/admin/app/session-stores/application.js @@ -12,16 +12,13 @@ export default EphemeralStore.extend({ // session cookie or not so we can use that as an indication of the session // being authenticated restore() { - return this.session.user.then(() => { + return this.session.populateUser().then(() => { // provide the necessary data for internal-session to mark the // session as authenticated let data = {authenticated: {authenticator: 'authenticator:cookie'}}; this.persist(data); return data; }).catch(() => { - // ensure the session.user doesn't return the same rejected promise - // after a succussful login - this.session.notifyPropertyChange('user'); return RSVP.reject(); }); } diff --git a/ghost/admin/tests/integration/services/feature-test.js b/ghost/admin/tests/integration/services/feature-test.js index ece333534b..1a9e007abb 100644 --- a/ghost/admin/tests/integration/services/feature-test.js +++ b/ghost/admin/tests/integration/services/feature-test.js @@ -84,12 +84,15 @@ describe('Integration: Service: feature', function () { server.shutdown(); }); - it('loads labs and user settings correctly', function () { + it('loads labs and user settings correctly', async function () { stubSettings(server, {testFlag: true}); stubUser(server, {testUserFlag: true}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); return service.fetch().then(() => { @@ -98,12 +101,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns false for set flag with config false and labs false', function () { + it('returns false for set flag with config false and labs false', async function () { stubSettings(server, {testFlag: false}); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', false); @@ -113,12 +119,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns true for set flag with config true and labs false', function () { + it('returns true for set flag with config true and labs false', async function () { stubSettings(server, {testFlag: false}); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', true); @@ -128,12 +137,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns true for set flag with config false and labs true', function () { + it('returns true for set flag with config false and labs true', async function () { stubSettings(server, {testFlag: true}); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', false); @@ -143,12 +155,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns true for set flag with config true and labs true', function () { + it('returns true for set flag with config true and labs true', async function () { stubSettings(server, {testFlag: true}); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', true); @@ -158,12 +173,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns false for set flag with accessibility false', function () { + it('returns false for set flag with accessibility false', async function () { stubSettings(server, {}); stubUser(server, {testUserFlag: false}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); return service.fetch().then(() => { @@ -172,12 +190,15 @@ describe('Integration: Service: feature', function () { }); }); - it('returns true for set flag with accessibility true', function () { + it('returns true for set flag with accessibility true', async function () { stubSettings(server, {}); stubUser(server, {testUserFlag: true}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); return service.fetch().then(() => { @@ -186,12 +207,15 @@ describe('Integration: Service: feature', function () { }); }); - it('saves labs setting correctly', function () { + it('saves labs setting correctly', async function () { stubSettings(server, {testFlag: false}); stubUser(server, {testUserFlag: false}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', false); @@ -209,12 +233,15 @@ describe('Integration: Service: feature', function () { }); }); - it('saves accessibility setting correctly', function () { + it('saves accessibility setting correctly', async function () { stubSettings(server, {}); stubUser(server, {testUserFlag: false}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); return service.fetch().then(() => { @@ -231,12 +258,15 @@ describe('Integration: Service: feature', function () { }); }); - it('notifies for server errors on labs save', function () { + it('notifies for server errors on labs save', async function () { stubSettings(server, {testFlag: false}, false); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', false); @@ -263,12 +293,15 @@ describe('Integration: Service: feature', function () { }); }); - it('notifies for server errors on accessibility save', function () { + it('notifies for server errors on accessibility save', async function () { stubSettings(server, {}); stubUser(server, {testUserFlag: false}, false); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); return service.fetch().then(() => { @@ -294,12 +327,15 @@ describe('Integration: Service: feature', function () { }); }); - it('notifies for validation errors', function () { + it('notifies for validation errors', async function () { stubSettings(server, {testFlag: false}, true, false); stubUser(server, {}); addTestFlag(); + let session = this.owner.lookup('service:session'); + await session.populateUser(); + let service = this.owner.lookup('service:feature'); service.get('config').set('testFlag', false); diff --git a/ghost/admin/tests/unit/authenticators/cookie-test.js b/ghost/admin/tests/unit/authenticators/cookie-test.js index 9185732eed..09ff2dd663 100644 --- a/ghost/admin/tests/unit/authenticators/cookie-test.js +++ b/ghost/admin/tests/unit/authenticators/cookie-test.js @@ -61,10 +61,6 @@ describe('Unit: Authenticator: cookie', () => { let authenticator = this.owner.lookup('authenticator:cookie'); let post = authenticator.ajax.post; - let config = this.owner.lookup('service:config'); - let feature = this.owner.lookup('service:feature'); - let settings = this.owner.lookup('service:settings'); - return authenticator.authenticate('AzureDiamond', 'hunter2').then(() => { expect(post.args[0][0]).to.equal(`${ghostPaths().apiRoot}/session`); expect(post.args[0][1]).to.deep.include({ @@ -79,11 +75,6 @@ describe('Unit: Authenticator: cookie', () => { expect(post.args[0][1]).to.deep.include({ contentType: 'application/json;charset=utf-8' }); - - // ensure our pre-loading calls have been made - expect(config.fetchAuthenticated.calledOnce, 'config.fetchAuthenticated called').to.be.true; - expect(feature.fetch.calledOnce, 'feature.fetch called').to.be.true; - expect(settings.fetch.calledOnce, 'settings.fetch called').to.be.true; }); }); });