From ab2265b89092692c6d556b49bc5d2702aacd898d Mon Sep 17 00:00:00 2001 From: Carlos Monastyrski Date: Tue, 30 Sep 2025 00:29:20 -0300 Subject: [PATCH] Address PR comments --- ...26000000_saml-configs-group-sync-fields.ts | 16 +-- backend/src/ee/routes/v1/saml-router.ts | 4 +- .../saml-config/saml-config-service.ts | 113 ++++++++---------- backend/src/lib/api-docs/constants.ts | 8 +- .../platform/sso/google-saml.mdx | 13 +- frontend/src/hooks/api/ssoConfig/queries.tsx | 7 +- 6 files changed, 80 insertions(+), 81 deletions(-) diff --git a/backend/src/db/migrations/20250926000000_saml-configs-group-sync-fields.ts b/backend/src/db/migrations/20250926000000_saml-configs-group-sync-fields.ts index a183cbf33..4b637e809 100644 --- a/backend/src/db/migrations/20250926000000_saml-configs-group-sync-fields.ts +++ b/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 { 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 { 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"); - } - }); + }); + } } diff --git a/backend/src/ee/routes/v1/saml-router.ts b/backend/src/ee/routes/v1/saml-router.ts index 2ee0d5016..76bff60e8 100644 --- a/backend/src/ee/routes/v1/saml-router.ts +++ b/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) })), diff --git a/backend/src/ee/services/saml-config/saml-config-service.ts b/backend/src/ee/services/saml-config/saml-config-service.ts index a4bb321ad..61d605ab4 100644 --- a/backend/src/ee/services/saml-config/saml-config-service.ts +++ b/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) { - let samlGroups: string[] = []; + if (shouldSyncGroups && metadata && foundUser.id) { + const samlGroups = groupsMetadata?.value ? parseSamlGroups(groupsMetadata.value) : []; - try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const parsed = JSON.parse(groupsMetadata.value); - 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 = groupsMetadata.value - .split(",") - .map((g) => g.trim()) - .filter(Boolean); - } - - if (samlGroups.length > 0) { - await syncUserGroupMemberships({ - userId: foundUser.id, - orgId, - samlGroups, - tx - }); - } + 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) { - let samlGroups: string[] = []; + if (shouldSyncGroups && metadata && newUser.id) { + const samlGroups = groupsMetadata?.value ? parseSamlGroups(groupsMetadata.value) : []; - try { - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - const parsed = JSON.parse(groupsMetadata.value); - 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 = groupsMetadata.value - .split(",") - .map((g) => g.trim()) - .filter(Boolean); - } - - if (samlGroups.length > 0) { - await syncUserGroupMemberships({ - userId: newUser.id, - orgId, - samlGroups, - tx - }); - } + await syncUserGroupMemberships({ + userId: newUser.id, + orgId, + samlGroups, + tx + }); } return newUser; diff --git a/backend/src/lib/api-docs/constants.ts b/backend/src/lib/api-docs/constants.ts index 2ac8f996e..2d0de59a2 100644 --- a/backend/src/lib/api-docs/constants.ts +++ b/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." } }; diff --git a/docs/documentation/platform/sso/google-saml.mdx b/docs/documentation/platform/sso/google-saml.mdx index f79998afa..639a11321 100644 --- a/docs/documentation/platform/sso/google-saml.mdx +++ b/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) - For group membership mapping (optional), you can also configure: - - **groups** -> **groups** (if you want to sync Google groups to Infisical groups) + If you want to sync Google groups to Infisical groups, you can also configure: + - **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. 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) + + 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. + + +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. + + 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. diff --git a/frontend/src/hooks/api/ssoConfig/queries.tsx b/frontend/src/hooks/api/ssoConfig/queries.tsx index 92189f2c2..3ee5ec1b1 100644 --- a/frontend/src/hooks/api/ssoConfig/queries.tsx +++ b/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;