github: support enterprise instances #33
Conversation
github/login.go
Outdated
| @@ -83,3 +92,19 @@ func validateResponse(user *github.User, resp *github.Response, err error) error | |||
| } | |||
| return nil | |||
| } | |||
|
|
|||
| func githubClientFromAuthURL(authURL string, httpClient *http.Client) (*github.Client, error) { | |||
| if strings.HasPrefix(authURL, "https://github.com/") { | |||
There was a problem hiding this comment.
It worries me some we can't have the regular Github Client as the default (else) behavior. I see why you've got it this way since GithubEnterprise auth url's don't look likely to have any structure we could detect and leverage.
I think everyone using gologin with Github today should have an Endpoint.AuthURL starting with https://github.com (Github forbits http), but there may be some cases someone proxies access through another endpoint.
There was a problem hiding this comment.
How about adding a func EnterpriseCallbackHandler(config *oauth2.Config, success, failure http.Handler) method that is the same as CallbackHandler, but sets an enterprise API root URL?
There was a problem hiding this comment.
I'd rather not. Exposing the function would make Github Enterprise a first-class use case. Then if Github's enterprise product were to make a breaking change, I'd be expected to accomodate that in gologin.
As is, this PR would just let Github login automatically best-effort allow GHE (undocumented). If it becomes problematic, it can be reverted.
There was a problem hiding this comment.
Taking a step back, if a users wants a handler for GHE, it would be very much in the gologin style to just add convenience functions that chain gologin/oauth2 with a EnterpriseCallbackHandler. And that handler can go in their code or here.
It really comes down to whether GHE should be in-scope for gologin or left to users.
There was a problem hiding this comment.
Alright, I think your EnterpriseCallbackHandler is a reasonable approach. It eliminates the risk this will break anyone using regular Github login and it will be intuitive - its clear and isolated and avoids any magic of having GHE handling in the Github handler. And gologin is somewhat insulated from GHE making drastic breakages since the go-github library would have to deal with them first.
Ignore my "I'd rather not" from earlier.
|
Rebasing should solve the go get dependency failures. |
|
Addressed comments, PTAL |
github/login_test.go
Outdated
| authURL string | ||
| expClientBaseURL string | ||
| }{ | ||
| {authURL: "https://github.com/login/oauth/authorize/", expClientBaseURL: "https://api.github.com/"}, |
There was a problem hiding this comment.
You don't have to repeat the field name if you don't want.
github/login_test.go
Outdated
| {authURL: "http://github.mycompany.com/login/oauth/authorize", expClientBaseURL: "http://github.mycompany.com/api/v3/"}, | ||
| } { | ||
| client, err := githubClientFromAuthURL(test.authURL, nil) | ||
| if err != nil { |
github/login_test.go
Outdated
| @@ -108,3 +109,23 @@ func TestValidateResponse(t *testing.T) { | |||
| assert.Equal(t, ErrUnableToGetGithubUser, validateResponse(validUser, invalidResponse, nil)) | |||
| assert.Equal(t, ErrUnableToGetGithubUser, validateResponse(&github.User{}, validResponse, nil)) | |||
| } | |||
|
|
|||
| func Test_githubClientFromAuthURL(t *testing.T) { | |||
| for _, test := range []struct { | |||
There was a problem hiding this comment.
While this is valid, can you define cases separately? (kinda a nitpick, but it matches other libraries)
cases := []struct{
...
}{
...
}
for _, c := range cases {
...
}
github/login.go
Outdated
| func githubClientFromAuthURL(authURL string, httpClient *http.Client) (*github.Client, error) { | ||
| client := github.NewClient(httpClient) | ||
| if !strings.HasPrefix(authURL, "https://github.com/") { | ||
| // convert authURL to GHE baseURL https://<mycompany>.git.luolix.top/api/v3/ |
There was a problem hiding this comment.
Example isn't right, GHE wouldn't be hosted under github.com
github/login_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if got, want := client.BaseURL.String(), test.expClientBaseURL; got != want { |
There was a problem hiding this comment.
gologin already makes use of testify assert, which you can use. assert.Equal(t, client.BaseURL.String(), c.expectedBaseURL)
|
Comments addressed, PTAL. Given the addition of |
|
Squashed and rebased as e2bffa0 btw. Thanks for contributing! |
This is the actual version we use because we had a replace directive. From PR dghubble/gologin#33
Thanks for making a wonderful library!
This PR adds support for GitHub Enterprise instances. It looks at the AuthURL endpoint to infer whether the GitHub instance is github.com or an enterprise instance.