From 43ac79685ad9fa16593c9b0ce103058292660a17 Mon Sep 17 00:00:00 2001 From: bergquist Date: Thu, 31 Jan 2019 15:45:11 +0100 Subject: [PATCH 1/4] delete auth token on signout --- pkg/api/common_test.go | 2 +- pkg/middleware/middleware_test.go | 2 +- pkg/services/auth/auth_token.go | 26 +++++++++++++++++++++++--- pkg/services/auth/auth_token_test.go | 25 +++++++++++++++++++++++++ 4 files changed, 50 insertions(+), 5 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index eb1f89e3f22..f6c6e53e91d 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -149,4 +149,4 @@ func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqC return nil } -func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) {} +func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) error { return nil } diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 11740574d0b..9bb45062e00 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -602,4 +602,4 @@ func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqC return nil } -func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) {} +func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) error { return nil } diff --git a/pkg/services/auth/auth_token.go b/pkg/services/auth/auth_token.go index 7e9433c2d70..d9c5e897f70 100644 --- a/pkg/services/auth/auth_token.go +++ b/pkg/services/auth/auth_token.go @@ -3,6 +3,7 @@ package auth import ( "crypto/sha256" "encoding/hex" + "errors" "net/http" "net/url" "time" @@ -31,7 +32,7 @@ var ( type UserAuthTokenService interface { InitContextWithToken(ctx *models.ReqContext, orgID int64) bool UserAuthenticatedHook(user *models.User, c *models.ReqContext) error - UserSignedOutHook(c *models.ReqContext) + UserSignedOutHook(c *models.ReqContext) error } type UserAuthTokenServiceImpl struct { @@ -111,8 +112,27 @@ func (s *UserAuthTokenServiceImpl) UserAuthenticatedHook(user *models.User, c *m return nil } -func (s *UserAuthTokenServiceImpl) UserSignedOutHook(c *models.ReqContext) { - s.writeSessionCookie(c, "", -1) +func (s *UserAuthTokenServiceImpl) UserSignedOutHook(c *models.ReqContext) error { + unhashedToken := c.GetCookie(s.Cfg.LoginCookieName) + if unhashedToken == "" { + return errors.New("cannot logout without session token") + } + + hashedToken := hashToken(unhashedToken) + + sql := `DELETE FROM user_auth_token WHERE auth_token = ?` + res, err := s.SQLStore.NewSession().Exec(sql, hashedToken) + if err != nil { + return err + } + + affected, _ := res.RowsAffected() + if affected > 0 { + s.writeSessionCookie(c, "", -1) + return nil + } + + return errors.New("failed to delete session") } func (s *UserAuthTokenServiceImpl) CreateToken(userId int64, clientIP, userAgent string) (*userAuthToken, error) { diff --git a/pkg/services/auth/auth_token_test.go b/pkg/services/auth/auth_token_test.go index 2f75c660d9d..0114939ea48 100644 --- a/pkg/services/auth/auth_token_test.go +++ b/pkg/services/auth/auth_token_test.go @@ -1,10 +1,13 @@ package auth import ( + "net/http" "testing" "time" + "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" + "gopkg.in/macaron.v1" "github.com/grafana/grafana/pkg/log" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -46,6 +49,28 @@ func TestUserAuthToken(t *testing.T) { So(err, ShouldEqual, ErrAuthTokenNotFound) So(LookupToken, ShouldBeNil) }) + + Convey("signing out should delete token and cookie if present", func() { + token, err := userAuthTokenService.CreateToken(userID, "192.168.1.1:1234", "some user agent2") + So(err, ShouldBeNil) + So(token, ShouldNotBeNil) + + httpreq := &http.Request{Header: make(http.Header)} + httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: token.AuthToken}) + + ctx := &models.ReqContext{Context: &macaron.Context{Req: macaron.Request{Request: httpreq}}} + + err = userAuthTokenService.UserSignedOutHook(ctx) + So(err, ShouldBeNil) + + // makes sure we tell the browser to overwrite the cookie + So(ctx.Resp.Header().Get("Set-Cookie"), ShouldEqual, "") + + // lookedUp, err = userAuthTokenService.LookupToken(token.UnhashedToken) + // So(err, ShouldBeNil) + // So(lookedUp, ShouldNotBeNil) + + }) }) Convey("expires correctly", func() { From 88ca54eba96195d1fc0e0138c17d8c6991deb938 Mon Sep 17 00:00:00 2001 From: bergquist Date: Thu, 31 Jan 2019 16:22:40 +0100 Subject: [PATCH 2/4] renames signout function --- pkg/api/common_test.go | 2 +- pkg/api/login.go | 2 +- pkg/middleware/middleware_test.go | 2 +- pkg/services/auth/auth_token.go | 4 ++-- pkg/services/auth/auth_token_test.go | 11 +++-------- 5 files changed, 8 insertions(+), 13 deletions(-) diff --git a/pkg/api/common_test.go b/pkg/api/common_test.go index f6c6e53e91d..fe02c94e277 100644 --- a/pkg/api/common_test.go +++ b/pkg/api/common_test.go @@ -149,4 +149,4 @@ func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqC return nil } -func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) error { return nil } +func (s *fakeUserAuthTokenService) SignOutUser(c *m.ReqContext) error { return nil } diff --git a/pkg/api/login.go b/pkg/api/login.go index 50c62e0835a..49da147724e 100644 --- a/pkg/api/login.go +++ b/pkg/api/login.go @@ -136,7 +136,7 @@ func (hs *HTTPServer) loginUserWithUser(user *m.User, c *m.ReqContext) { } func (hs *HTTPServer) Logout(c *m.ReqContext) { - hs.AuthTokenService.UserSignedOutHook(c) + hs.AuthTokenService.SignOutUser(c) if setting.SignoutRedirectUrl != "" { c.Redirect(setting.SignoutRedirectUrl) diff --git a/pkg/middleware/middleware_test.go b/pkg/middleware/middleware_test.go index 9bb45062e00..4679c449853 100644 --- a/pkg/middleware/middleware_test.go +++ b/pkg/middleware/middleware_test.go @@ -602,4 +602,4 @@ func (s *fakeUserAuthTokenService) UserAuthenticatedHook(user *m.User, c *m.ReqC return nil } -func (s *fakeUserAuthTokenService) UserSignedOutHook(c *m.ReqContext) error { return nil } +func (s *fakeUserAuthTokenService) SignOutUser(c *m.ReqContext) error { return nil } diff --git a/pkg/services/auth/auth_token.go b/pkg/services/auth/auth_token.go index d9c5e897f70..5f8f36fc373 100644 --- a/pkg/services/auth/auth_token.go +++ b/pkg/services/auth/auth_token.go @@ -32,7 +32,7 @@ var ( type UserAuthTokenService interface { InitContextWithToken(ctx *models.ReqContext, orgID int64) bool UserAuthenticatedHook(user *models.User, c *models.ReqContext) error - UserSignedOutHook(c *models.ReqContext) error + SignOutUser(c *models.ReqContext) error } type UserAuthTokenServiceImpl struct { @@ -112,7 +112,7 @@ func (s *UserAuthTokenServiceImpl) UserAuthenticatedHook(user *models.User, c *m return nil } -func (s *UserAuthTokenServiceImpl) UserSignedOutHook(c *models.ReqContext) error { +func (s *UserAuthTokenServiceImpl) SignOutUser(c *models.ReqContext) error { unhashedToken := c.GetCookie(s.Cfg.LoginCookieName) if unhashedToken == "" { return errors.New("cannot logout without session token") diff --git a/pkg/services/auth/auth_token_test.go b/pkg/services/auth/auth_token_test.go index 0114939ea48..47afe627479 100644 --- a/pkg/services/auth/auth_token_test.go +++ b/pkg/services/auth/auth_token_test.go @@ -51,7 +51,7 @@ func TestUserAuthToken(t *testing.T) { }) Convey("signing out should delete token and cookie if present", func() { - token, err := userAuthTokenService.CreateToken(userID, "192.168.1.1:1234", "some user agent2") + token, err := userAuthTokenService.CreateToken(userID, "192.168.1.1:1234", "user agent") So(err, ShouldBeNil) So(token, ShouldNotBeNil) @@ -60,16 +60,11 @@ func TestUserAuthToken(t *testing.T) { ctx := &models.ReqContext{Context: &macaron.Context{Req: macaron.Request{Request: httpreq}}} - err = userAuthTokenService.UserSignedOutHook(ctx) + err = userAuthTokenService.SignOutUser(ctx) So(err, ShouldBeNil) // makes sure we tell the browser to overwrite the cookie - So(ctx.Resp.Header().Get("Set-Cookie"), ShouldEqual, "") - - // lookedUp, err = userAuthTokenService.LookupToken(token.UnhashedToken) - // So(err, ShouldBeNil) - // So(lookedUp, ShouldNotBeNil) - + //So(ctx.Resp.Header().Get("Set-Cookie"), ShouldEqual, "") }) }) From 91bd908e03ecdfbc691c13e66cb9512535ca78fb Mon Sep 17 00:00:00 2001 From: bergquist Date: Thu, 31 Jan 2019 22:24:04 +0100 Subject: [PATCH 3/4] adds more tests signing out session --- pkg/services/auth/auth_token.go | 2 +- pkg/services/auth/auth_token_test.go | 35 +++++++++++++++++++++------- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/pkg/services/auth/auth_token.go b/pkg/services/auth/auth_token.go index 5f8f36fc373..deb3c1a5bba 100644 --- a/pkg/services/auth/auth_token.go +++ b/pkg/services/auth/auth_token.go @@ -86,7 +86,7 @@ func (s *UserAuthTokenServiceImpl) InitContextWithToken(ctx *models.ReqContext, func (s *UserAuthTokenServiceImpl) writeSessionCookie(ctx *models.ReqContext, value string, maxAge int) { if setting.Env == setting.DEV { - ctx.Logger.Info("new token", "unhashed token", value) + ctx.Logger.Debug("new token", "unhashed token", value) } ctx.Resp.Header().Del("Set-Cookie") diff --git a/pkg/services/auth/auth_token_test.go b/pkg/services/auth/auth_token_test.go index 47afe627479..e58fe795b4a 100644 --- a/pkg/services/auth/auth_token_test.go +++ b/pkg/services/auth/auth_token_test.go @@ -1,13 +1,15 @@ package auth import ( + "fmt" "net/http" + "net/http/httptest" "testing" "time" "github.com/grafana/grafana/pkg/models" "github.com/grafana/grafana/pkg/setting" - "gopkg.in/macaron.v1" + macaron "gopkg.in/macaron.v1" "github.com/grafana/grafana/pkg/log" "github.com/grafana/grafana/pkg/services/sqlstore" @@ -51,20 +53,37 @@ func TestUserAuthToken(t *testing.T) { }) Convey("signing out should delete token and cookie if present", func() { - token, err := userAuthTokenService.CreateToken(userID, "192.168.1.1:1234", "user agent") - So(err, ShouldBeNil) - So(token, ShouldNotBeNil) - httpreq := &http.Request{Header: make(http.Header)} - httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: token.AuthToken}) + httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: token.UnhashedToken}) - ctx := &models.ReqContext{Context: &macaron.Context{Req: macaron.Request{Request: httpreq}}} + ctx := &models.ReqContext{Context: &macaron.Context{ + Req: macaron.Request{Request: httpreq}, + Resp: macaron.NewResponseWriter("POST", httptest.NewRecorder()), + }, + Logger: log.New("fakelogger"), + } err = userAuthTokenService.SignOutUser(ctx) So(err, ShouldBeNil) // makes sure we tell the browser to overwrite the cookie - //So(ctx.Resp.Header().Get("Set-Cookie"), ShouldEqual, "") + cookieHeader := fmt.Sprintf("%s=; Path=/; Max-Age=0; HttpOnly", userAuthTokenService.Cfg.LoginCookieName) + So(ctx.Resp.Header().Get("Set-Cookie"), ShouldEqual, cookieHeader) + }) + + Convey("signing out an none existing session should return an error", func() { + httpreq := &http.Request{Header: make(http.Header)} + httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: "missing-session-cookie"}) + + ctx := &models.ReqContext{Context: &macaron.Context{ + Req: macaron.Request{Request: httpreq}, + Resp: macaron.NewResponseWriter("POST", httptest.NewRecorder()), + }, + Logger: log.New("fakelogger"), + } + + err = userAuthTokenService.SignOutUser(ctx) + So(err, ShouldNotBeNil) }) }) From a1b3986532dbbb51145471304fccc6f254899bfe Mon Sep 17 00:00:00 2001 From: bergquist Date: Fri, 1 Feb 2019 09:59:53 +0100 Subject: [PATCH 4/4] always delete session cookie even if db delete fails --- devenv/docker/blocks/loki/docker-compose.yaml | 2 ++ pkg/services/auth/auth_token.go | 14 +++----------- pkg/services/auth/auth_token_test.go | 2 +- 3 files changed, 6 insertions(+), 12 deletions(-) diff --git a/devenv/docker/blocks/loki/docker-compose.yaml b/devenv/docker/blocks/loki/docker-compose.yaml index d6cf21f7856..bd4f8d3c728 100644 --- a/devenv/docker/blocks/loki/docker-compose.yaml +++ b/devenv/docker/blocks/loki/docker-compose.yaml @@ -20,3 +20,5 @@ services: -config.file=/etc/promtail/docker-config.yaml networks: - loki + depends_on: + - loki diff --git a/pkg/services/auth/auth_token.go b/pkg/services/auth/auth_token.go index deb3c1a5bba..5cb43974d34 100644 --- a/pkg/services/auth/auth_token.go +++ b/pkg/services/auth/auth_token.go @@ -121,18 +121,10 @@ func (s *UserAuthTokenServiceImpl) SignOutUser(c *models.ReqContext) error { hashedToken := hashToken(unhashedToken) sql := `DELETE FROM user_auth_token WHERE auth_token = ?` - res, err := s.SQLStore.NewSession().Exec(sql, hashedToken) - if err != nil { - return err - } + _, err := s.SQLStore.NewSession().Exec(sql, hashedToken) - affected, _ := res.RowsAffected() - if affected > 0 { - s.writeSessionCookie(c, "", -1) - return nil - } - - return errors.New("failed to delete session") + s.writeSessionCookie(c, "", -1) + return err } func (s *UserAuthTokenServiceImpl) CreateToken(userId int64, clientIP, userAgent string) (*userAuthToken, error) { diff --git a/pkg/services/auth/auth_token_test.go b/pkg/services/auth/auth_token_test.go index e58fe795b4a..312e53a3970 100644 --- a/pkg/services/auth/auth_token_test.go +++ b/pkg/services/auth/auth_token_test.go @@ -73,7 +73,7 @@ func TestUserAuthToken(t *testing.T) { Convey("signing out an none existing session should return an error", func() { httpreq := &http.Request{Header: make(http.Header)} - httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: "missing-session-cookie"}) + httpreq.AddCookie(&http.Cookie{Name: userAuthTokenService.Cfg.LoginCookieName, Value: ""}) ctx := &models.ReqContext{Context: &macaron.Context{ Req: macaron.Request{Request: httpreq},