Skip to content

Commit be9f011

Browse files
mislavmislav
committed
Tweak auth flow re: interactivity
Fixes non-interactive login flow and make sure "prompt" configuration is respected by never prompting if it was explicitly disabled. No longer asks to press Enter again after "Authentication complete" message, since that didn't provide any value to the user.
1 parent a408f24 commit be9f011

File tree

5 files changed

+49
-58
lines changed

5 files changed

+49
-58
lines changed

‎internal/authflow/flow.go‎

Lines changed: 19 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,11 @@ type iconfig interface {
2727
Write() error
2828
}
2929

30-
func AuthFlowWithConfig(cfg iconfig, IO *iostreams.IOStreams, hostname, notice string, additionalScopes []string, interactive bool) (string, error) {
30+
func AuthFlowWithConfig(cfg iconfig, IO *iostreams.IOStreams, hostname, notice string, additionalScopes []string, isInteractive bool) (string, error) {
3131
// TODO this probably shouldn't live in this package. It should probably be in a new package that
3232
// depends on both iostreams and config.
33-
stderr := IO.ErrOut
34-
cs := IO.ColorScheme()
3533

36-
token, userLogin, err := authFlow(hostname, IO, notice, additionalScopes, interactive)
34+
token, userLogin, err := authFlow(hostname, IO, notice, additionalScopes, isInteractive)
3735
if err != nil {
3836
return "", err
3937
}
@@ -47,22 +45,10 @@ func AuthFlowWithConfig(cfg iconfig, IO *iostreams.IOStreams, hostname, notice s
4745
return "", err
4846
}
4947

50-
err = cfg.Write()
51-
if err != nil {
52-
return "", err
53-
}
54-
55-
if interactive {
56-
fmt.Fprintf(stderr, "%s Authentication complete. %s to continue...\n",
57-
cs.SuccessIcon(), cs.Bold("Press Enter"))
58-
_ = waitForEnter(IO.In)
59-
} else {
60-
fmt.Fprintf(stderr, "%s Authentication complete.\n", cs.SuccessIcon())
61-
}
62-
return token, nil
48+
return token, cfg.Write()
6349
}
6450

65-
func authFlow(oauthHost string, IO *iostreams.IOStreams, notice string, additionalScopes []string, interactive bool) (string, string, error) {
51+
func authFlow(oauthHost string, IO *iostreams.IOStreams, notice string, additionalScopes []string, isInteractive bool) (string, string, error) {
6652
w := IO.ErrOut
6753
cs := IO.ColorScheme()
6854

@@ -93,24 +79,25 @@ func authFlow(oauthHost string, IO *iostreams.IOStreams, notice string, addition
9379
return nil
9480
},
9581
BrowseURL: func(url string) error {
96-
if interactive {
97-
fmt.Fprintf(w, "- %s to open %s in your browser... ", cs.Bold("Press Enter"), oauthHost)
98-
_ = waitForEnter(IO.In)
99-
100-
// FIXME: read the browser from cmd Factory rather than recreating it
101-
browser := cmdutil.NewBrowser(os.Getenv("BROWSER"), IO.Out, IO.ErrOut)
102-
if err := browser.Browse(url); err != nil {
103-
fmt.Fprintf(w, "%s Failed opening a web browser at %s\n", cs.Red("!"), url)
104-
fmt.Fprintf(w, " %s\n", err)
105-
fmt.Fprint(w, " Please try entering the URL in your browser manually\n")
106-
}
107-
} else {
108-
fmt.Fprintf(w, "- Open this URL in your browser: %s", cs.Bold(url))
82+
if !isInteractive {
83+
fmt.Fprintf(w, "%s to continue in your web browser: %s\n", cs.Bold("Open this URL"), url)
84+
return nil
85+
}
86+
87+
fmt.Fprintf(w, "%s to open %s in your browser... ", cs.Bold("Press Enter"), oauthHost)
88+
_ = waitForEnter(IO.In)
89+
90+
// FIXME: read the browser from cmd Factory rather than recreating it
91+
browser := cmdutil.NewBrowser(os.Getenv("BROWSER"), IO.Out, IO.ErrOut)
92+
if err := browser.Browse(url); err != nil {
93+
fmt.Fprintf(w, "%s Failed opening a web browser at %s\n", cs.Red("!"), url)
94+
fmt.Fprintf(w, " %s\n", err)
95+
fmt.Fprint(w, " Please try entering the URL in your browser manually\n")
10996
}
11097
return nil
11198
},
11299
WriteSuccessHTML: func(w io.Writer) {
113-
fmt.Fprintln(w, oauthSuccessPage)
100+
fmt.Fprint(w, oauthSuccessPage)
114101
},
115102
HTTPClient: httpClient,
116103
Stdin: IO.In,

‎pkg/cmd/auth/login/login.go‎

Lines changed: 14 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -68,37 +68,34 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm
6868
$ gh auth login --hostname enterprise.internal
6969
`),
7070
RunE: func(cmd *cobra.Command, args []string) error {
71-
if !opts.IO.CanPrompt() && !(tokenStdin || opts.Web) {
72-
return cmdutil.FlagErrorf("--web or --with-token required when not running interactively")
73-
}
74-
7571
if tokenStdin && opts.Web {
76-
return cmdutil.FlagErrorf("specify only one of --web or --with-token")
72+
return cmdutil.FlagErrorf("specify only one of `--web` or `--with-token`")
73+
}
74+
if tokenStdin && len(opts.Scopes) > 0 {
75+
return cmdutil.FlagErrorf("specify only one of `--scopes` or `--with-token`")
7776
}
7877

7978
if tokenStdin {
8079
defer opts.IO.In.Close()
8180
token, err := ioutil.ReadAll(opts.IO.In)
8281
if err != nil {
83-
return fmt.Errorf("failed to read token from STDIN: %w", err)
82+
return fmt.Errorf("failed to read token from standard input: %w", err)
8483
}
8584
opts.Token = strings.TrimSpace(string(token))
8685
}
8786

88-
if opts.IO.CanPrompt() && opts.Token == "" && !opts.Web {
87+
if opts.IO.CanPrompt() && opts.Token == "" {
8988
opts.Interactive = true
9089
}
9190

9291
if cmd.Flags().Changed("hostname") {
9392
if err := ghinstance.HostnameValidator(opts.Hostname); err != nil {
94-
return cmdutil.FlagErrorf("error parsing --hostname: %w", err)
93+
return cmdutil.FlagErrorf("error parsing hostname: %w", err)
9594
}
9695
}
9796

98-
if !opts.Interactive {
99-
if opts.Hostname == "" {
100-
opts.Hostname = ghinstance.Default()
101-
}
97+
if opts.Hostname == "" && (!opts.Interactive || opts.Web) {
98+
opts.Hostname = ghinstance.Default()
10299
}
103100

104101
opts.MainExecutable = f.Executable()
@@ -125,15 +122,11 @@ func loginRun(opts *LoginOptions) error {
125122
}
126123

127124
hostname := opts.Hostname
128-
if hostname == "" {
129-
if opts.Interactive {
130-
var err error
131-
hostname, err = promptForHostname()
132-
if err != nil {
133-
return err
134-
}
135-
} else {
136-
return errors.New("must specify --hostname")
125+
if opts.Interactive && hostname == "" {
126+
var err error
127+
hostname, err = promptForHostname()
128+
if err != nil {
129+
return err
137130
}
138131
}
139132

‎pkg/cmd/auth/login/login_test.go‎

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -50,13 +50,19 @@ func Test_NewCmdLogin(t *testing.T) {
5050
name: "nontty, hostname",
5151
stdinTTY: false,
5252
cli: "--hostname claire.redfield",
53-
wantsErr: true,
53+
wants: LoginOptions{
54+
Hostname: "claire.redfield",
55+
Token: "",
56+
},
5457
},
5558
{
5659
name: "nontty",
5760
stdinTTY: false,
5861
cli: "",
59-
wantsErr: true,
62+
wants: LoginOptions{
63+
Hostname: "github.com",
64+
Token: "",
65+
},
6066
},
6167
{
6268
name: "nontty, with-token, hostname",
@@ -102,8 +108,9 @@ func Test_NewCmdLogin(t *testing.T) {
102108
stdinTTY: true,
103109
cli: "--web",
104110
wants: LoginOptions{
105-
Hostname: "github.com",
106-
Web: true,
111+
Hostname: "github.com",
112+
Web: true,
113+
Interactive: true,
107114
},
108115
},
109116
{

‎pkg/cmd/auth/refresh/refresh.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,9 @@ func refreshRun(opts *RefreshOptions) error {
158158
return err
159159
}
160160

161+
cs := opts.IO.ColorScheme()
162+
fmt.Fprintf(opts.IO.ErrOut, "%s Authentication complete.\n", cs.SuccessIcon())
163+
161164
if credentialFlow.ShouldSetup() {
162165
username, _ := cfg.Get(hostname, "user")
163166
password, _ := cfg.Get(hostname, "oauth_token")

‎pkg/cmd/auth/shared/login_flow.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ func Login(opts *LoginOptions) error {
9999
var authMode int
100100
if opts.Web {
101101
authMode = 0
102-
} else {
102+
} else if opts.Interactive {
103103
err := prompt.SurveyAskOne(&survey.Select{
104104
Message: "How would you like to authenticate GitHub CLI?",
105105
Options: []string{
@@ -121,6 +121,7 @@ func Login(opts *LoginOptions) error {
121121
if err != nil {
122122
return fmt.Errorf("failed to authenticate via web browser: %w", err)
123123
}
124+
fmt.Fprintf(opts.IO.ErrOut, "%s Authentication complete.\n", cs.SuccessIcon())
124125
userValidated = true
125126
} else {
126127
minimumScopes := append([]string{"repo", "read:org"}, additionalScopes...)

0 commit comments

Comments
 (0)