Skip to content

Commit

Permalink
feat: better detection if credentials exist on identifier first login (
Browse files Browse the repository at this point in the history
  • Loading branch information
aeneasr authored Jun 21, 2024
1 parent b07329d commit 11a28a1
Show file tree
Hide file tree
Showing 35 changed files with 427 additions and 224 deletions.
13 changes: 13 additions & 0 deletions selfservice/flow/login/strategy_form_hydrator.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,19 @@ type FormHydrator interface {
PopulateLoginMethodFirstFactor(r *http.Request, sr *Flow) error
PopulateLoginMethodSecondFactor(r *http.Request, sr *Flow) error
PopulateLoginMethodSecondFactorRefresh(r *http.Request, sr *Flow) error

// PopulateLoginMethodIdentifierFirstCredentials populates the login form with the first factor credentials.
// This method is called when the login flow is set to identifier first. The method will receive information
// about the identity that is being used to log in and the identifier that was used to find the identity.
//
// The method should populate the login form with the credentials of the identity.
//
// If the method can not find any credentials (because the identity does not exist) idfirst.ErrNoCredentialsFound
// must be returned. When returning idfirst.ErrNoCredentialsFound the strategy will appropriately deal with
// account enumeration mitigation.
//
// This method does however need to take appropriate steps to show/hide certain fields depending on the account
// enumeration configuration.
PopulateLoginMethodIdentifierFirstCredentials(r *http.Request, sr *Flow, options ...FormHydratorModifier) error
PopulateLoginMethodIdentifierFirstIdentification(r *http.Request, sr *Flow) error
}
Expand Down
13 changes: 9 additions & 4 deletions selfservice/strategy/code/strategy_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"net/http"
"strings"

"github.com/ory/kratos/selfservice/strategy/idfirst"
"github.com/ory/kratos/text"

"github.com/ory/x/sqlcon"
Expand Down Expand Up @@ -389,11 +390,15 @@ func (s *Strategy) PopulateLoginMethodSecondFactorRefresh(r *http.Request, f *lo
}

func (s *Strategy) PopulateLoginMethodIdentifierFirstCredentials(r *http.Request, f *login.Flow, _ ...login.FormHydratorModifier) error {
if s.deps.Config().SelfServiceCodeStrategy(r.Context()).PasswordlessEnabled {
f.GetUI().Nodes.Append(
node.NewInputField("method", s.ID(), node.CodeGroup, node.InputAttributeTypeSubmit).WithMetaLabel(text.NewInfoSelfServiceLoginCode()),
)
if !s.deps.Config().SelfServiceCodeStrategy(r.Context()).PasswordlessEnabled {
// We only return this if passwordless is disabled, because if it is enabled we can always sign in using this method.
return idfirst.ErrNoCredentialsFound
}

f.GetUI().Nodes.Append(
node.NewInputField("method", s.ID(), node.CodeGroup, node.InputAttributeTypeSubmit).WithMetaLabel(text.NewInfoSelfServiceLoginCode()),
)

return nil
}

Expand Down
10 changes: 6 additions & 4 deletions selfservice/strategy/code/strategy_login_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import (
"testing"
"time"

"github.com/ory/kratos/selfservice/strategy/idfirst"

configtesthelpers "github.com/ory/kratos/driver/config/testhelpers"

"github.com/ory/kratos/driver"
Expand Down Expand Up @@ -916,7 +918,7 @@ func TestFormHydration(t *testing.T) {
t.Run("case=WithIdentifier", func(t *testing.T) {
t.Run("case=code is used for 2fa", func(t *testing.T) {
r, f := newFlow(mfaEnabled, t)
require.NoError(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentifier("foo@bar.com")))
require.ErrorIs(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentifier("foo@bar.com")), idfirst.ErrNoCredentialsFound)
toSnapshot(t, f)
})

Expand All @@ -934,7 +936,7 @@ func TestFormHydration(t *testing.T) {
configtesthelpers.WithConfigValue(mfaEnabled, config.ViperKeySecurityAccountEnumerationMitigate, true),
t,
)
require.NoError(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentifier("foo@bar.com")))
require.ErrorIs(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentifier("foo@bar.com")), idfirst.ErrNoCredentialsFound)
toSnapshot(t, f)
})

Expand All @@ -955,7 +957,7 @@ func TestFormHydration(t *testing.T) {

t.Run("case=code is used for 2fa", func(t *testing.T) {
r, f := newFlow(mfaEnabled, t)
require.NoError(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentityHint(id)))
require.ErrorIs(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentityHint(id)), idfirst.ErrNoCredentialsFound)
toSnapshot(t, f)
})

Expand All @@ -971,7 +973,7 @@ func TestFormHydration(t *testing.T) {

t.Run("case=code is used for 2fa", func(t *testing.T) {
r, f := newFlow(mfaEnabled, t)
require.NoError(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentityHint(id)))
require.ErrorIs(t, fh.PopulateLoginMethodIdentifierFirstCredentials(r, f, login.WithIdentityHint(id)), idfirst.ErrNoCredentialsFound)
toSnapshot(t, f)
})

Expand Down
64 changes: 38 additions & 26 deletions selfservice/strategy/idfirst/strategy_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ package idfirst
import (
"net/http"

"github.com/ory/kratos/schema"

"github.com/pkg/errors"

"github.com/ory/kratos/identity"
"github.com/ory/kratos/schema"
"github.com/ory/kratos/selfservice/flow"
"github.com/ory/kratos/selfservice/flow/login"
"github.com/ory/kratos/session"
Expand All @@ -22,6 +23,7 @@ import (

var _ login.FormHydrator = new(Strategy)
var _ login.Strategy = new(Strategy)
var ErrNoCredentialsFound = errors.New("no credentials found")

func (s *Strategy) handleLoginError(w http.ResponseWriter, r *http.Request, f *login.Flow, payload *updateLoginFlowWithIdentifierFirstMethod, err error) error {
if f != nil {
Expand Down Expand Up @@ -64,13 +66,10 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow,
false,
)
if errors.Is(err, sqlcon.ErrNoRows) {
// User not found
if !s.d.Config().SecurityAccountEnumerationMitigate(r.Context()) {
// We don't have to mitigate account enumeration and show the user that the account doesn't exist
return nil, s.handleLoginError(w, r, f, &p, errors.WithStack(schema.NewAccountNotFoundError()))
}

// If the user is not found, we still want to potentially show the UI for some method. That's why we don't exit here.
// We have to mitigate account enumeration. So we continue without setting the identity hint.
//
// This will later be handled by `didPopulate`.
} else if err != nil {
// An error happened during lookup
return nil, s.handleLoginError(w, r, f, &p, err)

Check warning on line 75 in selfservice/strategy/idfirst/strategy_login.go

View check run for this annotation

Codecov / codecov/patch

selfservice/strategy/idfirst/strategy_login.go#L75

Added line #L75 was not covered by tests
Expand All @@ -88,17 +87,47 @@ func (s *Strategy) Login(w http.ResponseWriter, r *http.Request, f *login.Flow,
opts = append(opts, login.WithIdentityHint(identityHint))
opts = append(opts, login.WithIdentifier(p.Identifier))

didPopulate := false
for _, ls := range s.d.LoginStrategies(r.Context()) {
populator, ok := ls.(login.FormHydrator)
if !ok {
continue

Check warning on line 94 in selfservice/strategy/idfirst/strategy_login.go

View check run for this annotation

Codecov / codecov/patch

selfservice/strategy/idfirst/strategy_login.go#L94

Added line #L94 was not covered by tests
}

if err := populator.PopulateLoginMethodIdentifierFirstCredentials(r, f, opts...); errors.Is(err, login.ErrBreakLoginPopulate) {
didPopulate = true
break

Check warning on line 99 in selfservice/strategy/idfirst/strategy_login.go

View check run for this annotation

Codecov / codecov/patch

selfservice/strategy/idfirst/strategy_login.go#L98-L99

Added lines #L98 - L99 were not covered by tests
} else if errors.Is(err, ErrNoCredentialsFound) {
// This strategy is not responsible for this flow. We do not set didPopulate to true if that happens.
} else if err != nil {
return nil, s.handleLoginError(w, r, f, &p, err)

Check warning on line 103 in selfservice/strategy/idfirst/strategy_login.go

View check run for this annotation

Codecov / codecov/patch

selfservice/strategy/idfirst/strategy_login.go#L103

Added line #L103 was not covered by tests
} else {
didPopulate = true
}
}

// If no strategy populated, it means that the account (very likely) does not exist. We show a user not found error,
// but only if account enumeration mitigation is disabled. Otherwise, we proceed to render the rest of the form.
if !didPopulate && !s.d.Config().SecurityAccountEnumerationMitigate(r.Context()) {
return nil, s.handleLoginError(w, r, f, &p, errors.WithStack(schema.NewAccountNotFoundError()))
}

// We found credentials - hide the identifier.
f.UI.GetNodes().RemoveMatching(node.NewInputField("method", s.ID(), s.NodeGroup(), node.InputAttributeTypeSubmit))

// We set the identifier to hidden, so it's still available in the form but not visible to the user.
for k, n := range f.UI.Nodes {
if n.ID() != "identifier" {
continue
}

attrs, ok := f.UI.Nodes[k].Attributes.(*node.InputAttributes)
if !ok {
continue

Check warning on line 126 in selfservice/strategy/idfirst/strategy_login.go

View check run for this annotation

Codecov / codecov/patch

selfservice/strategy/idfirst/strategy_login.go#L126

Added line #L126 was not covered by tests
}

attrs.Type = node.InputAttributeTypeHidden
f.UI.Nodes[k].Attributes = attrs
}

f.Active = s.ID()
Expand Down Expand Up @@ -149,25 +178,8 @@ func (s *Strategy) PopulateLoginMethodIdentifierFirstIdentification(r *http.Requ
return nil
}

func (s *Strategy) PopulateLoginMethodIdentifierFirstCredentials(_ *http.Request, f *login.Flow, _ ...login.FormHydratorModifier) error {
f.UI.GetNodes().RemoveMatching(node.NewInputField("method", s.ID(), s.NodeGroup(), node.InputAttributeTypeSubmit))

// We set the identifier to hidden, so it's still available in the form but not visible to the user.
for k, n := range f.UI.Nodes {
if n.ID() != "identifier" {
continue
}

attrs, ok := f.UI.Nodes[k].Attributes.(*node.InputAttributes)
if !ok {
continue
}

attrs.Type = node.InputAttributeTypeHidden
f.UI.Nodes[k].Attributes = attrs
}

return nil
func (s *Strategy) PopulateLoginMethodIdentifierFirstCredentials(_ *http.Request, f *login.Flow, opts ...login.FormHydratorModifier) error {
return ErrNoCredentialsFound
}

func (s *Strategy) RegisterLoginRoutes(_ *x.RouterPublic) {}
Loading

0 comments on commit 11a28a1

Please sign in to comment.