fix: ensure correct error propagation

ory · Oct 22, 2021 · 77ce709 · 77ce709
1 parent 3093b80
commit 77ce709
Show file tree

Hide file tree

Showing 6 changed files with 18 additions and 26 deletions.
diff --git a/selfservice/flow/error.go b/selfservice/flow/error.go
@@ -24,7 +24,7 @@ var (
 //
 // swagger:model selfServiceFlowExpiredError
 type ExpiredError struct {
-	DefaultError *herodot.DefaultError `json:"error"`
+	*herodot.DefaultError `json:"error"`
 
 	// Since when the flow has expired
 	Ago time.Duration `json:"since"`
@@ -49,10 +49,6 @@ func (e *ExpiredError) EnhanceJSONError() interface{} {
 	return e
 }
 
-func (e *ExpiredError) Error() string {
-	return e.DefaultError.Error()
-}
-
 func NewFlowExpiredError(at time.Time) *ExpiredError {
 	ago := time.Since(at)
 	return &ExpiredError{
@@ -67,7 +63,7 @@ func NewFlowExpiredError(at time.Time) *ExpiredError {
 //
 // swagger:model selfServiceBrowserLocationChangeRequiredError
 type BrowserLocationChangeRequiredError struct {
-	DefaultError *herodot.DefaultError `json:"error"`
+	*herodot.DefaultError `json:"error"`
 
 	// Since when the flow has expired
 	RedirectBrowserTo string `json:"redirect_browser_to"`
@@ -90,7 +86,3 @@ func NewBrowserLocationChangeRequiredError(redirectTo string) *BrowserLocationCh
 		},
 	}
 }
-
-func (e *BrowserLocationChangeRequiredError) Error() string {
-	return e.DefaultError.Error()
-}
diff --git a/selfservice/flow/settings/error.go b/selfservice/flow/settings/error.go
@@ -142,7 +142,7 @@ func (s *ErrorHandler) WriteFlowError(
 
 	if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) {
 		if shouldRespondWithJSON {
-			s.d.Writer().WriteError(w, r, err)
+			s.d.Writer().WriteError(w, r, aalErr)
 		} else {
 			http.Redirect(w, r, urlx.CopyWithQuery(
 				urlx.AppendPaths(s.d.Config(r.Context()).SelfPublicURL(r), login.RouteInitBrowserFlow),

diff --git a/selfservice/flow/settings/error_test.go b/selfservice/flow/settings/error_test.go
@@ -231,7 +231,8 @@ func TestHandleError(t *testing.T) {
 
 				body, err := ioutil.ReadAll(res.Body)
 				require.NoError(t, err)
-				assert.Equal(t, session.NewErrAALNotSatisfied("").Reason(), gjson.GetBytes(body, "error.reason").String(), "%s", body)
+				require.NotEmpty(t, gjson.GetBytes(body, "redirect_browser_to").String())
+				assertx.EqualAsJSONExcept(t, session.NewErrAALNotSatisfied(""), json.RawMessage(body), []string{"redirect_browser_to"})
 			})
 
 			t.Run("case=generic error", func(t *testing.T) {

diff --git a/selfservice/flow/settings/handler_test.go b/selfservice/flow/settings/handler_test.go
@@ -9,6 +9,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/ory/x/assertx"
+
 	"github.com/ory/kratos/session"
 
 	"github.com/gofrs/uuid"
@@ -124,8 +126,8 @@ func TestHandler(t *testing.T) {
 			t.Run("description=can not init if identity has aal2 but session has aal1", func(t *testing.T) {
 				conf.MustSet(config.ViperKeySelfServiceSettingsRequiredAAL, config.HighestAvailableAAL)
 				res, body := initFlow(t, aal2Identity, true)
-				assert.Equal(t, http.StatusForbidden, res.StatusCode)
-				assert.EqualValues(t, session.NewErrAALNotSatisfied("").Reason(), gjson.GetBytes(body, "error.reason").String())
+				assert.Equal(t, http.StatusForbidden, res.StatusCode, "%s", body)
+				assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied(publicTS.URL+"/self-service/login/browser?aal=aal2"), json.RawMessage(body))
 			})
 		})
 
@@ -164,7 +166,7 @@ func TestHandler(t *testing.T) {
 					}})
 				res, body := initSPAFlow(t, user1)
 				assert.Equal(t, http.StatusForbidden, res.StatusCode)
-				assert.EqualValues(t, session.NewErrAALNotSatisfied("").Reason(), gjson.GetBytes(body, "error.reason").String())
+				assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied(publicTS.URL+"/self-service/login/browser?aal=aal2"), json.RawMessage(body))
 			})
 		})
 	})
@@ -264,7 +266,7 @@ func TestHandler(t *testing.T) {
 				require.NoError(t, res.Body.Close())
 
 				require.EqualValues(t, res.StatusCode, http.StatusForbidden)
-				assert.Equal(t, session.NewErrAALNotSatisfied("").Reason(), gjson.GetBytes(body, "error.reason").String(), "%s", body)
+				assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied(publicTS.URL+"/self-service/login/browser?aal=aal2"), json.RawMessage(body))
 			})
 		})
 	})
@@ -302,7 +304,8 @@ func TestHandler(t *testing.T) {
 				conf.MustSet(config.ViperKeySelfServiceSettingsRequiredAAL, config.HighestAvailableAAL)
 				actual, res := testhelpers.SettingsMakeRequest(t, false, true, &f, aal2Identity, `{"method":"not-exists"}`)
 				assert.Equal(t, http.StatusForbidden, res.StatusCode)
-				assert.Equal(t, session.NewErrAALNotSatisfied("").Reason(), gjson.Get(actual, "error.reason").String(), actual)
+				require.NotEmpty(t, gjson.Get(actual, "redirect_browser_to").String())
+				assertx.EqualAsJSONExcept(t, session.NewErrAALNotSatisfied(""), json.RawMessage(actual), []string{"redirect_browser_to"})
 			})
 
 			t.Run("type=api", func(t *testing.T) {
@@ -319,7 +322,7 @@ func TestHandler(t *testing.T) {
 				conf.MustSet(config.ViperKeySelfServiceSettingsRequiredAAL, config.HighestAvailableAAL)
 				actual, res := testhelpers.SettingsMakeRequest(t, true, false, &f, aal2Identity, `{"method":"not-exists"}`)
 				assert.Equal(t, http.StatusForbidden, res.StatusCode)
-				assert.Equal(t, session.NewErrAALNotSatisfied("").Reason(), gjson.Get(actual, "error.reason").String(), actual)
+				assertx.EqualAsJSON(t, session.NewErrAALNotSatisfied(publicTS.URL+"/self-service/login/browser?aal=aal2"), json.RawMessage(actual))
 			})
 		})
 

diff --git a/session/manager.go b/session/manager.go
@@ -22,18 +22,14 @@ var (
 //
 // swagger:model errorAuthenticatorAssuranceLevelNotSatisfied
 type ErrAALNotSatisfied struct {
-	DefaultError *herodot.DefaultError `json:"error"`
-	RedirectTo string `json:"redirect_browser_to"`
+	*herodot.DefaultError `json:"error"`
+	RedirectTo            string `json:"redirect_browser_to"`
 }
 
 func (e *ErrAALNotSatisfied) EnhanceJSONError() interface{} {
 	return e
 }
 
-func (e *ErrAALNotSatisfied) Error() string {
-	return e.DefaultError.Error()
-}
-
 // NewErrAALNotSatisfied creates a new ErrAALNotSatisfied.
 func NewErrAALNotSatisfied(redirectTo string) *ErrAALNotSatisfied {
 	return &ErrAALNotSatisfied{

diff --git a/session/manager_http.go b/session/manager_http.go
@@ -196,8 +196,8 @@ func (s *ManagerHTTP) DoesSessionSatisfy(r *http.Request, sess *Session, request
 			return nil
 		}
 
-		return errors.WithStack(NewErrAALNotSatisfied(
-			urlx.CopyWithQuery(urlx.AppendPaths(s.r.Config(r.Context()).SelfPublicURL(r), "/self-service/login/browser"), url.Values{"aal": {"aal2"}}).String()))
+		return NewErrAALNotSatisfied(
+			urlx.CopyWithQuery(urlx.AppendPaths(s.r.Config(r.Context()).SelfPublicURL(r), "/self-service/login/browser"), url.Values{"aal": {"aal2"}}).String())
 	}
 	return errors.Errorf("requested unknown aal: %s", requestedAAL)
 }