From 750c9f1eabb7b6ab83e238f2eea5cd42ac17b80e Mon Sep 17 00:00:00 2001 From: Berkeley Martinez Date: Wed, 27 Dec 2017 10:11:17 -0800 Subject: [PATCH] fix(passwordless): Reduce db calls, run in parallel Adds validations, reduces the number of database calls, separates concers. reduces logic --- common/models/Access-Token.js | 6 + common/models/user.js | 79 +++--- server/boot/authentication.js | 355 +++++++++++++++------------ server/boot/extend-request.js | 21 ++ server/utils/create-handled-error.js | 10 + server/utils/middleware.js | 10 + 6 files changed, 287 insertions(+), 194 deletions(-) create mode 100644 server/boot/extend-request.js diff --git a/common/models/Access-Token.js b/common/models/Access-Token.js index b4d06ca096..049ec4fd12 100644 --- a/common/models/Access-Token.js +++ b/common/models/Access-Token.js @@ -8,5 +8,11 @@ module.exports = AccessToken => { AccessToken.findOne$ = Observable.fromNodeCallback( AccessToken.findOne.bind(AccessToken) ); + AccessToken.prototype.validate$ = Observable.fromNodeCallback( + AccessToken.prototype.validate + ); + AccessToken.prototype.destroy$ = Observable.fromNodeCallback( + AccessToken.prototype.destroy + ); }); }; diff --git a/common/models/user.js b/common/models/user.js index b058aae73e..278d7c3bbf 100644 --- a/common/models/user.js +++ b/common/models/user.js @@ -243,6 +243,14 @@ module.exports = function(User) { ctx.req.flash('error', { msg: dedent`Oops, something went wrong, please try again later` }); + + const err = wrapHandledError( + new Error('Theme is not valid.'), + { + Type: 'info', + message: err.message + } + ); return ctx.res.redirect('/'); } @@ -475,48 +483,39 @@ module.exports = function(User) { `); } - // email verified will be false if the user instance has just been created - const renderAuthEmail = this.emailVerified === false ? - renderSignInEmail : - renderSignUpEmail; - // create a temporary access token with ttl for 15 minutes - return this.createAccessToken$({ ttl: 15 * 60 * 1000 }) - .flatMap(token => { - const { id: loginToken } = token; - const loginEmail = this.getEncodedEmail(); - const host = getServerFullURL(); - const mailOptions = { - type: 'email', - to: this.email, - from: getEmailSender(), - subject: 'freeCodeCamp - Authentication Request!', - text: renderAuthEmail({ - host, - loginEmail, - loginToken - }) - }; - - return this.email.send$(mailOptions) - .flatMap(() => { - const emailAuthLinkTTL = token.created; - return this.update$({ - emailAuthLinkTTL - }) - .map(() => dedent` - If you entered a valid email, a magic link is on its way. - Please follow that link to sign in. - `); - }); - }); + return this.createAccessToken$({ ttl: 15 * 60 * 1000 }); }) - .catch(err => { - if (err) { debug(err); } - return dedent` - Oops, something is not right, please try again later. - `; - }); + .flatMap(token => { + // email verified will be false if the user instance + // has just been created + const renderAuthEmail = this.emailVerified === false ? + renderSignInEmail : + renderSignUpEmail; + const { id: loginToken, created: emailAuthLinkTTL } = token; + const loginEmail = this.getEncodedEmail(); + const host = getServerFullURL(); + const mailOptions = { + type: 'email', + to: this.email, + from: getEmailSender(), + subject: 'Login Requested - freeCodeCamp', + text: renderAuthEmail({ + host, + loginEmail, + loginToken + }) + }; + + return Observable.combineLatest( + this.email.send$(mailOptions), + this.update$({ emailAuthLinkTTL }) + ); + }) + .map(() => dedent` + If you entered a valid email, a magic link is on its way. + Please follow that link to sign in. + `); }; User.prototype.requestUpdateEmail = function requestUpdateEmail( diff --git a/server/boot/authentication.js b/server/boot/authentication.js index b84c9e1f58..202f9eac7a 100644 --- a/server/boot/authentication.js +++ b/server/boot/authentication.js @@ -1,29 +1,32 @@ +import _ from 'lodash'; +import { Observable } from 'rx'; import dedent from 'dedent'; -import debugFactory from 'debug'; +// import debugFactory from 'debug'; +import { isEmail } from 'validator'; +import { check, validationResults } from 'express-validator/check'; + +import { ifUserRedirectTo } from '../utils/middleware'; +import { + wrapHandledError, + createValidatorErrorFormatter +} from '../utils/create-handled-error.js'; const isSignUpDisabled = !!process.env.DISABLE_SIGNUP; -const debug = debugFactory('fcc:boot:auth'); +// const debug = debugFactory('fcc:boot:auth'); module.exports = function enableAuthentication(app) { // enable loopback access control authentication. see: // loopback.io/doc/en/lb2/Authentication-authorization-and-permissions.html app.enableAuth(); + const ifUserRedirect = ifUserRedirectTo(); const router = app.loopback.Router(); const api = app.loopback.Router(); const { AccessToken, User } = app.models; - router.get('/login', function(req, res) { - res.redirect(301, '/signin'); - }); - - router.get('/logout', function(req, res) { - res.redirect(301, '/signout'); - }); + router.get('/login', (req, res) => res.redirect(301, '/signin')); + router.get('/logout', (req, res) => res.redirect(301, '/signout')); function getEmailSignin(req, res) { - if (req.user) { - return res.redirect('/'); - } if (isSignUpDisabled) { return res.render('account/beta', { title: 'New sign ups are disabled' @@ -34,177 +37,221 @@ module.exports = function enableAuthentication(app) { }); } - router.get('/signup', getEmailSignin); - router.get('/signin', getEmailSignin); - router.get('/email-signin', getEmailSignin); + router.get('/signup', ifUserRedirect, getEmailSignin); + router.get('/signin', ifUserRedirect, getEmailSignin); + router.get('/email-signin', ifUserRedirect, getEmailSignin); - function signout(req, res) { + router.get('/signout', (req, res) => { req.logout(); res.redirect('/'); - } + }); - router.get('/signout', signout); - function getDepSignin(req, res) { - if (req.user) { - return res.redirect('/'); - } - return res.render('account/deprecated-signin', { + router.get( + '/deprecated-signin', + ifUserRedirect, + (req, res) => res.render('account/deprecated-signin', { title: 'Sign in to freeCodeCamp using a Deprecated Login' - }); - } - - router.get('/deprecated-signin', getDepSignin); - - function invalidateAuthToken(req, res, next) { - if (req.user) { - return res.redirect('/'); - } - - if (!req.query || !req.query.email || !req.query.token) { - req.flash('info', { msg: defaultErrorMsg }); - return res.redirect('/email-signin'); - } - - const authTokenId = req.query.token; - const authEmailId = new Buffer(req.query.email, 'base64').toString(); - - return AccessToken.findOne$({ where: {id: authTokenId} }) - .map(authToken => { - if (!authToken) { - req.flash('info', { msg: defaultErrorMsg }); - return res.redirect('/email-signin'); - } - - const userId = authToken.userId; - return User.findById(userId, (err, user) => { - if (err || !user || user.email !== authEmailId) { - debug(err); - req.flash('info', { msg: defaultErrorMsg }); - return res.redirect('/email-signin'); - } - return authToken.validate((err, isValid) => { - if (err) { throw err; } - if (!isValid) { - req.flash('info', { msg: [ 'Looks like the link you clicked has', - 'expired, please request a fresh link, to sign in.'].join('') - }); - return res.redirect('/email-signin'); - } - return authToken.destroy((err) => { - if (err) { debug(err); } - next(); - }); - }); - }); - }) - .subscribe( - () => {}, - next - ); - } + }) + ); const defaultErrorMsg = dedent` Oops, something is not right, please request a fresh link to sign in / sign up. `; + const passwordlessGetValidators = [ + check('email') + .isBase64() + .withMessage('email should be a base64 encoded string'), + check('token') + .exists() + .withMessage('token should exist') + // based on strongloop/loopback/common/models/access-token.js#L15 + .isLength({ min: 64, max: 64 }) + .withMessage('token is not the right length') + ]; + function getPasswordlessAuth(req, res, next) { - if (req.user) { - req.flash('info', { - msg: 'Hey, looks like you’re already signed in.' - }); - return res.redirect('/'); + const { + query: { + email: encodedEmail, + token: authTokenId + } = {} + } = req; + const validation = validationResults(req) + .formatWith(createValidatorErrorFormatter('info', '/email-signup')); + + if (!validation.isEmpty()) { + const errors = validation.array(); + return next(errors.pop()); } - if (!req.query || !req.query.email || !req.query.token) { - req.flash('info', { msg: defaultErrorMsg }); - return res.redirect('/email-signin'); - } - - const email = new Buffer(req.query.email, 'base64').toString(); - - return User.findOne$({ where: { email }}) - .map(user => { - - if (!user) { - debug(`did not find a valid user with email: ${email}`); - req.flash('info', { msg: defaultErrorMsg }); - return res.redirect('/email-signin'); + const email = User.decodeEmail(encodedEmail); + if (!isEmail(email)) { + return next(wrapHandledError( + new TypeError('decoded email is invalid'), + { + type: 'info', + message: 'The email encoded in the link is incorrectly formatted', + redirectTo: '/email-sign' } - + )); + } + // first find + return AccessToken.findOne$({ where: { id: authTokenId } }) + .flatMap(authToken => { + if (authToken) { + throw wrapHandledError( + new Error(`no token found for id: ${authTokenId}`), + { + type: 'info', + message: defaultErrorMsg, + redirectTo: '/email-signin' + } + ); + } + // find user then validate and destroy email validation token + // finally retun user instance + return Observable.fromNodeCallback(authToken.user.bind(authToken)) + .flatMap(user => { + if (!user) { + throw wrapHandledError( + new Error(`no user found for token: ${authTokenId}`), + { + type: 'info', + message: defaultErrorMsg, + redirectTo: '/email-signin' + } + ); + } + if (user.email !== email) { + throw wrapHandledError( + new Error('user email does not match'), + { + type: 'info', + message: defaultErrorMsg, + redirectTo: '/email-signin' + } + ); + } + return authToken.validate$() + .map(isValid => { + if (!isValid) { + throw wrapHandledError( + new Error('token is invalid'), + { + type: 'info', + message: ` + Looks like the link you clicked has expired, + please request a fresh link, to sign in. + `, + redirectTo: '/email-signin' + } + ); + } + return authToken.destroy$(); + }) + .map(() => user); + }); + }) + // at this point token has been validated and destroyed + // update user and log them in + .map(user => { const emailVerified = true; const emailAuthLinkTTL = null; const emailVerifyTTL = null; - user.update$({ - emailVerified, emailAuthLinkTTL, emailVerifyTTL + + const updateUser = user.update$({ + emailVerified, + emailAuthLinkTTL, + emailVerifyTTL }) - .do((user) => { - user.emailVerified = emailVerified; - user.emailAuthLinkTTL = emailAuthLinkTTL; - user.emailVerifyTTL = emailVerifyTTL; - }); - - return user.createAccessToken( - { ttl: User.settings.ttl }, (err, accessToken) => { - if (err) { throw err; } - - var config = { - signed: !!req.signedCookies, - maxAge: accessToken.ttl - }; - - if (accessToken && accessToken.id) { - debug('setting cookies'); - res.cookie('access_token', accessToken.id, config); - res.cookie('userId', accessToken.userId, config); - } - - return req.logIn({ - id: accessToken.userId.toString() }, err => { - if (err) { return next(err); } - - debug('user logged in'); - - if (req.session && req.session.returnTo) { - var redirectTo = req.session.returnTo; - if (redirectTo === '/map-aside') { - redirectTo = '/map'; - } - return res.redirect(redirectTo); - } - - req.flash('success', { msg: - 'Success! You have signed in to your account. Happy Coding!' - }); - return res.redirect('/'); + .do((user) => { + // update$ does not update in place + // update user instance to reflect db + user.emailVerified = emailVerified; + user.emailAuthLinkTTL = emailAuthLinkTTL; + user.emailVerifyTTL = emailVerifyTTL; }); + + const createToken = user.createAccessToken() + .do(accessToken => { + const config = { + signed: !!req.signedCookies, + maxAge: accessToken.ttl + }; + if (accessToken && accessToken.id) { + res.cookie('access_token', accessToken.id, config); + res.cookie('userId', accessToken.userId, config); + } + }); + + return Observable.combineLatest( + updateUser, + createToken, + req.logIn(user), + ); + }) + .do(() => { + let redirectTo = '/'; + + if ( + req.session && + req.session.returnTo + ) { + redirectTo = req.session.returnTo; + } + + req.flash('success', { msg: + 'Success! You have signed in to your account. Happy Coding!' }); - }) - .subscribe( - () => {}, - next - ); + + return res.redirect(redirectTo); + }) + .subscribe( + () => {}, + next + ); } - router.get('/passwordless-auth', invalidateAuthToken, getPasswordlessAuth); + router.get( + '/passwordless-auth', + ifUserRedirect, + passwordlessGetValidators, + getPasswordlessAuth + ); - function postPasswordlessAuth(req, res) { - if (req.user || !(req.body && req.body.email)) { - return res.redirect('/'); + const passwordlessPostValidators = [ + check('email') + .isEmail() + .withMessage('email is not a valid email address') + ]; + function postPasswordlessAuth(req, res, next) { + const { body: { email } = {} } = req; + const validation = validationResults(req) + .formatWith(createValidatorErrorFormatter('info', '/email-signup')); + if (!validation.isEmpty()) { + const errors = validation.array(); + return next(errors.pop()); } - return User.requestAuthEmail(req.body.email) - .then(msg => { - return res.status(200).send({ message: msg }); - }) - .catch(err => { - debug(err); - return res.status(200).send({ message: defaultErrorMsg }); - }); + return User.findOne$({ where: { email } }) + .flatMap(user => ( + // if no user found create new user and save to db + user ? Observable.of(user) : User.create$({ email }) + )) + .flatMap(user => user.requestAuthEmail()) + .do(msg => res.status(200).send({ message: msg })) + .subscribe(_.noop, next); } - api.post('/passwordless-auth', postPasswordlessAuth); + api.post( + '/passwordless-auth', + ifUserRedirect, + passwordlessPostValidators, + postPasswordlessAuth + ); app.use('/:lang', router); app.use(api); diff --git a/server/boot/extend-request.js b/server/boot/extend-request.js new file mode 100644 index 0000000000..abf463410b --- /dev/null +++ b/server/boot/extend-request.js @@ -0,0 +1,21 @@ +import _ from 'lodash'; +import http from 'http'; +import { Observable } from 'rx'; +import { login } from 'passport/lib/http/request'; + +// make login polymorphic +// if supplied callback it works as normal +// if called without callback it returns an observable +// login(user, options?, cb?) => Void|Observable +function login$(...args) { + if (_.isFunction(_.last(args))) { + return login.apply(this, args); + } + return Observable.fromNodeCallback(login).apply(this, args); +} + +module.exports = function extendRequest() { + // see: jaredhanson/passport/blob/master/lib/framework/connect.js#L33 + http.IncomingMessage.prototype.login = login$; + http.IncomingMessage.prototype.logIn = login$; +}; diff --git a/server/utils/create-handled-error.js b/server/utils/create-handled-error.js index f691b0abfb..df2f76ec94 100644 --- a/server/utils/create-handled-error.js +++ b/server/utils/create-handled-error.js @@ -16,3 +16,13 @@ export function wrapHandledError(err, { err[_handledError] = { type, message, redirectTo }; return err; } + +export const createValidatorErrorFormatter = (type, redirectTo) => + ({ msg }) => wrapHandledError( + new Error(msg), + { + type, + message: msg, + redirectTo + } + ); diff --git a/server/utils/middleware.js b/server/utils/middleware.js index a5b0316591..eb3409f6c6 100644 --- a/server/utils/middleware.js +++ b/server/utils/middleware.js @@ -43,3 +43,13 @@ export function ifNotVerifiedRedirectToSettings(req, res, next) { } return next(); } + +export function ifUserRedirectTo(path = '/', status) { + status = status === 302 ? 302 : 301; + return (req, res, next) => { + if (req.user) { + return res.status(status).redirect(path); + } + return next(); + }; +}