Merge pull request #2219 from overleaf/jel-institution-email-already-exists

Check for email before adding institution identifier

GitOrigin-RevId: 71f498116c8c8df2c3c1a33eafce3e56c87e7ee0

services/web/app/src/Features/User/SAMLIdentityManager.js

@@ -6,7 +6,19 @@ const { User } = require('../../models/User')
 const UserGetter = require('../User/UserGetter')
 const UserUpdater = require('../User/UserUpdater')
 
-function _addIdentifier(userId, externalUserId, providerId, hasEntitlement) {
+async function _addIdentifier(
+  userId,
+  externalUserId,
+  providerId,
+  hasEntitlement,
+  institutionEmail
+) {
+  // first check if institutionEmail linked to another account
+  // before adding the identifier for the email
+  const user = await UserGetter.promises.getUserByAnyEmail(institutionEmail)
+  if (user && user._id.toString() !== userId.toString()) {
+    throw new Errors.EmailExistsError()
+  }
   providerId = providerId.toString()
   hasEntitlement = !!hasEntitlement
   const query = {
@@ -137,7 +149,13 @@ async function linkAccounts(
   providerName,
   hasEntitlement
 ) {
-  await _addIdentifier(userId, externalUserId, providerId, hasEntitlement)
+  await _addIdentifier(
+    userId,
+    externalUserId,
+    providerId,
+    hasEntitlement,
+    institutionEmail
+  )
   await _addInstitutionEmail(userId, institutionEmail, providerId)
   await _sendLinkedEmail(userId, providerName)
 }

services/web/test/unit/src/User/SAMLIdentityManagerTests.js

@@ -7,6 +7,7 @@ const modulePath = '../../../../app/src/Features/User/SAMLIdentityManager.js'
 describe('SAMLIdentityManager', function() {
   beforeEach(function() {
     this.Errors = {
+      EmailExistsError: sinon.stub(),
       NotFoundError: sinon.stub(),
       SAMLIdentityExistsError: sinon.stub(),
       SAMLUserNotFoundError: sinon.stub()
@@ -35,6 +36,7 @@ describe('SAMLIdentityManager', function() {
         '../Errors/Errors': this.Errors,
         '../../models/User': {
           User: (this.User = {
+            findOneAndUpdate: sinon.stub(),
             findOne: sinon.stub(this.user),
             update: sinon.stub().returns({
               exec: sinon.stub().resolves()
@@ -44,7 +46,8 @@ describe('SAMLIdentityManager', function() {
         '../User/UserGetter': (this.UserGetter = {
           getUser: sinon.stub(),
           promises: {
-            getUser: sinon.stub().resolves(this.user)
+            getUser: sinon.stub().resolves(this.user),
+            getUserByAnyEmail: sinon.stub().resolves()
           }
         }),
         '../User/UserUpdater': (this.UserUpdater = {
@@ -76,6 +79,30 @@ describe('SAMLIdentityManager', function() {
         expect(error).to.exist
       }
     })
+    describe('when email is already associated with another Overleaf account', function() {
+      beforeEach(function() {
+        this.UserGetter.promises.getUserByAnyEmail.resolves({
+          user_id: 'user-id-2'
+        })
+      })
+      it('should throw an error if email is associated with another account', async function() {
+        let error
+        try {
+          await this.SAMLIdentityManager.linkAccounts(
+            'user-id-1',
+            '2',
+            'overleaf',
+            true,
+            'test@overleaf.com'
+          )
+        } catch (e) {
+          error = e
+        } finally {
+          expect(error).to.exist
+          expect(this.User.findOneAndUpdate).to.not.have.been.called
+        }
+      })
+    })
   })
   describe('unlinkAccounts', function() {
     it('should send an email notification email', function() {