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 custom sms hook #1474

Merged
merged 12 commits into from
Mar 27, 2024
Merged

feat: add custom sms hook #1474

merged 12 commits into from
Mar 27, 2024

Conversation

J0
Copy link
Contributor

@J0 J0 commented Mar 6, 2024

What kind of change does this PR introduce?

Allows devs to use a Custom SMS Provider via a HTTP Hook. SQL Hooks are not supported at this time as it requires significantly more effort to invoke a Hook via pg_net and handle errors as compared to using HTTP/HTTPS.

HTTP Hook invocation is done via the standardwebhooks library for symmetric hooks. Asymmetric hooks are being finalized by the committee and support will be added shortly.

The following changes are being made from the internal RFC:

  • Hooks have a timeout of 5 seconds instead of 15 seconds so as not to run for too long
  • Hooks have a size limit of 20kb. This is not stated in internal RFC but is part of the recommendations under the standard webhooks RFC.
  • We allow hooks using the http protocol . This is to support local development. Restriction to https can be done on the dashboard page for connecting a hook.
  • Add log statements where relevant and write proper error messages
  • Add more Gock tests

internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
@J0 J0 force-pushed the j0/add_hook_trigger_logic branch from 98ffa35 to 6aa36dd Compare March 13, 2024 09:34
@J0 J0 force-pushed the j0/add_hook_trigger_logic branch from fbce0d4 to 7283622 Compare March 15, 2024 08:18
J0 added a commit that referenced this pull request Mar 17, 2024
## What kind of change does this PR introduce?
Splits out config changes from #1474 
- rename runHook to runPostgresHook
- Allow http with https hooks only on localhost
@J0 J0 force-pushed the j0/add_hook_trigger_logic branch from 5f2b7ef to e885fe3 Compare March 17, 2024 13:33
@J0 J0 force-pushed the j0/add_hook_trigger_logic branch from 8d1341e to a156a5b Compare March 17, 2024 15:14
@J0 J0 marked this pull request as ready for review March 17, 2024 15:16
@J0 J0 requested a review from a team as a code owner March 17, 2024 15:16
@J0
Copy link
Contributor Author

J0 commented Mar 17, 2024

I've realized I'd need an endpoint to run any sort of load against in order for there to be a meaningful metric. I've considered the following options:

  1. Add command to Makefile and script / yaml file for testing.
  • Run benchmarks on an as needed basis, not in CI
  • Doesn't have to live in CI, minimal overhead
  1. Use testcontainers to spin up a docker container with an edge function container that can be hit
  • Introduces a new dependency
  1. Use a live Supabase project to test
  • Couples the concept of Supabase with Auth / GoTrue
  • Need to regularly swap out the URL and/or ensure project is not paused.
  1. Port load tests to benchmarks repo

Let me know if there are any preferences

As an aside, I've realized I will likely need to allow for http with host.docker.internal as well, likely in the morrow.

internal/api/errors.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
@J0
Copy link
Contributor Author

J0 commented Mar 20, 2024

Test script for future reference (simulate periodic requests)

  1. Create an export generate-phone.js
module.exports = {
  generatePhoneNumber: (context, events, done) => {
    const phonePrefix = "+1"; // Assuming US phone numbers for example
    const randomNumber = Math.floor(Math.random() * 9000000000) + 1000000000; // Generates a 10 digit number
    const phone = phonePrefix + randomNumber.toString();
    context.vars.phone = phone; // Set the generated phone number in the context
    return done();
  }
};
  1. Create a phone-test.yml file:
config:
  target: 'http://localhost:9999'
  phases:
    - duration: 60 # Test duration in seconds
      arrivalRate: 5 # Number of new virtual users per second
  processor: "./generate-phone.js" # Custom JavaScript code for generating phone numbers

scenarios:
  - flow:
      - function: "generatePhoneNumber"
      - post:
          url: "/otp"
          json:
            phone: "{{ phone }}"
  1. Run artillery run phone-test.yml. Ensure that these env vars are set:
GOTRUE_HOOK_CUSTOM_SMS_PROVIDER_ENABLED="true"
GOTRUE_HOOK_CUSTOM_SMS_PROVIDER_URI="http://host.docker.internal:54321/functions/v1/sms_sender"
GOTRUE_HOOK_CUSTOM_SMS_PROVIDER_SECRETS="v1,whsec_<base64_encoded>"

@J0 J0 requested a review from hf March 21, 2024 03:57
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
@J0 J0 requested a review from hf March 21, 2024 07:18
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.

Looks good, would improve the errors / logs a bit.

internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Show resolved Hide resolved
internal/api/hooks.go Show resolved Hide resolved
internal/api/hooks.go Show resolved Hide resolved
internal/api/hooks.go Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/hooks.go Outdated Show resolved Hide resolved
J0 and others added 2 commits March 27, 2024 13:58
Co-authored-by: Stojan Dimitrovski <sdimitrovski@gmail.com>
@J0
Copy link
Contributor Author

J0 commented Mar 27, 2024

For reference closer to release:

  • We need to document that the compression header is set to identity and not gzip compression - see this comment
  • Drop ContentLenght check if not relevant. See this comment for details.

@J0 J0 merged commit 0f6b29a into master Mar 27, 2024
2 checks passed
@J0 J0 deleted the j0/add_hook_trigger_logic branch March 27, 2024 06:30
kangmingtay pushed a commit that referenced this pull request Apr 3, 2024
🤖 I have created a release *beep* *boop*
---


##
[2.146.0](v2.145.0...v2.146.0)
(2024-04-03)


### Features

* add custom sms hook
([#1474](#1474))
([0f6b29a](0f6b29a))
* forbid generating an access token without a session
([#1504](#1504))
([795e93d](795e93d))


### Bug Fixes

* add cleanup statement for anonymous users
([#1497](#1497))
([cf2372a](cf2372a))
* generate signup link should not error
([#1514](#1514))
([4fc3881](4fc3881))
* move all EmailActionTypes to mailer package
([#1510](#1510))
([765db08](765db08))
* refactor mfa and aal update methods
([#1503](#1503))
([31a5854](31a5854))
* rename from CustomSMSProvider to SendSMS
([#1513](#1513))
([c0bc37b](c0bc37b))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@J0 J0 mentioned this pull request Apr 3, 2024
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.

None yet

2 participants