Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions src/api/functions/identity.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import { DynamoDBClient, GetItemCommand } from "@aws-sdk/client-dynamodb";
import { unmarshall } from "@aws-sdk/util-dynamodb";
import { ValidLoggers } from "api/types.js";
import { genericConfig } from "common/config.js";

export interface UserIdentity {
id: string;
uinHash?: string;
netId?: string;
firstName?: string;
lastName?: string;
stripeCustomerId?: string;
updatedAt?: string;
}

export interface GetUserIdentityInputs {
netId: string;
dynamoClient: DynamoDBClient;
logger: ValidLoggers;
}

export async function getUserIdentity({
netId,
dynamoClient,
logger,
}: GetUserIdentityInputs): Promise<UserIdentity | null> {
const userId = `${netId}@illinois.edu`;

try {
const result = await dynamoClient.send(
new GetItemCommand({
TableName: genericConfig.UserInfoTable,
Key: {
id: { S: userId },
},
ConsistentRead: true,
}),
);

if (!result.Item) {
logger.info(`No user found for netId: ${netId}`);
return null;
}
return unmarshall(result.Item) as UserIdentity;
} catch (error) {
logger.error(`Error fetching user identity for ${netId}: ${error}`);
throw error;
}
}
81 changes: 81 additions & 0 deletions src/api/functions/stripe.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { isProd } from "api/utils.js";
import { InternalServerError, ValidationError } from "common/errors/index.js";
import { capitalizeFirstLetter } from "common/types/roomRequest.js";
import Stripe from "stripe";
Expand All @@ -23,6 +24,18 @@ export type StripeCheckoutSessionCreateParams = {
customFields?: Stripe.Checkout.SessionCreateParams.CustomField[];
};

export type StripeCheckoutSessionCreateWithCustomerParams = {
successUrl?: string;
returnUrl?: string;
customerId: string;
stripeApiKey: string;
items: { price: string; quantity: number }[];
initiator: string;
metadata?: Record<string, string>;
allowPromotionCodes: boolean;
customFields?: Stripe.Checkout.SessionCreateParams.CustomField[];
};

/**
* Create a Stripe payment link for an invoice. Note that invoiceAmountUsd MUST IN CENTS!!
* @param {StripeLinkCreateParams} options
Expand Down Expand Up @@ -107,6 +120,44 @@ export const createCheckoutSession = async ({
return session.url;
};

export const createCheckoutSessionWithCustomer = async ({
successUrl,
returnUrl,
stripeApiKey,
customerId,
items,
initiator,
allowPromotionCodes,
customFields,
metadata,
}: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => {
const stripe = new Stripe(stripeApiKey);
const payload: Stripe.Checkout.SessionCreateParams = {
success_url: successUrl || "",
cancel_url: returnUrl || "",
payment_method_types: ["card"],
line_items: items.map((item) => ({
price: item.price,
quantity: item.quantity,
})),
mode: "payment",
customer: customerId,
metadata: {
...(metadata || {}),
initiator,
},
allow_promotion_codes: allowPromotionCodes,
custom_fields: customFields,
};
const session = await stripe.checkout.sessions.create(payload);
if (!session.url) {
throw new InternalServerError({
message: "Could not create Stripe checkout session.",
});
}
return session.url;
};
Comment on lines +133 to +159
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add a runtime guard for missing customerId.

Defensive check to avoid silent bad requests to Stripe.

 export const createCheckoutSessionWithCustomer = async ({
   successUrl,
   returnUrl,
   stripeApiKey,
   customerId,
   items,
   initiator,
   allowPromotionCodes,
   customFields,
   metadata,
 }: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => {
+  if (!customerId) {
+    throw new ValidationError({ message: "Missing Stripe customerId." });
+  }
   const stripe = new Stripe(stripeApiKey);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
}: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => {
const stripe = new Stripe(stripeApiKey);
const payload: Stripe.Checkout.SessionCreateParams = {
success_url: successUrl || "",
cancel_url: returnUrl || "",
payment_method_types: ["card"],
line_items: items.map((item) => ({
price: item.price,
quantity: item.quantity,
})),
mode: "payment",
customer: customerId,
metadata: {
...(metadata || {}),
initiator,
},
allow_promotion_codes: allowPromotionCodes,
custom_fields: customFields,
};
const session = await stripe.checkout.sessions.create(payload);
if (!session.url) {
throw new InternalServerError({
message: "Could not create Stripe checkout session.",
});
}
return session.url;
};
}: StripeCheckoutSessionCreateWithCustomerParams): Promise<string> => {
if (!customerId) {
throw new ValidationError({ message: "Missing Stripe customerId." });
}
const stripe = new Stripe(stripeApiKey);
const payload: Stripe.Checkout.SessionCreateParams = {
success_url: successUrl || "",
cancel_url: returnUrl || "",
payment_method_types: ["card"],
line_items: items.map((item) => ({
price: item.price,
quantity: item.quantity,
})),
mode: "payment",
customer: customerId,
metadata: {
...(metadata || {}),
initiator,
},
allow_promotion_codes: allowPromotionCodes,
custom_fields: customFields,
};
const session = await stripe.checkout.sessions.create(payload);
if (!session.url) {
throw new InternalServerError({
message: "Could not create Stripe checkout session.",
});
}
return session.url;
};
🤖 Prompt for AI Agents
In src/api/functions/stripe.ts around lines 133 to 159, add a runtime guard that
validates customerId before constructing the Stripe payload and calling
stripe.checkout.sessions.create; if customerId is missing or empty, immediately
throw a descriptive error (e.g., BadRequest or a clear InternalServerError)
indicating that customerId is required so the code does not send a malformed
request to Stripe.


export const deactivateStripeLink = async ({
linkId,
stripeApiKey,
Expand Down Expand Up @@ -244,3 +295,33 @@ export const getPaymentMethodDescriptionString = ({
return `${friendlyName} (${cardBrandMap[cardPresentData.brand || "unknown"]} ending in ${cardPresentData.last4})`;
}
};

export type StripeCustomerCreateParams = {
email: string;
name: string;
stripeApiKey: string;
metadata?: Record<string, string>;
idempotencyKey?: string;
};

export const createStripeCustomer = async ({
email,
name,
stripeApiKey,
metadata,
idempotencyKey,
}: StripeCustomerCreateParams): Promise<string> => {
const stripe = new Stripe(stripeApiKey, { maxNetworkRetries: 2 });
const customer = await stripe.customers.create(
{
email,
name,
metadata: {
...(metadata ?? {}),
...(isProd ? {} : { environment: process.env.RunEnvironment }),
},
},
idempotencyKey ? { idempotencyKey } : undefined,
);
return customer.id;
};
112 changes: 88 additions & 24 deletions src/api/functions/sync.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,22 @@ import {
UpdateItemCommand,
type DynamoDBClient,
} from "@aws-sdk/client-dynamodb";
import { Redis, ValidLoggers } from "api/types.js";
import { genericConfig } from "common/config.js";
import { createLock, IoredisAdapter } from "redlock-universal";
import { createStripeCustomer } from "./stripe.js";
import { InternalServerError } from "common/errors/index.js";
import { unmarshall } from "@aws-sdk/util-dynamodb";

export interface SyncFullProfileInputs {
uinHash: string;
netId: string;
firstName: string;
lastName: string;
dynamoClient: DynamoDBClient;
redisClient: Redis;
stripeApiKey: string;
logger: ValidLoggers;
}

export async function syncFullProfile({
Expand All @@ -18,29 +26,85 @@ export async function syncFullProfile({
firstName,
lastName,
dynamoClient,
redisClient,
stripeApiKey,
logger,
}: SyncFullProfileInputs) {
return dynamoClient.send(
new UpdateItemCommand({
TableName: genericConfig.UserInfoTable,
Key: {
id: { S: `${netId}@illinois.edu` },
},
UpdateExpression:
"SET #uinHash = :uinHash, #netId = :netId, #updatedAt = :updatedAt, #firstName = :firstName, #lastName = :lastName",
ExpressionAttributeNames: {
"#uinHash": "uinHash",
"#netId": "netId",
"#updatedAt": "updatedAt",
"#firstName": "firstName",
"#lastName": "lastName",
},
ExpressionAttributeValues: {
":uinHash": { S: uinHash },
":netId": { S: netId },
":firstName": { S: firstName },
":lastName": { S: lastName },
":updatedAt": { S: new Date().toISOString() },
},
}),
);
const lock = createLock({
adapter: new IoredisAdapter(redisClient),
key: `userSync:${netId}`,
retryAttempts: 5,
retryDelay: 300,
});
Comment on lines +33 to +38
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Set an explicit lock TTL and reduce work under the lock.

Add a clear duration to avoid silent expiry during network calls. Also check for lock loss both before and after external calls.

   const lock = createLock({
     adapter: new IoredisAdapter(redisClient),
     key: `userSync:${netId}`,
+    duration: 15000, // 15s; tune based on p95 of the section below
     retryAttempts: 5,
     retryDelay: 300,
   });

And add a second abort check just before the final Dynamo write (see diff further below).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const lock = createLock({
adapter: new IoredisAdapter(redisClient),
key: `userSync:${netId}`,
retryAttempts: 5,
retryDelay: 300,
});
const lock = createLock({
adapter: new IoredisAdapter(redisClient),
key: `userSync:${netId}`,
duration: 15000, // 15s; tune based on p95 of the section below
retryAttempts: 5,
retryDelay: 300,
});
🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 33 to 38, the created lock is missing
an explicit TTL and too much work runs while the lock is held; set a clear TTL
on the lock (e.g. ttl in ms) so it cannot silently expire during network calls,
minimize the work done inside the locked section by moving long external/network
calls out of the critical region, and add checks for lock ownership both
immediately after the external calls return and again just before the final
Dynamo write (abort if the lock was lost) so we never proceed with the final
write when we no longer hold the lock.


return await lock.using(async (signal) => {
const userId = `${netId}@illinois.edu`;
const updateResult = await dynamoClient.send(
new UpdateItemCommand({
TableName: genericConfig.UserInfoTable,
Key: {
id: { S: userId },
},
UpdateExpression:
"SET #uinHash = :uinHash, #netId = :netId, #updatedAt = :updatedAt, #firstName = :firstName, #lastName = :lastName",
ExpressionAttributeNames: {
"#uinHash": "uinHash",
"#netId": "netId",
"#updatedAt": "updatedAt",
"#firstName": "firstName",
"#lastName": "lastName",
},
ExpressionAttributeValues: {
":uinHash": { S: uinHash },
":netId": { S: netId },
":firstName": { S: firstName },
":lastName": { S: lastName },
":updatedAt": { S: new Date().toISOString() },
},
ReturnValues: "ALL_NEW",
}),
);

const stripeCustomerId = updateResult.Attributes?.stripeCustomerId?.S;

if (!stripeCustomerId) {
if (signal.aborted) {
throw new InternalServerError({
message:
"Checked on lock before creating Stripe customer, we've lost the lock!",
});
}
const newStripeCustomerId = await createStripeCustomer({
email: userId,
name: `${firstName} ${lastName}`,
stripeApiKey,
});
logger.info(`Created new Stripe customer for ${userId}.`);
const newInfo = await dynamoClient.send(
Comment on lines +70 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Prevent duplicate Stripe customers: use an idempotency key when creating the customer.

If the lock expires mid‑flow, concurrent runs can each create a Stripe customer. Protect the Stripe call with a deterministic idempotency key tied to the user.

-      const newStripeCustomerId = await createStripeCustomer({
-        email: userId,
-        name: `${firstName} ${lastName}`,
-        stripeApiKey,
-      });
+      const idempotencyKey = `acm-create-stripe-customer:${userId}`;
+      const newStripeCustomerId = await createStripeCustomer({
+        email: userId,
+        name: `${firstName} ${lastName}`,
+        stripeApiKey,
+        metadata: { userId },
+        idempotencyKey,
+      });

Companion changes are proposed in src/api/functions/stripe.ts to accept and pass the idempotency key.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 70 to 83, the Stripe customer creation
is vulnerable to duplicates if the lock expires; generate a deterministic
idempotency key (e.g., HMAC/SHA256 of the user id or user id + tenant) and pass
it into createStripeCustomer as an extra argument, then forward that key to
stripe.ts so the Stripe API request includes it in the Idempotency-Key header;
keep the existing lock/abort check and use the deterministic key for all
concurrent attempts for the same user to prevent duplicate customers.

new UpdateItemCommand({
TableName: genericConfig.UserInfoTable,
Key: {
id: { S: userId },
},
UpdateExpression: "SET #stripeCustomerId = :stripeCustomerId",
ExpressionAttributeNames: {
"#stripeCustomerId": "stripeCustomerId",
},
ExpressionAttributeValues: {
":stripeCustomerId": { S: newStripeCustomerId },
},
ReturnValues: "ALL_NEW",
}),
);
return newInfo && newInfo.Attributes
Comment on lines +83 to +99
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Make the stripeCustomerId write conditional; re‑read on conflict.

Ensure only the first writer sets the field; others should not clobber and should read the winning value.

-      const newInfo = await dynamoClient.send(
-        new UpdateItemCommand({
+      if (signal.aborted) {
+        throw new InternalServerError({ message: "Lost lock before persisting Stripe customer ID." });
+      }
+      try {
+        const newInfo = await dynamoClient.send(
+          new UpdateItemCommand({
             TableName: genericConfig.UserInfoTable,
             Key: {
               id: { S: userId },
             },
             UpdateExpression: "SET #stripeCustomerId = :stripeCustomerId",
             ExpressionAttributeNames: {
               "#stripeCustomerId": "stripeCustomerId",
             },
             ExpressionAttributeValues: {
               ":stripeCustomerId": { S: newStripeCustomerId },
             },
+            ConditionExpression: "attribute_not_exists(#stripeCustomerId)",
             ReturnValues: "ALL_NEW",
-        }),
-      );
-      return newInfo && newInfo.Attributes
-        ? unmarshall(newInfo.Attributes)
-        : updateResult && updateResult.Attributes
-          ? unmarshall(updateResult.Attributes)
-          : undefined;
+          }),
+        );
+        return newInfo && newInfo.Attributes
+          ? unmarshall(newInfo.Attributes)
+          : updateResult && updateResult.Attributes
+            ? unmarshall(updateResult.Attributes)
+            : undefined;
+      } catch (err: any) {
+        // Another writer won the race; fetch the latest record.
+        const latest = await dynamoClient.send(
+          new UpdateItemCommand({
+            TableName: genericConfig.UserInfoTable,
+            Key: { id: { S: userId } },
+            // no-op update to get ALL_NEW without needing an extra import
+            UpdateExpression: "SET #updatedAt = :updatedAt",
+            ExpressionAttributeNames: { "#updatedAt": "updatedAt" },
+            ExpressionAttributeValues: { ":updatedAt": { S: new Date().toISOString() } },
+            ReturnValues: "ALL_NEW",
+          }),
+        );
+        return latest && latest.Attributes ? unmarshall(latest.Attributes) : undefined;
+      }

Note: If preferred, switch to GetItem for a pure read.

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents
In src/api/functions/sync.ts around lines 83 to 99, the UpdateItem
unconditionally overwrites stripeCustomerId; change it to a conditional write
that only sets stripeCustomerId when it does not yet exist (e.g. use
ConditionExpression like "attribute_not_exists(#stripeCustomerId)" or check for
NULL) so only the first writer succeeds; on ConditionalCheckFailedException
re-read the item (GetItem) and return the existing stripeCustomerId/Attributes
instead of clobbering them; ensure errors are handled and the successful path
returns the updated Attributes.

? unmarshall(newInfo.Attributes)
: updateResult && updateResult.Attributes
? unmarshall(updateResult.Attributes)
: undefined;
}

return updateResult && updateResult.Attributes
? unmarshall(updateResult.Attributes)
: undefined;
});
}
66 changes: 65 additions & 1 deletion src/api/routes/syncIdentity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
resolveEmailToOid,
} from "api/functions/entraId.js";
import { syncFullProfile } from "api/functions/sync.js";
import { getUserIdentity, UserIdentity } from "api/functions/identity.js";

const syncIdentityPlugin: FastifyPluginAsync = async (fastify, _options) => {
const getAuthorizedClients = async () => {
Expand Down Expand Up @@ -109,6 +110,9 @@ const syncIdentityPlugin: FastifyPluginAsync = async (fastify, _options) => {
lastName: surname,
netId,
dynamoClient: fastify.dynamoClient,
redisClient: fastify.redisClient,
stripeApiKey: fastify.secretConfig.stripe_secret_key,
logger: request.log,
});
Comment on lines +113 to 116
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Validate Stripe secret before calling downstream.

Fail fast with a clear error if stripe_secret_key is unset.

         await syncFullProfile({
+          ...(fastify.secretConfig.stripe_secret_key
+            ? {}
+            : (() => {
+                request.log.error("Missing Stripe secret key.");
+                throw new InternalServerError({ message: "Stripe is not configured." });
+              })()),
           uinHash,
           firstName: givenName,
           lastName: surname,
           netId,
           dynamoClient: fastify.dynamoClient,
           redisClient: fastify.redisClient,
           stripeApiKey: fastify.secretConfig.stripe_secret_key,
           logger: request.log,
         });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
redisClient: fastify.redisClient,
stripeApiKey: fastify.secretConfig.stripe_secret_key,
logger: request.log,
});
await syncFullProfile({
...(fastify.secretConfig.stripe_secret_key
? {}
: (() => {
request.log.error("Missing Stripe secret key.");
throw new InternalServerError({ message: "Stripe is not configured." });
})()),
uinHash,
firstName: givenName,
lastName: surname,
netId,
dynamoClient: fastify.dynamoClient,
redisClient: fastify.redisClient,
stripeApiKey: fastify.secretConfig.stripe_secret_key,
logger: request.log,
});
🤖 Prompt for AI Agents
In src/api/routes/syncIdentity.ts around lines 112 to 115, the code passes
fastify.secretConfig.stripe_secret_key into downstream code without validating
it; add an early check for the presence (non-empty) of stripe_secret_key before
calling the downstream constructor, and if it is missing log an error and fail
fast by returning a clear error response (e.g., throw
fastify.httpErrors.internal('Stripe secret key not configured') or
reply.code(500).send(...)) so the route never proceeds with an undefined/empty
key.

let isPaidMember = await checkPaidMembershipFromRedis(
netId,
Expand All @@ -123,7 +127,7 @@ const syncIdentityPlugin: FastifyPluginAsync = async (fastify, _options) => {
}
if (isPaidMember) {
const username = `${netId}@illinois.edu`;
request.log.info("User is paid member, syncing profile!");
request.log.info("User is paid member, syncing Entra user!");
const entraIdToken = await getEntraIdToken({
clients: await getAuthorizedClients(),
clientId: fastify.environmentConfig.AadValidClientId,
Expand All @@ -141,6 +145,66 @@ const syncIdentityPlugin: FastifyPluginAsync = async (fastify, _options) => {
return reply.status(201).send();
},
);
fastify.withTypeProvider<FastifyZodOpenApiTypeProvider>().get(
"/isRequired",
{
schema: withTags(["Generic"], {
headers: z.object({
"x-uiuc-token": z.jwt().min(1).meta({
description:
"An access token for the user in the UIUC Entra ID tenant.",
}),
}),
summary: "Check if a user needs a full user identity sync.",
response: {
200: {
description: "The status was retrieved.",
content: {
"application/json": {
schema: z.object({
syncRequired: z.boolean().default(false),
}),
},
},
},
403: notAuthenticatedError,
},
}),
},
async (request, reply) => {
const accessToken = request.headers["x-uiuc-token"];
const verifiedData = await verifyUiucAccessToken({
accessToken,
logger: request.log,
});
const { userPrincipalName: upn, givenName, surname } = verifiedData;
const netId = upn.replace("@illinois.edu", "");
if (netId.includes("@")) {
request.log.error(
`Found UPN ${upn} which cannot be turned into NetID via simple replacement.`,
);
throw new ValidationError({
message: "ID token could not be parsed.",
});
}
const userIdentity = await getUserIdentity({
netId,
dynamoClient: fastify.dynamoClient,
logger: request.log,
});

const requiredFields: (keyof UserIdentity)[] = [
"uinHash",
"firstName",
"lastName",
"stripeCustomerId",
];

const syncRequired =
!userIdentity || requiredFields.some((field) => !userIdentity[field]);
return reply.send({ syncRequired });
},
);
};
fastify.register(limitedRoutes);
};
Expand Down
Loading
Loading