Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(identity): migrate identity_addresses to lower case #2517

Merged
merged 2 commits into from
Jul 5, 2022
Merged

fix(identity): migrate identity_addresses to lower case #2517

merged 2 commits into from
Jul 5, 2022

Conversation

flxbk
Copy link
Contributor

@flxbk flxbk commented Jun 10, 2022

This adds a migration to convert all values of email addresses in identity_recovery_addresses and identity_verifiable_addresses to lower case as expected by FindVerifiableAddressByValue and FindRecoveryAddressByValue.

As far as I can tell, this migration was meant to be added as part of 731b3c7. However, it does not get considered by go:embed in that location (and also that version refers to a non-existent table identity_verification_addresses).

Related issue(s)

#2426

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security.
    vulnerability, I confirm that I got green light (please contact security@ory.sh) from the
    maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or changed the documentation.

Further Comments

@codecov
Copy link

codecov bot commented Jun 10, 2022

Codecov Report

Merging #2517 (65e4a8e) into master (09c5cc9) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2517      +/-   ##
==========================================
- Coverage   76.59%   76.58%   -0.02%     
==========================================
  Files         317      317              
  Lines       17636    17636              
==========================================
- Hits        13508    13506       -2     
- Misses       3190     3192       +2     
  Partials      938      938              
Impacted Files Coverage Δ
session/test/persistence.go 98.61% <0.00%> (-1.39%) ⬇️
persistence/sql/persister_courier.go 89.70% <0.00%> (+2.94%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd86783...65e4a8e. Read the comment docs.

Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find, thank you! Just one comment!

@aeneasr
Copy link
Member

aeneasr commented Jun 17, 2022

Thank you! One more thing we need to cover is the "test" checkbox - how do we know this patch is indeed effective? To test migrations, we have a package https://github.com/ory/kratos/tree/master/persistence/sql/migratest/ where testdata contains SQL rows we create after executing the related migration, and fixtures are the end result. Can you please add a test to verify that your changes indeed fix #2426 ?

Lastly, we need to inform people affected by #2426. What do they need to do? Examples (I don't know which one is true, it needs a bit of manual checking probably):

  1. Run the next release with SQL migrations and everything is resolved
  2. Execute manual steps because the SQL migrations are not resolving issues in the past
  3. Hotfix by running an SQL query

@aeneasr aeneasr merged commit c058e23 into ory:master Jul 5, 2022
@vinckr
Copy link
Member

vinckr commented Jul 7, 2022

Hello @srecko17
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants