X Tutup
Skip to content

Commit c9407b2

Browse files
mislavmislav
committed
More descriptive error when aborting auth due to environment variables
Old message: read-only token in GH_TOKEN cannot be modified This message was vague and some users did not understand that this refers to the value that is read from environment variables. New message: $ GH_TOKEN=123 ghd auth login -h github.com The value of the GH_TOKEN environment variable is being used for authentication. To have GitHub CLI store credentials instead, first clear the value from the environment.
1 parent b5366c6 commit c9407b2

File tree

6 files changed

+150
-82
lines changed

6 files changed

+150
-82
lines changed

‎internal/config/from_env.go‎

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,14 @@ const (
1414
GITHUB_ENTERPRISE_TOKEN = "GITHUB_ENTERPRISE_TOKEN"
1515
)
1616

17+
type ReadOnlyEnvError struct {
18+
Variable string
19+
}
20+
21+
func (e *ReadOnlyEnvError) Error() string {
22+
return fmt.Sprintf("read-only value in %s", e.Variable)
23+
}
24+
1725
func InheritEnv(c Config) Config {
1826
return &envConfig{Config: c}
1927
}
@@ -56,7 +64,7 @@ func (c *envConfig) GetWithSource(hostname, key string) (string, string, error)
5664
func (c *envConfig) CheckWriteable(hostname, key string) error {
5765
if hostname != "" && key == "oauth_token" {
5866
if token, env := AuthTokenFromEnv(hostname); token != "" {
59-
return fmt.Errorf("read-only token in %s cannot be modified", env)
67+
return &ReadOnlyEnvError{Variable: env}
6068
}
6169
}
6270

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

Lines changed: 66 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -120,70 +120,56 @@ func loginRun(opts *LoginOptions) error {
120120
return err
121121
}
122122

123-
if opts.Token != "" {
124-
// I chose to not error on existing host here; my thinking is that for --with-token the user
125-
// probably doesn't care if a token is overwritten since they have a token in hand they
126-
// explicitly want to use.
127-
if opts.Hostname == "" {
128-
return errors.New("empty hostname would leak oauth_token")
123+
hostname := opts.Hostname
124+
if hostname == "" {
125+
if opts.Interactive {
126+
var err error
127+
hostname, err = promptForHostname()
128+
if err != nil {
129+
return err
130+
}
131+
} else {
132+
return errors.New("must specify --hostname")
129133
}
134+
}
130135

131-
err := cfg.Set(opts.Hostname, "oauth_token", opts.Token)
132-
if err != nil {
133-
return err
136+
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
137+
var roErr *config.ReadOnlyEnvError
138+
if errors.As(err, &roErr) {
139+
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
140+
fmt.Fprint(opts.IO.ErrOut, "To have GitHub CLI store credentials instead, first clear the value from the environment.\n")
141+
return cmdutil.SilentError
134142
}
143+
return err
144+
}
135145

136-
err = shared.ValidateHostCfg(opts.Hostname, cfg)
146+
if opts.Token != "" {
147+
err := cfg.Set(hostname, "oauth_token", opts.Token)
137148
if err != nil {
138149
return err
139150
}
140151

141-
return cfg.Write()
142-
}
143-
144-
// TODO consider explicitly telling survey what io to use since it's implicit right now
145-
146-
hostname := opts.Hostname
147-
148-
if hostname == "" {
149-
var hostType int
150-
err := prompt.SurveyAskOne(&survey.Select{
151-
Message: "What account do you want to log into?",
152-
Options: []string{
153-
"GitHub.com",
154-
"GitHub Enterprise Server",
155-
},
156-
}, &hostType)
157-
152+
apiClient, err := shared.ClientFromCfg(hostname, cfg)
158153
if err != nil {
159-
return fmt.Errorf("could not prompt: %w", err)
154+
return err
160155
}
161156

162-
isEnterprise := hostType == 1
163-
164-
hostname = ghinstance.Default()
165-
if isEnterprise {
166-
err := prompt.SurveyAskOne(&survey.Input{
167-
Message: "GHE hostname:",
168-
}, &hostname, survey.WithValidator(ghinstance.HostnameValidator))
169-
if err != nil {
170-
return fmt.Errorf("could not prompt: %w", err)
171-
}
157+
if err := apiClient.HasMinimumScopes(hostname); err != nil {
158+
return fmt.Errorf("error validating token: %w", err)
172159
}
173-
}
174160

175-
fmt.Fprintf(opts.IO.ErrOut, "- Logging into %s\n", hostname)
161+
return cfg.Write()
162+
}
176163

177164
existingToken, _ := cfg.Get(hostname, "oauth_token")
178165

179166
if existingToken != "" && opts.Interactive {
180-
err := shared.ValidateHostCfg(hostname, cfg)
181-
if err == nil {
182-
apiClient, err := shared.ClientFromCfg(hostname, cfg)
183-
if err != nil {
184-
return err
185-
}
167+
apiClient, err := shared.ClientFromCfg(hostname, cfg)
168+
if err != nil {
169+
return err
170+
}
186171

172+
if err := apiClient.HasMinimumScopes(hostname); err == nil {
187173
username, err := api.CurrentLoginName(apiClient, hostname)
188174
if err != nil {
189175
return fmt.Errorf("error using api: %w", err)
@@ -206,10 +192,6 @@ func loginRun(opts *LoginOptions) error {
206192
}
207193
}
208194

209-
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
210-
return err
211-
}
212-
213195
var authMode int
214196
if opts.Web {
215197
authMode = 0
@@ -244,19 +226,19 @@ func loginRun(opts *LoginOptions) error {
244226
return fmt.Errorf("could not prompt: %w", err)
245227
}
246228

247-
if hostname == "" {
248-
return errors.New("empty hostname would leak oauth_token")
249-
}
250-
251229
err = cfg.Set(hostname, "oauth_token", token)
252230
if err != nil {
253231
return err
254232
}
255233

256-
err = shared.ValidateHostCfg(hostname, cfg)
234+
apiClient, err := shared.ClientFromCfg(hostname, cfg)
257235
if err != nil {
258236
return err
259237
}
238+
239+
if err := apiClient.HasMinimumScopes(hostname); err != nil {
240+
return fmt.Errorf("error validating token: %w", err)
241+
}
260242
}
261243

262244
cs := opts.IO.ColorScheme()
@@ -322,6 +304,35 @@ func loginRun(opts *LoginOptions) error {
322304
return nil
323305
}
324306

307+
func promptForHostname() (string, error) {
308+
var hostType int
309+
err := prompt.SurveyAskOne(&survey.Select{
310+
Message: "What account do you want to log into?",
311+
Options: []string{
312+
"GitHub.com",
313+
"GitHub Enterprise Server",
314+
},
315+
}, &hostType)
316+
317+
if err != nil {
318+
return "", fmt.Errorf("could not prompt: %w", err)
319+
}
320+
321+
isEnterprise := hostType == 1
322+
323+
hostname := ghinstance.Default()
324+
if isEnterprise {
325+
err := prompt.SurveyAskOne(&survey.Input{
326+
Message: "GHE hostname:",
327+
}, &hostname, survey.WithValidator(ghinstance.HostnameValidator))
328+
if err != nil {
329+
return "", fmt.Errorf("could not prompt: %w", err)
330+
}
331+
}
332+
333+
return hostname, nil
334+
}
335+
325336
func getAccessTokenTip(hostname string) string {
326337
ghHostname := hostname
327338
if ghHostname == "" {

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

Lines changed: 63 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,11 @@ package login
33
import (
44
"bytes"
55
"net/http"
6+
"os"
67
"regexp"
78
"testing"
89

10+
"github.com/MakeNowJust/heredoc"
911
"github.com/cli/cli/api"
1012
"github.com/cli/cli/internal/config"
1113
"github.com/cli/cli/pkg/cmd/auth/shared"
@@ -189,18 +191,23 @@ func Test_NewCmdLogin(t *testing.T) {
189191

190192
func Test_loginRun_nontty(t *testing.T) {
191193
tests := []struct {
192-
name string
193-
opts *LoginOptions
194-
httpStubs func(*httpmock.Registry)
195-
wantHosts string
196-
wantErr string
194+
name string
195+
opts *LoginOptions
196+
httpStubs func(*httpmock.Registry)
197+
env map[string]string
198+
wantHosts string
199+
wantErr string
200+
wantStderr string
197201
}{
198202
{
199203
name: "with token",
200204
opts: &LoginOptions{
201205
Hostname: "github.com",
202206
Token: "abc123",
203207
},
208+
httpStubs: func(reg *httpmock.Registry) {
209+
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org"))
210+
},
204211
wantHosts: "github.com:\n oauth_token: abc123\n",
205212
},
206213
{
@@ -223,7 +230,7 @@ func Test_loginRun_nontty(t *testing.T) {
223230
httpStubs: func(reg *httpmock.Registry) {
224231
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("read:org"))
225232
},
226-
wantErr: `could not validate token: missing required scope 'repo'`,
233+
wantErr: `error validating token: missing required scope 'repo'`,
227234
},
228235
{
229236
name: "missing read scope",
@@ -234,7 +241,7 @@ func Test_loginRun_nontty(t *testing.T) {
234241
httpStubs: func(reg *httpmock.Registry) {
235242
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo"))
236243
},
237-
wantErr: `could not validate token: missing required scope 'read:org'`,
244+
wantErr: `error validating token: missing required scope 'read:org'`,
238245
},
239246
{
240247
name: "has admin scope",
@@ -247,6 +254,36 @@ func Test_loginRun_nontty(t *testing.T) {
247254
},
248255
wantHosts: "github.com:\n oauth_token: abc456\n",
249256
},
257+
{
258+
name: "github.com token from environment",
259+
opts: &LoginOptions{
260+
Hostname: "github.com",
261+
Token: "abc456",
262+
},
263+
env: map[string]string{
264+
"GH_TOKEN": "value_from_env",
265+
},
266+
wantErr: "SilentError",
267+
wantStderr: heredoc.Doc(`
268+
The value of the GH_TOKEN environment variable is being used for authentication.
269+
To have GitHub CLI store credentials instead, first clear the value from the environment.
270+
`),
271+
},
272+
{
273+
name: "GHE token from environment",
274+
opts: &LoginOptions{
275+
Hostname: "ghe.io",
276+
Token: "abc456",
277+
},
278+
env: map[string]string{
279+
"GH_ENTERPRISE_TOKEN": "value_from_env",
280+
},
281+
wantErr: "SilentError",
282+
wantStderr: heredoc.Doc(`
283+
The value of the GH_ENTERPRISE_TOKEN environment variable is being used for authentication.
284+
To have GitHub CLI store credentials instead, first clear the value from the environment.
285+
`),
286+
},
250287
}
251288

252289
for _, tt := range tests {
@@ -256,7 +293,8 @@ func Test_loginRun_nontty(t *testing.T) {
256293
io.SetStdoutTTY(false)
257294

258295
tt.opts.Config = func() (config.Config, error) {
259-
return config.NewBlankConfig(), nil
296+
cfg := config.NewBlankConfig()
297+
return config.InheritEnv(cfg), nil
260298
}
261299

262300
tt.opts.IO = io
@@ -271,10 +309,23 @@ func Test_loginRun_nontty(t *testing.T) {
271309
return api.NewClientFromHTTP(httpClient), nil
272310
}
273311

312+
old_GH_TOKEN := os.Getenv("GH_TOKEN")
313+
os.Setenv("GH_TOKEN", tt.env["GH_TOKEN"])
314+
old_GITHUB_TOKEN := os.Getenv("GITHUB_TOKEN")
315+
os.Setenv("GITHUB_TOKEN", tt.env["GITHUB_TOKEN"])
316+
old_GH_ENTERPRISE_TOKEN := os.Getenv("GH_ENTERPRISE_TOKEN")
317+
os.Setenv("GH_ENTERPRISE_TOKEN", tt.env["GH_ENTERPRISE_TOKEN"])
318+
old_GITHUB_ENTERPRISE_TOKEN := os.Getenv("GITHUB_ENTERPRISE_TOKEN")
319+
os.Setenv("GITHUB_ENTERPRISE_TOKEN", tt.env["GITHUB_ENTERPRISE_TOKEN"])
320+
defer func() {
321+
os.Setenv("GH_TOKEN", old_GH_TOKEN)
322+
os.Setenv("GITHUB_TOKEN", old_GITHUB_TOKEN)
323+
os.Setenv("GH_ENTERPRISE_TOKEN", old_GH_ENTERPRISE_TOKEN)
324+
os.Setenv("GITHUB_ENTERPRISE_TOKEN", old_GITHUB_ENTERPRISE_TOKEN)
325+
}()
326+
274327
if tt.httpStubs != nil {
275328
tt.httpStubs(reg)
276-
} else {
277-
reg.Register(httpmock.REST("GET", ""), httpmock.ScopesResponder("repo,read:org"))
278329
}
279330

280331
mainBuf := bytes.Buffer{}
@@ -289,7 +340,7 @@ func Test_loginRun_nontty(t *testing.T) {
289340
}
290341

291342
assert.Equal(t, "", stdout.String())
292-
assert.Equal(t, "", stderr.String())
343+
assert.Equal(t, tt.wantStderr, stderr.String())
293344
assert.Equal(t, tt.wantHosts, hostsBuf.String())
294345
reg.Verify(t)
295346
})
@@ -325,7 +376,7 @@ func Test_loginRun_Survey(t *testing.T) {
325376
as.StubOne(false) // do not continue
326377
},
327378
wantHosts: "", // nothing should have been written to hosts
328-
wantErrOut: regexp.MustCompile("Logging into github.com"),
379+
wantErrOut: nil,
329380
},
330381
{
331382
name: "hostname set",

‎pkg/cmd/auth/logout/logout.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,12 @@ func logoutRun(opts *LogoutOptions) error {
105105
}
106106

107107
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
108+
var roErr *config.ReadOnlyEnvError
109+
if errors.As(err, &roErr) {
110+
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
111+
fmt.Fprint(opts.IO.ErrOut, "To erase credentials stored in GitHub CLI, first clear the value from the environment.\n")
112+
return cmdutil.SilentError
113+
}
108114
return err
109115
}
110116

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -112,6 +112,12 @@ func refreshRun(opts *RefreshOptions) error {
112112
}
113113

114114
if err := cfg.CheckWriteable(hostname, "oauth_token"); err != nil {
115+
var roErr *config.ReadOnlyEnvError
116+
if errors.As(err, &roErr) {
117+
fmt.Fprintf(opts.IO.ErrOut, "The value of the %s environment variable is being used for authentication.\n", roErr.Variable)
118+
fmt.Fprint(opts.IO.ErrOut, "To refresh credentials stored in GitHub CLI, first clear the value from the environment.\n")
119+
return cmdutil.SilentError
120+
}
115121
return err
116122
}
117123

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

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,20 +8,6 @@ import (
88
"github.com/cli/cli/internal/config"
99
)
1010

11-
func ValidateHostCfg(hostname string, cfg config.Config) error {
12-
apiClient, err := ClientFromCfg(hostname, cfg)
13-
if err != nil {
14-
return err
15-
}
16-
17-
err = apiClient.HasMinimumScopes(hostname)
18-
if err != nil {
19-
return fmt.Errorf("could not validate token: %w", err)
20-
}
21-
22-
return nil
23-
}
24-
2511
var ClientFromCfg = func(hostname string, cfg config.Config) (*api.Client, error) {
2612
var opts []api.ClientOption
2713

0 commit comments

Comments
 (0)
X Tutup