From 74ca0c4220ed241f16a711bcba12188185bda8b0 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 13:52:56 +0100 Subject: [PATCH 1/3] Refactor email template system --- .../Email/Bodies/SingleCTAEmailBody.coffee | 16 +- .../Email/Bodies/ol-SingleCTAEmailBody.coffee | 16 +- .../coffee/Features/Email/EmailBuilder.coffee | 236 +++++++----------- 3 files changed, 110 insertions(+), 158 deletions(-) diff --git a/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee b/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee index cf4adf8cd7..bffa485f07 100644 --- a/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee +++ b/services/web/app/coffee/Features/Email/Bodies/SingleCTAEmailBody.coffee @@ -4,13 +4,17 @@ settings = require "settings-sharelatex" module.exports = _.template """
-

- <%= title %> -

+ <% if (title) { %> +

+ <%= title %> +

+ <% } %>
 
-

- <%= greeting %> -

+ <% if (greeting) { %> +

+ <%= greeting %> +

+ <% } %>

<%= message %>

diff --git a/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee b/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee index 45bc00383c..ee9907c019 100644 --- a/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee +++ b/services/web/app/coffee/Features/Email/Bodies/ol-SingleCTAEmailBody.coffee @@ -4,13 +4,17 @@ settings = require "settings-sharelatex" module.exports = _.template """
-

- <%= title %> -

+ <% if (title) { %> +

+ <%= title %> +

+ <% } %>
 
-

- <%= greeting %> -

+ <% if (greeting) { %> +

+ <%= greeting %> +

+ <% } %>

<%= message %>

diff --git a/services/web/app/coffee/Features/Email/EmailBuilder.coffee b/services/web/app/coffee/Features/Email/EmailBuilder.coffee index 8952e7b864..a33f02e695 100644 --- a/services/web/app/coffee/Features/Email/EmailBuilder.coffee +++ b/services/web/app/coffee/Features/Email/EmailBuilder.coffee @@ -1,5 +1,6 @@ _ = require('underscore') settings = require("settings-sharelatex") +marked = require('marked') PersonalEmailLayout = require("./Layouts/PersonalEmailLayout") NotificationEmailLayout = require("./Layouts/NotificationEmailLayout") @@ -7,169 +8,113 @@ BaseWithHeaderEmailLayout = require("./Layouts/" + settings.brandPrefix + "BaseW SingleCTAEmailBody = require("./Bodies/" + settings.brandPrefix + "SingleCTAEmailBody") +CTAEmailTemplate = (content) -> + content.greeting ?= () -> 'Hi,' + content.secondaryMessage ?= () -> "" + return { + subject: (opts) -> content.subject(opts), + layout: BaseWithHeaderEmailLayout, + plainTextTemplate: (opts) -> """ +#{content.greeting(opts)} + +#{content.message(opts).trim()} + +#{content.ctaText(opts)}: #{content.ctaURL(opts)} + +#{content.secondaryMessage?(opts).trim() or ""} + +Regards, +The #{settings.appName} Team - #{settings.siteUrl} + """ + compiledTemplate: (opts) -> + SingleCTAEmailBody({ + title: content.title?(opts) + greeting: content.greeting(opts) + message: marked(content.message(opts).trim()) + secondaryMessage: marked(content.secondaryMessage(opts).trim()) + ctaText: content.ctaText(opts) + ctaURL: content.ctaURL(opts) + gmailGoToAction: content.gmailGoToAction?(opts) + }) + } + templates = {} -templates.registered = - subject: _.template "Activate your #{settings.appName} Account" - layout: PersonalEmailLayout - type: "notification" - plainTextTemplate: _.template """ -Congratulations, you've just had an account created for you on #{settings.appName} with the email address "<%= to %>". +templates.registered = CTAEmailTemplate({ + subject: () -> "Activate your #{settings.appName} Account" + message: (opts) -> """ +Congratulations, you've just had an account created for you on #{settings.appName} with the email address '#{opts.to}'. -Click here to set your password and log in: <%= setNewPasswordUrl %> - -If you have any questions or problems, please contact #{settings.adminEmail} +Click here to set your password and log in: """ - compiledTemplate: _.template """ -

Congratulations, you've just had an account created for you on #{settings.appName} with the email address "<%= to %>".

+ secondaryMessage: () -> "If you have any questions or problems, please contact #{settings.adminEmail}" + ctaText: () -> "Set password" + ctaURL: (opts) -> opts.setNewPasswordUrl +}) -

Click here to set your password and log in.

- -

If you have any questions or problems, please contact #{settings.adminEmail}.

+templates.canceledSubscription = CTAEmailTemplate({ + subject: () -> "#{settings.appName} thoughts" + message: () -> """ +I'm sorry to see you cancelled your #{settings.appName} premium account. Would you mind giving us some feedback on what the site is lacking at the moment via this quick survey? """ + secondaryMessage: () -> "Thank you in advance!" + ctaText: () -> "Leave Feedback" + ctaURL: (opts) -> "https://sharelatex.typeform.com/to/f5lBiZ" +}) - -templates.canceledSubscription = - subject: _.template "ShareLaTeX thoughts" - layout: PersonalEmailLayout - type:"lifecycle" - plainTextTemplate: _.template """ -Hi <%= first_name %>, - -I'm sorry to see you cancelled your ShareLaTeX premium account. Would you mind giving me some advice on what the site is lacking at the moment via this survey?: - -https://sharelatex.typeform.com/to/f5lBiZ - -Thank you in advance. - -Henry - -ShareLaTeX Co-founder -""" - compiledTemplate: _.template ''' -

Hi <%= first_name %>,

- -

I'm sorry to see you cancelled your ShareLaTeX premium account. Would you mind giving me some advice on what the site is lacking at the moment via this survey?

- -

Thank you in advance.

- -

-Henry
-ShareLaTeX Co-founder -

-''' - - -templates.passwordResetRequested = - subject: _.template "Password Reset - #{settings.appName}" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Password Reset - -We got a request to reset your #{settings.appName} password. - -Click this link to reset your password: <%= setNewPasswordUrl %> - +templates.passwordResetRequested = CTAEmailTemplate({ + subject: () -> "Password Reset - #{settings.appName}" + title: () -> "Password Reset" + message: () -> "We got a request to reset your #{settings.appName} password." + secondaryMessage: () -> """ If you ignore this message, your password won't be changed. If you didn't request a password reset, let us know. - -Thank you - -#{settings.appName} - <%= siteUrl %> """ - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "Password Reset" - greeting: "Hi," - message: "We got a request to reset your #{settings.appName} password." - secondaryMessage: "If you ignore this message, your password won't be changed.
If you didn't request a password reset, let us know." - ctaText: "Reset password" - ctaURL: opts.setNewPasswordUrl - gmailGoToAction: null - }) + ctaText: () -> "Reset password" + ctaURL: (opts) -> opts.setNewPasswordUrl +}) +templates.confirmEmail = CTAEmailTemplate({ + subject: () -> "Confirm Email - #{settings.appName}" + title: () -> "Confirm Email" + message: () -> "Please confirm your email on #{settings.appName}." + ctaText: () -> "Confirm Email" + ctaURL: (opts) -> opts.confirmEmailUrl +}) -templates.projectInvite = - subject: _.template "<%= project.name %> - shared by <%= owner.email %>" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Hi, <%= owner.email %> wants to share '<%= project.name %>' with you. +templates.projectInvite = CTAEmailTemplate({ + subject: (opts) -> "#{opts.project.name} - shared by #{opts.owner.email}" + title: (opts) -> "#{ opts.project.name } - shared by #{ opts.owner.email }" + message: (opts) -> "#{ opts.owner.email } wants to share '#{ opts.project.name }' with you." + ctaText: () -> "View project" + ctaURL: (opts) -> opts.inviteUrl + gmailGoToAction: (opts) -> + target: opts.inviteUrl + name: "View project" + description: "Join #{ opts.project.name } at #{ settings.appName }" +}) -Follow this link to view the project: <%= inviteUrl %> - -Thank you - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "#{ opts.project.name } – shared by #{ opts.owner.email }" - greeting: "Hi," - message: "#{ opts.owner.email } wants to share “#{ opts.project.name }” with you." - secondaryMessage: null - ctaText: "View project" - ctaURL: opts.inviteUrl - gmailGoToAction: - target: opts.inviteUrl - name: "View project" - description: "Join #{ opts.project.name } at ShareLaTeX" - }) - - -templates.verifyEmailToJoinTeam = - subject: _.template "<%= inviterName %> has invited you to join a team on #{settings.appName}" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ - -Please click the button below to join the team and enjoy the benefits of an upgraded <%= appName %> account. - -<%= acceptInviteUrl %> - -Thank You - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "#{opts.inviterName} has invited you to join a team on #{settings.appName}" - greeting: "Hi," - message: "Please click the button below to join the team and enjoy the benefits of an upgraded #{ opts.appName } account." - secondaryMessage: null - ctaText: "Verify now" - ctaURL: opts.acceptInviteUrl - gmailGoToAction: null - }) - -templates.testEmail = - subject: _.template "A Test Email from ShareLaTeX" - layout: BaseWithHeaderEmailLayout - type:"notification" - plainTextTemplate: _.template """ -Hi, - -This is a test email sent from ShareLaTeX. - -#{settings.appName} - <%= siteUrl %> -""" - compiledTemplate: (opts) -> - SingleCTAEmailBody({ - title: "A Test Email from ShareLaTeX" - greeting: "Hi," - message: "This is a test email sent from ShareLaTeX" - secondaryMessage: null - ctaText: "Open ShareLaTeX" - ctaURL: "/" - gmailGoToAction: null - }) +templates.verifyEmailToJoinTeam = CTAEmailTemplate({ + subject: (opts) -> "#{ opts.inviterName } has invited you to join a team on #{settings.appName}" + title: (opts) -> "#{opts.inviterName} has invited you to join a team on #{settings.appName}" + message: (opts) -> "Please click the button below to join the team and enjoy the benefits of an upgraded #{ settings.appName } account." + ctaText: (opts) -> "Join now" + ctaURL: (opts) -> opts.acceptInviteUrl +}) +templates.testEmail = CTAEmailTemplate({ + subject: () -> "A Test Email from #{settings.appName}" + title: () -> "A Test Email from #{settings.appName}" + greeting: () -> "Hi," + message: () -> "This is a test Email from #{settings.appName}" + ctaText: () -> "Open #{settings.appName}" + ctaURL: () -> settings.siteUrl +}) module.exports = templates: templates - + CTAEmailTemplate: CTAEmailTemplate buildEmail: (templateName, opts)-> template = templates[templateName] opts.siteUrl = settings.siteUrl @@ -180,5 +125,4 @@ module.exports = subject : template.subject(opts) html: template.layout(opts) text: template?.plainTextTemplate?(opts) - type:template.type } From 0dcbc5facb934907ab212eebaa556e4e191091b1 Mon Sep 17 00:00:00 2001 From: James Allen Date: Tue, 19 Jun 2018 13:55:34 +0100 Subject: [PATCH 2/3] Send out confirmation emails on register and record confirmedAt date --- .../PasswordReset/PasswordResetHandler.coffee | 4 +- .../Security/OneTimeTokenHandler.coffee | 18 +-- .../User/UserEmailsConfirmationHandler.coffee | 42 ++++++ .../Features/User/UserEmailsController.coffee | 43 ++++-- .../User/UserRegistrationHandler.coffee | 2 +- .../coffee/Features/User/UserUpdater.coffee | 25 +++- services/web/app/coffee/models/User.coffee | 3 +- services/web/app/coffee/router.coffee | 4 + services/web/app/views/user/confirm_email.pug | 28 ++++ .../public/coffee/directives/asyncForm.coffee | 16 ++- .../acceptance/coffee/UserEmailsTests.coffee | 132 ++++++++++++++++++ .../PasswordResetHandlerTests.coffee | 10 +- .../Security/OneTimeTokenHandlerTests.coffee | 8 +- .../UserEmailsConfirmationHandlerTests.coffee | 125 +++++++++++++++++ .../User/UserEmailsControllerTests.coffee | 92 +++++++----- .../User/UserRegistrationHandlerTests.coffee | 4 +- .../unit/coffee/User/UserUpdaterTests.coffee | 39 +++++- 17 files changed, 516 insertions(+), 79 deletions(-) create mode 100644 services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee create mode 100644 services/web/app/views/user/confirm_email.pug create mode 100644 services/web/test/acceptance/coffee/UserEmailsTests.coffee create mode 100644 services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee index 3947b63004..d383e8b669 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetHandler.coffee @@ -14,7 +14,7 @@ module.exports = if !user? or user.holdingAccount logger.err email:email, "user could not be found for password reset" return callback(null, false) - OneTimeTokenHandler.getNewToken user._id, (err, token)-> + OneTimeTokenHandler.getNewToken 'password', user._id, (err, token)-> if err then return callback(err) emailOptions = to : email @@ -24,7 +24,7 @@ module.exports = callback null, true setNewUserPassword: (token, password, callback = (error, found, user_id) ->)-> - OneTimeTokenHandler.getValueFromTokenAndExpire token, (err, user_id)-> + OneTimeTokenHandler.getValueFromTokenAndExpire 'password', token, (err, user_id)-> if err then return callback(err) if !user_id? return callback null, false, null diff --git a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee index 3824703efb..c989ef9505 100644 --- a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee +++ b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee @@ -6,29 +6,29 @@ logger = require("logger-sharelatex") ONE_HOUR_IN_S = 60 * 60 -buildKey = (token)-> return "password_token:#{token}" +buildKey = (use, token)-> return "#{use}_token:#{token}" module.exports = - getNewToken: (value, options = {}, callback)-> + getNewToken: (use, value, options = {}, callback)-> # options is optional if typeof options == "function" callback = options options = {} expiresIn = options.expiresIn or ONE_HOUR_IN_S - logger.log value:value, "generating token for password reset" token = crypto.randomBytes(32).toString("hex") + logger.log {value, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" multi = rclient.multi() - multi.set buildKey(token), value - multi.expire buildKey(token), expiresIn + multi.set buildKey(use, token), value + multi.expire buildKey(use, token), expiresIn multi.exec (err)-> callback(err, token) - getValueFromTokenAndExpire: (token, callback)-> - logger.log token:token, "getting user id from password token" + getValueFromTokenAndExpire: (use, token, callback)-> + logger.log token_start: token.slice(0,8), "getting value from #{use} token" multi = rclient.multi() - multi.get buildKey(token) - multi.del buildKey(token) + multi.get buildKey(use, token) + multi.del buildKey(use, token) multi.exec (err, results)-> callback err, results?[0] diff --git a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee new file mode 100644 index 0000000000..1fbad4a294 --- /dev/null +++ b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee @@ -0,0 +1,42 @@ +EmailHelper = require "../Helpers/EmailHelper" +EmailHandler = require "../Email/EmailHandler" +OneTimeTokenHandler = require "../Security/OneTimeTokenHandler" +settings = require 'settings-sharelatex' +Errors = require "../Errors/Errors" +logger = require "logger-sharelatex" +UserUpdater = require "./UserUpdater" + +ONE_YEAR_IN_S = 365 * 24 * 60 * 60 + +module.exports = UserEmailsConfirmationHandler = + serializeData: (user_id, email) -> + JSON.stringify({user_id, email}) + + deserializeData: (data) -> + JSON.parse(data) + + sendConfirmationEmail: (user_id, email, emailTemplate, callback = (error) ->) -> + if arguments.length == 3 + callback = emailTemplate + emailTemplate = 'confirmEmail' + email = EmailHelper.parseEmail(email) + return callback(new Error('invalid email')) if !email? + value = UserEmailsConfirmationHandler.serializeData(user_id, email) + OneTimeTokenHandler.getNewToken 'email_confirmation', value, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> + return callback(err) if err? + emailOptions = + to: email + confirmEmailUrl: "#{settings.siteUrl}/user/emails/confirm?token=#{token}" + EmailHandler.sendEmail emailTemplate, emailOptions, callback + + confirmEmailFromToken: (token, callback = (error) ->) -> + logger.log {token_start: token.slice(0,8)}, 'confirming email from token' + OneTimeTokenHandler.getValueFromTokenAndExpire 'email_confirmation', token, (error, data) -> + return callback(error) if error? + if !data? + return callback(new Errors.NotFoundError('no token found')) + {user_id, email} = UserEmailsConfirmationHandler.deserializeData(data) + logger.log {data, user_id, email, token_start: token.slice(0,8)}, 'found data for email confirmation' + if !user_id? or email != EmailHelper.parseEmail(email) + return callback(new Errors.NotFoundError('invalid data')) + UserUpdater.confirmEmail user_id, email, callback diff --git a/services/web/app/coffee/Features/User/UserEmailsController.coffee b/services/web/app/coffee/Features/User/UserEmailsController.coffee index 07433d2dde..f877cbe345 100644 --- a/services/web/app/coffee/Features/User/UserEmailsController.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsController.coffee @@ -2,42 +2,67 @@ AuthenticationController = require('../Authentication/AuthenticationController') UserGetter = require("./UserGetter") UserUpdater = require("./UserUpdater") EmailHelper = require("../Helpers/EmailHelper") +UserEmailsConfirmationHandler = require "./UserEmailsConfirmationHandler" logger = require("logger-sharelatex") +Errors = require "../Errors/Errors" module.exports = UserEmailsController = - list: (req, res) -> + list: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) UserGetter.getUserFullEmails userId, (error, fullEmails) -> - return res.sendStatus 500 if error? + return next(error) if error? res.json fullEmails - add: (req, res) -> + add: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.addEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? - res.sendStatus 200 + return next(error) if error? + UserEmailsConfirmationHandler.sendConfirmationEmail userId, email, (err) -> + return next(error) if error? + res.sendStatus 204 - remove: (req, res) -> + remove: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.removeEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? + return next(error) if error? res.sendStatus 200 - setDefault: (req, res) -> + setDefault: (req, res, next) -> userId = AuthenticationController.getLoggedInUserId(req) email = EmailHelper.parseEmail(req.body.email) return res.sendStatus 422 unless email? UserUpdater.setDefaultEmailAddress userId, email, (error)-> - return res.sendStatus 500 if error? + return next(error) if error? res.sendStatus 200 + + showConfirm: (req, res, next) -> + res.render 'user/confirm_email', { + token: req.query.token, + title: 'confirm_email' + } + + confirm: (req, res, next) -> + token = req.body.token + if !token? + return res.sendStatus 422 + UserEmailsConfirmationHandler.confirmEmailFromToken token, (error) -> + if error? + if error instanceof Errors.NotFoundError + res.status(404).json({ + message: 'Sorry, your confirmation token is invalid or has expired. Please request a new email confirmation link.' + }) + else + next(error) + else + res.sendStatus 200 \ No newline at end of file diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 85cdabdbdf..df7fe93218 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -74,7 +74,7 @@ module.exports = UserRegistrationHandler = logger.log {email}, "user already exists, resending welcome email" ONE_WEEK = 7 * 24 * 60 * 60 # seconds - OneTimeTokenHandler.getNewToken user._id, { expiresIn: ONE_WEEK }, (err, token)-> + OneTimeTokenHandler.getNewToken 'password', user._id, { expiresIn: ONE_WEEK }, (err, token)-> return callback(err) if err? setNewPasswordUrl = "#{settings.siteUrl}/user/activate?token=#{token}&user_id=#{user._id}" diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index 22b31239bd..4de994b5c2 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -5,6 +5,8 @@ db = mongojs.db async = require("async") ObjectId = mongojs.ObjectId UserGetter = require("./UserGetter") +EmailHelper = require "../Helpers/EmailHelper" +Errors = require "../Errors/Errors" module.exports = UserUpdater = updateUser: (query, update, callback = (error) ->) -> @@ -53,7 +55,6 @@ module.exports = UserUpdater = return callback(error) callback() - # remove one of the user's email addresses. The email cannot be the user's # default email address removeEmailAddress: (userId, email, callback) -> @@ -63,7 +64,7 @@ module.exports = UserUpdater = if error? logger.err error:error, 'problem removing users email' return callback(error) - if res.nMatched == 0 + if res.n == 0 return callback(new Error('Cannot remove default email')) callback() @@ -77,10 +78,28 @@ module.exports = UserUpdater = if error? logger.err error:error, 'problem setting default emails' return callback(error) - if res.nMatched == 0 + if res.n == 0 # TODO: Check n or nMatched? return callback(new Error('Default email does not belong to user')) callback() + confirmEmail: (userId, email, callback) -> + email = EmailHelper.parseEmail(email) + return callback(new Error('invalid email')) if !email? + logger.log {userId, email}, 'confirming user email' + query = + _id: userId + 'emails.email': email + update = + $set: + 'emails.$.confirmedAt': new Date() + @updateUser query, update, (error, res) -> + return callback(error) if error? + logger.log {res, userId, email}, "tried to confirm email" + if res.n == 0 + return callback(new Errors.NotFoundError('user id and email do no match')) + callback() + + [ 'updateUser' 'changeEmailAddress' diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index 71982ba40b..730b3668c6 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -10,7 +10,8 @@ UserSchema = new Schema email : {type : String, default : ''} emails: [{ email: { type : String, default : '' }, - createdAt: { type : Date, default: () -> new Date() } + createdAt: { type : Date, default: () -> new Date() }, + confirmedAt: { type: Date } }], first_name : {type : String, default : ''} last_name : {type : String, default : ''} diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 339b4abe1c..6014fea661 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -120,6 +120,10 @@ module.exports = class Router webRouter.post '/user/emails/default', AuthenticationController.requireLogin(), UserEmailsController.setDefault + webRouter.get '/user/emails/confirm', + UserEmailsController.showConfirm + webRouter.post '/user/emails/confirm', + UserEmailsController.confirm webRouter.get '/user/sessions', AuthenticationController.requireLogin(), diff --git a/services/web/app/views/user/confirm_email.pug b/services/web/app/views/user/confirm_email.pug new file mode 100644 index 0000000000..388a0a0252 --- /dev/null +++ b/services/web/app/views/user/confirm_email.pug @@ -0,0 +1,28 @@ +extends ../layout + +block content + .content.content-alt + .container + .row + .col-md-6.col-md-offset-3.col-lg-4.col-lg-offset-4 + .card + .page-header + h1 #{translate("confirm_email")} + form( + async-form="confirm-email", + name="confirmEmailForm" + action="/user/emails/confirm", + method="POST", + id="confirmEmailForm", + auto-submit="true", + ng-cloak + ) + input(type="hidden", name="_csrf", value=csrfToken) + input(type="hidden", name="token", value=token) + form-messages(for="confirmEmailForm") + .alert.alert-success(ng-show="confirmEmailForm.response.success") + | Thank you, your email is now confirmed + p.text-center(ng-show="!confirmEmailForm.response.success && !confirmEmailForm.response.error") + i.fa.fa-fw.fa-spin.fa-spinner + | + | Confirming your email... diff --git a/services/web/public/coffee/directives/asyncForm.coffee b/services/web/public/coffee/directives/asyncForm.coffee index 15e3c9b3fe..acafec563d 100644 --- a/services/web/public/coffee/directives/asyncForm.coffee +++ b/services/web/public/coffee/directives/asyncForm.coffee @@ -15,11 +15,6 @@ define [ scope[attrs.name].response = response = {} scope[attrs.name].inflight = false - element.on "submit", (e) -> - e.preventDefault() - validateCaptchaIfEnabled (response) -> - submitRequest response - validateCaptchaIfEnabled = (callback = (response) ->) -> if attrs.captcha? validateCaptcha callback @@ -84,6 +79,17 @@ define [ text: data.message?.text or data.message or "Something went wrong talking to the server :(. Please try again." type: 'error' ga('send', 'event', formName, 'failure', data.message) + + submit = () -> + validateCaptchaIfEnabled (response) -> + submitRequest response + + element.on "submit", (e) -> + e.preventDefault() + submit() + + if attrs.autoSubmit + submit() } App.directive "formMessages", () -> diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee new file mode 100644 index 0000000000..8cdbea75fa --- /dev/null +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -0,0 +1,132 @@ +expect = require("chai").expect +async = require("async") +User = require "./helpers/User" +request = require "./helpers/request" +settings = require "settings-sharelatex" +rclient = require("redis-sharelatex").createClient(settings.redis.web) + +describe "UserEmails", -> + beforeEach (done) -> + @timeout(20000) + @user = new User() + @user.login done + + describe 'confirming an email', -> + it 'should confirm the email', (done) -> + token = null + async.series [ + (cb) => + @user.request { + method: 'POST', + url: '/user/emails', + json: + email: 'newly-added-email@example.com' + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 204 + cb() + (cb) => + @user.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.not.exist + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # There should only be one confirmation token at the moment + expect(keys.length).to.equal 1 + token = keys[0].split(':')[1] + cb() + (cb) => + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 200 + cb() + (cb) => + @user.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.exist + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # Token should be deleted after use + expect(keys.length).to.equal 0 + cb() + ], done + + it 'should not allow confirmation of the email if the user has changed', (done) -> + token1 = null + token2 = null + @user2 = new User() + @email = 'duplicate-email@example.com' + async.series [ + (cb) => @user2.login cb + (cb) => + # Create email for first user + @user.request { + method: 'POST', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # There should only be one confirmation token at the moment, + # for the first user + expect(keys.length).to.equal 1 + token1 = keys[0].split(':')[1] + cb() + (cb) => + # Delete the email from the first user + @user.request { + method: 'DELETE', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + # Create email for second user + @user2.request { + method: 'POST', + url: '/user/emails', + json: {@email} + }, cb + (cb) => + # Original confirmation token should no longer work + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token1 + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 404 + cb() + (cb) => + rclient.keys 'email_confirmation_token:*', (error, keys) -> + # The first token has been used, so this should be token2 now + expect(keys.length).to.equal 1 + token2 = keys[0].split(':')[1] + cb() + (cb) => + # Second user should be able to confirm the email + @user2.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token2 + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 200 + cb() + (cb) => + @user2.request { url: '/user/emails', json: true }, (error, response, body) -> + expect(response.statusCode).to.equal 200 + expect(body[0].confirmedAt).to.not.exist + expect(body[1].confirmedAt).to.exist + cb() + ], done diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee index 261f5582dd..7ee82fbd7e 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetHandlerTests.coffee @@ -41,7 +41,7 @@ describe "PasswordResetHandler", -> it "should check the user exists", (done)-> @UserGetter.getUserByMainEmail.callsArgWith(1) - @OneTimeTokenHandler.getNewToken.callsArgWith(1) + @OneTimeTokenHandler.getNewToken.yields() @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> exists.should.equal false done() @@ -50,7 +50,7 @@ describe "PasswordResetHandler", -> it "should send the email with the token", (done)-> @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.callsArgWith(1, null, @token) + @OneTimeTokenHandler.getNewToken.yields(null, @token) @EmailHandler.sendEmail.callsArgWith(2) @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> @EmailHandler.sendEmail.called.should.equal true @@ -63,7 +63,7 @@ describe "PasswordResetHandler", -> it "should return exists = false for a holdingAccount", (done) -> @user.holdingAccount = true @UserGetter.getUserByMainEmail.callsArgWith(1, null, @user) - @OneTimeTokenHandler.getNewToken.callsArgWith(1) + @OneTimeTokenHandler.getNewToken.yields() @PasswordResetHandler.generateAndEmailResetToken @user.email, (err, exists)=> exists.should.equal false done() @@ -71,14 +71,14 @@ describe "PasswordResetHandler", -> describe "setNewUserPassword", -> it "should return false if no user id can be found", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.callsArgWith(1) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields() @PasswordResetHandler.setNewUserPassword @token, @password, (err, found) => found.should.equal false @AuthenticationManager.setUserPassword.called.should.equal false done() it "should set the user password", (done)-> - @OneTimeTokenHandler.getValueFromTokenAndExpire.callsArgWith(1, null, @user_id) + @OneTimeTokenHandler.getValueFromTokenAndExpire.yields(null, @user_id) @AuthenticationManager.setUserPassword.callsArgWith(2) @PasswordResetHandler.setNewUserPassword @token, @password, (err, found, user_id) => found.should.equal true diff --git a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee index 1940f34d07..21e8d9f288 100644 --- a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee +++ b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee @@ -37,21 +37,21 @@ describe "OneTimeTokenHandler", -> it "should set a new token into redis with a ttl", (done)-> @redisMulti.exec.callsArgWith(0) - @OneTimeTokenHandler.getNewToken @value, (err, token) => + @OneTimeTokenHandler.getNewToken 'password', @value, (err, token) => @redisMulti.set.calledWith("password_token:#{@stubbedToken.toString("hex")}", @value).should.equal true @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", 60 * 60).should.equal true done() it "should return if there was an error", (done)-> @redisMulti.exec.callsArgWith(0, "error") - @OneTimeTokenHandler.getNewToken @value, (err, token)=> + @OneTimeTokenHandler.getNewToken 'password', @value, (err, token)=> err.should.exist done() it "should allow the expiry time to be overridden", (done) -> @redisMulti.exec.callsArgWith(0) @ttl = 42 - @OneTimeTokenHandler.getNewToken @value, {expiresIn: @ttl}, (err, token) => + @OneTimeTokenHandler.getNewToken 'password', @value, {expiresIn: @ttl}, (err, token) => @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", @ttl).should.equal true done() @@ -59,7 +59,7 @@ describe "OneTimeTokenHandler", -> it "should get and delete the token", (done)-> @redisMulti.exec.callsArgWith(0, null, [@value]) - @OneTimeTokenHandler.getValueFromTokenAndExpire @stubbedToken, (err, value)=> + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', @stubbedToken, (err, value)=> value.should.equal @value @redisMulti.get.calledWith("password_token:#{@stubbedToken}").should.equal true @redisMulti.del.calledWith("password_token:#{@stubbedToken}").should.equal true diff --git a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee new file mode 100644 index 0000000000..22cd70c4bc --- /dev/null +++ b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee @@ -0,0 +1,125 @@ +should = require('chai').should() +SandboxedModule = require('sandboxed-module') +assert = require('assert') +path = require('path') +sinon = require('sinon') +modulePath = path.join __dirname, "../../../../app/js/Features/User/UserEmailsConfirmationHandler" +expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" +EmailHelper = require "../../../../app/js/Features/Helpers/EmailHelper" + +describe "UserEmailsConfirmationHandler", -> + beforeEach -> + @UserEmailsConfirmationHandler = SandboxedModule.require modulePath, requires: + "settings-sharelatex": @settings = + siteUrl: "emails.example.com" + "logger-sharelatex": @logger = { log: sinon.stub() } + "../Security/OneTimeTokenHandler": @OneTimeTokenHandler = {} + "../Errors/Errors": Errors + "./UserUpdater": @UserUpdater = {} + "../Email/EmailHandler": @EmailHandler = {} + "../Helpers/EmailHelper": EmailHelper + @user_id = "mock-user-id" + @email = "mock@example.com" + @callback = sinon.stub() + + describe "sendConfirmationEmail", -> + beforeEach -> + @OneTimeTokenHandler.getNewToken = sinon.stub().yields(null, @token = "new-token") + @EmailHandler.sendEmail = sinon.stub().yields() + + describe 'successfully', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, @email, @callback + + it "should generate a token for the user which references their id and email", -> + @OneTimeTokenHandler.getNewToken + .calledWith( + 'email_confirmation', + JSON.stringify({@user_id, @email}), + { expiresIn: 365 * 24 * 60 * 60 } + ) + .should.equal true + + it 'should send an email to the user', -> + @EmailHandler.sendEmail + .calledWith('confirmEmail', { + to: @email, + confirmEmailUrl: 'emails.example.com/user/emails/confirm?token=new-token' + }) + .should.equal true + + it 'should call the callback', -> + @callback.called.should.equal true + + describe 'with invalid email', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, '!"£$%^&*()', @callback + + it 'should return an error', -> + @callback.calledWith(sinon.match.instanceOf(Error)).should.equal true + + describe 'a custom template', -> + beforeEach -> + @UserEmailsConfirmationHandler.sendConfirmationEmail @user_id, @email, 'myCustomTemplate', @callback + + it 'should send an email with the given template', -> + @EmailHandler.sendEmail + .calledWith('myCustomTemplate') + .should.equal true + + describe "confirmEmailFromToken", -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@user_id, @email}) + ) + @UserUpdater.confirmEmail = sinon.stub().yields() + + describe "successfully", -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call getValueFromTokenAndExpire", -> + @OneTimeTokenHandler.getValueFromTokenAndExpire + .calledWith('email_confirmation', @token) + .should.equal true + + it "should confirm the email of the user_id", -> + @UserUpdater.confirmEmail + .calledWith(@user_id, @email) + .should.equal true + + it "should call the callback", -> + @callback.called.should.equal true + + describe 'with an expired token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields(null, null) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + + describe 'with no user_id in the token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@email}) + ) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + + describe 'with no email in the token', -> + beforeEach -> + @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( + null, + JSON.stringify({@user_id}) + ) + @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback + + it "should call the callback with a NotFoundError", -> + @callback.calledWith(sinon.match.instanceOf(Errors.NotFoundError)).should.equal true + diff --git a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee index 5b4a32fb38..d0e8076276 100644 --- a/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee +++ b/services/web/test/unit/coffee/User/UserEmailsControllerTests.coffee @@ -8,6 +8,7 @@ modulePath = "../../../../app/js/Features/User/UserEmailsController.js" SandboxedModule = require('sandboxed-module') MockRequest = require "../helpers/MockRequest" MockResponse = require "../helpers/MockResponse" +Errors = require("../../../../app/js/Features/Errors/Errors") describe "UserEmailsController", -> beforeEach -> @@ -30,6 +31,8 @@ describe "UserEmailsController", -> "./UserGetter": @UserGetter "./UserUpdater": @UserUpdater "../Helpers/EmailHelper": @EmailHelper + "./UserEmailsConfirmationHandler": @UserEmailsConfirmationHandler = {} + "../Errors/Errors": Errors "logger-sharelatex": log: -> console.log(arguments) err: -> @@ -47,30 +50,29 @@ describe "UserEmailsController", -> assertCalledWith @UserGetter.getUserFullEmails, @user._id done() - it 'handles error', (done) -> - @UserGetter.getUserFullEmails.callsArgWith 1, new Error('Oups') - - @UserEmailsController.list @req, - sendStatus: (code) => - code.should.equal 500 - done() - describe 'Add', -> beforeEach -> @newEmail = 'new_email@baz.com' @req.body.email = @newEmail @EmailHelper.parseEmail.returns @newEmail + @UserUpdater.addEmailAddress.callsArgWith 2, null + @UserEmailsConfirmationHandler.sendConfirmationEmail = sinon.stub().yields() it 'adds new email', (done) -> - @UserUpdater.addEmailAddress.callsArgWith 2, null - @UserEmailsController.add @req, sendStatus: (code) => - code.should.equal 200 + code.should.equal 204 assertCalledWith @EmailHelper.parseEmail, @newEmail assertCalledWith @UserUpdater.addEmailAddress, @user._id, @newEmail done() + it 'sends an email confirmation', (done) -> + @UserEmailsController.add @req, + sendStatus: (code) => + code.should.equal 204 + assertCalledWith @UserEmailsConfirmationHandler.sendConfirmationEmail, @user._id, @newEmail + done() + it 'handles email parse error', (done) -> @EmailHelper.parseEmail.returns null @@ -80,14 +82,6 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.addEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.addEmailAddress.callsArgWith 2, new Error('Oups') - - @UserEmailsController.add @req, - sendStatus: (code) => - code.should.equal 500 - done() - describe 'remove', -> beforeEach -> @email = 'email_to_remove@bar.com' @@ -113,15 +107,6 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.removeEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.removeEmailAddress.callsArgWith 2, new Error('Oups') - - @UserEmailsController.remove @req, - sendStatus: (code) => - code.should.equal 500 - done() - - describe 'setDefault', -> beforeEach -> @email = "email_to_set_default@bar.com" @@ -147,11 +132,50 @@ describe "UserEmailsController", -> assertNotCalled @UserUpdater.setDefaultEmailAddress done() - it 'handles error', (done) -> - @UserUpdater.setDefaultEmailAddress.callsArgWith 2, new Error('Oups') + describe 'confirm', -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken = sinon.stub().yields() + @res = + sendStatus: sinon.stub() + json: sinon.stub() + @res.status = sinon.stub().returns(@res) + @next = sinon.stub() + @token = 'mock-token' + @req.body = token: @token + + describe 'successfully', -> + beforeEach -> + @UserEmailsController.confirm @req, @res, @next + + it 'should confirm the email from the token', -> + @UserEmailsConfirmationHandler.confirmEmailFromToken + .calledWith(@token) + .should.equal true + + it 'should return a 200 status', -> + @res.sendStatus.calledWith(200).should.equal true + + describe 'without a token', -> + beforeEach -> + @req.body.token = null + @UserEmailsController.confirm @req, @res, @next + + it 'should return a 422 status', -> + @res.sendStatus.calledWith(422).should.equal true + + describe 'when confirming fails', -> + beforeEach -> + @UserEmailsConfirmationHandler.confirmEmailFromToken = sinon.stub().yields( + new Errors.NotFoundError('not found') + ) + @UserEmailsController.confirm @req, @res, @next + + it 'should return a 404 error code with a message', -> + @res.status.calledWith(404).should.equal true + @res.json.calledWith({ + message: 'Sorry, your confirmation token is invalid or has expired. Please request a new email confirmation link.' + }).should.equal true + + - @UserEmailsController.setDefault @req, - sendStatus: (code) => - code.should.equal 500 - done() diff --git a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee index 3a8801bc08..e9a6c568dd 100644 --- a/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserRegistrationHandlerTests.coffee @@ -152,7 +152,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @email = "email@example.com" @crypto.randomBytes = sinon.stub().returns({toString: () => @password = "mock-password"}) - @OneTimeTokenHandler.getNewToken.callsArgWith(2, null, @token = "mock-token") + @OneTimeTokenHandler.getNewToken.yields(null, @token = "mock-token") @handler.registerNewUser = sinon.stub() @callback = sinon.stub() @@ -171,7 +171,7 @@ describe "UserRegistrationHandler", -> it "should generate a new password reset token", -> @OneTimeTokenHandler.getNewToken - .calledWith(@user_id, expiresIn: 7 * 24 * 60 * 60) + .calledWith('password', @user_id, expiresIn: 7 * 24 * 60 * 60) .should.equal true it "should send a registered email", -> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 7b3be3df2b..6b9a1ecc85 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -5,11 +5,12 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/User/UserUpdater" expect = require("chai").expect +tk = require('timekeeper') describe "UserUpdater", -> beforeEach -> - + tk.freeze(Date.now()) @settings = {} @mongojs = db:{} @@ -32,6 +33,9 @@ describe "UserUpdater", -> email:"hello@world.com" @newEmail = "bob@bob.com" + afterEach -> + tk.reset() + describe 'changeEmailAddress', -> beforeEach -> @UserGetter.getUserEmail.callsArgWith(1, null, @stubbedUser.email) @@ -103,7 +107,7 @@ describe "UserUpdater", -> done() it 'handle missed update', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) @UserUpdater.removeEmailAddress @stubbedUser._id, @newEmail, (err)=> should.exist(err) @@ -111,7 +115,7 @@ describe "UserUpdater", -> describe 'setDefaultEmailAddress', -> it 'set default', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 1) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> should.not.exist(err) @@ -129,10 +133,37 @@ describe "UserUpdater", -> done() it 'handle missed update', (done)-> - @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, nMatched: 0) + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) @UserUpdater.setDefaultEmailAddress @stubbedUser._id, @newEmail, (err)=> should.exist(err) done() + describe 'confirmEmail', -> + it 'should update the email record', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 1) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.not.exist(err) + @UserUpdater.updateUser.calledWith( + { _id: @stubbedUser._id, 'emails.email': @newEmail }, + $set: { 'emails.$.confirmedAt': new Date() } + ).should.equal true + done() + + it 'handle error', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, new Error('nope')) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + it 'handle missed update', (done)-> + @UserUpdater.updateUser = sinon.stub().callsArgWith(2, null, n: 0) + + @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> + should.exist(err) + done() + + From 4608a59e3db340deb489e2872da5e70bc2e56c62 Mon Sep 17 00:00:00 2001 From: James Allen Date: Wed, 20 Jun 2018 17:27:22 +0100 Subject: [PATCH 3/3] Store OneTimeTokens in mongo rather than redis --- .../Security/OneTimeTokenHandler.coffee | 54 +++++--- .../User/UserEmailsConfirmationHandler.coffee | 12 +- .../app/coffee/infrastructure/mongojs.coffee | 2 +- .../acceptance/coffee/UserEmailsTests.coffee | 92 ++++++++++--- .../Security/OneTimeTokenHandlerTests.coffee | 121 +++++++++++------- .../UserEmailsConfirmationHandlerTests.coffee | 8 +- 6 files changed, 198 insertions(+), 91 deletions(-) diff --git a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee index c989ef9505..69c9f5b0e9 100644 --- a/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee +++ b/services/web/app/coffee/Features/Security/OneTimeTokenHandler.coffee @@ -1,34 +1,50 @@ Settings = require('settings-sharelatex') -RedisWrapper = require("../../infrastructure/RedisWrapper") -rclient = RedisWrapper.client("one_time_token") crypto = require("crypto") logger = require("logger-sharelatex") +{db} = require "../../infrastructure/mongojs" +Errors = require "../Errors/Errors" ONE_HOUR_IN_S = 60 * 60 -buildKey = (use, token)-> return "#{use}_token:#{token}" - module.exports = - - getNewToken: (use, value, options = {}, callback)-> + getNewToken: (use, data, options = {}, callback = (error, data) ->)-> # options is optional if typeof options == "function" callback = options options = {} expiresIn = options.expiresIn or ONE_HOUR_IN_S + createdAt = new Date() + expiresAt = new Date(createdAt.getTime() + expiresIn * 1000) token = crypto.randomBytes(32).toString("hex") - logger.log {value, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" - multi = rclient.multi() - multi.set buildKey(use, token), value - multi.expire buildKey(use, token), expiresIn - multi.exec (err)-> - callback(err, token) + logger.log {data, expiresIn, token_start: token.slice(0,8)}, "generating token for #{use}" + db.tokens.insert { + use: use + token: token, + data: data, + createdAt: createdAt, + expiresAt: expiresAt + }, (error) -> + return callback(error) if error? + callback null, token - getValueFromTokenAndExpire: (use, token, callback)-> - logger.log token_start: token.slice(0,8), "getting value from #{use} token" - multi = rclient.multi() - multi.get buildKey(use, token) - multi.del buildKey(use, token) - multi.exec (err, results)-> - callback err, results?[0] + getValueFromTokenAndExpire: (use, token, callback = (error, data) ->)-> + logger.log token_start: token.slice(0,8), "getting data from #{use} token" + now = new Date() + db.tokens.findAndModify { + query: { + use: use, + token: token, + expiresAt: { $gt: now }, + usedAt: { $exists: false } + }, + update: { + $set: { + usedAt: now + } + } + }, (error, token) -> + return callback(error) if error? + if !token? + return callback(new Errors.NotFoundError('no token found')) + return callback null, token.data diff --git a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee index 1fbad4a294..dd87570450 100644 --- a/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserEmailsConfirmationHandler.coffee @@ -9,20 +9,14 @@ UserUpdater = require "./UserUpdater" ONE_YEAR_IN_S = 365 * 24 * 60 * 60 module.exports = UserEmailsConfirmationHandler = - serializeData: (user_id, email) -> - JSON.stringify({user_id, email}) - - deserializeData: (data) -> - JSON.parse(data) - sendConfirmationEmail: (user_id, email, emailTemplate, callback = (error) ->) -> if arguments.length == 3 callback = emailTemplate emailTemplate = 'confirmEmail' email = EmailHelper.parseEmail(email) return callback(new Error('invalid email')) if !email? - value = UserEmailsConfirmationHandler.serializeData(user_id, email) - OneTimeTokenHandler.getNewToken 'email_confirmation', value, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> + data = {user_id, email} + OneTimeTokenHandler.getNewToken 'email_confirmation', data, {expiresIn: ONE_YEAR_IN_S}, (err, token)-> return callback(err) if err? emailOptions = to: email @@ -35,7 +29,7 @@ module.exports = UserEmailsConfirmationHandler = return callback(error) if error? if !data? return callback(new Errors.NotFoundError('no token found')) - {user_id, email} = UserEmailsConfirmationHandler.deserializeData(data) + {user_id, email} = data logger.log {data, user_id, email, token_start: token.slice(0,8)}, 'found data for email confirmation' if !user_id? or email != EmailHelper.parseEmail(email) return callback(new Errors.NotFoundError('invalid data')) diff --git a/services/web/app/coffee/infrastructure/mongojs.coffee b/services/web/app/coffee/infrastructure/mongojs.coffee index f1ed213435..8492b9023c 100644 --- a/services/web/app/coffee/infrastructure/mongojs.coffee +++ b/services/web/app/coffee/infrastructure/mongojs.coffee @@ -1,6 +1,6 @@ Settings = require "settings-sharelatex" mongojs = require "mongojs" -db = mongojs(Settings.mongo.url, ["projects", "users", "userstubs"]) +db = mongojs(Settings.mongo.url, ["projects", "users", "userstubs", "tokens"]) module.exports = db: db ObjectId: mongojs.ObjectId diff --git a/services/web/test/acceptance/coffee/UserEmailsTests.coffee b/services/web/test/acceptance/coffee/UserEmailsTests.coffee index 8cdbea75fa..248c0331ae 100644 --- a/services/web/test/acceptance/coffee/UserEmailsTests.coffee +++ b/services/web/test/acceptance/coffee/UserEmailsTests.coffee @@ -3,7 +3,7 @@ async = require("async") User = require "./helpers/User" request = require "./helpers/request" settings = require "settings-sharelatex" -rclient = require("redis-sharelatex").createClient(settings.redis.web) +{db, ObjectId} = require("../../../app/js/infrastructure/mongojs") describe "UserEmails", -> beforeEach (done) -> @@ -32,10 +32,15 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.not.exist cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # There should only be one confirmation token at the moment - expect(keys.length).to.equal 1 - token = keys[0].split(':')[1] + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal 'newly-added-email@example.com' + expect(tokens[0].data.user_id).to.equal @user._id + token = tokens[0].token cb() (cb) => @user.request { @@ -54,9 +59,12 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.exist cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # Token should be deleted after use - expect(keys.length).to.equal 0 + expect(tokens.length).to.equal 0 cb() ], done @@ -75,11 +83,15 @@ describe "UserEmails", -> json: {@email} }, cb (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> - # There should only be one confirmation token at the moment, - # for the first user - expect(keys.length).to.equal 1 - token1 = keys[0].split(':')[1] + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => + # There should only be one confirmation token at the moment + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user._id + token1 = tokens[0].token cb() (cb) => # Delete the email from the first user @@ -107,14 +119,19 @@ describe "UserEmails", -> expect(response.statusCode).to.equal 404 cb() (cb) => - rclient.keys 'email_confirmation_token:*', (error, keys) -> + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => # The first token has been used, so this should be token2 now - expect(keys.length).to.equal 1 - token2 = keys[0].split(':')[1] + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user2._id + token2 = tokens[0].token cb() (cb) => # Second user should be able to confirm the email - @user2.request { + @user2.request { method: 'POST', url: '/user/emails/confirm', json: @@ -130,3 +147,48 @@ describe "UserEmails", -> expect(body[1].confirmedAt).to.exist cb() ], done + + describe "with an expired token", -> + it 'should not confirm the email', (done) -> + token = null + async.series [ + (cb) => + @user.request { + method: 'POST', + url: '/user/emails', + json: + email: @email = 'expired-token-email@example.com' + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 204 + cb() + (cb) => + db.tokens.find { + use: 'email_confirmation', + usedAt: { $exists: false } + }, (error, tokens) => + # There should only be one confirmation token at the moment + expect(tokens.length).to.equal 1 + expect(tokens[0].data.email).to.equal @email + expect(tokens[0].data.user_id).to.equal @user._id + token = tokens[0].token + cb() + (cb) => + db.tokens.update { + token: token + }, { + $set: { + expiresAt: new Date(Date.now() - 1000000) + } + }, cb + (cb) => + @user.request { + method: 'POST', + url: '/user/emails/confirm', + json: + token: token + }, (error, response, body) => + return done(error) if error? + expect(response.statusCode).to.equal 404 + cb() + ], done diff --git a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee index 21e8d9f288..c0e6d9cc13 100644 --- a/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee +++ b/services/web/test/unit/coffee/Security/OneTimeTokenHandlerTests.coffee @@ -5,64 +5,99 @@ path = require('path') sinon = require('sinon') modulePath = path.join __dirname, "../../../../app/js/Features/Security/OneTimeTokenHandler" expect = require("chai").expect +Errors = require "../../../../app/js/Features/Errors/Errors" +tk = require("timekeeper") describe "OneTimeTokenHandler", -> - beforeEach -> - @value = "user id here" - @stubbedToken = require("crypto").randomBytes(32) - - @settings = - redis: - web:{} - @redisMulti = - set:sinon.stub() - get:sinon.stub() - del:sinon.stub() - expire:sinon.stub() - exec:sinon.stub() - self = @ + tk.freeze Date.now() # freeze the time for these tests + @stubbedToken = "mock-token" + @callback = sinon.stub() @OneTimeTokenHandler = SandboxedModule.require modulePath, requires: - "../../infrastructure/RedisWrapper" : - client: => - auth:-> - multi: -> return self.redisMulti - "settings-sharelatex":@settings "logger-sharelatex": log:-> "crypto": randomBytes: () => @stubbedToken + "../Errors/Errors": Errors + "../../infrastructure/mongojs": db: @db = tokens: {} + afterEach -> + tk.reset() describe "getNewToken", -> + beforeEach -> + @db.tokens.insert = sinon.stub().yields() - it "should set a new token into redis with a ttl", (done)-> - @redisMulti.exec.callsArgWith(0) - @OneTimeTokenHandler.getNewToken 'password', @value, (err, token) => - @redisMulti.set.calledWith("password_token:#{@stubbedToken.toString("hex")}", @value).should.equal true - @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", 60 * 60).should.equal true - done() + describe 'normally', -> + beforeEach -> + @OneTimeTokenHandler.getNewToken 'password', 'mock-data-to-store', @callback - it "should return if there was an error", (done)-> - @redisMulti.exec.callsArgWith(0, "error") - @OneTimeTokenHandler.getNewToken 'password', @value, (err, token)=> - err.should.exist - done() + it "should insert a generated token with a 1 hour expiry", -> + @db.tokens.insert + .calledWith({ + use: 'password' + token: @stubbedToken, + createdAt: new Date(), + expiresAt: new Date(Date.now() + 60 * 60 * 1000) + data: 'mock-data-to-store' + }) + .should.equal true - it "should allow the expiry time to be overridden", (done) -> - @redisMulti.exec.callsArgWith(0) - @ttl = 42 - @OneTimeTokenHandler.getNewToken 'password', @value, {expiresIn: @ttl}, (err, token) => - @redisMulti.expire.calledWith("password_token:#{@stubbedToken.toString("hex")}", @ttl).should.equal true - done() + it 'should call the callback with the token', -> + @callback.calledWith(null, @stubbedToken).should.equal true + + describe 'with an optional expiresIn parameter', -> + beforeEach -> + @OneTimeTokenHandler.getNewToken 'password', 'mock-data-to-store', { expiresIn: 42 }, @callback + + it "should insert a generated token with a custom expiry", -> + @db.tokens.insert + .calledWith({ + use: 'password' + token: @stubbedToken, + createdAt: new Date(), + expiresAt: new Date(Date.now() + 42 * 1000) + data: 'mock-data-to-store' + }) + .should.equal true + + it 'should call the callback with the token', -> + @callback.calledWith(null, @stubbedToken).should.equal true describe "getValueFromTokenAndExpire", -> + describe 'successfully', -> + beforeEach -> + @db.tokens.findAndModify = sinon.stub().yields(null, { data: 'mock-data' }) + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', 'mock-token', @callback + + it 'should expire the token', -> + @db.tokens.findAndModify + .calledWith({ + query: { + use: 'password' + token: 'mock-token', + expiresAt: { $gt: new Date() }, + usedAt: { $exists: false } + }, + update: { + $set: { usedAt: new Date() } + } + }) + .should.equal true + + it 'should return the data', -> + @callback.calledWith(null, 'mock-data').should.equal true + + describe 'when a valid token is not found', -> + beforeEach -> + @db.tokens.findAndModify = sinon.stub().yields(null, null) + @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', 'mock-token', @callback + + it 'should return a NotFoundError', -> + @callback + .calledWith(sinon.match.instanceOf(Errors.NotFoundError)) + .should.equal true + + - it "should get and delete the token", (done)-> - @redisMulti.exec.callsArgWith(0, null, [@value]) - @OneTimeTokenHandler.getValueFromTokenAndExpire 'password', @stubbedToken, (err, value)=> - value.should.equal @value - @redisMulti.get.calledWith("password_token:#{@stubbedToken}").should.equal true - @redisMulti.del.calledWith("password_token:#{@stubbedToken}").should.equal true - done() diff --git a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee index 22cd70c4bc..95f72e0907 100644 --- a/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee +++ b/services/web/test/unit/coffee/User/UserEmailsConfirmationHandlerTests.coffee @@ -36,7 +36,7 @@ describe "UserEmailsConfirmationHandler", -> @OneTimeTokenHandler.getNewToken .calledWith( 'email_confirmation', - JSON.stringify({@user_id, @email}), + {@user_id, @email}, { expiresIn: 365 * 24 * 60 * 60 } ) .should.equal true @@ -72,7 +72,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@user_id, @email}) + {@user_id, @email} ) @UserUpdater.confirmEmail = sinon.stub().yields() @@ -105,7 +105,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@email}) + {@email} ) @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback @@ -116,7 +116,7 @@ describe "UserEmailsConfirmationHandler", -> beforeEach -> @OneTimeTokenHandler.getValueFromTokenAndExpire = sinon.stub().yields( null, - JSON.stringify({@user_id}) + {@user_id} ) @UserEmailsConfirmationHandler.confirmEmailFromToken @token = 'mock-token', @callback