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: add sms verification for phone numbers #3649

Merged
merged 14 commits into from
Dec 28, 2023
Merged

Conversation

jonas-jonas
Copy link
Member

@jonas-jonas jonas-jonas commented Nov 30, 2023

Adds the ability to verify phone numbers via SMS.

Related issue(s)

Fixes #3559

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 the approval (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

@jonas-jonas jonas-jonas self-assigned this Nov 30, 2023
Copy link

codecov bot commented Nov 30, 2023

Codecov Report

Attention: 93 lines in your changes are missing coverage. Please review.

Comparison is base (0c5ea9b) 78.40% compared to head (0e9d499) 78.34%.
Report is 1 commits behind head on master.

Files Patch % Lines
courier/smtp_channel.go 62.19% 25 Missing and 6 partials ⚠️
courier/http_channel.go 64.91% 17 Missing and 3 partials ⚠️
selfservice/strategy/code/code_sender.go 25.92% 18 Missing and 2 partials ⚠️
driver/config/config.go 68.62% 12 Missing and 4 partials ⚠️
courier/courier.go 77.77% 3 Missing and 1 partial ⚠️
courier/smtp.go 88.88% 1 Missing ⚠️
courier/template/testhelpers/testhelpers.go 92.30% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3649      +/-   ##
==========================================
- Coverage   78.40%   78.34%   -0.07%     
==========================================
  Files         346      346              
  Lines       23529    23548      +19     
==========================================
- Hits        18449    18448       -1     
- Misses       3695     3711      +16     
- Partials     1385     1389       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jonas-jonas jonas-jonas marked this pull request as ready for review December 13, 2023 09:57
@alnr
Copy link
Contributor

alnr commented Dec 18, 2023

Nice work! Two thoughts:

  1. We might want SSRF protection on the SMTP+HTTP endpoints configurable by the user.
  2. The distiction between HTTP email and SMS is not quite clear to me. I like the use of via to distinguish which template and address to select, but exactly is this extendable to other "channels"? For example, native notifications?

@jonas-jonas
Copy link
Member Author

We might want SSRF protection on the SMTP+HTTP endpoints configurable by the user.

Good point, maybe we can do this in a follow-up, depending on demand? Not sure if this is a requirement for anyone right now.

The distiction between HTTP email and SMS is not quite clear to me. I like the use of via to distinguish which template and address to select, but exactly is this extendable to other "channels"? For example, native notifications?

It's not quite complete yet. The long term plan is to deprecate the smtp: and http: sections in the config and do everything via channels. You'd then be able to set the channel to use for verification, recovery, login via code, etc. in the identity schema (the via key there). But that change seems rather breaking to me, so I'd like to do this in a follow-up, once we verified that this works with the SMS channel.

alnr
alnr previously approved these changes Dec 18, 2023
@jonas-jonas jonas-jonas force-pushed the jonas-jonas/smsGateway branch from 2670af7 to ca7ccc6 Compare December 27, 2023 10:02
@aeneasr
Copy link
Member

aeneasr commented Dec 27, 2023

We might want SSRF protection on the SMTP+HTTP endpoints configurable by the user.

SSRF is currently governed globally, I would keep it that way and not add more configuration options for the courier's SSRF protections.

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.

Very nice, just a few minor comments!

@aeneasr
Copy link
Member

aeneasr commented Dec 27, 2023

Thinking about the channels for a bit, isn't this a bit too powerful & complicated? Do we need separate sender configs for verification and recovery emails? I imagine this to be quite complex to set up in the UI and to make people understand how to use this feature, especially because you need to understand how it maps to the identity schema.

In the end what most people want is "send an SMS code" and "send an email code". Push notifications maybe at some point too, but that is a whole different beast we can tackle when it becomes a requirement. I would favor a more simplistic configuartion approach, especially for the time being as we only have SMS sending.

@jonas-jonas
Copy link
Member Author

As discussed directly, the channels approach gives great flexibility, and we want to keep it. However, in the Ory Network we're not going to expose this in the UI (for now) and only offer smtp, http, and sms channels. That way, first time users do not get lost in these dependent config values (the via key in the identity schema corresponds to the channel ID, etc.). (The first release of this, is going to be CLI configuration only, anyway.)

Once we have an identity schema editor, this might be easier to describe in the UI and we can re-think this approach.

@aeneasr aeneasr merged commit e3a3c4f into master Dec 28, 2023
26 of 27 checks passed
@aeneasr aeneasr deleted the jonas-jonas/smsGateway branch December 28, 2023 15:35
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.

SMTP Connection URI is required on HTTP delivery strategy
4 participants