From 82661b9f692658985d6a49d14c3508706c3017bd Mon Sep 17 00:00:00 2001 From: Oleg Gaidarenko Date: Fri, 2 Aug 2019 12:16:31 +0300 Subject: [PATCH] Auth: consistently return same basic auth errors (#18310) * Auth: consistently return same basic auth errors * Put repeated errors in consts and return only those consts as error strings * Add tests for errors basic auth cases and moves tests to separate test-case. Also names test cases consistently * Add more error logs and makes their messages consistent * A bit of code style * Add additional test helper * Auth: do not expose even incorrect password * Auth: address review comments Use `Debug` for the cases when it's an user error --- pkg/middleware/middleware.go | 59 ++++++++++++++++++++++--------- pkg/middleware/middleware_test.go | 7 ++-- 2 files changed, 46 insertions(+), 20 deletions(-) diff --git a/pkg/middleware/middleware.go b/pkg/middleware/middleware.go index d4a0b2da2aa..2bb0f8a49d5 100644 --- a/pkg/middleware/middleware.go +++ b/pkg/middleware/middleware.go @@ -21,11 +21,19 @@ import ( var getTime = time.Now +const ( + errStringInvalidUsernamePassword = "Invalid username or password" + errStringInvalidAPIKey = "Invalid API key" +) + var ( - ReqGrafanaAdmin = Auth(&AuthOptions{ReqSignedIn: true, ReqGrafanaAdmin: true}) - ReqSignedIn = Auth(&AuthOptions{ReqSignedIn: true}) - ReqEditorRole = RoleAuth(models.ROLE_EDITOR, models.ROLE_ADMIN) - ReqOrgAdmin = RoleAuth(models.ROLE_ADMIN) + ReqGrafanaAdmin = Auth(&AuthOptions{ + ReqSignedIn: true, + ReqGrafanaAdmin: true, + }) + ReqSignedIn = Auth(&AuthOptions{ReqSignedIn: true}) + ReqEditorRole = RoleAuth(models.ROLE_EDITOR, models.ROLE_ADMIN) + ReqOrgAdmin = RoleAuth(models.ROLE_ADMIN) ) func GetContextHandler( @@ -106,14 +114,14 @@ func initContextWithApiKey(ctx *models.ReqContext) bool { // base64 decode key decoded, err := apikeygen.Decode(keyString) if err != nil { - ctx.JsonApiErr(401, "Invalid API key", err) + ctx.JsonApiErr(401, errStringInvalidAPIKey, err) return true } // fetch key keyQuery := models.GetApiKeyByNameQuery{KeyName: decoded.Name, OrgId: decoded.OrgId} if err := bus.Dispatch(&keyQuery); err != nil { - ctx.JsonApiErr(401, "Invalid API key", err) + ctx.JsonApiErr(401, errStringInvalidAPIKey, err) return true } @@ -121,7 +129,7 @@ func initContextWithApiKey(ctx *models.ReqContext) bool { // validate api key if !apikeygen.IsValid(decoded, apikey.Key) { - ctx.JsonApiErr(401, "Invalid API key", err) + ctx.JsonApiErr(401, errStringInvalidAPIKey, err) return true } @@ -140,7 +148,6 @@ func initContextWithApiKey(ctx *models.ReqContext) bool { } func initContextWithBasicAuth(ctx *models.ReqContext, orgId int64) bool { - if !setting.BasicAuthEnabled { return false } @@ -158,21 +165,39 @@ func initContextWithBasicAuth(ctx *models.ReqContext, orgId int64) bool { loginQuery := models.GetUserByLoginQuery{LoginOrEmail: username} if err := bus.Dispatch(&loginQuery); err != nil { - ctx.JsonApiErr(401, "Basic auth failed", err) + ctx.Logger.Debug( + "Failed to look up the username", + "username", username, + ) + ctx.JsonApiErr(401, errStringInvalidUsernamePassword, err) + return true } user := loginQuery.Result - - loginUserQuery := models.LoginUserQuery{Username: username, Password: password, User: user} + loginUserQuery := models.LoginUserQuery{ + Username: username, + Password: password, + User: user, + } if err := bus.Dispatch(&loginUserQuery); err != nil { - ctx.JsonApiErr(401, "Invalid username or password", err) + ctx.Logger.Debug( + "Failed to authorize the user", + "username", username, + ) + + ctx.JsonApiErr(401, errStringInvalidUsernamePassword, err) return true } query := models.GetSignedInUserQuery{UserId: user.Id, OrgId: orgId} if err := bus.Dispatch(&query); err != nil { - ctx.JsonApiErr(401, "Authentication error", err) + ctx.Logger.Error( + "Failed at user signed in", + "id", user.Id, + "org", orgId, + ) + ctx.JsonApiErr(401, errStringInvalidUsernamePassword, err) return true } @@ -193,14 +218,14 @@ func initContextWithToken(authTokenService models.UserTokenService, ctx *models. token, err := authTokenService.LookupToken(ctx.Req.Context(), rawToken) if err != nil { - ctx.Logger.Error("failed to look up user based on cookie", "error", err) + ctx.Logger.Error("Failed to look up user based on cookie", "error", err) WriteSessionCookie(ctx, "", -1) return false } query := models.GetSignedInUserQuery{UserId: token.UserId, OrgId: orgID} if err := bus.Dispatch(&query); err != nil { - ctx.Logger.Error("failed to get user with id", "userId", token.UserId, "error", err) + ctx.Logger.Error("Failed to get user with id", "userId", token.UserId, "error", err) return false } @@ -210,7 +235,7 @@ func initContextWithToken(authTokenService models.UserTokenService, ctx *models. rotated, err := authTokenService.TryRotateToken(ctx.Req.Context(), token, ctx.RemoteAddr(), ctx.Req.UserAgent()) if err != nil { - ctx.Logger.Error("failed to rotate token", "error", err) + ctx.Logger.Error("Failed to rotate token", "error", err) return true } @@ -223,7 +248,7 @@ func initContextWithToken(authTokenService models.UserTokenService, ctx *models. func WriteSessionCookie(ctx *models.ReqContext, value string, maxLifetimeDays int) { if setting.Env == setting.DEV { - ctx.Logger.Info("new token", "unhashed token", value) + ctx.Logger.Info("New token", "unhashed token", value) } var maxAge int diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 95ab3a778e8..c8213ff2e6b 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -11,6 +11,10 @@ import ( "testing" "time" + . "github.com/smartystreets/goconvey/convey" + "github.com/stretchr/testify/assert" + "gopkg.in/macaron.v1" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/infra/remotecache" @@ -19,9 +23,6 @@ import ( "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" - . "github.com/smartystreets/goconvey/convey" - "github.com/stretchr/testify/assert" - "gopkg.in/macaron.v1" ) const errorTemplate = "error-template"