From 38f0f607e9f9c7cc25e4be465a5e2b07ec7b5268 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Oct 2020 17:08:39 +0000 Subject: [PATCH 1/5] :fire: outdated CheckScopes --- api/client.go | 52 ------------------------- api/client_test.go | 95 ---------------------------------------------- 2 files changed, 147 deletions(-) diff --git a/api/client.go b/api/client.go index d4a32d87d03..47eb2a37321 100644 --- a/api/client.go +++ b/api/client.go @@ -99,58 +99,6 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption { } } -var issuedScopesWarning bool - -const ( - httpOAuthAppID = "X-Oauth-Client-Id" - httpOAuthScopes = "X-Oauth-Scopes" -) - -// CheckScopes checks whether an OAuth scope is present in a response -func CheckScopes(wantedScope string, cb func(string) error) ClientOption { - wantedCandidates := []string{wantedScope} - if strings.HasPrefix(wantedScope, "read:") { - wantedCandidates = append(wantedCandidates, "admin:"+strings.TrimPrefix(wantedScope, "read:")) - } - - return func(tr http.RoundTripper) http.RoundTripper { - return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { - res, err := tr.RoundTrip(req) - if err != nil || res.StatusCode > 299 || issuedScopesWarning { - return res, err - } - - _, hasHeader := res.Header[httpOAuthAppID] - if !hasHeader { - return res, nil - } - - appID := res.Header.Get(httpOAuthAppID) - hasScopes := strings.Split(res.Header.Get(httpOAuthScopes), ",") - - hasWanted := false - outer: - for _, s := range hasScopes { - for _, w := range wantedCandidates { - if w == strings.TrimSpace(s) { - hasWanted = true - break outer - } - } - } - - if !hasWanted { - if err := cb(appID); err != nil { - return res, err - } - issuedScopesWarning = true - } - - return res, nil - }} - } -} - type funcTripper struct { roundTrip func(*http.Request) (*http.Response, error) } diff --git a/api/client_test.go b/api/client_test.go index d2925bd207e..348f5678a62 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -104,98 +104,3 @@ func TestRESTError(t *testing.T) { } } - -func Test_CheckScopes(t *testing.T) { - tests := []struct { - name string - wantScope string - responseApp string - responseScopes string - responseError error - expectCallback bool - }{ - { - name: "missing read:org", - wantScope: "read:org", - responseApp: "APPID", - responseScopes: "repo, gist", - expectCallback: true, - }, - { - name: "has read:org", - wantScope: "read:org", - responseApp: "APPID", - responseScopes: "repo, read:org, gist", - expectCallback: false, - }, - { - name: "has admin:org", - wantScope: "read:org", - responseApp: "APPID", - responseScopes: "repo, admin:org, gist", - expectCallback: false, - }, - { - name: "no scopes in response", - wantScope: "read:org", - responseApp: "", - responseScopes: "", - expectCallback: false, - }, - { - name: "errored response", - wantScope: "read:org", - responseApp: "", - responseScopes: "", - responseError: errors.New("Network Failed"), - expectCallback: false, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - tr := &httpmock.Registry{} - tr.Register(httpmock.MatchAny, func(*http.Request) (*http.Response, error) { - if tt.responseError != nil { - return nil, tt.responseError - } - if tt.responseScopes == "" { - return &http.Response{StatusCode: 200}, nil - } - return &http.Response{ - StatusCode: 200, - Header: http.Header{ - "X-Oauth-Client-Id": []string{tt.responseApp}, - "X-Oauth-Scopes": []string{tt.responseScopes}, - }, - }, nil - }) - - callbackInvoked := false - var gotAppID string - fn := CheckScopes(tt.wantScope, func(appID string) error { - callbackInvoked = true - gotAppID = appID - return nil - }) - - rt := fn(tr) - req, err := http.NewRequest("GET", "https://api.github.com/hello", nil) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - - issuedScopesWarning = false - _, err = rt.RoundTrip(req) - if err != nil && !errors.Is(err, tt.responseError) { - t.Fatalf("unexpected error: %v", err) - } - - if tt.expectCallback != callbackInvoked { - t.Fatalf("expected CheckScopes callback: %v", tt.expectCallback) - } - if tt.expectCallback && gotAppID != tt.responseApp { - t.Errorf("unexpected app ID: %q", gotAppID) - } - }) - } -} From 626be2a0951b5550562fc1352bb3630b9813138e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Oct 2020 16:54:07 +0000 Subject: [PATCH 2/5] Fix formatting in MissingScopesError --- api/client.go | 32 ++++++++++++++++--------- api/client_test.go | 59 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 11 deletions(-) diff --git a/api/client.go b/api/client.go index 47eb2a37321..b290aae3d74 100644 --- a/api/client.go +++ b/api/client.go @@ -3,7 +3,6 @@ package api import ( "bytes" "encoding/json" - "errors" "fmt" "io" "io/ioutil" @@ -155,7 +154,20 @@ func (err HTTPError) Error() string { } type MissingScopesError struct { - error + MissingScopes []string +} + +func (e MissingScopesError) Error() string { + var missing []string + for _, s := range e.MissingScopes { + missing = append(missing, fmt.Sprintf("'%s'", s)) + } + scopes := strings.Join(missing, ", ") + + if len(e.MissingScopes) == 1 { + return "missing required scope " + scopes + } + return "missing required scopes " + scopes } func (c Client) HasMinimumScopes(hostname string) error { @@ -183,31 +195,29 @@ func (c Client) HasMinimumScopes(hostname string) error { return HandleHTTPError(res) } - hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") + scopesHeader := res.Header.Get("X-Oauth-Scopes") search := map[string]bool{ "repo": false, "read:org": false, "admin:org": false, } - - for _, s := range hasScopes { + for _, s := range strings.Split(scopesHeader, ",") { search[strings.TrimSpace(s)] = true } - errorMsgs := []string{} + var missingScopes []string if !search["repo"] { - errorMsgs = append(errorMsgs, "missing required scope 'repo'") + missingScopes = append(missingScopes, "repo") } if !search["read:org"] && !search["admin:org"] { - errorMsgs = append(errorMsgs, "missing required scope 'read:org'") + missingScopes = append(missingScopes, "read:org") } - if len(errorMsgs) > 0 { - return &MissingScopesError{error: errors.New(strings.Join(errorMsgs, ";"))} + if len(missingScopes) > 0 { + return &MissingScopesError{MissingScopes: missingScopes} } - return nil } diff --git a/api/client_test.go b/api/client_test.go index 348f5678a62..fd24a540431 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -104,3 +104,62 @@ func TestRESTError(t *testing.T) { } } + +func Test_HasMinimumScopes(t *testing.T) { + tests := []struct { + name string + header string + wantErr string + }{ + { + name: "default scopes", + header: "repo, read:org", + wantErr: "", + }, + { + name: "admin:org satisfies read:org", + header: "repo, admin:org", + wantErr: "", + }, + { + name: "insufficient scope", + header: "repo", + wantErr: "missing required scope 'read:org'", + }, + { + name: "insufficient scopes", + header: "gist", + wantErr: "missing required scopes 'repo', 'read:org'", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fakehttp := &httpmock.Registry{} + client := NewClient(ReplaceTripper(fakehttp)) + + fakehttp.Register(httpmock.REST("GET", ""), func(req *http.Request) (*http.Response, error) { + return &http.Response{ + Request: req, + StatusCode: 200, + Body: ioutil.NopCloser(&bytes.Buffer{}), + Header: map[string][]string{ + "X-Oauth-Scopes": {tt.header}, + }, + }, nil + }) + + err := client.HasMinimumScopes("github.com") + if tt.wantErr == "" { + if err != nil { + t.Errorf("error: %v", err) + } + return + } + if err.Error() != tt.wantErr { + t.Errorf("want %q, got %q", tt.wantErr, err.Error()) + + } + }) + } + +} From 53cea2667e58e5cbd3261d237b36aa42280094e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Oct 2020 16:56:23 +0000 Subject: [PATCH 3/5] Support "integration" tokens Integration tokens are different than OAuth token in it that they don't report any `X-Oauth-Scopes` in response headers. --- api/client.go | 5 +++++ api/client_test.go | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/api/client.go b/api/client.go index b290aae3d74..09195181bf2 100644 --- a/api/client.go +++ b/api/client.go @@ -196,6 +196,11 @@ func (c Client) HasMinimumScopes(hostname string) error { } 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 + return nil + } search := map[string]bool{ "repo": false, diff --git a/api/client_test.go b/api/client_test.go index fd24a540431..35a45af8c38 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -111,6 +111,11 @@ func Test_HasMinimumScopes(t *testing.T) { header string wantErr string }{ + { + name: "no scopes", + header: "", + wantErr: "", + }, { name: "default scopes", header: "repo, read:org", From 4bda6fceecd188ac56d7bae9f3e295c34fca26a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Oct 2020 17:09:09 +0000 Subject: [PATCH 4/5] auth status: fix reporting the token source --- pkg/cmd/auth/status/status.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/auth/status/status.go b/pkg/cmd/auth/status/status.go index 78b4b074efa..d9d59fa5031 100644 --- a/pkg/cmd/auth/status/status.go +++ b/pkg/cmd/auth/status/status.go @@ -96,7 +96,7 @@ func statusRun(opts *StatusOptions) error { if err != nil { var missingScopes *api.MissingScopesError if errors.As(err, &missingScopes) { - addMsg("%s %s: %s", utils.Red("X"), hostname, err) + addMsg("%s %s: the token in %s is %s", utils.Red("X"), hostname, tokenSource, err) if tokenIsWriteable { addMsg("- To request missing scopes, run: %s %s\n", utils.Bold("gh auth refresh -h"), From 35517ebd771e58b993995942c90b39352600e086 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Oct 2020 17:20:12 +0000 Subject: [PATCH 5/5] Skip update notifier in Codespaces --- cmd/gh/main.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/gh/main.go b/cmd/gh/main.go index 242df7ed2e4..358d9525033 100644 --- a/cmd/gh/main.go +++ b/cmd/gh/main.go @@ -189,6 +189,9 @@ func shouldCheckForUpdate() bool { if os.Getenv("GH_NO_UPDATE_NOTIFIER") != "" { return false } + if os.Getenv("CODESPACES") != "" { + return false + } return updaterEnabled != "" && !isCI() && !isCompletionCommand() && utils.IsTerminal(os.Stderr) }