fix: properly normalize uppercase mail addresses

[hperl]

Related issue(s)

Fixes #3187
Fixes #3289
Fixes ory/network#273

[codecov]

Codecov Report

Merging #3330 (200e57a) into master (7182eca) will increase coverage by 0.00%.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##           master    #3330   +/-   ##
=======================================
  Coverage   78.11%   78.12%           
=======================================
  Files         325      325           
  Lines       21081    21085    +4     
=======================================
+ Hits        16468    16472    +4     
  Misses       3384     3384           
  Partials     1229     1229           

Impacted Files	Coverage Δ
identity/extension_recovery.go	96.66% <100.00%> (+0.23%)	⬆️
identity/extension_verify.go	97.67% <100.00%> (+0.11%)	⬆️

[aeneasr]

LGTM!

How will this affect existing addresses? Does it make sense to use SQL's ToLower like for credential identifiers?

[BrandonNoad]

May also fix ory/network#273

[hperl]

May also fix ory/network#273

Indeed, putting that on the list as well!

[hperl]

LGTM!

How will this affect existing addresses? Does it make sense to use SQL's ToLower like for credential identifiers?

We already normalize the recovery and verifiable addresses on identity create and update here:

kratos/persistence/sql/identity/persister_identity.go

Lines 351 to 356 in c93b543

	func (p *IdentityPersister) normalizeAllAddressess(ctx context.Context, identities ...*identity.Identity) {
	for _, id := range identities {
	p.normalizeRecoveryAddresses(ctx, id)
	p.normalizeVerifiableAddresses(ctx, id)
	}
	}

. So in the database there should always only be lowercase recovery and verifiable addresses.

We also had a migration that made all addresses lowercase here:

kratos/persistence/sql/migrations/sql/20220610155809000000_identity_address_casing.cockroach.up.sql

Lines 1 to 2 in c93b543

	UPDATE identity_recovery_addresses SET value = LOWER(value) WHERE TRUE;
	UPDATE identity_verifiable_addresses SET value = LOWER(value) WHERE TRUE;

So existing addresses should be fine, as all addresses are written lowercase, and we migrated all mixedcase addresses to lowercase at some point.

---

Note, this fix is still necessary (or actually fixes things), because in a patch/update of an identity we get the identity from the database (with a lowercase email address), and then verify the traits to see if the verified address changed (which may be mixed case). Finally, in

kratos/persistence/sql/identity/persister_identity.go

Line 304 in c93b543

func updateAssociation[T interface {

we compare the addresses by a hash to see if the value needs to be updated. Since the lowercase value from the verifiable address does not match the mixed case email from the trait, the association gets removed.

[adamstrawson]

Thanks for such a quick turn around on this @hperl 👏