From 267d53a56aee086f1ce03f55239461f65b2fc231 Mon Sep 17 00:00:00 2001 From: Emil Tullstedt Date: Tue, 31 Jan 2023 13:22:09 +0100 Subject: [PATCH] [v9.3.x] Login: Fix panic when UpsertUser is called without ReqContext (#62571) * Login: Fix panic when UpsertUser is called without ReqContext (#62539) (cherry picked from commit b1151dd118f2a1c305a6f757661e42e59cf145c1) * login->models --- .../login/loginservice/loginservice.go | 9 +++++-- .../login/loginservice/loginservice_test.go | 27 +++++++++++++++++-- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/pkg/services/login/loginservice/loginservice.go b/pkg/services/login/loginservice/loginservice.go index 24cd0da7b45..53e9bebda78 100644 --- a/pkg/services/login/loginservice/loginservice.go +++ b/pkg/services/login/loginservice/loginservice.go @@ -54,6 +54,11 @@ func (ls *Implementation) CreateUser(cmd user.CreateUserCommand) (*user.User, er // UpsertUser updates an existing user, or if it doesn't exist, inserts a new one. func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error { + var logger log.Logger = logger + if cmd.ReqContext != nil && cmd.ReqContext.Logger != nil { + logger = cmd.ReqContext.Logger + } + extUser := cmd.ExternalUser usr, errAuthLookup := ls.AuthInfoService.LookupAndUpdate(ctx, &models.GetUserByAuthInfoQuery{ @@ -67,7 +72,7 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser } if !cmd.SignupAllowed { - cmd.ReqContext.Logger.Warn("Not allowing login, user not found in internal user database and allow signup = false", "authmode", extUser.AuthModule) + logger.Warn("Not allowing login, user not found in internal user database and allow signup = false", "authmode", extUser.AuthModule) return login.ErrSignupNotAllowed } @@ -76,7 +81,7 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser for _, srv := range []string{user.QuotaTargetSrv, org.QuotaTargetSrv} { limitReached, errLimit := ls.QuotaService.QuotaReached(cmd.ReqContext, quota.TargetSrv(srv)) if errLimit != nil { - cmd.ReqContext.Logger.Warn("Error getting user quota.", "error", errLimit) + logger.Warn("Error getting user quota.", "error", errLimit) return login.ErrGettingUserQuota } if limitReached { diff --git a/pkg/services/login/loginservice/loginservice_test.go b/pkg/services/login/loginservice/loginservice_test.go index 616c80d3d84..a1431b6247d 100644 --- a/pkg/services/login/loginservice/loginservice_test.go +++ b/pkg/services/login/loginservice/loginservice_test.go @@ -8,6 +8,9 @@ import ( "github.com/go-kit/log" "github.com/go-kit/log/level" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/accesscontrol/actest" "github.com/grafana/grafana/pkg/services/login" @@ -17,8 +20,6 @@ import ( "github.com/grafana/grafana/pkg/services/quota/quotatest" "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/services/user/usertest" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" ) func Test_syncOrgRoles_doesNotBreakWhenTryingToRemoveLastOrgAdmin(t *testing.T) { @@ -116,6 +117,28 @@ func Test_teamSync(t *testing.T) { }) } +func TestUpsertUser_crashOnLog_issue62538(t *testing.T) { + authInfoMock := &logintest.AuthInfoServiceFake{} + authInfoMock.ExpectedError = user.ErrUserNotFound + loginsvc := Implementation{ + QuotaService: quotatest.New(false, nil), + AuthInfoService: authInfoMock, + } + + email := "test_user@example.org" + upsertCmd := &models.UpsertUserCommand{ + ExternalUser: &models.ExternalUserInfo{Email: email}, + UserLookupParams: models.UserLookupParams{Email: &email}, + SignupAllowed: false, + } + + var err error + require.NotPanics(t, func() { + err = loginsvc.UpsertUser(context.Background(), upsertCmd) + }) + require.ErrorIs(t, err, login.ErrSignupNotAllowed) +} + func createSimpleUser() user.User { user := user.User{ ID: 1,