Apply reviewer notes: style changes, make sure unit test verifies pagination

connector/github/github.go

@@ -331,7 +331,8 @@ func (c *githubConnector) getGroups(ctx context.Context, client *http.Client, gr
 	return nil, nil
 }
 
-// formatTeamName return unique team name: prgs might have the same team names team name should be prefixed with org name to make team names unique across orgs.
+// 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)
 }
@@ -342,12 +343,13 @@ func formatTeamName(org string, team string) string {
 // 	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
@@ -355,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
@@ -371,22 +373,23 @@ func (c *githubConnector) groupsForOrgs(ctx context.Context, client *http.Client
 		}
 	}
 	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) (groups []string, err error) {
+func (c *githubConnector) userGroups(ctx context.Context, client *http.Client) ([]string, error) {
 	orgs, err := c.userOrgs(ctx, client)
 	if err != nil {
-		return groups, err
+		return nil, err
 	}
 
 	orgTeams, err := c.userOrgTeams(ctx, client)
 	if err != nil {
-		return groups, err
+		return nil, err
 	}
 
+	groups := make([]string, 0)
 	for _, org := range orgs {
 		if teams, ok := orgTeams[org]; !ok {
 			groups = append(groups, org)
@@ -397,12 +400,13 @@ func (c *githubConnector) userGroups(ctx context.Context, client *http.Client) (
 		}
 	}
 
-	return groups, err
+	return groups, nil
 }
 
 // userOrgs retrieves list of current user orgs
 func (c *githubConnector) userOrgs(ctx context.Context, client *http.Client) ([]string, error) {
-	apiURL, groups := c.apiURL+"/user/orgs", []string{}
+	groups := make([]string, 0)
+	apiURL := c.apiURL + "/user/orgs"
 	for {
 		// https://developer.github.com/v3/orgs/#list-your-organizations
 		var (
@@ -413,8 +417,8 @@ func (c *githubConnector) userOrgs(ctx context.Context, client *http.Client) ([]
 			return nil, fmt.Errorf("github: get orgs: %v", err)
 		}
 
-		for _, org := range orgs {
+		for _, o := range orgs {
-			groups = append(groups, org.Login)
+			groups = append(groups, o.Login)
 		}
 
 		if apiURL == "" {
@@ -428,7 +432,8 @@ func (c *githubConnector) userOrgs(ctx context.Context, client *http.Client) ([]
 // 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) {
-	apiURL, groups := c.apiURL+"/user/teams", map[string][]string{}
+	groups := make(map[string][]string, 0)
+	apiURL := c.apiURL + "/user/teams"
 	for {
 		// https://developer.github.com/v3/orgs/teams/#list-user-teams
 		var (
@@ -439,8 +444,8 @@ func (c *githubConnector) userOrgTeams(ctx context.Context, client *http.Client)
 			return nil, fmt.Errorf("github: get teams: %v", err)
 		}
 
-		for _, team := range teams {
+		for _, t := range teams {
-			groups[team.Org.Login] = append(groups[team.Org.Login], team.Name)
+			groups[t.Org.Login] = append(groups[t.Org.Login], t.Name)
 		}
 
 		if apiURL == "" {

connector/github/github_test.go

@@ -4,37 +4,52 @@ import (
 	"context"
 	"crypto/tls"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"reflect"
+	"strings"
 	"testing"
 
 	"github.com/dexidp/dex/connector"
 )
 
-func TestUserGroups(t *testing.T) {
+type testResponse struct {
-
+	data     interface{}
-	orgs := []org{
+	nextLink string
-		{Login: "org-1"},
+	lastLink string
-		{Login: "org-2"},
-		{Login: "org-3"},
 }
 
-	teams := []team{
+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",
-	s := newTestServer(map[string]interface{}{
+			lastLink: "/user/teams?since=2",
-		"/user/orgs":  orgs,
+		},
-		"/user/teams": teams,
 	})
+	defer s.Close()
 
-	connector := githubConnector{apiURL: s.URL}
+	c := githubConnector{apiURL: s.URL}
-	groups, err := connector.userGroups(context.Background(), newClient())
+	groups, err := c.userGroups(context.Background(), newClient())
 
 	expectNil(t, err)
 	expectEquals(t, groups, []string{
@@ -44,40 +59,39 @@ func TestUserGroups(t *testing.T) {
 		"org-2:team-4",
 		"org-3",
 	})
-
-	s.Close()
 }
 
 func TestUserGroupsWithoutOrgs(t *testing.T) {
 
-	s := newTestServer(map[string]interface{}{
+	s := newTestServer(map[string]testResponse{
-		"/user/orgs":  []org{},
+		"/user/orgs":  {data: []org{}},
-		"/user/teams": []team{},
+		"/user/teams": {data: []team{}},
 	})
+	defer s.Close()
 
-	connector := githubConnector{apiURL: s.URL}
+	c := githubConnector{apiURL: s.URL}
-	groups, err := connector.userGroups(context.Background(), newClient())
+	groups, err := c.userGroups(context.Background(), newClient())
 
 	expectNil(t, err)
 	expectEquals(t, len(groups), 0)
 
-	s.Close()
 }
 
 func TestUsernameIncludedInFederatedIdentity(t *testing.T) {
 
-	s := newTestServer(map[string]interface{}{
+	s := newTestServer(map[string]testResponse{
-		"/user": user{Login: "some-login"},
+		"/user": {data: user{Login: "some-login"}},
-		"/user/emails": []userEmail{{
+		"/user/emails": {data: []userEmail{{
 			Email:    "some@email.com",
 			Verified: true,
 			Primary:  true,
-		}},
+		}}},
-		"/login/oauth/access_token": map[string]interface{}{
+		"/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)
@@ -85,20 +99,32 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) {
 	req, err := http.NewRequest("GET", hostURL.String(), nil)
 	expectNil(t, err)
 
-	githubConnector := githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient()}
+	c := githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient()}
-	identity, err := githubConnector.HandleCallback(connector.Scopes{}, req)
+	identity, err := c.HandleCallback(connector.Scopes{}, req)
 
 	expectNil(t, err)
 	expectEquals(t, identity.Username, "some-login")
 
-	s.Close()
 }
 
-func newTestServer(responses map[string]interface{}) *httptest.Server {
+func newTestServer(responses map[string]testResponse) *httptest.Server {
-	return httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+	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(responses[r.URL.Path])
+		json.NewEncoder(w).Encode(response.data)
 	}))
+	return s
 }
 
 func newClient() *http.Client {