From efe6df145c24a667cb86bfc5d1256dd37b4760bc Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 25 Oct 2016 14:33:47 +0100 Subject: [PATCH 1/6] WIP: ask for password when deleting account --- .../Features/User/UserController.coffee | 21 +++++++++++++++++++ services/web/app/coffee/router.coffee | 1 + services/web/app/views/user/settings.jade | 11 ++++++++++ .../coffee/main/account-settings.coffee | 18 ++++++++++++---- 4 files changed, 47 insertions(+), 4 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 389de1a0f2..839968c277 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -15,6 +15,7 @@ settings = require "settings-sharelatex" module.exports = UserController = + # TODO: deprecated, remove deleteUser: (req, res)-> user_id = AuthenticationController.getLoggedInUserId(req) UserDeleter.deleteUser user_id, (err)-> @@ -22,6 +23,26 @@ module.exports = UserController = req.session?.destroy() res.sendStatus(200) + tryDeleteUser: (req, res, next) -> + user_id = AuthenticationController.getLoggedInUserId(req) + password = req.body.password + console.log '>> here', user_id, password + return res.sendStatus(500) + if !password? or password == '' + logger.err {user_id}, 'no password supplied for attempt to delete account' + return res.sendStatus(403) + AuthenticationManager.authenticate {_id: user_id}, password, (err, user) -> + if err? + logger.err {user_id}, 'error authenticating during attempt to delete account' + return next(err) + if user + UserDeleter.deleteUser user_id, (err) -> + if err? + logger.err {user_id}, "error while deleting user account" + return next(err) + req.session?.destroy() + res.sendStatus(200) + unsubscribe: (req, res)-> user_id = AuthenticationController.getLoggedInUserId(req) UserLocator.findById user_id, (err, user)-> diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 56dd8d821b..26bfd6618f 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -93,6 +93,7 @@ module.exports = class Router webRouter.delete '/user/newsletter/unsubscribe', AuthenticationController.requireLogin(), UserController.unsubscribe webRouter.delete '/user', AuthenticationController.requireLogin(), UserController.deleteUser + webRouter.post '/user/delete', AuthenticationController.requireLogin(), UserController.tryDeleteUser webRouter.get '/user/personal_info', AuthenticationController.requireLogin(), UserInfoController.getLoggedInUsersPersonalInfo apiRouter.get '/user/:user_id/personal_info', AuthenticationController.httpAuth, UserInfoController.getPersonalInfo diff --git a/services/web/app/views/user/settings.jade b/services/web/app/views/user/settings.jade index d2fa8326d1..5c9ede6304 100644 --- a/services/web/app/views/user/settings.jade +++ b/services/web/app/views/user/settings.jade @@ -153,6 +153,7 @@ block content .modal-body p !{translate("delete_account_warning_message_2")} form(novalidate, name="deleteAccountForm") + label #{translate('email')} input.form-control( type="text", placeholder="", @@ -160,6 +161,16 @@ block content focus-on="open", ng-keyup="checkValidation()" ) + label #{translate('password')} + input.form-control( + type="password", + placeholder="", + ng-model="state.password", + ) + div(ng-if="state.error") + br + div.alert.alert-danger + | #{translate('generic_something_went_wrong')} .modal-footer button.btn.btn-default( ng-click="cancel()" diff --git a/services/web/public/coffee/main/account-settings.coffee b/services/web/public/coffee/main/account-settings.coffee index 29ec146051..24ef77b4aa 100644 --- a/services/web/public/coffee/main/account-settings.coffee +++ b/services/web/public/coffee/main/account-settings.coffee @@ -29,10 +29,11 @@ define [ App.controller "DeleteAccountModalController", [ "$scope", "$modalInstance", "$timeout", "$http", ($scope, $modalInstance, $timeout, $http) -> - $scope.state = + $scope.state = isValid : false deleteText: "" inflight: false + error: false $modalInstance.opened.then () -> $timeout () -> @@ -44,16 +45,25 @@ define [ $scope.delete = () -> $scope.state.inflight = true - + $scope.state.error = false $http({ - method: "DELETE" - url: "/user" + method: "POST" + url: "/user/delete" headers: "X-CSRF-Token": window.csrfToken + "Content-Type": 'application/json' + data: + password: $scope.state.password }) .success () -> $modalInstance.close() + $scope.state.inflight = false + $scope.state.error = false window.location = "/" + .error (err) -> + console.log ">> error", err + $scope.state.error = true + $scope.state.inflight = false $scope.cancel = () -> $modalInstance.dismiss('cancel') From 1c8721ceabda250dfcd54bb0a1e12f62c916d765 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Tue, 25 Oct 2016 16:23:50 +0100 Subject: [PATCH 2/6] Functioning account deletion with password --- .../Features/User/UserController.coffee | 19 ++++++++++--------- services/web/app/views/user/settings.jade | 9 ++++++--- .../coffee/main/account-settings.coffee | 14 ++++++++++---- .../stylesheets/app/account-settings.less | 9 ++++++++- 4 files changed, 34 insertions(+), 17 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 839968c277..f6b43c9b84 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -26,8 +26,7 @@ module.exports = UserController = tryDeleteUser: (req, res, next) -> user_id = AuthenticationController.getLoggedInUserId(req) password = req.body.password - console.log '>> here', user_id, password - return res.sendStatus(500) + logger.info {user_id}, "trying to delete user account" if !password? or password == '' logger.err {user_id}, 'no password supplied for attempt to delete account' return res.sendStatus(403) @@ -35,13 +34,15 @@ module.exports = UserController = if err? logger.err {user_id}, 'error authenticating during attempt to delete account' return next(err) - if user - UserDeleter.deleteUser user_id, (err) -> - if err? - logger.err {user_id}, "error while deleting user account" - return next(err) - req.session?.destroy() - res.sendStatus(200) + if !user + logger.err {user_id}, 'auth failde during attempt to delete account' + return res.sendStatus(403) + UserDeleter.deleteUser user_id, (err) -> + if err? + logger.err {user_id}, "error while deleting user account" + return next(err) + req.session?.destroy() + res.sendStatus(200) unsubscribe: (req, res)-> user_id = AuthenticationController.getLoggedInUserId(req) diff --git a/services/web/app/views/user/settings.jade b/services/web/app/views/user/settings.jade index 5c9ede6304..497f3fd1e6 100644 --- a/services/web/app/views/user/settings.jade +++ b/services/web/app/views/user/settings.jade @@ -150,8 +150,8 @@ block content script(type='text/ng-template', id='deleteAccountModalTemplate') .modal-header h3 #{translate("delete_account")} - .modal-body - p !{translate("delete_account_warning_message_2")} + div.modal-body#delete-account-modal + p !{translate("delete_account_warning_message_3")} form(novalidate, name="deleteAccountForm") label #{translate('email')} input.form-control( @@ -166,11 +166,14 @@ block content type="password", placeholder="", ng-model="state.password", + ng-keyup="checkValidation()" ) div(ng-if="state.error") - br div.alert.alert-danger | #{translate('generic_something_went_wrong')} + div(ng-if="state.invalidCredentials") + div.alert.alert-danger + | #{translate('email_or_password_wrong_try_again')} .modal-footer button.btn.btn-default( ng-click="cancel()" diff --git a/services/web/public/coffee/main/account-settings.coffee b/services/web/public/coffee/main/account-settings.coffee index 24ef77b4aa..08226ab399 100644 --- a/services/web/public/coffee/main/account-settings.coffee +++ b/services/web/public/coffee/main/account-settings.coffee @@ -32,8 +32,10 @@ define [ $scope.state = isValid : false deleteText: "" + password: "" inflight: false error: false + invalidCredentials: false $modalInstance.opened.then () -> $timeout () -> @@ -41,11 +43,12 @@ define [ , 700 $scope.checkValidation = -> - $scope.state.isValid = $scope.state.deleteText == $scope.email + $scope.state.isValid = $scope.state.deleteText == $scope.email and $scope.state.password.length > 0 $scope.delete = () -> $scope.state.inflight = true $scope.state.error = false + $scope.state.invalidCredentials = false $http({ method: "POST" url: "/user/delete" @@ -59,11 +62,14 @@ define [ $modalInstance.close() $scope.state.inflight = false $scope.state.error = false + $scope.state.invalidCredentials = false window.location = "/" - .error (err) -> - console.log ">> error", err - $scope.state.error = true + .error (data, status) -> $scope.state.inflight = false + if status == 403 + $scope.state.invalidCredentials = true + else + $scope.state.error = true $scope.cancel = () -> $modalInstance.dismiss('cancel') diff --git a/services/web/public/stylesheets/app/account-settings.less b/services/web/public/stylesheets/app/account-settings.less index 23769e055b..c232e36fab 100644 --- a/services/web/public/stylesheets/app/account-settings.less +++ b/services/web/public/stylesheets/app/account-settings.less @@ -2,4 +2,11 @@ .alert { margin-bottom: 0; } -} \ No newline at end of file +} + +#delete-account-modal { + .alert { + margin-top: 25px; + margin-bottom: 4px; + } +} From fc7bd4c2d365e29b6dabab62adc46b599caedb1a Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 26 Oct 2016 10:57:34 +0100 Subject: [PATCH 3/6] fix logging --- services/web/app/coffee/Features/User/UserController.coffee | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index f6b43c9b84..6c4f51408d 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -26,7 +26,7 @@ module.exports = UserController = tryDeleteUser: (req, res, next) -> user_id = AuthenticationController.getLoggedInUserId(req) password = req.body.password - logger.info {user_id}, "trying to delete user account" + logger.log {user_id}, "trying to delete user account" if !password? or password == '' logger.err {user_id}, 'no password supplied for attempt to delete account' return res.sendStatus(403) @@ -35,7 +35,7 @@ module.exports = UserController = logger.err {user_id}, 'error authenticating during attempt to delete account' return next(err) if !user - logger.err {user_id}, 'auth failde during attempt to delete account' + logger.err {user_id}, 'auth failed during attempt to delete account' return res.sendStatus(403) UserDeleter.deleteUser user_id, (err) -> if err? From 7cc26f22077ddd52b60428ac5cb034af7b28e150 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 26 Oct 2016 10:57:54 +0100 Subject: [PATCH 4/6] Disable autocomplete on user-delete form --- services/web/app/views/user/settings.jade | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/web/app/views/user/settings.jade b/services/web/app/views/user/settings.jade index 497f3fd1e6..eb5bf671fa 100644 --- a/services/web/app/views/user/settings.jade +++ b/services/web/app/views/user/settings.jade @@ -156,6 +156,7 @@ block content label #{translate('email')} input.form-control( type="text", + autocomplete="off", placeholder="", ng-model="state.deleteText", focus-on="open", @@ -164,6 +165,7 @@ block content label #{translate('password')} input.form-control( type="password", + autocomplete="off", placeholder="", ng-model="state.password", ng-keyup="checkValidation()" From a4167fcccd2563c28c9705e089ab3798a4fe6be3 Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 26 Oct 2016 11:01:35 +0100 Subject: [PATCH 5/6] Unit tests for `tryDeleteUser` --- .../coffee/User/UserControllerTests.coffee | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index a9c98e02ec..052c8caa71 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -84,6 +84,7 @@ describe "UserController", -> sendStatus: sinon.stub() json: sinon.stub() @next = sinon.stub() + describe "deleteUser", -> it "should delete the user", (done)-> @@ -94,6 +95,81 @@ describe "UserController", -> done() @UserController.deleteUser @req, @res + describe 'tryDeleteUser', -> + + beforeEach -> + @req.body.password = 'wat' + @AuthenticationController.getLoggedInUserId = sinon.stub().returns(@user._id) + @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, @user) + @UserDeleter.deleteUser = sinon.stub().callsArgWith(1, null) + + it 'should send 200', (done) -> + @res.sendStatus = (code) => + code.should.equal 200 + done() + @UserController.tryDeleteUser @req, @res, @next + + it 'should try to authenticate user', (done) -> + @res.sendStatus = (code) => + @AuthenticationManager.authenticate.callCount.should.equal 1 + @AuthenticationManager.authenticate.calledWith({_id: @user._id}, @req.body.password).should.equal true + done() + @UserController.tryDeleteUser @req, @res, @next + + it 'should delete the user', (done) -> + @res.sendStatus = (code) => + @UserDeleter.deleteUser.callCount.should.equal 1 + @UserDeleter.deleteUser.calledWith(@user._id).should.equal true + done() + @UserController.tryDeleteUser @req, @res, @next + + describe 'when no password is supplied', -> + + beforeEach -> + @req.body.password = '' + + it 'should return 403', (done) -> + @res.sendStatus = (code) => + code.should.equal 403 + done() + @UserController.tryDeleteUser @req, @res, @next + + describe 'when authenticate produces an error', -> + + beforeEach -> + @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, new Error('woops')) + + it 'should call next with an error', (done) -> + @next = (err) => + expect(err).to.not.equal null + expect(err).to.be.instanceof Error + done() + @UserController.tryDeleteUser @req, @res, @next + + describe 'when authenticate does not produce a user', -> + + beforeEach -> + @AuthenticationManager.authenticate = sinon.stub().callsArgWith(2, null, null) + + it 'should return 403', (done) -> + @res.sendStatus = (code) => + code.should.equal 403 + done() + @UserController.tryDeleteUser @req, @res, @next + + describe 'when deleteUser produces an error', -> + + beforeEach -> + @UserDeleter.deleteUser = sinon.stub().callsArgWith(1, new Error('woops')) + + it 'should call next with an error', (done) -> + @next = (err) => + expect(err).to.not.equal null + expect(err).to.be.instanceof Error + done() + @UserController.tryDeleteUser @req, @res, @next + + describe "unsubscribe", -> it "should send the user to unsubscribe", (done)-> From dc62b1a86e2bff66925e121bd18e82bf230da9da Mon Sep 17 00:00:00 2001 From: Shane Kilkelly Date: Wed, 26 Oct 2016 11:18:36 +0100 Subject: [PATCH 6/6] Remove deprecated endpoint for old delete-user --- .../web/app/coffee/Features/User/UserController.coffee | 8 -------- services/web/app/coffee/router.coffee | 1 - .../UnitTests/coffee/User/UserControllerTests.coffee | 10 ---------- 3 files changed, 19 deletions(-) diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 6c4f51408d..f170250ccf 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -15,14 +15,6 @@ settings = require "settings-sharelatex" module.exports = UserController = - # TODO: deprecated, remove - deleteUser: (req, res)-> - user_id = AuthenticationController.getLoggedInUserId(req) - UserDeleter.deleteUser user_id, (err)-> - if !err? - req.session?.destroy() - res.sendStatus(200) - tryDeleteUser: (req, res, next) -> user_id = AuthenticationController.getLoggedInUserId(req) password = req.body.password diff --git a/services/web/app/coffee/router.coffee b/services/web/app/coffee/router.coffee index 26bfd6618f..a027f6359e 100644 --- a/services/web/app/coffee/router.coffee +++ b/services/web/app/coffee/router.coffee @@ -92,7 +92,6 @@ module.exports = class Router webRouter.post '/user/sessions/clear', AuthenticationController.requireLogin(), UserController.clearSessions webRouter.delete '/user/newsletter/unsubscribe', AuthenticationController.requireLogin(), UserController.unsubscribe - webRouter.delete '/user', AuthenticationController.requireLogin(), UserController.deleteUser webRouter.post '/user/delete', AuthenticationController.requireLogin(), UserController.tryDeleteUser webRouter.get '/user/personal_info', AuthenticationController.requireLogin(), UserInfoController.getLoggedInUsersPersonalInfo diff --git a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee index 052c8caa71..cc1d2190ef 100644 --- a/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee +++ b/services/web/test/UnitTests/coffee/User/UserControllerTests.coffee @@ -85,16 +85,6 @@ describe "UserController", -> json: sinon.stub() @next = sinon.stub() - describe "deleteUser", -> - - it "should delete the user", (done)-> - - @res.sendStatus = (code)=> - @UserDeleter.deleteUser.calledWith(@user_id) - code.should.equal 200 - done() - @UserController.deleteUser @req, @res - describe 'tryDeleteUser', -> beforeEach ->