From 1b2d860bebabf23d26812d1938d3bfa1fdcf463b Mon Sep 17 00:00:00 2001 From: surtur Date: Mon, 4 Sep 2023 19:21:01 +0200 Subject: [PATCH] fix(go,tmpl): solve the Chromium/Safari logout... ...issue by deleting the session cookie after successful password change and forcing the user to re-authenticate. additionally, split the InitialPasswordChange func into separate "GET" and "POST" variants. --- app/routes.go | 2 +- handlers/home.go | 21 ++- handlers/search-hibp.go | 43 +++++ handlers/signin.go | 33 +++- handlers/user-init-password-change.go | 231 +++++++++++++---------- templates/home.tmpl | 15 +- templates/signin.tmpl | 5 + templates/user/init-password-change.tmpl | 22 ++- 8 files changed, 257 insertions(+), 115 deletions(-) diff --git a/app/routes.go b/app/routes.go index 21069f7..c604f16 100644 --- a/app/routes.go +++ b/app/routes.go @@ -75,7 +75,7 @@ func (a *App) SetupRoutes() error { user := e.Group("/user", handlers.MiddlewareSession, xsrf) user.GET("/initial-password-change", handlers.InitialPasswordChange()) - user.POST("/initial-password-change", handlers.InitialPasswordChange()) + user.POST("/initial-password-change", handlers.InitialPasswordChangePost()) user.GET("/hibp-search", handlers.GetSearchHIBP()) user.POST("/hibp-search", handlers.SearchHIBP()) user.GET("/hibp-breach-details/:name", handlers.ViewHIBP()) diff --git a/handlers/home.go b/handlers/home.go index 6d3ca7a..6b4b5ed 100644 --- a/handlers/home.go +++ b/handlers/home.go @@ -36,7 +36,7 @@ func Home(client *ent.Client) echo.HandlerFunc { return c.Redirect(http.StatusSeeOther, "/signin") } - log.Info("gorilla session", "username", sess.Values["username"].(string)) + log.Debug("session", "username", sess.Values["username"].(string), "endpoint", "/home") username = sess.Values["username"].(string) // example denial. @@ -94,17 +94,22 @@ func Home(client *ent.Client) echo.HandlerFunc { p.Name = username p.User = u - data := make(map[string]any) - flash := sess.Values["flash"] - - if flash != nil { - data["flash"] = flash.(string) + if flsh, ok := sess.Values["flash"].(string); ok { + p.Data["flash"] = flsh delete(sess.Values, "flash") - - _ = sess.Save(c.Request(), c.Response()) } + if _, ok := sess.Values["reauthFlash"].(string); ok { + p.Data["info"] = "First time after changing the password, yay!" + + // if this is the first login after the initial password change, delete + // the cookie value. + delete(sess.Values, "reauthFlash") + } + + _ = sess.Save(c.Request(), c.Response()) + err := c.Render(http.StatusOK, "home.tmpl", p) if err != nil { c.Logger().Errorf("error: %q", err) diff --git a/handlers/search-hibp.go b/handlers/search-hibp.go index 626bb57..454c7de 100644 --- a/handlers/search-hibp.go +++ b/handlers/search-hibp.go @@ -4,6 +4,7 @@ package handlers import ( + "context" "errors" "fmt" "net/http" @@ -30,6 +31,25 @@ func GetSearchHIBP() echo.HandlerFunc { ) } + if !u.IsAdmin { + ctx := context.WithValue(context.Background(), moduser.CtxKey{}, slogger) + + f, err := moduser.UsrFinishedSetup(ctx, dbclient, u.ID) + if err != nil { + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), + ) + } + + if !f { + log.Warn("resource access attempt without performing the initial password change", "user", u.Username, "endpoint", "/user/hibp-search") + return c.Redirect(http.StatusSeeOther, "/user/initial-password-change") + } + } + csrf := c.Get("csrf").(string) p := newPage() @@ -90,6 +110,29 @@ func SearchHIBP() echo.HandlerFunc { ) } + if !u.IsAdmin { + ctx := context.WithValue(context.Background(), moduser.CtxKey{}, slogger) + + f, err := moduser.UsrFinishedSetup(ctx, dbclient, u.ID) + if err != nil { + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), + ) + } + + if !f { + return renderErrorPage( + c, + http.StatusUnauthorized, + http.StatusText(http.StatusUnauthorized)+" - you need to perform your initial password change before accessing this resource", + "user never changed password", + ) + } + } + csrf := c.Get("csrf").(string) a := new(hibpSearch) diff --git a/handlers/signin.go b/handlers/signin.go index c387063..7a07ba6 100644 --- a/handlers/signin.go +++ b/handlers/signin.go @@ -20,10 +20,27 @@ func Signin() echo.HandlerFunc { return func(c echo.Context) error { addHeaders(c) - if sess, _ := session.Get(setting.SessionCookieName(), c); sess != nil { + reauth := false + r := c.QueryParam("reauth") + + if r == "true" { + reauth = true + } + + // slog.Info("XXX session cookie name:", setting.SessionCookieName()) + + sess, _ := session.Get(setting.SessionCookieName(), c) + + var uf *userSignin + + if sess != nil { username := sess.Values["username"] if username != nil { - return c.Redirect(http.StatusFound, "/home") + if !reauth { + return c.Redirect(http.StatusPermanentRedirect, "/home") + } + + uf = &userSignin{Username: username.(string)} } } @@ -34,6 +51,18 @@ func Signin() echo.HandlerFunc { p.Current = "signin" p.CSRF = csrf + if reauth { + fl := sess.Values["reauthFlash"] + if _, ok := fl.(string); !ok { + p.Data["flash"] = "re-login please" + } else { + p.Data["reauthFlash"] = fl.(string) + if uf != nil { + p.Data["form"] = uf + } + } + } + return c.Render( http.StatusOK, "signin.tmpl", diff --git a/handlers/user-init-password-change.go b/handlers/user-init-password-change.go index f2dd596..35167c3 100644 --- a/handlers/user-init-password-change.go +++ b/handlers/user-init-password-change.go @@ -13,6 +13,107 @@ import ( "github.com/labstack/echo/v4" ) +func InitialPasswordChangePost() echo.HandlerFunc { + return func(c echo.Context) error { + addHeaders(c) + + u, ok := c.Get("sessUsr").(moduser.User) + if !ok { + return renderErrorPage( + c, + http.StatusUnauthorized, + http.StatusText(http.StatusUnauthorized)+", perhaps you need to log in first?", + "Username was nil", + ) + } else if u.IsAdmin { + status := http.StatusUnauthorized + msg := http.StatusText(status) + + return renderErrorPage( + c, status, msg+": You should not be here", "This endpoint is for users only", + ) + } + + ctx, ok := c.Get("sloggerCtx").(context.Context) + if !ok { + ctx = context.WithValue(context.Background(), moduser.CtxKey{}, slogger) + } + + f, err := moduser.UsrFinishedSetup(ctx, dbclient, u.ID) + + switch { + case err != nil: + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), + ) + + case f: + return c.Redirect(http.StatusSeeOther, "/user/init-password-change") + } + + pw := new(initPasswordChange) + + if err := c.Bind(pw); err != nil { + return renderErrorPage( + c, + http.StatusBadRequest, + http.StatusText(http.StatusBadRequest), + err.Error(), + ) + } + + err = moduser.ChangePassFirstLogin(ctx, dbclient, u.ID, pw.NewPassword) + if err != nil { + c.Logger().Errorf("error changing initial user password: %q", err) + + switch { + case errors.Is(err, moduser.ErrPasswordEmpty): + return renderErrorPage( + c, + http.StatusBadRequest, + http.StatusText(http.StatusBadRequest), + err.Error(), + ) + + case errors.Is(err, moduser.ErrNewPasswordCannotEqual): + return renderErrorPage( + c, + http.StatusBadRequest, + http.StatusText(http.StatusBadRequest)+" - the new password needs to be different from the original", + err.Error(), + ) + } + + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), + ) + } + + log.Info("successfully performed initial password change", "user", u.Username) + + if sess, ok := c.Get("sess").(*sessions.Session); ok { + sess.Values["reauthFlash"] = "Successfully updated your password, log in again, please" + + if err = sess.Save(c.Request(), c.Response()); err != nil { + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError)+" - could not change the session cookie", + err.Error(), + ) + } + } + + return c.Redirect(http.StatusSeeOther, "/signin?reauth=true") + } +} + func InitialPasswordChange() echo.HandlerFunc { return func(c echo.Context) error { addHeaders(c) @@ -42,105 +143,41 @@ func InitialPasswordChange() echo.HandlerFunc { f, err := moduser.UsrFinishedSetup(ctx, dbclient, u.ID) switch { - case c.Request().Method == "POST": - switch { - case err != nil: - return renderErrorPage( - c, - http.StatusInternalServerError, - http.StatusText(http.StatusInternalServerError), - err.Error(), - ) - - case f: - return c.Redirect(http.StatusSeeOther, "/user/init-password-change") - } - - pw := new(initPasswordChange) - - if err := c.Bind(pw); err != nil { - return renderErrorPage( - c, - http.StatusBadRequest, - http.StatusText(http.StatusBadRequest), - err.Error(), - ) - } - - err = moduser.ChangePassFirstLogin(ctx, dbclient, u.ID, pw.NewPassword) - if err != nil { - c.Logger().Errorf("error changing initial user password: %q", err) - - switch { - case errors.Is(err, moduser.ErrPasswordEmpty): - return renderErrorPage( - c, - http.StatusBadRequest, - http.StatusText(http.StatusBadRequest), - err.Error(), - ) - - case errors.Is(err, moduser.ErrNewPasswordCannotEqual): - return renderErrorPage( - c, - http.StatusBadRequest, - http.StatusText(http.StatusBadRequest)+" - the new password needs to be different from the original", - err.Error(), - ) - } - - return renderErrorPage( - c, - http.StatusInternalServerError, - http.StatusText(http.StatusInternalServerError), - err.Error(), - ) - } - - if sess, ok := c.Get("sess").(*sessions.Session); ok { - sess.Values["flash"] = "Successfully updated your password" - _ = sess.Save(c.Request(), c.Response()) - } - - return c.Redirect(http.StatusSeeOther, "/home") - - case c.Request().Method == "GET": - switch { - case err != nil: - return renderErrorPage( - c, - http.StatusInternalServerError, - http.StatusText(http.StatusInternalServerError), - err.Error(), - ) - - case f: - return c.Redirect(http.StatusSeeOther, "/home") - } - - csrf := c.Get("csrf").(string) - p := newPage() - - p.Title = "Initial password change" - p.Current = "init-password-change" - p.CSRF = csrf - p.User = u - - err := c.Render( - http.StatusOK, - "user/init-password-change.tmpl", - p, + case err != nil: + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), ) - if err != nil { - c.Logger().Errorf("error: %q", err) - return renderErrorPage( - c, - http.StatusInternalServerError, - http.StatusText(http.StatusInternalServerError), - err.Error(), - ) - } + case f: + return c.Redirect(http.StatusSeeOther, "/home") + } + + csrf := c.Get("csrf").(string) + p := newPage() + + p.Title = "Initial password change" + p.Current = "init-password-change" + p.CSRF = csrf + p.User = u + p.Name = u.Username + + err = c.Render( + http.StatusOK, + "user/init-password-change.tmpl", + p, + ) + if err != nil { + c.Logger().Errorf("error: %q", err) + + return renderErrorPage( + c, + http.StatusInternalServerError, + http.StatusText(http.StatusInternalServerError), + err.Error(), + ) } return nil diff --git a/templates/home.tmpl b/templates/home.tmpl index 6fe410c..3a53f4a 100644 --- a/templates/home.tmpl +++ b/templates/home.tmpl @@ -4,13 +4,22 @@
{{ if and .Data .Data.flash }} -

+

{{ .Data.flash }}

{{- end }} + {{ if and .Data .Data.info }} +

+ {{ .Data.info }} +

+ {{- end }} {{ if .Name -}} -

- Welcome, {{.Name}}!
+

+ Welcome to + + {{ .AppName }}, + + {{.Name}}!

{{if .User}} {{if .User.IsAdmin}} diff --git a/templates/signin.tmpl b/templates/signin.tmpl index bcf3c94..1c0b7e8 100644 --- a/templates/signin.tmpl +++ b/templates/signin.tmpl @@ -20,6 +20,11 @@

Error: {{.Data.flash}}

{{- else -}}{{end}} + {{ if and .Data .Data.reauthFlash }} +
+

Success: {{.Data.reauthFlash}}

+
+ {{- else -}}{{end}}
diff --git a/templates/user/init-password-change.tmpl b/templates/user/init-password-change.tmpl index fe878b5..66fc5cf 100644 --- a/templates/user/init-password-change.tmpl +++ b/templates/user/init-password-change.tmpl @@ -10,16 +10,30 @@ {{- end }} {{ if .User -}}

- Welcome, {{.Name}}, since you have never changed your password here before, now is the time to do that!
+ Welcome, + + {{.Name}}!

+

+ Since you have never changed your password here before, now is the time to do that! +

- This is necessary in order to make sure that only you know the password, since the account was pre-created for you. + This is necessary in order to make sure that only you know the + password, as the account was pre-created for you.
-
+
-
+ +
{{ template "svg-password.tmpl" }}