feat(github): support team hierarchy in GH_TEAM_ALLOWLIST#6365
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds support for GitHub team hierarchy in the Atlantis allowlist. When a parent team is added to ATLANTIS_GH_TEAM_ALLOWLIST, users who are members of descendant (child/grandchild/etc.) teams are now correctly authorized. The implementation uses a duck-typed interface to support team hierarchies for VCS clients that support them (like GitHub), while remaining compatible with VCS clients that don't support this feature.
Changes:
- Added
GetChildTeams()method to the GitHub client that queries the GraphQL API for direct child teams with pagination support - Added
childTeamFetcherinterface for VCS clients supporting team hierarchies - Added
fetchDescendantTeams()recursive function with depth limiting to expand teams to all descendants - Updated
checkUserPermissions()with a two-path approach: fast path for direct membership, slow path for hierarchical expansion - Added test for
GetChildTeams()method
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| server/events/vcs/github/client.go | Added GetChildTeams() method to query GitHub's GraphQL API for direct child teams with pagination |
| server/events/vcs/github/client_test.go | Added TestClient_GetChildTeams() to verify the method correctly parses GraphQL responses |
| server/events/command_runner.go | Added childTeamFetcher interface, fetchDescendantTeams() function, and updated checkUserPermissions() to support team hierarchy expansion with a two-path authorization check |
73d098f to
bb0b23d
Compare
bb0b23d to
b92451e
Compare
6d9cb59 to
6dab523
Compare
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 runatlantis#6107 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: hussein-mimi <hussein.mimi@harri.com>
…ions 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 <noreply@anthropic.com> Signed-off-by: hussein-mimi <hussein.mimi@harri.com>
…ntedGithubClient 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 <noreply@anthropic.com> Signed-off-by: hussein-mimi <hussein.mimi@harri.com>
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 <noreply@anthropic.com> Signed-off-by: hussein-mimi <hussein.mimi@harri.com>
77c3ffd to
f57331f
Compare
…on-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 <hussein.mimi@harri.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 <hussein.mimi@harri.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
@lukemassa can you take a look ? |
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 <noreply@anthropic.com> Signed-off-by: hussein-mimi <hussein.mimi@harri.com>
lukemassa
left a comment
There was a problem hiding this comment.
I left a small comment, but otherwise this looks good to me!
| // 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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ?
|
@GenPage @jamengual @chenrui333 |
|
checking |
Signed-off-by: Rui Chen <rui@chenrui.dev>
Signed-off-by: Rui Chen <rui@chenrui.dev>
Summary
Closes #6107
When a parent team is added to
ATLANTIS_GH_TEAM_ALLOWLIST, users who are members of any descendant (child/grandchild/etc.) team are now correctly authorized, instead of being rejected.Before: User in
child-team→ allowlist hasparent-team→ ❌ rejectedAfter: User in
child-team→ allowlist hasparent-team→ ✅ authorizedHow it works
GetChildTeams(logger, repo, teamSlug)to the GitHub client — queries the GitHub GraphQL API for direct child teams of a given team slug (with pagination).checkUserPermissions, after the fast path (direct team membership, zero extra API calls) fails, a slow path kicks in:GetChildTeams(up to 20 levels deep to prevent infinite loops).childTeamFetcherinterface, so no changes to the sharedClientinterface or any other VCS provider.Test plan
TestClient_GetTeamNamesForUser— existing test, still passes unchangedTestClient_GetChildTeams— new test verifyingGetChildTeamsreturns direct children from a mocked GraphQL responsego build ./server/events/...— clean buildATLANTIS_GH_TEAM_ALLOWLIST, comment as a user who is only in a child team, verify they are now authorized🤖 Generated with Claude Code