[pull] trunk from cli:trunk

acceptance/acceptance_test.go

@@ -4,6 +4,9 @@ package acceptance_test
 
 import (
 	"bytes"
+	"crypto/ed25519"
+	cryptorand "crypto/rand"
+	"errors"
 	"fmt"
 	"os"
 	"path"
@@ -20,6 +23,9 @@ import (
 	"github.com/cli/cli/v2/internal/ghcmd"
 	"github.com/cli/go-gh/v2/pkg/jq"
 	"github.com/cli/go-internal/testscript"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.org/x/crypto/ssh"
 )
 
 func ghMain() int {
@@ -32,6 +38,35 @@ func TestMain(m *testing.M) {
 	}))
 }
 
+func TestGenerateSSHPublicKey(t *testing.T) {
+	first, err := generateSSHPublicKey("myTitle")
+	require.NoError(t, err)
+	second, err := generateSSHPublicKey("myTitle")
+	require.NoError(t, err)
+
+	publicKey, comment, options, rest, err := ssh.ParseAuthorizedKey(first)
+	require.NoError(t, err)
+	assert.Equal(t, ssh.KeyAlgoED25519, publicKey.Type())
+	assert.Equal(t, "myTitle", comment)
+	assert.Empty(t, options)
+	assert.Empty(t, rest)
+	assert.NotEqual(t, first, second)
+}
+
+func TestSandboxFilePath(t *testing.T) {
+	root := t.TempDir()
+
+	path, err := sandboxFilePath(root, root, "keys/deploy.pub")
+	require.NoError(t, err)
+	assert.Equal(t, filepath.Join(root, "keys/deploy.pub"), path)
+
+	_, err = sandboxFilePath(root, root, filepath.Join(root, "deploy.pub"))
+	assert.EqualError(t, err, "path must be relative to the testscript sandbox")
+
+	_, err = sandboxFilePath(root, root, "../deploy.pub")
+	assert.EqualError(t, err, "path must stay within the testscript sandbox")
+}
+
 func TestAPI(t *testing.T) {
 	var tsEnv testScriptEnv
 	if err := tsEnv.fromEnv(); err != nil {
@@ -322,6 +357,24 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript,
 				ts.Setenv(env[:i], strings.ToUpper(env[i+1:]))
 			}
 		},
+		"generate-ssh-key": func(ts *testscript.TestScript, neg bool, args []string) {
+			if neg {
+				ts.Fatalf("unsupported: ! generate-ssh-key")
+			}
+			if len(args) < 1 || len(args) > 2 {
+				ts.Fatalf("usage: generate-ssh-key file [comment]")
+			}
+
+			comment := ""
+			if len(args) == 2 {
+				comment = args[1]
+			}
+			publicKey, err := generateSSHPublicKey(comment)
+			ts.Check(err)
+			outputPath, err := sandboxFilePath(ts.Getenv("WORK"), ts.MkAbs("."), args[0])
+			ts.Check(err)
+			ts.Check(os.WriteFile(outputPath, publicKey, 0o644))
+		},
 		"replace": func(ts *testscript.TestScript, neg bool, args []string) {
 			if neg {
 				ts.Fatalf("unsupported: ! replace")
@@ -440,6 +493,41 @@ func sharedCmds(tsEnv testScriptEnv) map[string]func(ts *testscript.TestScript,
 	}
 }
 
+func generateSSHPublicKey(comment string) ([]byte, error) {
+	publicKey, _, err := ed25519.GenerateKey(cryptorand.Reader)
+	if err != nil {
+		return nil, err
+	}
+
+	sshPublicKey, err := ssh.NewPublicKey(publicKey)
+	if err != nil {
+		return nil, err
+	}
+
+	authorizedKey := bytes.TrimSpace(ssh.MarshalAuthorizedKey(sshPublicKey))
+	if comment != "" {
+		authorizedKey = append(authorizedKey, ' ')
+		authorizedKey = append(authorizedKey, comment...)
+	}
+	return append(authorizedKey, '\n'), nil
+}
+
+func sandboxFilePath(root, currentDir, name string) (string, error) {
+	if filepath.IsAbs(name) {
+		return "", errors.New("path must be relative to the testscript sandbox")
+	}
+
+	outputPath := filepath.Clean(filepath.Join(currentDir, name))
+	relativePath, err := filepath.Rel(root, outputPath)
+	if err != nil {
+		return "", err
+	}
+	if relativePath == ".." || strings.HasPrefix(relativePath, ".."+string(filepath.Separator)) {
+		return "", errors.New("path must stay within the testscript sandbox")
+	}
+	return outputPath, nil
+}
+
 var letters = []rune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ")
 
 func randomString(n int) string {

acceptance/testdata/repo/repo-deploy-key.txtar

@@ -4,6 +4,9 @@ exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private --cl
 # Defer repo cleanup
 defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING
 
+# Generate a globally unique deploy key
+generate-ssh-key deployKey.pub myTitle
+
 # cd to the repo and list the deploy keys. There should be no keys
 cd $SCRIPT_NAME-$RANDOM_STRING
 exec gh repo deploy-key list --json=title
@@ -26,6 +29,3 @@ exec gh repo deploy-key delete $DEPLOY_KEY_ID
 # Ensure the deploy key was deleted
 exec gh repo deploy-key list --json=id --jq='.[].id'
 ! stdout $DEPLOY_KEY_ID
-
--- deployKey.pub --
-ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAZmdeRNskfpvYL5YHB/YJaW8hTEXpnvPMkx5Ri+YwUr myTitle

acceptance/testdata/ssh-key/ssh-key.txtar

@@ -2,6 +2,9 @@ skip 'it modifies the user''s personal GitHub account SSH keys'
 
 # scopes admin:ssh_signing_key,admin:public_key
 
+# Generate a globally unique account SSH key
+generate-ssh-key sshKey.pub acceptance
+
 # Add an SSH key to the account
 exec gh ssh-key add sshKey.pub --title 'acceptance-test-key'
 
@@ -19,6 +22,3 @@ exec gh ssh-key delete --yes ${SSH_KEY_ID}
 # Check the key is deleted
 exec gh ssh-key list
 ! stdout 'acceptance-test-key'
-
--- sshKey.pub --
-ssh-ed25519 AAAAC3NzaC1lZDI1NTE5AAAAIAZmdeRNskfpvYL5YHB/YJaW8hTEXpnvPMkx5Ri+YwUr acceptance

pkg/cmd/gpg-key/add/add_test.go

@@ -2,16 +2,93 @@ package add
 
 import (
 	"net/http"
+	"strings"
 	"testing"
 
 	"github.com/MakeNowJust/heredoc"
+	"github.com/cli/cli/v2/api"
 	"github.com/cli/cli/v2/internal/config"
 	"github.com/cli/cli/v2/internal/gh"
 	"github.com/cli/cli/v2/pkg/httpmock"
 	"github.com/cli/cli/v2/pkg/iostreams"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func Test_gpgKeyUploadScopesMissing(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+	reg.Register(
+		httpmock.REST("POST", "user/gpg_keys"),
+		httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`),
+	)
+
+	err := gpgKeyUpload(&http.Client{Transport: reg}, "github.com", strings.NewReader("-----BEGIN PGP PUBLIC KEY BLOCK-----"), "")
+
+	require.Same(t, errScopesMissing, err)
+}
+
+func Test_gpgKeyUploadDuplicateKeyBeforeWrongFormat(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+	reg.Register(
+		httpmock.REST("POST", "user/gpg_keys"),
+		httpmock.WithHeader(httpmock.StatusStringResponse(http.StatusUnprocessableEntity, `{
+			"message": "Validation Failed",
+			"errors": [{
+				"resource": "GpgKey",
+				"code": "custom",
+				"field": "key_id",
+				"message": "key_id already exists"
+			}]
+		}`), "Content-Type", "application/json"),
+	)
+
+	err := gpgKeyUpload(&http.Client{Transport: reg}, "github.com", strings.NewReader("binary-key"), "")
+
+	require.Same(t, errDuplicateKey, err)
+}
+
+func Test_gpgKeyUploadWrongFormat(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+	reg.Register(
+		httpmock.REST("POST", "user/gpg_keys"),
+		httpmock.StatusStringResponse(http.StatusUnprocessableEntity, `{
+			"message": "Validation Failed",
+			"errors": [{
+				"resource": "GpgKey",
+				"code": "custom",
+				"message": "We got an error doing that."
+			}]
+		}`),
+	)
+
+	err := gpgKeyUpload(&http.Client{Transport: reg}, "github.com", strings.NewReader("binary-key"), "")
+
+	require.Same(t, errWrongFormat, err)
+}
+
+func Test_gpgKeyUploadHTTPError(t *testing.T) {
+	reg := &httpmock.Registry{}
+	defer reg.Verify(t)
+	reg.Register(
+		httpmock.REST("POST", "user/gpg_keys"),
+		httpmock.WithHeader(
+			httpmock.StatusStringResponse(http.StatusInternalServerError, `{"message":"Internal Server Error"}`),
+			"Content-Type", "application/json",
+		),
+	)
+
+	err := gpgKeyUpload(&http.Client{Transport: reg}, "github.com", strings.NewReader("-----BEGIN PGP PUBLIC KEY BLOCK-----"), "")
+
+	var httpErr api.HTTPError
+	require.ErrorAs(t, err, &httpErr)
+	assert.Equal(t, http.StatusInternalServerError, httpErr.StatusCode)
+	assert.Contains(t, err.Error(), "HTTP 500")
+	assert.EqualError(t, err, "HTTP 500: Internal Server Error (https://api.github.com/user/gpg_keys)")
+}
+
 func Test_runAdd(t *testing.T) {
 	tests := []struct {
 		name       string
@@ -28,7 +105,7 @@ func Test_runAdd(t *testing.T) {
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
 					httpmock.REST("POST", "user/gpg_keys"),
-					httpmock.RESTPayload(200, ``, func(payload map[string]interface{}) {
+					httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) {
 						assert.Contains(t, payload, "armored_public_key")
 						assert.NotContains(t, payload, "title")
 					}))
@@ -44,7 +121,7 @@ func Test_runAdd(t *testing.T) {
 			httpStubs: func(reg *httpmock.Registry) {
 				reg.Register(
 					httpmock.REST("POST", "user/gpg_keys"),
-					httpmock.RESTPayload(200, ``, func(payload map[string]interface{}) {
+					httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) {
 						assert.Contains(t, payload, "armored_public_key")
 						assert.Contains(t, payload, "name")
 					}))

pkg/cmd/gpg-key/add/http.go

@@ -8,7 +8,6 @@ import (
 	"net/http"
 
 	"github.com/cli/cli/v2/api"
-	"github.com/cli/cli/v2/internal/ghinstance"
 	"github.com/cli/cli/v2/internal/safeurl"
 )
 
@@ -17,11 +16,6 @@ var errDuplicateKey = errors.New("key already exists")
 var errWrongFormat = errors.New("key in wrong format")
 
 func gpgKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, title string) error {
-	u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "user", "gpg_keys")
-	if err != nil {
-		return err
-	}
-
 	keyBytes, err := io.ReadAll(keyFile)
 	if err != nil {
 		return err
@@ -39,36 +33,33 @@ func gpgKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, t
 		return err
 	}
 
-	req, err := http.NewRequest("POST", u.String(), bytes.NewBuffer(payloadBytes))
+	path, err := safeurl.JoinPath("user", "gpg_keys")
 	if err != nil {
 		return err
 	}
 
-	resp, err := httpClient.Do(req)
+	// TODO(api-client-rollout)
+	// This line of code is part of a mechanical roll out of the api client.
+	// As a follow up, consider whether the api client can be injected to this call site, rather than constructed
+	apiClient := api.NewClientFromHTTP(httpClient)
+	err = apiClient.REST(hostname, "POST", path.String(), bytes.NewBuffer(payloadBytes), nil)
 	if err != nil {
-		return err
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode == 404 {
-		return errScopesMissing
-	} else if resp.StatusCode > 299 {
-		err := api.HandleHTTPError(resp)
-		var httpError api.HTTPError
-		if errors.As(err, &httpError) {
+		if httpError, ok := errors.AsType[api.HTTPError](err); ok {
+			if httpError.StatusCode == 404 {
+				return errScopesMissing
+			}
 			for _, e := range httpError.Errors {
-				if resp.StatusCode == 422 && e.Field == "key_id" && e.Message == "key_id already exists" {
+				if httpError.StatusCode == 422 && e.Field == "key_id" && e.Message == "key_id already exists" {
 					return errDuplicateKey
 				}
 			}
-		}
-		if resp.StatusCode == 422 && !isGpgKeyArmored(keyBytes) {
-			return errWrongFormat
+			if httpError.StatusCode == 422 && !isGpgKeyArmored(keyBytes) {
+				return errWrongFormat
+			}
 		}
 		return err
 	}
 
-	_, _ = io.Copy(io.Discard, resp.Body)
 	return nil
 }