Skip to content

feat: do not automatically add verification hook #4342

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: do not automatically add verification hook #4342

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 1 addition & 7 deletions driver/registry_default_registration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,13 @@ func (m *RegistryDefault) PostRegistrationPrePersistHooks(ctx context.Context, c
}

func (m *RegistryDefault) PostRegistrationPostPersistHooks(ctx context.Context, credentialsType identity.CredentialsType) (b []registration.PostHookPostPersistExecutor) {
initialHookCount := 0
if m.Config().SelfServiceFlowVerificationEnabled(ctx) {
b = append(b, m.HookVerifier())
initialHookCount = 1
}

for _, v := range m.getHooks(string(credentialsType), m.Config().SelfServiceFlowRegistrationAfterHooks(ctx, string(credentialsType))) {
if hook, ok := v.(registration.PostHookPostPersistExecutor); ok {
b = append(b, hook)
}
}

if len(b) == initialHookCount {
if len(b) == 0 {
// since we don't want merging hooks defined in a specific strategy and
// global hooks are added only if no strategy specific hooks are defined
for _, v := range m.getHooks(config.HookGlobal, m.Config().SelfServiceFlowRegistrationAfterHooks(ctx, config.HookGlobal)) {
Expand Down
8 changes: 1 addition & 7 deletions driver/registry_default_settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,13 @@ func (m *RegistryDefault) PreSettingsHooks(ctx context.Context) (b []settings.Pr
}

func (m *RegistryDefault) PostSettingsPostPersistHooks(ctx context.Context, settingsType string) (b []settings.PostHookPostPersistExecutor) {
initialHookCount := 0
if m.Config().SelfServiceFlowVerificationEnabled(ctx) {
b = append(b, m.HookVerifier())
initialHookCount = 1
}

for _, v := range m.getHooks(settingsType, m.Config().SelfServiceFlowSettingsAfterHooks(ctx, settingsType)) {
if hook, ok := v.(settings.PostHookPostPersistExecutor); ok {
b = append(b, hook)
}
}

if len(b) == initialHookCount {
if len(b) == 0 {
// since we don't want merging hooks defined in a specific strategy and global hooks
// global hooks are added only if no strategy specific hooks are defined
for _, v := range m.getHooks(config.HookGlobal, m.Config().SelfServiceFlowSettingsAfterHooks(ctx, config.HookGlobal)) {
Expand Down
27 changes: 17 additions & 10 deletions driver/registry_default_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,11 +263,24 @@ func TestDriverDefault_Hooks(t *testing.T) {
{
uc: "Only session hook configured for password strategy",
config: map[string]any{
config.ViperKeySelfServiceVerificationEnabled: true,
config.ViperKeySelfServiceRegistrationAfter + ".password.hooks": []map[string]any{
{"hook": "session"},
},
},
expect: func(reg *driver.RegistryDefault) []registration.PostHookPostPersistExecutor {
return []registration.PostHookPostPersistExecutor{
hook.NewSessionIssuer(reg),
}
},
},
{
uc: "Session hook and verification hook configured for password strategy",
config: map[string]any{
config.ViperKeySelfServiceRegistrationAfter + ".password.hooks": []map[string]any{
{"hook": "verification"},
{"hook": "session"},
},
},
expect: func(reg *driver.RegistryDefault) []registration.PostHookPostPersistExecutor {
return []registration.PostHookPostPersistExecutor{
hook.NewVerifier(reg),
Expand All @@ -278,15 +291,13 @@ func TestDriverDefault_Hooks(t *testing.T) {
{
uc: "A session hook and a web_hook are configured for password strategy",
config: map[string]any{
config.ViperKeySelfServiceVerificationEnabled: true,
config.ViperKeySelfServiceRegistrationAfter + ".password.hooks": []map[string]any{
{"hook": "web_hook", "config": map[string]any{"headers": map[string]string{"X-Custom-Header": "test"}, "url": "foo", "method": "POST", "body": "bar"}},
{"hook": "session"},
},
},
expect: func(reg *driver.RegistryDefault) []registration.PostHookPostPersistExecutor {
return []registration.PostHookPostPersistExecutor{
hook.NewVerifier(reg),
hook.NewWebHook(reg, json.RawMessage(`{"body":"bar","headers":{"X-Custom-Header":"test"},"method":"POST","url":"foo"}`)),
hook.NewSessionIssuer(reg),
}
Expand Down Expand Up @@ -317,11 +328,9 @@ func TestDriverDefault_Hooks(t *testing.T) {
config.ViperKeySelfServiceRegistrationAfter + ".hooks": []map[string]any{
{"hook": "web_hook", "config": map[string]any{"url": "bar", "method": "POST", "headers": map[string]string{"X-Custom-Header": "test"}}},
},
config.ViperKeySelfServiceVerificationEnabled: true,
},
expect: func(reg *driver.RegistryDefault) []registration.PostHookPostPersistExecutor {
return []registration.PostHookPostPersistExecutor{
hook.NewVerifier(reg),
hook.NewWebHook(reg, json.RawMessage(`{"headers":{"X-Custom-Header":"test"},"method":"GET","url":"foo"}`)),
hook.NewSessionIssuer(reg),
}
Expand Down Expand Up @@ -553,7 +562,9 @@ func TestDriverDefault_Hooks(t *testing.T) {
{
uc: "Only verify hook configured for the strategy",
config: map[string]any{
config.ViperKeySelfServiceVerificationEnabled: true,
config.ViperKeySelfServiceSettingsAfter + ".profile.hooks": []map[string]any{
{"hook": "verification"},
},
// I think this is a bug as there is a hook named verify defined for both profile and password
// strategies. Instead of using it, the code makes use of the property used above and which
// is defined in an entirely different flow (verification).
Expand All @@ -570,11 +581,9 @@ func TestDriverDefault_Hooks(t *testing.T) {
config.ViperKeySelfServiceSettingsAfter + ".profile.hooks": []map[string]any{
{"hook": "web_hook", "config": map[string]any{"headers": []map[string]string{{"X-Custom-Header": "test"}}, "url": "foo", "method": "POST", "body": "bar"}},
},
config.ViperKeySelfServiceVerificationEnabled: true,
},
expect: func(reg *driver.RegistryDefault) []settings.PostHookPostPersistExecutor {
return []settings.PostHookPostPersistExecutor{
hook.NewVerifier(reg),
hook.NewWebHook(reg, json.RawMessage(`{"body":"bar","headers":[{"X-Custom-Header":"test"}],"method":"POST","url":"foo"}`)),
}
},
Expand All @@ -597,7 +606,6 @@ func TestDriverDefault_Hooks(t *testing.T) {
{
uc: "Hooks are configured on a global level, as well as on a strategy level",
config: map[string]any{
config.ViperKeySelfServiceVerificationEnabled: true,
config.ViperKeySelfServiceSettingsAfter + ".profile.hooks": []map[string]any{
{"hook": "web_hook", "config": map[string]any{"url": "foo", "method": "GET", "headers": map[string]string{"X-Custom-Header": "test"}}},
},
Expand All @@ -607,7 +615,6 @@ func TestDriverDefault_Hooks(t *testing.T) {
},
expect: func(reg *driver.RegistryDefault) []settings.PostHookPostPersistExecutor {
return []settings.PostHookPostPersistExecutor{
hook.NewVerifier(reg),
hook.NewWebHook(reg, json.RawMessage(`{"headers":{"X-Custom-Header":"test"},"method":"GET","url":"foo"}`)),
}
},
Expand Down
25 changes: 18 additions & 7 deletions selfservice/flow/registration/hook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,9 +187,12 @@ func TestRegistrationExecutor(t *testing.T) {
t.Run("case=should redirect to verification UI if show_verification_ui hook is set", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerifier,
},
{

"hook": hook.KeyVerificationUI,
},
})
Expand All @@ -205,10 +208,14 @@ func TestRegistrationExecutor(t *testing.T) {
t.Run("case=should redirect to verification UI if there is a login_challenge", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{{
"hook": hook.KeyVerificationUI,
}})
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerifier,
},
{
"hook": hook.KeyVerificationUI,
},
})
i := testhelpers.SelfServiceHookFakeIdentity(t)
i.Traits = identity.Traits(`{"email": "[email protected]"}`)

Expand All @@ -228,8 +235,10 @@ func TestRegistrationExecutor(t *testing.T) {
t.Run("case=should redirect to first verification UI if show_verification_ui hook is set and multiple verifiable addresses", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerifier,
},
{
"hook": hook.KeyVerificationUI,
},
Expand All @@ -248,8 +257,10 @@ func TestRegistrationExecutor(t *testing.T) {
t.Run("case=should still sent session if show_verification_ui is set after session hook", func(t *testing.T) {
verificationTS := testhelpers.NewVerificationUIFlowEchoServer(t, reg)
t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf))
conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceRegistrationAfter+".hooks", []map[string]interface{}{
{
"hook": hook.KeyVerifier,
},
{
"hook": hook.KeyVerificationUI,
},
Expand Down
4 changes: 3 additions & 1 deletion selfservice/strategy/profile/strategy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import (
"testing"
"time"

"github.com/ory/kratos/selfservice/hook"

"github.com/ory/x/jsonx"

kratos "github.com/ory/kratos/internal/httpclient"
Expand Down Expand Up @@ -532,7 +534,7 @@ func TestStrategyTraits(t *testing.T) {
t.Run("description=should send email with verifiable address", func(t *testing.T) {
setPrivileged(t)

conf.MustSet(ctx, config.ViperKeySelfServiceVerificationEnabled, true)
conf.MustSet(ctx, config.ViperKeySelfServiceSettingsAfter+".profile.hooks", []map[string]any{{"hook": hook.KeyVerifier}})
conf.MustSet(ctx, config.ViperKeyCourierSMTPURL, "smtp://foo:[email protected]/")
t.Cleanup(func() {
conf.MustSet(ctx, config.HookStrategyKey(config.ViperKeySelfServiceSettingsAfter, settings.StrategyProfile), nil)
Expand Down
Loading