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.

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

fix: whoami latency #4070

Merged
merged 12 commits into from
Aug 30, 2024
5 changes: 1 addition & 4 deletions selfservice/strategy/code/.schema/login.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
"type": "object",
"properties": {
"method": {
"type": "string",
"enum": [
"code"
]
"type": "string"
},
"code": {
"type": "string"
Expand Down
6 changes: 1 addition & 5 deletions selfservice/strategy/code/.schema/recovery.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@
"type": "object",
"properties": {
"method": {
"type": "string",
"enum": [
"code",
"link"
]
"type": "string"
},
"code": {
"type": "string"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,7 @@
"type": "object",
"properties": {
"method": {
"type": "string",
"enum": [
"code"
]
"type": "string"
},
"csrf_token": {
"type": "string"
Expand Down
20 changes: 15 additions & 5 deletions selfservice/strategy/code/strategy_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ func (s *Strategy) methodEnabledAndAllowedFromRequest(r *http.Request, f *login.
decoderx.HTTPDecoderAllowedMethods("POST", "PUT", "PATCH", "GET"),
decoderx.HTTPDecoderSetValidatePayloads(false),
decoderx.HTTPDecoderJSONFollowsFormFormat()); err != nil {
return nil, errors.WithStack(err)
return &method, errors.WithStack(err)
}

if err := flow.MethodEnabledAndAllowed(r.Context(), f.GetFlowName(), s.ID().String(), method.Method, s.deps); err != nil {
return nil, err
return &method, err
}

return &method, nil
Expand All @@ -192,9 +192,14 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow,
}

if p, err := s.methodEnabledAndAllowedFromRequest(r, f); errors.Is(err, flow.ErrStrategyNotResponsible) {
if !(s.deps.Config().SelfServiceCodeStrategy(ctx).MFAEnabled && (p == nil || len(p.Address) > 0)) {
if !s.deps.Config().SelfServiceCodeStrategy(ctx).MFAEnabled {
return nil, err
}

if p == nil || len(p.Address) == 0 {
return nil, err
}

// In this special case we only expect `address` to be set.
} else if err != nil {
return nil, err
Expand Down Expand Up @@ -261,6 +266,10 @@ func (s *Strategy) findIdentifierInVerifiableAddress(i *identity.Identity, ident

func (s *Strategy) findIdentityForIdentifier(ctx context.Context, identifier string, requestedAAL identity.AuthenticatorAssuranceLevel, session *session.Session) (_ *identity.Identity, _ []Address, err error) {
ctx, span := s.deps.Tracer(ctx).Tracer().Start(ctx, "selfservice.strategy.code.strategy.findIdentityForIdentifier")
span.SetAttributes(
attribute.String("flow.requested_aal", string(requestedAAL)),
)

defer otelx.End(span, &err)

if len(identifier) == 0 {
Expand All @@ -279,7 +288,7 @@ func (s *Strategy) findIdentityForIdentifier(ctx context.Context, identifier str
// we need to gracefully handle this flow.
//
// TODO this section should be removed at some point when we are sure that all identities have a code credential.
if errors.Is(err, schema.NewNoCodeAuthnCredentials()) {
if codeCred := new(schema.ValidationError); errors.As(err, &codeCred) && codeCred.ValidationError.Message == "account does not exist or has not setup up sign in with code" {
fallbackAllowed := s.deps.Config().SelfServiceCodeMethodMissingCredentialFallbackEnabled(ctx)
span.SetAttributes(
attribute.Bool(config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, fallbackAllowed),
Expand All @@ -290,7 +299,7 @@ func (s *Strategy) findIdentityForIdentifier(ctx context.Context, identifier str
return nil, nil, errors.WithStack(schema.NewNoCodeAuthnCredentials())
}

_, err := s.findIdentifierInVerifiableAddress(session.Identity, identifier)
address, err := s.findIdentifierInVerifiableAddress(session.Identity, identifier)
if err != nil {
return nil, nil, err
}
Expand All @@ -307,6 +316,7 @@ func (s *Strategy) findIdentityForIdentifier(ctx context.Context, identifier str
//
// So we accept that the identity in this case will simply not have code credentials, and we will rely on the
// fallback mechanism to authenticate the user.
return session.Identity, []Address{*address}, nil
} else if err != nil {
return nil, nil, err
}
Expand Down
38 changes: 26 additions & 12 deletions selfservice/strategy/code/strategy_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -751,7 +751,7 @@ func TestLoginCodeStrategy(t *testing.T) {
})

t.Run("case=should be able to get AAL2 session", func(t *testing.T) {
run := func(t *testing.T, withoutCodeCredential bool, overrideCodeCredential *identity.Credentials) (*state, *http.Client) {
run := func(t *testing.T, withoutCodeCredential bool, overrideCodeCredential *identity.Credentials, overrideAllCredentials map[identity.CredentialsType]identity.Credentials) (*state, *http.Client) {
user := createIdentity(ctx, t, reg, withoutCodeCredential)
if overrideCodeCredential != nil {
toUpdate := user.Credentials[identity.CredentialsTypeCodeAuth]
Expand All @@ -763,6 +763,10 @@ func TestLoginCodeStrategy(t *testing.T) {
}
user.Credentials[identity.CredentialsTypeCodeAuth] = toUpdate
}
if overrideAllCredentials != nil {
user.Credentials = overrideAllCredentials
}
require.NoError(t, reg.PrivilegedIdentityPool().UpdateIdentity(ctx, user))

var cl *http.Client
var f *oryClient.LoginFlow
Expand Down Expand Up @@ -805,14 +809,14 @@ func TestLoginCodeStrategy(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json") // has code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, false) // fallback enabled

_, cl := run(t, true, nil)
_, cl := run(t, true, nil, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})

t.Run("case=disabling mfa does not lock out the users", func(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json") // has code identifier

s, cl := run(t, true, nil)
s, cl := run(t, true, nil, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")

email := gjson.GetBytes(s.identity.Traits, "email").String()
Expand Down Expand Up @@ -865,36 +869,48 @@ func TestLoginCodeStrategy(t *testing.T) {
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, false)
})

_, cl := run(t, false, nil)
_, cl := run(t, false, nil, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})

t.Run("case=missing code credential with fallback works even when identity schema has no code identifier set", func(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/no-code.schema.json") // missing the code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, true) // fallback enabled
t.Cleanup(func() {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json")
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, false)
})

_, cl := run(t, false, nil, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})

t.Run("case=missing code credential with fallback works even when identity schema has no code identifier set", func(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/default.schema.json") // missing the code identifier
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/no-code-id.schema.json") // missing the code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, true) // fallback enabled
t.Cleanup(func() {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json")
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, false)
})

_, cl := run(t, false, nil)
_, cl := run(t, true, &identity.Credentials{}, map[identity.CredentialsType]identity.Credentials{})
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})

t.Run("case=legacy code credential with fallback works when identity schema has the code identifier not set", func(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/default.schema.json") // has code identifier
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/no-code.schema.json") // has code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, true) // fallback enabled
t.Cleanup(func() {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json") // has code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, false)
})

_, cl := run(t, false, &identity.Credentials{Config: []byte(`{"via":""}`)})
_, cl := run(t, false, &identity.Credentials{Config: []byte(`{"via":""}`)}, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})

t.Run("case=legacy code credential with fallback works when identity schema has the code identifier not set", func(t *testing.T) {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/default.schema.json") // has code identifier
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/no-code.schema.json") // has code identifier
conf.MustSet(ctx, config.ViperKeyCodeConfigMissingCredentialFallbackEnabled, true) // fallback enabled
t.Cleanup(func() {
testhelpers.SetDefaultIdentitySchema(conf, "file://./stub/code.identity.schema.json") // has code identifier
Expand All @@ -906,12 +922,11 @@ func TestLoginCodeStrategy(t *testing.T) {
`{"address_type": "email", "used_at": {"Time": "0001-01-01T00:00:00Z", "Valid": false}}`,
`{"address_type": "", "used_at": {"Time": "0001-01-01T00:00:00Z", "Valid": false}}`,
`{"address_type": ""}`,
`{"address_type": "sms"}`,
`{"address_type": "phone"}`,
`{}`,
} {
t.Run(fmt.Sprintf("config=%d", k), func(t *testing.T) {
_, cl := run(t, false, &identity.Credentials{Config: []byte(credentialsConfig)})
_, cl := run(t, false, &identity.Credentials{Config: []byte(credentialsConfig)}, nil)
testhelpers.EnsureAAL(t, cl, public, "aal2", "code")
})
}
Expand Down Expand Up @@ -1097,7 +1112,6 @@ func TestLoginCodeStrategy(t *testing.T) {
}
require.Equal(t, "No value found for trait email_1 in the current identity.", gjson.GetBytes(body, "reason").String(), "%s", body)
})

})
})
}
Expand Down
26 changes: 26 additions & 0 deletions selfservice/strategy/code/stub/no-code-id.schema.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
{
"$id": "https://example.com/person.schema.json",
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "Person",
"type": "object",
"properties": {
"traits": {
"type": "object",
"properties": {
"email": {
"type": "string",
"ory.sh/kratos": {
"credentials": {
},
"verification": {
"via": "email"
},
"recovery": {
"via": "email"
}
}
}
}
}
}
}
5 changes: 1 addition & 4 deletions selfservice/strategy/idfirst/.schema/login.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,7 @@
"minLength": 1
},
"method": {
"type": "string",
"enum": [
"identifier_first"
]
"type": "string"
},
"transient_payload": {
"type": "object",
Expand Down
2 changes: 1 addition & 1 deletion session/manager_http.go
Original file line number Diff line number Diff line change
Expand Up @@ -242,7 +242,7 @@ func (s *ManagerHTTP) FetchFromRequest(ctx context.Context, r *http.Request) (_
return nil, errors.WithStack(NewErrNoCredentialsForSession())
}

se, err := s.r.SessionPersister().GetSessionByToken(ctx, token, ExpandEverything, identity.ExpandDefault)
se, err := s.r.SessionPersister().GetSessionByToken(ctx, token, ExpandEverything, identity.ExpandEverything)
Copy link
Member Author

@aeneasr aeneasr Aug 30, 2024

Choose a reason for hiding this comment

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

Saves us having to call hydrate again later.

if err != nil {
if errors.Is(err, herodot.ErrNotFound) || errors.Is(err, sqlcon.ErrNoRows) {
return nil, errors.WithStack(NewErrNoActiveSessionFound())
Expand Down
1 change: 1 addition & 0 deletions test/e2e/cypress/integration/profiles/mfa/lookup.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ context("2FA lookup secrets", () => {
beforeEach(() => {
cy.visit(base)
cy.clearAllCookies()
cy.useConfig((builder) => builder.disableCodeMfa())
email = gen.email()
password = gen.password()
cy.registerApi({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ context("2FA UI settings tests", () => {

beforeEach(() => {
cy.clearAllCookies()
cy.useConfig((builder) => builder.disableCodeMfa())
cy.login({ email, password, cookieUrl: base })
cy.visit(settings)
})
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/cypress/integration/profiles/mfa/totp.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ context("2FA TOTP", () => {

beforeEach(() => {
cy.useConfig((builder) =>
builder.longPrivilegedSessionTime().useLaxAal(),
builder.longPrivilegedSessionTime().useLaxAal().disableCodeMfa(),
)
email = gen.email()
password = gen.password()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ context("2FA WebAuthn", () => {
beforeEach(() => {
cy.clearAllCookies()

cy.useConfig((builder) => builder.disableCodeMfa())
email = gen.email()
password = gen.password()
cy.registerApi({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ context("Mobile Profile", () => {

beforeEach(() => {
cy.loginMobile({ email, password })
cy.location("pathname").should("not.contain", "/Login")
cy.visit(MOBILE_URL + "/Settings")
})

Expand Down Expand Up @@ -67,6 +68,7 @@ context("Mobile Profile", () => {
fields: { "traits.website": website },
})
cy.loginMobile({ email, password })
cy.location("pathname").should("not.contain", "/Login")
cy.visit(MOBILE_URL + "/Settings")
})

Expand Down
5 changes: 5 additions & 0 deletions test/e2e/cypress/support/configHelpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ export class ConfigBuilder {
return this
}

public disableCodeMfa() {
this.config.selfservice.methods.code.mfa_enabled = false
return this
}

public enableRecovery() {
if (!this.config.selfservice.flows.recovery) {
this.config.selfservice.flows.recovery = {}
Expand Down
2 changes: 2 additions & 0 deletions test/e2e/profiles/mfa/.kratos.yml
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ selfservice:
ui_url: http://localhost:4455/recovery

methods:
code:
mfa_enabled: true
totp:
enabled: true
config:
Expand Down
Loading