From 124369b89b5797df72dab8ec3c224945eb13fe44 Mon Sep 17 00:00:00 2001 From: Dipti Pai Date: Wed, 4 Dec 2024 12:35:25 -0800 Subject: [PATCH] review comments Signed-off-by: Dipti Pai --- auth/github/client.go | 15 ++- auth/github/client_test.go | 17 ++-- git/credentials_test.go | 43 +++++---- git/gogit/client_test.go | 186 +++++++++++++------------------------ 4 files changed, 112 insertions(+), 149 deletions(-) diff --git a/auth/github/client.go b/auth/github/client.go index fd78cbf4..1c190206 100644 --- a/auth/github/client.go +++ b/auth/github/client.go @@ -32,7 +32,7 @@ const ( AppIDKey = "githubAppID" AppInstallationIDKey = "githubAppInstallationID" AppPrivateKey = "githubAppPrivateKey" - AppBaseUrl = "githubAppBaseURL" + AppBaseUrlKey = "githubAppBaseURL" ) // Client is an authentication provider for GitHub Apps. @@ -68,15 +68,26 @@ func New(opts ...OptFunc) (*Client, error) { transport.Proxy = proxyFunc } + if len(p.appID) == 0 { + return nil, fmt.Errorf("app ID must be provided to use github app authentication") + } appID, err := strconv.Atoi(p.appID) if err != nil { return nil, fmt.Errorf("invalid app id, err: %w", err) } + + if len(p.installationID) == 0 { + return nil, fmt.Errorf("app installation ID must be provided to use github app authentication") + } installationID, err := strconv.Atoi(p.installationID) if err != nil { return nil, fmt.Errorf("invalid app installation id, err: %w", err) } + if len(p.privateKey) == 0 { + return nil, fmt.Errorf("private key must be provided to use github app authentication") + } + p.ghTransport, err = ghinstallation.New(transport, int64(appID), int64(installationID), p.privateKey) if err != nil { return nil, err @@ -133,7 +144,7 @@ func WithAppData(appData map[string][]byte) OptFunc { if ok { p.privateKey = val } - val, ok = appData[AppBaseUrl] + val, ok = appData[AppBaseUrlKey] if ok { p.apiURL = string(val) } diff --git a/auth/github/client_test.go b/auth/github/client_test.go index 8bc76be7..a0b66e9c 100644 --- a/auth/github/client_test.go +++ b/auth/github/client_test.go @@ -67,7 +67,7 @@ func TestClient_Options(t *testing.T) { { name: "Create new client with empty data", opts: []OptFunc{WithAppData(map[string][]byte{})}, - wantErr: errors.New("invalid app id, err: strconv.Atoi: parsing \"\": invalid syntax"), + wantErr: errors.New("app ID must be provided to use github app authentication"), }, { name: "Create new client with app data with missing AppID Key", @@ -76,16 +76,16 @@ func TestClient_Options(t *testing.T) { AppPrivateKey: kp.PrivateKey, }, )}, - wantErr: errors.New("invalid app id, err: strconv.Atoi: parsing \"\": invalid syntax"), + wantErr: errors.New("app ID must be provided to use github app authentication"), }, { - name: "Create new client with app data with missing AppInstallationIDKey Key", + name: "Create new client with app data with missing AppInstallationID Key", opts: []OptFunc{WithAppData(map[string][]byte{ AppIDKey: []byte("123"), AppPrivateKey: kp.PrivateKey, }, )}, - wantErr: errors.New("invalid app installation id, err: strconv.Atoi: parsing \"\": invalid syntax"), + wantErr: errors.New("app installation ID must be provided to use github app authentication"), }, { name: "Create new client with app data with missing private Key", @@ -94,7 +94,7 @@ func TestClient_Options(t *testing.T) { AppInstallationIDKey: []byte(installationID), }, )}, - wantErr: errors.New("could not parse private key: invalid key: Key must be a PEM encoded PKCS1 or PKCS8 key"), + wantErr: errors.New("private key must be provided to use github app authentication"), }, { name: "Create new client with invalid appID in app data", @@ -121,16 +121,11 @@ func TestClient_Options(t *testing.T) { opts: []OptFunc{WithAppData(map[string][]byte{ AppIDKey: []byte(appID), AppInstallationIDKey: []byte(installationID), - AppPrivateKey: []byte(""), + AppPrivateKey: []byte(" "), }, )}, wantErr: errors.New("could not parse private key: invalid key: Key must be a PEM encoded PKCS1 or PKCS8 key"), }, - { - name: "Create new client with no private key option", - opts: []OptFunc{WithInstllationID(installationID), WithAppID(appID)}, - wantErr: errors.New("could not parse private key: invalid key: Key must be a PEM encoded PKCS1 or PKCS8 key"), - }, } for _, tt := range tests { diff --git a/git/credentials_test.go b/git/credentials_test.go index db529bdc..1a5a531a 100644 --- a/git/credentials_test.go +++ b/git/credentials_test.go @@ -105,30 +105,39 @@ func TestGetCredentials(t *testing.T) { } func TestGetCredentials_GitHub(t *testing.T) { + kp, _ := ssh.GenerateKeyPair(ssh.RSA_4096) expiresAt := time.Now().UTC().Add(time.Hour) tests := []struct { name string + githubOpts []github.OptFunc accessToken *github.AppToken statusCode int wantCredentials *Credentials - wantErr bool + wantErr string }{ { - name: "get credentials from github success", - statusCode: http.StatusOK, - accessToken: &github.AppToken{ - Token: "access-token", - ExpiresAt: expiresAt, - }, - wantCredentials: &Credentials{ - Username: GitHubAccessTokenUsername, - Password: "access-token", - }, + name: "get credentials from github success", + githubOpts: []github.OptFunc{github.WithAppID("123"), github.WithInstllationID("456"), github.WithPrivateKey(kp.PrivateKey)}, + statusCode: http.StatusOK, + accessToken: &github.AppToken{Token: "access-token", ExpiresAt: expiresAt}, + wantCredentials: &Credentials{Username: GitHubAccessTokenUsername, Password: "access-token"}, }, { name: "get credentials from github failure", + githubOpts: []github.OptFunc{github.WithAppID("123"), github.WithInstllationID("456"), github.WithPrivateKey(kp.PrivateKey)}, + statusCode: http.StatusInternalServerError, + wantErr: "could not refresh installation id 456's token: received non 2xx response status \"500 Internal Server Error\"", + }, + { + name: "get credentials from github new client failure", + githubOpts: []github.OptFunc{github.WithInstllationID("456"), github.WithPrivateKey(kp.PrivateKey)}, statusCode: http.StatusInternalServerError, - wantErr: true, + wantErr: "app ID must be provided to use github app authentication", + }, + { + name: "get credentials from github with nil github Opts", + githubOpts: nil, + wantErr: "provider options are not specified for GitHub", }, } @@ -151,13 +160,13 @@ func TestGetCredentials_GitHub(t *testing.T) { srv.Close() }) - kp, err := ssh.GenerateKeyPair(ssh.RSA_4096) - g.Expect(err).ToNot(HaveOccurred()) - providerOpts := &ProviderOptions{ Name: ProviderGitHub, - GitHubOpts: []github.OptFunc{github.WithAppBaseURL(srv.URL), github.WithAppID("123"), - github.WithInstllationID("456"), github.WithPrivateKey(kp.PrivateKey)}, + } + if tt.githubOpts != nil { + providerOpts.GitHubOpts = append(tt.githubOpts, github.WithAppBaseURL(srv.URL)) + } else { + providerOpts.GitHubOpts = nil } creds, expiry, err := GetCredentials(context.TODO(), providerOpts) diff --git a/git/gogit/client_test.go b/git/gogit/client_test.go index 8a887fa1..1afbb3cc 100644 --- a/git/gogit/client_test.go +++ b/git/gogit/client_test.go @@ -651,6 +651,7 @@ func TestValidateUrl(t *testing.T) { password string bearerToken string url string + provider string credentialsOverHttp bool expectedError string }{ @@ -715,6 +716,36 @@ func TestValidateUrl(t *testing.T) { url: "https://url", expectedError: "basic auth and bearer token cannot be set at the same time", }, + { + name: "blocked: authopts with azure provider and username/password/https", + transport: git.HTTPS, + username: "user", + password: "pass", + provider: git.ProviderAzure, + url: "https://url", + expectedError: "basic auth and provider cannot be set at the same time", + }, + { + name: "blocked: authopts with azure provider and username/password/http", + transport: git.HTTP, + username: "user", + password: "pass", + provider: git.ProviderAzure, + url: "https://url", + expectedError: "basic auth and provider cannot be set at the same time", + }, + { + name: "blocked: authopts with azure provider and http", + provider: git.ProviderAzure, + url: "http://url", + expectedError: "azure provider cannot be used with HTTP", + }, + { + name: "blocked: authopts with github provider and http", + provider: git.ProviderGitHub, + url: "http://url", + expectedError: "github provider cannot be used with HTTP", + }, } for _, tt := range tests { @@ -727,10 +758,11 @@ func TestValidateUrl(t *testing.T) { } ggc, err := NewClient(t.TempDir(), &git.AuthOptions{ - Transport: tt.transport, - Username: tt.username, - Password: tt.password, - BearerToken: tt.bearerToken, + Transport: tt.transport, + Username: tt.username, + Password: tt.password, + BearerToken: tt.bearerToken, + ProviderOpts: &git.ProviderOptions{Name: tt.provider}, }, opts...) g.Expect(err).ToNot(HaveOccurred()) @@ -749,15 +781,14 @@ func TestValidateUrl(t *testing.T) { func TestProviderAuthValidations(t *testing.T) { expiresAt := time.Now().UTC().Add(time.Hour) tests := []struct { - name string - authOpts *git.AuthOptions - proxy transport.ProxyOptions - url string - wantAuthErr error - wantValidationErr error - wantBearerToken string - wantUsername string - wantPassword string + name string + authOpts *git.AuthOptions + proxy transport.ProxyOptions + url string + wantAuthErr error + wantBearerToken string + wantUsername string + wantPassword string }{ { name: "nil authopts", @@ -765,15 +796,18 @@ func TestProviderAuthValidations(t *testing.T) { wantBearerToken: "", }, { - name: "authopts with bearer token and no provider", - url: "https://url", - authOpts: &git.AuthOptions{ - BearerToken: "bearer-token", - }, + name: "authopts with invalid provider", + authOpts: &git.AuthOptions{ProviderOpts: &git.ProviderOptions{Name: "invalid provider"}}, + wantAuthErr: errors.New("invalid provider"), + }, + { + name: "authopts with bearer token and no provider", + url: "https://url", + authOpts: &git.AuthOptions{BearerToken: "bearer-token"}, wantBearerToken: "bearer-token", }, { - name: "authopts with bearer token and provider", + name: "authopts with bearer token and azure provider, bearer token takes precedence", url: "https://url", authOpts: &git.AuthOptions{ BearerToken: "bearer-token", @@ -790,7 +824,7 @@ func TestProviderAuthValidations(t *testing.T) { wantBearerToken: "bearer-token", }, { - name: "authopts with provider and no bearer token", + name: "authopts with azure provider and no bearer token", url: "https://url", authOpts: &git.AuthOptions{ ProviderOpts: &git.ProviderOptions{ @@ -827,7 +861,7 @@ func TestProviderAuthValidations(t *testing.T) { wantBearerToken: "ado-token", }, { - name: "authopts with provider and error", + name: "authopts with azure provider and error", url: "https://url", authOpts: &git.AuthOptions{ ProviderOpts: &git.ProviderOptions{ @@ -842,74 +876,7 @@ func TestProviderAuthValidations(t *testing.T) { wantAuthErr: errors.New("oh no!"), }, { - name: "authopts with invalid provider", - authOpts: &git.AuthOptions{ - ProviderOpts: &git.ProviderOptions{ - Name: "invalid provider", - }, - }, - wantAuthErr: errors.New("invalid provider"), - }, - { - name: "authopts with azure provider and username/password/https", - url: "https://url", - authOpts: &git.AuthOptions{ - ProviderOpts: &git.ProviderOptions{ - Name: git.ProviderAzure, - AzureOpts: []azure.OptFunc{ - azure.WithCredential(&azure.FakeTokenCredential{ - Token: "ado-token", - ExpiresOn: expiresAt, - }), - azure.WithAzureDevOpsScope(), - }, - }, - Username: "user", - Password: "password", - Transport: git.HTTPS, - }, - wantValidationErr: errors.New("basic auth and provider cannot be set at the same time"), - }, - { - name: "authopts with azure provider and username/password/http", - url: "http://url", - authOpts: &git.AuthOptions{ - ProviderOpts: &git.ProviderOptions{ - Name: git.ProviderAzure, - AzureOpts: []azure.OptFunc{ - azure.WithCredential(&azure.FakeTokenCredential{ - Token: "ado-token", - ExpiresOn: expiresAt, - }), - azure.WithAzureDevOpsScope(), - }, - }, - Username: "user", - Password: "password", - Transport: git.HTTP, - }, - wantValidationErr: errors.New("basic auth and provider cannot be set at the same time"), - }, - { - name: "authopts with azure provider and http", - url: "http://url", - authOpts: &git.AuthOptions{ - ProviderOpts: &git.ProviderOptions{ - Name: git.ProviderAzure, - AzureOpts: []azure.OptFunc{ - azure.WithCredential(&azure.FakeTokenCredential{ - Token: "ado-token", - ExpiresOn: expiresAt, - }), - azure.WithAzureDevOpsScope(), - }, - }, - Transport: git.HTTP, - }, - wantValidationErr: errors.New("azure provider cannot be used with HTTP"), - }, - { - name: "authopts with github provider and username/password/https", + name: "authopts with github provider and username/password/https, username/password takes precedence", url: "https://url", authOpts: &git.AuthOptions{ ProviderOpts: &git.ProviderOptions{ @@ -923,18 +890,6 @@ func TestProviderAuthValidations(t *testing.T) { wantUsername: "user", wantPassword: "password", }, - { - name: "authopts with github provider and http", - url: "http://url", - authOpts: &git.AuthOptions{ - ProviderOpts: &git.ProviderOptions{ - Name: git.ProviderGitHub, - GitHubOpts: []github.OptFunc{}, - }, - Transport: git.HTTP, - }, - wantValidationErr: errors.New("github provider cannot be used with HTTP"), - }, } for _, tt := range tests { @@ -946,27 +901,20 @@ func TestProviderAuthValidations(t *testing.T) { ggc, err := NewClient(t.TempDir(), tt.authOpts, opts...) g.Expect(err).ToNot(HaveOccurred()) - err = ggc.validateUrlAndAuthOptions(tt.url) - if tt.wantValidationErr != nil { + err = ggc.providerAuth(context.TODO()) + if tt.wantAuthErr != nil { g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(Equal(tt.wantValidationErr)) + g.Expect(err).To(Equal(tt.wantAuthErr)) } else { g.Expect(err).ToNot(HaveOccurred()) - err = ggc.providerAuth(context.TODO()) - if tt.wantAuthErr != nil { - g.Expect(err).To(HaveOccurred()) - g.Expect(err).To(Equal(tt.wantAuthErr)) - } else { - g.Expect(err).ToNot(HaveOccurred()) - if tt.wantBearerToken != "" { - g.Expect(tt.authOpts.BearerToken).To(Equal(tt.wantBearerToken)) - } - if tt.wantUsername != "" { - g.Expect(tt.authOpts.Username).To(Equal(tt.wantUsername)) - } - if tt.wantPassword != "" { - g.Expect(tt.authOpts.Password).To(Equal(tt.authOpts.Password)) - } + if tt.wantBearerToken != "" { + g.Expect(tt.authOpts.BearerToken).To(Equal(tt.wantBearerToken)) + } + if tt.wantUsername != "" { + g.Expect(tt.authOpts.Username).To(Equal(tt.wantUsername)) + } + if tt.wantPassword != "" { + g.Expect(tt.authOpts.Password).To(Equal(tt.authOpts.Password)) } } })