From d4c9890c5a9eb564ac3ff650104de7bc4545ecda Mon Sep 17 00:00:00 2001 From: Luan Vieira Date: Tue, 7 Feb 2023 14:46:08 -0500 Subject: [PATCH] Default to authenticated user on codespace delete (#6944) When a username option is not provided for the `gh codespace delete` command, we will use the authenticated user's login as the default to avoid deleting anyone else's codespace by mistake. Prior to this change, running `gh codespace delete --org MYORG --all` would fetch all of the codespacese associated with the org regardless of user and then only delete the ones associated with the authenticated user, which would lead to 404 errors when MYORG had codespaces owned by members other than the authenticated member. Co-authored-by: Victoria Dye Co-authored-by: Lessley Dennington --- pkg/cmd/codespace/common.go | 1 + pkg/cmd/codespace/delete.go | 10 +++++++- pkg/cmd/codespace/delete_test.go | 23 +++++++++++++++++ pkg/cmd/codespace/mock_api.go | 44 ++++++++++++++++++++++++++++++++ 4 files changed, 77 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go index 91ecaeca4..5c5c28551 100644 --- a/pkg/cmd/codespace/common.go +++ b/pkg/cmd/codespace/common.go @@ -80,6 +80,7 @@ func startLiveShareSession(ctx context.Context, codespace *api.Codespace, a *App //go:generate moq -fmt goimports -rm -skip-ensure -out mock_api.go . apiClient type apiClient interface { + GetUser(ctx context.Context) (*api.User, error) GetCodespace(ctx context.Context, name string, includeConnection bool) (*api.Codespace, error) GetOrgMemberCodespace(ctx context.Context, orgName string, userName string, codespaceName string) (*api.Codespace, error) ListCodespaces(ctx context.Context, opts api.ListCodespacesOptions) ([]*api.Codespace, error) diff --git a/pkg/cmd/codespace/delete.go b/pkg/cmd/codespace/delete.go index d0dc338cb..3af44395e 100644 --- a/pkg/cmd/codespace/delete.go +++ b/pkg/cmd/codespace/delete.go @@ -84,7 +84,15 @@ func (a *App) Delete(ctx context.Context, opts deleteOptions) (err error) { nameFilter := opts.codespaceName if nameFilter == "" { a.StartProgressIndicatorWithLabel("Fetching codespaces") - codespaces, err = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{OrgName: opts.orgName, UserName: opts.userName}) + userName := opts.userName + if userName == "" && opts.orgName != "" { + currentUser, err := a.apiClient.GetUser(ctx) + if err != nil { + return err + } + userName = currentUser.Login + } + codespaces, err = a.apiClient.ListCodespaces(ctx, api.ListCodespacesOptions{OrgName: opts.orgName, UserName: userName}) a.StopProgressIndicator() if err != nil { return fmt.Errorf("error getting codespaces: %w", err) diff --git a/pkg/cmd/codespace/delete_test.go b/pkg/cmd/codespace/delete_test.go index 93f89c775..1b381369a 100644 --- a/pkg/cmd/codespace/delete_test.go +++ b/pkg/cmd/codespace/delete_test.go @@ -202,10 +202,28 @@ func TestDelete(t *testing.T) { wantStdout: "", wantErr: true, }, + { + name: "deletion for org codespace succeeds without username", + opts: deleteOptions{ + deleteAll: true, + orgName: "bookish", + }, + codespaces: []*api.Codespace{ + { + Name: "monalisa-spoonknife-123", + Owner: api.User{Login: "monalisa"}, + }, + }, + wantDeleted: []string{"monalisa-spoonknife-123"}, + wantStdout: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { apiMock := &apiClientMock{ + GetUserFunc: func(_ context.Context) (*api.User, error) { + return &api.User{Login: "monalisa"}, nil + }, DeleteCodespaceFunc: func(_ context.Context, name string, orgName string, userName string) error { if tt.deleteErr != nil { return tt.deleteErr @@ -253,6 +271,11 @@ func TestDelete(t *testing.T) { if (err != nil) != tt.wantErr { t.Errorf("delete() error = %v, wantErr %v", err, tt.wantErr) } + for _, listArgs := range apiMock.ListCodespacesCalls() { + if listArgs.Opts.OrgName != "" && listArgs.Opts.UserName == "" { + t.Errorf("ListCodespaces() expected username option to be set") + } + } var gotDeleted []string for _, delArgs := range apiMock.DeleteCodespaceCalls() { gotDeleted = append(gotDeleted, delArgs.Name) diff --git a/pkg/cmd/codespace/mock_api.go b/pkg/cmd/codespace/mock_api.go index 772b87f56..0796679fc 100644 --- a/pkg/cmd/codespace/mock_api.go +++ b/pkg/cmd/codespace/mock_api.go @@ -46,6 +46,9 @@ import ( // GetRepositoryFunc: func(ctx context.Context, nwo string) (*api.Repository, error) { // panic("mock out the GetRepository method") // }, +// GetUserFunc: func(ctx context.Context) (*api.User, error) { +// panic("mock out the GetUser method") +// }, // ListCodespacesFunc: func(ctx context.Context, opts api.ListCodespacesOptions) ([]*api.Codespace, error) { // panic("mock out the ListCodespaces method") // }, @@ -95,6 +98,9 @@ type apiClientMock struct { // GetRepositoryFunc mocks the GetRepository method. GetRepositoryFunc func(ctx context.Context, nwo string) (*api.Repository, error) + // GetUserFunc mocks the GetUser method. + GetUserFunc func(ctx context.Context) (*api.User, error) + // ListCodespacesFunc mocks the ListCodespaces method. ListCodespacesFunc func(ctx context.Context, opts api.ListCodespacesOptions) ([]*api.Codespace, error) @@ -201,6 +207,11 @@ type apiClientMock struct { // Nwo is the nwo argument value. Nwo string } + // GetUser holds details about calls to the GetUser method. + GetUser []struct { + // Ctx is the ctx argument value. + Ctx context.Context + } // ListCodespaces holds details about calls to the ListCodespaces method. ListCodespaces []struct { // Ctx is the ctx argument value. @@ -248,6 +259,7 @@ type apiClientMock struct { lockGetCodespacesMachines sync.RWMutex lockGetOrgMemberCodespace sync.RWMutex lockGetRepository sync.RWMutex + lockGetUser sync.RWMutex lockListCodespaces sync.RWMutex lockListDevContainers sync.RWMutex lockStartCodespace sync.RWMutex @@ -658,6 +670,38 @@ func (mock *apiClientMock) GetRepositoryCalls() []struct { return calls } +// GetUser calls GetUserFunc. +func (mock *apiClientMock) GetUser(ctx context.Context) (*api.User, error) { + if mock.GetUserFunc == nil { + panic("apiClientMock.GetUserFunc: method is nil but apiClient.GetUser was just called") + } + callInfo := struct { + Ctx context.Context + }{ + Ctx: ctx, + } + mock.lockGetUser.Lock() + mock.calls.GetUser = append(mock.calls.GetUser, callInfo) + mock.lockGetUser.Unlock() + return mock.GetUserFunc(ctx) +} + +// GetUserCalls gets all the calls that were made to GetUser. +// Check the length with: +// +// len(mockedapiClient.GetUserCalls()) +func (mock *apiClientMock) GetUserCalls() []struct { + Ctx context.Context +} { + var calls []struct { + Ctx context.Context + } + mock.lockGetUser.RLock() + calls = mock.calls.GetUser + mock.lockGetUser.RUnlock() + return calls +} + // ListCodespaces calls ListCodespacesFunc. func (mock *apiClientMock) ListCodespaces(ctx context.Context, opts api.ListCodespacesOptions) ([]*api.Codespace, error) { if mock.ListCodespacesFunc == nil {