Skip to content

Commit 9757781

Browse files
devksingh4devksingh4
authored
Lock user entries when updating leads (#319)
1 parent f11cf9d commit 9757781

File tree

4 files changed

+112
-82
lines changed

4 files changed

+112
-82
lines changed

‎src/api/functions/organizations.ts‎

Lines changed: 103 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,8 @@ import { EntraGroupActions } from "common/types/iam.js";
2323
import { buildAuditLogTransactPut } from "./auditLog.js";
2424
import { Modules } from "common/modules.js";
2525
import { retryDynamoTransactionWithBackoff } from "api/utils.js";
26-
import { ValidLoggers } from "api/types.js";
26+
import { Redis, ValidLoggers } from "api/types.js";
27+
import { createLock, IoredisAdapter, type SimpleLock } from "redlock-universal";
2728

2829
export interface GetOrgInfoInputs {
2930
id: string;
@@ -184,6 +185,7 @@ export const addLead = async ({
184185
dynamoClient,
185186
logger,
186187
officersEmail,
188+
redisClient,
187189
}: {
188190
user: z.infer<typeof enforcedOrgLeadEntry>;
189191
orgId: string;
@@ -194,6 +196,7 @@ export const addLead = async ({
194196
dynamoClient: DynamoDBClient;
195197
logger: FastifyBaseLogger;
196198
officersEmail: string;
199+
redisClient: Redis;
197200
}): Promise<SQSMessage | null> => {
198201
const { username } = user;
199202

@@ -229,51 +232,60 @@ export const addLead = async ({
229232

230233
return await dynamoClient.send(addTransaction);
231234
};
235+
const lock = createLock({
236+
adapter: new IoredisAdapter(redisClient),
237+
key: `user:${username}`,
238+
retryAttempts: 5,
239+
retryDelay: 300,
240+
}) as SimpleLock;
241+
return await lock.using(async () => {
242+
try {
243+
await retryDynamoTransactionWithBackoff(
244+
addOperation,
245+
logger,
246+
`Add lead ${username} to ${orgId}`,
247+
);
248+
} catch (e: any) {
249+
if (
250+
e.name === "TransactionCanceledException" &&
251+
e.message.includes("ConditionalCheckFailed")
252+
) {
253+
logger.info(
254+
`User ${username} is already a lead for ${orgId}. Skipping add operation.`,
255+
);
256+
return null;
257+
}
258+
throw e;
259+
}
232260

233-
try {
234-
await retryDynamoTransactionWithBackoff(
235-
addOperation,
236-
logger,
237-
`Add lead ${username} to ${orgId}`,
261+
logger.info(
262+
`Successfully added ${username} as lead for ${orgId} in DynamoDB.`,
238263
);
239-
} catch (e: any) {
240-
if (
241-
e.name === "TransactionCanceledException" &&
242-
e.message.includes("ConditionalCheckFailed")
243-
) {
264+
265+
if (entraGroupId) {
266+
await modifyGroup(
267+
entraIdToken,
268+
username,
269+
entraGroupId,
270+
EntraGroupActions.ADD,
271+
dynamoClient,
272+
);
244273
logger.info(
245-
`User ${username} is already a lead for ${orgId}. Skipping add operation.`,
274+
`Successfully added ${username} to Entra group for ${orgId}.`,
246275
);
247-
return null;
248276
}
249-
throw e;
250-
}
251-
252-
logger.info(
253-
`Successfully added ${username} as lead for ${orgId} in DynamoDB.`,
254-
);
255-
256-
if (entraGroupId) {
257-
await modifyGroup(
258-
entraIdToken,
259-
username,
260-
entraGroupId,
261-
EntraGroupActions.ADD,
262-
dynamoClient,
263-
);
264-
logger.info(`Successfully added ${username} to Entra group for ${orgId}.`);
265-
}
266277

267-
return {
268-
function: AvailableSQSFunctions.EmailNotifications,
269-
metadata: { initiator: actorUsername, reqId },
270-
payload: {
271-
to: getAllUserEmails(username),
272-
cc: [officersEmail],
273-
subject: `Lead added for ${orgId}`,
274-
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.`,
275-
},
276-
};
278+
return {
279+
function: AvailableSQSFunctions.EmailNotifications,
280+
metadata: { initiator: actorUsername, reqId },
281+
payload: {
282+
to: getAllUserEmails(username),
283+
cc: [officersEmail],
284+
subject: `${user.nonVotingMember ? "Non-voting lead" : "Lead"} added for ${orgId}`,
285+
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.`,
286+
},
287+
};
288+
});
277289
};
278290

279291
export const removeLead = async ({
@@ -286,6 +298,7 @@ export const removeLead = async ({
286298
dynamoClient,
287299
logger,
288300
officersEmail,
301+
redisClient,
289302
}: {
290303
username: string;
291304
orgId: string;
@@ -296,6 +309,7 @@ export const removeLead = async ({
296309
dynamoClient: DynamoDBClient;
297310
logger: FastifyBaseLogger;
298311
officersEmail: string;
312+
redisClient: Redis;
299313
}): Promise<SQSMessage | null> => {
300314
const removeOperation = async () => {
301315
const removeTransaction = new TransactWriteItemsCommand({
@@ -325,52 +339,61 @@ export const removeLead = async ({
325339
return await dynamoClient.send(removeTransaction);
326340
};
327341

328-
try {
329-
await retryDynamoTransactionWithBackoff(
330-
removeOperation,
331-
logger,
332-
`Remove lead ${username} from ${orgId}`,
333-
);
334-
} catch (e: any) {
335-
if (
336-
e.name === "TransactionCanceledException" &&
337-
e.message.includes("ConditionalCheckFailed")
338-
) {
339-
logger.info(
340-
`User ${username} was not a lead for ${orgId}. Skipping remove operation.`,
342+
const lock = createLock({
343+
adapter: new IoredisAdapter(redisClient),
344+
key: `user:${username}`,
345+
retryAttempts: 5,
346+
retryDelay: 300,
347+
}) as SimpleLock;
348+
349+
return await lock.using(async () => {
350+
try {
351+
await retryDynamoTransactionWithBackoff(
352+
removeOperation,
353+
logger,
354+
`Remove lead ${username} from ${orgId}`,
341355
);
342-
return null;
356+
} catch (e: any) {
357+
if (
358+
e.name === "TransactionCanceledException" &&
359+
e.message.includes("ConditionalCheckFailed")
360+
) {
361+
logger.info(
362+
`User ${username} was not a lead for ${orgId}. Skipping remove operation.`,
363+
);
364+
return null;
365+
}
366+
throw e;
343367
}
344-
throw e;
345-
}
346-
347-
logger.info(
348-
`Successfully removed ${username} as lead for ${orgId} in DynamoDB.`,
349-
);
350368

351-
if (entraGroupId) {
352-
await modifyGroup(
353-
entraIdToken,
354-
username,
355-
entraGroupId,
356-
EntraGroupActions.REMOVE,
357-
dynamoClient,
358-
);
359369
logger.info(
360-
`Successfully removed ${username} from Entra group for ${orgId}.`,
370+
`Successfully removed ${username} as lead for ${orgId} in DynamoDB.`,
361371
);
362-
}
363372

364-
return {
365-
function: AvailableSQSFunctions.EmailNotifications,
366-
metadata: { initiator: actorUsername, reqId },
367-
payload: {
368-
to: getAllUserEmails(username),
369-
cc: [officersEmail],
370-
subject: `Lead removed for ${orgId}`,
371-
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.`,
372-
},
373-
};
373+
if (entraGroupId) {
374+
await modifyGroup(
375+
entraIdToken,
376+
username,
377+
entraGroupId,
378+
EntraGroupActions.REMOVE,
379+
dynamoClient,
380+
);
381+
logger.info(
382+
`Successfully removed ${username} from Entra group for ${orgId}.`,
383+
);
384+
}
385+
386+
return {
387+
function: AvailableSQSFunctions.EmailNotifications,
388+
metadata: { initiator: actorUsername, reqId },
389+
payload: {
390+
to: getAllUserEmails(username),
391+
cc: [officersEmail],
392+
subject: `Lead removed for ${orgId}`,
393+
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.`,
394+
},
395+
};
396+
});
374397
};
375398

376399
/**

‎src/api/package.json‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@
5757
"pino": "^9.6.0",
5858
"pluralize": "^8.0.0",
5959
"qrcode": "^1.5.4",
60+
"redlock-universal": "^0.6.0",
6061
"sanitize-html": "^2.17.0",
6162
"stripe": "^18.0.0",
6263
"uuid": "^11.1.0",

‎src/api/routes/organizations.ts‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -580,6 +580,7 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => {
580580
dynamoClient: fastify.dynamoClient,
581581
logger: request.log,
582582
officersEmail,
583+
redisClient: fastify.redisClient,
583584
};
584585

585586
const addPromises = add.map((user) => addLead({ ...commonArgs, user }));
@@ -639,8 +640,8 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => {
639640
orgName: fastify.environmentConfig.GithubOrgName,
640641
});
641642
} else {
642-
request.log.info(
643-
"IDP sync is disabled in this environment - the newly created group will have no members!",
643+
request.log.warn(
644+
"IdP sync is disabled in this environment - the newly created group will have no members!",
644645
);
645646
}
646647

‎yarn.lock‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8859,6 +8859,11 @@ redis-parser@^3.0.0:
88598859
dependencies:
88608860
redis-errors "^1.0.0"
88618861

8862+
redlock-universal@^0.6.0:
8863+
version "0.6.0"
8864+
resolved "https://registry.yarnpkg.com/redlock-universal/-/redlock-universal-0.6.0.tgz#b67d88fc36c5250c91a6beca02de7efd611aa90f"
8865+
integrity sha512-UrxoupAzkLBZevbzbec9hotqNwytJoO8g7NdXan5hBLjODmMW/5TE5joY3voKN1er14UnOi/tz/I5LkPpmillA==
8866+
88628867
reflect.getprototypeof@^1.0.6, reflect.getprototypeof@^1.0.9:
88638868
version "1.0.10"
88648869
resolved "https://registry.yarnpkg.com/reflect.getprototypeof/-/reflect.getprototypeof-1.0.10.tgz#c629219e78a3316d8b604c765ef68996964e7bf9"

0 commit comments

Comments
 (0)