From a9daaadd5066025dcd4a1ba256cbf6264d16b8f5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Agn=C3=A8s=20Toulet?= <35176601+AgnesToulet@users.noreply.github.com> Date: Fri, 4 Sep 2020 14:54:59 +0200 Subject: [PATCH] API: send Login actions (#27249) * API: first version to send events about login actions * API: improve login actions events * Login: update auth test with new behavior * Login: update auth test for auth module * Login OAuth: improve functions structure * API: make struct public to use for saml * API: add send login log tests for grafana and ldap login * API: remove log from tests * Login API: fix test linting * Update pkg/api/login_oauth.go Co-authored-by: Emil Tullstedt * Login API: refactor using defer Co-authored-by: Emil Tullstedt --- pkg/api/login.go | 58 ++++++++++++--- pkg/api/login_oauth.go | 134 +++++++++++++++++++++++++++++------ pkg/api/login_test.go | 109 ++++++++++++++++++++++++++++ pkg/login/auth.go | 6 +- pkg/login/auth_test.go | 10 ++- pkg/middleware/middleware.go | 5 ++ pkg/models/user_auth.go | 10 +++ 7 files changed, 295 insertions(+), 37 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index ba23399f962..96c2b3e26ad 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -3,6 +3,8 @@ package api import ( "encoding/hex" "errors" + "fmt" + "net/http" "net/url" "strings" @@ -159,8 +161,27 @@ func (hs *HTTPServer) LoginAPIPing(c *models.ReqContext) Response { } func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Response { + action := "login" + var user *models.User + var response *NormalResponse + + defer func() { + err := response.err + if err == nil && response.errMessage != "" { + err = errors.New(response.errMessage) + } + hs.SendLoginLog(&models.SendLoginLogCommand{ + ReqContext: c, + LogAction: action, + User: user, + HTTPStatus: response.status, + Error: err, + }) + }() + if setting.DisableLoginForm { - return Error(401, "Login is disabled", nil) + response = Error(http.StatusUnauthorized, "Login is disabled", nil) + return response } authQuery := &models.LoginUserQuery{ @@ -170,27 +191,33 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res IpAddress: c.Req.RemoteAddr, } - if err := bus.Dispatch(authQuery); err != nil { - e401 := Error(401, "Invalid username or password", err) - if err == login.ErrInvalidCredentials || err == login.ErrTooManyLoginAttempts { - return e401 + err := bus.Dispatch(authQuery) + if 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 { + return response } // Do not expose disabled status, // just show incorrect user credentials error (see #17947) if err == login.ErrUserDisabled { hs.log.Warn("User is disabled", "user", cmd.User) - return e401 + return response } - return Error(500, "Error while trying to authenticate user", err) + response = Error(500, "Error while trying to authenticate user", err) + return response } - user := authQuery.User + user = authQuery.User - err := hs.loginUserWithUser(user, c) + err = hs.loginUserWithUser(user, c) if err != nil { - return Error(500, "Error while signing in user", err) + response = Error(http.StatusInternalServerError, "Error while signing in user", err) + return response } result := map[string]interface{}{ @@ -207,7 +234,8 @@ func (hs *HTTPServer) LoginPost(c *models.ReqContext, cmd dtos.LoginCommand) Res } metrics.MApiLoginPost.Inc() - return JSON(200, result) + response = JSON(http.StatusOK, result) + return response } func (hs *HTTPServer) loginUserWithUser(user *models.User, c *models.ReqContext) error { @@ -283,3 +311,11 @@ 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) + } + } +} diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index e80a176b3f7..20201c8b2eb 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -8,6 +8,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/hex" + "errors" "fmt" "io/ioutil" "net/http" @@ -40,15 +41,25 @@ func GenStateString() (string, error) { } func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { + loginInfo := LoginInformation{ + Action: "login-oauth", + } if setting.OAuthService == nil { - ctx.Handle(404, "OAuth not enabled", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusNotFound, + PublicMessage: "OAuth not enabled", + }) return } name := ctx.Params(":name") + loginInfo.Action += fmt.Sprintf("-%s", name) connect, ok := social.SocialMap[name] if !ok { - ctx.Handle(404, fmt.Sprintf("No OAuth with name %s configured", name), nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusNotFound, + PublicMessage: fmt.Sprintf("No OAuth with name %s configured", name), + }) return } @@ -56,7 +67,7 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { if errorParam != "" { errorDesc := ctx.Query("error_description") oauthLogger.Error("failed to login ", "error", errorParam, "errorDesc", errorDesc) - hs.redirectWithError(ctx, login.ErrProviderDeniedRequest, "error", errorParam, "errorDesc", errorDesc) + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, login.ErrProviderDeniedRequest, "error", errorParam, "errorDesc", errorDesc) return } @@ -65,7 +76,10 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { state, err := GenStateString() if err != nil { ctx.Logger.Error("Generating state string failed", "err", err) - ctx.Handle(500, "An internal error occurred", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "An internal error occurred", + }) return } @@ -85,14 +99,20 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { middleware.DeleteCookie(ctx.Resp, OauthStateCookieName, hs.CookieOptionsFromCfg) if cookieState == "" { - ctx.Handle(500, "login.OAuthLogin(missing saved state)", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "login.OAuthLogin(missing saved state)", + }) return } queryState := hashStatecode(ctx.Query("state"), setting.OAuthService.OAuthInfos[name].ClientSecret) oauthLogger.Info("state check", "queryState", queryState, "cookieState", cookieState) if cookieState != queryState { - ctx.Handle(500, "login.OAuthLogin(state mismatch)", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "login.OAuthLogin(state mismatch)", + }) return } @@ -111,7 +131,10 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { cert, err := tls.LoadX509KeyPair(setting.OAuthService.OAuthInfos[name].TlsClientCert, setting.OAuthService.OAuthInfos[name].TlsClientKey) if err != nil { ctx.Logger.Error("Failed to setup TlsClientCert", "oauth", name, "error", err) - ctx.Handle(500, "login.OAuthLogin(Failed to setup TlsClientCert)", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "login.OAuthLogin(Failed to setup TlsClientCert)", + }) return } @@ -122,7 +145,10 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { caCert, err := ioutil.ReadFile(setting.OAuthService.OAuthInfos[name].TlsClientCa) if err != nil { ctx.Logger.Error("Failed to setup TlsClientCa", "oauth", name, "error", err) - ctx.Handle(500, "login.OAuthLogin(Failed to setup TlsClientCa)", nil) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "login.OAuthLogin(Failed to setup TlsClientCa)", + }) return } @@ -137,7 +163,11 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { // get token from provider token, err := connect.Exchange(oauthCtx, code) if err != nil { - ctx.Handle(500, "login.OAuthLogin(NewTransportWithCode)", err) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: "login.OAuthLogin(NewTransportWithCode)", + Err: err, + }) return } // token.TokenType was defaulting to "bearer", which is out of spec, so we explicitly set to "Bearer" @@ -152,9 +182,13 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { userInfo, err := connect.UserInfo(client, token) if err != nil { if sErr, ok := err.(*social.Error); ok { - hs.redirectWithError(ctx, sErr) + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, sErr) } else { - ctx.Handle(500, fmt.Sprintf("login.OAuthLogin(get info from %s)", name), err) + hs.handleOAuthLoginError(ctx, loginInfo, LoginError{ + HttpStatus: http.StatusInternalServerError, + PublicMessage: fmt.Sprintf("login.OAuthLogin(get info from %s)", name), + Err: err, + }) } return } @@ -163,28 +197,36 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { // validate that we got at least an email address if userInfo.Email == "" { - hs.redirectWithError(ctx, login.ErrNoEmail) + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, login.ErrNoEmail) return } // validate that the email is allowed to login to grafana if !connect.IsEmailAllowed(userInfo.Email) { - hs.redirectWithError(ctx, login.ErrEmailNotAllowed) + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, login.ErrEmailNotAllowed) return } - user, err := syncUser(ctx, token, userInfo, name, connect) + loginInfo.ExtUserInfo = buildExternalUserInfo(token, userInfo, name) + loginInfo.User, err = syncUser(ctx, loginInfo.ExtUserInfo, connect) if err != nil { - hs.redirectWithError(ctx, err) + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, err) return } // login - if err := hs.loginUserWithUser(user, ctx); err != nil { - hs.redirectWithError(ctx, err) + if err := hs.loginUserWithUser(loginInfo.User, ctx); err != nil { + hs.handleOAuthLoginErrorWithRedirect(ctx, loginInfo, err) return } + hs.SendLoginLog(&models.SendLoginLogCommand{ + ReqContext: 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 { @@ -199,10 +241,10 @@ func (hs *HTTPServer) OAuthLogin(ctx *models.ReqContext) { ctx.Redirect(setting.AppSubUrl + "/") } -// syncUser syncs a Grafana user profile with the corresponding OAuth profile. -func syncUser(ctx *models.ReqContext, token *oauth2.Token, userInfo *social.BasicUserInfo, name string, - connect social.SocialConnector) (*models.User, error) { - oauthLogger.Debug("Syncing Grafana user with corresponding OAuth profile") +// buildExternalUserInfo returns a ExternalUserInfo struct from OAuth user profile +func buildExternalUserInfo(token *oauth2.Token, userInfo *social.BasicUserInfo, name string) *models.ExternalUserInfo { + oauthLogger.Debug("Building external user info from OAuth user info") + extUser := &models.ExternalUserInfo{ AuthModule: fmt.Sprintf("oauth_%s", name), OAuthToken: token, @@ -232,6 +274,16 @@ func syncUser(ctx *models.ReqContext, token *oauth2.Token, userInfo *social.Basi } } + return extUser +} + +// syncUser syncs a Grafana user profile with the corresponding OAuth profile. +func syncUser( + ctx *models.ReqContext, + extUser *models.ExternalUserInfo, + connect social.SocialConnector, +) (*models.User, error) { + oauthLogger.Debug("Syncing Grafana user with corresponding OAuth profile") // add/update user in Grafana cmd := &models.UpsertUserCommand{ ReqContext: ctx, @@ -256,3 +308,43 @@ func hashStatecode(code, seed string) string { hashBytes := sha256.Sum256([]byte(code + setting.SecretKey + seed)) return hex.EncodeToString(hashBytes[:]) } + +type LoginError struct { + HttpStatus int + PublicMessage string + Err error +} + +type LoginInformation struct { + 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 + if logErr == nil { + logErr = errors.New(err.PublicMessage) + } + + hs.SendLoginLog(&models.SendLoginLogCommand{ + 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{ + ReqContext: ctx, + LogAction: info.Action, + User: info.User, + ExternalUser: info.ExtUserInfo, + Error: err, + }) +} diff --git a/pkg/api/login_test.go b/pkg/api/login_test.go index bf76921eeb3..a84668bfda8 100644 --- a/pkg/api/login_test.go +++ b/pkg/api/login_test.go @@ -1,6 +1,7 @@ package api import ( + "context" "encoding/hex" "errors" "fmt" @@ -21,6 +22,7 @@ import ( "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func mockSetIndexViewData() { @@ -553,3 +555,110 @@ func setupAuthProxyLoginTest(enableLoginToken bool) *scenarioContext { return sc } + +type loginLogTestReceiver struct { + cmd *models.SendLoginLogCommand +} + +func (r *loginLogTestReceiver) SaveLoginLog(ctx context.Context, cmd *models.SendLoginLogCommand) error { + r.cmd = cmd + return nil +} + +func TestLoginPostSendLoginLog(t *testing.T) { + sc := setupScenarioContext("/login") + hs := &HTTPServer{ + log: log.New("test"), + Cfg: setting.NewCfg(), + License: &licensing.OSSLicensingService{}, + AuthTokenService: auth.NewFakeUserAuthTokenService(), + } + + sc.defaultHandler = Wrap(func(w http.ResponseWriter, c *models.ReqContext) Response { + cmd := dtos.LoginCommand{ + User: "admin", + Password: "admin", + } + return hs.LoginPost(c, cmd) + }) + + testReceiver := loginLogTestReceiver{} + bus.AddHandlerCtx("login-log-receiver", testReceiver.SaveLoginLog) + + type sendLoginLogCase struct { + desc string + authUser *models.User + authModule string + authErr error + cmd models.SendLoginLogCommand + } + + testUser := &models.User{ + Id: 42, + Email: "", + } + + testCases := []sendLoginLogCase{ + { + desc: "invalid credentials", + authErr: login.ErrInvalidCredentials, + cmd: models.SendLoginLogCommand{ + LogAction: "login", + HTTPStatus: 401, + Error: login.ErrInvalidCredentials, + }, + }, + { + desc: "user disabled", + authErr: login.ErrUserDisabled, + cmd: models.SendLoginLogCommand{ + LogAction: "login", + HTTPStatus: 401, + Error: login.ErrUserDisabled, + }, + }, + { + desc: "valid Grafana user", + authUser: testUser, + authModule: "grafana", + cmd: models.SendLoginLogCommand{ + LogAction: "login-grafana", + User: testUser, + HTTPStatus: 200, + }, + }, + { + desc: "valid LDAP user", + authUser: testUser, + authModule: "ldap", + cmd: models.SendLoginLogCommand{ + LogAction: "login-ldap", + User: testUser, + HTTPStatus: 200, + }, + }, + } + + for _, c := range testCases { + t.Run(c.desc, func(t *testing.T) { + bus.AddHandler("grafana-auth", func(query *models.LoginUserQuery) error { + query.User = c.authUser + query.AuthModule = c.authModule + return c.authErr + }) + + sc.m.Post(sc.url, sc.defaultHandler) + sc.fakeReqNoAssertions("POST", sc.url).exec() + + cmd := testReceiver.cmd + assert.Equal(t, c.cmd.LogAction, cmd.LogAction) + assert.Equal(t, c.cmd.HTTPStatus, cmd.HTTPStatus) + assert.Equal(t, c.cmd.Error, cmd.Error) + + if c.cmd.User != nil { + require.NotEmpty(t, cmd.User) + assert.Equal(t, c.cmd.User.Id, cmd.User.Id) + } + }) + } +} diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 2c7778ca009..314e964c832 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -41,11 +41,13 @@ func AuthenticateUser(query *models.LoginUserQuery) error { err := loginUsingGrafanaDB(query) if err == nil || (err != models.ErrUserNotFound && err != ErrInvalidCredentials && err != ErrUserDisabled) { + query.AuthModule = "grafana" return err } ldapEnabled, ldapErr := loginUsingLDAP(query) if ldapEnabled { + query.AuthModule = models.AuthModuleLDAP if ldapErr == nil || ldapErr != ldap.ErrInvalidCredentials { return ldapErr } @@ -63,10 +65,6 @@ func AuthenticateUser(query *models.LoginUserQuery) error { return ErrInvalidCredentials } - if err == models.ErrUserNotFound { - return ErrInvalidCredentials - } - return err } diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index a1284b3bc50..82e76f46f19 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -27,6 +27,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeFalse) So(sc.ldapLoginWasCalled, ShouldBeFalse) So(err, ShouldEqual, ErrPasswordEmpty) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "") }) }) @@ -44,6 +45,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeFalse) So(sc.ldapLoginWasCalled, ShouldBeFalse) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "") }) }) @@ -61,6 +63,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeFalse) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "grafana") }) }) @@ -79,6 +82,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeFalse) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "grafana") }) }) @@ -91,11 +95,12 @@ func TestAuthenticateUser(t *testing.T) { err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { - So(err, ShouldEqual, ErrInvalidCredentials) + So(err, ShouldEqual, models.ErrUserNotFound) So(sc.loginAttemptValidationWasCalled, ShouldBeTrue) So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeTrue) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "") }) }) @@ -113,6 +118,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeTrue) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeTrue) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "ldap") }) }) @@ -130,6 +136,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeTrue) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "ldap") }) }) @@ -148,6 +155,7 @@ func TestAuthenticateUser(t *testing.T) { So(sc.grafanaLoginWasCalled, ShouldBeTrue) So(sc.ldapLoginWasCalled, ShouldBeTrue) So(sc.saveInvalidLoginAttemptWasCalled, ShouldBeFalse) + So(sc.loginUserQuery.AuthModule, ShouldEqual, "ldap") }) }) diff --git a/pkg/middleware/middleware.go b/pkg/middleware/middleware.go index b2267cd4923..72bf1d95ce2 100644 --- a/pkg/middleware/middleware.go +++ b/pkg/middleware/middleware.go @@ -14,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/components/apikeygen" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/infra/remotecache" + "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/rendering" "github.com/grafana/grafana/pkg/setting" @@ -178,8 +179,12 @@ func initContextWithBasicAuth(ctx *models.ReqContext, orgId int64) bool { ctx.Logger.Debug( "Failed to authorize the user", "username", username, + "err", err, ) + if err == models.ErrUserNotFound { + err = login.ErrInvalidCredentials + } ctx.JsonApiErr(401, errStringInvalidUsernamePassword, err) return true } diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index c8fb46e357a..80cd730a3a5 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -65,6 +65,15 @@ type DeleteAuthInfoCommand struct { UserAuth *UserAuth } +type SendLoginLogCommand struct { + ReqContext *ReqContext + LogAction string + User *User + ExternalUser *ExternalUserInfo + HTTPStatus int + Error error +} + // ---------------------- // QUERIES @@ -74,6 +83,7 @@ type LoginUserQuery struct { Password string User *User IpAddress string + AuthModule string } type GetUserByAuthInfoQuery struct {