Repository: cli/cli
Pull request #7278 (2023-04-04 22:24:38)

extend secure storage default to auth refresh
This PR makes `gh auth refresh` use secure storage by default, requiring `--insecure-storage` for old plaintext storage behavior.

Note that this means that if you've stored your creds in plaintext intentionally (perhaps because the OS keychain doesn't work for you) running `gh auth refresh` without `--insecure-storage` may lead to broken creds.



Patch:
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
 	}
 


Test patch:
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,18 +247,18 @@ 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"},
+				hostname:      "github.com",
+				scopes:        []string{"repo:invite", "public_key:read", "delete_repo", "codespace"},
+				secureStorage: true,
 			},
 		},
 		{
-			name: "explicit secure storage",
+			name: "secure storage",
 			cfgHosts: []string{
 				"obed.morton",
 			},
 			opts: &RefreshOptions{
-				Hostname:      "obed.morton",
-				SecureStorage: true,
+				Hostname: "obed.morton",
 			},
 			wantAuthArgs: authArgs{
 				hostname:      "obed.morton",
@@ -257,24 +267,17 @@ func Test_refreshRun(t *testing.T) {
 			},
 		},
 		{
-			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: "insecure storage",
+			cfgHosts: []string{
+				"obed.morton",
+			},
 			opts: &RefreshOptions{
-				Hostname: "obed.morton",
+				Hostname:        "obed.morton",
+				InsecureStorage: true,
 			},
 			wantAuthArgs: authArgs{
-				hostname:      "obed.morton",
-				scopes:        nil,
-				secureStorage: true,
+				hostname: "obed.morton",
+				scopes:   nil,
 			},
 		},
 	}