From 38a11deffaec8168093d94b8f0fb554dfee13dd3 Mon Sep 17 00:00:00 2001 From: Frederic Jahn Date: Wed, 28 Aug 2024 11:56:09 +0200 Subject: [PATCH] fix: only use transaction connection in a transaction (#1598) * fix: only use transaction connection in a transaction * test: fix webhook tests --- .../credential_usage/action_password_login.go | 2 +- .../action_resend_passcode.go | 4 ++-- .../credential_usage/hook_send_passcode.go | 4 ++-- .../flow/profile/action_account_delete.go | 2 +- .../action_register_login_identifier.go | 2 +- backend/flow_api/services/passcode.go | 6 ++--- backend/handler/admin_router.go | 2 +- backend/handler/passcode.go | 4 ++-- backend/handler/public_router.go | 2 +- backend/handler/thirdparty.go | 2 +- backend/handler/user.go | 4 ++-- backend/handler/user_admin.go | 4 ++-- backend/middleware/webhook.go | 2 +- backend/webhooks/manager.go | 13 +++++----- backend/webhooks/manager_test.go | 24 +++++++++---------- backend/webhooks/utils/webhook.go | 7 +++--- backend/webhooks/utils/webhook_test.go | 7 +++--- 17 files changed, 46 insertions(+), 45 deletions(-) diff --git a/backend/flow_api/flow/credential_usage/action_password_login.go b/backend/flow_api/flow/credential_usage/action_password_login.go index f70f6f8ec..69a9f3c0e 100644 --- a/backend/flow_api/flow/credential_usage/action_password_login.go +++ b/backend/flow_api/flow/credential_usage/action_password_login.go @@ -60,7 +60,7 @@ func (a PasswordLogin) Execute(c flowpilot.ExecutionContext) error { var userID uuid.UUID if c.Stash().Get(shared.StashPathEmail).Exists() { - emailModel, err := deps.Persister.GetEmailPersister().FindByAddress(c.Stash().Get(shared.StashPathEmail).String()) + emailModel, err := deps.Persister.GetEmailPersisterWithConnection(deps.Tx).FindByAddress(c.Stash().Get(shared.StashPathEmail).String()) if err != nil { return fmt.Errorf("failed to find user by email: %w", err) } diff --git a/backend/flow_api/flow/credential_usage/action_resend_passcode.go b/backend/flow_api/flow/credential_usage/action_resend_passcode.go index 1360b3f7d..b6e33a841 100644 --- a/backend/flow_api/flow/credential_usage/action_resend_passcode.go +++ b/backend/flow_api/flow/credential_usage/action_resend_passcode.go @@ -59,7 +59,7 @@ func (a ReSendPasscode) Execute(c flowpilot.ExecutionContext) error { EmailAddress: c.Stash().Get(shared.StashPathEmail).String(), Language: deps.HttpContext.Request().Header.Get("Accept-Language"), } - passcodeResult, err := deps.PasscodeService.SendPasscode(sendParams) + passcodeResult, err := deps.PasscodeService.SendPasscode(deps.Tx, sendParams) if err != nil { return fmt.Errorf("passcode service failed: %w", err) } @@ -79,7 +79,7 @@ func (a ReSendPasscode) Execute(c flowpilot.ExecutionContext) error { }, } - err = utils.TriggerWebhooks(deps.HttpContext, events.EmailSend, webhookData) + err = utils.TriggerWebhooks(deps.HttpContext, deps.Tx, events.EmailSend, webhookData) if err != nil { return fmt.Errorf("failed to trigger webhook: %w", err) } diff --git a/backend/flow_api/flow/credential_usage/hook_send_passcode.go b/backend/flow_api/flow/credential_usage/hook_send_passcode.go index e0d86a746..085eb25cb 100644 --- a/backend/flow_api/flow/credential_usage/hook_send_passcode.go +++ b/backend/flow_api/flow/credential_usage/hook_send_passcode.go @@ -70,7 +70,7 @@ func (h SendPasscode) Execute(c flowpilot.HookExecutionContext) error { Language: deps.HttpContext.Request().Header.Get("Accept-Language"), } - passcodeResult, err := deps.PasscodeService.SendPasscode(sendParams) + passcodeResult, err := deps.PasscodeService.SendPasscode(deps.Tx, sendParams) if err != nil { return fmt.Errorf("passcode service failed: %w", err) } @@ -100,7 +100,7 @@ func (h SendPasscode) Execute(c flowpilot.HookExecutionContext) error { }, } - err = utils.TriggerWebhooks(deps.HttpContext, events.EmailSend, webhookData) + err = utils.TriggerWebhooks(deps.HttpContext, deps.Tx, events.EmailSend, webhookData) if err != nil { return fmt.Errorf("failed to trigger webhook: %w", err) } diff --git a/backend/flow_api/flow/profile/action_account_delete.go b/backend/flow_api/flow/profile/action_account_delete.go index 22b1919e4..01539e3a9 100644 --- a/backend/flow_api/flow/profile/action_account_delete.go +++ b/backend/flow_api/flow/profile/action_account_delete.go @@ -63,7 +63,7 @@ func (a AccountDelete) Execute(c flowpilot.ExecutionContext) error { deps.HttpContext.SetCookie(cookie) - err = utils.TriggerWebhooks(deps.HttpContext, events.UserDelete, admin.FromUserModel(*userModel)) + err = utils.TriggerWebhooks(deps.HttpContext, deps.Tx, events.UserDelete, admin.FromUserModel(*userModel)) if err != nil { return fmt.Errorf("failed to trrigger webhook: %w", err) } diff --git a/backend/flow_api/flow/registration/action_register_login_identifier.go b/backend/flow_api/flow/registration/action_register_login_identifier.go index ef570ec77..dbe5fc37e 100644 --- a/backend/flow_api/flow/registration/action_register_login_identifier.go +++ b/backend/flow_api/flow/registration/action_register_login_identifier.go @@ -113,7 +113,7 @@ func (a RegisterLoginIdentifier) Execute(c flowpilot.ExecutionContext) error { // Check that email is not already taken // this check is non-exhaustive as the email is not blocked here and might be created after the check here and the user creation - emailModel, err := deps.Persister.GetEmailPersister().FindByAddress(email) + emailModel, err := deps.Persister.GetEmailPersisterWithConnection(deps.Tx).FindByAddress(email) if err != nil { return err } diff --git a/backend/flow_api/services/passcode.go b/backend/flow_api/services/passcode.go index 61b7a103b..c2a5fa9cc 100644 --- a/backend/flow_api/services/passcode.go +++ b/backend/flow_api/services/passcode.go @@ -42,7 +42,7 @@ type SendPasscodeResult struct { type Passcode interface { ValidatePasscode(ValidatePasscodeParams) (bool, error) - SendPasscode(SendPasscodeParams) (*SendPasscodeResult, error) + SendPasscode(*pop.Connection, SendPasscodeParams) (*SendPasscodeResult, error) VerifyPasscodeCode(tx *pop.Connection, passcodeID uuid.UUID, passcode string) error } @@ -110,7 +110,7 @@ func (s *passcode) VerifyPasscodeCode(tx *pop.Connection, passcodeID uuid.UUID, return nil } -func (s *passcode) SendPasscode(p SendPasscodeParams) (*SendPasscodeResult, error) { +func (s *passcode) SendPasscode(tx *pop.Connection, p SendPasscodeParams) (*SendPasscodeResult, error) { code, err := s.passcodeGenerator.Generate() if err != nil { return nil, err @@ -135,7 +135,7 @@ func (s *passcode) SendPasscode(p SendPasscodeParams) (*SendPasscodeResult, erro UpdatedAt: now, } - err = s.persister.GetPasscodePersister().Create(passcodeModel) + err = s.persister.GetPasscodePersisterWithConnection(tx).Create(passcodeModel) if err != nil { return nil, err } diff --git a/backend/handler/admin_router.go b/backend/handler/admin_router.go index e4116a62e..2bbd2fa87 100644 --- a/backend/handler/admin_router.go +++ b/backend/handler/admin_router.go @@ -49,7 +49,7 @@ func NewAdminRouter(cfg *config.Config, persister persistence.Persister, prometh panic(fmt.Errorf("failed to create jwk manager: %w", err)) } - webhookMiddleware := hankoMiddleware.WebhookMiddleware(cfg, jwkManager, persister.GetWebhookPersister(nil)) + webhookMiddleware := hankoMiddleware.WebhookMiddleware(cfg, jwkManager, persister) userHandler := NewUserHandlerAdmin(persister) emailHandler := NewEmailAdminHandler(cfg, persister) diff --git a/backend/handler/passcode.go b/backend/handler/passcode.go index 9d082f0f2..e5cd8d5bf 100644 --- a/backend/handler/passcode.go +++ b/backend/handler/passcode.go @@ -228,14 +228,14 @@ func (h *PasscodeHandler) Init(c echo.Context) error { return fmt.Errorf("failed to send passcode: %w", err) } - err = utils.TriggerWebhooks(c, events.EmailSend, webhookData) + err = utils.TriggerWebhooks(c, h.persister.GetConnection(), events.EmailSend, webhookData) if err != nil { zeroLogger.Warn().Err(err).Msg("failed to trigger webhook") } } else { webhookData.DeliveredByHanko = false - err = utils.TriggerWebhooks(c, events.EmailSend, webhookData) + err = utils.TriggerWebhooks(c, h.persister.GetConnection(), events.EmailSend, webhookData) if err != nil { return fmt.Errorf(fmt.Sprintf("failed to trigger webhook: %s", err)) diff --git a/backend/handler/public_router.go b/backend/handler/public_router.go index 68cc52e68..4639acfe3 100644 --- a/backend/handler/public_router.go +++ b/backend/handler/public_router.go @@ -77,7 +77,7 @@ func NewPublicRouter(cfg *config.Config, persister persistence.Persister, promet sessionMiddleware := hankoMiddleware.Session(cfg, sessionManager) - webhookMiddleware := hankoMiddleware.WebhookMiddleware(cfg, jwkManager, persister.GetWebhookPersister(nil)) + webhookMiddleware := hankoMiddleware.WebhookMiddleware(cfg, jwkManager, persister) e.POST("/registration", flowAPIHandler.RegistrationFlowHandler, webhookMiddleware) e.POST("/login", flowAPIHandler.LoginFlowHandler, webhookMiddleware) diff --git a/backend/handler/thirdparty.go b/backend/handler/thirdparty.go index f4437848e..d0f1c850d 100644 --- a/backend/handler/thirdparty.go +++ b/backend/handler/thirdparty.go @@ -201,7 +201,7 @@ func (h *ThirdPartyHandler) Callback(c echo.Context) error { } if accountLinkingResult.WebhookEvent != nil { - err = webhookUtils.TriggerWebhooks(c, *accountLinkingResult.WebhookEvent, admin.FromUserModel(*accountLinkingResult.User)) + err = webhookUtils.TriggerWebhooks(c, h.persister.GetConnection(), *accountLinkingResult.WebhookEvent, admin.FromUserModel(*accountLinkingResult.User)) if err != nil { c.Logger().Warn(err) } diff --git a/backend/handler/user.go b/backend/handler/user.go index d4d753ec4..7304ded6b 100644 --- a/backend/handler/user.go +++ b/backend/handler/user.go @@ -158,7 +158,7 @@ func (h *UserHandler) Create(c echo.Context) error { } if !h.cfg.Email.RequireVerification { - err = utils.TriggerWebhooks(c, events.UserCreate, admin.FromUserModel(newUser)) + err = utils.TriggerWebhooks(c, tx, events.UserCreate, admin.FromUserModel(newUser)) if err != nil { c.Logger().Warn(err) } @@ -288,7 +288,7 @@ func (h *UserHandler) Delete(c echo.Context) error { c.SetCookie(cookie) - err = utils.TriggerWebhooks(c, events.UserDelete, admin.FromUserModel(*user)) + err = utils.TriggerWebhooks(c, tx, events.UserDelete, admin.FromUserModel(*user)) if err != nil { c.Logger().Warn(err) } diff --git a/backend/handler/user_admin.go b/backend/handler/user_admin.go index 68f87bfc3..d4a693a01 100644 --- a/backend/handler/user_admin.go +++ b/backend/handler/user_admin.go @@ -51,7 +51,7 @@ func (h *UserHandlerAdmin) Delete(c echo.Context) error { return fmt.Errorf("failed to delete user: %w", err) } - err = utils.TriggerWebhooks(c, events.UserDelete, admin.FromUserModel(*user)) + err = utils.TriggerWebhooks(c, h.persister.GetConnection(), events.UserDelete, admin.FromUserModel(*user)) if err != nil { c.Logger().Warn(err) } @@ -284,7 +284,7 @@ func (h *UserHandlerAdmin) Create(c echo.Context) error { userDto := admin.FromUserModel(*user) - err = utils.TriggerWebhooks(c, events.UserCreate, userDto) + err = utils.TriggerWebhooks(c, h.persister.GetConnection(), events.UserCreate, userDto) if err != nil { c.Logger().Warn(err) } diff --git a/backend/middleware/webhook.go b/backend/middleware/webhook.go index 20f447e5d..b874cdda4 100644 --- a/backend/middleware/webhook.go +++ b/backend/middleware/webhook.go @@ -8,7 +8,7 @@ import ( "github.com/teamhanko/hanko/backend/webhooks" ) -func WebhookMiddleware(cfg *config.Config, jwkManager hankoJwk.Manager, persister persistence.WebhookPersister) echo.MiddlewareFunc { +func WebhookMiddleware(cfg *config.Config, jwkManager hankoJwk.Manager, persister persistence.Persister) echo.MiddlewareFunc { return func(next echo.HandlerFunc) echo.HandlerFunc { return func(ctx echo.Context) error { diff --git a/backend/webhooks/manager.go b/backend/webhooks/manager.go index d2ab4b9ea..eca9e8fa2 100644 --- a/backend/webhooks/manager.go +++ b/backend/webhooks/manager.go @@ -2,6 +2,7 @@ package webhooks import ( "fmt" + "github.com/gobuffalo/pop/v6" "github.com/labstack/echo/v4" "github.com/lestrrat-go/jwx/v2/jwt" "github.com/teamhanko/hanko/backend/config" @@ -13,7 +14,7 @@ import ( ) type Manager interface { - Trigger(evt events.Event, data interface{}) + Trigger(tx *pop.Connection, evt events.Event, data interface{}) GenerateJWT(data interface{}, event events.Event) (string, error) } @@ -22,11 +23,11 @@ type manager struct { webhooks Webhooks jwtGenerator hankoJwt.Generator audience []string - persister persistence.WebhookPersister + persister persistence.Persister canExpireAtTime bool } -func NewManager(cfg *config.Config, persister persistence.WebhookPersister, jwkManager hankoJwk.Manager, logger echo.Logger) (Manager, error) { +func NewManager(cfg *config.Config, persister persistence.Persister, jwkManager hankoJwk.Manager, logger echo.Logger) (Manager, error) { hooks := make(Webhooks, 0) if cfg.Webhooks.Enabled { @@ -73,9 +74,9 @@ func NewManager(cfg *config.Config, persister persistence.WebhookPersister, jwkM }, nil } -func (m *manager) Trigger(evt events.Event, data interface{}) { +func (m *manager) Trigger(tx *pop.Connection, evt events.Event, data interface{}) { // add db hooks - Done here to prevent a restart in case a hook is added or removed from the database - dbHooks, err := m.persister.List(false) + dbHooks, err := m.persister.GetWebhookPersister(tx).List(false) if err != nil { m.logger.Error(fmt.Errorf("unable to get database webhooks: %w", err)) return @@ -83,7 +84,7 @@ func (m *manager) Trigger(evt events.Event, data interface{}) { hooks := m.webhooks for _, dbHook := range dbHooks { - hooks = append(hooks, NewDatabaseHook(dbHook, m.persister, m.logger)) + hooks = append(hooks, NewDatabaseHook(dbHook, m.persister.GetWebhookPersister(nil), m.logger)) } dataToken, err := m.GenerateJWT(data, evt) diff --git a/backend/webhooks/manager_test.go b/backend/webhooks/manager_test.go index 08ba2a31e..fce8c57f2 100644 --- a/backend/webhooks/manager_test.go +++ b/backend/webhooks/manager_test.go @@ -27,7 +27,7 @@ func (s *managerSuite) TestNewManager() { cfg := config.Config{} jwkManager := test.JwkManager{} - manager, err := NewManager(&cfg, s.Storage.GetWebhookPersister(nil), jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.NoError(err) s.NotEmpty(manager) } @@ -36,7 +36,7 @@ func (s *managerSuite) TestManager_GenerateJWT() { cfg := config.Config{} jwkManager := test.JwkManager{} - manager, err := NewManager(&cfg, s.Storage.GetWebhookPersister(nil), jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) testData := "lorem-ipsum" @@ -55,10 +55,10 @@ func (s *managerSuite) TestManager_TriggerWithoutHook() { cfg := config.Config{} jwkManager := test.JwkManager{} - manager, err := NewManager(&cfg, s.Storage.GetWebhookPersister(nil), jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.Require().NoError(err) - manager.Trigger(events.UserCreate, "lorem-ipsum") + manager.Trigger(s.Storage.GetConnection(), events.UserCreate, "lorem-ipsum") // give it 1 sec to trigger time.Sleep(1 * time.Second) @@ -87,10 +87,10 @@ func (s *managerSuite) TestManager_TriggerWithConfigHook() { } jwkManager := test.JwkManager{} - manager, err := NewManager(&cfg, s.Storage.GetWebhookPersister(nil), jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.Require().NoError(err) - manager.Trigger(events.UserCreate, "lorem-ipsum") + manager.Trigger(s.Storage.GetConnection(), events.UserCreate, "lorem-ipsum") // give it 1 sec to trigger time.Sleep(1 * time.Second) @@ -120,10 +120,10 @@ func (s *managerSuite) TestManager_TriggerWithDisabledConfigHook() { } jwkManager := test.JwkManager{} - manager, err := NewManager(&cfg, s.Storage.GetWebhookPersister(nil), jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.Require().NoError(err) - manager.Trigger(events.UserCreate, "lorem-ipsum") + manager.Trigger(s.Storage.GetConnection(), events.UserCreate, "lorem-ipsum") // give it 1 sec to trigger time.Sleep(1 * time.Second) @@ -145,10 +145,10 @@ func (s *managerSuite) TestManager_TriggerWithDbHook() { s.createTestDatabaseWebhook(persister, true, server.URL) - manager, err := NewManager(&cfg, persister, jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.Require().NoError(err) - manager.Trigger(events.UserCreate, "lorem-ipsum") + manager.Trigger(s.Storage.GetConnection(), events.UserCreate, "lorem-ipsum") // give it 1 sec to trigger time.Sleep(1 * time.Second) @@ -169,10 +169,10 @@ func (s *managerSuite) TestManager_TriggerWithDisabledDbHook() { s.createTestDatabaseWebhook(persister, false, server.URL) - manager, err := NewManager(&cfg, persister, jwkManager, nil) + manager, err := NewManager(&cfg, s.Storage, jwkManager, nil) s.Require().NoError(err) - manager.Trigger(events.UserCreate, "lorem-ipsum") + manager.Trigger(s.Storage.GetConnection(), events.UserCreate, "lorem-ipsum") // give it 1 sec to trigger time.Sleep(1 * time.Second) diff --git a/backend/webhooks/utils/webhook.go b/backend/webhooks/utils/webhook.go index 0b96efaa9..3e20e7be5 100644 --- a/backend/webhooks/utils/webhook.go +++ b/backend/webhooks/utils/webhook.go @@ -11,17 +11,16 @@ import ( "github.com/teamhanko/hanko/backend/webhooks/events" ) -func TriggerWebhooks(ctx echo.Context, evt events.Event, data interface{}) error { +func TriggerWebhooks(ctx echo.Context, tx *pop.Connection, evt events.Event, data interface{}) error { webhookCtx := ctx.Get("webhook_manager") if webhookCtx == nil { return fmt.Errorf("unable to load webhooks manager from webhook middleware") } webhookManager := webhookCtx.(webhooks.Manager) - webhookManager.Trigger(evt, data) + webhookManager.Trigger(tx, evt, data) return nil - } func NotifyUserChange(ctx echo.Context, tx *pop.Connection, persister persistence.Persister, event events.Event, userId uuid.UUID) { @@ -31,7 +30,7 @@ func NotifyUserChange(ctx echo.Context, tx *pop.Connection, persister persistenc return } - err = TriggerWebhooks(ctx, event, admin.FromUserModel(*updatedUser)) + err = TriggerWebhooks(ctx, tx, event, admin.FromUserModel(*updatedUser)) if err != nil { ctx.Logger().Warn(err) } diff --git a/backend/webhooks/utils/webhook_test.go b/backend/webhooks/utils/webhook_test.go index c0bd5f76c..ab424160e 100644 --- a/backend/webhooks/utils/webhook_test.go +++ b/backend/webhooks/utils/webhook_test.go @@ -1,6 +1,7 @@ package utils import ( + "github.com/gobuffalo/pop/v6" "github.com/labstack/echo/v4" "github.com/stretchr/testify/require" "github.com/teamhanko/hanko/backend/webhooks/events" @@ -13,7 +14,7 @@ type testManager struct { TestFunc func() } -func (tm *testManager) Trigger(evt events.Event, data interface{}) { +func (tm *testManager) Trigger(tx *pop.Connection, evt events.Event, data interface{}) { tm.TestFunc() } @@ -28,7 +29,7 @@ func TestWebhook_TriggerWithoutManager(t *testing.T) { ctx := e.NewContext(req, rec) - err := TriggerWebhooks(ctx, "user", "lorem") + err := TriggerWebhooks(ctx, nil, "user", "lorem") require.Error(t, err) err = e.Close() @@ -47,7 +48,7 @@ func TestWebhook_Trigger(t *testing.T) { ctx := e.NewContext(req, rec) ctx.Set("webhook_manager", tm) - err := TriggerWebhooks(ctx, "user", "lorem") + err := TriggerWebhooks(ctx, nil, "user", "lorem") require.NoError(t, err) err = e.Close()