From a742a850c3a903433fa3801688bd2af5f3cbe0f8 Mon Sep 17 00:00:00 2001 From: amazon-meaisiah Date: Fri, 5 Jun 2020 13:49:05 -0700 Subject: [PATCH 1/3] Fix admin promotion to actually be idempotent --- .../dev-portal-common/customers-controller.js | 59 +++-- .../shared/__tests__/customers-controller.js | 234 +++++++++++++++++- 2 files changed, 270 insertions(+), 23 deletions(-) diff --git a/lambdas/common-layer/nodejs/node_modules/dev-portal-common/customers-controller.js b/lambdas/common-layer/nodejs/node_modules/dev-portal-common/customers-controller.js index 5884e5aa7..cabb16a04 100644 --- a/lambdas/common-layer/nodejs/node_modules/dev-portal-common/customers-controller.js +++ b/lambdas/common-layer/nodejs/node_modules/dev-portal-common/customers-controller.js @@ -1053,6 +1053,17 @@ const getEmailForUserSub = async userSub => { return found.Users[0].Attributes[0].Value } +async function __isUserAnAdmin (targetUserId) { + const result = await cognitoIdp + .adminListGroupsForUser({ + Username: targetUserId, + UserPoolId: getEnv('UserPoolId') + }) + .promise() + + return result.Groups.some(g => g.GroupName === getEnv('AdminsGroupName')) +} + const __recordPromotionInDdb = async ({ targetUserId, promoterUserId, @@ -1076,29 +1087,33 @@ const __recordPromotionInDdb = async ({ } const addAccountToAdminsGroup = async ({ targetUserId, promoterUserSub, promoterUserId }) => { - console.log(`Adding account with userId=[${targetUserId}] to AdminsGroup`) - try { - await Promise.all([ - getEmailForUserSub(promoterUserSub).then(promoterEmailAddress => - __recordPromotionInDdb({ - targetUserId, - promoterUserId, - promoterEmailAddress - }) - ), - cognitoIdp - .adminAddUserToGroup({ - GroupName: getEnv('AdminsGroupName'), - UserPoolId: getEnv('UserPoolId'), - Username: targetUserId - }) - .promise() - ]) - } catch (error) { - console.error('Failed to add account to AdminsGroup:', error) - throw error + console.log(`Checking if account with userId=[${targetUserId}] is in AdminsGroup`) + + if (await __isUserAnAdmin(targetUserId)) { + console.log(`Account with userId=[${targetUserId}] is in AdminsGroup, skipping`) + return } - console.log('Adding account to AdminsGroup') + + console.log(`Adding account with userId=[${targetUserId}] to AdminsGroup`) + + await Promise.all([ + getEmailForUserSub(promoterUserSub).then(promoterEmailAddress => + __recordPromotionInDdb({ + targetUserId, + promoterUserId, + promoterEmailAddress + }) + ), + cognitoIdp + .adminAddUserToGroup({ + GroupName: getEnv('AdminsGroupName'), + UserPoolId: getEnv('UserPoolId'), + Username: targetUserId + }) + .promise() + ]) + + console.log('Added account to AdminsGroup') } const createAccountInvite = async ({ targetEmailAddress, inviterUserSub, inviterUserId }) => { diff --git a/lambdas/shared/__tests__/customers-controller.js b/lambdas/shared/__tests__/customers-controller.js index 0b91e830c..12f79bbfb 100644 --- a/lambdas/shared/__tests__/customers-controller.js +++ b/lambdas/shared/__tests__/customers-controller.js @@ -1,6 +1,6 @@ const customers = require('dev-portal-common/customers-controller') const pager = require('dev-portal-common/pager') -const { promiser, bindEnv } = require('../../setup-jest') +const { promiser, bindEnv, bindMock } = require('../../setup-jest') describe('customersController', () => { const setEnv = bindEnv() @@ -276,4 +276,236 @@ describe('customersController', () => { expect(pager.fetchApiGatewayApiKeys).toHaveBeenCalledTimes(0) }) }) + + describe('addAccountToAdminsGroup()', () => { + const setEnv = bindEnv() + const setMock = bindMock() + + const isoDateRegexp = /^\d{4,}-\d\d-\d\dT\d\d:\d\d:\d\d\.\d{2,}Z$/ + + test('promotes an existing non-admin account in pre-login table to admin', async () => { + setEnv('UserPoolId', 'user-pool-id') + setEnv('PreLoginAccountsTableName', 'user-bucket-id') + setEnv('CustomersTableName', 'TestCustomersTable') + setEnv('AdminsGroupName', 'TestAdminGroup') + + const promoterName = 'promoter@example.com' + const promoterId = '98765432-9876-5432-1098-987654321098' + const promoterSub = 'a1b2c3d4-a1b2-c3d4-e5f6-a1b2c3d4e5f6' + const userId = '12345678-1234-5678-9abc-123456789abc' + + setMock(customers.cognitoIdp, 'adminListGroupsForUser').mockReturnValue(promiser({ + Groups: [{ GroupName: 'TestUserGroup' }] + })) + setMock(customers.cognitoIdp, 'listUsers').mockReturnValue(promiser({ + Users: [ + { Attributes: [{ Name: 'email', Value: promoterName }] } + ] + })) + setMock(customers.dynamoDb, 'get', opts => { + if (opts.TableName === 'user-bucket-id') { + return promiser({ Item: { IdentityId: userId } }) + } + return promiser({ Item: null }) + }) + setMock(customers.dynamoDb, 'scan').mockReturnValue(promiser({ Items: [] })) + setMock(customers.dynamoDb, 'put').mockReturnValue(promiser('updated account')) + setMock(customers.cognitoIdp, 'adminAddUserToGroup') + .mockReturnValue(promiser('updated user')) + + await customers.addAccountToAdminsGroup({ + targetUserId: userId, + promoterUserId: promoterId, + promoterUserSub: promoterSub + }) + + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledWith( + expect.objectContaining({ + Username: userId, + UserPoolId: 'user-pool-id' + }) + ) + + expect(customers.cognitoIdp.listUsers).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.listUsers).toHaveBeenCalledWith( + expect.objectContaining({ + Filter: `sub = "${promoterSub}"`, + Limit: 1, + AttributesToGet: ['email'], + UserPoolId: 'user-pool-id' + }) + ) + + expect(customers.dynamoDb.get).toHaveBeenCalledTimes(1) + expect(customers.dynamoDb.get).toHaveBeenCalledWith( + expect.objectContaining({ + TableName: 'user-bucket-id', + Key: { UserId: userId } + }) + ) + + expect(customers.dynamoDb.scan).toHaveBeenCalledTimes(0) + + expect(customers.dynamoDb.put).toHaveBeenCalledTimes(1) + expect(customers.dynamoDb.put).toHaveBeenCalledWith( + expect.objectContaining({ + TableName: 'user-bucket-id', + Item: expect.objectContaining({ + IdentityId: userId, + PromoterUserId: promoterId, + PromoterEmailAddress: promoterName, + DatePromoted: expect.stringMatching(isoDateRegexp) + }), + ConditionExpression: 'attribute_exists(UserId)' + }) + ) + + expect(customers.cognitoIdp.adminAddUserToGroup).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.adminAddUserToGroup).toHaveBeenCalledWith( + expect.objectContaining({ + GroupName: 'TestAdminGroup', + UserPoolId: 'user-pool-id', + Username: userId + }) + ) + }) + + test('promotes an existing non-admin account in customers table to admin', async () => { + setEnv('UserPoolId', 'user-pool-id') + setEnv('PreLoginAccountsTableName', 'user-bucket-id') + setEnv('CustomersTableName', 'TestCustomersTable') + setEnv('AdminsGroupName', 'TestAdminGroup') + + const promoterName = 'promoter@example.com' + const promoterId = '98765432-9876-5432-1098-987654321098' + const promoterSub = 'a1b2c3d4-a1b2-c3d4-e5f6-a1b2c3d4e5f6' + const userId = '12345678-1234-5678-9abc-123456789abc' + + setMock(customers.cognitoIdp, 'adminListGroupsForUser').mockReturnValue(promiser({ + Groups: [{ GroupName: 'TestUserGroup' }] + })) + setMock(customers.cognitoIdp, 'listUsers').mockReturnValue(promiser({ + Users: [ + { Attributes: [{ Name: 'email', Value: promoterName }] } + ] + })) + setMock(customers.dynamoDb, 'get', opts => { + if (opts.TableName === 'TestCustomersTable') { + return promiser({ Item: { IdentityId: userId } }) + } + return promiser({ Item: null }) + }) + setMock(customers.dynamoDb, 'scan').mockReturnValue(promiser({ + Items: [{ Id: userId }] + })) + setMock(customers.dynamoDb, 'put').mockReturnValue(promiser('updated account')) + setMock(customers.cognitoIdp, 'adminAddUserToGroup') + .mockReturnValue(promiser('updated user')) + + await customers.addAccountToAdminsGroup({ + targetUserId: userId, + promoterUserId: promoterId, + promoterUserSub: promoterSub + }) + + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledWith( + expect.objectContaining({ + Username: userId, + UserPoolId: 'user-pool-id' + }) + ) + + expect(customers.cognitoIdp.listUsers).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.listUsers).toHaveBeenCalledWith( + expect.objectContaining({ + Filter: `sub = "${promoterSub}"`, + Limit: 1, + AttributesToGet: ['email'], + UserPoolId: 'user-pool-id' + }) + ) + + expect(customers.dynamoDb.get).toHaveBeenCalledTimes(1) + expect(customers.dynamoDb.get).toHaveBeenCalledWith( + expect.objectContaining({ + TableName: 'user-bucket-id', + Key: { UserId: userId } + }) + ) + + expect(customers.dynamoDb.scan).toHaveBeenCalledTimes(1) + expect(customers.dynamoDb.scan).toHaveBeenCalledWith( + expect.objectContaining({ + TableName: 'TestCustomersTable', + FilterExpression: 'UserPoolId = :userId', + ExpressionAttributeValues: { ':userId': userId } + }) + ) + + expect(customers.dynamoDb.put).toHaveBeenCalledTimes(1) + expect(customers.dynamoDb.put).toHaveBeenCalledWith( + expect.objectContaining({ + TableName: 'TestCustomersTable', + Item: expect.objectContaining({ + Id: userId, + PromoterUserId: promoterId, + PromoterEmailAddress: promoterName, + DatePromoted: expect.stringMatching(isoDateRegexp) + }), + ConditionExpression: 'attribute_exists(Id)' + }) + ) + + expect(customers.cognitoIdp.adminAddUserToGroup).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.adminAddUserToGroup).toHaveBeenCalledWith( + expect.objectContaining({ + GroupName: 'TestAdminGroup', + UserPoolId: 'user-pool-id', + Username: userId + }) + ) + }) + + test('does not re-promote an existing admin account', async () => { + setEnv('UserPoolId', 'user-pool-id') + setEnv('PreLoginAccountsTableName', 'user-bucket-id') + setEnv('CustomersTableName', 'TestCustomersTable') + setEnv('AdminsGroupName', 'TestAdminGroup') + + const promoterId = '98765432-9876-5432-1098-987654321098' + const promoterSub = 'a1b2c3d4-a1b2-c3d4-e5f6-a1b2c3d4e5f6' + const userId = '12345678-1234-5678-9abc-123456789abc' + + setMock(customers.cognitoIdp, 'adminListGroupsForUser').mockReturnValue(promiser({ + Groups: [{ GroupName: 'TestUserGroup' }, { GroupName: 'TestAdminGroup' }] + })) + setMock(customers.cognitoIdp, 'listUsers') + setMock(customers.dynamoDb, 'get') + setMock(customers.dynamoDb, 'scan') + setMock(customers.dynamoDb, 'put') + setMock(customers.cognitoIdp, 'adminAddUserToGroup') + + await customers.addAccountToAdminsGroup({ + targetUserId: userId, + promoterUserId: promoterId, + promoterUserSub: promoterSub + }) + + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledTimes(1) + expect(customers.cognitoIdp.adminListGroupsForUser).toHaveBeenCalledWith( + expect.objectContaining({ + Username: userId, + UserPoolId: 'user-pool-id' + }) + ) + + expect(customers.cognitoIdp.listUsers).toHaveBeenCalledTimes(0) + expect(customers.dynamoDb.get).toHaveBeenCalledTimes(0) + expect(customers.dynamoDb.scan).toHaveBeenCalledTimes(0) + expect(customers.dynamoDb.put).toHaveBeenCalledTimes(0) + expect(customers.cognitoIdp.adminAddUserToGroup).toHaveBeenCalledTimes(0) + }) + }) }) From 2b524d2b499cdbd69656715f17acadeb35bf5890 Mon Sep 17 00:00:00 2001 From: amazon-meaisiah Date: Fri, 1 May 2020 12:16:23 -0700 Subject: [PATCH 2/3] Fix typo in feedback --- dev-portal/src/components/Feedback.jsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dev-portal/src/components/Feedback.jsx b/dev-portal/src/components/Feedback.jsx index 1cd0343d6..ac17ff482 100644 --- a/dev-portal/src/components/Feedback.jsx +++ b/dev-portal/src/components/Feedback.jsx @@ -62,7 +62,7 @@ class Feedback extends React.PureComponent { borderTopLeftRadius: '0', borderTopRightRadius: '0' }} onClick={this.handleOpenModal} - >Got an opinon? + >Got an opinion? } From b177a9e0517bc2a27d469a67979b6668843110bf Mon Sep 17 00:00:00 2001 From: amazon-meaisiah Date: Mon, 8 Jun 2020 11:05:18 -0700 Subject: [PATCH 3/3] Forgot some line breaks. --- dev-portal/example-deployer.config.js | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/dev-portal/example-deployer.config.js b/dev-portal/example-deployer.config.js index 473c37d00..79116fbdc 100644 --- a/dev-portal/example-deployer.config.js +++ b/dev-portal/example-deployer.config.js @@ -1,8 +1,13 @@ // Here's how you set this up: // -// 1. Replace YOUR_LAMBDA_ARTIFACTS_BUCKET_NAME with the name of the bucket you created in step 3 of the dev setup. -// 2. Replace 'CUSTOM_PREFIX' in the properties that have it with your name, your org name, or some other unique identifier. For the S3 buckets and the Cognito user pool domain prefix, they must be globally unique. For the CloudFormation stack name, it need only be unique to all stacks deployed to your account. -// 3. Set any other optional parameters as desired. For the DynamoDB tables, their names must be unique to all DynamoDB tables within your account. +// 1. Replace YOUR_LAMBDA_ARTIFACTS_BUCKET_NAME with the name of the bucket you created in step 3 +// of the dev setup. +// 2. Replace 'CUSTOM_PREFIX' in the properties that have it with your name, your org name, or some +// other unique identifier. For the S3 buckets and the Cognito user pool domain prefix, they +// must be globally unique. For the CloudFormation stack name, it need only be unique to all +// stacks deployed to your account. +// 3. Set any other optional parameters as desired. For the DynamoDB tables, their names must be +// unique to all DynamoDB tables within your account. // 4. Save the file. // // Note: these configuration parameters are *not* the same as the SAM template parameters - the names differ and the behavior in many areas also differ. Furthermore, some SAM template parameters like `StaticAssetsRebuildToken` are handled automatically internally and cannot be configured.