Skip to content
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

refactor: review notes + webhooks PLATFORM-8899 #97

Merged
merged 9 commits into from
Feb 19, 2024
Merged

refactor: review notes + webhooks PLATFORM-8899 #97

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
82ae7cb
PLATFORM-8899 migrate deleteMultifactorCredential
mmeller-wikia Feb 5, 2024
599ad32
PLATFORM-8899 migrate settings hook configuration
mmeller-wikia Feb 5, 2024
1818117
PLATFORM-8899 migrate webhooks
mmeller-wikia Feb 5, 2024
976097c
Update selfservice/hook/web_hook.go
mmeller-wikia Feb 5, 2024
000093e
PLATFORM-9039: webhooks reliability
korzepadawid Feb 15, 2024
6f4ba3f
PLATFORM-9039: minor consistency fix with upstream
korzepadawid Feb 15, 2024
e6fd7cb
PLATFORM-9039 fixed tests
korzepadawid Feb 16, 2024
3b90cf7
PLATFORM-9039 code review notes
korzepadawid Feb 18, 2024
078e2b2
Merge branch 'fandom_master_1_0_0' into PLATFORM-8899-CR
adpaste Feb 19, 2024
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.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,14 +237,8 @@ func (m *RegistryDefault) RegisterRoutes(ctx context.Context, public *x.RouterPu
func NewRegistryDefault() *RegistryDefault {
return &RegistryDefault{
trc: otelx.NewNoop(nil, new(otelx.Config)),
rc: map[string]*retryablehttp.Client{},
}

// TODO fandom - fix webhooks
/*
return &RegistryDefault{
rc: map[string]*retryablehttp.Client{},
}
*/
}

func (m *RegistryDefault) WithLogger(l *logrusx.Logger) Registry {
Expand Down
6 changes: 3 additions & 3 deletions driver/registry_default_settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
)

func (m *RegistryDefault) PostSettingsPrePersistHooks(ctx context.Context, settingsType string) (b []settings.PostHookPrePersistExecutor) {
for _, v := range m.getHooks(settingsType, m.Config().SelfServiceFlowSettingsAfterHooks(ctx, settingsType)) {
for _, v := range m.getHooks(settingsType, filter(m.Config().SelfServiceFlowSettingsAfterHooks(ctx, settingsType), config.PrePersist)) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: check if all filter calls are in place

if hook, ok := v.(settings.PostHookPrePersistExecutor); ok {
b = append(b, hook)
}
Expand All @@ -35,7 +35,7 @@ func (m *RegistryDefault) PostSettingsPostPersistHooks(ctx context.Context, sett
initialHookCount = 1
}

for _, v := range m.getHooks(settingsType, m.Config().SelfServiceFlowSettingsAfterHooks(ctx, settingsType)) {
for _, v := range m.getHooks(settingsType, filter(m.Config().SelfServiceFlowSettingsAfterHooks(ctx, settingsType), config.PostPersist)) {
if hook, ok := v.(settings.PostHookPostPersistExecutor); ok {
b = append(b, hook)
}
Expand All @@ -44,7 +44,7 @@ func (m *RegistryDefault) PostSettingsPostPersistHooks(ctx context.Context, sett
if len(b) == initialHookCount {
// 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)) {
for _, v := range m.getHooks(config.HookGlobal, filter(m.Config().SelfServiceFlowSettingsAfterHooks(ctx, config.HookGlobal), config.PostPersist)) {
if hook, ok := v.(settings.PostHookPostPersistExecutor); ok {
b = append(b, hook)
}
Expand Down
68 changes: 68 additions & 0 deletions identity/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ func (h *Handler) RegisterAdminRoutes(admin *x.RouterAdmin) {
admin.POST(RouteCollection, h.create)
// fandom-start - add API to validate email before saving user in UCP
admin.POST(RouteValidate, h.validate)
// allow admins to remove multifactor authentication for an identity
admin.DELETE(RouteMultifactor, h.deleteMultifactorCredential)
// fandom-end
admin.PATCH(RouteCollection, h.batchPatchIdentities)
admin.PUT(RouteItem, h.update)
Expand Down Expand Up @@ -734,6 +736,72 @@ func (h *Handler) validate(w http.ResponseWriter, r *http.Request, _ httprouter.
}

// fandom-end
// fandom start - allow admins to remove multifactor authentication for an identity

// swagger:parameters adminDeleteIdentityCredential
// nolint:deadcode,unused
type adminDeleteIdentityCredential struct {
// ID is the identity's ID.
//
// required: true
// in: path
ID string `json:"id"`
// credentialType is the type of credential
//
// required: true
// in: path
CredentialType string `json:"credentialType"`
}

// swagger:route DELETE /identities/{id}/credential/{credentialType} v0alpha2 adminDeleteIdentityCredential
//
// # Update an Identity
//
// Allow admins to remove multifactor authentication for an identity
//
// Consumes:
// - application/json
//
// Produces:
// - application/json
//
// Schemes: http, https
//
// Security:
// oryAccessToken:
//
// Responses:
// 204: emptyResponse
// 404: jsonError
// 500: jsonError
func (h *Handler) deleteMultifactorCredential(w http.ResponseWriter, r *http.Request, ps httprouter.Params) {
Copy link
Author

@mmeller-wikia mmeller-wikia Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: It should be removed once we migrate to new API.
Note: New API will return 404 for not existing credential

identity, err := h.r.PrivilegedIdentityPool().GetIdentityConfidential(r.Context(), x.ParseUUID(ps.ByName("id")))
if err != nil {
h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Invalid identity")))
return
}

credentialType := ps.ByName("credentialType")
if credentialType != CredentialsTypeTOTP.String() && credentialType != CredentialsTypeLookup.String() &&
credentialType != CredentialsTypeWebAuthn.String() {
h.r.Writer().WriteError(w, r, errors.WithStack(herodot.ErrBadRequest.WithReasonf("Invalid credential type: %s", credentialType)))
return
}

identity.DeleteCredentialsType(CredentialsType(credentialType))
if err := h.r.IdentityManager().Update(
r.Context(),
identity,
ManagerAllowWriteProtectedTraits,
); err != nil {
h.r.Writer().WriteError(w, r, err)
return
}
w.WriteHeader(http.StatusAccepted)
h.r.Writer().Write(w, r, "OK")
}

// fandom - end

// Delete Identity Parameters
//
Expand Down
53 changes: 39 additions & 14 deletions selfservice/hook/web_hook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
grpccodes "google.golang.org/grpc/codes"

"github.com/ory/kratos/ui/node"
"github.com/ory/x/httpx"
"github.com/ory/x/jsonnetsecure"
"github.com/ory/x/otelx"

Expand Down Expand Up @@ -90,15 +91,14 @@ type (

errorMessage struct {
InstancePtr string `json:"instance_ptr"`
Message string `json:"message,omitempty"`
DetailedMessages []detailedMessage `json:"messages"`
}

rawHookResponse struct {
Messages []errorMessage `json:"messages"`
}

// TODO fandom - fix webhooks
//nolint:deadcode,unused
httpConfig struct {
sum string
retries int
Expand Down Expand Up @@ -130,6 +130,7 @@ func (e *WebHook) ExecuteLoginPreHook(_ http.ResponseWriter, req *http.Request,
RequestMethod: req.Method,
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
HookType: "LoginPreHook",
})
})
}
Expand All @@ -151,9 +152,8 @@ func (e *WebHook) ExecuteLoginPostHook(_ http.ResponseWriter, req *http.Request,
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
Identity: session.Identity,
// fandom-start
Fields: req.Form,
// fandom-end
HookType: "LoginPostHook",
Fields: req.Form,
})
})
}
Expand All @@ -166,6 +166,7 @@ func (e *WebHook) ExecuteVerificationPreHook(_ http.ResponseWriter, req *http.Re
RequestMethod: req.Method,
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
HookType: "VerificationPreHook",
})
})
}
Expand All @@ -179,6 +180,7 @@ func (e *WebHook) ExecutePostVerificationHook(_ http.ResponseWriter, req *http.R
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
Identity: id,
HookType: "VerificationPostHook",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Previously it was LoginPreHook

})
})
}
Expand All @@ -191,6 +193,7 @@ func (e *WebHook) ExecuteRecoveryPreHook(_ http.ResponseWriter, req *http.Reques
RequestMethod: req.Method,
RequestCookies: cookies(req),
RequestURL: x.RequestURL(req).String(),
HookType: "RecoveryPreHook",
})
})
}
Expand All @@ -204,6 +207,7 @@ func (e *WebHook) ExecutePostRecoveryHook(_ http.ResponseWriter, req *http.Reque
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
Identity: session.Identity,
HookType: "RecoveryPostHook",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: Previously it was PostRecoveryHook

})
})
}
Expand All @@ -216,6 +220,7 @@ func (e *WebHook) ExecuteRegistrationPreHook(_ http.ResponseWriter, req *http.Re
RequestMethod: req.Method,
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
HookType: "RegistrationPreHook",
})
})
}
Expand Down Expand Up @@ -286,9 +291,8 @@ func (e *WebHook) ExecutePostRegistrationPostPersistHook(_ http.ResponseWriter,
})
}

// TODO fandom - fix webhooks
//
//nolint:deadcode,unused
// fandom-start

func newHttpConfig(r json.RawMessage) (*httpConfig, error) {
type rawHttpConfig struct {
Retries int
Expand Down Expand Up @@ -338,6 +342,7 @@ func (e *WebHook) ExecuteSettingsPreHook(_ http.ResponseWriter, req *http.Reques
RequestMethod: req.Method,
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
HookType: "SettingsPreHook",
})
})
}
Expand All @@ -346,6 +351,14 @@ func (e *WebHook) ExecuteSettingsPostPersistHook(_ http.ResponseWriter, req *htt
if gjson.GetBytes(e.conf, "can_interrupt").Bool() || gjson.GetBytes(e.conf, "response.parse").Bool() {
return nil
}
// fandom-start
var credentials *identity.Credentials
if settingsType == "password" {
credentials, _ = id.GetCredentials(identity.CredentialsTypePassword)
} else if settingsType == "oidc" {
credentials, _ = id.GetCredentials(identity.CredentialsTypeOIDC)
}
// fandom-end
return otelx.WithSpan(req.Context(), "selfservice.hook.WebHook.ExecuteSettingsPostPersistHook", func(ctx context.Context) error {
return e.execute(ctx, &templateContext{
Flow: flow,
Expand All @@ -354,6 +367,8 @@ func (e *WebHook) ExecuteSettingsPostPersistHook(_ http.ResponseWriter, req *htt
RequestURL: x.RequestURL(req).String(),
RequestCookies: cookies(req),
Identity: id,
Credentials: credentials,
HookType: "SettingsPostPersistHook",
mmeller-wikia marked this conversation as resolved.
Show resolved Hide resolved
})
})
}
Expand Down Expand Up @@ -389,9 +404,19 @@ func (e *WebHook) ExecuteSettingsPrePersistHook(_ http.ResponseWriter, req *http
}

func (e *WebHook) execute(ctx context.Context, data *templateContext) error {
// TODO fandom - fix webhooks
conf, err := newHttpConfig(e.conf)
if err != nil {
return fmt.Errorf("failed to parse http config: %w", err)
}
var (
httpClient = e.deps.HTTPClient(ctx)
httpClient = e.deps.NamedHTTPClient(
ctx,
data.HookType+conf.sum,
httpx.ResilientClientWithMaxRetry(conf.retries),
httpx.ResilientClientWithConnectionTimeout(conf.timeout),
httpx.ResilientClientWithMinxRetryWait(conf.minWait),
httpx.ResilientClientWithMaxRetryWait(conf.maxWait),
)
Comment on lines +412 to +419
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could push this change to upstream

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should add ACs to all such tickets

ignoreResponse = gjson.GetBytes(e.conf, "response.ignore").Bool()
canInterrupt = gjson.GetBytes(e.conf, "can_interrupt").Bool()
parseResponse = gjson.GetBytes(e.conf, "response.parse").Bool()
Expand Down Expand Up @@ -484,7 +509,8 @@ func (e *WebHook) execute(ctx context.Context, data *templateContext) error {
if resp.StatusCode >= http.StatusBadRequest {
span.SetStatus(codes.Error, "HTTP status code >= 400")
if canInterrupt || parseResponse {
if err := parseWebhookResponse(resp, data.Identity); err != nil {
// TODO: double-check if we could use upstream `parseWebhookResponse`
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO:

if err := e.parseResponse(resp); err != nil {
return err
}
}
Expand All @@ -498,7 +524,8 @@ func (e *WebHook) execute(ctx context.Context, data *templateContext) error {
}

if parseResponse {
return parseWebhookResponse(resp, data.Identity)
// TODO: double-check if we could use upstream `parseWebhookResponse`
return e.parseResponse(resp)
}
return nil
}
Expand Down Expand Up @@ -607,8 +634,6 @@ func isTimeoutError(err error) bool {
return errors.As(err, &te) && te.Timeout() || errors.Is(err, context.DeadlineExceeded)
}

// TODO fandom - fix webhooks
//
//nolint:deadcode,unused
func (e *WebHook) parseResponse(resp *http.Response) (err error) {
if resp == nil {
Expand Down
Loading