From 4757540bdfe15dad2a8499347eb4ebff37c3939d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 31 Jul 2020 19:47:13 +0200 Subject: [PATCH 1/7] Align `bump-homebrew-formula` config for updated formula format --- .github/workflows/releases.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index 7de6e176a..1654e74df 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -32,6 +32,7 @@ jobs: if: "!contains(github.ref, '-')" # skip prereleases with: formula-name: gh + download-url: https://github.com/cli/cli.git env: COMMITTER_TOKEN: ${{ secrets.UPLOAD_GITHUB_TOKEN }} - name: Checkout documentation site From 35f18b6c0251be7c1789cbd2da894f808b046e4c Mon Sep 17 00:00:00 2001 From: vilmibm Date: Mon, 27 Jul 2020 14:51:23 -0500 Subject: [PATCH 2/7] gh auth login --- api/client.go | 46 ++-- command/root.go | 5 + internal/config/config_setup.go | 6 +- pkg/cmd/auth/auth.go | 18 ++ pkg/cmd/auth/login/client.go | 48 ++++ pkg/cmd/auth/login/login.go | 292 +++++++++++++++++++++ pkg/cmd/auth/login/login_test.go | 436 +++++++++++++++++++++++++++++++ pkg/cmd/repo/create/create.go | 7 +- pkg/cmd/repo/fork/fork.go | 11 +- pkg/httpmock/stub.go | 2 +- pkg/prompt/prompt.go | 4 + pkg/prompt/stubber.go | 55 +++- utils/utils.go | 4 + 13 files changed, 896 insertions(+), 38 deletions(-) create mode 100644 pkg/cmd/auth/auth.go create mode 100644 pkg/cmd/auth/login/client.go create mode 100644 pkg/cmd/auth/login/login.go create mode 100644 pkg/cmd/auth/login/login_test.go diff --git a/api/client.go b/api/client.go index 36abbfbf8..233d460f0 100644 --- a/api/client.go +++ b/api/client.go @@ -3,6 +3,7 @@ package api import ( "bytes" "encoding/json" + "errors" "fmt" "io" "io/ioutil" @@ -195,19 +196,18 @@ func (err HTTPError) Error() string { return fmt.Sprintf("HTTP %d (%s)", err.StatusCode, err.RequestURL) } -// Returns whether or not scopes are present, appID, and error -func (c Client) HasScopes(wantedScopes ...string) (bool, string, error) { - url := "https://api.github.com/user" - req, err := http.NewRequest("GET", url, nil) +func (c Client) HasMinimumScopes(hostname string) (bool, error) { + apiEndpoint := ghinstance.RESTPrefix(hostname) + + req, err := http.NewRequest("GET", apiEndpoint, nil) if err != nil { - return false, "", err + return false, err } req.Header.Set("Content-Type", "application/json; charset=utf-8") - res, err := c.http.Do(req) if err != nil { - return false, "", err + return false, err } defer func() { @@ -218,26 +218,36 @@ func (c Client) HasScopes(wantedScopes ...string) (bool, string, error) { }() if res.StatusCode != 200 { - return false, "", handleHTTPError(res) + return false, handleHTTPError(res) } - appID := res.Header.Get("X-Oauth-Client-Id") hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") - found := 0 + search := map[string]bool{ + "repo": false, + "read:org": false, + "admin:org": false, + } + for _, s := range hasScopes { - for _, w := range wantedScopes { - if w == strings.TrimSpace(s) { - found++ - } - } + search[strings.TrimSpace(s)] = true } - if found == len(wantedScopes) { - return true, appID, nil + errorMsgs := []string{} + if !search["repo"] { + errorMsgs = append(errorMsgs, "missing required scope 'repo'") } - return false, appID, nil + if !search["read:org"] && !search["admin:org"] { + errorMsgs = append(errorMsgs, "missing required scope 'read:org'") + } + + if len(errorMsgs) > 0 { + return false, errors.New(strings.Join(errorMsgs, ";")) + } + + return true, nil + } // GraphQL performs a GraphQL request and parses the response diff --git a/command/root.go b/command/root.go index 452db221b..cd89f05d7 100644 --- a/command/root.go +++ b/command/root.go @@ -22,6 +22,8 @@ import ( "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/internal/run" apiCmd "github.com/cli/cli/pkg/cmd/api" + authCmd "github.com/cli/cli/pkg/cmd/auth" + authLoginCmd "github.com/cli/cli/pkg/cmd/auth/login" gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" prCheckoutCmd "github.com/cli/cli/pkg/cmd/pr/checkout" prDiffCmd "github.com/cli/cli/pkg/cmd/pr/diff" @@ -134,6 +136,9 @@ func init() { RootCmd.AddCommand(gistCmd) gistCmd.AddCommand(gistCreateCmd.NewCmdCreate(cmdFactory, nil)) + RootCmd.AddCommand(authCmd.Cmd) + authCmd.Cmd.AddCommand(authLoginCmd.NewCmdLogin(cmdFactory, nil)) + resolvedBaseRepo := func() (ghrepo.Interface, error) { httpClient, err := cmdFactory.HttpClient() if err != nil { diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go index 0d40e1a0f..d8fcef1ec 100644 --- a/internal/config/config_setup.go +++ b/internal/config/config_setup.go @@ -9,6 +9,7 @@ import ( "github.com/cli/cli/api" "github.com/cli/cli/auth" + "github.com/cli/cli/utils" ) var ( @@ -67,7 +68,7 @@ func authFlow(oauthHost, notice string) (string, string, error) { } fmt.Fprintln(os.Stderr, notice) - fmt.Fprintf(os.Stderr, "Press Enter to open %s in your browser... ", flow.Hostname) + fmt.Fprintf(os.Stderr, "- %s to open %s in your browser... ", utils.Bold("Press Enter"), flow.Hostname) _ = waitForEnter(os.Stdin) token, err := flow.ObtainAccessToken() if err != nil { @@ -83,7 +84,8 @@ func authFlow(oauthHost, notice string) (string, string, error) { } func AuthFlowComplete() { - fmt.Fprintln(os.Stderr, "Authentication complete. Press Enter to continue... ") + fmt.Fprintf(os.Stderr, "%s Authentication complete. %s to continue...\n", + utils.GreenCheck(), utils.Bold("Press Enter")) _ = waitForEnter(os.Stdin) } diff --git a/pkg/cmd/auth/auth.go b/pkg/cmd/auth/auth.go new file mode 100644 index 000000000..ff1f79c7c --- /dev/null +++ b/pkg/cmd/auth/auth.go @@ -0,0 +1,18 @@ +package auth + +import ( + "github.com/spf13/cobra" +) + +var Cmd = &cobra.Command{ + Use: "auth ", + Short: "Login, logout, and refresh your authentication", + Long: `Manage gh's authentication state.`, + // TODO this all doesn't exist yet + //Example: heredoc.Doc(` + // $ gh auth login + // $ gh auth status + // $ gh auth refresh --scopes gist + // $ gh auth logout + //`), +} diff --git a/pkg/cmd/auth/login/client.go b/pkg/cmd/auth/login/client.go new file mode 100644 index 000000000..e133ca5c8 --- /dev/null +++ b/pkg/cmd/auth/login/client.go @@ -0,0 +1,48 @@ +package login + +import ( + "fmt" + "net/http" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" +) + +func validateHostCfg(hostname string, cfg config.Config) error { + apiClient, err := clientFromCfg(hostname, cfg) + if err != nil { + return err + } + + _, err = apiClient.HasMinimumScopes(hostname) + if err != nil { + return fmt.Errorf("could not validate token: %w", err) + } + + return nil +} + +var clientFromCfg = func(hostname string, cfg config.Config) (*api.Client, error) { + var opts []api.ClientOption + + token, err := cfg.Get(hostname, "oauth_token") + if err != nil { + return nil, err + } + + if token == "" { + return nil, fmt.Errorf("no token found in config for %s", hostname) + } + + opts = append(opts, + // no access to Version so the user agent is more generic here. + api.AddHeader("User-Agent", "GitHub CLI"), + api.AddHeaderFunc("Authorization", func(req *http.Request) (string, error) { + return fmt.Sprintf("token %s", token), nil + }), + ) + + httpClient := api.NewHTTPClient(opts...) + + return api.NewClientFromHTTP(httpClient), nil +} diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go new file mode 100644 index 000000000..60687dad8 --- /dev/null +++ b/pkg/cmd/auth/login/login.go @@ -0,0 +1,292 @@ +package login + +import ( + "errors" + "fmt" + "io/ioutil" + "net/http" + "os" + "strings" + + "github.com/AlecAivazis/survey/v2" + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/internal/ghinstance" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type LoginOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + Config func() (config.Config, error) + + Hostname string + Token string + OnlyValidate bool +} + +func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Command { + opts := &LoginOptions{ + HttpClient: f.HttpClient, + IO: f.IOStreams, + Config: f.Config, + } + + cmd := &cobra.Command{ + Use: "login", + Args: cobra.ExactArgs(0), + Short: "Authenticate with a GitHub host", + Long: heredoc.Doc(`Authenticate with a GitHub host. + + This interactive command initializes your authentication state either by helping you log into + GitHub via browser-based OAuth or by accepting a Personal Access Token. + + The interactivity can be avoided by specifying --with-token and passing a token on STDIN. + `), + Example: heredoc.Doc(` + $ gh auth login + # => do an interactive setup + + $ gh auth login --with-token < mytoken.txt + # => read token from mytoken.txt and authenticate against github.com + + $ gh auth login --hostname enterprise.internal --with-token < mytoken.txt + # => read token from mytoken.txt and authenticate against a GitHub Enterprise instance + `), + RunE: func(cmd *cobra.Command, args []string) error { + isTTY := opts.IO.IsStdinTTY() + + // TODO support other ways of naming + ghToken := os.Getenv("GITHUB_TOKEN") + + if !isTTY && (!cmd.Flags().Changed("with-token") && ghToken == "") { + return &cmdutil.FlagError{Err: errors.New("no terminal detected; please use '--with-token' or set GITHUB_TOKEN")} + } + + wt, _ := cmd.Flags().GetBool("with-token") + if wt { + defer opts.IO.In.Close() + token, err := ioutil.ReadAll(opts.IO.In) + if err != nil { + return &cmdutil.FlagError{Err: fmt.Errorf("failed to read token from STDIN: %w", err)} + } + + opts.Token = strings.TrimSpace(string(token)) + } else if ghToken != "" { + opts.OnlyValidate = true + opts.Token = ghToken + } + + if opts.Token != "" { + // Assume non-interactive if a token is specified + if opts.Hostname == "" { + opts.Hostname = ghinstance.Default() + } + } + + if runF != nil { + return runF(opts) + } + + return loginRun(opts) + }, + } + + cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to authenticate with") + cmd.Flags().Bool("with-token", false, "Read token from standard input") + + return cmd +} + +func loginRun(opts *LoginOptions) error { + cfg, err := opts.Config() + if err != nil { + return err + } + + if opts.Token != "" { + // I chose to not error on existing host here; my thinking is that for --with-token the user + // probably doesn't care if a token is overwritten since they have a token in hand they + // explicitly want to use. + if opts.Hostname == "" { + return errors.New("empty hostname would leak oauth_token") + } + + err := cfg.Set(opts.Hostname, "oauth_token", opts.Token) + if err != nil { + return err + } + + err = validateHostCfg(opts.Hostname, cfg) + if err != nil { + return err + } + + if opts.OnlyValidate { + return nil + } + + return cfg.Write() + } + + // TODO consider explicitly telling survey what io to use since it's implicit right now + + hostname := opts.Hostname + + if hostname == "" { + var hostType int + err := prompt.SurveyAskOne(&survey.Select{ + Message: "What account do you want to log into?", + Options: []string{ + "GitHub.com", + "GitHub Enterprise", + }, + }, &hostType) + + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + isEnterprise := hostType == 1 + + hostname = ghinstance.Default() + if isEnterprise { + err := prompt.SurveyAskOne(&survey.Input{ + Message: "GHE hostname:", + }, &hostname, survey.WithValidator(survey.Required)) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + } + } + + fmt.Fprintf(opts.IO.ErrOut, "- Logging into %s\n", hostname) + + existingToken, _ := cfg.Get(hostname, "oauth_token") + + if existingToken != "" { + err := validateHostCfg(hostname, cfg) + if err == nil { + apiClient, err := clientFromCfg(hostname, cfg) + if err != nil { + return err + } + + username, err := api.CurrentLoginName(apiClient, hostname) + if err != nil { + return fmt.Errorf("error using api: %w", err) + } + var keepGoing bool + err = prompt.SurveyAskOne(&survey.Confirm{ + Message: fmt.Sprintf( + "You're already logged into %s as %s. Do you want to re-authenticate?", + hostname, + username), + Default: false, + }, &keepGoing) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + if !keepGoing { + return nil + } + } + } + + var authMode int + err = prompt.SurveyAskOne(&survey.Select{ + Message: "How would you like to authenticate?", + Options: []string{ + "Login with a web browser", + "Paste an authentication token", + }, + }, &authMode) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + if authMode == 0 { + _, err := config.AuthFlowWithConfig(cfg, hostname, "") + if err != nil { + return fmt.Errorf("failed to authenticate via web browser: %w", err) + } + } else { + fmt.Fprintln(opts.IO.ErrOut) + fmt.Fprintln(opts.IO.ErrOut, heredoc.Doc(` + Tip: you can generate a Personal Access Token here https://github.com/settings/tokens + The minimum required scopes are 'repo' and 'read:org'.`)) + var token string + err := prompt.SurveyAskOne(&survey.Password{ + Message: "Paste your authentication token:", + }, &token, survey.WithValidator(survey.Required)) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + if hostname == "" { + return errors.New("empty hostname would leak oauth_token") + } + + err = cfg.Set(hostname, "oauth_token", token) + if err != nil { + return err + } + + err = validateHostCfg(hostname, cfg) + if err != nil { + return err + } + } + + var gitProtocol string + err = prompt.SurveyAskOne(&survey.Select{ + Message: "Choose default git protocol", + Options: []string{ + "HTTPS", + "SSH", + }, + }, &gitProtocol) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + gitProtocol = strings.ToLower(gitProtocol) + + fmt.Fprintf(opts.IO.ErrOut, "- gh config set -h%s git_protocol %s\n", hostname, gitProtocol) + err = cfg.Set(hostname, "git_protocol", gitProtocol) + if err != nil { + return err + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Configured git protocol\n", utils.GreenCheck()) + + apiClient, err := clientFromCfg(hostname, cfg) + if err != nil { + return err + } + + username, err := api.CurrentLoginName(apiClient, hostname) + if err != nil { + return fmt.Errorf("error using api: %w", err) + } + + err = cfg.Set(hostname, "user", username) + if err != nil { + return err + } + + err = cfg.Write() + if err != nil { + return err + } + + fmt.Fprintf(opts.IO.ErrOut, "%s Logged in as %s\n", utils.GreenCheck(), utils.Bold(username)) + + return nil +} diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go new file mode 100644 index 000000000..4cc818670 --- /dev/null +++ b/pkg/cmd/auth/login/login_test.go @@ -0,0 +1,436 @@ +package login + +import ( + "bytes" + "io/ioutil" + "net/http" + "os" + "regexp" + "testing" + + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func Test_NewCmdLogin(t *testing.T) { + tests := []struct { + name string + cli string + stdin string + stdinTTY bool + wants LoginOptions + wantsErr bool + ghtoken string + }{ + { + name: "nontty, with-token", + stdin: "abc123\n", + cli: "--with-token", + wants: LoginOptions{ + Hostname: "github.com", + Token: "abc123", + }, + }, + { + name: "tty, with-token", + stdinTTY: true, + stdin: "def456", + cli: "--with-token", + wants: LoginOptions{ + Hostname: "github.com", + Token: "def456", + }, + }, + { + name: "nontty, hostname", + cli: "--hostname claire.redfield", + wantsErr: true, + }, + { + name: "nontty", + cli: "", + wantsErr: true, + }, + { + name: "nontty, with-token, hostname", + cli: "--hostname claire.redfield --with-token", + stdin: "abc123\n", + wants: LoginOptions{ + Hostname: "claire.redfield", + Token: "abc123", + }, + }, + { + name: "tty, with-token, hostname", + stdinTTY: true, + stdin: "ghi789", + cli: "--with-token --hostname brad.vickers", + wants: LoginOptions{ + Hostname: "brad.vickers", + Token: "ghi789", + }, + }, + { + name: "tty, hostname", + stdinTTY: true, + cli: "--hostname barry.burton", + wants: LoginOptions{ + Hostname: "barry.burton", + Token: "", + }, + }, + { + name: "tty", + stdinTTY: true, + cli: "", + wants: LoginOptions{ + Hostname: "", + Token: "", + }, + }, + { + name: "tty, GITHUB_TOKEN", + stdinTTY: true, + cli: "", + ghtoken: "abc123", + wants: LoginOptions{ + Hostname: "github.com", + Token: "abc123", + OnlyValidate: true, + }, + }, + { + name: "nontty, GITHUB_TOKEN", + stdinTTY: false, + cli: "", + ghtoken: "abc123", + wants: LoginOptions{ + Hostname: "github.com", + Token: "abc123", + OnlyValidate: true, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ghtoken := os.Getenv("GITHUB_TOKEN") + defer func() { + os.Setenv("GITHUB_TOKEN", ghtoken) + }() + os.Setenv("GITHUB_TOKEN", tt.ghtoken) + io, stdin, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: io, + } + + io.SetStdinTTY(tt.stdinTTY) + if tt.stdin != "" { + stdin.WriteString(tt.stdin) + } + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *LoginOptions + cmd := NewCmdLogin(f, func(opts *LoginOptions) error { + gotOpts = opts + return nil + }) + // TODO cobra hack-around + cmd.Flags().BoolP("help", "x", false, "") + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } + assert.NoError(t, err) + + assert.Equal(t, tt.wants.Token, gotOpts.Token) + assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) + }) + } +} + +func scopesResponder(scopes string) func(*http.Request) (*http.Response, error) { + return func(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: 200, + Request: req, + Header: map[string][]string{ + "X-Oauth-Scopes": {scopes}, + }, + Body: ioutil.NopCloser(bytes.NewBufferString("")), + }, nil + } +} + +func Test_loginRun_nontty(t *testing.T) { + tests := []struct { + name string + opts *LoginOptions + httpStubs func(*httpmock.Registry) + wantHosts string + wantErr *regexp.Regexp + }{ + { + name: "with token", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "abc123", + }, + wantHosts: "github.com:\n oauth_token: abc123\n", + }, + { + name: "with token and non-default host", + opts: &LoginOptions{ + Hostname: "albert.wesker", + Token: "abc123", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "api/v3/"), scopesResponder("repo,read:org")) + }, + wantHosts: "albert.wesker:\n oauth_token: abc123\n", + }, + { + name: "missing repo scope", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "abc456", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), scopesResponder("read:org")) + }, + wantErr: regexp.MustCompile(`missing required scope 'repo'`), + }, + { + name: "missing read scope", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "abc456", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), scopesResponder("repo")) + }, + wantErr: regexp.MustCompile(`missing required scope 'read:org'`), + }, + { + name: "has admin scope", + opts: &LoginOptions{ + Hostname: "github.com", + Token: "abc456", + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), scopesResponder("repo,admin:org")) + }, + wantHosts: "github.com:\n oauth_token: abc456\n", + }, + } + + for _, tt := range tests { + io, _, stdout, stderr := iostreams.Test() + + io.SetStdinTTY(false) + io.SetStdoutTTY(false) + + tt.opts.Config = func() (config.Config, error) { + return config.NewBlankConfig(), nil + } + + tt.opts.IO = io + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + origClientFromCfg := clientFromCfg + defer func() { + clientFromCfg = origClientFromCfg + }() + clientFromCfg = func(_ string, _ config.Config) (*api.Client, error) { + httpClient := &http.Client{Transport: reg} + return api.NewClientFromHTTP(httpClient), nil + } + + if tt.httpStubs != nil { + tt.httpStubs(reg) + } else { + reg.Register(httpmock.REST("GET", ""), scopesResponder("repo,read:org")) + } + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + err := loginRun(tt.opts) + assert.Equal(t, tt.wantErr == nil, err == nil) + if err != nil { + if tt.wantErr != nil { + assert.True(t, tt.wantErr.MatchString(err.Error())) + return + } else { + t.Fatalf("unexpected error: %s", err) + } + } + + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "", stderr.String()) + assert.Equal(t, tt.wantHosts, hostsBuf.String()) + reg.Verify(t) + }) + } +} + +func Test_loginRun_Survey(t *testing.T) { + tests := []struct { + name string + opts *LoginOptions + httpStubs func(*httpmock.Registry) + askStubs func(*prompt.AskStubber) + wantHosts string + cfg func(config.Config) + }{ + { + name: "already authenticated", + cfg: func(cfg config.Config) { + _ = cfg.Set("github.com", "oauth_token", "ghi789") + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", ""), scopesResponder("repo,read:org,")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"jillv"}}}`)) + }, + askStubs: func(as *prompt.AskStubber) { + as.StubOne(0) // host type github.com + as.StubOne(false) // do not continue + }, + wantHosts: "", // nothing should have been written to hosts + }, + { + name: "hostname set", + opts: &LoginOptions{ + Hostname: "rebecca.chambers", + }, + wantHosts: "rebecca.chambers:\n oauth_token: def456\n git_protocol: https\n user: jillv\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne(1) // auth mode: token + as.StubOne("def456") // auth token + as.StubOne("HTTPS") // git_protocol + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "api/v3/"), scopesResponder("repo,read:org,")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"jillv"}}}`)) + }, + }, + { + name: "choose enterprise", + wantHosts: "brad.vickers:\n oauth_token: def456\n git_protocol: https\n user: jillv\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne(1) // host type enterprise + as.StubOne("brad.vickers") // hostname + as.StubOne(1) // auth mode: token + as.StubOne("def456") // auth token + as.StubOne("HTTPS") // git_protocol + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "api/v3/"), scopesResponder("repo,read:org,")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"jillv"}}}`)) + }, + }, + { + name: "choose github.com", + wantHosts: "github.com:\n oauth_token: def456\n git_protocol: https\n user: jillv\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne(0) // host type github.com + as.StubOne(1) // auth mode: token + as.StubOne("def456") // auth token + as.StubOne("HTTPS") // git_protocol + }, + }, + { + name: "sets git_protocol", + wantHosts: "github.com:\n oauth_token: def456\n git_protocol: ssh\n user: jillv\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne(0) // host type github.com + as.StubOne(1) // auth mode: token + as.StubOne("def456") // auth token + as.StubOne("SSH") // git_protocol + }, + }, + // TODO how to test browser auth? + } + + for _, tt := range tests { + if tt.opts == nil { + tt.opts = &LoginOptions{} + } + io, _, _, _ := iostreams.Test() + + io.SetStdinTTY(true) + io.SetStderrTTY(true) + io.SetStdoutTTY(true) + + tt.opts.IO = io + + cfg := config.NewBlankConfig() + + if tt.cfg != nil { + tt.cfg(cfg) + } + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + origClientFromCfg := clientFromCfg + defer func() { + clientFromCfg = origClientFromCfg + }() + clientFromCfg = func(_ string, _ config.Config) (*api.Client, error) { + httpClient := &http.Client{Transport: reg} + return api.NewClientFromHTTP(httpClient), nil + } + if tt.httpStubs != nil { + tt.httpStubs(reg) + } else { + reg.Register(httpmock.REST("GET", ""), scopesResponder("repo,read:org,")) + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"jillv"}}}`)) + } + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + as, teardown := prompt.InitAskStubber() + defer teardown() + if tt.askStubs != nil { + tt.askStubs(as) + } + + err := loginRun(tt.opts) + if err != nil { + t.Fatalf("unexpected error: %s", err) + } + + assert.Equal(t, tt.wantHosts, hostsBuf.String()) + reg.Verify(t) + }) + } +} diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 32313ca52..4bac5d6bd 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -133,11 +133,10 @@ func createRun(opts *CreateOptions) error { stderr := opts.IO.ErrOut stdout := opts.IO.Out - greenCheck := utils.Green("✓") isTTY := opts.IO.IsStdoutTTY() if isTTY { - fmt.Fprintf(stderr, "%s Created repository %s on GitHub\n", greenCheck, ghrepo.FullName(repo)) + fmt.Fprintf(stderr, "%s Created repository %s on GitHub\n", utils.GreenCheck(), ghrepo.FullName(repo)) } else { fmt.Fprintln(stdout, repo.URL) } @@ -160,7 +159,7 @@ func createRun(opts *CreateOptions) error { return err } if isTTY { - fmt.Fprintf(stderr, "%s Added remote %s\n", greenCheck, remoteURL) + fmt.Fprintf(stderr, "%s Added remote %s\n", utils.GreenCheck(), remoteURL) } } else if isTTY { doSetup := false @@ -187,7 +186,7 @@ func createRun(opts *CreateOptions) error { return err } - fmt.Fprintf(stderr, "%s Initialized repository in './%s/'\n", greenCheck, path) + fmt.Fprintf(stderr, "%s Initialized repository in './%s/'\n", utils.GreenCheck(), path) } } return nil diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 4fad93fec..6a7131a32 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -125,7 +125,6 @@ func forkRun(opts *ForkOptions) error { connectedToTerminal := opts.IO.IsStdoutTTY() && opts.IO.IsStderrTTY() && opts.IO.IsStdinTTY() - greenCheck := utils.Green("✓") stderr := opts.IO.ErrOut s := utils.Spinner(stderr) stopSpinner := func() {} @@ -173,7 +172,7 @@ func forkRun(opts *ForkOptions) error { } } else { if connectedToTerminal { - fmt.Fprintf(stderr, "%s Created fork %s\n", greenCheck, utils.Bold(ghrepo.FullName(forkedRepo))) + fmt.Fprintf(stderr, "%s Created fork %s\n", utils.GreenCheck(), utils.Bold(ghrepo.FullName(forkedRepo))) } } @@ -199,7 +198,7 @@ func forkRun(opts *ForkOptions) error { } if remote, err := remotes.FindByRepo(forkedRepo.RepoOwner(), forkedRepo.RepoName()); err == nil { if connectedToTerminal { - fmt.Fprintf(stderr, "%s Using existing remote %s\n", greenCheck, utils.Bold(remote.Name)) + fmt.Fprintf(stderr, "%s Using existing remote %s\n", utils.GreenCheck(), utils.Bold(remote.Name)) } return nil } @@ -226,7 +225,7 @@ func forkRun(opts *ForkOptions) error { return err } if connectedToTerminal { - fmt.Fprintf(stderr, "%s Renamed %s remote to %s\n", greenCheck, utils.Bold(remoteName), utils.Bold(renameTarget)) + fmt.Fprintf(stderr, "%s Renamed %s remote to %s\n", utils.GreenCheck(), utils.Bold(remoteName), utils.Bold(renameTarget)) } } @@ -238,7 +237,7 @@ func forkRun(opts *ForkOptions) error { } if connectedToTerminal { - fmt.Fprintf(stderr, "%s Added remote %s\n", greenCheck, utils.Bold(remoteName)) + fmt.Fprintf(stderr, "%s Added remote %s\n", utils.GreenCheck(), utils.Bold(remoteName)) } } } else { @@ -263,7 +262,7 @@ func forkRun(opts *ForkOptions) error { } if connectedToTerminal { - fmt.Fprintf(stderr, "%s Cloned fork\n", greenCheck) + fmt.Fprintf(stderr, "%s Cloned fork\n", utils.GreenCheck()) } } } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 0a25beac8..5f4ca58ec 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -43,7 +43,7 @@ func GraphQL(q string) Matcher { if !strings.EqualFold(req.Method, "POST") { return false } - if req.URL.Path != "/graphql" { + if req.URL.Path != "/graphql" && req.URL.Path != "/api/graphql" { return false } diff --git a/pkg/prompt/prompt.go b/pkg/prompt/prompt.go index 05683640e..169b0f42c 100644 --- a/pkg/prompt/prompt.go +++ b/pkg/prompt/prompt.go @@ -21,6 +21,10 @@ var Confirm = func(prompt string, result *bool) error { return survey.AskOne(p, result) } +var SurveyAskOne = func(p survey.Prompt, response interface{}, opts ...survey.AskOpt) error { + return survey.AskOne(p, response, opts...) +} + var SurveyAsk = func(qs []*survey.Question, response interface{}, opts ...survey.AskOpt) error { return survey.Ask(qs, response, opts...) } diff --git a/pkg/prompt/stubber.go b/pkg/prompt/stubber.go index e988962d5..77d37b350 100644 --- a/pkg/prompt/stubber.go +++ b/pkg/prompt/stubber.go @@ -8,15 +8,38 @@ import ( "github.com/AlecAivazis/survey/v2/core" ) -type askStubber struct { - Asks [][]*survey.Question - Count int - Stubs [][]*QuestionStub +type AskStubber struct { + Asks [][]*survey.Question + AskOnes []*survey.Prompt + Count int + OneCount int + Stubs [][]*QuestionStub + StubOnes []*PromptStub } -func InitAskStubber() (*askStubber, func()) { +func InitAskStubber() (*AskStubber, func()) { origSurveyAsk := SurveyAsk - as := askStubber{} + origSurveyAskOne := SurveyAskOne + as := AskStubber{} + + SurveyAskOne = func(p survey.Prompt, response interface{}, opts ...survey.AskOpt) error { + as.AskOnes = append(as.AskOnes, &p) + count := as.OneCount + as.OneCount += 1 + if count > len(as.StubOnes) { + panic(fmt.Sprintf("more asks than stubs. most recent call: %v", p)) + } + stubbedPrompt := as.StubOnes[count] + if stubbedPrompt.Default { + defaultValue := reflect.ValueOf(p).Elem().FieldByName("Default") + _ = core.WriteAnswer(response, "", defaultValue) + } else { + _ = core.WriteAnswer(response, "", stubbedPrompt.Value) + } + + return nil + } + SurveyAsk = func(qs []*survey.Question, response interface{}, opts ...survey.AskOpt) error { as.Asks = append(as.Asks, qs) count := as.Count @@ -44,17 +67,35 @@ func InitAskStubber() (*askStubber, func()) { } teardown := func() { SurveyAsk = origSurveyAsk + SurveyAskOne = origSurveyAskOne } return &as, teardown } +type PromptStub struct { + Value interface{} + Default bool +} + type QuestionStub struct { Name string Value interface{} Default bool } -func (as *askStubber) Stub(stubbedQuestions []*QuestionStub) { +func (as *AskStubber) StubOne(value interface{}) { + as.StubOnes = append(as.StubOnes, &PromptStub{ + Value: value, + }) +} + +func (as *AskStubber) StubOneDefault() { + as.StubOnes = append(as.StubOnes, &PromptStub{ + Default: true, + }) +} + +func (as *AskStubber) Stub(stubbedQuestions []*QuestionStub) { // A call to .Ask takes a list of questions; a stub is then a list of questions in the same order. as.Stubs = append(as.Stubs, stubbedQuestions) } diff --git a/utils/utils.go b/utils/utils.go index 20a5e55a8..cee56bf58 100644 --- a/utils/utils.go +++ b/utils/utils.go @@ -114,3 +114,7 @@ func DisplayURL(urlStr string) string { } return u.Hostname() + u.Path } + +func GreenCheck() string { + return Green("✓") +} From 2e7511cd182c9196c259ca480cb1dba33838d039 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 5 Aug 2020 12:01:47 -0500 Subject: [PATCH 3/7] add helpers to config type for hosts --- internal/config/config_type.go | 37 +++++++++++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/internal/config/config_type.go b/internal/config/config_type.go index 85e5c0d4a..523430c54 100644 --- a/internal/config/config_type.go +++ b/internal/config/config_type.go @@ -4,7 +4,9 @@ import ( "bytes" "errors" "fmt" + "sort" + "github.com/cli/cli/internal/ghinstance" "gopkg.in/yaml.v3" ) @@ -14,6 +16,8 @@ const defaultGitProtocol = "https" type Config interface { Get(string, string) (string, error) Set(string, string, string) error + UnsetHost(string) + Hosts() ([]string, error) Aliases() (*AliasConfig, error) Write() error } @@ -29,7 +33,7 @@ type HostConfig struct { // This type implements a low-level get/set config that is backed by an in-memory tree of Yaml // nodes. It allows us to interact with a yaml-based config programmatically, preserving any -// comments that were present when the yaml waas parsed. +// comments that were present when the yaml was parsed. type ConfigMap struct { Root *yaml.Node } @@ -236,6 +240,20 @@ func (c *fileConfig) Set(hostname, key, value string) error { } } +func (c *fileConfig) UnsetHost(hostname string) { + if hostname == "" { + return + } + + hostsEntry, err := c.FindEntry("hosts") + if err != nil { + return + } + + cm := ConfigMap{hostsEntry.ValueNode} + cm.RemoveEntry(hostname) +} + func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) { hosts, err := c.hostEntries() if err != nil { @@ -357,6 +375,23 @@ func (c *fileConfig) hostEntries() ([]*HostConfig, error) { return hostConfigs, nil } +// Hosts returns a list of all known hostnames configred in hosts.yml +func (c *fileConfig) Hosts() ([]string, error) { + entries, err := c.hostEntries() + if err != nil { + return nil, err + } + + hostnames := []string{} + for _, entry := range entries { + hostnames = append(hostnames, entry.Host) + } + + sort.SliceStable(hostnames, func(i, j int) bool { return hostnames[i] == ghinstance.Default() }) + + return hostnames, nil +} + func (c *fileConfig) makeConfigForHost(hostname string) *HostConfig { hostRoot := &yaml.Node{Kind: yaml.MappingNode} hostCfg := &HostConfig{ From da33b65d32c65a6d8040024ba60a6bb900ae2cfc Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 5 Aug 2020 12:02:51 -0500 Subject: [PATCH 4/7] remove vestigial httpclient struct member --- pkg/cmd/auth/login/login.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 60687dad8..72710f5d0 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -4,7 +4,6 @@ import ( "errors" "fmt" "io/ioutil" - "net/http" "os" "strings" @@ -21,9 +20,8 @@ import ( ) type LoginOptions struct { - HttpClient func() (*http.Client, error) - IO *iostreams.IOStreams - Config func() (config.Config, error) + IO *iostreams.IOStreams + Config func() (config.Config, error) Hostname string Token string @@ -32,9 +30,8 @@ type LoginOptions struct { func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Command { opts := &LoginOptions{ - HttpClient: f.HttpClient, - IO: f.IOStreams, - Config: f.Config, + IO: f.IOStreams, + Config: f.Config, } cmd := &cobra.Command{ From 9da75395005f92232c1793340a3106632b975263 Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 5 Aug 2020 12:03:08 -0500 Subject: [PATCH 5/7] gh auth logout --- command/root.go | 2 + pkg/cmd/auth/logout/logout.go | 157 +++++++++++++++++ pkg/cmd/auth/logout/logout_test.go | 259 +++++++++++++++++++++++++++++ 3 files changed, 418 insertions(+) create mode 100644 pkg/cmd/auth/logout/logout.go create mode 100644 pkg/cmd/auth/logout/logout_test.go diff --git a/command/root.go b/command/root.go index cd89f05d7..218b3a5eb 100644 --- a/command/root.go +++ b/command/root.go @@ -24,6 +24,7 @@ import ( apiCmd "github.com/cli/cli/pkg/cmd/api" authCmd "github.com/cli/cli/pkg/cmd/auth" authLoginCmd "github.com/cli/cli/pkg/cmd/auth/login" + authLogoutCmd "github.com/cli/cli/pkg/cmd/auth/logout" gistCreateCmd "github.com/cli/cli/pkg/cmd/gist/create" prCheckoutCmd "github.com/cli/cli/pkg/cmd/pr/checkout" prDiffCmd "github.com/cli/cli/pkg/cmd/pr/diff" @@ -138,6 +139,7 @@ func init() { RootCmd.AddCommand(authCmd.Cmd) authCmd.Cmd.AddCommand(authLoginCmd.NewCmdLogin(cmdFactory, nil)) + authCmd.Cmd.AddCommand(authLogoutCmd.NewCmdLogout(cmdFactory, nil)) resolvedBaseRepo := func() (ghrepo.Interface, error) { httpClient, err := cmdFactory.HttpClient() diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go new file mode 100644 index 000000000..744cd6a3f --- /dev/null +++ b/pkg/cmd/auth/logout/logout.go @@ -0,0 +1,157 @@ +package logout + +import ( + "errors" + "fmt" + "net/http" + + "github.com/AlecAivazis/survey/v2" + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/api" + "github.com/cli/cli/internal/config" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/cli/cli/utils" + "github.com/spf13/cobra" +) + +type LogoutOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + Config func() (config.Config, error) + + Hostname string +} + +func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Command { + opts := &LogoutOptions{ + HttpClient: f.HttpClient, + IO: f.IOStreams, + Config: f.Config, + } + + cmd := &cobra.Command{ + Use: "logout", + Args: cobra.ExactArgs(0), + Short: "Log out of a GitHub host", + Long: heredoc.Doc(`Remove authentication for a GitHub host. + + This command removes the authentication configuration for a host either specified + interactively or via --hostname. + `), + Example: heredoc.Doc(` + $ gh auth logout + # => select what host to log out of via a prompt + + $ gh auth logout --hostname enterprise.internal + # => log out of specified host + `), + RunE: func(cmd *cobra.Command, args []string) error { + if runF != nil { + return runF(opts) + } + + return logoutRun(opts) + }, + } + + cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The hostname of the GitHub instance to log out of") + + return cmd +} + +func logoutRun(opts *LogoutOptions) error { + isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() + + hostname := opts.Hostname + + if !isTTY && hostname == "" { + return errors.New("--hostname required when not attached to a terminal") + } + + showConfirm := isTTY && hostname == "" + + cfg, err := opts.Config() + if err != nil { + return err + } + + candidates, err := cfg.Hosts() + if err != nil { + return fmt.Errorf("not logged in to any hosts") + } + + if hostname == "" { + if len(candidates) == 1 { + hostname = candidates[0] + } else { + err = prompt.SurveyAskOne(&survey.Select{ + Message: "What account do you want to log out of?", + Options: candidates, + }, &hostname) + + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + } + } else { + var found bool + for _, c := range candidates { + if c == hostname { + found = true + break + } + } + + if !found { + return fmt.Errorf("not logged into %s", hostname) + } + } + + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(httpClient) + + username, err := api.CurrentLoginName(apiClient, hostname) + if err != nil { + // suppressing; the user is trying to delete this token and it might be bad. + // we'll see if the username is in the config and fall back to that. + username, _ = cfg.Get(hostname, "user") + } + + usernameStr := "" + if username != "" { + usernameStr = fmt.Sprintf(" account '%s'", username) + } + + if showConfirm { + var keepGoing bool + err := prompt.SurveyAskOne(&survey.Confirm{ + Message: fmt.Sprintf("Are you sure you want to log out of %s%s?", hostname, usernameStr), + Default: true, + }, &keepGoing) + if err != nil { + return fmt.Errorf("could not prompt: %w", err) + } + + if !keepGoing { + return nil + } + } + + cfg.UnsetHost(hostname) + err = cfg.Write() + if err != nil { + return fmt.Errorf("failed to write config, authentication configuration not updated: %w", err) + } + + if isTTY { + fmt.Fprintf(opts.IO.ErrOut, "%s Logged out of %s%s\n", + utils.GreenCheck(), utils.Bold(hostname), usernameStr) + } + + return nil +} diff --git a/pkg/cmd/auth/logout/logout_test.go b/pkg/cmd/auth/logout/logout_test.go new file mode 100644 index 000000000..83d13aad3 --- /dev/null +++ b/pkg/cmd/auth/logout/logout_test.go @@ -0,0 +1,259 @@ +package logout + +import ( + "bytes" + "net/http" + "regexp" + "testing" + + "github.com/cli/cli/internal/config" + "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/httpmock" + "github.com/cli/cli/pkg/iostreams" + "github.com/cli/cli/pkg/prompt" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func Test_NewCmdLogout(t *testing.T) { + tests := []struct { + name string + cli string + wants LogoutOptions + }{ + { + name: "with hostname", + cli: "--hostname harry.mason", + wants: LogoutOptions{ + Hostname: "harry.mason", + }, + }, + { + name: "no arguments", + cli: "", + wants: LogoutOptions{ + Hostname: "", + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: io, + } + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *LogoutOptions + cmd := NewCmdLogout(f, func(opts *LogoutOptions) error { + gotOpts = opts + return nil + }) + // TODO cobra hack-around + cmd.Flags().BoolP("help", "x", false, "") + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + assert.NoError(t, err) + + assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) + }) + + } +} + +func Test_logoutRun_tty(t *testing.T) { + tests := []struct { + name string + opts *LogoutOptions + askStubs func(*prompt.AskStubber) + cfgHosts []string + wantHosts string + wantErrOut *regexp.Regexp + wantErr *regexp.Regexp + }{ + { + name: "no arguments, multiple hosts", + opts: &LogoutOptions{}, + cfgHosts: []string{"cheryl.mason", "github.com"}, + wantHosts: "cheryl.mason:\n oauth_token: abc123\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne("github.com") + as.StubOne(true) + }, + wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + }, + { + name: "no arguments, one host", + opts: &LogoutOptions{}, + cfgHosts: []string{"github.com"}, + askStubs: func(as *prompt.AskStubber) { + as.StubOne(true) + }, + wantErrOut: regexp.MustCompile(`Logged out of github.com account 'cybilb'`), + }, + { + name: "no arguments, no hosts", + opts: &LogoutOptions{}, + wantErr: regexp.MustCompile(`not logged in to any hosts`), + }, + { + name: "hostname", + opts: &LogoutOptions{ + Hostname: "cheryl.mason", + }, + cfgHosts: []string{"cheryl.mason", "github.com"}, + wantHosts: "github.com:\n oauth_token: abc123\n", + askStubs: func(as *prompt.AskStubber) { + as.StubOne(true) + }, + wantErrOut: regexp.MustCompile(`Logged out of cheryl.mason account 'cybilb'`), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, stderr := iostreams.Test() + + io.SetStdinTTY(true) + io.SetStdoutTTY(true) + + tt.opts.IO = io + cfg := config.NewBlankConfig() + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + + for _, hostname := range tt.cfgHosts { + _ = cfg.Set(hostname, "oauth_token", "abc123") + } + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`query UserCurrent\b`), + httpmock.StringResponse(`{"data":{"viewer":{"login":"cybilb"}}}`)) + + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + as, teardown := prompt.InitAskStubber() + defer teardown() + if tt.askStubs != nil { + tt.askStubs(as) + } + + err := logoutRun(tt.opts) + assert.Equal(t, tt.wantErr == nil, err == nil) + if err != nil { + if tt.wantErr != nil { + assert.True(t, tt.wantErr.MatchString(err.Error())) + return + } else { + t.Fatalf("unexpected error: %s", err) + } + } + + if tt.wantErrOut == nil { + assert.Equal(t, "", stderr.String()) + } else { + assert.True(t, tt.wantErrOut.MatchString(stderr.String())) + } + + assert.Equal(t, tt.wantHosts, hostsBuf.String()) + reg.Verify(t) + }) + } +} + +func Test_logoutRun_nontty(t *testing.T) { + tests := []struct { + name string + opts *LogoutOptions + cfgHosts []string + wantHosts string + wantErr *regexp.Regexp + }{ + { + name: "no arguments", + wantErr: regexp.MustCompile(`hostname required when not`), + opts: &LogoutOptions{}, + }, + { + name: "hostname, one host", + opts: &LogoutOptions{ + Hostname: "harry.mason", + }, + cfgHosts: []string{"harry.mason"}, + }, + { + name: "hostname, multiple hosts", + opts: &LogoutOptions{ + Hostname: "harry.mason", + }, + cfgHosts: []string{"harry.mason", "cheryl.mason"}, + wantHosts: "cheryl.mason:\n oauth_token: abc123\n", + }, + { + name: "hostname, no hosts", + opts: &LogoutOptions{ + Hostname: "harry.mason", + }, + wantErr: regexp.MustCompile(`not logged in to any hosts`), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + io, _, _, stderr := iostreams.Test() + + io.SetStdinTTY(false) + io.SetStdoutTTY(false) + + tt.opts.IO = io + cfg := config.NewBlankConfig() + tt.opts.Config = func() (config.Config, error) { + return cfg, nil + } + + for _, hostname := range tt.cfgHosts { + _ = cfg.Set(hostname, "oauth_token", "abc123") + } + + reg := &httpmock.Registry{} + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + + mainBuf := bytes.Buffer{} + hostsBuf := bytes.Buffer{} + defer config.StubWriteConfig(&mainBuf, &hostsBuf)() + + err := logoutRun(tt.opts) + assert.Equal(t, tt.wantErr == nil, err == nil) + if err != nil { + if tt.wantErr != nil { + assert.True(t, tt.wantErr.MatchString(err.Error())) + return + } else { + t.Fatalf("unexpected error: %s", err) + } + } + + assert.Equal(t, "", stderr.String()) + + assert.Equal(t, tt.wantHosts, hostsBuf.String()) + reg.Verify(t) + }) + } +} From 27765f9987976ac20c420800a50cd5253df4afe7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=ADn=20Montes?= Date: Fri, 7 Aug 2020 09:01:24 +0200 Subject: [PATCH 6/7] Generalize REST error parsing --- pkg/cmd/api/api.go | 38 +++++++++++++++++++++++++++++--------- pkg/cmd/api/api_test.go | 11 +++++++++++ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/api/api.go b/pkg/cmd/api/api.go index 8900f3f39..27810187b 100644 --- a/pkg/cmd/api/api.go +++ b/pkg/cmd/api/api.go @@ -426,23 +426,43 @@ func parseErrorResponse(r io.Reader, statusCode int) (io.Reader, string, error) var parsedBody struct { Message string - Errors []struct { - Message string - } + Errors []json.RawMessage } err = json.Unmarshal(b, &parsedBody) if err != nil { return r, "", err } - if parsedBody.Message != "" { return bodyCopy, fmt.Sprintf("%s (HTTP %d)", parsedBody.Message, statusCode), nil - } else if len(parsedBody.Errors) > 0 { - msgs := make([]string, len(parsedBody.Errors)) - for i, e := range parsedBody.Errors { - msgs[i] = e.Message + } + + type errorMessage struct { + Message string + } + var errors []string + for _, rawErr := range parsedBody.Errors { + if len(rawErr) == 0 { + continue } - return bodyCopy, strings.Join(msgs, "\n"), nil + if rawErr[0] == '{' { + var objectError errorMessage + err := json.Unmarshal(rawErr, &objectError) + if err != nil { + return r, "", err + } + errors = append(errors, objectError.Message) + } else if rawErr[0] == '"' { + var stringError string + err := json.Unmarshal(rawErr, &stringError) + if err != nil { + return r, "", err + } + errors = append(errors, stringError) + } + } + + if len(errors) > 0 { + return bodyCopy, strings.Join(errors, "\n"), nil } return bodyCopy, "", nil diff --git a/pkg/cmd/api/api_test.go b/pkg/cmd/api/api_test.go index f590d93b4..e5aa9f31d 100644 --- a/pkg/cmd/api/api_test.go +++ b/pkg/cmd/api/api_test.go @@ -264,6 +264,17 @@ func Test_apiRun(t *testing.T) { stdout: `{"message": "THIS IS FINE"}`, stderr: "gh: THIS IS FINE (HTTP 400)\n", }, + { + name: "REST string errors", + httpResponse: &http.Response{ + StatusCode: 400, + Body: ioutil.NopCloser(bytes.NewBufferString(`{"errors": ["ALSO", "FINE"]}`)), + Header: http.Header{"Content-Type": []string{"application/json; charset=utf-8"}}, + }, + err: cmdutil.SilentError, + stdout: `{"errors": ["ALSO", "FINE"]}`, + stderr: "gh: ALSO\nFINE\n", + }, { name: "GraphQL error", options: ApiOptions{ From cee93b08f072f2abeb29c2baa5c5d3e93f496df0 Mon Sep 17 00:00:00 2001 From: Lee Reilly Date: Sat, 8 Aug 2020 17:13:28 -0700 Subject: [PATCH 7/7] Remove period from command description for consistency --- command/alias.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/command/alias.go b/command/alias.go index 7f02b03d5..2a9e13fbc 100644 --- a/command/alias.go +++ b/command/alias.go @@ -193,7 +193,7 @@ func aliasList(cmd *cobra.Command, args []string) error { var aliasDeleteCmd = &cobra.Command{ Use: "delete ", - Short: "Delete an alias.", + Short: "Delete an alias", Args: cobra.ExactArgs(1), RunE: aliasDelete, }