From 6c8f8a18ec938c4e6d99dd2b947fc326d55939e9 Mon Sep 17 00:00:00 2001
From: Henning Perl <henning.perl@ory.sh>
Date: Tue, 2 Jul 2024 11:36:38 +0200
Subject: [PATCH] improve test coverage

---
 selfservice/hook/password_migration_hook.go |  9 ---------
 selfservice/strategy/password/login.go      |  1 -
 selfservice/strategy/password/login_test.go | 20 ++++++++++++++++++++
 3 files changed, 20 insertions(+), 10 deletions(-)

diff --git a/selfservice/hook/password_migration_hook.go b/selfservice/hook/password_migration_hook.go
index 55a628fc8ca7..065dc5dcddc6 100644
--- a/selfservice/hook/password_migration_hook.go
+++ b/selfservice/hook/password_migration_hook.go
@@ -75,15 +75,6 @@ func (p *PasswordMigration) Execute(ctx context.Context, data *PasswordMigration
 
 	resp, err := httpClient.Do(req)
 	if err != nil {
-		if isTimeoutError(err) {
-			return herodot.DefaultError{
-				CodeField:     http.StatusGatewayTimeout,
-				StatusField:   http.StatusText(http.StatusGatewayTimeout),
-				GRPCCodeField: grpccodes.DeadlineExceeded,
-				ReasonField:   "A third-party upstream service could not be reached. Please try again later.",
-				ErrorField:    "calling the password migration hook failed",
-			}.WithWrap(errors.WithStack(err))
-		}
 		return herodot.DefaultError{
 			CodeField:     http.StatusBadGateway,
 			StatusField:   http.StatusText(http.StatusBadGateway),
diff --git a/selfservice/strategy/password/login.go b/selfservice/strategy/password/login.go
index 7aefa509410f..3600e29b4e0e 100644
--- a/selfservice/strategy/password/login.go
+++ b/selfservice/strategy/password/login.go
@@ -86,7 +86,6 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow,
 	if o.ShouldUsePasswordMigrationHook() {
 		pwHook := s.d.Config().PasswordMigrationHook(r.Context())
 		if !pwHook.Enabled {
-			// TODO: How can we make sure this does not trigger on-call?
 			return nil, errors.WithStack(herodot.ErrInternalServerError.WithReasonf("Password migration hook is not enabled but password migration is requested."))
 		}
 
diff --git a/selfservice/strategy/password/login_test.go b/selfservice/strategy/password/login_test.go
index d4656ed0dea7..17c63a453544 100644
--- a/selfservice/strategy/password/login_test.go
+++ b/selfservice/strategy/password/login_test.go
@@ -882,6 +882,9 @@ func TestCompleteLogin(t *testing.T) {
 				_, _ = io.WriteString(w, `{"status":"password_match"}`)
 
 			// Set up diverse fake passwords to trigger special response status codes:
+			case "timeout":
+				// We'll just block until the request gets cancelled
+				<-r.Context().Done()
 			case "500":
 				w.WriteHeader(http.StatusInternalServerError)
 			case "201":
@@ -905,6 +908,7 @@ func TestCompleteLogin(t *testing.T) {
 		for _, tc := range []struct {
 			name              string
 			addPassword       func(identifier, password string)
+			setupFn           func() func()
 			credentialsConfig string
 			expectSuccess     bool
 		}{{
@@ -937,9 +941,25 @@ func TestCompleteLogin(t *testing.T) {
 			credentialsConfig: `{"use_password_migration_hook": true, "hashed_password":"hash"}`,
 			addPassword:       func(identifier, password string) { passwordDB[identifier] = password },
 			expectSuccess:     false,
+		}, {
+			name:              "should not call migration hook if disabled",
+			credentialsConfig: `{"use_password_migration_hook": true}`,
+			addPassword:       func(identifier, password string) { passwordDB[identifier] = password },
+			setupFn: func() func() {
+				require.NoError(t, reg.Config().Set(ctx, config.ViperKeyPasswordMigrationHook+".enabled", false))
+				return func() {
+					require.NoError(t, reg.Config().Set(ctx, config.ViperKeyPasswordMigrationHook+".enabled", true))
+				}
+			},
+			expectSuccess: false,
 		}} {
 
 			t.Run("case="+tc.name, func(t *testing.T) {
+				if tc.setupFn != nil {
+					cleanup := tc.setupFn()
+					t.Cleanup(cleanup)
+				}
+
 				identifier := x.NewUUID().String()
 				password := x.NewUUID().String()
 				if tc.addPassword != nil {