From 1c5afa731f80ca9ec045fb45f34f5d6d9858a82a Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Thu, 8 Feb 2018 17:13:58 -0500 Subject: [PATCH 01/20] shared library for managing external user accounts --- pkg/api/login.go | 4 +- pkg/api/login_oauth.go | 73 ++---- pkg/login/auth.go | 21 +- pkg/login/auth_test.go | 10 +- pkg/login/brute_force_login_protection.go | 2 +- .../brute_force_login_protection_test.go | 4 +- pkg/login/ext_user.go | 157 ++++++++++++ pkg/login/grafana_login.go | 2 +- pkg/login/grafana_login_test.go | 4 +- pkg/login/ldap.go | 231 +++++------------- pkg/login/ldap_login.go | 5 +- pkg/login/ldap_login_test.go | 8 +- pkg/middleware/auth_proxy.go | 2 +- pkg/middleware/auth_proxy_test.go | 2 +- pkg/middleware/middleware.go | 3 +- pkg/middleware/middleware_test.go | 3 +- pkg/models/user_auth.go | 66 +++++ .../sqlstore/migrations/migrations.go | 1 + .../sqlstore/migrations/user_auth_mig.go | 24 ++ pkg/services/sqlstore/user.go | 1 + pkg/services/sqlstore/user_auth.go | 130 ++++++++++ pkg/social/grafana_com_oauth.go | 2 + pkg/social/social.go | 1 + 23 files changed, 509 insertions(+), 247 deletions(-) create mode 100644 pkg/login/ext_user.go create mode 100644 pkg/models/user_auth.go create mode 100644 pkg/services/sqlstore/migrations/user_auth_mig.go create mode 100644 pkg/services/sqlstore/user_auth.go diff --git a/pkg/api/login.go b/pkg/api/login.go index 671e5fb7ecd..5b1d96758bb 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -101,13 +101,13 @@ func LoginPost(c *m.ReqContext, cmd dtos.LoginCommand) Response { return Error(401, "Login is disabled", nil) } - authQuery := login.LoginUserQuery{ + authQuery := m.LoginUserQuery{ Username: cmd.User, Password: cmd.Password, IpAddress: c.Req.RemoteAddr, } - if err := bus.Dispatch(&authQuery); err != nil { + if err := login.AuthenticateUser(c, &authQuery); err != nil { if err == login.ErrInvalidCredentials || err == login.ErrTooManyLoginAttempts { return Error(401, "Invalid username or password", err) } diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 1dba38e9cbd..38ded183947 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -6,7 +6,6 @@ import ( "crypto/tls" "crypto/x509" "encoding/base64" - "errors" "fmt" "io/ioutil" "net/http" @@ -14,24 +13,16 @@ import ( "golang.org/x/oauth2" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" + "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/metrics" m "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/quota" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/social" ) -var ( - ErrProviderDeniedRequest = errors.New("Login provider denied login request") - ErrEmailNotAllowed = errors.New("Required email domain not fulfilled") - ErrSignUpNotAllowed = errors.New("Signup is not allowed for this adapter") - ErrUsersQuotaReached = errors.New("Users quota reached") - ErrNoEmail = errors.New("Login provider didn't return an email address") - oauthLogger = log.New("oauth") -) +var oauthLogger = log.New("oauth") func GenStateString() string { rnd := make([]byte, 32) @@ -56,7 +47,7 @@ func OAuthLogin(ctx *m.ReqContext) { if errorParam != "" { errorDesc := ctx.Query("error_description") oauthLogger.Error("failed to login ", "error", errorParam, "errorDesc", errorDesc) - redirectWithError(ctx, ErrProviderDeniedRequest, "error", errorParam, "errorDesc", errorDesc) + redirectWithError(ctx, login.ErrProviderDeniedRequest, "error", errorParam, "errorDesc", errorDesc) return } @@ -149,54 +140,42 @@ func OAuthLogin(ctx *m.ReqContext) { // validate that we got at least an email address if userInfo.Email == "" { - redirectWithError(ctx, ErrNoEmail) + redirectWithError(ctx, login.ErrNoEmail) return } // validate that the email is allowed to login to grafana if !connect.IsEmailAllowed(userInfo.Email) { - redirectWithError(ctx, ErrEmailNotAllowed) + redirectWithError(ctx, login.ErrEmailNotAllowed) return } - userQuery := m.GetUserByEmailQuery{Email: userInfo.Email} - err = bus.Dispatch(&userQuery) - - // create account if missing - if err == m.ErrUserNotFound { - if !connect.IsSignupAllowed() { - redirectWithError(ctx, ErrSignUpNotAllowed) - return - } - limitReached, err := quota.QuotaReached(ctx, "user") - if err != nil { - ctx.Handle(500, "Failed to get user quota", err) - return - } - if limitReached { - redirectWithError(ctx, ErrUsersQuotaReached) - return - } - cmd := m.CreateUserCommand{ - Login: userInfo.Login, - Email: userInfo.Email, - Name: userInfo.Name, - Company: userInfo.Company, - DefaultOrgRole: userInfo.Role, - } + extUser := m.ExternalUserInfo{ + AuthModule: "oauth_" + name, + AuthId: userInfo.Id, + Name: userInfo.Name, + Login: userInfo.Login, + Email: userInfo.Email, + OrgRoles: map[int64]m.RoleType{}, + } - if err = bus.Dispatch(&cmd); err != nil { - ctx.Handle(500, "Failed to create account", err) - return - } + if userInfo.Role != "" { + extUser.OrgRoles[1] = m.RoleType(userInfo.Role) + } - userQuery.Result = &cmd.Result - } else if err != nil { - ctx.Handle(500, "Unexpected error", err) + // add/update user in grafana + userQuery := &m.UpsertUserCommand{ + ExternalUser: &extUser, + SignupAllowed: connect.IsSignupAllowed(), + } + err = login.UpsertUser(ctx, userQuery) + if err != nil { + redirectWithError(ctx, err) + return } // login - loginUserWithUser(userQuery.Result, ctx) + loginUserWithUser(userQuery.User, ctx) metrics.M_Api_Login_OAuth.Inc() diff --git a/pkg/login/auth.go b/pkg/login/auth.go index 5527c7271d6..b95126271cf 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -8,23 +8,22 @@ import ( ) var ( - ErrInvalidCredentials = errors.New("Invalid Username or Password") - ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked") + ErrEmailNotAllowed = errors.New("Required email domain not fulfilled") + ErrInvalidCredentials = errors.New("Invalid Username or Password") + ErrNoEmail = errors.New("Login provider didn't return an email address") + ErrProviderDeniedRequest = errors.New("Login provider denied login request") + ErrSignUpNotAllowed = errors.New("Signup is not allowed for this adapter") + ErrTooManyLoginAttempts = errors.New("Too many consecutive incorrect login attempts for user. Login for user temporarily blocked") + ErrUsersQuotaReached = errors.New("Users quota reached") + ErrGettingUserQuota = errors.New("Error getting user quota") ) -type LoginUserQuery struct { - Username string - Password string - User *m.User - IpAddress string -} - func Init() { bus.AddHandler("auth", AuthenticateUser) loadLdapConfig() } -func AuthenticateUser(query *LoginUserQuery) error { +func AuthenticateUser(ctx *m.ReqContext, query *m.LoginUserQuery) error { if err := validateLoginAttempts(query.Username); err != nil { return err } @@ -34,7 +33,7 @@ func AuthenticateUser(query *LoginUserQuery) error { return err } - ldapEnabled, ldapErr := loginUsingLdap(query) + ldapEnabled, ldapErr := loginUsingLdap(ctx, query) if ldapEnabled { if ldapErr == nil || ldapErr != ErrInvalidCredentials { return ldapErr diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index 59d3c8f2b33..932125c410e 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -151,7 +151,7 @@ func TestAuthenticateUser(t *testing.T) { } type authScenarioContext struct { - loginUserQuery *LoginUserQuery + loginUserQuery *m.LoginUserQuery grafanaLoginWasCalled bool ldapLoginWasCalled bool loginAttemptValidationWasCalled bool @@ -161,14 +161,14 @@ type authScenarioContext struct { type authScenarioFunc func(sc *authScenarioContext) func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) { - loginUsingGrafanaDB = func(query *LoginUserQuery) error { + loginUsingGrafanaDB = func(query *m.LoginUserQuery) error { sc.grafanaLoginWasCalled = true return err } } func mockLoginUsingLdap(enabled bool, err error, sc *authScenarioContext) { - loginUsingLdap = func(query *LoginUserQuery) (bool, error) { + loginUsingLdap = func(query *m.LoginUserQuery) (bool, error) { sc.ldapLoginWasCalled = true return enabled, err } @@ -182,7 +182,7 @@ func mockLoginAttemptValidation(err error, sc *authScenarioContext) { } func mockSaveInvalidLoginAttempt(sc *authScenarioContext) { - saveInvalidLoginAttempt = func(query *LoginUserQuery) { + saveInvalidLoginAttempt = func(query *m.LoginUserQuery) { sc.saveInvalidLoginAttemptWasCalled = true } } @@ -195,7 +195,7 @@ func authScenario(desc string, fn authScenarioFunc) { origSaveInvalidLoginAttempt := saveInvalidLoginAttempt sc := &authScenarioContext{ - loginUserQuery: &LoginUserQuery{ + loginUserQuery: &m.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", diff --git a/pkg/login/brute_force_login_protection.go b/pkg/login/brute_force_login_protection.go index 2ea93979c7a..ca5e0a667ff 100644 --- a/pkg/login/brute_force_login_protection.go +++ b/pkg/login/brute_force_login_protection.go @@ -34,7 +34,7 @@ var validateLoginAttempts = func(username string) error { return nil } -var saveInvalidLoginAttempt = func(query *LoginUserQuery) { +var saveInvalidLoginAttempt = func(query *m.LoginUserQuery) { if setting.DisableBruteForceLoginProtection { return } diff --git a/pkg/login/brute_force_login_protection_test.go b/pkg/login/brute_force_login_protection_test.go index 5375134ba88..aca100760c7 100644 --- a/pkg/login/brute_force_login_protection_test.go +++ b/pkg/login/brute_force_login_protection_test.go @@ -50,7 +50,7 @@ func TestLoginAttemptsValidation(t *testing.T) { return nil }) - saveInvalidLoginAttempt(&LoginUserQuery{ + saveInvalidLoginAttempt(&m.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", @@ -103,7 +103,7 @@ func TestLoginAttemptsValidation(t *testing.T) { return nil }) - saveInvalidLoginAttempt(&LoginUserQuery{ + saveInvalidLoginAttempt(&m.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go new file mode 100644 index 00000000000..5ae00b1b895 --- /dev/null +++ b/pkg/login/ext_user.go @@ -0,0 +1,157 @@ +package login + +import ( + "fmt" + + "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/log" + m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/quota" +) + +func UpsertUser(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + extUser := cmd.ExternalUser + + userQuery := m.GetUserByAuthInfoQuery{ + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + UserId: extUser.UserId, + Email: extUser.Email, + Login: extUser.Login, + } + err := bus.Dispatch(&userQuery) + if err != nil { + if err != m.ErrUserNotFound { + return err + } + + if !cmd.SignupAllowed { + log.Warn(fmt.Sprintf("Not allowing %s login, user not found in internal user database and allow signup = false", extUser.AuthModule)) + return ErrInvalidCredentials + } + + limitReached, err := quota.QuotaReached(ctx, "user") + if err != nil { + log.Warn("Error getting user quota", "err", err) + return ErrGettingUserQuota + } + if limitReached { + return ErrUsersQuotaReached + } + + cmd.User, err = createUser(extUser) + if err != nil { + return err + } + } else { + cmd.User = userQuery.User + + // sync user info + err = updateUser(cmd.User, extUser) + if err != nil { + return err + } + } + + err = syncOrgRoles(cmd.User, extUser) + if err != nil { + return err + } + + return nil +} + +func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { + cmd := m.CreateUserCommand{ + Login: extUser.Login, + Email: extUser.Email, + Name: extUser.Name, + } + if err := bus.Dispatch(&cmd); err != nil { + return nil, err + } + + cmd2 := m.SetAuthInfoCommand{ + UserId: cmd.Result.Id, + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + } + if err := bus.Dispatch(&cmd2); err != nil { + return nil, err + } + + return &cmd.Result, nil +} + +func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { + // sync user info + if user.Login != extUser.Login || user.Email != extUser.Email || user.Name != extUser.Name { + log.Debug("Syncing user info", "id", user.Id, "login", extUser.Login, "email", extUser.Email) + updateCmd := m.UpdateUserCommand{ + UserId: user.Id, + Login: extUser.Login, + Email: extUser.Email, + Name: extUser.Name, + } + err := bus.Dispatch(&updateCmd) + if err != nil { + return err + } + } + + return nil +} + +func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { + if len(extUser.OrgRoles) == 0 { + // log.Warn("No group mappings defined") + return nil + } + + orgsQuery := m.GetUserOrgListQuery{UserId: user.Id} + if err := bus.Dispatch(&orgsQuery); err != nil { + return err + } + + handledOrgIds := map[int64]bool{} + deleteOrgIds := []int64{} + + // update existing org roles + for _, org := range orgsQuery.Result { + handledOrgIds[org.OrgId] = true + + if extUser.OrgRoles[org.OrgId] == "" { + deleteOrgIds = append(deleteOrgIds, org.OrgId) + } else if extUser.OrgRoles[org.OrgId] != org.Role { + // update role + cmd := m.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: extUser.OrgRoles[org.OrgId]} + if err := bus.Dispatch(&cmd); err != nil { + return err + } + } + } + + // add any new org roles + for orgId, orgRole := range extUser.OrgRoles { + if _, exists := handledOrgIds[orgId]; exists { + continue + } + + // add role + cmd := m.AddOrgUserCommand{UserId: user.Id, Role: orgRole, OrgId: orgId} + err := bus.Dispatch(&cmd) + if err != nil && err != m.ErrOrgNotFound { + return err + } + } + + // delete any removed org roles + for _, orgId := range deleteOrgIds { + cmd := m.RemoveOrgUserCommand{OrgId: orgId, UserId: user.Id} + if err := bus.Dispatch(&cmd); err != nil { + return err + } + } + + return nil +} diff --git a/pkg/login/grafana_login.go b/pkg/login/grafana_login.go index 677ba776e4f..e8594fdd190 100644 --- a/pkg/login/grafana_login.go +++ b/pkg/login/grafana_login.go @@ -17,7 +17,7 @@ var validatePassword = func(providedPassword string, userPassword string, userSa return nil } -var loginUsingGrafanaDB = func(query *LoginUserQuery) error { +var loginUsingGrafanaDB = func(query *m.LoginUserQuery) error { userQuery := m.GetUserByLoginQuery{LoginOrEmail: query.Username} if err := bus.Dispatch(&userQuery); err != nil { diff --git a/pkg/login/grafana_login_test.go b/pkg/login/grafana_login_test.go index 88e52224113..90422678fd2 100644 --- a/pkg/login/grafana_login_test.go +++ b/pkg/login/grafana_login_test.go @@ -66,7 +66,7 @@ func TestGrafanaLogin(t *testing.T) { } type grafanaLoginScenarioContext struct { - loginUserQuery *LoginUserQuery + loginUserQuery *m.LoginUserQuery validatePasswordCalled bool } @@ -77,7 +77,7 @@ func grafanaLoginScenario(desc string, fn grafanaLoginScenarioFunc) { origValidatePassword := validatePassword sc := &grafanaLoginScenarioContext{ - loginUserQuery: &LoginUserQuery{ + loginUserQuery: &m.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index be3babac02e..fe3a88f3565 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -10,7 +10,6 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/go-ldap/ldap" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" @@ -24,10 +23,9 @@ type ILdapConn interface { } type ILdapAuther interface { - Login(query *LoginUserQuery) error - SyncSignedInUser(signedInUser *m.SignedInUser) error - GetGrafanaUserFor(ldapUser *LdapUserInfo) (*m.User, error) - SyncOrgRoles(user *m.User, ldapUser *LdapUserInfo) error + Login(ctx *m.ReqContext, query *m.LoginUserQuery) error + SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error + GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) } type ldapAuther struct { @@ -89,89 +87,36 @@ func (a *ldapAuther) Dial() error { return err } -func (a *ldapAuther) Login(query *LoginUserQuery) error { - if err := a.Dial(); err != nil { +func (a *ldapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { + // connect to ldap server + err := a.Dial() + if err != nil { return err } defer a.conn.Close() // perform initial authentication - if err := a.initialBind(query.Username, query.Password); err != nil { + err = a.initialBind(query.Username, query.Password) + if err != nil { return err } // find user entry & attributes - if ldapUser, err := a.searchForUser(query.Username); err != nil { - return err - } else { - a.log.Debug("Ldap User found", "info", spew.Sdump(ldapUser)) - - // check if a second user bind is needed - if a.requireSecondBind { - if err := a.secondBind(ldapUser, query.Password); err != nil { - return err - } - } - - if grafanaUser, err := a.GetGrafanaUserFor(ldapUser); err != nil { - return err - } else { - if syncErr := a.syncInfoAndOrgRoles(grafanaUser, ldapUser); syncErr != nil { - return syncErr - } - query.User = grafanaUser - return nil - } - } -} - -func (a *ldapAuther) SyncSignedInUser(signedInUser *m.SignedInUser) error { - grafanaUser := m.User{ - Id: signedInUser.UserId, - Login: signedInUser.Login, - Email: signedInUser.Email, - Name: signedInUser.Name, - } - - if err := a.Dial(); err != nil { + ldapUser, err := a.searchForUser(query.Username) + if err != nil { return err } - defer a.conn.Close() - if err := a.serverBind(); err != nil { - return err - } + a.log.Debug("Ldap User found", "info", spew.Sdump(ldapUser)) - if ldapUser, err := a.searchForUser(signedInUser.Login); err != nil { - a.log.Error("Failed searching for user in ldap", "error", err) - - return err - } else { - if err := a.syncInfoAndOrgRoles(&grafanaUser, ldapUser); err != nil { + // check if a second user bind is needed + if a.requireSecondBind { + err = a.secondBind(ldapUser, query.Password) + if err != nil { return err } - - a.log.Debug("Got Ldap User Info", "user", spew.Sdump(ldapUser)) } - return nil -} - -// Sync info for ldap user and grafana user -func (a *ldapAuther) syncInfoAndOrgRoles(user *m.User, ldapUser *LdapUserInfo) error { - // sync user details - if err := a.syncUserInfo(user, ldapUser); err != nil { - return err - } - // sync org roles - if err := a.SyncOrgRoles(user, ldapUser); err != nil { - return err - } - - return nil -} - -func (a *ldapAuther) GetGrafanaUserFor(ldapUser *LdapUserInfo) (*m.User, error) { // validate that the user has access // if there are no ldap group mappings access is true // otherwise a single group must match @@ -184,123 +129,85 @@ func (a *ldapAuther) GetGrafanaUserFor(ldapUser *LdapUserInfo) (*m.User, error) } if !access { - a.log.Info("Ldap Auth: user does not belong in any of the specified ldap groups", "username", ldapUser.Username, "groups", ldapUser.MemberOf) - return nil, ErrInvalidCredentials + a.log.Info( + "Ldap Auth: user does not belong in any of the specified ldap groups", + "username", ldapUser.Username, + "groups", ldapUser.MemberOf) + return ErrInvalidCredentials } - // get user from grafana db - userQuery := m.GetUserByLoginQuery{LoginOrEmail: ldapUser.Username} - if err := bus.Dispatch(&userQuery); err != nil { - if err == m.ErrUserNotFound && setting.LdapAllowSignup { - return a.createGrafanaUser(ldapUser) - } else if err == m.ErrUserNotFound { - a.log.Warn("Not allowing LDAP login, user not found in internal user database, and ldap allow signup = false") - return nil, ErrInvalidCredentials - } else { - return nil, err - } + grafanaUser, err := a.GetGrafanaUserFor(ctx, ldapUser) + if err != nil { + return err } - return userQuery.Result, nil - + query.User = grafanaUser + return nil } -func (a *ldapAuther) createGrafanaUser(ldapUser *LdapUserInfo) (*m.User, error) { - cmd := m.CreateUserCommand{ - Login: ldapUser.Username, - Email: ldapUser.Email, - Name: fmt.Sprintf("%s %s", ldapUser.FirstName, ldapUser.LastName), - } - if err := bus.Dispatch(&cmd); err != nil { - return nil, err +func (a *ldapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { + err := a.Dial() + if err != nil { + return err } - return &cmd.Result, nil -} + defer a.conn.Close() -func (a *ldapAuther) syncUserInfo(user *m.User, ldapUser *LdapUserInfo) error { - var name = fmt.Sprintf("%s %s", ldapUser.FirstName, ldapUser.LastName) - if user.Email == ldapUser.Email && user.Name == name { - return nil + err = a.serverBind() + if err != nil { + return err } - a.log.Debug("Syncing user info", "username", ldapUser.Username) - updateCmd := m.UpdateUserCommand{} - updateCmd.UserId = user.Id - updateCmd.Login = user.Login - updateCmd.Email = ldapUser.Email - updateCmd.Name = fmt.Sprintf("%s %s", ldapUser.FirstName, ldapUser.LastName) - return bus.Dispatch(&updateCmd) -} - -func (a *ldapAuther) SyncOrgRoles(user *m.User, ldapUser *LdapUserInfo) error { - if len(a.server.LdapGroups) == 0 { - a.log.Warn("No group mappings defined") - return nil + ldapUser, err := a.searchForUser(signedInUser.Login) + if err != nil { + a.log.Error("Failed searching for user in ldap", "error", err) + return err } - orgsQuery := m.GetUserOrgListQuery{UserId: user.Id} - if err := bus.Dispatch(&orgsQuery); err != nil { + grafanaUser, err := a.GetGrafanaUserFor(ctx, ldapUser) + if err != nil { return err } - handledOrgIds := map[int64]bool{} + signedInUser.Login = grafanaUser.Login + signedInUser.Email = grafanaUser.Email + signedInUser.Name = grafanaUser.Name - // update or remove org roles - for _, org := range orgsQuery.Result { - match := false - handledOrgIds[org.OrgId] = true - - for _, group := range a.server.LdapGroups { - if org.OrgId != group.OrgId { - continue - } - - if ldapUser.isMemberOf(group.GroupDN) { - match = true - if org.Role != group.OrgRole { - // update role - cmd := m.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: group.OrgRole} - if err := bus.Dispatch(&cmd); err != nil { - return err - } - } - // ignore subsequent ldap group mapping matches - break - } - } + return nil +} - // remove role if no mappings match - if !match { - cmd := m.RemoveOrgUserCommand{OrgId: org.OrgId, UserId: user.Id} - if err := bus.Dispatch(&cmd); err != nil { - return err - } - } +func (a *ldapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) { + extUser := m.ExternalUserInfo{ + AuthModule: "ldap", + AuthId: ldapUser.DN, + Name: fmt.Sprintf("%s %s", ldapUser.FirstName, ldapUser.LastName), + Login: ldapUser.Username, + Email: ldapUser.Email, + OrgRoles: map[int64]m.RoleType{}, } - // add missing org roles for _, group := range a.server.LdapGroups { - if !ldapUser.isMemberOf(group.GroupDN) { + // only use the first match for each org + if extUser.OrgRoles[group.OrgId] != "" { continue } - if _, exists := handledOrgIds[group.OrgId]; exists { - continue - } - - // add role - cmd := m.AddOrgUserCommand{UserId: user.Id, Role: group.OrgRole, OrgId: group.OrgId} - err := bus.Dispatch(&cmd) - if err != nil && err != m.ErrOrgNotFound { - return err + if ldapUser.isMemberOf(group.GroupDN) { + extUser.OrgRoles[group.OrgId] = group.OrgRole } + } - // mark this group has handled so we do not process it again - handledOrgIds[group.OrgId] = true + // add/update user in grafana + userQuery := m.UpsertUserCommand{ + ExternalUser: &extUser, + SignupAllowed: setting.LdapAllowSignup, + } + err := UpsertUser(ctx, &userQuery) + if err != nil { + return nil, err } - return nil + return userQuery.User, nil } func (a *ldapAuther) serverBind() error { @@ -470,7 +377,3 @@ func getLdapAttrArray(name string, result *ldap.SearchResult) []string { } return []string{} } - -func createUserFromLdapInfo() error { - return nil -} diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index b74b69db036..03f6390a991 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -1,17 +1,18 @@ package login import ( + m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" ) -var loginUsingLdap = func(query *LoginUserQuery) (bool, error) { +var loginUsingLdap = func(ctx *m.ReqContext, query *m.LoginUserQuery) (bool, error) { if !setting.LdapEnabled { return false, nil } for _, server := range LdapCfg.Servers { author := NewLdapAuthenticator(server) - err := author.Login(query) + err := author.Login(ctx, query) if err == nil || err != ErrInvalidCredentials { return true, err } diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index 6af125566e8..43cf216d366 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -79,7 +79,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - enabled, err := loginUsingLdap(&LoginUserQuery{ + enabled, err := loginUsingLdap(&m.LoginUserQuery{ Username: "user", Password: "pwd", }) @@ -117,7 +117,7 @@ type mockLdapAuther struct { loginCalled bool } -func (a *mockLdapAuther) Login(query *LoginUserQuery) error { +func (a *mockLdapAuther) Login(query *m.LoginUserQuery) error { a.loginCalled = true if !a.validLogin { @@ -140,7 +140,7 @@ func (a *mockLdapAuther) SyncOrgRoles(user *m.User, ldapUser *LdapUserInfo) erro } type ldapLoginScenarioContext struct { - loginUserQuery *LoginUserQuery + loginUserQuery *m.LoginUserQuery ldapAuthenticatorMock *mockLdapAuther } @@ -151,7 +151,7 @@ func ldapLoginScenario(desc string, fn ldapLoginScenarioFunc) { origNewLdapAuthenticator := NewLdapAuthenticator sc := &ldapLoginScenarioContext{ - loginUserQuery: &LoginUserQuery{ + loginUserQuery: &m.LoginUserQuery{ Username: "user", Password: "pwd", IpAddress: "192.168.1.1:56433", diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index adf0b7b53d5..10a3e549112 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -112,7 +112,7 @@ var syncGrafanaUserWithLdapUser = func(ctx *m.ReqContext, query *m.GetSignedInUs for _, server := range ldapCfg.Servers { author := login.NewLdapAuthenticator(server) - if err := author.SyncSignedInUser(query.Result); err != nil { + if err := author.SyncSignedInUser(ctx, query.Result); err != nil { return err } } diff --git a/pkg/middleware/auth_proxy_test.go b/pkg/middleware/auth_proxy_test.go index b3c011bd870..cd5b666e113 100644 --- a/pkg/middleware/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy_test.go @@ -116,7 +116,7 @@ type mockLdapAuthenticator struct { syncSignedInUserCalled bool } -func (a *mockLdapAuthenticator) Login(query *login.LoginUserQuery) error { +func (a *mockLdapAuthenticator) Login(query *m.LoginUserQuery) error { return nil } diff --git a/pkg/middleware/middleware.go b/pkg/middleware/middleware.go index b5b244d5bff..93db49ed880 100644 --- a/pkg/middleware/middleware.go +++ b/pkg/middleware/middleware.go @@ -8,7 +8,6 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/apikeygen" "github.com/grafana/grafana/pkg/log" - l "github.com/grafana/grafana/pkg/login" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" @@ -165,7 +164,7 @@ func initContextWithBasicAuth(ctx *m.ReqContext, orgId int64) bool { user := loginQuery.Result - loginUserQuery := l.LoginUserQuery{Username: username, Password: password, User: user} + loginUserQuery := m.LoginUserQuery{Username: username, Password: password, User: user} if err := bus.Dispatch(&loginUserQuery); err != nil { ctx.JsonApiErr(401, "Invalid username or password", err) return true diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index f80a30de02f..09c38bed644 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -9,7 +9,6 @@ import ( ms "github.com/go-macaron/session" "github.com/grafana/grafana/pkg/bus" - l "github.com/grafana/grafana/pkg/login" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" @@ -72,7 +71,7 @@ func TestMiddlewareContext(t *testing.T) { return nil }) - bus.AddHandler("test", func(loginUserQuery *l.LoginUserQuery) error { + bus.AddHandler("test", func(loginUserQuery *m.LoginUserQuery) error { return nil }) diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go new file mode 100644 index 00000000000..ef880806963 --- /dev/null +++ b/pkg/models/user_auth.go @@ -0,0 +1,66 @@ +package models + +type UserAuth struct { + Id int64 + UserId int64 + AuthModule string + AuthId string +} + +type ExternalUserInfo struct { + AuthModule string + AuthId string + UserId int64 + Email string + Login string + Name string + OrgRoles map[int64]RoleType +} + +// --------------------- +// COMMANDS + +type UpsertUserCommand struct { + ExternalUser *ExternalUserInfo + SignupAllowed bool + + User *User +} + +type SetAuthInfoCommand struct { + AuthModule string + AuthId string + UserId int64 +} + +type DeleteAuthInfoCommand struct { + UserAuth *UserAuth +} + +// ---------------------- +// QUERIES + +type LoginUserQuery struct { + Username string + Password string + User *User + IpAddress string +} + +type GetUserByAuthInfoQuery struct { + AuthModule string + AuthId string + UserId int64 + Email string + Login string + + User *User + UserAuth *UserAuth +} + +type GetAuthInfoQuery struct { + AuthModule string + AuthId string + + UserAuth *UserAuth +} diff --git a/pkg/services/sqlstore/migrations/migrations.go b/pkg/services/sqlstore/migrations/migrations.go index 282f98e7318..58ac6256f41 100644 --- a/pkg/services/sqlstore/migrations/migrations.go +++ b/pkg/services/sqlstore/migrations/migrations.go @@ -30,6 +30,7 @@ func AddMigrations(mg *Migrator) { addDashboardAclMigrations(mg) addTagMigration(mg) addLoginAttemptMigrations(mg) + addUserAuthMigrations(mg) } func addMigrationLogMigrations(mg *Migrator) { diff --git a/pkg/services/sqlstore/migrations/user_auth_mig.go b/pkg/services/sqlstore/migrations/user_auth_mig.go new file mode 100644 index 00000000000..026d41a4830 --- /dev/null +++ b/pkg/services/sqlstore/migrations/user_auth_mig.go @@ -0,0 +1,24 @@ +package migrations + +import . "github.com/grafana/grafana/pkg/services/sqlstore/migrator" + +func addUserAuthMigrations(mg *Migrator) { + userAuthV1 := Table{ + Name: "user_auth", + Columns: []*Column{ + {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, + {Name: "user_id", Type: DB_BigInt, Nullable: false}, + {Name: "auth_module", Type: DB_NVarchar, Length: 30, Nullable: false}, + {Name: "auth_id", Type: DB_NVarchar, Length: 100, Nullable: false}, + {Name: "created", Type: DB_DateTime, Nullable: false}, + }, + Indices: []*Index{ + {Cols: []string{"auth_module", "auth_id"}}, + }, + } + + // create table + mg.AddMigration("create user auth table", NewAddTableMigration(userAuthV1)) + // add indices + addTableIndicesMigrations(mg, "v1", userAuthV1) +} diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index db7e851435c..996cc47fc1c 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -445,6 +445,7 @@ func DeleteUser(cmd *m.DeleteUserCommand) error { "DELETE FROM dashboard_acl WHERE user_id = ?", "DELETE FROM preferences WHERE user_id = ?", "DELETE FROM team_member WHERE user_id = ?", + "DELETE FROM user_auth WHERE user_id = ?", } for _, sql := range deletes { diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go new file mode 100644 index 00000000000..abd5152d59f --- /dev/null +++ b/pkg/services/sqlstore/user_auth.go @@ -0,0 +1,130 @@ +package sqlstore + +import ( + "github.com/grafana/grafana/pkg/bus" + m "github.com/grafana/grafana/pkg/models" +) + +func init() { + bus.AddHandler("sql", GetUserByAuthInfo) + bus.AddHandler("sql", GetAuthInfo) + bus.AddHandler("sql", SetAuthInfo) + bus.AddHandler("sql", DeleteAuthInfo) +} + +func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { + user := new(m.User) + has := false + var err error + + // Try to find the user by auth module and id first + if query.AuthModule != "" && query.AuthId != "" { + authQuery := &m.GetAuthInfoQuery{ + AuthModule: query.AuthModule, + AuthId: query.AuthId, + } + + err = GetAuthInfo(authQuery) + // if user id was specified and doesn't match the user_auth entry, remove it + if err == nil && query.UserId != 0 && query.UserId != authQuery.UserAuth.UserId { + DeleteAuthInfo(&m.DeleteAuthInfoCommand{ + UserAuth: authQuery.UserAuth, + }) + } else if err == nil { + has, err = x.Id(authQuery.UserAuth.UserId).Get(user) + if err != nil { + return err + } + + if has { + query.UserAuth = authQuery.UserAuth + } else { + // if the user has been deleted then remove the entry + DeleteAuthInfo(&m.DeleteAuthInfoCommand{ + UserAuth: authQuery.UserAuth, + }) + } + } else if err != m.ErrUserNotFound { + return err + } + } + + // If not found, try to find the user by id + if !has && query.UserId != 0 { + has, err = x.Id(query.UserId).Get(user) + if err != nil { + return err + } + } + + // If not found, try to find the user by email address + if !has && query.Email != "" { + user = &m.User{Email: query.Email} + has, err = x.Get(user) + if err != nil { + return err + } + } + + // If not found, try to find the user by login + if !has && query.Login != "" { + user = &m.User{Login: query.Login} + has, err = x.Get(user) + if err != nil { + return err + } + } + + // No user found + if !has { + return m.ErrUserNotFound + } + + query.User = user + return nil +} + +func GetAuthInfo(query *m.GetAuthInfoQuery) error { + userAuth := &m.UserAuth{ + AuthModule: query.AuthModule, + AuthId: query.AuthId, + } + has, err := x.Get(userAuth) + if err != nil { + return err + } + if !has { + return m.ErrUserNotFound + } + + query.UserAuth = userAuth + return nil +} + +func SetAuthInfo(cmd *m.SetAuthInfoCommand) error { + return inTransaction(func(sess *DBSession) error { + authUser := m.UserAuth{ + UserId: cmd.UserId, + AuthModule: cmd.AuthModule, + AuthId: cmd.AuthId, + } + + _, err := sess.Insert(&authUser) + if err != nil { + return err + } + + return nil + }) +} + +func DeleteAuthInfo(cmd *m.DeleteAuthInfoCommand) error { + return inTransaction(func(sess *DBSession) error { + _, err := sess.Delete(cmd.UserAuth) + if err != nil { + return err + } + + return nil + }) +} diff --git a/pkg/social/grafana_com_oauth.go b/pkg/social/grafana_com_oauth.go index d3614520d61..87601788c3f 100644 --- a/pkg/social/grafana_com_oauth.go +++ b/pkg/social/grafana_com_oauth.go @@ -51,6 +51,7 @@ func (s *SocialGrafanaCom) IsOrganizationMember(organizations []OrgRecord) bool func (s *SocialGrafanaCom) UserInfo(client *http.Client, token *oauth2.Token) (*BasicUserInfo, error) { var data struct { + Id int `json:"id"` Name string `json:"name"` Login string `json:"username"` Email string `json:"email"` @@ -69,6 +70,7 @@ func (s *SocialGrafanaCom) UserInfo(client *http.Client, token *oauth2.Token) (* } userInfo := &BasicUserInfo{ + Id: fmt.Sprintf("%d", data.Id), Name: data.Name, Login: data.Login, Email: data.Email, diff --git a/pkg/social/social.go b/pkg/social/social.go index b763e2d71b2..8f0618b7f74 100644 --- a/pkg/social/social.go +++ b/pkg/social/social.go @@ -14,6 +14,7 @@ import ( ) type BasicUserInfo struct { + Id string Name string Email string Login string From d2eab2ff4c42283b57f06126d51939046920bdd3 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 19 Mar 2018 14:08:55 -0400 Subject: [PATCH 02/20] fix tests --- pkg/login/auth_test.go | 18 +-- pkg/login/ldap.go | 30 ++--- pkg/login/ldap_login_test.go | 18 ++- pkg/login/ldap_test.go | 175 +++++++----------------------- pkg/middleware/auth_proxy_test.go | 9 +- 5 files changed, 72 insertions(+), 178 deletions(-) diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index 932125c410e..2853b1fd15d 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -16,7 +16,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrTooManyLoginAttempts) @@ -33,7 +33,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, nil) @@ -51,7 +51,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, customErr) @@ -68,7 +68,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(false, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -85,7 +85,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -102,7 +102,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldBeNil) @@ -120,7 +120,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, customErr, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, customErr) @@ -137,7 +137,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(sc.loginUserQuery) + err := AuthenticateUser(nil, sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -168,7 +168,7 @@ func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) { } func mockLoginUsingLdap(enabled bool, err error, sc *authScenarioContext) { - loginUsingLdap = func(query *m.LoginUserQuery) (bool, error) { + loginUsingLdap = func(c *m.ReqContext, query *m.LoginUserQuery) (bool, error) { sc.ldapLoginWasCalled = true return enabled, err } diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index fe3a88f3565..fd98320008e 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -117,25 +117,6 @@ func (a *ldapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { } } - // validate that the user has access - // if there are no ldap group mappings access is true - // otherwise a single group must match - access := len(a.server.LdapGroups) == 0 - for _, ldapGroup := range a.server.LdapGroups { - if ldapUser.isMemberOf(ldapGroup.GroupDN) { - access = true - break - } - } - - if !access { - a.log.Info( - "Ldap Auth: user does not belong in any of the specified ldap groups", - "username", ldapUser.Username, - "groups", ldapUser.MemberOf) - return ErrInvalidCredentials - } - grafanaUser, err := a.GetGrafanaUserFor(ctx, ldapUser) if err != nil { return err @@ -197,6 +178,17 @@ func (a *ldapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo } } + // validate that the user has access + // if there are no ldap group mappings access is true + // otherwise a single group must match + if len(a.server.LdapGroups) > 0 && len(extUser.OrgRoles) < 1 { + a.log.Info( + "Ldap Auth: user does not belong in any of the specified ldap groups", + "username", ldapUser.Username, + "groups", ldapUser.MemberOf) + return nil, ErrInvalidCredentials + } + // add/update user in grafana userQuery := m.UpsertUserCommand{ ExternalUser: &extUser, diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index 43cf216d366..b6d1f619324 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -19,7 +19,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login with invalid credentials", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - enabled, err := loginUsingLdap(sc.loginUserQuery) + enabled, err := loginUsingLdap(nil, sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -36,7 +36,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login with valid credentials", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(true) - enabled, err := loginUsingLdap(sc.loginUserQuery) + enabled, err := loginUsingLdap(nil, sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -58,7 +58,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(true) - enabled, err := loginUsingLdap(sc.loginUserQuery) + enabled, err := loginUsingLdap(nil, sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -79,7 +79,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - enabled, err := loginUsingLdap(&m.LoginUserQuery{ + enabled, err := loginUsingLdap(nil, &m.LoginUserQuery{ Username: "user", Password: "pwd", }) @@ -117,7 +117,7 @@ type mockLdapAuther struct { loginCalled bool } -func (a *mockLdapAuther) Login(query *m.LoginUserQuery) error { +func (a *mockLdapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { a.loginCalled = true if !a.validLogin { @@ -127,18 +127,14 @@ func (a *mockLdapAuther) Login(query *m.LoginUserQuery) error { return nil } -func (a *mockLdapAuther) SyncSignedInUser(signedInUser *m.SignedInUser) error { +func (a *mockLdapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { return nil } -func (a *mockLdapAuther) GetGrafanaUserFor(ldapUser *LdapUserInfo) (*m.User, error) { +func (a *mockLdapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) { return nil, nil } -func (a *mockLdapAuther) SyncOrgRoles(user *m.User, ldapUser *LdapUserInfo) error { - return nil -} - type ldapLoginScenarioContext struct { loginUserQuery *m.LoginUserQuery ldapAuthenticatorMock *mockLdapAuther diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 8677bbeae42..43d37a621d5 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -18,7 +18,7 @@ func TestLdapAuther(t *testing.T) { ldapAuther := NewLdapAuthenticator(&LdapServerConf{ LdapGroups: []*LdapGroupToOrgRole{{}}, }) - _, err := ldapAuther.GetGrafanaUserFor(&LdapUserInfo{}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{}) So(err, ShouldEqual, ErrInvalidCredentials) }) @@ -34,7 +34,7 @@ func TestLdapAuther(t *testing.T) { sc.userQueryReturns(user1) - result, err := ldapAuther.GetGrafanaUserFor(&LdapUserInfo{}) + result, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{}) So(err, ShouldBeNil) So(result, ShouldEqual, user1) }) @@ -48,7 +48,7 @@ func TestLdapAuther(t *testing.T) { sc.userQueryReturns(user1) - result, err := ldapAuther.GetGrafanaUserFor(&LdapUserInfo{MemberOf: []string{"cn=users"}}) + result, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{MemberOf: []string{"cn=users"}}) So(err, ShouldBeNil) So(result, ShouldEqual, user1) }) @@ -64,7 +64,7 @@ func TestLdapAuther(t *testing.T) { sc.userQueryReturns(nil) - result, err := ldapAuther.GetGrafanaUserFor(&LdapUserInfo{ + result, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ Username: "torkelo", Email: "my@email.com", MemberOf: []string{"cn=editor"}, @@ -72,133 +72,20 @@ func TestLdapAuther(t *testing.T) { So(err, ShouldBeNil) - Convey("Should create new user", func() { - So(sc.createUserCmd.Login, ShouldEqual, "torkelo") - So(sc.createUserCmd.Email, ShouldEqual, "my@email.com") - }) - Convey("Should return new user", func() { So(result.Login, ShouldEqual, "torkelo") }) - }) - - }) - - Convey("When syncing ldap groups to grafana org roles", t, func() { - - ldapAutherScenario("given no current user orgs", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=users", OrgRole: "Admin"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=users"}, - }) - - Convey("Should create new org user", func() { - So(err, ShouldBeNil) - So(sc.addOrgUserCmd, ShouldNotBeNil) - So(sc.addOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) - }) - }) - - ldapAutherScenario("given different current org role", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=users", OrgId: 1, OrgRole: "Admin"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_EDITOR}}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=users"}, - }) + /* + Convey("Should create new user", func() { + So(sc.getUserByAuthInfoQuery.Login, ShouldEqual, "torkelo") + So(sc.getUserByAuthInfoQuery.Email, ShouldEqual, "my@email.com") - Convey("Should update org role", func() { - So(err, ShouldBeNil) - So(sc.updateOrgUserCmd, ShouldNotBeNil) - So(sc.updateOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) - }) - }) + So(sc.createUserCmd.Login, ShouldEqual, "torkelo") + So(sc.createUserCmd.Email, ShouldEqual, "my@email.com") + }) + */ - ldapAutherScenario("given current org role is removed in ldap", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=users", OrgId: 1, OrgRole: "Admin"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_EDITOR}}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=other"}, - }) - - Convey("Should remove org role", func() { - So(err, ShouldBeNil) - So(sc.removeOrgUserCmd, ShouldNotBeNil) - }) - }) - - ldapAutherScenario("given org role is updated in config", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=admin", OrgId: 1, OrgRole: "Admin"}, - {GroupDN: "cn=users", OrgId: 1, OrgRole: "Viewer"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_EDITOR}}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=users"}, - }) - - Convey("Should update org role", func() { - So(err, ShouldBeNil) - So(sc.removeOrgUserCmd, ShouldBeNil) - So(sc.updateOrgUserCmd, ShouldNotBeNil) - }) - }) - - ldapAutherScenario("given multiple matching ldap groups", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=admins", OrgId: 1, OrgRole: "Admin"}, - {GroupDN: "*", OrgId: 1, OrgRole: "Viewer"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_ADMIN}}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=admins"}, - }) - - Convey("Should take first match, and ignore subsequent matches", func() { - So(err, ShouldBeNil) - So(sc.updateOrgUserCmd, ShouldBeNil) - }) - }) - - ldapAutherScenario("given multiple matching ldap groups and no existing groups", func(sc *scenarioContext) { - ldapAuther := NewLdapAuthenticator(&LdapServerConf{ - LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=admins", OrgId: 1, OrgRole: "Admin"}, - {GroupDN: "*", OrgId: 1, OrgRole: "Viewer"}, - }, - }) - - sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) - err := ldapAuther.SyncOrgRoles(&m.User{}, &LdapUserInfo{ - MemberOf: []string{"cn=admins"}, - }) - - Convey("Should take first match, and ignore subsequent matches", func() { - So(err, ShouldBeNil) - So(sc.addOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) - }) }) }) @@ -250,10 +137,16 @@ func TestLdapAuther(t *testing.T) { Login: "roelgerrits", } + sc.userQueryReturns(&m.User{ + Id: 1, + Email: "roel@test.net", + Name: "Roel Gerrits", + Login: "roelgerrits", + }) sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) // act - syncErrResult := ldapAuther.SyncSignedInUser(signedInUser) + syncErrResult := ldapAuther.SyncSignedInUser(nil, signedInUser) // assert So(dialCalled, ShouldBeTrue) @@ -299,6 +192,17 @@ func ldapAutherScenario(desc string, fn scenarioFunc) { sc := &scenarioContext{} + bus.AddHandler("test", func(cmd *m.GetUserByAuthInfoQuery) error { + sc.getUserByAuthInfoQuery = cmd + sc.getUserByAuthInfoQuery.User = &m.User{Login: cmd.Login} + return nil + }) + + bus.AddHandler("test", func(cmd *m.GetUserOrgListQuery) error { + sc.getUserOrgListQuery = cmd + return nil + }) + bus.AddHandler("test", func(cmd *m.CreateUserCommand) error { sc.createUserCmd = cmd sc.createUserCmd.Result = m.User{Login: cmd.Login} @@ -330,22 +234,27 @@ func ldapAutherScenario(desc string, fn scenarioFunc) { } type scenarioContext struct { - createUserCmd *m.CreateUserCommand - addOrgUserCmd *m.AddOrgUserCommand - updateOrgUserCmd *m.UpdateOrgUserCommand - removeOrgUserCmd *m.RemoveOrgUserCommand - updateUserCmd *m.UpdateUserCommand + getUserByAuthInfoQuery *m.GetUserByAuthInfoQuery + getUserOrgListQuery *m.GetUserOrgListQuery + createUserCmd *m.CreateUserCommand + addOrgUserCmd *m.AddOrgUserCommand + updateOrgUserCmd *m.UpdateOrgUserCommand + removeOrgUserCmd *m.RemoveOrgUserCommand + updateUserCmd *m.UpdateUserCommand } func (sc *scenarioContext) userQueryReturns(user *m.User) { - bus.AddHandler("test", func(query *m.GetUserByLoginQuery) error { + bus.AddHandler("test", func(query *m.GetUserByAuthInfoQuery) error { if user == nil { return m.ErrUserNotFound } else { - query.Result = user + query.User = user return nil } }) + bus.AddHandler("test", func(query *m.SetAuthInfoCommand) error { + return nil + }) } func (sc *scenarioContext) userOrgsQueryReturns(orgs []*m.UserOrgDTO) { diff --git a/pkg/middleware/auth_proxy_test.go b/pkg/middleware/auth_proxy_test.go index cd5b666e113..8fbaf7f2de8 100644 --- a/pkg/middleware/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy_test.go @@ -116,18 +116,15 @@ type mockLdapAuthenticator struct { syncSignedInUserCalled bool } -func (a *mockLdapAuthenticator) Login(query *m.LoginUserQuery) error { +func (a *mockLdapAuthenticator) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { return nil } -func (a *mockLdapAuthenticator) SyncSignedInUser(signedInUser *m.SignedInUser) error { +func (a *mockLdapAuthenticator) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { a.syncSignedInUserCalled = true return nil } -func (a *mockLdapAuthenticator) GetGrafanaUserFor(ldapUser *login.LdapUserInfo) (*m.User, error) { +func (a *mockLdapAuthenticator) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *login.LdapUserInfo) (*m.User, error) { return nil, nil } -func (a *mockLdapAuthenticator) SyncOrgRoles(user *m.User, ldapUser *login.LdapUserInfo) error { - return nil -} From 23f163e8cfec76d77bebd84d5f5e1841d6907c67 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 19 Mar 2018 17:09:49 -0400 Subject: [PATCH 03/20] fixes --- pkg/login/ext_user.go | 20 ++++++++++--------- pkg/models/user_auth.go | 5 +++++ .../sqlstore/migrations/user_auth_mig.go | 2 +- pkg/services/sqlstore/user_auth.go | 3 +++ 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 5ae00b1b895..02f70b3e00b 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -53,6 +53,17 @@ func UpsertUser(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { } } + if userQuery.UserAuth == nil && extUser.AuthModule != "" && extUser.AuthId != "" { + cmd2 := m.SetAuthInfoCommand{ + UserId: cmd.User.Id, + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + } + if err := bus.Dispatch(&cmd2); err != nil { + return err + } + } + err = syncOrgRoles(cmd.User, extUser) if err != nil { return err @@ -71,15 +82,6 @@ func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { return nil, err } - cmd2 := m.SetAuthInfoCommand{ - UserId: cmd.Result.Id, - AuthModule: extUser.AuthModule, - AuthId: extUser.AuthId, - } - if err := bus.Dispatch(&cmd2); err != nil { - return nil, err - } - return &cmd.Result, nil } diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index ef880806963..99c1a70c153 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -1,10 +1,15 @@ package models +import ( + "time" +) + type UserAuth struct { Id int64 UserId int64 AuthModule string AuthId string + Created time.Time } type ExternalUserInfo struct { diff --git a/pkg/services/sqlstore/migrations/user_auth_mig.go b/pkg/services/sqlstore/migrations/user_auth_mig.go index 026d41a4830..4d8a18ce33e 100644 --- a/pkg/services/sqlstore/migrations/user_auth_mig.go +++ b/pkg/services/sqlstore/migrations/user_auth_mig.go @@ -8,7 +8,7 @@ func addUserAuthMigrations(mg *Migrator) { Columns: []*Column{ {Name: "id", Type: DB_BigInt, IsPrimaryKey: true, IsAutoIncrement: true}, {Name: "user_id", Type: DB_BigInt, Nullable: false}, - {Name: "auth_module", Type: DB_NVarchar, Length: 30, Nullable: false}, + {Name: "auth_module", Type: DB_NVarchar, Length: 190, Nullable: false}, {Name: "auth_id", Type: DB_NVarchar, Length: 100, Nullable: false}, {Name: "created", Type: DB_DateTime, Nullable: false}, }, diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index abd5152d59f..071f003d6ad 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -1,6 +1,8 @@ package sqlstore import ( + "time" + "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" ) @@ -107,6 +109,7 @@ func SetAuthInfo(cmd *m.SetAuthInfoCommand) error { UserId: cmd.UserId, AuthModule: cmd.AuthModule, AuthId: cmd.AuthId, + Created: time.Now(), } _, err := sess.Insert(&authUser) From d5dd1c9bca020ba1913e313ac24a55cb034fb216 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Thu, 22 Mar 2018 17:02:34 -0400 Subject: [PATCH 04/20] update auth proxy --- pkg/login/ext_user.go | 30 +++++--- pkg/middleware/auth_proxy.go | 113 +++++++++++++++--------------- pkg/middleware/middleware_test.go | 17 ++++- 3 files changed, 92 insertions(+), 68 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 02f70b3e00b..82ebcf481b9 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -9,7 +9,7 @@ import ( "github.com/grafana/grafana/pkg/services/quota" ) -func UpsertUser(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { +var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { extUser := cmd.ExternalUser userQuery := m.GetUserByAuthInfoQuery{ @@ -87,14 +87,26 @@ func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { // sync user info - if user.Login != extUser.Login || user.Email != extUser.Email || user.Name != extUser.Name { - log.Debug("Syncing user info", "id", user.Id, "login", extUser.Login, "email", extUser.Email) - updateCmd := m.UpdateUserCommand{ - UserId: user.Id, - Login: extUser.Login, - Email: extUser.Email, - Name: extUser.Name, - } + updateCmd := m.UpdateUserCommand{ + UserId: user.Id, + } + needsUpdate := false + + if extUser.Login != "" && extUser.Login != user.Login { + updateCmd.Login = extUser.Login + needsUpdate = true + } + if extUser.Email != "" && extUser.Email != user.Email { + updateCmd.Email = extUser.Email + needsUpdate = true + } + if extUser.Name != "" && extUser.Name != user.Name { + updateCmd.Name = extUser.Name + needsUpdate = true + } + + if needsUpdate { + log.Debug("Syncing user info", "id", user.Id, "update", updateCmd) err := bus.Dispatch(&updateCmd) if err != nil { return err diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index 10a3e549112..424d0719f50 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -3,6 +3,7 @@ package middleware import ( "fmt" "net" + "net/mail" "strings" "time" @@ -14,6 +15,8 @@ import ( "github.com/grafana/grafana/pkg/setting" ) +var AUTH_PROXY_SESSION_VAR = "authProxyHeaderValue" + func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { if !setting.AuthProxyEnabled { return false @@ -30,51 +33,75 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { return true } - query := getSignedInUserQueryForProxyAuth(proxyHeaderValue) - query.OrgId = orgID - if err := bus.Dispatch(query); err != nil { - if err != m.ErrUserNotFound { - ctx.Handle(500, "Failed to find user specified in auth proxy header", err) + // initialize session + if err := ctx.Session.Start(ctx.Context); err != nil { + log.Error(3, "Failed to start session", err) + return false + } + + query := &m.GetSignedInUserQuery{OrgId: orgID} + + // if this session has already been authenticated by authProxy just load the user + sessProxyValue := ctx.Session.Get(AUTH_PROXY_SESSION_VAR) + if sessProxyValue != nil && sessProxyValue.(string) == proxyHeaderValue && getRequestUserId(ctx) > 0 { + query.UserId = getRequestUserId(ctx) + if err := bus.Dispatch(query); err != nil { + ctx.Handle(500, "Failed to find user", err) return true } - - if !setting.AuthProxyAutoSignUp { - return false + } else { + extUser := m.ExternalUserInfo{ + AuthModule: "authproxy", + AuthId: proxyHeaderValue, } - cmd := getCreateUserCommandForProxyAuth(proxyHeaderValue) - if setting.LdapEnabled { - cmd.SkipOrgSetup = true + if setting.AuthProxyHeaderProperty == "username" { + extUser.Login = proxyHeaderValue + + // only set Email if it can be parsed as an email address + emailAddr, emailErr := mail.ParseAddress(proxyHeaderValue) + if emailErr == nil { + extUser.Email = emailAddr.Address + } + } else if setting.AuthProxyHeaderProperty == "email" { + extUser.Email = proxyHeaderValue + extUser.Login = proxyHeaderValue + } else { + ctx.Handle(500, "Auth proxy header property invalid", nil) } - if err := bus.Dispatch(cmd); err != nil { - ctx.Handle(500, "Failed to create user specified in auth proxy header", err) + // add/update user in grafana + userQuery := &m.UpsertUserCommand{ + ExternalUser: &extUser, + SignupAllowed: setting.AuthProxyAutoSignUp, + } + err := login.UpsertUser(ctx, userQuery) + if err != nil { + ctx.Handle(500, "Failed to login as user specified in auth proxy header", err) return true } - query = &m.GetSignedInUserQuery{UserId: cmd.Result.Id, OrgId: orgID} + + query.UserId = userQuery.User.Id + if err := bus.Dispatch(query); err != nil { - ctx.Handle(500, "Failed find user after creation", err) + ctx.Handle(500, "Failed to find user", err) return true } - } - // initialize session - if err := ctx.Session.Start(ctx.Context); err != nil { - log.Error(3, "Failed to start session", err) - return false - } + // Make sure that we cannot share a session between different users! + if getRequestUserId(ctx) > 0 && getRequestUserId(ctx) != query.Result.UserId { + // remove session + if err := ctx.Session.Destory(ctx.Context); err != nil { + log.Error(3, "Failed to destroy session, err") + } - // Make sure that we cannot share a session between different users! - if getRequestUserId(ctx) > 0 && getRequestUserId(ctx) != query.Result.UserId { - // remove session - if err := ctx.Session.Destory(ctx.Context); err != nil { - log.Error(3, "Failed to destroy session, err") + // initialize a new session + if err := ctx.Session.Start(ctx.Context); err != nil { + log.Error(3, "Failed to start session", err) + } } - // initialize a new session - if err := ctx.Session.Start(ctx.Context); err != nil { - log.Error(3, "Failed to start session", err) - } + ctx.Session.Set(AUTH_PROXY_SESSION_VAR, proxyHeaderValue) } // When ldap is enabled, sync userinfo and org roles @@ -143,29 +170,3 @@ func checkAuthenticationProxy(remoteAddr string, proxyHeaderValue string) error return fmt.Errorf("Request for user (%s) from %s is not from the authentication proxy", proxyHeaderValue, sourceIP) } - -func getSignedInUserQueryForProxyAuth(headerVal string) *m.GetSignedInUserQuery { - query := m.GetSignedInUserQuery{} - if setting.AuthProxyHeaderProperty == "username" { - query.Login = headerVal - } else if setting.AuthProxyHeaderProperty == "email" { - query.Email = headerVal - } else { - panic("Auth proxy header property invalid") - } - return &query -} - -func getCreateUserCommandForProxyAuth(headerVal string) *m.CreateUserCommand { - cmd := m.CreateUserCommand{} - if setting.AuthProxyHeaderProperty == "username" { - cmd.Login = headerVal - cmd.Email = headerVal - } else if setting.AuthProxyHeaderProperty == "email" { - cmd.Email = headerVal - cmd.Login = headerVal - } else { - panic("Auth proxy header property invalid") - } - return &cmd -} diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 09c38bed644..b7a6afa47aa 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -9,6 +9,7 @@ import ( ms "github.com/go-macaron/session" "github.com/grafana/grafana/pkg/bus" + "github.com/grafana/grafana/pkg/login" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" @@ -182,6 +183,11 @@ func TestMiddlewareContext(t *testing.T) { return nil }) + login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + cmd.User = &m.User{Id: 12} + return nil + } + sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") sc.exec() @@ -208,10 +214,10 @@ func TestMiddlewareContext(t *testing.T) { } }) - bus.AddHandler("test", func(cmd *m.CreateUserCommand) error { - cmd.Result = m.User{Id: 33} + login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + cmd.User = &m.User{Id: 33} return nil - }) + } sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") @@ -270,6 +276,11 @@ func TestMiddlewareContext(t *testing.T) { return nil }) + login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + cmd.User = &m.User{Id: 33} + return nil + } + sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") sc.req.RemoteAddr = "[2001::23]:12345" From a1b1d2fe80d17927b5157908838a53f8bdbb65c1 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Fri, 23 Mar 2018 11:16:11 -0400 Subject: [PATCH 05/20] switch to Result --- pkg/api/login_oauth.go | 6 +++--- pkg/login/ext_user.go | 10 +++++----- pkg/login/ldap.go | 2 +- pkg/middleware/auth_proxy.go | 6 +++--- pkg/middleware/middleware_test.go | 6 +++--- pkg/models/user_auth.go | 2 +- 6 files changed, 16 insertions(+), 16 deletions(-) diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 38ded183947..0a2800257d0 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -164,18 +164,18 @@ func OAuthLogin(ctx *m.ReqContext) { } // add/update user in grafana - userQuery := &m.UpsertUserCommand{ + cmd := &m.UpsertUserCommand{ ExternalUser: &extUser, SignupAllowed: connect.IsSignupAllowed(), } - err = login.UpsertUser(ctx, userQuery) + err = login.UpsertUser(ctx, cmd) if err != nil { redirectWithError(ctx, err) return } // login - loginUserWithUser(userQuery.User, ctx) + loginUserWithUser(cmd.Result, ctx) metrics.M_Api_Login_OAuth.Inc() diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 82ebcf481b9..05eef3e35dd 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -39,15 +39,15 @@ var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { return ErrUsersQuotaReached } - cmd.User, err = createUser(extUser) + cmd.Result, err = createUser(extUser) if err != nil { return err } } else { - cmd.User = userQuery.User + cmd.Result = userQuery.User // sync user info - err = updateUser(cmd.User, extUser) + err = updateUser(cmd.Result, extUser) if err != nil { return err } @@ -55,7 +55,7 @@ var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { if userQuery.UserAuth == nil && extUser.AuthModule != "" && extUser.AuthId != "" { cmd2 := m.SetAuthInfoCommand{ - UserId: cmd.User.Id, + UserId: cmd.Result.Id, AuthModule: extUser.AuthModule, AuthId: extUser.AuthId, } @@ -64,7 +64,7 @@ var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { } } - err = syncOrgRoles(cmd.User, extUser) + err = syncOrgRoles(cmd.Result, extUser) if err != nil { return err } diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index fd98320008e..5f7f44f2df5 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -199,7 +199,7 @@ func (a *ldapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo return nil, err } - return userQuery.User, nil + return userQuery.Result, nil } func (a *ldapAuther) serverBind() error { diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index 424d0719f50..0da0a460a8b 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -71,17 +71,17 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { } // add/update user in grafana - userQuery := &m.UpsertUserCommand{ + cmd := &m.UpsertUserCommand{ ExternalUser: &extUser, SignupAllowed: setting.AuthProxyAutoSignUp, } - err := login.UpsertUser(ctx, userQuery) + err := login.UpsertUser(ctx, cmd) if err != nil { ctx.Handle(500, "Failed to login as user specified in auth proxy header", err) return true } - query.UserId = userQuery.User.Id + query.UserId = cmd.Result.Id if err := bus.Dispatch(query); err != nil { ctx.Handle(500, "Failed to find user", err) diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index b7a6afa47aa..a2cccf1fd5f 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -184,7 +184,7 @@ func TestMiddlewareContext(t *testing.T) { }) login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { - cmd.User = &m.User{Id: 12} + cmd.Result = &m.User{Id: 12} return nil } @@ -215,7 +215,7 @@ func TestMiddlewareContext(t *testing.T) { }) login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { - cmd.User = &m.User{Id: 33} + cmd.Result = &m.User{Id: 33} return nil } @@ -277,7 +277,7 @@ func TestMiddlewareContext(t *testing.T) { }) login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { - cmd.User = &m.User{Id: 33} + cmd.Result = &m.User{Id: 33} return nil } diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index 99c1a70c153..65d39e6f748 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -29,7 +29,7 @@ type UpsertUserCommand struct { ExternalUser *ExternalUserInfo SignupAllowed bool - User *User + Result *User } type SetAuthInfoCommand struct { From e53315dce847bed39d939fadfd231284e0bee31b Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Fri, 23 Mar 2018 11:58:38 -0400 Subject: [PATCH 06/20] cleanup --- pkg/login/ext_user.go | 2 +- pkg/middleware/auth_proxy.go | 1 + pkg/services/sqlstore/user_auth.go | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 05eef3e35dd..763f8b09da6 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -117,8 +117,8 @@ func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { } func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { + // don't sync org roles if none are specified if len(extUser.OrgRoles) == 0 { - // log.Warn("No group mappings defined") return nil } diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index 0da0a460a8b..b452d8e23b7 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -68,6 +68,7 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { extUser.Login = proxyHeaderValue } else { ctx.Handle(500, "Auth proxy header property invalid", nil) + return true } // add/update user in grafana diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index 071f003d6ad..6ddccb0da47 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -15,7 +15,7 @@ func init() { } func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { - user := new(m.User) + user := &m.User{} has := false var err error From d837be91ecbeedabf904e1f3a66a7f57d757bc8b Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Fri, 23 Mar 2018 15:50:07 -0400 Subject: [PATCH 07/20] switch to passing ReqContext as a property --- pkg/api/login.go | 11 ++++++----- pkg/api/login_oauth.go | 8 +++++--- pkg/login/auth.go | 4 ++-- pkg/login/auth_test.go | 18 +++++++++--------- pkg/login/ext_user.go | 8 ++++++-- pkg/login/ldap.go | 22 +++++++++++----------- pkg/login/ldap_login.go | 4 ++-- pkg/login/ldap_login_test.go | 10 +++++----- pkg/login/ldap_test.go | 2 ++ pkg/middleware/auth_proxy.go | 7 ++++--- pkg/middleware/auth_proxy_test.go | 2 +- pkg/middleware/middleware_test.go | 23 ++++++++++++++++------- pkg/models/user_auth.go | 10 ++++++---- 13 files changed, 75 insertions(+), 54 deletions(-) diff --git a/pkg/api/login.go b/pkg/api/login.go index 5b1d96758bb..9d0fa31946f 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -101,13 +101,14 @@ func LoginPost(c *m.ReqContext, cmd dtos.LoginCommand) Response { return Error(401, "Login is disabled", nil) } - authQuery := m.LoginUserQuery{ - Username: cmd.User, - Password: cmd.Password, - IpAddress: c.Req.RemoteAddr, + authQuery := &m.LoginUserQuery{ + ReqContext: c, + Username: cmd.User, + Password: cmd.Password, + IpAddress: c.Req.RemoteAddr, } - if err := login.AuthenticateUser(c, &authQuery); err != nil { + if err := bus.Dispatch(authQuery); err != nil { if err == login.ErrInvalidCredentials || err == login.ErrTooManyLoginAttempts { return Error(401, "Invalid username or password", err) } diff --git a/pkg/api/login_oauth.go b/pkg/api/login_oauth.go index 0a2800257d0..c4a5f8fdacf 100644 --- a/pkg/api/login_oauth.go +++ b/pkg/api/login_oauth.go @@ -13,6 +13,7 @@ import ( "golang.org/x/oauth2" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/metrics" @@ -150,7 +151,7 @@ func OAuthLogin(ctx *m.ReqContext) { return } - extUser := m.ExternalUserInfo{ + extUser := &m.ExternalUserInfo{ AuthModule: "oauth_" + name, AuthId: userInfo.Id, Name: userInfo.Name, @@ -165,10 +166,11 @@ func OAuthLogin(ctx *m.ReqContext) { // add/update user in grafana cmd := &m.UpsertUserCommand{ - ExternalUser: &extUser, + ReqContext: ctx, + ExternalUser: extUser, SignupAllowed: connect.IsSignupAllowed(), } - err = login.UpsertUser(ctx, cmd) + err = bus.Dispatch(cmd) if err != nil { redirectWithError(ctx, err) return diff --git a/pkg/login/auth.go b/pkg/login/auth.go index b95126271cf..215a22cde33 100644 --- a/pkg/login/auth.go +++ b/pkg/login/auth.go @@ -23,7 +23,7 @@ func Init() { loadLdapConfig() } -func AuthenticateUser(ctx *m.ReqContext, query *m.LoginUserQuery) error { +func AuthenticateUser(query *m.LoginUserQuery) error { if err := validateLoginAttempts(query.Username); err != nil { return err } @@ -33,7 +33,7 @@ func AuthenticateUser(ctx *m.ReqContext, query *m.LoginUserQuery) error { return err } - ldapEnabled, ldapErr := loginUsingLdap(ctx, query) + ldapEnabled, ldapErr := loginUsingLdap(query) if ldapEnabled { if ldapErr == nil || ldapErr != ErrInvalidCredentials { return ldapErr diff --git a/pkg/login/auth_test.go b/pkg/login/auth_test.go index 2853b1fd15d..932125c410e 100644 --- a/pkg/login/auth_test.go +++ b/pkg/login/auth_test.go @@ -16,7 +16,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrTooManyLoginAttempts) @@ -33,7 +33,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, nil) @@ -51,7 +51,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, customErr) @@ -68,7 +68,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(false, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -85,7 +85,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -102,7 +102,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, nil, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldBeNil) @@ -120,7 +120,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, customErr, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, customErr) @@ -137,7 +137,7 @@ func TestAuthenticateUser(t *testing.T) { mockLoginUsingLdap(true, ErrInvalidCredentials, sc) mockSaveInvalidLoginAttempt(sc) - err := AuthenticateUser(nil, sc.loginUserQuery) + err := AuthenticateUser(sc.loginUserQuery) Convey("it should result in", func() { So(err, ShouldEqual, ErrInvalidCredentials) @@ -168,7 +168,7 @@ func mockLoginUsingGrafanaDB(err error, sc *authScenarioContext) { } func mockLoginUsingLdap(enabled bool, err error, sc *authScenarioContext) { - loginUsingLdap = func(c *m.ReqContext, query *m.LoginUserQuery) (bool, error) { + loginUsingLdap = func(query *m.LoginUserQuery) (bool, error) { sc.ldapLoginWasCalled = true return enabled, err } diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 763f8b09da6..e8234e505e3 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -9,7 +9,11 @@ import ( "github.com/grafana/grafana/pkg/services/quota" ) -var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { +func init() { + bus.AddHandler("auth", UpsertUser) +} + +func UpsertUser(cmd *m.UpsertUserCommand) error { extUser := cmd.ExternalUser userQuery := m.GetUserByAuthInfoQuery{ @@ -30,7 +34,7 @@ var UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { return ErrInvalidCredentials } - limitReached, err := quota.QuotaReached(ctx, "user") + limitReached, err := quota.QuotaReached(cmd.ReqContext, "user") if err != nil { log.Warn("Error getting user quota", "err", err) return ErrGettingUserQuota diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index 5f7f44f2df5..2d0394a47c3 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -10,6 +10,7 @@ import ( "github.com/davecgh/go-spew/spew" "github.com/go-ldap/ldap" + "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" @@ -23,7 +24,7 @@ type ILdapConn interface { } type ILdapAuther interface { - Login(ctx *m.ReqContext, query *m.LoginUserQuery) error + Login(query *m.LoginUserQuery) error SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) } @@ -87,17 +88,15 @@ func (a *ldapAuther) Dial() error { return err } -func (a *ldapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { +func (a *ldapAuther) Login(query *m.LoginUserQuery) error { // connect to ldap server - err := a.Dial() - if err != nil { + if err := a.Dial(); err != nil { return err } defer a.conn.Close() // perform initial authentication - err = a.initialBind(query.Username, query.Password) - if err != nil { + if err := a.initialBind(query.Username, query.Password); err != nil { return err } @@ -117,7 +116,7 @@ func (a *ldapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { } } - grafanaUser, err := a.GetGrafanaUserFor(ctx, ldapUser) + grafanaUser, err := a.GetGrafanaUserFor(query.ReqContext, ldapUser) if err != nil { return err } @@ -158,7 +157,7 @@ func (a *ldapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedI } func (a *ldapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) { - extUser := m.ExternalUserInfo{ + extUser := &m.ExternalUserInfo{ AuthModule: "ldap", AuthId: ldapUser.DN, Name: fmt.Sprintf("%s %s", ldapUser.FirstName, ldapUser.LastName), @@ -190,11 +189,12 @@ func (a *ldapAuther) GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo } // add/update user in grafana - userQuery := m.UpsertUserCommand{ - ExternalUser: &extUser, + userQuery := &m.UpsertUserCommand{ + ReqContext: ctx, + ExternalUser: extUser, SignupAllowed: setting.LdapAllowSignup, } - err := UpsertUser(ctx, &userQuery) + err := bus.Dispatch(userQuery) if err != nil { return nil, err } diff --git a/pkg/login/ldap_login.go b/pkg/login/ldap_login.go index 03f6390a991..5974e19d691 100644 --- a/pkg/login/ldap_login.go +++ b/pkg/login/ldap_login.go @@ -5,14 +5,14 @@ import ( "github.com/grafana/grafana/pkg/setting" ) -var loginUsingLdap = func(ctx *m.ReqContext, query *m.LoginUserQuery) (bool, error) { +var loginUsingLdap = func(query *m.LoginUserQuery) (bool, error) { if !setting.LdapEnabled { return false, nil } for _, server := range LdapCfg.Servers { author := NewLdapAuthenticator(server) - err := author.Login(ctx, query) + err := author.Login(query) if err == nil || err != ErrInvalidCredentials { return true, err } diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index b6d1f619324..1eefc45752d 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -19,7 +19,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login with invalid credentials", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - enabled, err := loginUsingLdap(nil, sc.loginUserQuery) + enabled, err := loginUsingLdap(sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -36,7 +36,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login with valid credentials", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(true) - enabled, err := loginUsingLdap(nil, sc.loginUserQuery) + enabled, err := loginUsingLdap(sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -58,7 +58,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(true) - enabled, err := loginUsingLdap(nil, sc.loginUserQuery) + enabled, err := loginUsingLdap(sc.loginUserQuery) Convey("it should return true", func() { So(enabled, ShouldBeTrue) @@ -79,7 +79,7 @@ func TestLdapLogin(t *testing.T) { ldapLoginScenario("When login", func(sc *ldapLoginScenarioContext) { sc.withLoginResult(false) - enabled, err := loginUsingLdap(nil, &m.LoginUserQuery{ + enabled, err := loginUsingLdap(&m.LoginUserQuery{ Username: "user", Password: "pwd", }) @@ -117,7 +117,7 @@ type mockLdapAuther struct { loginCalled bool } -func (a *mockLdapAuther) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { +func (a *mockLdapAuther) Login(query *m.LoginUserQuery) error { a.loginCalled = true if !a.validLogin { diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 43d37a621d5..63c6704ef9d 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -192,6 +192,8 @@ func ldapAutherScenario(desc string, fn scenarioFunc) { sc := &scenarioContext{} + bus.AddHandler("test", UpsertUser) + bus.AddHandler("test", func(cmd *m.GetUserByAuthInfoQuery) error { sc.getUserByAuthInfoQuery = cmd sc.getUserByAuthInfoQuery.User = &m.User{Login: cmd.Login} diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index b452d8e23b7..4072868abb1 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -50,7 +50,7 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { return true } } else { - extUser := m.ExternalUserInfo{ + extUser := &m.ExternalUserInfo{ AuthModule: "authproxy", AuthId: proxyHeaderValue, } @@ -73,10 +73,11 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { // add/update user in grafana cmd := &m.UpsertUserCommand{ - ExternalUser: &extUser, + ReqContext: ctx, + ExternalUser: extUser, SignupAllowed: setting.AuthProxyAutoSignUp, } - err := login.UpsertUser(ctx, cmd) + err := bus.Dispatch(cmd) if err != nil { ctx.Handle(500, "Failed to login as user specified in auth proxy header", err) return true diff --git a/pkg/middleware/auth_proxy_test.go b/pkg/middleware/auth_proxy_test.go index 8fbaf7f2de8..71513abeae0 100644 --- a/pkg/middleware/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy_test.go @@ -116,7 +116,7 @@ type mockLdapAuthenticator struct { syncSignedInUserCalled bool } -func (a *mockLdapAuthenticator) Login(ctx *m.ReqContext, query *m.LoginUserQuery) error { +func (a *mockLdapAuthenticator) Login(query *m.LoginUserQuery) error { return nil } diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index a2cccf1fd5f..3b508d06387 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -9,7 +9,6 @@ import ( ms "github.com/go-macaron/session" "github.com/grafana/grafana/pkg/bus" - "github.com/grafana/grafana/pkg/login" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" @@ -183,10 +182,10 @@ func TestMiddlewareContext(t *testing.T) { return nil }) - login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + bus.AddHandler("test", func(cmd *m.UpsertUserCommand) error { cmd.Result = &m.User{Id: 12} return nil - } + }) sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") @@ -214,10 +213,10 @@ func TestMiddlewareContext(t *testing.T) { } }) - login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + bus.AddHandler("test", func(cmd *m.UpsertUserCommand) error { cmd.Result = &m.User{Id: 33} return nil - } + }) sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") @@ -276,10 +275,10 @@ func TestMiddlewareContext(t *testing.T) { return nil }) - login.UpsertUser = func(ctx *m.ReqContext, cmd *m.UpsertUserCommand) error { + bus.AddHandler("test", func(cmd *m.UpsertUserCommand) error { cmd.Result = &m.User{Id: 33} return nil - } + }) sc.fakeReq("GET", "/") sc.req.Header.Add("X-WEBAUTH-USER", "torkelo") @@ -299,6 +298,11 @@ func TestMiddlewareContext(t *testing.T) { setting.AuthProxyHeaderProperty = "username" setting.AuthProxyWhitelist = "" + bus.AddHandler("test", func(query *m.UpsertUserCommand) error { + query.Result = &m.User{Id: 32} + return nil + }) + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { query.Result = &m.SignedInUser{OrgId: 4, UserId: 32} return nil @@ -334,6 +338,11 @@ func TestMiddlewareContext(t *testing.T) { return nil } + bus.AddHandler("test", func(query *m.UpsertUserCommand) error { + query.Result = &m.User{Id: 32} + return nil + }) + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { query.Result = &m.SignedInUser{OrgId: 4, UserId: 32} return nil diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index 65d39e6f748..c72a327665f 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -26,6 +26,7 @@ type ExternalUserInfo struct { // COMMANDS type UpsertUserCommand struct { + ReqContext *ReqContext ExternalUser *ExternalUserInfo SignupAllowed bool @@ -46,10 +47,11 @@ type DeleteAuthInfoCommand struct { // QUERIES type LoginUserQuery struct { - Username string - Password string - User *User - IpAddress string + ReqContext *ReqContext + Username string + Password string + User *User + IpAddress string } type GetUserByAuthInfoQuery struct { From 65847da1df8f8b1f2ce2c570399cfaad7ceffa08 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Wed, 28 Mar 2018 18:06:22 -0400 Subject: [PATCH 08/20] use Result in GetAuthInfoQuery --- pkg/models/user_auth.go | 2 +- pkg/services/sqlstore/user_auth.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index c72a327665f..85a063775c9 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -69,5 +69,5 @@ type GetAuthInfoQuery struct { AuthModule string AuthId string - UserAuth *UserAuth + Result *UserAuth } diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index 6ddccb0da47..f9410c4682c 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -28,22 +28,22 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { err = GetAuthInfo(authQuery) // if user id was specified and doesn't match the user_auth entry, remove it - if err == nil && query.UserId != 0 && query.UserId != authQuery.UserAuth.UserId { + if err == nil && query.UserId != 0 && query.UserId != authQuery.Result.UserId { DeleteAuthInfo(&m.DeleteAuthInfoCommand{ - UserAuth: authQuery.UserAuth, + UserAuth: authQuery.Result, }) } else if err == nil { - has, err = x.Id(authQuery.UserAuth.UserId).Get(user) + has, err = x.Id(authQuery.Result.UserId).Get(user) if err != nil { return err } if has { - query.UserAuth = authQuery.UserAuth + query.UserAuth = authQuery.Result } else { // if the user has been deleted then remove the entry DeleteAuthInfo(&m.DeleteAuthInfoCommand{ - UserAuth: authQuery.UserAuth, + UserAuth: authQuery.Result, }) } } else if err != m.ErrUserNotFound { @@ -99,7 +99,7 @@ func GetAuthInfo(query *m.GetAuthInfoQuery) error { return m.ErrUserNotFound } - query.UserAuth = userAuth + query.Result = userAuth return nil } From c4168c2396e58d31877e200c31f10df8e0215bf1 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Wed, 28 Mar 2018 21:07:02 -0400 Subject: [PATCH 09/20] error handling --- pkg/services/sqlstore/user_auth.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index f9410c4682c..32d9e246dd7 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -29,9 +29,12 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { err = GetAuthInfo(authQuery) // if user id was specified and doesn't match the user_auth entry, remove it if err == nil && query.UserId != 0 && query.UserId != authQuery.Result.UserId { - DeleteAuthInfo(&m.DeleteAuthInfoCommand{ + err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, }) + if err != nil { + sqlog.Error("Error removing user_auth entry", "error", err) + } } else if err == nil { has, err = x.Id(authQuery.Result.UserId).Get(user) if err != nil { @@ -42,9 +45,12 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { query.UserAuth = authQuery.Result } else { // if the user has been deleted then remove the entry - DeleteAuthInfo(&m.DeleteAuthInfoCommand{ + err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, }) + if err != nil { + sqlog.Error("Error removing user_auth entry", "error", err) + } } } else if err != m.ErrUserNotFound { return err @@ -113,21 +119,13 @@ func SetAuthInfo(cmd *m.SetAuthInfoCommand) error { } _, err := sess.Insert(&authUser) - if err != nil { - return err - } - - return nil + return err }) } func DeleteAuthInfo(cmd *m.DeleteAuthInfoCommand) error { return inTransaction(func(sess *DBSession) error { _, err := sess.Delete(cmd.UserAuth) - if err != nil { - return err - } - - return nil + return err }) } From e53e039b1b8071ac8ff88c72eda6e01119977d31 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Wed, 28 Mar 2018 21:26:35 -0400 Subject: [PATCH 10/20] restructure GetUserByAuthInfo --- pkg/login/ext_user.go | 52 +++++++++++++++--------------- pkg/models/user_auth.go | 3 +- pkg/services/sqlstore/user_auth.go | 33 +++++++++++++------ 3 files changed, 50 insertions(+), 38 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index e8234e505e3..f60d5d1e22d 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -16,14 +16,14 @@ func init() { func UpsertUser(cmd *m.UpsertUserCommand) error { extUser := cmd.ExternalUser - userQuery := m.GetUserByAuthInfoQuery{ + userQuery := &m.GetUserByAuthInfoQuery{ AuthModule: extUser.AuthModule, AuthId: extUser.AuthId, UserId: extUser.UserId, Email: extUser.Email, Login: extUser.Login, } - err := bus.Dispatch(&userQuery) + err := bus.Dispatch(userQuery) if err != nil { if err != m.ErrUserNotFound { return err @@ -47,8 +47,19 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { if err != nil { return err } + + if extUser.AuthModule != "" && extUser.AuthId != "" { + cmd2 := &m.SetAuthInfoCommand{ + UserId: cmd.Result.Id, + AuthModule: extUser.AuthModule, + AuthId: extUser.AuthId, + } + if err := bus.Dispatch(cmd2); err != nil { + return err + } + } } else { - cmd.Result = userQuery.User + cmd.Result = userQuery.Result // sync user info err = updateUser(cmd.Result, extUser) @@ -57,17 +68,6 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { } } - if userQuery.UserAuth == nil && extUser.AuthModule != "" && extUser.AuthId != "" { - cmd2 := m.SetAuthInfoCommand{ - UserId: cmd.Result.Id, - AuthModule: extUser.AuthModule, - AuthId: extUser.AuthId, - } - if err := bus.Dispatch(&cmd2); err != nil { - return err - } - } - err = syncOrgRoles(cmd.Result, extUser) if err != nil { return err @@ -77,12 +77,12 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { } func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { - cmd := m.CreateUserCommand{ + cmd := &m.CreateUserCommand{ Login: extUser.Login, Email: extUser.Email, Name: extUser.Name, } - if err := bus.Dispatch(&cmd); err != nil { + if err := bus.Dispatch(cmd); err != nil { return nil, err } @@ -91,7 +91,7 @@ func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { // sync user info - updateCmd := m.UpdateUserCommand{ + updateCmd := &m.UpdateUserCommand{ UserId: user.Id, } needsUpdate := false @@ -111,7 +111,7 @@ func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { if needsUpdate { log.Debug("Syncing user info", "id", user.Id, "update", updateCmd) - err := bus.Dispatch(&updateCmd) + err := bus.Dispatch(updateCmd) if err != nil { return err } @@ -126,8 +126,8 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { return nil } - orgsQuery := m.GetUserOrgListQuery{UserId: user.Id} - if err := bus.Dispatch(&orgsQuery); err != nil { + orgsQuery := &m.GetUserOrgListQuery{UserId: user.Id} + if err := bus.Dispatch(orgsQuery); err != nil { return err } @@ -142,8 +142,8 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { deleteOrgIds = append(deleteOrgIds, org.OrgId) } else if extUser.OrgRoles[org.OrgId] != org.Role { // update role - cmd := m.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: extUser.OrgRoles[org.OrgId]} - if err := bus.Dispatch(&cmd); err != nil { + cmd := &m.UpdateOrgUserCommand{OrgId: org.OrgId, UserId: user.Id, Role: extUser.OrgRoles[org.OrgId]} + if err := bus.Dispatch(cmd); err != nil { return err } } @@ -156,8 +156,8 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { } // add role - cmd := m.AddOrgUserCommand{UserId: user.Id, Role: orgRole, OrgId: orgId} - err := bus.Dispatch(&cmd) + cmd := &m.AddOrgUserCommand{UserId: user.Id, Role: orgRole, OrgId: orgId} + err := bus.Dispatch(cmd) if err != nil && err != m.ErrOrgNotFound { return err } @@ -165,8 +165,8 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { // delete any removed org roles for _, orgId := range deleteOrgIds { - cmd := m.RemoveOrgUserCommand{OrgId: orgId, UserId: user.Id} - if err := bus.Dispatch(&cmd); err != nil { + cmd := &m.RemoveOrgUserCommand{OrgId: orgId, UserId: user.Id} + if err := bus.Dispatch(cmd); err != nil { return err } } diff --git a/pkg/models/user_auth.go b/pkg/models/user_auth.go index 85a063775c9..0ecd144d52c 100644 --- a/pkg/models/user_auth.go +++ b/pkg/models/user_auth.go @@ -61,8 +61,7 @@ type GetUserByAuthInfoQuery struct { Email string Login string - User *User - UserAuth *UserAuth + Result *User } type GetAuthInfoQuery struct { diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index 32d9e246dd7..ca26791c440 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -18,13 +18,12 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { user := &m.User{} has := false var err error + authQuery := &m.GetAuthInfoQuery{} // Try to find the user by auth module and id first if query.AuthModule != "" && query.AuthId != "" { - authQuery := &m.GetAuthInfoQuery{ - AuthModule: query.AuthModule, - AuthId: query.AuthId, - } + authQuery.AuthModule = query.AuthModule + authQuery.AuthId = query.AuthId err = GetAuthInfo(authQuery) // if user id was specified and doesn't match the user_auth entry, remove it @@ -35,15 +34,15 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { if err != nil { sqlog.Error("Error removing user_auth entry", "error", err) } + + authQuery.Result = nil } else if err == nil { has, err = x.Id(authQuery.Result.UserId).Get(user) if err != nil { return err } - if has { - query.UserAuth = authQuery.Result - } else { + if !has { // if the user has been deleted then remove the entry err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, @@ -51,6 +50,8 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { if err != nil { sqlog.Error("Error removing user_auth entry", "error", err) } + + authQuery.Result = nil } } else if err != m.ErrUserNotFound { return err @@ -88,7 +89,19 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { return m.ErrUserNotFound } - query.User = user + // create authInfo record to link accounts + if authQuery.Result == nil && query.AuthModule != "" && query.AuthId != "" { + cmd2 := &m.SetAuthInfoCommand{ + UserId: user.Id, + AuthModule: query.AuthModule, + AuthId: query.AuthId, + } + if err := SetAuthInfo(cmd2); err != nil { + return err + } + } + + query.Result = user return nil } @@ -111,14 +124,14 @@ func GetAuthInfo(query *m.GetAuthInfoQuery) error { func SetAuthInfo(cmd *m.SetAuthInfoCommand) error { return inTransaction(func(sess *DBSession) error { - authUser := m.UserAuth{ + authUser := &m.UserAuth{ UserId: cmd.UserId, AuthModule: cmd.AuthModule, AuthId: cmd.AuthId, Created: time.Now(), } - _, err := sess.Insert(&authUser) + _, err := sess.Insert(authUser) return err }) } From 59f83ca5d8796954449fe059a445ac499a574cda Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Wed, 28 Mar 2018 21:30:34 -0400 Subject: [PATCH 11/20] fix ldap test --- pkg/login/ldap_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 63c6704ef9d..9d9e7c53667 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -196,7 +196,7 @@ func ldapAutherScenario(desc string, fn scenarioFunc) { bus.AddHandler("test", func(cmd *m.GetUserByAuthInfoQuery) error { sc.getUserByAuthInfoQuery = cmd - sc.getUserByAuthInfoQuery.User = &m.User{Login: cmd.Login} + sc.getUserByAuthInfoQuery.Result = &m.User{Login: cmd.Login} return nil }) @@ -250,7 +250,7 @@ func (sc *scenarioContext) userQueryReturns(user *m.User) { if user == nil { return m.ErrUserNotFound } else { - query.User = user + query.Result = user return nil } }) From daeba40b3be5de681835a03adfb2877192e8b37b Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Thu, 5 Apr 2018 21:05:24 -0400 Subject: [PATCH 12/20] tests for user auth module --- pkg/services/sqlstore/user_auth_test.go | 131 ++++++++++++++++++++++++ 1 file changed, 131 insertions(+) create mode 100644 pkg/services/sqlstore/user_auth_test.go diff --git a/pkg/services/sqlstore/user_auth_test.go b/pkg/services/sqlstore/user_auth_test.go new file mode 100644 index 00000000000..279fd7aa0f5 --- /dev/null +++ b/pkg/services/sqlstore/user_auth_test.go @@ -0,0 +1,131 @@ +package sqlstore + +import ( + "fmt" + "testing" + + . "github.com/smartystreets/goconvey/convey" + + m "github.com/grafana/grafana/pkg/models" +) + +func TestUserAuth(t *testing.T) { + InitTestDB(t) + + Convey("Given 5 users", t, func() { + var err error + var cmd *m.CreateUserCommand + users := []m.User{} + for i := 0; i < 5; i++ { + cmd = &m.CreateUserCommand{ + Email: fmt.Sprint("user", i, "@test.com"), + Name: fmt.Sprint("user", i), + Login: fmt.Sprint("loginuser", i), + } + err = CreateUser(cmd) + So(err, ShouldBeNil) + users = append(users, cmd.Result) + } + + Reset(func() { + _, err := x.Exec("DELETE FROM org_user WHERE 1=1") + So(err, ShouldBeNil) + _, err = x.Exec("DELETE FROM org WHERE 1=1") + So(err, ShouldBeNil) + _, err = x.Exec("DELETE FROM user WHERE 1=1") + So(err, ShouldBeNil) + _, err = x.Exec("DELETE FROM user_auth WHERE 1=1") + So(err, ShouldBeNil) + }) + + Convey("Can find existing user", func() { + // By Login + login := "loginuser0" + + query := &m.GetUserByAuthInfoQuery{Login: login} + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Login, ShouldEqual, login) + + // By ID + id := query.Result.Id + + query = &m.GetUserByAuthInfoQuery{UserId: id} + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Id, ShouldEqual, id) + + // By Email + email := "user1@test.com" + + query = &m.GetUserByAuthInfoQuery{Email: email} + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Email, ShouldEqual, email) + + // Don't find nonexistent user + email = "nonexistent@test.com" + + query = &m.GetUserByAuthInfoQuery{Email: email} + err = GetUserByAuthInfo(query) + + So(err, ShouldEqual, m.ErrUserNotFound) + So(query.Result, ShouldBeNil) + }) + + Convey("Can set & locate by AuthModule and AuthId", func() { + // get nonexistent user_auth entry + query := &m.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test"} + err = GetUserByAuthInfo(query) + + So(err, ShouldEqual, m.ErrUserNotFound) + So(query.Result, ShouldBeNil) + + // create user_auth entry + login := "loginuser0" + + query.Login = login + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Login, ShouldEqual, login) + + // get via user_auth + query = &m.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test"} + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Login, ShouldEqual, login) + + // get with non-matching id + id := query.Result.Id + + query.UserId = id + 1 + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Login, ShouldEqual, "loginuser1") + + // get via user_auth + query = &m.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test"} + err = GetUserByAuthInfo(query) + + So(err, ShouldBeNil) + So(query.Result.Login, ShouldEqual, "loginuser1") + + // remove user + _, err = x.Exec("DELETE FROM user WHERE id=?", query.Result.Id) + So(err, ShouldBeNil) + + // get via user_auth for deleted user + query = &m.GetUserByAuthInfoQuery{AuthModule: "test", AuthId: "test"} + err = GetUserByAuthInfo(query) + + So(err, ShouldEqual, m.ErrUserNotFound) + So(query.Result, ShouldBeNil) + }) + }) +} From c8a0c1e6b8ffcb813b2f826603721347fea28381 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Fri, 6 Apr 2018 11:23:17 -0400 Subject: [PATCH 13/20] pass DN in ldap test --- pkg/login/ldap_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 9d9e7c53667..8aa1a0652cb 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -65,6 +65,7 @@ func TestLdapAuther(t *testing.T) { sc.userQueryReturns(nil) result, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + DN: "torkelo", Username: "torkelo", Email: "my@email.com", MemberOf: []string{"cn=editor"}, From 52503d9cb58a01cb5787f6ba77feec9dbfbea2b8 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 16 Apr 2018 16:17:01 -0400 Subject: [PATCH 14/20] refactor authproxy & ldap integration, address comments --- pkg/login/ext_user.go | 7 +- pkg/login/ldap.go | 18 ++--- pkg/login/ldap_login_test.go | 2 +- pkg/login/ldap_test.go | 11 ++-- pkg/middleware/auth_proxy.go | 101 ++++++++++++++++++----------- pkg/middleware/auth_proxy_test.go | 53 +++++++++------ pkg/middleware/middleware_test.go | 5 +- pkg/services/sqlstore/user_auth.go | 36 +++++----- 8 files changed, 140 insertions(+), 93 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index f60d5d1e22d..66c39ca960f 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -78,9 +78,10 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { cmd := &m.CreateUserCommand{ - Login: extUser.Login, - Email: extUser.Email, - Name: extUser.Name, + Login: extUser.Login, + Email: extUser.Email, + Name: extUser.Name, + SkipOrgSetup: len(extUser.OrgRoles) > 0, } if err := bus.Dispatch(cmd); err != nil { return nil, err diff --git a/pkg/login/ldap.go b/pkg/login/ldap.go index 2d0394a47c3..d8b1b187093 100644 --- a/pkg/login/ldap.go +++ b/pkg/login/ldap.go @@ -25,7 +25,7 @@ type ILdapConn interface { type ILdapAuther interface { Login(query *m.LoginUserQuery) error - SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error + SyncUser(query *m.LoginUserQuery) error GetGrafanaUserFor(ctx *m.ReqContext, ldapUser *LdapUserInfo) (*m.User, error) } @@ -125,12 +125,12 @@ func (a *ldapAuther) Login(query *m.LoginUserQuery) error { return nil } -func (a *ldapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { +func (a *ldapAuther) SyncUser(query *m.LoginUserQuery) error { + // connect to ldap server err := a.Dial() if err != nil { return err } - defer a.conn.Close() err = a.serverBind() @@ -138,21 +138,21 @@ func (a *ldapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedI return err } - ldapUser, err := a.searchForUser(signedInUser.Login) + // find user entry & attributes + ldapUser, err := a.searchForUser(query.Username) if err != nil { a.log.Error("Failed searching for user in ldap", "error", err) return err } - grafanaUser, err := a.GetGrafanaUserFor(ctx, ldapUser) + a.log.Debug("Ldap User found", "info", spew.Sdump(ldapUser)) + + grafanaUser, err := a.GetGrafanaUserFor(query.ReqContext, ldapUser) if err != nil { return err } - signedInUser.Login = grafanaUser.Login - signedInUser.Email = grafanaUser.Email - signedInUser.Name = grafanaUser.Name - + query.User = grafanaUser return nil } diff --git a/pkg/login/ldap_login_test.go b/pkg/login/ldap_login_test.go index 1eefc45752d..6067a063795 100644 --- a/pkg/login/ldap_login_test.go +++ b/pkg/login/ldap_login_test.go @@ -127,7 +127,7 @@ func (a *mockLdapAuther) Login(query *m.LoginUserQuery) error { return nil } -func (a *mockLdapAuther) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { +func (a *mockLdapAuther) SyncUser(query *m.LoginUserQuery) error { return nil } diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 8aa1a0652cb..bb6e9ea2ed4 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -91,7 +91,7 @@ func TestLdapAuther(t *testing.T) { }) - Convey("When calling SyncSignedInUser", t, func() { + Convey("When calling SyncUser", t, func() { mockLdapConnection := &mockLdapConn{} ldapAuther := NewLdapAuthenticator( @@ -131,11 +131,8 @@ func TestLdapAuther(t *testing.T) { ldapAutherScenario("When ldapUser found call syncInfo and orgRoles", func(sc *scenarioContext) { // arrange - signedInUser := &m.SignedInUser{ - Email: "roel@test.net", - UserId: 1, - Name: "Roel Gerrits", - Login: "roelgerrits", + query := &m.LoginUserQuery{ + Username: "roelgerrits", } sc.userQueryReturns(&m.User{ @@ -147,7 +144,7 @@ func TestLdapAuther(t *testing.T) { sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) // act - syncErrResult := ldapAuther.SyncSignedInUser(nil, signedInUser) + syncErrResult := ldapAuther.SyncUser(query) // assert So(dialCalled, ShouldBeTrue) diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index 4072868abb1..c4e28252964 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -44,11 +44,49 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { // if this session has already been authenticated by authProxy just load the user sessProxyValue := ctx.Session.Get(AUTH_PROXY_SESSION_VAR) if sessProxyValue != nil && sessProxyValue.(string) == proxyHeaderValue && getRequestUserId(ctx) > 0 { + // if we're using ldap, sync user periodically + if setting.LdapEnabled { + syncQuery := &m.LoginUserQuery{ + ReqContext: ctx, + Username: proxyHeaderValue, + } + + if err := syncGrafanaUserWithLdapUser(syncQuery); err != nil { + if err == login.ErrInvalidCredentials { + ctx.Handle(500, "Unable to authenticate user", err) + return false + } + + ctx.Handle(500, "Failed to sync user", err) + return false + } + } + query.UserId = getRequestUserId(ctx) - if err := bus.Dispatch(query); err != nil { - ctx.Handle(500, "Failed to find user", err) - return true + // if we're using ldap, pass authproxy login name to ldap user sync + } else if setting.LdapEnabled { + syncQuery := &m.LoginUserQuery{ + ReqContext: ctx, + Username: proxyHeaderValue, + } + + if err := syncGrafanaUserWithLdapUser(syncQuery); err != nil { + if err == login.ErrInvalidCredentials { + ctx.Handle(500, "Unable to authenticate user", err) + return false + } + + ctx.Handle(500, "Failed to sync user", err) + return false } + + if syncQuery.User == nil { + ctx.Handle(500, "Failed to sync user", nil) + return false + } + + query.UserId = syncQuery.User.Id + // no ldap, just use the info we have } else { extUser := &m.ExternalUserInfo{ AuthModule: "authproxy", @@ -84,39 +122,28 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { } query.UserId = cmd.Result.Id + } - if err := bus.Dispatch(query); err != nil { - ctx.Handle(500, "Failed to find user", err) - return true - } - - // Make sure that we cannot share a session between different users! - if getRequestUserId(ctx) > 0 && getRequestUserId(ctx) != query.Result.UserId { - // remove session - if err := ctx.Session.Destory(ctx.Context); err != nil { - log.Error(3, "Failed to destroy session, err") - } - - // initialize a new session - if err := ctx.Session.Start(ctx.Context); err != nil { - log.Error(3, "Failed to start session", err) - } - } - - ctx.Session.Set(AUTH_PROXY_SESSION_VAR, proxyHeaderValue) + if err := bus.Dispatch(query); err != nil { + ctx.Handle(500, "Failed to find user", err) + return true } - // When ldap is enabled, sync userinfo and org roles - if err := syncGrafanaUserWithLdapUser(ctx, query); err != nil { - if err == login.ErrInvalidCredentials { - ctx.Handle(500, "Unable to authenticate user", err) - return false + // Make sure that we cannot share a session between different users! + if getRequestUserId(ctx) > 0 && getRequestUserId(ctx) != query.Result.UserId { + // remove session + if err := ctx.Session.Destory(ctx.Context); err != nil { + log.Error(3, "Failed to destroy session, err") } - ctx.Handle(500, "Failed to sync user", err) - return false + // initialize a new session + if err := ctx.Session.Start(ctx.Context); err != nil { + log.Error(3, "Failed to start session", err) + } } + ctx.Session.Set(AUTH_PROXY_SESSION_VAR, proxyHeaderValue) + ctx.SignedInUser = query.Result ctx.IsSignedIn = true ctx.Session.Set(session.SESS_KEY_USERID, ctx.UserId) @@ -124,29 +151,29 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { return true } -var syncGrafanaUserWithLdapUser = func(ctx *m.ReqContext, query *m.GetSignedInUserQuery) error { - if !setting.LdapEnabled { - return nil - } - +var syncGrafanaUserWithLdapUser = func(query *m.LoginUserQuery) error { expireEpoch := time.Now().Add(time.Duration(-setting.AuthProxyLdapSyncTtl) * time.Minute).Unix() var lastLdapSync int64 - if lastLdapSyncInSession := ctx.Session.Get(session.SESS_KEY_LASTLDAPSYNC); lastLdapSyncInSession != nil { + if lastLdapSyncInSession := query.ReqContext.Session.Get(session.SESS_KEY_LASTLDAPSYNC); lastLdapSyncInSession != nil { lastLdapSync = lastLdapSyncInSession.(int64) } if lastLdapSync < expireEpoch { ldapCfg := login.LdapCfg + if len(ldapCfg.Servers) < 1 { + return fmt.Errorf("No LDAP servers available") + } + for _, server := range ldapCfg.Servers { author := login.NewLdapAuthenticator(server) - if err := author.SyncSignedInUser(ctx, query.Result); err != nil { + if err := author.SyncUser(query); err != nil { return err } } - ctx.Session.Set(session.SESS_KEY_LASTLDAPSYNC, time.Now().Unix()) + query.ReqContext.Session.Set(session.SESS_KEY_LASTLDAPSYNC, time.Now().Unix()) } return nil diff --git a/pkg/middleware/auth_proxy_test.go b/pkg/middleware/auth_proxy_test.go index 71513abeae0..47ed2f71a79 100644 --- a/pkg/middleware/auth_proxy_test.go +++ b/pkg/middleware/auth_proxy_test.go @@ -26,57 +26,71 @@ func TestAuthProxyWithLdapEnabled(t *testing.T) { return &mockLdapAuther } - signedInUser := m.SignedInUser{} - query := m.GetSignedInUserQuery{Result: &signedInUser} - - Convey("When session variable lastLdapSync not set, call syncSignedInUser and set lastLdapSync", func() { + Convey("When user logs in, call SyncUser", func() { // arrange - sess := mockSession{} + sess := newMockSession() ctx := m.ReqContext{Session: &sess} So(sess.Get(session.SESS_KEY_LASTLDAPSYNC), ShouldBeNil) // act - syncGrafanaUserWithLdapUser(&ctx, &query) + syncGrafanaUserWithLdapUser(&m.LoginUserQuery{ + ReqContext: &ctx, + Username: "test", + }) // assert - So(mockLdapAuther.syncSignedInUserCalled, ShouldBeTrue) + So(mockLdapAuther.syncUserCalled, ShouldBeTrue) So(sess.Get(session.SESS_KEY_LASTLDAPSYNC), ShouldBeGreaterThan, 0) }) Convey("When session variable not expired, don't sync and don't change session var", func() { // arrange - sess := mockSession{} + sess := newMockSession() ctx := m.ReqContext{Session: &sess} now := time.Now().Unix() sess.Set(session.SESS_KEY_LASTLDAPSYNC, now) + sess.Set(AUTH_PROXY_SESSION_VAR, "test") // act - syncGrafanaUserWithLdapUser(&ctx, &query) + syncGrafanaUserWithLdapUser(&m.LoginUserQuery{ + ReqContext: &ctx, + Username: "test", + }) // assert So(sess.Get(session.SESS_KEY_LASTLDAPSYNC), ShouldEqual, now) - So(mockLdapAuther.syncSignedInUserCalled, ShouldBeFalse) + So(mockLdapAuther.syncUserCalled, ShouldBeFalse) }) Convey("When lastldapsync is expired, session variable should be updated", func() { // arrange - sess := mockSession{} + sess := newMockSession() ctx := m.ReqContext{Session: &sess} expiredTime := time.Now().Add(time.Duration(-120) * time.Minute).Unix() sess.Set(session.SESS_KEY_LASTLDAPSYNC, expiredTime) + sess.Set(AUTH_PROXY_SESSION_VAR, "test") // act - syncGrafanaUserWithLdapUser(&ctx, &query) + syncGrafanaUserWithLdapUser(&m.LoginUserQuery{ + ReqContext: &ctx, + Username: "test", + }) // assert So(sess.Get(session.SESS_KEY_LASTLDAPSYNC), ShouldBeGreaterThan, expiredTime) - So(mockLdapAuther.syncSignedInUserCalled, ShouldBeTrue) + So(mockLdapAuther.syncUserCalled, ShouldBeTrue) }) }) } type mockSession struct { - value interface{} + value map[interface{}]interface{} +} + +func newMockSession() mockSession { + session := mockSession{} + session.value = make(map[interface{}]interface{}) + return session } func (s *mockSession) Start(c *macaron.Context) error { @@ -84,15 +98,16 @@ func (s *mockSession) Start(c *macaron.Context) error { } func (s *mockSession) Set(k interface{}, v interface{}) error { - s.value = v + s.value[k] = v return nil } func (s *mockSession) Get(k interface{}) interface{} { - return s.value + return s.value[k] } func (s *mockSession) Delete(k interface{}) interface{} { + delete(s.value, k) return nil } @@ -113,15 +128,15 @@ func (s *mockSession) RegenerateId(c *macaron.Context) error { } type mockLdapAuthenticator struct { - syncSignedInUserCalled bool + syncUserCalled bool } func (a *mockLdapAuthenticator) Login(query *m.LoginUserQuery) error { return nil } -func (a *mockLdapAuthenticator) SyncSignedInUser(ctx *m.ReqContext, signedInUser *m.SignedInUser) error { - a.syncSignedInUserCalled = true +func (a *mockLdapAuthenticator) SyncUser(query *m.LoginUserQuery) error { + a.syncUserCalled = true return nil } diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 3b508d06387..072cb793d3c 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -176,6 +176,7 @@ func TestMiddlewareContext(t *testing.T) { setting.AuthProxyEnabled = true setting.AuthProxyHeaderName = "X-WEBAUTH-USER" setting.AuthProxyHeaderProperty = "username" + setting.LdapEnabled = false bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { query.Result = &m.SignedInUser{OrgId: 2, UserId: 12} @@ -203,6 +204,7 @@ func TestMiddlewareContext(t *testing.T) { setting.AuthProxyHeaderName = "X-WEBAUTH-USER" setting.AuthProxyHeaderProperty = "username" setting.AuthProxyAutoSignUp = true + setting.LdapEnabled = false bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { if query.UserId > 0 { @@ -333,8 +335,9 @@ func TestMiddlewareContext(t *testing.T) { setting.LdapEnabled = true called := false - syncGrafanaUserWithLdapUser = func(ctx *m.ReqContext, query *m.GetSignedInUserQuery) error { + syncGrafanaUserWithLdapUser = func(query *m.LoginUserQuery) error { called = true + query.User = &m.User{Id: 32} return nil } diff --git a/pkg/services/sqlstore/user_auth.go b/pkg/services/sqlstore/user_auth.go index ca26791c440..aec828451a4 100644 --- a/pkg/services/sqlstore/user_auth.go +++ b/pkg/services/sqlstore/user_auth.go @@ -26,24 +26,13 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { authQuery.AuthId = query.AuthId err = GetAuthInfo(authQuery) - // if user id was specified and doesn't match the user_auth entry, remove it - if err == nil && query.UserId != 0 && query.UserId != authQuery.Result.UserId { - err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ - UserAuth: authQuery.Result, - }) - if err != nil { - sqlog.Error("Error removing user_auth entry", "error", err) - } - - authQuery.Result = nil - } else if err == nil { - has, err = x.Id(authQuery.Result.UserId).Get(user) + if err != m.ErrUserNotFound { if err != nil { return err } - if !has { - // if the user has been deleted then remove the entry + // if user id was specified and doesn't match the user_auth entry, remove it + if query.UserId != 0 && query.UserId != authQuery.Result.UserId { err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ UserAuth: authQuery.Result, }) @@ -52,9 +41,24 @@ func GetUserByAuthInfo(query *m.GetUserByAuthInfoQuery) error { } authQuery.Result = nil + } else { + has, err = x.Id(authQuery.Result.UserId).Get(user) + if err != nil { + return err + } + + if !has { + // if the user has been deleted then remove the entry + err = DeleteAuthInfo(&m.DeleteAuthInfoCommand{ + UserAuth: authQuery.Result, + }) + if err != nil { + sqlog.Error("Error removing user_auth entry", "error", err) + } + + authQuery.Result = nil + } } - } else if err != m.ErrUserNotFound { - return err } } From 857830d98150e645527ec50c7eac19cd437a1595 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 16 Apr 2018 17:32:44 -0400 Subject: [PATCH 15/20] org role sync tests --- pkg/login/ldap_test.go | 121 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index bb6e9ea2ed4..577a70bb1dc 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -91,6 +91,127 @@ func TestLdapAuther(t *testing.T) { }) + Convey("When syncing ldap groups to grafana org roles", t, func() { + + ldapAutherScenario("given no current user orgs", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=users", OrgRole: "Admin"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=users"}, + }) + + Convey("Should create new org user", func() { + So(err, ShouldBeNil) + So(sc.addOrgUserCmd, ShouldNotBeNil) + So(sc.addOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) + }) + }) + + ldapAutherScenario("given different current org role", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=users", OrgId: 1, OrgRole: "Admin"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_EDITOR}}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=users"}, + }) + + Convey("Should update org role", func() { + So(err, ShouldBeNil) + So(sc.updateOrgUserCmd, ShouldNotBeNil) + So(sc.updateOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) + }) + }) + + ldapAutherScenario("given current org role is removed in ldap", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=users", OrgId: 1, OrgRole: "Admin"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{ + {OrgId: 1, Role: m.ROLE_EDITOR}, + {OrgId: 2, Role: m.ROLE_EDITOR}, + }) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=users"}, + }) + + Convey("Should remove org role", func() { + So(err, ShouldBeNil) + So(sc.removeOrgUserCmd, ShouldNotBeNil) + }) + }) + + ldapAutherScenario("given org role is updated in config", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=admin", OrgId: 1, OrgRole: "Admin"}, + {GroupDN: "cn=users", OrgId: 1, OrgRole: "Viewer"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_EDITOR}}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=users"}, + }) + + Convey("Should update org role", func() { + So(err, ShouldBeNil) + So(sc.removeOrgUserCmd, ShouldBeNil) + So(sc.updateOrgUserCmd, ShouldNotBeNil) + }) + }) + + ldapAutherScenario("given multiple matching ldap groups", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=admins", OrgId: 1, OrgRole: "Admin"}, + {GroupDN: "*", OrgId: 1, OrgRole: "Viewer"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{{OrgId: 1, Role: m.ROLE_ADMIN}}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=admins"}, + }) + + Convey("Should take first match, and ignore subsequent matches", func() { + So(err, ShouldBeNil) + So(sc.updateOrgUserCmd, ShouldBeNil) + }) + }) + + ldapAutherScenario("given multiple matching ldap groups and no existing groups", func(sc *scenarioContext) { + ldapAuther := NewLdapAuthenticator(&LdapServerConf{ + LdapGroups: []*LdapGroupToOrgRole{ + {GroupDN: "cn=admins", OrgId: 1, OrgRole: "Admin"}, + {GroupDN: "*", OrgId: 1, OrgRole: "Viewer"}, + }, + }) + + sc.userOrgsQueryReturns([]*m.UserOrgDTO{}) + _, err := ldapAuther.GetGrafanaUserFor(nil, &LdapUserInfo{ + MemberOf: []string{"cn=admins"}, + }) + + Convey("Should take first match, and ignore subsequent matches", func() { + So(err, ShouldBeNil) + So(sc.addOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) + }) + }) + + }) + Convey("When calling SyncUser", t, func() { mockLdapConnection := &mockLdapConn{} From 00e74990ed0d583b20ff1b3aad6d6ed3ba6ef5c2 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 16 Apr 2018 17:44:39 -0400 Subject: [PATCH 16/20] remove old comment --- pkg/login/ldap_test.go | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 577a70bb1dc..2e4fcaa5fab 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -77,16 +77,6 @@ func TestLdapAuther(t *testing.T) { So(result.Login, ShouldEqual, "torkelo") }) - /* - Convey("Should create new user", func() { - So(sc.getUserByAuthInfoQuery.Login, ShouldEqual, "torkelo") - So(sc.getUserByAuthInfoQuery.Email, ShouldEqual, "my@email.com") - - So(sc.createUserCmd.Login, ShouldEqual, "torkelo") - So(sc.createUserCmd.Email, ShouldEqual, "my@email.com") - }) - */ - }) }) From 33760b5c3b5d5b0933bab2fd14c9607e75dfd137 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Tue, 17 Apr 2018 16:51:05 -0400 Subject: [PATCH 17/20] make sure user's default org is kept up to date --- pkg/login/ext_user.go | 18 ++++++++++++++++++ pkg/login/ldap_test.go | 11 +++++++++++ pkg/services/sqlstore/user.go | 9 +++++---- 3 files changed, 34 insertions(+), 4 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 66c39ca960f..17630357350 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -99,14 +99,17 @@ func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { if extUser.Login != "" && extUser.Login != user.Login { updateCmd.Login = extUser.Login + user.Login = extUser.Login needsUpdate = true } if extUser.Email != "" && extUser.Email != user.Email { updateCmd.Email = extUser.Email + user.Email = extUser.Email needsUpdate = true } if extUser.Name != "" && extUser.Name != user.Name { updateCmd.Name = extUser.Name + user.Name = extUser.Name needsUpdate = true } @@ -172,5 +175,20 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { } } + // update user's default org if needed + if _, ok := extUser.OrgRoles[user.OrgId]; !ok { + for orgId := range extUser.OrgRoles { + user.OrgId = orgId + break + } + err := bus.Dispatch(&m.SetUsingOrgCommand{ + UserId: user.Id, + OrgId: user.OrgId, + }) + if err != nil { + return err + } + } + return nil } diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 2e4fcaa5fab..6ad7c261cc6 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -118,6 +118,7 @@ func TestLdapAuther(t *testing.T) { So(err, ShouldBeNil) So(sc.updateOrgUserCmd, ShouldNotBeNil) So(sc.updateOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) }) }) @@ -139,6 +140,7 @@ func TestLdapAuther(t *testing.T) { Convey("Should remove org role", func() { So(err, ShouldBeNil) So(sc.removeOrgUserCmd, ShouldNotBeNil) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) }) }) @@ -159,6 +161,7 @@ func TestLdapAuther(t *testing.T) { So(err, ShouldBeNil) So(sc.removeOrgUserCmd, ShouldBeNil) So(sc.updateOrgUserCmd, ShouldNotBeNil) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) }) }) @@ -178,6 +181,7 @@ func TestLdapAuther(t *testing.T) { Convey("Should take first match, and ignore subsequent matches", func() { So(err, ShouldBeNil) So(sc.updateOrgUserCmd, ShouldBeNil) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) }) }) @@ -197,6 +201,7 @@ func TestLdapAuther(t *testing.T) { Convey("Should take first match, and ignore subsequent matches", func() { So(err, ShouldBeNil) So(sc.addOrgUserCmd.Role, ShouldEqual, m.ROLE_ADMIN) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) }) }) @@ -340,6 +345,11 @@ func ldapAutherScenario(desc string, fn scenarioFunc) { return nil }) + bus.AddHandler("test", func(cmd *m.SetUsingOrgCommand) error { + sc.setUsingOrgCmd = cmd + return nil + }) + fn(sc) }) } @@ -352,6 +362,7 @@ type scenarioContext struct { updateOrgUserCmd *m.UpdateOrgUserCommand removeOrgUserCmd *m.RemoveOrgUserCommand updateUserCmd *m.UpdateUserCommand + setUsingOrgCmd *m.SetUsingOrgCommand } func (sc *scenarioContext) userQueryReturns(user *m.User) { diff --git a/pkg/services/sqlstore/user.go b/pkg/services/sqlstore/user.go index 996cc47fc1c..547c0e5a3ce 100644 --- a/pkg/services/sqlstore/user.go +++ b/pkg/services/sqlstore/user.go @@ -295,11 +295,12 @@ func SetUsingOrg(cmd *m.SetUsingOrgCommand) error { } return inTransaction(func(sess *DBSession) error { - user := m.User{} - sess.Id(cmd.UserId).Get(&user) + user := m.User{ + Id: cmd.UserId, + OrgId: cmd.OrgId, + } - user.OrgId = cmd.OrgId - _, err := sess.Id(user.Id).Update(&user) + _, err := sess.Id(cmd.UserId).Update(&user) return err }) } From 3fedcb1e4beca613b70cb19b50e5411ae29d07e6 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Tue, 17 Apr 2018 17:48:56 -0400 Subject: [PATCH 18/20] cleanup, make sure users are always synced with ldap --- pkg/login/ext_user.go | 7 +------ pkg/middleware/auth_proxy.go | 2 ++ 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 17630357350..253bbec514c 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -68,12 +68,7 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { } } - err = syncOrgRoles(cmd.Result, extUser) - if err != nil { - return err - } - - return nil + return syncOrgRoles(cmd.Result, extUser) } func createUser(extUser *m.ExternalUserInfo) (*m.User, error) { diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index c4e28252964..36b059e4ae7 100644 --- a/pkg/middleware/auth_proxy.go +++ b/pkg/middleware/auth_proxy.go @@ -65,6 +65,8 @@ func initContextWithAuthProxy(ctx *m.ReqContext, orgID int64) bool { query.UserId = getRequestUserId(ctx) // if we're using ldap, pass authproxy login name to ldap user sync } else if setting.LdapEnabled { + ctx.Session.Delete(session.SESS_KEY_LASTLDAPSYNC) + syncQuery := &m.LoginUserQuery{ ReqContext: ctx, Username: proxyHeaderValue, From d9b3ff1952290440dfa7f7d5ba5eec8aaeffc0c2 Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 18 Apr 2018 09:21:51 +0200 Subject: [PATCH 19/20] changelog: adds note about closing #11613 and #11602 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 627e9bfe5af..2d30ed5d8bd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,8 @@ * **Permission list**: Improved ux [#10747](https://github.com/grafana/grafana/issues/10747) * **Dashboard**: Sizing and positioning of settings menu icons [#11572](https://github.com/grafana/grafana/pull/11572) * **Folders**: User with org viewer role should not be able to save/move dashboards in/to general folder [#11553](https://github.com/grafana/grafana/issues/11553) +* **Tech**: Backend code simplification [#11613](https://github.com/grafana/grafana/pull/11613), thx [@knweiss](https://github.com/knweiss) +* **Tech**: Add codespell to CI [#11602](https://github.com/grafana/grafana/pull/11602), thx [@mjtrangoni](https://github.com/mjtrangoni) ### Tech * Migrated JavaScript files to TypeScript From dbf61355b355bc2c6099a843a31b0559880b3d0b Mon Sep 17 00:00:00 2001 From: bergquist Date: Wed, 18 Apr 2018 12:07:28 +0200 Subject: [PATCH 20/20] style: code simplifications --- pkg/login/ext_user.go | 35 +++++++++++++++-------------------- pkg/login/ldap_test.go | 4 ++-- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/pkg/login/ext_user.go b/pkg/login/ext_user.go index 253bbec514c..e1d5e3e3b48 100644 --- a/pkg/login/ext_user.go +++ b/pkg/login/ext_user.go @@ -1,8 +1,6 @@ package login import ( - "fmt" - "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" @@ -24,13 +22,13 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { Login: extUser.Login, } err := bus.Dispatch(userQuery) - if err != nil { - if err != m.ErrUserNotFound { - return err - } + if err != m.ErrUserNotFound && err != nil { + return err + } + if err != nil { if !cmd.SignupAllowed { - log.Warn(fmt.Sprintf("Not allowing %s login, user not found in internal user database and allow signup = false", extUser.AuthModule)) + log.Warn("Not allowing %s login, user not found in internal user database and allow signup = false", extUser.AuthModule) return ErrInvalidCredentials } @@ -58,10 +56,10 @@ func UpsertUser(cmd *m.UpsertUserCommand) error { return err } } + } else { cmd.Result = userQuery.Result - // sync user info err = updateUser(cmd.Result, extUser) if err != nil { return err @@ -90,33 +88,32 @@ func updateUser(user *m.User, extUser *m.ExternalUserInfo) error { updateCmd := &m.UpdateUserCommand{ UserId: user.Id, } - needsUpdate := false + needsUpdate := false if extUser.Login != "" && extUser.Login != user.Login { updateCmd.Login = extUser.Login user.Login = extUser.Login needsUpdate = true } + if extUser.Email != "" && extUser.Email != user.Email { updateCmd.Email = extUser.Email user.Email = extUser.Email needsUpdate = true } + if extUser.Name != "" && extUser.Name != user.Name { updateCmd.Name = extUser.Name user.Name = extUser.Name needsUpdate = true } - if needsUpdate { - log.Debug("Syncing user info", "id", user.Id, "update", updateCmd) - err := bus.Dispatch(updateCmd) - if err != nil { - return err - } + if !needsUpdate { + return nil } - return nil + log.Debug("Syncing user info", "id", user.Id, "update", updateCmd) + return bus.Dispatch(updateCmd) } func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { @@ -176,13 +173,11 @@ func syncOrgRoles(user *m.User, extUser *m.ExternalUserInfo) error { user.OrgId = orgId break } - err := bus.Dispatch(&m.SetUsingOrgCommand{ + + return bus.Dispatch(&m.SetUsingOrgCommand{ UserId: user.Id, OrgId: user.OrgId, }) - if err != nil { - return err - } } return nil diff --git a/pkg/login/ldap_test.go b/pkg/login/ldap_test.go index 6ad7c261cc6..6085fffb638 100644 --- a/pkg/login/ldap_test.go +++ b/pkg/login/ldap_test.go @@ -125,7 +125,7 @@ func TestLdapAuther(t *testing.T) { ldapAutherScenario("given current org role is removed in ldap", func(sc *scenarioContext) { ldapAuther := NewLdapAuthenticator(&LdapServerConf{ LdapGroups: []*LdapGroupToOrgRole{ - {GroupDN: "cn=users", OrgId: 1, OrgRole: "Admin"}, + {GroupDN: "cn=users", OrgId: 2, OrgRole: "Admin"}, }, }) @@ -140,7 +140,7 @@ func TestLdapAuther(t *testing.T) { Convey("Should remove org role", func() { So(err, ShouldBeNil) So(sc.removeOrgUserCmd, ShouldNotBeNil) - So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 1) + So(sc.setUsingOrgCmd.OrgId, ShouldEqual, 2) }) })