fix: do not invalidate recovery addr on update

[hperl]

This PR modifies persister.UpdateIdentity to only update the recovery addresses that changed, leaving the unchanged once (and their associated recovery tokens) valid.

Shout-out to @thcyron for the analysis.

Fixes #2433

---

TODO

• Is everything Go 1.18 ready? (CI, Docker, ...)

[codecov]

Codecov Report

Merging #2699 (c553d8c) into master (0856bd7) will increase coverage by 0.04%.
The diff coverage is 84.61%.

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

@@            Coverage Diff             @@
##           master    #2699      +/-   ##
==========================================
+ Coverage   75.13%   75.17%   +0.04%     
==========================================
  Files         293      293              
  Lines       16740    16792      +52     
==========================================
+ Hits        12578    12624      +46     
- Misses       3197     3200       +3     
- Partials      965      968       +3     

Impacted Files	Coverage Δ
cmd/cleanup/sql.go	96.00% <ø> (ø)
cmd/migrate/sql.go	100.00% <ø> (ø)
examples/go/selfservice/error/main.go	50.00% <ø> (ø)
examples/go/selfservice/login/main.go	78.94% <ø> (ø)
examples/go/selfservice/logout/main.go	66.66% <ø> (ø)
examples/go/selfservice/recovery/main.go	73.33% <ø> (ø)
examples/go/selfservice/registration/main.go	77.77% <ø> (ø)
examples/go/selfservice/settings/main.go	80.00% <ø> (ø)
examples/go/selfservice/verification/main.go	73.33% <ø> (ø)
examples/go/session/tosession/main.go	75.00% <ø> (ø)
... and 26 more

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

[aeneasr]

Thank you for the quick turnaround! We have the problem both in recovery as well as verification :) It would also be awesome if we could write a small test to ensure that the diff is working correctly. Speaking about this, it's not uncommon that we need to check whether a 1:n relationship has changed in Ory Kratos (same goes for credentials for example) so maybe we can generalize this problem using a (potentially generic) diff function to return the IDs that need to be removed and inserted?

[hperl]

@aeneasr all review comments addressed. As far as the test for the verified address go, the suite is very thorough:

kratos/identity/test/pool.go

Lines 555 to 719 in bece076

	t.Run("suite=verifiable-address", func(t *testing.T) {
	createIdentityWithAddresses := func(t *testing.T, email string) identity.VerifiableAddress {
	var i identity.Identity
	require.NoError(t, faker.FakeData(&i))
	address := identity.NewVerifiableEmailAddress(email, i.ID)
	i.VerifiableAddresses = append(i.VerifiableAddresses, *address)
	require.NoError(t, p.CreateIdentity(ctx, &i))
	return i.VerifiableAddresses[0]
	}
	t.Run("case=not found", func(t *testing.T) {
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "does-not-exist")
	require.Equal(t, sqlcon.ErrNoRows, errorsx.Cause(err))
	})
	transform := func(k int, value string) string {
	switch k % 5 {
	case 0:
	value = strings.ToLower(value)
	case 1:
	value = strings.ToUpper(value)
	case 2:
	value = " " + value
	case 3:
	value = value + " "
	}
	return value
	}
	t.Run("case=create and find", func(t *testing.T) {
	addresses := make([]identity.VerifiableAddress, 15)
	for k := range addresses {
	value := randx.MustString(16, randx.AlphaLowerNum) + "@ory.sh"
	addresses[k] = createIdentityWithAddresses(t, transform(k, value))
	require.NotEmpty(t, addresses[k].ID)
	}
	compare := func(t *testing.T, expected, actual identity.VerifiableAddress) {
	actual.CreatedAt = actual.CreatedAt.UTC().Truncate(time.Hour * 24)
	actual.UpdatedAt = actual.UpdatedAt.UTC().Truncate(time.Hour * 24)
	expected.CreatedAt = expected.CreatedAt.UTC().Truncate(time.Hour * 24)
	expected.UpdatedAt = expected.UpdatedAt.UTC().Truncate(time.Hour * 24)
	expected.Value = strings.TrimSpace(strings.ToLower(expected.Value))
	assert.EqualValues(t, expected, actual)
	}
	for k, expected := range addresses {
	t.Run("method=FindVerifiableAddressByValue", func(t *testing.T) {
	t.Run(fmt.Sprintf("case=%d", k), func(t *testing.T) {
	actual, err := p.FindVerifiableAddressByValue(ctx, expected.Via, transform(k+1, expected.Value))
	require.NoError(t, err)
	compare(t, expected, *actual)
	t.Run("not if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, expected.Via, transform(k+1, expected.Value))
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	})
	})
	}
	})
	t.Run("case=update", func(t *testing.T) {
	address := createIdentityWithAddresses(t, "[email protected] ")
	address.Value = "new-codE "
	require.NoError(t, p.UpdateVerifiableAddress(ctx, &address))
	t.Run("not if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	require.ErrorIs(t, p.UpdateVerifiableAddress(ctx, &address), sqlcon.ErrNoRows)
	})
	actual, err := p.FindVerifiableAddressByValue(ctx, address.Via, address.Value)
	require.NoError(t, err)
	assert.Equal(t, "new-code", actual.Value)
	})
	t.Run("case=create and update and find", func(t *testing.T) {
	var i identity.Identity
	require.NoError(t, faker.FakeData(&i))
	address := identity.NewVerifiableEmailAddress("[email protected]", i.ID)
	i.VerifiableAddresses = append(i.VerifiableAddresses, *address)
	require.NoError(t, p.CreateIdentity(ctx, &i))
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.NoError(t, err)
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	address = identity.NewVerifiableEmailAddress("[email protected]", i.ID)
	i.VerifiableAddresses = []identity.VerifiableAddress{*address}
	require.NoError(t, p.UpdateIdentity(ctx, &i))
	_, err = p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.EqualError(t, err, sqlcon.ErrNoRows.Error())
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	actual, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.NoError(t, err)
	assert.Equal(t, identity.VerifiableAddressTypeEmail, actual.Via)
	assert.Equal(t, "[email protected]", actual.Value)
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "[email protected]")
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	})
	t.Run("case=create and update and find case insensitive", func(t *testing.T) {
	var i identity.Identity
	require.NoError(t, faker.FakeData(&i))
	address := identity.NewVerifiableEmailAddress("verification.TestPersister.Update-Identity-case-insensitive@ory.sh", i.ID)
	i.VerifiableAddresses = append(i.VerifiableAddresses, *address)
	require.NoError(t, p.CreateIdentity(ctx, &i))
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, strings.ToUpper("verification.TestPersister.Update-Identity-case-insensitive@ory.sh"))
	require.NoError(t, err)
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, strings.ToUpper("verification.TestPersister.Update-Identity-case-insensitive@ory.sh"))
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	address = identity.NewVerifiableEmailAddress("verification.TestPersister.Update-Identity-case-insensitive-next@ory.sh", i.ID)
	i.VerifiableAddresses = []identity.VerifiableAddress{*address}
	require.NoError(t, p.UpdateIdentity(ctx, &i))
	_, err = p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, strings.ToUpper("verification.TestPersister.Update-Identity-case-insensitive@ory.sh"))
	require.EqualError(t, err, sqlcon.ErrNoRows.Error())
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, strings.ToUpper("verification.TestPersister.Update-Identity-case-insensitive@ory.sh"))
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	actual, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, strings.ToUpper("verification.TestPersister.Update-Identity-case-insensitive-next@ory.sh"))
	require.NoError(t, err)
	assert.Equal(t, identity.VerifiableAddressTypeEmail, actual.Via)
	assert.Equal(t, "verification.testpersister.update-identity-case-insensitive-next@ory.sh", actual.Value)
	t.Run("can not find if on another network", func(t *testing.T) {
	_, p := testhelpers.NewNetwork(t, ctx, p)
	_, err := p.FindVerifiableAddressByValue(ctx, identity.VerifiableAddressTypeEmail, "verification.TestPersister.Update-Identity-case-insensitive-next@ory.sh")
	require.ErrorIs(t, err, sqlcon.ErrNoRows)
	})
	})
	})

[aeneasr]

Great job :)