Merge pull request #2957 from overleaf/ew-validate-saml-email

Validate saml email before register

GitOrigin-RevId: 6dcf3bccd280abd7bd3ced2d4fd2f69c590f74c1

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
 }

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

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

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 {

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)
       }

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

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
+        )
       })
     })
 

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),