Skip to content

Commit

Permalink
Merge pull request #7278 from cli/auth-refresh-storage
Browse files Browse the repository at this point in the history 
extend secure storage default to auth refresh
  • Loading branch information
Nate Smith committed Apr 4, 2023
2 parents 69573b3 + 75dc9c0 commit 633ca03
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 44 deletions.
22 changes: 10 additions & 12 deletions pkg/cmd/auth/refresh/refresh.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand All @@ -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{
Expand All @@ -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
}

Expand Down
67 changes: 35 additions & 32 deletions pkg/cmd/auth/refresh/refresh_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
},
},
}
Expand Down Expand Up @@ -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,
},
},
{
Expand All @@ -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,
},
},
{
Expand All @@ -210,8 +218,9 @@ func Test_refreshRun(t *testing.T) {
}
},
wantAuthArgs: authArgs{
hostname: "github.com",
scopes: nil,
hostname: "github.com",
scopes: nil,
secureStorage: true,
},
},
{
Expand All @@ -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",
},
Expand All @@ -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",
Expand All @@ -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,
},
},
}
Expand Down

0 comments on commit 633ca03

Please sign in to comment.