Improve member check and IAM (#293)

devksingh4 · web-flow · commit e5060bfba541 · 2025-09-10T21:06:07.000-05:00
- Remove dependence on Entra ID for membership checks 
- Get user and group roles from the same table
diff --git a/src/api/functions/authorization.ts b/src/api/functions/authorization.ts
@@ -12,11 +12,11 @@ export async function getUserRoles(
   dynamoClient: DynamoDBClient,
   userId: string,
 ): Promise<AppRoles[]> {
-  const tableName = `${genericConfig.IAMTablePrefix}-userroles`;
+  const tableName = `${genericConfig.IAMTablePrefix}-assignments`;
   const command = new GetItemCommand({
     TableName: tableName,
     Key: {
-      userEmail: { S: userId },
+      id: { S: `USER#${userId}` },
     },
   });
   const response = await dynamoClient.send(command);
@@ -42,11 +42,11 @@ export async function getGroupRoles(
   dynamoClient: DynamoDBClient,
   groupId: string,
 ) {
-  const tableName = `${genericConfig.IAMTablePrefix}-grouproles`;
+  const tableName = `${genericConfig.IAMTablePrefix}-assignments`;
   const command = new GetItemCommand({
     TableName: tableName,
     Key: {
-      groupUuid: { S: groupId },
+      id: { S: `GROUP#${groupId}` },
     },
   });
   const response = await dynamoClient.send(command);
diff --git a/src/api/functions/membership.ts b/src/api/functions/membership.ts
@@ -2,7 +2,6 @@ import {
   BatchWriteItemCommand,
   ConditionalCheckFailedException,
   DynamoDBClient,
-  PutItemCommand,
   QueryCommand,
   UpdateItemCommand,
 } from "@aws-sdk/client-dynamodb";
@@ -239,7 +238,10 @@ export async function checkPaidMembershipFromTable(
   return true;
 }
 
-export async function checkPaidMembershipFromEntra(
+/**
+ * This check is slow!! Don't use it unless you have a really good reason.
+ * Membership data is replicated to DynamoDB on provision (and EntraID second) so just read from the user-info table. */
+async function checkPaidMembershipFromEntra(
   netId: string,
   entraToken: string,
   paidMemberGroup: string,
diff --git a/src/api/routes/membership.ts b/src/api/routes/membership.ts
@@ -1,6 +1,5 @@
 import {
   checkExternalMembership,
-  checkPaidMembershipFromEntra,
   checkPaidMembershipFromTable,
   setPaidMembershipInTable,
   MEMBER_CACHE_SECONDS,
@@ -188,41 +187,8 @@ const membershipPlugin: FastifyPluginAsync = async (fastify, _options) => {
             .header("X-ACM-Data-Source", "dynamo")
             .send({ givenName, surname, netId, isPaidMember: true });
         }
-        const entraIdToken = await getEntraIdToken({
-          clients: await getAuthorizedClients(),
-          clientId: fastify.environmentConfig.AadValidClientId,
-          secretName: genericConfig.EntraSecretName,
-          logger: request.log,
-        });
-        const paidMemberGroup = fastify.environmentConfig.PaidMemberGroupId;
-        const isAadMember = await checkPaidMembershipFromEntra(
-          netId,
-          entraIdToken,
-          paidMemberGroup,
-        );
-        if (isAadMember) {
-          await setKey({
-            redisClient: fastify.redisClient,
-            key: cacheKey,
-            data: JSON.stringify({ isMember: true }),
-            expiresIn: MEMBER_CACHE_SECONDS,
-            logger: request.log,
-          });
-          reply
-            .header("X-ACM-Data-Source", "aad")
-            .send({ givenName, surname, netId, isPaidMember: true });
-          await setPaidMembershipInTable(netId, fastify.dynamoClient);
-          return;
-        }
-        await setKey({
-          redisClient: fastify.redisClient,
-          key: cacheKey,
-          data: JSON.stringify({ isMember: false }),
-          expiresIn: MEMBER_CACHE_SECONDS,
-          logger: request.log,
-        });
         return reply
-          .header("X-ACM-Data-Source", "aad")
+          .header("X-ACM-Data-Source", "dynamo")
           .send({ givenName, surname, netId, isPaidMember: false });
       },
     );
diff --git a/src/api/routes/v2/membership.ts b/src/api/routes/v2/membership.ts
@@ -3,7 +3,6 @@ import {
   checkPaidMembershipFromRedis,
   checkExternalMembership,
   MEMBER_CACHE_SECONDS,
-  checkPaidMembershipFromEntra,
   setPaidMembershipInTable,
 } from "api/functions/membership.js";
 import { FastifyPluginAsync } from "fastify";
@@ -321,47 +320,10 @@ const membershipV2Plugin: FastifyPluginAsync = async (fastify, _options) => {
                   "EX",
                   MEMBER_CACHE_SECONDS,
                 );
-              }
-            }
-          }
-
-          const netIdsForEntra = netIdsToCheck.filter(
-            (id) => !foundInDynamo.has(id),
-          );
-          if (netIdsForEntra.length > 0) {
-            const entraIdToken = await getEntraIdToken({
-              clients: await getAuthorizedClients(),
-              clientId: fastify.environmentConfig.AadValidClientId,
-              secretName: genericConfig.EntraSecretName,
-              logger: request.log,
-            });
-            const paidMemberGroup = fastify.environmentConfig.PaidMemberGroupId;
-            const entraCheckPromises = netIdsForEntra.map(async (netId) => {
-              const isMember = await checkPaidMembershipFromEntra(
-                netId,
-                entraIdToken,
-                paidMemberGroup,
-              );
-              if (isMember) {
-                members.add(netId);
-                setPaidMembershipInTable(netId, fastify.dynamoClient).catch(
-                  (err) =>
-                    request.log.error(
-                      err,
-                      `Failed to write back Entra membership for ${netId}`,
-                    ),
-                );
               } else {
                 notMembers.add(netId);
               }
-              cachePipeline.set(
-                `membership:${netId}:${list}`,
-                JSON.stringify({ isMember }),
-                "EX",
-                MEMBER_CACHE_SECONDS,
-              );
-            });
-            await Promise.all(entraCheckPromises);
+            }
           }
         }
 
@@ -491,32 +453,6 @@ const membershipV2Plugin: FastifyPluginAsync = async (fastify, _options) => {
             .header("X-ACM-Data-Source", "dynamo")
             .send({ netId, isPaidMember: true });
         }
-        const entraIdToken = await getEntraIdToken({
-          clients: await getAuthorizedClients(),
-          clientId: fastify.environmentConfig.AadValidClientId,
-          secretName: genericConfig.EntraSecretName,
-          logger: request.log,
-        });
-        const paidMemberGroup = fastify.environmentConfig.PaidMemberGroupId;
-        const isAadMember = await checkPaidMembershipFromEntra(
-          netId,
-          entraIdToken,
-          paidMemberGroup,
-        );
-        if (isAadMember) {
-          await setKey({
-            redisClient: fastify.redisClient,
-            key: cacheKey,
-            data: JSON.stringify({ isMember: true }),
-            expiresIn: MEMBER_CACHE_SECONDS,
-            logger: request.log,
-          });
-          reply
-            .header("X-ACM-Data-Source", "aad")
-            .send({ netId, isPaidMember: true });
-          await setPaidMembershipInTable(netId, fastify.dynamoClient);
-          return;
-        }
         await setKey({
           redisClient: fastify.redisClient,
           key: cacheKey,
@@ -525,7 +461,7 @@ const membershipV2Plugin: FastifyPluginAsync = async (fastify, _options) => {
           logger: request.log,
         });
         return reply
-          .header("X-ACM-Data-Source", "aad")
+          .header("X-ACM-Data-Source", "dynamo")
           .send({ netId, isPaidMember: false });
       },
     );
diff --git a/terraform/modules/dynamo/main.tf b/terraform/modules/dynamo/main.tf
@@ -159,10 +159,26 @@ resource "aws_dynamodb_table" "external_membership" {
 
 }
 
+
+resource "aws_dynamodb_table" "iam_assignments" {
+  billing_mode                = "PAY_PER_REQUEST"
+  name                        = "${var.ProjectId}-iam-assignments"
+  deletion_protection_enabled = true
+  hash_key                    = "id"
+  point_in_time_recovery {
+    enabled = true
+  }
+  attribute {
+    name = "id"
+    type = "S"
+  }
+}
+
+
 resource "aws_dynamodb_table" "iam_group_roles" {
   billing_mode                = "PAY_PER_REQUEST"
   name                        = "${var.ProjectId}-iam-grouproles"
-  deletion_protection_enabled = true
+  deletion_protection_enabled = false
   hash_key                    = "groupUuid"
   point_in_time_recovery {
     enabled = true
@@ -176,7 +192,7 @@ resource "aws_dynamodb_table" "iam_group_roles" {
 resource "aws_dynamodb_table" "iam_user_roles" {
   billing_mode                = "PAY_PER_REQUEST"
   name                        = "${var.ProjectId}-iam-userroles"
-  deletion_protection_enabled = true
+  deletion_protection_enabled = false
   hash_key                    = "userEmail"
   point_in_time_recovery {
     enabled = true
diff --git a/terraform/modules/lambdas/main.tf b/terraform/modules/lambdas/main.tf
@@ -221,8 +221,7 @@ resource "aws_iam_policy" "shared_iam_policy" {
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-events-tickets",
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-events-ticketing-metadata",
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-merchstore-metadata",
-          "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-iam-userroles",
-          "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-iam-grouproles",
+          "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-iam-assignments",
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-stripe-links",
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-stripe-links/index/*",
           "arn:aws:dynamodb:${data.aws_region.current.region}:${data.aws_caller_identity.current.account_id}:table/infra-core-api-membership-external-v3",
diff --git a/tests/live/membership.test.ts b/tests/live/membership.test.ts
@@ -55,7 +55,7 @@ describe("Membership API basic checks", async () => {
         isPaidMember: true,
       });
 
-      const wasCached = (value: string | null) => value && value !== "aad";
+      const wasCached = (value: string | null) => value && value !== "dynamo";
 
       expect(wasCached(response.headers.get("x-acm-data-source"))).toBe(true);
     },
diff --git a/tests/unit/membership.test.ts b/tests/unit/membership.test.ts
@@ -46,7 +46,7 @@ describe("Test membership routes", async () => {
     expect(response.statusCode).toBe(200);
     const responseDataJson = (await response.json()) as EventGetResponse;
     expect(response.headers).toHaveProperty("x-acm-data-source");
-    expect(response.headers["x-acm-data-source"]).toEqual("aad");
+    expect(response.headers["x-acm-data-source"]).toEqual("dynamo");
     expect(responseDataJson).toEqual({
       givenName: "Infra",
       surname: "Testing",
@@ -87,42 +87,10 @@ describe("Test membership routes", async () => {
     expect(response.statusCode).toBe(200);
     const responseDataJson = (await response.json()) as EventGetResponse;
     expect(response.headers).toHaveProperty("x-acm-data-source");
-    expect(response.headers["x-acm-data-source"]).toEqual("aad");
+    expect(response.headers["x-acm-data-source"]).toEqual("dynamo");
     expect(responseDataJson).toEqual({ netId: "invalid", isPaidMember: false });
   });
 
-  test("Entra-only members are added to Dynamo", async () => {
-    const testJwt = createJwt();
-    let response = await app.inject({
-      method: "GET",
-      url: "/api/v2/membership/eadon2",
-      headers: {
-        authorization: `Bearer ${testJwt}`,
-      },
-    });
-
-    expect(response.statusCode).toBe(200);
-    let responseDataJson = (await response.json()) as EventGetResponse;
-    expect(response.headers).toHaveProperty("x-acm-data-source");
-    expect(response.headers["x-acm-data-source"]).toEqual("aad");
-    expect(responseDataJson).toEqual({ netId: "eadon2", isPaidMember: true });
-    expect(spySetPaidMembership).toHaveBeenCalledWith(
-      "eadon2",
-      expect.any(Object),
-    );
-    response = await app.inject({
-      method: "GET",
-      url: "/api/v2/membership/eadon2",
-      headers: {
-        authorization: `Bearer ${testJwt}`,
-      },
-    });
-    expect(response.statusCode).toBe(200);
-    responseDataJson = (await response.json()) as EventGetResponse;
-    expect(response.headers).toHaveProperty("x-acm-data-source");
-    expect(response.headers["x-acm-data-source"]).toEqual("cache");
-    expect(responseDataJson).toEqual({ netId: "eadon2", isPaidMember: true });
-  });
   test("External list members are correctly found", async () => {
     ddbMock.on(QueryCommand).callsFake((command) => {
       if (
diff --git a/tests/unit/vitest.setup.ts b/tests/unit/vitest.setup.ts
@@ -132,18 +132,6 @@ vi.mock(
             return false;
         }
       }),
-      checkPaidMembershipFromEntra: vi.fn(
-        async (netId, _entraToken, _paidMemberGroup) => {
-          switch (netId) {
-            case "valid":
-              return true;
-            case "eadon2":
-              return true;
-            default:
-              return false;
-          }
-        },
-      ),
     };
   },
 );
