gypsydiver/1347-pr-gitlab-groups

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.  
+      # Credentials can be string literals or pulled from the environment.
-      clientID: $GITLAB_APPLICATION_ID 
+      clientID: $GITLAB_APPLICATION_ID
       clientSecret: $GITLAB_CLIENT_SECRET
       redirectURI: http://127.0.0.1:5556/dex/callback
 ```

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"
+	// used to retrieve groups from /oauth/userinfo
-)
+	// https://docs.gitlab.com/ee/integration/openid_connect_provider.html
-
+	scopeOpenID = "openid"
-var (
-	reNext = regexp.MustCompile("<([^>]+)>; rel=\"next\"")
-	reLast = regexp.MustCompile("<([^>]+)>; rel=\"last\"")
 )
 
 // 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"
+	if resp.StatusCode != http.StatusOK {
-
+		body, err := ioutil.ReadAll(resp.Body)
-	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 err != nil {
-			return nil, fmt.Errorf("gitlab: new req: %v", err)
+			return nil, fmt.Errorf("gitlab: read body: %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("%s: %s", resp.Status, body)
 	}
-	return groups, nil
+	u := struct {
-}
+		Groups []string
-
+	}{}
-func nextURL(url string, link string) string {
+	if err := json.NewDecoder(resp.Body).Decode(&u); err != nil {
-	if len(reLast.FindStringSubmatch(link)) > 1 {
+		return nil, fmt.Errorf("failed to decode response: %v", err)
-		lastPageURL := reLast.FindStringSubmatch(link)[1]
+	}
-
+
-		if url == lastPageURL {
+	return u.Groups, nil
-			return ""
-		}
-	} else {
-		return ""
-	}
-
-	if len(reNext.FindStringSubmatch(link)) > 1 {
-		return reNext.FindStringSubmatch(link)[1]
-	}
-
-	return ""
 }

connector/gitlab/gitlab_test.go

@@ -1,30 +0,0 @@
-package gitlab
-
-import "testing"
-
-var nextURLTests = []struct {
-	link     string
-	expected string
-}{
-	{"<https://gitlab.com/api/v4/groups?page=2&per_page=20>; rel=\"next\", " +
-		"<https://gitlab.com/api/v4/groups?page=1&per_page=20>; rel=\"prev\"; pet=\"cat\", " +
-		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"last\"",
-		"https://gitlab.com/api/v4/groups?page=2&per_page=20"},
-	{"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"next\", " +
-		"<https://gitlab.com/api/v4/groups?page=2&per_page=20>; rel=\"prev\"; pet=\"dog\", " +
-		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"last\"",
-		"https://gitlab.com/api/v4/groups?page=3&per_page=20"},
-	{"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; rel=\"prev\"; pet=\"bunny\", " +
-		"<https://gitlab.com/api/v4/groups?page=3&per_page=20>; 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)
-		}
-	}
-}