Merge pull request #1340 from alexmt/1184-github-groups

Issue #1184 - Github connector now returns a full group list when no org is specified

Documentation/connectors/github.md

@@ -42,8 +42,12 @@ connectors:
     # For example if a user is part of the "engineering" team of the "coreos"
     # org, the group claim would include "coreos:engineering".
     #
-    # A user MUST be a member of at least one of the following orgs to
+    # If orgs are specified in the config then user MUST be a member of at least one of the specified orgs to
     # authenticate with dex.
+    #
+    # If neither 'org' nor 'orgs' are specified in the config then user authenticate with ALL user's Github groups.
+    # Typical use case for this setup:
+    # provide read-only access to everyone and give full permissions if user has 'my-organization:admins-team' group claim.  
     orgs:
     - name: my-organization
       # Include all teams as claims.

connector/github/github.go

@@ -325,22 +325,31 @@ func (c *githubConnector) getGroups(ctx context.Context, client *http.Client, gr
 		return c.groupsForOrgs(ctx, client, userLogin)
 	} else if c.org != "" {
 		return c.teamsForOrg(ctx, client, c.org)
+	} else if groupScope {
+		return c.userGroups(ctx, client)
 	}
 	return nil, nil
 }
 
+// formatTeamName returns unique team name.
+// Orgs might have the same team names. To make team name unique it should be prefixed with the org name.
+func formatTeamName(org string, team string) string {
+	return fmt.Sprintf("%s:%s", org, team)
+}
+
 // groupsForOrgs enforces org and team constraints on user authorization
 // Cases in which user is authorized:
 // 	N orgs, no teams: user is member of at least 1 org
 // 	N orgs, M teams per org: user is member of any team from at least 1 org
 // 	N-1 orgs, M teams per org, 1 org with no teams: user is member of any team
 // from at least 1 org, or member of org with no teams
-func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client, userName string) (groups []string, err error) {
+func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client, userName string) ([]string, error) {
+	groups := make([]string, 0)
 	var inOrgNoTeams bool
 	for _, org := range c.orgs {
 		inOrg, err := c.userInOrg(ctx, client, userName, org.Name)
 		if err != nil {
-			return groups, err
+			return nil, err
 		}
 		if !inOrg {
 			continue
@@ -348,7 +357,7 @@ func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client
 
 		teams, err := c.teamsForOrg(ctx, client, org.Name)
 		if err != nil {
-			return groups, err
+			return nil, err
 		}
 		// User is in at least one org. User is authorized if no teams are specified
 		// in config; include all teams in claim. Otherwise filter out teams not in
@@ -359,19 +368,93 @@ func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client
 			c.logger.Infof("github: user %q in org %q but no teams", userName, org.Name)
 		}
 
-		// Orgs might have the same team names. We append orgPrefix to team name,
-		// i.e. "org:team", to make team names unique across orgs.
-		orgPrefix := org.Name + ":"
 		for _, teamName := range teams {
-			groups = append(groups, orgPrefix+teamName)
+			groups = append(groups, formatTeamName(org.Name, teamName))
 		}
 	}
 	if inOrgNoTeams || len(groups) > 0 {
-		return
+		return groups, nil
 	}
 	return groups, fmt.Errorf("github: user %q not in required orgs or teams", userName)
 }
 
+func (c *githubConnector) userGroups(ctx context.Context, client *http.Client) ([]string, error) {
+	orgs, err := c.userOrgs(ctx, client)
+	if err != nil {
+		return nil, err
+	}
+
+	orgTeams, err := c.userOrgTeams(ctx, client)
+	if err != nil {
+		return nil, err
+	}
+
+	groups := make([]string, 0)
+	for _, o := range orgs {
+		groups = append(groups, o)
+		if teams, ok := orgTeams[o]; ok {
+			for _, t := range teams {
+				groups = append(groups, formatTeamName(o, t))
+			}
+		}
+	}
+
+	return groups, nil
+}
+
+// userOrgs retrieves list of current user orgs
+func (c *githubConnector) userOrgs(ctx context.Context, client *http.Client) ([]string, error) {
+	groups := make([]string, 0)
+	apiURL := c.apiURL + "/user/orgs"
+	for {
+		// https://developer.github.com/v3/orgs/#list-your-organizations
+		var (
+			orgs []org
+			err  error
+		)
+		if apiURL, err = get(ctx, client, apiURL, &orgs); err != nil {
+			return nil, fmt.Errorf("github: get orgs: %v", err)
+		}
+
+		for _, o := range orgs {
+			groups = append(groups, o.Login)
+		}
+
+		if apiURL == "" {
+			break
+		}
+	}
+
+	return groups, nil
+}
+
+// userOrgTeams retrieves teams which current user belongs to.
+// Method returns a map where key is an org name and value list of teams under the org.
+func (c *githubConnector) userOrgTeams(ctx context.Context, client *http.Client) (map[string][]string, error) {
+	groups := make(map[string][]string, 0)
+	apiURL := c.apiURL + "/user/teams"
+	for {
+		// https://developer.github.com/v3/orgs/teams/#list-user-teams
+		var (
+			teams []team
+			err   error
+		)
+		if apiURL, err = get(ctx, client, apiURL, &teams); err != nil {
+			return nil, fmt.Errorf("github: get teams: %v", err)
+		}
+
+		for _, t := range teams {
+			groups[t.Org.Login] = append(groups[t.Org.Login], c.teamGroupClaim(t))
+		}
+
+		if apiURL == "" {
+			break
+		}
+	}
+
+	return groups, nil
+}
+
 // Filter the users' team memberships by 'teams' from config.
 func filterTeams(userTeams, configTeams []string) (teams []string) {
 	teamFilter := make(map[string]struct{})
@@ -419,10 +502,10 @@ func get(ctx context.Context, client *http.Client, apiURL string, v interface{})
 	return getPagination(apiURL, resp), nil
 }
 
-// getPagination checks the "Link" header field for "next" or "last" pagination
-// URLs, and returns true only if a "next" URL is found. The next pages' URL is
-// returned if a "next" URL is found. apiURL is returned if apiURL equals the
-// "last" URL or no "next" or "last" URL are found.
+// getPagination checks the "Link" header field for "next" or "last" pagination URLs,
+// and returns "next" page URL or empty string to indicate that there are no more pages.
+// Non empty next pages' URL is returned if both "last" and "next" URLs are found and next page
+// URL is not equal to last.
 //
 // https://developer.github.com/v3/#pagination
 func getPagination(apiURL string, resp *http.Response) string {
@@ -572,12 +655,14 @@ func (c *githubConnector) userInOrg(ctx context.Context, client *http.Client, us
 // https://developer.github.com/v3/orgs/teams/#response-12
 type team struct {
 	Name string `json:"name"`
-	Org  struct {
-		Login string `json:"login"`
-	} `json:"organization"`
+	Org  org    `json:"organization"`
 	Slug string `json:"slug"`
 }
 
+type org struct {
+	Login string `json:"login"`
+}
+
 // teamsForOrg queries the GitHub API for team membership within a specific organization.
 //
 // The HTTP passed client is expected to be constructed by the golang.org/x/oauth2 package,
@@ -594,14 +679,9 @@ func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client,
 			return nil, fmt.Errorf("github: get teams: %v", err)
 		}
 
-		for _, team := range teams {
-			if team.Org.Login == orgName {
-				switch c.teamNameField {
-				case "name", "":
-					groups = append(groups, team.Name)
-				case "slug":
-					groups = append(groups, team.Slug)
-				}
+		for _, t := range teams {
+			if t.Org.Login == orgName {
+				groups = append(groups, c.teamGroupClaim(t))
 			}
 		}
 
@@ -612,3 +692,13 @@ func (c *githubConnector) teamsForOrg(ctx context.Context, client *http.Client,
 
 	return groups, nil
 }
+
+// teamGroupClaim returns team slag if 'teamNameField; option is set to 'slug' otherwise returns team name.
+func (c *githubConnector) teamGroupClaim(t team) string {
+	switch c.teamNameField {
+	case "slug":
+		return t.Slug
+	default:
+		return t.Name
+	}
+}

connector/github/github_test.go

+package github
+
+import (
+	"context"
+	"crypto/tls"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/dexidp/dex/connector"
+)
+
+type testResponse struct {
+	data     interface{}
+	nextLink string
+	lastLink string
+}
+
+func TestUserGroups(t *testing.T) {
+	s := newTestServer(map[string]testResponse{
+		"/user/orgs": {
+			data:     []org{{Login: "org-1"}, {Login: "org-2"}},
+			nextLink: "/user/orgs?since=2",
+			lastLink: "/user/orgs?since=2",
+		},
+		"/user/orgs?since=2": {data: []org{{Login: "org-3"}}},
+		"/user/teams": {
+			data: []team{
+				{Name: "team-1", Org: org{Login: "org-1"}},
+				{Name: "team-2", Org: org{Login: "org-1"}},
+			},
+			nextLink: "/user/teams?since=2",
+			lastLink: "/user/teams?since=2",
+		},
+		"/user/teams?since=2": {
+			data: []team{
+				{Name: "team-3", Org: org{Login: "org-1"}},
+				{Name: "team-4", Org: org{Login: "org-2"}},
+			},
+			nextLink: "/user/teams?since=2",
+			lastLink: "/user/teams?since=2",
+		},
+	})
+	defer s.Close()
+
+	c := githubConnector{apiURL: s.URL}
+	groups, err := c.userGroups(context.Background(), newClient())
+
+	expectNil(t, err)
+	expectEquals(t, groups, []string{
+		"org-1",
+		"org-1:team-1",
+		"org-1:team-2",
+		"org-1:team-3",
+		"org-2",
+		"org-2:team-4",
+		"org-3",
+	})
+}
+
+func TestUserGroupsWithoutOrgs(t *testing.T) {
+
+	s := newTestServer(map[string]testResponse{
+		"/user/orgs":  {data: []org{}},
+		"/user/teams": {data: []team{}},
+	})
+	defer s.Close()
+
+	c := githubConnector{apiURL: s.URL}
+	groups, err := c.userGroups(context.Background(), newClient())
+
+	expectNil(t, err)
+	expectEquals(t, len(groups), 0)
+
+}
+
+func TestUserGroupsWithTeamNameFieldConfig(t *testing.T) {
+	s := newTestServer(map[string]testResponse{
+		"/user/orgs": {
+			data: []org{{Login: "org-1"}},
+		},
+		"/user/teams": {
+			data: []team{
+				{Name: "Team 1", Slug: "team-1", Org: org{Login: "org-1"}},
+			},
+		},
+	})
+	defer s.Close()
+
+	c := githubConnector{apiURL: s.URL, teamNameField: "slug"}
+	groups, err := c.userGroups(context.Background(), newClient())
+
+	expectNil(t, err)
+	expectEquals(t, groups, []string{
+		"org-1",
+		"org-1:team-1",
+	})
+}
+
+func TestUsernameIncludedInFederatedIdentity(t *testing.T) {
+
+	s := newTestServer(map[string]testResponse{
+		"/user": {data: user{Login: "some-login"}},
+		"/user/emails": {data: []userEmail{{
+			Email:    "some@email.com",
+			Verified: true,
+			Primary:  true,
+		}}},
+		"/login/oauth/access_token": {data: map[string]interface{}{
+			"access_token": "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9",
+			"expires_in":   "30",
+		}},
+	})
+	defer s.Close()
+
+	hostURL, err := url.Parse(s.URL)
+	expectNil(t, err)
+
+	req, err := http.NewRequest("GET", hostURL.String(), nil)
+	expectNil(t, err)
+
+	c := githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient()}
+	identity, err := c.HandleCallback(connector.Scopes{}, req)
+
+	expectNil(t, err)
+	expectEquals(t, identity.Username, "some-login")
+
+}
+
+func newTestServer(responses map[string]testResponse) *httptest.Server {
+	var s *httptest.Server
+	s = httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		response := responses[r.RequestURI]
+		linkParts := make([]string, 0)
+		if response.nextLink != "" {
+			linkParts = append(linkParts, fmt.Sprintf("<%s%s>; rel=\"next\"", s.URL, response.nextLink))
+		}
+		if response.lastLink != "" {
+			linkParts = append(linkParts, fmt.Sprintf("<%s%s>; rel=\"last\"", s.URL, response.lastLink))
+		}
+		if len(linkParts) > 0 {
+			w.Header().Add("Link", strings.Join(linkParts, ", "))
+		}
+		w.Header().Add("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(response.data)
+	}))
+	return s
+}
+
+func newClient() *http.Client {
+	tr := &http.Transport{
+		TLSClientConfig: &tls.Config{InsecureSkipVerify: true},
+	}
+	return &http.Client{Transport: tr}
+}
+
+func expectNil(t *testing.T, a interface{}) {
+	if a != nil {
+		t.Errorf("Expected %+v to equal nil", a)
+	}
+}
+
+func expectEquals(t *testing.T, a interface{}, b interface{}) {
+	if !reflect.DeepEqual(a, b) {
+		t.Errorf("Expected %+v to equal %+v", a, b)
+	}
+}