From 409ce329e23eb3da7d3fa71a926034dc40893c37 Mon Sep 17 00:00:00 2001 From: Dev Singh Date: Mon, 6 Oct 2025 16:39:00 -0500 Subject: [PATCH] Lock users when updating leads --- src/api/functions/organizations.ts | 183 ++++++++++++++++------------- src/api/package.json | 1 + src/api/routes/organizations.ts | 5 +- yarn.lock | 5 + 4 files changed, 112 insertions(+), 82 deletions(-) diff --git a/src/api/functions/organizations.ts b/src/api/functions/organizations.ts index 47adc815..682248cb 100644 --- a/src/api/functions/organizations.ts +++ b/src/api/functions/organizations.ts @@ -23,7 +23,8 @@ import { EntraGroupActions } from "common/types/iam.js"; import { buildAuditLogTransactPut } from "./auditLog.js"; import { Modules } from "common/modules.js"; import { retryDynamoTransactionWithBackoff } from "api/utils.js"; -import { ValidLoggers } from "api/types.js"; +import { Redis, ValidLoggers } from "api/types.js"; +import { createLock, IoredisAdapter, type SimpleLock } from "redlock-universal"; export interface GetOrgInfoInputs { id: string; @@ -184,6 +185,7 @@ export const addLead = async ({ dynamoClient, logger, officersEmail, + redisClient, }: { user: z.infer; orgId: string; @@ -194,6 +196,7 @@ export const addLead = async ({ dynamoClient: DynamoDBClient; logger: FastifyBaseLogger; officersEmail: string; + redisClient: Redis; }): Promise => { const { username } = user; @@ -229,51 +232,60 @@ export const addLead = async ({ return await dynamoClient.send(addTransaction); }; + const lock = createLock({ + adapter: new IoredisAdapter(redisClient), + key: `user:${username}`, + retryAttempts: 5, + retryDelay: 300, + }) as SimpleLock; + return await lock.using(async () => { + try { + await retryDynamoTransactionWithBackoff( + addOperation, + logger, + `Add lead ${username} to ${orgId}`, + ); + } catch (e: any) { + if ( + e.name === "TransactionCanceledException" && + e.message.includes("ConditionalCheckFailed") + ) { + logger.info( + `User ${username} is already a lead for ${orgId}. Skipping add operation.`, + ); + return null; + } + throw e; + } - try { - await retryDynamoTransactionWithBackoff( - addOperation, - logger, - `Add lead ${username} to ${orgId}`, + logger.info( + `Successfully added ${username} as lead for ${orgId} in DynamoDB.`, ); - } catch (e: any) { - if ( - e.name === "TransactionCanceledException" && - e.message.includes("ConditionalCheckFailed") - ) { + + if (entraGroupId) { + await modifyGroup( + entraIdToken, + username, + entraGroupId, + EntraGroupActions.ADD, + dynamoClient, + ); logger.info( - `User ${username} is already a lead for ${orgId}. Skipping add operation.`, + `Successfully added ${username} to Entra group for ${orgId}.`, ); - return null; } - throw e; - } - - logger.info( - `Successfully added ${username} as lead for ${orgId} in DynamoDB.`, - ); - - if (entraGroupId) { - await modifyGroup( - entraIdToken, - username, - entraGroupId, - EntraGroupActions.ADD, - dynamoClient, - ); - logger.info(`Successfully added ${username} to Entra group for ${orgId}.`); - } - return { - function: AvailableSQSFunctions.EmailNotifications, - metadata: { initiator: actorUsername, reqId }, - payload: { - to: getAllUserEmails(username), - cc: [officersEmail], - subject: `Lead added for ${orgId}`, - content: `Hello,\n\nWe're letting you know that ${username} has been added as a lead for ${orgId} by ${actorUsername}. Changes may take up to 2 hours to reflect in all systems.\n\nNo action is required from you at this time.`, - }, - }; + return { + function: AvailableSQSFunctions.EmailNotifications, + metadata: { initiator: actorUsername, reqId }, + payload: { + to: getAllUserEmails(username), + cc: [officersEmail], + subject: `${user.nonVotingMember ? "Non-voting lead" : "Lead"} added for ${orgId}`, + content: `Hello,\n\nWe're letting you know that ${username} has been added as a ${user.nonVotingMember ? "non-voting" : ""} lead for ${orgId} by ${actorUsername}. Changes may take up to 2 hours to reflect in all systems.`, + }, + }; + }); }; export const removeLead = async ({ @@ -286,6 +298,7 @@ export const removeLead = async ({ dynamoClient, logger, officersEmail, + redisClient, }: { username: string; orgId: string; @@ -296,6 +309,7 @@ export const removeLead = async ({ dynamoClient: DynamoDBClient; logger: FastifyBaseLogger; officersEmail: string; + redisClient: Redis; }): Promise => { const removeOperation = async () => { const removeTransaction = new TransactWriteItemsCommand({ @@ -325,52 +339,61 @@ export const removeLead = async ({ return await dynamoClient.send(removeTransaction); }; - try { - await retryDynamoTransactionWithBackoff( - removeOperation, - logger, - `Remove lead ${username} from ${orgId}`, - ); - } catch (e: any) { - if ( - e.name === "TransactionCanceledException" && - e.message.includes("ConditionalCheckFailed") - ) { - logger.info( - `User ${username} was not a lead for ${orgId}. Skipping remove operation.`, + const lock = createLock({ + adapter: new IoredisAdapter(redisClient), + key: `user:${username}`, + retryAttempts: 5, + retryDelay: 300, + }) as SimpleLock; + + return await lock.using(async () => { + try { + await retryDynamoTransactionWithBackoff( + removeOperation, + logger, + `Remove lead ${username} from ${orgId}`, ); - return null; + } catch (e: any) { + if ( + e.name === "TransactionCanceledException" && + e.message.includes("ConditionalCheckFailed") + ) { + logger.info( + `User ${username} was not a lead for ${orgId}. Skipping remove operation.`, + ); + return null; + } + throw e; } - throw e; - } - - logger.info( - `Successfully removed ${username} as lead for ${orgId} in DynamoDB.`, - ); - if (entraGroupId) { - await modifyGroup( - entraIdToken, - username, - entraGroupId, - EntraGroupActions.REMOVE, - dynamoClient, - ); logger.info( - `Successfully removed ${username} from Entra group for ${orgId}.`, + `Successfully removed ${username} as lead for ${orgId} in DynamoDB.`, ); - } - return { - function: AvailableSQSFunctions.EmailNotifications, - metadata: { initiator: actorUsername, reqId }, - payload: { - to: getAllUserEmails(username), - cc: [officersEmail], - subject: `Lead removed for ${orgId}`, - content: `Hello,\n\nWe're letting you know that ${username} has been removed as a lead for ${orgId} by ${actorUsername}.\n\nNo action is required from you at this time.`, - }, - }; + if (entraGroupId) { + await modifyGroup( + entraIdToken, + username, + entraGroupId, + EntraGroupActions.REMOVE, + dynamoClient, + ); + logger.info( + `Successfully removed ${username} from Entra group for ${orgId}.`, + ); + } + + return { + function: AvailableSQSFunctions.EmailNotifications, + metadata: { initiator: actorUsername, reqId }, + payload: { + to: getAllUserEmails(username), + cc: [officersEmail], + subject: `Lead removed for ${orgId}`, + content: `Hello,\n\nWe're letting you know that ${username} has been removed as a lead for ${orgId} by ${actorUsername}.\n\nNo action is required from you at this time.`, + }, + }; + }); }; /** diff --git a/src/api/package.json b/src/api/package.json index 9ec35860..3f59adde 100644 --- a/src/api/package.json +++ b/src/api/package.json @@ -57,6 +57,7 @@ "pino": "^9.6.0", "pluralize": "^8.0.0", "qrcode": "^1.5.4", + "redlock-universal": "^0.6.0", "sanitize-html": "^2.17.0", "stripe": "^18.0.0", "uuid": "^11.1.0", diff --git a/src/api/routes/organizations.ts b/src/api/routes/organizations.ts index 58ce9081..6798db25 100644 --- a/src/api/routes/organizations.ts +++ b/src/api/routes/organizations.ts @@ -580,6 +580,7 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => { dynamoClient: fastify.dynamoClient, logger: request.log, officersEmail, + redisClient: fastify.redisClient, }; const addPromises = add.map((user) => addLead({ ...commonArgs, user })); @@ -639,8 +640,8 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => { orgName: fastify.environmentConfig.GithubOrgName, }); } else { - request.log.info( - "IDP sync is disabled in this environment - the newly created group will have no members!", + request.log.warn( + "IdP sync is disabled in this environment - the newly created group will have no members!", ); } diff --git a/yarn.lock b/yarn.lock index 6176d58b..99e2c968 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8859,6 +8859,11 @@ redis-parser@^3.0.0: dependencies: redis-errors "^1.0.0" +redlock-universal@^0.6.0: + version "0.6.0" + resolved "https://registry.yarnpkg.com/redlock-universal/-/redlock-universal-0.6.0.tgz#b67d88fc36c5250c91a6beca02de7efd611aa90f" + integrity sha512-UrxoupAzkLBZevbzbec9hotqNwytJoO8g7NdXan5hBLjODmMW/5TE5joY3voKN1er14UnOi/tz/I5LkPpmillA== + reflect.getprototypeof@^1.0.6, reflect.getprototypeof@^1.0.9: version "1.0.10" resolved "https://registry.yarnpkg.com/reflect.getprototypeof/-/reflect.getprototypeof-1.0.10.tgz#c629219e78a3316d8b604c765ef68996964e7bf9"