Merge pull request #1788 from overleaf/ta-ensure-can-delete-account

Prevent Subscription Owner from Deleting their Account

GitOrigin-RevId: bd60484bdfb6381216cfc0799e45b28300014ca0

services/web/app/coffee/Features/Errors/Errors.coffee

@@ -118,6 +118,14 @@ ThirdPartyUserNotFoundError = (message) ->
 	return error
 ThirdPartyUserNotFoundError.prototype.__proto__ = Error.prototype
 
+SubscriptionAdminDeletionError = (message) ->
+	message = "subscription admins cannot be deleted" unless message?
+	error = new Error(message)
+	error.name = "SubscriptionAdminDeletionError"
+	error.__proto__ = SubscriptionAdminDeletionError.prototype
+	return error
+SubscriptionAdminDeletionError.prototype.__proto__ = Error.prototype
+
 module.exports = Errors =
 	NotFoundError: NotFoundError
 	ForbiddenError: ForbiddenError
@@ -136,3 +144,4 @@ module.exports = Errors =
 	NotInV2Error: NotInV2Error
 	SLInV2Error: SLInV2Error
 	ThirdPartyUserNotFoundError: ThirdPartyUserNotFoundError
+	SubscriptionAdminDeletionError: SubscriptionAdminDeletionError

services/web/app/coffee/Features/User/UserController.coffee

@@ -39,8 +39,11 @@ module.exports = UserController =
 				return res.sendStatus(403)
 			deleteMethod user_id, (err) ->
 				if err?
-					logger.err {user_id}, "error while deleting user account"
-					return next(err)
+					if err instanceof Errors.SubscriptionAdminDeletionError
+						return res.status(422).json(error: err.name)
+					else
+						logger.err {user_id}, "error while deleting user account"
+						return next(err)
 				sessionId = req.sessionID
 				req.logout?()
 				req.session.destroy (err) ->

services/web/app/coffee/Features/User/UserDeleter.coffee

@@ -4,6 +4,7 @@ ProjectDeleter = require("../Project/ProjectDeleter")
 logger = require("logger-sharelatex")
 SubscriptionHandler = require("../Subscription/SubscriptionHandler")
 SubscriptionUpdater = require("../Subscription/SubscriptionUpdater")
+SubscriptionLocator = require("../Subscription/SubscriptionLocator")
 UserMembershipsHandler = require("../UserMembership/UserMembershipsHandler")
 async = require("async")
 InstitutionsAPI = require("../Institutions/InstitutionsAPI")
@@ -20,6 +21,8 @@ module.exports = UserDeleter =
 			return callback(err) if err?
 			return callback(new Errors.NotFoundError("user not found")) unless user?
 			async.series([
+				(cb) ->
+					UserDeleter._ensureCanDeleteUser user, cb
 				(cb) ->
 					UserDeleter._cleanupUser user, cb
 				(cb) ->
@@ -40,6 +43,8 @@ module.exports = UserDeleter =
 				return callback(err)
 			logger.log user:user, "deleting user"
 			async.series [
+				(cb) ->
+					UserDeleter._ensureCanDeleteUser user, cb
 				(cb)->
 					UserDeleter._cleanupUser user, cb
 				(cb)->
@@ -67,3 +72,9 @@ module.exports = UserDeleter =
 			(cb)->
 				UserMembershipsHandler.removeUserFromAllEntities user._id, cb
 		], callback)
+
+	_ensureCanDeleteUser: (user, callback) ->
+		SubscriptionLocator.getUsersSubscription user, (error, subscription) ->
+			if subscription?
+				error ||= new Errors.SubscriptionAdminDeletionError()
+			callback(error)

services/web/app/views/user/settings.pug

@@ -266,13 +266,15 @@ block content
 					label(style="display: inline")  I understand this will delete all projects in my Overleaf v2 account (and ShareLaTeX account, if any) with email address #[em {{ userDefaultEmail }}]
 
 			div(ng-if="state.error")
-				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')}
+				div.alert.alert-danger(ng-switch="state.error.code")
+					span(ng-switch-when="InvalidCredentialsError")
+						| #{translate('email_or_password_wrong_try_again')}
+					span(ng-switch-when="SubscriptionAdminDeletionError")
+						| #{translate('subscription_admins_cannot_be_deleted')}
+					span(ng-switch-default)
+						| #{translate('generic_something_went_wrong')}
 			if settings.createV1AccountOnLogin && settings.overleaf
-				div(ng-if="state.error || state.invalidCredentials")
+				div(ng-if="state.error && state.error.code == 'InvalidCredentialsError'")
 					div.alert.alert-info
 						| If you can't remember your password, or if you are using Single-Sign-On with another provider
 						| to sign in (such as Twitter or Google), please

services/web/public/src/main/account-settings.js

@@ -86,8 +86,7 @@ define(['base'], function(App) {
         confirmV1Purge: false,
         confirmSharelatexDelete: false,
         inflight: false,
-        error: false,
-        invalidCredentials: false
+        error: null
       }
 
       $scope.userDefaultEmail = userDefaultEmail
@@ -107,8 +106,7 @@ define(['base'], function(App) {
 
       $scope.delete = function() {
         $scope.state.inflight = true
-        $scope.state.error = false
-        $scope.state.invalidCredentials = false
+        $scope.state.error = null
         return $http({
           method: 'POST',
           url: '/user/delete',
@@ -124,17 +122,16 @@ define(['base'], function(App) {
           .then(function() {
             $modalInstance.close()
             $scope.state.inflight = false
-            $scope.state.error = false
-            $scope.state.invalidCredentials = false
+            $scope.state.error = null
             return setTimeout(() => (window.location = '/login'), 1000)
           })
           .catch(function(response) {
             const { data, status } = response
             $scope.state.inflight = false
             if (status === 403) {
-              return ($scope.state.invalidCredentials = true)
+              $scope.state.error = { code: 'InvalidCredentialsError' }
             } else {
-              return ($scope.state.error = true)
+              $scope.state.error = { code: data.error }
             }
           })
       }

services/web/test/unit/coffee/User/UserControllerTests.coffee

@@ -167,6 +167,20 @@ describe "UserController", ->
 					done()
 				@UserController.tryDeleteUser @req, @res, @next
 
+		describe 'when deleteUser produces a known error', ->
+
+			beforeEach ->
+				@UserDeleter.deleteUser = sinon.stub().yields(
+					new Errors.SubscriptionAdminDeletionError()
+				)
+
+			it 'should return a json error', (done) ->
+				@UserController.tryDeleteUser @req, status: (status) ->
+					expect(status).to.equal 422
+					json: (json) ->
+						expect(json.error).to.equal Errors.SubscriptionAdminDeletionError.name
+						done()
+
 		describe 'when session.destroy produces an error', ->
 
 			beforeEach ->

services/web/test/unit/coffee/User/UserDeleterTests.coffee

@@ -1,8 +1,10 @@
 sinon = require('sinon')
 chai = require('chai')
 should = chai.should()
+expect = chai.expect
 modulePath = "../../../../app/js/Features/User/UserDeleter.js"
 SandboxedModule = require('sandboxed-module')
+Errors = require('../../../../app/js/Features/Errors/Errors')
 
 describe "UserDeleter", ->
 
@@ -27,6 +29,9 @@ describe "UserDeleter", ->
 		@SubscriptionUpdater =
 			removeUserFromAllGroups: sinon.stub().callsArgWith(1)
 
+		@SubscriptionLocator =
+			getUsersSubscription: sinon.stub().yields(null, null)
+
 		@UserMembershipsHandler =
 			removeUserFromAllEntities: sinon.stub().callsArgWith(1)
 
@@ -44,14 +49,18 @@ describe "UserDeleter", ->
 			"../Newsletter/NewsletterManager":  @NewsletterManager
 			"../Subscription/SubscriptionHandler": @SubscriptionHandler
 			"../Subscription/SubscriptionUpdater": @SubscriptionUpdater
+			"../Subscription/SubscriptionLocator": @SubscriptionLocator
 			"../UserMembership/UserMembershipsHandler": @UserMembershipsHandler
 			"../Project/ProjectDeleter": @ProjectDeleter
 			"../Institutions/InstitutionsAPI":
 				deleteAffiliations: @deleteAffiliations
 			"../../infrastructure/mongojs": @mongojs
 			"logger-sharelatex": @logger = { log: sinon.stub(), err: sinon.stub() }
+			"../Errors/Errors": Errors
 
 	describe "softDeleteUserForMigration", ->
+		beforeEach ->
+			@UserDeleter._ensureCanDeleteUser = sinon.stub().yields(null)
 
 		it "should delete the user in mongo", (done)->
 			@UserDeleter.softDeleteUserForMigration @user._id, (err)=>
@@ -94,7 +103,19 @@ describe "UserDeleter", ->
 				@UserMembershipsHandler.removeUserFromAllEntities.calledWith(@user._id).should.equal true
 				done()
 
+		it "ensures user can be deleted first", (done)->
+			@UserDeleter._ensureCanDeleteUser.yields(
+				new Errors.SubscriptionAdminDeletionError()
+			)
+			@UserDeleter.softDeleteUserForMigration @user._id, (error) =>
+				sinon.assert.calledWith(@UserDeleter._ensureCanDeleteUser, @user)
+				sinon.assert.notCalled(@user.remove)
+				expect(error).to.be.instanceof Errors.SubscriptionAdminDeletionError
+				done()
+
 	describe "deleteUser", ->
+		beforeEach ->
+			@UserDeleter._ensureCanDeleteUser = sinon.stub().yields(null)
 
 		it "should delete the user in mongo", (done)->
 			@UserDeleter.deleteUser @user._id, (err)=>
@@ -132,6 +153,16 @@ describe "UserDeleter", ->
 				@UserMembershipsHandler.removeUserFromAllEntities.calledWith(@user._id).should.equal true
 				done()
 
+		it "ensures user can be deleted first", (done)->
+			@UserDeleter._ensureCanDeleteUser.yields(
+				new Errors.SubscriptionAdminDeletionError()
+			)
+			@UserDeleter.deleteUser @user._id, (error) =>
+				sinon.assert.calledWith(@UserDeleter._ensureCanDeleteUser, @user)
+				sinon.assert.notCalled(@user.remove)
+				expect(error).to.be.instanceof Errors.SubscriptionAdminDeletionError
+				done()
+
 		describe "when unsubscribing from mailchimp fails", ->
 			beforeEach ->
 				@NewsletterManager.unsubscribe = sinon.stub().callsArgWith(1, new Error("something went wrong"))
@@ -152,3 +183,22 @@ describe "UserDeleter", ->
 				@UserDeleter.deleteUser @user._id, (err)=>
 					sinon.assert.called(@logger.err)
 					done()
+
+	describe '_ensureCanDeleteUser', ->
+		it 'should not return error when user can be deleted', (done) ->
+			@SubscriptionLocator.getUsersSubscription.yields(null, null)
+			@UserDeleter._ensureCanDeleteUser @user, (error) ->
+				expect(error).to.not.exist
+				done()
+
+		it 'should return custom error when user is group admin', (done) ->
+			@SubscriptionLocator.getUsersSubscription.yields(null, { _id: '123abc' })
+			@UserDeleter._ensureCanDeleteUser @user, (error) ->
+				expect(error).to.be.instanceof Errors.SubscriptionAdminDeletionError
+				done()
+
+		it 'propagate errors', (done) ->
+			@SubscriptionLocator.getUsersSubscription.yields(new Error('Some error'))
+			@UserDeleter._ensureCanDeleteUser @user, (error) ->
+				expect(error).to.be.instanceof Error
+				done()