From eac997c6d6b69fcc359ae212e373dda3ff8f3a5d Mon Sep 17 00:00:00 2001 From: mouismail Date: Tue, 25 Jun 2024 11:20:05 +0200 Subject: [PATCH 1/6] create new func to avoid the bug on issue #61 --- examples_test.go | 31 ++++++------- oauth.go | 114 ++++++++++++++++++++++++++--------------------- oauth_device.go | 8 +++- oauth_webapp.go | 7 ++- 4 files changed, 93 insertions(+), 67 deletions(-) diff --git a/examples_test.go b/examples_test.go index 336c72a..06a65d1 100644 --- a/examples_test.go +++ b/examples_test.go @@ -1,10 +1,10 @@ package oauth_test import ( - "fmt" - "os" + "fmt" + "os" - "github.com/cli/oauth" + "github.com/cli/oauth" ) // DetectFlow attempts to initiate OAuth Device flow with the server and falls back to OAuth Web @@ -12,18 +12,19 @@ import ( // github.com, as its Device flow support is globally available, but it enables logging in to // self-hosted GitHub instances as well. func ExampleFlow_DetectFlow() { - flow := &oauth.Flow{ - Host: oauth.GitHubHost("https://github.com"), - ClientID: os.Getenv("OAUTH_CLIENT_ID"), - ClientSecret: os.Getenv("OAUTH_CLIENT_SECRET"), // only applicable to web app flow - CallbackURI: "http://127.0.0.1/callback", // only applicable to web app flow - Scopes: []string{"repo", "read:org", "gist"}, - } + host, err := oauth.NewGitHubHost("https://github.com") + flow := &oauth.Flow{ + Host: host, + ClientID: os.Getenv("OAUTH_CLIENT_ID"), + ClientSecret: os.Getenv("OAUTH_CLIENT_SECRET"), // only applicable to web app flow + CallbackURI: "http://127.0.0.1/callback", // only applicable to web app flow + Scopes: []string{"repo", "read:org", "gist"}, + } - accessToken, err := flow.DetectFlow() - if err != nil { - panic(err) - } + accessToken, err := flow.DetectFlow() + if err != nil { + panic(err) + } - fmt.Printf("Access token: %s\n", accessToken.Token) + fmt.Printf("Access token: %s\n", accessToken.Token) } diff --git a/oauth.go b/oauth.go index 6439782..dda0448 100644 --- a/oauth.go +++ b/oauth.go @@ -3,77 +3,91 @@ package oauth import ( - "errors" - "fmt" - "io" - "net/http" - "net/url" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" - "github.com/cli/oauth/api" - "github.com/cli/oauth/device" + "github.com/cli/oauth/api" + "github.com/cli/oauth/device" ) type httpClient interface { - PostForm(string, url.Values) (*http.Response, error) + PostForm(string, url.Values) (*http.Response, error) } // Host defines the endpoints used to authorize against an OAuth server. type Host struct { - DeviceCodeURL string - AuthorizeURL string - TokenURL string + DeviceCodeURL string + AuthorizeURL string + TokenURL string +} + +func NewGitHubHost(hostURL string) (*Host, error) { + u, err := url.Parse(strings.TrimSpace(hostURL)) + if err != nil { + return nil, err + } + + return &Host{ + DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), + AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), + TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), + }, nil } // GitHubHost constructs a Host from the given URL to a GitHub instance. func GitHubHost(hostURL string) *Host { - u, _ := url.Parse(hostURL) + u, _ := url.Parse(hostURL) - return &Host{ - DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), - AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), - TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), - } + return &Host{ + DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), + AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), + TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), + } } // Flow facilitates a single OAuth authorization flow. type Flow struct { - // The hostname to authorize the app with. - // - // Deprecated: Use Host instead. - Hostname string - // Host configuration to authorize the app with. - Host *Host - // OAuth scopes to request from the user. - Scopes []string - // OAuth application ID. - ClientID string - // OAuth application secret. Only applicable in web application flow. - ClientSecret string - // The localhost URI for web application flow callback, e.g. "http://127.0.0.1/callback". - CallbackURI string + // The hostname to authorize the app with. + // + // Deprecated: Use Host instead. + Hostname string + // Host configuration to authorize the app with. + Host *Host + // OAuth scopes to request from the user. + Scopes []string + // OAuth application ID. + ClientID string + // OAuth application secret. Only applicable in web application flow. + ClientSecret string + // The localhost URI for web application flow callback, e.g. "http://127.0.0.1/callback". + CallbackURI string - // Display a one-time code to the user. Receives the code and the browser URL as arguments. Defaults to printing the - // code to the user on Stdout with instructions to copy the code and to press Enter to continue in their browser. - DisplayCode func(string, string) error - // Open a web browser at a URL. Defaults to opening the default system browser. - BrowseURL func(string) error - // Render an HTML page to the user upon completion of web application flow. The default is to - // render a simple message that informs the user they can close the browser tab and return to the app. - WriteSuccessHTML func(io.Writer) + // Display a one-time code to the user. Receives the code and the browser URL as arguments. Defaults to printing the + // code to the user on Stdout with instructions to copy the code and to press Enter to continue in their browser. + DisplayCode func(string, string) error + // Open a web browser at a URL. Defaults to opening the default system browser. + BrowseURL func(string) error + // Render an HTML page to the user upon completion of web application flow. The default is to + // render a simple message that informs the user they can close the browser tab and return to the app. + WriteSuccessHTML func(io.Writer) - // The HTTP client to use for API POST requests. Defaults to http.DefaultClient. - HTTPClient httpClient - // The stream to listen to keyboard input on. Defaults to os.Stdin. - Stdin io.Reader - // The stream to print UI messages to. Defaults to os.Stdout. - Stdout io.Writer + // The HTTP client to use for API POST requests. Defaults to http.DefaultClient. + HTTPClient httpClient + // The stream to listen to keyboard input on. Defaults to os.Stdin. + Stdin io.Reader + // The stream to print UI messages to. Defaults to os.Stdout. + Stdout io.Writer } // DetectFlow tries to perform Device flow first and falls back to Web application flow. func (oa *Flow) DetectFlow() (*api.AccessToken, error) { - accessToken, err := oa.DeviceFlow() - if errors.Is(err, device.ErrUnsupported) { - return oa.WebAppFlow() - } - return accessToken, err + accessToken, err := oa.DeviceFlow() + if errors.Is(err, device.ErrUnsupported) { + return oa.WebAppFlow() + } + return accessToken, err } diff --git a/oauth_device.go b/oauth_device.go index d4615ef..89edfa7 100644 --- a/oauth_device.go +++ b/oauth_device.go @@ -29,9 +29,15 @@ func (oa *Flow) DeviceFlow() (*api.AccessToken, error) { if stdout == nil { stdout = os.Stdout } + host := oa.Host if host == nil { - host = GitHubHost("https://" + oa.Hostname) + _, err := NewGitHubHost("https://" + oa.Hostname) + if err != nil { + return nil, fmt.Errorf("error parsing the hostname %w", err) + } + + //host = GitHubHost("https://" + oa.Hostname) } code, err := device.RequestCode(httpClient, host.DeviceCodeURL, oa.ClientID, oa.Scopes) diff --git a/oauth_webapp.go b/oauth_webapp.go index e448715..453de7a 100644 --- a/oauth_webapp.go +++ b/oauth_webapp.go @@ -14,8 +14,13 @@ import ( // flow, blocks until the user completes authorization and is redirected back, and returns the access token. func (oa *Flow) WebAppFlow() (*api.AccessToken, error) { host := oa.Host + if host == nil { - host = GitHubHost("https://" + oa.Hostname) + _, err := NewGitHubHost("https://" + oa.Hostname) + if err != nil { + return nil, fmt.Errorf("error parsing the hostname %w", err) + } + //host = GitHubHost("https://" + oa.Hostname) } flow, err := webapp.InitFlow() From 165a129a856bc4d6767f0d7caa7ebd0b3fdcb7f1 Mon Sep 17 00:00:00 2001 From: mouismail Date: Tue, 25 Jun 2024 11:29:48 +0200 Subject: [PATCH 2/6] Refactor func --- oauth.go | 126 +++++++++++++++++++++++++----------------------- oauth_device.go | 7 ++- oauth_webapp.go | 6 +-- 3 files changed, 72 insertions(+), 67 deletions(-) diff --git a/oauth.go b/oauth.go index dda0448..15be169 100644 --- a/oauth.go +++ b/oauth.go @@ -3,91 +3,97 @@ package oauth import ( - "errors" - "fmt" - "io" - "net/http" - "net/url" - "strings" + "errors" + "fmt" + "io" + "net/http" + "net/url" + "strings" - "github.com/cli/oauth/api" - "github.com/cli/oauth/device" + "github.com/cli/oauth/api" + "github.com/cli/oauth/device" ) type httpClient interface { - PostForm(string, url.Values) (*http.Response, error) + PostForm(string, url.Values) (*http.Response, error) } // Host defines the endpoints used to authorize against an OAuth server. type Host struct { - DeviceCodeURL string - AuthorizeURL string - TokenURL string + DeviceCodeURL string + AuthorizeURL string + TokenURL string } func NewGitHubHost(hostURL string) (*Host, error) { - u, err := url.Parse(strings.TrimSpace(hostURL)) - if err != nil { - return nil, err - } + base, err := url.Parse(strings.TrimSpace(hostURL)) + if err != nil { + return nil, err + } - return &Host{ - DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), - AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), - TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), - }, nil + createURL := func(path string) string { + u := *base // Copy base URL + u.Path = path + return u.String() + } + + return &Host{ + DeviceCodeURL: createURL("/login/device/code"), + AuthorizeURL: createURL("/login/oauth/authorize"), + TokenURL: createURL("/login/oauth/access_token"), + }, nil } // GitHubHost constructs a Host from the given URL to a GitHub instance. func GitHubHost(hostURL string) *Host { - u, _ := url.Parse(hostURL) + u, _ := url.Parse(hostURL) - return &Host{ - DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), - AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), - TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), - } + return &Host{ + DeviceCodeURL: fmt.Sprintf("%s://%s/login/device/code", u.Scheme, u.Host), + AuthorizeURL: fmt.Sprintf("%s://%s/login/oauth/authorize", u.Scheme, u.Host), + TokenURL: fmt.Sprintf("%s://%s/login/oauth/access_token", u.Scheme, u.Host), + } } // Flow facilitates a single OAuth authorization flow. type Flow struct { - // The hostname to authorize the app with. - // - // Deprecated: Use Host instead. - Hostname string - // Host configuration to authorize the app with. - Host *Host - // OAuth scopes to request from the user. - Scopes []string - // OAuth application ID. - ClientID string - // OAuth application secret. Only applicable in web application flow. - ClientSecret string - // The localhost URI for web application flow callback, e.g. "http://127.0.0.1/callback". - CallbackURI string + // The hostname to authorize the app with. + // + // Deprecated: Use Host instead. + Hostname string + // Host configuration to authorize the app with. + Host *Host + // OAuth scopes to request from the user. + Scopes []string + // OAuth application ID. + ClientID string + // OAuth application secret. Only applicable in web application flow. + ClientSecret string + // The localhost URI for web application flow callback, e.g. "http://127.0.0.1/callback". + CallbackURI string - // Display a one-time code to the user. Receives the code and the browser URL as arguments. Defaults to printing the - // code to the user on Stdout with instructions to copy the code and to press Enter to continue in their browser. - DisplayCode func(string, string) error - // Open a web browser at a URL. Defaults to opening the default system browser. - BrowseURL func(string) error - // Render an HTML page to the user upon completion of web application flow. The default is to - // render a simple message that informs the user they can close the browser tab and return to the app. - WriteSuccessHTML func(io.Writer) + // Display a one-time code to the user. Receives the code and the browser URL as arguments. Defaults to printing the + // code to the user on Stdout with instructions to copy the code and to press Enter to continue in their browser. + DisplayCode func(string, string) error + // Open a web browser at a URL. Defaults to opening the default system browser. + BrowseURL func(string) error + // Render an HTML page to the user upon completion of web application flow. The default is to + // render a simple message that informs the user they can close the browser tab and return to the app. + WriteSuccessHTML func(io.Writer) - // The HTTP client to use for API POST requests. Defaults to http.DefaultClient. - HTTPClient httpClient - // The stream to listen to keyboard input on. Defaults to os.Stdin. - Stdin io.Reader - // The stream to print UI messages to. Defaults to os.Stdout. - Stdout io.Writer + // The HTTP client to use for API POST requests. Defaults to http.DefaultClient. + HTTPClient httpClient + // The stream to listen to keyboard input on. Defaults to os.Stdin. + Stdin io.Reader + // The stream to print UI messages to. Defaults to os.Stdout. + Stdout io.Writer } // DetectFlow tries to perform Device flow first and falls back to Web application flow. func (oa *Flow) DetectFlow() (*api.AccessToken, error) { - accessToken, err := oa.DeviceFlow() - if errors.Is(err, device.ErrUnsupported) { - return oa.WebAppFlow() - } - return accessToken, err + accessToken, err := oa.DeviceFlow() + if errors.Is(err, device.ErrUnsupported) { + return oa.WebAppFlow() + } + return accessToken, err } diff --git a/oauth_device.go b/oauth_device.go index 89edfa7..3e39877 100644 --- a/oauth_device.go +++ b/oauth_device.go @@ -32,12 +32,11 @@ func (oa *Flow) DeviceFlow() (*api.AccessToken, error) { host := oa.Host if host == nil { - _, err := NewGitHubHost("https://" + oa.Hostname) + host, err := NewGitHubHost("https://" + oa.Hostname) if err != nil { - return nil, fmt.Errorf("error parsing the hostname %w", err) + return nil, fmt.Errorf("error parsing the hostname '%s': %w", host, err) } - - //host = GitHubHost("https://" + oa.Hostname) + oa.Host = host } code, err := device.RequestCode(httpClient, host.DeviceCodeURL, oa.ClientID, oa.Scopes) diff --git a/oauth_webapp.go b/oauth_webapp.go index 453de7a..22a77aa 100644 --- a/oauth_webapp.go +++ b/oauth_webapp.go @@ -16,11 +16,11 @@ func (oa *Flow) WebAppFlow() (*api.AccessToken, error) { host := oa.Host if host == nil { - _, err := NewGitHubHost("https://" + oa.Hostname) + host, err := NewGitHubHost("https://" + oa.Hostname) if err != nil { - return nil, fmt.Errorf("error parsing the hostname %w", err) + return nil, fmt.Errorf("error parsing the hostname '%s': %w", host, err) } - //host = GitHubHost("https://" + oa.Hostname) + oa.Host = host } flow, err := webapp.InitFlow() From b0f6cc74d2a0bd32b14b87ff6569fc5543160648 Mon Sep 17 00:00:00 2001 From: Mohammad Ismail <96207520+mouismail@users.noreply.github.com> Date: Mon, 7 Oct 2024 18:46:03 +0200 Subject: [PATCH 3/6] Add deprecation notice on the old function Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- oauth.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/oauth.go b/oauth.go index 15be169..9f26052 100644 --- a/oauth.go +++ b/oauth.go @@ -45,6 +45,8 @@ func NewGitHubHost(hostURL string) (*Host, error) { } // GitHubHost constructs a Host from the given URL to a GitHub instance. +// +// Deprecated: `GitHubHost` can panic with a malformed `hostURL`. Use `NewGitHubHost` instead for graceful error handling. func GitHubHost(hostURL string) *Host { u, _ := url.Parse(hostURL) From 3ded35331e19b530ad418dd04cae84b6f61e27b1 Mon Sep 17 00:00:00 2001 From: Mohammad Ismail <96207520+mouismail@users.noreply.github.com> Date: Mon, 7 Oct 2024 18:46:23 +0200 Subject: [PATCH 4/6] Add Go doc comment Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- oauth.go | 1 + 1 file changed, 1 insertion(+) diff --git a/oauth.go b/oauth.go index 9f26052..4e1c5ef 100644 --- a/oauth.go +++ b/oauth.go @@ -25,6 +25,7 @@ type Host struct { TokenURL string } +// NewGitHubHost constructs a Host from the given URL to a GitHub instance. func NewGitHubHost(hostURL string) (*Host, error) { base, err := url.Parse(strings.TrimSpace(hostURL)) if err != nil { From 7e4a31f848fe0c134919c040fbe453689406bdd2 Mon Sep 17 00:00:00 2001 From: Mohammad Ismail <96207520+mouismail@users.noreply.github.com> Date: Mon, 7 Oct 2024 18:50:54 +0200 Subject: [PATCH 5/6] handle this error to demonstrate the new intended usage of this function Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- examples_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/examples_test.go b/examples_test.go index 06a65d1..834e439 100644 --- a/examples_test.go +++ b/examples_test.go @@ -13,6 +13,9 @@ import ( // self-hosted GitHub instances as well. func ExampleFlow_DetectFlow() { host, err := oauth.NewGitHubHost("https://github.com") + if err != nil { + panic(err) + } flow := &oauth.Flow{ Host: host, ClientID: os.Getenv("OAUTH_CLIENT_ID"), From 9585a356d2b988ddd8cd690a89169cc2cd0c85ae Mon Sep 17 00:00:00 2001 From: bagtoad <47394200+BagToad@users.noreply.github.com> Date: Tue, 8 Oct 2024 15:15:24 -0600 Subject: [PATCH 6/6] Make linter happy --- examples_test.go | 38 +++++++++++++++++++------------------- oauth.go | 2 +- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/examples_test.go b/examples_test.go index 834e439..975c8c9 100644 --- a/examples_test.go +++ b/examples_test.go @@ -1,10 +1,10 @@ package oauth_test import ( - "fmt" - "os" + "fmt" + "os" - "github.com/cli/oauth" + "github.com/cli/oauth" ) // DetectFlow attempts to initiate OAuth Device flow with the server and falls back to OAuth Web @@ -12,22 +12,22 @@ import ( // github.com, as its Device flow support is globally available, but it enables logging in to // self-hosted GitHub instances as well. func ExampleFlow_DetectFlow() { - host, err := oauth.NewGitHubHost("https://github.com") - if err != nil { - panic(err) - } - flow := &oauth.Flow{ - Host: host, - ClientID: os.Getenv("OAUTH_CLIENT_ID"), - ClientSecret: os.Getenv("OAUTH_CLIENT_SECRET"), // only applicable to web app flow - CallbackURI: "http://127.0.0.1/callback", // only applicable to web app flow - Scopes: []string{"repo", "read:org", "gist"}, - } + host, err := oauth.NewGitHubHost("https://github.com") + if err != nil { + panic(err) + } + flow := &oauth.Flow{ + Host: host, + ClientID: os.Getenv("OAUTH_CLIENT_ID"), + ClientSecret: os.Getenv("OAUTH_CLIENT_SECRET"), // only applicable to web app flow + CallbackURI: "http://127.0.0.1/callback", // only applicable to web app flow + Scopes: []string{"repo", "read:org", "gist"}, + } - accessToken, err := flow.DetectFlow() - if err != nil { - panic(err) - } + accessToken, err := flow.DetectFlow() + if err != nil { + panic(err) + } - fmt.Printf("Access token: %s\n", accessToken.Token) + fmt.Printf("Access token: %s\n", accessToken.Token) } diff --git a/oauth.go b/oauth.go index 4e1c5ef..7e38c71 100644 --- a/oauth.go +++ b/oauth.go @@ -46,7 +46,7 @@ func NewGitHubHost(hostURL string) (*Host, error) { } // GitHubHost constructs a Host from the given URL to a GitHub instance. -// +// // Deprecated: `GitHubHost` can panic with a malformed `hostURL`. Use `NewGitHubHost` instead for graceful error handling. func GitHubHost(hostURL string) *Host { u, _ := url.Parse(hostURL)