Skip to content

Support both v4 and v3 GitLab User APIs #19961

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
enj wants to merge 1 commit into from
Closed

Support both v4 and v3 GitLab User APIs #19961

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 60 additions & 28 deletions pkg/oauthserver/oauth/external/gitlab/gitlab.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,26 @@ const (
// Uses the GitLab User-API (http://doc.gitlab.com/ce/api/users.html#current-user)
// and OAuth-Provider (http://doc.gitlab.com/ce/integration/oauth_provider.html)
// with default OAuth scope (http://doc.gitlab.com/ce/api/users.html#current-user)
// Requires GitLab 7.7.0 or higher
// Requires GitLab 7.7.0 or higher (released prior to 2015-09-22, we support it in OS v3.1+)
gitlabAuthorizePath = "/oauth/authorize"
gitlabTokenPath = "/oauth/token"
gitlabUserAPIPath = "/api/v3/user"
gitlabOAuthScope = "api"

// the only thing different about the APIs is the endpoint
// the JSON data is the same
// v3 was removed in GitLab 11 2018-06-22 (deprecated in GitLab 9.5 2017-08-22)
gitlabUserAPIPathV3 = "/api/v3/user"
// v4 was added in GitLab 9 2017-03-22
gitlabUserAPIPathV4 = "/api/v4/user"
)

type provider struct {
providerName string
transport http.RoundTripper
authorizeURL string
tokenURL string
userAPIURL string
userAPIURLV3 string
userAPIURLV4 string
Copy link
Copy Markdown
Contributor

@simo5 simo5 Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we may want to keep just userAPIURL and populate it with V4 endpoint.
On the first attempt if we get a not found error we check if the URL is the V4 and we switch it to V3 and retry.
I do not think it makes sense to keep falling back at every request.

Copy link
Copy Markdown
Contributor Author

@enj enj Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That will require adding some form of locking.

Copy link
Copy Markdown
Contributor

@simo5 simo5 Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or an atomic holding an index to a 2 elements array with the two strings

clientID string
clientSecret string
}
Expand All @@ -42,6 +49,7 @@ type gitlabUser struct {
Username string
Email string
Name string
Error string
}

func NewProvider(providerName string, transport http.RoundTripper, URL, clientID, clientSecret string) (external.Provider, error) {
Expand All @@ -56,7 +64,8 @@ func NewProvider(providerName string, transport http.RoundTripper, URL, clientID
transport: transport,
authorizeURL: appendPath(*u, gitlabAuthorizePath),
tokenURL: appendPath(*u, gitlabTokenPath),
userAPIURL: appendPath(*u, gitlabUserAPIPath),
userAPIURLV3: appendPath(*u, gitlabUserAPIPathV3),
userAPIURLV4: appendPath(*u, gitlabUserAPIPathV4),
clientID: clientID,
clientSecret: clientSecret,
}, nil
Expand Down Expand Up @@ -91,33 +100,10 @@ func (p *provider) AddCustomParameters(req *osincli.AuthorizeRequest) {

// GetUserIdentity implements external/interfaces/Provider.GetUserIdentity
func (p *provider) GetUserIdentity(data *osincli.AccessData) (authapi.UserIdentityInfo, bool, error) {
req, _ := http.NewRequest("GET", p.userAPIURL, nil)
req.Header.Set("Authorization", fmt.Sprintf("bearer %s", data.AccessToken))

client := http.DefaultClient
if p.transport != nil {
client = &http.Client{Transport: p.transport}
}
res, err := client.Do(req)
userdata, err := p.getUserData(data.AccessToken)
if err != nil {
return nil, false, err
}
defer res.Body.Close()

body, err := ioutil.ReadAll(res.Body)
if err != nil {
return nil, false, err
}

userdata := gitlabUser{}
err = json.Unmarshal(body, &userdata)
if err != nil {
return nil, false, err
}

if userdata.ID == 0 {
return nil, false, errors.New("Could not retrieve GitLab id")
}

identity := authapi.NewDefaultUserIdentityInfo(p.providerName, fmt.Sprintf("%d", userdata.ID))
if len(userdata.Name) > 0 {
Expand All @@ -133,3 +119,49 @@ func (p *provider) GetUserIdentity(data *osincli.AccessData) (authapi.UserIdenti

return identity, true, nil
}

func (p *provider) getUserData(token string) (*gitlabUser, error) {
// try the v4 API first
userdata, err := p.getUserDataURL(token, p.userAPIURLV4)
if err == nil {
return userdata, nil
}
Copy link
Copy Markdown
Contributor

@simo5 simo5 Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above, plus we shouldn't fall back for every error, but just for errors that indicate the endpoint does not exist.
If it does and it returns an error we should just return the error and not try any fallback.

Copy link
Copy Markdown
Contributor Author

@enj enj Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just for errors that indicate the endpoint does not exist

A 404?

// fallback to the v3 API
return p.getUserDataURL(token, p.userAPIURLV3)
}

func (p *provider) getUserDataURL(token, url string) (*gitlabUser, error) {
Copy link
Copy Markdown
Contributor

@simo5 simo5 Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like wrapping getUserIdentity with a retry with fallback to V3 only on specific errors and only once and for all would makes for less code changes too.

Copy link
Copy Markdown
Contributor Author

@enj enj Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specific errors

Unclear to me what those errors are.

only once

Unclear to me how to do that without locking.

Copy link
Copy Markdown
Contributor

@simo5 simo5 Jun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

specific errors -> 404 or other if gitlab returnssomething special for missing endpoints
only once -> do we have a way to atomically replace a pointer ?

req, err := http.NewRequest("GET", url, nil)
if err != nil {
return nil, err
}

req.Header.Set("Authorization", fmt.Sprintf("bearer %s", token))

client := &http.Client{Transport: p.transport}
res, err := client.Do(req)
if err != nil {
return nil, err
}
defer res.Body.Close()

body, err := ioutil.ReadAll(res.Body)
if err != nil {
return nil, err
}

userdata := &gitlabUser{}
if err := json.Unmarshal(body, userdata); err != nil {
return nil, err
}

if len(userdata.Error) > 0 {
return nil, errors.New(userdata.Error)
}

if userdata.ID == 0 {
return nil, errors.New("could not retrieve GitLab id")
}

return userdata, nil
}
3 changes: 2 additions & 1 deletion pkg/oauthserver/oauth/external/gitlab/gitlab_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ func TestGitLab(t *testing.T) {
providerName: "gitlab",
authorizeURL: "https://gitlab.com/oauth/authorize",
tokenURL: "https://gitlab.com/oauth/token",
userAPIURL: "https://gitlab.com/api/v3/user",
userAPIURLV3: "https://gitlab.com/api/v3/user",
userAPIURLV4: "https://gitlab.com/api/v4/user",
clientID: "clientid",
clientSecret: "clientsecret",
}
Expand Down