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

feat: added phone number identifier #1938

Merged
merged 2 commits into from
Dec 29, 2021

Conversation

oleksiireshetnik
Copy link
Contributor

@oleksiireshetnik oleksiireshetnik commented Nov 8, 2021

Added phone number identifier support for verification.

Related issue(s)

#137

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
    [email protected]) 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

@CLAassistant
Copy link

CLAassistant commented Nov 8, 2021

CLA assistant check
All committers have signed the CLA.

@oleksiireshetnik oleksiireshetnik changed the title feat: added phone number identtifier (ory#137) feat: added phone number identifier Nov 8, 2021
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.

Looking pretty good so far! How do you want to handle sending SMS?

@oleksiireshetnik
Copy link
Contributor Author

oleksiireshetnik commented Nov 9, 2021

Created a separate PR for sending sms (for easier review): #1941

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.

This looks good on a high level (will need to dig into the tests still)! Could you also please add some docs for this feature, probably around here https://www.ory.sh/kratos/docs/concepts/email-sms#sending-sms?

identity/extension_verify.go Show resolved Hide resolved
@nicolasburtey
Copy link

this PR/features is awesome. we (https://github.com/GaloyMoney/galoy) will likely switch to Ory once this is done.

@saltbo
Copy link

saltbo commented Dec 4, 2021

Look forward to this update. Come on.

@ZhenmingMa

This comment has been minimized.

@aeneasr
Copy link
Member

aeneasr commented Dec 8, 2021

Please follow our code of conduct and community guidelines and refrain from putting pressure on open source contributors. No one is preventing you from doing the work yourself and you have no right to try and force others to do it for you. Thank you.

@oleksiireshetnik
Copy link
Contributor Author

oleksiireshetnik commented Dec 24, 2021

Removed PR from draft status, added validation and test.

Have one problem with failing docker image scanner, but it is related to protobuf package which was not changed in this PR (also you can check out anchore/grype#558, seems like this failing check is false-positive).

@aeneasr please, review once again

@aeneasr
Copy link
Member

aeneasr commented Dec 28, 2021

Have one problem with failing docker image scanner, but it is related to protobuf package which was not changed in this PR (also you can check out anchore/grype#558, seems like this failing check is false-positive).

Yes, that is indeed a false positive. No problem. The e2e test is also flaky 😅

How would you like to continue with the other SMS PR that is going on? Do we merge this one, then rebase the other? I have time to review tomorrow :)

@aeneasr
Copy link
Member

aeneasr commented Dec 28, 2021

Scanning over the code quickly, this looks good to merge 🎉

I will do another pass tomorrow but I think this is a no-brainer in terms of merging, great job :)

@aeneasr aeneasr merged commit 294dfa8 into ory:master Dec 29, 2021
@oleksiireshetnik
Copy link
Contributor Author

Yep, I am finishing the pull request with the courier now - encountered a few errors

@vinckr
Copy link
Member

vinckr commented Jan 4, 2022

Hello @alexey-reshetnik
Congrats on merging your first PR in Ory 🎉 !
Your contribution is 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 to claim your Ory swag!

@oleksiireshetnik oleksiireshetnik deleted the phone-number-identifier branch January 12, 2022 17:51
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.

7 participants