Skip to content

Seperate Exec membership from Org admin #343

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 26, 2025
Merged

Seperate Exec membership from Org admin #343

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a1bb10c
Seperate Exec membership from Org admin
devksingh4 Oct 24, 2025
8c268b8
Move parent team not to be exec but instead to be some holder team
devksingh4 Oct 24, 2025
2b4672f
Use correct client for entra
devksingh4 Oct 24, 2025
0fa1fa8
Auto-update feature branch with changes from the main branch
github-actions[bot] Oct 24, 2025
196f234
Auto-update feature branch with changes from the main branch
github-actions[bot] Oct 24, 2025
cc72e32
Fix UI strings
devksingh4 Oct 26, 2025
1fe80d0
Fix 2
devksingh4 Oct 26, 2025
0a12700
Fix returning non-voting members
devksingh4 Oct 26, 2025
2022c0a
Add a unit test for non-voting members
devksingh4 Oct 26, 2025
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
2 changes: 1 addition & 1 deletion src/api/functions/github.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Octokit } from "octokit";
export interface CreateGithubTeamInputs {
githubToken: string;
orgId: string;
parentTeamId: number;
parentTeamId?: number;
name: string;
description?: string;
privacy?: "secret" | "closed";
Expand Down
132 changes: 101 additions & 31 deletions src/api/functions/organizations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,13 @@ export async function getOrgInfo({
name: x.name,
username: x.username,
title: x.title,
}) as { name: string; username: string; title: string | undefined },
nonVotingMember: x.nonVotingMember || false,
}) as {
name: string;
username: string;
title: string | undefined;
nonVotingMember: boolean;
},
);
response = { ...response, leads: unmarshalledLeads };
}
Expand Down Expand Up @@ -525,42 +531,106 @@ export const removeLead = async ({
};

/**
* Returns the Microsoft 365 Dynamic User query to return all members of all lead groups.
* Currently used to setup the Exec member list.
* Returns all voting org leads across all organizations.
* Uses consistent reads to avoid eventual consistency issues.
* @param dynamoClient A DynamoDB client.
* @param includeGroupIds Used to ensure that a specific group ID is included (Scan could be eventually consistent.)
* @param logger A logger instance.
*/
export async function getLeadsM365DynamicQuery({
export async function getAllVotingLeads({
dynamoClient,
includeGroupIds,
logger,
}: {
dynamoClient: DynamoDBClient;
includeGroupIds?: string[];
}): Promise<string | null> {
const command = new ScanCommand({
TableName: genericConfig.SigInfoTableName,
IndexName: "LeadsGroupIdIndex",
logger: ValidLoggers;
}): Promise<
Array<{ username: string; org: string; name: string; title: string }>
> {
// Query all organizations in parallel for better performance
const queryPromises = AllOrganizationNameList.map(async (orgName) => {
const leadsQuery = new QueryCommand({
TableName: genericConfig.SigInfoTableName,
KeyConditionExpression: "primaryKey = :leadName",
ExpressionAttributeValues: {
":leadName": { S: `LEAD#${orgName}` },
},
ConsistentRead: true,
});

try {
const responseMarshall = await dynamoClient.send(leadsQuery);
if (responseMarshall.Items) {
return responseMarshall.Items.map((x) => unmarshall(x))
.filter((x) => x.username && !x.nonVotingMember)
.map((x) => ({
username: x.username as string,
org: orgName,
name: x.name as string,
title: x.title as string,
}));
}
return [];
} catch (e) {
if (e instanceof BaseError) {
throw e;
}
logger.error(e);
throw new DatabaseFetchError({
message: `Failed to get leads for org ${orgName}.`,
});
}
});
const results = await dynamoClient.send(command);
if (!results || !results.Items || results.Items.length === 0) {
return null;
}
const entries = results.Items.map((x) => unmarshall(x)) as {
primaryKey: string;
leadsEntraGroupId: string;
}[];
const groupIds = entries
.filter((x) => x.primaryKey.startsWith("DEFINE#"))
.map((x) => x.leadsEntraGroupId);

if (groupIds.length === 0) {
return null;

const results = await Promise.all(queryPromises);
return results.flat();
}
Comment on lines +539 to +585
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

Fix return type for title (can be undefined) + trim payload + optional resiliency

  • Title may be missing on some lead items; current return type promises string. Make it optional to avoid misleading consumers and potential runtime issues. Also project only needed attributes to cut payload. Optionally, return partial results if one org read fails.

Apply:

 export async function getAllVotingLeads({
   dynamoClient,
   logger,
 }: {
   dynamoClient: DynamoDBClient;
-  logger: ValidLoggers;
-}): Promise<
-  Array<{ username: string; org: string; name: string; title: string }>
-> {
+  logger: ValidLoggers;
+}): Promise<
+  Array<{ username: string; org: string; name: string; title?: string }>
+> {
   // Query all organizations in parallel for better performance
   const queryPromises = AllOrganizationNameList.map(async (orgName) => {
     const leadsQuery = new QueryCommand({
       TableName: genericConfig.SigInfoTableName,
       KeyConditionExpression: "primaryKey = :leadName",
       ExpressionAttributeValues: {
         ":leadName": { S: `LEAD#${orgName}` },
       },
+      ProjectionExpression: "#u, #n, title, nonVotingMember",
+      ExpressionAttributeNames: { "#u": "username", "#n": "name" },
       ConsistentRead: true,
     });
 
     try {
       const responseMarshall = await dynamoClient.send(leadsQuery);
       if (responseMarshall.Items) {
         return responseMarshall.Items.map((x) => unmarshall(x))
           .filter((x) => x.username && !x.nonVotingMember)
           .map((x) => ({
             username: x.username as string,
             org: orgName,
-            name: x.name as string,
-            title: x.title as string,
+            name: x.name as string,
+            title: x.title,
           }));
       }
       return [];
     } catch (e) {
       if (e instanceof BaseError) {
         throw e;
       }
       logger.error(e);
       throw new DatabaseFetchError({
         message: `Failed to get leads for org ${orgName}.`,
       });
     }
   });
 
-  const results = await Promise.all(queryPromises);
-  return results.flat();
+  // Option B (resiliency): tolerate one-org failures and return partial results
+  // const settled = await Promise.allSettled(queryPromises);
+  // const results: Array<{ username: string; org: string; name: string; title?: string }> = [];
+  // for (const s of settled) {
+  //   if (s.status === "fulfilled") results.push(...s.value);
+  //   else logger.error(s.reason);
+  // }
+  // return results;
+  const results = await Promise.all(queryPromises);
+  return results.flat();
 }
📝 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
export async function getAllVotingLeads({
dynamoClient,
includeGroupIds,
logger,
}: {
dynamoClient: DynamoDBClient;
includeGroupIds?: string[];
}): Promise<string | null> {
const command = new ScanCommand({
TableName: genericConfig.SigInfoTableName,
IndexName: "LeadsGroupIdIndex",
logger: ValidLoggers;
}): Promise<
Array<{ username: string; org: string; name: string; title: string }>
> {
// Query all organizations in parallel for better performance
const queryPromises = AllOrganizationNameList.map(async (orgName) => {
const leadsQuery = new QueryCommand({
TableName: genericConfig.SigInfoTableName,
KeyConditionExpression: "primaryKey = :leadName",
ExpressionAttributeValues: {
":leadName": { S: `LEAD#${orgName}` },
},
ConsistentRead: true,
});
try {
const responseMarshall = await dynamoClient.send(leadsQuery);
if (responseMarshall.Items) {
return responseMarshall.Items.map((x) => unmarshall(x))
.filter((x) => x.username && !x.nonVotingMember)
.map((x) => ({
username: x.username as string,
org: orgName,
name: x.name as string,
title: x.title as string,
}));
}
return [];
} catch (e) {
if (e instanceof BaseError) {
throw e;
}
logger.error(e);
throw new DatabaseFetchError({
message: `Failed to get leads for org ${orgName}.`,
});
}
});
const results = await dynamoClient.send(command);
if (!results || !results.Items || results.Items.length === 0) {
return null;
}
const entries = results.Items.map((x) => unmarshall(x)) as {
primaryKey: string;
leadsEntraGroupId: string;
}[];
const groupIds = entries
.filter((x) => x.primaryKey.startsWith("DEFINE#"))
.map((x) => x.leadsEntraGroupId);
if (groupIds.length === 0) {
return null;
const results = await Promise.all(queryPromises);
return results.flat();
}
export async function getAllVotingLeads({
dynamoClient,
logger,
}: {
dynamoClient: DynamoDBClient;
logger: ValidLoggers;
}): Promise<
Array<{ username: string; org: string; name: string; title?: string }>
> {
// Query all organizations in parallel for better performance
const queryPromises = AllOrganizationNameList.map(async (orgName) => {
const leadsQuery = new QueryCommand({
TableName: genericConfig.SigInfoTableName,
KeyConditionExpression: "primaryKey = :leadName",
ExpressionAttributeValues: {
":leadName": { S: `LEAD#${orgName}` },
},
ProjectionExpression: "#u, #n, title, nonVotingMember",
ExpressionAttributeNames: { "#u": "username", "#n": "name" },
ConsistentRead: true,
});
try {
const responseMarshall = await dynamoClient.send(leadsQuery);
if (responseMarshall.Items) {
return responseMarshall.Items.map((x) => unmarshall(x))
.filter((x) => x.username && !x.nonVotingMember)
.map((x) => ({
username: x.username as string,
org: orgName,
name: x.name as string,
title: x.title,
}));
}
return [];
} catch (e) {
if (e instanceof BaseError) {
throw e;
}
logger.error(e);
throw new DatabaseFetchError({
message: `Failed to get leads for org ${orgName}.`,
});
}
});
// Option B (resiliency): tolerate one-org failures and return partial results
// const settled = await Promise.allSettled(queryPromises);
// const results: Array<{ username: string; org: string; name: string; title?: string }> = [];
// for (const s of settled) {
// if (s.status === "fulfilled") results.push(...s.value);
// else logger.error(s.reason);
// }
// return results;
const results = await Promise.all(queryPromises);
return results.flat();
}
🤖 Prompt for AI Agents 
In src/api/functions/organizations.ts around lines 533 to 579, the function
currently declares title as string, fetches full items, and throws if any org
query fails; change the return type so title is optional (title?: string),
restrict the DynamoDB query to only the needed attributes using
ProjectionExpression (e.g., username, name, title) and ExpressionAttributeNames
to reduce payload, and make the per-org catch block resilient by logging the
error and returning an empty array for that org instead of throwing (still
rethrow BaseError). Ensure callers/platform types are updated to accept title as
possibly undefined.


/**
* Checks if a user should remain in exec council by verifying they are a voting lead of at least one org.
* Uses consistent reads to avoid eventual consistency issues.
* @param username The username to check.
* @param dynamoClient A DynamoDB client.
* @param logger A logger instance.
*/
export async function shouldBeInExecCouncil({
username,
dynamoClient,
logger,
}: {
username: string;
dynamoClient: DynamoDBClient;
logger: ValidLoggers;
}): Promise<boolean> {
// Query all orgs to see if this user is a voting lead of any org
for (const orgName of AllOrganizationNameList) {
const leadsQuery = new QueryCommand({
TableName: genericConfig.SigInfoTableName,
KeyConditionExpression: "primaryKey = :leadName AND entryId = :username",
ExpressionAttributeValues: {
":leadName": { S: `LEAD#${orgName}` },
":username": { S: username },
},
ConsistentRead: true,
});

try {
const responseMarshall = await dynamoClient.send(leadsQuery);
if (responseMarshall.Items && responseMarshall.Items.length > 0) {
const lead = unmarshall(responseMarshall.Items[0]);
// If they're a lead and not a non-voting member, they should be in exec
if (!lead.nonVotingMember) {
return true;
}
}
} catch (e) {
if (e instanceof BaseError) {
throw e;
}
logger.error(e);
throw new DatabaseFetchError({
message: `Failed to check lead status for ${username} in org ${orgName}.`,
});
}
}

const formattedGroupIds = [
...new Set([...(includeGroupIds || []), ...groupIds]),
]
.map((id) => `'${id}'`)
.join(", ");
return `user.memberOf -any (group.objectId -in [${formattedGroupIds}])`;
return false;
}
52 changes: 19 additions & 33 deletions src/api/routes/organizations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,5 @@
import { FastifyError, FastifyPluginAsync } from "fastify";
import {
AllOrganizationNameList,
getOrgByName,
Organizations,
} from "@acm-uiuc/js-shared";
import { FastifyPluginAsync } from "fastify";
import { AllOrganizationNameList, getOrgByName } from "@acm-uiuc/js-shared";
import rateLimiter from "api/plugins/rateLimiter.js";
import { withRoles, withTags } from "api/components/index.js";
import { z } from "zod/v4";
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

🧩 Analysis chain

Verify Zod v4 compatibility across the codebase.

The import has been updated to use zod/v4, which introduces breaking changes. While the usage in this file appears compatible, ensure that other files using Zod schemas have been reviewed for:

  • Default value handling changes (.default() now short-circuits on undefined)
  • Number validation changes (no infinite values, .int() only allows safe integers)
  • Record schema changes (now requires both key and value schemas)
  • Function schema redesign
  • Enum handling changes

🏁 Script executed:

#!/bin/bash
# Description: Search for Zod patterns that may be affected by v4 breaking changes

echo "=== Checking for .default() usage with transforms ==="
rg -n "\.transform\(" --type=ts -A 5 | rg "\.default\("

echo -e "\n=== Checking for z.record() without explicit schemas ==="
rg -n "z\.record\([^,)]*\)" --type=ts

echo -e "\n=== Checking for z.function() usage ==="
rg -n "z\.function\(" --type=ts

echo -e "\n=== Checking for number validation with potential infinity ==="
rg -n "z\.number\(\)" --type=ts -A 2 | rg -i "infinite|infinity"

Length of output: 309

🏁 Script executed:

#!/bin/bash
# Get the full line from events.ts for the z.record() call
echo "=== Full z.record() line from events.ts ==="
sed -n '8p' src/common/types/events.ts

echo -e "\n=== Verify z.record() context (lines 5-12) ==="
sed -n '5,12p' src/common/types/events.ts

echo -e "\n=== Check all Zod version imports in the codebase ==="
rg -n "from\s+[\"']zod" --type=ts

echo -e "\n=== Check for .merge() chains or other schema composition ==="
rg -n "\.merge\(" --type=ts | head -20

echo -e "\n=== Check for .pick() or .omit() usage ==="
rg -n "\.pick\(|\.omit\(" --type=ts | head -20

Length of output: 3811

🏁 Script executed:

#!/bin/bash
# Check if api/functions/organizations.ts is related to the reviewed file
echo "=== Check what's imported from api/functions/organizations.ts ==="
rg -n "from.*api/functions/organizations" --type=ts

echo -e "\n=== Check what api/functions/organizations.ts exports ==="
head -50 src/api/functions/organizations.ts | grep -E "export|^import"

echo -e "\n=== Check if api/functions/organizations.ts was modified in this PR ==="
rg -n "import.*organizations" src/api/routes/organizations.ts

echo -e "\n=== Verify Zod import resolution - check if 'zod' resolves to v4 ==="
rg -n "\"zod\"[^/]" --type=ts | head -10

Length of output: 1825

Update src/api/functions/organizations.ts to use Zod v4.

The reviewed file imports from api/functions/organizations.ts, which still uses import z from "zod" instead of import z from "zod/v4". This creates a Zod version mismatch that can cause validation and type compatibility issues. Update line 20 of src/api/functions/organizations.ts to:

import z from "zod/v4";

Additionally, verify all other files importing from this module are compatible with the updated import.

🤖 Prompt for AI Agents 
In src/api/functions/organizations.ts around line 20, the file currently imports
Zod using the old path; change the import to use Zod v4 by replacing the
existing import with: import z from "zod/v4"; then run a quick grep across the
repo for imports of this module and ensure callers remain compatible with the v4
export (update any imports or type usages if necessary) and run the test/compile
step to confirm no type or runtime errors.

Expand All @@ -23,7 +19,6 @@ import {
} from "common/errors/index.js";
import {
addLead,
getLeadsM365DynamicQuery,
getOrgInfo,
removeLead,
SQSMessage,
Expand All @@ -35,8 +30,6 @@ import {
TransactWriteItemsCommand,
} from "@aws-sdk/client-dynamodb";
import {
execCouncilGroupId,
execCouncilTestingGroupId,
genericConfig,
notificationRecipients,
roleArns,
Expand All @@ -46,20 +39,12 @@ import { buildAuditLogTransactPut } from "api/functions/auditLog.js";
import { Modules } from "common/modules.js";
import { authorizeByOrgRoleOrSchema } from "api/functions/authorization.js";
import { checkPaidMembership } from "api/functions/membership.js";
import {
createM365Group,
getEntraIdToken,
setGroupMembershipRule,
} from "api/functions/entraId.js";
import { createM365Group, getEntraIdToken } from "api/functions/entraId.js";
import { SecretsManagerClient } from "@aws-sdk/client-secrets-manager";
import { getRoleCredentials } from "api/functions/sts.js";
import { SendMessageCommand, SQSClient } from "@aws-sdk/client-sqs";
import { SQSClient } from "@aws-sdk/client-sqs";
import { sendSqsMessagesInBatches } from "api/functions/sqs.js";
import { retryDynamoTransactionWithBackoff } from "api/utils.js";
import {
assignIdpGroupsToTeam,
createGithubTeam,
} from "api/functions/github.js";
import { SKIP_EXTERNAL_ORG_LEAD_UPDATE } from "common/overrides.js";
import { AvailableSQSFunctions, SQSPayload } from "common/types/sqsMessage.js";

Expand Down Expand Up @@ -499,20 +484,8 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => {
`Store Entra group ID for ${request.params.orgId}`,
);

// Update dynamic membership query
const newQuery = await getLeadsM365DynamicQuery({
dynamoClient: fastify.dynamoClient,
includeGroupIds: [entraGroupId],
});
if (newQuery) {
const groupToUpdate =
fastify.runEnvironment === "prod"
? execCouncilGroupId
: execCouncilTestingGroupId;
request.log.info("Changing Exec group membership dynamic query...");
await setGroupMembershipRule(entraIdToken, groupToUpdate, newQuery);
request.log.info("Changed Exec group membership dynamic query!");
}
// Note: Exec council membership is now managed via SQS sync handler
// instead of dynamic membership rules
} catch (e) {
request.log.error(e, "Failed to create Entra group");
throw new InternalServerError({
Expand Down Expand Up @@ -589,6 +562,19 @@ const organizationsPlugin: FastifyPluginAsync = async (fastify, _options) => {
};
sqsPayloads.push(sqsPayload);
}

// Queue exec council sync to ensure voting members are added/removed
const syncExecPayload: SQSPayload<AvailableSQSFunctions.SyncExecCouncil> =
{
function: AvailableSQSFunctions.SyncExecCouncil,
metadata: {
initiator: request.username!,
reqId: request.id,
},
payload: {},
};
sqsPayloads.push(syncExecPayload);

if (sqsPayloads.length > 0) {
await sendSqsMessagesInBatches({
sqsClient: fastify.sqsClient,
Expand Down
2 changes: 1 addition & 1 deletion src/api/sqs/handlers/createOrgGithubTeam.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export const createOrgGithubTeamHandler: SQSHandlerFunction<
const { updated, id: teamId } = await createGithubTeam({
orgId: currentEnvironmentConfig.GithubOrgName,
githubToken: secretConfig.github_pat,
parentTeamId: currentEnvironmentConfig.ExecGithubTeam,
parentTeamId: currentEnvironmentConfig.OrgAdminGithubParentTeam,
name: finalName,
description: githubTeamDescription,
logger,
Expand Down
1 change: 1 addition & 0 deletions src/api/sqs/handlers/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ export { provisionNewMemberHandler } from "./provisionNewMember.js";
export { sendSaleEmailHandler } from "./sendSaleEmailHandler.js";
export { emailNotificationsHandler } from "./emailNotifications.js";
export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js";
export { syncExecCouncilHandler } from "./syncExecCouncil.js";
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

🧩 Analysis chain

ESLint import/extensions warning.

If using TS + ESM with NodeNext and emitted .js paths, keep the extension and relax the rule; otherwise drop the extension for local exports.

🏁 Script executed:

#!/bin/bash
# Show existing pattern in this repo
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' src/api/sqs/handlers | wc -l

Length of output: 1


---

🏁 Script executed:

```shell
#!/bin/bash
# Check tsconfig.json for module resolution strategy
cat tsconfig.json 2>/dev/null | grep -A5 -B5 "moduleResolution\|module"

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Check for ESLint configuration files and import/extensions rule
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) 2>/dev/null | head -5

Length of output: 19

🏁 Script executed:

#!/bin/bash
# Check package.json for ESLint config and import/extensions rule mention
cat package.json 2>/dev/null | grep -A20 '"eslintConfig"' | head -30

Length of output: 0

🏁 Script executed:

#!/bin/bash
# Show actual exports with .js in src/api/sqs/handlers to confirm pattern
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' src/api/sqs/handlers

Length of output: 680


---

🏁 Script executed:

```shell
#!/bin/bash
# Read eslint.config.mjs to see import/extensions rule
cat eslint.config.mjs | head -100

Length of output: 2565

🏁 Script executed:

#!/bin/bash
# Read full tsconfig.json
cat tsconfig.json

Length of output: 45

🏁 Script executed:

#!/bin/bash
# Check if .js extensions are used elsewhere in codebase (not just handlers)
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' --type=ts --max-count=20 | head -20

Length of output: 680

Remove .js extensions from all export statements—they violate the ESLint rule.

The project's ESLint configuration explicitly forbids .js extensions on local imports and exports ("import/extensions": ["error", "ignorePackages", { js: "never", ... }] at eslint.config.mjs). All 7 exports at lines 1–7 must drop the .js extension to comply:

export { syncExecCouncilHandler } from "./syncExecCouncil";
🧰 Tools
🪛 ESLint

[error] 7-7: Unexpected use of file extension "js" for "./syncExecCouncil.js"

(import/extensions)

🤖 Prompt for AI Agents 
In src/api/sqs/handlers/index.ts around line 7, the export statement includes a
.js extension which violates the project's ESLint import/extensions rule; remove
the .js extension from this and the other local export statements so they read
e.g. export { syncExecCouncilHandler } from "./syncExecCouncil" and repeat for
all seven exports to comply with the lint rule.

Loading
Loading