store: removing an org member drops their team memberships !335

merged merged by cmc on 2026-09-08 02:35 UTC · krz/gitbay:org-remove-cascade into main

3 files changed, +137 −1

Layout: unified · split

e2e/orgremove_test.go added +72

@@ -0,0 +1,72 @@
	1	package e2e
	2
	3	import (
	4	"os"
	5	"path/filepath"
	6	"strings"
	7	"testing"
	8	)
	9
	10	// Removing a member from an org ends the access they held through its
	11	// teams (#196). Before the fix, team_members rows survived removal, so a
	12	// former member kept pushing.
	13	func TestOrgMemberRemovalEndsTeamAccess(t *testing.T) {
	14	inst := startInstance(t)
	15	aliceKey := inst.newKey(t, "alice")
	16	bobKey := inst.newKey(t, "bob")
	17	inst.admin(t, "admin", "user", "create", "alice", "--key", aliceKey+".pub")
	18	inst.admin(t, "admin", "user", "create", "bob", "--key", bobKey+".pub")
	19
	20	for _, args := range [][]string{
	21	{"org", "create", "acme"},
	22	{"org", "settings", "members-role", "acme", "none"},
	23	{"org", "members", "add", "acme", "bob"},
	24	{"repo", "create", "acme/widget", "--private"},
	25	{"org", "team", "create", "acme", "core"},
	26	{"org", "team", "add", "acme", "core", "bob"},
	27	{"org", "team", "grant", "acme", "core", "acme/widget", "write"},
	28	} {
	29	if _, errOut, code := inst.ssh(t, aliceKey, "", args...); code != 0 {
	30	t.Fatalf("%v: %s", args, errOut)
	31	}
	32	}
	33
	34	// bob pushes through the team grant.
	35	bobEnv := inst.gitEnv(bobKey)
	36	work := t.TempDir()
	37	mustGit(t, work, bobEnv, "clone", inst.sshURL("acme/widget"), "widget")
	38	dir := filepath.Join(work, "widget")
	39	if err := os.WriteFile(filepath.Join(dir, "README"), []byte("hi\n"), 0o644); err != nil {
	40	t.Fatal(err)
	41	}
	42	mustGit(t, dir, bobEnv, "checkout", "-q", "-b", "main")
	43	mustGit(t, dir, bobEnv, "add", "README")
	44	mustGit(t, dir, bobEnv, "commit", "-q", "-m", "one")
	45	mustGit(t, dir, bobEnv, "push", "-q", "origin", "main")
	46
	47	if _, errOut, code := inst.ssh(t, aliceKey, "", "org", "members", "remove", "acme", "bob"); code != 0 {
	48	t.Fatalf("members remove: %s", errOut)
	49	}
	50	out, _, _ := inst.ssh(t, aliceKey, "", "org", "team", "show", "acme", "core", "--json")
	51	if strings.Contains(out, `"bob"`) {
	52	t.Fatalf("team still lists the removed member: %s", out)
	53	}
	54	if err := os.WriteFile(filepath.Join(dir, "README"), []byte("again\n"), 0o644); err != nil {
	55	t.Fatal(err)
	56	}
	57	mustGit(t, dir, bobEnv, "commit", "-q", "-am", "two")
	58	if out, code := gitRun(t, dir, bobEnv, "push", "-q", "origin", "main"); code == 0 {
	59	t.Fatalf("removed member still pushes:\n%s", out)
	60	}
	61	if _, _, code := inst.ssh(t, bobKey, "", "repo", "show", "acme/widget"); code != 3 {
	62	t.Fatalf("removed member still sees the private repo: exit %d", code)
	63	}
	64
	65	// Re-adding to the org does not silently restore the team grant.
	66	if _, errOut, code := inst.ssh(t, aliceKey, "", "org", "members", "add", "acme", "bob"); code != 0 {
	67	t.Fatalf("members add: %s", errOut)
	68	}
	69	if _, _, code := inst.ssh(t, bobKey, "", "repo", "show", "acme/widget"); code != 3 {
	70	t.Fatalf("re-added member regained the team grant: exit %d", code)
	71	}
	72	}

internal/store/orgremove_test.go added +57

@@ -0,0 +1,57 @@
	1	package store
	2
	3	import "testing"
	4
	5	// Removing an org member drops their team memberships in that org. Team
	6	// add requires membership, so a row that survived removal would grant a
	7	// non-member access through the team (#196).
	8	func TestRemoveOrgMemberDropsTeamRows(t *testing.T) {
	9	s := open(t)
	10	if err := s.MigrateUp(); err != nil {
	11	t.Fatal(err)
	12	}
	13	alice, err := s.CreateUser("alice", false)
	14	if err != nil {
	15	t.Fatal(err)
	16	}
	17	bob, err := s.CreateUser("bob", false)
	18	if err != nil {
	19	t.Fatal(err)
	20	}
	21	oid, err := s.CreateOrg("acme", alice)
	22	if err != nil {
	23	t.Fatal(err)
	24	}
	25	other, err := s.CreateOrg("other", alice)
	26	if err != nil {
	27	t.Fatal(err)
	28	}
	29	for _, o := range []int64{oid, other} {
	30	if err := s.SetOrgMember(o, bob, "member"); err != nil {
	31	t.Fatal(err)
	32	}
	33	}
	34	core, err := s.CreateTeam(oid, "core")
	35	if err != nil {
	36	t.Fatal(err)
	37	}
	38	elsewhere, err := s.CreateTeam(other, "core")
	39	if err != nil {
	40	t.Fatal(err)
	41	}
	42	for _, tm := range []int64{core, elsewhere} {
	43	if err := s.AddTeamMember(tm, bob); err != nil {
	44	t.Fatal(err)
	45	}
	46	}
	47	if err := s.RemoveOrgMember(oid, bob); err != nil {
	48	t.Fatal(err)
	49	}
	50	if m, _ := s.TeamMembers(core); len(m) != 0 {
	51	t.Fatalf("team rows survived org removal: %v", m)
	52	}
	53	// The other org's team is untouched.
	54	if m, _ := s.TeamMembers(elsewhere); len(m) != 1 \|\| m[0] != "bob" {
	55	t.Fatalf("removal reached another org's team: %v", m)
	56	}
	57	}

internal/store/orgs.go +8 −1

 	return tx.Commit()
 }
-// RemoveOrgMember drops a member, refusing to remove the last admin.
+// RemoveOrgMember drops a member, refusing to remove the last admin. The
+// account's team memberships in the org go with it: team add requires
+// membership, so a row left behind would grant access to a non-member.
 func (s *Store) RemoveOrgMember(orgID, userID int64) error {
 	tx, err := s.DB.Begin()
 	if err != nil {
 	if n, _ := res.RowsAffected(); n == 0 {
 		return ErrNotFound
 	}
+	if _, err := tx.Exec(
+		"DELETE FROM team_members WHERE user_id = ? AND team_id IN (SELECT id FROM teams WHERE org_id = ?)",
+		userID, orgID); err != nil {
+		return err
+	}
 	return tx.Commit()
 }