API: replace SendLoginLogCommand with LoginHook (#28777)

* API: replace SendLoginLogCommand with LoginHook

* LoginInfo: Query -> LoginUsername

pkg/api/login.go

@@ -3,7 +3,6 @@ package api
 import (
 	"encoding/hex"
 	"errors"
-	"fmt"
 	"net/http"
 	"net/url"
 	"strings"
@@ -168,7 +167,7 @@ func (hs *HTTPServer) LoginAPIPing(c *models.ReqContext) Response {
 }
 
 func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Response {
-	action := "login"
+	authModule := ""
 	var user *models.User
 	var response *NormalResponse
 
@@ -177,14 +176,13 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
 		if err == nil && response.errMessage != "" {
 			err = errors.New(response.errMessage)
 		}
-		hs.SendLoginLog(&models.SendLoginLogCommand{
+		hs.HooksService.RunLoginHook(&models.LoginInfo{
-			ReqContext:    c,
+			AuthModule:    authModule,
-			LogAction:     action,
 			User:          user,
 			LoginUsername: cmd.User,
 			HTTPStatus:    response.status,
 			Error:         err,
-		})
+		}, c)
 	}()
 
 	if setting.DisableLoginForm {
@@ -200,9 +198,7 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res
 	}
 
 	err := bus.Dispatch(authQuery)
-	if authQuery.AuthModule != "" {
+	authModule = authQuery.AuthModule
-		action += fmt.Sprintf("-%s", authQuery.AuthModule)
-	}
 	if err != nil {
 		response = Error(401, "Invalid username or password", err)
 		if err == login.ErrInvalidCredentials || err == login.ErrTooManyLoginAttempts || err == models.ErrUserNotFound {
@@ -324,11 +320,3 @@ func (hs *HTTPServer) RedirectResponseWithError(ctx *models.ReqContext, err erro
 
 	return Redirect(setting.AppSubUrl + "/login")
 }
-
-func (hs *HTTPServer) SendLoginLog(cmd *models.SendLoginLogCommand) {
-	if err := bus.Dispatch(cmd); err != nil {
-		if err != bus.ErrHandlerNotFound {
-			hs.log.Warn("Error while sending login log", "err", err)
-		}
-	}
-}

pkg/api/login_oauth.go

@@ -38,8 +38,8 @@ func GenStateString() (string, error) {
 }
 
 func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
-	loginInfo := LoginInformation{
+	loginInfo := models.LoginInfo{
-		Action: "login-oauth",
+		AuthModule: "oauth",
 	}
 	if setting.OAuthService == nil {
 		hs.handleOAuthLoginError(ctx, loginInfo, LoginError{
@@ -50,7 +50,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
 	}
 
 	name := ctx.Params(":name")
-	loginInfo.Action += fmt.Sprintf("-%s", name)
+	loginInfo.AuthModule = name
 	connect, ok := social.SocialMap[name]
 	if !ok {
 		hs.handleOAuthLoginError(ctx, loginInfo, LoginError{
@@ -172,8 +172,8 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
 		return
 	}
 
-	loginInfo.ExtUserInfo = buildExternalUserInfo(token, userInfo, name)
+	loginInfo.ExternalUser = *buildExternalUserInfo(token, userInfo, name)
-	loginInfo.User, err = syncUser(ctx, loginInfo.ExtUserInfo, connect)
+	loginInfo.User, err = syncUser(ctx, &loginInfo.ExternalUser, connect)
 	if err != nil {
 		hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, err)
 		return
@@ -185,13 +185,8 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) {
 		return
 	}
 
-	hs.SendLoginLog(&models.SendLoginLogCommand{
+	loginInfo.HTTPStatus = http.StatusOK
-		ReqContext:   ctx,
+	hs.HooksService.RunLoginHook(&loginInfo, ctx)
-		LogAction:    loginInfo.Action,
-		User:         loginInfo.User,
-		ExternalUser: loginInfo.ExtUserInfo,
-		HTTPStatus:   http.StatusOK,
-	})
 	metrics.MApiLoginOAuth.Inc()
 
 	if redirectTo, err := url.QueryUnescape(ctx.GetCookie("redirect_to")); err == nil && len(redirectTo) > 0 {
@@ -280,36 +275,21 @@ type LoginError struct {
 	Err           error
 }
 
-type LoginInformation struct {
+func (hs *HTTPServer) handleOAuthLoginError(ctx *models.ReqContext, info models.LoginInfo, err LoginError) {
-	Action      string
-	User        *models.User
-	ExtUserInfo *models.ExternalUserInfo
-}
-
-func (hs *HTTPServer) handleOAuthLoginError(ctx *models.ReqContext, info LoginInformation, err LoginError) {
 	ctx.Handle(err.HttpStatus, err.PublicMessage, err.Err)
 
-	logErr := err.Err
+	info.Error = err.Err
-	if logErr == nil {
+	if info.Error == nil {
-		logErr = errors.New(err.PublicMessage)
+		info.Error = errors.New(err.PublicMessage)
+	}
+	info.HTTPStatus = err.HttpStatus
+
+	hs.HooksService.RunLoginHook(&info, ctx)
 }
 
-	hs.SendLoginLog(&models.SendLoginLogCommand{
+func (hs *HTTPServer) handleOAuthLoginErrorWithRedirect(ctx *models.ReqContext, info models.LoginInfo, err error, v ...interface{}) {
-		ReqContext: ctx,
-		LogAction:  info.Action,
-		HTTPStatus: err.HttpStatus,
-		Error:      logErr,
-	})
-}
-
-func (hs *HTTPServer) handleOAuthLoginErrorWithRedirect(ctx *models.ReqContext, info LoginInformation, err error, v ...interface{}) {
 	hs.redirectWithError(ctx, err, v...)
 
-	hs.SendLoginLog(&models.SendLoginLogCommand{
+	info.Error = err
-		ReqContext:   ctx,
+	hs.HooksService.RunLoginHook(&info, ctx)
-		LogAction:    info.Action,
-		User:         info.User,
-		ExternalUser: info.ExtUserInfo,
-		Error:        err,
-	})
 }

pkg/api/login_test.go

@@ -1,7 +1,6 @@
 package api
 
 import (
-	"context"
 	"encoding/hex"
 	"errors"
 	"fmt"
@@ -18,6 +17,7 @@ import (
 	"github.com/grafana/grafana/pkg/login"
 	"github.com/grafana/grafana/pkg/models"
 	"github.com/grafana/grafana/pkg/services/auth"
+	"github.com/grafana/grafana/pkg/services/hooks"
 	"github.com/grafana/grafana/pkg/services/licensing"
 	"github.com/grafana/grafana/pkg/setting"
 	"github.com/grafana/grafana/pkg/util"
@@ -318,6 +318,7 @@ func TestLoginPostRedirect(t *testing.T) {
 	hs := &HTTPServer{
 		log:              &FakeLogger{},
 		Cfg:              setting.NewCfg(),
+		HooksService:     &hooks.HooksService{},
 		License:          &licensing.OSSLicensingService{},
 		AuthTokenService: auth.NewFakeUserAuthTokenService(),
 	}
@@ -591,22 +592,23 @@ func setupAuthProxyLoginTest(enableLoginToken bool) *scenarioContext {
 	return sc
 }
 
-type loginLogTestReceiver struct {
+type loginHookTest struct {
-	cmd *models.SendLoginLogCommand
+	info *models.LoginInfo
 }
 
-func (r *loginLogTestReceiver) SaveLoginLog(ctx context.Context, cmd *models.SendLoginLogCommand) error {
+func (r *loginHookTest) LoginHook(loginInfo *models.LoginInfo, req *models.ReqContext) {
-	r.cmd = cmd
+	r.info = loginInfo
-	return nil
 }
 
-func TestLoginPostSendLoginLog(t *testing.T) {
+func TestLoginPostRunLokingHook(t *testing.T) {
 	sc := setupScenarioContext("/login")
+	hookService := &hooks.HooksService{}
 	hs := &HTTPServer{
 		log:              log.New("test"),
 		Cfg:              setting.NewCfg(),
 		License:          &licensing.OSSLicensingService{},
 		AuthTokenService: auth.NewFakeUserAuthTokenService(),
+		HooksService:     hookService,
 	}
 
 	sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response {
@@ -617,28 +619,26 @@ func TestLoginPostSendLoginLog(t *testing.T) {
 		return hs.LoginPost(c, cmd)
 	})
 
-	testReceiver := loginLogTestReceiver{}
+	testHook := loginHookTest{}
-	bus.AddHandlerCtx("login-log-receiver", testReceiver.SaveLoginLog)
+	hookService.AddLoginHook(testHook.LoginHook)
-
-	type sendLoginLogCase struct {
-		desc       string
-		authUser   *models.User
-		authModule string
-		authErr    error
-		cmd        models.SendLoginLogCommand
-	}
 
 	testUser := &models.User{
 		Id:    42,
 		Email: "",
 	}
 
-	testCases := []sendLoginLogCase{
+	testCases := []struct {
+		desc       string
+		authUser   *models.User
+		authModule string
+		authErr    error
+		info       models.LoginInfo
+	}{
 		{
 			desc:    "invalid credentials",
 			authErr: login.ErrInvalidCredentials,
-			cmd: models.SendLoginLogCommand{
+			info: models.LoginInfo{
-				LogAction:  "login",
+				AuthModule: "",
 				HTTPStatus: 401,
 				Error:      login.ErrInvalidCredentials,
 			},
@@ -646,8 +646,8 @@ func TestLoginPostSendLoginLog(t *testing.T) {
 		{
 			desc:    "user disabled",
 			authErr: login.ErrUserDisabled,
-			cmd: models.SendLoginLogCommand{
+			info: models.LoginInfo{
-				LogAction:  "login",
+				AuthModule: "",
 				HTTPStatus: 401,
 				Error:      login.ErrUserDisabled,
 			},
@@ -656,8 +656,8 @@ func TestLoginPostSendLoginLog(t *testing.T) {
 			desc:       "valid Grafana user",
 			authUser:   testUser,
 			authModule: "grafana",
-			cmd: models.SendLoginLogCommand{
+			info: models.LoginInfo{
-				LogAction:  "login-grafana",
+				AuthModule: "grafana",
 				User:       testUser,
 				HTTPStatus: 200,
 			},
@@ -666,8 +666,8 @@ func TestLoginPostSendLoginLog(t *testing.T) {
 			desc:       "valid LDAP user",
 			authUser:   testUser,
 			authModule: "ldap",
-			cmd: models.SendLoginLogCommand{
+			info: models.LoginInfo{
-				LogAction:  "login-ldap",
+				AuthModule: "ldap",
 				User:       testUser,
 				HTTPStatus: 200,
 			},
@@ -685,15 +685,15 @@ func TestLoginPostSendLoginLog(t *testing.T) {
 			sc.m.Post(sc.url, sc.defaultHandler)
 			sc.fakeReqNoAssertions("POST", sc.url).exec()
 
-			cmd := testReceiver.cmd
+			info := testHook.info
-			assert.Equal(t, c.cmd.LogAction, cmd.LogAction)
+			assert.Equal(t, c.info.AuthModule, info.AuthModule)
-			assert.Equal(t, "admin", cmd.LoginUsername)
+			assert.Equal(t, "admin", info.LoginUsername)
-			assert.Equal(t, c.cmd.HTTPStatus, cmd.HTTPStatus)
+			assert.Equal(t, c.info.HTTPStatus, info.HTTPStatus)
-			assert.Equal(t, c.cmd.Error, cmd.Error)
+			assert.Equal(t, c.info.Error, info.Error)
 
-			if c.cmd.User != nil {
+			if c.info.User != nil {
-				require.NotEmpty(t, cmd.User)
+				require.NotEmpty(t, info.User)
-				assert.Equal(t, c.cmd.User.Id, cmd.User.Id)
+				assert.Equal(t, c.info.User.Id, info.User.Id)
 			}
 		})
 	}

pkg/models/user_auth.go

@@ -36,6 +36,15 @@ type ExternalUserInfo struct {
 	IsDisabled     bool
 }
 
+type LoginInfo struct {
+	AuthModule    string
+	User          *User
+	ExternalUser  ExternalUserInfo
+	LoginUsername string
+	HTTPStatus    int
+	Error         error
+}
+
 // ---------------------
 // COMMANDS
 
@@ -65,16 +74,6 @@ type DeleteAuthInfoCommand struct {
 	UserAuth *UserAuth
 }
 
-type SendLoginLogCommand struct {
-	ReqContext    *ReqContext
-	LogAction     string
-	User          *User
-	ExternalUser  *ExternalUserInfo
-	LoginUsername string
-	HTTPStatus    int
-	Error         error
-}
-
 // ----------------------
 // QUERIES
 

pkg/services/hooks/hooks.go

@@ -8,8 +8,11 @@ import (
 
 type IndexDataHook func(indexData *dtos.IndexViewData, req *models.ReqContext)
 
+type LoginHook func(loginInfo *models.LoginInfo, req *models.ReqContext)
+
 type HooksService struct {
 	indexDataHooks []IndexDataHook
+	loginHooks     []LoginHook
 }
 
 func init() {
@@ -29,3 +32,13 @@ func (srv *HooksService) RunIndexDataHooks(indexData *dtos.IndexViewData, req *m
 		hook(indexData, req)
 	}
 }
+
+func (srv *HooksService) AddLoginHook(hook LoginHook) {
+	srv.loginHooks = append(srv.loginHooks, hook)
+}
+
+func (srv *HooksService) RunLoginHook(loginInfo *models.LoginInfo, req *models.ReqContext) {
+	for _, hook := range srv.loginHooks {
+		hook(loginInfo, req)
+	}
+}