diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index cd56f769ca..edea7cad1b 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -3,9 +3,11 @@ UserLocator = require("./UserLocator") User = require("../../models/User").User newsLetterManager = require('../Newsletter/NewsletterManager') sanitize = require('sanitizer') +UserRegistrationHandler = require("./UserRegistrationHandler") logger = require("logger-sharelatex") metrics = require("../../infrastructure/Metrics") - +Url = require("url") +AuthenticationController = require("../Authentication/AuthenticationController") module.exports = deleteUser: (req, res)-> @@ -45,4 +47,25 @@ module.exports = logger.err err: err, 'error destorying session' res.redirect '/login' + register : (req, res, next = (error) ->)-> + logger.log email: req.body.email, "attempted register" + redir = Url.parse(req.body.redir or "/project").path + UserRegistrationHandler.registerNewUser req.body, (err, user)-> + console.log err + if err == "EmailAlreadyRegisterd" + return AuthenticationController.login req, res + else if err? + next(err) + else + metrics.inc "user.register.success" + req.session.user = user + req.session.justRegistered = true + res.send + redir:redir + id:user._id.toString() + first_name: user.first_name + last_name: user.last_name + email: user.email + created: Date.now() + diff --git a/services/web/app/coffee/Features/User/UserCreator.coffee b/services/web/app/coffee/Features/User/UserCreator.coffee index 944597f78f..52e46fad12 100644 --- a/services/web/app/coffee/Features/User/UserCreator.coffee +++ b/services/web/app/coffee/Features/User/UserCreator.coffee @@ -15,5 +15,14 @@ module.exports = user = new User() user.email = opts.email user.holdingAccount = opts.holdingAccount + + username = opts.email.match(/^[^@]*/) + if username? + user.first_name = username[0] + else + user.first_name = "" + user.last_name = "" + + user.save (err)-> callback(err, user) diff --git a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee index c5983c6665..451f38fb10 100644 --- a/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee +++ b/services/web/app/coffee/Features/User/UserRegistrationHandler.coffee @@ -1,4 +1,11 @@ sanitize = require('sanitizer') +User = require("../../models/User").User +UserCreator = require("./UserCreator") +AuthenticationManager = require("../Authentication/AuthenticationManager") +NewsLetterManager = require("../NewsLetter/NewsLetterManager") +async = require("async") +EmailHandler = require("../Email/EmailHandler") +logger = require("logger-sharelatex") module.exports = validateEmail : (email) -> @@ -12,25 +19,50 @@ module.exports = hasZeroLength = true return hasZeroLength - validateRegisterRequest : (req, callback)-> - email = sanitize.escape(req.body.email).trim().toLowerCase() - password = req.body.password + _registrationRequestIsValid : (body, callback)-> + email = sanitize.escape(body.email).trim().toLowerCase() + password = body.password username = email.match(/^[^@]*/) - if username? - first_name = username[0] - else - first_name = "" - last_name = "" - if @hasZeroLengths([password, email]) - callback('please fill in all the fields', null) + return false else if !@validateEmail(email) - callback('not valid email', null) + return false else - callback(null, {first_name:first_name, last_name:last_name, email:email, password:password}) - + return true + _createNewUserIfRequired: (user, userDetails, callback)-> + if !user? + UserCreator.createNewUser {holdingAccount:false, email:userDetails.email}, callback + else + callback null, user registerNewUser: (userDetails, callback)-> + self = @ + requestIsValid = @_registrationRequestIsValid userDetails + if !requestIsValid + return callback("request is not valid") + userDetails.email = userDetails.email?.trim()?.toLowerCase() + User.findOne email:userDetails.email, (err, user)-> + if err? + return callback err + if user?.holdingAccount == false + return callback("EmailAlreadyRegisterd") + self._createNewUserIfRequired user, userDetails, (err, user)-> + if err? + return callback(err) + async.series [ + (cb)-> User.update {_id: user._id}, {"$set":{holdingAccount:false}}, cb + (cb)-> AuthenticationManager.setUserPassword user._id, userDetails.password, cb + (cb)-> NewsLetterManager.subscribe user, cb + (cb)-> + emailOpts = + first_name:user.first_name + to: user.email + EmailHandler.sendEmail "welcome", emailOpts, cb + ], (err)-> + logger.log user: user, "registered" + callback(err, user) + + + - callback() \ No newline at end of file diff --git a/services/web/app/coffee/controllers/UserController.coffee b/services/web/app/coffee/controllers/UserController.coffee index aa8b5d79d3..411c4b235d 100644 --- a/services/web/app/coffee/controllers/UserController.coffee +++ b/services/web/app/coffee/controllers/UserController.coffee @@ -19,53 +19,8 @@ uuid = require("node-uuid") module.exports = - apiRegister : (req, res, next = (error) ->)-> - logger.log email: req.body.email, "attempted register" - redir = Url.parse(req.body.redir or "/project").path - userRegistrationHandler.validateRegisterRequest req, (err, data)-> - if err? - logger.log validation_error: err, "user validation error" - metrics.inc "user.register.validation-error" - res.send message: - text:err - type:'error' - else - User.findOne {email:data.email}, (err, foundUser)-> - if foundUser? && foundUser.holdingAccount == false - AuthenticationController.login req, res - logger.log email: data.email, "email already registered" - metrics.inc "user.register.already-registered" - return AuthenticationController.login req, res - else if foundUser? && foundUser.holdingAccount == true #someone put them in as a collaberator - user = foundUser - user.holdingAccount = false - else - user = new User email: data.email - d = new Date() - user.first_name = data.first_name - user.last_name = data.last_name - user.signUpDate = new Date() - metrics.inc "user.register.success" - user.save (err)-> - req.session.user = user - req.session.justRegistered = true - logger.log user: user, "registered" - AuthenticationManager.setUserPassword user._id, data.password, (error) -> - return next(error) if error? - res.send - redir:redir - id:user._id.toString() - first_name: user.first_name - last_name: user.last_name - email: user.email - created: Date.now() - #things that can be fired and forgot. - newsLetterManager.subscribe user - ReferalAllocator.allocate req.session.referal_id, user._id, req.session.referal_source, req.session.referal_medium - emailOpts = - first_name:user.first_name - to: user.email - EmailHandler.sendEmail "welcome", emailOpts + + diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 02e25e1825..eeb3621029 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -67,7 +67,7 @@ module.exports = class Router app.get '/dropbox', InfoController.dropbox app.get '/register', UserPagesController.registerPage - app.post '/register', UserController.apiRegister + app.post '/register', UserController_new.register SubscriptionRouter.apply(app) UploadsRouter.apply(app) diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index 30e413a3b5..f7002d39da 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -8,6 +8,7 @@ events = require "events" MockResponse = require "../helpers/MockResponse" MockRequest = require "../helpers/MockRequest" ObjectId = require("mongojs").ObjectId +assert = require("assert") describe "UserController", -> beforeEach -> @@ -24,11 +25,17 @@ describe "UserController", -> findById: sinon.stub().callsArgWith(1, null, @user) @NewsLetterManager = unsubscribe: sinon.stub().callsArgWith(1) + @UserRegistrationHandler = + registerNewUser: sinon.stub() + @AuthenticationController = {} + @UserController = SandboxedModule.require modulePath, requires: "./UserLocator": @UserLocator "./UserDeleter": @UserDeleter "../../models/User": User:@User '../Newsletter/NewsletterManager':@NewsLetterManager + "./UserRegistrationHandler":@UserRegistrationHandler + "../Authentication/AuthenticationController": @AuthenticationController "logger-sharelatex": {log:->} @@ -97,3 +104,44 @@ describe "UserController", -> @UserController.logout @req, @res + describe "register", -> + + it "should ask the UserRegistrationHandler to register user", (done)-> + @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) + @res.send = => + @UserRegistrationHandler.registerNewUser.calledWith(@req.body).should.equal true + done() + @UserController.register @req, @res + + it "should try and log the user in if there is an EmailAlreadyRegisterd error", (done)-> + + @UserRegistrationHandler.registerNewUser.callsArgWith(1, message:"EmailAlreadyRegisterd") + @AuthenticationController.login = (req, res)=> + assert.deepEqual req, @req + assert.deepEqual res, @res + done() + @UserController.register @req, @res + + it "should put the user on the session and mark them as justRegistered", (done)-> + @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) + @res.send = => + assert.deepEqual @user, @req.session.user + assert.equal @req.session.justRegistered, true + done() + @UserController.register @req, @res + + it "should redirect to project page", (done)-> + @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) + @res.send = (opts)=> + opts.redir.should.equal "/project" + done() + @UserController.register @req, @res + + + it "should redirect passed redir if it exists", (done)-> + @UserRegistrationHandler.registerNewUser.callsArgWith(1, null, @user) + @req.body.redir = "/somewhere" + @res.send = (opts)=> + opts.redir.should.equal "/somewhere" + done() + @UserController.register @req, @res diff --git a/services/web/test/UnitTests/coffee/User/UserCreatorTests.coffee b/services/web/test/UnitTests/coffee/User/UserCreatorTests.coffee index 5e2dd8bb6b..5088075656 100644 --- a/services/web/test/UnitTests/coffee/User/UserCreatorTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserCreatorTests.coffee @@ -50,7 +50,7 @@ describe "UserCreator", -> @UserCreator.createNewUser opts, (err, user)=> assert.equal user.email, @email assert.equal user.holdingAccount, true - + assert.equal user.first_name, "bob.oswald" done() diff --git a/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee b/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee index 74dc9ba960..68b67f887f 100644 --- a/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserRegistrationHandlerTests.coffee @@ -3,68 +3,135 @@ SandboxedModule = require('sandboxed-module') assert = require('assert') path = require('path') modulePath = path.join __dirname, '../../../../app/js/Features/User/UserRegistrationHandler' +sinon = require("sinon") +expect = require("chai").expect describe "UserRegistrationHandler", -> - + beforeEach -> - @handler = SandboxedModule.require modulePath, requires:{} - @passingRequest = {body:{email:"something@email.com", password:"123"}} + @user = + _id: "31j2lk21kjl" + @User = + findOne:sinon.stub() + update: sinon.stub().callsArgWith(2) + @UserCreator = + createNewUser:sinon.stub().callsArgWith(1, null, @user) + @AuthenticationManager = + setUserPassword: sinon.stub().callsArgWith(2) + @NewsLetterManager = + subscribe: sinon.stub().callsArgWith(1) + @EmailHandler = + sendEmail:sinon.stub().callsArgWith(2) + @handler = SandboxedModule.require modulePath, requires: + "../../models/User": {User:@User} + "./UserCreator": @UserCreator + "../Authentication/AuthenticationManager":@AuthenticationManager + "../NewsLetter/NewsLetterManager":@NewsLetterManager + "../Email/EmailHandler": @EmailHandler + + @passingRequest = {email:"something@email.com", password:"123"} describe 'validate Register Request', -> it 'allow working account through', -> - @handler.validateRegisterRequest @passingRequest, (err, data)=> - should.not.exist(err) - data.first_name.should.equal "something" - data.last_name.should.equal "" - data.email.should.equal @passingRequest.body.email - data.password.should.equal @passingRequest.body.password - - it 'not allow not valid email through ', (done)-> - @passingRequest.body.email = "notemail" - @handler.validateRegisterRequest @passingRequest, (err, data)-> - should.exist(err) - err.should.equal "not valid email" - done() + result = @handler._registrationRequestIsValid @passingRequest + result.should.equal true + it 'not allow not valid email through ', ()-> + @passingRequest.email = "notemail" + result = @handler._registrationRequestIsValid @passingRequest + result.should.equal false + it 'not allow no email through ', -> - @passingRequest.body.email = "" - @handler.validateRegisterRequest @passingRequest, (err, data)-> - should.exist(err) - err.should.equal "please fill in all the fields" + @passingRequest.email = "" + result = @handler._registrationRequestIsValid @passingRequest + result.should.equal false - it 'not allow no password through ', (done)-> - @passingRequest.body.password= "" - @handler.validateRegisterRequest @passingRequest, (err, data)-> - should.exist(err) - err.should.equal "please fill in all the fields" - done() - - it 'trim white space from email', (done)-> - @passingRequest.body.email = " some@email.com " - @handler.validateRegisterRequest @passingRequest, (err, data)-> - should.not.exist(err) - data.email.should.equal "some@email.com" - done() - - it 'lower case email', (done)-> - @passingRequest.body.email = "soMe@eMail.cOm" - @handler.validateRegisterRequest @passingRequest, (err, data)-> - should.not.exist(err) - data.email.should.equal "some@email.com" - done() - - it 'should allow a short registeration request through', (done) -> - @handler.validateRegisterRequest body: { - email: "user_name@example.com" - password: @passingRequest.body.password - }, (err, data) => - should.not.exist(err) - data.email.should.equal "user_name@example.com" - data.password.should.equal @passingRequest.body.password - data.first_name.should.equal "user_name" - done() + it 'not allow no password through ', ()-> + @passingRequest.password= "" + result = @handler._registrationRequestIsValid @passingRequest + result.should.equal false + + + + describe "registerNewUser", -> + + describe "holdingAccount", (done)-> + + beforeEach -> + @user.holdingAccount = true + @handler._registrationRequestIsValid = sinon.stub().returns true + @User.findOne.callsArgWith(1, null, @user) + + it "should not create a new user if there is a holding account there", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + @UserCreator.createNewUser.called.should.equal false + done() + + it "should set holding account to false", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + update = @User.update.args[0] + assert.deepEqual update[0], {_id:@user._id} + assert.deepEqual update[1], {"$set":{holdingAccount:false}} + done() + + describe "invalidRequest", -> + + it "should not create a new user if the the request is not valid", (done)-> + @handler._registrationRequestIsValid = sinon.stub().returns false + @handler.registerNewUser @passingRequest, (err)=> + expect(err).to.exist + @UserCreator.createNewUser.called.should.equal false + done() + + it "should return email registered in the error if there is a non holdingAccount there", (done)-> + @User.findOne.callsArgWith(1, null, {holdingAccount:false}) + @handler.registerNewUser @passingRequest, (err)=> + err.should.equal "EmailAlreadyRegisterd" + done() + + describe "validRequest", -> + beforeEach -> + @handler._registrationRequestIsValid = sinon.stub().returns true + @User.findOne.callsArgWith 1 + + it "should create a new user", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + @UserCreator.createNewUser.calledWith({email:@passingRequest.email, holdingAccount:false}).should.equal true + done() + + it 'lower case email', (done)-> + @passingRequest.email = "soMe@eMail.cOm" + @handler.registerNewUser @passingRequest, (err)=> + @UserCreator.createNewUser.calledWith({email:@passingRequest.email.toLowerCase(), holdingAccount:false}).should.equal true + done() + + it 'trim white space from email', (done)-> + @passingRequest.email = " some@email.com " + @handler.registerNewUser @passingRequest, (err)=> + @UserCreator.createNewUser.calledWith({email:"some@email.com", holdingAccount:false}).should.equal true + done() + + + it "should set the password", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + @AuthenticationManager.setUserPassword.calledWith(@user._id, @passingRequest.password).should.equal true + done() + + it "should add the user to the news letter manager", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + @NewsLetterManager.subscribe.calledWith(@user).should.equal true + done() + + it "should send a welcome email", (done)-> + @handler.registerNewUser @passingRequest, (err)=> + @EmailHandler.sendEmail.calledWith("welcome").should.equal true + done() + + + it "should call the ReferalAllocator", (done)-> + done()