From d53e64a32c48fca9149d2e291313a6dc8b04bb53 Mon Sep 17 00:00:00 2001 From: Marcus Efraimsson Date: Mon, 4 Feb 2019 23:44:28 +0100 Subject: [PATCH] move auth token middleware/hooks to middleware package fix/adds auth token middleware tests --- pkg/api/common_test.go | 63 +++++++++-- pkg/api/http_server.go | 18 +-- pkg/api/login.go | 17 ++- pkg/middleware/middleware.go | 65 ++++++++++- pkg/middleware/middleware_test.go | 165 +++++++++++++++++++++++++--- pkg/middleware/org_redirect_test.go | 31 ++++-- pkg/middleware/quota_test.go | 16 ++- pkg/models/context.go | 2 + 8 files changed, 324 insertions(+), 53 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index fe02c94e277..853a04b5c11 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -8,6 +8,7 @@ import ( "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/auth" "gopkg.in/macaron.v1" . "github.com/smartystreets/goconvey/convey" @@ -129,24 +130,70 @@ func setupScenarioContext(url string) *scenarioContext { return sc } +type fakeUserToken interface { + auth.UserToken + SetToken(token string) +} + +type userTokenImpl struct { + userId int64 + token string +} + +func (ut *userTokenImpl) GetUserId() int64 { + return ut.userId +} + +func (ut *userTokenImpl) GetToken() string { + return ut.token +} + +func (ut *userTokenImpl) SetToken(token string) { + ut.token = token +} + type fakeUserAuthTokenService struct { - initContextWithTokenProvider func(ctx *m.ReqContext, orgID int64) bool + createTokenProvider func(userId int64, clientIP, userAgent string) (auth.UserToken, error) + tryRotateTokenProvider func(token auth.UserToken, clientIP, userAgent string) (bool, error) + lookupTokenProvider func(unhashedToken string) (auth.UserToken, error) + revokeTokenProvider func(token auth.UserToken) error } func newFakeUserAuthTokenService() *fakeUserAuthTokenService { return &fakeUserAuthTokenService{ - initContextWithTokenProvider: func(ctx *m.ReqContext, orgID int64) bool { - return false + createTokenProvider: func(userId int64, clientIP, userAgent string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 0, + token: "", + }, nil + }, + tryRotateTokenProvider: func(token auth.UserToken, clientIP, userAgent string) (bool, error) { + return false, nil + }, + lookupTokenProvider: func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 0, + token: "", + }, nil + }, + revokeTokenProvider: func(token auth.UserToken) error { + return nil }, } } -func (s *fakeUserAuthTokenService) InitContextWithToken(ctx *m.ReqContext, orgID int64) bool { - return s.initContextWithTokenProvider(ctx, orgID) +func (s *fakeUserAuthTokenService) CreateToken(userId int64, clientIP, userAgent string) (auth.UserToken, error) { + return s.createTokenProvider(userId, clientIP, userAgent) } -func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqContext) error { - return nil +func (s *fakeUserAuthTokenService) LookupToken(unhashedToken string) (auth.UserToken, error) { + return s.lookupTokenProvider(unhashedToken) } -func (s *fakeUserAuthTokenService) SignOutUser(c *m.ReqContext) error { return nil } +func (s *fakeUserAuthTokenService) TryRotateToken(token auth.UserToken, clientIP, userAgent string) (bool, error) { + return s.tryRotateTokenProvider(token, clientIP, userAgent) +} + +func (s *fakeUserAuthTokenService) RevokeToken(token auth.UserToken) error { + return s.revokeTokenProvider(token) +} diff --git a/pkg/api/http_server.go b/pkg/api/http_server.go index 7b7c1478a4c..a0a65d73244 100644 --- a/pkg/api/http_server.go +++ b/pkg/api/http_server.go @@ -21,7 +21,7 @@ import ( "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/plugins" "github.com/grafana/grafana/pkg/registry" - "github.com/grafana/grafana/pkg/services/auth" + "github.com/grafana/grafana/pkg/services/auth/authtoken" "github.com/grafana/grafana/pkg/services/cache" "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/hooks" @@ -48,14 +48,14 @@ type HTTPServer struct { streamManager *live.StreamManager httpSrv *http.Server - RouteRegister routing.RouteRegister `inject:""` - Bus bus.Bus `inject:""` - RenderService rendering.Service `inject:""` - Cfg *setting.Cfg `inject:""` - HooksService *hooks.HooksService `inject:""` - CacheService *cache.CacheService `inject:""` - DatasourceCache datasources.CacheService `inject:""` - AuthTokenService auth.UserAuthTokenService `inject:""` + RouteRegister routing.RouteRegister `inject:""` + Bus bus.Bus `inject:""` + RenderService rendering.Service `inject:""` + Cfg *setting.Cfg `inject:""` + HooksService *hooks.HooksService `inject:""` + CacheService *cache.CacheService `inject:""` + DatasourceCache datasources.CacheService `inject:""` + AuthTokenService authtoken.UserAuthTokenService `inject:""` } func (hs *HTTPServer) Init() error { diff --git a/pkg/api/login.go b/pkg/api/login.go index 49da147724e..d25e83d34e8 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -5,11 +5,14 @@ import ( "net/http" "net/url" + "github.com/grafana/grafana/pkg/services/auth/authtoken" + "github.com/grafana/grafana/pkg/api/dtos" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/log" "github.com/grafana/grafana/pkg/login" "github.com/grafana/grafana/pkg/metrics" + "github.com/grafana/grafana/pkg/middleware" m "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" @@ -126,17 +129,23 @@ func (hs *HTTPServer) LoginPost(c *m.ReqContext, cmd dtos.LoginCommand) Response func (hs *HTTPServer) loginUserWithUser(user *m.User, c *m.ReqContext) { if user == nil { - hs.log.Error("User login with nil user") + hs.log.Error("user login with nil user") } - err := hs.AuthTokenService.UserAuthenticatedHook(user, c) + userToken, err := hs.AuthTokenService.CreateToken(user.Id, c.RemoteAddr(), c.Req.UserAgent()) if err != nil { - hs.log.Error("User auth hook failed", "error", err) + hs.log.Error("failed to create auth token", "error", err) } + + middleware.WriteSessionCookie(c, userToken.GetToken(), middleware.OneYearInSeconds) } func (hs *HTTPServer) Logout(c *m.ReqContext) { - hs.AuthTokenService.SignOutUser(c) + if err := hs.AuthTokenService.RevokeToken(c.UserToken); err != nil && err != authtoken.ErrAuthTokenNotFound { + hs.log.Error("failed to revoke auth token", "error", err) + } + + middleware.WriteSessionCookie(c, "", -1) if setting.SignoutRedirectUrl != "" { c.Redirect(setting.SignoutRedirectUrl) diff --git a/pkg/middleware/middleware.go b/pkg/middleware/middleware.go index 3722ac3058f..6cf29340b82 100644 --- a/pkg/middleware/middleware.go +++ b/pkg/middleware/middleware.go @@ -1,13 +1,15 @@ package middleware import ( + "net/http" + "net/url" "strconv" "github.com/grafana/grafana/pkg/bus" "github.com/grafana/grafana/pkg/components/apikeygen" "github.com/grafana/grafana/pkg/log" m "github.com/grafana/grafana/pkg/models" - "github.com/grafana/grafana/pkg/services/auth" + "github.com/grafana/grafana/pkg/services/auth/authtoken" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" @@ -21,7 +23,7 @@ var ( ReqOrgAdmin = RoleAuth(m.ROLE_ADMIN) ) -func GetContextHandler(ats auth.UserAuthTokenService) macaron.Handler { +func GetContextHandler(ats authtoken.UserAuthTokenService) macaron.Handler { return func(c *macaron.Context) { ctx := &m.ReqContext{ Context: c, @@ -49,7 +51,7 @@ func GetContextHandler(ats auth.UserAuthTokenService) macaron.Handler { case initContextWithApiKey(ctx): case initContextWithBasicAuth(ctx, orgId): case initContextWithAuthProxy(ctx, orgId): - case ats.InitContextWithToken(ctx, orgId): + case initContextWithToken(ats, ctx, orgId): case initContextWithAnonymousUser(ctx): } @@ -166,6 +168,63 @@ func initContextWithBasicAuth(ctx *m.ReqContext, orgId int64) bool { return true } +const cookieName = "grafana_session" +const OneYearInSeconds = 31557600 //used as default maxage for session cookies. We validate/rotate them more often. + +func initContextWithToken(authTokenService authtoken.UserAuthTokenService, ctx *m.ReqContext, orgID int64) bool { + rawToken := ctx.GetCookie(cookieName) + if rawToken == "" { + return false + } + + token, err := authTokenService.LookupToken(rawToken) + if err != nil { + ctx.Logger.Error("failed to look up user based on cookie", "error", err) + return false + } + + query := m.GetSignedInUserQuery{UserId: token.GetUserId(), OrgId: orgID} + if err := bus.Dispatch(&query); err != nil { + ctx.Logger.Error("failed to get user with id", "userId", token.GetUserId(), "error", err) + return false + } + + ctx.SignedInUser = query.Result + ctx.IsSignedIn = true + ctx.UserToken = token + + rotated, err := authTokenService.TryRotateToken(token, ctx.RemoteAddr(), ctx.Req.UserAgent()) + if err != nil { + ctx.Logger.Error("failed to rotate token", "error", err) + return true + } + + if rotated { + WriteSessionCookie(ctx, token.GetToken(), OneYearInSeconds) + } + + return true +} + +func WriteSessionCookie(ctx *m.ReqContext, value string, maxAge int) { + if setting.Env == setting.DEV { + ctx.Logger.Info("new token", "unhashed token", value) + } + + ctx.Resp.Header().Del("Set-Cookie") + cookie := http.Cookie{ + Name: cookieName, + Value: url.QueryEscape(value), + HttpOnly: true, + Path: setting.AppSubUrl + "/", + Secure: false, // TODO: use setting SecurityHTTPSCookies + MaxAge: maxAge, + SameSite: http.SameSiteLaxMode, // TODO: use setting LoginCookieSameSite + } + + http.SetCookie(ctx.Resp, &cookie) +} + func AddDefaultResponseHeaders() macaron.Handler { return func(ctx *m.ReqContext) { if ctx.IsApiRequest() && ctx.Req.Method == "GET" { diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 4679c449853..4e10ee39201 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -10,6 +10,8 @@ import ( msession "github.com/go-macaron/session" "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/auth" + "github.com/grafana/grafana/pkg/services/auth/authtoken" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" @@ -146,17 +148,91 @@ func TestMiddlewareContext(t *testing.T) { }) }) - middlewareScenario("Auth token service", func(sc *scenarioContext) { - var wasCalled bool - sc.userAuthTokenService.initContextWithTokenProvider = func(ctx *m.ReqContext, orgId int64) bool { - wasCalled = true - return false + middlewareScenario("Non-expired auth token in cookie which not are being rotated", func(sc *scenarioContext) { + sc.withTokenSessionCookie("token") + + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { + query.Result = &m.SignedInUser{OrgId: 2, UserId: 12} + return nil + }) + + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 12, + token: unhashedToken, + }, nil } sc.fakeReq("GET", "/").exec() - Convey("should call middleware", func() { - So(wasCalled, ShouldBeTrue) + Convey("should init context with user info", func() { + So(sc.context.IsSignedIn, ShouldBeTrue) + So(sc.context.UserId, ShouldEqual, 12) + So(sc.context.UserToken.GetUserId(), ShouldEqual, 12) + So(sc.context.UserToken.GetToken(), ShouldEqual, "token") + }) + + Convey("should not set cookie", func() { + So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, "") + }) + }) + + middlewareScenario("Non-expired auth token in cookie which are being rotated", func(sc *scenarioContext) { + sc.withTokenSessionCookie("token") + + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { + query.Result = &m.SignedInUser{OrgId: 2, UserId: 12} + return nil + }) + + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 12, + token: unhashedToken, + }, nil + } + + sc.userAuthTokenService.tryRotateTokenProvider = func(userToken auth.UserToken, clientIP, userAgent string) (bool, error) { + userToken.(fakeUserToken).SetToken("rotated") + return true, nil + } + + expectedCookie := &http.Cookie{ + Name: cookieName, + Value: "rotated", + Path: setting.AppSubUrl + "/", + HttpOnly: true, + MaxAge: OneYearInSeconds, + SameSite: http.SameSiteLaxMode, + } + + sc.fakeReq("GET", "/").exec() + + Convey("should init context with user info", func() { + So(sc.context.IsSignedIn, ShouldBeTrue) + So(sc.context.UserId, ShouldEqual, 12) + So(sc.context.UserToken.GetUserId(), ShouldEqual, 12) + So(sc.context.UserToken.GetToken(), ShouldEqual, "rotated") + }) + + Convey("should set cookie", func() { + So(sc.resp.Header().Get("Set-Cookie"), ShouldEqual, expectedCookie.String()) + }) + }) + + middlewareScenario("Invalid/expired auth token in cookie", func(sc *scenarioContext) { + sc.withTokenSessionCookie("token") + + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return nil, authtoken.ErrAuthTokenNotFound + } + + sc.fakeReq("GET", "/").exec() + + Convey("should not init context with user info", func() { + So(sc.context.IsSignedIn, ShouldBeFalse) + So(sc.context.UserId, ShouldEqual, 0) + So(sc.context.UserToken, ShouldBeNil) }) }) @@ -508,6 +584,7 @@ type scenarioContext struct { resp *httptest.ResponseRecorder apiKey string authHeader string + tokenSessionCookie string respJson map[string]interface{} handlerFunc handlerFunc defaultHandler macaron.Handler @@ -522,6 +599,11 @@ func (sc *scenarioContext) withValidApiKey() *scenarioContext { return sc } +func (sc *scenarioContext) withTokenSessionCookie(unhashedToken string) *scenarioContext { + sc.tokenSessionCookie = unhashedToken + return sc +} + func (sc *scenarioContext) withAuthorizationHeader(authHeader string) *scenarioContext { sc.authHeader = authHeader return sc @@ -571,6 +653,13 @@ func (sc *scenarioContext) exec() { sc.req.Header.Add("Authorization", sc.authHeader) } + if sc.tokenSessionCookie != "" { + sc.req.AddCookie(&http.Cookie{ + Name: cookieName, + Value: sc.tokenSessionCookie, + }) + } + sc.m.ServeHTTP(sc.resp, sc.req) if sc.resp.Header().Get("Content-Type") == "application/json; charset=UTF-8" { @@ -582,24 +671,70 @@ func (sc *scenarioContext) exec() { type scenarioFunc func(c *scenarioContext) type handlerFunc func(c *m.ReqContext) +type fakeUserToken interface { + auth.UserToken + SetToken(token string) +} + +type userTokenImpl struct { + userId int64 + token string +} + +func (ut *userTokenImpl) GetUserId() int64 { + return ut.userId +} + +func (ut *userTokenImpl) GetToken() string { + return ut.token +} + +func (ut *userTokenImpl) SetToken(token string) { + ut.token = token +} + type fakeUserAuthTokenService struct { - initContextWithTokenProvider func(ctx *m.ReqContext, orgID int64) bool + createTokenProvider func(userId int64, clientIP, userAgent string) (auth.UserToken, error) + tryRotateTokenProvider func(token auth.UserToken, clientIP, userAgent string) (bool, error) + lookupTokenProvider func(unhashedToken string) (auth.UserToken, error) + revokeTokenProvider func(token auth.UserToken) error } func newFakeUserAuthTokenService() *fakeUserAuthTokenService { return &fakeUserAuthTokenService{ - initContextWithTokenProvider: func(ctx *m.ReqContext, orgID int64) bool { - return false + createTokenProvider: func(userId int64, clientIP, userAgent string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 0, + token: "", + }, nil + }, + tryRotateTokenProvider: func(token auth.UserToken, clientIP, userAgent string) (bool, error) { + return false, nil + }, + lookupTokenProvider: func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 0, + token: "", + }, nil + }, + revokeTokenProvider: func(token auth.UserToken) error { + return nil }, } } -func (s *fakeUserAuthTokenService) InitContextWithToken(ctx *m.ReqContext, orgID int64) bool { - return s.initContextWithTokenProvider(ctx, orgID) +func (s *fakeUserAuthTokenService) CreateToken(userId int64, clientIP, userAgent string) (auth.UserToken, error) { + return s.createTokenProvider(userId, clientIP, userAgent) } -func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqContext) error { - return nil +func (s *fakeUserAuthTokenService) LookupToken(unhashedToken string) (auth.UserToken, error) { + return s.lookupTokenProvider(unhashedToken) } -func (s *fakeUserAuthTokenService) SignOutUser(c *m.ReqContext) error { return nil } +func (s *fakeUserAuthTokenService) TryRotateToken(token auth.UserToken, clientIP, userAgent string) (bool, error) { + return s.tryRotateTokenProvider(token, clientIP, userAgent) +} + +func (s *fakeUserAuthTokenService) RevokeToken(token auth.UserToken) error { + return s.revokeTokenProvider(token) +} diff --git a/pkg/middleware/org_redirect_test.go b/pkg/middleware/org_redirect_test.go index 46b8776fdcc..c7479b3e9bc 100644 --- a/pkg/middleware/org_redirect_test.go +++ b/pkg/middleware/org_redirect_test.go @@ -3,6 +3,8 @@ package middleware import ( "testing" + "github.com/grafana/grafana/pkg/services/auth" + "fmt" "github.com/grafana/grafana/pkg/bus" @@ -14,14 +16,21 @@ func TestOrgRedirectMiddleware(t *testing.T) { Convey("Can redirect to correct org", t, func() { middlewareScenario("when setting a correct org for the user", func(sc *scenarioContext) { + sc.withTokenSessionCookie("token") bus.AddHandler("test", func(query *m.SetUsingOrgCommand) error { return nil }) - sc.userAuthTokenService.initContextWithTokenProvider = func(ctx *m.ReqContext, orgId int64) bool { - ctx.SignedInUser = &m.SignedInUser{OrgId: 1, UserId: 12} - ctx.IsSignedIn = true - return true + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { + query.Result = &m.SignedInUser{OrgId: 1, UserId: 12} + return nil + }) + + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 12, + token: "", + }, nil } sc.m.Get("/", sc.defaultHandler) @@ -33,21 +42,23 @@ func TestOrgRedirectMiddleware(t *testing.T) { }) middlewareScenario("when setting an invalid org for user", func(sc *scenarioContext) { + sc.withTokenSessionCookie("token") bus.AddHandler("test", func(query *m.SetUsingOrgCommand) error { return fmt.Errorf("") }) - sc.userAuthTokenService.initContextWithTokenProvider = func(ctx *m.ReqContext, orgId int64) bool { - ctx.SignedInUser = &m.SignedInUser{OrgId: 1, UserId: 12} - ctx.IsSignedIn = true - return true - } - bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { query.Result = &m.SignedInUser{OrgId: 1, UserId: 12} return nil }) + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 12, + token: "", + }, nil + } + sc.m.Get("/", sc.defaultHandler) sc.fakeReq("GET", "/?orgId=3").exec() diff --git a/pkg/middleware/quota_test.go b/pkg/middleware/quota_test.go index 4f2203a5d3d..af22f41deba 100644 --- a/pkg/middleware/quota_test.go +++ b/pkg/middleware/quota_test.go @@ -5,6 +5,7 @@ import ( "github.com/grafana/grafana/pkg/bus" m "github.com/grafana/grafana/pkg/models" + "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" . "github.com/smartystreets/goconvey/convey" @@ -74,10 +75,17 @@ func TestMiddlewareQuota(t *testing.T) { }) middlewareScenario("with user logged in", func(sc *scenarioContext) { - sc.userAuthTokenService.initContextWithTokenProvider = func(ctx *m.ReqContext, orgId int64) bool { - ctx.SignedInUser = &m.SignedInUser{OrgId: 2, UserId: 12} - ctx.IsSignedIn = true - return true + sc.withTokenSessionCookie("token") + bus.AddHandler("test", func(query *m.GetSignedInUserQuery) error { + query.Result = &m.SignedInUser{OrgId: 2, UserId: 12} + return nil + }) + + sc.userAuthTokenService.lookupTokenProvider = func(unhashedToken string) (auth.UserToken, error) { + return &userTokenImpl{ + userId: 12, + token: "", + }, nil } bus.AddHandler("globalQuota", func(query *m.GetGlobalQuotaByTargetQuery) error { diff --git a/pkg/models/context.go b/pkg/models/context.go index df970451304..da63db63f45 100644 --- a/pkg/models/context.go +++ b/pkg/models/context.go @@ -4,6 +4,7 @@ import ( "strings" "github.com/grafana/grafana/pkg/log" + "github.com/grafana/grafana/pkg/services/auth" "github.com/grafana/grafana/pkg/services/session" "github.com/grafana/grafana/pkg/setting" "github.com/prometheus/client_golang/prometheus" @@ -13,6 +14,7 @@ import ( type ReqContext struct { *macaron.Context *SignedInUser + UserToken auth.UserToken // This should only be used by the auth_proxy Session session.SessionStore