From 0bc8aae45f2feb1cf142ac317691f058621fbcc0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mislav=20Marohni=C4=87?= <mislav@github.com>
Date: Mon, 25 Apr 2022 13:24:19 +0200
Subject: [PATCH] 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".
---
 pkg/cmd/gist/delete/delete.go      | 27 ++++------
 pkg/cmd/gist/delete/delete_test.go | 81 ++++++++++--------------------
 2 files changed, 36 insertions(+), 72 deletions(-)

diff --git a/pkg/cmd/gist/delete/delete.go b/pkg/cmd/gist/delete/delete.go
index d6c810c77..70dbb8d2a 100644
--- a/pkg/cmd/gist/delete/delete.go
+++ b/pkg/cmd/gist/delete/delete.go
@@ -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
diff --git a/pkg/cmd/gist/delete/delete_test.go b/pkg/cmd/gist/delete/delete_test.go
index bb04b5c3c..a6cde9ae0 100644
--- a/pkg/cmd/gist/delete/delete_test.go
+++ b/pkg/cmd/gist/delete/delete_test.go
@@ -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)