From 222de7ccb82993f21842fa8cde2073c12b0d5d76 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 5 Apr 2026 16:06:22 +0300 Subject: [PATCH 01/13] feat(github): support team hierarchy in GH_TEAM_ALLOWLIST When a parent team is added to ATLANTIS_GH_TEAM_ALLOWLIST, users who are members of any descendant (child/grandchild) team are now correctly authorized, instead of being rejected. The fix adds GetChildTeams to the GitHub client, which fetches direct child teams via GraphQL. In checkUserPermissions, after the fast-path direct-membership check fails, each allowlisted team is expanded to all its descendants (up to 20 levels deep) using recursive GetChildTeams calls. The user's direct teams are then checked against this expanded set. Non-GitHub VCS clients are unaffected. Fixes #6107 Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner.go | 70 +++++++++++++++++++++++-- server/events/vcs/github/client.go | 45 ++++++++++++++++ server/events/vcs/github/client_test.go | 48 +++++++++++++++++ 3 files changed, 160 insertions(+), 3 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 0bdcf8dbab..8bcf491fb6 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -17,6 +17,7 @@ import ( "errors" "fmt" "strconv" + "strings" "github.com/drmaxgit/go-azuredevops/azuredevops" "github.com/google/go-github/v83/github" @@ -262,7 +263,39 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models } } -// checkUserPermissions checks if the user has permissions to execute the command +// childTeamFetcher is implemented by VCS clients that support GitHub-style team hierarchies. +// GetChildTeams returns the direct child team slugs for a given team slug. +type childTeamFetcher interface { + GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) +} + +// fetchDescendantTeams recursively fetches all descendant team slugs for the given team, +// up to maxDepth levels deep. This prevents infinite loops in circular-hierarchy edge cases. +func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { + if maxDepth <= 0 { + return nil, nil + } + children, err := fetcher.GetChildTeams(logger, repo, teamSlug) + if err != nil { + return nil, err + } + result := make([]string, len(children)) + copy(result, children) + for _, child := range children { + grandchildren, err := fetchDescendantTeams(fetcher, logger, repo, child, maxDepth-1) + if err != nil { + logger.Warn("Could not fetch child teams for '%s': %s", child, err) + continue + } + result = append(result, grandchildren...) + } + return result, nil +} + +// checkUserPermissions checks if the user has permissions to execute the command. +// It first checks direct team membership against the allowlist. If that fails and the +// VCS client supports team hierarchy (childTeamFetcher), it expands each allowlisted team +// to include all its descendant teams (up to 20 levels deep) and re-checks. func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user models.User, cmdName string) (bool, error) { if c.TeamAllowlistChecker == nil || !c.TeamAllowlistChecker.HasRules() { // allowlist restriction is not enabled @@ -277,11 +310,42 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user model Verbose: false, API: false, } - ok := c.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(ctx, user.Teams, cmdName) + + // Fast path: user is a direct member of an allowlisted team. + if c.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(ctx, user.Teams, cmdName) { + return true, nil + } + + // Slow path: check if the user belongs to a child team of any allowlisted team. + // Only available when the VCS client supports fetching child teams (e.g. GitHub). + fetcher, ok := c.VCSClient.(childTeamFetcher) if !ok { return false, nil } - return true, nil + + const maxHierarchyDepth = 20 + for _, allowedTeam := range c.TeamAllowlistChecker.AllTeams() { + if allowedTeam == "*" { + continue + } + // Only expand teams that actually grant permission for this command. + if !c.TeamAllowlistChecker.IsCommandAllowedForTeam(ctx, allowedTeam, cmdName) { + continue + } + descendants, err := fetchDescendantTeams(fetcher, c.Logger, repo, allowedTeam, maxHierarchyDepth) + if err != nil { + c.Logger.Warn("Could not fetch child teams for '%s': %s", allowedTeam, err) + continue + } + for _, userTeam := range user.Teams { + for _, desc := range descendants { + if strings.EqualFold(userTeam, desc) { + return true, nil + } + } + } + } + return false, nil } // checkVarFilesInPlanCommandAllowlisted checks if paths in a 'plan' command are allowlisted. diff --git a/server/events/vcs/github/client.go b/server/events/vcs/github/client.go index 57f91383ab..0ce41f8541 100644 --- a/server/events/vcs/github/client.go +++ b/server/events/vcs/github/client.go @@ -1097,6 +1097,51 @@ func (g *Client) GetTeamNamesForUser(logger logging.SimpleLogging, repo models.R return teamNames, nil } +// GetChildTeams returns the slugs of the direct child teams of the given team. +// Use this with fetchDescendantTeams in the command runner to expand an allowlisted team +// to all its descendants, supporting GitHub team hierarchy. +// https://docs.github.com/en/graphql/reference/objects#team +func (g *Client) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { + logger.Debug("Getting child teams for GitHub team '%s'", teamSlug) + orgName := repo.Owner + variables := map[string]any{ + "orgName": githubv4.String(orgName), + "teamSlug": githubv4.String(teamSlug), + "childCursor": (*githubv4.String)(nil), + } + var q struct { + Organization struct { + Team struct { + ChildTeams struct { + Nodes []struct { + Slug string + } + PageInfo struct { + EndCursor githubv4.String + HasNextPage bool + } + } `graphql:"childTeams(first: 100, after: $childCursor)"` + } `graphql:"team(slug: $teamSlug)"` + } `graphql:"organization(login: $orgName)"` + } + var childSlugs []string + ctx := context.Background() + for { + err := g.v4Client.Query(ctx, &q, variables) + if err != nil { + return nil, err + } + for _, node := range q.Organization.Team.ChildTeams.Nodes { + childSlugs = append(childSlugs, node.Slug) + } + if !q.Organization.Team.ChildTeams.PageInfo.HasNextPage { + break + } + variables["childCursor"] = githubv4.NewString(q.Organization.Team.ChildTeams.PageInfo.EndCursor) + } + return childSlugs, nil +} + // ExchangeCode returns a newly created app's info func (g *Client) ExchangeCode(logger logging.SimpleLogging, code string) (*GithubAppTemporarySecrets, error) { logger.Debug("Exchanging code for app secrets") diff --git a/server/events/vcs/github/client_test.go b/server/events/vcs/github/client_test.go index 44970cce70..15d60ce1bb 100644 --- a/server/events/vcs/github/client_test.go +++ b/server/events/vcs/github/client_test.go @@ -1589,6 +1589,54 @@ func TestClient_GetTeamNamesForUser(t *testing.T) { Equals(t, []string{"frontend-developers", "employees"}, teams) } +// TestClient_GetChildTeams verifies that direct child teams of a given team are returned. +func TestClient_GetChildTeams(t *testing.T) { + logger := logging.NewNoopLogger(t) + // Mocked GraphQL response: "parent-team" has two direct children. + resp := `{ + "data":{ + "organization": { + "team": { + "childTeams": { + "nodes": [ + {"slug": "child-team-a"}, + {"slug": "child-team-b"} + ], + "pageInfo": { + "endCursor": "Y3Vyc29yOnYyOpHOAFMoLQ==", + "hasNextPage": false + } + } + } + } + } + }` + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/graphql": + w.Write([]byte(resp)) // nolint: errcheck + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + client, err := github.New(testServerURL.Host, &github.UserCredentials{"user", "pass", ""}, github.Config{}, 0, logger) + Ok(t, err) + defer disableSSLVerification()() + + children, err := client.GetChildTeams( + logger, + models.Repo{Owner: "testrepo"}, + "parent-team", + ) + Ok(t, err) + Equals(t, []string{"child-team-a", "child-team-b"}, children) +} + func TestClient_DiscardReviews(t *testing.T) { logger := logging.NewNoopLogger(t) type ResponseDef struct { From f386f36b260f744c62dd99a1a42377a9ac180861 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Tue, 7 Apr 2026 19:08:40 +0300 Subject: [PATCH 02/13] test(github): add tests for fetchDescendantTeams and checkUserPermissions hierarchy Addresses the missing test coverage flagged by Copilot and code review: - TestFetchDescendantTeams: leaf team, single/multi-level nesting, maxDepth cutoff at 0 and 1, error propagation at root, and soft-fail on recursive errors while sibling subtrees continue - TestCheckUserPermissions: nil checker, direct member fast path, non-member rejection without hierarchy support, and table-driven slow-path cases (child team allowed, grandchild team allowed, unrelated team rejected, wrong command rejected, wildcard rule with no-team user) Also adds childTeamVCSClient test helper that satisfies both vcs.Client and childTeamFetcher, enabling the slow path to be exercised without a real VCS connection. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner_internal_test.go | 214 ++++++++++++++++++ 1 file changed, 214 insertions(+) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 8cecfbcadd..9628839442 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -4,14 +4,228 @@ package events import ( + "errors" "testing" "github.com/runatlantis/atlantis/server/events/command" "github.com/runatlantis/atlantis/server/events/models" + "github.com/runatlantis/atlantis/server/events/vcs" "github.com/runatlantis/atlantis/server/logging" . "github.com/runatlantis/atlantis/testing" ) +// mockChildTeamFetcher is a test double for childTeamFetcher. +// It maps team slug -> list of direct child slugs, and returns an error for +// any slug present in errOn. +type mockChildTeamFetcher struct { + children map[string][]string + errOn map[string]bool +} + +func (m *mockChildTeamFetcher) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, teamSlug string) ([]string, error) { + if m.errOn[teamSlug] { + return nil, errors.New("API error for " + teamSlug) + } + return m.children[teamSlug], nil +} + +// childTeamVCSClient combines vcs.NotConfiguredVCSClient (satisfying vcs.Client) +// with a mockChildTeamFetcher (satisfying childTeamFetcher), allowing the slow +// path in checkUserPermissions to be exercised without a real VCS connection. +type childTeamVCSClient struct { + vcs.NotConfiguredVCSClient + mockChildTeamFetcher +} + +func (c *childTeamVCSClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { + return c.mockChildTeamFetcher.GetChildTeams(logger, repo, teamSlug) +} + +func TestFetchDescendantTeams(t *testing.T) { + logger := logging.NewNoopLogger(t) + repo := models.Repo{Owner: "test-org"} + + t.Run("leaf team returns empty", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{children: map[string][]string{}} + result, err := fetchDescendantTeams(fetcher, logger, repo, "leaf-team", 20) + Ok(t, err) + Equals(t, 0, len(result)) + }) + + t.Run("single level of children", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{children: map[string][]string{ + "parent": {"child-a", "child-b"}, + }} + result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 20) + Ok(t, err) + Equals(t, []string{"child-a", "child-b"}, result) + }) + + t.Run("multiple levels of nesting", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{children: map[string][]string{ + "grandparent": {"parent"}, + "parent": {"child"}, + }} + result, err := fetchDescendantTeams(fetcher, logger, repo, "grandparent", 20) + Ok(t, err) + Equals(t, []string{"parent", "child"}, result) + }) + + t.Run("maxDepth=0 returns nothing", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{children: map[string][]string{ + "parent": {"child"}, + }} + result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 0) + Ok(t, err) + Equals(t, []string(nil), result) + }) + + t.Run("maxDepth=1 returns only direct children", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{children: map[string][]string{ + "grandparent": {"parent"}, + "parent": {"child"}, + }} + result, err := fetchDescendantTeams(fetcher, logger, repo, "grandparent", 1) + Ok(t, err) + Equals(t, []string{"parent"}, result) + }) + + t.Run("error at root propagates", func(t *testing.T) { + fetcher := &mockChildTeamFetcher{ + children: map[string][]string{}, + errOn: map[string]bool{"parent": true}, + } + _, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 20) + Assert(t, err != nil, "expected error to propagate from root team") + }) + + t.Run("error in recursive call is logged and skipped", func(t *testing.T) { + // parent fetches OK; fetching child-a's children errors. + // child-b and its subtree should still be traversed. + fetcher := &mockChildTeamFetcher{ + children: map[string][]string{ + "parent": {"child-a", "child-b"}, + "child-b": {"grandchild-b"}, + }, + errOn: map[string]bool{"child-a": true}, + } + result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 20) + Ok(t, err) + // child-a is present (direct child), its subtree is skipped on error. + // child-b and grandchild-b are also present. + Equals(t, 3, len(result)) + }) +} + +func TestCheckUserPermissions(t *testing.T) { + logger := logging.NewNoopLogger(t) + repo := models.Repo{Owner: "test-org"} + + t.Run("no rules allows everyone", func(t *testing.T) { + cr := &DefaultCommandRunner{ + Logger: logger, + TeamAllowlistChecker: &command.DefaultTeamAllowlistChecker{}, + } + user := models.User{Username: "alice", Teams: []string{"some-team"}} + ok, err := cr.checkUserPermissions(repo, user, "plan") + Ok(t, err) + Assert(t, ok, "expected allowed when no rules") + }) + + t.Run("fast path: direct team member is allowed", func(t *testing.T) { + checker, _ := command.NewTeamAllowlistChecker("dev-team:plan") + cr := &DefaultCommandRunner{ + Logger: logger, + TeamAllowlistChecker: checker, + VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher + } + user := models.User{Username: "alice", Teams: []string{"dev-team"}} + ok, err := cr.checkUserPermissions(repo, user, "plan") + Ok(t, err) + Assert(t, ok, "expected direct member to be allowed") + }) + + t.Run("fast path: non-member without hierarchy support is rejected", func(t *testing.T) { + checker, _ := command.NewTeamAllowlistChecker("dev-team:plan") + cr := &DefaultCommandRunner{ + Logger: logger, + TeamAllowlistChecker: checker, + VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher + } + user := models.User{Username: "alice", Teams: []string{"other-team"}} + ok, err := cr.checkUserPermissions(repo, user, "plan") + Ok(t, err) + Assert(t, !ok, "expected non-member to be rejected when no hierarchy support") + }) + + hierarchyCases := map[string]struct { + allowlist string + userTeams []string + hierarchy map[string][]string + cmdName string + expectAllow bool + }{ + "slow path: user in direct child team is allowed": { + allowlist: "parent-team:plan", + userTeams: []string{"child-team"}, + hierarchy: map[string][]string{"parent-team": {"child-team"}}, + cmdName: "plan", + expectAllow: true, + }, + "slow path: user in grandchild team is allowed": { + allowlist: "grandparent-team:plan", + userTeams: []string{"grandchild-team"}, + hierarchy: map[string][]string{ + "grandparent-team": {"parent-team"}, + "parent-team": {"grandchild-team"}, + }, + cmdName: "plan", + expectAllow: true, + }, + "slow path: user not in any descendant is rejected": { + allowlist: "parent-team:plan", + userTeams: []string{"unrelated-team"}, + hierarchy: map[string][]string{"parent-team": {"child-team"}}, + cmdName: "plan", + expectAllow: false, + }, + "slow path: user in child team but wrong command is rejected": { + allowlist: "parent-team:apply", + userTeams: []string{"child-team"}, + hierarchy: map[string][]string{"parent-team": {"child-team"}}, + cmdName: "plan", + expectAllow: false, + }, + // *:plan allows everyone including users with no team memberships. + // The fast path handles this via IsCommandAllowedForAnyTeam's zero-team wildcard check. + "fast path: wildcard team rule allows user with no teams": { + allowlist: "*:plan", + userTeams: []string{}, + hierarchy: map[string][]string{}, + cmdName: "plan", + expectAllow: true, + }, + } + + for name, tc := range hierarchyCases { + t.Run(name, func(t *testing.T) { + checker, err := command.NewTeamAllowlistChecker(tc.allowlist) + Ok(t, err) + cr := &DefaultCommandRunner{ + Logger: logger, + TeamAllowlistChecker: checker, + VCSClient: &childTeamVCSClient{ + mockChildTeamFetcher: mockChildTeamFetcher{children: tc.hierarchy}, + }, + } + user := models.User{Username: "testuser", Teams: tc.userTeams} + ok, checkErr := cr.checkUserPermissions(repo, user, tc.cmdName) + Ok(t, checkErr) + Equals(t, tc.expectAllow, ok) + }) + } +} + func TestApplyUpdateCommitStatus(t *testing.T) { cases := map[string]struct { cmd command.Name From f32da28c6dbde061ef31b4a992531cb646657709 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Thu, 9 Apr 2026 11:27:53 +0300 Subject: [PATCH 03/13] fix(github): propagate GetChildTeams through ClientProxy and InstrumentedGithubClient The team hierarchy slow path in checkUserPermissions asserts c.VCSClient against childTeamFetcher, but c.VCSClient is always a *ClientProxy wrapping an *InstrumentedGithubClient. Neither type implemented GetChildTeams, so the assertion always failed and hierarchy expansion was silently skipped. Add GetChildTeams to both ClientProxy and InstrumentedGithubClient so the interface is satisfied and calls are correctly delegated to the underlying *github.Client. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/vcs/github/instrumented_client.go | 12 ++++++++++++ server/events/vcs/proxy.go | 12 ++++++++++++ 2 files changed, 24 insertions(+) diff --git a/server/events/vcs/github/instrumented_client.go b/server/events/vcs/github/instrumented_client.go index a09d371273..f9d96fe714 100644 --- a/server/events/vcs/github/instrumented_client.go +++ b/server/events/vcs/github/instrumented_client.go @@ -50,6 +50,18 @@ type InstrumentedGithubClient struct { Logger logging.SimpleLogging } +// GetChildTeams delegates to the underlying GitHub client so that *InstrumentedGithubClient +// satisfies the childTeamFetcher interface used by the command runner for team hierarchy checks. +func (c *InstrumentedGithubClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { + type childTeamFetcher interface { + GetChildTeams(logging.SimpleLogging, models.Repo, string) ([]string, error) + } + if fetcher, ok := c.InstrumentedClient.Client.(childTeamFetcher); ok { + return fetcher.GetChildTeams(logger, repo, teamSlug) + } + return nil, nil +} + func (c *InstrumentedGithubClient) GetPullRequest(logger logging.SimpleLogging, repo models.Repo, pullNum int) (*github.PullRequest, error) { scope := c.StatsScope.SubScope("get_pull_request") scope = common.SetGitScopeTags(scope, repo.FullName, pullNum) diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index 700bf3798e..dd3bd04667 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -116,3 +116,15 @@ func (d *ClientProxy) GetCloneURL(logger logging.SimpleLogging, VCSHostType mode func (d *ClientProxy) GetPullLabels(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) ([]string, error) { return d.clients[repo.VCSHost.Type].GetPullLabels(logger, repo, pull) } + +// GetChildTeams delegates to the underlying VCS client if it supports fetching child teams +// (e.g. GitHub). Returns nil, nil for VCS providers that don't support team hierarchies. +func (d *ClientProxy) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { + type childTeamFetcher interface { + GetChildTeams(logging.SimpleLogging, models.Repo, string) ([]string, error) + } + if fetcher, ok := d.clients[repo.VCSHost.Type].(childTeamFetcher); ok { + return fetcher.GetChildTeams(logger, repo, teamSlug) + } + return nil, nil +} From f57331f1a38c46a48f860185c36bc039ff2bcdaf Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Thu, 9 Apr 2026 15:54:15 +0300 Subject: [PATCH 04/13] fix(allowlist): propagate matched parent team to per-project filter When a user belongs to a child team of an allowlisted team, the global checkUserPermissions passes via the hierarchy slow path, but the per-project filter in buildAllCommandsByCfg/buildProjectCommandCtxs only does a direct team membership check. This caused child-team members to always see "Ran Plan for 0 projects:" even though the command-level check allowed them through. Fix by changing checkUserPermissions to accept *models.User and appending the matched allowlisted parent team to user.Teams when a hierarchy match is found. This ensures subsequent per-project allowlist checks (which use direct membership) also pass. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner.go | 15 +++++++--- server/events/command_runner_internal_test.go | 29 ++++++++++++++++--- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 8bcf491fb6..65e9d96430 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -166,7 +166,7 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo return } - ok, err := c.checkUserPermissions(baseRepo, user, "plan") + ok, err := c.checkUserPermissions(baseRepo, &user, "plan") if err != nil { log.Err("Unable to check user permissions: %s", err) return @@ -296,7 +296,10 @@ func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging // It first checks direct team membership against the allowlist. If that fails and the // VCS client supports team hierarchy (childTeamFetcher), it expands each allowlisted team // to include all its descendant teams (up to 20 levels deep) and re-checks. -func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user models.User, cmdName string) (bool, error) { +// When a match is found via hierarchy, the matched allowlisted parent team is appended to +// user.Teams so that subsequent per-project allowlist checks (which use direct membership +// only) also pass. +func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *models.User, cmdName string) (bool, error) { if c.TeamAllowlistChecker == nil || !c.TeamAllowlistChecker.HasRules() { // allowlist restriction is not enabled return true, nil @@ -306,7 +309,7 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user model CommandName: cmdName, Log: c.Logger, Pull: models.PullRequest{}, - User: user, + User: *user, Verbose: false, API: false, } @@ -340,6 +343,10 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user model for _, userTeam := range user.Teams { for _, desc := range descendants { if strings.EqualFold(userTeam, desc) { + // Add the matched allowlisted parent team to user.Teams so that + // per-project allowlist filters (which check direct membership) + // also pass for this user. + user.Teams = append(user.Teams, allowedTeam) return true, nil } } @@ -390,7 +397,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead return } - ok, err := c.checkUserPermissions(baseRepo, user, cmd.Name.String()) + ok, err := c.checkUserPermissions(baseRepo, &user, cmd.Name.String()) if err != nil { c.Logger.Err("Unable to check user permissions: %s", err) return diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 9628839442..d6e3200eb9 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -127,7 +127,7 @@ func TestCheckUserPermissions(t *testing.T) { TeamAllowlistChecker: &command.DefaultTeamAllowlistChecker{}, } user := models.User{Username: "alice", Teams: []string{"some-team"}} - ok, err := cr.checkUserPermissions(repo, user, "plan") + ok, err := cr.checkUserPermissions(repo, &user, "plan") Ok(t, err) Assert(t, ok, "expected allowed when no rules") }) @@ -140,7 +140,7 @@ func TestCheckUserPermissions(t *testing.T) { VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher } user := models.User{Username: "alice", Teams: []string{"dev-team"}} - ok, err := cr.checkUserPermissions(repo, user, "plan") + ok, err := cr.checkUserPermissions(repo, &user, "plan") Ok(t, err) Assert(t, ok, "expected direct member to be allowed") }) @@ -153,7 +153,7 @@ func TestCheckUserPermissions(t *testing.T) { VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher } user := models.User{Username: "alice", Teams: []string{"other-team"}} - ok, err := cr.checkUserPermissions(repo, user, "plan") + ok, err := cr.checkUserPermissions(repo, &user, "plan") Ok(t, err) Assert(t, !ok, "expected non-member to be rejected when no hierarchy support") }) @@ -219,11 +219,32 @@ func TestCheckUserPermissions(t *testing.T) { }, } user := models.User{Username: "testuser", Teams: tc.userTeams} - ok, checkErr := cr.checkUserPermissions(repo, user, tc.cmdName) + ok, checkErr := cr.checkUserPermissions(repo, &user, tc.cmdName) Ok(t, checkErr) Equals(t, tc.expectAllow, ok) }) } + + t.Run("slow path: matched parent team is appended to user.Teams", func(t *testing.T) { + checker, err := command.NewTeamAllowlistChecker("parent-team:plan") + Ok(t, err) + cr := &DefaultCommandRunner{ + Logger: logger, + TeamAllowlistChecker: checker, + VCSClient: &childTeamVCSClient{ + mockChildTeamFetcher: mockChildTeamFetcher{ + children: map[string][]string{"parent-team": {"child-team"}}, + }, + }, + } + user := models.User{Username: "alice", Teams: []string{"child-team"}} + ok, checkErr := cr.checkUserPermissions(repo, &user, "plan") + Ok(t, checkErr) + Assert(t, ok, "expected child team member to be allowed") + // The matched parent team should be appended so per-project allowlist checks pass. + Assert(t, len(user.Teams) == 2, "expected user.Teams to contain both child-team and parent-team") + Equals(t, "parent-team", user.Teams[1]) + }) } func TestApplyUpdateCommitStatus(t *testing.T) { From b1cd614da34599c466362cc1bf5103b4d03f9498 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 11:22:17 +0300 Subject: [PATCH 05/13] refactor(vcs): add GetChildTeams to Client interface with stubs for non-GitHub providers Addresses PR review feedback: instead of using anonymous types and type assertions in the proxy and instrumented client, GetChildTeams is now a first-class method on the Client interface. All non-GitHub VCS providers return nil, nil as they don't support team hierarchies. Signed-off-by: hussein-mimi Co-Authored-By: Claude Sonnet 4.6 --- server/events/vcs/azuredevops/client.go | 4 ++ server/events/vcs/bitbucketcloud/client.go | 4 ++ server/events/vcs/bitbucketserver/client.go | 4 ++ server/events/vcs/client.go | 4 ++ server/events/vcs/gitea/client.go | 4 ++ .../events/vcs/github/instrumented_client.go | 11 ---- server/events/vcs/gitlab/client.go | 4 ++ server/events/vcs/mocks/mock_client.go | 60 +++++++++++++++++++ .../events/vcs/not_configured_vcs_client.go | 4 ++ server/events/vcs/proxy.go | 10 +--- 10 files changed, 89 insertions(+), 20 deletions(-) diff --git a/server/events/vcs/azuredevops/client.go b/server/events/vcs/azuredevops/client.go index 0538c7be84..b1d90c17d1 100644 --- a/server/events/vcs/azuredevops/client.go +++ b/server/events/vcs/azuredevops/client.go @@ -454,3 +454,7 @@ func (g *Client) GetCloneURL(_ logging.SimpleLogging, VCSHostType models.VCSHost func (g *Client) GetPullLabels(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest) ([]string, error) { return nil, fmt.Errorf("not yet implemented") } + +func (g *Client) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} diff --git a/server/events/vcs/bitbucketcloud/client.go b/server/events/vcs/bitbucketcloud/client.go index 68f8d2316a..cdb00b3aac 100644 --- a/server/events/vcs/bitbucketcloud/client.go +++ b/server/events/vcs/bitbucketcloud/client.go @@ -393,3 +393,7 @@ func (b *Client) GetCloneURL(_ logging.SimpleLogging, _ models.VCSHostType, _ st func (b *Client) GetPullLabels(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest) ([]string, error) { return nil, fmt.Errorf("not yet implemented") } + +func (b *Client) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} diff --git a/server/events/vcs/bitbucketserver/client.go b/server/events/vcs/bitbucketserver/client.go index ea00e4d71f..35cd5827bb 100644 --- a/server/events/vcs/bitbucketserver/client.go +++ b/server/events/vcs/bitbucketserver/client.go @@ -376,3 +376,7 @@ func (b *Client) GetCloneURL(_ logging.SimpleLogging, _ models.VCSHostType, _ st func (b *Client) GetPullLabels(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest) ([]string, error) { return nil, fmt.Errorf("not yet implemented") } + +func (b *Client) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index eff8b3cc08..f47e6eac12 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -53,4 +53,8 @@ type Client interface { // GetPullLabels returns the labels of a pull request GetPullLabels(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) ([]string, error) + + // GetChildTeams returns the slugs of all teams that are children of the given team. + // Returns nil, nil for VCS providers that don't support team hierarchies. + GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) } diff --git a/server/events/vcs/gitea/client.go b/server/events/vcs/gitea/client.go index abce0ddf45..c82634afa4 100644 --- a/server/events/vcs/gitea/client.go +++ b/server/events/vcs/gitea/client.go @@ -506,6 +506,10 @@ func (c *Client) GetPullLabels(logger logging.SimpleLogging, repo models.Repo, p return results, nil } +func (c *Client) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} + func ValidateSignature(payload []byte, signature string, secretKey []byte) error { isValid, err := gitea.VerifyWebhookSignature(string(secretKey), signature, payload) if err != nil { diff --git a/server/events/vcs/github/instrumented_client.go b/server/events/vcs/github/instrumented_client.go index f9d96fe714..94c28c1051 100644 --- a/server/events/vcs/github/instrumented_client.go +++ b/server/events/vcs/github/instrumented_client.go @@ -50,17 +50,6 @@ type InstrumentedGithubClient struct { Logger logging.SimpleLogging } -// GetChildTeams delegates to the underlying GitHub client so that *InstrumentedGithubClient -// satisfies the childTeamFetcher interface used by the command runner for team hierarchy checks. -func (c *InstrumentedGithubClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { - type childTeamFetcher interface { - GetChildTeams(logging.SimpleLogging, models.Repo, string) ([]string, error) - } - if fetcher, ok := c.InstrumentedClient.Client.(childTeamFetcher); ok { - return fetcher.GetChildTeams(logger, repo, teamSlug) - } - return nil, nil -} func (c *InstrumentedGithubClient) GetPullRequest(logger logging.SimpleLogging, repo models.Repo, pullNum int) (*github.PullRequest, error) { scope := c.StatsScope.SubScope("get_pull_request") diff --git a/server/events/vcs/gitlab/client.go b/server/events/vcs/gitlab/client.go index 0e02014388..d52d1c8ab2 100644 --- a/server/events/vcs/gitlab/client.go +++ b/server/events/vcs/gitlab/client.go @@ -781,3 +781,7 @@ func (g *Client) GetPullLabels(logger logging.SimpleLogging, repo models.Repo, p return mr.Labels, nil } + +func (g *Client) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} diff --git a/server/events/vcs/mocks/mock_client.go b/server/events/vcs/mocks/mock_client.go index 338308935a..b60e935d95 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -117,6 +117,25 @@ func (mock *MockClient) GetModifiedFiles(logger logging.SimpleLogging, repo mode return _ret0, _ret1 } +func (mock *MockClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { + if mock == nil { + panic("mock must not be nil. Use myMock := NewMockClient().") + } + _params := []pegomock.Param{logger, repo, teamSlug} + _result := pegomock.GetGenericMockFrom(mock).Invoke("GetChildTeams", _params, []reflect.Type{reflect.TypeOf((*[]string)(nil)).Elem(), reflect.TypeOf((*error)(nil)).Elem()}) + var _ret0 []string + var _ret1 error + if len(_result) != 0 { + if _result[0] != nil { + _ret0 = _result[0].([]string) + } + if _result[1] != nil { + _ret1 = _result[1].(error) + } + } + return _ret0, _ret1 +} + func (mock *MockClient) GetPullLabels(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) ([]string, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") @@ -547,6 +566,47 @@ func (c *MockClient_GetModifiedFiles_OngoingVerification) GetAllCapturedArgument return } +func (verifier *VerifierMockClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) *MockClient_GetChildTeams_OngoingVerification { + _params := []pegomock.Param{logger, repo, teamSlug} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "GetChildTeams", _params, verifier.timeout) + return &MockClient_GetChildTeams_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type MockClient_GetChildTeams_OngoingVerification struct { + mock *MockClient + methodInvocations []pegomock.MethodInvocation +} + +func (c *MockClient_GetChildTeams_OngoingVerification) GetCapturedArguments() (logging.SimpleLogging, models.Repo, string) { + logger, repo, teamSlug := c.GetAllCapturedArguments() + return logger[len(logger)-1], repo[len(repo)-1], teamSlug[len(teamSlug)-1] +} + +func (c *MockClient_GetChildTeams_OngoingVerification) GetAllCapturedArguments() (_param0 []logging.SimpleLogging, _param1 []models.Repo, _param2 []string) { + _params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) + if len(_params) > 0 { + if len(_params) > 0 { + _param0 = make([]logging.SimpleLogging, len(c.methodInvocations)) + for u, param := range _params[0] { + _param0[u] = param.(logging.SimpleLogging) + } + } + if len(_params) > 1 { + _param1 = make([]models.Repo, len(c.methodInvocations)) + for u, param := range _params[1] { + _param1[u] = param.(models.Repo) + } + } + if len(_params) > 2 { + _param2 = make([]string, len(c.methodInvocations)) + for u, param := range _params[2] { + _param2[u] = param.(string) + } + } + } + return +} + func (verifier *VerifierMockClient) GetPullLabels(logger logging.SimpleLogging, repo models.Repo, pull models.PullRequest) *MockClient_GetPullLabels_OngoingVerification { _params := []pegomock.Param{logger, repo, pull} methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "GetPullLabels", _params, verifier.timeout) diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 7648ccbce8..29c2216b63 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -78,3 +78,7 @@ func (a *NotConfiguredVCSClient) GetCloneURL(_ logging.SimpleLogging, _ models.V func (a *NotConfiguredVCSClient) GetPullLabels(_ logging.SimpleLogging, _ models.Repo, _ models.PullRequest) ([]string, error) { return nil, a.err() } + +func (a *NotConfiguredVCSClient) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, _ string) ([]string, error) { + return nil, nil +} diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index dd3bd04667..f559b4ebe7 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -117,14 +117,6 @@ func (d *ClientProxy) GetPullLabels(logger logging.SimpleLogging, repo models.Re return d.clients[repo.VCSHost.Type].GetPullLabels(logger, repo, pull) } -// GetChildTeams delegates to the underlying VCS client if it supports fetching child teams -// (e.g. GitHub). Returns nil, nil for VCS providers that don't support team hierarchies. func (d *ClientProxy) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { - type childTeamFetcher interface { - GetChildTeams(logging.SimpleLogging, models.Repo, string) ([]string, error) - } - if fetcher, ok := d.clients[repo.VCSHost.Type].(childTeamFetcher); ok { - return fetcher.GetChildTeams(logger, repo, teamSlug) - } - return nil, nil + return d.clients[repo.VCSHost.Type].GetChildTeams(logger, repo, teamSlug) } From 699401d8759f3068738c88f1673411ceb17e9086 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 11:22:29 +0300 Subject: [PATCH 06/13] fix(dockerfile): bump openssh-server to 1:9.2p1-2+deb12u9 The debian-security repo updated openssh to u9 which requires a matching openssh-client version, causing a dependency conflict with the previously pinned u7 version. Signed-off-by: hussein-mimi Co-Authored-By: Claude Sonnet 4.6 --- Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 4b3fb5c57b..a0a072f821 100644 --- a/Dockerfile +++ b/Dockerfile @@ -63,7 +63,7 @@ ENV DEBIAN_GIT_VERSION="1:2.39.5-0+deb12u2" # renovate: datasource=repology depName=debian_12/unzip versioning=loose ENV DEBIAN_UNZIP_VERSION="6.0-28" # renovate: datasource=repology depName=debian_12/openssh-server versioning=loose -ENV DEBIAN_OPENSSH_SERVER_VERSION="1:9.2p1-2+deb12u7" +ENV DEBIAN_OPENSSH_SERVER_VERSION="1:9.2p1-2+deb12u9" # renovate: datasource=repology depName=debian_12/dumb-init versioning=loose ENV DEBIAN_DUMB_INIT_VERSION="1.2.5-2" # renovate: datasource=repology depName=debian_12/gnupg versioning=loose From 75cae5fb30a8ca538da0e476ca7abcb56dedba26 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 11:41:12 +0300 Subject: [PATCH 07/13] fix(allowlist): address Copilot review feedback - fetchDescendantTeams: replace recursion with iterative BFS + visited set to prevent duplicate API calls and handle cycles deterministically - checkUserPermissions: remove now-redundant childTeamFetcher type assertion (GetChildTeams is on the Client interface); use a descendant set for O(1) user team membership lookups instead of O(n*m) nested loop - GetChildTeams: use context.WithTimeout(30s) instead of context.Background() - TestClient_GetChildTeams: add defer testServer.Close() to avoid resource leak - instrumented_client.go: remove spurious blank line Signed-off-by: hussein-mimi Co-Authored-By: Claude Sonnet 4.6 --- server/events/command_runner.go | 82 +++++++++++-------- server/events/vcs/github/client.go | 3 +- server/events/vcs/github/client_test.go | 1 + .../events/vcs/github/instrumented_client.go | 1 - 4 files changed, 51 insertions(+), 36 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 65e9d96430..1f423905dd 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -263,32 +263,50 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models } } -// childTeamFetcher is implemented by VCS clients that support GitHub-style team hierarchies. -// GetChildTeams returns the direct child team slugs for a given team slug. -type childTeamFetcher interface { - GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) -} - -// fetchDescendantTeams recursively fetches all descendant team slugs for the given team, -// up to maxDepth levels deep. This prevents infinite loops in circular-hierarchy edge cases. -func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { +// fetchDescendantTeams fetches all descendant team slugs for the given team up to maxDepth +// levels deep using an iterative BFS with a visited set to avoid duplicate API calls and +// handle any cycles in unexpected hierarchy configurations. +func fetchDescendantTeams(fetcher vcs.Client, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { if maxDepth <= 0 { return nil, nil } - children, err := fetcher.GetChildTeams(logger, repo, teamSlug) - if err != nil { - return nil, err + + type queueItem struct { + slug string + depth int } - result := make([]string, len(children)) - copy(result, children) - for _, child := range children { - grandchildren, err := fetchDescendantTeams(fetcher, logger, repo, child, maxDepth-1) + + visited := map[string]struct{}{teamSlug: {}} + queue := []queueItem{{slug: teamSlug, depth: 0}} + var result []string + + for len(queue) > 0 { + current := queue[0] + queue = queue[1:] + + if current.depth >= maxDepth { + continue + } + + children, err := fetcher.GetChildTeams(logger, repo, current.slug) if err != nil { - logger.Warn("Could not fetch child teams for '%s': %s", child, err) + if current.slug == teamSlug { + return nil, err + } + logger.Warn("Could not fetch child teams for '%s': %s", current.slug, err) continue } - result = append(result, grandchildren...) + + for _, child := range children { + if _, ok := visited[child]; ok { + continue + } + visited[child] = struct{}{} + result = append(result, child) + queue = append(queue, queueItem{slug: child, depth: current.depth + 1}) + } } + return result, nil } @@ -319,13 +337,7 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode return true, nil } - // Slow path: check if the user belongs to a child team of any allowlisted team. - // Only available when the VCS client supports fetching child teams (e.g. GitHub). - fetcher, ok := c.VCSClient.(childTeamFetcher) - if !ok { - return false, nil - } - + // Slow path: check if the user belongs to a descendant team of any allowlisted team. const maxHierarchyDepth = 20 for _, allowedTeam := range c.TeamAllowlistChecker.AllTeams() { if allowedTeam == "*" { @@ -335,20 +347,22 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode if !c.TeamAllowlistChecker.IsCommandAllowedForTeam(ctx, allowedTeam, cmdName) { continue } - descendants, err := fetchDescendantTeams(fetcher, c.Logger, repo, allowedTeam, maxHierarchyDepth) + descendants, err := fetchDescendantTeams(c.VCSClient, c.Logger, repo, allowedTeam, maxHierarchyDepth) if err != nil { c.Logger.Warn("Could not fetch child teams for '%s': %s", allowedTeam, err) continue } + descendantSet := make(map[string]struct{}, len(descendants)) + for _, desc := range descendants { + descendantSet[strings.ToLower(desc)] = struct{}{} + } for _, userTeam := range user.Teams { - for _, desc := range descendants { - if strings.EqualFold(userTeam, desc) { - // Add the matched allowlisted parent team to user.Teams so that - // per-project allowlist filters (which check direct membership) - // also pass for this user. - user.Teams = append(user.Teams, allowedTeam) - return true, nil - } + if _, ok := descendantSet[strings.ToLower(userTeam)]; ok { + // Add the matched allowlisted parent team to user.Teams so that + // per-project allowlist filters (which check direct membership) + // also pass for this user. + user.Teams = append(user.Teams, allowedTeam) + return true, nil } } } diff --git a/server/events/vcs/github/client.go b/server/events/vcs/github/client.go index 0ce41f8541..64487e98e6 100644 --- a/server/events/vcs/github/client.go +++ b/server/events/vcs/github/client.go @@ -1125,7 +1125,8 @@ func (g *Client) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, t } `graphql:"organization(login: $orgName)"` } var childSlugs []string - ctx := context.Background() + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() for { err := g.v4Client.Query(ctx, &q, variables) if err != nil { diff --git a/server/events/vcs/github/client_test.go b/server/events/vcs/github/client_test.go index 15d60ce1bb..394957d1e9 100644 --- a/server/events/vcs/github/client_test.go +++ b/server/events/vcs/github/client_test.go @@ -1622,6 +1622,7 @@ func TestClient_GetChildTeams(t *testing.T) { return } })) + defer testServer.Close() testServerURL, err := url.Parse(testServer.URL) Ok(t, err) client, err := github.New(testServerURL.Host, &github.UserCredentials{"user", "pass", ""}, github.Config{}, 0, logger) diff --git a/server/events/vcs/github/instrumented_client.go b/server/events/vcs/github/instrumented_client.go index 94c28c1051..a09d371273 100644 --- a/server/events/vcs/github/instrumented_client.go +++ b/server/events/vcs/github/instrumented_client.go @@ -50,7 +50,6 @@ type InstrumentedGithubClient struct { Logger logging.SimpleLogging } - func (c *InstrumentedGithubClient) GetPullRequest(logger logging.SimpleLogging, repo models.Repo, pullNum int) (*github.PullRequest, error) { scope := c.StatsScope.SubScope("get_pull_request") scope = common.SetGitScopeTags(scope, repo.FullName, pullNum) From 7d45c40d5f50d830cadb08c6aeb1a5ebd0c4f3da Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 11:59:15 +0300 Subject: [PATCH 08/13] fix(allowlist): address second round of Copilot review feedback - Restore childTeamFetcher as a narrow interface so fetchDescendantTeams accepts it instead of the full vcs.Client; this fixes test compilation (mockChildTeamFetcher only implements GetChildTeams) - Switch BFS queue to index-based iteration to avoid front-slice churn - Update stale test comments: NotConfiguredVCSClient now has GetChildTeams (returns nil) rather than not implementing childTeamFetcher at all Signed-off-by: hussein-mimi Co-Authored-By: Claude Sonnet 4.6 --- server/events/command_runner.go | 19 ++++++++++++++----- server/events/command_runner_internal_test.go | 8 ++++---- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 1f423905dd..2af15d3da0 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -263,10 +263,15 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models } } +// childTeamFetcher is satisfied by VCS clients that support GitHub-style team hierarchies. +type childTeamFetcher interface { + GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) +} + // fetchDescendantTeams fetches all descendant team slugs for the given team up to maxDepth // levels deep using an iterative BFS with a visited set to avoid duplicate API calls and // handle any cycles in unexpected hierarchy configurations. -func fetchDescendantTeams(fetcher vcs.Client, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { +func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { if maxDepth <= 0 { return nil, nil } @@ -280,9 +285,8 @@ func fetchDescendantTeams(fetcher vcs.Client, logger logging.SimpleLogging, repo queue := []queueItem{{slug: teamSlug, depth: 0}} var result []string - for len(queue) > 0 { - current := queue[0] - queue = queue[1:] + for i := 0; i < len(queue); i++ { + current := queue[i] if current.depth >= maxDepth { continue @@ -338,6 +342,11 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode } // Slow path: check if the user belongs to a descendant team of any allowlisted team. + fetcher, ok := c.VCSClient.(childTeamFetcher) + if !ok { + return false, nil + } + const maxHierarchyDepth = 20 for _, allowedTeam := range c.TeamAllowlistChecker.AllTeams() { if allowedTeam == "*" { @@ -347,7 +356,7 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode if !c.TeamAllowlistChecker.IsCommandAllowedForTeam(ctx, allowedTeam, cmdName) { continue } - descendants, err := fetchDescendantTeams(c.VCSClient, c.Logger, repo, allowedTeam, maxHierarchyDepth) + descendants, err := fetchDescendantTeams(fetcher, c.Logger, repo, allowedTeam, maxHierarchyDepth) if err != nil { c.Logger.Warn("Could not fetch child teams for '%s': %s", allowedTeam, err) continue diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index d6e3200eb9..a1a9b79cc5 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -30,8 +30,8 @@ func (m *mockChildTeamFetcher) GetChildTeams(_ logging.SimpleLogging, _ models.R } // childTeamVCSClient combines vcs.NotConfiguredVCSClient (satisfying vcs.Client) -// with a mockChildTeamFetcher (satisfying childTeamFetcher), allowing the slow -// path in checkUserPermissions to be exercised without a real VCS connection. +// with a mockChildTeamFetcher, overriding GetChildTeams so the slow path in +// checkUserPermissions can be exercised without a real VCS connection. type childTeamVCSClient struct { vcs.NotConfiguredVCSClient mockChildTeamFetcher @@ -137,7 +137,7 @@ func TestCheckUserPermissions(t *testing.T) { cr := &DefaultCommandRunner{ Logger: logger, TeamAllowlistChecker: checker, - VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher + VCSClient: &vcs.NotConfiguredVCSClient{}, // GetChildTeams returns nil (no hierarchy support) } user := models.User{Username: "alice", Teams: []string{"dev-team"}} ok, err := cr.checkUserPermissions(repo, &user, "plan") @@ -150,7 +150,7 @@ func TestCheckUserPermissions(t *testing.T) { cr := &DefaultCommandRunner{ Logger: logger, TeamAllowlistChecker: checker, - VCSClient: &vcs.NotConfiguredVCSClient{}, // does not implement childTeamFetcher + VCSClient: &vcs.NotConfiguredVCSClient{}, // GetChildTeams returns nil (no hierarchy support) } user := models.User{Username: "alice", Teams: []string{"other-team"}} ok, err := cr.checkUserPermissions(repo, &user, "plan") From 0479bb86863591475b0158be9e5a34f2c983300d Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 12:10:26 +0300 Subject: [PATCH 09/13] test(allowlist): assert exact elements in error-skip traversal test Replace len-only assertion with exact slice comparison so the test catches wrong teams or duplicates, not just wrong counts. Signed-off-by: hussein-mimi Co-Authored-By: Claude Sonnet 4.6 --- server/events/command_runner_internal_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index a1a9b79cc5..5160b15dd6 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -111,9 +111,9 @@ func TestFetchDescendantTeams(t *testing.T) { } result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 20) Ok(t, err) - // child-a is present (direct child), its subtree is skipped on error. - // child-b and grandchild-b are also present. - Equals(t, 3, len(result)) + // child-a is included (direct child of parent); its subtree is skipped on error. + // child-b and grandchild-b are also traversed successfully. + Equals(t, []string{"child-a", "child-b", "grandchild-b"}, result) }) } From e618f8ef5cf84f78317dacc8087c0b0d93e34e4a Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 12:29:31 +0300 Subject: [PATCH 10/13] fix: remove redundant childTeamFetcher type assertion in checkUserPermissions Since GetChildTeams is now on the vcs.Client interface (per reviewer request), the type assertion c.VCSClient.(childTeamFetcher) always succeeds, causing the slow path to execute for all VCS providers even when they just return nil. Remove the assertion and call c.VCSClient directly; non-GitHub providers return nil from GetChildTeams so fetchDescendantTeams produces empty results and the loop is a no-op. Keep childTeamFetcher as a narrow interface for fetchDescendantTeams so tests can pass a mock without implementing all of vcs.Client. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner.go | 20 +++++++++---------- server/events/command_runner_internal_test.go | 6 +++--- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 2af15d3da0..12ba57d831 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -263,7 +263,10 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models } } -// childTeamFetcher is satisfied by VCS clients that support GitHub-style team hierarchies. +// childTeamFetcher is a narrow interface used by fetchDescendantTeams so that +// callers (and tests) only need to provide GetChildTeams rather than the full +// vcs.Client. In production the vcs.Client is passed directly since it +// includes GetChildTeams; non-GitHub providers return nil, nil. type childTeamFetcher interface { GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) } @@ -315,9 +318,11 @@ func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging } // checkUserPermissions checks if the user has permissions to execute the command. -// It first checks direct team membership against the allowlist. If that fails and the -// VCS client supports team hierarchy (childTeamFetcher), it expands each allowlisted team -// to include all its descendant teams (up to 20 levels deep) and re-checks. +// It first checks direct team membership against the allowlist. If that fails, +// it expands each allowlisted team to include all its descendant teams (up to +// 20 levels deep) via GetChildTeams on the VCS client and re-checks. +// Non-GitHub VCS providers return nil from GetChildTeams, so the expansion +// loop is effectively a no-op for them. // When a match is found via hierarchy, the matched allowlisted parent team is appended to // user.Teams so that subsequent per-project allowlist checks (which use direct membership // only) also pass. @@ -342,11 +347,6 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode } // Slow path: check if the user belongs to a descendant team of any allowlisted team. - fetcher, ok := c.VCSClient.(childTeamFetcher) - if !ok { - return false, nil - } - const maxHierarchyDepth = 20 for _, allowedTeam := range c.TeamAllowlistChecker.AllTeams() { if allowedTeam == "*" { @@ -356,7 +356,7 @@ func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *mode if !c.TeamAllowlistChecker.IsCommandAllowedForTeam(ctx, allowedTeam, cmdName) { continue } - descendants, err := fetchDescendantTeams(fetcher, c.Logger, repo, allowedTeam, maxHierarchyDepth) + descendants, err := fetchDescendantTeams(c.VCSClient, c.Logger, repo, allowedTeam, maxHierarchyDepth) if err != nil { c.Logger.Warn("Could not fetch child teams for '%s': %s", allowedTeam, err) continue diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 5160b15dd6..27955b2c55 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -14,9 +14,9 @@ import ( . "github.com/runatlantis/atlantis/testing" ) -// mockChildTeamFetcher is a test double for childTeamFetcher. -// It maps team slug -> list of direct child slugs, and returns an error for -// any slug present in errOn. +// mockChildTeamFetcher is a test double for the childTeamFetcher interface +// used by fetchDescendantTeams. It maps team slug -> list of direct child +// slugs, and returns an error for any slug present in errOn. type mockChildTeamFetcher struct { children map[string][]string errOn map[string]bool From 3d53209f57c0c7a4f7cce5834a0680c655617018 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 12:47:36 +0300 Subject: [PATCH 11/13] test: add multi-page pagination test for GetChildTeams Add a 'multiple pages' subtest to TestClient_GetChildTeams that verifies cursor-based pagination works correctly: the first GraphQL request has no cursor, the second includes the endCursor from the first page, and results from both pages are accumulated. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/vcs/github/client_test.go | 167 ++++++++++++++++++------ 1 file changed, 126 insertions(+), 41 deletions(-) diff --git a/server/events/vcs/github/client_test.go b/server/events/vcs/github/client_test.go index 394957d1e9..7b14049173 100644 --- a/server/events/vcs/github/client_test.go +++ b/server/events/vcs/github/client_test.go @@ -1591,51 +1591,136 @@ func TestClient_GetTeamNamesForUser(t *testing.T) { // TestClient_GetChildTeams verifies that direct child teams of a given team are returned. func TestClient_GetChildTeams(t *testing.T) { - logger := logging.NewNoopLogger(t) - // Mocked GraphQL response: "parent-team" has two direct children. - resp := `{ - "data":{ - "organization": { - "team": { - "childTeams": { - "nodes": [ - {"slug": "child-team-a"}, - {"slug": "child-team-b"} - ], - "pageInfo": { - "endCursor": "Y3Vyc29yOnYyOpHOAFMoLQ==", - "hasNextPage": false + t.Run("single page", func(t *testing.T) { + logger := logging.NewNoopLogger(t) + resp := `{ + "data":{ + "organization": { + "team": { + "childTeams": { + "nodes": [ + {"slug": "child-team-a"}, + {"slug": "child-team-b"} + ], + "pageInfo": { + "endCursor": "Y3Vyc29yOnYyOpHOAFMoLQ==", + "hasNextPage": false + } } } } - } - } - }` - testServer := httptest.NewTLSServer( - http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - switch r.RequestURI { - case "/api/graphql": - w.Write([]byte(resp)) // nolint: errcheck - default: - t.Errorf("got unexpected request at %q", r.RequestURI) - http.Error(w, "not found", http.StatusNotFound) - return + } + }` + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/graphql": + w.Write([]byte(resp)) // nolint: errcheck + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + defer testServer.Close() + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + client, err := github.New(testServerURL.Host, &github.UserCredentials{"user", "pass", ""}, github.Config{}, 0, logger) + Ok(t, err) + defer disableSSLVerification()() + + children, err := client.GetChildTeams( + logger, + models.Repo{Owner: "testrepo"}, + "parent-team", + ) + Ok(t, err) + Equals(t, []string{"child-team-a", "child-team-b"}, children) + }) + + t.Run("multiple pages", func(t *testing.T) { + logger := logging.NewNoopLogger(t) + firstCursor := "cursor-page-1" + firstResp := `{ + "data":{ + "organization": { + "team": { + "childTeams": { + "nodes": [ + {"slug": "child-team-a"}, + {"slug": "child-team-b"} + ], + "pageInfo": { + "endCursor": "cursor-page-1", + "hasNextPage": true + } + } + } } - })) - defer testServer.Close() - testServerURL, err := url.Parse(testServer.URL) - Ok(t, err) - client, err := github.New(testServerURL.Host, &github.UserCredentials{"user", "pass", ""}, github.Config{}, 0, logger) - Ok(t, err) - defer disableSSLVerification()() - - children, err := client.GetChildTeams( - logger, - models.Repo{Owner: "testrepo"}, - "parent-team", - ) - Ok(t, err) - Equals(t, []string{"child-team-a", "child-team-b"}, children) + } + }` + secondResp := `{ + "data":{ + "organization": { + "team": { + "childTeams": { + "nodes": [ + {"slug": "child-team-c"} + ], + "pageInfo": { + "endCursor": "cursor-page-2", + "hasNextPage": false + } + } + } + } + } + }` + + requestBodies := make([]string, 0, 2) + testServer := httptest.NewTLSServer( + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.RequestURI { + case "/api/graphql": + body, err := io.ReadAll(r.Body) + if err != nil { + t.Errorf("reading request body: %s", err) + http.Error(w, "bad request", http.StatusBadRequest) + return + } + requestBodies = append(requestBodies, string(body)) + if len(requestBodies) == 1 { + w.Write([]byte(firstResp)) // nolint: errcheck + } else { + w.Write([]byte(secondResp)) // nolint: errcheck + } + default: + t.Errorf("got unexpected request at %q", r.RequestURI) + http.Error(w, "not found", http.StatusNotFound) + return + } + })) + defer testServer.Close() + testServerURL, err := url.Parse(testServer.URL) + Ok(t, err) + client, err := github.New(testServerURL.Host, &github.UserCredentials{"user", "pass", ""}, github.Config{}, 0, logger) + Ok(t, err) + defer disableSSLVerification()() + + children, err := client.GetChildTeams( + logger, + models.Repo{Owner: "testrepo"}, + "parent-team", + ) + Ok(t, err) + Equals(t, []string{"child-team-a", "child-team-b", "child-team-c"}, children) + Equals(t, 2, len(requestBodies)) + // First request should not contain the cursor; second should. + Assert(t, !strings.Contains(requestBodies[0], firstCursor), + "expected first request not to include cursor") + Assert(t, strings.Contains(requestBodies[1], firstCursor), + "expected second request to include cursor") + }) } func TestClient_DiscardReviews(t *testing.T) { From 4133a20ed4c6600afbb283d6f73cbcea6413d661 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Sun, 12 Apr 2026 13:00:44 +0300 Subject: [PATCH 12/13] test: add cycle detection test for fetchDescendantTeams Verify that a cyclic hierarchy (a -> b -> a) terminates and produces deduplicated results thanks to the visited set. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner_internal_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index 27955b2c55..f29990ec61 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -115,6 +115,17 @@ func TestFetchDescendantTeams(t *testing.T) { // child-b and grandchild-b are also traversed successfully. Equals(t, []string{"child-a", "child-b", "grandchild-b"}, result) }) + + t.Run("cycle is handled without infinite loop", func(t *testing.T) { + // a -> b -> a forms a cycle; visited set should break it. + fetcher := &mockChildTeamFetcher{children: map[string][]string{ + "team-a": {"team-b"}, + "team-b": {"team-a"}, + }} + result, err := fetchDescendantTeams(fetcher, logger, repo, "team-a", 20) + Ok(t, err) + Equals(t, []string{"team-b"}, result) + }) } func TestCheckUserPermissions(t *testing.T) { From 5b6cd444162f0ff0cf47581f4185a9bc5151ff01 Mon Sep 17 00:00:00 2001 From: hussein-mimi Date: Mon, 27 Apr 2026 16:53:15 +0300 Subject: [PATCH 13/13] refactor: remove redundant childTeamFetcher interface MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GetChildTeams is already on vcs.Client, so childTeamFetcher was a strict subset of it — the type assertion always succeeded and the interface served no selection purpose. fetchDescendantTeams now takes vcs.Client directly, and the separate mockChildTeamFetcher test double is merged into childTeamVCSClient so there is one test helper instead of two. Co-Authored-By: Claude Sonnet 4.6 Signed-off-by: hussein-mimi --- server/events/command_runner.go | 10 +--- server/events/command_runner_internal_test.go | 51 +++++++------------ 2 files changed, 19 insertions(+), 42 deletions(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 50f9399241..41c5fa4973 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -263,18 +263,10 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models } } -// childTeamFetcher is a narrow interface used by fetchDescendantTeams so that -// callers (and tests) only need to provide GetChildTeams rather than the full -// vcs.Client. In production the vcs.Client is passed directly since it -// includes GetChildTeams; non-GitHub providers return nil, nil. -type childTeamFetcher interface { - GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) -} - // fetchDescendantTeams fetches all descendant team slugs for the given team up to maxDepth // levels deep using an iterative BFS with a visited set to avoid duplicate API calls and // handle any cycles in unexpected hierarchy configurations. -func fetchDescendantTeams(fetcher childTeamFetcher, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { +func fetchDescendantTeams(fetcher vcs.Client, logger logging.SimpleLogging, repo models.Repo, teamSlug string, maxDepth int) ([]string, error) { if maxDepth <= 0 { return nil, nil } diff --git a/server/events/command_runner_internal_test.go b/server/events/command_runner_internal_test.go index f29990ec61..4c9ef7a7a6 100644 --- a/server/events/command_runner_internal_test.go +++ b/server/events/command_runner_internal_test.go @@ -14,31 +14,20 @@ import ( . "github.com/runatlantis/atlantis/testing" ) -// mockChildTeamFetcher is a test double for the childTeamFetcher interface -// used by fetchDescendantTeams. It maps team slug -> list of direct child -// slugs, and returns an error for any slug present in errOn. -type mockChildTeamFetcher struct { +// childTeamVCSClient combines vcs.NotConfiguredVCSClient (satisfying vcs.Client) +// with a configurable team hierarchy map, so tests can exercise GetChildTeams +// without a real VCS connection. +type childTeamVCSClient struct { + vcs.NotConfiguredVCSClient children map[string][]string errOn map[string]bool } -func (m *mockChildTeamFetcher) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, teamSlug string) ([]string, error) { - if m.errOn[teamSlug] { +func (c *childTeamVCSClient) GetChildTeams(_ logging.SimpleLogging, _ models.Repo, teamSlug string) ([]string, error) { + if c.errOn[teamSlug] { return nil, errors.New("API error for " + teamSlug) } - return m.children[teamSlug], nil -} - -// childTeamVCSClient combines vcs.NotConfiguredVCSClient (satisfying vcs.Client) -// with a mockChildTeamFetcher, overriding GetChildTeams so the slow path in -// checkUserPermissions can be exercised without a real VCS connection. -type childTeamVCSClient struct { - vcs.NotConfiguredVCSClient - mockChildTeamFetcher -} - -func (c *childTeamVCSClient) GetChildTeams(logger logging.SimpleLogging, repo models.Repo, teamSlug string) ([]string, error) { - return c.mockChildTeamFetcher.GetChildTeams(logger, repo, teamSlug) + return c.children[teamSlug], nil } func TestFetchDescendantTeams(t *testing.T) { @@ -46,14 +35,14 @@ func TestFetchDescendantTeams(t *testing.T) { repo := models.Repo{Owner: "test-org"} t.Run("leaf team returns empty", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{children: map[string][]string{}} + fetcher := &childTeamVCSClient{children: map[string][]string{}} result, err := fetchDescendantTeams(fetcher, logger, repo, "leaf-team", 20) Ok(t, err) Equals(t, 0, len(result)) }) t.Run("single level of children", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{children: map[string][]string{ + fetcher := &childTeamVCSClient{children: map[string][]string{ "parent": {"child-a", "child-b"}, }} result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 20) @@ -62,7 +51,7 @@ func TestFetchDescendantTeams(t *testing.T) { }) t.Run("multiple levels of nesting", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{children: map[string][]string{ + fetcher := &childTeamVCSClient{children: map[string][]string{ "grandparent": {"parent"}, "parent": {"child"}, }} @@ -72,7 +61,7 @@ func TestFetchDescendantTeams(t *testing.T) { }) t.Run("maxDepth=0 returns nothing", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{children: map[string][]string{ + fetcher := &childTeamVCSClient{children: map[string][]string{ "parent": {"child"}, }} result, err := fetchDescendantTeams(fetcher, logger, repo, "parent", 0) @@ -81,7 +70,7 @@ func TestFetchDescendantTeams(t *testing.T) { }) t.Run("maxDepth=1 returns only direct children", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{children: map[string][]string{ + fetcher := &childTeamVCSClient{children: map[string][]string{ "grandparent": {"parent"}, "parent": {"child"}, }} @@ -91,7 +80,7 @@ func TestFetchDescendantTeams(t *testing.T) { }) t.Run("error at root propagates", func(t *testing.T) { - fetcher := &mockChildTeamFetcher{ + fetcher := &childTeamVCSClient{ children: map[string][]string{}, errOn: map[string]bool{"parent": true}, } @@ -102,7 +91,7 @@ func TestFetchDescendantTeams(t *testing.T) { t.Run("error in recursive call is logged and skipped", func(t *testing.T) { // parent fetches OK; fetching child-a's children errors. // child-b and its subtree should still be traversed. - fetcher := &mockChildTeamFetcher{ + fetcher := &childTeamVCSClient{ children: map[string][]string{ "parent": {"child-a", "child-b"}, "child-b": {"grandchild-b"}, @@ -118,7 +107,7 @@ func TestFetchDescendantTeams(t *testing.T) { t.Run("cycle is handled without infinite loop", func(t *testing.T) { // a -> b -> a forms a cycle; visited set should break it. - fetcher := &mockChildTeamFetcher{children: map[string][]string{ + fetcher := &childTeamVCSClient{children: map[string][]string{ "team-a": {"team-b"}, "team-b": {"team-a"}, }} @@ -225,9 +214,7 @@ func TestCheckUserPermissions(t *testing.T) { cr := &DefaultCommandRunner{ Logger: logger, TeamAllowlistChecker: checker, - VCSClient: &childTeamVCSClient{ - mockChildTeamFetcher: mockChildTeamFetcher{children: tc.hierarchy}, - }, + VCSClient: &childTeamVCSClient{children: tc.hierarchy}, } user := models.User{Username: "testuser", Teams: tc.userTeams} ok, checkErr := cr.checkUserPermissions(repo, &user, tc.cmdName) @@ -243,9 +230,7 @@ func TestCheckUserPermissions(t *testing.T) { Logger: logger, TeamAllowlistChecker: checker, VCSClient: &childTeamVCSClient{ - mockChildTeamFetcher: mockChildTeamFetcher{ - children: map[string][]string{"parent-team": {"child-team"}}, - }, + children: map[string][]string{"parent-team": {"child-team"}}, }, } user := models.User{Username: "alice", Teams: []string{"child-team"}}