From 73610d4cfb16789385d2660e278419664b1ea3f3 Mon Sep 17 00:00:00 2001 From: Toon van Strijp Date: Mon, 8 Nov 2021 16:16:35 +0100 Subject: [PATCH] fix: add error.id to invalid cookie/token settings flow (#1919) Closes #1888 Co-authored-by: aeneasr <3372410+aeneasr@users.noreply.github.com> --- selfservice/flow/login/handler.go | 4 ++-- selfservice/flow/settings/error.go | 2 +- selfservice/flow/settings/error_test.go | 6 +++--- selfservice/flow/settings/handler.go | 2 +- selfservice/flow/settings/handler_test.go | 14 ++++++++++++++ session/handler.go | 6 ++---- session/handler_test.go | 2 +- session/manager.go | 19 +++++++++++++++---- session/manager_http.go | 6 +++--- 9 files changed, 42 insertions(+), 19 deletions(-) diff --git a/selfservice/flow/login/handler.go b/selfservice/flow/login/handler.go index c9c5af8d76ee..1adbd1e9aa44 100644 --- a/selfservice/flow/login/handler.go +++ b/selfservice/flow/login/handler.go @@ -108,7 +108,7 @@ func (h *Handler) NewLoginFlow(w http.ResponseWriter, r *http.Request, ft flow.T // We assume an error means the user has no session sess, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) - if errors.Is(err, session.ErrNoActiveSessionFound) { + if e := new(session.ErrNoActiveSessionFound); errors.As(err, &e) { // No session exists yet // We can not request an AAL > 1 because we must first verify the first factor. @@ -550,7 +550,7 @@ func (h *Handler) submitFlow(w http.ResponseWriter, r *http.Request, _ httproute http.Redirect(w, r, h.d.Config(r.Context()).SelfServiceBrowserDefaultReturnTo().String(), http.StatusSeeOther) return - } else if errors.Is(err, session.ErrNoActiveSessionFound) { + } else if e := new(session.ErrNoActiveSessionFound); errors.As(err, &e) { // Only failure scenario here is if we try to upgrade the session to a higher AAL without actually // having a session. if f.RequestedAAL > identity.AuthenticatorAssuranceLevel1 { diff --git a/selfservice/flow/settings/error.go b/selfservice/flow/settings/error.go index f9d34d301679..918c15e18bff 100644 --- a/selfservice/flow/settings/error.go +++ b/selfservice/flow/settings/error.go @@ -131,7 +131,7 @@ func (s *ErrorHandler) WriteFlowError( shouldRespondWithJSON = true } - if errors.Is(err, session.ErrNoActiveSessionFound) { + if e := new(session.ErrNoActiveSessionFound); errors.As(err, &e) { if shouldRespondWithJSON { s.d.Writer().WriteError(w, r, err) } else { diff --git a/selfservice/flow/settings/error_test.go b/selfservice/flow/settings/error_test.go index b64f4eab6d39..dcb4e8156994 100644 --- a/selfservice/flow/settings/error_test.go +++ b/selfservice/flow/settings/error_test.go @@ -203,7 +203,7 @@ func TestHandleError(t *testing.T) { settingsFlow = newFlow(t, time.Minute, flow.TypeBrowser) settingsFlow.IdentityID = id.ID - flowError = errors.WithStack(session.ErrNoActiveSessionFound) + flowError = errors.WithStack(session.NewErrNoActiveSessionFound()) flowMethod = settings.StrategyProfile res, err := ts.Client().Do(testhelpers.NewHTTPGetJSONRequest(t, ts.URL+"/error")) @@ -213,7 +213,7 @@ func TestHandleError(t *testing.T) { body, err := ioutil.ReadAll(res.Body) require.NoError(t, err) - assert.Equal(t, session.ErrNoActiveSessionFound.Reason(), gjson.GetBytes(body, "error.reason").String(), "%s", body) + assert.Equal(t, session.NewErrNoActiveSessionFound().Reason(), gjson.GetBytes(body, "error.reason").String(), "%s", body) }) t.Run("case=aal too low", func(t *testing.T) { @@ -294,7 +294,7 @@ func TestHandleError(t *testing.T) { settingsFlow = newFlow(t, time.Minute, flow.TypeBrowser) settingsFlow.IdentityID = id.ID - flowError = errors.WithStack(session.ErrNoActiveSessionFound) + flowError = errors.WithStack(session.NewErrNoActiveSessionFound()) flowMethod = settings.StrategyProfile res, err := ts.Client().Get(ts.URL + "/error") diff --git a/selfservice/flow/settings/handler.go b/selfservice/flow/settings/handler.go index dac24a01e550..dabfd3965018 100644 --- a/selfservice/flow/settings/handler.go +++ b/selfservice/flow/settings/handler.go @@ -84,7 +84,7 @@ func (h *Handler) RegisterPublicRoutes(public *x.RouterPublic) { public.GET(RouteInitBrowserFlow, h.d.SessionHandler().IsAuthenticated(h.initBrowserFlow, func(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { if x.IsJSONRequest(r) { - h.d.Writer().WriteError(w, r, errors.WithStack(herodot.ErrForbidden.WithReason("Please include a valid session cookie or session token when calling this endpoint."))) + h.d.Writer().WriteError(w, r, session.NewErrNoActiveSessionFound()) } else { http.Redirect(w, r, h.d.Config(r.Context()).SelfServiceFlowLoginUI().String(), http.StatusFound) } diff --git a/selfservice/flow/settings/handler_test.go b/selfservice/flow/settings/handler_test.go index 5f4f1af84748..6b81c7ab7236 100644 --- a/selfservice/flow/settings/handler_test.go +++ b/selfservice/flow/settings/handler_test.go @@ -3,6 +3,7 @@ package settings_test import ( "context" "encoding/json" + "github.com/ory/kratos/text" "io/ioutil" "net/http" "net/url" @@ -116,6 +117,12 @@ func TestHandler(t *testing.T) { t.Run("endpoint=init", func(t *testing.T) { t.Run("description=init a flow as API", func(t *testing.T) { + t.Run("description=without privileges", func(t *testing.T) { + res, body := initFlow(t, new(http.Client), true) + assert.Equal(t, http.StatusUnauthorized, res.StatusCode, "%s", body) + assert.Equal(t, text.ErrNoActiveSession, gjson.GetBytes(body, "error.id").String(),"%s", body) + }) + t.Run("description=success", func(t *testing.T) { user1 := testhelpers.NewHTTPClientWithArbitrarySessionToken(t, reg) res, body := initFlow(t, user1, true) @@ -132,6 +139,12 @@ func TestHandler(t *testing.T) { }) t.Run("description=init a flow as browser", func(t *testing.T) { + t.Run("description=without privileges", func(t *testing.T) { + res, body := initSPAFlow(t, new(http.Client)) + assert.Equal(t, http.StatusUnauthorized, res.StatusCode, "%s", body) + assert.Equal(t, text.ErrNoActiveSession, gjson.GetBytes(body, "error.id").String(),"%s", body) + }) + t.Run("description=success", func(t *testing.T) { user1 := testhelpers.NewHTTPClientWithArbitrarySessionCookie(t, reg) res, body := initFlow(t, user1, false) @@ -363,6 +376,7 @@ func TestHandler(t *testing.T) { }) }) }) + t.Run("case=relative redirect when self-service settings ui is a relative url", func(t *testing.T) { reg.Config(context.Background()).MustSet(config.ViperKeySelfServiceSettingsURL, "/settings-ts") user1 := testhelpers.NewNoRedirectHTTPClientWithArbitrarySessionCookie(t, reg) diff --git a/session/handler.go b/session/handler.go index 4bb015746083..251140d86800 100644 --- a/session/handler.go +++ b/session/handler.go @@ -9,8 +9,6 @@ import ( "github.com/ory/x/decoderx" - "github.com/ory/x/errorsx" - "github.com/ory/herodot" "github.com/ory/kratos/driver/config" @@ -234,7 +232,7 @@ func (h *Handler) IsAuthenticated(wrap httprouter.Handle, onUnauthenticated http return } - h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrForbidden.WithReason("This endpoint can only be accessed with a valid session. Please log in and try again."))) + h.r.Writer().WriteError(w, r, errors.WithStack(NewErrNoActiveSessionFound().WithReason("This endpoint can only be accessed with a valid session. Please log in and try again."))) return } @@ -245,7 +243,7 @@ func (h *Handler) IsAuthenticated(wrap httprouter.Handle, onUnauthenticated http func (h *Handler) IsNotAuthenticated(wrap httprouter.Handle, onAuthenticated httprouter.Handle) httprouter.Handle { return func(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { if _, err := h.r.SessionManager().FetchFromRequest(r.Context(), r); err != nil { - if errorsx.Cause(err).Error() == ErrNoActiveSessionFound.Error() { + if e := new(ErrNoActiveSessionFound); errors.As(err, &e) { wrap(w, r, ps) return } diff --git a/session/handler_test.go b/session/handler_test.go index 894ac83af1ad..534d1c51e730 100644 --- a/session/handler_test.go +++ b/session/handler_test.go @@ -345,7 +345,7 @@ func TestIsAuthenticated(t *testing.T) { { c: http.DefaultClient, call: "/privileged/without-callback", - code: http.StatusForbidden, + code: http.StatusUnauthorized, }, } { t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) { diff --git a/session/manager.go b/session/manager.go index ad05a9c05234..aad2ce60d2c8 100644 --- a/session/manager.go +++ b/session/manager.go @@ -13,10 +13,21 @@ import ( "github.com/ory/herodot" ) -var ( - // ErrNoActiveSessionFound is returned when no active cookie session could be found in the request. - ErrNoActiveSessionFound = herodot.ErrUnauthorized.WithID(text.ErrNoActiveSession).WithError("request does not have a valid authentication session").WithReason("No active session was found in this request.") -) +// ErrNoActiveSessionFound is returned when no active cookie session could be found in the request. +type ErrNoActiveSessionFound struct { + *herodot.DefaultError `json:"error"` +} + +// NewErrNoActiveSessionFound creates a new ErrNoActiveSessionFound +func NewErrNoActiveSessionFound() *ErrNoActiveSessionFound { + return &ErrNoActiveSessionFound{ + DefaultError: herodot.ErrUnauthorized.WithID(text.ErrNoActiveSession).WithError("request does not have a valid authentication session").WithReason("No active session was found in this request."), + } +} + +func (e *ErrNoActiveSessionFound) EnhanceJSONError() interface{} { + return e +} // ErrAALNotSatisfied is returned when an active session was found but the requested AAL is not satisfied. // diff --git a/session/manager_http.go b/session/manager_http.go index 440c67cb8b1d..95751d06a401 100644 --- a/session/manager_http.go +++ b/session/manager_http.go @@ -132,19 +132,19 @@ func (s *ManagerHTTP) extractToken(r *http.Request) string { func (s *ManagerHTTP) FetchFromRequest(ctx context.Context, r *http.Request) (*Session, error) { token := s.extractToken(r) if token == "" { - return nil, errors.WithStack(ErrNoActiveSessionFound) + return nil, errors.WithStack(NewErrNoActiveSessionFound()) } se, err := s.r.SessionPersister().GetSessionByToken(ctx, token) if err != nil { if errors.Is(err, herodot.ErrNotFound) || errors.Is(err, sqlcon.ErrNoRows) { - return nil, errors.WithStack(ErrNoActiveSessionFound) + return nil, errors.WithStack(NewErrNoActiveSessionFound()) } return nil, err } if !se.IsActive() { - return nil, errors.WithStack(ErrNoActiveSessionFound) + return nil, errors.WithStack(NewErrNoActiveSessionFound()) } se.Identity = se.Identity.CopyWithoutCredentials()