From fdfa88f05a27574c231ade19fe52dfc298e1cf82 Mon Sep 17 00:00:00 2001 From: Paul Erickson Date: Thu, 11 Nov 2021 15:26:16 -0600 Subject: [PATCH] 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,