feat: refactor identity persister #3101

alnr · 2023-02-14T10:16:40Z

This is some groundwork to more easily plug different persisters into kratos.

I've shuffled some code around:

moved the persister code related to identites and session devices to their own packages.
the (new) IdentityPersister now uses a faster path to hydrate identity associations (addresses, credentials) in all cases, not just for session lookups.
moved to github.com/laher/mergefs in favor of github.com/ory/x/fsx: this allows merging on top of already merged filesystems to be more flexible with custom migrations
all kratos servers (public, admin, metrics) are now cancelable via context. Previously, you could only stop the server via SIGINT/SIGTERM
pass through driver options to the migrate command

Other notes:

the InternalCredentials field is no longer routinely used to get credentials. Instead, we use an intelligent join to get the data in one go. Please review carefully.

ref https://github.com/ory-corp/cloud/issues/4013

codecov · 2023-02-15T11:43:18Z

Codecov Report

Merging #3101 (fc4daf1) into master (2d489e7) will increase coverage by 0.08%.
The diff coverage is 87.02%.

❗ Current head fc4daf1 differs from pull request most recent head b8a7b72. Consider uploading reports for the commit b8a7b72 to get more accurate results

@@            Coverage Diff             @@
##           master    #3101      +/-   ##
==========================================
+ Coverage   77.50%   77.58%   +0.08%     
==========================================
  Files         314      316       +2     
  Lines       19897    19908      +11     
==========================================
+ Hits        15421    15446      +25     
+ Misses       3285     3273      -12     
+ Partials     1191     1189       -2

Impacted Files	Coverage Δ
cmd/hashers/argon2/loadtest.go	`7.33% <ø> (ø)`
cmd/jsonnet/format.go	`50.00% <0.00%> (ø)`
courier/smtp.go	`69.56% <ø> (ø)`
driver/registry_default.go	`86.92% <0.00%> (+0.22%)`	⬆️
hash/hasher_pbkdf2.go	`78.37% <ø> (ø)`
persistence/sql/persister_continuity.go	`91.42% <ø> (ø)`
persistence/sql/persister_courier.go	`83.33% <0.00%> (ø)`
selfservice/strategy/password/validator.go	`93.10% <ø> (ø)`
x/time.go	`81.81% <ø> (ø)`
identity/identity.go	`88.05% <50.00%> (ø)`
... and 21 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

…command

aeneasr

This is looking pretty good already! Note to myself: i need to review the identity persister changes

identity/pool.go

aeneasr · 2023-03-02T12:19:37Z

identity/test/pool.go

@@ -134,7 +132,6 @@ func TestPool(ctx context.Context, conf *config.Config, p interface {
 					assert.Empty(t, actual.RecoveryAddresses)
 					assert.Empty(t, actual.VerifiableAddresses)

-					require.Len(t, actual.InternalCredentials, 2)


Reason for removal?

Software that imports kratos as a package might choose to hydrate the credentials in a different way, bypassing InternalCredentials. This tests implementational details, which we shouldn't do IMO.

LMKWYT.

session/manager_http.go

aeneasr

Persister reviewed :) Great stuff! Conceptually, this is shippable. Just a few comments :)

persistence/sql/identity/persister_identity.go

CaptainStandby

LGTM. Just one question concerning SQL injection.

persistence/sql/identity/persister_identity.go

…te delete function

https://github.com/securego/gosec#annotating-code

go.mod

This reverts commit 0f187f6.

aeneasr · 2023-03-03T14:30:22Z

persistence/sql/update/update.go

+	if err := c.Store.GetContext(ctx, &count, c.Dialect.TranslateSQL(stmt), v.GetID(), v.GetNID()); err != nil {
+		return sqlcon.HandleError(err)
+	} else if count == 0 {
+		return errors.WithStack(sqlcon.ErrNoRows)
+	}
+
+	//#nosec G201 -- TableName is static
+	stmt = fmt.Sprintf("UPDATE %s AS %s SET %s WHERE %s AND %s.nid = :nid",
+		quoter.Quote(model.TableName()),
+		model.Alias(),
+		cols.Writeable().QuotedUpdateString(quoter),
+		model.WhereNamedID(),
+		model.Alias(),
+	)
+
+	if _, err := c.Store.NamedExecContext(ctx, stmt, v); err != nil {


Just saw this (it exists in the current code too) - NamedExecContext should return the number of rows affected, which we could use to return an error. But anyways this is an optimization for another day and another PR!

jonas-jonas · 2023-03-03T16:36:52Z

persistence/sql/identity/persister_identity.go

 	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.FindByCredentialsIdentifier")
-	defer span.End()
+	otelx.End(span, &err)


Did the defer go missing here? @alnr

Wow, well spotted!
#3145

thank you for double checking @jonas-jonas 🙏

jonas-jonas · 2023-03-03T16:37:01Z

persistence/sql/identity/persister_identity.go

 	ctx, span := p.r.Tracer(ctx).Tracer().Start(ctx, "persistence.sql.findIdentityCredentialsType")
-	defer span.End()
+	otelx.End(span, &err)


alnr self-assigned this Feb 14, 2023

alnr force-pushed the persister-dependencies branch 4 times, most recently from 0d3659e to 54d4d28 Compare February 14, 2023 17:54

alnr added 10 commits February 22, 2023 17:58

chore: factor out identity persister

802ae87

fix: add limit clause in FindByCredentialsIdentifier

19589c2

feat: refactor

f251cba

fix: nid injection

68c2c66

feat: factor out device persister

dc60adb

fix: WithNetworkID

8028d94

fix: don't test internals

c0b38a1

fix: verbose logging

9f2361f

feat: expose MigrationBox and pass through driver options to migrate …

a9e8e79

…command

fix: use laher/mergefs instead of home-grown filesystem merging

a0fb6bd

alnr force-pushed the persister-dependencies branch from 0a97994 to a0fb6bd Compare February 22, 2023 17:00

alnr added 2 commits February 23, 2023 21:05

chore: bump laher/mergefs

6360905

feat: allow server termination through context cancelation

5b06143

alnr marked this pull request as ready for review March 1, 2023 16:10

alnr requested review from aeneasr and zepatrik as code owners March 1, 2023 16:10

alnr requested review from jonas-jonas, Benehiko, CaptainStandby and hperl March 1, 2023 16:11

aeneasr reviewed Mar 2, 2023

View reviewed changes

persistence/sql/identity/persister_identity.go Outdated Show resolved Hide resolved

CaptainStandby reviewed Mar 3, 2023

View reviewed changes

persistence/sql/identity/persister_identity.go Outdated Show resolved Hide resolved

persistence/sql/identity/persister_identity.go Outdated Show resolved Hide resolved

chore: bump dependencies

0f187f6

alnr added 4 commits March 3, 2023 10:55

fix: consistently use otelx.End in IdentityPersister and inline priva…

b015153

…te delete function

Inline private (*Persister).delete function into persister_session.go

58d6461

chore: update #nosec lint directives

476f5d9

https://github.com/securego/gosec#annotating-code

fix: lint

bf12ceb

alnr force-pushed the persister-dependencies branch from ae2b1f9 to 9b15dcf Compare March 3, 2023 10:44

feat: factor out (*Persister).update()

cb16649

alnr force-pushed the persister-dependencies branch from 9b15dcf to cb16649 Compare March 3, 2023 10:56

alnr added 3 commits March 3, 2023 12:09

fix: hydrate session identity

58c82c7

fix: InjectTraitsSchemaURL description

f85a033

fix: expose (*Identity).Validate()

076fe90

alnr requested review from aeneasr and CaptainStandby March 3, 2023 11:46

alnr added 2 commits March 3, 2023 12:50

fix: delete (*IdentityPersister).update()

864c868

fix: hydrate session identity

d5ead1c

aeneasr reviewed Mar 3, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

Revert "chore: bump dependencies"

b8a7b72

This reverts commit 0f187f6.

aeneasr reviewed Mar 3, 2023

View reviewed changes

aeneasr merged commit ceb5cc2 into master Mar 3, 2023

aeneasr deleted the persister-dependencies branch March 3, 2023 14:34

jonas-jonas reviewed Mar 3, 2023

View reviewed changes

alnr mentioned this pull request Mar 4, 2023

fix: webhook tracing and missing defers #3145

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: refactor identity persister #3101

feat: refactor identity persister #3101

alnr commented Feb 14, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading

aeneasr left a comment

aeneasr Mar 2, 2023

alnr Mar 3, 2023

aeneasr left a comment

CaptainStandby left a comment

aeneasr Mar 3, 2023

jonas-jonas Mar 3, 2023

alnr Mar 4, 2023

aeneasr Mar 5, 2023

jonas-jonas Mar 3, 2023

feat: refactor identity persister #3101

feat: refactor identity persister #3101

Conversation

alnr commented Feb 14, 2023 • edited Loading

codecov bot commented Feb 15, 2023 • edited Loading

Codecov Report

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr Mar 2, 2023

Choose a reason for hiding this comment

alnr Mar 3, 2023

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

CaptainStandby left a comment

Choose a reason for hiding this comment

aeneasr Mar 3, 2023

Choose a reason for hiding this comment

jonas-jonas Mar 3, 2023

Choose a reason for hiding this comment

alnr Mar 4, 2023

Choose a reason for hiding this comment

aeneasr Mar 5, 2023

Choose a reason for hiding this comment

jonas-jonas Mar 3, 2023

Choose a reason for hiding this comment

alnr commented Feb 14, 2023 •

edited

Loading

codecov bot commented Feb 15, 2023 •

edited

Loading