Auth: change the error HTTP status codes (#18584)

* Auth: change the error HTTP status codes

* Use 407 HTTP status code for incorrect credentials error

* Improve proxy auth logs

* Remove no longer needed TODO comment

Fixes #18439

pkg/middleware/auth_proxy.go

@@ -1,9 +1,11 @@
 package middleware
 
 import (
+	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/remotecache"
 	authproxy "github.com/grafana/grafana/pkg/middleware/auth_proxy"
 	m "github.com/grafana/grafana/pkg/models"
+	"github.com/grafana/grafana/pkg/setting"
 )
 
 const (
@@ -12,13 +14,18 @@ const (
 	cachePrefix = authproxy.CachePrefix
 )
 
+var header = setting.AuthProxyHeaderName
+
 func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *m.ReqContext, orgID int64) bool {
+	username := ctx.Req.Header.Get(header)
 	auth := authproxy.New(&authproxy.Options{
 		Store: store,
 		Ctx:   ctx,
 		OrgID: orgID,
 	})
 
+	logger := log.New("auth.proxy")
+
 	// Bail if auth proxy is not enabled
 	if !auth.IsEnabled() {
 		return false
@@ -31,7 +38,11 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *m.ReqContext,
 
 	// Check if allowed to continue with this IP
 	if result, err := auth.IsAllowedIP(); !result {
-		ctx.Logger.Error("auth proxy: failed to check whitelisted ip addresses", "message", err.Error(), "error", err.DetailsError)
+		logger.Error(
+			"Failed to check whitelisted IP addresses",
+			"message", err.Error(),
+			"error", err.DetailsError,
+		)
 		ctx.Handle(407, err.Error(), err.DetailsError)
 		return true
 	}
@@ -39,16 +50,26 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *m.ReqContext,
 	// Try to log in user from various providers
 	id, err := auth.Login()
 	if err != nil {
-		ctx.Logger.Error("auth proxy: failed to login", "message", err.Error(), "error", err.DetailsError)
-		ctx.Handle(500, err.Error(), err.DetailsError)
+		logger.Error(
+			"Failed to login",
+			"username", username,
+			"message", err.Error(),
+			"error", err.DetailsError,
+		)
+		ctx.Handle(407, err.Error(), err.DetailsError)
 		return true
 	}
 
 	// Get full user info
 	user, err := auth.GetSignedUser(id)
 	if err != nil {
-		ctx.Logger.Error("auth proxy: failed to get signed in user", "message", err.Error(), "error", err.DetailsError)
-		ctx.Handle(500, err.Error(), err.DetailsError)
+		logger.Error(
+			"Failed to get signed user",
+			"username", username,
+			"message", err.Error(),
+			"error", err.DetailsError,
+		)
+		ctx.Handle(407, err.Error(), err.DetailsError)
 		return true
 	}
 
@@ -58,7 +79,12 @@ func initContextWithAuthProxy(store *remotecache.RemoteCache, ctx *m.ReqContext,
 
 	// Remember user data it in cache
 	if err := auth.Remember(id); err != nil {
-		ctx.Logger.Error("auth proxy: failed to store user in cache", "message", err.Error(), "error", err.DetailsError)
+		logger.Error(
+			"Failed to store user in cache",
+			"username", username,
+			"message", err.Error(),
+			"error", err.DetailsError,
+		)
 		ctx.Handle(500, err.Error(), err.DetailsError)
 		return true
 	}

pkg/middleware/auth_proxy/auth_proxy.go

@@ -238,7 +238,6 @@ func (auth *AuthProxy) LoginViaLDAP() (int64, *Error) {
 }
 
 // LoginViaHeader logs in user from the header only
-// TODO: refactor - cyclomatic complexity should be much lower
 func (auth *AuthProxy) LoginViaHeader() (int64, error) {
 	extUser := &models.ExternalUserInfo{
 		AuthModule: "authproxy",

pkg/middleware/middleware_test.go

@@ -3,6 +3,7 @@ package middleware
 import (
 	"context"
 	"encoding/base32"
+	"errors"
 	"fmt"
 	"net/http"
 	"path/filepath"
@@ -374,7 +375,7 @@ func TestMiddlewareContext(t *testing.T) {
 				sc.exec()
 
 				assert.False(t, *actualAuthProxyAutoSignUp)
-				assert.Equal(t, sc.resp.Code, 500)
+				assert.Equal(t, sc.resp.Code, 407)
 				assert.Nil(t, sc.context)
 			})
 
@@ -480,6 +481,36 @@ func TestMiddlewareContext(t *testing.T) {
 					So(sc.context, ShouldBeNil)
 				})
 			})
+
+			middlewareScenario(t, "Should return 407 status code if LDAP says no", func(sc *scenarioContext) {
+				bus.AddHandler("LDAP", func(cmd *models.UpsertUserCommand) error {
+					return errors.New("Do not add user")
+				})
+
+				sc.fakeReq("GET", "/")
+				sc.req.Header.Add(setting.AuthProxyHeaderName, name)
+				sc.exec()
+
+				Convey("Should return 407 status code", func() {
+					So(sc.resp.Code, ShouldEqual, 407)
+					So(sc.context, ShouldBeNil)
+				})
+			})
+
+			middlewareScenario(t, "Should return 407 status code if there is cache mishap", func(sc *scenarioContext) {
+				bus.AddHandler("Do not have the user", func(query *models.GetSignedInUserQuery) error {
+					return errors.New("Do not add user")
+				})
+
+				sc.fakeReq("GET", "/")
+				sc.req.Header.Add(setting.AuthProxyHeaderName, name)
+				sc.exec()
+
+				Convey("Should return 407 status code", func() {
+					So(sc.resp.Code, ShouldEqual, 407)
+					So(sc.context, ShouldBeNil)
+				})
+			})
 		})
 	})
 }