From 81c5abcbd43555c86bd3080ba54fa1dd593893be Mon Sep 17 00:00:00 2001 From: Aidan Lakshman Date: Fri, 14 May 2021 15:57:10 -0400 Subject: [PATCH] bulkUserAdd API bugfix (#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 --- .../lib/user/__tests__/user-service.test.js | 60 +++++++++++++++- .../lib/user/user-service.js | 68 +++++++++++++++---- .../api-tests/users/bulk-add-users.test.js | 13 +++- openapi.yaml | 2 +- 4 files changed, 126 insertions(+), 17 deletions(-) diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/user/__tests__/user-service.test.js b/addons/addon-base-raas/packages/base-raas-services/lib/user/__tests__/user-service.test.js index 47d5dc69be..9ea50aeb1b 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/user/__tests__/user-service.test.js +++ b/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 + } + }); }); }); diff --git a/addons/addon-base-raas/packages/base-raas-services/lib/user/user-service.js b/addons/addon-base-raas/packages/base-raas-services/lib/user/user-service.js index d9d6774ad1..d64ef5d972 100644 --- a/addons/addon-base-raas/packages/base-raas-services/lib/user/user-service.js +++ b/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,7 +68,9 @@ 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, @@ -73,13 +78,30 @@ class UserService extends BaseUserService { }); 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 }; } diff --git a/main/integration-tests/__test__/api-tests/users/bulk-add-users.test.js b/main/integration-tests/__test__/api-tests/users/bulk-add-users.test.js index b9901bd60c..2519e7cbb2 100644 --- a/main/integration-tests/__test__/api-tests/users/bulk-add-users.test.js +++ b/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, }); }); }); diff --git a/openapi.yaml b/openapi.yaml index a0f0ae4228..212df37bfb 100644 --- a/openapi.yaml +++ b/openapi.yaml @@ -3062,7 +3062,7 @@ components: - pending - error - reachable - - * + - "*" description: id must be * when status is specified RegisterStudy: type: object