From 89ad87019043642fc8ef144fa51eb9b8e0601a5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 14 Oct 2021 19:52:59 +0200 Subject: [PATCH] auth refresh: preserve existing scopes when requesting new ones When there was a previously valid token that was granted some scopes, ensure all those scopes will be re-requested when doing the authentication flow for the new token. --- pkg/cmd/auth/refresh/refresh.go | 18 +++++++++-- pkg/cmd/auth/refresh/refresh_test.go | 46 ++++++++++++++++++++++++---- pkg/cmd/auth/shared/oauth_scopes.go | 18 ++++++++--- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 6244fabf5..4137341a9 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -3,6 +3,8 @@ package refresh import ( "errors" "fmt" + "net/http" + "strings" "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" @@ -16,8 +18,9 @@ import ( ) type RefreshOptions struct { - IO *iostreams.IOStreams - Config func() (config.Config, error) + IO *iostreams.IOStreams + Config func() (config.Config, error) + httpClient *http.Client MainExecutable string @@ -36,6 +39,7 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. _, err := authflow.AuthFlowWithConfig(cfg, io, hostname, "", scopes) return err }, + httpClient: http.DefaultClient, } cmd := &cobra.Command{ @@ -128,6 +132,16 @@ func refreshRun(opts *RefreshOptions) error { } var additionalScopes []string + if oldToken, _ := cfg.Get(hostname, "oauth_token"); 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) + } + } + } + } credentialFlow := &shared.GitCredentialFlow{ Executable: opts.MainExecutable, diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index 943864424..dbdae26af 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -2,6 +2,9 @@ package refresh import ( "bytes" + "io/ioutil" + "net/http" + "strings" "testing" "github.com/cli/cli/v2/internal/config" @@ -134,6 +137,7 @@ func Test_refreshRun(t *testing.T) { opts *RefreshOptions askStubs func(*prompt.AskStubber) cfgHosts []string + oldScopes string wantErr string nontty bool wantAuthArgs authArgs @@ -211,6 +215,20 @@ func Test_refreshRun(t *testing.T) { scopes: []string{"repo:invite", "public_key:read"}, }, }, + { + name: "scopes provided", + cfgHosts: []string{ + "github.com", + }, + oldScopes: "delete_repo, codespace", + opts: &RefreshOptions{ + Scopes: []string{"repo:invite", "public_key:read"}, + }, + wantAuthArgs: authArgs{ + hostname: "github.com", + scopes: []string{"repo:invite", "public_key:read", "delete_repo", "codespace"}, + }, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -234,10 +252,26 @@ func Test_refreshRun(t *testing.T) { for _, hostname := range tt.cfgHosts { _ = cfg.Set(hostname, "oauth_token", "abc123") } - reg := &httpmock.Registry{} - reg.Register( - httpmock.GraphQL(`query UserCurrent\b`), - httpmock.StringResponse(`{"data":{"viewer":{"login":"cybilb"}}}`)) + + httpReg := &httpmock.Registry{} + httpReg.Register( + httpmock.REST("GET", ""), + func(req *http.Request) (*http.Response, error) { + statusCode := 200 + if req.Header.Get("Authorization") != "token abc123" { + statusCode = 400 + } + return &http.Response{ + Request: req, + StatusCode: statusCode, + Body: ioutil.NopCloser(strings.NewReader(``)), + Header: http.Header{ + "X-Oauth-Scopes": {tt.oldScopes}, + }, + }, nil + }, + ) + tt.opts.httpClient = &http.Client{Transport: httpReg} mainBuf := bytes.Buffer{} hostsBuf := bytes.Buffer{} @@ -258,8 +292,8 @@ func Test_refreshRun(t *testing.T) { assert.NoError(t, err) } - assert.Equal(t, aa.hostname, tt.wantAuthArgs.hostname) - assert.Equal(t, aa.scopes, tt.wantAuthArgs.scopes) + assert.Equal(t, tt.wantAuthArgs.hostname, aa.hostname) + assert.Equal(t, tt.wantAuthArgs.scopes, aa.scopes) }) } } diff --git a/pkg/cmd/auth/shared/oauth_scopes.go b/pkg/cmd/auth/shared/oauth_scopes.go index 35619ea7b..c076722b2 100644 --- a/pkg/cmd/auth/shared/oauth_scopes.go +++ b/pkg/cmd/auth/shared/oauth_scopes.go @@ -32,19 +32,19 @@ type httpClient interface { Do(*http.Request) (*http.Response, error) } -func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { +func GetScopes(httpClient httpClient, hostname, authToken string) (string, error) { apiEndpoint := ghinstance.RESTPrefix(hostname) req, err := http.NewRequest("GET", apiEndpoint, nil) if err != nil { - return err + return "", err } req.Header.Set("Authorization", "token "+authToken) res, err := httpClient.Do(req) if err != nil { - return err + return "", err } defer func() { @@ -55,10 +55,18 @@ func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { }() if res.StatusCode != 200 { - return api.HandleHTTPError(res) + return "", api.HandleHTTPError(res) + } + + return res.Header.Get("X-Oauth-Scopes"), nil +} + +func HasMinimumScopes(httpClient httpClient, hostname, authToken string) error { + scopesHeader, err := GetScopes(httpClient, hostname, authToken) + if err != nil { + return err } - scopesHeader := res.Header.Get("X-Oauth-Scopes") if scopesHeader == "" { // if the token reports no scopes, assume that it's an integration token and give up on // detecting its capabilities