Repository: cli/cli
Pull request #8569 (2024-01-12 20:20:29)

Move create codespace permission opt out behind `else` to fix token permissions
Related issue: https://github.com/github/codespaces/issues/16058

When https://github.com/cli/cli/pull/8179 was merged, [this line](https://github.com/cli/cli/blob/72b6dc5d8c70525a6409abe6b1cbdf8d667f6193/pkg/cmd/codespace/create.go#L400) was always executed meaning that when we tried to create the codespace in the backend, we would remove all permissions on the token. The fix is to only execute it if the user chooses to not accept additional permissions. I also added tests to make sure this is behaving as intended.




Patch:
diff --git a/pkg/cmd/codespace/common.go b/pkg/cmd/codespace/common.go
index 99d1f339c..fc70febee 100644
--- a/pkg/cmd/codespace/common.go
+++ b/pkg/cmd/codespace/common.go
@@ -114,10 +114,11 @@ func chooseCodespaceFromList(ctx context.Context, codespaces []*api.Codespace, i
 		},
 	}
 
+	prompter := &Prompter{}
 	var answers struct {
 		Codespace int
 	}
-	if err := ask(csSurvey, &answers); err != nil {
+	if err := prompter.Ask(csSurvey, &answers); err != nil {
 		return nil, fmt.Errorf("error getting answers: %w", err)
 	}
 
@@ -145,9 +146,15 @@ func safeClose(closer io.Closer, err *error) {
 // It is not portable to assume stdin/stdout are fds 0 and 1.
 var hasTTY = term.IsTerminal(int(os.Stdin.Fd())) && term.IsTerminal(int(os.Stdout.Fd()))
 
+type SurveyPrompter interface {
+	Ask(qs []*survey.Question, response interface{}) error
+}
+
+type Prompter struct{}
+
 // ask asks survey questions on the terminal, using standard options.
 // It fails unless hasTTY, but ideally callers should avoid calling it in that case.
-func ask(qs []*survey.Question, response interface{}) error {
+func (p *Prompter) Ask(qs []*survey.Question, response interface{}) error {
 	if !hasTTY {
 		return fmt.Errorf("no terminal")
 	}
diff --git a/pkg/cmd/codespace/create.go b/pkg/cmd/codespace/create.go
index 821faa10b..67eb5461e 100644
--- a/pkg/cmd/codespace/create.go
+++ b/pkg/cmd/codespace/create.go
@@ -140,6 +140,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 		return a.browser.Browse(fmt.Sprintf("%s/codespaces/new", a.apiClient.ServerURL()))
 	}
 
+	prompter := &Prompter{}
 	promptForRepoAndBranch := userInputs.Repository == "" && !opts.useWeb
 	if promptForRepoAndBranch {
 		var defaultRepo string
@@ -167,7 +168,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 				Validate: survey.Required,
 			},
 		}
-		if err := ask(repoQuestions, &userInputs); err != nil {
+		if err := prompter.Ask(repoQuestions, &userInputs); err != nil {
 			return fmt.Errorf("failed to prompt: %w", err)
 		}
 	}
@@ -212,7 +213,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 			},
 		}
 
-		if err := ask(branchQuestions, &userInputs); err != nil {
+		if err := prompter.Ask(branchQuestions, &userInputs); err != nil {
 			return fmt.Errorf("failed to prompt: %w", err)
 		}
 	}
@@ -259,7 +260,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 					},
 				}
 
-				if err := ask([]*survey.Question{devContainerPathQuestion}, &devContainerPath); err != nil {
+				if err := prompter.Ask([]*survey.Question{devContainerPathQuestion}, &devContainerPath); err != nil {
 					return fmt.Errorf("failed to prompt: %w", err)
 				}
 			}
@@ -277,7 +278,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 	// web UI also provide a way to select machine type
 	// therefore we let the user choose from the web UI instead of prompting from CLI
 	if !(opts.useWeb && opts.machine == "") {
-		machine, err = getMachineName(ctx, a.apiClient, repository.ID, opts.machine, branch, userInputs.Location, devContainerPath)
+		machine, err = getMachineName(ctx, a.apiClient, prompter, repository.ID, opts.machine, branch, userInputs.Location, devContainerPath)
 		if err != nil {
 			return fmt.Errorf("error getting machine type: %w", err)
 		}
@@ -320,7 +321,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 			return fmt.Errorf("error creating codespace: %w", err)
 		}
 
-		codespace, err = a.handleAdditionalPermissions(ctx, createParams, aerr.AllowPermissionsURL)
+		codespace, err = a.handleAdditionalPermissions(ctx, prompter, createParams, aerr.AllowPermissionsURL)
 		if err != nil {
 			// this error could be a cmdutil.SilentError (in the case that the user opened the browser) so we don't want to wrap it
 			return err
@@ -344,7 +345,7 @@ func (a *App) Create(ctx context.Context, opts createOptions) error {
 	return nil
 }
 
-func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api.CreateCodespaceParams, allowPermissionsURL string) (*api.Codespace, error) {
+func (a *App) handleAdditionalPermissions(ctx context.Context, prompter SurveyPrompter, createParams *api.CreateCodespaceParams, allowPermissionsURL string) (*api.Codespace, error) {
 	var (
 		isInteractive = a.io.CanPrompt()
 		cs            = a.io.ColorScheme()
@@ -379,7 +380,7 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api
 		Accept string
 	}
 
-	if err := ask(permsSurvey, &answers); err != nil {
+	if err := prompter.Ask(permsSurvey, &answers); err != nil {
 		return nil, fmt.Errorf("error getting answers: %w", err)
 	}
 
@@ -393,12 +394,12 @@ func (a *App) handleAdditionalPermissions(ctx context.Context, createParams *api
 		if err := a.pollForPermissions(ctx, createParams); err != nil {
 			return nil, fmt.Errorf("error polling for permissions: %w", err)
 		}
+	} else {
+		// If the user chose to create the codespace without the permissions,
+		// we can continue with the create opting out of the additional permissions
+		createParams.PermissionsOptOut = true
 	}
 
-	// if the user chose to create the codespace without the permissions,
-	// we can continue with the create opting out of the additional permissions
-	createParams.PermissionsOptOut = true
-
 	var codespace *api.Codespace
 	err := a.RunWithProgress("Creating codespace", func() (err error) {
 		codespace, err = a.apiClient.CreateCodespace(ctx, createParams)
@@ -510,7 +511,7 @@ func (a *App) showStatus(ctx context.Context, codespace *api.Codespace) error {
 }
 
 // getMachineName prompts the user to select the machine type, or validates the machine if non-empty.
-func getMachineName(ctx context.Context, apiClient apiClient, repoID int, machine, branch, location string, devcontainerPath string) (string, error) {
+func getMachineName(ctx context.Context, apiClient apiClient, prompter SurveyPrompter, repoID int, machine, branch, location string, devcontainerPath string) (string, error) {
 	machines, err := apiClient.GetCodespacesMachines(ctx, repoID, branch, location, devcontainerPath)
 	if err != nil {
 		return "", fmt.Errorf("error requesting machine instance types: %w", err)
@@ -561,7 +562,7 @@ func getMachineName(ctx context.Context, apiClient apiClient, repoID int, machin
 	}
 
 	var machineAnswers struct{ Machine string }
-	if err := ask(machineSurvey, &machineAnswers); err != nil {
+	if err := prompter.Ask(machineSurvey, &machineAnswers); err != nil {
 		return "", fmt.Errorf("error getting machine: %w", err)
 	}
 
diff --git a/pkg/cmd/codespace/delete.go b/pkg/cmd/codespace/delete.go
index e54bf09fb..5da24a634 100644
--- a/pkg/cmd/codespace/delete.go
+++ b/pkg/cmd/codespace/delete.go
@@ -220,6 +220,7 @@ func confirmDeletion(p prompter, apiCodespace *api.Codespace, isInteractive bool
 type surveyPrompter struct{}
 
 func (p *surveyPrompter) Confirm(message string) (bool, error) {
+	prompter := &Prompter{}
 	var confirmed struct {
 		Confirmed bool
 	}
@@ -231,7 +232,7 @@ func (p *surveyPrompter) Confirm(message string) (bool, error) {
 			},
 		},
 	}
-	if err := ask(q, &confirmed); err != nil {
+	if err := prompter.Ask(q, &confirmed); err != nil {
 		return false, fmt.Errorf("failed to prompt: %w", err)
 	}
 


Test patch:
diff --git a/pkg/cmd/codespace/create_test.go b/pkg/cmd/codespace/create_test.go
index 78039db0d..6466d9be7 100644
--- a/pkg/cmd/codespace/create_test.go
+++ b/pkg/cmd/codespace/create_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 	"time"
 
+	"github.com/AlecAivazis/survey/v2"
 	"github.com/cli/cli/v2/internal/browser"
 	"github.com/cli/cli/v2/internal/codespaces/api"
 	"github.com/cli/cli/v2/pkg/cmdutil"
@@ -703,6 +704,127 @@ func TestBuildDisplayName(t *testing.T) {
 	}
 }
 
+type MockSurveyPrompter struct {
+	AskFunc func(qs []*survey.Question, response interface{}) error
+}
+
+func (m *MockSurveyPrompter) Ask(qs []*survey.Question, response interface{}) error {
+	return m.AskFunc(qs, response)
+}
+
+type MockBrowser struct {
+	Err error
+}
+
+func (b *MockBrowser) Browse(url string) error {
+	if b.Err != nil {
+		return b.Err
+	}
+
+	return nil
+}
+
+func TestHandleAdditionalPermissions(t *testing.T) {
+	tests := []struct {
+		name                  string
+		isInteractive         bool
+		accept                string
+		permissionsOptOut     bool
+		browserErr            error
+		pollForPermissionsErr error
+		createCodespaceErr    error
+		wantErr               bool
+	}{
+		{
+			name:              "non-interactive",
+			isInteractive:     false,
+			permissionsOptOut: false,
+			wantErr:           true,
+		},
+		{
+			name:              "interactive, continue in browser, browser error",
+			isInteractive:     true,
+			accept:            "Continue in browser to review and authorize additional permissions (Recommended)",
+			permissionsOptOut: false,
+			browserErr:        fmt.Errorf("browser error"),
+			wantErr:           true,
+		},
+		{
+			name:                  "interactive, continue in browser, poll for permissions error",
+			isInteractive:         true,
+			accept:                "Continue in browser to review and authorize additional permissions (Recommended)",
+			permissionsOptOut:     false,
+			pollForPermissionsErr: fmt.Errorf("poll for permissions error"),
+			wantErr:               true,
+		},
+		{
+			name:               "interactive, continue in browser, create codespace error",
+			isInteractive:      true,
+			accept:             "Continue in browser to review and authorize additional permissions (Recommended)",
+			permissionsOptOut:  false,
+			createCodespaceErr: fmt.Errorf("create codespace error"),
+			wantErr:            true,
+		},
+		{
+			name:               "interactive, continue without authorizing",
+			isInteractive:      true,
+			accept:             "Continue without authorizing additional permissions",
+			permissionsOptOut:  true,
+			createCodespaceErr: fmt.Errorf("create codespace error"),
+			wantErr:            true,
+		},
+		{
+			name:              "interactive, continue without authorizing, create codespace success",
+			isInteractive:     true,
+			accept:            "Continue without authorizing additional permissions",
+			permissionsOptOut: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			ios, _, _, _ := iostreams.Test()
+			a := &App{
+				io: ios,
+				browser: &MockBrowser{
+					Err: tt.browserErr,
+				},
+				apiClient: &apiClientMock{
+					CreateCodespaceFunc: func(ctx context.Context, params *api.CreateCodespaceParams) (*api.Codespace, error) {
+						return nil, tt.createCodespaceErr
+					},
+					GetCodespacesPermissionsCheckFunc: func(ctx context.Context, repoID int, branch string, devcontainerPath string) (bool, error) {
+						if tt.pollForPermissionsErr != nil {
+							return false, tt.pollForPermissionsErr
+						}
+						return true, nil
+					},
+				},
+			}
+
+			if tt.isInteractive {
+				a.io.SetStdinTTY(true)
+				a.io.SetStdoutTTY(true)
+				a.io.SetStderrTTY(true)
+			}
+
+			params := &api.CreateCodespaceParams{}
+			_, err := a.handleAdditionalPermissions(context.Background(), &MockSurveyPrompter{
+				AskFunc: func(qs []*survey.Question, response interface{}) error {
+					*response.(*struct{ Accept string }) = struct{ Accept string }{Accept: tt.accept}
+					return nil
+				},
+			}, params, "http://example.com")
+			if (err != nil) != tt.wantErr {
+				t.Errorf("handleAdditionalPermissions() error = %v, wantErr %v", err, tt.wantErr)
+			}
+			if tt.permissionsOptOut != params.PermissionsOptOut {
+				t.Errorf("handleAdditionalPermissions() permissionsOptOut = %v, want %v", params.PermissionsOptOut, tt.permissionsOptOut)
+			}
+		})
+	}
+}
+
 func apiCreateDefaults(c *apiClientMock) *apiClientMock {
 	if c.GetRepositoryFunc == nil {
 		c.GetRepositoryFunc = func(ctx context.Context, nwo string) (*api.Repository, error) {