diff --git a/docs/docs.go b/docs/docs.go index dced44d9..832543f5 100644 --- a/docs/docs.go +++ b/docs/docs.go @@ -32491,6 +32491,10 @@ const docTemplate = `{ "displayName": { "type": "string" }, + "inherited": { + "description": "Inherited is true when the membership was materialized from an SSO IdP group (source=sso).\nSuch memberships are owned by the IdP: the remove-member API refuses to delete them, so the\nUI must surface them as read-only rather than offering a Remove action that 403s.", + "type": "boolean" + }, "userId": { "type": "string" } @@ -32518,6 +32522,10 @@ const docTemplate = `{ "description": "Name is the group's policy-facing identifier — the token that appears in\nsubject.groups and that role-assignment config matches on. Unique among live groups.", "type": "string" }, + "source": { + "description": "Source records how the group came to exist: GroupSourceManual (an admin created it via the API)\nor GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to\nmanual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that\nhave become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is\nset only when the group is first created, so an admin group that later appears in SSO config\nstays manual.", + "type": "string" + }, "updatedAt": { "type": "string" } @@ -41899,6 +41907,10 @@ const docTemplate = `{ "provider": { "type": "string" }, + "source": { + "description": "Source records who owns the mapping: MappingSourceConfig (declared in a provider's\ngroup_mapping and reconciled by boot provisioning) or MappingSourceManual (added at runtime via\nthe admin API). It defaults to config so pre-attribution rows — all of which came from config\nprovisioning — are reconciled declaratively: provisioning prunes config rows the config no\nlonger declares, while manual rows are never auto-removed.", + "type": "string" + }, "updatedAt": { "type": "string" } @@ -42524,6 +42536,10 @@ const docTemplate = `{ "description": "Name is the group's policy-facing identifier — the token that appears in\nsubject.groups and that role-assignment config matches on. Unique among live groups.", "type": "string" }, + "source": { + "description": "Source records how the group came to exist: GroupSourceManual (an admin created it via the API)\nor GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to\nmanual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that\nhave become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is\nset only when the group is first created, so an admin group that later appears in SSO config\nstays manual.", + "type": "string" + }, "updatedAt": { "type": "string" } diff --git a/docs/swagger.json b/docs/swagger.json index ea862477..43bfd4cf 100644 --- a/docs/swagger.json +++ b/docs/swagger.json @@ -32485,6 +32485,10 @@ "displayName": { "type": "string" }, + "inherited": { + "description": "Inherited is true when the membership was materialized from an SSO IdP group (source=sso).\nSuch memberships are owned by the IdP: the remove-member API refuses to delete them, so the\nUI must surface them as read-only rather than offering a Remove action that 403s.", + "type": "boolean" + }, "userId": { "type": "string" } @@ -32512,6 +32516,10 @@ "description": "Name is the group's policy-facing identifier — the token that appears in\nsubject.groups and that role-assignment config matches on. Unique among live groups.", "type": "string" }, + "source": { + "description": "Source records how the group came to exist: GroupSourceManual (an admin created it via the API)\nor GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to\nmanual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that\nhave become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is\nset only when the group is first created, so an admin group that later appears in SSO config\nstays manual.", + "type": "string" + }, "updatedAt": { "type": "string" } @@ -41893,6 +41901,10 @@ "provider": { "type": "string" }, + "source": { + "description": "Source records who owns the mapping: MappingSourceConfig (declared in a provider's\ngroup_mapping and reconciled by boot provisioning) or MappingSourceManual (added at runtime via\nthe admin API). It defaults to config so pre-attribution rows — all of which came from config\nprovisioning — are reconciled declaratively: provisioning prunes config rows the config no\nlonger declares, while manual rows are never auto-removed.", + "type": "string" + }, "updatedAt": { "type": "string" } @@ -42518,6 +42530,10 @@ "description": "Name is the group's policy-facing identifier — the token that appears in\nsubject.groups and that role-assignment config matches on. Unique among live groups.", "type": "string" }, + "source": { + "description": "Source records how the group came to exist: GroupSourceManual (an admin created it via the API)\nor GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to\nmanual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that\nhave become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is\nset only when the group is first created, so an admin group that later appears in SSO config\nstays manual.", + "type": "string" + }, "updatedAt": { "type": "string" } diff --git a/docs/swagger.yaml b/docs/swagger.yaml index 0e67826a..5c0f3b67 100644 --- a/docs/swagger.yaml +++ b/docs/swagger.yaml @@ -2182,6 +2182,12 @@ definitions: properties: displayName: type: string + inherited: + description: |- + Inherited is true when the membership was materialized from an SSO IdP group (source=sso). + Such memberships are owned by the IdP: the remove-member API refuses to delete them, so the + UI must surface them as read-only rather than offering a Remove action that 403s. + type: boolean userId: type: string type: object @@ -2202,6 +2208,15 @@ definitions: Name is the group's policy-facing identifier — the token that appears in subject.groups and that role-assignment config matches on. Unique among live groups. type: string + source: + description: |- + Source records how the group came to exist: GroupSourceManual (an admin created it via the API) + or GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to + manual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that + have become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is + set only when the group is first created, so an admin group that later appears in SSO config + stays manual. + type: string updatedAt: type: string type: object @@ -8429,6 +8444,14 @@ definitions: type: string provider: type: string + source: + description: |- + Source records who owns the mapping: MappingSourceConfig (declared in a provider's + group_mapping and reconciled by boot provisioning) or MappingSourceManual (added at runtime via + the admin API). It defaults to config so pre-attribution rows — all of which came from config + provisioning — are reconciled declaratively: provisioning prunes config rows the config no + longer declares, while manual rows are never auto-removed. + type: string updatedAt: type: string type: object @@ -8849,6 +8872,15 @@ definitions: Name is the group's policy-facing identifier — the token that appears in subject.groups and that role-assignment config matches on. Unique among live groups. type: string + source: + description: |- + Source records how the group came to exist: GroupSourceManual (an admin created it via the API) + or GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to + manual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that + have become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is + set only when the group is first created, so an admin group that later appears in SSO config + stays manual. + type: string updatedAt: type: string type: object diff --git a/internal/api/handler/auth/sso.go b/internal/api/handler/auth/sso.go index 3855ab21..197d7bd8 100644 --- a/internal/api/handler/auth/sso.go +++ b/internal/api/handler/auth/sso.go @@ -90,6 +90,13 @@ func provisionSSOGroupMappings(logger *zap.SugaredLogger, db *gorm.DB, ssoCfg *c logger.Errorw("Failed to provision SSO group mappings", "provider", name, "error", err) } } + + // After every provider's mappings are applied, reclaim sso-created groups that nothing references + // anymore (e.g. a group left behind by a renamed group_mapping value). Best-effort: a failure is + // logged, not fatal. + if err := relational.CleanupOrphanedSSOGroups(db); err != nil { + logger.Errorw("Failed to clean up orphaned SSO groups", "error", err) + } } func (h *SSOHandler) Register(api *echo.Group) { diff --git a/internal/api/handler/groups.go b/internal/api/handler/groups.go index 0d3823f8..7dd1d687 100644 --- a/internal/api/handler/groups.go +++ b/internal/api/handler/groups.go @@ -43,6 +43,10 @@ func (h *GroupsHandler) Register(api *echo.Group) { type groupMemberResponse struct { UserID string `json:"userId"` DisplayName string `json:"displayName"` + // Inherited is true when the membership was materialized from an SSO IdP group (source=sso). + // Such memberships are owned by the IdP: the remove-member API refuses to delete them, so the + // UI must surface them as read-only rather than offering a Remove action that 403s. + Inherited bool `json:"inherited"` } type groupResponse struct { @@ -288,17 +292,26 @@ func (h *GroupsHandler) ListMembers(ctx echo.Context) error { return h.groupError(ctx, err) } - // Two-step lookup (member ids, then the users) avoids a column-to-column uuid = text + // Two-step lookup (membership rows, then the users) avoids a column-to-column uuid = text // join between ccf_users.id (uuid) and ccf_user_groups.user_id (text), which Postgres - // rejects; the string IN clause coerces cleanly on both Postgres and SQLite. - var memberIDs []string - if err := h.db.Model(&relational.UserGroupMembership{}). + // rejects; the string IN clause coerces cleanly on both Postgres and SQLite. We keep each + // membership's source so the response can flag SSO-synced (inherited) members the remove + // API won't delete. The (user_id, group_id) unique index means at most one row per user. + var memberships []relational.UserGroupMembership + if err := h.db. Where("group_id = ?", group.ID.String()). - Pluck("user_id", &memberIDs).Error; err != nil { + Find(&memberships).Error; err != nil { h.sugar.Errorw("Failed to list group member ids", "error", err) return ctx.JSON(500, api.NewError(err)) } + sourceByUser := make(map[string]string, len(memberships)) + memberIDs := make([]string, 0, len(memberships)) + for _, m := range memberships { + sourceByUser[m.UserID] = m.Source + memberIDs = append(memberIDs, m.UserID) + } + var users []relational.User if len(memberIDs) > 0 { if err := h.db. @@ -318,6 +331,7 @@ func (h *GroupsHandler) ListMembers(ctx echo.Context) error { responses = append(responses, groupMemberResponse{ UserID: u.ID.String(), DisplayName: UserDisplayName(u), + Inherited: sourceByUser[u.ID.String()] == relational.MembershipSourceSSO, }) } @@ -517,6 +531,8 @@ func (h *GroupsHandler) AddSSOMapping(ctx echo.Context) error { Provider: provider, ExternalGroup: externalGroup, GroupID: group.ID.String(), + // Manual so boot provisioning never prunes an admin-added mapping as "not in config". + Source: relational.MappingSourceManual, } if err := h.db.Create(mapping).Error; err != nil { if isUniqueViolation(err) { diff --git a/internal/api/handler/groups_integration_test.go b/internal/api/handler/groups_integration_test.go index 081ff3c3..9b546cf0 100644 --- a/internal/api/handler/groups_integration_test.go +++ b/internal/api/handler/groups_integration_test.go @@ -128,6 +128,7 @@ func (suite *GroupsApiIntegrationSuite) TestMembershipLifecycleAndUserSurfacing( suite.Require().NoError(json.Unmarshal(members.Body.Bytes(), &memberResp)) suite.Require().Len(memberResp.Data, 1) suite.Equal(userID, memberResp.Data[0].UserID) + suite.False(memberResp.Data[0].Inherited, "a manually added member is not inherited") // Membership surfaces on the admin user view. userView := suite.do("GET", "/api/admin/users/"+userID, nil) @@ -211,6 +212,15 @@ func (suite *GroupsApiIntegrationSuite) TestRemoveSSOMemberReturns403() { Source: relational.MembershipSourceSSO, }).Error) + // The member list flags the sso membership as inherited so the UI can render it read-only + // instead of offering a Remove action that would 403. + members := suite.do("GET", "/api/admin/groups/"+groupID+"/members", nil) + suite.Require().Equal(200, members.Code) + var memberResp GenericDataListResponse[groupMemberResponse] + suite.Require().NoError(json.Unmarshal(members.Body.Bytes(), &memberResp)) + suite.Require().Len(memberResp.Data, 1) + suite.True(memberResp.Data[0].Inherited, "an sso-synced member must be flagged inherited") + rem := suite.do("DELETE", "/api/admin/groups/"+groupID+"/members/"+userID, nil) suite.Require().Equal(403, rem.Code, rem.Body.String()) diff --git a/internal/service/relational/groups.go b/internal/service/relational/groups.go index b4e0ed20..85af287d 100644 --- a/internal/service/relational/groups.go +++ b/internal/service/relational/groups.go @@ -24,12 +24,29 @@ type UserGroup struct { // subject.groups and that role-assignment config matches on. Unique among live groups. Name string `json:"name" gorm:"uniqueIndex:idx_ccf_groups_name,WHERE:deleted_at IS NULL;not null"` Description string `json:"description"` + + // Source records how the group came to exist: GroupSourceManual (an admin created it via the API) + // or GroupSourceSSO (SSO provisioning materialized it from a group_mapping value). It defaults to + // manual so admin-created and pre-attribution groups are never auto-removed. Only sso groups that + // have become fully unreferenced are cleaned up at boot (CleanupOrphanedSSOGroups); the source is + // set only when the group is first created, so an admin group that later appears in SSO config + // stays manual. + Source string `json:"source" gorm:"not null;default:manual"` } func (UserGroup) TableName() string { return "ccf_groups" } +// Group source discriminators (see UserGroup.Source). +const ( + // GroupSourceManual is an admin-created native group. It is the default and is never auto-removed. + GroupSourceManual = "manual" + // GroupSourceSSO is a group materialized by SSO provisioning from a group_mapping value. It is + // eligible for boot-time cleanup once nothing references it (no mapping, no members, no grants). + GroupSourceSSO = "sso" +) + // Membership source discriminators (BCH-1331). A membership records how it came to exist so the // SSO sync and the admin API never clobber each other: only the IdP owns sso memberships, only an // admin owns manual ones. @@ -59,6 +76,15 @@ type UserGroupMembership struct { // sso rows; the admin remove-member API refuses to delete sso rows (BCH-1331). Source string `json:"source" gorm:"not null;default:manual"` + // Provider attributes an sso membership to the SSO provider whose mapping materialized it + // (matching SSOUserLink.Provider / the login callback's provider name); empty for manual rows and + // for sso rows created before attribution existed. Reconcile uses it to de-provision exactly the + // rows the logging-in provider granted, so a group_mapping change is treated like the IdP + // changing the user's groups, and two providers that map the SAME native group do not pin each + // other's memberships — the previous group-scoped reconcile could not tell whose membership a + // shared group was, leaving a renamed mapping's old membership stranded as an un-removable sso row. + Provider string `json:"provider,omitempty" gorm:"default:''"` + Group UserGroup `json:"group,omitempty" gorm:"foreignKey:GroupID;references:ID"` } @@ -81,6 +107,13 @@ type SSOGroupMapping struct { ExternalGroup string `json:"externalGroup" gorm:"not null;uniqueIndex:idx_ccf_sso_group_mappings_provider_group,priority:2"` GroupID string `json:"groupId" gorm:"not null;index"` + // Source records who owns the mapping: MappingSourceConfig (declared in a provider's + // group_mapping and reconciled by boot provisioning) or MappingSourceManual (added at runtime via + // the admin API). It defaults to config so pre-attribution rows — all of which came from config + // provisioning — are reconciled declaratively: provisioning prunes config rows the config no + // longer declares, while manual rows are never auto-removed. + Source string `json:"source" gorm:"not null;default:config"` + Group UserGroup `json:"group,omitempty" gorm:"foreignKey:GroupID;references:ID"` } @@ -88,6 +121,15 @@ func (SSOGroupMapping) TableName() string { return "ccf_sso_group_mappings" } +// SSO group-mapping source discriminators (see SSOGroupMapping.Source). +const ( + // MappingSourceConfig is a mapping declared in a provider's group_mapping. Boot provisioning owns + // it: it is created/re-pointed to match config and pruned when config drops it. + MappingSourceConfig = "config" + // MappingSourceManual is a mapping added at runtime via the admin API. Provisioning never prunes it. + MappingSourceManual = "manual" +) + // GroupNamesForUser returns the names of the native CCF groups the user belongs to, sorted // and de-duplicated. It is the single native-membership query shared by the authz group // resolver (which unions it with SSO groups for subject.groups) and the builtin admin check diff --git a/internal/service/relational/sso_groups.go b/internal/service/relational/sso_groups.go index 45b79f53..50e10e69 100644 --- a/internal/service/relational/sso_groups.go +++ b/internal/service/relational/sso_groups.go @@ -1,6 +1,7 @@ package relational import ( + "errors" "strings" "gorm.io/gorm" @@ -16,10 +17,12 @@ import ( // "hd:example.com": // - ccf-authorized-users // -// For each listed native name it creates the UserGroup if absent, then upserts one SSOGroupMapping -// row keyed by (provider, externalGroup=claim) pointing at that group. It is idempotent and safe to -// run on every boot. It only creates/updates — it never deletes mappings, so mappings an admin added -// via the API for the same provider are left intact. +// For each listed native name it creates the UserGroup if absent, then upserts one source=config +// SSOGroupMapping row keyed by (provider, externalGroup=claim) pointing at that group, and finally +// PRUNES this provider's source=config rows the config no longer declares (a removed group_mapping +// entry). Mappings an admin added at runtime (source=manual) are never pruned. It is idempotent and +// safe to run on every boot; pruning a mapping is what lets the now-unreferenced group be reclaimed +// by CleanupOrphanedSSOGroups. // // Caveat: the (provider, external_group) unique index means one IdP claim maps to exactly one native // group. If a claim lists multiple native names, the last one wins (each overwrites the prior row's @@ -31,11 +34,18 @@ func ProvisionSSOGroupMappings(db *gorm.DB, provider string, mapping map[string] } return db.Transaction(func(tx *gorm.DB) error { + // External claim keys the config declares for this provider — the keep-set for the prune. + configExternals := make([]string, 0, len(mapping)) + seen := make(map[string]struct{}, len(mapping)) for rawExternal, nativeNames := range mapping { external := strings.TrimSpace(rawExternal) if external == "" { continue } + if _, ok := seen[external]; !ok { + seen[external] = struct{}{} + configExternals = append(configExternals, external) + } for _, rawName := range nativeNames { name := strings.TrimSpace(rawName) @@ -44,43 +54,154 @@ func ProvisionSSOGroupMappings(db *gorm.DB, provider string, mapping map[string] } // Find-or-create the native group by name (live rows only; the index is partial). + // Attrs applies Source only on the CREATE path, so a group an admin already created + // keeps its manual source (and its protection from cleanup) even if SSO config later + // names it. group := UserGroup{Name: name} - if err := tx.Where("name = ?", name).FirstOrCreate(&group).Error; err != nil { + if err := tx.Where("name = ?", name). + Attrs(UserGroup{Source: GroupSourceSSO}). + FirstOrCreate(&group).Error; err != nil { return err } if group.ID == nil { continue } - row := SSOGroupMapping{Provider: provider, ExternalGroup: external, GroupID: group.ID.String()} - // Upsert on the (provider, external_group) unique index: a re-point of an existing - // mapping updates its group_id rather than failing or duplicating. + row := SSOGroupMapping{ + Provider: provider, ExternalGroup: external, + GroupID: group.ID.String(), Source: MappingSourceConfig, + } + // Upsert on the (provider, external_group) unique index: a re-point updates the + // group_id and re-affirms config ownership rather than failing or duplicating. if err := tx.Clauses(clause.OnConflict{ Columns: []clause.Column{{Name: "provider"}, {Name: "external_group"}}, - DoUpdates: clause.AssignmentColumns([]string{"group_id"}), + DoUpdates: clause.AssignmentColumns([]string{"group_id", "source"}), }).Create(&row).Error; err != nil { return err } } } + + // Prune this provider's config-sourced mappings that the config no longer declares. Manual + // (admin-API) mappings are left intact. + del := tx.Where("provider = ? AND source = ?", provider, MappingSourceConfig) + if len(configExternals) > 0 { + del = del.Where("external_group NOT IN ?", configExternals) + } + if err := del.Delete(&SSOGroupMapping{}).Error; err != nil { + return err + } return nil }) } -// ReconcileSSOGroupMemberships makes a user's source=sso native memberships for THIS provider +// CleanupOrphanedSSOGroups soft-deletes native groups that SSO provisioning created (Source=sso) and +// that nothing references anymore: no SSOGroupMapping points at them, no user is a member, and no +// role assignment grants by their name. This is what reclaims a group left behind when a provider's +// group_mapping value is renamed or removed (e.g. "ccf-authorized-uzers" after a typo'd rename) — the +// mapping re-points to the new group and the old one becomes an unreferenced orphan. +// +// It is the boot-time SWEEP over every sso group. The per-login de-provision path also cleans up +// (see ReconcileSSOGroupMemberships → pruneOrphanedSSOGroups), which is what catches a group that +// only becomes empty when the last member is removed at login — too late for the preceding boot. +// +// It is deliberately conservative: +// - Source=manual groups (admin-created, and all pre-attribution rows) are never touched. +// - A group with any member, mapping, or group role assignment is kept — the same emptiness guard +// the admin DeleteGroup API enforces, so a group still in use is never silently removed. +// +// Run it AFTER provisioning every provider so a group mapped by one provider is not deleted while +// another provider's mappings are still being applied. +func CleanupOrphanedSSOGroups(db *gorm.DB) error { + return db.Transaction(func(tx *gorm.DB) error { + var ids []string + if err := tx.Model(&UserGroup{}).Where("source = ?", GroupSourceSSO). + Pluck("id", &ids).Error; err != nil { + return err + } + return pruneOrphanedSSOGroups(tx, ids) + }) +} + +// pruneOrphanedSSOGroups soft-deletes, among the given group ids, the ones that are Source=sso and +// now fully unreferenced (no SSOGroupMapping, no members, no group role assignment). It is the shared +// guard used by both the boot sweep and the login de-provision path, and runs in the caller's +// transaction. Non-sso, still-referenced, or already-deleted ids are skipped. +func pruneOrphanedSSOGroups(tx *gorm.DB, groupIDs []string) error { + for _, groupID := range groupIDs { + groupID = strings.TrimSpace(groupID) + if groupID == "" { + continue + } + + // Must be a live sso group; anything else (manual, or already gone) is left alone. + var group UserGroup + if err := tx.Where("id = ? AND source = ?", groupID, GroupSourceSSO). + First(&group).Error; err != nil { + if errors.Is(err, gorm.ErrRecordNotFound) { + continue + } + return err + } + + var mappingCount int64 + if err := tx.Model(&SSOGroupMapping{}).Where("group_id = ?", groupID). + Count(&mappingCount).Error; err != nil { + return err + } + if mappingCount > 0 { + continue // still mapped by some provider + } + + var memberCount int64 + if err := tx.Model(&UserGroupMembership{}).Where("group_id = ?", groupID). + Count(&memberCount).Error; err != nil { + return err + } + if memberCount > 0 { + continue // still has members (sso or manual) + } + + var grantCount int64 + if err := tx.Model(&CCFRoleAssignment{}). + Where("assignee_type = ? AND assignee_id = ?", + RoleAssigneeTypeGroup, NormalizeAssigneeID(group.Name)). + Count(&grantCount).Error; err != nil { + return err + } + if grantCount > 0 { + continue // a role assignment (config or admin) still grants by this group's name + } + + if err := tx.Delete(&UserGroup{}, "id = ?", groupID).Error; err != nil { + return err + } + } + return nil +} + +// ReconcileSSOGroupMemberships makes the user's sso native memberships attributed to THIS provider // exactly match the native groups implied by idpGroups (BCH-1331). It translates each IdP group -// through the provider's SSOGroupMapping rows, upserts a source=sso membership for every mapped -// group the user currently has, and removes the source=sso memberships for groups THIS provider -// governs that the user has lost at the IdP (login-time de-provisioning). Unmapped IdP groups are -// ignored. source=manual memberships are never read or written here, so an admin's hand-assignment -// survives even when it names the same group. +// through the provider's SSOGroupMapping rows, then materializes a source=sso membership (attributed +// to this provider) for every mapped group the user currently presents and DELETES the memberships +// this provider granted that the user no longer has. Unmapped IdP groups are ignored. source=manual +// memberships are never read or written here, so an admin's hand-assignment survives even when it +// names the same group. // -// De-provisioning is scoped to the set of native groups this provider maps to: a user linked to -// two IdPs (resolved to one CCF user by email) keeps the other provider's sso memberships when -// logging in here, instead of having them wiped until the next login via that provider. Residual -// ambiguity only arises if two providers map to the *same* native group, which self-heals on the -// next login via the other provider. (UserGroupMembership carries no provider attribution, so this -// group-scoped approach is the complete fix short of adding one.) +// A group_mapping change is therefore treated exactly like the IdP changing the user's groups: if a +// provider's mapping is removed or re-pointed (e.g. a value renamed from "ccf-authorized-users" to +// "ccf-authorized-uzers"), that provider stops granting the old group, so on the next login the old +// membership is removed and the new one added — there is no lingering, un-removable row. +// +// Attribution by provider is what makes this correct when two providers map the SAME native group +// (the default config has google AND github both mapping "ccf-authorized-users"): the membership +// records which provider granted it, so renaming GOOGLE's mapping removes google's membership even +// though github still maps that group. A user genuinely entitled via github re-acquires it on their +// next github login. Memberships owned by a DIFFERENT provider are left for that provider to +// reconcile; an unattributed pre-attribution row is treated as the logging-in provider's (so the +// historical stuck rows self-heal). The row is adopted to the current provider whenever this +// provider grants the group, keeping ownership current. (user_id, group_id) stays unique: a group +// has at most one membership row, whose Provider reflects its most recent grantor. // // All reads and writes run inside a single transaction so concurrent logins for the same user // reconcile atomically. @@ -92,23 +213,21 @@ func ReconcileSSOGroupMemberships(db *gorm.DB, userID, provider string, idpGroup } return db.Transaction(func(tx *gorm.DB) error { - // external(lower) -> native group id, and the set of native groups this provider governs. + // external(lower) -> native group id for this provider's mappings. var mappings []SSOGroupMapping if err := tx.Where("provider = ?", provider).Find(&mappings).Error; err != nil { return err } mapByExternal := make(map[string]string, len(mappings)) - managed := make(map[string]struct{}, len(mappings)) // native group ids this provider maps to for _, m := range mappings { groupID := strings.TrimSpace(m.GroupID) key := strings.ToLower(strings.TrimSpace(m.ExternalGroup)) if key != "" && groupID != "" { mapByExternal[key] = groupID - managed[groupID] = struct{}{} } } - // desired = the native group ids the user's current IdP groups map to. + // desired = the native group ids the user's current IdP groups map to under this provider. desired := make(map[string]struct{}, len(idpGroups)) for _, g := range idpGroups { key := strings.ToLower(strings.TrimSpace(g)) @@ -127,24 +246,33 @@ func ReconcileSSOGroupMemberships(db *gorm.DB, userID, provider string, idpGroup return err } have := make(map[string]struct{}, len(existing)) - var toRemove []string + var toRemove []string // delete: owned by this provider but no longer granted + var toAdopt []string // (re)claim ownership: this provider grants it but the row names another for _, m := range existing { have[m.GroupID] = struct{}{} - // Only de-provision groups THIS provider governs; another IdP's memberships are left - // alone (they have no mapping here, so they are absent from `managed`). - if _, governed := managed[m.GroupID]; !governed { + if _, want := desired[m.GroupID]; want { + // This provider grants the group now: keep it and make sure the row is attributed + // here (adopt another provider's row or a pre-attribution one). + if strings.TrimSpace(m.Provider) != provider { + toAdopt = append(toAdopt, m.GroupID) + } continue } - if _, ok := desired[m.GroupID]; !ok { - toRemove = append(toRemove, m.GroupID) + // Not granted now. Only de-provision rows this provider owns (or unattributed legacy + // rows); a different provider's membership is left for that provider to reconcile. + if rowProvider := strings.TrimSpace(m.Provider); rowProvider != "" && rowProvider != provider { + continue } + toRemove = append(toRemove, m.GroupID) } for groupID := range desired { if _, ok := have[groupID]; ok { continue } - row := UserGroupMembership{UserID: userID, GroupID: groupID, Source: MembershipSourceSSO} + row := UserGroupMembership{ + UserID: userID, GroupID: groupID, Source: MembershipSourceSSO, Provider: provider, + } // DoNothing on the (user_id, group_id) unique index: if a manual membership already // covers this group the user is in it either way, and we must not flip its source. if err := tx.Clauses(clause.OnConflict{ @@ -160,6 +288,21 @@ func ReconcileSSOGroupMemberships(db *gorm.DB, userID, provider string, idpGroup Delete(&UserGroupMembership{}).Error; err != nil { return err } + // Removing this user may have emptied a now-unmapped sso group (e.g. the old group after + // a group_mapping change): reclaim it here, at the moment it becomes orphaned, rather + // than waiting for the next boot sweep. + if err := pruneOrphanedSSOGroups(tx, toRemove); err != nil { + return err + } + } + if len(toAdopt) > 0 { + // Scoped to sso rows so a manual membership for the same group is never touched. + if err := tx.Model(&UserGroupMembership{}). + Where("user_id = ? AND source = ? AND group_id IN ?", + userID, MembershipSourceSSO, toAdopt). + Update("provider", provider).Error; err != nil { + return err + } } return nil }) diff --git a/internal/service/relational/sso_groups_test.go b/internal/service/relational/sso_groups_test.go index 3974f7a8..19ab4fe3 100644 --- a/internal/service/relational/sso_groups_test.go +++ b/internal/service/relational/sso_groups_test.go @@ -14,7 +14,7 @@ func setupGroupsDB(t *testing.T) *gorm.DB { db, err := gorm.Open(sqlite.Open(":memory:"), &gorm.Config{}) require.NoError(t, err) require.NoError(t, db.AutoMigrate( - &User{}, &UserGroup{}, &UserGroupMembership{}, &SSOGroupMapping{}, + &User{}, &UserGroup{}, &UserGroupMembership{}, &SSOGroupMapping{}, &CCFRoleAssignment{}, )) return db } @@ -176,6 +176,262 @@ func TestReconcileSSOGroupMembershipsIsProviderScoped(t *testing.T) { require.Equal(t, []string{dataID}, ssoMembershipGroupIDs(t, db, userID)) } +// membershipSource returns the source of the user's membership in the given group, or "" if none. +func membershipSource(t *testing.T, db *gorm.DB, userID, groupID string) string { + t.Helper() + var sources []string + require.NoError(t, db.Model(&UserGroupMembership{}). + Where("user_id = ? AND group_id = ?", userID, groupID). + Pluck("source", &sources).Error) + if len(sources) == 0 { + return "" + } + return sources[0] +} + +// TestReconcileSSOGroupMembershipsRemovesAfterRename proves a group_mapping value rename behaves like +// the IdP changing the user's groups: the claim key stays the same but maps to a new native group, so +// the next login removes the old membership and adds the new one — no lingering row. +func TestReconcileSSOGroupMembershipsRemovesAfterRename(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + + // Original config: claim "hd:example.com" grants native group "authorized-users". + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{ + "hd:example.com": {"authorized-users"}, + })) + oldID := groupIDByName(t, db, "authorized-users") + + // User logs in and is materialized into the old group. + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:example.com"})) + require.Equal(t, []string{oldID}, ssoMembershipGroupIDs(t, db, userID)) + + // Config value renamed; the claim key is unchanged so the mapping row re-points at the new group. + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{ + "hd:example.com": {"authorized-uzers"}, + })) + newID := groupIDByName(t, db, "authorized-uzers") + + // Next login: only the new group remains; the old membership is gone entirely (not stale). + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:example.com"})) + require.Equal(t, []string{newID}, ssoMembershipGroupIDs(t, db, userID)) + require.Equal(t, "", membershipSource(t, db, userID, oldID), "old membership must be deleted") +} + +// TestReconcileSSOGroupMembershipsRemovesAfterRenameDespiteOtherProvider reproduces the reported +// production bug exactly (gustavo@container-solutions.com, a Google user): the default config has TWO +// providers (google + github) mapping the SAME native group ("ccf-authorized-users"). Renaming only +// google's value must still REMOVE google's membership — the previous group-scoped reconcile left it +// pinned because github kept the group "mapped", stranding the user in BOTH groups as un-removable +// sso rows. Provider attribution fixes it: the membership is google's, and google no longer grants it. +func TestReconcileSSOGroupMembershipsRemovesAfterRenameDespiteOtherProvider(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + // Both providers map the same native group, like sso.yaml's google + github. + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"authorized-users"}})) + require.NoError(t, ProvisionSSOGroupMappings(db, "github", map[string][]string{"gh:org": {"authorized-users"}})) + authID := groupIDByName(t, db, "authorized-users") + + // User logs in via google only and is materialized into the shared group (attributed to google). + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + require.Equal(t, []string{authID}, ssoMembershipGroupIDs(t, db, userID)) + + // Google's value is renamed; github STILL maps "authorized-users". + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"authorized-uzers"}})) + uzersID := groupIDByName(t, db, "authorized-uzers") + + // Google re-login: the old group is removed despite github still mapping it (the membership was + // google's), and the new group becomes the only sso membership. + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + require.Equal(t, []string{uzersID}, ssoMembershipGroupIDs(t, db, userID)) + require.Equal(t, "", membershipSource(t, db, userID, authID), "google's old membership must be deleted") + + // A github login (the user IS in the github org) re-grants the shared group via github. + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "github", []string{"gh:org"})) + require.Equal(t, MembershipSourceSSO, membershipSource(t, db, userID, authID)) +} + +// TestReconcileSSOGroupMembershipsAdoptsLegacyUnattributedRow proves a pre-attribution sso row +// (Provider == "") is treated as the logging-in provider's: it is adopted when still granted, and — +// crucially for the stuck rows already in the DB — removed when that provider's mapping renames away. +func TestReconcileSSOGroupMembershipsAdoptsLegacyUnattributedRow(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"authorized-users"}})) + authID := groupIDByName(t, db, "authorized-users") + + // Simulate a row created before provider attribution existed: source=sso, provider empty. + require.NoError(t, db.Create(&UserGroupMembership{ + UserID: userID, GroupID: authID, Source: MembershipSourceSSO, Provider: "", + }).Error) + + // A login that still grants the group adopts the row to the provider. + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + var adopted UserGroupMembership + require.NoError(t, db.Where("user_id = ? AND group_id = ?", userID, authID).First(&adopted).Error) + require.Equal(t, "google", adopted.Provider) + + // Rename google's mapping; the (now-attributed) row is removed on the next login. + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"authorized-uzers"}})) + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + require.Equal(t, "", membershipSource(t, db, userID, authID)) +} + +// groupExists reports whether a live (not soft-deleted) group with the given name exists. +func groupExists(t *testing.T, db *gorm.DB, name string) bool { + t.Helper() + var count int64 + require.NoError(t, db.Model(&UserGroup{}).Where("name = ?", name).Count(&count).Error) + return count > 0 +} + +// TestProvisionMarksCreatedGroupsSSO proves provisioning tags groups it creates as Source=sso, while +// a pre-existing admin (manual) group keeps its source even when SSO config later names it. +func TestProvisionMarksCreatedGroupsSSO(t *testing.T) { + db := setupGroupsDB(t) + + // An admin-created group that SSO config will also reference. + require.NoError(t, db.Create(&UserGroup{Name: "shared", Source: GroupSourceManual}).Error) + + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{ + "hd:cs": {"sso-made"}, + "gh:org": {"shared"}, + })) + + var ssoMade, shared UserGroup + require.NoError(t, db.Where("name = ?", "sso-made").First(&ssoMade).Error) + require.Equal(t, GroupSourceSSO, ssoMade.Source) + require.NoError(t, db.Where("name = ?", "shared").First(&shared).Error) + require.Equal(t, GroupSourceManual, shared.Source, "admin group must keep manual source") +} + +// TestReconcileCleansUpOrphanedGroupAtLogin proves the old sso group is reclaimed AT LOGIN, the +// moment reconcile removes the user's last membership — the exact flow the user described: change the +// map, user logs in, gets the new group, loses the old, and the now-empty unmapped old group is gone. +func TestReconcileCleansUpOrphanedGroupAtLogin(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + + // Boot-equivalent: provision maps the claim to the old group; user logs in and joins it. + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"old-group"}})) + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + oldID := groupIDByName(t, db, "old-group") + require.Equal(t, []string{oldID}, ssoMembershipGroupIDs(t, db, userID)) + + // Admin changes the map: same claim now grants a different group. (Provisioning re-points the + // mapping and prunes nothing here since the claim key is unchanged.) + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"new-group"}})) + require.True(t, groupExists(t, db, "old-group"), "old group still present before the user re-logs in") + + // User logs in again: joins new-group, leaves old-group, and old-group (empty, unmapped, roleless) + // is cleaned up right here at login. + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + require.Equal(t, []string{groupIDByName(t, db, "new-group")}, ssoMembershipGroupIDs(t, db, userID)) + require.False(t, groupExists(t, db, "old-group"), "emptied unmapped sso group must be cleaned at login") +} + +// TestReconcileKeepsRoleGrantedGroupAtLogin proves login cleanup respects the role guard: an emptied +// unmapped sso group that still has a role assignment is kept, not deleted. +func TestReconcileKeepsRoleGrantedGroupAtLogin(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"old-group"}})) + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + require.NoError(t, db.Create(&CCFRoleAssignment{ + RoleName: "viewer", AssigneeType: RoleAssigneeTypeGroup, + AssigneeID: NormalizeAssigneeID("old-group"), Source: RoleAssignmentSourceManual, + }).Error) + + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"new-group"}})) + require.NoError(t, ReconcileSSOGroupMemberships(db, userID, "google", []string{"hd:cs"})) + + require.True(t, groupExists(t, db, "old-group"), "role-granted group must survive even when emptied") +} + +// mappingExists reports whether a (provider, externalGroup) mapping row exists. +func mappingExists(t *testing.T, db *gorm.DB, provider, external string) bool { + t.Helper() + var count int64 + require.NoError(t, db.Model(&SSOGroupMapping{}). + Where("provider = ? AND external_group = ?", provider, external).Count(&count).Error) + return count > 0 +} + +// TestProvisionPrunesConfigRemovedMappings proves a removed group_mapping entry's row is deleted on +// the next provisioning, while an admin-added (source=manual) mapping for the same provider survives. +// This is the case that left ccf-github-users mapped (and thus undeletable) after its config entry +// was removed. +func TestProvisionPrunesConfigRemovedMappings(t *testing.T) { + db := setupGroupsDB(t) + + // Initial config: two github mappings. + require.NoError(t, ProvisionSSOGroupMappings(db, "github", map[string][]string{ + "gh:org-a": {"team-a"}, + "gh:org-b": {"github-users"}, + })) + // An admin adds a runtime mapping the config never declares. + adminGrp := UserGroup{Name: "admin-mapped", Source: GroupSourceManual} + require.NoError(t, db.Create(&adminGrp).Error) + require.NoError(t, db.Create(&SSOGroupMapping{ + Provider: "github", ExternalGroup: "gh:manual", GroupID: adminGrp.ID.String(), + Source: MappingSourceManual, + }).Error) + + // Config now drops "gh:org-b" (the ccf-github-users analogue). + require.NoError(t, ProvisionSSOGroupMappings(db, "github", map[string][]string{ + "gh:org-a": {"team-a"}, + })) + + require.True(t, mappingExists(t, db, "github", "gh:org-a"), "still-declared mapping kept") + require.False(t, mappingExists(t, db, "github", "gh:org-b"), "config-removed mapping must be pruned") + require.True(t, mappingExists(t, db, "github", "gh:manual"), "admin (manual) mapping must survive") + + // With the mapping gone, the orphaned sso group is reclaimable by cleanup. + require.NoError(t, CleanupOrphanedSSOGroups(db)) + require.False(t, groupExists(t, db, "github-users"), "unmapped sso group must be cleaned up") + require.True(t, groupExists(t, db, "team-a"), "still-mapped group kept") + require.True(t, groupExists(t, db, "admin-mapped"), "admin-mapped group kept") +} + +// TestCleanupOrphanedSSOGroups proves boot cleanup removes only fully-unreferenced sso groups and +// protects everything else: still-mapped groups, groups with members, groups granted a role, and +// admin (manual) groups. +func TestCleanupOrphanedSSOGroups(t *testing.T) { + db := setupGroupsDB(t) + userID := createGroupsUser(t, db) + + // orphan: sso-made, no mapping, no members, no grants -> deleted. + require.NoError(t, db.Create(&UserGroup{Name: "orphan", Source: GroupSourceSSO}).Error) + + // mapped: sso-made and still has a mapping -> kept. + require.NoError(t, ProvisionSSOGroupMappings(db, "google", map[string][]string{"hd:cs": {"mapped"}})) + + // withMember: sso-made, no mapping, but has a member -> kept. + withMember := UserGroup{Name: "with-member", Source: GroupSourceSSO} + require.NoError(t, db.Create(&withMember).Error) + require.NoError(t, db.Create(&UserGroupMembership{ + UserID: userID, GroupID: withMember.ID.String(), Source: MembershipSourceManual, + }).Error) + + // withGrant: sso-made, no mapping/members, but a role assignment grants by its name -> kept. + require.NoError(t, db.Create(&UserGroup{Name: "with-grant", Source: GroupSourceSSO}).Error) + require.NoError(t, db.Create(&CCFRoleAssignment{ + RoleName: "viewer", AssigneeType: RoleAssigneeTypeGroup, + AssigneeID: NormalizeAssigneeID("with-grant"), Source: RoleAssignmentSourceConfig, + }).Error) + + // adminOrphan: manual, unreferenced -> kept (never auto-deleted). + require.NoError(t, db.Create(&UserGroup{Name: "admin-orphan", Source: GroupSourceManual}).Error) + + require.NoError(t, CleanupOrphanedSSOGroups(db)) + + require.False(t, groupExists(t, db, "orphan"), "unreferenced sso group must be deleted") + require.True(t, groupExists(t, db, "mapped"), "still-mapped sso group must be kept") + require.True(t, groupExists(t, db, "with-member"), "sso group with members must be kept") + require.True(t, groupExists(t, db, "with-grant"), "sso group granted a role must be kept") + require.True(t, groupExists(t, db, "admin-orphan"), "manual group must never be auto-deleted") +} + func groupIDByName(t *testing.T, db *gorm.DB, name string) string { t.Helper() var g UserGroup