From 8e29b68a595d2ef18e48c2a01072335cefa36d86 Mon Sep 17 00:00:00 2001 From: Patrik Date: Tue, 29 Oct 2024 10:30:48 +0100 Subject: [PATCH] fix: account linking should only happen after 2fa when required (#4174) --- selfservice/flow/login/export_test.go | 6 +- selfservice/flow/login/hook.go | 59 +++--- selfservice/flow/login/hook_test.go | 201 +++++++++++++++++---- selfservice/flow/settings/handler.go | 66 +++---- selfservice/strategy/oidc/strategy_test.go | 6 +- session/handler.go | 2 +- session/manager.go | 2 +- session/manager_http.go | 12 +- session/manager_http_test.go | 10 +- 9 files changed, 250 insertions(+), 114 deletions(-) diff --git a/selfservice/flow/login/export_test.go b/selfservice/flow/login/export_test.go index 645bcaf7c000..eafe4a70d179 100644 --- a/selfservice/flow/login/export_test.go +++ b/selfservice/flow/login/export_test.go @@ -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) } diff --git a/selfservice/flow/login/hook.go b/selfservice/flow/login/hook.go index be25529241f0..402aacef665b 100644 --- a/selfservice/flow/login/hook.go +++ b/selfservice/flow/login/hook.go @@ -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" @@ -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 { @@ -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 } @@ -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 @@ -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.") @@ -195,7 +199,7 @@ 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.") @@ -203,7 +207,7 @@ func (e *HookExecutor) PostLoginHook( 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(). @@ -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), @@ -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 } @@ -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) } @@ -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, @@ -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)) @@ -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(), @@ -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 @@ -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(), @@ -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 diff --git a/selfservice/flow/login/hook_test.go b/selfservice/flow/login/hook_test.go index 7d7f8e158174..160373a47955 100644 --- a/selfservice/flow/login/hook_test.go +++ b/selfservice/flow/login/hook_test.go @@ -5,28 +5,28 @@ package login_test import ( "context" + "database/sql" "net/http" + "net/http/httptest" "net/url" "testing" "time" - "github.com/stretchr/testify/require" - - "github.com/gobuffalo/httptest" "github.com/julienschmidt/httprouter" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/tidwall/gjson" - "github.com/ory/kratos/hydra" - "github.com/ory/kratos/schema" - "github.com/ory/kratos/session" - "github.com/ory/kratos/driver/config" + confighelpers "github.com/ory/kratos/driver/config/testhelpers" + "github.com/ory/kratos/hydra" "github.com/ory/kratos/identity" "github.com/ory/kratos/internal" "github.com/ory/kratos/internal/testhelpers" + "github.com/ory/kratos/schema" "github.com/ory/kratos/selfservice/flow" "github.com/ory/kratos/selfservice/flow/login" + "github.com/ory/kratos/session" "github.com/ory/kratos/x" ) @@ -35,8 +35,6 @@ func TestLoginExecutor(t *testing.T) { ctx := context.Background() for _, strategy := range identity.AllCredentialTypes { - strategy := strategy - t.Run("strategy="+strategy.String(), func(t *testing.T) { t.Parallel() @@ -75,6 +73,26 @@ func TestLoginExecutor(t *testing.T) { reg.LoginHookExecutor().PostLoginHook(w, r, strategy.ToUiNodeGroup(), loginFlow, useIdentity, sess, "")) }) + router.GET("/login/post2fa", func(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + loginFlow, err := login.NewFlow(conf, time.Minute, "", r, ft) + require.NoError(t, err) + loginFlow.Active = strategy + loginFlow.RequestURL = x.RequestURL(r).String() + for _, cb := range flowCallback { + cb(loginFlow) + } + + sess := session.NewInactiveSession() + sess.CompletedLoginFor(identity.CredentialsTypePassword, identity.AuthenticatorAssuranceLevel1) + sess.CompletedLoginFor(identity.CredentialsTypeTOTP, identity.AuthenticatorAssuranceLevel2) + if useIdentity == nil { + useIdentity = testhelpers.SelfServiceHookCreateFakeIdentity(t, reg) + } + + testhelpers.SelfServiceHookLoginErrorHandler(t, w, r, + reg.LoginHookExecutor().PostLoginHook(w, r, strategy.ToUiNodeGroup(), loginFlow, useIdentity, sess, "")) + }) + ts := httptest.NewServer(router) t.Cleanup(ts.Close) conf.MustSet(ctx, config.ViperKeyPublicBaseURL, ts.URL) @@ -98,10 +116,9 @@ func TestLoginExecutor(t *testing.T) { ts := newServer(t, flow.TypeBrowser, nil) res, body := makeRequestPost(t, ts, true, url.Values{}) - assert.EqualValues(t, http.StatusOK, res.StatusCode) + require.Equal(t, http.StatusOK, res.StatusCode) assert.Contains(t, res.Request.URL.String(), ts.URL) assert.EqualValues(t, gjson.Get(body, "continue_with").Raw, `[{"action":"redirect_browser_to","redirect_browser_to":"https://www.ory.sh/"}]`) - t.Logf("%s", body) }) t.Run("case=pass if hooks pass", func(t *testing.T) { @@ -109,17 +126,19 @@ func TestLoginExecutor(t *testing.T) { viperSetPost(t, conf, strategy.String(), []config.SelfServiceHook{{Name: "err", Config: []byte(`{}`)}}) res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{}) - assert.EqualValues(t, http.StatusOK, res.StatusCode) - assert.EqualValues(t, "https://www.ory.sh/", res.Request.URL.String()) + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, "https://www.ory.sh/", res.Request.URL.String()) }) t.Run("case=fail if hooks fail", func(t *testing.T) { t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) viperSetPost(t, conf, strategy.String(), []config.SelfServiceHook{{Name: "err", Config: []byte(`{"ExecuteLoginPostHook": "abort"}`)}}) - res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{}) - assert.EqualValues(t, http.StatusOK, res.StatusCode) - assert.Equal(t, "", body) + ts := newServer(t, flow.TypeBrowser, nil) + res, body := makeRequestPost(t, ts, false, url.Values{}) + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Contains(t, res.Request.URL.String(), ts.URL) + assert.Empty(t, body) }) t.Run("case=use return_to value", func(t *testing.T) { @@ -127,8 +146,8 @@ func TestLoginExecutor(t *testing.T) { conf.MustSet(ctx, config.ViperKeyURLsAllowedReturnToDomains, []string{"https://www.ory.sh/"}) res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, nil), false, url.Values{"return_to": {"https://www.ory.sh/kratos/"}}) - assert.EqualValues(t, http.StatusOK, res.StatusCode) - assert.EqualValues(t, "https://www.ory.sh/kratos/", res.Request.URL.String()) + require.Equal(t, http.StatusOK, res.StatusCode) + assert.Equal(t, "https://www.ory.sh/kratos/", res.Request.URL.String()) }) t.Run("case=use nested config value", func(t *testing.T) { @@ -300,46 +319,100 @@ func TestLoginExecutor(t *testing.T) { t.Run("case=maybe links credential", func(t *testing.T) { t.Cleanup(testhelpers.SelfServiceHookConfigReset(t, conf)) + conf.MustSet(ctx, config.ViperKeySessionWhoAmIAAL, config.HighestAvailableAAL) - email := testhelpers.RandomEmail() - useIdentity := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ + email1, email2 := testhelpers.RandomEmail(), testhelpers.RandomEmail() + passwordOnlyIdentity := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ identity.CredentialsTypePassword: { Type: identity.CredentialsTypePassword, Config: []byte(`{"hashed_password": "$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRNw"}`), - Identifiers: []string{email}, + Identifiers: []string{email1}, }, }} - require.NoError(t, reg.Persister().CreateIdentity(context.Background(), useIdentity)) + twoFAIdentitiy := &identity.Identity{Credentials: map[identity.CredentialsType]identity.Credentials{ + identity.CredentialsTypePassword: { + Type: identity.CredentialsTypePassword, + Config: []byte(`{"hashed_password": "$argon2id$v=19$m=32,t=2,p=4$cm94YnRVOW5jZzFzcVE4bQ$MNzk5BtR2vUhrp6qQEjRNw"}`), + Identifiers: []string{email2}, + }, + identity.CredentialsTypeTOTP: { + Type: identity.CredentialsTypeTOTP, + Config: []byte(`{"totp_url":"otpauth://totp/test"}`), + Identifiers: []string{email2}, + }, + }} + require.NoError(t, reg.Persister().CreateIdentity(ctx, passwordOnlyIdentity)) + require.NoError(t, reg.Persister().CreateIdentity(ctx, twoFAIdentitiy)) - credsOIDC, err := identity.NewCredentialsOIDC( + credsOIDCPWOnly, err := identity.NewCredentialsOIDC( + &identity.CredentialsOIDCEncryptedTokens{IDToken: "id-token", AccessToken: "access-token", RefreshToken: "refresh-token"}, + "my-provider", + email1, + "", + ) + require.NoError(t, err) + credsOIDC2FA, err := identity.NewCredentialsOIDC( &identity.CredentialsOIDCEncryptedTokens{IDToken: "id-token", AccessToken: "access-token", RefreshToken: "refresh-token"}, "my-provider", - email, + email2, "", ) require.NoError(t, err) + t.Run("sub-case=does not link after first factor when second factor is available", func(t *testing.T) { + ts := newServer(t, flow.TypeBrowser, twoFAIdentitiy, func(l *login.Flow) { + require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ + CredentialsType: identity.CredentialsTypeOIDC, + CredentialsConfig: credsOIDC2FA.Config, + DuplicateIdentifier: email2, + })) + }) + res, body := makeRequestPost(t, ts, false, url.Values{}) + assert.Equal(t, res.Request.URL.String(), ts.URL+login.RouteInitBrowserFlow+"?aal=aal2", "%s", body) + + ident, err := reg.Persister().GetIdentity(ctx, twoFAIdentitiy.ID, identity.ExpandCredentials) + require.NoError(t, err) + assert.Len(t, ident.Credentials, 2) + }) + + t.Run("sub-case=links after second factor when second factor is available", func(t *testing.T) { + ts := newServer(t, flow.TypeBrowser, twoFAIdentitiy, func(l *login.Flow) { + require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ + CredentialsType: identity.CredentialsTypeOIDC, + CredentialsConfig: credsOIDC2FA.Config, + DuplicateIdentifier: email2, + })) + }) + res, body := testhelpers.SelfServiceMakeHookRequest(t, ts, "/login/post2fa", false, url.Values{}) + assert.Equalf(t, http.StatusOK, res.StatusCode, "%s", body) + assert.Equalf(t, "https://www.ory.sh/", res.Request.URL.String(), "%s", body) + + ident, err := reg.Persister().GetIdentity(ctx, twoFAIdentitiy.ID, identity.ExpandCredentials) + require.NoError(t, err) + assert.Len(t, ident.Credentials, 3) + }) + t.Run("sub-case=links matching identity", func(t *testing.T) { - res, _ := makeRequestPost(t, newServer(t, flow.TypeBrowser, useIdentity, func(l *login.Flow) { + res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, passwordOnlyIdentity, func(l *login.Flow) { require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ CredentialsType: identity.CredentialsTypeOIDC, - CredentialsConfig: credsOIDC.Config, - DuplicateIdentifier: email, + CredentialsConfig: credsOIDCPWOnly.Config, + DuplicateIdentifier: email1, })) }), false, url.Values{}) - assert.EqualValues(t, http.StatusOK, res.StatusCode) - assert.EqualValues(t, "https://www.ory.sh/", res.Request.URL.String()) + assert.Equalf(t, http.StatusOK, res.StatusCode, "%s", body) + assert.Equalf(t, "https://www.ory.sh/", res.Request.URL.String(), "%s", body) - ident, err := reg.Persister().GetIdentity(ctx, useIdentity.ID, identity.ExpandCredentials) + ident, err := reg.Persister().GetIdentity(ctx, passwordOnlyIdentity.ID, identity.ExpandCredentials) require.NoError(t, err) - assert.Equal(t, 2, len(ident.Credentials)) + assert.Len(t, ident.Credentials, 2) }) t.Run("sub-case=errors on non-matching identity", func(t *testing.T) { - res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, useIdentity, func(l *login.Flow) { + res, body := makeRequestPost(t, newServer(t, flow.TypeBrowser, passwordOnlyIdentity, func(l *login.Flow) { require.NoError(t, flow.SetDuplicateCredentials(l, flow.DuplicateCredentialsData{ CredentialsType: identity.CredentialsTypeOIDC, - CredentialsConfig: credsOIDC.Config, + CredentialsConfig: credsOIDCPWOnly.Config, DuplicateIdentifier: "wrong@example.com", })) }), false, url.Values{}) @@ -369,12 +442,62 @@ func TestLoginExecutor(t *testing.T) { conf, )) }) - - t.Run("requiresAAL2 should return true if there's an error", func(t *testing.T) { - requiresAAL2, err := login.RequiresAAL2ForTest(*reg.LoginHookExecutor(), &http.Request{}, &session.Session{}) - require.NotNil(t, err) - require.True(t, requiresAAL2) - }) }) } + + t.Run("method=checkAAL", func(t *testing.T) { + ctx := confighelpers.WithConfigValue(ctx, config.ViperKeyPublicBaseURL, "https://www.ory.sh/") + + conf, reg := internal.NewFastRegistryWithMocks(t) + testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/login.schema.json") + conf.MustSet(ctx, config.ViperKeySelfServiceBrowserDefaultReturnTo, "https://www.ory.sh/") + + t.Run("returns no error when sufficient", func(t *testing.T) { + ctx := confighelpers.WithConfigValue(ctx, config.ViperKeySessionWhoAmIAAL, identity.AuthenticatorAssuranceLevel1) + assert.NoError(t, + login.CheckAALForTest(ctx, reg.LoginHookExecutor(), &session.Session{ + AMR: session.AuthenticationMethods{{ + Method: identity.CredentialsTypePassword, + AAL: identity.AuthenticatorAssuranceLevel1, + }}, + AuthenticatorAssuranceLevel: identity.AuthenticatorAssuranceLevel1, + }, nil), + ) + + ctx = confighelpers.WithConfigValue(ctx, config.ViperKeySessionWhoAmIAAL, config.HighestAvailableAAL) + assert.NoError(t, + login.CheckAALForTest(ctx, reg.LoginHookExecutor(), &session.Session{ + AMR: session.AuthenticationMethods{{ + Method: identity.CredentialsTypePassword, + AAL: identity.AuthenticatorAssuranceLevel1, + }, { + Method: identity.CredentialsTypeLookup, + AAL: identity.AuthenticatorAssuranceLevel2, + }}, + AuthenticatorAssuranceLevel: identity.AuthenticatorAssuranceLevel2, + }, nil), + ) + }) + + t.Run("copies parameters to redirect URL when AAL is not sufficient", func(t *testing.T) { + ctx := confighelpers.WithConfigValue(ctx, config.ViperKeySessionWhoAmIAAL, config.HighestAvailableAAL) + aalErr := new(session.ErrAALNotSatisfied) + require.ErrorAs(t, + login.CheckAALForTest(ctx, reg.LoginHookExecutor(), &session.Session{ + AMR: session.AuthenticationMethods{{ + Method: identity.CredentialsTypePassword, + AAL: identity.AuthenticatorAssuranceLevel1, + }}, + AuthenticatorAssuranceLevel: identity.AuthenticatorAssuranceLevel1, + Identity: &identity.Identity{ + InternalAvailableAAL: identity.NullableAuthenticatorAssuranceLevel{sql.NullString{String: string(identity.AuthenticatorAssuranceLevel2), Valid: true}}, + }, + }, &login.Flow{ + RequestURL: "https://www.ory.sh/?return_to=https://www.ory.sh/kratos&login_challenge=challenge", + }), + &aalErr, + ) + assert.Equal(t, "https://www.ory.sh/self-service/login/browser?aal=aal2&login_challenge=challenge&return_to=https%3A%2F%2Fwww.ory.sh%2Fkratos", aalErr.RedirectTo) + }) + }) } diff --git a/selfservice/flow/settings/handler.go b/selfservice/flow/settings/handler.go index efc1e5a84bc3..6232f47b7a9b 100644 --- a/selfservice/flow/settings/handler.go +++ b/selfservice/flow/settings/handler.go @@ -213,13 +213,14 @@ type createNativeSettingsFlow struct { // 400: errorGeneric // default: errorGeneric func (h *Handler) createNativeSettingsFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { - s, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) + ctx := r.Context() + s, err := h.d.SessionManager().FetchFromRequest(ctx, r) if err != nil { h.d.Writer().WriteError(w, r, err) return } - if err := h.d.SessionManager().DoesSessionSatisfy(r, s, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context())); err != nil { + if err := h.d.SessionManager().DoesSessionSatisfy(ctx, s, h.d.Config().SelfServiceSettingsRequiredAAL(ctx)); err != nil { h.d.Writer().WriteError(w, r, err) return } @@ -295,10 +296,11 @@ type createBrowserSettingsFlow struct { // 401: errorGeneric // 403: errorGeneric // default: errorGeneric -func (h *Handler) createBrowserSettingsFlow(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - s, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) +func (h *Handler) createBrowserSettingsFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + ctx := r.Context() + s, err := h.d.SessionManager().FetchFromRequest(ctx, r) if err != nil { - h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) + h.d.SelfServiceErrorManager().Forward(ctx, w, r, err) return } @@ -308,18 +310,18 @@ func (h *Handler) createBrowserSettingsFlow(w http.ResponseWriter, r *http.Reque managerOptions = append(managerOptions, session.WithRequestURL(requestURL.String())) } - if err := h.d.SessionManager().DoesSessionSatisfy(r, s, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), managerOptions...); err != nil { + if err := h.d.SessionManager().DoesSessionSatisfy(ctx, s, h.d.Config().SelfServiceSettingsRequiredAAL(ctx), managerOptions...); err != nil { h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, nil, nil, err) return } f, err := h.NewFlow(w, r, s.Identity, flow.TypeBrowser) if err != nil { - h.d.SelfServiceErrorManager().Forward(r.Context(), w, r, err) + h.d.SelfServiceErrorManager().Forward(ctx, w, r, err) return } - redirTo := f.AppendTo(h.d.Config().SelfServiceFlowSettingsUI(r.Context())).String() + redirTo := f.AppendTo(h.d.Config().SelfServiceFlowSettingsUI(ctx)).String() x.AcceptToRedirectOrJSON(w, r, h.d.Writer(), f, redirTo) } @@ -393,55 +395,54 @@ type getSettingsFlow struct { // 404: errorGeneric // 410: errorGeneric // default: errorGeneric -func (h *Handler) getSettingsFlow(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { - if err := h.fetchFlow(w, r); err != nil { - h.d.Writer().WriteError(w, r, err) - return - } -} - -func (h *Handler) fetchFlow(w http.ResponseWriter, r *http.Request) error { +func (h *Handler) getSettingsFlow(w http.ResponseWriter, r *http.Request, _ httprouter.Params) { + ctx := r.Context() rid := x.ParseUUID(r.URL.Query().Get("id")) - pr, err := h.d.SettingsFlowPersister().GetSettingsFlow(r.Context(), rid) + pr, err := h.d.SettingsFlowPersister().GetSettingsFlow(ctx, rid) if err != nil { - return err + h.d.Writer().WriteError(w, r, err) + return } - sess, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) + sess, err := h.d.SessionManager().FetchFromRequest(ctx, r) if err != nil { - return err + h.d.Writer().WriteError(w, r, err) + return } if pr.IdentityID != sess.Identity.ID { - return errors.WithStack(herodot.ErrForbidden.WithID(text.ErrIDInitiatedBySomeoneElse).WithReasonf("The request was made for another identity and has been blocked for security reasons.")) + h.d.Writer().WriteError(w, r, errors.WithStack(herodot.ErrForbidden. + WithID(text.ErrIDInitiatedBySomeoneElse). + WithReasonf("The request was made for another identity and has been blocked for security reasons."))) + return } // we cannot redirect back to the request URL (/self-service/settings/flows?id=...) since it would just redirect // to a page displaying raw JSON to the client (browser), which is not what we want. // Let's rather carry over the flow ID as a query parameter and redirect to the settings UI URL. - requestURL := urlx.CopyWithQuery(h.d.Config().SelfServiceFlowSettingsUI(r.Context()), url.Values{"flow": {rid.String()}}) - if err := h.d.SessionManager().DoesSessionSatisfy(r, sess, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), session.WithRequestURL(requestURL.String())); err != nil { - return err + requestURL := urlx.CopyWithQuery(h.d.Config().SelfServiceFlowSettingsUI(ctx), url.Values{"flow": {rid.String()}}) + if err := h.d.SessionManager().DoesSessionSatisfy(ctx, sess, h.d.Config().SelfServiceSettingsRequiredAAL(ctx), session.WithRequestURL(requestURL.String())); err != nil { + h.d.Writer().WriteError(w, r, err) + return } if pr.ExpiresAt.Before(time.Now().UTC()) { if pr.Type == flow.TypeBrowser { - redirectURL := flow.GetFlowExpiredRedirectURL(r.Context(), h.d.Config(), RouteInitBrowserFlow, pr.ReturnTo) + redirectURL := flow.GetFlowExpiredRedirectURL(ctx, h.d.Config(), RouteInitBrowserFlow, pr.ReturnTo) h.d.Writer().WriteError(w, r, errors.WithStack(x.ErrGone. WithReason("The settings flow has expired. Redirect the user to the settings flow init endpoint to initialize a new settings flow."). WithDetail("redirect_to", redirectURL.String()). WithDetail("return_to", pr.ReturnTo))) - return nil + return } h.d.Writer().WriteError(w, r, errors.WithStack(x.ErrGone. WithReason("The settings flow has expired. Call the settings flow init API endpoint to initialize a new settings flow."). - WithDetail("api", urlx.AppendPaths(h.d.Config().SelfPublicURL(r.Context()), RouteInitAPIFlow).String()))) - return nil + WithDetail("api", urlx.AppendPaths(h.d.Config().SelfPublicURL(ctx), RouteInitAPIFlow).String()))) + return } h.d.Writer().Write(w, r, pr) - return nil } // Update Settings Flow Parameters @@ -557,13 +558,14 @@ type updateSettingsFlowBody struct{} // 422: errorBrowserLocationChangeRequired // default: errorGeneric func (h *Handler) updateSettingsFlow(w http.ResponseWriter, r *http.Request, ps httprouter.Params) { + ctx := r.Context() rid, err := GetFlowID(r) if err != nil { h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, nil, nil, err) return } - f, err := h.d.SettingsFlowPersister().GetSettingsFlow(r.Context(), rid) + f, err := h.d.SettingsFlowPersister().GetSettingsFlow(ctx, rid) if errors.Is(err, sqlcon.ErrNoRows) { h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, nil, nil, errors.WithStack(herodot.ErrNotFound.WithReasonf("The settings request could not be found. Please restart the flow."))) return @@ -572,14 +574,14 @@ func (h *Handler) updateSettingsFlow(w http.ResponseWriter, r *http.Request, ps return } - ss, err := h.d.SessionManager().FetchFromRequest(r.Context(), r) + ss, err := h.d.SessionManager().FetchFromRequest(ctx, r) if err != nil { h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, f, nil, err) return } requestURL := x.RequestURL(r).String() - if err := h.d.SessionManager().DoesSessionSatisfy(r, ss, h.d.Config().SelfServiceSettingsRequiredAAL(r.Context()), session.WithRequestURL(requestURL)); err != nil { + if err := h.d.SessionManager().DoesSessionSatisfy(ctx, ss, h.d.Config().SelfServiceSettingsRequiredAAL(ctx), session.WithRequestURL(requestURL)); err != nil { h.d.SettingsFlowErrorHandler().WriteFlowError(w, r, node.DefaultGroup, f, nil, err) return } diff --git a/selfservice/strategy/oidc/strategy_test.go b/selfservice/strategy/oidc/strategy_test.go index 25faf0e91a55..f019e0256129 100644 --- a/selfservice/strategy/oidc/strategy_test.go +++ b/selfservice/strategy/oidc/strategy_test.go @@ -1589,9 +1589,9 @@ func TestStrategy(t *testing.T) { body, err := io.ReadAll(res.Body) require.NoError(t, res.Body.Close()) require.NoError(t, err) - assert.Equal(t, - strconv.Itoa(int(text.ErrorValidationLoginLinkedCredentialsDoNotMatch)), - gjson.GetBytes(body, "ui.messages.0.id").String(), + assert.EqualValues(t, + text.ErrorValidationLoginLinkedCredentialsDoNotMatch, + gjson.GetBytes(body, "ui.messages.0.id").Int(), prettyJSON(t, body), ) }) diff --git a/session/handler.go b/session/handler.go index 84be9c25e1b8..1699abd766bc 100644 --- a/session/handler.go +++ b/session/handler.go @@ -226,7 +226,7 @@ func (h *Handler) whoami(w http.ResponseWriter, r *http.Request, _ httprouter.Pa } var aalErr *ErrAALNotSatisfied - if err := h.r.SessionManager().DoesSessionSatisfy(r, s, c.SessionWhoAmIAAL(ctx), + if err := h.r.SessionManager().DoesSessionSatisfy(ctx, s, c.SessionWhoAmIAAL(ctx), // For the time being we want to update the AAL in the database if it is unset. UpsertAAL, ); errors.As(err, &aalErr) { diff --git a/session/manager.go b/session/manager.go index 596d0ab7da97..e44d91f9111f 100644 --- a/session/manager.go +++ b/session/manager.go @@ -146,7 +146,7 @@ type Manager interface { // This method is implemented in such a way, that if a second factor is found for the user, it is always assumed // that the user is able to authenticate with it. This means that if a user has a second factor, the user is always // asked to authenticate with it if `highest_available` is set and the session's AAL is `aal1`. - DoesSessionSatisfy(r *http.Request, sess *Session, matcher string, opts ...ManagerOptions) error + DoesSessionSatisfy(ctx context.Context, sess *Session, matcher string, opts ...ManagerOptions) error // SessionAddAuthenticationMethods adds one or more authentication method to the session. SessionAddAuthenticationMethods(ctx context.Context, sid uuid.UUID, methods ...AuthenticationMethod) error diff --git a/session/manager_http.go b/session/manager_http.go index cf68e7289737..b56a16be88ee 100644 --- a/session/manager_http.go +++ b/session/manager_http.go @@ -284,13 +284,14 @@ func (s *ManagerHTTP) PurgeFromRequest(ctx context.Context, w http.ResponseWrite return nil } -func (s *ManagerHTTP) DoesSessionSatisfy(r *http.Request, sess *Session, requestedAAL string, opts ...ManagerOptions) (err error) { - ctx, span := s.r.Tracer(r.Context()).Tracer().Start(r.Context(), "sessions.ManagerHTTP.DoesSessionSatisfy") +func (s *ManagerHTTP) DoesSessionSatisfy(ctx context.Context, sess *Session, requestedAAL string, opts ...ManagerOptions) (err error) { + ctx, span := s.r.Tracer(ctx).Tracer().Start(ctx, "sessions.ManagerHTTP.DoesSessionSatisfy") defer otelx.End(span, &err) - // If we already have AAL2 there is no need to check further because it is the highest AAL. sess.SetAuthenticatorAssuranceLevel() - if sess.AuthenticatorAssuranceLevel > identity.AuthenticatorAssuranceLevel1 { + + // If we already have AAL2 there is no need to check further because it is the highest AAL. + if sess.AuthenticatorAssuranceLevel == identity.AuthenticatorAssuranceLevel2 { return nil } @@ -311,8 +312,9 @@ func (s *ManagerHTTP) DoesSessionSatisfy(r *http.Request, sess *Session, request if sess.AuthenticatorAssuranceLevel >= identity.AuthenticatorAssuranceLevel1 { return nil } + return NewErrAALNotSatisfied(loginURL.String()) case config.HighestAvailableAAL: - if sess.AuthenticatorAssuranceLevel >= identity.AuthenticatorAssuranceLevel2 { + if sess.AuthenticatorAssuranceLevel == identity.AuthenticatorAssuranceLevel2 { // The session has AAL2, nothing to check. return nil } diff --git a/session/manager_http_test.go b/session/manager_http_test.go index 2a6eac16b5b3..a2ca87893075 100644 --- a/session/manager_http_test.go +++ b/session/manager_http_test.go @@ -408,7 +408,7 @@ func TestManagerHTTP(t *testing.T) { s.CompletedLoginFor(m, "") } require.NoError(t, reg.SessionManager().ActivateSession(req, s, i, time.Now().UTC())) - err := reg.SessionManager().DoesSessionSatisfy((&http.Request{}).WithContext(context.Background()), s, requested) + err := reg.SessionManager().DoesSessionSatisfy(ctx, s, requested) if expectedError != nil { require.ErrorAs(t, err, &expectedError) } else { @@ -455,7 +455,7 @@ func TestManagerHTTP(t *testing.T) { s := session.NewInactiveSession() s.CompletedLoginFor(identity.CredentialsTypePassword, "") require.NoError(t, reg.SessionManager().ActivateSession(req, s, idAAL1, time.Now().UTC())) - require.Error(t, reg.SessionManager().DoesSessionSatisfy((&http.Request{}).WithContext(context.Background()), s, config.HighestAvailableAAL, session.UpsertAAL)) + require.Error(t, reg.SessionManager().DoesSessionSatisfy(ctx, s, config.HighestAvailableAAL, session.UpsertAAL)) result, err := reg.IdentityPool().GetIdentity(context.Background(), idAAL1.ID, identity.ExpandNothing) require.NoError(t, err) @@ -858,7 +858,7 @@ func TestDoesSessionSatisfy(t *testing.T) { } require.NoError(t, reg.SessionManager().ActivateSession(req, s, id, time.Now().UTC())) - err := reg.SessionManager().DoesSessionSatisfy((&http.Request{}).WithContext(ctx), s, string(tc.matcher), tc.sessionManagerOptions...) + err := reg.SessionManager().DoesSessionSatisfy(ctx, s, string(tc.matcher), tc.sessionManagerOptions...) if tc.errAs != nil || tc.errIs != nil { if tc.expectedFunc != nil { tc.expectedFunc(t, err, tc.errAs) @@ -875,7 +875,7 @@ func TestDoesSessionSatisfy(t *testing.T) { // This should still work even if the session does not have identity data attached yet ... s.Identity = nil - err = reg.SessionManager().DoesSessionSatisfy((&http.Request{}).WithContext(ctx), s, string(tc.matcher), tc.sessionManagerOptions...) + err = reg.SessionManager().DoesSessionSatisfy(ctx, s, string(tc.matcher), tc.sessionManagerOptions...) if tc.errAs != nil { if tc.expectedFunc != nil { tc.expectedFunc(t, err, tc.errAs) @@ -888,7 +888,7 @@ func TestDoesSessionSatisfy(t *testing.T) { // ... or no credentials attached. s.Identity = id s.Identity.Credentials = nil - err = reg.SessionManager().DoesSessionSatisfy((&http.Request{}).WithContext(ctx), s, string(tc.matcher), tc.sessionManagerOptions...) + err = reg.SessionManager().DoesSessionSatisfy(ctx, s, string(tc.matcher), tc.sessionManagerOptions...) if tc.errAs != nil { if tc.expectedFunc != nil { tc.expectedFunc(t, err, tc.errAs)