From 95d7c2082ccd4036472ae5de3e91568515de38af Mon Sep 17 00:00:00 2001 From: Scott Wilson Date: Fri, 31 Jan 2025 11:01:54 -0800 Subject: [PATCH] improvements: address feedback --- .../ee/services/audit-log/audit-log-types.ts | 28 ++++++++++- .../src/ee/services/group/group-service.ts | 8 +++- .../ee/services/oidc/oidc-config-service.ts | 46 ++++++++++++++++++- backend/src/server/routes/index.ts | 3 +- .../group-membership-mapping.mdx | 3 +- .../src/hooks/api/auditLogs/constants.tsx | 6 ++- frontend/src/hooks/api/auditLogs/enums.tsx | 4 +- .../AuditLogsPage/components/LogsTableRow.tsx | 6 +++ .../components/AddGroupMemberModal.tsx | 4 +- .../components/OrgAuthTab/OrgOIDCSection.tsx | 6 ++- 10 files changed, 100 insertions(+), 14 deletions(-) diff --git a/backend/src/ee/services/audit-log/audit-log-types.ts b/backend/src/ee/services/audit-log/audit-log-types.ts index 6e8314731..ffabb3cc4 100644 --- a/backend/src/ee/services/audit-log/audit-log-types.ts +++ b/backend/src/ee/services/audit-log/audit-log-types.ts @@ -249,7 +249,9 @@ export enum EventType { DELETE_SECRET_SYNC = "delete-secret-sync", SECRET_SYNC_SYNC_SECRETS = "secret-sync-sync-secrets", SECRET_SYNC_IMPORT_SECRETS = "secret-sync-import-secrets", - SECRET_SYNC_REMOVE_SECRETS = "secret-sync-remove-secrets" + SECRET_SYNC_REMOVE_SECRETS = "secret-sync-remove-secrets", + OIDC_GROUP_MEMBERSHIP_MAPPING_ASSIGN_USER = "oidc-group-membership-mapping-assign-user", + OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER = "oidc-group-membership-mapping-remove-user" } interface UserActorMetadata { @@ -2044,6 +2046,26 @@ interface SecretSyncRemoveSecretsEvent { }; } +interface OidcGroupMembershipMappingAssignUserEvent { + type: EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_ASSIGN_USER; + metadata: { + assignedToGroups: { id: string; name: string }[]; + userId: string; + userEmail: string; + userGroupsClaim: string[]; + }; +} + +interface OidcGroupMembershipMappingRemoveUserEvent { + type: EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER; + metadata: { + removedFromGroups: { id: string; name: string }[]; + userId: string; + userEmail: string; + userGroupsClaim: string[]; + }; +} + export type Event = | GetSecretsEvent | GetSecretEvent @@ -2232,4 +2254,6 @@ export type Event = | DeleteSecretSyncEvent | SecretSyncSyncSecretsEvent | SecretSyncImportSecretsEvent - | SecretSyncRemoveSecretsEvent; + | SecretSyncRemoveSecretsEvent + | OidcGroupMembershipMappingAssignUserEvent + | OidcGroupMembershipMappingRemoveUserEvent; diff --git a/backend/src/ee/services/group/group-service.ts b/backend/src/ee/services/group/group-service.ts index 30a37e22e..7de3f8f92 100644 --- a/backend/src/ee/services/group/group-service.ts +++ b/backend/src/ee/services/group/group-service.ts @@ -320,7 +320,10 @@ export const groupServiceFactory = ({ }); if (oidcConfig?.manageGroupMemberships) { - throw new BadRequestError({ message: "Cannot add user to group: OIDC group membership mapping is enabled." }); + throw new BadRequestError({ + message: + "Cannot add user to group: OIDC group membership mapping is enabled - user must be assigned to this group in your OIDC provider." + }); } const { permission: groupRolePermission } = await permissionService.getOrgPermissionByRole(group.role, actorOrgId); @@ -385,7 +388,8 @@ export const groupServiceFactory = ({ if (oidcConfig?.manageGroupMemberships) { throw new BadRequestError({ - message: "Cannot remove user from group: OIDC group membership mapping is enabled." + message: + "Cannot remove user from group: OIDC group membership mapping is enabled - user must be removed from this group in your OIDC provider." }); } diff --git a/backend/src/ee/services/oidc/oidc-config-service.ts b/backend/src/ee/services/oidc/oidc-config-service.ts index b17d40b69..0c037a2d3 100644 --- a/backend/src/ee/services/oidc/oidc-config-service.ts +++ b/backend/src/ee/services/oidc/oidc-config-service.ts @@ -5,6 +5,8 @@ import { Issuer, Issuer as OpenIdIssuer, Strategy as OpenIdStrategy, TokenSet } import { OrgMembershipStatus, SecretKeyEncoding, TableName, TUsers } from "@app/db/schemas"; import { TOidcConfigsUpdate } from "@app/db/schemas/oidc-configs"; +import { TAuditLogServiceFactory } from "@app/ee/services/audit-log/audit-log-service"; +import { EventType } from "@app/ee/services/audit-log/audit-log-types"; import { TGroupDALFactory } from "@app/ee/services/group/group-dal"; import { addUsersToGroupByUserIds, removeUsersFromGroupByUserIds } from "@app/ee/services/group/group-fns"; import { TUserGroupMembershipDALFactory } from "@app/ee/services/group/user-group-membership-dal"; @@ -22,7 +24,7 @@ import { } from "@app/lib/crypto/encryption"; import { BadRequestError, ForbiddenRequestError, NotFoundError, OidcAuthError } from "@app/lib/errors"; import { OrgServiceActor } from "@app/lib/types"; -import { AuthMethod, AuthTokenType } from "@app/services/auth/auth-type"; +import { ActorType, AuthMethod, AuthTokenType } from "@app/services/auth/auth-type"; import { TAuthTokenServiceFactory } from "@app/services/auth-token/auth-token-service"; import { TokenType } from "@app/services/auth-token/auth-token-types"; import { TGroupProjectDALFactory } from "@app/services/group-project/group-project-dal"; @@ -88,6 +90,7 @@ type TOidcConfigServiceFactoryDep = { projectKeyDAL: Pick; projectDAL: Pick; projectBotDAL: Pick; + auditLogService: Pick; }; export type TOidcConfigServiceFactory = ReturnType; @@ -108,7 +111,8 @@ export const oidcConfigServiceFactory = ({ groupProjectDAL, projectKeyDAL, projectDAL, - projectBotDAL + projectBotDAL, + auditLogService }: TOidcConfigServiceFactoryDep) => { const getOidc = async (dto: TGetOidcCfgDTO) => { const org = await orgDAL.findOne({ slug: dto.orgSlug }); @@ -382,6 +386,25 @@ export const oidcConfigServiceFactory = ({ }); } + if (groupsToAddUserTo.length) { + await auditLogService.createAuditLog({ + actor: { + type: ActorType.PLATFORM, + metadata: {} + }, + orgId, + event: { + type: EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_ASSIGN_USER, + metadata: { + userId: user.id, + userEmail: user.email ?? user.username, + assignedToGroups: groupsToAddUserTo.map(({ id, name }) => ({ id, name })), + userGroupsClaim: groups + } + } + }); + } + const membershipsToRemove = userGroups .filter((membership) => !groups.includes(membership.groupName)) .map((membership) => membership.groupId); @@ -397,6 +420,25 @@ export const oidcConfigServiceFactory = ({ projectKeyDAL }); } + + if (groupsToRemoveUserFrom.length) { + await auditLogService.createAuditLog({ + actor: { + type: ActorType.PLATFORM, + metadata: {} + }, + orgId, + event: { + type: EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER, + metadata: { + userId: user.id, + userEmail: user.email ?? user.username, + removedFromGroups: groupsToRemoveUserFrom.map(({ id, name }) => ({ id, name })), + userGroupsClaim: groups + } + } + }); + } } await licenseService.updateSubscriptionOrgMemberCount(organization.id); diff --git a/backend/src/server/routes/index.ts b/backend/src/server/routes/index.ts index 00010b7d5..3e5947956 100644 --- a/backend/src/server/routes/index.ts +++ b/backend/src/server/routes/index.ts @@ -1344,7 +1344,8 @@ export const registerRoutes = async ( projectDAL, userGroupMembershipDAL, groupProjectDAL, - groupDAL + groupDAL, + auditLogService }); const userEngagementService = userEngagementServiceFactory({ diff --git a/docs/documentation/platform/sso/keycloak-oidc/group-membership-mapping.mdx b/docs/documentation/platform/sso/keycloak-oidc/group-membership-mapping.mdx index a75564ed8..c423bac5a 100644 --- a/docs/documentation/platform/sso/keycloak-oidc/group-membership-mapping.mdx +++ b/docs/documentation/platform/sso/keycloak-oidc/group-membership-mapping.mdx @@ -16,7 +16,8 @@ Infisical groups not present in their groups claim. Group membership changes in the Keycloak only sync with Infisical when a - user logs in. For example, if you remove a user from a group in Keycloak, this change will not be reflected in Infisical until their next login. + user logs in via OIDC. For example, if you remove a user from a group in Keycloak, this change will not be reflected in Infisical until their next OIDC login. To ensure this behavior, Infisical recommends enabling Enforce OIDC + SSO in the OIDC settings. diff --git a/frontend/src/hooks/api/auditLogs/constants.tsx b/frontend/src/hooks/api/auditLogs/constants.tsx index c736a564e..6a990f80e 100644 --- a/frontend/src/hooks/api/auditLogs/constants.tsx +++ b/frontend/src/hooks/api/auditLogs/constants.tsx @@ -114,7 +114,11 @@ export const eventToNameMap: { [K in EventType]: string } = { [EventType.DELETE_SECRET_SYNC]: "Delete Secret Sync", [EventType.SECRET_SYNC_SYNC_SECRETS]: "Secret Sync synced secrets", [EventType.SECRET_SYNC_IMPORT_SECRETS]: "Secret Sync imported secrets", - [EventType.SECRET_SYNC_REMOVE_SECRETS]: "Secret Sync removed secrets" + [EventType.SECRET_SYNC_REMOVE_SECRETS]: "Secret Sync removed secrets", + [EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_ASSIGN_USER]: + "OIDC group membership mapping assigned user to groups", + [EventType.OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER]: + "OIDC group membership mapping removed user from groups" }; export const userAgentTTypeoNameMap: { [K in UserAgentType]: string } = { diff --git a/frontend/src/hooks/api/auditLogs/enums.tsx b/frontend/src/hooks/api/auditLogs/enums.tsx index 7219a446e..349811180 100644 --- a/frontend/src/hooks/api/auditLogs/enums.tsx +++ b/frontend/src/hooks/api/auditLogs/enums.tsx @@ -127,5 +127,7 @@ export enum EventType { DELETE_SECRET_SYNC = "delete-secret-sync", SECRET_SYNC_SYNC_SECRETS = "secret-sync-sync-secrets", SECRET_SYNC_IMPORT_SECRETS = "secret-sync-import-secrets", - SECRET_SYNC_REMOVE_SECRETS = "secret-sync-remove-secrets" + SECRET_SYNC_REMOVE_SECRETS = "secret-sync-remove-secrets", + OIDC_GROUP_MEMBERSHIP_MAPPING_ASSIGN_USER = "oidc-group-membership-mapping-assign-user", + OIDC_GROUP_MEMBERSHIP_MAPPING_REMOVE_USER = "oidc-group-membership-mapping-remove-user" } diff --git a/frontend/src/pages/organization/AuditLogsPage/components/LogsTableRow.tsx b/frontend/src/pages/organization/AuditLogsPage/components/LogsTableRow.tsx index 5e663f283..f7c7398ee 100644 --- a/frontend/src/pages/organization/AuditLogsPage/components/LogsTableRow.tsx +++ b/frontend/src/pages/organization/AuditLogsPage/components/LogsTableRow.tsx @@ -40,6 +40,12 @@ export const LogsTableRow = ({ auditLog, isOrgAuditLogs, showActorColumn }: Prop

Machine Identity

); + case ActorType.PLATFORM: + return ( + +

Platform

+ + ); case ActorType.UNKNOWN_USER: return ( diff --git a/frontend/src/pages/organization/GroupDetailsByIDPage/components/AddGroupMemberModal.tsx b/frontend/src/pages/organization/GroupDetailsByIDPage/components/AddGroupMemberModal.tsx index e9ac094c2..bcebea5c7 100644 --- a/frontend/src/pages/organization/GroupDetailsByIDPage/components/AddGroupMemberModal.tsx +++ b/frontend/src/pages/organization/GroupDetailsByIDPage/components/AddGroupMemberModal.tsx @@ -82,9 +82,9 @@ export const AddGroupMembersModal = ({ popUp, handlePopUpToggle }: Props) => { text: "Successfully assigned user to the group", type: "success" }); - } catch (error) { + } catch { createNotification({ - text: (error as Error)?.message ?? "Failed to assign user to the group", + text: "Failed to assign user to the group", type: "error" }); } diff --git a/frontend/src/pages/organization/SettingsPage/components/OrgAuthTab/OrgOIDCSection.tsx b/frontend/src/pages/organization/SettingsPage/components/OrgAuthTab/OrgOIDCSection.tsx index 037c0790d..bec65bd8f 100644 --- a/frontend/src/pages/organization/SettingsPage/components/OrgAuthTab/OrgOIDCSection.tsx +++ b/frontend/src/pages/organization/SettingsPage/components/OrgAuthTab/OrgOIDCSection.tsx @@ -203,8 +203,10 @@ export const OrgOIDCSection = (): JSX.Element => {

Group membership changes in the OIDC provider only sync with Infisical when a - user logs in. For example, if you remove a user from a group in the OIDC - provider, this change will not be reflected in Infisical until their next login. + user logs in via OIDC. For example, if you remove a user from a group in the + OIDC provider, this change will not be reflected in Infisical until their next + OIDC login. To ensure this behavior, Infisical recommends enabling Enforce OIDC + SSO.

}