From 0d9e0bc91a8feadf8650a2fe09311e8e3178ecf7 Mon Sep 17 00:00:00 2001 From: Nilesh Singh Date: Mon, 19 Jun 2023 21:56:15 +0530 Subject: [PATCH 1/3] Add remove/reset to auth refresh --- pkg/cmd/auth/refresh/refresh.go | 42 ++++++++++++++----- pkg/cmd/auth/refresh/refresh_test.go | 63 ++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index fc068440c..6d2fe42af 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -24,9 +24,11 @@ type RefreshOptions struct { MainExecutable string - Hostname string - Scopes []string - AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool, bool) error + Hostname string + Scopes []string + RemoveScopes []string + ResetScopes bool + AuthFlow func(*config.AuthConfig, *iostreams.IOStreams, string, []string, bool, bool) error Interactive bool InsecureStorage bool @@ -90,6 +92,8 @@ 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().StringSliceVarP(&opts.RemoveScopes, "remove-scope", "r", nil, "The authentication scopes to remove from gh") + cmd.Flags().BoolVarP(&opts.ResetScopes, "reset-scopes", "R", false, "Reset all authentication scopes for gh") // 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") @@ -144,12 +148,14 @@ func refreshRun(opts *RefreshOptions) error { } var additionalScopes []string - 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) - if s != "" { - additionalScopes = append(additionalScopes, s) + if !opts.ResetScopes { + 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) + if s != "" { + additionalScopes = append(additionalScopes, s) + } } } } @@ -161,13 +167,29 @@ func refreshRun(opts *RefreshOptions) error { GitClient: opts.GitClient, } gitProtocol, _ := authCfg.GitProtocol(hostname) - if opts.Interactive && gitProtocol == "https" { + if !opts.ResetScopes && opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err } additionalScopes = append(additionalScopes, credentialFlow.Scopes()...) } + if len(opts.RemoveScopes) > 0 { + var scopes []string + for _, scope := range additionalScopes { + var match bool + for _, rmScope := range opts.RemoveScopes { + if rmScope == scope { + match = true + break + } + } + if !match { + scopes = append(scopes, scope) + } + } + additionalScopes = scopes + } 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 8743f1835..ac3bd8694 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -99,6 +99,22 @@ func Test_NewCmdRefresh(t *testing.T) { InsecureStorage: true, }, }, + { + name: "reset scopes", + tty: true, + cli: "--reset-scopes", + wants: RefreshOptions{ + ResetScopes: true, + }, + }, + { + name: "remove scope", + tty: true, + cli: "--remove-scope read:public_key", + wants: RefreshOptions{ + RemoveScopes: []string{"read:public_key"}, + }, + }, } for _, tt := range tests { @@ -280,6 +296,53 @@ func Test_refreshRun(t *testing.T) { scopes: nil, }, }, + { + name: "reset scopes", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "delete_repo, codespace", + opts: &RefreshOptions{ + Hostname: "github.com", + ResetScopes: true, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: nil, + secureStorage: true, + }, + }, + { + name: "remove scopes", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "delete_repo, codespace, repo:invite, public_key:read", + opts: &RefreshOptions{ + Hostname: "github.com", + RemoveScopes: []string{"delete_repo", "repo:invite"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"codespace", "public_key:read"}, + secureStorage: true, + }, + }, + { + name: "remove scope but no old scope", + cfgHosts: []string{ + "github.com", + }, + opts: &RefreshOptions{ + Hostname: "github.com", + RemoveScopes: []string{"delete_repo"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: nil, + secureStorage: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From becccebae167c8cc4d7cdb1e0471e3b2a6f7b3a6 Mon Sep 17 00:00:00 2001 From: Nilesh Singh Date: Sun, 25 Jun 2023 01:30:09 +0530 Subject: [PATCH 2/3] Refactor as per review Co-authored-by: Shion Ichikawa --- pkg/cmd/auth/refresh/refresh.go | 24 +++++++---- pkg/cmd/auth/refresh/refresh_test.go | 63 ++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 6d2fe42af..c00b9a025 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -64,9 +64,12 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. your gh credentials to have. If no scopes are provided, the command maintains previously added scopes. - The command can only add additional scopes, but not remove previously - added ones. To reset scopes to the default minimum set of scopes, you - will need to create new credentials using the auth login command. + The --remove-scope flag accepts a comma separated list of scopes you + want to remove from your gh credentials. Scope removal is idempotent. + The minimum set of scopes ("repo", "read:org" and "gist") cannot be removed. + + The --reset-scopes flag resets the scopes for your gh credentials to + the default set of scopes for your auth flow. `), Example: heredoc.Doc(` $ gh auth refresh --scopes write:org,read:public_key @@ -74,6 +77,12 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. $ gh auth refresh # => open a browser to ensure your authentication credentials have the correct minimum scopes + + $ gh auth refresh --remove-scope delete_repo + # => open a browser to idempotently remove the delete_repo scope + + $ gh auth refresh --reset-scopes + # => open a browser to re-authenticate with the default minimum scopes `), RunE: func(cmd *cobra.Command, args []string) error { opts.Interactive = opts.IO.CanPrompt() @@ -92,8 +101,8 @@ 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().StringSliceVarP(&opts.RemoveScopes, "remove-scope", "r", nil, "The authentication scopes to remove from gh") - cmd.Flags().BoolVarP(&opts.ResetScopes, "reset-scopes", "R", false, "Reset all authentication scopes for gh") + cmd.Flags().StringSliceVarP(&opts.RemoveScopes, "remove-scope", "r", nil, "The authentication scope to remove from gh") + cmd.Flags().BoolVarP(&opts.ResetScopes, "reset-scopes", "R", false, "Reset authentication scopes to the default minimum set of scopes") // 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") @@ -167,13 +176,14 @@ func refreshRun(opts *RefreshOptions) error { GitClient: opts.GitClient, } gitProtocol, _ := authCfg.GitProtocol(hostname) - if !opts.ResetScopes && opts.Interactive && gitProtocol == "https" { + if opts.Interactive && gitProtocol == "https" { if err := credentialFlow.Prompt(hostname); err != nil { return err } additionalScopes = append(additionalScopes, credentialFlow.Scopes()...) } + additionalScopes = append(opts.Scopes, additionalScopes...) if len(opts.RemoveScopes) > 0 { var scopes []string for _, scope := range additionalScopes { @@ -190,7 +200,7 @@ func refreshRun(opts *RefreshOptions) error { } additionalScopes = scopes } - if err := opts.AuthFlow(authCfg, opts.IO, hostname, append(opts.Scopes, additionalScopes...), opts.Interactive, !opts.InsecureStorage); err != nil { + if err := opts.AuthFlow(authCfg, opts.IO, hostname, 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 ac3bd8694..59c9d490f 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -107,6 +107,14 @@ func Test_NewCmdRefresh(t *testing.T) { ResetScopes: true, }, }, + { + name: "reset scopes shorthand", + tty: true, + cli: "-R", + wants: RefreshOptions{ + ResetScopes: true, + }, + }, { name: "remove scope", tty: true, @@ -115,6 +123,14 @@ func Test_NewCmdRefresh(t *testing.T) { RemoveScopes: []string{"read:public_key"}, }, }, + { + name: "remove scope shorthand", + tty: true, + cli: "-r read:public_key", + wants: RefreshOptions{ + RemoveScopes: []string{"read:public_key"}, + }, + }, } for _, tt := range tests { @@ -312,6 +328,22 @@ func Test_refreshRun(t *testing.T) { secureStorage: true, }, }, + { + name: "reset scopes and add some scopes", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "repo:invite, delete_repo, codespace", + opts: &RefreshOptions{ + Scopes: []string{"public_key:read", "workflow"}, + ResetScopes: true, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"public_key:read", "workflow"}, + secureStorage: true, + }, + }, { name: "remove scopes", cfgHosts: []string{ @@ -343,6 +375,37 @@ func Test_refreshRun(t *testing.T) { secureStorage: true, }, }, + { + name: "remove and add scopes at the same time", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "repo:invite, delete_repo, codespace", + opts: &RefreshOptions{ + Scopes: []string{"repo:invite", "public_key:read", "workflow"}, + RemoveScopes: []string{"codespace", "repo:invite", "workflow"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"public_key:read", "delete_repo"}, + secureStorage: true, + }, + }, + { + name: "remove scopes that don't exist", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "repo:invite, delete_repo, codespace", + opts: &RefreshOptions{ + RemoveScopes: []string{"codespace", "repo:invite", "public_key:read"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"delete_repo"}, + secureStorage: true, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 0ddf32d58986039324da846e99552dae80eec05f Mon Sep 17 00:00:00 2001 From: Nilesh Singh Date: Mon, 26 Jun 2023 23:52:55 +0530 Subject: [PATCH 3/3] Add cli test to remove multiple scopes at once --- pkg/cmd/auth/refresh/refresh_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 59c9d490f..41d92144a 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -123,6 +123,14 @@ func Test_NewCmdRefresh(t *testing.T) { RemoveScopes: []string{"read:public_key"}, }, }, + { + name: "remove multiple scopes", + tty: true, + cli: "--remove-scope workflow,read:public_key", + wants: RefreshOptions{ + RemoveScopes: []string{"workflow", "read:public_key"}, + }, + }, { name: "remove scope shorthand", tty: true,