diff --git a/services/web/app/src/Features/Errors/Errors.js b/services/web/app/src/Features/Errors/Errors.js index b281c005d0..709d12b10a 100644 --- a/services/web/app/src/Features/Errors/Errors.js +++ b/services/web/app/src/Features/Errors/Errors.js @@ -172,6 +172,8 @@ class InvalidQueryError extends OErrorV2CompatibleError { class ProjectIsArchivedOrTrashedError extends BackwardCompatibleError {} +class AffiliationError extends OError {} + module.exports = { OError, BackwardCompatibleError, @@ -201,5 +203,6 @@ module.exports = { UserNotCollaboratorError, DocHasRangesError, InvalidQueryError, - ProjectIsArchivedOrTrashedError + ProjectIsArchivedOrTrashedError, + AffiliationError } diff --git a/services/web/app/src/Features/User/UserCreator.js b/services/web/app/src/Features/User/UserCreator.js index eded669f8b..d3d856d43d 100644 --- a/services/web/app/src/Features/User/UserCreator.js +++ b/services/web/app/src/Features/User/UserCreator.js @@ -1,16 +1,21 @@ const logger = require('logger-sharelatex') const util = require('util') -const { User } = require('../../models/User') +const { AffiliationError } = require('../Errors/Errors') const Features = require('../../infrastructure/Features') -const UserUpdater = require('./UserUpdater') +const { User } = require('../../models/User') +const UserDeleter = require('./UserDeleter') const UserGetter = require('./UserGetter') +const UserUpdater = require('./UserUpdater') -async function _addAffiliation(user) { +async function _addAffiliation(user, affiliationOptions) { try { - await UserUpdater.promises.addAffiliationForNewUser(user._id, user.email) + await UserUpdater.promises.addAffiliationForNewUser( + user._id, + user.email, + affiliationOptions + ) } catch (error) { - // do not pass error back during registration - // errors are logged in UserUpdater and InstitutionsAPI + throw new AffiliationError('add affiliation failed').withCause(error) } try { @@ -24,7 +29,7 @@ async function _addAffiliation(user) { return user } -async function createNewUser(attributes) { +async function createNewUser(attributes, options = {}) { let user = new User() if (attributes.first_name == null || attributes.first_name === '') { @@ -65,7 +70,16 @@ async function createNewUser(attributes) { user = await user.save() if (Features.hasFeature('affiliations')) { - user = await _addAffiliation(user) + try { + user = await _addAffiliation(user, options.affiliationOptions || {}) + } catch (error) { + if (options.requireAffiliation) { + await UserDeleter.promises.deleteMongoUser(user._id) + throw error + } else { + logger.error(error) + } + } } return user diff --git a/services/web/app/src/Features/User/UserDeleter.js b/services/web/app/src/Features/User/UserDeleter.js index f55bec3589..ce748be3af 100644 --- a/services/web/app/src/Features/User/UserDeleter.js +++ b/services/web/app/src/Features/User/UserDeleter.js @@ -15,12 +15,14 @@ const Errors = require('../Errors/Errors') module.exports = { deleteUser: callbackify(deleteUser), + deleteMongoUser: callbackify(deleteMongoUser), expireDeletedUser: callbackify(expireDeletedUser), ensureCanDeleteUser: callbackify(ensureCanDeleteUser), expireDeletedUsersAfterDuration: callbackify(expireDeletedUsersAfterDuration), promises: { deleteUser: deleteUser, + deleteMongoUser: deleteMongoUser, expireDeletedUser: expireDeletedUser, ensureCanDeleteUser: ensureCanDeleteUser, expireDeletedUsersAfterDuration: expireDeletedUsersAfterDuration @@ -41,13 +43,24 @@ async function deleteUser(userId, options = {}) { await _cleanupUser(user) await _createDeletedUser(user, options) await ProjectDeleter.promises.deleteUsersProjects(user._id) - await User.deleteOne({ _id: userId }).exec() + await deleteMongoUser(user._id) } catch (error) { logger.warn({ error, userId }, 'something went wrong deleting the user') throw error } } +/** + * delete a user document only + */ +async function deleteMongoUser(userId) { + if (!userId) { + throw new Error('no user_id') + } + + await User.deleteOne({ _id: userId }).exec() +} + async function expireDeletedUser(userId) { let deletedUser = await DeletedUser.findOne({ 'deleterData.deletedUserId': userId diff --git a/services/web/app/src/Features/User/UserRegistrationHandler.js b/services/web/app/src/Features/User/UserRegistrationHandler.js index 36f2067643..84578e1faa 100644 --- a/services/web/app/src/Features/User/UserRegistrationHandler.js +++ b/services/web/app/src/Features/User/UserRegistrationHandler.js @@ -35,6 +35,7 @@ const UserRegistrationHandler = { first_name: userDetails.first_name, last_name: userDetails.last_name }, + {}, callback ) } else { diff --git a/services/web/app/src/Features/User/UserUpdater.js b/services/web/app/src/Features/User/UserUpdater.js index 8bb1a91f20..83da39724b 100644 --- a/services/web/app/src/Features/User/UserUpdater.js +++ b/services/web/app/src/Features/User/UserUpdater.js @@ -105,8 +105,13 @@ async function setDefaultEmailAddress( } const UserUpdater = { - addAffiliationForNewUser(userId, email, callback) { - addAffiliation(userId, email, error => { + addAffiliationForNewUser(userId, email, affiliationOptions, callback) { + if (callback == null) { + // affiliationOptions is optional + callback = affiliationOptions + affiliationOptions = {} + } + addAffiliation(userId, email, affiliationOptions, error => { if (error) { return callback(error) } diff --git a/services/web/test/acceptance/src/ProjectStructureMongoLockTest.js b/services/web/test/acceptance/src/ProjectStructureMongoLockTest.js index dd3909def1..028e5d43b8 100644 --- a/services/web/test/acceptance/src/ProjectStructureMongoLockTest.js +++ b/services/web/test/acceptance/src/ProjectStructureMongoLockTest.js @@ -48,7 +48,7 @@ describe('ProjectStructureMongoLock', function() { holdingAccount: false, email: 'test@example.com' } - UserCreator.createNewUser(userDetails, (err, user) => { + UserCreator.createNewUser(userDetails, {}, (err, user) => { this.user = user if (err != null) { throw err diff --git a/services/web/test/unit/src/User/UserCreatorTests.js b/services/web/test/unit/src/User/UserCreatorTests.js index 35d1192746..e53a442ce4 100644 --- a/services/web/test/unit/src/User/UserCreatorTests.js +++ b/services/web/test/unit/src/User/UserCreatorTests.js @@ -23,13 +23,18 @@ describe('UserCreator', function() { '../../models/User': { User: this.UserModel }, - 'logger-sharelatex': { + 'logger-sharelatex': (this.Logger = { error: sinon.stub() - }, + }), 'metrics-sharelatex': { timeAsyncMethod() {} }, '../../infrastructure/Features': (this.Features = { hasFeature: sinon.stub().returns(false) }), + './UserDeleter': (this.UserDeleter = { + promises: { + deleteNewUser: sinon.stub().resolves() + } + }), './UserGetter': (this.UserGetter = { promises: { getUser: sinon.stub().resolves(this.user) @@ -51,79 +56,60 @@ describe('UserCreator', function() { describe('createNewUser', function() { describe('with callbacks', function() { - it('should take the opts and put them in the model', function(done) { - const opts = { + it('should take the opts and put them in the model', async function() { + const user = await this.UserCreator.promises.createNewUser({ email: this.email, holdingAccount: true - } - this.UserCreator.createNewUser(opts, (err, user) => { - assert.ifError(err) - assert.equal(user.email, this.email) - assert.equal(user.holdingAccount, true) - assert.equal(user.first_name, 'bob.oswald') - done() }) + assert.equal(user.email, this.email) + assert.equal(user.holdingAccount, true) + assert.equal(user.first_name, 'bob.oswald') }) - it('should use the start of the email if the first name is empty string', function(done) { - const opts = { + it('should use the start of the email if the first name is empty string', async function() { + const user = await this.UserCreator.promises.createNewUser({ email: this.email, holdingAccount: true, first_name: '' - } - this.UserCreator.createNewUser(opts, (err, user) => { - assert.ifError(err) - assert.equal(user.email, this.email) - assert.equal(user.holdingAccount, true) - assert.equal(user.first_name, 'bob.oswald') - done() }) + assert.equal(user.email, this.email) + assert.equal(user.holdingAccount, true) + assert.equal(user.first_name, 'bob.oswald') }) - it('should use the first name if passed', function(done) { - const opts = { + it('should use the first name if passed', async function() { + const user = await this.UserCreator.promises.createNewUser({ email: this.email, holdingAccount: true, first_name: 'fiiirstname' - } - this.UserCreator.createNewUser(opts, (err, user) => { - assert.ifError(err) - assert.equal(user.email, this.email) - assert.equal(user.holdingAccount, true) - assert.equal(user.first_name, 'fiiirstname') - done() }) + assert.equal(user.email, this.email) + assert.equal(user.holdingAccount, true) + assert.equal(user.first_name, 'fiiirstname') }) - it('should use the last name if passed', function(done) { - const opts = { + it('should use the last name if passed', async function() { + const user = await this.UserCreator.promises.createNewUser({ email: this.email, holdingAccount: true, last_name: 'lastNammmmeee' - } - this.UserCreator.createNewUser(opts, (err, user) => { - assert.ifError(err) - assert.equal(user.email, this.email) - assert.equal(user.holdingAccount, true) - assert.equal(user.last_name, 'lastNammmmeee') - done() }) + assert.equal(user.email, this.email) + assert.equal(user.holdingAccount, true) + assert.equal(user.last_name, 'lastNammmmeee') }) - it('should set emails attribute', function(done) { - this.UserCreator.createNewUser({ email: this.email }, (err, user) => { - assert.ifError(err) - user.email.should.equal(this.email) - user.emails.length.should.equal(1) - user.emails[0].email.should.equal(this.email) - user.emails[0].createdAt.should.be.a('date') - user.emails[0].reversedHostname.should.equal('moc.liamg') - done() + it('should set emails attribute', async function() { + const user = await this.UserCreator.promises.createNewUser({ + email: this.email }) + user.email.should.equal(this.email) + user.emails.length.should.equal(1) + user.emails[0].email.should.equal(this.email) + user.emails[0].createdAt.should.be.a('date') + user.emails[0].reversedHostname.should.equal('moc.liamg') }) - it('should not add affiliation', function() {}) - describe('with affiliations feature', function() { let attributes, user beforeEach(function() { @@ -133,17 +119,16 @@ describe('UserCreator', function() { .withArgs('affiliations') .returns(true) }) + describe('when v1 affiliations API does not return an error', function() { - beforeEach(function(done) { - this.UserCreator.createNewUser(attributes, (err, createdUser) => { - user = createdUser - assert.ifError(err) - done() - }) + beforeEach(async function() { + user = await this.UserCreator.promises.createNewUser(attributes) }) + it('should flag that affiliation is unchecked', function() { user.emails[0].affiliationUnchecked.should.equal(true) }) + it('should try to add affiliation to v1', function() { sinon.assert.calledOnce( this.UserUpdater.promises.addAffiliationForNewUser @@ -154,22 +139,22 @@ describe('UserCreator', function() { this.email ) }) + it('should query for updated user data', function() { sinon.assert.calledOnce(this.UserGetter.promises.getUser) }) }) + describe('when v1 affiliations API does return an error', function() { - beforeEach(function(done) { + beforeEach(async function() { this.UserUpdater.promises.addAffiliationForNewUser.rejects() - this.UserCreator.createNewUser(attributes, (error, createdUser) => { - user = createdUser - assert.ifError(error) - done() - }) + user = await this.UserCreator.promises.createNewUser(attributes) }) + it('should flag that affiliation is unchecked', function() { user.emails[0].affiliationUnchecked.should.equal(true) }) + it('should try to add affiliation to v1', function() { sinon.assert.calledOnce( this.UserUpdater.promises.addAffiliationForNewUser @@ -180,21 +165,53 @@ describe('UserCreator', function() { this.email ) }) - it('should query for updated user data', function() { - sinon.assert.calledOnce(this.UserGetter.promises.getUser) + + it('should not query for updated user data', function() { + sinon.assert.notCalled(this.UserGetter.promises.getUser) + }) + + it('should log error', function() { + sinon.assert.calledOnce(this.Logger.error) + }) + }) + + describe('when v1 affiliations API returns an error and requireAffiliation=true', function() { + beforeEach(async function() { + this.UserUpdater.promises.addAffiliationForNewUser.rejects() + user = await this.UserCreator.promises.createNewUser(attributes) + }) + + it('should flag that affiliation is unchecked', function() { + user.emails[0].affiliationUnchecked.should.equal(true) + }) + + it('should try to add affiliation to v1', function() { + sinon.assert.calledOnce( + this.UserUpdater.promises.addAffiliationForNewUser + ) + sinon.assert.calledWithMatch( + this.UserUpdater.promises.addAffiliationForNewUser, + user._id, + this.email + ) + }) + + it('should not query for updated user data', function() { + sinon.assert.notCalled(this.UserGetter.promises.getUser) + }) + + it('should log error', function() { + sinon.assert.calledOnce(this.Logger.error) }) }) }) - it('should not add affiliation when without affiliation feature', function(done) { + it('should not add affiliation when without affiliation feature', async function() { const attributes = { email: this.email } - this.UserCreator.createNewUser(attributes, (err, user) => { - assert.ifError(err) - sinon.assert.notCalled( - this.UserUpdater.promises.addAffiliationForNewUser - ) - done() - }) + await this.UserCreator.promises.createNewUser(attributes) + sinon.assert.notCalled( + this.UserUpdater.promises.addAffiliationForNewUser + ) }) }) diff --git a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js index 802fdeb237..8765beb5ea 100644 --- a/services/web/test/unit/src/User/UserRegistrationHandlerTests.js +++ b/services/web/test/unit/src/User/UserRegistrationHandlerTests.js @@ -29,7 +29,7 @@ describe('UserRegistrationHandler', function() { this.User = { update: sinon.stub().callsArgWith(2) } this.UserGetter = { getUserByAnyEmail: sinon.stub() } this.UserCreator = { - createNewUser: sinon.stub().callsArgWith(1, null, this.user) + createNewUser: sinon.stub().callsArgWith(2, null, this.user) } this.AuthenticationManager = { validateEmail: sinon.stub().returns(null),