bulkUserAdd API bugfix (awslabs#490)

* fix: bug in openapi.yaml introduced in previous PR with unescaped asterisk causing malformed YAML file
* fix: adds validation checks and more informative error messages for bulk-add-users API call.
* fix: added unit and integration tests for changes

addons/addon-base-raas/packages/base-raas-services/lib/user/__tests__/user-service.test.js

@@ -159,7 +159,7 @@ describe('UserService', () => {
       }
     });
 
-    it('should fail because a duplicate user exists', async () => {
+    it('should fail without calling createUser because a duplicate user exists', async () => {
       // BUILD
       const user1 = {
         email: 'example@amazon.com',
@@ -173,6 +173,10 @@ describe('UserService', () => {
         lastName: 'Guy',
       };
 
+      const user3 = {
+        email: 'othervaliduser@amazon.com',
+      };
+
       service.toUserType = jest.fn(() => {
         return { userType: 'root' };
       });
@@ -183,13 +187,16 @@ describe('UserService', () => {
 
       // OPERATE
       try {
-        await service.createUsers({}, [user2], 'internal');
+        await service.createUsers({}, [user2, user3], 'internal');
         expect.hasAssertions();
       } catch (err) {
         // CHECK
+        expect(err.code).toEqual('badRequest');
         expect(err.payload).toBeDefined();
         const error = err.payload[0];
         expect(error).toEqual('Error creating user example@amazon.com. Cannot add user. The user already exists.');
+        expect(service.createUser).not.toHaveBeenCalled();
+        // If duplicate user exists, a badRequest error should be thrown, and no users should be added (even if not all are invalid)
       }
     });
 
@@ -256,5 +263,54 @@ describe('UserService', () => {
       // CHECK
       expect(service.createUser).toHaveBeenCalledTimes(4);
     });
+
+    it('should throw internalError and log audit event appropriately if an internal error occurs', async () => {
+      // BUILD
+      const user1 = {
+        email: 'example1@amazon.com',
+      };
+
+      const user2 = {
+        email: 'example2@amazon.com',
+      };
+
+      const expectedAuditCall = {
+        action: 'create-users-batch',
+        body: {
+          totalUsers: 2,
+          completedSuccessfully: false,
+          numErrors: 1,
+          numSuccesses: 1,
+        },
+      };
+
+      service.toUserType = jest.fn(() => {
+        return { userType: 'root' };
+      });
+      service.getUserByPrincipal = jest.fn(() => {
+        return null;
+      });
+      service.audit = jest.fn();
+
+      service.createUser = jest.fn().mockImplementationOnce(() => {
+        throw new Error('unexpected error!');
+      });
+
+      // OPERATE
+      try {
+        await service.createUsers({}, [user1, user2], 'internal');
+        expect.hasAssertions();
+      } catch (err) {
+        // CHECK
+        expect(err.code).toEqual('internalError');
+        expect(err.payload).toBeDefined();
+        const error = err.payload[0];
+        expect(error).toEqual('Error creating user example1@amazon.com');
+        expect(service.createUser).toHaveBeenCalledTimes(2);
+        expect(service.audit).toHaveBeenCalledWith({}, expectedAuditCall);
+        // If we get an error after validation, internalError should be thrown
+        // Audit event should be logged with details on the failed call
+      }
+    });
   });
 });

addons/addon-base-raas/packages/base-raas-services/lib/user/user-service.js

@@ -42,9 +42,12 @@ class UserService extends BaseUserService {
     await this.assertAuthorized(requestContext, { action: 'createBulk' });
 
     const errors = [];
+    const validationErrors = [];
+    const usersToCreate = [];
     let successCount = 0;
     let errorCount = 0;
-    const createUser = async curUser => {
+
+    const validateUser = async curUser => {
       try {
         const isAdmin = curUser.isAdmin === true;
         const authenticationProviderId =
@@ -65,21 +68,40 @@ class UserService extends BaseUserService {
           status: 'active',
           isExternalUser: userType === 'EXTERNAL',
         };
-        if (!_.isEmpty(userToCreate.email)) {
+
+        // Check if user already exists
+        if (!_.isEmpty(curUser.email)) {
           const user = await this.findUserByPrincipal({
             username: userToCreate.username,
             authenticationProviderId: userToCreate.authenticationProviderId,
             identityProviderName: userToCreate.identityProviderName,
           });
           if (user) {
             throw this.boom.alreadyExists('Cannot add user. The user already exists.', true);
-          } else {
-            await this.createUser(requestContext, userToCreate);
-            successCount += 1;
           }
         }
+
+        // Validate user data
+        await this.validateCreateUser(requestContext, userToCreate);
+
+        usersToCreate.push(userToCreate);
       } catch (e) {
-        const errorMsg = e.safe // if error is boom error then see if it is safe to propagate it's message
+        const errorMsg = e.safe // if error is boom error then see if it is safe to propagate its message
+          ? `Error creating user ${curUser.email}. ${e.message}`
+          : `Error creating user ${curUser.email}`;
+
+        this.log.error(errorMsg);
+        this.log.error(e);
+        validationErrors.push(errorMsg);
+      }
+    };
+
+    const createUser = async curUser => {
+      try {
+        await this.createUser(requestContext, curUser);
+        successCount += 1;
+      } catch (e) {
+        const errorMsg = e.safe // if error is boom error then see if it is safe to propagate its message
           ? `Error creating user ${curUser.email}. ${e.message}`
           : `Error creating user ${curUser.email}`;
 
@@ -90,15 +112,37 @@ class UserService extends BaseUserService {
         errorCount += 1;
       }
     };
+    // Check to make sure users to create don't already exist (if so, exit early with badRequest)
+    await processInBatches(users, batchSize, validateUser);
+    if (!_.isEmpty(validationErrors)) {
+      throw this.boom
+        .badRequest(`Error: Some entries have validation errors. No users were added.`, true)
+        .withPayload(validationErrors);
+    }
+
     // Create users in parallel in the specified batches
-    await processInBatches(users, batchSize, createUser);
+    await processInBatches(usersToCreate, batchSize, createUser);
     if (!_.isEmpty(errors)) {
-      throw this.boom.internalError(`Errors creating users in bulk`, true).withPayload(errors);
+      // Write audit event before throwing error since some users were still added
+      await this.audit(requestContext, {
+        action: 'create-users-batch',
+        body: {
+          totalUsers: _.size(users),
+          completedSuccessfully: false,
+          numErrors: errorCount,
+          numSuccesses: successCount,
+        },
+      });
+      throw this.boom
+        .internalError(`Errors creating users in bulk. Check the payload for more details.`, true)
+        .withPayload(errors);
+    } else {
+      // Write audit event
+      await this.audit(requestContext, {
+        action: 'create-users-batch',
+        body: { totalUsers: _.size(users), completedSuccessfully: true },
+      });
     }
-
-    // Write audit event
-    await this.audit(requestContext, { action: 'create-users-batch', body: { totalUsers: _.size(users) } });
-
     return { successCount, errorCount };
   }
 

main/integration-tests/__test__/api-tests/users/bulk-add-users.test.js

@@ -57,11 +57,20 @@ describe('Create user scenarios', () => {
       });
     });
 
-    it('should fail for adding user that already exist', async () => {
+    it('should fail with badRequest code for adding users that already exist', async () => {
       const admin1Session = await setup.createAdminSession();
       const newUser = admin1Session.resources.users.defaults();
       await expect(admin1Session.resources.users.bulkAddUsers([defaultUser, newUser])).rejects.toMatchObject({
-        code: errorCode.http.code.internalError,
+        code: errorCode.http.code.badRequest,
+      });
+    });
+
+    it('should fail with badRequest code for adding malformed users', async () => {
+      const admin1Session = await setup.createAdminSession();
+      const badUser = {};
+      const newUser = admin1Session.resources.users.defaults();
+      await expect(admin1Session.resources.users.bulkAddUsers([newUser, badUser])).rejects.toMatchObject({
+        code: errorCode.http.code.badRequest,
       });
     });
   });

openapi.yaml

@@ -3062,7 +3062,7 @@ components:
             - pending
             - error
             - reachable
-            - *
+            - "*"
           description: id must be * when status is specified
     RegisterStudy:
       type: object