diff --git a/controllers/usergroups_admin_controller.go b/controllers/usergroups_admin_controller.go index 3b5d83c..991fb0f 100644 --- a/controllers/usergroups_admin_controller.go +++ b/controllers/usergroups_admin_controller.go @@ -121,23 +121,36 @@ func (c usergroupsAdminController) FormBeforeUpdate(ctx context.Context, model a // FormBeforeDelete refuses the delete of a privileged group without // golem15.users.manage_privileged_groups: it would take the privilege from // every member. The record is the stored row, loaded in the delete's -// transaction. +// transaction. A delete that goes ahead reads the members there; each gets +// a pact.MCPSubjectChanged permissions event after the delete commits, +// since the group's grants are gone (FormAfterDelete removes the +// memberships). func (c usergroupsAdminController) FormBeforeDelete(ctx context.Context, model any) error { group, ok := model.(*models.UserGroup) if !ok || group == nil { return errors.New("user groups admin: form model is not a user group") } - if !classes.IsPrivilegedCode(classes.PrivilegedGroupCodes(c.app.config()), group.Code) { + if classes.IsPrivilegedCode(classes.PrivilegedGroupCodes(c.app.config()), group.Code) { + if err := c.requirePrivilegedPermission(ctx); err != nil { + return err + } + } + tx, ok := cabana.TxFromContext(ctx) + if !ok { return nil } - return c.requirePrivilegedPermission(ctx) + members, err := groupMemberIDs(ctx, tx, group.ID) + if err != nil { + return err + } + c.permissionsChanged(ctx, tx, members) + return nil } // FormAfterDelete removes the memberships of the deleted group in the same // transaction: users_groups has no foreign key that would do it. The member -// users stay; each gets a pact.MCPSubjectChanged permissions event after the -// delete commits, since the group's grants are gone. -func (c usergroupsAdminController) FormAfterDelete(ctx context.Context, model any) error { +// users stay. +func (usergroupsAdminController) FormAfterDelete(ctx context.Context, model any) error { group, ok := model.(*models.UserGroup) if !ok || group == nil { return errors.New("user groups admin: form model is not a user group") @@ -146,16 +159,8 @@ func (c usergroupsAdminController) FormAfterDelete(ctx context.Context, model an if !ok { return errors.New("user groups admin: a delete needs its transaction") } - members, err := groupMemberIDs(ctx, tx, group.ID) - if err != nil { - return err - } - if err := tx.Session(&gorm.Session{NewDB: true}).WithContext(ctx). - Where("user_group_id = ?", group.ID).Delete(&models.UsersGroup{}).Error; err != nil { - return err - } - c.permissionsChanged(ctx, tx, members) - return nil + return tx.Session(&gorm.Session{NewDB: true}).WithContext(ctx). + Where("user_group_id = ?", group.ID).Delete(&models.UsersGroup{}).Error } // FormAfterUpdate tells the MCP endpoint that the members of the group may diff --git a/mcp_guard_smoke_test.go b/mcp_guard_smoke_test.go index aa5fa85..a5ebee8 100644 --- a/mcp_guard_smoke_test.go +++ b/mcp_guard_smoke_test.go @@ -175,6 +175,12 @@ func TestUserMCPGuardSmoke(t *testing.T) { if !spy.has("user", user.ID, pact.MCPSubjectPermissions) { t.Fatalf("group save fired %+v", spy.events) } + // Deleting a group fires permissions for its members once it commits. + spy.reset() + env.expect(t, http.StatusOK, http.MethodDelete, groupPath(groupB.ID), manager, nil) + if !spy.has("user", user.ID, pact.MCPSubjectPermissions) { + t.Fatalf("group delete fired %+v", spy.events) + } // Saving the user on the Users screen fires permissions too. spy.reset() users := env.admin(t, permAccessUsers)