From f455a11aa82b44a495552895331acba64d932a50 Mon Sep 17 00:00:00 2001 From: Jessica Lawshe Date: Wed, 17 Apr 2019 09:00:13 -0500 Subject: [PATCH] Merge pull request #1655 from sharelatex/jel-user-must-reconfirm Reconfirm user accounts GitOrigin-RevId: 0343ff745e881cd51b5efbfb97404b6b926905c8 --- .../AuthenticationController.coffee | 37 +++++++++++----- .../PasswordResetController.coffee | 27 ++++++------ .../PasswordReset/PasswordResetRouter.coffee | 1 + .../Features/User/UserPagesController.coffee | 7 +++ .../coffee/Features/User/UserUpdater.coffee | 7 +++ services/web/app/coffee/models/User.coffee | 1 + services/web/app/coffee/router.coffee | 3 ++ services/web/app/views/user/passwordReset.pug | 4 +- services/web/app/views/user/reconfirm.pug | 44 +++++++++++++++++++ .../coffee/UserReconfirmTests.coffee | 34 ++++++++++++++ .../acceptance/coffee/helpers/User.coffee | 10 +++++ .../AuthenticationControllerTests.coffee | 9 ++++ .../PasswordResetControllerTests.coffee | 19 ++++++-- .../unit/coffee/User/UserUpdaterTests.coffee | 2 +- 14 files changed, 172 insertions(+), 33 deletions(-) create mode 100644 services/web/app/views/user/reconfirm.pug create mode 100644 services/web/test/acceptance/coffee/UserReconfirmTests.coffee diff --git a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee index 859ae2b099..5bd8d256e6 100644 --- a/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee +++ b/services/web/app/coffee/Features/Authentication/AuthenticationController.coffee @@ -30,6 +30,7 @@ module.exports = AuthenticationController = referal_id: user.referal_id session_created: (new Date()).toISOString() ip_address: user._login_req_ip + must_reconfirm: user.must_reconfirm callback(null, lightUser) deserializeUser: (user, cb) -> @@ -77,19 +78,22 @@ module.exports = AuthenticationController = finishLogin: (user, req, res, next) -> return res.redirect('/login') if user == false # OAuth2 'state' mismatch - redir = AuthenticationController._getRedirectFromSession(req) || "/project" - AuthenticationController._loginAsyncHandlers(req, user) - AuthenticationController.afterLoginSessionSetup req, user, (err) -> - if err? - return next(err) - SudoModeHandler.activateSudoMode user._id, (err) -> + if user.must_reconfirm + AuthenticationController._redirectToReconfirmPage req, res, user + else + redir = AuthenticationController._getRedirectFromSession(req) || "/project" + AuthenticationController._loginAsyncHandlers(req, user) + AuthenticationController.afterLoginSessionSetup req, user, (err) -> if err? - logger.err {err, user_id: user._id}, "Error activating Sudo Mode on login, continuing" - AuthenticationController._clearRedirectFromSession(req) - if req.headers?['accept']?.match(/^application\/json.*$/) - res.json {redir: redir} - else - res.redirect(redir) + return next(err) + SudoModeHandler.activateSudoMode user._id, (err) -> + if err? + logger.err {err, user_id: user._id}, "Error activating Sudo Mode on login, continuing" + AuthenticationController._clearRedirectFromSession(req) + if req.headers?['accept']?.match(/^application\/json.*$/) + res.json {redir: redir} + else + res.redirect(redir) doPassportLogin: (req, username, password, done) -> email = username.toLowerCase() @@ -242,6 +246,15 @@ module.exports = AuthenticationController = res.redirect url Metrics.inc "security.login-redirect" + _redirectToReconfirmPage: (req, res, user) -> + logger.log url: req.url, "user needs to reconfirm so redirecting to reconfirm page" + req.session.reconfirm_email = user?.email + redir = "/user/reconfirm" + if req.headers?['accept']?.match(/^application\/json.*$/) + res.json {redir: redir} + else + res.redirect redir + _redirectToRegisterPage: (req, res) -> logger.log url: req.url, "user not logged in so redirecting to register page" AuthenticationController.setRedirectInSession(req) diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee index 6cadf03853..926cd1dfb6 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetController.coffee @@ -3,6 +3,7 @@ RateLimiter = require("../../infrastructure/RateLimiter") AuthenticationController = require("../Authentication/AuthenticationController") AuthenticationManager = require("../Authentication/AuthenticationManager") UserGetter = require("../User/UserGetter") +UserUpdater = require("../User/UserUpdater") UserSessionsManager = require("../User/UserSessionsManager") logger = require "logger-sharelatex" @@ -26,10 +27,8 @@ module.exports = PasswordResetHandler.generateAndEmailResetToken email, (err, exists)-> if err? res.send 500, {message:err?.message} - else if exists - res.sendStatus 200 else - res.send 404, {message: req.i18n.translate("cant_find_email")} + res.send 200, {message: {text: req.i18n.translate("if_registered_email_sent")}} renderSetPasswordForm: (req, res)-> if req.query.passwordResetToken? @@ -61,15 +60,17 @@ module.exports = return res.sendStatus 200 if !user_id? # will not exist for v1-only users UserSessionsManager.revokeAllUserSessions {_id: user_id}, [], (err) -> return next(err) if err? - if req.body.login_after - UserGetter.getUser user_id, {email: 1}, (err, user) -> - return next(err) if err? - AuthenticationController.afterLoginSessionSetup req, user, (err) -> - if err? - logger.err {err, email: user.email}, "Error setting up session after setting password" - return next(err) - res.json {redir: AuthenticationController._getRedirectFromSession(req) || "/project"} - else - res.sendStatus 200 + UserUpdater.removeReconfirmFlag user_id, (err) -> + return next(err) if err? + if req.body.login_after + UserGetter.getUser user_id, {email: 1}, (err, user) -> + return next(err) if err? + AuthenticationController.afterLoginSessionSetup req, user, (err) -> + if err? + logger.err {err, email: user.email}, "Error setting up session after setting password" + return next(err) + res.json {redir: AuthenticationController._getRedirectFromSession(req) || "/project"} + else + res.sendStatus 200 else res.sendStatus 404 diff --git a/services/web/app/coffee/Features/PasswordReset/PasswordResetRouter.coffee b/services/web/app/coffee/Features/PasswordReset/PasswordResetRouter.coffee index e049d43075..d3591f8623 100644 --- a/services/web/app/coffee/Features/PasswordReset/PasswordResetRouter.coffee +++ b/services/web/app/coffee/Features/PasswordReset/PasswordResetRouter.coffee @@ -12,3 +12,4 @@ module.exports = webRouter.post '/user/password/set', PasswordResetController.setNewUserPassword AuthenticationController.addEndpointToLoginWhitelist '/user/password/set' + webRouter.post '/user/reconfirm', PasswordResetController.requestReset \ No newline at end of file diff --git a/services/web/app/coffee/Features/User/UserPagesController.coffee b/services/web/app/coffee/Features/User/UserPagesController.coffee index 7670ced4d8..c228831687 100644 --- a/services/web/app/coffee/Features/User/UserPagesController.coffee +++ b/services/web/app/coffee/Features/User/UserPagesController.coffee @@ -59,6 +59,13 @@ module.exports = logoutPage: (req, res) -> res.render 'user/logout' + renderReconfirmAccountPage: (req, res) -> + page_data = { + reconfirm_email: req?.session?.reconfirm_email + } + # when a user must reconfirm their account + res.render 'user/reconfirm', page_data + settingsPage : (req, res, next)-> user_id = AuthenticationController.getLoggedInUserId(req) logger.log user: user_id, "loading settings page" diff --git a/services/web/app/coffee/Features/User/UserUpdater.coffee b/services/web/app/coffee/Features/User/UserUpdater.coffee index b640aae3c3..05e37b9870 100644 --- a/services/web/app/coffee/Features/User/UserUpdater.coffee +++ b/services/web/app/coffee/Features/User/UserUpdater.coffee @@ -186,11 +186,18 @@ module.exports = UserUpdater = return callback(new Errors.NotFoundError('user id and email do no match')) FeaturesUpdater.refreshFeatures userId, true, callback + removeReconfirmFlag: (user_id, callback) -> + UserUpdater.updateUser user_id.toString(), { + $set: { "must_reconfirm": false } + }, (error) -> + callback(error) + [ 'updateUser' 'changeEmailAddress' 'setDefaultEmailAddress' 'addEmailAddress' 'removeEmailAddress' + 'removeReconfirmFlag' ].map (method) -> metrics.timeAsyncMethod(UserUpdater, method, 'mongo.UserUpdater', logger) diff --git a/services/web/app/coffee/models/User.coffee b/services/web/app/coffee/models/User.coffee index a8717ac3ac..891de71f96 100644 --- a/services/web/app/coffee/models/User.coffee +++ b/services/web/app/coffee/models/User.coffee @@ -64,6 +64,7 @@ UserSchema = new Schema zotero: { type:Boolean, default: Settings.defaultFeatures.zotero } referencesSearch: { type:Boolean, default: Settings.defaultFeatures.referencesSearch } } + must_reconfirm:{ type:Boolean, default: false } referal_id : {type:String, default:() -> uuid.v4().split("-")[0]} refered_users: [ type:ObjectId, ref:'User' ] refered_user_count: { type:Number, default: 0 } diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 5a98beb284..257f0193de 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -185,6 +185,9 @@ module.exports = class Router webRouter.get '/user/personal_info', AuthenticationController.requireLogin(), UserInfoController.getLoggedInUsersPersonalInfo privateApiRouter.get '/user/:user_id/personal_info', AuthenticationController.httpAuth, UserInfoController.getPersonalInfo + webRouter.get '/user/reconfirm', UserPagesController.renderReconfirmAccountPage + # for /user/reconfirm POST, see password router + webRouter.get '/user/projects', AuthenticationController.requireLogin(), ProjectController.userProjectsJson webRouter.get '/project/:Project_id/entities', AuthenticationController.requireLogin(), AuthorizationMiddleware.ensureUserCanReadProject, diff --git a/services/web/app/views/user/passwordReset.pug b/services/web/app/views/user/passwordReset.pug index 7bb9ede1ab..c59cecc004 100644 --- a/services/web/app/views/user/passwordReset.pug +++ b/services/web/app/views/user/passwordReset.pug @@ -20,9 +20,7 @@ block content ng-cloak ) input(type="hidden", name="_csrf", value=csrfToken) - form-messages(for="passwordResetForm") - .alert.alert-success(ng-show="passwordResetForm.response.success") - | #{translate("password_reset_email_sent")} + form-messages(for="passwordResetForm" role="alert") .form-group label(for='email') #{translate("please_enter_email")} input.form-control( diff --git a/services/web/app/views/user/reconfirm.pug b/services/web/app/views/user/reconfirm.pug new file mode 100644 index 0000000000..c5b25990ad --- /dev/null +++ b/services/web/app/views/user/reconfirm.pug @@ -0,0 +1,44 @@ +extends ../layout + +block content + - var email = reconfirm_email ? reconfirm_email : "" + .content.content-alt + .container + .row + .col-sm-12.col-md-6.col-md-offset-3 + .card + h1.card-header.text-capitalize #{translate("reconfirm")} #{translate("Account")} + p #{translate('reconfirm_explained')}  + a(href=`mailto:${settings.adminEmail}`, ng-non-bindable) #{settings.adminEmail} + | . + form( + async-form="reconfirm-account-request", + name="reconfirmAccountForm" + action="/user/reconfirm", + method="POST", + ng-cloak + ng-init="email='"+email+"'" + aria-label=translate('request_reconfirmation_email') + ) + input(type="hidden", name="_csrf", value=csrfToken) + form-messages(for="reconfirmAccountForm" role="alert") + .form-group + label(for='email') #{translate("please_enter_email")} + input.form-control( + aria-label="email" + type='email', + name='email', + placeholder='email@example.com', + required, + ng-model="email", + autofocus + ) + span.small.text-primary( + ng-show="reconfirmAccountForm.email.$invalid && reconfirmAccountForm.email.$dirty" + ) #{translate("must_be_email_address")} + .actions + button.btn.btn-primary( + type='submit', + ng-disabled="reconfirmAccountForm.$invalid" + aria-label=translate('request_reconfirmation_email') + ) #{translate('request_reconfirmation_email')} diff --git a/services/web/test/acceptance/coffee/UserReconfirmTests.coffee b/services/web/test/acceptance/coffee/UserReconfirmTests.coffee new file mode 100644 index 0000000000..3a9fb9ddbb --- /dev/null +++ b/services/web/test/acceptance/coffee/UserReconfirmTests.coffee @@ -0,0 +1,34 @@ +expect = require("chai").expect +should = require('chai').should() +async = require("async") +User = require "./helpers/User" + +describe 'User Must Reconfirm', -> + + before (done) -> + @user = new User() + async.series [ + @user.ensureUserExists.bind(@user) + (cb) => @user.mongoUpdate {$set: {'must_reconfirm': true}}, cb + ], done + + it 'should not allow sign in', (done) -> + @user.login (err) => + expect(err?).to.equal false + @user.isLoggedIn (err, isLoggedIn) -> + expect(isLoggedIn).to.equal false + done() + + describe 'Requesting reconfirmation email', -> + it 'should return a success to client for existing account', (done) -> + @user.reconfirmAccountRequest @user.email, (err, response) => + expect(err?).to.equal false + expect(response.statusCode).to.equal 200 + done() + + it 'should return a success to client for non-existent account', (done) -> + # we return success so that we do not leak account info + @user.reconfirmAccountRequest 'fake@overleaf.com', (err, response) => + expect(err?).to.equal false + expect(response.statusCode).to.equal 200 + done() \ No newline at end of file diff --git a/services/web/test/acceptance/coffee/helpers/User.coffee b/services/web/test/acceptance/coffee/helpers/User.coffee index 61ff3d3cb3..738d6d206c 100644 --- a/services/web/test/acceptance/coffee/helpers/User.coffee +++ b/services/web/test/acceptance/coffee/helpers/User.coffee @@ -273,6 +273,16 @@ class User return callback(error) if error? callback() + reconfirmAccountRequest: (user_email, callback = (error) ->) -> + @getCsrfToken (error) => + return callback(error) if error? + @request.post { + url: "/user/reconfirm" + json: + email: user_email + }, (error, response, body) => + callback(error, response) + getUserSettingsPage: (callback = (error, statusCode) ->) -> @getCsrfToken (error) => return callback(error) if error? diff --git a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee index 434c80de61..7f17d9d0a4 100644 --- a/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee +++ b/services/web/test/unit/coffee/Authentication/AuthenticationControllerTests.coffee @@ -734,6 +734,7 @@ describe "AuthenticationController", -> @AuthenticationController._loginAsyncHandlers = sinon.stub() @AuthenticationController.afterLoginSessionSetup = sinon.stub().callsArgWith(2, null) @AuthenticationController._clearRedirectFromSession = sinon.stub() + @AuthenticationController._redirectToReconfirmPage = sinon.stub() @req.headers = {accept: 'application/json, whatever'} @res.json = sinon.stub() @res.redirect = sinon.stub() @@ -775,3 +776,11 @@ describe "AuthenticationController", -> expect(@res.json.callCount).to.equal 0 expect(@res.redirect.callCount).to.equal 1 expect(@res.redirect.calledWith('/some/page')).to.equal true + + describe "when user is flagged to reconfirm", -> + beforeEach -> + @req.session = {} + @user.must_reconfirm = true + it "should redirect to reconfirm page", () -> + @AuthenticationController.finishLogin(@user, @req, @res, @next) + expect(@AuthenticationController._redirectToReconfirmPage.calledWith(@req)).to.equal true \ No newline at end of file diff --git a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee index 0af316a349..eb30916b80 100644 --- a/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee +++ b/services/web/test/unit/coffee/PasswordReset/PasswordResetControllerTests.coffee @@ -21,6 +21,8 @@ describe "PasswordResetController", -> revokeAllUserSessions: sinon.stub().callsArgWith(2, null) @AuthenticationManager = validatePassword: sinon.stub() + @UserUpdater = + removeReconfirmFlag: sinon.stub().callsArgWith(1, null) @PasswordResetController = SandboxedModule.require modulePath, requires: "settings-sharelatex":@settings "./PasswordResetHandler":@PasswordResetHandler @@ -30,6 +32,7 @@ describe "PasswordResetController", -> "../Authentication/AuthenticationManager": @AuthenticationManager "../User/UserGetter": @UserGetter = {} "../User/UserSessionsManager": @UserSessionsManager + "../User/UserUpdater": @UserUpdater @email = "bob@bob.com " @user_id = 'mock-user-id' @@ -63,7 +66,7 @@ describe "PasswordResetController", -> it "should tell the handler to process that email", (done)-> @RateLimiter.addCount.callsArgWith(1, null, true) @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, true) - @res.sendStatus = (code)=> + @res.send = (code)=> code.should.equal 200 @PasswordResetHandler.generateAndEmailResetToken.calledWith(@email.trim()).should.equal true done() @@ -77,11 +80,12 @@ describe "PasswordResetController", -> done() @PasswordResetController.requestReset @req, @res - it "should send a 404 if the email doesn't exist", (done)-> + it "should send a 200 if the email doesn't exist", (done)-> + # we do not send a 404 so that we do not leak account info @RateLimiter.addCount.callsArgWith(1, null, true) @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, false) @res.send = (code)=> - code.should.equal 404 + code.should.equal 200 done() @PasswordResetController.requestReset @req, @res @@ -90,7 +94,7 @@ describe "PasswordResetController", -> @req.body.email = @email @RateLimiter.addCount.callsArgWith(1, null, true) @PasswordResetHandler.generateAndEmailResetToken.callsArgWith(1, null, true) - @res.sendStatus = (code)=> + @res.send = (code)=> code.should.equal 200 @PasswordResetHandler.generateAndEmailResetToken.calledWith(@email.toLowerCase()).should.equal true done() @@ -159,6 +163,13 @@ describe "PasswordResetController", -> done() @PasswordResetController.setNewUserPassword @req, @res + it 'should call removeReconfirmFlag', (done) -> + @PasswordResetHandler.setNewUserPassword.callsArgWith(2, null, true, @user_id) + @res.sendStatus = (code)=> + @UserUpdater.removeReconfirmFlag.callCount.should.equal 1 + done() + @PasswordResetController.setNewUserPassword @req, @res + describe 'when login_after is set', -> beforeEach -> diff --git a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee index 034c8b68ab..ff52dbb804 100644 --- a/services/web/test/unit/coffee/User/UserUpdaterTests.coffee +++ b/services/web/test/unit/coffee/User/UserUpdaterTests.coffee @@ -275,4 +275,4 @@ describe "UserUpdater", -> @UserUpdater.confirmEmail @stubbedUser._id, @newEmail, (err)=> should.not.exist(err) sinon.assert.calledWith(@refreshFeatures, @stubbedUser._id, true) - done() + done() \ No newline at end of file