From 31c0768ca81c392ca6554c77962360baaf3ffa64 Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Wed, 16 Dec 2020 13:06:43 -0500 Subject: [PATCH 1/5] Add support for fetching gh app user. --- cmd/server.go | 5 ++++ server/events/command_runner.go | 1 + server/events/vcs/github_client.go | 11 +++++++-- server/events/vcs/github_credentials.go | 32 +++++++++++++++++++------ server/server.go | 1 + server/user_config.go | 1 + 6 files changed, 42 insertions(+), 9 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 2ae1bab24..c9ee96e5a 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -61,6 +61,7 @@ const ( GHUserFlag = "gh-user" GHAppIDFlag = "gh-app-id" GHAppKeyFileFlag = "gh-app-key-file" + GHAppSlugFlag = "gh-app-slug" GHOrganizationFlag = "gh-org" GHWebhookSecretFlag = "gh-webhook-secret" // nolint: gosec GitlabHostnameFlag = "gitlab-hostname" @@ -181,6 +182,10 @@ var stringFlags = map[string]stringFlag{ description: "A path to a file containing the GitHub App's private key", defaultValue: "", }, + GHAppSlugFlag: { + description: "The Github app slug (ie. the URL-friendly name of your GitHub App)", + defaultValue: "atlantis", + }, GHOrganizationFlag: { description: "The name of the GitHub organization to use during the creation of a Github App for Atlantis", defaultValue: "", diff --git a/server/events/command_runner.go b/server/events/command_runner.go index 053185c38..eca7ca0dd 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -350,6 +350,7 @@ func (c *DefaultCommandRunner) updatePull(ctx *CommandContext, command PullComma // clutter in a pull/merge request. This will not delete the comment, since the // comment trail may be useful in auditing or backtracing problems. if c.HidePrevPlanComments { + ctx.Log.Info("Hide prev plan comments enabled") if err := c.VCSClient.HidePrevPlanComments(ctx.Pull.BaseRepo, ctx.Pull.Num); err != nil { ctx.Log.Err("unable to hide old comments: %s", err) } diff --git a/server/events/vcs/github_client.go b/server/events/vcs/github_client.go index e97c863b9..476260fc9 100644 --- a/server/events/vcs/github_client.go +++ b/server/events/vcs/github_client.go @@ -97,8 +97,15 @@ func NewGithubClient(hostname string, credentials GithubCredentials, logger *log transport, graphql.WithHeader("Accept", "application/vnd.github.queen-beryl-preview+json"), ) + + user, err := credentials.GetUser() + logger.Debug("GH User: %s", user) + + if err != nil { + return nil, errors.Wrap(err, "getting user") + } return &GithubClient{ - user: credentials.GetUser(), + user: user, client: client, v4MutateClient: v4MutateClient, ctx: context.Background(), @@ -179,7 +186,7 @@ func (g *GithubClient) HidePrevPlanComments(repo models.Repo, pullNum int) error ListOptions: github.ListOptions{Page: nextPage}, }) if err != nil { - return err + return errors.Wrap(err, "listing comments") } allComments = append(allComments, comments...) if resp.NextPage == 0 { diff --git a/server/events/vcs/github_credentials.go b/server/events/vcs/github_credentials.go index 29db764e0..f45d1b52e 100644 --- a/server/events/vcs/github_credentials.go +++ b/server/events/vcs/github_credentials.go @@ -18,7 +18,7 @@ import ( type GithubCredentials interface { Client() (*http.Client, error) GetToken() (string, error) - GetUser() string + GetUser() (string, error) } // GithubAnonymousCredentials expose no credentials. @@ -31,8 +31,8 @@ func (c *GithubAnonymousCredentials) Client() (*http.Client, error) { } // GetUser returns the username for these credentials. -func (c *GithubAnonymousCredentials) GetUser() string { - return "anonymous" +func (c *GithubAnonymousCredentials) GetUser() (string, error) { + return "anonymous", nil } // GetToken returns an empty token. @@ -56,8 +56,8 @@ func (c *GithubUserCredentials) Client() (*http.Client, error) { } // GetUser returns the username for these credentials. -func (c *GithubUserCredentials) GetUser() string { - return c.User +func (c *GithubUserCredentials) GetUser() (string, error) { + return c.User, nil } // GetToken returns the user token. @@ -73,6 +73,7 @@ type GithubAppCredentials struct { apiURL *url.URL installationID int64 tr *ghinstallation.Transport + AppSlug string } // Client returns a github app installation client. @@ -85,8 +86,25 @@ func (c *GithubAppCredentials) Client() (*http.Client, error) { } // GetUser returns the username for these credentials. -func (c *GithubAppCredentials) GetUser() string { - return "" +func (c *GithubAppCredentials) GetUser() (string, error) { + client, err := c.Client() + + if err != nil { + return "", errors.Wrap(err, "initializing client") + } + + ghClient := github.NewClient(client) + ghClient.BaseURL = c.getAPIURL() + ctx := context.Background() + + app, _, err := ghClient.Apps.Get(ctx, c.AppSlug) + + if err != nil { + return "", errors.Wrap(err, "getting app details") + } + // Currently there is no way to get the bot's login info, so this is a + // hack until Github exposes that. + return fmt.Sprintf("%s[bot]", app.GetName()), nil } // GetToken returns a fresh installation token. diff --git a/server/server.go b/server/server.go index 1727cca4f..fd60c40ac 100644 --- a/server/server.go +++ b/server/server.go @@ -158,6 +158,7 @@ func NewServer(userConfig UserConfig, config Config) (*Server, error) { AppID: userConfig.GithubAppID, KeyPath: userConfig.GithubAppKey, Hostname: userConfig.GithubHostname, + AppSlug: userConfig.GithubAppSlug, } githubAppEnabled = true } diff --git a/server/user_config.go b/server/user_config.go index 82cae6a75..d138c43f5 100644 --- a/server/user_config.go +++ b/server/user_config.go @@ -33,6 +33,7 @@ type UserConfig struct { GithubOrg string `mapstructure:"gh-org"` GithubAppID int64 `mapstructure:"gh-app-id"` GithubAppKey string `mapstructure:"gh-app-key-file"` + GithubAppSlug string `mapstructure:"gh-app-slug"` GitlabHostname string `mapstructure:"gitlab-hostname"` GitlabToken string `mapstructure:"gitlab-token"` GitlabUser string `mapstructure:"gitlab-user"` From 1f57d04a89290318954f9bc1fedf0fcf961c084a Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Wed, 16 Dec 2020 14:38:48 -0500 Subject: [PATCH 2/5] fix tests. --- cmd/server.go | 6 +- cmd/server_test.go | 1 + server/events/vcs/fixtures/fixtures.go | 55 ++++++++++++++++++- .../vcs/mocks/matchers/ptr_to_http_client.go | 15 ++++- .../vcs/mocks/mock_github_credentials.go | 18 +++--- 5 files changed, 85 insertions(+), 10 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index c9ee96e5a..64af9cfdb 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -105,6 +105,7 @@ const ( DefaultGitlabHostname = "gitlab.com" DefaultLogLevel = "info" DefaultStatsNamespace = "atlantis" + DefaultGithubAppSlug = "atlantis" DefaultPort = 4141 DefaultTFDownloadURL = "https://releases.hashicorp.com" DefaultTFEHostname = "app.terraform.io" @@ -184,7 +185,7 @@ var stringFlags = map[string]stringFlag{ }, GHAppSlugFlag: { description: "The Github app slug (ie. the URL-friendly name of your GitHub App)", - defaultValue: "atlantis", + defaultValue: DefaultGithubAppSlug, }, GHOrganizationFlag: { description: "The name of the GitHub organization to use during the creation of a Github App for Atlantis", @@ -572,6 +573,9 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig) { if c.StatsNamespace == "" { c.StatsNamespace = DefaultStatsNamespace } + if c.GithubAppSlug == "" { + c.GithubAppSlug = DefaultGithubAppSlug + } if c.Port == 0 { c.Port = DefaultPort } diff --git a/cmd/server_test.go b/cmd/server_test.go index d60f8bffe..e22b372ad 100644 --- a/cmd/server_test.go +++ b/cmd/server_test.go @@ -71,6 +71,7 @@ var testFlags = map[string]interface{}{ GHUserFlag: "user", GHAppIDFlag: int64(0), GHAppKeyFileFlag: "", + GHAppSlugFlag: "atlantis", GHOrganizationFlag: "", GHWebhookSecretFlag: "secret", GitlabHostnameFlag: "gitlab-hostname", diff --git a/server/events/vcs/fixtures/fixtures.go b/server/events/vcs/fixtures/fixtures.go index 0f9f2c331..2c4269114 100644 --- a/server/events/vcs/fixtures/fixtures.go +++ b/server/events/vcs/fixtures/fixtures.go @@ -332,7 +332,7 @@ var githubAppInstallationJSON = `[ // nolint: gosec var githubAppTokenJSON = `{ - "token": "v1.1f699f1069f60xx%d", + "token": "some-token", "expires_at": "2050-01-01T00:00:00Z", "permissions": { "issues": "write", @@ -452,6 +452,48 @@ var githubAppTokenJSON = `{ ] }` +var githubAppJSON = `{ + "id": 1, + "slug": "octoapp", + "node_id": "MDExOkludGVncmF0aW9uMQ==", + "owner": { + "login": "github", + "id": 1, + "node_id": "MDEyOk9yZ2FuaXphdGlvbjE=", + "url": "https://api.github.com/orgs/github", + "repos_url": "https://api.github.com/orgs/github/repos", + "events_url": "https://api.github.com/orgs/github/events", + "avatar_url": "https://github.com/images/error/octocat_happy.gif", + "gravatar_id": "", + "html_url": "https://github.com/octocat", + "followers_url": "https://api.github.com/users/octocat/followers", + "following_url": "https://api.github.com/users/octocat/following{/other_user}", + "gists_url": "https://api.github.com/users/octocat/gists{/gist_id}", + "starred_url": "https://api.github.com/users/octocat/starred{/owner}{/repo}", + "subscriptions_url": "https://api.github.com/users/octocat/subscriptions", + "organizations_url": "https://api.github.com/users/octocat/orgs", + "received_events_url": "https://api.github.com/users/octocat/received_events", + "type": "User", + "site_admin": true + }, + "name": "Octocat App", + "description": "", + "external_url": "https://example.com", + "html_url": "https://github.com/apps/octoapp", + "created_at": "2017-07-08T16:18:44-04:00", + "updated_at": "2017-07-08T16:18:44-04:00", + "permissions": { + "metadata": "read", + "contents": "read", + "issues": "write", + "single_file": "write" + }, + "events": [ + "push", + "pull_request" + ] + }` + func validateGithubToken(tokenString string) error { key, err := jwt.ParseRSAPrivateKeyFromPEM([]byte(GithubPrivateKey)) if err != nil { @@ -499,6 +541,17 @@ func GithubAppTestServer(t *testing.T) (string, error) { w.Write([]byte(githubAppInstallationJSON)) // nolint: errcheck return + case "/api/v3/app": + token := strings.Replace(r.Header.Get("Authorization"), "token ", "", 1) + + // token is taken from githubAppTokenJSON + if token != "some-token" { + w.WriteHeader(403) + w.Write([]byte("Invalid installation token")) // nolint: errcheck + return + } + w.Write([]byte(githubAppJSON)) // nolint: errcheck + return case "/api/v3/app/installations/1/access_tokens": token := strings.Replace(r.Header.Get("Authorization"), "Bearer ", "", 1) if err := validateGithubToken(token); err != nil { diff --git a/server/events/vcs/mocks/matchers/ptr_to_http_client.go b/server/events/vcs/mocks/matchers/ptr_to_http_client.go index 4ec255001..74766e888 100644 --- a/server/events/vcs/mocks/matchers/ptr_to_http_client.go +++ b/server/events/vcs/mocks/matchers/ptr_to_http_client.go @@ -2,8 +2,9 @@ package matchers import ( - "reflect" "github.com/petergtz/pegomock" + "reflect" + http "net/http" ) @@ -18,3 +19,15 @@ func EqPtrToHttpClient(value *http.Client) *http.Client { var nullValue *http.Client return nullValue } + +func NotEqPtrToHttpClient(value *http.Client) *http.Client { + pegomock.RegisterMatcher(&pegomock.NotEqMatcher{Value: value}) + var nullValue *http.Client + return nullValue +} + +func PtrToHttpClientThat(matcher pegomock.ArgumentMatcher) *http.Client { + pegomock.RegisterMatcher(matcher) + var nullValue *http.Client + return nullValue +} diff --git a/server/events/vcs/mocks/mock_github_credentials.go b/server/events/vcs/mocks/mock_github_credentials.go index 2837977c4..d491009dc 100644 --- a/server/events/vcs/mocks/mock_github_credentials.go +++ b/server/events/vcs/mocks/mock_github_credentials.go @@ -63,19 +63,23 @@ func (mock *MockGithubCredentials) GetToken() (string, error) { return ret0, ret1 } -func (mock *MockGithubCredentials) GetUser() string { +func (mock *MockGithubCredentials) GetUser() (string, error) { if mock == nil { panic("mock must not be nil. Use myMock := NewMockGithubCredentials().") } params := []pegomock.Param{} - result := pegomock.GetGenericMockFrom(mock).Invoke("GetUser", params, []reflect.Type{reflect.TypeOf((*string)(nil)).Elem()}) + result := pegomock.GetGenericMockFrom(mock).Invoke("GetUser", 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 + return ret0, ret1 } func (mock *MockGithubCredentials) VerifyWasCalledOnce() *VerifierMockGithubCredentials { @@ -85,14 +89,14 @@ func (mock *MockGithubCredentials) VerifyWasCalledOnce() *VerifierMockGithubCred } } -func (mock *MockGithubCredentials) VerifyWasCalled(invocationCountMatcher pegomock.Matcher) *VerifierMockGithubCredentials { +func (mock *MockGithubCredentials) VerifyWasCalled(invocationCountMatcher pegomock.InvocationCountMatcher) *VerifierMockGithubCredentials { return &VerifierMockGithubCredentials{ mock: mock, invocationCountMatcher: invocationCountMatcher, } } -func (mock *MockGithubCredentials) VerifyWasCalledInOrder(invocationCountMatcher pegomock.Matcher, inOrderContext *pegomock.InOrderContext) *VerifierMockGithubCredentials { +func (mock *MockGithubCredentials) VerifyWasCalledInOrder(invocationCountMatcher pegomock.InvocationCountMatcher, inOrderContext *pegomock.InOrderContext) *VerifierMockGithubCredentials { return &VerifierMockGithubCredentials{ mock: mock, invocationCountMatcher: invocationCountMatcher, @@ -100,7 +104,7 @@ func (mock *MockGithubCredentials) VerifyWasCalledInOrder(invocationCountMatcher } } -func (mock *MockGithubCredentials) VerifyWasCalledEventually(invocationCountMatcher pegomock.Matcher, timeout time.Duration) *VerifierMockGithubCredentials { +func (mock *MockGithubCredentials) VerifyWasCalledEventually(invocationCountMatcher pegomock.InvocationCountMatcher, timeout time.Duration) *VerifierMockGithubCredentials { return &VerifierMockGithubCredentials{ mock: mock, invocationCountMatcher: invocationCountMatcher, @@ -110,7 +114,7 @@ func (mock *MockGithubCredentials) VerifyWasCalledEventually(invocationCountMatc type VerifierMockGithubCredentials struct { mock *MockGithubCredentials - invocationCountMatcher pegomock.Matcher + invocationCountMatcher pegomock.InvocationCountMatcher inOrderContext *pegomock.InOrderContext timeout time.Duration } From 98847ffa8d37e22fd12ac1b5b307941d79729f59 Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Wed, 16 Dec 2020 15:08:55 -0500 Subject: [PATCH 3/5] make param optional. --- cmd/server.go | 5 --- server/events/vcs/fixtures/fixtures.go | 2 +- server/events/vcs/github_credentials.go | 4 +++ server/events/vcs/github_credentials_test.go | 35 ++++++++++++++++++++ 4 files changed, 40 insertions(+), 6 deletions(-) diff --git a/cmd/server.go b/cmd/server.go index 64af9cfdb..b8bca7814 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -105,7 +105,6 @@ const ( DefaultGitlabHostname = "gitlab.com" DefaultLogLevel = "info" DefaultStatsNamespace = "atlantis" - DefaultGithubAppSlug = "atlantis" DefaultPort = 4141 DefaultTFDownloadURL = "https://releases.hashicorp.com" DefaultTFEHostname = "app.terraform.io" @@ -185,7 +184,6 @@ var stringFlags = map[string]stringFlag{ }, GHAppSlugFlag: { description: "The Github app slug (ie. the URL-friendly name of your GitHub App)", - defaultValue: DefaultGithubAppSlug, }, GHOrganizationFlag: { description: "The name of the GitHub organization to use during the creation of a Github App for Atlantis", @@ -573,9 +571,6 @@ func (s *ServerCmd) setDefaults(c *server.UserConfig) { if c.StatsNamespace == "" { c.StatsNamespace = DefaultStatsNamespace } - if c.GithubAppSlug == "" { - c.GithubAppSlug = DefaultGithubAppSlug - } if c.Port == 0 { c.Port = DefaultPort } diff --git a/server/events/vcs/fixtures/fixtures.go b/server/events/vcs/fixtures/fixtures.go index 2c4269114..5bd935ba4 100644 --- a/server/events/vcs/fixtures/fixtures.go +++ b/server/events/vcs/fixtures/fixtures.go @@ -541,7 +541,7 @@ func GithubAppTestServer(t *testing.T) (string, error) { w.Write([]byte(githubAppInstallationJSON)) // nolint: errcheck return - case "/api/v3/app": + case "/api/v3/apps/some-app": token := strings.Replace(r.Header.Get("Authorization"), "token ", "", 1) // token is taken from githubAppTokenJSON diff --git a/server/events/vcs/github_credentials.go b/server/events/vcs/github_credentials.go index f45d1b52e..dc18457e0 100644 --- a/server/events/vcs/github_credentials.go +++ b/server/events/vcs/github_credentials.go @@ -87,6 +87,10 @@ func (c *GithubAppCredentials) Client() (*http.Client, error) { // GetUser returns the username for these credentials. func (c *GithubAppCredentials) GetUser() (string, error) { + // Keeping backwards compatibility since this flag is optional + if c.AppSlug == "" { + return "", nil + } client, err := c.Client() if err != nil { diff --git a/server/events/vcs/github_credentials_test.go b/server/events/vcs/github_credentials_test.go index 3be20f3b2..c643c9706 100644 --- a/server/events/vcs/github_credentials_test.go +++ b/server/events/vcs/github_credentials_test.go @@ -9,6 +9,36 @@ import ( . "github.com/runatlantis/atlantis/testing" ) +func TestGithubClient_GetUser_AppSlug(t *testing.T) { + defer disableSSLVerification()() + testServer, err := fixtures.GithubAppTestServer(t) + Ok(t, err) + + anonCreds := &vcs.GithubAnonymousCredentials{} + anonClient, err := vcs.NewGithubClient(testServer, anonCreds, nil) + Ok(t, err) + tempSecrets, err := anonClient.ExchangeCode("good-code") + Ok(t, err) + + tmpDir, cleanup := DirStructure(t, map[string]interface{}{ + "key.pem": tempSecrets.Key, + }) + defer cleanup() + keyPath := fmt.Sprintf("%v/key.pem", tmpDir) + + appCreds := &vcs.GithubAppCredentials{ + AppID: tempSecrets.ID, + KeyPath: keyPath, + Hostname: testServer, + AppSlug: "some-app", + } + + user, err := appCreds.GetUser() + Ok(t, err) + + Assert(t, user == "Octocat App[bot]", "user should not empty") +} + func TestGithubClient_AppAuthentication(t *testing.T) { defer disableSSLVerification()() testServer, err := fixtures.GithubAppTestServer(t) @@ -40,6 +70,11 @@ func TestGithubClient_AppAuthentication(t *testing.T) { newToken, err := appCreds.GetToken() Ok(t, err) + user, err := appCreds.GetUser() + Ok(t, err) + + Assert(t, user == "", "user should be empty") + if token != newToken { t.Errorf("app token was not cached: %q != %q", token, newToken) } From e2b2a88db02e215b83fb9a5e2486c0f8c16ba9e3 Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Wed, 16 Dec 2020 15:10:52 -0500 Subject: [PATCH 4/5] remove log statement. --- server/events/command_runner.go | 1 - 1 file changed, 1 deletion(-) diff --git a/server/events/command_runner.go b/server/events/command_runner.go index eca7ca0dd..053185c38 100644 --- a/server/events/command_runner.go +++ b/server/events/command_runner.go @@ -350,7 +350,6 @@ func (c *DefaultCommandRunner) updatePull(ctx *CommandContext, command PullComma // clutter in a pull/merge request. This will not delete the comment, since the // comment trail may be useful in auditing or backtracing problems. if c.HidePrevPlanComments { - ctx.Log.Info("Hide prev plan comments enabled") if err := c.VCSClient.HidePrevPlanComments(ctx.Pull.BaseRepo, ctx.Pull.Num); err != nil { ctx.Log.Err("unable to hide old comments: %s", err) } From 6fe161776c66c0c5c214690ac0602d79a07b2eb6 Mon Sep 17 00:00:00 2001 From: Nish Krishnan Date: Wed, 16 Dec 2020 15:16:22 -0500 Subject: [PATCH 5/5] fmt. --- cmd/server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/server.go b/cmd/server.go index b8bca7814..101d7fd43 100644 --- a/cmd/server.go +++ b/cmd/server.go @@ -183,7 +183,7 @@ var stringFlags = map[string]stringFlag{ defaultValue: "", }, GHAppSlugFlag: { - description: "The Github app slug (ie. the URL-friendly name of your GitHub App)", + description: "The Github app slug (ie. the URL-friendly name of your GitHub App)", }, GHOrganizationFlag: { description: "The name of the GitHub organization to use during the creation of a Github App for Atlantis",