Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
222de7c
feat(github): support team hierarchy in GH_TEAM_ALLOWLIST
hussein-mimi Apr 5, 2026
f386f36
test(github): add tests for fetchDescendantTeams and checkUserPermiss…
hussein-mimi Apr 7, 2026
f32da28
fix(github): propagate GetChildTeams through ClientProxy and Instrume…
hussein-mimi Apr 9, 2026
f57331f
fix(allowlist): propagate matched parent team to per-project filter
hussein-mimi Apr 9, 2026
b1cd614
refactor(vcs): add GetChildTeams to Client interface with stubs for n…
hussein-mimi Apr 12, 2026
699401d
fix(dockerfile): bump openssh-server to 1:9.2p1-2+deb12u9
hussein-mimi Apr 12, 2026
3bf6370
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 12, 2026
75cae5f
fix(allowlist): address Copilot review feedback
hussein-mimi Apr 12, 2026
7d45c40
fix(allowlist): address second round of Copilot review feedback
hussein-mimi Apr 12, 2026
0479bb8
test(allowlist): assert exact elements in error-skip traversal test
hussein-mimi Apr 12, 2026
e618f8e
fix: remove redundant childTeamFetcher type assertion in checkUserPer…
hussein-mimi Apr 12, 2026
3d53209
test: add multi-page pagination test for GetChildTeams
hussein-mimi Apr 12, 2026
4133a20
test: add cycle detection test for fetchDescendantTeams
hussein-mimi Apr 12, 2026
d5d19bf
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 13, 2026
dfca196
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 16, 2026
6cfec46
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 19, 2026
46279b9
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 21, 2026
30a53ea
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 24, 2026
44bb779
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 27, 2026
5b6cd44
refactor: remove redundant childTeamFetcher interface
hussein-mimi Apr 27, 2026
f5efeaa
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 28, 2026
3ce40a6
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Apr 30, 2026
fa51f2d
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi May 3, 2026
f125cbb
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi May 18, 2026
92f3b90
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Jun 1, 2026
608c19e
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Jun 4, 2026
992e4e1
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Jun 8, 2026
79fe11b
Merge branch 'main' into feat/github-team-hierarchy-allowlist
hussein-mimi Jun 21, 2026
d0f622e
Merge branch 'main' into feat/github-team-hierarchy-allowlist
chenrui333 Jun 26, 2026
09c6dc9
test: gofmt command runner hierarchy test
chenrui333 Jun 26, 2026
3168239
fix: preserve hierarchy grants for policy checks
chenrui333 Jun 26, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
153 changes: 144 additions & 9 deletions server/events/command_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"slices"
"strconv"
"strings"

"github.com/drmaxgit/go-azuredevops/azuredevops"
"github.com/google/go-github/v88/github"
Expand Down Expand Up @@ -155,15 +156,17 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo
log.Err("Unable to fetch user teams: %s", err)
return
}
directUserTeams := append([]string(nil), user.Teams...)

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
}
if !ok {
return
}
c.addPolicyCheckHierarchyTeamsForPlan(baseRepo, &user, command.Plan, directUserTeams)
}

ctx := &command.Context{
Expand Down Expand Up @@ -253,8 +256,130 @@ func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models
}
}

// checkUserPermissions checks if the user has permissions to execute the command
func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user models.User, cmdName string) (bool, 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
}

type queueItem struct {
slug string
depth int
}

visited := map[string]struct{}{teamSlug: {}}
queue := []queueItem{{slug: teamSlug, depth: 0}}
var result []string

for i := 0; i < len(queue); i++ {
current := queue[i]

if current.depth >= maxDepth {
continue
}

children, err := fetcher.GetChildTeams(logger, repo, current.slug)
if err != nil {
if current.slug == teamSlug {
return nil, err
}
logger.Warn("Could not fetch child teams for '%s': %s", current.slug, err)
continue
Comment thread
hussein-mimi marked this conversation as resolved.
}

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
}

func teamSet(teams []string) map[string]struct{} {
result := make(map[string]struct{}, len(teams))
for _, team := range teams {
result[strings.ToLower(team)] = struct{}{}
}
return result
}

func (c *DefaultCommandRunner) addHierarchyTeamsForCommand(repo models.Repo, user *models.User, cmdName string) {
c.addHierarchyTeamsForCommandForTeams(repo, user, cmdName, user.Teams)
}

func (c *DefaultCommandRunner) addHierarchyTeamsForCommandForTeams(repo models.Repo, user *models.User, cmdName string, teams []string) {
if c.TeamAllowlistChecker == nil || !c.TeamAllowlistChecker.HasRules() {
return
}

ctx := models.TeamAllowlistCheckerContext{
BaseRepo: repo,
CommandName: cmdName,
Log: c.Logger,
Pull: models.PullRequest{},
User: *user,
Verbose: false,
API: false,
}

// Only direct user teams should authorize hierarchy grants. Parent teams inferred
// during this pass are appended for downstream direct-membership filters, not for
// chaining additional hierarchy grants.
directUserTeams := teamSet(teams)
currentUserTeams := teamSet(user.Teams)

const maxHierarchyDepth = 20
for _, allowedTeam := range c.TeamAllowlistChecker.AllTeams() {
if allowedTeam == "*" {
continue
}
normalizedAllowedTeam := strings.ToLower(allowedTeam)
if _, ok := currentUserTeams[normalizedAllowedTeam]; ok {
continue
}
if !c.TeamAllowlistChecker.IsCommandAllowedForTeam(ctx, allowedTeam, cmdName) {
continue
}
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
}
for _, descendant := range descendants {
if _, ok := directUserTeams[strings.ToLower(descendant)]; !ok {
continue
}
user.Teams = append(user.Teams, allowedTeam)
currentUserTeams[normalizedAllowedTeam] = struct{}{}
break
}
}
}

func (c *DefaultCommandRunner) addPolicyCheckHierarchyTeamsForPlan(repo models.Repo, user *models.User, cmdName command.Name, directUserTeams []string) {
if cmdName != command.Plan {
return
}
c.addHierarchyTeamsForCommandForTeams(repo, user, command.PolicyCheck.String(), directUserTeams)
}

// checkUserPermissions checks if the user has permissions to execute the command.
// 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.
func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user *models.User, cmdName string) (bool, error) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was the signature changed to make user a pointer type? It seems like all callers of this function are taking a pointer at the call site, and the user is immediately dereferenced below, what is that gaining?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually all changes done by claude, i just want to solve this bug, can we get this PR merged, so we start using it in production ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if c.TeamAllowlistChecker == nil || !c.TeamAllowlistChecker.HasRules() {
// allowlist restriction is not enabled
return true, nil
Expand All @@ -264,15 +389,23 @@ 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,
}
ok := c.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(ctx, user.Teams, cmdName)
if !ok {
return false, nil

// 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 descendant team of any allowlisted team.
c.addHierarchyTeamsForCommand(repo, user, cmdName)
ctx.User = *user
if c.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(ctx, user.Teams, cmdName) {
return true, nil
}
Comment thread
hussein-mimi marked this conversation as resolved.
return true, nil
return false, nil
}

// checkVarFilesInPlanCommandAllowlisted checks if paths in a 'plan' command are allowlisted.
Expand Down Expand Up @@ -316,8 +449,9 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
c.Logger.Err("Unable to fetch user teams: %s", err)
return
}
directUserTeams := append([]string(nil), user.Teams...)

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
Expand All @@ -326,6 +460,7 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead
c.commentUserDoesNotHavePermissions(baseRepo, pullNum, user, cmd)
return
}
c.addPolicyCheckHierarchyTeamsForPlan(baseRepo, &user, cmd.Name, directUserTeams)
}

// Check if the provided var files in a 'plan' command are allowlisted
Expand Down
Loading
Loading