From 03665662a78207c0487b4aadc6c7d3a61c67a387 Mon Sep 17 00:00:00 2001 From: Tim Leslie Date: Thu, 6 Dec 2018 15:29:38 +1100 Subject: [PATCH] Move session handling code into a SessionManager class --- .changeset/e3ae8a80/changes.json | 31 +++++++ .changeset/e3ae8a80/changes.md | 4 + packages/admin-ui/server/AdminUI.js | 4 +- .../admin-ui/tests/server/AdminUI.test.js | 4 +- packages/core/Keystone/index.js | 4 +- packages/core/Keystone/session.js | 83 ++++++++++--------- packages/core/auth/index.md | 16 ++-- packages/fields/tests/fields.test.js | 1 - packages/server/WebServer/index.js | 9 +- projects/basic/index.js | 6 -- projects/login/index.js | 6 +- projects/login/tests/auth-header.test.js | 4 +- projects/twitter-login/twitter.js | 2 +- 13 files changed, 95 insertions(+), 79 deletions(-) create mode 100644 .changeset/e3ae8a80/changes.json create mode 100644 .changeset/e3ae8a80/changes.md diff --git a/.changeset/e3ae8a80/changes.json b/.changeset/e3ae8a80/changes.json new file mode 100644 index 00000000000..872717bc80b --- /dev/null +++ b/.changeset/e3ae8a80/changes.json @@ -0,0 +1,31 @@ +{ + "releases": [ + { "name": "@voussoir/admin-ui", "type": "patch" }, + { "name": "@voussoir/core", "type": "major" }, + { "name": "@voussoir/fields", "type": "patch" }, + { "name": "@voussoir/server", "type": "patch" }, + { "name": "@voussoir/cypress-project-basic", "type": "patch" }, + { "name": "@voussoir/cypress-project-login", "type": "patch" }, + { "name": "@voussoir/cypress-project-twitter-login", "type": "patch" } + ], + "dependents": [ + { "name": "@voussoir/adapter-mongoose", "type": "patch", "dependencies": ["@voussoir/core"] }, + { + "name": "@voussoir/test-utils", + "type": "patch", + "dependencies": ["@voussoir/adapter-mongoose", "@voussoir/core", "@voussoir/server"] + }, + { + "name": "@voussoir/cypress-project-access-control", + "type": "patch", + "dependencies": [ + "@voussoir/adapter-mongoose", + "@voussoir/test-utils", + "@voussoir/admin-ui", + "@voussoir/core", + "@voussoir/fields", + "@voussoir/server" + ] + } + ] +} diff --git a/.changeset/e3ae8a80/changes.md b/.changeset/e3ae8a80/changes.md new file mode 100644 index 00000000000..8c0d2067668 --- /dev/null +++ b/.changeset/e3ae8a80/changes.md @@ -0,0 +1,4 @@ +- Rename keystone.session to keystone.sessionManager + - Rename keystone.session.validate to keystone.sessionManager.populateAuthedItemMiddleware + - Rename keystone.session.create to keystone.sessionManager.startAuthedSession + - Rename keystone.session.destroy to keystone.sessionManager.endAuthedSession diff --git a/packages/admin-ui/server/AdminUI.js b/packages/admin-ui/server/AdminUI.js index 6b81b3b5d0e..4ab6911c554 100644 --- a/packages/admin-ui/server/AdminUI.js +++ b/packages/admin-ui/server/AdminUI.js @@ -55,7 +55,7 @@ module.exports = class AdminUI { }); } - await this.keystone.session.create(req, result); + await this.keystone.sessionManager.startAuthedSession(req, result); } catch (e) { return next(e); } @@ -75,7 +75,7 @@ module.exports = class AdminUI { async signout(req, res, next) { let success; try { - await this.keystone.session.destroy(req); + await this.keystone.sessionManager.endAuthedSession(req); success = true; } catch (e) { success = false; diff --git a/packages/admin-ui/tests/server/AdminUI.test.js b/packages/admin-ui/tests/server/AdminUI.test.js index e1e9cda3385..1c3a51aef14 100644 --- a/packages/admin-ui/tests/server/AdminUI.test.js +++ b/packages/admin-ui/tests/server/AdminUI.test.js @@ -19,9 +19,7 @@ jest.doMock('html-webpack-plugin', () => { const AdminUI = require('../../server/AdminUI.js'); const keystone = { - session: { - validate: () => jest.fn(), - }, + sessionManager: {}, getAdminSchema: jest.fn(), getAdminMeta: jest.fn(), }; diff --git a/packages/core/Keystone/index.js b/packages/core/Keystone/index.js index 33ddecab0b0..c1251f414ef 100644 --- a/packages/core/Keystone/index.js +++ b/packages/core/Keystone/index.js @@ -14,7 +14,7 @@ const { mergeRelationships, } = require('./relationship-utils'); const List = require('../List'); -const bindSession = require('./session'); +const SessionManager = require('./session'); const unique = arr => [...new Set(arr)]; @@ -30,7 +30,7 @@ module.exports = class Keystone { this.lists = {}; this.listsArray = []; this.getListByKey = key => this.lists[key]; - this.session = bindSession(this); + this.sessionManager = new SessionManager(this); if (config.adapters) { this.adapters = config.adapters; diff --git a/packages/core/Keystone/session.js b/packages/core/Keystone/session.js index b9657a7a822..f537305a40d 100644 --- a/packages/core/Keystone/session.js +++ b/packages/core/Keystone/session.js @@ -1,47 +1,48 @@ -const noop = () => undefined; - -const validate = keystone => ({ valid = noop, invalid = noop }) => async (req, res, next) => { - if (!req.session || !req.session.keystoneItemId) { - invalid({ req, reason: 'empty' }); - return next(); +class SessionManager { + constructor(keystone) { + this.keystone = keystone; + this.populateAuthedItemMiddleware = this.populateAuthedItemMiddleware.bind(this); } - const list = keystone.lists[req.session.keystoneListKey]; - if (!list) { - // TODO: probably destroy the session - invalid({ req, reason: 'invalid-list' }); - return next(); + + async populateAuthedItemMiddleware(req, res, next) { + if (!req.session || !req.session.keystoneItemId) { + return next(); + } + const list = this.keystone.lists[req.session.keystoneListKey]; + if (!list) { + // TODO: probably destroy the session + return next(); + } + const item = await list.adapter.findById(req.session.keystoneItemId); + if (!item) { + // TODO: probably destroy the session + return next(); + } + req.user = item; + req.authedListKey = list.key; + + next(); } - const item = await list.adapter.findById(req.session.keystoneItemId); - if (!item) { - // TODO: probably destroy the session - invalid({ req, reason: 'invalid-item' }); - return next(); + + startAuthedSession(req, { item, list }) { + return new Promise((resolve, reject) => + req.session.regenerate(err => { + if (err) return reject(err); + req.session.keystoneListKey = list.key; + req.session.keystoneItemId = item.id; + resolve(); + }) + ); } - valid({ req, list, item }); - next(); -}; -function create(req, { item, list }) { - return new Promise((resolve, reject) => - req.session.regenerate(err => { - if (err) return reject(err); - req.session.keystoneListKey = list.key; - req.session.keystoneItemId = item.id; - resolve(); - }) - ); -} -function destroy(req) { - return new Promise((resolve, reject) => - req.session.regenerate(err => { - if (err) return reject(err); - resolve({ success: true }); - }) - ); + endAuthedSession(req) { + return new Promise((resolve, reject) => + req.session.regenerate(err => { + if (err) return reject(err); + resolve({ success: true }); + }) + ); + } } -module.exports = keystone => ({ - create: create, - destroy: destroy, - validate: validate(keystone), -}); +module.exports = SessionManager; diff --git a/packages/core/auth/index.md b/packages/core/auth/index.md index 239ca099c09..8e60c535d94 100644 --- a/packages/core/auth/index.md +++ b/packages/core/auth/index.md @@ -127,10 +127,8 @@ The time spend verifying an actors credentials should be constant-time regardles ```javascript server.app.use( - keystone.session.validate({ - // When logged in, we'll get a req.user object - valid: ({ req, item }) => (req.user = item), - }) + // When logged in, we'll get a req.user object + keystone.sessionManager.populateAuthedItemMiddleware ); const twitterAuth = keystone.createAuthStrategy({ @@ -168,7 +166,7 @@ server.app.get( try { await keystone.auth.User.twitter.connectItem(req, { item: authedItem }); - await keystone.session.create(req, { + await keystone.sessionManager.startAuthedSession(req, { item: authedItem, list: info.list, }); @@ -198,10 +196,8 @@ bank's password over multiple server requests / page refreshes: ```javascript server.app.use( - keystone.session.validate({ - // When logged in, we'll get a req.user object - valid: ({ req, item }) => (req.user = item), - }) + // When logged in, we'll get a req.user object + keystone.sessionManager.populateAuthedItemMiddleware ); const twitterAuth = keystone.createAuthStrategy({ @@ -280,7 +276,7 @@ server.app.post('/auth/twitter/complete', async (req, res, next) => { }); await keystone.auth.User.twitter.connectItem(req, { item }); - await keystone.session.create(req, { item, list }); + await keystone.sessionManager.startAuthedSession(req, { item, list }); res.redirect('/api/session'); } catch (createError) { next(createError); diff --git a/packages/fields/tests/fields.test.js b/packages/fields/tests/fields.test.js index 3bd3b438b0c..8fac561ce5e 100644 --- a/packages/fields/tests/fields.test.js +++ b/packages/fields/tests/fields.test.js @@ -77,7 +77,6 @@ describe('Test CRUD for all fields', () => { // Set up a server (but do not .listen(), we will use supertest to access the app) const server = new WebServer(keystone, { 'cookie secret': 'qwerty', - session: false, }); return { server, keystone }; diff --git a/packages/server/WebServer/index.js b/packages/server/WebServer/index.js index d91d8213a40..443aad13a5f 100644 --- a/packages/server/WebServer/index.js +++ b/packages/server/WebServer/index.js @@ -78,14 +78,7 @@ module.exports = class WebServer { this.app.use(injectAuthCookieMiddleware, sessionMiddleware); // Attach the user to the request for all following route handlers - this.app.use( - this.keystone.session.validate({ - valid: ({ req, list, item }) => { - req.user = item; - req.authedListKey = list.key; - }, - }) - ); + this.app.use(this.keystone.sessionManager.populateAuthedItemMiddleware); } if (adminUI && this.config.authStrategy) { diff --git a/projects/basic/index.js b/projects/basic/index.js index d591a598c9a..229bb09e02b 100644 --- a/projects/basic/index.js +++ b/projects/basic/index.js @@ -151,12 +151,6 @@ const server = new WebServer(keystone, { port, }); -server.app.use( - keystone.session.validate({ - valid: ({ req, item }) => (req.user = item), - }) -); - server.app.get('/reset-db', (req, res) => { const reset = async () => { Object.values(keystone.adapters).forEach(async adapter => { diff --git a/projects/login/index.js b/projects/login/index.js index fd1e6e86040..d0d5bb6788e 100644 --- a/projects/login/index.js +++ b/projects/login/index.js @@ -65,7 +65,7 @@ server.app.post( bodyParser.urlencoded({ extended: true }), async (req, res, next) => { // Cleanup any previous session - await keystone.session.destroy(req); + await keystone.sessionManager.endAuthedSession(req); try { const result = await keystone.auth.User.password.validate({ @@ -77,7 +77,7 @@ server.app.post( success: false, }); } - await keystone.session.create(req, result); + await keystone.sessionManager.startAuthedSession(req, result); res.json({ success: true, itemId: result.item.id, @@ -91,7 +91,7 @@ server.app.post( server.app.get('/signout', async (req, res, next) => { try { - await keystone.session.destroy(req); + await keystone.sessionManager.endAuthedSession(req); res.json({ success: true, }); diff --git a/projects/login/tests/auth-header.test.js b/projects/login/tests/auth-header.test.js index c12735a5268..8c38ba4d98a 100644 --- a/projects/login/tests/auth-header.test.js +++ b/projects/login/tests/auth-header.test.js @@ -48,7 +48,7 @@ function setupKeystone() { bodyParser.urlencoded({ extended: true }), async (req, res, next) => { // Cleanup any previous session - await keystone.session.destroy(req); + await keystone.sessionManager.endAuthedSession(req); try { const result = await keystone.auth.User.password.validate({ @@ -60,7 +60,7 @@ function setupKeystone() { success: false, }); } - await keystone.session.create(req, result); + await keystone.sessionManager.startAuthedSession(req, result); res.json({ success: true, token: req.sessionID, diff --git a/projects/twitter-login/twitter.js b/projects/twitter-login/twitter.js index d87e98e0495..e77353419cd 100644 --- a/projects/twitter-login/twitter.js +++ b/projects/twitter-login/twitter.js @@ -78,7 +78,7 @@ exports.configureTwitterAuth = function(keystone, server) { }); await keystone.auth.User.twitter.connectItem(req, { item }); - await keystone.session.create(req, { item, list }); + await keystone.sessionManager.startAuthedSession(req, { item, list }); res.redirect('/api/session'); } catch (createError) { next(createError);