From e8f21986dd42c1920c6f8e52ee8d9f89c7a8364f Mon Sep 17 00:00:00 2001 From: James Allen Date: Fri, 11 Dec 2015 17:11:13 +0000 Subject: [PATCH] Refactor registration so it can be called from modules --- .../Features/User/UserController.coffee | 35 ++-------- .../User/UserRegistrationHandler.coffee | 31 ++++++++- .../AuthenticationManagerTests.coffee | 2 +- .../coffee/User/UserControllerTests.coffee | 67 +++++-------------- .../User/UserRegistrationHandlerTests.coffee | 56 +++++++++++++++- 5 files changed, 108 insertions(+), 83 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 516b8a9546..f342e74351 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -8,10 +8,7 @@ metrics = require("../../infrastructure/Metrics") Url = require("url") AuthenticationManager = require("../Authentication/AuthenticationManager") UserUpdater = require("./UserUpdater") -EmailHandler = require("../Email/EmailHandler") -OneTimeTokenHandler = require "../Security/OneTimeTokenHandler" settings = require "settings-sharelatex" -crypto = require "crypto" module.exports = @@ -85,32 +82,12 @@ module.exports = if !email? or email == "" res.sendStatus 422 # Unprocessable Entity return - logger.log {email}, "registering new user" - UserRegistrationHandler.registerNewUser { - email: email - password: crypto.randomBytes(32).toString("hex") - }, (err, user)-> - if err? and err?.message != "EmailAlreadyRegistered" - return next(err) - - if err?.message == "EmailAlreadyRegistered" - 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)-> - return next(err) if err? - - setNewPasswordUrl = "#{settings.siteUrl}/user/activate?token=#{token}&user_id=#{user._id}" - - EmailHandler.sendEmail "registered", { - to: user.email - setNewPasswordUrl: setNewPasswordUrl - }, () -> - - res.json { - email: user.email - setNewPasswordUrl: setNewPasswordUrl - } + UserRegistrationHandler.registerNewUserAndSendActivationEmail email, (error, user, setNewPasswordUrl) -> + return next(error) if error? + res.json { + email: user.email + setNewPasswordUrl: setNewPasswordUrl + } changePassword : (req, res, next = (error) ->)-> metrics.inc "user.password-change" diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index 7dfde21c8a..7465268023 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -5,8 +5,12 @@ AuthenticationManager = require("../Authentication/AuthenticationManager") NewsLetterManager = require("../Newsletter/NewsletterManager") async = require("async") logger = require("logger-sharelatex") +crypto = require("crypto") +EmailHandler = require("../Email/EmailHandler") +OneTimeTokenHandler = require "../Security/OneTimeTokenHandler" +settings = require "settings-sharelatex" -module.exports = +module.exports = UserRegistrationHandler = validateEmail : (email) -> re = /^(([^<>()[\]\\.,;:\s@\"]+(\.[^<>()[\]\\.,;:\s@\"]+)*)|(\ ".+\"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\])|(([a-zA -Z\-0-9]+\.)+[a-zA-Z]{2,}))$/ return re.test(email) @@ -58,6 +62,31 @@ module.exports = ], (err)-> logger.log user: user, "registered" callback(err, user) + + registerNewUserAndSendActivationEmail: (email, callback = (error, user, setNewPasswordUrl) ->) -> + logger.log {email}, "registering new user" + UserRegistrationHandler.registerNewUser { + email: email + password: crypto.randomBytes(32).toString("hex") + }, (err, user)-> + if err? and err?.message != "EmailAlreadyRegistered" + return next(err) + + if err?.message == "EmailAlreadyRegistered" + 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)-> + return next(err) if err? + + setNewPasswordUrl = "#{settings.siteUrl}/user/activate?token=#{token}&user_id=#{user._id}" + + EmailHandler.sendEmail "registered", { + to: user.email + setNewPasswordUrl: setNewPasswordUrl + }, () -> + + callback null, user, setNewPasswordUrl diff --git a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee index 3360099cef..3a5c60cf66 100644 --- a/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee +++ b/services/web/test/UnitTests/coffee/Authentication/AuthenticationManagerTests.coffee @@ -71,7 +71,7 @@ describe "AuthenticationManager", -> @bcrypt.genSalt = sinon.stub().callsArgWith(1, null, @salt) @bcrypt.hash = sinon.stub().callsArgWith(2, null, @hashedPassword) @db.users.update = sinon.stub().callsArg(2) - @AuthenticationManager.setUserPassword(@user_id, @unencryptedPassword, @callback) + @AuthenticationManager.setUserPassword(@user_id, @password, @callback) it "should update the user's password in the database", -> @db.users.update diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index a96e8ba209..5010f91299 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -40,10 +40,6 @@ describe "UserController", -> autoAllocate:sinon.stub() @UserUpdater = changeEmailAddress:sinon.stub() - @EmailHandler = - sendEmail:sinon.stub().callsArgWith(2) - @OneTimeTokenHandler = - getNewToken: sinon.stub() @settings = siteUrl: "sharelatex.example.com" @UserController = SandboxedModule.require modulePath, requires: @@ -57,9 +53,6 @@ describe "UserController", -> "../Authentication/AuthenticationManager": @AuthenticationManager "../Referal/ReferalAllocator":@ReferalAllocator "../Subscription/SubscriptionDomainHandler":@SubscriptionDomainHandler - "../Email/EmailHandler": @EmailHandler - "../Security/OneTimeTokenHandler": @OneTimeTokenHandler - "crypto": @crypto = {} "settings-sharelatex": @settings "logger-sharelatex": {log:->} @@ -175,51 +168,23 @@ describe "UserController", -> describe "register", -> beforeEach -> - @req.body.email = @user.email = "email@example.com" - @crypto.randomBytes = sinon.stub().returns({toString: () => @password = "mock-password"}) - @OneTimeTokenHandler.getNewToken.callsArgWith(2, null, @token = "mock-token") + @UserRegistrationHandler.registerNewUserAndSendActivationEmail = sinon.stub().callsArgWith(1, null, @user, @url = "mock/url") + @req.body.email = @user.email = @email = "email@example.com" + @UserController.register @req, @res - describe "with a new user", -> - beforeEach -> - @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) - @UserController.register @req, @res - - it "should ask the UserRegistrationHandler to register user", -> - @UserRegistrationHandler.registerNewUser - .calledWith({ - email: @req.body.email - password: @password - }).should.equal true - - it "should generate a new password reset token", -> - @OneTimeTokenHandler.getNewToken - .calledWith(@user_id, expiresIn: 7 * 24 * 60 * 60) - .should.equal true - - it "should send a registered email", -> - @EmailHandler.sendEmail - .calledWith("registered", { - to: @user.email - setNewPasswordUrl: "#{@settings.siteUrl}/user/activate?token=#{@token}&user_id=#{@user_id}" - }) - .should.equal true - - it "should return the user", -> - @res.json - .calledWith({ - email: @user.email - setNewPasswordUrl: "#{@settings.siteUrl}/user/activate?token=#{@token}&user_id=#{@user_id}" - }) - .should.equal true - - describe "with a user that already exists", -> - beforeEach -> - @UserRegistrationHandler.registerNewUser.callsArgWith(1, new Error("EmailAlreadyRegistered"), @user) - @UserController.register @req, @res - - it "should still generate a new password token and email", -> - @OneTimeTokenHandler.getNewToken.called.should.equal true - @EmailHandler.sendEmail.called.should.equal true + it "should register the user and send them an email", -> + @UserRegistrationHandler.registerNewUserAndSendActivationEmail + .calledWith(@email) + .should.equal true + + it "should return the user and activation url", -> + console.log @res.json.args + @res.json + .calledWith({ + email: @email, + setNewPasswordUrl: @url + }) + .should.equal true describe "changePassword", -> diff --git a/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee index 72beb10ade..60ae1719a7 100644 --- a/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee @@ -10,7 +10,7 @@ describe "UserRegistrationHandler", -> beforeEach -> @user = - _id: "31j2lk21kjl" + _id: @user_id = "31j2lk21kjl" @User = findOne:sinon.stub() update: sinon.stub().callsArgWith(2) @@ -20,12 +20,20 @@ describe "UserRegistrationHandler", -> setUserPassword: sinon.stub().callsArgWith(2) @NewsLetterManager = subscribe: sinon.stub().callsArgWith(1) + @EmailHandler = + sendEmail:sinon.stub().callsArgWith(2) + @OneTimeTokenHandler = + getNewToken: sinon.stub() @handler = SandboxedModule.require modulePath, requires: "../../models/User": {User:@User} "./UserCreator": @UserCreator "../Authentication/AuthenticationManager":@AuthenticationManager "../Newsletter/NewsletterManager":@NewsLetterManager "logger-sharelatex": @logger = { log: sinon.stub() } + "crypto": @crypto = {} + "../Email/EmailHandler": @EmailHandler + "../Security/OneTimeTokenHandler": @OneTimeTokenHandler + "settings-sharelatex": @settings = {siteUrl: "http://sl.example.com"} @passingRequest = {email:"something@email.com", password:"123"} @@ -128,4 +136,50 @@ describe "UserRegistrationHandler", -> it "should call the ReferalAllocator", (done)-> done() + describe "registerNewUserAndSendActivationEmail", -> + beforeEach -> + @email = "email@example.com" + @crypto.randomBytes = sinon.stub().returns({toString: () => @password = "mock-password"}) + @OneTimeTokenHandler.getNewToken.callsArgWith(2, null, @token = "mock-token") + @handler.registerNewUser = sinon.stub() + @callback = sinon.stub() + + describe "with a new user", -> + beforeEach -> + @handler.registerNewUser.callsArgWith(1, null, @user) + @handler.registerNewUserAndSendActivationEmail @email, @callback + + it "should ask the UserRegistrationHandler to register user", -> + @handler.registerNewUser + .calledWith({ + email: @email + password: @password + }).should.equal true + + it "should generate a new password reset token", -> + + @OneTimeTokenHandler.getNewToken + .calledWith(@user_id, expiresIn: 7 * 24 * 60 * 60) + .should.equal true + it "should send a registered email", -> + @EmailHandler.sendEmail + .calledWith("registered", { + to: @user.email + setNewPasswordUrl: "#{@settings.siteUrl}/user/activate?token=#{@token}&user_id=#{@user_id}" + }) + .should.equal true + + it "should return the user", -> + @callback + .calledWith(null, @user, "#{@settings.siteUrl}/user/activate?token=#{@token}&user_id=#{@user_id}") + .should.equal true + + describe "with a user that already exists", -> + beforeEach -> + @handler.registerNewUser.callsArgWith(1, new Error("EmailAlreadyRegistered"), @user) + @handler.registerNewUserAndSendActivationEmail @email, @callback + + it "should still generate a new password token and email", -> + @OneTimeTokenHandler.getNewToken.called.should.equal true + @EmailHandler.sendEmail.called.should.equal true \ No newline at end of file