From fc4cd12cbfe0a63f3fd24cf9f4fa3e80d3ec8ec9 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Sun, 8 May 2022 12:32:41 -0700 Subject: [PATCH] Ignore FORBIDDEN errors for `gh status` Fixes #5587 --- pkg/cmd/status/fixtures/search_forbidden.json | 104 ++++++++++++++ pkg/cmd/status/status.go | 64 ++++++++- pkg/cmd/status/status_test.go | 132 ++++++++++++++++++ 3 files changed, 293 insertions(+), 7 deletions(-) create mode 100644 pkg/cmd/status/fixtures/search_forbidden.json diff --git a/pkg/cmd/status/fixtures/search_forbidden.json b/pkg/cmd/status/fixtures/search_forbidden.json new file mode 100644 index 000000000..7ec0727f9 --- /dev/null +++ b/pkg/cmd/status/fixtures/search_forbidden.json @@ -0,0 +1,104 @@ +{ + "data": { + "assignments": { + "edges": [ + { + "node": { + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" + } + } + }, + null, + { + "node": { + "__typename": "Issue", + "updatedAt": "2022-03-10T21:33:57Z", + "title": "yolo", + "number": 157, + "repository": { + "nameWithOwner": "vilmibm/testing" + } + } + }, + null + ] + }, + "reviewRequested": { + "edges": [ + { + "node": { + "updatedAt": "2022-03-15T17:10:25Z", + "__typename": "PullRequest", + "title": "Pin extensions", + "number": 5272, + "repository": { + "nameWithOwner": "cli/cli" + } + } + }, + null + ] + } + }, + "errors": [ + { + "type": "FORBIDDEN", + "path": [ + "assignments", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }, + { + "type": "FORBIDDEN", + "path": [ + "assignments", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }, + { + "type": "FORBIDDEN", + "path": [ + "reviewRequested", + "edges", + 0 + ], + "extensions": { + "saml_failure": true + }, + "locations": [ + { + "line": 4, + "column": 5 + } + ], + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + } + ] +} diff --git a/pkg/cmd/status/status.go b/pkg/cmd/status/status.go index c9e5ff3e6..d9b534566 100644 --- a/pkg/cmd/status/status.go +++ b/pkg/cmd/status/status.go @@ -8,6 +8,7 @@ import ( "net/url" "sort" "strings" + "sync" "time" "github.com/MakeNowJust/heredoc" @@ -44,7 +45,7 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co cmd := &cobra.Command{ Use: "status", Short: "Print information about relevant issues, pull requests, and notifications across repositories", - Long: heredoc.Doc(` + Long: heredoc.Docf(` The status command prints information about your work on GitHub across all the repositories you're subscribed to, including: - Assigned Issues @@ -52,7 +53,9 @@ func NewCmdStatus(f *cmdutil.Factory, runF func(*StatusOptions) error) *cobra.Co - Review Requests - Mentions - Repository Activity (new issues/pull requests, comments) - `), + + Some status may be unavailable due to organization SAML enforcement. Run %[1]sgh auth refresh%[1]s to authorization access to organizations. + `, "`"), Example: heredoc.Doc(` $ gh status -e cli/cli -e cli/go-gh # Exclude multiple repositories $ gh status -o cli # Limit results to a single organization @@ -166,6 +169,8 @@ type StatusGetter struct { Mentions []StatusItem ReviewRequests []StatusItem RepoActivity []StatusItem + hasAuthErrors bool + authErrorsMu sync.Mutex } func NewStatusGetter(client *http.Client, hostname string, opts *StatusOptions) *StatusGetter { @@ -295,7 +300,14 @@ func (s *StatusGetter) LoadNotifications() error { preview: actual, }) } else if err != nil { - return fmt.Errorf("could not fetch comment: %w", err) + var httpErr api.HTTPError + if !errors.As(err, &httpErr) || httpErr.StatusCode != 403 { + return fmt.Errorf("could not fetch comment: %w", err) + } else if httpErr.StatusCode == 403 { + s.authErrorsMu.Lock() + s.hasAuthErrors = true + s.authErrorsMu.Unlock() + } } } @@ -372,7 +384,18 @@ func (s *StatusGetter) LoadSearchResults() error { } err := c.GraphQL(s.hostname(), searchQuery, variables, &resp) if err != nil { - return fmt.Errorf("could not search for assignments: %w", err) + // Exclude any FORBIDDEN errors and show status for what we can. + if gqlErrResponse, ok := err.(*api.GraphQLErrorResponse); ok { + err = filterGraphQLErrors(gqlErrResponse) + if err == nil { + s.authErrorsMu.Lock() + s.hasAuthErrors = true + s.authErrorsMu.Unlock() + } + } + if err != nil { + return fmt.Errorf("could not search for assignments: %w", err) + } } prs := []SearchResult{} @@ -384,13 +407,16 @@ func (s *StatusGetter) LoadSearchResults() error { issues = append(issues, e.Node) } else if e.Node.Type == "PullRequest" { prs = append(prs, e.Node) - } else { + } else if e.Node.Type != "" { + // FORBIDDEN nodes are null in response, so Type == "" here. panic("you shouldn't be here") } } for _, e := range resp.ReviewRequested.Edges { - reviewRequested = append(reviewRequested, e.Node) + if e.Node.Type != "" { + reviewRequested = append(reviewRequested, e.Node) + } } sort.Sort(Results(issues)) @@ -428,6 +454,19 @@ func (s *StatusGetter) LoadSearchResults() error { return nil } +func filterGraphQLErrors(gqlErrResponse *api.GraphQLErrorResponse) error { + gqlErrors := make([]api.GraphQLError, 0, len(gqlErrResponse.Errors)) + for _, gqlErr := range gqlErrResponse.Errors { + if gqlErr.Type != "FORBIDDEN" { + gqlErrors = append(gqlErrors, gqlErr) + } + } + if len(gqlErrors) > 0 { + return &api.GraphQLErrorResponse{Errors: gqlErrors} + } + return nil +} + // Populate .RepoActivity func (s *StatusGetter) LoadEvents() error { perPage := 100 @@ -506,6 +545,13 @@ func (s *StatusGetter) LoadEvents() error { return nil } +func (s *StatusGetter) HasAuthErrors() bool { + s.authErrorsMu.Lock() + defer s.authErrorsMu.Unlock() + + return s.hasAuthErrors +} + func statusRun(opts *StatusOptions) error { client, err := opts.HttpClient() if err != nil { @@ -548,10 +594,10 @@ func statusRun(opts *StatusOptions) error { }) err = g.Wait() + opts.IO.StopProgressIndicator() if err != nil { return err } - opts.IO.StopProgressIndicator() cs := opts.IO.ColorScheme() out := opts.IO.Out @@ -628,5 +674,9 @@ func statusRun(opts *StatusOptions) error { fmt.Fprintln(out, lipgloss.JoinHorizontal(lipgloss.Top, rrSection, mSection)) fmt.Fprintln(out, raSection) + if sg.HasAuthErrors() { + fmt.Fprintln(out, cs.Gray("Some status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.")) + } + return nil } diff --git a/pkg/cmd/status/status_test.go b/pkg/cmd/status/status_test.go index 9475ade1c..a845cab90 100644 --- a/pkg/cmd/status/status_test.go +++ b/pkg/cmd/status/status_test.go @@ -271,6 +271,138 @@ func TestStatusRun(t *testing.T) { }, wantOut: "Assigned Issues │ Assigned Pull Requests \nvilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions \ncli/cli#3223 Repo garden...│ rpd/todo#73 Board up RPD windows \nrpd/todo#514 Reducing zo...│ cli/cli#4768 Issue Frecency \nvilmibm/testing#74 welp │ \nadreyer/arkestrator#22 complete mo...│ \n │ \nReview Requests │ Mentions \ncli/cli#5272 Pin extensions │ rpd/todo#110 hello @jillvalentine ...\nvilmibm/testing#1234 Foobar │ \nrpd/todo#50 Welcome party...│ \ncli/cli#4671 This pull req...│ \nrpd/todo#49 Haircut for Leon│ \n │ \nRepository Activity\nrpd/todo#5326 new PR Only write UTF-8 BOM on Windows where it is needed\n\n", }, + { + name: "forbidden errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/110"), + httpmock.StatusStringResponse(403, `{ + "message": "Resource protected by organization SAML enforcement. You must grant your OAuth token access to this organization." + }`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/4113"), + httpmock.StringResponse(`{"body":"this is a comment"}`)) + reg.Register( + httpmock.REST("GET", "repos/cli/cli/issues/1096"), + httpmock.StringResponse(`{"body":"@jillvalentine hi"}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/comments/1065"), + httpmock.StringResponse(`{"body":"not a real mention"}`)) + reg.Register( + httpmock.REST("GET", "repos/vilmibm/gh-screensaver/issues/comments/10"), + httpmock.StringResponse(`{"body":"a message for @jillvalentine"}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.FileResponse("./fixtures/search_forbidden.json")) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.FileResponse("./fixtures/notifications.json")) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.FileResponse("./fixtures/events.json")) + }, + opts: &StatusOptions{}, + wantOut: "Assigned Issues │ Assigned Pull Requests \nvilmibm/testing#157 yolo │ cli/cli#5272 Pin extensions \n │ \nReview Requests │ Mentions \ncli/cli#5272 Pin extensions │ cli/cli#1096 @jillval...\n │ vilmibm/gh-screensaver#15 a messag...\n \nRepository Activity\nrpd/todo#5326 new PR Only write UTF-8 BOM on W...\nvilmibm/testing#5325 comment on Ability to sea... We are working on dedicat...\ncli/cli#5319 comment on [Codespaces] D... Wondering if we shouldn't...\ncli/cli#5300 new issue Terminal bell when a runn...\n\nSome status unavailable due to organization SAML enforcement. Run `gh auth refresh` to authorization access to organizations.\n", + }, + { + name: "notification errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.REST("GET", "repos/rpd/todo/issues/110"), + httpmock.StatusStringResponse(429, `{ + "message": "Too many requests." + }`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.StringResponse(`{"data": { "assignments": {"edges": [] }, "reviewRequested": {"edges": []}}}`)) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.StringResponse(`[ + { + "reason": "mention", + "subject": { + "title": "Good", + "url": "https://api.github.com/repos/rpd/todo/issues/110", + "latest_comment_url": "https://api.github.com/repos/rpd/todo/issues/110", + "type": "Issue" + }, + "repository": { + "full_name": "rpd/todo", + "owner": { + "login": "rpd" + } + } + } + ]`)) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.StringResponse(`[]`)) + }, + opts: &StatusOptions{}, + wantErrMsg: "could not load notifications: could not fetch comment: HTTP 429 (https://api.github.com/repos/rpd/todo/issues/110)", + }, + { + name: "search errors", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.GraphQL("UserCurrent"), + httpmock.StringResponse(`{"data": {"viewer": {"login": "jillvalentine"}}}`)) + reg.Register( + httpmock.GraphQL("AssignedSearch"), + httpmock.StringResponse(`{ + "data": { + "assignments": { + "edges": [ + null + ] + } + }, + "errors": [ + { + "type": "NOT FOUND", + "path": [ + "assignments", + "edges", + 0 + ], + "message": "Not found." + } + ] + }`)) + reg.Register( + httpmock.REST("GET", "notifications"), + httpmock.StringResponse(`[]`)) + reg.Register( + httpmock.REST("GET", "users/jillvalentine/received_events"), + httpmock.StringResponse(`[]`)) + }, + opts: &StatusOptions{}, + wantErrMsg: "failed to search: could not search for assignments: GraphQL: Not found. (assignments.edges.0)", + }, } for _, tt := range tests {