From 8032b43838637a599170705f803147039924fa7a Mon Sep 17 00:00:00 2001 From: Alexander Zobnin Date: Tue, 20 Oct 2020 09:56:48 +0300 Subject: [PATCH] OAuth: configurable user name attribute (#28286) * OAuth: more user-frienly logging * OAuth: custom user name attribute * OAuth: remove deprecated nameAttributeName option * OAuth: nameAttributePath tests * OAuth: add name_attribute_path config option * OAuth: docs for name_attribute_path option * move docs to the separate branch --- conf/defaults.ini | 1 + conf/sample.ini | 1 + pkg/login/social/generic_oauth.go | 38 +++++++--- pkg/login/social/generic_oauth_test.go | 98 ++++++++++++++++++++++++++ pkg/login/social/social.go | 1 + 5 files changed, 130 insertions(+), 9 deletions(-) diff --git a/conf/defaults.ini b/conf/defaults.ini index 750cc23a329..4c4c37d0414 100644 --- a/conf/defaults.ini +++ b/conf/defaults.ini @@ -442,6 +442,7 @@ scopes = user:email email_attribute_name = email:primary email_attribute_path = login_attribute_path = +name_attribute_path = role_attribute_path = id_token_attribute_name = auth_url = diff --git a/conf/sample.ini b/conf/sample.ini index 1ebd84a236f..46f45b68864 100644 --- a/conf/sample.ini +++ b/conf/sample.ini @@ -432,6 +432,7 @@ ;email_attribute_name = email:primary ;email_attribute_path = ;login_attribute_path = +;name_attribute_path = ;id_token_attribute_name = ;auth_url = https://foo.bar/login/oauth/authorize ;token_url = https://foo.bar/login/oauth/access_token diff --git a/pkg/login/social/generic_oauth.go b/pkg/login/social/generic_oauth.go index efcc7472f32..dc9af92790f 100644 --- a/pkg/login/social/generic_oauth.go +++ b/pkg/login/social/generic_oauth.go @@ -24,6 +24,7 @@ type SocialGenericOAuth struct { emailAttributeName string emailAttributePath string loginAttributePath string + nameAttributePath string roleAttributePath string idTokenAttributeName string teamIds []int @@ -107,13 +108,7 @@ func (s *SocialGenericOAuth) UserInfo(client *http.Client, token *oauth2.Token) s.log.Debug("Processing external user info", "source", data.source, "data", data) if userInfo.Name == "" { - if data.Name != "" { - s.log.Debug("Setting user info name from name field") - userInfo.Name = data.Name - } else if data.DisplayName != "" { - s.log.Debug("Setting user info name from display name field") - userInfo.Name = data.DisplayName - } + userInfo.Name = s.extractUserName(data) } if userInfo.Login == "" { @@ -250,7 +245,7 @@ func (s *SocialGenericOAuth) extractFromToken(token *oauth2.Token) *UserInfoJson data.rawJSON = rawJSON data.source = "token" - s.log.Debug("Received id_token", "raw_json", string(data.rawJSON), "data", data) + s.log.Debug("Received id_token", "raw_json", string(data.rawJSON), "data", data.String()) return &data } @@ -272,7 +267,7 @@ func (s *SocialGenericOAuth) extractFromAPI(client *http.Client) *UserInfoJson { data.rawJSON = rawJSON data.source = "API" - s.log.Debug("Received user info response from API", "raw_json", string(rawJSON), "data", data) + s.log.Debug("Received user info response from API", "raw_json", string(rawJSON), "data", data.String()) return &data } @@ -306,6 +301,31 @@ func (s *SocialGenericOAuth) extractEmail(data *UserInfoJson) string { return "" } +func (s *SocialGenericOAuth) extractUserName(data *UserInfoJson) string { + if s.nameAttributePath != "" { + name, err := s.searchJSONForAttr(s.nameAttributePath, data.rawJSON) + if err != nil { + s.log.Error("Failed to search JSON for attribute", "error", err) + } else if name != "" { + s.log.Debug("Setting user info name from nameAttributePath", "nameAttributePath", s.nameAttributePath) + return name + } + } + + if data.Name != "" { + s.log.Debug("Setting user info name from name field") + return data.Name + } + + if data.DisplayName != "" { + s.log.Debug("Setting user info name from display name field") + return data.DisplayName + } + + s.log.Debug("Unable to find user info name") + return "" +} + func (s *SocialGenericOAuth) extractRole(data *UserInfoJson) (string, error) { if s.roleAttributePath == "" { return "", nil diff --git a/pkg/login/social/generic_oauth_test.go b/pkg/login/social/generic_oauth_test.go index d246188513b..0832e04c899 100644 --- a/pkg/login/social/generic_oauth_test.go +++ b/pkg/login/social/generic_oauth_test.go @@ -428,6 +428,104 @@ func TestUserInfoSearchesForLogin(t *testing.T) { }) } +func TestUserInfoSearchesForName(t *testing.T) { + t.Run("Given a generic OAuth provider", func(t *testing.T) { + provider := SocialGenericOAuth{ + SocialBase: &SocialBase{ + log: log.NewWithLevel("generic_oauth_test", log15.LvlDebug), + }, + nameAttributePath: "name", + } + + tests := []struct { + Name string + ResponseBody interface{} + OAuth2Extra interface{} + NameAttributePath string + ExpectedName string + }{ + { + Name: "Given a valid id_token, a valid name path, no API response, use id_token", + OAuth2Extra: map[string]interface{}{ + // { "name": "John Doe", "login": "johndoe", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJsb2dpbiI6ImpvaG5kb2UiLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwibmFtZSI6IkpvaG4gRG9lIn0.oMsXH0mHxUSYMXh6FonZIWh8LgNIcYbKRLSO1bwnfSI", + }, + NameAttributePath: "name", + ExpectedName: "John Doe", + }, + { + Name: "Given a valid id_token, no name path, no API response, use id_token", + OAuth2Extra: map[string]interface{}{ + // { "name": "John Doe", "login": "johndoe", "email": "john.doe@example.com" } + "id_token": "eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJsb2dpbiI6ImpvaG5kb2UiLCJlbWFpbCI6ImpvaG4uZG9lQGV4YW1wbGUuY29tIiwibmFtZSI6IkpvaG4gRG9lIn0.oMsXH0mHxUSYMXh6FonZIWh8LgNIcYbKRLSO1bwnfSI", + }, + NameAttributePath: "", + ExpectedName: "John Doe", + }, + { + Name: "Given no id_token, a valid name path, a valid API response, use API response", + ResponseBody: map[string]interface{}{ + "user_name": "John Doe", + "login": "johndoe", + "email": "john.doe@example.com", + }, + NameAttributePath: "user_name", + ExpectedName: "John Doe", + }, + { + Name: "Given no id_token, no name path, a valid API response, use API response", + ResponseBody: map[string]interface{}{ + "display_name": "John Doe", + "login": "johndoe", + }, + NameAttributePath: "", + ExpectedName: "John Doe", + }, + { + Name: "Given no id_token, a name path, a valid API response without a name, use API response", + ResponseBody: map[string]interface{}{ + "display_name": "John Doe", + "username": "john.doe", + }, + NameAttributePath: "name", + ExpectedName: "John Doe", + }, + { + Name: "Given no id_token, a valid name path, no API response, no data", + NameAttributePath: "name", + ExpectedName: "", + }, + } + + for _, test := range tests { + provider.nameAttributePath = test.NameAttributePath + t.Run(test.Name, func(t *testing.T) { + body, err := json.Marshal(test.ResponseBody) + require.NoError(t, err) + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + w.Header().Set("Content-Type", "application/json") + t.Log("Writing fake API response body", "body", test.ResponseBody) + _, err = w.Write(body) + require.NoError(t, err) + })) + provider.apiUrl = ts.URL + staticToken := oauth2.Token{ + AccessToken: "", + TokenType: "", + RefreshToken: "", + Expiry: time.Now(), + } + + token := staticToken.WithExtra(test.OAuth2Extra) + actualResult, err := provider.UserInfo(ts.Client(), token) + require.NoError(t, err) + require.Equal(t, test.ExpectedName, actualResult.Name) + }) + } + }) +} + func TestPayloadCompression(t *testing.T) { provider := SocialGenericOAuth{ SocialBase: &SocialBase{ diff --git a/pkg/login/social/social.go b/pkg/login/social/social.go index 74ee6056a6f..a9b44897aa0 100644 --- a/pkg/login/social/social.go +++ b/pkg/login/social/social.go @@ -181,6 +181,7 @@ func NewOAuthService() { apiUrl: info.ApiUrl, emailAttributeName: info.EmailAttributeName, emailAttributePath: info.EmailAttributePath, + nameAttributePath: sec.Key("name_attribute_path").String(), roleAttributePath: info.RoleAttributePath, loginAttributePath: sec.Key("login_attribute_path").String(), idTokenAttributeName: sec.Key("id_token_attribute_name").String(),