[Backport 5.0] No team cycles (#49621)

This change introduces a check on `UpdateTeam` to not introduce a
circular parent relationship.

The check effectively finds all teams that _are not descendants_ (so not
in the subtree of) modified team, and looks whether the parent is there.
It is only safe to set the parent team ID to a team outside of modified
team's own descendants.

## Test plan

*   Testing database stores with Postgres.
*   Testing GraphQL API with fakes.
 <br> Backport da92373 from #49556

Co-authored-by: Cezary Bartoszuk <cezary.bartoszuk@sourcegraph.com>

cmd/frontend/graphqlbackend/teams.go

@@ -453,6 +453,15 @@ func (r *schemaResolver) UpdateTeam(ctx context.Context, args *UpdateTeamArgs) (
 				return errors.Wrap(err, "cannot find parent team")
 			}
 			if parentTeam.ID != t.ParentTeamID {
+				parentOutsideOfTeamsDescendants, err := tx.Teams().ContainsTeam(ctx, parentTeam.ID, database.ListTeamsOpts{
+					ExceptAncestorID: t.ID,
+				})
+				if err != nil {
+					return errors.Newf("could not determine ancestorship on team update: %s", err)
+				}
+				if !parentOutsideOfTeamsDescendants {
+					return errors.Newf("circular dependency: new parent %q is descendant of updated team %q", parentTeam.Name, t.Name)
+				}
 				needsUpdate = true
 				t.ParentTeamID = parentTeam.ID
 			}

cmd/frontend/graphqlbackend/teams_test.go

@@ -769,6 +769,39 @@ func TestUpdateParentErrorBothNameAndID(t *testing.T) {
 	})
 }
 
+func TestUpdateParentCircular(t *testing.T) {
+	fs := fakedb.New()
+	db := database.NewMockDB()
+	fs.Wire(db)
+	userID := fs.AddUser(types.User{SiteAdmin: true})
+	ctx := userCtx(userID)
+	ctx = featureflag.WithFlags(ctx, featureflag.NewMemoryStore(map[string]bool{"search-ownership": true}, nil, nil))
+	parentTeamID := fs.AddTeam(&types.Team{Name: "parent"})
+	fs.AddTeam(&types.Team{Name: "child", ParentTeamID: parentTeamID})
+	RunTest(t, &Test{
+		Schema:  mustParseGraphQLSchema(t, db),
+		Context: ctx,
+		Query: `mutation UpdateTeam($name: String!, $newParentName: String!) {
+			updateTeam(name: $name, parentTeamName: $newParentName) {
+				parentTeam {
+					name
+				}
+			}
+		}`,
+		ExpectedResult: `null`,
+		ExpectedErrors: []*gqlerrors.QueryError{
+			{
+				Message: `circular dependency: new parent "child" is descendant of updated team "parent"`,
+				Path:    []any{"updateTeam"},
+			},
+		},
+		Variables: map[string]any{
+			"name":          "parent",
+			"newParentName": "child",
+		},
+	})
+}
+
 func TestDeleteTeamByID(t *testing.T) {
 	fs := fakedb.New()
 	db := database.NewMockDB()

internal/database/fakedb/teams.go

@@ -33,14 +33,24 @@ func (fs Fakes) AddTeamMember(moreMembers ...*types.TeamMember) {
 	fs.TeamStore.members = append(fs.TeamStore.members, moreMembers...)
 }
 
+func (fs Fakes) AddTeam(t *types.Team) int32 {
+	u := *t
+	fs.TeamStore.addTeam(&u)
+	return u.ID
+}
+
 func (teams *Teams) CreateTeam(_ context.Context, t *types.Team) error {
-	teams.lastUsedID++
 	u := *t
-	u.ID = teams.lastUsedID
-	teams.list = append(teams.list, &u)
+	teams.addTeam(&u)
 	return nil
 }
 
+func (teams *Teams) addTeam(t *types.Team) {
+	teams.lastUsedID++
+	t.ID = teams.lastUsedID
+	teams.list = append(teams.list, t)
+}
+
 func (teams *Teams) UpdateTeam(_ context.Context, t *types.Team) error {
 	if t == nil {
 		return errors.New("UpdateTeam: team cannot be nil")
@@ -89,7 +99,7 @@ func (teams *Teams) DeleteTeam(_ context.Context, id int32) error {
 
 func (teams *Teams) ListTeams(_ context.Context, opts database.ListTeamsOpts) (selected []*types.Team, next int32, err error) {
 	for _, t := range teams.list {
-		if matches(t, opts) {
+		if teams.matches(t, opts) {
 			selected = append(selected, t)
 		}
 	}
@@ -108,7 +118,20 @@ func (teams *Teams) CountTeams(ctx context.Context, opts database.ListTeamsOpts)
 	return int32(len(selected)), err
 }
 
-func matches(team *types.Team, opts database.ListTeamsOpts) bool {
+func (teams *Teams) ContainsTeam(ctx context.Context, teamID int32, opts database.ListTeamsOpts) (bool, error) {
+	selected, _, err := teams.ListTeams(ctx, opts)
+	if err != nil {
+		return false, err
+	}
+	for _, t := range selected {
+		if t.ID == teamID {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
+func (teams *Teams) matches(team *types.Team, opts database.ListTeamsOpts) bool {
 	if opts.Cursor != 0 && team.ID < opts.Cursor {
 		return false
 	}
@@ -126,10 +149,31 @@ func matches(team *types.Team, opts database.ListTeamsOpts) bool {
 			return false
 		}
 	}
+	if opts.ExceptAncestorID != 0 {
+		for _, id := range teams.ancestors(team.ID) {
+			if opts.ExceptAncestorID == id {
+				return false
+			}
+		}
+	}
 	// opts.ForUserMember is not supported yet as there is no membership fake.
 	return true
 }
 
+func (teams *Teams) ancestors(id int32) []int32 {
+	var ids []int32
+	parentID := id
+	for parentID != 0 {
+		ids = append(ids, parentID)
+		for _, t := range teams.list {
+			if t.ID == parentID {
+				parentID = t.ParentTeamID
+			}
+		}
+	}
+	return ids
+}
+
 type orderedTeamMembers []*types.TeamMember
 
 func (o orderedTeamMembers) Len() int { return len(o) }

internal/database/fakedb/teams_test.go

@@ -0,0 +1,39 @@
+package fakedb_test
+
+import (
+	"context"
+	"sort"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+
+	"github.com/sourcegraph/sourcegraph/internal/database"
+	"github.com/sourcegraph/sourcegraph/internal/database/fakedb"
+	"github.com/sourcegraph/sourcegraph/internal/types"
+)
+
+func TestAncestors(t *testing.T) {
+	fs := fakedb.New()
+	eng := fs.AddTeam(&types.Team{Name: "eng"})
+	source := fs.AddTeam(&types.Team{Name: "source", ParentTeamID: eng})
+	_ = fs.AddTeam(&types.Team{Name: "repo-management", ParentTeamID: source})
+	sales := fs.AddTeam(&types.Team{Name: "sales"})
+	salesLeads := fs.AddTeam(&types.Team{Name: "sales-leads", ParentTeamID: sales})
+	ts, cursor, err := fs.TeamStore.ListTeams(context.Background(), database.ListTeamsOpts{ExceptAncestorID: source})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if cursor != 0 {
+		t.Fatal("non-zero cursor")
+	}
+	want := []*types.Team{
+		{ID: eng, Name: "eng"},
+		{ID: sales, Name: "sales"},
+		{ID: salesLeads, Name: "sales-leads", ParentTeamID: sales},
+	}
+	sort.Slice(ts, func(i, j int) bool { return ts[i].ID < ts[j].ID })
+	sort.Slice(want, func(i, j int) bool { return want[i].ID < want[j].ID })
+	if diff := cmp.Diff(want, ts); diff != "" {
+		t.Errorf("ListTeams{ExceptAncestorID} -want+got: %s", diff)
+	}
+}