From 14ce1f99a705d9c8e15a21afa62780f6e888eb02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Apr 2020 14:28:07 +0200 Subject: [PATCH 1/4] Ask for an additional `read:org` OAuth scope This is to facilitate: - requesting teams for review on `pr create` - allowing `repo create ORG/REPO --team TEAM` --- auth/oauth.go | 10 ++++++++-- context/config_setup.go | 1 + 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/auth/oauth.go b/auth/oauth.go index dc7a9d085..3f9d9ddc4 100644 --- a/auth/oauth.go +++ b/auth/oauth.go @@ -11,6 +11,7 @@ import ( "net/http" "net/url" "os" + "strings" "github.com/cli/cli/pkg/browser" ) @@ -29,6 +30,7 @@ type OAuthFlow struct { Hostname string ClientID string ClientSecret string + Scopes []string WriteSuccessHTML func(io.Writer) VerboseStream io.Writer } @@ -45,11 +47,15 @@ func (oa *OAuthFlow) ObtainAccessToken() (accessToken string, err error) { } port := listener.Addr().(*net.TCPAddr).Port + scopes := "repo" + if oa.Scopes != nil { + scopes = strings.Join(oa.Scopes, " ") + } + q := url.Values{} q.Set("client_id", oa.ClientID) q.Set("redirect_uri", fmt.Sprintf("http://127.0.0.1:%d/callback", port)) - // TODO: make scopes configurable - q.Set("scope", "repo") + q.Set("scope", scopes) q.Set("state", state) startURL := fmt.Sprintf("https://%s/login/oauth/authorize?%s", oa.Hostname, q.Encode()) diff --git a/context/config_setup.go b/context/config_setup.go index 56bdfe55a..a55b8dfa6 100644 --- a/context/config_setup.go +++ b/context/config_setup.go @@ -36,6 +36,7 @@ func setupConfigFile(filename string) (*configEntry, error) { Hostname: oauthHost, ClientID: oauthClientID, ClientSecret: oauthClientSecret, + Scopes: []string{"repo", "read:org"}, WriteSuccessHTML: func(w io.Writer) { fmt.Fprintln(w, oauthSuccessPage) }, From 3d566dc5a6bb74abb5f8aa4ae763add14b4027d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 15 Apr 2020 17:25:15 +0200 Subject: [PATCH 2/4] Detect and warn about `read:org` OAuth scope being missing --- api/client.go | 41 +++++++++++++++++++++++++++++++++++++++++ command/root.go | 1 + 2 files changed, 42 insertions(+) diff --git a/api/client.go b/api/client.go index 2fc72aaa6..e1fef7219 100644 --- a/api/client.go +++ b/api/client.go @@ -7,6 +7,7 @@ import ( "io" "io/ioutil" "net/http" + "os" "regexp" "strings" @@ -63,6 +64,46 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption { } } +var issuedScopesWarning bool + +// CheckScopes checks whether an OAuth scope is present in a response +func CheckScopes(wantedScope string) ClientOption { + 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 || issuedScopesWarning { + return res, err + } + + isApp := res.Header.Get("X-Oauth-Client-Id") != "" + hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") + + hasWanted := false + for _, s := range hasScopes { + if wantedScope == strings.TrimSpace(s) { + hasWanted = true + break + } + } + + if !hasWanted { + fmt.Fprintln(os.Stderr, "Warning: gh now requires the `read:org` OAuth scope.") + // TODO: offer to take the person through the authentication flow again? + // TODO: retry the original request if it was a read? + if isApp { + fmt.Fprintln(os.Stderr, "To re-authenticate, please `rm ~/.config/gh/config.yml` and try again.") + } else { + // the person has pasted a Personal Access Token + fmt.Fprintln(os.Stderr, "Re-generate your token in `rm ~/.config/gh/config.yml` and try again.") + } + issuedScopesWarning = true + } + + return res, nil + }} + } +} + type funcTripper struct { roundTrip func(*http.Request) (*http.Response, error) } diff --git a/command/root.go b/command/root.go index fa65d7e05..6eafd01d4 100644 --- a/command/root.go +++ b/command/root.go @@ -131,6 +131,7 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { opts = append(opts, apiVerboseLog()) } opts = append(opts, + api.CheckScopes("read:org"), api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)), // antiope-preview: Checks From 3aaa231cc5388ecef05a218314f308cb49fcf7f4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Thu, 23 Apr 2020 18:15:20 +0200 Subject: [PATCH 3/4] Guide user through re-authorization flow if `read:org` scope is missing How this works for people with existing OAuth tokens: $ gh issue list -L1 Notice: additional authorization required Press Enter to open github.com in your browser... [auth flow in the browser...] Authentication complete. Press Enter to continue... Showing 1 of 132 issues in cli/cli ... Users of Personal Access Tokens get a different notice: Warning: gh now requires the `read:org` OAuth scope. Visit https://github.com/settings/tokens and edit your token to enable `read:org` or generate a new token and paste it via `gh config set -h github.com oauth_token MYTOKEN` --- api/client.go | 27 ++++++++++--------- command/root.go | 37 ++++++++++++++++++++++++-- internal/config/config_setup.go | 47 ++++++++++++++++++++++----------- 3 files changed, 81 insertions(+), 30 deletions(-) diff --git a/api/client.go b/api/client.go index e1fef7219..d7b981bee 100644 --- a/api/client.go +++ b/api/client.go @@ -7,7 +7,6 @@ import ( "io" "io/ioutil" "net/http" - "os" "regexp" "strings" @@ -38,6 +37,16 @@ func AddHeader(name, value string) ClientOption { } } +// AddHeaderFunc is an AddHeader that gets the string value from a function +func AddHeaderFunc(name string, value func() string) ClientOption { + return func(tr http.RoundTripper) http.RoundTripper { + return &funcTripper{roundTrip: func(req *http.Request) (*http.Response, error) { + req.Header.Add(name, value()) + return tr.RoundTrip(req) + }} + } +} + // VerboseLog enables request/response logging within a RoundTripper func VerboseLog(out io.Writer, logTraffic bool, colorize bool) ClientOption { logger := &httpretty.Logger{ @@ -67,15 +76,15 @@ func ReplaceTripper(tr http.RoundTripper) ClientOption { var issuedScopesWarning bool // CheckScopes checks whether an OAuth scope is present in a response -func CheckScopes(wantedScope string) ClientOption { +func CheckScopes(wantedScope string, cb func(string) error) ClientOption { 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 || issuedScopesWarning { + if err != nil || res.StatusCode > 299 || issuedScopesWarning { return res, err } - isApp := res.Header.Get("X-Oauth-Client-Id") != "" + appID := res.Header.Get("X-Oauth-Client-Id") hasScopes := strings.Split(res.Header.Get("X-Oauth-Scopes"), ",") hasWanted := false @@ -87,14 +96,8 @@ func CheckScopes(wantedScope string) ClientOption { } if !hasWanted { - fmt.Fprintln(os.Stderr, "Warning: gh now requires the `read:org` OAuth scope.") - // TODO: offer to take the person through the authentication flow again? - // TODO: retry the original request if it was a read? - if isApp { - fmt.Fprintln(os.Stderr, "To re-authenticate, please `rm ~/.config/gh/config.yml` and try again.") - } else { - // the person has pasted a Personal Access Token - fmt.Fprintln(os.Stderr, "Re-generate your token in `rm ~/.config/gh/config.yml` and try again.") + if err := cb(appID); err != nil { + return res, err } issuedScopesWarning = true } diff --git a/command/root.go b/command/root.go index 1624a57e0..8c0cbd046 100644 --- a/command/root.go +++ b/command/root.go @@ -142,13 +142,46 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { if err != nil { return nil, err } + var opts []api.ClientOption if verbose := os.Getenv("DEBUG"); verbose != "" { opts = append(opts, apiVerboseLog()) } + + getAuthValue := func() string { + return fmt.Sprintf("token %s", token) + } + checkScopesFunc := func(appID string) error { + if config.IsGitHubApp(appID) && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) { + newToken, loginHandle, err := config.AuthFlow("Notice: additional authorization required") + if err != nil { + return err + } + cfg, err := ctx.Config() + if err != nil { + return err + } + _ = cfg.Set(defaultHostname, "oauth_token", newToken) + _ = cfg.Set(defaultHostname, "user", loginHandle) + // update config file on disk + err = cfg.Write() + if err != nil { + return err + } + // update configuration in memory + token = newToken + config.AuthFlowComplete() + } else { + fmt.Fprintln(os.Stderr, "Warning: gh now requires the `read:org` OAuth scope.") + fmt.Fprintln(os.Stderr, "Visit https://github.com/settings/tokens and edit your token to enable `read:org`") + fmt.Fprintln(os.Stderr, "or generate a new token and paste it via `gh config set -h github.com oauth_token MYTOKEN`") + } + return nil + } + opts = append(opts, - api.CheckScopes("read:org"), - api.AddHeader("Authorization", fmt.Sprintf("token %s", token)), + api.CheckScopes("read:org", checkScopesFunc), + api.AddHeaderFunc("Authorization", getAuthValue), api.AddHeader("User-Agent", fmt.Sprintf("GitHub CLI %s", Version)), // antiope-preview: Checks api.AddHeader("Accept", "application/vnd.github.antiope-preview+json"), diff --git a/internal/config/config_setup.go b/internal/config/config_setup.go index 9564ecd4c..4d2a929cb 100644 --- a/internal/config/config_setup.go +++ b/internal/config/config_setup.go @@ -24,9 +24,14 @@ var ( oauthClientSecret = "34ddeff2b558a23d38fba8a6de74f086ede1cc0b" ) -// TODO: have a conversation about whether this belongs in the "context" package -// FIXME: make testable -func setupConfigFile(filename string) (Config, error) { +// IsGitHubApp reports whether an OAuth app is "GitHub CLI" or "GitHub CLI (dev)" +func IsGitHubApp(id string) bool { + // this intentionally doesn't use `oauthClientID` because that is a variable + // that can potentially be changed at build time via GH_OAUTH_CLIENT_ID + return id == "178c6fc778ccc68e1d6a" || id == "4d747ba5675d5d66553f" +} + +func AuthFlow(notice string) (string, string, error) { var verboseStream io.Writer if strings.Contains(os.Getenv("DEBUG"), "oauth") { verboseStream = os.Stderr @@ -43,15 +48,30 @@ func setupConfigFile(filename string) (Config, error) { VerboseStream: verboseStream, } - fmt.Fprintln(os.Stderr, "Notice: authentication required") + fmt.Fprintln(os.Stderr, notice) fmt.Fprintf(os.Stderr, "Press Enter to open %s in your browser... ", flow.Hostname) _ = waitForEnter(os.Stdin) token, err := flow.ObtainAccessToken() if err != nil { - return nil, err + return "", "", err } userLogin, err := getViewer(token) + if err != nil { + return "", "", err + } + + return token, userLogin, nil +} + +func AuthFlowComplete() { + fmt.Fprintln(os.Stderr, "Authentication complete. Press Enter to continue... ") + _ = waitForEnter(os.Stdin) +} + +// FIXME: make testable +func setupConfigFile(filename string) (Config, error) { + token, userLogin, err := AuthFlow("Notice: authentication required") if err != nil { return nil, err } @@ -62,9 +82,9 @@ func setupConfigFile(filename string) (Config, error) { } yamlHosts := map[string]map[string]string{} - yamlHosts[flow.Hostname] = map[string]string{} - yamlHosts[flow.Hostname]["user"] = userLogin - yamlHosts[flow.Hostname]["oauth_token"] = token + yamlHosts[oauthHost] = map[string]string{} + yamlHosts[oauthHost]["user"] = userLogin + yamlHosts[oauthHost]["oauth_token"] = token defaultConfig := yamlConfig{ Hosts: yamlHosts, @@ -85,14 +105,9 @@ func setupConfigFile(filename string) (Config, error) { if err != nil { return nil, err } - n, err := cfgFile.Write(yamlData) - if err == nil && n < len(yamlData) { - err = io.ErrShortWrite - } - - if err == nil { - fmt.Fprintln(os.Stderr, "Authentication complete. Press Enter to continue... ") - _ = waitForEnter(os.Stdin) + _, err = cfgFile.Write(yamlData) + if err != nil { + return nil, err } // TODO cleaner error handling? this "should" always work given that we /just/ wrote the file... From fb7b5446d1e40ea43da09bd7ec85a0addaee3d4a Mon Sep 17 00:00:00 2001 From: Nate Smith Date: Tue, 28 Apr 2020 20:19:52 -0500 Subject: [PATCH 4/4] Update command/root.go Co-Authored-By: Corey Johnson --- command/root.go | 1 + 1 file changed, 1 insertion(+) diff --git a/command/root.go b/command/root.go index 8c0cbd046..fcb3dca2b 100644 --- a/command/root.go +++ b/command/root.go @@ -151,6 +151,7 @@ var apiClientForContext = func(ctx context.Context) (*api.Client, error) { getAuthValue := func() string { return fmt.Sprintf("token %s", token) } + checkScopesFunc := func(appID string) error { if config.IsGitHubApp(appID) && utils.IsTerminal(os.Stdin) && utils.IsTerminal(os.Stderr) { newToken, loginHandle, err := config.AuthFlow("Notice: additional authorization required")