From 31ab8ad3b7effacf08400bcb5ff172b5a2361466 Mon Sep 17 00:00:00 2001 From: Trong Huu Nguyen Date: Thu, 21 Jul 2022 08:21:28 +0200 Subject: [PATCH] refactor(handler/default): redirect auto-login requests instead of inlining login handler --- pkg/handler/handler.go | 7 ++++++ pkg/handler/handler_default.go | 7 ++++-- pkg/handler/handler_test.go | 8 +++++++ pkg/middleware/logentry.go | 2 +- pkg/router/router.go | 3 +-- pkg/url/url.go | 11 +++++++--- pkg/url/url_test.go | 39 ++++++++++++++++++++++++++++++++++ 7 files changed, 69 insertions(+), 8 deletions(-) diff --git a/pkg/handler/handler.go b/pkg/handler/handler.go index 3771897..66208b6 100644 --- a/pkg/handler/handler.go +++ b/pkg/handler/handler.go @@ -25,6 +25,8 @@ type Handler struct { OpenIDConfig openidconfig.Config Provider provider.Provider Sessions session.Store + + path string } func NewHandler( @@ -54,5 +56,10 @@ func NewHandler( OpenIDConfig: openidConfig, Provider: openidProvider, Sessions: sessionStore, + path: config.ParseIngress(cfg.Ingress), }, nil } + +func (h *Handler) Path() string { + return h.path +} diff --git a/pkg/handler/handler_default.go b/pkg/handler/handler_default.go index 05c9c7b..209795d 100644 --- a/pkg/handler/handler_default.go +++ b/pkg/handler/handler_default.go @@ -6,6 +6,7 @@ import ( logentry "github.com/nais/wonderwall/pkg/middleware" "github.com/nais/wonderwall/pkg/session" + urlpkg "github.com/nais/wonderwall/pkg/url" ) // Default proxies all requests upstream. @@ -31,8 +32,10 @@ func (h *Handler) Default(w http.ResponseWriter, r *http.Request) { if h.AutoLogin.NeedsLogin(r, isAuthenticated) { logger.Debug("default: auto-login is enabled; request does not match skippable path") - r.Header.Add("Referer", r.URL.String()) - h.Login(w, r) + redirectTarget := r.URL.String() + loginUrl := urlpkg.LoginURL(h.Path(), redirectTarget) + + http.Redirect(w, r, loginUrl, http.StatusTemporaryRedirect) return } diff --git a/pkg/handler/handler_test.go b/pkg/handler/handler_test.go index 4023dfe..9f73639 100644 --- a/pkg/handler/handler_test.go +++ b/pkg/handler/handler_test.go @@ -187,6 +187,14 @@ func TestHandler_Default(t *testing.T) { resp := get(t, rpClient, target) assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + // redirect should point to local login endpoint + loginLocation := resp.Location + assert.Equal(t, idp.RelyingPartyServer.URL+"/oauth2/login?redirect=/", loginLocation.String()) + + // follow redirect to local login endpoint + resp = get(t, rpClient, loginLocation.String()) + assert.Equal(t, http.StatusTemporaryRedirect, resp.StatusCode) + // redirect should point to identity provider authorizeLocation := resp.Location diff --git a/pkg/middleware/logentry.go b/pkg/middleware/logentry.go index 055df3e..c172cbf 100644 --- a/pkg/middleware/logentry.go +++ b/pkg/middleware/logentry.go @@ -22,7 +22,7 @@ func LogEntryHandler(provider string) func(next http.Handler) http.Handler { return func(next http.Handler) http.Handler { fn := func(w http.ResponseWriter, r *http.Request) { entry := logger.NewLogEntry(r) - entry.WithRequestLogFields(r).Infof("request") + entry.WithRequestLogFields(r).Infof("%s - %s", r.Method, r.URL.Path) ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor) t1 := time.Now() diff --git a/pkg/router/router.go b/pkg/router/router.go index 968ce47..4c8c505 100644 --- a/pkg/router/router.go +++ b/pkg/router/router.go @@ -4,7 +4,6 @@ import ( "github.com/go-chi/chi/v5" chi_middleware "github.com/go-chi/chi/v5/middleware" - "github.com/nais/wonderwall/pkg/config" "github.com/nais/wonderwall/pkg/handler" "github.com/nais/wonderwall/pkg/middleware" "github.com/nais/wonderwall/pkg/router/paths" @@ -16,7 +15,7 @@ func New(handler *handler.Handler) chi.Router { r.Use(chi_middleware.Recoverer) prometheusMiddleware := middleware.NewPrometheusMiddleware("wonderwall", handler.OpenIDConfig.Provider().Name()) - prefix := config.ParseIngress(handler.Config.Ingress) + prefix := handler.Path() r.Group(func(r chi.Router) { r.Use(middleware.LogEntryHandler(handler.OpenIDConfig.Provider().Name())) diff --git a/pkg/url/url.go b/pkg/url/url.go index 767addf..b41d427 100644 --- a/pkg/url/url.go +++ b/pkg/url/url.go @@ -59,9 +59,14 @@ func Retry(r *http.Request, ingress string, loginCookie *openid.LoginCookie) str redirect = loginCookie.Referer } - retryURI = fmt.Sprintf(prefix + paths.OAuth2 + paths.Login) - retryURI = retryURI + fmt.Sprintf("?%s=%s", RedirectURLParameter, redirect) - return retryURI + return LoginURL(prefix, redirect) +} + +func LoginURL(prefix, redirectTarget string) string { + loginPath := prefix + paths.OAuth2 + paths.Login + redirectParam := fmt.Sprintf("?%s=%s", RedirectURLParameter, redirectTarget) + + return loginPath + redirectParam } func defaultRedirectURL(ingress string) string { diff --git a/pkg/url/url_test.go b/pkg/url/url_test.go index ac6eea3..5ecf49e 100644 --- a/pkg/url/url_test.go +++ b/pkg/url/url_test.go @@ -299,3 +299,42 @@ func TestRetry(t *testing.T) { }) } } + +func TestLoginURL(t *testing.T) { + for _, test := range []struct { + name string + prefix string + redirectTarget string + want string + }{ + { + name: "no prefix", + prefix: "", + redirectTarget: "https://test.example.com?some=param&other=param2", + want: "/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + }, + { + name: "with prefix", + prefix: "/path", + redirectTarget: "https://test.example.com?some=param&other=param2", + want: "/path/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + }, + { + name: "we need to go deeper", + prefix: "/deeper/path", + redirectTarget: "https://test.example.com?some=param&other=param2", + want: "/deeper/path/oauth2/login?redirect=https://test.example.com?some=param&other=param2", + }, + { + name: "relative target", + prefix: "", + redirectTarget: "/path?some=param&other=param2", + want: "/oauth2/login?redirect=/path?some=param&other=param2", + }, + } { + t.Run(test.name, func(t *testing.T) { + loginUrl := urlpkg.LoginURL(test.prefix, test.redirectTarget) + assert.Equal(t, test.want, loginUrl) + }) + } +}