From 039b620ecf8182a59e9c3847b474628ae25fed06 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Tue, 25 Jul 2017 19:23:08 +0700 Subject: [PATCH 01/10] Live Reactions should not always expect the comment to be there. --- plugin-api/beta/client/hocs/withReaction.js | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/plugin-api/beta/client/hocs/withReaction.js b/plugin-api/beta/client/hocs/withReaction.js index f7be6e701..40ee6e14c 100644 --- a/plugin-api/beta/client/hocs/withReaction.js +++ b/plugin-api/beta/client/hocs/withReaction.js @@ -54,6 +54,13 @@ export default (reaction) => (WrappedComponent) => { id: fragmentId }); + if (!data) { + if (self) { + throw new Error(`Comment ${action.item_id} was not found`); + } + return; + } + // Add our comment from the mutation to the end. let idx = data.action_summaries.findIndex(isReaction); @@ -96,6 +103,13 @@ export default (reaction) => (WrappedComponent) => { id: fragmentId }); + if (!data) { + if (self) { + throw new Error(`Comment ${action.item_id} was not found`); + } + return; + } + // Check whether we liked this comment. const idx = data.action_summaries.findIndex(isReaction); From 84a20042f208ea336439215f03d33ffb9bff1958 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Tue, 25 Jul 2017 22:53:26 +0700 Subject: [PATCH 02/10] Prefix redux constants in plugins --- plugins/coral-plugin-viewing-options/client/actions.js | 6 +++--- plugins/coral-plugin-viewing-options/client/constants.js | 7 +++++-- plugins/coral-plugin-viewing-options/client/reducer.js | 8 ++++---- plugins/talk-plugin-featured-comments/client/constants.js | 6 ++++-- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/plugins/coral-plugin-viewing-options/client/actions.js b/plugins/coral-plugin-viewing-options/client/actions.js index 8a6473e3c..48ea73bdd 100644 --- a/plugins/coral-plugin-viewing-options/client/actions.js +++ b/plugins/coral-plugin-viewing-options/client/actions.js @@ -1,9 +1,9 @@ -import {VIEWING_OPTIONS_OPEN, VIEWING_OPTIONS_CLOSE} from './constants'; +import {OPEN_MENU, CLOSE_MENU} from './constants'; export const openViewingOptions = () => ({ - type: VIEWING_OPTIONS_OPEN + type: OPEN_MENU, }); export const closeViewingOptions = () => ({ - type: VIEWING_OPTIONS_CLOSE + type: CLOSE_MENU, }); diff --git a/plugins/coral-plugin-viewing-options/client/constants.js b/plugins/coral-plugin-viewing-options/client/constants.js index 586fe7a28..857a76e1b 100644 --- a/plugins/coral-plugin-viewing-options/client/constants.js +++ b/plugins/coral-plugin-viewing-options/client/constants.js @@ -1,2 +1,5 @@ -export const VIEWING_OPTIONS_OPEN = 'VIEWING_OPTIONS_OPEN'; -export const VIEWING_OPTIONS_CLOSE = 'VIEWING_OPTIONS_CLOSE'; +const prefix = 'TALK_VIEWING_OPTIONS'; + +export const OPEN_MENU = `${prefix}_OPEN_MENU`; +export const CLOSE_MENU = `${prefix}_CLOSE_MENU`; + diff --git a/plugins/coral-plugin-viewing-options/client/reducer.js b/plugins/coral-plugin-viewing-options/client/reducer.js index ab9f077df..687de803a 100644 --- a/plugins/coral-plugin-viewing-options/client/reducer.js +++ b/plugins/coral-plugin-viewing-options/client/reducer.js @@ -1,17 +1,17 @@ -import {VIEWING_OPTIONS_OPEN, VIEWING_OPTIONS_CLOSE} from './constants'; +import {OPEN_MENU, CLOSE_MENU} from './constants'; const initialState = { open: false }; -export default function offTopic (state = initialState, action) { +export default function reducer(state = initialState, action) { switch (action.type) { - case VIEWING_OPTIONS_OPEN: + case OPEN_MENU: return { ...state, open: true }; - case VIEWING_OPTIONS_CLOSE: + case CLOSE_MENU: return { ...state, open: false diff --git a/plugins/talk-plugin-featured-comments/client/constants.js b/plugins/talk-plugin-featured-comments/client/constants.js index 7aad99014..c03bf4839 100644 --- a/plugins/talk-plugin-featured-comments/client/constants.js +++ b/plugins/talk-plugin-featured-comments/client/constants.js @@ -1,2 +1,4 @@ -export const SHOW_TOOLTIP = 'SHOW_TOOLTIP'; -export const HIDE_TOOLTIP = 'HIDE_TOOLTIP'; +const prefix = 'TALK_FEATURED_COMMENTS'; + +export const SHOW_TOOLTIP = `${prefix}_SHOW_TOOLTIP`; +export const HIDE_TOOLTIP = `${prefix}_HIDE_TOOLTIP`; From b8c5e564045277845f99ac0b58521d2d572ec810 Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Tue, 25 Jul 2017 23:04:32 +0700 Subject: [PATCH 03/10] Rename action names to match the new constant names :-D --- plugins/coral-plugin-viewing-options/client/actions.js | 4 ++-- .../client/components/ViewingOptions.js | 6 +++--- .../client/containers/ViewingOptions.js | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/plugins/coral-plugin-viewing-options/client/actions.js b/plugins/coral-plugin-viewing-options/client/actions.js index 48ea73bdd..fbac224bf 100644 --- a/plugins/coral-plugin-viewing-options/client/actions.js +++ b/plugins/coral-plugin-viewing-options/client/actions.js @@ -1,9 +1,9 @@ import {OPEN_MENU, CLOSE_MENU} from './constants'; -export const openViewingOptions = () => ({ +export const openMenu = () => ({ type: OPEN_MENU, }); -export const closeViewingOptions = () => ({ +export const closeMenu = () => ({ type: CLOSE_MENU, }); diff --git a/plugins/coral-plugin-viewing-options/client/components/ViewingOptions.js b/plugins/coral-plugin-viewing-options/client/components/ViewingOptions.js index 87680b370..a7b4be755 100644 --- a/plugins/coral-plugin-viewing-options/client/components/ViewingOptions.js +++ b/plugins/coral-plugin-viewing-options/client/components/ViewingOptions.js @@ -7,15 +7,15 @@ import {Icon} from 'plugin-api/beta/client/components/ui'; const ViewingOptions = (props) => { const toggleOpen = () => { if (!props.open) { - props.openViewingOptions(); + props.openMenu(); } else { - props.closeViewingOptions(); + props.closeMenu(); } }; const handleClickOutside = () => { if (props.open) { - props.closeViewingOptions(); + props.closeMenu(); } }; diff --git a/plugins/coral-plugin-viewing-options/client/containers/ViewingOptions.js b/plugins/coral-plugin-viewing-options/client/containers/ViewingOptions.js index cbe1e7fa3..84900b9c6 100644 --- a/plugins/coral-plugin-viewing-options/client/containers/ViewingOptions.js +++ b/plugins/coral-plugin-viewing-options/client/containers/ViewingOptions.js @@ -1,13 +1,13 @@ import {connect} from 'plugin-api/beta/client/hocs'; import {bindActionCreators} from 'redux'; import ViewingOptions from '../components/ViewingOptions'; -import {openViewingOptions, closeViewingOptions} from '../actions'; +import {openMenu, closeMenu} from '../actions'; const mapStateToProps = ({coralPluginViewingOptions: state}) => ({ open: state.open }); const mapDispatchToProps = (dispatch) => - bindActionCreators({openViewingOptions, closeViewingOptions}, dispatch); + bindActionCreators({openMenu, closeMenu}, dispatch); export default connect(mapStateToProps, mapDispatchToProps)(ViewingOptions); From a91624bf4bd4545759132c425ffc4f214d6830d0 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 26 Jul 2017 15:24:16 +1000 Subject: [PATCH 04/10] Expanded JWT Capabilities --- bin/cli-serve | 4 +- config.js | 51 ++++++++++++++++- services/jwt.js | 81 +++++++++++++++++++++++++++ services/mongoose.js | 2 +- services/passport.js | 21 +++---- test/server/graph/mutations/addTag.js | 1 - webpack.config.js | 3 +- 7 files changed, 147 insertions(+), 16 deletions(-) create mode 100644 services/jwt.js diff --git a/bin/cli-serve b/bin/cli-serve index 46fe51ff9..b04714690 100755 --- a/bin/cli-serve +++ b/bin/cli-serve @@ -90,7 +90,7 @@ async function onListening() { let bind = typeof addr === 'string' ? `pipe ${addr}` : `port ${addr.port}`; - console.log(`API Server Listening on ${bind}`); + debug(`API Server Listening on ${bind}`); } /** @@ -149,7 +149,7 @@ async function startApp(program) { // Mount the websocket server if requested. if (program.websockets) { - console.log(`Websocket Server Listening on ${port}`); + debug(`Websocket Server Listening on ${port}`); // Mount the subscriptions server on the application server. createSubscriptionManager(server); diff --git a/config.js b/config.js index 44747b188..2d8014911 100644 --- a/config.js +++ b/config.js @@ -7,6 +7,8 @@ // entrypoint for the entire applications configuration. require('env-rewrite').rewrite(); +const debug = require('debug')('talk:config'); + //============================================================================== // CONFIG INITIALIZATION //============================================================================== @@ -24,6 +26,8 @@ const CONFIG = { // application. JWT_SECRET: process.env.TALK_JWT_SECRET || null, + JWT_SECRETS: process.env.TALK_JWT_SECRETS || null, + // JWT_AUDIENCE is the value for the audience claim for the tokens that will be // verified when decoding. If `JWT_AUDIENCE` is not in the environment, then it // will default to `talk`. @@ -102,8 +106,53 @@ const CONFIG = { // JWT based configuration //------------------------------------------------------------------------------ +const jwt = require('./services/jwt'); + +if (CONFIG.JWT_SECRETS) { + CONFIG.JWT_SECRETS = JSON.parse(CONFIG.JWT_SECRETS); + if (!Array.isArray(CONFIG.JWT_SECRETS)) { + throw new Error('TALK_JWT_SECRETS must be a JSON array in the form [{"kid": kid, ["secret": secret | "private": private, "public": public]}, ...]'); + } + + if (CONFIG.JWT_SECRETS.length === 0) { + throw new Error('TALK_JWT_SECRETS must be a JSON array with non zero length'); + } + + // Wrap a multi-secret around the available secrets. + CONFIG.JWT_SECRET = new jwt.MultiSecret(CONFIG.JWT_SECRETS.map((secret) => { + if (!('kid' in secret)) { + throw new Error('when multiple keys are specified, kid\'s must be specified'); + } + + // HMAC secrets do not have public/private keys. + if (CONFIG.JWT_ALG.startsWith('HS')) { + return new jwt.SharedSecret(secret); + } + + if (!('public' in secret)) { + throw new Error('all symetric keys must provide a PEM encoded public key'); + } + + return new jwt.AsymmetricSecret(secret); + })); + + debug(`loaded ${CONFIG.JWT_SECRET.length} secrets`); +} else if (CONFIG.JWT_SECRET) { + if (CONFIG.JWT_ALG.startsWith('HS')) { + CONFIG.JWT_SECRET = new jwt.SharedSecret({ + secret: CONFIG.JWT_SECRET + }); + } else { + CONFIG.JWT_SECRET = new jwt.AsymmetricSecret(JSON.parse(CONFIG.JWT_SECRET)); + } + + debug('loaded 1 secret'); +} + if (process.env.NODE_ENV === 'test' && !CONFIG.JWT_SECRET) { - CONFIG.JWT_SECRET = 'keyboard cat'; + CONFIG.JWT_SECRET = new jwt.SharedSecret({ + secret: 'keyboard cat' + }); } else if (!CONFIG.JWT_SECRET) { throw new Error( 'TALK_JWT_SECRET must be provided in the environment to sign/verify tokens' diff --git a/services/jwt.js b/services/jwt.js new file mode 100644 index 000000000..ad881e926 --- /dev/null +++ b/services/jwt.js @@ -0,0 +1,81 @@ +const jwt = require('jsonwebtoken'); + +/** + * MultiSecret will take many secrets and provide a unified interface for + * handling verifying and signing. + */ +class MultiSecret { + constructor(secrets) { + this.secrets = secrets; + } + + sign(payload, options) { + return this.secrets[0].sign(payload, options); + } + + verify(token, options, callback) { + let header = null; + try { + header = JSON.parse(Buffer(token.split('.')[0], 'base64').toString()); + } catch(err) { + return callback(err); + } + + if (!('kid' in header)) { + return callback(new Error('expected kid to exist in the token header, it did not.')); + } + + let kid = header.kid; + let verifier = this.secrets.find((secret) => secret.kid === kid); + if (!verifier) { + return callback(new Error(`expected kid ${kid} was not available.`)); + } + + return verifier.verify(token, options, callback); + } +} + +class SharedSecret { + constructor({kid = undefined, secret}) { + this.kid = kid; + this.secret = secret; + } + + sign(payload, options) { + return jwt.sign(payload, this.secret, Object.assign({}, options, { + keyid: this.kid + })); + } + + verify(token, options, callback) { + jwt.verify(token, this.secret, options, callback); + } +} + +class AsymmetricSecret { + constructor({kid = undefined, private: privateKey, public: publicKey}) { + this.kid = kid; + this.public = Buffer.from(publicKey.replace(/\\n/g, '\n')); + this.private = privateKey ? Buffer.from(privateKey.replace(/\\n/g, '\n')) : null; + } + + sign(payload, options) { + if (!this.private) { + throw new Error('no private key on secret, cannot sign'); + } + + return jwt.sign(payload, this.private, Object.assign({}, options, { + keyid: this.kid + })); + } + + verify(token, options, callback) { + jwt.verify(token, this.public, options, callback); + } +} + +module.exports = { + AsymmetricSecret, + SharedSecret, + MultiSecret +}; diff --git a/services/mongoose.js b/services/mongoose.js index 2b05aaeaa..07f3b2189 100644 --- a/services/mongoose.js +++ b/services/mongoose.js @@ -43,7 +43,7 @@ if (enabled('talk:db')) { if (WEBPACK) { - console.warn('Not connecting to mongodb during webpack build'); + debug('Not connecting to mongodb during webpack build'); // @wyattjoh: We didn't call connect, but because we include mongoose, it will hold the socket ready, // preventing node from exiting. Calling disconnect here just ensures that the application diff --git a/services/passport.js b/services/passport.js index 4ff6198f0..65fa1ac03 100644 --- a/services/passport.js +++ b/services/passport.js @@ -4,7 +4,6 @@ const SettingsService = require('./settings'); const TokensService = require('./tokens'); const fetch = require('node-fetch'); const FormData = require('form-data'); -const JWT = require('jsonwebtoken'); const LocalStrategy = require('passport-local').Strategy; const errors = require('../errors'); const uuid = require('uuid'); @@ -28,13 +27,16 @@ const { // GenerateToken will sign a token to include all the authorization information // needed for the front end. -const GenerateToken = (user) => JWT.sign({}, JWT_SECRET, { - jwtid: uuid.v4(), - expiresIn: JWT_EXPIRY, - issuer: JWT_ISSUER, - subject: user.id, - audience: JWT_AUDIENCE -}); +const GenerateToken = (user) => { + return JWT_SECRET.sign({}, { + jwtid: uuid.v4(), + expiresIn: JWT_EXPIRY, + issuer: JWT_ISSUER, + subject: user.id, + audience: JWT_AUDIENCE, + algorithm: JWT_ALG + }); +}; // SetTokenForSafari sends the token in a cookie for Safari clients. const SetTokenForSafari = (req, res, token) => { @@ -187,7 +189,6 @@ const CheckBlacklisted = async (jwt) => { return checkGeneralTokenBlacklist(jwt); }; -const jwt = require('jsonwebtoken'); const JwtStrategy = require('passport-jwt').Strategy; const ExtractJwt = require('passport-jwt').ExtractJwt; @@ -204,7 +205,7 @@ let cookieExtractor = function(req) { // Override the JwtVerifier method on the JwtStrategy so we can pack the // original token into the payload. JwtStrategy.JwtVerifier = (token, secretOrKey, options, callback) => { - return jwt.verify(token, secretOrKey, options, (err, jwt) => { + return JWT_SECRET.verify(token, options, (err, jwt) => { if (err) { return callback(err); } diff --git a/test/server/graph/mutations/addTag.js b/test/server/graph/mutations/addTag.js index a17d83847..38a96ad1f 100644 --- a/test/server/graph/mutations/addTag.js +++ b/test/server/graph/mutations/addTag.js @@ -37,7 +37,6 @@ describe('graph.mutations.addTag', () => { console.error(res.errors); } - console.log('res.errors', res.errors); expect(res.errors).to.be.empty; let {tags} = await CommentsService.findById(comment.id); diff --git a/webpack.config.js b/webpack.config.js index 1f4c16ee4..21a70ad93 100644 --- a/webpack.config.js +++ b/webpack.config.js @@ -7,6 +7,7 @@ const _ = require('lodash'); const Copy = require('copy-webpack-plugin'); const LicenseWebpackPlugin = require('license-webpack-plugin'); const webpack = require('webpack'); +const debug = require('debug')('talk:webpack'); // Possibly load the config from the .env file (if there is one). require('dotenv').config(); @@ -15,7 +16,7 @@ const {plugins, pluginsPath, PluginManager} = require('./plugins'); const manager = new PluginManager(plugins); const targetPlugins = manager.section('targets').plugins; -console.log(`Using ${pluginsPath} as the plugin configuration path`); +debug(`Using ${pluginsPath} as the plugin configuration path`); const buildTargets = [ 'coral-admin', From 6a12ab28dbac1c08d95bf0929ad5c56c9246bf08 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 26 Jul 2017 15:58:52 +1000 Subject: [PATCH 05/10] fixes for tests --- config.js | 10 ++-- services/jwt.js | 85 +++++++++++++++++++-------- services/tokens.js | 3 +- services/users.js | 141 +++++++++++++++++++-------------------------- 4 files changed, 127 insertions(+), 112 deletions(-) diff --git a/config.js b/config.js index 2d8014911..823ecdcb7 100644 --- a/config.js +++ b/config.js @@ -126,14 +126,14 @@ if (CONFIG.JWT_SECRETS) { // HMAC secrets do not have public/private keys. if (CONFIG.JWT_ALG.startsWith('HS')) { - return new jwt.SharedSecret(secret); + return new jwt.SharedSecret(secret, CONFIG.JWT_ALG); } if (!('public' in secret)) { throw new Error('all symetric keys must provide a PEM encoded public key'); } - return new jwt.AsymmetricSecret(secret); + return new jwt.AsymmetricSecret(secret, CONFIG.JWT_ALG); })); debug(`loaded ${CONFIG.JWT_SECRET.length} secrets`); @@ -141,9 +141,9 @@ if (CONFIG.JWT_SECRETS) { if (CONFIG.JWT_ALG.startsWith('HS')) { CONFIG.JWT_SECRET = new jwt.SharedSecret({ secret: CONFIG.JWT_SECRET - }); + }, CONFIG.JWT_ALG); } else { - CONFIG.JWT_SECRET = new jwt.AsymmetricSecret(JSON.parse(CONFIG.JWT_SECRET)); + CONFIG.JWT_SECRET = new jwt.AsymmetricSecret(JSON.parse(CONFIG.JWT_SECRET), CONFIG.JWT_ALG); } debug('loaded 1 secret'); @@ -152,7 +152,7 @@ if (CONFIG.JWT_SECRETS) { if (process.env.NODE_ENV === 'test' && !CONFIG.JWT_SECRET) { CONFIG.JWT_SECRET = new jwt.SharedSecret({ secret: 'keyboard cat' - }); + }, CONFIG.JWT_ALG); } else if (!CONFIG.JWT_SECRET) { throw new Error( 'TALK_JWT_SECRET must be provided in the environment to sign/verify tokens' diff --git a/services/jwt.js b/services/jwt.js index ad881e926..c5f69c2fd 100644 --- a/services/jwt.js +++ b/services/jwt.js @@ -9,10 +9,17 @@ class MultiSecret { this.secrets = secrets; } + /** + * Sign will sign with the first secret. + */ sign(payload, options) { return this.secrets[0].sign(payload, options); } + /** + * Verify will parse the token and determine the kid, then match it to the + * available secrets, using that to perform the verification. + */ verify(token, options, callback) { let header = null; try { @@ -35,43 +42,73 @@ class MultiSecret { } } -class SharedSecret { - constructor({kid = undefined, secret}) { +/** + * Secret wraps the capabilities expected of a Secret, signing and verifying. + */ +class Secret { + constructor({kid, signingKey, verifiyingKey, algorithm}) { this.kid = kid; - this.secret = secret; + this.signingKey = signingKey; + this.verifiyingKey = verifiyingKey; + this.algorithm = algorithm; } + /** + * Sign will sign the payload with the secret. + * + * @param {Object} payload the object to sign + * @param {Object} options the signing options + */ sign(payload, options) { - return jwt.sign(payload, this.secret, Object.assign({}, options, { - keyid: this.kid + if (!this.signingKey) { + throw new Error('no signing key on secret, cannot sign'); + } + + return jwt.sign(payload, this.signingKey, Object.assign({}, options, { + keyid: this.kid, + algorithm: this.algorithm })); } + /** + * Verify will ensure that the given token was indeed signed with this secret. + * @param {String} token the token to verify + * @param {Object} options the verification options + * @param {Function} callback the function to call with the verification results + */ verify(token, options, callback) { - jwt.verify(token, this.secret, options, callback); + jwt.verify(token, this.verifiyingKey, Object.assign({}, options, { + algorithms: [this.algorithm] + }), callback); } } -class AsymmetricSecret { - constructor({kid = undefined, private: privateKey, public: publicKey}) { - this.kid = kid; - this.public = Buffer.from(publicKey.replace(/\\n/g, '\n')); - this.private = privateKey ? Buffer.from(privateKey.replace(/\\n/g, '\n')) : null; - } +/** + * SharedSecret is the HMAC based secret that's used for signing/verifying. + */ +function SharedSecret({kid = undefined, secret}, algorithm) { + return new Secret({ + kid, + signingKey: secret, + verifiyingKey: secret, + algorithm + }); +} - sign(payload, options) { - if (!this.private) { - throw new Error('no private key on secret, cannot sign'); - } +/** + * AsymmetricSecret is the Asymmetric based key, where a private key is optional + * and the public key is required. + */ +function AsymmetricSecret({kid = undefined, private: privateKey, public: publicKey}, algorithm) { + publicKey = Buffer.from(publicKey.replace(/\\n/g, '\n')); + privateKey = privateKey ? Buffer.from(privateKey.replace(/\\n/g, '\n')) : null; - return jwt.sign(payload, this.private, Object.assign({}, options, { - keyid: this.kid - })); - } - - verify(token, options, callback) { - jwt.verify(token, this.public, options, callback); - } + return new Secret({ + kid, + signingKey: privateKey, + verifiyingKey: publicKey, + algorithm + }); } module.exports = { diff --git a/services/tokens.js b/services/tokens.js index 3c78a0a94..552f64e24 100644 --- a/services/tokens.js +++ b/services/tokens.js @@ -1,6 +1,5 @@ const errors = require('../errors'); const UserModel = require('../models/user'); -const JWT = require('jsonwebtoken'); const uuid = require('uuid'); const { @@ -33,7 +32,7 @@ module.exports = class TokenService { }; // Sign the payload. - const jwt = JWT.sign(payload, JWT_SECRET, {}); + const jwt = JWT_SECRET.sign(payload, {}); // Create the PAT. let pat = { diff --git a/services/users.js b/services/users.js index 12da5ecce..7e7134fbc 100644 --- a/services/users.js +++ b/services/users.js @@ -2,7 +2,6 @@ const assert = require('assert'); const uuid = require('uuid'); const bcrypt = require('bcryptjs'); const url = require('url'); -const jwt = require('jsonwebtoken'); const Wordlist = require('./wordlist'); const errors = require('../errors'); const { @@ -354,7 +353,7 @@ module.exports = class UsersService { */ static disableUser(id) { return UserModel.update({ - id: id + id }, { $set: { disabled: true @@ -369,7 +368,7 @@ module.exports = class UsersService { */ static enableUser(id) { return UserModel.update({ - id: id + id }, { $set: { disabled: false @@ -413,9 +412,7 @@ module.exports = class UsersService { return Promise.reject(new Error(`role ${role} is not supported`)); } - return UserModel.update({ - id: id - }, { + return UserModel.update({id}, { $pull: { roles: role } @@ -461,16 +458,15 @@ module.exports = class UsersService { * @param {Date} until date until the suspension is valid. */ static async suspendUser(id, message, until) { - const user = await UserModel.findOneAndUpdate( - {id}, { - $set: { - suspension: { - until, - }, - } - }, { - new: true, - }); + const user = await UserModel.findOneAndUpdate({id}, { + $set: { + suspension: { + until, + }, + } + }, { + new: true, + }); if (message) { let localProfile = user.profiles.find((profile) => profile.provider === 'local'); @@ -500,9 +496,7 @@ module.exports = class UsersService { * @param {Date} until date until the suspension is valid. */ static async rejectUsername(id, message) { - const user = await UserModel.findOneAndUpdate({ - id - }, { + const user = await UserModel.findOneAndUpdate({id}, { $set: { status: 'BANNED', canEditName: true, @@ -512,18 +506,17 @@ module.exports = class UsersService { }); if (message) { - let localProfile = user.profiles.find((profile) => profile.provider === 'local'); + let localProfile = user.profiles.find(({provider}) => provider === 'local'); if (localProfile) { - const options = - { - template: 'suspension', // needed to know which template to render! - locals: { // specifies the template locals. - body: message - }, - subject: 'Email Suspension', - to: localProfile.id // This only works if the user has registered via e-mail. - // We may want a standard way to access a user's e-mail address in the future - }; + const options = { + template: 'suspension', // needed to know which template to render! + locals: { // specifies the template locals. + body: message + }, + subject: 'Email Suspension', + to: localProfile.id // This only works if the user has registered via e-mail. + // We may want a standard way to access a user's e-mail address in the future + }; await MailerService.sendSimple(options); } @@ -548,9 +541,7 @@ module.exports = class UsersService { static async findOrCreateByIDToken(id, token) { // Try to get the user. - let user = await UserModel.findOne({ - id - }); + let user = await UserModel.findOne({id}); // If the user was not found, try to look it up. if (user === null) { @@ -629,8 +620,7 @@ module.exports = class UsersService { version: user.__v }; - return jwt.sign(payload, JWT_SECRET, { - algorithm: 'HS256', + return JWT_SECRET.sign(payload, { expiresIn: '1d', subject: PASSWORD_RESET_JWT_SUBJECT }); @@ -644,11 +634,7 @@ module.exports = class UsersService { */ static verifyToken(token, options = {}) { return new Promise((resolve, reject) => { - - // Set the allowed algorithms. - options.algorithms = ['HS256']; - - jwt.verify(token, JWT_SECRET, options, (err, decoded) => { + JWT_SECRET.verify(token, options, (err, decoded) => { if (err) { return reject(err); } @@ -762,7 +748,7 @@ module.exports = class UsersService { * @param {String} email The email that we are needing to get confirmed. * @return {Promise} */ - static createEmailConfirmToken(userID = null, email, referer = ROOT_URL) { + static async createEmailConfirmToken(userID = null, email, referer = ROOT_URL) { if (!email || typeof email !== 'string') { return Promise.reject('email is required when creating a JWT for resetting passord'); } @@ -772,41 +758,37 @@ module.exports = class UsersService { const tokenOptions = { jwtid: uuid.v4(), - algorithm: 'HS256', expiresIn: '1d', subject: EMAIL_CONFIRM_JWT_SUBJECT }; - let userPromise; - + let user; if (!userID) { // If there is no userID, we're coming from the endpoint where a new user // is re-requesting a confirmation email and we don't know the userID. - userPromise = UserModel.findOne({profiles: {$elemMatch: {id: email, provider: 'local'}}}); + user = await UserModel.findOne({profiles: {$elemMatch: {id: email, provider: 'local'}}}); } else { - userPromise = UsersService.findById(userID); + user = await UsersService.findById(userID); } - return userPromise.then((user) => { - if (!user) { - return Promise.reject(errors.ErrNotFound); - } + if (!user) { + throw errors.ErrNotFound; + } - // Get the profile representing the local account. - let profile = user.profiles.find((profile) => profile.id === email && profile.provider === 'local'); + // Get the profile representing the local account. + let profile = user.profiles.find((profile) => profile.id === email && profile.provider === 'local'); - // Ensure that the user email hasn't already been verified. - if (profile && profile.metadata && profile.metadata.confirmed_at) { - return Promise.reject(new Error('email address already confirmed')); - } + // Ensure that the user email hasn't already been verified. + if (profile && profile.metadata && profile.metadata.confirmed_at) { + throw new Error('email address already confirmed'); + } - return jwt.sign({ - email, - referer, - userID: user.id - }, JWT_SECRET, tokenOptions); - }); + return JWT_SECRET.sign({ + email, + referer, + userID: user.id + }, tokenOptions); } /** @@ -816,17 +798,14 @@ module.exports = class UsersService { * signed with our secret. * @return {Promise} */ - static verifyEmailConfirmation(token) { - return UsersService - .verifyToken(token, { - subject: EMAIL_CONFIRM_JWT_SUBJECT - }) - .then(({userID, email, referer}) => { - return UsersService - .confirmEmail(userID, email) - .then(() => ({userID, email, referer})); - }); + static async verifyEmailConfirmation(token) { + let {userID, email, referer} = await UsersService.verifyToken(token, { + subject: EMAIL_CONFIRM_JWT_SUBJECT + }); + await UsersService.confirmEmail(userID, email); + + return {userID, email, referer}; } /** @@ -835,7 +814,7 @@ module.exports = class UsersService { static confirmEmail(id, email) { return UserModel .update({ - id: id, + id, profiles: { $elemMatch: { id: email, @@ -934,18 +913,18 @@ module.exports = class UsersService { /** * Ignore another user - * @param {String} userId the id of the user that is ignoring another users - * @param {String[]} usersToIgnore Array of user IDs to ignore + * @param {String} id the id of the user that is ignoring another users + * @param {Array} usersToIgnore Array of user IDs to ignore */ - static ignoreUsers(userId, usersToIgnore) { + static ignoreUsers(id, usersToIgnore) { assert(Array.isArray(usersToIgnore), 'usersToIgnore is an array'); assert(usersToIgnore.every((u) => typeof u === 'string'), 'usersToIgnore is an array of string user IDs'); - if (usersToIgnore.includes(userId)) { + if (usersToIgnore.includes(id)) { throw new Error('Users cannot ignore themselves'); } // TODO: For each usersToIgnore, make sure they exist? - return UserModel.update({id: userId}, { + return UserModel.update({id}, { $addToSet: { ignoresUsers: { $each: usersToIgnore @@ -957,12 +936,12 @@ module.exports = class UsersService { /** * Stop ignoring other users * @param {String} userId the id of the user that is ignoring another users - * @param {String[]} usersToStopIgnoring Array of user IDs to stop ignoring + * @param {Array} usersToStopIgnoring Array of user IDs to stop ignoring */ - static async stopIgnoringUsers(userId, usersToStopIgnoring) { + static async stopIgnoringUsers(id, usersToStopIgnoring) { assert(Array.isArray(usersToStopIgnoring), 'usersToStopIgnoring is an array'); assert(usersToStopIgnoring.every((u) => typeof u === 'string'), 'usersToStopIgnoring is an array of string user IDs'); - await UserModel.update({id: userId}, { + await UserModel.update({id}, { $pullAll: { ignoresUsers: usersToStopIgnoring } From b1b4e9fc57a3f403c0e5eae72f138e62cce2d7ee Mon Sep 17 00:00:00 2001 From: Chi Vinh Le Date: Wed, 26 Jul 2017 16:34:07 +0700 Subject: [PATCH 06/10] Don't use `yarn generate-introspection` --- package.json | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/package.json b/package.json index 65c802ba8..8064757d3 100644 --- a/package.json +++ b/package.json @@ -6,10 +6,10 @@ "scripts": { "postinstall": "./bin/cli plugins reconcile --skip-remote", "start": "./bin/cli serve -j -w", - "dev-start": "nodemon -w . -w bin/cli -w bin/cli-serve --config .nodemon.json --exec \"yarn generate-introspection && ./bin/cli -c .env serve -j -w\"", - "prebuild": "yarn generate-introspection", + "dev-start": "nodemon -w . -w bin/cli -w bin/cli-serve --config .nodemon.json --exec \"WEBPACK=true NODE_ENV=test ./scripts/generateIntrospectionResult.js && ./bin/cli -c .env serve -j -w\"", + "prebuild": "WEBPACK=true NODE_ENV=test ./scripts/generateIntrospectionResult.js", "build": "WEBPACK=true NODE_ENV=production webpack -p --config webpack.config.js --bail", - "prebuild-watch": "yarn generate-introspection", + "prebuild-watch": "WEBPACK=true NODE_ENV=test ./scripts/generateIntrospectionResult.js", "build-watch": "WEBPACK=true NODE_ENV=development webpack --progress --config webpack.config.js --watch", "lint": "eslint bin/* .", "lint-fix": "eslint bin/* . --fix", @@ -19,8 +19,7 @@ "e2e": "NODE_ENV=test nightwatch", "poste2e": "NODE_ENV=test scripts/poste2e.sh", "embed-start": "NODE_ENV=development yarn build && ./bin/cli serve --jobs", - "heroku-postbuild": "./bin/cli plugins reconcile && yarn build", - "generate-introspection": "WEBPACK=true NODE_ENV=test ./scripts/generateIntrospectionResult.js" + "heroku-postbuild": "./bin/cli plugins reconcile && yarn build" }, "talk": { "migration": { From 67f1ee1e39dc6c99e1d7604d45ac9e5b4b40f0c8 Mon Sep 17 00:00:00 2001 From: Samantha Hankins Date: Wed, 26 Jul 2017 15:00:48 -0400 Subject: [PATCH 07/10] changes to off-topic badge colors --- plugins/coral-plugin-offtopic/client/components/styles.css | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/plugins/coral-plugin-offtopic/client/components/styles.css b/plugins/coral-plugin-offtopic/client/components/styles.css index 94d7eaa85..24da7f587 100644 --- a/plugins/coral-plugin-offtopic/client/components/styles.css +++ b/plugins/coral-plugin-offtopic/client/components/styles.css @@ -8,10 +8,10 @@ } .tag { - background: #3D73D5; + background: #D2D7D3; font-size: 12px; font-weight: bold; - color: white; + color: #4A4A4A; display: inline-block; margin: 0px 5px; padding: 5px 5px; From 9b3776767472a5845fb71f7712f2b4180e8b9fc8 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 27 Jul 2017 10:49:33 +1000 Subject: [PATCH 08/10] refactored secret parsing --- config.js | 45 +---------------------------------------- secrets.js | 48 ++++++++++++++++++++++++++++++++++++++++++++ services/passport.js | 5 ++++- services/tokens.js | 5 ++++- services/users.js | 7 ++++++- 5 files changed, 63 insertions(+), 47 deletions(-) create mode 100644 secrets.js diff --git a/config.js b/config.js index 823ecdcb7..5f43b1c8d 100644 --- a/config.js +++ b/config.js @@ -7,8 +7,6 @@ // entrypoint for the entire applications configuration. require('env-rewrite').rewrite(); -const debug = require('debug')('talk:config'); - //============================================================================== // CONFIG INITIALIZATION //============================================================================== @@ -106,53 +104,12 @@ const CONFIG = { // JWT based configuration //------------------------------------------------------------------------------ -const jwt = require('./services/jwt'); - if (CONFIG.JWT_SECRETS) { CONFIG.JWT_SECRETS = JSON.parse(CONFIG.JWT_SECRETS); - if (!Array.isArray(CONFIG.JWT_SECRETS)) { - throw new Error('TALK_JWT_SECRETS must be a JSON array in the form [{"kid": kid, ["secret": secret | "private": private, "public": public]}, ...]'); - } - - if (CONFIG.JWT_SECRETS.length === 0) { - throw new Error('TALK_JWT_SECRETS must be a JSON array with non zero length'); - } - - // Wrap a multi-secret around the available secrets. - CONFIG.JWT_SECRET = new jwt.MultiSecret(CONFIG.JWT_SECRETS.map((secret) => { - if (!('kid' in secret)) { - throw new Error('when multiple keys are specified, kid\'s must be specified'); - } - - // HMAC secrets do not have public/private keys. - if (CONFIG.JWT_ALG.startsWith('HS')) { - return new jwt.SharedSecret(secret, CONFIG.JWT_ALG); - } - - if (!('public' in secret)) { - throw new Error('all symetric keys must provide a PEM encoded public key'); - } - - return new jwt.AsymmetricSecret(secret, CONFIG.JWT_ALG); - })); - - debug(`loaded ${CONFIG.JWT_SECRET.length} secrets`); -} else if (CONFIG.JWT_SECRET) { - if (CONFIG.JWT_ALG.startsWith('HS')) { - CONFIG.JWT_SECRET = new jwt.SharedSecret({ - secret: CONFIG.JWT_SECRET - }, CONFIG.JWT_ALG); - } else { - CONFIG.JWT_SECRET = new jwt.AsymmetricSecret(JSON.parse(CONFIG.JWT_SECRET), CONFIG.JWT_ALG); - } - - debug('loaded 1 secret'); } if (process.env.NODE_ENV === 'test' && !CONFIG.JWT_SECRET) { - CONFIG.JWT_SECRET = new jwt.SharedSecret({ - secret: 'keyboard cat' - }, CONFIG.JWT_ALG); + CONFIG.JWT_SECRET = 'keyboard cat'; } else if (!CONFIG.JWT_SECRET) { throw new Error( 'TALK_JWT_SECRET must be provided in the environment to sign/verify tokens' diff --git a/secrets.js b/secrets.js new file mode 100644 index 000000000..ee484a420 --- /dev/null +++ b/secrets.js @@ -0,0 +1,48 @@ +const { + JWT_SECRETS, + JWT_SECRET, + JWT_ALG +} = require('./config'); + +const debug = require('debug')('talk:secrets'); +const jwt = require('./services/jwt'); + +if (JWT_SECRETS) { + if (!Array.isArray(JWT_SECRETS)) { + throw new Error('TALK_JWT_SECRETS must be a JSON array in the form [{"kid": kid, ["secret": secret | "private": private, "public": public]}, ...]'); + } + + if (JWT_SECRETS.length === 0) { + throw new Error('TALK_JWT_SECRETS must be a JSON array with non zero length'); + } + + // Wrap a multi-secret around the available secrets. + module.exports.jwt = new jwt.MultiSecret(JWT_SECRETS.map((secret) => { + if (!('kid' in secret)) { + throw new Error('when multiple keys are specified, kid\'s must be specified'); + } + + // HMAC secrets do not have public/private keys. + if (JWT_ALG.startsWith('HS')) { + return new jwt.SharedSecret(secret, JWT_ALG); + } + + if (!('public' in secret)) { + throw new Error('all symetric keys must provide a PEM encoded public key'); + } + + return new jwt.AsymmetricSecret(secret, JWT_ALG); + })); + + debug(`loaded ${JWT_SECRET.length} secrets`); +} else if (JWT_SECRET) { + if (JWT_ALG.startsWith('HS')) { + module.exports.jwt = new jwt.SharedSecret({ + secret: JWT_SECRET + }, JWT_ALG); + } else { + module.exports.jwt = new jwt.AsymmetricSecret(JSON.parse(JWT_SECRET), JWT_ALG); + } + + debug('loaded 1 secret'); +} diff --git a/services/passport.js b/services/passport.js index 65fa1ac03..81338d75f 100644 --- a/services/passport.js +++ b/services/passport.js @@ -16,7 +16,6 @@ const {createClientFactory} = require('./redis'); const client = createClientFactory(); const { - JWT_SECRET, JWT_ISSUER, JWT_EXPIRY, JWT_AUDIENCE, @@ -25,6 +24,10 @@ const { RECAPTCHA_ENABLED } = require('../config'); +const { + jwt: JWT_SECRET +} = require('../secrets'); + // GenerateToken will sign a token to include all the authorization information // needed for the front end. const GenerateToken = (user) => { diff --git a/services/tokens.js b/services/tokens.js index 552f64e24..8184d47b5 100644 --- a/services/tokens.js +++ b/services/tokens.js @@ -3,11 +3,14 @@ const UserModel = require('../models/user'); const uuid = require('uuid'); const { - JWT_SECRET, JWT_ISSUER, JWT_AUDIENCE } = require('../config'); +const { + jwt: JWT_SECRET +} = require('../secrets'); + /** * TokenService manages Personal Access Tokens for users. These tokens are * persisted in the database and attached to the user. diff --git a/services/users.js b/services/users.js index 7e7134fbc..7ed55f712 100644 --- a/services/users.js +++ b/services/users.js @@ -4,10 +4,15 @@ const bcrypt = require('bcryptjs'); const url = require('url'); const Wordlist = require('./wordlist'); const errors = require('../errors'); + const { - JWT_SECRET, ROOT_URL } = require('../config'); + +const { + jwt: JWT_SECRET +} = require('../secrets'); + const debug = require('debug')('talk:services:users'); const UserModel = require('../models/user'); From 4733233cae9a9775460cc59db049e9a299a945b8 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 27 Jul 2017 10:58:12 +1000 Subject: [PATCH 09/10] Added new cookie config params --- config.js | 11 ++++++++++- services/passport.js | 14 ++++++++++---- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/config.js b/config.js index 5f43b1c8d..1d12b193e 100644 --- a/config.js +++ b/config.js @@ -14,7 +14,7 @@ require('env-rewrite').rewrite(); const CONFIG = { // WEBPACK indicates when webpack is currently building. - WEBPACK: process.env.WEBPACK === 'true', + WEBPACK: process.env.WEBPACK === 'TRUE', //------------------------------------------------------------------------------ // JWT based configuration @@ -24,8 +24,17 @@ const CONFIG = { // application. JWT_SECRET: process.env.TALK_JWT_SECRET || null, + // JWT_SECRETS is used when key rotation is available. JWT_SECRETS: process.env.TALK_JWT_SECRETS || null, + // JWT_COOKIE_NAME is the name of the cookie optionally containing the JWT + // token. + JWT_COOKIE_NAME: process.env.TALK_JWT_COOKIE_NAME || 'authorization', + + // JWT_CLEAR_COOKIE_LOGOUT specifies whether the named cookie should be + // cleared when the user is logged out. + JWT_CLEAR_COOKIE_LOGOUT: process.env.JWT_CLEAR_COOKIE_LOGOUT ? process.env.JWT_CLEAR_COOKIE_LOGOUT !== 'FALSE' : true, + // JWT_AUDIENCE is the value for the audience claim for the tokens that will be // verified when decoding. If `JWT_AUDIENCE` is not in the environment, then it // will default to `talk`. diff --git a/services/passport.js b/services/passport.js index 81338d75f..542fc9471 100644 --- a/services/passport.js +++ b/services/passport.js @@ -21,7 +21,9 @@ const { JWT_AUDIENCE, JWT_ALG, RECAPTCHA_SECRET, - RECAPTCHA_ENABLED + RECAPTCHA_ENABLED, + JWT_COOKIE_NAME, + JWT_CLEAR_COOKIE_LOGOUT } = require('../config'); const { @@ -45,7 +47,7 @@ const GenerateToken = (user) => { const SetTokenForSafari = (req, res, token) => { const browser = bowser._detect(req.headers['user-agent']); if (browser.ios || browser.safari) { - res.cookie('authorization', token, { + res.cookie(JWT_COOKIE_NAME, token, { httpOnly: true, secure: process.env.NODE_ENV === 'production', expires: new Date(Date.now() + ms(JWT_EXPIRY)) @@ -159,7 +161,11 @@ const HandleLogout = (req, res, next) => { return next(err); } - res.clearCookie('authorization'); + // Only clear the cookie on logout if enabled. + if (JWT_CLEAR_COOKIE_LOGOUT) { + res.clearCookie(JWT_COOKIE_NAME); + } + res.status(204).end(); }); }; @@ -199,7 +205,7 @@ let cookieExtractor = function(req) { let token = null; if (req && req.cookies) { - token = req.cookies['authorization']; + token = req.cookies[JWT_COOKIE_NAME]; } return token; From 59a1435089a134d159dbf2b0e50c79fa6323c488 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Thu, 27 Jul 2017 11:01:31 +1000 Subject: [PATCH 10/10] Updated config --- config.js | 2 +- package.json | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config.js b/config.js index 1d12b193e..5d76c8d0f 100644 --- a/config.js +++ b/config.js @@ -33,7 +33,7 @@ const CONFIG = { // JWT_CLEAR_COOKIE_LOGOUT specifies whether the named cookie should be // cleared when the user is logged out. - JWT_CLEAR_COOKIE_LOGOUT: process.env.JWT_CLEAR_COOKIE_LOGOUT ? process.env.JWT_CLEAR_COOKIE_LOGOUT !== 'FALSE' : true, + JWT_CLEAR_COOKIE_LOGOUT: process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT ? process.env.TALK_JWT_CLEAR_COOKIE_LOGOUT !== 'FALSE' : true, // JWT_AUDIENCE is the value for the audience claim for the tokens that will be // verified when decoding. If `JWT_AUDIENCE` is not in the environment, then it diff --git a/package.json b/package.json index 65c802ba8..21fd9768f 100644 --- a/package.json +++ b/package.json @@ -8,9 +8,9 @@ "start": "./bin/cli serve -j -w", "dev-start": "nodemon -w . -w bin/cli -w bin/cli-serve --config .nodemon.json --exec \"yarn generate-introspection && ./bin/cli -c .env serve -j -w\"", "prebuild": "yarn generate-introspection", - "build": "WEBPACK=true NODE_ENV=production webpack -p --config webpack.config.js --bail", + "build": "WEBPACK=TRUE NODE_ENV=production webpack -p --config webpack.config.js --bail", "prebuild-watch": "yarn generate-introspection", - "build-watch": "WEBPACK=true NODE_ENV=development webpack --progress --config webpack.config.js --watch", + "build-watch": "WEBPACK=TRUE NODE_ENV=development webpack --progress --config webpack.config.js --watch", "lint": "eslint bin/* .", "lint-fix": "eslint bin/* . --fix", "test": "TEST_MODE=unit NODE_ENV=test mocha -R ${MOCHA_REPORTER:-spec}", @@ -20,7 +20,7 @@ "poste2e": "NODE_ENV=test scripts/poste2e.sh", "embed-start": "NODE_ENV=development yarn build && ./bin/cli serve --jobs", "heroku-postbuild": "./bin/cli plugins reconcile && yarn build", - "generate-introspection": "WEBPACK=true NODE_ENV=test ./scripts/generateIntrospectionResult.js" + "generate-introspection": "WEBPACK=TRUE NODE_ENV=test ./scripts/generateIntrospectionResult.js" }, "talk": { "migration": {