Auth proxy: Return standard error type (#29502)

* Rewrite auth proxy tests to use standard lib

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

* Auth proxy: Use standard error type

Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

pkg/middleware/auth_proxy.go

@@ -12,18 +12,41 @@ import (
 
 var header = setting.AuthProxyHeaderName
 
-func logUserIn(auth *authproxy.AuthProxy, username string, logger log.Logger, ignoreCache bool) (int64, *authproxy.Error) {
+func logUserIn(auth *authproxy.AuthProxy, username string, logger log.Logger, ignoreCache bool) (int64, error) {
 	logger.Debug("Trying to log user in", "username", username, "ignoreCache", ignoreCache)
 	// Try to log in user via various providers
-	id, e := auth.Login(logger, ignoreCache)
-	if e != nil {
-		logger.Error("Failed to login", "username", username, "message", e.Error(), "error", e.DetailsError,
+	id, err := auth.Login(logger, ignoreCache)
+	if err != nil {
+		details := err
+		var e authproxy.Error
+		if errors.As(err, &e) {
+			details = e.DetailsError
+		}
+		logger.Error("Failed to login", "username", username, "message", err.Error(), "error", details,
 			"ignoreCache", ignoreCache)
-		return 0, e
+		return 0, err
 	}
 	return id, nil
 }
 
+// handleError calls ctx.Handle with the error message and the underlying error.
+// If the error is of type authproxy.Error, its DetailsError is unwrapped and passed to ctx.Handle.
+// If a callback is provided, it's called with either err.DetailsError, if err is of type
+// authproxy.Error, otherwise err itself.
+func handleError(ctx *models.ReqContext, err error, statusCode int, cb func(err error)) {
+	details := err
+	var e authproxy.Error
+	if errors.As(err, &e) {
+		details = e.DetailsError
+	}
+
+	ctx.Handle(statusCode, err.Error(), details)
+
+	if cb != nil {
+		cb(details)
+	}
+}
+
 func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *models.ReqContext, orgID int64) bool {
 	username := ctx.Req.Header.Get(header)
 	auth := authproxy.New(&authproxy.Options{
@@ -46,18 +69,15 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *models.ReqCon
 
 	// Check if allowed to continue with this IP
 	if err := auth.IsAllowedIP(); err != nil {
-		logger.Error(
-			"Failed to check whitelisted IP addresses",
-			"message", err.Error(),
-			"error", err.DetailsError,
-		)
-		ctx.Handle(407, err.Error(), err.DetailsError)
+		handleError(ctx, err, 407, func(details error) {
+			logger.Error("Failed to check whitelisted IP addresses", "message", err.Error(), "error", details)
+		})
 		return true
 	}
 
-	id, e := logUserIn(auth, username, logger, false)
-	if e != nil {
-		ctx.Handle(407, e.Error(), e.DetailsError)
+	id, err := logUserIn(auth, username, logger, false)
+	if err != nil {
+		handleError(ctx, err, 407, nil)
 		return true
 	}
 
@@ -76,15 +96,16 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *models.ReqCon
 				logger.Error("Got unexpected error when removing user from auth cache", "error", err)
 			}
 		}
-		id, e = logUserIn(auth, username, logger, true)
-		if e != nil {
-			ctx.Handle(407, e.Error(), e.DetailsError)
+		id, err = logUserIn(auth, username, logger, true)
+		if err != nil {
+			handleError(ctx, err, 407, nil)
 			return true
 		}
 
-		user, e = auth.GetSignedUser(id)
-		if e != nil {
-			ctx.Handle(407, e.Error(), e.DetailsError)
+		user, err = auth.GetSignedUser(id)
+		if err != nil {
+			handleError(ctx, err, 407, nil)
+
 			return true
 		}
 	}
@@ -96,14 +117,15 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *models.ReqCon
 	ctx.IsSignedIn = true
 
 	// Remember user data in cache
-	if e := auth.Remember(id); e != nil {
-		logger.Error(
-			"Failed to store user in cache",
-			"username", username,
-			"message", e.Error(),
-			"error", e.DetailsError,
-		)
-		ctx.Handle(500, e.Error(), e.DetailsError)
+	if err := auth.Remember(id); err != nil {
+		handleError(ctx, err, 500, func(details error) {
+			logger.Error(
+				"Failed to store user in cache",
+				"username", username,
+				"message", e.Error(),
+				"error", details,
+			)
+		})
 		return true
 	}
 

pkg/middleware/authproxy/auth_proxy.go

@@ -62,16 +62,16 @@ type Error struct {
 	DetailsError error
 }
 
-// newError creates the Error
-func newError(message string, err error) *Error {
-	return &Error{
+// newError returns an Error.
+func newError(message string, err error) Error {
+	return Error{
 		Message:      message,
 		DetailsError: err,
 	}
 }
 
-// Error returns a Error error string
-func (err *Error) Error() string {
+// Error returns the error message.
+func (err Error) Error() string {
 	return err.Message
 }
 
@@ -114,7 +114,7 @@ func (auth *AuthProxy) HasHeader() bool {
 }
 
 // IsAllowedIP compares presented IP with the whitelist one
-func (auth *AuthProxy) IsAllowedIP() *Error {
+func (auth *AuthProxy) IsAllowedIP() error {
 	ip := auth.ctx.Req.RemoteAddr
 
 	if len(strings.TrimSpace(auth.whitelistIP)) == 0 {
@@ -144,11 +144,10 @@ func (auth *AuthProxy) IsAllowedIP() *Error {
 		}
 	}
 
-	err = fmt.Errorf(
+	return newError("proxy authentication required", fmt.Errorf(
 		"request for user (%s) from %s is not from the authentication proxy", auth.header,
 		sourceIP,
-	)
-	return newError("proxy authentication required", err)
+	))
 }
 
 func HashCacheKey(key string) string {
@@ -173,7 +172,7 @@ func (auth *AuthProxy) getKey() string {
 }
 
 // Login logs in user ID by whatever means possible.
-func (auth *AuthProxy) Login(logger log.Logger, ignoreCache bool) (int64, *Error) {
+func (auth *AuthProxy) Login(logger log.Logger, ignoreCache bool) (int64, error) {
 	if !ignoreCache {
 		// Error here means absent cache - we don't need to handle that
 		id, err := auth.GetUserViaCache(logger)
@@ -229,15 +228,15 @@ func (auth *AuthProxy) RemoveUserFromCache(logger log.Logger) error {
 }
 
 // LoginViaLDAP logs in user via LDAP request
-func (auth *AuthProxy) LoginViaLDAP() (int64, *Error) {
+func (auth *AuthProxy) LoginViaLDAP() (int64, error) {
 	config, err := getLDAPConfig()
 	if err != nil {
-		return 0, newError("failed to get LDAP config", nil)
+		return 0, newError("failed to get LDAP config", err)
 	}
 
 	extUser, _, err := newLDAP(config.Servers).User(auth.header)
 	if err != nil {
-		return 0, newError(err.Error(), nil)
+		return 0, err
 	}
 
 	// Have to sync grafana and LDAP user during log in
@@ -246,9 +245,8 @@ func (auth *AuthProxy) LoginViaLDAP() (int64, *Error) {
 		SignupAllowed: auth.LDAPAllowSignup,
 		ExternalUser:  extUser,
 	}
-	err = bus.Dispatch(upsert)
-	if err != nil {
-		return 0, newError(err.Error(), nil)
+	if err := bus.Dispatch(upsert); err != nil {
+		return 0, err
 	}
 
 	return upsert.Result.Id, nil
@@ -273,7 +271,7 @@ func (auth *AuthProxy) LoginViaHeader() (int64, error) {
 		extUser.Email = auth.header
 		extUser.Login = auth.header
 	default:
-		return 0, newError("auth proxy header property invalid", nil)
+		return 0, fmt.Errorf("auth proxy header property invalid")
 	}
 
 	auth.headersIterator(func(field string, header string) {
@@ -314,21 +312,21 @@ func (auth *AuthProxy) headersIterator(fn func(field string, header string)) {
 }
 
 // GetSignedUser gets full signed user info.
-func (auth *AuthProxy) GetSignedUser(userID int64) (*models.SignedInUser, *Error) {
+func (auth *AuthProxy) GetSignedUser(userID int64) (*models.SignedInUser, error) {
 	query := &models.GetSignedInUserQuery{
 		OrgId:  auth.orgID,
 		UserId: userID,
 	}
 
 	if err := bus.Dispatch(query); err != nil {
-		return nil, newError(err.Error(), nil)
+		return nil, err
 	}
 
 	return query.Result, nil
 }
 
 // Remember user in cache
-func (auth *AuthProxy) Remember(id int64) *Error {
+func (auth *AuthProxy) Remember(id int64) error {
 	key := auth.getKey()
 
 	// Check if user already in cache
@@ -341,7 +339,7 @@ func (auth *AuthProxy) Remember(id int64) *Error {
 
 	err := auth.store.Set(key, id, expiration)
 	if err != nil {
-		return newError(err.Error(), nil)
+		return err
 	}
 
 	return nil

pkg/middleware/authproxy/auth_proxy_test.go

@@ -13,7 +13,8 @@ import (
 	"github.com/grafana/grafana/pkg/services/ldap"
 	"github.com/grafana/grafana/pkg/services/multildap"
 	"github.com/grafana/grafana/pkg/setting"
-	. "github.com/smartystreets/goconvey/convey"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gopkg.in/macaron.v1"
 )
 
@@ -68,134 +69,142 @@ func prepareMiddleware(t *testing.T, req *http.Request, store *remotecache.Remot
 
 func TestMiddlewareContext(t *testing.T) {
 	logger := log.New("test")
-	Convey("auth_proxy helper", t, func() {
-		req, err := http.NewRequest("POST", "http://example.com", nil)
-		So(err, ShouldBeNil)
-		setting.AuthProxyHeaderName = "X-Killa"
-		store := remotecache.NewFakeStore(t)
+	req, err := http.NewRequest("POST", "http://example.com", nil)
+	require.NoError(t, err)
+	setting.AuthProxyHeaderName = "X-Killa"
+	store := remotecache.NewFakeStore(t)
 
-		name := "markelog"
-		req.Header.Add(setting.AuthProxyHeaderName, name)
+	name := "markelog"
+	req.Header.Add(setting.AuthProxyHeaderName, name)
 
-		Convey("when the cache only contains the main header", func() {
-			Convey("with a simple cache key", func() {
-				// Set cache key
-				key := fmt.Sprintf(CachePrefix, HashCacheKey(name))
-				err := store.Set(key, int64(33), 0)
-				So(err, ShouldBeNil)
+	t.Run("When the cache only contains the main header with a simple cache key", func(t *testing.T) {
+		const id int64 = 33
+		// Set cache key
+		key := fmt.Sprintf(CachePrefix, HashCacheKey(name))
+		err := store.Set(key, id, 0)
+		require.NoError(t, err)
 
-				// Set up the middleware
-				auth := prepareMiddleware(t, req, store)
-				So(auth.getKey(), ShouldEqual, "auth-proxy-sync-ttl:0a7f3374e9659b10980fd66247b0cf2f")
+		// Set up the middleware
+		auth := prepareMiddleware(t, req, store)
+		assert.Equal(t, "auth-proxy-sync-ttl:0a7f3374e9659b10980fd66247b0cf2f", auth.getKey())
 
-				id, err := auth.Login(logger, false)
-				So(err, ShouldBeNil)
+		gotID, err := auth.Login(logger, false)
+		require.NoError(t, err)
 
-				So(id, ShouldEqual, 33)
-			})
+		assert.Equal(t, id, gotID)
+	})
 
-			Convey("when the cache key contains additional headers", func() {
-				setting.AuthProxyHeaders = map[string]string{"Groups": "X-WEBAUTH-GROUPS"}
-				group := "grafana-core-team"
-				req.Header.Add("X-WEBAUTH-GROUPS", group)
+	t.Run("When the cache key contains additional headers", func(t *testing.T) {
+		const id int64 = 33
+		setting.AuthProxyHeaders = map[string]string{"Groups": "X-WEBAUTH-GROUPS"}
+		group := "grafana-core-team"
+		req.Header.Add("X-WEBAUTH-GROUPS", group)
 
-				key := fmt.Sprintf(CachePrefix, HashCacheKey(name+"-"+group))
-				err := store.Set(key, int64(33), 0)
-				So(err, ShouldBeNil)
+		key := fmt.Sprintf(CachePrefix, HashCacheKey(name+"-"+group))
+		err := store.Set(key, id, 0)
+		require.NoError(t, err)
 
-				auth := prepareMiddleware(t, req, store)
-				So(auth.getKey(), ShouldEqual, "auth-proxy-sync-ttl:14f69b7023baa0ac98c96b31cec07bc0")
+		auth := prepareMiddleware(t, req, store)
+		assert.Equal(t, "auth-proxy-sync-ttl:14f69b7023baa0ac98c96b31cec07bc0", auth.getKey())
 
-				id, err := auth.Login(logger, false)
-				So(err, ShouldBeNil)
-				So(id, ShouldEqual, 33)
-			})
-		})
-
-		Convey("LDAP", func() {
-			Convey("logs in via LDAP", func() {
-				bus.AddHandler("test", func(cmd *models.UpsertUserCommand) error {
-					cmd.Result = &models.User{
-						Id: 42,
-					}
-
-					return nil
-				})
-
-				isLDAPEnabled = func() bool {
-					return true
-				}
-
-				stub := &fakeMultiLDAP{
-					ID: 42,
-				}
-
-				getLDAPConfig = func() (*ldap.Config, error) {
-					config := &ldap.Config{
-						Servers: []*ldap.ServerConfig{
-							{
-								SearchBaseDNs: []string{"BaseDNHere"},
-							},
-						},
-					}
-					return config, nil
-				}
-
-				newLDAP = func(servers []*ldap.ServerConfig) multildap.IMultiLDAP {
-					return stub
-				}
-
-				defer func() {
-					newLDAP = multildap.New
-					isLDAPEnabled = ldap.IsEnabled
-					getLDAPConfig = ldap.GetConfig
-				}()
-
-				store := remotecache.NewFakeStore(t)
-
-				auth := prepareMiddleware(t, req, store)
-
-				id, err := auth.Login(logger, false)
-
-				So(err, ShouldBeNil)
-				So(id, ShouldEqual, 42)
-				So(stub.userCalled, ShouldEqual, true)
-			})
-
-			Convey("gets nice error if ldap is enabled but not configured", func() {
-				isLDAPEnabled = func() bool {
-					return true
-				}
-
-				getLDAPConfig = func() (*ldap.Config, error) {
-					return nil, errors.New("Something went wrong")
-				}
-
-				defer func() {
-					newLDAP = multildap.New
-					isLDAPEnabled = ldap.IsEnabled
-					getLDAPConfig = ldap.GetConfig
-				}()
-
-				store := remotecache.NewFakeStore(t)
-
-				auth := prepareMiddleware(t, req, store)
-
-				stub := &fakeMultiLDAP{
-					ID: 42,
-				}
-
-				newLDAP = func(servers []*ldap.ServerConfig) multildap.IMultiLDAP {
-					return stub
-				}
-
-				id, err := auth.Login(logger, false)
-
-				So(err, ShouldNotBeNil)
-				So(err.Error(), ShouldContainSubstring, "failed to get the user")
-				So(id, ShouldNotEqual, 42)
-				So(stub.loginCalled, ShouldEqual, false)
-			})
-		})
+		gotID, err := auth.Login(logger, false)
+		require.NoError(t, err)
+		assert.Equal(t, id, gotID)
+	})
+}
+
+func TestMiddlewareContext_ldap(t *testing.T) {
+	logger := log.New("test")
+	req, err := http.NewRequest("POST", "http://example.com", nil)
+	require.NoError(t, err)
+	setting.AuthProxyHeaderName = "X-Killa"
+
+	const headerName = "markelog"
+	req.Header.Add(setting.AuthProxyHeaderName, headerName)
+
+	t.Run("Logs in via LDAP", func(t *testing.T) {
+		const id int64 = 42
+
+		bus.AddHandler("test", func(cmd *models.UpsertUserCommand) error {
+			cmd.Result = &models.User{
+				Id: id,
+			}
+
+			return nil
+		})
+
+		isLDAPEnabled = func() bool {
+			return true
+		}
+
+		stub := &fakeMultiLDAP{
+			ID: id,
+		}
+
+		getLDAPConfig = func() (*ldap.Config, error) {
+			config := &ldap.Config{
+				Servers: []*ldap.ServerConfig{
+					{
+						SearchBaseDNs: []string{"BaseDNHere"},
+					},
+				},
+			}
+			return config, nil
+		}
+
+		newLDAP = func(servers []*ldap.ServerConfig) multildap.IMultiLDAP {
+			return stub
+		}
+
+		defer func() {
+			newLDAP = multildap.New
+			isLDAPEnabled = ldap.IsEnabled
+			getLDAPConfig = ldap.GetConfig
+		}()
+
+		store := remotecache.NewFakeStore(t)
+
+		auth := prepareMiddleware(t, req, store)
+
+		gotID, err := auth.Login(logger, false)
+		require.NoError(t, err)
+
+		assert.Equal(t, id, gotID)
+		assert.True(t, stub.userCalled)
+	})
+
+	t.Run("Gets nice error if ldap is enabled but not configured", func(t *testing.T) {
+		const id int64 = 42
+		isLDAPEnabled = func() bool {
+			return true
+		}
+
+		getLDAPConfig = func() (*ldap.Config, error) {
+			return nil, errors.New("something went wrong")
+		}
+
+		defer func() {
+			newLDAP = multildap.New
+			isLDAPEnabled = ldap.IsEnabled
+			getLDAPConfig = ldap.GetConfig
+		}()
+
+		store := remotecache.NewFakeStore(t)
+
+		auth := prepareMiddleware(t, req, store)
+
+		stub := &fakeMultiLDAP{
+			ID: id,
+		}
+
+		newLDAP = func(servers []*ldap.ServerConfig) multildap.IMultiLDAP {
+			return stub
+		}
+
+		gotID, err := auth.Login(logger, false)
+		require.EqualError(t, err, "failed to get the user")
+
+		assert.NotEqual(t, id, gotID)
+		assert.False(t, stub.loginCalled)
 	})
 }

pkg/services/ldap/settings.go

@@ -107,10 +107,7 @@ func GetConfig() (*Config, error) {
 	loadingMutex.Lock()
 	defer loadingMutex.Unlock()
 
-	var err error
-	config, err = readConfig(setting.LDAPConfigFile)
-
-	return config, err
+	return readConfig(setting.LDAPConfigFile)
 }
 
 func readConfig(configFile string) (*Config, error) {