From e63fb182429d8538e1647d30c08c511087e7f656 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Sep 2024 10:05:53 -0500 Subject: [PATCH 1/3] feat: remove user from groups on org membership delete Groups inherently provide authz access to certain resources. If a user is removed from an organization, they should be removed from all their groups in said organization. --- coderd/database/dbmem/dbmem.go | 12 +++++- coderd/database/dump.sql | 21 ++++++++++ .../000251_group_member_trigger.down.sql | 2 + .../000251_group_member_trigger.up.sql | 23 ++++++++++ enterprise/members_test.go | 42 +++++++++++++++++++ 5 files changed, 99 insertions(+), 1 deletion(-) create mode 100644 coderd/database/migrations/000251_group_member_trigger.down.sql create mode 100644 coderd/database/migrations/000251_group_member_trigger.up.sql diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index b0a4703b5c4e2..64478065bc408 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1944,7 +1944,7 @@ func (q *FakeQuerier) DeleteOrganization(_ context.Context, id uuid.UUID) error return sql.ErrNoRows } -func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.DeleteOrganizationMemberParams) error { +func (q *FakeQuerier) DeleteOrganizationMember(ctx context.Context, arg database.DeleteOrganizationMemberParams) error { err := validateDatabaseType(arg) if err != nil { return err @@ -1959,6 +1959,16 @@ func (q *FakeQuerier) DeleteOrganizationMember(_ context.Context, arg database.D if len(deleted) == 0 { return sql.ErrNoRows } + + // Delete group member trigger + q.groupMembers = slices.DeleteFunc(q.groupMembers, func(member database.GroupMemberTable) bool { + if member.UserID != arg.UserID { + return false + } + g, _ := q.getGroupByIDNoLock(ctx, member.GroupID) + return g.OrganizationID == arg.OrganizationID + }) + return nil } diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index efa4dfb012a3d..0508ea164620c 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -286,6 +286,25 @@ BEGIN END; $$; +CREATE FUNCTION delete_group_members_on_org_member_delete() RETURNS trigger + LANGUAGE plpgsql + AS $$ +DECLARE +BEGIN + -- Remove the user from all groups associated with the same + -- organization as the organization_member being deleted. + DELETE FROM group_members + WHERE + user_id = OLD.user_id + AND group_id IN ( + SELECT id + FROM groups + WHERE organization_id = OLD.organization_id + ); + RETURN OLD; +END; +$$; + CREATE FUNCTION inhibit_enqueue_if_disabled() RETURNS trigger LANGUAGE plpgsql AS $$ @@ -2041,6 +2060,8 @@ CREATE TRIGGER tailnet_notify_peer_change AFTER INSERT OR DELETE OR UPDATE ON ta CREATE TRIGGER tailnet_notify_tunnel_change AFTER INSERT OR DELETE OR UPDATE ON tailnet_tunnels FOR EACH ROW EXECUTE FUNCTION tailnet_notify_tunnel_change(); +CREATE TRIGGER trigger_delete_group_members_on_org_member_delete BEFORE DELETE ON organization_members FOR EACH ROW EXECUTE FUNCTION delete_group_members_on_org_member_delete(); + CREATE TRIGGER trigger_delete_oauth2_provider_app_token AFTER DELETE ON oauth2_provider_app_tokens FOR EACH ROW EXECUTE FUNCTION delete_deleted_oauth2_provider_app_token_api_key(); CREATE TRIGGER trigger_insert_apikeys BEFORE INSERT ON api_keys FOR EACH ROW EXECUTE FUNCTION insert_apikey_fail_if_user_deleted(); diff --git a/coderd/database/migrations/000251_group_member_trigger.down.sql b/coderd/database/migrations/000251_group_member_trigger.down.sql new file mode 100644 index 0000000000000..477b282e1b3a9 --- /dev/null +++ b/coderd/database/migrations/000251_group_member_trigger.down.sql @@ -0,0 +1,2 @@ +DROP TRIGGER IF EXISTS trigger_delete_group_members_on_org_member_delete ON organization_members; +DROP FUNCTION IF EXISTS delete_group_members_on_org_member_delete; diff --git a/coderd/database/migrations/000251_group_member_trigger.up.sql b/coderd/database/migrations/000251_group_member_trigger.up.sql new file mode 100644 index 0000000000000..04bf61f304333 --- /dev/null +++ b/coderd/database/migrations/000251_group_member_trigger.up.sql @@ -0,0 +1,23 @@ +CREATE FUNCTION delete_group_members_on_org_member_delete() RETURNS TRIGGER + LANGUAGE plpgsql +AS $$ +DECLARE +BEGIN + -- Remove the user from all groups associated with the same + -- organization as the organization_member being deleted. + DELETE FROM group_members + WHERE + user_id = OLD.user_id + AND group_id IN ( + SELECT id + FROM groups + WHERE organization_id = OLD.organization_id + ); + RETURN OLD; +END; +$$; + +CREATE TRIGGER trigger_delete_group_members_on_org_member_delete + BEFORE DELETE ON organization_members + FOR EACH ROW +EXECUTE PROCEDURE delete_group_members_on_org_member_delete(); diff --git a/enterprise/members_test.go b/enterprise/members_test.go index e29912be1bda2..f1944f5c9b9c8 100644 --- a/enterprise/members_test.go +++ b/enterprise/members_test.go @@ -29,6 +29,7 @@ func TestEnterpriseMembers(t *testing.T) { LicenseOptions: &coderdenttest.LicenseOptions{ Features: license.Features{ codersdk.FeatureMultipleOrganizations: 1, + codersdk.FeatureTemplateRBAC: 1, }, }, }) @@ -39,6 +40,21 @@ func TestEnterpriseMembers(t *testing.T) { _, user := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID) ctx := testutil.Context(t, testutil.WaitMedium) + + // Groups exist to ensure a user removed from the org loses their + // group access. + g1, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{ + Name: "foo", + DisplayName: "Foo", + }) + require.NoError(t, err) + + g2, err := orgAdminClient.CreateGroup(ctx, secondOrg.ID, codersdk.CreateGroupRequest{ + Name: "bar", + DisplayName: "Bar", + }) + require.NoError(t, err) + // Verify the org of 3 members members, err := orgAdminClient.OrganizationMembers(ctx, secondOrg.ID) require.NoError(t, err) @@ -47,6 +63,25 @@ func TestEnterpriseMembers(t *testing.T) { []uuid.UUID{first.UserID, user.ID, orgAdmin.ID}, db2sdk.List(members, onlyIDs)) + // Add the member to some groups + _, err = orgAdminClient.PatchGroup(ctx, g1.ID, codersdk.PatchGroupRequest{ + AddUsers: []string{user.ID.String()}, + }) + require.NoError(t, err) + + _, err = orgAdminClient.PatchGroup(ctx, g2.ID, codersdk.PatchGroupRequest{ + AddUsers: []string{user.ID.String()}, + }) + require.NoError(t, err) + + // Verify group membership + userGroups, err := orgAdminClient.Groups(ctx, codersdk.GroupArguments{ + HasMember: user.ID.String(), + }) + require.NoError(t, err) + // Everyone group + 2 groups + require.Len(t, userGroups, 3) + // Delete a member err = orgAdminClient.DeleteOrganizationMember(ctx, secondOrg.ID, user.Username) require.NoError(t, err) @@ -57,6 +92,13 @@ func TestEnterpriseMembers(t *testing.T) { require.ElementsMatch(t, []uuid.UUID{first.UserID, orgAdmin.ID}, db2sdk.List(members, onlyIDs)) + + // User should now belong to 0 groups + userGroups, err = orgAdminClient.Groups(ctx, codersdk.GroupArguments{ + HasMember: user.ID.String(), + }) + require.NoError(t, err) + require.Len(t, userGroups, 0) }) t.Run("PostUser", func(t *testing.T) { From a5304441d8cf4f4df7f5d80363f2794c963601ee Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Sep 2024 10:24:57 -0500 Subject: [PATCH 2/3] test: add unit test in db package to test trigger --- coderd/database/querier_test.go | 97 +++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index 7b7fd8b0a2823..ee28acd631c4e 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -1216,6 +1216,103 @@ func TestExpectOne(t *testing.T) { }) } +func TestGroupRemovalTrigger(t *testing.T) { + t.Parallel() + + db, _ := dbtestutil.NewDB(t) + + orgA := dbgen.Organization(t, db, database.Organization{}) + _, err := db.InsertAllUsersGroup(context.Background(), orgA.ID) + require.NoError(t, err) + + orgB := dbgen.Organization(t, db, database.Organization{}) + _, err = db.InsertAllUsersGroup(context.Background(), orgB.ID) + require.NoError(t, err) + + orgs := []database.Organization{orgA, orgB} + + user := dbgen.User(t, db, database.User{}) + extra := dbgen.User(t, db, database.User{}) + users := []database.User{user, extra} + + groupA1 := dbgen.Group(t, db, database.Group{ + OrganizationID: orgA.ID, + }) + groupA2 := dbgen.Group(t, db, database.Group{ + OrganizationID: orgA.ID, + }) + + groupB1 := dbgen.Group(t, db, database.Group{ + OrganizationID: orgB.ID, + }) + groupB2 := dbgen.Group(t, db, database.Group{ + OrganizationID: orgB.ID, + }) + + groups := []database.Group{groupA1, groupA2, groupB1, groupB2} + + // Add users to all organizations + for _, u := range users { + for _, o := range orgs { + dbgen.OrganizationMember(t, db, database.OrganizationMember{ + OrganizationID: o.ID, + UserID: u.ID, + }) + } + } + + // Add users to all groups + for _, u := range users { + for _, g := range groups { + dbgen.GroupMember(t, db, database.GroupMemberTable{ + GroupID: g.ID, + UserID: u.ID, + }) + } + } + + // Verify user is in all groups + ctx := testutil.Context(t, testutil.WaitLong) + onlyGroupIDs := func(row database.GetGroupsRow) uuid.UUID { + return row.Group.ID + } + userGroups, err := db.GetGroups(ctx, database.GetGroupsParams{ + HasMemberID: user.ID, + }) + require.NoError(t, err) + require.ElementsMatch(t, []uuid.UUID{ + orgA.ID, orgB.ID, // Everyone groups + groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups + }, db2sdk.List(userGroups, onlyGroupIDs)) + + // Remove the user from org A + err = db.DeleteOrganizationMember(ctx, database.DeleteOrganizationMemberParams{ + OrganizationID: orgA.ID, + UserID: user.ID, + }) + require.NoError(t, err) + + // Verify user is no longer in org A groups + userGroups, err = db.GetGroups(ctx, database.GetGroupsParams{ + HasMemberID: user.ID, + }) + require.NoError(t, err) + require.ElementsMatch(t, []uuid.UUID{ + orgB.ID, // Everyone group + groupB1.ID, groupB2.ID, // Org groups + }, db2sdk.List(userGroups, onlyGroupIDs)) + + // Verify extra user is unchanged + extraUserGroups, err := db.GetGroups(ctx, database.GetGroupsParams{ + HasMemberID: extra.ID, + }) + require.NoError(t, err) + require.ElementsMatch(t, []uuid.UUID{ + orgA.ID, orgB.ID, // Everyone groups + groupA1.ID, groupA2.ID, groupB1.ID, groupB2.ID, // Org groups + }, db2sdk.List(extraUserGroups, onlyGroupIDs)) +} + func requireUsersMatch(t testing.TB, expected []database.User, found []database.GetUsersRow, msg string) { t.Helper() require.ElementsMatch(t, expected, database.ConvertUserRows(found), msg) From b0fc5db9a19196b95ab4ee170a5a746a19c658c7 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 17 Sep 2024 19:16:49 -0500 Subject: [PATCH 3/3] bump migration number --- ...mber_trigger.down.sql => 000252_group_member_trigger.down.sql} | 0 ...p_member_trigger.up.sql => 000252_group_member_trigger.up.sql} | 0 2 files changed, 0 insertions(+), 0 deletions(-) rename coderd/database/migrations/{000251_group_member_trigger.down.sql => 000252_group_member_trigger.down.sql} (100%) rename coderd/database/migrations/{000251_group_member_trigger.up.sql => 000252_group_member_trigger.up.sql} (100%) diff --git a/coderd/database/migrations/000251_group_member_trigger.down.sql b/coderd/database/migrations/000252_group_member_trigger.down.sql similarity index 100% rename from coderd/database/migrations/000251_group_member_trigger.down.sql rename to coderd/database/migrations/000252_group_member_trigger.down.sql diff --git a/coderd/database/migrations/000251_group_member_trigger.up.sql b/coderd/database/migrations/000252_group_member_trigger.up.sql similarity index 100% rename from coderd/database/migrations/000251_group_member_trigger.up.sql rename to coderd/database/migrations/000252_group_member_trigger.up.sql