From e10b8232d1a368bdaf7b645e776050f677dba27c Mon Sep 17 00:00:00 2001
From: Alexander Matyushentsev <Alexander_Matyushentsev@intuit.com>
Date: Thu, 15 Nov 2018 08:12:28 -0800
Subject: [PATCH] Apply reviewer notes: style changes, make sure unit test
verifies pagination
---
connector/github/github.go | 35 ++++++-----
connector/github/github_test.go | 104 ++++++++++++++++++++------------
2 files changed, 85 insertions(+), 54 deletions(-)
diff --git a/connector/github/github.go b/connector/github/github.go
index edb821ea..123e88ba 100644
--- a/connector/github/github.go
+++ b/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 {
- groups = append(groups, org.Login)
+ for _, o := range orgs {
+ 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 {
- groups[team.Org.Login] = append(groups[team.Org.Login], team.Name)
+ for _, t := range teams {
+ groups[t.Org.Login] = append(groups[t.Org.Login], t.Name)
}
if apiURL == "" {
diff --git a/connector/github/github_test.go b/connector/github/github_test.go
index 57aef554..44519a6e 100644
--- a/connector/github/github_test.go
+++ b/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) {
-
- orgs := []org{
- {Login: "org-1"},
- {Login: "org-2"},
- {Login: "org-3"},
- }
-
- teams := []team{
- {Name: "team-1", Org: org{Login: "org-1"}},
- {Name: "team-2", Org: org{Login: "org-1"}},
- {Name: "team-3", Org: org{Login: "org-1"}},
- {Name: "team-4", Org: org{Login: "org-2"}},
- }
+type testResponse struct {
+ data interface{}
+ nextLink string
+ lastLink string
+}
- s := newTestServer(map[string]interface{}{
- "/user/orgs": orgs,
- "/user/teams": teams,
+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()
- connector := githubConnector{apiURL: s.URL}
- groups, err := connector.userGroups(context.Background(), newClient())
+ c := githubConnector{apiURL: s.URL}
+ 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{}{
- "/user/orgs": []org{},
- "/user/teams": []team{},
+ s := newTestServer(map[string]testResponse{
+ "/user/orgs": {data: []org{}},
+ "/user/teams": {data: []team{}},
})
+ defer s.Close()
- connector := githubConnector{apiURL: s.URL}
- groups, err := connector.userGroups(context.Background(), newClient())
+ c := githubConnector{apiURL: s.URL}
+ 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{}{
- "/user": user{Login: "some-login"},
- "/user/emails": []userEmail{{
+ 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": 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()}
- identity, err := githubConnector.HandleCallback(connector.Scopes{}, req)
+ 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")
- s.Close()
}
-func newTestServer(responses map[string]interface{}) *httptest.Server {
- return httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+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(responses[r.URL.Path])
+ json.NewEncoder(w).Encode(response.data)
}))
+ return s
}
func newClient() *http.Client {
--
GitLab