review comments

Signed-off-by: Dipti Pai <diptipai89@outlook.com>
fluxcd · Dec 4, 2024 · 124369b · 124369b
1 parent c698941
commit 124369b
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 149 deletions.
diff --git 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
@@ -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
@@ -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)