Separate post register/login hooks #150
Conversation
|
|
|
Thank you! I'm working right now on fixing the go-acc issues and also some test related issues that surfaced due to this work. Once it's done, I'll ask you to rebase this PR on those changes and then it should be good for a review :) |
|
Ok, so the PR is now done. Let's see how the CI likes it: #154 |
|
Ok, merged - had to resolve several issues that surfaced in the tests :) Good to rebase now! |
driver/registry_default_hooks.go
Outdated
| @@ -73,3 +77,39 @@ func (m *RegistryDefault) hooksPost(credentialsType identity.CredentialsType, co | |||
|
|
|||
| return i | |||
| } | |||
|
|
|||
| func newRedirector(m *RegistryDefault, h configuration.SelfServiceHook, credentialsType identity.CredentialsType) (*hook.Redirector, error) { | |||
There was a problem hiding this comment.
I think that this logic belongs to NewRedirector in hook package, but then we will couple it with config parsing. So I've left it there.
There was a problem hiding this comment.
Yeah I was about to say that! :D We could however replace h configuration.SelfServiceHook with just config json.RawMessage and replace the WithField("hook", h.Run). line with the "redirector" because we know what hook we're creating here :) What do you think?
| b[k] = v | ||
| } | ||
| return b | ||
| return m.hooksPostRegistration(credentialsType, m.c.SelfServiceRegistrationAfterHooks(string(credentialsType))) |
There was a problem hiding this comment.
Do we even need separate hooksPostLogin/Registration? I would rather inline it here.
There was a problem hiding this comment.
We could probably reduce code duplication by having one function getHooks returning []interface{} and then looping through the list in PostRegistrationHooks and do a type assertion for registration.PostHookExecutor and if the type assertion passes, add it to the return list. The same of course for login. What do you think?
driver/registry_default_hooks.go
Outdated
| @@ -73,3 +77,39 @@ func (m *RegistryDefault) hooksPost(credentialsType identity.CredentialsType, co | |||
|
|
|||
| return i | |||
| } | |||
|
|
|||
| func newRedirector(m *RegistryDefault, h configuration.SelfServiceHook, credentialsType identity.CredentialsType) (*hook.Redirector, error) { | |||
There was a problem hiding this comment.
Yeah I was about to say that! :D We could however replace h configuration.SelfServiceHook with just config json.RawMessage and replace the WithField("hook", h.Run). line with the "redirector" because we know what hook we're creating here :) What do you think?
|
Perfect, thank you! |
Related issue
#149
Proposed changes
Checklist
vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
Further comments