Avoid crash when deleting gist with no owner

This removes the explicit check for the gist owner, speeding up the gist
deletion due to fewer API requests, but resulting in a more vague error
message in case the gist is "not found".
This commit is contained in:
Mislav Marohnić 2022-04-25 13:24:19 +02:00
parent 7678274464
commit 0bc8aae45f
2 changed files with 36 additions and 72 deletions

View file

@ -2,6 +2,7 @@ package delete
import (
"errors"
"fmt"
"net/http"
"strings"
@ -58,8 +59,6 @@ func deleteRun(opts *DeleteOptions) error {
return err
}
apiClient := api.NewClientFromHTTP(client)
cfg, err := opts.Config()
if err != nil {
return err
@ -70,21 +69,11 @@ func deleteRun(opts *DeleteOptions) error {
return err
}
gist, err := shared.GetGist(client, host, gistID)
if err != nil {
return err
}
username, err := api.CurrentLoginName(apiClient, host)
if err != nil {
return err
}
if username != gist.Owner.Login {
return errors.New("you do not own this gist")
}
err = deleteGist(apiClient, host, gistID)
if err != nil {
apiClient := api.NewClientFromHTTP(client)
if err := deleteGist(apiClient, host, gistID); err != nil {
if errors.Is(err, shared.NotFoundErr) {
return fmt.Errorf("unable to delete gist %s: either the gist is not found or it is not owned by you", gistID)
}
return err
}
@ -95,6 +84,10 @@ func deleteGist(apiClient *api.Client, hostname string, gistID string) error {
path := "gists/" + gistID
err := apiClient.REST(hostname, "DELETE", path, nil, nil)
if err != nil {
var httpErr api.HTTPError
if errors.As(err, &httpErr) && httpErr.StatusCode == 404 {
return shared.NotFoundErr
}
return err
}
return nil

View file

@ -6,7 +6,6 @@ import (
"testing"
"github.com/cli/cli/v2/internal/config"
"github.com/cli/cli/v2/pkg/cmd/gist/shared"
"github.com/cli/cli/v2/pkg/cmdutil"
"github.com/cli/cli/v2/pkg/httpmock"
"github.com/cli/cli/v2/pkg/iostreams"
@ -57,87 +56,59 @@ func TestNewCmdDelete(t *testing.T) {
func Test_deleteRun(t *testing.T) {
tests := []struct {
name string
opts *DeleteOptions
gist *shared.Gist
opts DeleteOptions
httpStubs func(*httpmock.Registry)
nontty bool
wantErr bool
wantStdout string
wantStderr string
wantParams map[string]interface{}
}{
{
name: "no such gist",
wantErr: true,
}, {
name: "another user's gist",
gist: &shared.Gist{
ID: "1234",
Files: map[string]*shared.GistFile{
"cicada.txt": {
Filename: "cicada.txt",
Content: "bwhiizzzbwhuiiizzzz",
Type: "text/plain",
},
},
Owner: &shared.GistOwner{Login: "octocat2"},
},
wantErr: true,
wantStderr: "you do not own this gist",
}, {
name: "successfully delete",
gist: &shared.Gist{
ID: "1234",
Files: map[string]*shared.GistFile{
"cicada.txt": {
Filename: "cicada.txt",
Content: "bwhiizzzbwhuiiizzzz",
Type: "text/plain",
},
},
Owner: &shared.GistOwner{Login: "octocat"},
opts: DeleteOptions{
Selector: "1234",
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(httpmock.REST("DELETE", "gists/1234"),
httpmock.StringResponse("{}"))
httpmock.StatusStringResponse(200, "{}"))
},
wantErr: false,
wantErr: false,
wantStdout: "",
wantStderr: "",
},
{
name: "not found",
opts: DeleteOptions{
Selector: "1234",
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(httpmock.REST("DELETE", "gists/1234"),
httpmock.StatusStringResponse(404, "{}"))
},
wantErr: true,
wantStdout: "",
wantStderr: "",
},
}
for _, tt := range tests {
reg := &httpmock.Registry{}
if tt.gist == nil {
reg.Register(httpmock.REST("GET", "gists/1234"),
httpmock.StatusStringResponse(404, "Not Found"))
} else {
reg.Register(httpmock.REST("GET", "gists/1234"),
httpmock.JSONResponse(tt.gist))
reg.Register(httpmock.GraphQL(`query UserCurrent\b`),
httpmock.StringResponse(`{"data":{"viewer":{"login":"octocat"}}}`))
}
if tt.httpStubs != nil {
tt.httpStubs(reg)
}
if tt.opts == nil {
tt.opts = &DeleteOptions{}
}
tt.opts.HttpClient = func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
}
tt.opts.Config = func() (config.Config, error) {
return config.NewBlankConfig(), nil
}
io, _, _, _ := iostreams.Test()
io.SetStdoutTTY(!tt.nontty)
io.SetStdinTTY(!tt.nontty)
tt.opts.IO = io
tt.opts.Selector = "1234"
ios, _, _, _ := iostreams.Test()
ios.SetStdoutTTY(false)
ios.SetStdinTTY(false)
tt.opts.IO = ios
t.Run(tt.name, func(t *testing.T) {
err := deleteRun(tt.opts)
err := deleteRun(&tt.opts)
reg.Verify(t)
if tt.wantErr {
assert.Error(t, err)