refactor authproxy & ldap integration, address comments

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,
+		Login:        extUser.Login,
-		Email: extUser.Email,
+		Email:        extUser.Email,
-		Name:  extUser.Name,
+		Name:         extUser.Name,
+		SkipOrgSetup: len(extUser.OrgRoles) > 0,
 	}
 	if err := bus.Dispatch(cmd); err != nil {
 		return nil, err

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
+	query.User = grafanaUser
-	signedInUser.Email = grafanaUser.Email
-	signedInUser.Name = grafanaUser.Name
-
 	return nil
 }
 

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
 }
 

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{
+			query := &m.LoginUserQuery{
-				Email:  "roel@test.net",
+				Username: "roelgerrits",
-				UserId: 1,
-				Name:   "Roel Gerrits",
-				Login:  "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)

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 {
-		query.UserId = getRequestUserId(ctx)
+		// if we're using ldap, sync user periodically
-		if err := bus.Dispatch(query); err != nil {
+		if setting.LdapEnabled {
-			ctx.Handle(500, "Failed to find user", err)
+			syncQuery := &m.LoginUserQuery{
-			return true
+				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 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)
 	}
 
-	// When ldap is enabled, sync userinfo and org roles
+	if err := bus.Dispatch(query); err != nil {
-	if err := syncGrafanaUserWithLdapUser(ctx, query); err != nil {
+		ctx.Handle(500, "Failed to find user", err)
-		if err == login.ErrInvalidCredentials {
+		return true
-			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)
+		// initialize a new session
-		return false
+		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 {
+var syncGrafanaUserWithLdapUser = func(query *m.LoginUserQuery) error {
-	if !setting.LdapEnabled {
-		return nil
-	}
-
 	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

pkg/middleware/auth_proxy_test.go

@@ -26,57 +26,71 @@ func TestAuthProxyWithLdapEnabled(t *testing.T) {
 			return &mockLdapAuther
 		}
 
-		signedInUser := m.SignedInUser{}
+		Convey("When user logs in, call SyncUser", func() {
-		query := m.GetSignedInUserQuery{Result: &signedInUser}
-
-		Convey("When session variable lastLdapSync not set, call syncSignedInUser and set lastLdapSync", 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 {
+func (a *mockLdapAuthenticator) SyncUser(query *m.LoginUserQuery) error {
-	a.syncSignedInUserCalled = true
+	a.syncUserCalled = true
 	return nil
 }
 

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
 			}
 

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 != m.ErrUserNotFound {
-		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 != nil {
 				return err
 			}
 
-			if !has {
+			// if user id was specified and doesn't match the user_auth entry, remove it
-				// if the user has been deleted then remove the entry
+			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
 		}
 	}