Improve Oauth and Session handling, fix session validation logic flaw
- commit
- f3a8d9740c38c1c968906b5e7ae653e7a2d758e2
- author
- bnema · · 2023-10-21 08:40 UTC
- parent
- 1a9b0aa
- signature
- unsigned
4 files · +30 -24
M internal/httpserve/handler/oauth.go +8 -2
| @@ -54,12 +54,18 @@ func RenderLoginPage(c echo.Context, a *app.App) error { | |||
| 54 | 54 | ||
| 55 | 55 | // StartOAuthGithub starts the Github OAuth flow | |
| 56 | 56 | func StartOAuthGithub(c echo.Context, a *app.App) error { | |
| 57 | - | // Clear the session in case it is already set or corrupted | |
| 58 | - | err := ResetSession(c) | |
| 57 | + | // Check if the session is already here and valid before going for the oauth flow | |
| 58 | + | sess, err := getSession(c) | |
| 59 | 59 | if err != nil { | |
| 60 | 60 | return err | |
| 61 | 61 | } | |
| 62 | 62 | ||
| 63 | + | // Check if the session values are valid | |
| 64 | + | accountID, ok := sess.Values["accountID"].(string) | |
| 65 | + | if ok && accountID != "" { | |
| 66 | + | return c.Redirect(http.StatusSeeOther, a.Config.Admin.Path) // StatusSeeOther is HTTP 303 | |
| 67 | + | } | |
| 68 | + | ||
| 63 | 69 | //Initiate the Github OAuth flow | |
| 64 | 70 | clientID := os.Getenv("GITHUB_APP_ID") | |
| 65 | 71 | redirectDomain := a.Config.GenerateOauthCallbackURL() | |
| @@ -54,12 +54,18 @@ func RenderLoginPage(c echo.Context, a *app.App) error { | |||||
| 54 | 54 | ||||
| 55 | // StartOAuthGithub starts the Github OAuth flow | 55 | // StartOAuthGithub starts the Github OAuth flow | ||
| 56 | func StartOAuthGithub(c echo.Context, a *app.App) error { | 56 | func StartOAuthGithub(c echo.Context, a *app.App) error { | ||
| 57 | - | // Clear the session in case it is already set or corrupted | 57 | + | // Check if the session is already here and valid before going for the oauth flow |
| 58 | - | err := ResetSession(c) | 58 | + | sess, err := getSession(c) |
| 59 | if err != nil { | 59 | if err != nil { | ||
| 60 | return err | 60 | return err | ||
| 61 | } | 61 | } | ||
| 62 | 62 | ||||
| 63 | + | // Check if the session values are valid | |||
| 64 | + | accountID, ok := sess.Values["accountID"].(string) | |||
| 65 | + | if ok && accountID != "" { | |||
| 66 | + | return c.Redirect(http.StatusSeeOther, a.Config.Admin.Path) // StatusSeeOther is HTTP 303 | |||
| 67 | + | } | |||
| 68 | + | ||||
| 63 | //Initiate the Github OAuth flow | 69 | //Initiate the Github OAuth flow | ||
| 64 | clientID := os.Getenv("GITHUB_APP_ID") | 70 | clientID := os.Getenv("GITHUB_APP_ID") | ||
| 65 | redirectDomain := a.Config.GenerateOauthCallbackURL() | 71 | redirectDomain := a.Config.GenerateOauthCallbackURL() | ||
M internal/httpserve/middleware/requirelogin.go +20 -15
| @@ -1,13 +1,16 @@ | |||
| 1 | 1 | package middleware | |
| 2 | 2 | ||
| 3 | 3 | import ( | |
| 4 | + | "fmt" | |
| 4 | 5 | "log" | |
| 5 | 6 | "net/http" | |
| 6 | 7 | "time" | |
| 7 | 8 | ||
| 8 | 9 | "github.com/bnema/gordon/internal/app" | |
| 9 | 10 | "github.com/bnema/gordon/internal/db/queries" | |
| 11 | + | ||
| 10 | 12 | // "github.com/labstack/echo-contrib/session" | |
| 13 | + | "github.com/labstack/echo-contrib/session" | |
| 11 | 14 | "github.com/labstack/echo/v4" | |
| 12 | 15 | ) | |
| 13 | 16 | ||
| @@ -24,24 +27,26 @@ func RequireLogin(a *app.App) echo.MiddlewareFunc { | |||
| 24 | 27 | } | |
| 25 | 28 | ||
| 26 | 29 | func validateSessionAndUser(c echo.Context, a *app.App) error { | |
| 27 | - | // sess, err := session.Get("session", c) | |
| 28 | - | // if err != nil { | |
| 29 | - | // return err | |
| 30 | - | // } | |
| 30 | + | sess, err := session.Get("session", c) | |
| 31 | + | if err != nil { | |
| 32 | + | return err | |
| 33 | + | } | |
| 31 | 34 | ||
| 32 | - | // if cookieExpired, err := isCookieExpired(c); err != nil || cookieExpired { | |
| 33 | - | // return err | |
| 34 | - | // } | |
| 35 | + | fmt.Print(sess) | |
| 35 | 36 | ||
| 36 | - | // accountID, ok := sess.Values["accountID"].(string) | |
| 37 | - | // if !ok { | |
| 38 | - | // return fmt.Errorf("invalid account ID type") | |
| 39 | - | // } | |
| 37 | + | if cookieExpired, err := isCookieExpired(c); err != nil || cookieExpired { | |
| 38 | + | return err | |
| 39 | + | } | |
| 40 | 40 | ||
| 41 | - | // sessionID, ok := sess.Values["sessionID"].(string) | |
| 42 | - | // if !ok { | |
| 43 | - | // return fmt.Errorf("invalid session ID type") | |
| 44 | - | // } | |
| 41 | + | _, ok := sess.Values["accountID"].(string) | |
| 42 | + | if !ok { | |
| 43 | + | return fmt.Errorf("invalid account ID type") | |
| 44 | + | } | |
| 45 | + | ||
| 46 | + | _, ok = sess.Values["sessionID"].(string) | |
| 47 | + | if !ok { | |
| 48 | + | return fmt.Errorf("invalid session ID type") | |
| 49 | + | } | |
| 45 | 50 | ||
| 46 | 51 | // if accountCheck, err := isAccountIDInDB(a, accountID); err != nil || !accountCheck { | |
| 47 | 52 | // return err | |
| @@ -1,13 +1,16 @@ | |||||
| 1 | package middleware | 1 | package middleware | ||
| 2 | 2 | ||||
| 3 | import ( | 3 | import ( | ||
| 4 | + | "fmt" | |||
| 4 | "log" | 5 | "log" | ||
| 5 | "net/http" | 6 | "net/http" | ||
| 6 | "time" | 7 | "time" | ||
| 7 | 8 | ||||
| 8 | "github.com/bnema/gordon/internal/app" | 9 | "github.com/bnema/gordon/internal/app" | ||
| 9 | "github.com/bnema/gordon/internal/db/queries" | 10 | "github.com/bnema/gordon/internal/db/queries" | ||
| 11 | + | ||||
| 10 | // "github.com/labstack/echo-contrib/session" | 12 | // "github.com/labstack/echo-contrib/session" | ||
| 13 | + | "github.com/labstack/echo-contrib/session" | |||
| 11 | "github.com/labstack/echo/v4" | 14 | "github.com/labstack/echo/v4" | ||
| 12 | ) | 15 | ) | ||
| 13 | 16 | ||||
| @@ -24,24 +27,26 @@ func RequireLogin(a *app.App) echo.MiddlewareFunc { | |||||
| 24 | } | 27 | } | ||
| 25 | 28 | ||||
| 26 | func validateSessionAndUser(c echo.Context, a *app.App) error { | 29 | func validateSessionAndUser(c echo.Context, a *app.App) error { | ||
| 27 | - | // sess, err := session.Get("session", c) | 30 | + | sess, err := session.Get("session", c) |
| 28 | - | // if err != nil { | 31 | + | if err != nil { |
| 29 | - | // return err | 32 | + | return err |
| 30 | - | // } | 33 | + | } |
| 31 | 34 | ||||
| 32 | - | // if cookieExpired, err := isCookieExpired(c); err != nil || cookieExpired { | 35 | + | fmt.Print(sess) |
| 33 | - | // return err | |||
| 34 | - | // } | |||
| 35 | 36 | ||||
| 36 | - | // accountID, ok := sess.Values["accountID"].(string) | 37 | + | if cookieExpired, err := isCookieExpired(c); err != nil || cookieExpired { |
| 37 | - | // if !ok { | 38 | + | return err |
| 38 | - | // return fmt.Errorf("invalid account ID type") | 39 | + | } |
| 39 | - | // } | |||
| 40 | 40 | ||||
| 41 | - | // sessionID, ok := sess.Values["sessionID"].(string) | 41 | + | _, ok := sess.Values["accountID"].(string) |
| 42 | - | // if !ok { | 42 | + | if !ok { |
| 43 | - | // return fmt.Errorf("invalid session ID type") | 43 | + | return fmt.Errorf("invalid account ID type") |
| 44 | - | // } | 44 | + | } |
| 45 | + | ||||
| 46 | + | _, ok = sess.Values["sessionID"].(string) | |||
| 47 | + | if !ok { | |||
| 48 | + | return fmt.Errorf("invalid session ID type") | |||
| 49 | + | } | |||
| 45 | 50 | ||||
| 46 | // if accountCheck, err := isAccountIDInDB(a, accountID); err != nil || !accountCheck { | 51 | // if accountCheck, err := isAccountIDInDB(a, accountID); err != nil || !accountCheck { | ||
| 47 | // return err | 52 | // return err | ||
M internal/httpserve/router.go +2 -2
| @@ -11,14 +11,14 @@ import ( | |||
| 11 | 11 | // RegisterRoutes registers all routes for the application | |
| 12 | 12 | func RegisterRoutes(e *echo.Echo, a *app.App) *echo.Echo { | |
| 13 | 13 | AdminPath := a.Config.Admin.Path | |
| 14 | - | // Use middlewares | |
| 15 | - | e.Use(middleware.SecureRoutes()) | |
| 16 | 14 | // SetCommonDataMiddleware will pass data to the renderer | |
| 17 | 15 | e.Use(middleware.SetCommonDataMiddleware(a)) | |
| 18 | 16 | // Error handler middleware | |
| 19 | 17 | e.Use(middleware.ErrorHandler) | |
| 20 | 18 | // Initiate the session middleware | |
| 21 | 19 | e.Use(middleware.InitSessionMiddleware()) | |
| 20 | + | // Use middlewares | |
| 21 | + | e.Use(middleware.SecureRoutes()) | |
| 22 | 22 | ||
| 23 | 23 | // Color scheme detection for dark/light mode | |
| 24 | 24 | e.Use(middleware.ColorSchemeDetection) | |
| @@ -11,14 +11,14 @@ import ( | |||||
| 11 | // RegisterRoutes registers all routes for the application | 11 | // RegisterRoutes registers all routes for the application | ||
| 12 | func RegisterRoutes(e *echo.Echo, a *app.App) *echo.Echo { | 12 | func RegisterRoutes(e *echo.Echo, a *app.App) *echo.Echo { | ||
| 13 | AdminPath := a.Config.Admin.Path | 13 | AdminPath := a.Config.Admin.Path | ||
| 14 | - | // Use middlewares | |||
| 15 | - | e.Use(middleware.SecureRoutes()) | |||
| 16 | // SetCommonDataMiddleware will pass data to the renderer | 14 | // SetCommonDataMiddleware will pass data to the renderer | ||
| 17 | e.Use(middleware.SetCommonDataMiddleware(a)) | 15 | e.Use(middleware.SetCommonDataMiddleware(a)) | ||
| 18 | // Error handler middleware | 16 | // Error handler middleware | ||
| 19 | e.Use(middleware.ErrorHandler) | 17 | e.Use(middleware.ErrorHandler) | ||
| 20 | // Initiate the session middleware | 18 | // Initiate the session middleware | ||
| 21 | e.Use(middleware.InitSessionMiddleware()) | 19 | e.Use(middleware.InitSessionMiddleware()) | ||
| 20 | + | // Use middlewares | |||
| 21 | + | e.Use(middleware.SecureRoutes()) | |||
| 22 | 22 | ||||
| 23 | // Color scheme detection for dark/light mode | 23 | // Color scheme detection for dark/light mode | ||
| 24 | e.Use(middleware.ColorSchemeDetection) | 24 | e.Use(middleware.ColorSchemeDetection) | ||
M internal/webui/public/assets/css/tailwind.css +0 -5
| @@ -1252,11 +1252,6 @@ video { | |||
| 1252 | 1252 | padding-top: 8rem; | |
| 1253 | 1253 | } | |
| 1254 | 1254 | ||
| 1255 | - | .md\:text-2xl { | |
| 1256 | - | font-size: 1.5rem; | |
| 1257 | - | line-height: 2rem; | |
| 1258 | - | } | |
| 1259 | - | ||
| 1260 | 1255 | .md\:text-4xl { | |
| 1261 | 1256 | font-size: 2.25rem; | |
| 1262 | 1257 | line-height: 2.5rem; | |
| @@ -1252,11 +1252,6 @@ video { | |||||
| 1252 | padding-top: 8rem; | 1252 | padding-top: 8rem; | ||
| 1253 | } | 1253 | } | ||
| 1254 | 1254 | ||||
| 1255 | - | .md\:text-2xl { | |||
| 1256 | - | font-size: 1.5rem; | |||
| 1257 | - | line-height: 2rem; | |||
| 1258 | - | } | |||
| 1259 | - | ||||
| 1260 | .md\:text-4xl { | 1255 | .md\:text-4xl { | ||
| 1261 | font-size: 2.25rem; | 1256 | font-size: 2.25rem; | ||
| 1262 | line-height: 2.5rem; | 1257 | line-height: 2.5rem; | ||