Oauth: Reduce error scope on upsert (#53242)

pkg/api/login_oauth.go

@@ -193,7 +193,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
 	// token.TokenType was defaulting to "bearer", which is out of spec, so we explicitly set to "Bearer"
 	token.TokenType = "Bearer"
 
-	oauthLogger.Debug("OAuthLogin: got token", "token", fmt.Sprintf("%v", token))
+	oauthLogger.Debug("OAuthLogin: got token", "token", fmt.Sprintf("%+v", token))
 
 	// set up oauth2 client
 	client := connect.Client(oauthCtx, token)

pkg/services/login/loginservice/loginservice.go

@@ -48,32 +48,33 @@ func (ls *Implementation) CreateUser(cmd user.CreateUserCommand) (*user.User, er
 func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUserCommand) error {
 	extUser := cmd.ExternalUser
 
-	usr, err := ls.AuthInfoService.LookupAndUpdate(ctx, &models.GetUserByAuthInfoQuery{
+	usr, errAuthLookup := ls.AuthInfoService.LookupAndUpdate(ctx, &models.GetUserByAuthInfoQuery{
 		AuthModule:       extUser.AuthModule,
 		AuthId:           extUser.AuthId,
 		UserLookupParams: cmd.UserLookupParams,
 	})
-	if err != nil {
+	if errAuthLookup != nil {
-		if !errors.Is(err, user.ErrUserNotFound) {
+		if !errors.Is(errAuthLookup, user.ErrUserNotFound) {
-			return err
+			return errAuthLookup
 		}
+
 		if !cmd.SignupAllowed {
 			cmd.ReqContext.Logger.Warn("Not allowing login, user not found in internal user database and allow signup = false", "authmode", extUser.AuthModule)
 			return login.ErrSignupNotAllowed
 		}
 
-		limitReached, err := ls.QuotaService.QuotaReached(cmd.ReqContext, "user")
+		limitReached, errLimit := ls.QuotaService.QuotaReached(cmd.ReqContext, "user")
-		if err != nil {
+		if errLimit != nil {
-			cmd.ReqContext.Logger.Warn("Error getting user quota.", "error", err)
+			cmd.ReqContext.Logger.Warn("Error getting user quota.", "error", errLimit)
 			return login.ErrGettingUserQuota
 		}
 		if limitReached {
 			return login.ErrUsersQuotaReached
 		}
 
-		result, err := ls.createUser(extUser)
+		result, errCreateUser := ls.createUser(extUser)
-		if err != nil {
+		if errCreateUser != nil {
-			return err
+			return errCreateUser
 		}
 
 		cmd.Result = &user.User{
@@ -105,49 +106,48 @@ func (ls *Implementation) UpsertUser(ctx context.Context, cmd *models.UpsertUser
 				AuthId:     extUser.AuthId,
 				OAuthToken: extUser.OAuthToken,
 			}
-			if err := ls.AuthInfoService.SetAuthInfo(ctx, cmd2); err != nil {
+			if errSetAuth := ls.AuthInfoService.SetAuthInfo(ctx, cmd2); errSetAuth != nil {
-				return err
+				return errSetAuth
 			}
 		}
 	} else {
 		cmd.Result = usr
 
-		err = ls.updateUser(ctx, cmd.Result, extUser)
+		if errUserMod := ls.updateUser(ctx, cmd.Result, extUser); errUserMod != nil {
-		if err != nil {
+			return errUserMod
-			return err
 		}
 
 		// Always persist the latest token at log-in
 		if extUser.AuthModule != "" && extUser.OAuthToken != nil {
-			err = ls.updateUserAuth(ctx, cmd.Result, extUser)
+			if errAuthMod := ls.updateUserAuth(ctx, cmd.Result, extUser); errAuthMod != nil {
-			if err != nil {
+				return errAuthMod
-				return err
 			}
 		}
 
 		if extUser.AuthModule == models.AuthModuleLDAP && usr.IsDisabled {
 			// Re-enable user when it found in LDAP
-			if err := ls.SQLStore.DisableUser(ctx, &models.DisableUserCommand{UserId: cmd.Result.ID, IsDisabled: false}); err != nil {
+			if errDisableUser := ls.SQLStore.DisableUser(ctx,
-				return err
+				&models.DisableUserCommand{
+					UserId: cmd.Result.ID, IsDisabled: false}); errDisableUser != nil {
+				return errDisableUser
 			}
 		}
 	}
 
-	if err := ls.syncOrgRoles(ctx, cmd.Result, extUser); err != nil {
+	if errSyncRole := ls.syncOrgRoles(ctx, cmd.Result, extUser); errSyncRole != nil {
-		return err
+		return errSyncRole
 	}
 
 	// Sync isGrafanaAdmin permission
 	if extUser.IsGrafanaAdmin != nil && *extUser.IsGrafanaAdmin != cmd.Result.IsAdmin {
-		if err := ls.SQLStore.UpdateUserPermissions(cmd.Result.ID, *extUser.IsGrafanaAdmin); err != nil {
+		if errPerms := ls.SQLStore.UpdateUserPermissions(cmd.Result.ID, *extUser.IsGrafanaAdmin); errPerms != nil {
-			return err
+			return errPerms
 		}
 	}
 
 	if ls.TeamSync != nil {
-		err := ls.TeamSync(cmd.Result, extUser)
+		if errTeamSync := ls.TeamSync(cmd.Result, extUser); errTeamSync != nil {
-		if err != nil {
+			return errTeamSync
-			return err
 		}
 	}