Address PR comments

backend/src/db/migrations/20250926000000_saml-configs-group-sync-fields.ts

@@ -5,19 +5,19 @@ import { TableName } from "../schemas";
 export async function up(knex: Knex): Promise<void> {
   const hasEnableGroupSyncCol = await knex.schema.hasColumn(TableName.SamlConfig, "enableGroupSync");
 
-  await knex.schema.alterTable(TableName.SamlConfig, (tb) => {
+  if (!hasEnableGroupSyncCol) {
-    if (!hasEnableGroupSyncCol) {
+    await knex.schema.alterTable(TableName.SamlConfig, (tb) => {
       tb.boolean("enableGroupSync").notNullable().defaultTo(false);
-    }
+    });
-  });
+  }
 }
 
 export async function down(knex: Knex): Promise<void> {
   const hasEnableGroupSyncCol = await knex.schema.hasColumn(TableName.SamlConfig, "enableGroupSync");
 
-  await knex.schema.alterTable(TableName.SamlConfig, (t) => {
+  if (hasEnableGroupSyncCol) {
-    if (hasEnableGroupSyncCol) {
+    await knex.schema.alterTable(TableName.SamlConfig, (t) => {
       t.dropColumn("enableGroupSync");
-    }
+    });
-  });
+  }
 }

backend/src/ee/routes/v1/saml-router.ts

@@ -327,7 +327,7 @@ export const registerSamlRouter = async (server: FastifyZodProvider) => {
         entryPoint: z.string().trim().describe(SamlSso.CREATE_CONFIG.entryPoint),
         issuer: z.string().trim().describe(SamlSso.CREATE_CONFIG.issuer),
         cert: z.string().trim().describe(SamlSso.CREATE_CONFIG.cert),
-        enableGroupSync: z.boolean().optional()
+        enableGroupSync: z.boolean().optional().describe(SamlSso.CREATE_CONFIG.enableGroupSync)
       }),
       response: {
         200: SanitizedSamlConfigSchema
@@ -376,7 +376,7 @@ export const registerSamlRouter = async (server: FastifyZodProvider) => {
           entryPoint: z.string().trim().describe(SamlSso.UPDATE_CONFIG.entryPoint),
           issuer: z.string().trim().describe(SamlSso.UPDATE_CONFIG.issuer),
           cert: z.string().trim().describe(SamlSso.UPDATE_CONFIG.cert),
-          enableGroupSync: z.boolean().optional()
+          enableGroupSync: z.boolean().optional().describe(SamlSso.UPDATE_CONFIG.enableGroupSync)
         })
         .partial()
         .merge(z.object({ organizationId: z.string().trim().describe(SamlSso.UPDATE_CONFIG.organizationId) })),

backend/src/ee/services/saml-config/saml-config-service.ts

@@ -1,6 +1,7 @@
 /* eslint-disable no-await-in-loop */
 import { ForbiddenError } from "@casl/ability";
 import { Knex } from "knex";
+import RE2 from "re2";
 
 import {
   OrgMembershipRole,
@@ -103,6 +104,31 @@ export const samlConfigServiceFactory = ({
   identityMetadataDAL,
   kmsService
 }: TSamlConfigServiceFactoryDep): TSamlConfigServiceFactory => {
+  const parseSamlGroups = (groupsValue: string): string[] => {
+    let samlGroups: string[] = [];
+
+    try {
+      // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+      const parsed = JSON.parse(groupsValue);
+      if (Array.isArray(parsed)) {
+        // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+        samlGroups = parsed;
+      } else if (typeof parsed === "string") {
+        samlGroups = parsed
+          .split(",")
+          .map((g) => g.trim())
+          .filter(Boolean);
+      }
+    } catch {
+      samlGroups = groupsValue
+        .split(",")
+        .map((g) => g.trim())
+        .filter(Boolean);
+    }
+
+    return samlGroups;
+  };
+
   const syncUserGroupMemberships = async ({
     userId,
     orgId,
@@ -147,9 +173,9 @@ export const samlConfigServiceFactory = ({
           const newGroup = await groupDAL.create(
             {
               name: groupName,
-              slug: `${groupName.toLowerCase().replace(/[^a-z0-9]/g, "-")}-${Date.now()}`,
+              slug: `${groupName.toLowerCase().replace(new RE2("[^a-z0-9]", "g"), "-")}-${Date.now()}`,
               orgId,
-              role: OrgMembershipRole.Member,
+              role: OrgMembershipRole.NoAccess,
               roleId: null
             },
             transaction
@@ -249,7 +275,7 @@ export const samlConfigServiceFactory = ({
 
     if (enableGroupSync && !GROUP_SYNC_SUPPORTED_PROVIDERS.includes(authProvider)) {
       throw new BadRequestError({
-        message: "Group sync is only supported for Google SAML SSO."
+        message: "Group sync is not supported for this SAML provider."
       });
     }
 
@@ -458,7 +484,7 @@ export const samlConfigServiceFactory = ({
 
     const samlConfig = await samlConfigDAL.findOne({ orgId });
     const groupsMetadata = metadata?.find(({ key }) => key === "groups");
-    const shouldSyncGroups = !!(samlConfig?.enableGroupSync && groupsMetadata?.value);
+    const shouldSyncGroups = !!samlConfig?.enableGroupSync;
 
     let user: TUsers;
     if (userAlias) {
@@ -481,7 +507,7 @@ export const samlConfigServiceFactory = ({
               orgId,
               role,
               roleId,
-              status: foundUser.isAccepted ? OrgMembershipStatus.Accepted : OrgMembershipStatus.Invited,
+              status: foundUser.isAccepted ? OrgMembershipStatus.Accepted : OrgMembershipStatus.Invited, 
               isActive: true
             },
             tx
@@ -512,36 +538,15 @@ export const samlConfigServiceFactory = ({
           }
         }
 
-        if (shouldSyncGroups && metadata && foundUser.id && groupsMetadata?.value) {
+        if (shouldSyncGroups && metadata && foundUser.id) {
-          let samlGroups: string[] = [];
+          const samlGroups = groupsMetadata?.value ? parseSamlGroups(groupsMetadata.value) : [];
 
-          try {
+          await syncUserGroupMemberships({
-            // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+            userId: foundUser.id,
-            const parsed = JSON.parse(groupsMetadata.value);
+            orgId,
-            if (Array.isArray(parsed)) {
+            samlGroups,
-              // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+            tx
-              samlGroups = parsed;
+          });
-            } else if (typeof parsed === "string") {
-              samlGroups = parsed
-                .split(",")
-                .map((g) => g.trim())
-                .filter(Boolean);
-            }
-          } catch {
-            samlGroups = groupsMetadata.value
-              .split(",")
-              .map((g) => g.trim())
-              .filter(Boolean);
-          }
-
-          if (samlGroups.length > 0) {
-            await syncUserGroupMemberships({
-              userId: foundUser.id,
-              orgId,
-              samlGroups,
-              tx
-            });
-          }
         }
 
         return foundUser;
@@ -605,11 +610,12 @@ export const samlConfigServiceFactory = ({
               orgId,
               role,
               roleId,
-              status: newUser.isAccepted ? OrgMembershipStatus.Accepted : OrgMembershipStatus.Invited,
+              status: newUser.isAccepted ? OrgMembershipStatus.Accepted : OrgMembershipStatus.Invited, // if user is fully completed, then set status to accepted, otherwise set it to invited so we can update it later
               isActive: true
             },
             tx
           );
+          // Only update the membership to Accepted if the user account is already completed.
         } else if (orgMembership.status === OrgMembershipStatus.Invited && newUser.isAccepted) {
           await orgDAL.updateMembershipById(
             orgMembership.id,
@@ -635,36 +641,15 @@ export const samlConfigServiceFactory = ({
           }
         }
 
-        if (shouldSyncGroups && metadata && newUser.id && groupsMetadata?.value) {
+        if (shouldSyncGroups && metadata && newUser.id) {
-          let samlGroups: string[] = [];
+          const samlGroups = groupsMetadata?.value ? parseSamlGroups(groupsMetadata.value) : [];
 
-          try {
+          await syncUserGroupMemberships({
-            // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+            userId: newUser.id,
-            const parsed = JSON.parse(groupsMetadata.value);
+            orgId,
-            if (Array.isArray(parsed)) {
+            samlGroups,
-              // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
+            tx
-              samlGroups = parsed;
+          });
-            } else if (typeof parsed === "string") {
-              samlGroups = parsed
-                .split(",")
-                .map((g) => g.trim())
-                .filter(Boolean);
-            }
-          } catch {
-            samlGroups = groupsMetadata.value
-              .split(",")
-              .map((g) => g.trim())
-              .filter(Boolean);
-          }
-
-          if (samlGroups.length > 0) {
-            await syncUserGroupMemberships({
-              userId: newUser.id,
-              orgId,
-              samlGroups,
-              tx
-            });
-          }
         }
 
         return newUser;

backend/src/lib/api-docs/constants.ts

@@ -2872,7 +2872,9 @@ export const SamlSso = {
     entryPoint:
       "The entry point for the SAML authentication. This is the URL that the user will be redirected to after they have authenticated with the SAML provider.",
     issuer: "The SAML provider issuer URL or entity ID.",
-    cert: "The certificate to use for SAML authentication."
+    cert: "The certificate to use for SAML authentication.",
+    enableGroupSync:
+      "Whether to enable automatic synchronization of group memberships from the SAML provider to Infisical groups."
   },
   CREATE_CONFIG: {
     organizationId: "The ID of the organization to create the SAML config for.",
@@ -2881,7 +2883,9 @@ export const SamlSso = {
     entryPoint:
       "The entry point for the SAML authentication. This is the URL that the user will be redirected to after they have authenticated with the SAML provider.",
     issuer: "The SAML provider issuer URL or entity ID.",
-    cert: "The certificate to use for SAML authentication."
+    cert: "The certificate to use for SAML authentication.",
+    enableGroupSync:
+      "Whether to enable automatic synchronization of group memberships from the SAML provider to Infisical groups."
   }
 };
 

docs/documentation/platform/sso/google-saml.mdx

@@ -54,10 +54,10 @@ description: "Learn how to configure Google SAML for Infisical SSO."
         ![Google SAML attribute mapping](../../../images/sso/google-saml/attribute-mapping.png)
 
         <Note>
-        For group membership mapping (optional), you can also configure:
+        If you want to sync Google groups to Infisical groups, you can also configure:
-        - **groups** -> **groups** (if you want to sync Google groups to Infisical groups)
+        - **groups** -> **groups**
 
-        This requires setting up group claims in Google Workspace. See the Group Membership Mapping section below for details.
+        This requires setting up group claims in Google Workspace. See the [Group Membership Mapping](#saml-group-membership-mapping) section below for details.
         </Note>
 
         Click **Finish**.
@@ -116,8 +116,15 @@ Automatically sync Google Workspace group memberships to Infisical.
 
         ![Google SAML group membership mapping](../../../images/sso/google-saml/group-membership-mapping.png)
     </Step>
+    <Step title="Group synchronization on login">
+        Once configured, Google groups will now be automatically synchronized when users log in through SAML. Users will be added to or removed from Infisical groups based on their current Google group memberships.
+    </Step>
 </Steps>
 
+<Warning>
+Group membership changes in the SAML provider only sync with Infisical when a user logs in via SAML. For example, if you remove a user from a group in the SAML provider, this change will not be reflected in Infisical until their next SAML login. To ensure this behavior, Infisical recommends enabling Enforce SAML SSO.
+</Warning>
+
 <Tip>
     If you are only using one organization on your Infisical instance, you can configure a default organization in the [Server Admin Console](../admin-panel/server-admin#default-organization) to expedite SAML login.
 </Tip>

frontend/src/hooks/api/ssoConfig/queries.tsx

@@ -34,7 +34,8 @@ export const useCreateSSOConfig = () => {
       isActive,
       entryPoint,
       issuer,
-      cert
+      cert,
+      enableGroupSync
     }: {
       organizationId: string;
       authProvider: string;
@@ -42,6 +43,7 @@ export const useCreateSSOConfig = () => {
       entryPoint: string;
       issuer: string;
       cert: string;
+      enableGroupSync?: boolean;
     }) => {
       const { data } = await apiRequest.post("/api/v1/sso/config", {
         organizationId,
@@ -49,7 +51,8 @@ export const useCreateSSOConfig = () => {
         isActive,
         entryPoint,
         issuer,
-        cert
+        cert,
+        ...(enableGroupSync !== undefined ? { enableGroupSync } : {})
       });
 
       return data;