feat: remove duplicate queries during settings flow and use better index hint for credentials lookup (#4193)

This patch reduces duplicate GetIdentity queries as part of submitting the settings flow, and improves an index to significantly reduce credential lookup.

For better debugging, more tracing ha been added to the settings module.

Author: aeneasr

cmd/clidoc/main.go

@@ -309,6 +309,8 @@ func validateAllMessages(path string) error {
 	info := &types.Info{
 		Defs: make(map[*ast.Ident]types.Object),
 	}
+
+	//nolint:staticcheck
 	var pack *ast.Package
 	for _, p := range packs {
 		if p.Name == "text" {

cmd/hashers/argon2/calibrate.go

@@ -246,6 +246,7 @@ Please note that the values depend on the machine you run the hashing on. If you
 				case res.MaxMem > conf.localConfig.DedicatedMemory:
 					_, _ = progressPrinter.Printf("The required memory was %s more than the maximum allowed of %s.\n", res.MaxMem-maxMemory, conf.localConfig.DedicatedMemory)
 
+					//nolint:gosec // disable G115
 					conf.localConfig.Memory -= (res.MaxMem - conf.localConfig.DedicatedMemory) / bytesize.ByteSize(reqPerMin)
 					_, _ = progressPrinter.Printf("Decreasing memory to %s\n", conf.localConfig.Memory)
 				// too slow

courier/courier_dispatcher.go

@@ -79,6 +79,7 @@ func (c *courier) DispatchQueue(ctx context.Context) error {
 	maxRetries := c.deps.CourierConfig().CourierMessageRetries(ctx)
 	pullCount := c.deps.CourierConfig().CourierWorkerPullCount(ctx)
 
+	//nolint:gosec // disable G115
 	messages, err := c.deps.CourierPersister().NextMessages(ctx, uint8(pullCount))
 	if err != nil {
 		if errors.Is(err, ErrQueueEmpty) {

driver/config/config.go

@@ -550,10 +550,14 @@ func (p *Config) HasherArgon2(ctx context.Context) *Argon2 {
 	// warn about usage of default values and point to the docs
 	// warning will require https://github.com/ory/viper/issues/19
 	return &Argon2{
-		Memory:            p.GetProvider(ctx).ByteSizeF(ViperKeyHasherArgon2ConfigMemory, Argon2DefaultMemory),
-		Iterations:        uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigIterations, int(Argon2DefaultIterations))),
-		Parallelism:       uint8(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigParallelism, int(Argon2DefaultParallelism))),
-		SaltLength:        uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigSaltLength, int(Argon2DefaultSaltLength))),
+		Memory: p.GetProvider(ctx).ByteSizeF(ViperKeyHasherArgon2ConfigMemory, Argon2DefaultMemory),
+		//nolint:gosec // disable G115
+		Iterations: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigIterations, int(Argon2DefaultIterations))),
+		//nolint:gosec // disable G115
+		Parallelism: uint8(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigParallelism, int(Argon2DefaultParallelism))),
+		//nolint:gosec // disable G115
+		SaltLength: uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigSaltLength, int(Argon2DefaultSaltLength))),
+		//nolint:gosec // disable G115
 		KeyLength:         uint32(p.GetProvider(ctx).IntF(ViperKeyHasherArgon2ConfigKeyLength, int(Argon2DefaultKeyLength))),
 		ExpectedDuration:  p.GetProvider(ctx).DurationF(ViperKeyHasherArgon2ConfigExpectedDuration, Argon2DefaultDuration),
 		ExpectedDeviation: p.GetProvider(ctx).DurationF(ViperKeyHasherArgon2ConfigExpectedDeviation, Argon2DefaultDeviation),

hash/hash_comparator.go

@@ -29,7 +29,7 @@ import (
 
 	//nolint:staticcheck
 	//lint:ignore SA1019 compatibility for imported passwords
-	"golang.org/x/crypto/md4" //#nosec G501 -- compatibility for imported passwords
+	"golang.org/x/crypto/md4" //nolint:gosec // disable G115 G501 -- compatibility for imported passwords
 	"golang.org/x/crypto/pbkdf2"
 	"golang.org/x/crypto/scrypt"
 
@@ -159,6 +159,7 @@ func CompareArgon2id(_ context.Context, password []byte, hash []byte) error {
 	}
 
 	// Derive the key from the other password using the same parameters.
+	//nolint:gosec // disable G115
 	otherHash := argon2.IDKey(password, salt, p.Iterations, uint32(p.Memory), p.Parallelism, p.KeyLength)
 
 	return comparePasswordHashConstantTime(hash, otherHash)

hash/hasher_argon2.go

@@ -41,6 +41,7 @@ func NewHasherArgon2(c Argon2Configuration) *Argon2 {
 }
 
 func toKB(mem bytesize.ByteSize) uint32 {
+	//nolint:gosec // disable G115
 	return uint32(mem / bytesize.KB)
 }
 

identity/manager.go

@@ -370,7 +370,7 @@ func (e *CreateIdentitiesError) Find(ident *Identity) *FailedIdentity {
 	return nil
 }
 func (e *CreateIdentitiesError) ErrOrNil() error {
-	if e.failedIdentities == nil || len(e.failedIdentities) == 0 {
+	if len(e.failedIdentities) == 0 {
 		return nil
 	}
 	return e

internal/client-go/go.sum

@@ -4,6 +4,7 @@ github.com/golang/protobuf v1.2.0/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5y
 golang.org/x/net v0.0.0-20180724234803-3673e40ba225/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
 golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e h1:bRhVy7zSSasaqNksaRZiA5EEI+Ei4I1nO5Jh72wfHlg=
 golang.org/x/net v0.0.0-20190108225652-1e06a53dbb7e/go.mod h1:mL1N/T3taQHkDXs73rZJwtUhF3w3ftmwwsq0BUmARs4=
+golang.org/x/oauth2 v0.0.0-20190604053449-0f29369cfe45/go.mod h1:gOpvHmFTYa4IltrdGE7lF6nIHvwfUNPOp7c8zoXwtLw=
 golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4 h1:YUO/7uOKsKeq9UokNS62b8FYywz3ker1l1vDZRCRefw=
 golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=

persistence/sql/identity/persister_identity.go

@@ -13,10 +13,6 @@ import (
 	"sync"
 	"time"
 
-	"github.com/ory/kratos/x/events"
-
-	"github.com/ory/x/crdbx"
-
 	"github.com/gobuffalo/pop/v6"
 	"github.com/gofrs/uuid"
 	"github.com/pkg/errors"
@@ -33,7 +29,9 @@ import (
 	"github.com/ory/kratos/persistence/sql/update"
 	"github.com/ory/kratos/schema"
 	"github.com/ory/kratos/x"
+	"github.com/ory/kratos/x/events"
 	"github.com/ory/x/contextx"
+	"github.com/ory/x/crdbx"
 	"github.com/ory/x/errorsx"
 	"github.com/ory/x/otelx"
 	"github.com/ory/x/pagination/keysetpagination"
@@ -806,11 +804,11 @@ func identifiersTableNameWithIndexHint(con *pop.Connection) string {
 	ici := "identity_credential_identifiers"
 	switch con.Dialect.Name() {
 	case "cockroach":
-		ici += "@identity_credential_identifiers_nid_i_ici_idx"
+		ici += "@identity_credential_identifiers_ici_nid_i_idx"
 	case "sqlite3":
-		ici += " INDEXED BY identity_credential_identifiers_nid_i_ici_idx"
+		ici += " INDEXED BY identity_credential_identifiers_ici_nid_i_idx"
 	case "mysql":
-		ici += " USE INDEX(identity_credential_identifiers_nid_i_ici_idx)"
+		ici += " USE INDEX(identity_credential_identifiers_ici_nid_i_idx)"
 	default:
 		// good luck 🤷‍♂️
 	}
@@ -932,7 +930,7 @@ func (p *IdentityPersister) ListIdentities(ctx context.Context, params identity.
 			)
 		}
 
-		if params.IdsFilter != nil && len(params.IdsFilter) != 0 {
+		if len(params.IdsFilter) > 0 {
 			wheres += `
 				AND identities.id in (?)
 			`
@@ -987,15 +985,15 @@ func (p *IdentityPersister) ListIdentities(ctx context.Context, params identity.
 				}
 			case identity.ExpandFieldVerifiableAddresses:
 				addrs := make([]identity.VerifiableAddress, 0)
-				if err := con.Where("nid = ?", nid).Where("identity_id IN (?)", identityIDs).Order("id").All(&addrs); err != nil {
+				if err := con.Where("identity_id IN (?)", identityIDs).Where("nid = ?", nid).Order("id").All(&addrs); err != nil {
 					return sqlcon.HandleError(err)
 				}
 				for _, addr := range addrs {
 					identitiesByID[addr.IdentityID].VerifiableAddresses = append(identitiesByID[addr.IdentityID].VerifiableAddresses, addr)
 				}
 			case identity.ExpandFieldRecoveryAddresses:
 				addrs := make([]identity.RecoveryAddress, 0)
-				if err := con.Where("nid = ?", nid).Where("identity_id IN (?)", identityIDs).Order("id").All(&addrs); err != nil {
+				if err := con.Where("identity_id IN (?)", identityIDs).Where("nid = ?", nid).Order("id").All(&addrs); err != nil {
 					return sqlcon.HandleError(err)
 				}
 				for _, addr := range addrs {

persistence/sql/migrations/sql/20241106142200000001_identities.autocommit.down.sql

@@ -0,0 +1 @@
+DROP INDEX IF EXISTS identity_credential_identifiers_nid_ici_i_idx;