Skip to content

Commit

Permalink
fix: do call send sms hook when SMS autoconfirm is enabled (#1562)
Browse files Browse the repository at this point in the history
## What kind of change does this PR introduce?

Small quirk discovered while testing - it currently looks like when SMS
Autoconfirm is set
```
 GOTRUE_SMS_AUTOCONFIRM="true" 
```

 and an OTP request is made:

```
curl -X POST http://localhost:9999/otp -H "Content-Type: application/json" -d '{"phone": "<phone>"}'
```
an OTP is still sent. There's a substantial number projects (see
internal for exact number) using this so probably will preserve this
behaviour.

This affects the edge case where `SMS_AUTOCONFIRM` is enabled but the
Hook returns an error which may leave the developer puzzled since one
might expect an SMS not to be sent with autoconfirm similar to
`MAILER_AUTOCONFIRM`

Before:
- Enable Send SMS and autoconfirm, make a request with faulty URI -
request should fail

After:
- Enable Send SMS and autoconfirm, make a request - message is sent as
per current behaviour

---------

Co-authored-by: Kang Ming <kang.ming1996@gmail.com>
  • Loading branch information
J0 and kangmingtay committed May 1, 2024
1 parent 434a59a commit bfe4d98
Showing 1 changed file with 2 additions and 1 deletion.
3 changes: 2 additions & 1 deletion internal/api/phone.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ func (a *API) sendPhoneConfirmation(ctx context.Context, r *http.Request, tx *st
if err != nil {
return "", err
}
if config.Hook.SendSMS.Enabled {
// Hook should only be called if SMS autoconfirm is disabled
if !config.Sms.Autoconfirm && config.Hook.SendSMS.Enabled {
input := hooks.SendSMSInput{
User: user,
SMS: hooks.SMS{
Expand Down

0 comments on commit bfe4d98

Please sign in to comment.