From f21e6a0f0019cc468afdc2fa2ad25a6551e504b0 Mon Sep 17 00:00:00 2001 From: gypsydiver Date: Tue, 20 Nov 2018 11:18:50 +0100 Subject: [PATCH] gypsydiver/1347-pr-gitlab-groups --- Documentation/connectors/gitlab.md | 8 ++- connector/gitlab/gitlab.go | 98 ++++++++---------------------- connector/gitlab/gitlab_test.go | 30 --------- 3 files changed, 32 insertions(+), 104 deletions(-) delete mode 100644 connector/gitlab/gitlab_test.go diff --git a/Documentation/connectors/gitlab.md b/Documentation/connectors/gitlab.md index 1ca93d50..e22d0b46 100644 --- a/Documentation/connectors/gitlab.md +++ b/Documentation/connectors/gitlab.md @@ -10,6 +10,8 @@ When a client redeems a refresh token through dex, dex will re-query GitLab to u Register a new application via `User Settings -> Applications` ensuring the callback URL is `(dex issuer)/callback`. For example if dex is listening at the non-root path `https://auth.example.com/dex` the callback would be `https://auth.example.com/dex/callback`. +The application requires the user to grant the `read_user` and `openid` scopes. The latter is required only if group membership is a desired claim. + The following is an example of a configuration for `examples/config-dev.yaml`: ```yaml @@ -20,10 +22,10 @@ connectors: # Required field for connector name. name: GitLab config: - # optional, default = https://gitlab.com + # optional, default = https://gitlab.com baseURL: https://gitlab.com - # Credentials can be string literals or pulled from the environment. - clientID: $GITLAB_APPLICATION_ID + # Credentials can be string literals or pulled from the environment. + clientID: $GITLAB_APPLICATION_ID clientSecret: $GITLAB_CLIENT_SECRET redirectURI: http://127.0.0.1:5556/dex/callback ``` diff --git a/connector/gitlab/gitlab.go b/connector/gitlab/gitlab.go index 5d1fc156..41b0beb2 100644 --- a/connector/gitlab/gitlab.go +++ b/connector/gitlab/gitlab.go @@ -8,7 +8,6 @@ import ( "fmt" "io/ioutil" "net/http" - "regexp" "strconv" "github.com/sirupsen/logrus" @@ -18,14 +17,11 @@ import ( ) const ( - // https://docs.gitlab.com/ee/integration/oauth_provider.html#authorized-applications + // read operations of the /api/v4/user endpoint scopeUser = "read_user" - scopeAPI = "api" -) - -var ( - reNext = regexp.MustCompile("<([^>]+)>; rel=\"next\"") - reLast = regexp.MustCompile("<([^>]+)>; rel=\"last\"") + // used to retrieve groups from /oauth/userinfo + // https://docs.gitlab.com/ee/integration/openid_connect_provider.html + scopeOpenID = "openid" ) // Config holds configuration options for gilab logins. @@ -45,12 +41,6 @@ type gitlabUser struct { IsAdmin bool } -type gitlabGroup struct { - ID int - Name string - Path string -} - // Open returns a strategy for logging in through GitLab. func (c *Config) Open(id string, logger logrus.FieldLogger) (connector.Connector, error) { if c.BaseURL == "" { @@ -87,7 +77,7 @@ type gitlabConnector struct { func (c *gitlabConnector) oauth2Config(scopes connector.Scopes) *oauth2.Config { gitlabScopes := []string{scopeUser} if scopes.Groups { - gitlabScopes = []string{scopeAPI} + gitlabScopes = []string{scopeUser, scopeOpenID} } gitlabEndpoint := oauth2.Endpoint{AuthURL: c.baseURL + "/oauth/authorize", TokenURL: c.baseURL + "/oauth/token"} @@ -239,64 +229,30 @@ func (c *gitlabConnector) user(ctx context.Context, client *http.Client) (gitlab // The HTTP passed client is expected to be constructed by the golang.org/x/oauth2 package, // which inserts a bearer token as part of the request. func (c *gitlabConnector) groups(ctx context.Context, client *http.Client) ([]string, error) { + req, err := http.NewRequest("GET", c.baseURL+"/oauth/userinfo", nil) + if err != nil { + return nil, fmt.Errorf("gitlab: new req: %v", err) + } + req = req.WithContext(ctx) + resp, err := client.Do(req) + if err != nil { + return nil, fmt.Errorf("gitlab: get URL %v", err) + } + defer resp.Body.Close() - apiURL := c.baseURL + "/api/v4/groups" - - groups := []string{} - var gitlabGroups []gitlabGroup - for { - // 100 is the maximum number for per_page that allowed by gitlab - req, err := http.NewRequest("GET", apiURL, nil) + if resp.StatusCode != http.StatusOK { + body, err := ioutil.ReadAll(resp.Body) if err != nil { - return nil, fmt.Errorf("gitlab: new req: %v", err) - } - req = req.WithContext(ctx) - resp, err := client.Do(req) - if err != nil { - return nil, fmt.Errorf("gitlab: get groups: %v", err) - } - defer resp.Body.Close() - - if resp.StatusCode != http.StatusOK { - body, err := ioutil.ReadAll(resp.Body) - if err != nil { - return nil, fmt.Errorf("gitlab: read body: %v", err) - } - return nil, fmt.Errorf("%s: %s", resp.Status, body) - } - - if err := json.NewDecoder(resp.Body).Decode(&gitlabGroups); err != nil { - return nil, fmt.Errorf("gitlab: unmarshal groups: %v", err) - } - - for _, group := range gitlabGroups { - groups = append(groups, group.Name) - } - - link := resp.Header.Get("Link") - - apiURL = nextURL(apiURL, link) - if apiURL == "" { - break + return nil, fmt.Errorf("gitlab: read body: %v", err) } + return nil, fmt.Errorf("%s: %s", resp.Status, body) } - return groups, nil -} - -func nextURL(url string, link string) string { - if len(reLast.FindStringSubmatch(link)) > 1 { - lastPageURL := reLast.FindStringSubmatch(link)[1] - - if url == lastPageURL { - return "" - } - } else { - return "" - } - - if len(reNext.FindStringSubmatch(link)) > 1 { - return reNext.FindStringSubmatch(link)[1] - } - - return "" + u := struct { + Groups []string + }{} + if err := json.NewDecoder(resp.Body).Decode(&u); err != nil { + return nil, fmt.Errorf("failed to decode response: %v", err) + } + + return u.Groups, nil } diff --git a/connector/gitlab/gitlab_test.go b/connector/gitlab/gitlab_test.go deleted file mode 100644 index ef669261..00000000 --- a/connector/gitlab/gitlab_test.go +++ /dev/null @@ -1,30 +0,0 @@ -package gitlab - -import "testing" - -var nextURLTests = []struct { - link string - expected string -}{ - {"; rel=\"next\", " + - "; rel=\"prev\"; pet=\"cat\", " + - "; rel=\"last\"", - "https://gitlab.com/api/v4/groups?page=2&per_page=20"}, - {"; rel=\"next\", " + - "; rel=\"prev\"; pet=\"dog\", " + - "; rel=\"last\"", - "https://gitlab.com/api/v4/groups?page=3&per_page=20"}, - {"; rel=\"prev\"; pet=\"bunny\", " + - "; rel=\"last\"", - ""}, -} - -func TestNextURL(t *testing.T) { - apiURL := "https://gitlab.com/api/v4/groups" - for _, tt := range nextURLTests { - apiURL = nextURL(apiURL, tt.link) - if apiURL != tt.expected { - t.Errorf("Should have returned %s, got %s", tt.expected, apiURL) - } - } -}