feat: added sms support to courier #1941

oleksiireshetnik · 2021-11-09T14:53:19Z

Related issue(s)

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

oleksiireshetnik · 2021-11-09T15:06:09Z

I try to be as close as possible to Twillio API contract for sending sms.
Also, I split SMTP and SMS logic into different files, but tried to preserve initial courier responsibility - to pull messages from database and send them regardless of type.

Also I tested this PR and #1938 in local setup, it works fine.

aeneasr

Hey, this looks awesome! We would need some docs for this as well: https://www.ory.sh/kratos/docs/concepts/email-sms#sending-sms

Once you mark it for ready for review, I'll finalize the review :)

oleksiireshetnik · 2021-12-15T16:37:11Z

Added documentation for sending SMS using courier

aeneasr

Thank you, this is looking grand! :)

Since I forgot to mention this in the other PR. Could you please add to the documentation how to use phone numbers as part of the identity traits over at

Thank you :)

courier/template/sms_login.go

courier/template/sms_stub.go

courier/sms.go

courier/dispatcher.go

courier/courier_test.go

courier/sms.go

aeneasr · 2021-12-29T13:04:55Z

Regarding the SMS sending logic, I think you could probably reuse the logic we have for the web hooks! Those allow you to:

Set custom auth
Set the HTTP body payload (as json)
Set the HTTP method
Set the HTTP URL

This could allow us to integrate with a lot of providers such as twilio and others out of the box :)

aeneasr · 2021-12-29T13:52:12Z

Once you need another review, just re-request review from me :)

splaunov · 2022-01-25T13:46:11Z

@alexey-reshetnik just to let you know. The code updates message status twice. The second time generating an error.
Fixed in this commit:
monta-app@6590f0a

oleksiireshetnik · 2022-01-26T10:21:42Z

@splaunov thank you. Included fix in the commit

oleksiireshetnik · 2022-01-26T10:26:15Z

@aeneasr added JsonNet support for making requests from the courier. Moved request forming logic to separate package, and added additional functionality to it:

setting custom headers
support for urlencoded content-type

Now it must be configurable with most SMS providers

oleksiireshetnik · 2022-02-16T14:42:49Z

Hi @Benehiko
All your comments make sense, thanks. Fixed them, adding documentation now

codecov · 2022-02-16T15:05:44Z

Codecov Report

Merging #1941 (f62f33c) into master (c5a04b5) will decrease coverage by 0.03%.
The diff coverage is 79.92%.

@@            Coverage Diff             @@
##           master    #1941      +/-   ##
==========================================
- Coverage   75.88%   75.85%   -0.04%     
==========================================
  Files         298      308      +10     
  Lines       15917    16111     +194     
==========================================
+ Hits        12079    12221     +142     
- Misses       2979     3012      +33     
- Partials      859      878      +19

Impacted Files	Coverage Δ
courier/message.go	`100.00% <ø> (ø)`
courier/sms.go	`60.37% <60.37%> (ø)`
courier/sms_templates.go	`63.63% <63.63%> (ø)`
courier/courier_dispatcher.go	`64.28% <64.28%> (ø)`
driver/config/config.go	`81.51% <66.66%> (-0.39%)`	⬇️
selfservice/strategy/link/sender.go	`71.27% <66.66%> (ø)`
courier/smtp.go	`74.00% <74.00%> (ø)`
request/builder.go	`77.77% <77.77%> (ø)`
request/auth.go	`80.00% <80.00%> (ø)`
courier/template/testhelpers/testhelpers.go	`95.06% <83.33%> (ø)`
... and 15 more

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 c5a04b5...f62f33c. Read the comment docs.

oleksiireshetnik · 2022-02-16T15:32:24Z

Added documentation: ory/docs#601

Benehiko

Nice job! This looks really good :)

I just have one last request, we just need some failure cases for the schema fixtures then this would be good to go IMO :)

embedx/config.schema.json

Benehiko · 2022-02-16T16:10:42Z

test/schema/fixtures/config.schema.test.success/root.courierSMS.yaml

+selfservice:
+  default_browser_return_url: "#/definitions/defaultReturnTo"
+
+dsn: foo
+
+identity:
+  schemas:
+    - id: default
+      url: https://example.com
+
+courier:
+  smtp:
+    connection_uri: smtps://foo:bar@my-mailserver:1234/
+    from_address: no-reply@ory.kratos.sh
+  sms:
+    enabled: true
+    from: "+19592155527"
+    request_config:
+      url: https://sms.example.com
+      method: POST
+      body: file://request.config.twilio.jsonnet
+      header:
+        'Content-Type': "application/x-www-form-urlencoded"


aeneasr · 2022-02-16T16:23:23Z

Shall I take a look also soon? Or do you want to address more things first?

Benehiko · 2022-02-16T17:11:04Z

Shall I take a look also soon? Or do you want to address more things first?

You can take a look :)
I'm satisfied with the PR (just need the schema fixtures for failures).

Benehiko

From my side, this looks good :)

embedx/config.schema.json

aeneasr

Thank you to everyone involved, this looks fantastic. I have added a few things and also ensured that SSRF can't happen if configured. I left a few comments for some easy-to-add tests but otherwise this is good to ship!

request/builder.go

embedx/config.schema.json

driver/config/config.go

courier/sms.go

oleksiireshetnik mentioned this pull request Nov 9, 2021

feat: added phone number identifier #1938

Merged

7 tasks

oleksiireshetnik force-pushed the courier-sms branch 2 times, most recently from 5c80670 to 0ffa6b7 Compare November 9, 2021 15:03

oleksiireshetnik changed the title ~~Added sms support to courier~~ feat: added sms support to courier Nov 9, 2021

aeneasr reviewed Nov 22, 2021

View reviewed changes

splaunov mentioned this pull request Dec 7, 2021

feat: Passwordless SMS code authentication #2033

Open

7 tasks

oleksiireshetnik force-pushed the courier-sms branch from 0ffa6b7 to 6d7978a Compare December 15, 2021 13:49

oleksiireshetnik marked this pull request as ready for review December 15, 2021 16:38

oleksiireshetnik requested a review from zepatrik as a code owner December 15, 2021 16:38

oleksiireshetnik requested a review from aeneasr December 15, 2021 16:38

oleksiireshetnik force-pushed the courier-sms branch from 7bc9f03 to eb11b41 Compare December 24, 2021 12:12

aeneasr requested changes Dec 29, 2021

View reviewed changes

oleksiireshetnik force-pushed the courier-sms branch from 9b786b5 to 268d099 Compare December 29, 2021 13:49

oleksiireshetnik force-pushed the courier-sms branch 4 times, most recently from 15e1048 to f82d446 Compare January 12, 2022 12:08

ederuiter mentioned this pull request Jan 12, 2022

Send emails via an api instead of smtp #2139

Closed

8 tasks

oleksiireshetnik force-pushed the courier-sms branch from f82d446 to c4ab5cf Compare January 12, 2022 17:48

oleksiireshetnik force-pushed the courier-sms branch from c4ab5cf to 49e1894 Compare January 26, 2022 10:17

oleksiireshetnik requested a review from aeneasr January 26, 2022 10:23

oleksiireshetnik force-pushed the courier-sms branch 2 times, most recently from 8684de0 to 9d239d4 Compare January 26, 2022 19:12

aeneasr reopened this Feb 14, 2022

aeneasr changed the base branch from v0.8 to master February 14, 2022 18:48

oleksiireshetnik force-pushed the courier-sms branch from e1b3608 to 0ad653f Compare February 15, 2022 11:25

oleksiireshetnik requested a review from Benehiko February 16, 2022 15:32

Benehiko reviewed Feb 16, 2022

View reviewed changes

Benehiko approved these changes Feb 17, 2022

View reviewed changes

embedx/config.schema.json Show resolved Hide resolved

oleksiireshetnik force-pushed the courier-sms branch from c96f4a6 to d379672 Compare February 21, 2022 10:22

aeneasr and others added 4 commits February 22, 2022 15:29

autogen(docs): generate and format documentation

68b4b2d

feat: added sms sending support to courier

b423c88

fix: mr comment fix

1d72d91

fix: added malformed config test

0ad2961

oleksiireshetnik force-pushed the courier-sms branch from d379672 to 0ad2961 Compare February 22, 2022 10:00

aeneasr assigned aeneasr and unassigned Benehiko Feb 23, 2022

aeneasr added 4 commits February 23, 2022 09:46

fix: resolve issues with the CI pipeline

80b7828

chore: update sms template body

1c21323

chore: code style

2f03ff6

fix: ensure no internal networks can be called in SMS sender

40fc6fc

aeneasr reviewed Feb 23, 2022

View reviewed changes

This comment was marked as resolved.

Sign in to view

aeneasr added 2 commits February 23, 2022 13:55

test: add tests for new sms options

fd66d6c

fix: lower-case jsonnet context for sms

f62f33c

aeneasr approved these changes Feb 23, 2022

View reviewed changes

aeneasr merged commit 8c58e94 into ory:master Feb 23, 2022

oleksiireshetnik deleted the courier-sms branch February 23, 2022 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: added sms support to courier #1941

feat: added sms support to courier #1941

oleksiireshetnik commented Nov 9, 2021 •

edited

oleksiireshetnik commented Nov 9, 2021 •

edited

aeneasr left a comment

oleksiireshetnik commented Dec 15, 2021

aeneasr left a comment

aeneasr commented Dec 29, 2021

aeneasr commented Dec 29, 2021

splaunov commented Jan 25, 2022

oleksiireshetnik commented Jan 26, 2022

oleksiireshetnik commented Jan 26, 2022 •

edited

oleksiireshetnik commented Feb 16, 2022 •

edited

codecov bot commented Feb 16, 2022 •

edited

oleksiireshetnik commented Feb 16, 2022

Benehiko left a comment

Benehiko Feb 16, 2022

aeneasr commented Feb 16, 2022

Benehiko commented Feb 16, 2022

Benehiko left a comment

aeneasr left a comment

This comment was marked as resolved.

feat: added sms support to courier #1941

feat: added sms support to courier #1941

Conversation

oleksiireshetnik commented Nov 9, 2021 • edited

Related issue(s)

Checklist

Further Comments

oleksiireshetnik commented Nov 9, 2021 • edited

aeneasr left a comment

Choose a reason for hiding this comment

oleksiireshetnik commented Dec 15, 2021

aeneasr left a comment

Choose a reason for hiding this comment

aeneasr commented Dec 29, 2021

aeneasr commented Dec 29, 2021

splaunov commented Jan 25, 2022

oleksiireshetnik commented Jan 26, 2022

oleksiireshetnik commented Jan 26, 2022 • edited

oleksiireshetnik commented Feb 16, 2022 • edited

codecov bot commented Feb 16, 2022 • edited

Codecov Report

oleksiireshetnik commented Feb 16, 2022

Benehiko left a comment

Choose a reason for hiding this comment

Benehiko Feb 16, 2022

Choose a reason for hiding this comment

aeneasr commented Feb 16, 2022

Benehiko commented Feb 16, 2022

Benehiko left a comment

Choose a reason for hiding this comment

aeneasr left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

oleksiireshetnik commented Nov 9, 2021 •

edited

oleksiireshetnik commented Nov 9, 2021 •

edited

oleksiireshetnik commented Jan 26, 2022 •

edited

oleksiireshetnik commented Feb 16, 2022 •

edited

codecov bot commented Feb 16, 2022 •

edited