From 2556daca81ff55b9ea352ff62f50666f37d53035 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 28 Jul 2026 12:53:33 +0200 Subject: [PATCH 1/5] Route deploy key requests through api.Client Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: df808792-75d4-4988-8ec0-242f20bad526 --- pkg/cmd/repo/deploy-key/add/add_test.go | 27 +++++++++++++++++ pkg/cmd/repo/deploy-key/add/http.go | 28 ++++------------- pkg/cmd/repo/deploy-key/delete/delete_test.go | 23 ++++++++++++++ pkg/cmd/repo/deploy-key/delete/http.go | 30 ++++--------------- pkg/cmd/repo/deploy-key/list/http.go | 29 ++++-------------- pkg/cmd/repo/deploy-key/list/list_test.go | 23 ++++++++++++++ 6 files changed, 88 insertions(+), 72 deletions(-) diff --git a/pkg/cmd/repo/deploy-key/add/add_test.go b/pkg/cmd/repo/deploy-key/add/add_test.go index d4b5c758f27..8eda3e5b732 100644 --- a/pkg/cmd/repo/deploy-key/add/add_test.go +++ b/pkg/cmd/repo/deploy-key/add/add_test.go @@ -2,11 +2,15 @@ package add import ( "net/http" + "strings" "testing" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "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_addRun(t *testing.T) { @@ -83,3 +87,26 @@ func Test_addRun(t *testing.T) { }) } } + +func TestUploadDeployKeyHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("POST", "repos/OWNER/REPO/keys"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + err := uploadDeployKey( + &http.Client{Transport: reg}, + ghrepo.New("OWNER", "REPO"), + strings.NewReader("PUBKEY\n"), + "my sacred key", + false, + ) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} diff --git a/pkg/cmd/repo/deploy-key/add/http.go b/pkg/cmd/repo/deploy-key/add/http.go index 5111049c8ed..c8134d965e4 100644 --- a/pkg/cmd/repo/deploy-key/add/http.go +++ b/pkg/cmd/repo/deploy-key/add/http.go @@ -7,13 +7,12 @@ import ( "net/http" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) func uploadDeployKey(httpClient *http.Client, repo ghrepo.Interface, keyFile io.Reader, title string, isWritable bool) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "keys") + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "keys") if err != nil { return err } @@ -34,25 +33,8 @@ func uploadDeployKey(httpClient *http.Client, repo ghrepo.Interface, keyFile io. return err } - req, err := http.NewRequest("POST", url.String(), bytes.NewBuffer(payloadBytes)) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return err - } - - return nil + // 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 + return api.NewClientFromHTTP(httpClient).REST(repo.RepoHost(), "POST", path.String(), bytes.NewBuffer(payloadBytes), nil) } diff --git a/pkg/cmd/repo/deploy-key/delete/delete_test.go b/pkg/cmd/repo/deploy-key/delete/delete_test.go index dd5b6acf4d9..30a6e83a249 100644 --- a/pkg/cmd/repo/deploy-key/delete/delete_test.go +++ b/pkg/cmd/repo/deploy-key/delete/delete_test.go @@ -4,10 +4,12 @@ import ( "net/http" "testing" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "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_deleteRun(t *testing.T) { @@ -38,3 +40,24 @@ func Test_deleteRun(t *testing.T) { assert.Equal(t, "", stderr.String()) assert.Equal(t, "✓ Deploy key deleted from OWNER/REPO\n", stdout.String()) } + +func TestDeleteDeployKeyHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/keys/1234"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + err := deleteDeployKey( + &http.Client{Transport: reg}, + ghrepo.New("OWNER", "REPO"), + "1234", + ) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} diff --git a/pkg/cmd/repo/deploy-key/delete/http.go b/pkg/cmd/repo/deploy-key/delete/http.go index 117ce697a29..e88e4c28123 100644 --- a/pkg/cmd/repo/deploy-key/delete/http.go +++ b/pkg/cmd/repo/deploy-key/delete/http.go @@ -1,40 +1,20 @@ package delete import ( - "io" "net/http" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) func deleteDeployKey(httpClient *http.Client, repo ghrepo.Interface, id string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "keys", id) + path, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "keys", id) if err != nil { return err } - - req, err := http.NewRequest("DELETE", url.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return err - } - - return nil + // 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 + return api.NewClientFromHTTP(httpClient).REST(repo.RepoHost(), "DELETE", path.String(), nil, nil) } diff --git a/pkg/cmd/repo/deploy-key/list/http.go b/pkg/cmd/repo/deploy-key/list/http.go index 391d6bbe17d..02f45eb66ac 100644 --- a/pkg/cmd/repo/deploy-key/list/http.go +++ b/pkg/cmd/repo/deploy-key/list/http.go @@ -1,13 +1,10 @@ package list import ( - "encoding/json" - "io" "net/http" "time" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/safeurl" ) @@ -21,33 +18,17 @@ type deployKey struct { } func repoKeys(httpClient *http.Client, repo ghrepo.Interface) ([]deployKey, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(repo.RepoHost()), "repos", repo.RepoOwner(), repo.RepoName(), "keys") + u, err := safeurl.JoinPath("repos", repo.RepoOwner(), repo.RepoName(), "keys") if err != nil { return nil, err } u.SetQuery("per_page", "100") - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } var keys []deployKey - err = json.Unmarshal(b, &keys) + // 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 + err = api.NewClientFromHTTP(httpClient).REST(repo.RepoHost(), "GET", u.String(), nil, &keys) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/deploy-key/list/list_test.go b/pkg/cmd/repo/deploy-key/list/list_test.go index 0f6977db7a1..50298f44980 100644 --- a/pkg/cmd/repo/deploy-key/list/list_test.go +++ b/pkg/cmd/repo/deploy-key/list/list_test.go @@ -7,9 +7,12 @@ import ( "time" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/ghrepo" "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 TestListRun(t *testing.T) { @@ -131,3 +134,23 @@ func TestListRun(t *testing.T) { }) } } + +func TestRepoKeysHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/keys"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + _, err := repoKeys( + &http.Client{Transport: reg}, + ghrepo.New("OWNER", "REPO"), + ) + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} From 4d9aefa98722601bf7fc9e36587fe8ff60307ddb Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 28 Jul 2026 15:34:04 +0200 Subject: [PATCH 2/5] Generate unique acceptance SSH keys Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: df808792-75d4-4988-8ec0-242f20bad526 --- acceptance/acceptance_test.go | 88 +++++++++++++++++++ .../testdata/repo/repo-deploy-key.txtar | 6 +- 2 files changed, 91 insertions(+), 3 deletions(-) diff --git a/acceptance/acceptance_test.go b/acceptance/acceptance_test.go index ac7fa8181e0..9030e050611 100644 --- a/acceptance/acceptance_test.go +++ b/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 { diff --git a/acceptance/testdata/repo/repo-deploy-key.txtar b/acceptance/testdata/repo/repo-deploy-key.txtar index d93d07ee5d2..5a1151d7d00 100644 --- a/acceptance/testdata/repo/repo-deploy-key.txtar +++ b/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 From c437c9d9a8088f114b19e87772a288989cce7fe1 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 28 Jul 2026 13:38:01 +0200 Subject: [PATCH 3/5] Route ssh key requests through api.Client Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7ea49581-d435-4617-8830-5f40a036754c --- pkg/cmd/ssh-key/add/add_test.go | 33 ++++++++++++++- pkg/cmd/ssh-key/add/http.go | 36 ++++------------ pkg/cmd/ssh-key/delete/delete_test.go | 35 +++++++++++++++ pkg/cmd/ssh-key/delete/http.go | 54 +++++------------------- pkg/cmd/ssh-key/shared/user_keys.go | 42 +++++------------- pkg/cmd/ssh-key/shared/user_keys_test.go | 28 ++++++++++++ 6 files changed, 125 insertions(+), 103 deletions(-) create mode 100644 pkg/cmd/ssh-key/shared/user_keys_test.go diff --git a/pkg/cmd/ssh-key/add/add_test.go b/pkg/cmd/ssh-key/add/add_test.go index 6d30b6d0d83..d167d2fe6e9 100644 --- a/pkg/cmd/ssh-key/add/add_test.go +++ b/pkg/cmd/ssh-key/add/add_test.go @@ -2,13 +2,16 @@ package add import ( "net/http" + "strings" "testing" + "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_runAdd(t *testing.T) { @@ -30,7 +33,7 @@ func Test_runAdd(t *testing.T) { httpmock.StringResponse("[]")) reg.Register( httpmock.REST("POST", "user/keys"), - httpmock.RESTPayload(200, ``, func(payload map[string]interface{}) { + httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { assert.Contains(t, payload, "key") assert.Empty(t, payload["title"]) })) @@ -49,7 +52,7 @@ func Test_runAdd(t *testing.T) { httpmock.StringResponse("[]")) reg.Register( httpmock.REST("POST", "user/ssh_signing_keys"), - httpmock.RESTPayload(200, ``, func(payload map[string]interface{}) { + httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { assert.Contains(t, payload, "key") assert.Empty(t, payload["title"]) })) @@ -149,3 +152,29 @@ func Test_runAdd(t *testing.T) { }) } } + +func TestSSHKeyUploadHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "user/keys"), + httpmock.StringResponse("[]"), + ) + reg.Register( + httpmock.REST("POST", "user/keys"), + httpmock.StatusStringResponse(http.StatusUnprocessableEntity, `{"message":"Validation Failed"}`), + ) + + uploaded, err := SSHKeyUpload( + &http.Client{Transport: reg}, + "github.com", + strings.NewReader("ssh-ed25519 asdf"), + "", + ) + + assert.False(t, uploaded) + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusUnprocessableEntity, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 422") +} diff --git a/pkg/cmd/ssh-key/add/http.go b/pkg/cmd/ssh-key/add/http.go index 1efe1d34197..084a77e43cf 100644 --- a/pkg/cmd/ssh-key/add/http.go +++ b/pkg/cmd/ssh-key/add/http.go @@ -9,14 +9,13 @@ import ( "strings" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" "github.com/cli/cli/v2/pkg/cmd/ssh-key/shared" ) // Uploads the provided SSH key. Returns true if the key was uploaded, false if it was not. func SSHKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, title string) (bool, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "user", "keys") + u, err := safeurl.JoinPath("user", "keys") if err != nil { return false, err } @@ -50,7 +49,7 @@ func SSHKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, t "key": fullUserKey, } - err = keyUpload(httpClient, u, payload) + err = keyUpload(httpClient, hostname, u, payload) if err != nil { return false, err @@ -61,7 +60,7 @@ func SSHKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, t // Uploads the provided SSH Signing key. Returns true if the key was uploaded, false if it was not. func SSHSigningKeyUpload(httpClient *http.Client, hostname string, keyFile io.Reader, title string) (bool, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(hostname), "user", "ssh_signing_keys") + u, err := safeurl.JoinPath("user", "ssh_signing_keys") if err != nil { return false, err } @@ -95,7 +94,7 @@ func SSHSigningKeyUpload(httpClient *http.Client, hostname string, keyFile io.Re "key": fullUserKey, } - err = keyUpload(httpClient, u, payload) + err = keyUpload(httpClient, hostname, u, payload) if err != nil { return false, err @@ -104,31 +103,14 @@ func SSHSigningKeyUpload(httpClient *http.Client, hostname string, keyFile io.Re return true, nil } -func keyUpload(httpClient *http.Client, u safeurl.SafeURL, payload map[string]string) error { +func keyUpload(httpClient *http.Client, hostname string, u safeurl.SafeURL, payload map[string]string) error { payloadBytes, err := json.Marshal(payload) if err != nil { return err } - req, err := http.NewRequest("POST", u.String(), bytes.NewBuffer(payloadBytes)) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - - _, err = io.Copy(io.Discard, resp.Body) - if err != nil { - return err - } - - return nil + // 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 + return api.NewClientFromHTTP(httpClient).REST(hostname, http.MethodPost, u.String(), bytes.NewBuffer(payloadBytes), nil) } diff --git a/pkg/cmd/ssh-key/delete/delete_test.go b/pkg/cmd/ssh-key/delete/delete_test.go index be2917c824f..8ec39f68449 100644 --- a/pkg/cmd/ssh-key/delete/delete_test.go +++ b/pkg/cmd/ssh-key/delete/delete_test.go @@ -5,6 +5,7 @@ import ( "net/http" "testing" + "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/internal/prompter" @@ -13,6 +14,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdDelete(t *testing.T) { @@ -209,3 +211,36 @@ func Test_deleteRun(t *testing.T) { }) } } + +func TestDeleteSSHKeyHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("DELETE", "user/keys/1234"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + err := deleteSSHKey(&http.Client{Transport: reg}, "github.com", "1234") + + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} + +func TestGetSSHKeyHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "user/keys/1234"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + key, err := getSSHKey(&http.Client{Transport: reg}, "github.com", "1234") + + assert.Nil(t, key) + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} diff --git a/pkg/cmd/ssh-key/delete/http.go b/pkg/cmd/ssh-key/delete/http.go index a23502e6489..f713d9b4c3e 100644 --- a/pkg/cmd/ssh-key/delete/http.go +++ b/pkg/cmd/ssh-key/delete/http.go @@ -1,12 +1,9 @@ package delete import ( - "encoding/json" - "io" "net/http" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -15,55 +12,26 @@ type sshKey struct { } func deleteSSHKey(httpClient *http.Client, host string, keyID string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "keys", keyID) + path, err := safeurl.JoinPath("user", "keys", keyID) if err != nil { return err } - req, err := http.NewRequest("DELETE", url.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - - return nil + // 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 + return api.NewClientFromHTTP(httpClient).REST(host, http.MethodDelete, path.String(), nil, nil) } func getSSHKey(httpClient *http.Client, host string, keyID string) (*sshKey, error) { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "keys", keyID) - if err != nil { - return nil, err - } - req, err := http.NewRequest("GET", url.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) + var key sshKey + path, err := safeurl.JoinPath("user", "keys", keyID) if err != nil { return nil, err } - - var key sshKey - err = json.Unmarshal(b, &key) + // 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 + err = api.NewClientFromHTTP(httpClient).REST(host, http.MethodGet, path.String(), nil, &key) if err != nil { return nil, err } diff --git a/pkg/cmd/ssh-key/shared/user_keys.go b/pkg/cmd/ssh-key/shared/user_keys.go index 4f1553afba8..8cc5a93fdc6 100644 --- a/pkg/cmd/ssh-key/shared/user_keys.go +++ b/pkg/cmd/ssh-key/shared/user_keys.go @@ -1,13 +1,10 @@ package shared import ( - "encoding/json" - "io" "net/http" "time" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -25,19 +22,19 @@ type sshKey struct { } func UserKeys(httpClient *http.Client, host, userHandle string) ([]sshKey, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "keys") + u, err := safeurl.JoinPath("user", "keys") if err != nil { return nil, err } if userHandle != "" { - u, err = safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "users", userHandle, "keys") + u, err = safeurl.JoinPath("users", userHandle, "keys") if err != nil { return nil, err } } u.SetQuery("per_page", "100") - keys, err := getUserKeys(httpClient, u) + keys, err := getUserKeys(httpClient, host, u) if err != nil { return nil, err @@ -51,19 +48,19 @@ func UserKeys(httpClient *http.Client, host, userHandle string) ([]sshKey, error } func UserSigningKeys(httpClient *http.Client, host, userHandle string) ([]sshKey, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "ssh_signing_keys") + u, err := safeurl.JoinPath("user", "ssh_signing_keys") if err != nil { return nil, err } if userHandle != "" { - u, err = safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "users", userHandle, "ssh_signing_keys") + u, err = safeurl.JoinPath("users", userHandle, "ssh_signing_keys") if err != nil { return nil, err } } u.SetQuery("per_page", "100") - keys, err := getUserKeys(httpClient, u) + keys, err := getUserKeys(httpClient, host, u) if err != nil { return nil, err @@ -76,29 +73,12 @@ func UserSigningKeys(httpClient *http.Client, host, userHandle string) ([]sshKey return keys, nil } -func getUserKeys(httpClient *http.Client, u safeurl.SafeURL) ([]sshKey, error) { - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - +func getUserKeys(httpClient *http.Client, hostname string, u safeurl.SafeURL) ([]sshKey, error) { var keys []sshKey - err = json.Unmarshal(b, &keys) + // 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 + err := api.NewClientFromHTTP(httpClient).REST(hostname, http.MethodGet, u.String(), nil, &keys) if err != nil { return nil, err } diff --git a/pkg/cmd/ssh-key/shared/user_keys_test.go b/pkg/cmd/ssh-key/shared/user_keys_test.go new file mode 100644 index 00000000000..3a3142a8721 --- /dev/null +++ b/pkg/cmd/ssh-key/shared/user_keys_test.go @@ -0,0 +1,28 @@ +package shared + +import ( + "net/http" + "testing" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestUserKeysHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("GET", "user/keys"), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + keys, err := UserKeys(&http.Client{Transport: reg}, "github.com", "") + + assert.Nil(t, keys) + var httpErr api.HTTPError + require.ErrorAs(t, err, &httpErr) + assert.Equal(t, http.StatusNotFound, httpErr.StatusCode) + assert.Contains(t, err.Error(), "HTTP 404") +} From 29a4d8b176ebf5d0ce8408f7de1052b5a582d801 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 28 Jul 2026 15:53:55 +0200 Subject: [PATCH 4/5] Use generated key in ssh-key acceptance test Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: d5151527-3286-43fd-a420-38f496a076fa --- acceptance/testdata/ssh-key/ssh-key.txtar | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/acceptance/testdata/ssh-key/ssh-key.txtar b/acceptance/testdata/ssh-key/ssh-key.txtar index 4ba8643bb33..62d04b02711 100644 --- a/acceptance/testdata/ssh-key/ssh-key.txtar +++ b/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 From a3ed50ad5fafddd0e15ab8838c2136c8d25572df Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 4 Aug 2026 11:01:26 +0200 Subject: [PATCH 5/5] Route gpg key requests through api.Client (#13997) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 605ad5d5-43b6-4332-b94b-6ae9b69bb3ff --- pkg/cmd/gpg-key/add/add_test.go | 81 ++++++++++++++++++++++++++- pkg/cmd/gpg-key/add/http.go | 37 +++++------- pkg/cmd/gpg-key/delete/delete_test.go | 48 +++++++++++++++- pkg/cmd/gpg-key/delete/http.go | 52 ++++------------- pkg/cmd/gpg-key/list/http.go | 36 +++--------- pkg/cmd/gpg-key/list/list_test.go | 53 ++++++++++++++++++ 6 files changed, 211 insertions(+), 96 deletions(-) diff --git a/pkg/cmd/gpg-key/add/add_test.go b/pkg/cmd/gpg-key/add/add_test.go index c6d7c18fbe5..45119bdfda3 100644 --- a/pkg/cmd/gpg-key/add/add_test.go +++ b/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") })) diff --git a/pkg/cmd/gpg-key/add/http.go b/pkg/cmd/gpg-key/add/http.go index b1f0fca74cd..41b220a4d57 100644 --- a/pkg/cmd/gpg-key/add/http.go +++ b/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 } diff --git a/pkg/cmd/gpg-key/delete/delete_test.go b/pkg/cmd/gpg-key/delete/delete_test.go index dc730b100ed..ef3b36f64ca 100644 --- a/pkg/cmd/gpg-key/delete/delete_test.go +++ b/pkg/cmd/gpg-key/delete/delete_test.go @@ -3,19 +3,63 @@ package delete import ( "bytes" "net/http" + "net/url" "testing" + "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/internal/prompter" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/cli/go-gh/v2/pkg/api" + ghAPI "github.com/cli/go-gh/v2/pkg/api" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +func Test_deleteGPGKeyHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.REST("DELETE", "user/gpg_keys/123"), + httpmock.WithHeader( + httpmock.StatusStringResponse(http.StatusInternalServerError, `{"message":"Internal Server Error"}`), + "Content-Type", "application/json", + ), + ) + + err := deleteGPGKey(&http.Client{Transport: reg}, "github.com", "123") + + 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/123)") +} + +func Test_getGPGKeysHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "user/gpg_keys", url.Values{"per_page": []string{"100"}}), + httpmock.WithHeader( + httpmock.StatusStringResponse(http.StatusInternalServerError, `{"message":"Internal Server Error"}`), + "Content-Type", "application/json", + ), + ) + + keys, err := getGPGKeys(&http.Client{Transport: reg}, "github.com") + + assert.Nil(t, keys) + 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?per_page=100)") +} + func TestNewCmdDelete(t *testing.T) { tests := []struct { name string @@ -177,7 +221,7 @@ func Test_deleteRun(t *testing.T) { opts: DeleteOptions{KeyID: "ABC123", Confirmed: true}, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "user/gpg_keys"), httpmock.StatusStringResponse(200, keysResp)) - reg.Register(httpmock.REST("DELETE", "user/gpg_keys/123"), httpmock.JSONErrorResponse(404, api.HTTPError{ + reg.Register(httpmock.REST("DELETE", "user/gpg_keys/123"), httpmock.JSONErrorResponse(404, ghAPI.HTTPError{ StatusCode: 404, Message: "GPG key 123 not found", })) diff --git a/pkg/cmd/gpg-key/delete/http.go b/pkg/cmd/gpg-key/delete/http.go index 9b6c2a46eae..af4f15a4557 100644 --- a/pkg/cmd/gpg-key/delete/http.go +++ b/pkg/cmd/gpg-key/delete/http.go @@ -1,12 +1,9 @@ package delete import ( - "encoding/json" - "io" "net/http" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -16,59 +13,30 @@ type gpgKey struct { } func deleteGPGKey(httpClient *http.Client, host, id string) error { - url, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "gpg_keys", id) + path, err := safeurl.JoinPath("user", "gpg_keys", id) if err != nil { return err } - req, err := http.NewRequest("DELETE", url.String(), nil) - if err != nil { - return err - } - - resp, err := httpClient.Do(req) - if err != nil { - return err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return api.HandleHTTPError(resp) - } - - return nil + // 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 + return api.NewClientFromHTTP(httpClient).REST(host, "DELETE", path.String(), nil, nil) } func getGPGKeys(httpClient *http.Client, host string) ([]gpgKey, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "gpg_keys") + u, err := safeurl.JoinPath("user", "gpg_keys") if err != nil { return nil, err } u.SetQuery("per_page", "100") - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } var keys []gpgKey - err = json.Unmarshal(b, &keys) + // 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 + err = api.NewClientFromHTTP(httpClient).REST(host, "GET", u.String(), nil, &keys) if err != nil { return nil, err } - return keys, nil } diff --git a/pkg/cmd/gpg-key/list/http.go b/pkg/cmd/gpg-key/list/http.go index 1b00684590e..8a282711025 100644 --- a/pkg/cmd/gpg-key/list/http.go +++ b/pkg/cmd/gpg-key/list/http.go @@ -1,15 +1,12 @@ package list import ( - "encoding/json" "errors" - "io" "net/http" "strings" "time" "github.com/cli/cli/v2/api" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/safeurl" ) @@ -38,42 +35,27 @@ type gpgKey struct { } func userKeys(httpClient *http.Client, host, userHandle string) ([]gpgKey, error) { - u, err := safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "user", "gpg_keys") + u, err := safeurl.JoinPath("user", "gpg_keys") if err != nil { return nil, err } if userHandle != "" { - u, err = safeurl.JoinPathWithHostPrefix(ghinstance.RESTPrefix(host), "users", userHandle, "gpg_keys") + u, err = safeurl.JoinPath("users", userHandle, "gpg_keys") if err != nil { return nil, err } } u.SetQuery("per_page", "100") - req, err := http.NewRequest("GET", u.String(), nil) - if err != nil { - return nil, err - } - - resp, err := httpClient.Do(req) - if err != nil { - return nil, err - } - defer resp.Body.Close() - - if resp.StatusCode == 404 { - return nil, errScopes - } else if resp.StatusCode > 299 { - return nil, api.HandleHTTPError(resp) - } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } var keys []gpgKey - err = json.Unmarshal(b, &keys) + // 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 + err = api.NewClientFromHTTP(httpClient).REST(host, "GET", u.String(), nil, &keys) if err != nil { + if httpErr, ok := errors.AsType[api.HTTPError](err); ok && httpErr.StatusCode == 404 { + return nil, errScopes + } return nil, err } diff --git a/pkg/cmd/gpg-key/list/list_test.go b/pkg/cmd/gpg-key/list/list_test.go index daf8c991d2c..cf9a9b45d25 100644 --- a/pkg/cmd/gpg-key/list/list_test.go +++ b/pkg/cmd/gpg-key/list/list_test.go @@ -3,17 +3,70 @@ package list import ( "fmt" "net/http" + "net/url" "testing" "time" "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_userKeysScopesMissing(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "user/gpg_keys", url.Values{"per_page": []string{"100"}}), + httpmock.StatusStringResponse(http.StatusNotFound, `{"message":"Not Found"}`), + ) + + keys, err := userKeys(&http.Client{Transport: reg}, "github.com", "") + + assert.Nil(t, keys) + require.Same(t, errScopes, err) +} + +func Test_userKeysHTTPError(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "user/gpg_keys", url.Values{"per_page": []string{"100"}}), + httpmock.WithHeader( + httpmock.StatusStringResponse(http.StatusInternalServerError, `{"message":"Internal Server Error"}`), + "Content-Type", "application/json", + ), + ) + + keys, err := userKeys(&http.Client{Transport: reg}, "github.com", "") + + assert.Nil(t, keys) + 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?per_page=100)") +} + +func Test_userKeysForUser(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "users/monalisa/gpg_keys", url.Values{"per_page": []string{"100"}}), + httpmock.StringResponse(`[{"key_id":"ABC123"}]`), + ) + + keys, err := userKeys(&http.Client{Transport: reg}, "github.com", "monalisa") + + require.NoError(t, err) + require.Len(t, keys, 1) + assert.Equal(t, "ABC123", keys[0].KeyID) +} + func Test_listRun(t *testing.T) { tests := []struct { name string