From 03ffd0798c5c4c33190731435bb84ed085585da4 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 31 Jan 2019 18:21:54 +0000 Subject: [PATCH 1/5] Allow an option to use the github user handle rather than an id. For downstream apps using a github handle is much simpler than working with numbers. WHilst the number is stable and the handle is not - GitHUb does give you a big scary wanring if you try and change it that bad things may happen to you, and generally few users ever change it. This can be enabled with a configuration option `useLoginAsId` --- connector/github/github.go | 9 ++++++++ connector/github/github_test.go | 38 ++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/connector/github/github.go b/connector/github/github.go index 07db4b3b..9d900414 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -48,6 +48,7 @@ type Config struct { RootCA string `json:"rootCA"` TeamNameField string `json:"teamNameField"` LoadAllGroups bool `json:"loadAllGroups"` + UseLoginAsId bool `json:"useLoginAsId"` } // Org holds org-team filters, in which teams are optional. @@ -83,6 +84,7 @@ func (c *Config) Open(id string, logger logrus.FieldLogger) (connector.Connector clientSecret: c.ClientSecret, apiURL: apiURL, logger: logger, + useLoginAsId: c.UseLoginAsId, } if c.HostName != "" { @@ -148,6 +150,8 @@ type githubConnector struct { teamNameField string // if set to true and no orgs are configured then connector loads all user claims (all orgs and team) loadAllGroups bool + // if set to true will use the users handle rather than their numeric id as the ID + useLoginAsId bool } // groupsRequired returns whether dex requires GitHub's 'read:org' scope. Dex @@ -260,12 +264,17 @@ func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (i if username == "" { username = user.Login } + identity = connector.Identity{ UserID: strconv.Itoa(user.ID), Username: username, Email: user.Email, EmailVerified: true, } + if c.useLoginAsId { + identity.UserID = user.Login + } + // Only set identity.Groups if 'orgs', 'org', or 'groups' scope are specified. if c.groupsRequired(s.Groups) { diff --git a/connector/github/github_test.go b/connector/github/github_test.go index 38a9a6cf..698bb4ce 100644 --- a/connector/github/github_test.go +++ b/connector/github/github_test.go @@ -124,10 +124,11 @@ func TestUserGroupsWithTeamNameAndSlugFieldConfig(t *testing.T) { }) } +// tests that the users login is used as their username when they have no username set func TestUsernameIncludedInFederatedIdentity(t *testing.T) { s := newTestServer(map[string]testResponse{ - "/user": {data: user{Login: "some-login"}}, + "/user": {data: user{Login: "some-login", ID: 12345678}}, "/user/emails": {data: []userEmail{{ Email: "some@email.com", Verified: true, @@ -154,6 +155,7 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) { expectNil(t, err) expectEquals(t, identity.Username, "some-login") + expectEquals(t, identity.UserID, "12345678") expectEquals(t, 0, len(identity.Groups)) c = githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient(), loadAllGroups: true} @@ -161,8 +163,42 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) { expectNil(t, err) expectEquals(t, identity.Username, "some-login") + expectEquals(t, identity.UserID, "12345678") expectEquals(t, identity.Groups, []string{"org-1"}) +} + +func TestLoginUsedAsIDWhenConfigured(t *testing.T) { + + s := newTestServer(map[string]testResponse{ + "/user": {data: user{Login: "some-login", ID: 12345678, Name:"Joe Bloggs"}}, + "/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", + }}, + "/user/orgs": { + data: []org{{Login: "org-1"}}, + }, + }) + 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(), useLoginAsId: true} + identity, err := c.HandleCallback(connector.Scopes{Groups: true}, req) + + expectNil(t, err) + expectEquals(t, identity.UserID, "some-login") + expectEquals(t, identity.Username, "Joe Bloggs") } func newTestServer(responses map[string]testResponse) *httptest.Server { From 1911b52c6be9f950626deb02fa1652cee21beed6 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 31 Jan 2019 18:26:10 +0000 Subject: [PATCH 2/5] Add documentation for the new GitHub useLoginAsId option --- Documentation/connectors/github.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Documentation/connectors/github.md b/Documentation/connectors/github.md index 78f7def4..b669e25e 100644 --- a/Documentation/connectors/github.md +++ b/Documentation/connectors/github.md @@ -67,6 +67,9 @@ connectors: # - ['acme:site-reliability-engineers'] for 'slug' # - ['acme:Site Reliability Engineers', 'acme:site-reliability-engineers'] for 'both' teamNameField: slug + # flag which will switch from using the internal GitHub id to the users handle (@mention) as the user id. + # It is possible for a user to change their own user name but it is very rare for them to do so + useLoginAsId: false ``` ## GitHub Enterprise From 5822a5ce9ec2054826e6f745707dbb86edbfb05c Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 1 Feb 2019 11:47:45 +0000 Subject: [PATCH 3/5] fix formatting of connector/github/github_test.go --- connector/github/github_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/connector/github/github_test.go b/connector/github/github_test.go index 698bb4ce..2ce085dc 100644 --- a/connector/github/github_test.go +++ b/connector/github/github_test.go @@ -167,11 +167,10 @@ func TestUsernameIncludedInFederatedIdentity(t *testing.T) { expectEquals(t, identity.Groups, []string{"org-1"}) } - func TestLoginUsedAsIDWhenConfigured(t *testing.T) { s := newTestServer(map[string]testResponse{ - "/user": {data: user{Login: "some-login", ID: 12345678, Name:"Joe Bloggs"}}, + "/user": {data: user{Login: "some-login", ID: 12345678, Name: "Joe Bloggs"}}, "/user/emails": {data: []userEmail{{ Email: "some@email.com", Verified: true, From 9840fccdbbe1f74ae920404bb3fe32a6ab98d8dd Mon Sep 17 00:00:00 2001 From: James Nord Date: Mon, 4 Feb 2019 13:53:59 +0000 Subject: [PATCH 4/5] rename useLoginAsId -> useLoginAsID --- Documentation/connectors/github.md | 2 +- connector/github/github.go | 8 ++++---- connector/github/github_test.go | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/connectors/github.md b/Documentation/connectors/github.md index b669e25e..f9395283 100644 --- a/Documentation/connectors/github.md +++ b/Documentation/connectors/github.md @@ -69,7 +69,7 @@ connectors: teamNameField: slug # flag which will switch from using the internal GitHub id to the users handle (@mention) as the user id. # It is possible for a user to change their own user name but it is very rare for them to do so - useLoginAsId: false + useLoginAsID: false ``` ## GitHub Enterprise diff --git a/connector/github/github.go b/connector/github/github.go index 9d900414..13a00f52 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -48,7 +48,7 @@ type Config struct { RootCA string `json:"rootCA"` TeamNameField string `json:"teamNameField"` LoadAllGroups bool `json:"loadAllGroups"` - UseLoginAsId bool `json:"useLoginAsId"` + UseLoginAsID bool `json:"useLoginAsID"` } // Org holds org-team filters, in which teams are optional. @@ -84,7 +84,7 @@ func (c *Config) Open(id string, logger logrus.FieldLogger) (connector.Connector clientSecret: c.ClientSecret, apiURL: apiURL, logger: logger, - useLoginAsId: c.UseLoginAsId, + useLoginAsID: c.UseLoginAsID, } if c.HostName != "" { @@ -151,7 +151,7 @@ type githubConnector struct { // if set to true and no orgs are configured then connector loads all user claims (all orgs and team) loadAllGroups bool // if set to true will use the users handle rather than their numeric id as the ID - useLoginAsId bool + useLoginAsID bool } // groupsRequired returns whether dex requires GitHub's 'read:org' scope. Dex @@ -271,7 +271,7 @@ func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (i Email: user.Email, EmailVerified: true, } - if c.useLoginAsId { + if c.useLoginAsID { identity.UserID = user.Login } diff --git a/connector/github/github_test.go b/connector/github/github_test.go index 2ce085dc..539a2e69 100644 --- a/connector/github/github_test.go +++ b/connector/github/github_test.go @@ -192,7 +192,7 @@ func TestLoginUsedAsIDWhenConfigured(t *testing.T) { req, err := http.NewRequest("GET", hostURL.String(), nil) expectNil(t, err) - c := githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient(), useLoginAsId: true} + c := githubConnector{apiURL: s.URL, hostName: hostURL.Host, httpClient: newClient(), useLoginAsID: true} identity, err := c.HandleCallback(connector.Scopes{Groups: true}, req) expectNil(t, err) From fe247b106b65e71a6f5b1b40a3b27b3850bd0a67 Mon Sep 17 00:00:00 2001 From: James Nord Date: Mon, 4 Feb 2019 13:55:20 +0000 Subject: [PATCH 5/5] remove blank line that tripped up `make verify-proto` --- connector/github/github.go | 1 - 1 file changed, 1 deletion(-) diff --git a/connector/github/github.go b/connector/github/github.go index 13a00f52..9f7c2782 100644 --- a/connector/github/github.go +++ b/connector/github/github.go @@ -275,7 +275,6 @@ func (c *githubConnector) HandleCallback(s connector.Scopes, r *http.Request) (i identity.UserID = user.Login } - // Only set identity.Groups if 'orgs', 'org', or 'groups' scope are specified. if c.groupsRequired(s.Groups) { groups, err := c.getGroups(ctx, client, s.Groups, user.Login)