From a24d940f5d58acd3bf2d8d1b22152e7f8d08e8f4 Mon Sep 17 00:00:00 2001 From: Luke Kysow Date: Wed, 28 Feb 2018 10:56:36 -0800 Subject: [PATCH] Rename DetermineCommand to Parse --- server/events/comment_parser.go | 47 +++++++++++++-------- server/events/comment_parser_test.go | 36 ++++++++-------- server/events/mocks/mock_comment_parsing.go | 16 +++---- server/events_controller.go | 2 +- server/events_controller_test.go | 10 ++--- 5 files changed, 62 insertions(+), 49 deletions(-) diff --git a/server/events/comment_parser.go b/server/events/comment_parser.go index afb2e2295..31991e356 100644 --- a/server/events/comment_parser.go +++ b/server/events/comment_parser.go @@ -12,10 +12,14 @@ import ( //go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_comment_parsing.go CommentParsing +// CommentParsing handles parsing pull request comments. type CommentParsing interface { - DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult + // Parse attempts to parse a pull request comment to see if it's an Atlantis + // commmand. + Parse(comment string, vcsHost vcs.Host) CommentParseResult } +// CommentParser implements CommentParsing type CommentParser struct { GithubUser string GithubToken string @@ -35,7 +39,7 @@ type CommentParseResult struct { Ignore bool } -// DetermineCommand parses the comment as an Atlantis command. +// Parse parses the comment as an Atlantis command. // // Valid commands contain: // - The initial "executable" name, 'run' or 'atlantis' or '@GithubUser' @@ -52,7 +56,7 @@ type CommentParseResult struct { // - atlantis plan --verbose -- -key=value -key2 value2 // // nolint: gocyclo -func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) CommentParseResult { +func (e *CommentParser) Parse(comment string, vcsHost vcs.Host) CommentParseResult { if multiLineRegex.MatchString(comment) { return CommentParseResult{Ignore: true} } @@ -137,6 +141,7 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme if err != nil { return CommentParseResult{CommentResponse: fmt.Sprintf("```\nError: %s.\nUsage of %s:\n%s\n```", err.Error(), command, flagSet.FlagUsagesWrapped(usagesCols))} } + // We only use the extra args after the --. For example given a comment: // "atlantis plan -bad-option -- -target=hi" // we only append "-target=hi" to the eventual command. @@ -152,21 +157,11 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme } } - // If dir is specified, must ensure it's a valid path. - if dir != "" { - validatedDir := filepath.Clean(dir) - // Join with . so the path is relative. This helps us if they use '/', - // and is safe to do if their path is relative since it's a no-op. - validatedDir = filepath.Join(".", validatedDir) - // Need to clean again to resolve relative validatedDirs. - validatedDir = filepath.Clean(validatedDir) - // Detect relative dirs since they're not allowed. - if strings.HasPrefix(validatedDir, "..") { - return CommentParseResult{CommentResponse: fmt.Sprintf("Error: Using a relative path %q with -d/--dir is not allowed", dir)} - } - - dir = validatedDir + dir, err = e.validateDir(dir) + if err != nil { + return CommentParseResult{CommentResponse: err.Error()} } + // Because we use the workspace name as a file, need to make sure it's // not doing something weird like being a relative dir. if strings.Contains(workspace, "..") { @@ -178,6 +173,24 @@ func (e *CommentParser) DetermineCommand(comment string, vcsHost vcs.Host) Comme } } +func (e *CommentParser) validateDir(dir string) (string, error) { + if dir == "" { + return dir, nil + } + validatedDir := filepath.Clean(dir) + // Join with . so the path is relative. This helps us if they use '/', + // and is safe to do if their path is relative since it's a no-op. + validatedDir = filepath.Join(".", validatedDir) + // Need to clean again to resolve relative validatedDirs. + validatedDir = filepath.Clean(validatedDir) + // Detect relative dirs since they're not allowed. + if strings.HasPrefix(validatedDir, "..") { + return "", fmt.Errorf("Error: Using a relative path %q with -d/--dir is not allowed", dir) + } + + return validatedDir, nil +} + func (e *CommentParser) stringInSlice(a string, list []string) bool { for _, b := range list { if b == a { diff --git a/server/events/comment_parser_test.go b/server/events/comment_parser_test.go index e19149fe0..8fbd69e2e 100644 --- a/server/events/comment_parser_test.go +++ b/server/events/comment_parser_test.go @@ -17,7 +17,7 @@ var commentParser = events.CommentParser{ GitlabToken: "gitlab-token", } -func TestDetermineCommand_Ignored(t *testing.T) { +func TestParse_Ignored(t *testing.T) { t.Log("given a comment that should be ignored we should set " + "CommentParseResult.Ignore to true") ignoreComments := []string{ @@ -28,12 +28,12 @@ func TestDetermineCommand_Ignored(t *testing.T) { "terraform plan\nbut with newlines", } for _, c := range ignoreComments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Assert(t, r.Ignore, "expected Ignore to be true for comment %q", c) } } -func TestDetermineCommand_HelpResponse(t *testing.T) { +func TestParse_HelpResponse(t *testing.T) { t.Log("given a comment that should result in help output we " + "should set CommentParseResult.CommentResult") helpComments := []string{ @@ -47,12 +47,12 @@ func TestDetermineCommand_HelpResponse(t *testing.T) { "atlantis help plan", } for _, c := range helpComments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Equals(t, events.HelpComment, r.CommentResponse) } } -func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { +func TestParse_DidYouMeanAtlantis(t *testing.T) { t.Log("given a comment that should result in a 'did you mean atlantis'" + "response, should set CommentParseResult.CommentResult") comments := []string{ @@ -65,13 +65,13 @@ func TestDetermineCommand_DidYouMeanAtlantis(t *testing.T) { "terraform plan -w workspace -d . -- test", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) Assert(t, r.CommentResponse == events.DidYouMeanAtlantisComment, "For comment %q expected CommentResponse==%q but got %q", c, events.DidYouMeanAtlantisComment, r.CommentResponse) } } -func TestDetermineCommand_InvalidCommand(t *testing.T) { +func TestParse_InvalidCommand(t *testing.T) { t.Log("given a comment with an invalid atlantis command, should return " + "a warning.") comments := []string{ @@ -80,14 +80,14 @@ func TestDetermineCommand_InvalidCommand(t *testing.T) { "atlantis appely apply", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := fmt.Sprintf("```\nError: unknown command %q.\nRun 'atlantis --help' for usage.\n```", strings.Fields(c)[1]) Assert(t, r.CommentResponse == exp, "For comment %q expected CommentResponse==%q but got %q", c, exp, r.CommentResponse) } } -func TestDetermineCommand_SubcommandUsage(t *testing.T) { +func TestParse_SubcommandUsage(t *testing.T) { t.Log("given a comment asking for the usage of a subcommand should " + "return help") comments := []string{ @@ -97,7 +97,7 @@ func TestDetermineCommand_SubcommandUsage(t *testing.T) { "atlantis apply --help", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Usage of " + strings.Fields(c)[1] Assert(t, strings.Contains(r.CommentResponse, exp), "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) @@ -106,7 +106,7 @@ func TestDetermineCommand_SubcommandUsage(t *testing.T) { } } -func TestDetermineCommand_InvalidFlags(t *testing.T) { +func TestParse_InvalidFlags(t *testing.T) { t.Log("given a comment with a valid atlantis command but invalid" + " flags, should return a warning and the proper usage") cases := []struct { @@ -131,7 +131,7 @@ func TestDetermineCommand_InvalidFlags(t *testing.T) { }, } for _, c := range cases { - r := commentParser.DetermineCommand(c.comment, vcs.Github) + r := commentParser.Parse(c.comment, vcs.Github) Assert(t, strings.Contains(r.CommentResponse, c.exp), "For comment %q expected CommentResponse %q to contain %q", c.comment, r.CommentResponse, c.exp) Assert(t, strings.Contains(r.CommentResponse, "Usage of "), @@ -139,7 +139,7 @@ func TestDetermineCommand_InvalidFlags(t *testing.T) { } } -func TestDetermineCommand_RelativeDirPath(t *testing.T) { +func TestParse_RelativeDirPath(t *testing.T) { t.Log("if -d is used with a relative path, should return an error") comments := []string{ "atlantis plan -d ..", @@ -153,14 +153,14 @@ func TestDetermineCommand_RelativeDirPath(t *testing.T) { "atlantis apply -d a/../..", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Error: Using a relative path" Assert(t, strings.Contains(r.CommentResponse, exp), "For comment %q expected CommentResponse %q to contain %q", c, r.CommentResponse, exp) } } -func TestDetermineCommand_InvalidWorkspace(t *testing.T) { +func TestParse_InvalidWorkspace(t *testing.T) { t.Log("if -w is used with '..', should return an error") comments := []string{ "atlantis plan -w ..", @@ -171,14 +171,14 @@ func TestDetermineCommand_InvalidWorkspace(t *testing.T) { "atlantis apply -w ../../../etc/passwd", } for _, c := range comments { - r := commentParser.DetermineCommand(c, vcs.Github) + r := commentParser.Parse(c, vcs.Github) exp := "Error: Value for -w/--workspace can't contain '..'" Assert(t, r.CommentResponse == exp, "For comment %q expected CommentResponse %q to be %q", c, r.CommentResponse, exp) } } -func TestDetermineCommand_Parsing(t *testing.T) { +func TestParse_Parsing(t *testing.T) { cases := []struct { flags string expWorkspace string @@ -346,7 +346,7 @@ func TestDetermineCommand_Parsing(t *testing.T) { for _, test := range cases { for _, cmdName := range []string{"plan", "apply"} { comment := fmt.Sprintf("atlantis %s %s", cmdName, test.flags) - r := commentParser.DetermineCommand(comment, vcs.Github) + r := commentParser.Parse(comment, vcs.Github) Assert(t, r.CommentResponse == "", "CommentResponse should have been empty but was %q for comment %q", r.CommentResponse, comment) Assert(t, test.expDir == r.Command.Dir, "exp dir to equal %q but was %q for comment %q", test.expDir, r.Command.Dir, comment) Assert(t, test.expWorkspace == r.Command.Workspace, "exp workspace to equal %q but was %q for comment %q", test.expWorkspace, r.Command.Workspace, comment) diff --git a/server/events/mocks/mock_comment_parsing.go b/server/events/mocks/mock_comment_parsing.go index e61cda226..54dabbd7d 100644 --- a/server/events/mocks/mock_comment_parsing.go +++ b/server/events/mocks/mock_comment_parsing.go @@ -19,9 +19,9 @@ func NewMockCommentParsing() *MockCommentParsing { return &MockCommentParsing{fail: pegomock.GlobalFailHandler} } -func (mock *MockCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) events.CommentParseResult { +func (mock *MockCommentParsing) Parse(comment string, vcsHost vcs.Host) events.CommentParseResult { params := []pegomock.Param{comment, vcsHost} - result := pegomock.GetGenericMockFrom(mock).Invoke("DetermineCommand", params, []reflect.Type{reflect.TypeOf((*events.CommentParseResult)(nil)).Elem()}) + result := pegomock.GetGenericMockFrom(mock).Invoke("Parse", params, []reflect.Type{reflect.TypeOf((*events.CommentParseResult)(nil)).Elem()}) var ret0 events.CommentParseResult if len(result) != 0 { if result[0] != nil { @@ -49,23 +49,23 @@ type VerifierCommentParsing struct { inOrderContext *pegomock.InOrderContext } -func (verifier *VerifierCommentParsing) DetermineCommand(comment string, vcsHost vcs.Host) *CommentParsing_DetermineCommand_OngoingVerification { +func (verifier *VerifierCommentParsing) Parse(comment string, vcsHost vcs.Host) *CommentParsing_Parse_OngoingVerification { params := []pegomock.Param{comment, vcsHost} - methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "DetermineCommand", params) - return &CommentParsing_DetermineCommand_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} + methodInvocations := pegomock.GetGenericMockFrom(verifier.mock).Verify(verifier.inOrderContext, verifier.invocationCountMatcher, "Parse", params) + return &CommentParsing_Parse_OngoingVerification{mock: verifier.mock, methodInvocations: methodInvocations} } -type CommentParsing_DetermineCommand_OngoingVerification struct { +type CommentParsing_Parse_OngoingVerification struct { mock *MockCommentParsing methodInvocations []pegomock.MethodInvocation } -func (c *CommentParsing_DetermineCommand_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { +func (c *CommentParsing_Parse_OngoingVerification) GetCapturedArguments() (string, vcs.Host) { comment, vcsHost := c.GetAllCapturedArguments() return comment[len(comment)-1], vcsHost[len(vcsHost)-1] } -func (c *CommentParsing_DetermineCommand_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { +func (c *CommentParsing_Parse_OngoingVerification) GetAllCapturedArguments() (_param0 []string, _param1 []vcs.Host) { params := pegomock.GetGenericMockFrom(c.mock).GetInvocationParams(c.methodInvocations) if len(params) > 0 { _param0 = make([]string, len(params[0])) diff --git a/server/events_controller.go b/server/events_controller.go index 084e1d172..145c5f263 100644 --- a/server/events_controller.go +++ b/server/events_controller.go @@ -152,7 +152,7 @@ func (e *EventsController) HandleGitlabCommentEvent(w http.ResponseWriter, event } func (e *EventsController) handleCommentEvent(w http.ResponseWriter, baseRepo models.Repo, headRepo models.Repo, user models.User, pullNum int, comment string, vcsHost vcs.Host) { - parseResult := e.CommentParser.DetermineCommand(comment, vcsHost) + parseResult := e.CommentParser.Parse(comment, vcsHost) if parseResult.Ignore { truncated := comment if len(truncated) > 40 { diff --git a/server/events_controller_test.go b/server/events_controller_test.go index c9f46b907..5179ed59d 100644 --- a/server/events_controller_test.go +++ b/server/events_controller_test.go @@ -124,7 +124,7 @@ func TestPost_GitlabCommentInvalidCommand(t *testing.T) { e, _, gl, _, _, _, _, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{Ignore: true}) + When(cp.Parse("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -137,7 +137,7 @@ func TestPost_GithubCommentInvalidCommand(t *testing.T) { event := `{"action": "created"}` When(v.Validate(eventsReq, secret)).ThenReturn([]byte(event), nil) When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(models.Repo{}, models.User{}, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Ignore: true}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{Ignore: true}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Ignoring non-command comment: \"\"") @@ -148,7 +148,7 @@ func TestPost_GitlabCommentResponse(t *testing.T) { e, _, gl, _, _, _, vcsClient, cp := setup(t) eventsReq.Header.Set(gitlabHeader, "value") When(gl.Validate(eventsReq, secret)).ThenReturn(gitlab.MergeCommentEvent{}, nil) - When(cp.DetermineCommand("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) + When(cp.Parse("", vcs.Gitlab)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) vcsClient.VerifyWasCalledOnce().CreateComment(models.Repo{}, 0, "a comment", vcs.Gitlab) @@ -164,7 +164,7 @@ func TestPost_GithubCommentResponse(t *testing.T) { baseRepo := models.Repo{} user := models.User{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{CommentResponse: "a comment"}) w := httptest.NewRecorder() e.Post(w, eventsReq) @@ -196,7 +196,7 @@ func TestPost_GithubCommentSuccess(t *testing.T) { user := models.User{} cmd := events.Command{} When(p.ParseGithubIssueCommentEvent(matchers.AnyPtrToGithubIssueCommentEvent())).ThenReturn(baseRepo, user, 1, nil) - When(cp.DetermineCommand("", vcs.Github)).ThenReturn(events.CommentParseResult{Command: &cmd}) + When(cp.Parse("", vcs.Github)).ThenReturn(events.CommentParseResult{Command: &cmd}) w := httptest.NewRecorder() e.Post(w, eventsReq) responseContains(t, w, http.StatusOK, "Processing...")