connector/github: multiple orgs, query by teams #1013

estroz · 2017-08-03T22:41:01Z

connector/github: support for multiple orgs and teams within orgs; allow filtering by both orgs and teams

Documentation: examples of GitHub orgs field with multiple orgs and org with teams

The GitHub connector should give more fine-grained control over which organizations and teams have access to an application. Restricting authorization to (multiple) teams within (multiple) orgs by specifying them in the connector config section gives dex users such control.

Tests: set up organizations with/out teams and a test user; went through scenarios listed in this function description

Note: this PR obsoletes #1008 because similar errors are returned if user is not a org member

ericchiang · 2017-08-04T16:39:32Z

connector/github/github.go

@@ -34,16 +34,22 @@ type Config struct {
 	ClientID     string `json:"clientID"`
 	ClientSecret string `json:"clientSecret"`
 	RedirectURI  string `json:"redirectURI"`
-	Org          string `json:"org"`


We should keep this field and behavior around to not break existing users.

You've still removed the feature.

@estroz btw this behavior has changed. You'll start prefixing "(org):" to any group.

Adding legacy logic now.

ericchiang · 2017-08-04T16:40:15Z

connector/github/github.go

 	HostName     string `json:"hostName"`
 	RootCA       string `json:"rootCA"`
 }

+// Org holds org-team filters, in which teams are optional


Need to explain the impact of providing the teams field. Also the logic about what teams or orgs a user has to belong to to login.

ericchiang · 2017-08-04T16:46:05Z

connector/github/github.go

+// 	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) isUserAuthorized(ctx context.Context, client *http.Client, userName string) (groups []string, isAuthorized bool, err error) {


Method name could probably be more specific. Maybe just listGroups

Also two orgs can have teams with the same name. We probably need to come up with some naming schema for the groups. e.g. ( org ):( team ) "coreos:engineers"

ericchiang · 2017-08-04T16:48:43Z

connector/github/github.go

+		err = fmt.Errorf("unexpected return status (code %d): %q", resp.StatusCode, resp.Status)
+	}
+
+	// 204 if user is a member


Ha. Fun API.

ericchiang · 2017-08-04T16:49:40Z

connector/github/github.go

+
+	err = nil
+	switch resp.StatusCode {
+	case http.StatusNoContent, http.StatusFound, http.StatusNotFound:


Probably want to map these to a message about what they represent https://developer.github.com/v3/orgs/members/#check-membership

Status(Not)Found mean the same thing in our context (requester == user), so I'll return a 'user not in org' error

ericchiang · 2017-08-04T16:49:44Z

connector/github/github.go

+	}
+	defer resp.Body.Close()
+
+	err = nil


err is already nil

ericchiang · 2017-08-04T16:52:09Z

connector/github/github.go

+			isAuthorized = true
+			groups = append(groups, teams...)
+			c.logger.Debugf("github: user %q in org %q", userName, org.Name)
+			continue


Really confused by this continue and the one following it. Not clear what you're trying to do here.

ericchiang · 2017-08-04T16:55:37Z

connector/github/github.go

+		return userTeams, true, nil
+	}
+
+	wantedTeams := make(map[string]struct{})


wantedTeams is probably a bad name here. Can probably just write this as a method

func filterTeams(whiteList, userTeams []string) []string

ericchiang · 2017-08-04T16:55:54Z

connector/github/github.go

+// 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 *githubConnector) userInOrgTeams(ctx context.Context, client *http.Client, userName string, org Org) ([]string, bool, error) {
+	if inOrg, err := c.userInOrg(ctx, client, userName, org.Name); err != nil || !inOrg {


Can this logic just be done in isUserAuthorized?

ericchiang · 2017-08-07T20:56:04Z

connector/github/github.go

+		}
+
+		if teams, err = c.teams(ctx, client, org.Name); err != nil {
+			c.logger.Errorf("teams: %s", err)


Just return this error immediately.

ericchiang · 2017-08-07T20:57:11Z

connector/github/github.go

+	for _, org := range c.orgs {
+		inOrg, err = c.userInOrg(ctx, client, userName, org.Name)
+		if err != nil {
+			c.logger.Errorf("user in org: %s", err)


Just return this error immediately.

ericchiang · 2017-08-07T20:58:07Z

connector/github/github.go

+		// User is in at least in one org. No team restrictions are imposed from
+		// config. We add all the teams in the organization the user is a member of
+		// to 'groups' to preserve legacy behavior.
+		if len(org.Teams) == 0 {


if len(org.Teams) > 0 { teams = filterTeams(teams, org.Teams) } groups = append(groups, joinOrgToTeams(org.Name, teams)...)

No need to have these two continue statements.

How about something like:

orgPrefix := org.Name + ":" if len(org.Teams) > 0 { for _, teamName := range filterTeams(teams, org.Teams) { groups = append(groups, orgPrefix+teamName) } } else { groups = append(groups, orgPrefix) }

If teams are specified in the config file, then check if the user is a team member and add those teams to groups. Otherwise, use an org-level group "orgName:" to indicate that the user is an org member.

ericchiang · 2017-08-07T20:58:27Z

connector/github/github.go

+
+// Orgs might have the same team names. We append the orgName to team name
+// delimited by a colon, "org:team", to make team names unique across orgs.
+func joinOrgToTeams(orgName string, teamNames []string) []string {


doesn't need to be it's own method.

ericchiang · 2017-08-07T21:47:52Z

connector/github/github.go

+				groups = append(groups, orgPrefix+teamName)
+				c.logger.Debugf("github: user %q in org %q team %q", userName, org.Name, teamName)
+			}
+		} else { // No team restrictions. Append "org:" to groups


I really don't like this group value. I'd rather people just evaluate teams, not orgs for auth-Z decisions.

ericchiang · 2017-08-07T21:48:54Z

connector/github/github.go

@@ -34,16 +34,22 @@ type Config struct {
 	ClientID     string `json:"clientID"`
 	ClientSecret string `json:"clientSecret"`
 	RedirectURI  string `json:"redirectURI"`
-	Org          string `json:"org"`


@estroz btw this behavior has changed. You'll start prefixing "(org):" to any group.

ericchiang · 2017-08-07T23:06:09Z

Documentation/github-connector.md

+        teams:
+          - read-team
+          - blue-team
+      - name: my-organization-no-teams


Need to document why these two are different.

ericchiang · 2017-08-07T23:06:49Z

Documentation/github-connector.md

-    # Optional organization to pull teams from, communicate through the
-    # "groups" scope.
+    # Optional organizations and teams, communicated through the
+    # "groups" scope. The 'org' field is kept for legacy users.


Lots of "legacy" references. I'm starting to feel old :)

We can probably just drop these docs.

ericchiang · 2017-08-07T23:27:45Z

Documentation/github-connector.md

+    # this field to restrict access to the specified GitHub org.
+    # org: my-organization
+    #
+    # New 'orgs' field. 'orgs' supports organizations with or without teams.


Few nits.

This field is 'new' in the release notes, for docs it just is.

"restrict access" is probably the wrong phrase.

Let's just remove the NOTE: ... line and Legacy 'org' field... block.

Maybe something like this:

# Query the following organizations for group information if the "groups" # scope is provided. Group claims are formatted as "(org):(team)". 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 # authenticate with dex. orgs: - name: my-org # Include all teams as claims. - name: my-org-with-teams # A white list of teams. Only include group claims for these teams. teams: - read-team - blue-team

ericchiang · 2017-08-08T00:20:41Z

@estroz lgtm. Have you tested this?

estroz · 2017-08-08T00:22:40Z

@ericchiang yes, although I'm going through the process once more to be sure.

ericchiang · 2017-08-08T00:24:20Z

connector/github/github.go

+// from at least 1 org, or member of org with no teams
+func (c *githubConnector) listGroups(ctx context.Context, client *http.Client, userName string) ([]string, bool, error) {
+	var (
+		inOrg, inOrgNoTeams bool


inOrgNoTeams is never set. It's always false.

ericchiang · 2017-08-08T00:25:10Z

connector/github/github.go

+func (c *githubConnector) listGroups(ctx context.Context, client *http.Client, userName string) ([]string, bool, error) {
+	var (
+		inOrg, inOrgNoTeams bool
+		teams, groups       []string


inOrg, teams, and err do not need to be defined outside the for loop. Just define them as used in the for loop,

estroz · 2017-08-08T01:35:31Z

@ericchiang tested with both orgs and org fields and made some updates. works as expected in all scenarios described by docs.

ericchiang · 2017-08-08T16:55:46Z

connector/github/github.go

+// 	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) listGroups(ctx context.Context, client *http.Client, userName string) (groups []string, err error) {


If a user isn't in any of the orgs, this will still return []string{}, nil and the user will login successfully.

Good catch. Fixing now.

Fixed and tested.

Documentation: examples of GitHub `orgs` field with multiple orgs and org with teams; note legacy behavior

ericchiang

lgtm

connector/github: multiple orgs, query by teams

estroz requested review from ericchiang and rithujohn191 August 3, 2017 22:41

ericchiang reviewed Aug 4, 2017

View reviewed changes

ericchiang mentioned this pull request Aug 4, 2017

connector/github: prevent users not in org from authenticating #1008

Closed

estroz force-pushed the multi-org-team-filters branch 5 times, most recently from e1e8a52 to c950d82 Compare August 7, 2017 19:34

ericchiang reviewed Aug 7, 2017

View reviewed changes

estroz force-pushed the multi-org-team-filters branch 3 times, most recently from 054ed7b to 2d48bc8 Compare August 7, 2017 22:21

ericchiang reviewed Aug 7, 2017

View reviewed changes

estroz force-pushed the multi-org-team-filters branch from 2d48bc8 to 670e23e Compare August 7, 2017 23:18

ericchiang reviewed Aug 7, 2017

View reviewed changes

estroz force-pushed the multi-org-team-filters branch 2 times, most recently from 46ed036 to 589aa35 Compare August 7, 2017 23:52

ericchiang reviewed Aug 8, 2017

View reviewed changes

estroz force-pushed the multi-org-team-filters branch from 589aa35 to cba6ba0 Compare August 8, 2017 01:31

ericchiang reviewed Aug 8, 2017

View reviewed changes

connector/github: multiple orgs, query by teams

9d15480

Documentation: examples of GitHub `orgs` field with multiple orgs and org with teams; note legacy behavior

estroz force-pushed the multi-org-team-filters branch from cba6ba0 to 9d15480 Compare August 8, 2017 17:58

ericchiang approved these changes Aug 8, 2017

View reviewed changes

estroz merged commit 45bf061 into dexidp:master Aug 8, 2017

estroz deleted the multi-org-team-filters branch August 8, 2017 18:37

estroz mentioned this pull request Aug 8, 2017

Documentation: github org redirect caveat #1019

Merged

estroz mentioned this pull request Aug 24, 2017

connector/github: grab membership of orgs and/or teams instead of just teams #691

Closed

rithujohn191 mentioned this pull request Aug 24, 2017

connectors/github: Authentication via GitHub allows users outside of Org to login #920

Closed

mmrath pushed a commit to mmrath/dex that referenced this pull request Sep 2, 2019

Merge pull request dexidp#1013 from estroz/multi-org-team-filters

2ba67c6

connector/github: multiple orgs, query by teams

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connector/github: multiple orgs, query by teams #1013

connector/github: multiple orgs, query by teams #1013

estroz commented Aug 3, 2017 •

edited

Loading

ericchiang Aug 4, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

estroz Aug 7, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

estroz Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 4, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

estroz Aug 7, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017 •

edited

Loading

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

ericchiang Aug 7, 2017

ericchiang commented Aug 8, 2017

estroz commented Aug 8, 2017

ericchiang Aug 8, 2017

ericchiang Aug 8, 2017

estroz commented Aug 8, 2017

ericchiang Aug 8, 2017 •

edited

Loading

estroz Aug 8, 2017

estroz Aug 8, 2017

ericchiang left a comment

connector/github: multiple orgs, query by teams #1013

connector/github: multiple orgs, query by teams #1013

Conversation

estroz commented Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang commented Aug 8, 2017

estroz commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

estroz commented Aug 8, 2017

ericchiang Aug 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericchiang left a comment

Choose a reason for hiding this comment

estroz commented Aug 3, 2017 •

edited

Loading

ericchiang Aug 7, 2017 •

edited

Loading

ericchiang Aug 8, 2017 •

edited

Loading