Skip to content

Commit

Permalink
fix: account linking should only happen after 2fa when required (#4174)
Browse files Browse the repository at this point in the history
  • Loading branch information
@zepatrik
zepatrik authored Oct 29, 2024
1 parent 1a78af0 commit 8e29b68
Show file tree
Hide file tree
Showing 9 changed files with 250 additions and 114 deletions.
6 changes: 3 additions & 3 deletions selfservice/flow/login/export_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@
package login

import (
"net/http"
"context"

"github.com/ory/kratos/session"
)

func RequiresAAL2ForTest(e HookExecutor, r *http.Request, s *session.Session) (bool, error) {
return e.requiresAAL2(r, s, nil) // *login.Flow is nil to avoid an import cycle
func CheckAALForTest(ctx context.Context, e *HookExecutor, s *session.Session, flow *Flow) error {
return e.checkAAL(ctx, s, flow)
}
59 changes: 34 additions & 25 deletions selfservice/flow/login/hook.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"github.com/gofrs/uuid"
"github.com/pkg/errors"
"go.opentelemetry.io/otel/attribute"
"go.opentelemetry.io/otel/trace"

"github.com/ory/kratos/driver/config"
"github.com/ory/kratos/hydra"
Expand Down Expand Up @@ -81,19 +80,20 @@ func NewHookExecutor(d executorDependencies) *HookExecutor {
return &HookExecutor{d: d}
}

func (e *HookExecutor) requiresAAL2(r *http.Request, s *session.Session, a *Flow) (bool, error) {
err := e.d.SessionManager().DoesSessionSatisfy(r, s, e.d.Config().SessionWhoAmIAAL(r.Context()))
func (e *HookExecutor) checkAAL(ctx context.Context, s *session.Session, a *Flow) error {
err := e.d.SessionManager().DoesSessionSatisfy(ctx, s, e.d.Config().SessionWhoAmIAAL(ctx))
if err == nil {
return nil
}

if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) {
if aalErr.PassReturnToAndLoginChallengeParameters(a.RequestURL) != nil {
if a != nil && aalErr.PassReturnToAndLoginChallengeParameters(a.RequestURL) != nil {
_ = aalErr.WithDetail("pass_request_params_error", "failed to pass request parameters to aalErr.RedirectTo")
}
return true, aalErr
} else if err != nil {
return true, errors.WithStack(err)
return aalErr
}

return false, nil
return err
}

func (e *HookExecutor) handleLoginError(_ http.ResponseWriter, r *http.Request, g node.UiNodeGroup, f *Flow, i *identity.Identity, flowError error) error {
Expand Down Expand Up @@ -133,7 +133,11 @@ func (e *HookExecutor) PostLoginHook(
r = r.WithContext(ctx)
defer otelx.End(span, &err)

if err := e.maybeLinkCredentials(r.Context(), s, i, f); err != nil {
// We need to set the identity here because we check the available AAL in maybeLinkCredentials.
s.IdentityID = i.ID
s.Identity = i

if err := e.maybeLinkCredentials(ctx, s, i, f); err != nil {
return err
}

Expand All @@ -144,12 +148,12 @@ func (e *HookExecutor) PostLoginHook(
c := e.d.Config()
// Verify the redirect URL before we do any other processing.
returnTo, err := x.SecureRedirectTo(r,
c.SelfServiceBrowserDefaultReturnTo(r.Context()),
c.SelfServiceBrowserDefaultReturnTo(ctx),
x.SecureRedirectReturnTo(f.ReturnTo),
x.SecureRedirectUseSourceURL(f.RequestURL),
x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains(r.Context())),
x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL(r.Context())),
x.SecureRedirectOverrideDefaultReturnTo(c.SelfServiceFlowLoginReturnTo(r.Context(), f.Active.String())),
x.SecureRedirectAllowURLs(c.SelfServiceBrowserAllowedReturnToDomains(ctx)),
x.SecureRedirectAllowSelfServiceURLs(c.SelfPublicURL(ctx)),
x.SecureRedirectOverrideDefaultReturnTo(c.SelfServiceFlowLoginReturnTo(ctx, f.Active.String())),
)
if err != nil {
return err
Expand All @@ -172,14 +176,14 @@ func (e *HookExecutor) PostLoginHook(
WithField("identity_id", i.ID).
WithField("flow_method", f.Active).
Debug("Running ExecuteLoginPostHook.")
for k, executor := range e.d.PostLoginHooks(r.Context(), f.Active) {
for k, executor := range e.d.PostLoginHooks(ctx, f.Active) {
if err := executor.ExecuteLoginPostHook(w, r, g, f, s); err != nil {
if errors.Is(err, ErrHookAbortFlow) {
e.d.Logger().
WithRequest(r).
WithField("executor", fmt.Sprintf("%T", executor)).
WithField("executor_position", k).
WithField("executors", PostHookExecutorNames(e.d.PostLoginHooks(r.Context(), f.Active))).
WithField("executors", PostHookExecutorNames(e.d.PostLoginHooks(ctx, f.Active))).
WithField("identity_id", i.ID).
WithField("flow_method", f.Active).
Debug("A ExecuteLoginPostHook hook aborted early.")
Expand All @@ -195,15 +199,15 @@ func (e *HookExecutor) PostLoginHook(
WithRequest(r).
WithField("executor", fmt.Sprintf("%T", executor)).
WithField("executor_position", k).
WithField("executors", PostHookExecutorNames(e.d.PostLoginHooks(r.Context(), f.Active))).
WithField("executors", PostHookExecutorNames(e.d.PostLoginHooks(ctx, f.Active))).
WithField("identity_id", i.ID).
WithField("flow_method", f.Active).
Debug("ExecuteLoginPostHook completed successfully.")
}

if f.Type == flow.TypeAPI {
span.SetAttributes(attribute.String("flow_type", string(flow.TypeAPI)))
if err := e.d.SessionPersister().UpsertSession(r.Context(), s); err != nil {
if err := e.d.SessionPersister().UpsertSession(ctx, s); err != nil {
return errors.WithStack(err)
}
e.d.Audit().
Expand All @@ -212,7 +216,7 @@ func (e *HookExecutor) PostLoginHook(
WithField("identity_id", i.ID).
Info("Identity authenticated successfully and was issued an Ory Kratos Session Token.")

span.AddEvent(events.NewLoginSucceeded(r.Context(), &events.LoginSucceededOpts{
span.AddEvent(events.NewLoginSucceeded(ctx, &events.LoginSucceededOpts{
SessionID: s.ID,
IdentityID: i.ID,
FlowType: string(f.Type),
Expand All @@ -235,7 +239,7 @@ func (e *HookExecutor) PostLoginHook(
Token: s.Token,
ContinueWith: f.ContinueWith(),
}
if required, _ := e.requiresAAL2(r, classified, f); required {
if e.checkAAL(ctx, classified, f) != nil {
// If AAL is not satisfied, we omit the identity to preserve the user's privacy in case of a phishing attack.
response.Session.Identity = nil
}
Expand All @@ -244,7 +248,7 @@ func (e *HookExecutor) PostLoginHook(
return nil
}

if err := e.d.SessionManager().UpsertAndIssueCookie(r.Context(), w, r, s); err != nil {
if err := e.d.SessionManager().UpsertAndIssueCookie(ctx, w, r, s); err != nil {
return errors.WithStack(err)
}

Expand All @@ -254,7 +258,7 @@ func (e *HookExecutor) PostLoginHook(
WithField("session_id", s.ID).
Info("Identity authenticated successfully and was issued an Ory Kratos Session Cookie.")

trace.SpanFromContext(r.Context()).AddEvent(events.NewLoginSucceeded(r.Context(), &events.LoginSucceededOpts{
span.AddEvent(events.NewLoginSucceeded(ctx, &events.LoginSucceededOpts{
SessionID: s.ID,
IdentityID: i.ID, FlowType: string(f.Type), RequestedAAL: string(f.RequestedAAL), IsRefresh: f.Refresh, Method: f.Active.String(),
SSOProvider: provider,
Expand All @@ -267,7 +271,7 @@ func (e *HookExecutor) PostLoginHook(
s.Token = ""

// If we detect that whoami would require a higher AAL, we redirect!
if _, err := e.requiresAAL2(r, classified, f); err != nil {
if err := e.checkAAL(ctx, classified, f); err != nil {
if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) {
span.SetAttributes(attribute.String("return_to", aalErr.RedirectTo), attribute.String("redirect_reason", "requires aal2"))
e.d.Writer().WriteError(w, r, flow.NewBrowserLocationChangeRequiredError(aalErr.RedirectTo))
Expand All @@ -279,7 +283,7 @@ func (e *HookExecutor) PostLoginHook(
// If Kratos is used as a Hydra login provider, we need to redirect back to Hydra by returning a 422 status
// with the post login challenge URL as the body.
if f.OAuth2LoginChallenge != "" {
postChallengeURL, err := e.d.Hydra().AcceptLoginRequest(r.Context(),
postChallengeURL, err := e.d.Hydra().AcceptLoginRequest(ctx,
hydra.AcceptLoginRequestParams{
LoginChallenge: string(f.OAuth2LoginChallenge),
IdentityID: i.ID.String(),
Expand All @@ -303,7 +307,7 @@ func (e *HookExecutor) PostLoginHook(
}

// If we detect that whoami would require a higher AAL, we redirect!
if _, err := e.requiresAAL2(r, classified, f); err != nil {
if err := e.checkAAL(ctx, classified, f); err != nil {
if aalErr := new(session.ErrAALNotSatisfied); errors.As(err, &aalErr) {
http.Redirect(w, r, aalErr.RedirectTo, http.StatusSeeOther)
return nil
Expand All @@ -313,7 +317,7 @@ func (e *HookExecutor) PostLoginHook(

finalReturnTo := returnTo.String()
if f.OAuth2LoginChallenge != "" {
rt, err := e.d.Hydra().AcceptLoginRequest(r.Context(),
rt, err := e.d.Hydra().AcceptLoginRequest(ctx,
hydra.AcceptLoginRequestParams{
LoginChallenge: string(f.OAuth2LoginChallenge),
IdentityID: i.ID.String(),
Expand Down Expand Up @@ -346,6 +350,11 @@ func (e *HookExecutor) PreLoginHook(w http.ResponseWriter, r *http.Request, a *F

// maybeLinkCredentials links the identity with the credentials of the inner context of the login flow.
func (e *HookExecutor) maybeLinkCredentials(ctx context.Context, sess *session.Session, ident *identity.Identity, loginFlow *Flow) error {
if e.checkAAL(ctx, sess, loginFlow) != nil {
// we don't yet want to link credentials because the required AAL is not satisfied
return nil
}

lc, err := flow.DuplicateCredentials(loginFlow)
if err != nil {
return err
Expand Down
Loading

0 comments on commit 8e29b68

Please sign in to comment.