From 5a6afd909697a40e151c9b83ba076e4753fad419 Mon Sep 17 00:00:00 2001 From: Arve Knudsen Date: Fri, 31 Jul 2020 09:41:13 +0200 Subject: [PATCH] OAuth: Add some debug logs (#26716) * OAuth: Add some debug logs Signed-off-by: Arve Knudsen --- pkg/api/login_oauth.go | 1 + pkg/login/social/generic_oauth.go | 2 +- pkg/services/login/login.go | 28 ++++++++++++++++++---------- pkg/services/login/login_test.go | 7 ++++--- 4 files changed, 24 insertions(+), 14 deletions(-) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 2d141c46dc8..6c947629a10 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -186,6 +186,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { if userInfo.Role != "" { rt := models.RoleType(userInfo.Role) if rt.IsValid() { + // The user will be assigned a role in either the auto-assigned organization or in the default one var orgID int64 if setting.AutoAssignOrg && setting.AutoAssignOrgId > 0 { orgID = int64(setting.AutoAssignOrgId) diff --git a/pkg/login/social/generic_oauth.go b/pkg/login/social/generic_oauth.go index 04f8e4a6b0d..505694b78f5 100644 --- a/pkg/login/social/generic_oauth.go +++ b/pkg/login/social/generic_oauth.go @@ -181,7 +181,7 @@ func (s *SocialGenericOAuth) extractToken(data *UserInfoJson, token *oauth2.Toke func (s *SocialGenericOAuth) extractAPI(data *UserInfoJson, client *http.Client) bool { rawUserInfoResponse, err := HttpGet(client, s.apiUrl) if err != nil { - s.log.Debug("Error getting user info response", "url", s.apiUrl, "error", err) + s.log.Debug("Error getting user info", "url", s.apiUrl, "error", err) return false } data.rawJSON = rawUserInfoResponse.Body diff --git a/pkg/services/login/login.go b/pkg/services/login/login.go index bc188080fbb..fa18ebc8c00 100644 --- a/pkg/services/login/login.go +++ b/pkg/services/login/login.go @@ -1,6 +1,8 @@ package login import ( + "errors" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/models" @@ -180,8 +182,11 @@ func updateUserAuth(user *models.User, extUser *models.ExternalUserInfo) error { } func syncOrgRoles(user *models.User, extUser *models.ExternalUserInfo) error { - // don't sync org roles if none are specified + logger.Debug("Syncing organization roles", "id", user.Id, "extOrgRoles", extUser.OrgRoles) + + // don't sync org roles if none is specified if len(extUser.OrgRoles) == 0 { + logger.Debug("Not syncing organization roles since external user doesn't have any") return nil } @@ -197,11 +202,12 @@ func syncOrgRoles(user *models.User, extUser *models.ExternalUserInfo) error { for _, org := range orgsQuery.Result { handledOrgIds[org.OrgId] = true - if extUser.OrgRoles[org.OrgId] == "" { + extRole := extUser.OrgRoles[org.OrgId] + if extRole == "" { deleteOrgIds = append(deleteOrgIds, org.OrgId) - } else if extUser.OrgRoles[org.OrgId] != org.Role { + } else if extRole != org.Role { // update role - cmd := &models.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: extUser.OrgRoles[org.OrgId]} + cmd := &models.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: extRole} if err := bus.Dispatch(cmd); err != nil { return err } @@ -224,13 +230,15 @@ func syncOrgRoles(user *models.User, extUser *models.ExternalUserInfo) error { // delete any removed org roles for _, orgId := range deleteOrgIds { + logger.Debug("Removing user's organization membership as part of syncing with OAuth login", + "userId", user.Id, "orgId", orgId) cmd := &models.RemoveOrgUserCommand{OrgId: orgId, UserId: user.Id} - err := bus.Dispatch(cmd) - if err == models.ErrLastOrgAdmin { - logger.Error(err.Error(), "userId", cmd.UserId, "orgId", cmd.OrgId) - continue - } - if err != nil { + if err := bus.Dispatch(cmd); err != nil { + if errors.Is(err, models.ErrLastOrgAdmin) { + logger.Error(err.Error(), "userId", cmd.UserId, "orgId", cmd.OrgId) + continue + } + return err } } diff --git a/pkg/services/login/login_test.go b/pkg/services/login/login_test.go index a133dc301ee..084f38e9f09 100644 --- a/pkg/services/login/login_test.go +++ b/pkg/services/login/login_test.go @@ -6,6 +6,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/models" log "github.com/inconshreveable/log15" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -39,9 +40,9 @@ func Test_syncOrgRoles_doesNotBreakWhenTryingToRemoveLastOrgAdmin(t *testing.T) } func Test_syncOrgRoles_whenTryingToRemoveLastOrgLogsError(t *testing.T) { - var logOutput string + logs := []string{} logger.SetHandler(log.FuncHandler(func(r *log.Record) error { - logOutput = r.Msg + logs = append(logs, r.Msg) return nil })) @@ -70,7 +71,7 @@ func Test_syncOrgRoles_whenTryingToRemoveLastOrgLogsError(t *testing.T) { err := syncOrgRoles(&user, &externalUser) require.NoError(t, err) - require.Equal(t, models.ErrLastOrgAdmin.Error(), logOutput) + assert.Contains(t, logs, models.ErrLastOrgAdmin.Error()) } func createSimpleUser() models.User {