fix: don't return 500 if active strategy is disabled (#3197)

ory · Mar 30, 2023 · 3a734c2 · 3a734c2
1 parent 411633d
commit 3a734c2
Show file tree

Hide file tree

Showing 13 changed files with 281 additions and 21 deletions.
diff --git a/driver/registry_default_recovery.go b/driver/registry_default_recovery.go
@@ -6,6 +6,9 @@ package driver
 import (
 	"context"
 
+	"github.com/pkg/errors"
+
+	"github.com/ory/herodot"
 	"github.com/ory/kratos/driver/config"
 	"github.com/ory/kratos/selfservice/flow/recovery"
 	"github.com/ory/kratos/selfservice/strategy/code"
@@ -41,8 +44,13 @@ func (m *RegistryDefault) RecoveryStrategies(ctx context.Context) (recoveryStrat
 // GetActiveRecoveryStrategy returns the currently active recovery strategy
 // If no recovery strategy has been set, an error is returned
 func (m *RegistryDefault) GetActiveRecoveryStrategy(ctx context.Context) (recovery.Strategy, error) {
-	activeRecoveryStrategy := m.Config().SelfServiceFlowRecoveryUse(ctx)
-	return m.RecoveryStrategies(ctx).Strategy(activeRecoveryStrategy)
+	as := m.Config().SelfServiceFlowRecoveryUse(ctx)
+	s, err := m.RecoveryStrategies(ctx).Strategy(as)
+	if err != nil {
+		return nil, errors.WithStack(herodot.ErrBadRequest.
+			WithReasonf("The active recovery strategy %s is not enabled. Please enable it in the configuration.", as))
+	}
+	return s, nil
 }
 
 func (m *RegistryDefault) AllRecoveryStrategies() (recoveryStrategies recovery.Strategies) {

diff --git a/driver/registry_default_test.go b/driver/registry_default_test.go
@@ -869,3 +869,57 @@ func TestDefaultRegistry_AllStrategies(t *testing.T) {
 		}
 	})
 }
+
+func TestGetActiveRecoveryStrategy(t *testing.T) {
+	t.Parallel()
+	conf, reg := internal.NewVeryFastRegistryWithoutDB(t)
+	t.Run("returns error if active strategy is disabled", func(t *testing.T) {
+		conf.Set(context.Background(), "selfservice.methods.code.enabled", false)
+		conf.Set(context.Background(), config.ViperKeySelfServiceRecoveryUse, "code")
+
+		_, err := reg.GetActiveRecoveryStrategy(context.Background())
+		require.Error(t, err)
+	})
+
+	t.Run("returns active strategy", func(t *testing.T) {
+		for _, sID := range []string{
+			"code", "link",
+		} {
+			t.Run(fmt.Sprintf("strategy=%s", sID), func(t *testing.T) {
+				conf.Set(context.Background(), fmt.Sprintf("selfservice.methods.%s.enabled", sID), true)
+				conf.Set(context.Background(), config.ViperKeySelfServiceRecoveryUse, sID)
+
+				s, err := reg.GetActiveRecoveryStrategy(context.Background())
+				require.NoError(t, err)
+				require.Equal(t, sID, s.RecoveryStrategyID())
+			})
+		}
+	})
+}
+
+func TestGetActiveVerificationStrategy(t *testing.T) {
+	t.Parallel()
+	conf, reg := internal.NewVeryFastRegistryWithoutDB(t)
+	t.Run("returns error if active strategy is disabled", func(t *testing.T) {
+		conf.Set(context.Background(), "selfservice.methods.code.enabled", false)
+		conf.Set(context.Background(), config.ViperKeySelfServiceVerificationUse, "code")
+
+		_, err := reg.GetActiveVerificationStrategy(context.Background())
+		require.Error(t, err)
+	})
+
+	t.Run("returns active strategy", func(t *testing.T) {
+		for _, sID := range []string{
+			"code", "link",
+		} {
+			t.Run(fmt.Sprintf("strategy=%s", sID), func(t *testing.T) {
+				conf.Set(context.Background(), fmt.Sprintf("selfservice.methods.%s.enabled", sID), true)
+				conf.Set(context.Background(), config.ViperKeySelfServiceVerificationUse, sID)
+
+				s, err := reg.GetActiveVerificationStrategy(context.Background())
+				require.NoError(t, err)
+				require.Equal(t, sID, s.VerificationStrategyID())
+			})
+		}
+	})
+}
diff --git a/driver/registry_default_verify.go → driver/registry_default_verification.go b/driver/registry_default_verify.go → driver/registry_default_verification.go
@@ -6,6 +6,9 @@ package driver
 import (
 	"context"
 
+	"github.com/pkg/errors"
+
+	"github.com/ory/herodot"
 	"github.com/ory/kratos/driver/config"
 	"github.com/ory/kratos/identity"
 	"github.com/ory/kratos/selfservice/flow/verification"
@@ -51,8 +54,13 @@ func (m *RegistryDefault) LinkSender() *link.Sender {
 // GetActiveVerificationStrategy returns the currently active verification strategy
 // If no verification strategy has been set, an error is returned
 func (m *RegistryDefault) GetActiveVerificationStrategy(ctx context.Context) (verification.Strategy, error) {
-	activeVerificationStrategy := m.Config().SelfServiceFlowVerificationUse(ctx)
-	return m.VerificationStrategies(ctx).Strategy(activeVerificationStrategy)
+	as := m.Config().SelfServiceFlowVerificationUse(ctx)
+	s, err := m.VerificationStrategies(ctx).Strategy(as)
+	if err != nil {
+		return nil, errors.WithStack(herodot.ErrBadRequest.
+			WithReasonf("The active verification strategy %s is not enabled. Please enable it in the configuration.", as))
+	}
+	return s, nil
 }
 
 func (m *RegistryDefault) VerificationStrategies(ctx context.Context) (verificationStrategies verification.Strategies) {

diff --git a/go.mod b/go.mod
@@ -77,7 +77,7 @@ require (
 	github.com/ory/jsonschema/v3 v3.0.7
 	github.com/ory/mail/v3 v3.0.0
 	github.com/ory/nosurf v1.2.7
-	github.com/ory/x v0.0.537
+	github.com/ory/x v0.0.544-0.20230329113955-ae93d0a743db
 	github.com/phayes/freeport v0.0.0-20180830031419-95f893ade6f2
 	github.com/pkg/errors v0.9.1
 	github.com/pquerna/otp v1.4.0

diff --git a/go.sum b/go.sum
@@ -1113,8 +1113,8 @@ github.com/ory/nosurf v1.2.7/go.mod h1:d4L3ZBa7Amv55bqxCBtCs63wSlyaiCkWVl4vKf3OU
 github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2 h1:zm6sDvHy/U9XrGpixwHiuAwpp0Ock6khSVHkrv6lQQU=
 github.com/ory/sessions v1.2.2-0.20220110165800-b09c17334dc2/go.mod h1:dk2InVEVJ0sfLlnXv9EAgkf6ecYs/i80K/zI+bUmuGM=
 github.com/ory/viper v1.7.5/go.mod h1:ypOuyJmEUb3oENywQZRgeAMwqgOyDqwboO1tj3DjTaM=
-github.com/ory/x v0.0.537 h1:FB8Tioza6pihvy/RsVNzX08Qg3/VpIhI9vBnEQ4iFmQ=
-github.com/ory/x v0.0.537/go.mod h1:CQopDsCC9t0tQsddE9UlyRFVEFd2xjKBVcw4nLMMMS0=
+github.com/ory/x v0.0.544-0.20230329113955-ae93d0a743db h1:vLipG1wUGPSRkd4f6HoLcruDu0rNxS91GnzDyyuAnKI=
+github.com/ory/x v0.0.544-0.20230329113955-ae93d0a743db/go.mod h1:ktXUvx51Ok1gMGr3ysvktanqr+eiB4FXglt4nF4w2Uo=
 github.com/otiai10/copy v1.2.0/go.mod h1:rrF5dJ5F0t/EWSYODDu4j9/vEeYHMkc8jt0zJChqQWw=
 github.com/otiai10/curr v0.0.0-20150429015615-9b4961190c95/go.mod h1:9qAhocn7zKJG+0mI8eUu6xqkFDYS2kb2saOteoSB3cE=
 github.com/otiai10/curr v1.0.0/go.mod h1:LskTG5wDwr8Rs+nNQ+1LlxRjAtTZZjtJW4rMXl6j4vs=
@@ -1519,8 +1519,8 @@ go.uber.org/atomic v1.6.0/go.mod h1:sABNBOSYdrvTF6hTgEIbc7YasKWGhgEQZyfxyTvoXHQ=
 go.uber.org/atomic v1.7.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/atomic v1.10.0 h1:9qC72Qh0+3MqyJbAn8YU5xVq1frD8bn3JtD2oXtafVQ=
 go.uber.org/atomic v1.10.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
-go.uber.org/goleak v1.1.11 h1:wy28qYRKZgnJTxGxvye5/wgWr1EKjmUDGYox5mGlRlI=
 go.uber.org/goleak v1.1.11/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
+go.uber.org/goleak v1.2.1 h1:NBol2c7O1ZokfZ0LEU9K6Whx/KnwvepVetCUhtKja4A=
 go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
 go.uber.org/multierr v1.3.0/go.mod h1:VgVr7evmIr6uPjLBxg28wmKNXyqE9akIJ5XnfpiKl+4=
 go.uber.org/multierr v1.5.0/go.mod h1:FeouvMocqHpRaaGuG9EjoKcStLC43Zu/fmqdUMPcKYU=

diff --git a/...covery/.snapshots/TestHandleError-flow=api-case=fails_if_active_strategy_is_disabled.json b/...covery/.snapshots/TestHandleError-flow=api-case=fails_if_active_strategy_is_disabled.json
@@ -0,0 +1,70 @@
+{
+  "type": "api",
+  "request_url": "http:///",
+  "active": "link",
+  "ui": {
+    "method": "POST",
+    "nodes": [
+      {
+        "type": "input",
+        "group": "default",
+        "attributes": {
+          "name": "csrf_token",
+          "type": "hidden",
+          "required": true,
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {}
+      },
+      {
+        "type": "input",
+        "group": "code",
+        "attributes": {
+          "name": "email",
+          "type": "email",
+          "required": true,
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {
+          "label": {
+            "id": 1070007,
+            "text": "Email",
+            "type": "info"
+          }
+        }
+      },
+      {
+        "type": "input",
+        "group": "code",
+        "attributes": {
+          "name": "method",
+          "type": "submit",
+          "value": "code",
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {
+          "label": {
+            "id": 1070005,
+            "text": "Submit",
+            "type": "info"
+          }
+        }
+      }
+    ],
+    "messages": [
+      {
+        "id": 4000001,
+        "text": "The active recovery strategy code is not enabled. Please enable it in the configuration.",
+        "type": "error"
+      }
+    ]
+  },
+  "state": "choose_method"
+}
+
diff --git a/...ndleError-flow=browser-case=fails_to_retry_flow_if_recovery_strategy_id_is_not_valid.json b/...ndleError-flow=browser-case=fails_to_retry_flow_if_recovery_strategy_id_is_not_valid.json
@@ -0,0 +1,6 @@
+{
+  "code": 400,
+  "message": "The request was malformed or contained invalid parameters",
+  "reason": "The active recovery strategy not-valid is not enabled. Please enable it in the configuration.",
+  "status": "Bad Request"
+}
diff --git a/...covery/.snapshots/TestHandleError-flow=spa-case=fails_if_active_strategy_is_disabled.json b/...covery/.snapshots/TestHandleError-flow=spa-case=fails_if_active_strategy_is_disabled.json
@@ -0,0 +1,70 @@
+{
+  "type": "browser",
+  "request_url": "http:///",
+  "active": "link",
+  "ui": {
+    "method": "POST",
+    "nodes": [
+      {
+        "type": "input",
+        "group": "default",
+        "attributes": {
+          "name": "csrf_token",
+          "type": "hidden",
+          "required": true,
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {}
+      },
+      {
+        "type": "input",
+        "group": "code",
+        "attributes": {
+          "name": "email",
+          "type": "email",
+          "required": true,
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {
+          "label": {
+            "id": 1070007,
+            "text": "Email",
+            "type": "info"
+          }
+        }
+      },
+      {
+        "type": "input",
+        "group": "code",
+        "attributes": {
+          "name": "method",
+          "type": "submit",
+          "value": "code",
+          "disabled": false,
+          "node_type": "input"
+        },
+        "messages": [],
+        "meta": {
+          "label": {
+            "id": 1070005,
+            "text": "Submit",
+            "type": "info"
+          }
+        }
+      }
+    ],
+    "messages": [
+      {
+        "id": 4000001,
+        "text": "The active recovery strategy code is not enabled. Please enable it in the configuration.",
+        "type": "error"
+      }
+    ]
+  },
+  "state": "choose_method"
+}
+
diff --git a/selfservice/flow/recovery/error_test.go b/selfservice/flow/recovery/error_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/gofrs/uuid"
 
 	"github.com/ory/x/jsonx"
+	"github.com/ory/x/snapshotx"
 
 	"github.com/ory/kratos/ui/node"
 
@@ -182,6 +183,26 @@ func TestHandleError(t *testing.T) {
 				require.NoError(t, err)
 				assert.JSONEq(t, x.MustEncodeJSON(t, flowError), gjson.GetBytes(body, "error").Raw)
 			})
+
+			t.Run("case=fails if active strategy is disabled", func(t *testing.T) {
+				c, reg := internal.NewVeryFastRegistryWithoutDB(t)
+				c.Set(context.Background(), "selfservice.methods.code.enabled", false)
+				c.Set(context.Background(), config.ViperKeySelfServiceRecoveryUse, "code")
+				_, err := reg.GetActiveRecoveryStrategy(context.Background())
+				recoveryFlow = newFlow(t, time.Minute, tc.t)
+				flowError = err
+				methodName = recovery.StrategyRecoveryLinkName
+
+				res, err := ts.Client().Do(testhelpers.NewHTTPGetJSONRequest(t, ts.URL+"/error"))
+				require.NoError(t, err)
+				defer res.Body.Close()
+				require.Equal(t, http.StatusBadRequest, res.StatusCode)
+
+				body, err := io.ReadAll(res.Body)
+				require.NoError(t, err)
+
+				snapshotx.SnapshotTJSON(t, body, snapshotx.ExceptPaths("id", "expires_at", "issued_at", "ui.action", "ui.nodes.0.attributes.value"))
+			})
 		})
 	}
 
@@ -272,7 +293,7 @@ func TestHandleError(t *testing.T) {
 
 			conf.MustSet(ctx, config.ViperKeySelfServiceRecoveryUse, "not-valid")
 			sse, _ := expectErrorUI(t)
-			assertx.EqualAsJSON(t, herodot.ErrInternalServerError.WithReason("unable to find strategy for not-valid have [code]"), sse)
+			snapshotx.SnapshotT(t, sse)
 		})
 	})
 }
diff --git a/selfservice/flow/recovery/handler.go b/selfservice/flow/recovery/handler.go
@@ -120,11 +120,15 @@ func (h *Handler) RegisterAdminRoutes(admin *x.RouterAdmin) {
 //	  400: errorGeneric
 //	  default: errorGeneric
 func (h *Handler) createNativeRecoveryFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
-	activeRecoveryStrategy, err := h.d.GetActiveRecoveryStrategy(r.Context())
-	if !h.d.Config().SelfServiceFlowRecoveryEnabled(r.Context()) || err != nil {
+	if !h.d.Config().SelfServiceFlowRecoveryEnabled(r.Context()) {
 		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Recovery is not allowed because it was disabled.")))
 		return
 	}
+	activeRecoveryStrategy, err := h.d.GetActiveRecoveryStrategy(r.Context())
+	if err != nil {
+		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+		return
+	}
 
 	req, err := NewFlow(h.d.Config(), h.d.Config().SelfServiceFlowRecoveryRequestLifespan(r.Context()), h.d.GenerateCSRFToken(r), r, activeRecoveryStrategy, flow.TypeAPI)
 	if err != nil {
@@ -181,12 +185,16 @@ type createBrowserRecoveryFlow struct {
 //	  400: errorGeneric
 //	  default: errorGeneric
 func (h *Handler) createBrowserRecoveryFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) {
-	activeRecoveryStrategy, err := h.d.GetActiveRecoveryStrategy(r.Context())
 
-	if !h.d.Config().SelfServiceFlowRecoveryEnabled(r.Context()) || err != nil {
+	if !h.d.Config().SelfServiceFlowRecoveryEnabled(r.Context()) {
 		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Recovery is not allowed because it was disabled.")))
 		return
 	}
+	activeRecoveryStrategy, err := h.d.GetActiveRecoveryStrategy(r.Context())
+	if err != nil {
+		h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err)
+		return
+	}
 
 	f, err := NewFlow(h.d.Config(), h.d.Config().SelfServiceFlowRecoveryRequestLifespan(r.Context()), h.d.GenerateCSRFToken(r), r, activeRecoveryStrategy, flow.TypeBrowser)
 	if err != nil {

diff --git a/selfservice/flow/recovery/strategy.go b/selfservice/flow/recovery/strategy.go
@@ -53,14 +53,6 @@ func (s Strategies) Strategy(id string) (Strategy, error) {
 	return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("unable to find strategy for %s have %v", id, ids))
 }
 
-func (s Strategies) MustStrategy(id string) Strategy {
-	strategy, err := s.Strategy(id)
-	if err != nil {
-		panic(err)
-	}
-	return strategy
-}
-
 func (s Strategies) RegisterPublicRoutes(r *x.RouterPublic) {
 	for _, ss := range s {
 		if h, ok := ss.(PublicHandler); ok {

diff --git a/...ndleError-flow=browser-case=fails_to_retry_flow_if_recovery_strategy_id_is_not_valid.json b/...ndleError-flow=browser-case=fails_to_retry_flow_if_recovery_strategy_id_is_not_valid.json
@@ -0,0 +1,6 @@
+{
+  "code": 400,
+  "message": "The request was malformed or contained invalid parameters",
+  "reason": "The active verification strategy not-valid is not enabled. Please enable it in the configuration.",
+  "status": "Bad Request"
+}