fix: do not double-commit webhooks on registration (#2888)

ory · Nov 14, 2022 · 88e75d9 · 88e75d9
1 parent 5bce0b9
commit 88e75d9
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 9 deletions.
diff --git a/selfservice/hook/web_hook.go b/selfservice/hook/web_hook.go
@@ -32,11 +32,17 @@ import (
 	"github.com/ory/x/otelx"
 )
 
-var _ registration.PostHookPostPersistExecutor = new(WebHook)
-var _ registration.PostHookPrePersistExecutor = new(WebHook)
-var _ verification.PostHookExecutor = new(WebHook)
-var _ recovery.PostHookExecutor = new(WebHook)
-var _ settings.PostHookPostPersistExecutor = new(WebHook)
+var (
+	_ registration.PostHookPostPersistExecutor = new(WebHook)
+	_ registration.PostHookPrePersistExecutor  = new(WebHook)
+
+	_ verification.PostHookExecutor = new(WebHook)
+
+	_ recovery.PostHookExecutor = new(WebHook)
+
+	_ settings.PostHookPostPersistExecutor = new(WebHook)
+	_ settings.PostHookPrePersistExecutor  = new(WebHook)
+)
 
 type (
 	webHookDependencies interface {
@@ -155,6 +161,10 @@ func (e *WebHook) ExecuteRegistrationPreHook(_ http.ResponseWriter, req *http.Re
 
 func (e *WebHook) ExecutePostRegistrationPrePersistHook(_ http.ResponseWriter, req *http.Request, flow *registration.Flow, id *identity.Identity) error {
 	ctx, _ := e.deps.Tracer(req.Context()).Tracer().Start(req.Context(), "selfservice.hook.ExecutePostRegistrationPrePersistHook")
+	if !gjson.GetBytes(e.conf, "can_interrupt").Bool() {
+		return nil
+	}
+
 	return e.execute(ctx, &templateContext{
 		Flow:           flow,
 		RequestHeaders: req.Header,
@@ -166,6 +176,10 @@ func (e *WebHook) ExecutePostRegistrationPrePersistHook(_ http.ResponseWriter, r
 
 func (e *WebHook) ExecutePostRegistrationPostPersistHook(_ http.ResponseWriter, req *http.Request, flow *registration.Flow, session *session.Session) error {
 	ctx, _ := e.deps.Tracer(req.Context()).Tracer().Start(req.Context(), "selfservice.hook.ExecutePostRegistrationPostPersistHook")
+	if gjson.GetBytes(e.conf, "can_interrupt").Bool() {
+		return nil
+	}
+
 	return e.execute(ctx, &templateContext{
 		Flow:           flow,
 		RequestHeaders: req.Header,
@@ -187,6 +201,25 @@ func (e *WebHook) ExecuteSettingsPreHook(_ http.ResponseWriter, req *http.Reques
 
 func (e *WebHook) ExecuteSettingsPostPersistHook(_ http.ResponseWriter, req *http.Request, flow *settings.Flow, id *identity.Identity) error {
 	ctx, _ := e.deps.Tracer(req.Context()).Tracer().Start(req.Context(), "selfservice.hook.ExecuteSettingsPostPersistHook")
+	if gjson.GetBytes(e.conf, "can_interrupt").Bool() {
+		return nil
+	}
+
+	return e.execute(ctx, &templateContext{
+		Flow:           flow,
+		RequestHeaders: req.Header,
+		RequestMethod:  req.Method,
+		RequestURL:     x.RequestURL(req).String(),
+		Identity:       id,
+	})
+}
+
+func (e *WebHook) ExecuteSettingsPrePersistHook(_ http.ResponseWriter, req *http.Request, flow *settings.Flow, id *identity.Identity) error {
+	ctx, _ := e.deps.Tracer(req.Context()).Tracer().Start(req.Context(), "selfservice.hook.ExecuteSettingsPrePersistHook")
+	if !gjson.GetBytes(e.conf, "can_interrupt").Bool() {
+		return nil
+	}
+
 	return e.execute(ctx, &templateContext{
 		Flow:           flow,
 		RequestHeaders: req.Header,

diff --git a/selfservice/hook/web_hook_integration_test.go b/selfservice/hook/web_hook_integration_test.go
@@ -425,15 +425,16 @@ func TestWebHooks(t *testing.T) {
 			expectedError: nil,
 		},
 		{
-			uc:         "Post Registration Post Persists Hook - block",
+			uc:         "Post Registration Post Persist Hook - block has no effect",
 			createFlow: func() flow.Flow { return &registration.Flow{ID: x.NewUUID()} },
 			callWebHook: func(wh *hook.WebHook, req *http.Request, f flow.Flow, s *session.Session) error {
 				return wh.ExecutePostRegistrationPostPersistHook(nil, req, f.(*registration.Flow), s)
 			},
+			// This would usually error, but post persist does not execute blocking web hooks, so we expect no error.
 			webHookResponse: func() (int, []byte) {
 				return http.StatusBadRequest, webHookResponse
 			},
-			expectedError: webhookError,
+			expectedError: nil,
 		},
 		{
 			uc:         "Post Registration Pre Persist Hook - no block",
@@ -513,16 +514,27 @@ func TestWebHooks(t *testing.T) {
 			expectedError: nil,
 		},
 		{
-			uc:         "Post Settings Hook - block",
+			uc:         "Post Settings Hook Pre Persist - block",
 			createFlow: func() flow.Flow { return &settings.Flow{ID: x.NewUUID()} },
 			callWebHook: func(wh *hook.WebHook, req *http.Request, f flow.Flow, s *session.Session) error {
-				return wh.ExecuteSettingsPostPersistHook(nil, req, f.(*settings.Flow), s.Identity)
+				return wh.ExecuteSettingsPrePersistHook(nil, req, f.(*settings.Flow), s.Identity)
 			},
 			webHookResponse: func() (int, []byte) {
 				return http.StatusBadRequest, webHookResponse
 			},
 			expectedError: webhookError,
 		},
+		{
+			uc:         "Post Settings Hook Post Persist - block has no effect",
+			createFlow: func() flow.Flow { return &settings.Flow{ID: x.NewUUID()} },
+			callWebHook: func(wh *hook.WebHook, req *http.Request, f flow.Flow, s *session.Session) error {
+				return wh.ExecuteSettingsPostPersistHook(nil, req, f.(*settings.Flow), s.Identity)
+			},
+			webHookResponse: func() (int, []byte) {
+				return http.StatusBadRequest, webHookResponse
+			},
+			expectedError: nil,
+		},
 	} {
 		t.Run("uc="+tc.uc, func(t *testing.T) {
 			for _, method := range []string{"CONNECT", "DELETE", "GET", "OPTIONS", "PATCH", "POST", "PUT", "TRACE"} {