diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index ab13e0552..fc068440c 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -28,8 +28,8 @@ type RefreshOptions struct { Scopes []string AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool, bool) error - Interactive bool - SecureStorage bool + Interactive bool + InsecureStorage bool } func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra.Command { @@ -90,7 +90,12 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. cmd.Flags().StringVarP(&opts.Hostname, "hostname", "h", "", "The GitHub host to use for authentication") cmd.Flags().StringSliceVarP(&opts.Scopes, "scopes", "s", nil, "Additional authentication scopes for gh to have") - cmd.Flags().BoolVarP(&opts.SecureStorage, "secure-storage", "", false, "Save authentication credentials in secure credential store") + // secure storage became the default on 2023/4/04; this flag is left as a no-op for backwards compatibility + var secureStorage bool + cmd.Flags().BoolVar(&secureStorage, "secure-storage", false, "Save authentication credentials in secure credential store") + _ = cmd.Flags().MarkHidden("secure-storage") + + cmd.Flags().BoolVarP(&opts.InsecureStorage, "insecure-storage", "", false, "Save authentication credentials in plain text instead of credential store") return cmd } @@ -139,7 +144,7 @@ func refreshRun(opts *RefreshOptions) error { } var additionalScopes []string - if oldToken, source := authCfg.Token(hostname); oldToken != "" { + if oldToken, _ := authCfg.Token(hostname); oldToken != "" { if oldScopes, err := shared.GetScopes(opts.HttpClient, hostname, oldToken); err == nil { for _, s := range strings.Split(oldScopes, ",") { s = strings.TrimSpace(s) @@ -148,13 +153,6 @@ func refreshRun(opts *RefreshOptions) error { } } } - - // If previous token was stored in secure storage assume - // user wants to continue storing it there even if not - // explicitly stated with the secure storage flag. - if source == "keyring" { - opts.SecureStorage = true - } } credentialFlow := &shared.GitCredentialFlow{ @@ -170,7 +168,7 @@ func refreshRun(opts *RefreshOptions) error { additionalScopes = append(additionalScopes, credentialFlow.Scopes()...) } - if err := opts.AuthFlow(authCfg, opts.IO, hostname, append(opts.Scopes, additionalScopes...), opts.Interactive, opts.SecureStorage); err != nil { + if err := opts.AuthFlow(authCfg, opts.IO, hostname, append(opts.Scopes, additionalScopes...), opts.Interactive, !opts.InsecureStorage); err != nil { return err } diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 7d14cdaa1..8743f1835 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -86,11 +86,17 @@ func Test_NewCmdRefresh(t *testing.T) { }, }, { - name: "secure storage", + name: "secure storage", + tty: true, + cli: "--secure-storage", + wants: RefreshOptions{}, + }, + { + name: "insecure storage", tty: true, - cli: "--secure-storage", + cli: "--insecure-storage", wants: RefreshOptions{ - SecureStorage: true, + InsecureStorage: true, }, }, } @@ -178,8 +184,9 @@ func Test_refreshRun(t *testing.T) { Hostname: "obed.morton", }, wantAuthArgs: authArgs{ - hostname: "obed.morton", - scopes: nil, + hostname: "obed.morton", + scopes: nil, + secureStorage: true, }, }, { @@ -191,8 +198,9 @@ func Test_refreshRun(t *testing.T) { Hostname: "", }, wantAuthArgs: authArgs{ - hostname: "github.com", - scopes: nil, + hostname: "github.com", + scopes: nil, + secureStorage: true, }, }, { @@ -210,8 +218,9 @@ func Test_refreshRun(t *testing.T) { } }, wantAuthArgs: authArgs{ - hostname: "github.com", - scopes: nil, + hostname: "github.com", + scopes: nil, + secureStorage: true, }, }, { @@ -223,12 +232,13 @@ func Test_refreshRun(t *testing.T) { Scopes: []string{"repo:invite", "public_key:read"}, }, wantAuthArgs: authArgs{ - hostname: "github.com", - scopes: []string{"repo:invite", "public_key:read"}, + hostname: "github.com", + scopes: []string{"repo:invite", "public_key:read"}, + secureStorage: true, }, }, { - name: "scopes provided", + name: "more scopes provided", cfgHosts: []string{ "github.com", }, @@ -237,37 +247,16 @@ func Test_refreshRun(t *testing.T) { Scopes: []string{"repo:invite", "public_key:read"}, }, wantAuthArgs: authArgs{ - hostname: "github.com", - scopes: []string{"repo:invite", "public_key:read", "delete_repo", "codespace"}, - }, - }, - { - name: "explicit secure storage", - cfgHosts: []string{ - "obed.morton", - }, - opts: &RefreshOptions{ - Hostname: "obed.morton", - SecureStorage: true, - }, - wantAuthArgs: authArgs{ - hostname: "obed.morton", - scopes: nil, + hostname: "github.com", + scopes: []string{"repo:invite", "public_key:read", "delete_repo", "codespace"}, secureStorage: true, }, }, { - name: "implicit secure storage", - config: func() config.Config { - cfg := config.NewFromString("") - authCfg := cfg.Authentication() - authCfg.SetHosts([]string{"obed.morton"}) - authCfg.SetToken("abc123", "keyring") - cfg.AuthenticationFunc = func() *config.AuthConfig { - return authCfg - } - return cfg - }(), + name: "secure storage", + cfgHosts: []string{ + "obed.morton", + }, opts: &RefreshOptions{ Hostname: "obed.morton", }, @@ -277,6 +266,20 @@ func Test_refreshRun(t *testing.T) { secureStorage: true, }, }, + { + name: "insecure storage", + cfgHosts: []string{ + "obed.morton", + }, + opts: &RefreshOptions{ + Hostname: "obed.morton", + InsecureStorage: true, + }, + wantAuthArgs: authArgs{ + hostname: "obed.morton", + scopes: nil, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) {