From 43c5afe70e084fb3c6a0437abef23931ccda9771 Mon Sep 17 00:00:00 2001 From: "Jonathan A. Sternberg" Date: Wed, 11 Apr 2018 13:27:15 -0500 Subject: [PATCH] Fix the github oauth2 implementation with multiple emails If an account had multiple emails, the current implementation would always select the first one regardless of any other settings. This fixes it so it only chooses the primary email address that is verified. This also fixes the generic oauth2 to require verified and primary to be true if they are present. If they are not present, they are not required. --- CHANGELOG.md | 1 + oauth2/generic.go | 24 +++++++++++++++++++++++- oauth2/generic_test.go | 4 +++- oauth2/github.go | 30 ++++++++++++++++++++++-------- oauth2/github_test.go | 10 ++++++---- 5 files changed, 55 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d23bd1818..e696c3de8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,7 @@ 1. [#3128](https://github.com/influxdata/chronograf/pull/3128): Fix type error bug in Kapacitor Alert Config page and persist deleting of team and recipient in OpsGenieConfig 1. [#3149](https://github.com/influxdata/chronograf/pull/3149): Fixes errors caused by switching query tabs in CEO 1. [#3162](https://github.com/influxdata/chronograf/pull/3162): Only send threshold value to parent on blur +1. [#3168](https://github.com/influxdata/chronograf/pull/3168): Fix the github oauth2 implementation with multiple emails ## v1.4.3.1 [2018-04-02] diff --git a/oauth2/generic.go b/oauth2/generic.go index c53c98c87..54b4392fc 100644 --- a/oauth2/generic.go +++ b/oauth2/generic.go @@ -165,6 +165,28 @@ type UserEmail struct { Verified *bool `json:"verified,omitempty"` } +// GetPrimary returns if the email is the primary email. +// If primary is not present, all emails are considered the primary. +func (u *UserEmail) GetPrimary() bool { + if u == nil { + return false + } else if u.Primary == nil { + return true + } + return *u.Primary +} + +// GetVerified returns if the email has been verified. +// If verified is not present, all emails are considered verified. +func (u *UserEmail) GetVerified() bool { + if u == nil { + return false + } else if u.Verified == nil { + return true + } + return *u.Verified +} + // getPrimaryEmail gets the private email account for the authenticated user. func (g *Generic) getPrimaryEmail(client *http.Client) (string, error) { emailsEndpoint := g.APIURL + "/emails" @@ -189,7 +211,7 @@ func (g *Generic) getPrimaryEmail(client *http.Client) (string, error) { func (g *Generic) primaryEmail(emails []*UserEmail) (string, error) { for _, m := range emails { - if m != nil && m.Primary != nil && m.Verified != nil && m.Email != nil { + if m != nil && m.GetPrimary() && m.GetVerified() && m.Email != nil { return *m.Email, nil } } diff --git a/oauth2/generic_test.go b/oauth2/generic_test.go index 89bfc8818..33e54c5de 100644 --- a/oauth2/generic_test.go +++ b/oauth2/generic_test.go @@ -155,7 +155,9 @@ func TestGenericPrincipalIDDomain(t *testing.T) { Primary bool `json:"primary"` Verified bool `json:"verified"` }{ - {"martymcfly@pinheads.rok", true, false}, + {"mcfly@example.com", false, true}, + {"martymcspelledwrong@example.com", false, false}, + {"martymcfly@pinheads.rok", true, true}, } mockAPI := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if r.URL.Path == "/" { diff --git a/oauth2/github.go b/oauth2/github.go index d3a50c7c3..bfa881f0a 100644 --- a/oauth2/github.go +++ b/oauth2/github.go @@ -26,22 +26,22 @@ type Github struct { Logger chronograf.Logger } -// Name is the name of the provider +// Name is the name of the provider. func (g *Github) Name() string { return "github" } -// ID returns the github application client id +// ID returns the github application client id. func (g *Github) ID() string { return g.ClientID } -// Secret returns the github application client secret +// Secret returns the github application client secret. func (g *Github) Secret() string { return g.ClientSecret } -// Scopes for github is only the email addres and possible organizations if +// Scopes for github is only the email address and possible organizations if // we are filtering by organizations. func (g *Github) Scopes() []string { scopes := []string{"user:email"} @@ -51,7 +51,7 @@ func (g *Github) Scopes() []string { return scopes } -// Config is the Github OAuth2 exchange information and endpoints +// Config is the Github OAuth2 exchange information and endpoints. func (g *Github) Config() *oauth2.Config { return &oauth2.Config{ ClientID: g.ID(), @@ -122,7 +122,7 @@ func logResponseError(log chronograf.Logger, resp *github.Response, err error) { } } -// isMember makes sure that the user is in one of the required organizations +// isMember makes sure that the user is in one of the required organizations. func isMember(requiredOrgs []string, userOrgs []*github.Organization) bool { for _, requiredOrg := range requiredOrgs { for _, userOrg := range userOrgs { @@ -134,7 +134,7 @@ func isMember(requiredOrgs []string, userOrgs []*github.Organization) bool { return false } -// getOrganizations gets all organization for the currently authenticated user +// getOrganizations gets all organization for the currently authenticated user. func getOrganizations(client *github.Client, log chronograf.Logger) ([]*github.Organization, error) { // Get all pages of results var allOrgs []*github.Organization @@ -175,9 +175,23 @@ func getPrimaryEmail(client *github.Client, log chronograf.Logger) (string, erro func primaryEmail(emails []*github.UserEmail) (string, error) { for _, m := range emails { - if m != nil && m.Primary != nil && m.Verified != nil && m.Email != nil { + if m != nil && getPrimary(m) && getVerified(m) && m.Email != nil { return *m.Email, nil } } return "", errors.New("No primary email address") } + +func getPrimary(m *github.UserEmail) bool { + if m == nil || m.Primary == nil { + return false + } + return *m.Primary +} + +func getVerified(m *github.UserEmail) bool { + if m == nil || m.Verified == nil { + return false + } + return *m.Verified +} diff --git a/oauth2/github_test.go b/oauth2/github_test.go index dc9659570..7491769f9 100644 --- a/oauth2/github_test.go +++ b/oauth2/github_test.go @@ -18,7 +18,9 @@ func TestGithubPrincipalID(t *testing.T) { Primary bool `json:"primary"` Verified bool `json:"verified"` }{ - {"martymcfly@example.com", true, false}, + {"mcfly@example.com", false, true}, + {"martymcspelledwrong@example.com", false, false}, + {"martymcfly@example.com", true, true}, } mockAPI := httptest.NewServer(http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { if r.URL.Path != "/user/emails" { @@ -50,8 +52,8 @@ func TestGithubPrincipalID(t *testing.T) { t.Fatal("Unexpected error while retrieiving PrincipalID: err:", err) } - if email != expected[0].Email { - t.Fatal("Retrieved email was not as expected. Want:", expected[0].Email, "Got:", email) + if got, want := email, "martymcfly@example.com"; got != want { + t.Fatal("Retrieved email was not as expected. Want:", want, "Got:", got) } } @@ -63,7 +65,7 @@ func TestGithubPrincipalIDOrganization(t *testing.T) { Primary bool `json:"primary"` Verified bool `json:"verified"` }{ - {"martymcfly@example.com", true, false}, + {"martymcfly@example.com", true, true}, } expectedOrg := []struct { Login string `json:"login"`