diff --git a/pkg/middleware/auth_proxy.go b/pkg/middleware/auth_proxy.go index 2365df81852..4c5f9d22c48 100644 --- a/pkg/middleware/auth_proxy.go +++ b/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 } diff --git a/pkg/middleware/authproxy/auth_proxy.go b/pkg/middleware/authproxy/auth_proxy.go index 0e0267721a9..2b80e0f8fd4 100644 --- a/pkg/middleware/authproxy/auth_proxy.go +++ b/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 diff --git a/pkg/middleware/authproxy/auth_proxy_test.go b/pkg/middleware/authproxy/auth_proxy_test.go index a6625407af8..5a3604946e0 100644 --- a/pkg/middleware/authproxy/auth_proxy_test.go +++ b/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) }) } diff --git a/pkg/services/ldap/settings.go b/pkg/services/ldap/settings.go index 19d6dd2c872..fa4248108e3 100644 --- a/pkg/services/ldap/settings.go +++ b/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) {