From bb81a47d5879e17884d84ee74d3e52e9e7e96baf Mon Sep 17 00:00:00 2001 From: Simon Detheridge Date: Tue, 26 Mar 2019 13:43:59 +0000 Subject: [PATCH] Merge pull request #1641 from sharelatex/spd-soft-delete-users Add initial support for soft-deletion of users GitOrigin-RevId: 22e47536732c5aec843d120773d2565112ad80b7 --- .../Features/Project/ProjectDeleter.coffee | 32 ++++++- .../Features/User/UserController.coffee | 8 +- .../coffee/Features/User/UserDeleter.coffee | 42 +++++++-- .../coffee/Project/ProjectDeleterTests.coffee | 92 +++++++++++++++++-- .../unit/coffee/User/UserDeleterTests.coffee | 45 +++++++++ 5 files changed, 199 insertions(+), 20 deletions(-) diff --git a/services/web/app/coffee/Features/Project/ProjectDeleter.coffee b/services/web/app/coffee/Features/Project/ProjectDeleter.coffee index 3daca09632..7a7bbfa984 100644 --- a/services/web/app/coffee/Features/Project/ProjectDeleter.coffee +++ b/services/web/app/coffee/Features/Project/ProjectDeleter.coffee @@ -5,6 +5,7 @@ tagsHandler = require("../Tags/TagsHandler") async = require("async") FileStoreHandler = require("../FileStore/FileStoreHandler") CollaboratorsHandler = require("../Collaborators/CollaboratorsHandler") +{db, ObjectId} = require("../../infrastructure/mongojs") module.exports = ProjectDeleter = @@ -25,9 +26,36 @@ module.exports = ProjectDeleter = deleteUsersProjects: (user_id, callback)-> logger.log {user_id}, "deleting users projects" - Project.remove owner_ref:user_id, (error) -> + ProjectDeleter._deleteUsersProjectWithMethod user_id, ProjectDeleter.deleteProject, callback + + softDeleteUsersProjects: (user_id, callback)-> + logger.log {user_id}, "soft-deleting users projects" + ProjectDeleter._deleteUsersProjectWithMethod user_id, ProjectDeleter.softDeleteProject, callback + + _deleteUsersProjectWithMethod: (user_id, deleteMethod, callback) -> + Project.find {owner_ref: user_id}, (error, projects) -> return callback(error) if error? - CollaboratorsHandler.removeUserFromAllProjets user_id, callback + async.each( + projects, + (project, cb) -> + deleteMethod project._id, cb + (err) -> + return callback(err) if err? + CollaboratorsHandler.removeUserFromAllProjets user_id, callback + ) + + softDeleteProject: (project_id, callback) -> + logger.log project_id: project_id, "soft-deleting project" + async.waterfall [ + (cb) -> + Project.findOne {_id: project_id}, (err, project) -> cb(err, project) + (project, cb) -> + return callback(new Errors.NotFoundError("project not found")) unless project? + project.deletedAt = new Date() + db.deletedProjects.insert project, (err) -> cb(err) + (cb) -> + ProjectDeleter.deleteProject project_id, cb + ], callback deleteProject: (project_id, callback = (error) ->) -> logger.log project_id: project_id, "deleting project" diff --git a/services/web/app/coffee/Features/User/UserController.coffee b/services/web/app/coffee/Features/User/UserController.coffee index 72e2eaff88..877caaec45 100644 --- a/services/web/app/coffee/Features/User/UserController.coffee +++ b/services/web/app/coffee/Features/User/UserController.coffee @@ -18,6 +18,12 @@ Errors = require "../Errors/Errors" module.exports = UserController = tryDeleteUser: (req, res, next) -> + UserController._tryDeleteUser(UserDeleter.deleteUser, req, res, next) + + trySoftDeleteUser: (req, res, next) -> + UserController._tryDeleteUser(UserDeleter.softDeleteUser, req, res, next) + + _tryDeleteUser: (deleteMethod, req, res, next) -> user_id = AuthenticationController.getLoggedInUserId(req) password = req.body.password logger.log {user_id}, "trying to delete user account" @@ -31,7 +37,7 @@ module.exports = UserController = if !user logger.err {user_id}, 'auth failed during attempt to delete account' return res.sendStatus(403) - UserDeleter.deleteUser user_id, (err) -> + deleteMethod user_id, (err) -> if err? logger.err {user_id}, "error while deleting user account" return next(err) diff --git a/services/web/app/coffee/Features/User/UserDeleter.coffee b/services/web/app/coffee/Features/User/UserDeleter.coffee index 149959c00f..19827b8530 100644 --- a/services/web/app/coffee/Features/User/UserDeleter.coffee +++ b/services/web/app/coffee/Features/User/UserDeleter.coffee @@ -4,9 +4,30 @@ ProjectDeleter = require("../Project/ProjectDeleter") logger = require("logger-sharelatex") SubscriptionHandler = require("../Subscription/SubscriptionHandler") async = require("async") -{ deleteAffiliations } = require("../Institutions/InstitutionsAPI") +InstitutionsAPI = require("../Institutions/InstitutionsAPI") +Errors = require("../Errors/Errors") +{db, ObjectId} = require("../../infrastructure/mongojs") -module.exports = +module.exports = UserDeleter = + + softDeleteUser: (user_id, callback = (err)->)-> + if !user_id? + logger.err "user_id is null when trying to delete user" + return callback(new Error("no user_id")) + User.findById user_id, (err, user)-> + return callback(err) if err? + return callback(new Errors.NotFoundError("user not found")) unless user? + async.series([ + (cb) -> + UserDeleter._cleanupUser user, cb + (cb) -> + ProjectDeleter.softDeleteUsersProjects user._id, cb + (cb) -> + user.deletedAt = new Date() + db.deletedUsers.insert user, cb + (cb) -> + user.remove cb + ], callback) deleteUser: (user_id, callback = ()->)-> if !user_id? @@ -18,16 +39,23 @@ module.exports = logger.log user:user, "deleting user" async.series [ (cb)-> - NewsletterManager.unsubscribe user, cb + UserDeleter._cleanupUser user, cb (cb)-> ProjectDeleter.deleteUsersProjects user._id, cb - (cb)-> - SubscriptionHandler.cancelSubscription user, cb - (cb)-> - deleteAffiliations user._id, cb (cb)-> user.remove cb ], (err)-> if err? logger.err err:err, user_id:user_id, "something went wrong deleteing the user" callback err + + _cleanupUser: (user, callback) -> + return callback(new Error("no user supplied")) unless user? + async.series([ + (cb)-> + NewsletterManager.unsubscribe user, cb + (cb)-> + SubscriptionHandler.cancelSubscription user, cb + (cb)-> + InstitutionsAPI.deleteAffiliations user._id, cb + ], callback) diff --git a/services/web/test/unit/coffee/Project/ProjectDeleterTests.coffee b/services/web/test/unit/coffee/Project/ProjectDeleterTests.coffee index 5d7fe189bd..08dcbcd7c3 100644 --- a/services/web/test/unit/coffee/Project/ProjectDeleterTests.coffee +++ b/services/web/test/unit/coffee/Project/ProjectDeleterTests.coffee @@ -14,11 +14,20 @@ describe 'ProjectDeleter', -> collaberator_refs:["collab1", "collab2"] readOnly_refs:["readOnly1", "readOnly2"] owner_ref:"owner ref here" + remove: sinon.stub().callsArg(0) + + @user_id = 1234 + + @mongojs = + db: + deletedProjects: + insert: sinon.stub().callsArg(1) @Project = update: sinon.stub().callsArgWith(3) remove: sinon.stub().callsArgWith(1) - findById: sinon.stub().callsArgWith(1, null, @project) + findOne: sinon.stub().callsArgWith(1, null, @project) + find: sinon.stub().callsArgWith(1, null, [@project]) applyToAllFilesRecursivly: sinon.stub() @documentUpdaterHandler = flushProjectToMongoAndDelete:sinon.stub().callsArgWith(1) @@ -27,6 +36,7 @@ describe 'ProjectDeleter', -> removeProjectFromAllTags: sinon.stub().callsArgWith(2) @CollaboratorsHandler = removeUserFromAllProjets: sinon.stub().yields() + getMemberIds: sinon.stub().withArgs(@project_id).yields(null, ["member-id-1", "member-id-2"]) @deleter = SandboxedModule.require modulePath, requires: "../Editor/EditorController": @editorController '../../models/Project':{Project:@Project} @@ -34,6 +44,7 @@ describe 'ProjectDeleter', -> "../Tags/TagsHandler":@TagsHandler "../FileStore/FileStoreHandler": @FileStoreHandler = {} "../Collaborators/CollaboratorsHandler": @CollaboratorsHandler + "../../infrastructure/mongojs": @mongojs 'logger-sharelatex': log:-> @@ -66,24 +77,66 @@ describe 'ProjectDeleter', -> .should.equal true describe "deleteUsersProjects", -> + beforeEach -> + @deleter.deleteProject = sinon.stub().callsArg(1) - it "should remove all the projects owned by the user_id", (done)-> - user_id = 1234 - @deleter.deleteUsersProjects user_id, => - @Project.remove.calledWith(owner_ref:user_id).should.equal true + it "should find all the projects owned by the user_id", (done)-> + @deleter.deleteUsersProjects @user_id, => + sinon.assert.calledWith(@Project.find, owner_ref: @user_id) + done() + + it "should call deleteProject on the found projects", (done)-> + @deleter.deleteUsersProjects @user_id, => + sinon.assert.calledWith(@deleter.deleteProject, @project._id) + done() + + it "should call deleteProject once for each project", (done)-> + @Project.find.callsArgWith(1, null, [ + {_id: 'potato'}, {_id: 'wombat'} + ]) + @deleter.deleteUsersProjects @user_id, => + sinon.assert.calledTwice(@deleter.deleteProject) + sinon.assert.calledWith(@deleter.deleteProject, 'wombat') + sinon.assert.calledWith(@deleter.deleteProject, 'potato') done() it "should remove all the projects the user is a collaborator of", (done)-> - user_id = 1234 - @deleter.deleteUsersProjects user_id, => - @CollaboratorsHandler.removeUserFromAllProjets.calledWith(user_id).should.equal true + @deleter.deleteUsersProjects @user_id, => + @CollaboratorsHandler.removeUserFromAllProjets.calledWith(@user_id).should.equal true + done() + + describe "softDeleteUsersProjects", -> + beforeEach -> + @deleter.softDeleteProject = sinon.stub().callsArg(1) + + it "should find all the projects owned by the user_id", (done)-> + @deleter.softDeleteUsersProjects @user_id, => + @Project.find.calledWith(owner_ref: @user_id).should.equal true + done() + + it "should call deleteProject on the found projects", (done)-> + @deleter.softDeleteUsersProjects @user_id, => + sinon.assert.calledWith(@deleter.softDeleteProject, @project._id) + done() + + it "should call deleteProject once for each project", (done)-> + @Project.find.callsArgWith(1, null, [ + {_id: 'potato'}, {_id: 'wombat'} + ]) + @deleter.softDeleteUsersProjects @user_id, => + sinon.assert.calledTwice(@deleter.softDeleteProject) + sinon.assert.calledWith(@deleter.softDeleteProject, 'wombat') + sinon.assert.calledWith(@deleter.softDeleteProject, 'potato') + done() + + it "should remove all the projects the user is a collaborator of", (done)-> + @deleter.softDeleteUsersProjects @user_id, => + @CollaboratorsHandler.removeUserFromAllProjets.calledWith(@user_id).should.equal true done() describe "deleteProject", -> beforeEach (done) -> @project_id = "mock-project-id-123" - @CollaboratorsHandler.getMemberIds = sinon.stub() - @CollaboratorsHandler.getMemberIds.withArgs(@project_id).yields(null, ["member-id-1", "member-id-2"]) @Project.remove.callsArgWith(1) done() @@ -105,6 +158,25 @@ describe 'ProjectDeleter', -> }).should.equal true done() + describe "softDeleteProject", -> + beforeEach -> + @deleter.deleteProject = sinon.stub().callsArg(1) + + it "should set the deletedAt time", (done)-> + @deleter.softDeleteProject @project_id, => + @project.deletedAt.should.exist + done() + + it "should insert the project into the deleted projects collection", (done)-> + @deleter.softDeleteProject @project_id, => + sinon.assert.calledWith(@mongojs.db.deletedProjects.insert, @project) + done() + + it "should delete the project", (done)-> + @deleter.softDeleteProject @project_id, => + sinon.assert.calledWith(@deleter.deleteProject, @project_id) + done() + describe "archiveProject", -> beforeEach -> @Project.update.callsArgWith(2) diff --git a/services/web/test/unit/coffee/User/UserDeleterTests.coffee b/services/web/test/unit/coffee/User/UserDeleterTests.coffee index fc5f2ef370..99d3b02c89 100644 --- a/services/web/test/unit/coffee/User/UserDeleterTests.coffee +++ b/services/web/test/unit/coffee/User/UserDeleterTests.coffee @@ -20,12 +20,18 @@ describe "UserDeleter", -> @ProjectDeleter = deleteUsersProjects: sinon.stub().callsArgWith(1) + softDeleteUsersProjects: sinon.stub().callsArgWith(1) @SubscriptionHandler = cancelSubscription: sinon.stub().callsArgWith(1) @deleteAffiliations = sinon.stub().callsArgWith(1) + @mongojs = + db: + deletedUsers: + insert: sinon.stub().callsArg(1) + @UserDeleter = SandboxedModule.require modulePath, requires: "../../models/User": User: @User "../Newsletter/NewsletterManager": @NewsletterManager @@ -33,8 +39,47 @@ describe "UserDeleter", -> "../Project/ProjectDeleter": @ProjectDeleter "../Institutions/InstitutionsAPI": deleteAffiliations: @deleteAffiliations + "../../infrastructure/mongojs": @mongojs "logger-sharelatex": @logger = { log: sinon.stub() } + describe "softDeleteUser", -> + + it "should delete the user in mongo", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @User.findById.calledWith(@user._id).should.equal true + @user.remove.called.should.equal true + done() + + it "should add the user to the deletedUsers collection", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + sinon.assert.calledWith(@mongojs.db.deletedUsers.insert, @user) + done() + + it "should set the deletedAt field on the user", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @user.deletedAt.should.exist + done() + + it "should unsubscribe the user from the news letter", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @NewsletterManager.unsubscribe.calledWith(@user).should.equal true + done() + + it "should unsubscribe the user", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @SubscriptionHandler.cancelSubscription.calledWith(@user).should.equal true + done() + + it "should delete user affiliations", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @deleteAffiliations.calledWith(@user._id).should.equal true + done() + + it "should soft-delete all the projects of a user", (done)-> + @UserDeleter.softDeleteUser @user._id, (err)=> + @ProjectDeleter.softDeleteUsersProjects.calledWith(@user._id).should.equal true + done() + describe "deleteUser", -> it "should delete the user in mongo", (done)->