From dbbf50703cc3af03c7c6beee6526d2157509eca1 Mon Sep 17 00:00:00 2001 From: "PePe (Jose) Amengual" Date: Fri, 2 Jul 2021 17:49:39 -0500 Subject: [PATCH 1/5] Add GitHub team allowlist configuration option Co-authored-by: PePe (Jose) Amengual Co-authored-by: Troy Neeriemer Co-authored-by: Ted Roby Co-authored-by: Paul Erickson --- cmd/server.go | 17 +++++ runatlantis.io/docs/server-configuration.md | 6 ++ .../controllers/events/events_controller.go | 37 ++++++++++ server/events/team_allowlist_checker.go | 72 +++++++++++++++++++ server/events/team_allowlist_checker_test.go | 35 +++++++++ server/events/vcs/azuredevops_client.go | 5 ++ server/events/vcs/bitbucketcloud/client.go | 5 ++ server/events/vcs/bitbucketserver/client.go | 5 ++ server/events/vcs/client.go | 1 + server/events/vcs/github_client.go | 27 +++++++ server/events/vcs/gitlab_client.go | 5 ++ server/events/vcs/mocks/mock_client.go | 30 ++++++++ .../events/vcs/not_configured_vcs_client.go | 3 + server/events/vcs/proxy.go | 4 ++ server/server.go | 5 ++ server/user_config.go | 1 + 16 files changed, 258 insertions(+) create mode 100644 server/events/team_allowlist_checker.go create mode 100644 server/events/team_allowlist_checker_test.go diff --git a/cmd/server.go b/cmd/server.go index 882ac23ab5..40cb82613f 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -64,6 +64,7 @@ const ( EnableRegExpCmdFlag = "enable-regexp-cmd" EnableDiffMarkdownFormat = "enable-diff-markdown-format" GHHostnameFlag = "gh-hostname" + GHTeamAllowlistFlag = "gh-team-allowlist" GHTokenFlag = "gh-token" GHUserFlag = "gh-user" GHAppIDFlag = "gh-app-id" @@ -116,6 +117,7 @@ const ( DefaultBitbucketBaseURL = bitbucketcloud.BaseURL DefaultDataDir = "~/.atlantis" DefaultGHHostname = "github.com" + DefaultGHTeamAllowlist = "*:plan,*:apply" DefaultGitlabHostname = "gitlab.com" DefaultLogLevel = "info" DefaultParallelPoolSize = 15 @@ -199,6 +201,18 @@ var stringFlags = map[string]stringFlag{ description: "Hostname of your Github Enterprise installation. If using github.com, no need to set.", defaultValue: DefaultGHHostname, }, + GHTeamAllowlistFlag: { + description: "Comma separated list of key-value pairs representing the GitHub teams and the operations that " + + "the members of a particular team are allowed to perform. " + + "The format is {team}:{command},{team}:{command}. " + + "Valid values for 'command' are 'plan', 'apply' and '*', e.g. 'dev:plan,ops:apply,devops:*'" + + "This example gives the users from the 'dev' GitHub team the permissions to execute the 'plan' command, " + + "the 'ops' team the permissions to execute the 'apply' command, " + + "and allows the 'devops' team to perform any operation. If this argument is not provided, the default value (*:*) " + + "will be used and the default behavior will be to not check permissions " + + "and to allow users from any team to perform any operation.", + defaultValue: DefaultGHTeamAllowlist, + }, GHUserFlag: { description: "GitHub username of API user.", defaultValue: "", @@ -649,6 +663,9 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig) { if c.VCSStatusName == "" { c.VCSStatusName = DefaultVCSStatusName } + if c.GithubTeamAllowlist == "" { + c.GithubTeamAllowlist = DefaultGHTeamAllowlist + } if c.TFEHostname == "" { c.TFEHostname = DefaultTFEHostname } diff --git a/runatlantis.io/docs/server-configuration.md b/runatlantis.io/docs/server-configuration.md index 0035ddbd70..8fed8e8e01 100644 --- a/runatlantis.io/docs/server-configuration.md +++ b/runatlantis.io/docs/server-configuration.md @@ -359,6 +359,12 @@ Values are chosen in this order: ::: warning SECURITY WARNING The contents of the private key will be visible by anyone that can run `ps` or look at the shell history of the machine where Atlantis is running. Use `--gh-app-key-file` to mitigate that risk. ::: +- +- ### `--gh-team-allowlist` + ```bash + atlantis server --gh-team-allowlist="myteam:plan, secteam:apply" + ``` + Comma-separated list of GitHub team and permission pairs. By default, any team can plan and apply. * ### `--gitlab-hostname` ```bash diff --git a/server/controllers/events/events_controller.go b/server/controllers/events/events_controller.go index cd4d097aeb..b6bfd76402 100644 --- a/server/controllers/events/events_controller.go +++ b/server/controllers/events/events_controller.go @@ -62,6 +62,7 @@ type VCSEventsController struct { // request validation is done. GitlabWebhookSecret []byte RepoAllowlistChecker *events.RepoAllowlistChecker + TeamAllowlistChecker *events.TeamAllowlistChecker // SilenceAllowlistErrors controls whether we write an error comment on // pull requests from non-allowlisted repos. SilenceAllowlistErrors bool @@ -436,6 +437,20 @@ func (e *VCSEventsController) handleCommentEvent(w http.ResponseWriter, baseRepo return } + // Check if the user who commented has the permissions to execute the 'plan' or 'apply' commands + if !e.TestingMode { + ok, err := e.checkUserPermissions(baseRepo, user, parseResult.Command) + if err != nil { + e.Logger.Err("unable to comment on pull request: %s", err) + return + } + if !ok { + e.commentUserDoesNotHavePermissions(baseRepo, pullNum, user, parseResult.Command) + e.respond(w, logging.Warn, http.StatusForbidden, "User @%s does not have permissions to execute '%s' command", user.Username, parseResult.Command.Name.String()) + return + } + } + e.Logger.Debug("executing command") fmt.Fprintln(w, "Processing...") if !e.TestingMode { @@ -553,3 +568,25 @@ func (e *VCSEventsController) commentNotAllowlisted(baseRepo models.Repo, pullNu e.Logger.Err("unable to comment on pull request: %s", err) } } + +// commentUserDoesNotHavePermissions comments on the pull request that the user +// is not allowed to execute the command. +func (e *VCSEventsController) commentUserDoesNotHavePermissions(baseRepo models.Repo, pullNum int, user models.User, cmd *events.CommentCommand) { + errMsg := fmt.Sprintf("```\nError: User @%s does not have permissions to execute '%s' command.\n```", user.Username, cmd.Name) + if err := e.VCSClient.CreateComment(baseRepo, pullNum, errMsg, ""); err != nil { + e.Logger.Err("unable to comment on pull request: %s", err) + } +} + +// checkUserPermissions checks if the user has permissions to execute the command +func (e *VCSEventsController) checkUserPermissions(repo models.Repo, user models.User, cmd *events.CommentCommand) (bool, error) { + teams, err := e.VCSClient.GetTeamNamesForUser(repo, user) + if err != nil { + return false, err + } + ok := e.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(teams, cmd.Name.String()) + if !ok { + return false, nil + } + return true, nil +} diff --git a/server/events/team_allowlist_checker.go b/server/events/team_allowlist_checker.go new file mode 100644 index 0000000000..8995365edc --- /dev/null +++ b/server/events/team_allowlist_checker.go @@ -0,0 +1,72 @@ +package events + +import ( + "strings" +) + +// Wildcard matches all teams and all commands +const wildcard = "*" + +// mapOfStrings is an alias for map[string]string +type mapOfStrings map[string]string + +// TeamAllowlistChecker implements checking the teams and the operations that the members +// of a particular team are allowed to perform +type TeamAllowlistChecker struct { + rules []mapOfStrings +} + +// NewTeamAllowlistChecker constructs a new checker +func NewTeamAllowlistChecker(allowlist string) (*TeamAllowlistChecker, error) { + var rules []mapOfStrings + pairs := strings.Split(allowlist, ",") + if pairs[0] != "" { + for _, pair := range pairs { + values := strings.Split(pair, ":") + team := strings.TrimSpace(values[0]) + command := strings.TrimSpace(values[1]) + m := mapOfStrings{team: command} + rules = append(rules, m) + } + } + return &TeamAllowlistChecker{ + rules: rules, + }, nil +} + +// IsCommandAllowedForTeam returns true if the team is allowed to execute the command +// and false otherwise. +func (checker *TeamAllowlistChecker) IsCommandAllowedForTeam(team string, command string) bool { + t := strings.TrimSpace(team) + c := strings.TrimSpace(command) + for _, rule := range checker.rules { + for key, value := range rule { + if (key == wildcard || strings.EqualFold(key, t)) && (value == wildcard || strings.EqualFold(value, c)) { + return true + } + } + } + return false +} + +// IsCommandAllowedForAnyTeam returns true if any of the teams is allowed to execute the command +// and false otherwise. +func (checker *TeamAllowlistChecker) IsCommandAllowedForAnyTeam(teams []string, command string) bool { + c := strings.TrimSpace(command) + if len(teams) == 0 { + for _, rule := range checker.rules { + for key, value := range rule { + if (key == wildcard) && (value == wildcard || strings.EqualFold(value, c)) { + return true + } + } + } + } else { + for _, t := range teams { + if checker.IsCommandAllowedForTeam(t, command) { + return true + } + } + } + return false +} diff --git a/server/events/team_allowlist_checker_test.go b/server/events/team_allowlist_checker_test.go new file mode 100644 index 0000000000..ec8039ded5 --- /dev/null +++ b/server/events/team_allowlist_checker_test.go @@ -0,0 +1,35 @@ +package events_test + +import ( + "testing" + + "github.com/runatlantis/atlantis/server/events" + . "github.com/runatlantis/atlantis/testing" +) + +func TestNewTeamAllowListChecker(t *testing.T) { + allowlist := `bob:plan, dave:apply` + _, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) +} + +func TestIsCommandAllowedForTeam(t *testing.T) { + allowlist := `bob:plan, dave:apply, connie:plan, connie:apply` + checker, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) + Equals(t, true, checker.IsCommandAllowedForTeam("connie", "plan")) + Equals(t, true, checker.IsCommandAllowedForTeam("connie", "apply")) + Equals(t, true, checker.IsCommandAllowedForTeam("dave", "apply")) + Equals(t, true, checker.IsCommandAllowedForTeam("bob", "plan")) + Equals(t, false, checker.IsCommandAllowedForTeam("bob", "apply")) +} + +func TestIsCommandAllowedForAnyTeam(t *testing.T) { + allowlist := `alpha:plan,beta:release` + teams := []string{`alpha`, `beta`} + checker, err := events.NewTeamAllowlistChecker(allowlist) + Ok(t, err) + Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `plan`)) + Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `release`)) + Equals(t, false, checker.IsCommandAllowedForAnyTeam(teams, `noop`)) +} diff --git a/server/events/vcs/azuredevops_client.go b/server/events/vcs/azuredevops_client.go index 2369048b4b..6d0b4469dd 100644 --- a/server/events/vcs/azuredevops_client.go +++ b/server/events/vcs/azuredevops_client.go @@ -379,6 +379,11 @@ func SplitAzureDevopsRepoFullName(repoFullName string) (owner string, project st return repoFullName[:lastSlashIdx], "", repoFullName[lastSlashIdx+1:] } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (g *AzureDevopsClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (g *AzureDevopsClient) SupportsSingleFileDownload(repo models.Repo) bool { return false } diff --git a/server/events/vcs/bitbucketcloud/client.go b/server/events/vcs/bitbucketcloud/client.go index 38dfa56a6b..b926c8cf4a 100644 --- a/server/events/vcs/bitbucketcloud/client.go +++ b/server/events/vcs/bitbucketcloud/client.go @@ -245,6 +245,11 @@ func (b *Client) makeRequest(method string, path string, reqBody io.Reader) ([]b return respBody, nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (b *Client) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (b *Client) SupportsSingleFileDownload(models.Repo) bool { return false } diff --git a/server/events/vcs/bitbucketserver/client.go b/server/events/vcs/bitbucketserver/client.go index 38a8acac14..e0041e1d3e 100644 --- a/server/events/vcs/bitbucketserver/client.go +++ b/server/events/vcs/bitbucketserver/client.go @@ -333,6 +333,11 @@ func (b *Client) makeRequest(method string, path string, reqBody io.Reader) ([]b return respBody, nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (b *Client) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + func (b *Client) SupportsSingleFileDownload(repo models.Repo) bool { return false } diff --git a/server/events/vcs/client.go b/server/events/vcs/client.go index 711e558204..e63fdcd1f7 100644 --- a/server/events/vcs/client.go +++ b/server/events/vcs/client.go @@ -38,6 +38,7 @@ type Client interface { UpdateStatus(repo models.Repo, pull models.PullRequest, state models.CommitStatus, src string, description string, url string) error MergePull(pull models.PullRequest, pullOptions models.PullRequestOptions) error MarkdownPullLink(pull models.PullRequest) (string, error) + GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) // DownloadRepoConfigFile return `atlantis.yaml` content from VCS (which support fetch a single file from repository) // The first return value indicate that repo contain atlantis.yaml or not diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index d8c310f343..3af7247a6f 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -454,6 +454,33 @@ func (g *GithubClient) MarkdownPullLink(pull models.PullRequest) (string, error) return fmt.Sprintf("#%d", pull.Num), nil } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +// https://developer.github.com/v3/teams/members/#get-team-membership +func (g *GithubClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + var teamNames []string + opts := &github.ListOptions{} + org := repo.Owner + for { + teams, resp, err := g.client.Teams.ListTeams(g.ctx, org, opts) + if err != nil { + return nil, err + } + for _, t := range teams { + membership, _, err := g.client.Teams.GetTeamMembershipBySlug(g.ctx, org, *t.Slug, user.Username) + if err == nil && membership != nil { + if *membership.State == "active" && (*membership.Role == "member" || *membership.Role == "maintainer") { + teamNames = append(teamNames, t.GetName()) + } + } + } + if resp.NextPage == 0 { + break + } + opts.Page = resp.NextPage + } + return teamNames, nil +} + // ExchangeCode returns a newly created app's info func (g *GithubClient) ExchangeCode(code string) (*GithubAppTemporarySecrets, error) { ctx := context.Background() diff --git a/server/events/vcs/gitlab_client.go b/server/events/vcs/gitlab_client.go index 7d380735a6..509d0e97df 100644 --- a/server/events/vcs/gitlab_client.go +++ b/server/events/vcs/gitlab_client.go @@ -354,6 +354,11 @@ func MustConstraint(constraint string) version.Constraints { return c } +// GetTeamNamesForUser returns the names of the teams or groups that the user belongs to (in the organization the repository belongs to). +func (g *GitlabClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, nil +} + // DownloadRepoConfigFile return `atlantis.yaml` content from VCS (which support fetch a single file from repository) // The first return value indicate that repo contain atlantis.yaml or not // if BaseRepo had one repo config file, its content will placed on the second return value diff --git a/server/events/vcs/mocks/mock_client.go b/server/events/vcs/mocks/mock_client.go index 38123421bb..a2bfd40d1c 100644 --- a/server/events/vcs/mocks/mock_client.go +++ b/server/events/vcs/mocks/mock_client.go @@ -150,6 +150,25 @@ func (mock *MockClient) PullIsApproved(_param0 models.Repo, _param1 models.PullR return ret0, ret1 } +func (mock *MockClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + if mock == nil { + panic("mock must not be nil. Use myMock := NewMockClient().") + } + params := []pegomock.Param{repo, user} + result := pegomock.GetGenericMockFrom(mock).Invoke("GetTeamNamesForUser", 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) PullIsMergeable(_param0 models.Repo, _param1 models.PullRequest) (bool, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockClient().") @@ -463,6 +482,17 @@ func (verifier *VerifierMockClient) PullIsMergeable(_param0 models.Repo, _param1 return &MockClient_PullIsMergeable_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } +func (verifier *VerifierMockClient) GetTeamNamesForUser(repo models.Repo, user models.User) *MockClient_GetTeamNamesForUser_OngoingVerification { + params := []pegomock.Param{repo, user} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "GetTeamNamesForUser", params, verifier.timeout) + return &MockClient_GetTeamNamesForUser_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} +} + +type MockClient_GetTeamNamesForUser_OngoingVerification struct { + mock *MockClient + methodInvocations []pegomock.MethodInvocation +} + type MockClient_PullIsMergeable_OngoingVerification struct { mock *MockClient methodInvocations []pegomock.MethodInvocation diff --git a/server/events/vcs/not_configured_vcs_client.go b/server/events/vcs/not_configured_vcs_client.go index 7780b1ef0d..45d4416228 100644 --- a/server/events/vcs/not_configured_vcs_client.go +++ b/server/events/vcs/not_configured_vcs_client.go @@ -53,6 +53,9 @@ func (a *NotConfiguredVCSClient) MarkdownPullLink(pull models.PullRequest) (stri func (a *NotConfiguredVCSClient) err() error { return fmt.Errorf("atlantis was not configured to support repos from %s", a.Host.String()) } +func (a *NotConfiguredVCSClient) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return nil, a.err() +} func (a *NotConfiguredVCSClient) SupportsSingleFileDownload(repo models.Repo) bool { return false diff --git a/server/events/vcs/proxy.go b/server/events/vcs/proxy.go index 5a9ed03132..2e937b4cf4 100644 --- a/server/events/vcs/proxy.go +++ b/server/events/vcs/proxy.go @@ -84,6 +84,10 @@ func (d *ClientProxy) MarkdownPullLink(pull models.PullRequest) (string, error) return d.clients[pull.BaseRepo.VCSHost.Type].MarkdownPullLink(pull) } +func (d *ClientProxy) GetTeamNamesForUser(repo models.Repo, user models.User) ([]string, error) { + return d.clients[repo.VCSHost.Type].GetTeamNamesForUser(repo, user) +} + func (d *ClientProxy) DownloadRepoConfigFile(pull models.PullRequest) (bool, []byte, error) { return d.clients[pull.BaseRepo.VCSHost.Type].DownloadRepoConfigFile(pull) } diff --git a/server/server.go b/server/server.go index 94ce797039..ec07ebb8e7 100644 --- a/server/server.go +++ b/server/server.go @@ -618,6 +618,10 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { if err != nil { return nil, err } + githubTeamAllowlistChecker, err := events.NewTeamAllowlistChecker(userConfig.GithubTeamAllowlist) + if err != nil { + return nil, err + } locksController := &controllers.LocksController{ AtlantisVersion: config.AtlantisVersion, AtlantisURL: parsedURL, @@ -643,6 +647,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { GitlabRequestParserValidator: &events_controllers.DefaultGitlabRequestParserValidator{}, GitlabWebhookSecret: []byte(userConfig.GitlabWebhookSecret), RepoAllowlistChecker: repoAllowlist, + TeamAllowlistChecker: githubTeamAllowlistChecker, SilenceAllowlistErrors: userConfig.SilenceAllowlistErrors, SupportedVCSHosts: supportedVCSHosts, VCSClient: vcsClient, diff --git a/server/user_config.go b/server/user_config.go index 13d7071e62..d0d1811aac 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -41,6 +41,7 @@ type UserConfig struct { GithubAppKey string `mapstructure:"gh-app-key"` GithubAppKeyFile string `mapstructure:"gh-app-key-file"` GithubAppSlug string `mapstructure:"gh-app-slug"` + GithubTeamAllowlist string `mapstructure:"gh-team-allowlist"` GitlabHostname string `mapstructure:"gitlab-hostname"` GitlabToken string `mapstructure:"gitlab-token"` GitlabUser string `mapstructure:"gitlab-user"` From f22d9513fdfde80be3b2fe81e65036dd6c4cebef Mon Sep 17 00:00:00 2001 From: Paul Erickson Date: Thu, 11 Nov 2021 15:26:16 -0600 Subject: [PATCH 2/5] Check team allowlist in command runner, rather than event controller --- .../controllers/events/events_controller.go | 37 ------------------ server/events/command_runner.go | 38 +++++++++++++++++++ server/server.go | 11 +++--- 3 files changed, 44 insertions(+), 42 deletions(-) diff --git a/server/controllers/events/events_controller.go b/server/controllers/events/events_controller.go index b6bfd76402..cd4d097aeb 100644 --- a/server/controllers/events/events_controller.go +++ b/server/controllers/events/events_controller.go @@ -62,7 +62,6 @@ type VCSEventsController struct { // request validation is done. GitlabWebhookSecret []byte RepoAllowlistChecker *events.RepoAllowlistChecker - TeamAllowlistChecker *events.TeamAllowlistChecker // SilenceAllowlistErrors controls whether we write an error comment on // pull requests from non-allowlisted repos. SilenceAllowlistErrors bool @@ -437,20 +436,6 @@ func (e *VCSEventsController) handleCommentEvent(w http.ResponseWriter, baseRepo return } - // Check if the user who commented has the permissions to execute the 'plan' or 'apply' commands - if !e.TestingMode { - ok, err := e.checkUserPermissions(baseRepo, user, parseResult.Command) - if err != nil { - e.Logger.Err("unable to comment on pull request: %s", err) - return - } - if !ok { - e.commentUserDoesNotHavePermissions(baseRepo, pullNum, user, parseResult.Command) - e.respond(w, logging.Warn, http.StatusForbidden, "User @%s does not have permissions to execute '%s' command", user.Username, parseResult.Command.Name.String()) - return - } - } - e.Logger.Debug("executing command") fmt.Fprintln(w, "Processing...") if !e.TestingMode { @@ -568,25 +553,3 @@ func (e *VCSEventsController) commentNotAllowlisted(baseRepo models.Repo, pullNu e.Logger.Err("unable to comment on pull request: %s", err) } } - -// commentUserDoesNotHavePermissions comments on the pull request that the user -// is not allowed to execute the command. -func (e *VCSEventsController) commentUserDoesNotHavePermissions(baseRepo models.Repo, pullNum int, user models.User, cmd *events.CommentCommand) { - errMsg := fmt.Sprintf("```\nError: User @%s does not have permissions to execute '%s' command.\n```", user.Username, cmd.Name) - if err := e.VCSClient.CreateComment(baseRepo, pullNum, errMsg, ""); err != nil { - e.Logger.Err("unable to comment on pull request: %s", err) - } -} - -// checkUserPermissions checks if the user has permissions to execute the command -func (e *VCSEventsController) checkUserPermissions(repo models.Repo, user models.User, cmd *events.CommentCommand) (bool, error) { - teams, err := e.VCSClient.GetTeamNamesForUser(repo, user) - if err != nil { - return false, err - } - ok := e.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(teams, cmd.Name.String()) - if !ok { - return false, nil - } - return true, nil -} diff --git a/server/events/command_runner.go b/server/events/command_runner.go index cb15e75eb7..106e392061 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -116,6 +116,7 @@ type DefaultCommandRunner struct { Drainer *Drainer PreWorkflowHooksCommandRunner PreWorkflowHooksCommandRunner PullStatusFetcher PullStatusFetcher + TeamAllowlistChecker *TeamAllowlistChecker } // RunAutoplanCommand runs plan and policy_checks when a pull request is opened or updated. @@ -162,6 +163,32 @@ func (c *DefaultCommandRunner) RunAutoplanCommand(baseRepo models.Repo, headRepo autoPlanRunner.Run(ctx, nil) } +// commentUserDoesNotHavePermissions comments on the pull request that the user +// is not allowed to execute the command. +func (c *DefaultCommandRunner) commentUserDoesNotHavePermissions(baseRepo models.Repo, pullNum int, user models.User, cmd *CommentCommand) { + errMsg := fmt.Sprintf("```\nError: User @%s does not have permissions to execute '%s' command.\n```", user.Username, cmd.Name.String()) + if err := c.VCSClient.CreateComment(baseRepo, pullNum, errMsg, ""); err != nil { + c.Logger.Err("unable to comment on pull request: %s", err) + } +} + +// checkUserPermissions checks if the user has permissions to execute the command +func (c *DefaultCommandRunner) checkUserPermissions(repo models.Repo, user models.User, cmd *CommentCommand) (bool, error) { + if c.TeamAllowlistChecker == nil || len(c.TeamAllowlistChecker.rules) == 0 { + // allowlist restriction is not enabled + return true, nil + } + teams, err := c.VCSClient.GetTeamNamesForUser(repo, user) + if err != nil { + return false, err + } + ok := c.TeamAllowlistChecker.IsCommandAllowedForAnyTeam(teams, cmd.Name.String()) + if !ok { + return false, nil + } + return true, nil +} + // RunCommentCommand executes the command. // We take in a pointer for maybeHeadRepo because for some events there isn't // enough data to construct the Repo model and callers might want to wait until @@ -179,6 +206,17 @@ func (c *DefaultCommandRunner) RunCommentCommand(baseRepo models.Repo, maybeHead log := c.buildLogger(baseRepo.FullName, pullNum) defer c.logPanics(baseRepo, pullNum, log) + // Check if the user who commented has the permissions to execute the 'plan' or 'apply' commands + ok, err := c.checkUserPermissions(baseRepo, user, cmd) + if err != nil { + c.Logger.Err("Unable to check user permissions: %s", err) + return + } + if !ok { + c.commentUserDoesNotHavePermissions(baseRepo, pullNum, user, cmd) + return + } + headRepo, pull, err := c.ensureValidRepoMetadata(baseRepo, maybeHeadRepo, maybePull, user, pullNum, log) if err != nil { return diff --git a/server/server.go b/server/server.go index ec07ebb8e7..af25be02b4 100644 --- a/server/server.go +++ b/server/server.go @@ -596,6 +596,11 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { models.VersionCommand: versionCommandRunner, } + githubTeamAllowlistChecker, err := events.NewTeamAllowlistChecker(userConfig.GithubTeamAllowlist) + if err != nil { + return nil, err + } + commandRunner := &events.DefaultCommandRunner{ VCSClient: vcsClient, GithubPullGetter: githubClient, @@ -613,15 +618,12 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { Drainer: drainer, PreWorkflowHooksCommandRunner: preWorkflowHooksCommandRunner, PullStatusFetcher: boltdb, + TeamAllowlistChecker: githubTeamAllowlistChecker, } repoAllowlist, err := events.NewRepoAllowlistChecker(userConfig.RepoAllowlist) if err != nil { return nil, err } - githubTeamAllowlistChecker, err := events.NewTeamAllowlistChecker(userConfig.GithubTeamAllowlist) - if err != nil { - return nil, err - } locksController := &controllers.LocksController{ AtlantisVersion: config.AtlantisVersion, AtlantisURL: parsedURL, @@ -647,7 +649,6 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { GitlabRequestParserValidator: &events_controllers.DefaultGitlabRequestParserValidator{}, GitlabWebhookSecret: []byte(userConfig.GitlabWebhookSecret), RepoAllowlistChecker: repoAllowlist, - TeamAllowlistChecker: githubTeamAllowlistChecker, SilenceAllowlistErrors: userConfig.SilenceAllowlistErrors, SupportedVCSHosts: supportedVCSHosts, VCSClient: vcsClient, From fbcd74f7d1d817f484f67cfb08aed842279219dc Mon Sep 17 00:00:00 2001 From: Paul Erickson Date: Fri, 17 Dec 2021 16:17:18 -0600 Subject: [PATCH 3/5] Remove unneeded trimming --- server/events/team_allowlist_checker.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/server/events/team_allowlist_checker.go b/server/events/team_allowlist_checker.go index 8995365edc..5d0cc49f03 100644 --- a/server/events/team_allowlist_checker.go +++ b/server/events/team_allowlist_checker.go @@ -37,11 +37,9 @@ func NewTeamAllowlistChecker(allowlist string) (*TeamAllowlistChecker, error) { // IsCommandAllowedForTeam returns true if the team is allowed to execute the command // and false otherwise. func (checker *TeamAllowlistChecker) IsCommandAllowedForTeam(team string, command string) bool { - t := strings.TrimSpace(team) - c := strings.TrimSpace(command) for _, rule := range checker.rules { for key, value := range rule { - if (key == wildcard || strings.EqualFold(key, t)) && (value == wildcard || strings.EqualFold(value, c)) { + if (key == wildcard || strings.EqualFold(key, team)) && (value == wildcard || strings.EqualFold(value, command)) { return true } } @@ -52,11 +50,10 @@ func (checker *TeamAllowlistChecker) IsCommandAllowedForTeam(team string, comman // IsCommandAllowedForAnyTeam returns true if any of the teams is allowed to execute the command // and false otherwise. func (checker *TeamAllowlistChecker) IsCommandAllowedForAnyTeam(teams []string, command string) bool { - c := strings.TrimSpace(command) if len(teams) == 0 { for _, rule := range checker.rules { for key, value := range rule { - if (key == wildcard) && (value == wildcard || strings.EqualFold(value, c)) { + if (key == wildcard) && (value == wildcard || strings.EqualFold(value, command)) { return true } } From 23c5cd9833a2e0d76983249961abebeda39b54a7 Mon Sep 17 00:00:00 2001 From: Paul Erickson Date: Fri, 17 Dec 2021 16:17:39 -0600 Subject: [PATCH 4/5] Test wildcard groups and commands --- server/events/team_allowlist_checker_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/server/events/team_allowlist_checker_test.go b/server/events/team_allowlist_checker_test.go index ec8039ded5..59087cf141 100644 --- a/server/events/team_allowlist_checker_test.go +++ b/server/events/team_allowlist_checker_test.go @@ -25,11 +25,12 @@ func TestIsCommandAllowedForTeam(t *testing.T) { } func TestIsCommandAllowedForAnyTeam(t *testing.T) { - allowlist := `alpha:plan,beta:release` + allowlist := `alpha:plan,beta:release,*:unlock,nobody:*` teams := []string{`alpha`, `beta`} checker, err := events.NewTeamAllowlistChecker(allowlist) Ok(t, err) Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `plan`)) Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `release`)) + Equals(t, true, checker.IsCommandAllowedForAnyTeam(teams, `unlock`)) Equals(t, false, checker.IsCommandAllowedForAnyTeam(teams, `noop`)) } From ac049eaade6232e6a6dde0ff8dea3a37e5fcdd8c Mon Sep 17 00:00:00 2001 From: Paul Erickson Date: Fri, 17 Dec 2021 16:27:22 -0600 Subject: [PATCH 5/5] Improve error logging --- server/events/vcs/github_client.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index 3af7247a6f..96f7877d6c 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -463,11 +463,13 @@ func (g *GithubClient) GetTeamNamesForUser(repo models.Repo, user models.User) ( for { teams, resp, err := g.client.Teams.ListTeams(g.ctx, org, opts) if err != nil { - return nil, err + return nil, errors.Wrap(err, "retrieving GitHub teams") } for _, t := range teams { membership, _, err := g.client.Teams.GetTeamMembershipBySlug(g.ctx, org, *t.Slug, user.Username) - if err == nil && membership != nil { + if err != nil { + g.logger.Err("Failed to get team membership from GitHub: %s", err) + } else if membership != nil { if *membership.State == "active" && (*membership.Role == "member" || *membership.Role == "maintainer") { teamNames = append(teamNames, t.GetName()) }