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

Add custom sms provider #558

Closed
wants to merge 11 commits into from

Conversation

mamousavi
Copy link

@mamousavi mamousavi commented Jul 21, 2022

What kind of change does this PR introduce?

Add support for using a custom SMS gateway to send out OTPs.

What is the current behavior?

Does not support custom gateways.

What is the new behavior?

By changing the following env variables, users will be able to send out OTPs through a custom SMS gateway:

GOTRUE_SMS_PROVIDER=gateway
GOTRUE_SMS_GATEWAY_URL=<gateway_url>
GOTRUE_SMS_GATEWAY_SENDER=<sender_number>
GOTRUE_SMS_GATEWAY_BEARER_TOKEN=<token>

Implementation will send the following to the SMS gateway:

{
  "recipient": "+1 234 567",
  "body": "This is from supabase. Your code is 123456 .",
  "sender": "<sender_number>"
}

Implementation will only consider the HTTP code returned from the SMS gateway; it ignores the rest of the response (e.g., response body and response type).

Additional context

@J0
Copy link
Contributor

J0 commented Jul 22, 2022

Hey @mamousavi,

Thanks for the PR! Let us know if you need any help with this

Joel

@mamousavi
Copy link
Author

Hey @J0,
Glad I could help. I believe it's done.

@kangmingtay
Copy link
Member

@mamousavi thanks for the PR! Unfortunately, this wouldn't work because other SMS providers might have different API specifications for sending SMSes. Take Vonage for example, it requires the ApiKey and ApiSecret.

@mamousavi
Copy link
Author

@kangmingtay that's exactly right, hence the name "custom". Ideally users need to wrap their preferred provider in their own endpoint.

@mamousavi
Copy link
Author

mamousavi commented Jul 22, 2022

@kangmingtay The default providers are not affordable for many developers in some countries. Plus Auth0 supports using a custom sms gateway which this PR was inspired by.

@kangmingtay kangmingtay reopened this Jul 22, 2022
@hf
Copy link
Contributor

hf commented Jul 25, 2022

Hey @mamousavi, this is a great PR!

We've been thinking about adding more general webhook capabilities to GoTrue, and this is a good first start. Would you mind using terms like WEBHOOK_SMS to align more closely with this strategy of ours?

For example, call the env. vars. something like WEBHOOK_SMS_URL and WEBHOOK_SMS_HEADERS. I'd personally prefer using a headers-type of environment variable to include any non-basic authorization to the URL, as this is a more flexible solution. I'd go with a JSON structure for it, with this TypeScript type for reference:

{
  [header: string]: string | string[]
}

So in your case, adding the Authorization header would look like:

{
  "Authorization": "Bearer <...>"
}

One really important piece that is missing from this is a way to authenticate GoTrue at the receiving service. Typically this is done by exchanging some sort of secret where GoTrue computes an HMAC value and adds it to the headers of the request. A good approach is Stripe's.

Also, we really appreciate tests so if you could add them that would be splendid!

@mamousavi
Copy link
Author

mamousavi commented Jul 25, 2022

Hey @hf thanks for the comment.
As a matter of fact, I'd prefer to add this as a new webhook event; I just thought It'd be easier to implement it this way since Auth0 already features the exact same thing.
Actually, this is my first time writing go and I just followed similar PRs; If you want to make it a webhook event, please go ahead and close this, I'd really appreciate it.

@mamousavi
Copy link
Author

mamousavi commented Jul 25, 2022

@hf In the current approach, the gateway is not supposed to be a webhook endpoint in the first place. Users might want to use the same gateway in their own microservices. There's no need for gotrue to sign the requests.

Copy link
Contributor

@hf hf left a comment

Choose a reason for hiding this comment

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

Please also add tests for your custom provider!

Comment on lines 29 to 56
body, err := json.Marshal(map[string]string{
"recipient": phone,
"body": message,
"sender": t.Config.Sender,
})
if err != nil {
return err
}

client := &http.Client{Timeout: defaultTimeout}
r, err := http.NewRequest("POST", t.Config.Url, bytes.NewBuffer(body))
if err != nil {
return err
}
r.Header.Add("Content-Type", "application/json")
if len(t.Config.BearerToken) > 0 {
r.Header.Add("Authorization", "Bearer "+t.Config.BearerToken)
}
res, err := client.Do(r)
if err != nil {
return err
}

if res.StatusCode/100 != 2 {
return fmt.Errorf("Unexpected response while calling the SMS gateway: %v", res.StatusCode)
}

return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest using a more generic approach here as mentioned in one of the previous comments. Instead of specifying an Authorization header specifically, it is better and more flexible in my opinion to set any headers that you may need -- like API keys or other special values.

Furthermore, I don't see the need for the "sender" field. You only need to post the message and recipient phone number.

Finally, please handle HTTP errors and redirects correctly. For example, if the URL returns a 301 response, what happens? What's the timeout?

Copy link
Author

@mamousavi mamousavi Aug 24, 2022

Choose a reason for hiding this comment

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

I just followed the guide from Auth0 which covers most use cases. It's not a matter of opinion; It's best we KISS.

Copy link
Author

@mamousavi mamousavi Aug 24, 2022

Choose a reason for hiding this comment

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

Finally, please handle HTTP errors and redirects correctly. For example, if the URL returns a 301 response, what happens? What's the timeout?

It returns an error for any non-2xx response similar to the twilio provider. Also it uses the predefined timeout similar to all external providers.

example.env Outdated
Comment on lines 177 to 178
GOTRUE_SMS_CUSTOM_SENDER=""
GOTRUE_SMS_CUSTOM_BEARER_TOKEN=""
Copy link
Contributor

Choose a reason for hiding this comment

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

SENDER is not necessary IMO, while the BEARER_TOKEN should be replaced with a JSON-like map that sets various headers on the request (not just Authorization).

Copy link
Author

Choose a reason for hiding this comment

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

I know SENDER is not necessary but supplying it aligns better with other provider configurations.

@mamousavi
Copy link
Author

I can't find any sms provider tests for reference.

@mamousavi mamousavi requested a review from hf August 26, 2022 11:14
@hadi-codes
Copy link

any updates?

@mamousavi mamousavi requested a review from a team as a code owner September 28, 2022 19:04
@sahalMoidu
Copy link

@mamousavi @hf will this PR be merged and released any time soon?

@rokk4
Copy link

rokk4 commented Aug 20, 2023

I am willing to take care if this. I need this very urgently and I kinda don't want to maintain a fork.
@mamousavi @hf will this be merged soon?

@abdirahmn1
Copy link

any updates? i reaaaaaaaaaaaally need this asap! and just like @rokk4 it will be tedious to maintain a fork, also i want to use the supabase platform instead of hosting said fork myself. Thanks to all the maintainers, all of your efforts are appreciated. 🤝

@J0
Copy link
Contributor

J0 commented Aug 21, 2023

Hey @abdirahmn1 and team,

Thanks for patiently waiting and for taking the time to put together this PR. I'm really sorry to be the bearer of bad news but we're unlikely going to move forward with this PR as we'll be pursuing a hook based approach. The hope is that there'll be more room for flexibility in terms of SMS (e.g. dynamically select senders) with the hook since developers can modify code within the hook. Custom SMS Providers is on the top of our priority list and discussion is underway. We'll have more updates to share soon.

In the meantime, please let us know if there are any questions or concerns. Thanks!

@abdirahmn1
Copy link

Hey @abdirahmn1 and team,

Thanks for patiently waiting and for taking the time to put together this PR. I'm really sorry to be the bearer of bad news but we're unlikely going to move forward with this PR as we'll be pursuing a hook based approach. The hope is that there'll be more room for flexibility in terms of SMS (e.g. dynamically select senders) with the hook since developers can modify code within the hook. Custom SMS Providers is on the top of our priority list and discussion is underway. We'll have more updates to share soon.

In the meantime, please let us know if there are any questions or concerns. Thanks!

Thanks @J0 for taking the time, is there a timeline for when this feature will be published?, if not, can you please give me a rough estimate and your opinion of when you think it might be available. Take care and stay safe 😊.

@rohankm
Copy link

rohankm commented Jan 23, 2024

Hi, when will these feature be merged ?

@J0
Copy link
Contributor

J0 commented Feb 21, 2024

Thanks @J0 for taking the time, is there a timeline for when this feature will be published?, if not, can you please give me a rough estimate and your opinion of when you think it might be available. Take care and stay safe 😊.

We aim to merge support before the end of March, likely sooner. Going to close for now Let us know if there are further concerns.

Thanks everyone for the contribution!

@J0 J0 closed this Feb 21, 2024
@h8f1z
Copy link

h8f1z commented Apr 3, 2024

@J0 March has ended. What's the update?

@J0
Copy link
Contributor

J0 commented Apr 3, 2024

Ah thanks for patiently waiting. We’ve merged support in #1474 . Docs will follow within the next 2 weeks

@h8f1z
Copy link

h8f1z commented Jun 22, 2024

Ah thanks for patiently waiting. We’ve merged support in #1474 . Docs will follow within the next 2 weeks

Why can't I still see the option to choose custom SMS provider on Supabase Auth Providers?
Does this change only apply to self-hosted Supabase?

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.

Using custom (Unlimited) SMS Provider for sending OTP
10 participants