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 email sender hook #1496

Closed
wants to merge 2 commits into from
Closed

Conversation

J0
Copy link
Contributor

@J0 J0 commented Mar 26, 2024

What kind of change does this PR introduce?

Adds a custom email sender at an API layer. Hook invocation happens on the API so the abstraction is done at the API level instead of on the mailer to avoid having the mailer import API.

The custom hook email sender serves as a substitute for the default email sender across all endpoints. It is not possible to selectively enable the hook on certain email flows.

@J0 J0 changed the base branch from master to j0/add_hook_trigger_logic March 26, 2024 08:45
@J0 J0 changed the base branch from j0/add_hook_trigger_logic to master March 26, 2024 08:45
@J0 J0 force-pushed the j0/add_custom_email_sender_hook branch from 8468da5 to f1d5e9a Compare March 27, 2024 06:54
@coveralls
Copy link

coveralls commented Mar 27, 2024

Pull Request Test Coverage Report for Build 8520521705

Details

  • 53 of 152 (34.87%) changed or added relevant lines in 2 files are covered.
  • 24 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.6%) to 64.522%

Changes Missing Coverage Covered Lines Changed/Added Lines %
internal/api/hooks.go 0 16 0.0%
internal/api/mail.go 53 136 38.97%
Files with Coverage Reduction New Missed Lines %
internal/mailer/template.go 24 77.73%
Totals Coverage Status
Change from base Build 8520470508: -0.6%
Covered Lines: 8033
Relevant Lines: 12450

💛 - Coveralls

@J0 J0 changed the title feat: add custom email sender approach feat: add custom email sender hook Mar 28, 2024
output := hooks.CustomEmailProviderOutput{}
err = a.invokeHTTPHook(ctx, r, &input, &output, config.Hook.CustomEmailProvider.URI)
currentEmail := u.GetEmail()
if config.Mailer.SecureEmailChangeEnabled && currentEmail != "" {
Copy link
Contributor Author

@J0 J0 Mar 29, 2024

Choose a reason for hiding this comment

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

We can potentially merge the two invocations into one by adding additional parameters though that makes handling on the function slightly more cumbersome as there's a need to check for two different paramers otp and newOTP or similar.

@J0 J0 marked this pull request as ready for review March 29, 2024 07:37
@J0 J0 requested a review from a team as a code owner March 29, 2024 07:37
@J0
Copy link
Contributor Author

J0 commented Mar 29, 2024

Going to add RedirectTo and Data.

internal/api/hooks.go Outdated Show resolved Hide resolved
internal/api/mail.go Outdated Show resolved Hide resolved
internal/api/mail.go Outdated Show resolved Hide resolved
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.

Some improvements needed...

@J0 J0 marked this pull request as draft March 31, 2024 15:03
@J0 J0 force-pushed the j0/add_custom_email_sender_hook branch from c636931 to da18f75 Compare April 1, 2024 02:01

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@J0 J0 force-pushed the j0/add_custom_email_sender_hook branch from da18f75 to 283f791 Compare April 2, 2024 08:30
J0 added a commit that referenced this pull request Apr 2, 2024
## What kind of change does this PR introduce?

Moves the mail related refactors from #1496 into a new PR so the hook
related PR is easier to review. This PR moves all EmailActionTypes to
mailer package to establish an explicit link between mailer and the
packages it is used in.

The changes are cosmetic and should not affect underlying functionality.
@J0 J0 mentioned this pull request Apr 2, 2024
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 so far! Please fix the nits to align the wording... we should maybe do the same for the SMS one too while it's not fully released yet

var err error

if response, err = a.runHTTPHook(ctx, r, a.config.Hook.SendEmail, input, output); err != nil {
return internalServerError("Error invoking custom email provider hook.").WithInternalError(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Error message should use "send email hook" wording

@@ -452,6 +452,7 @@ type HookConfiguration struct {
PasswordVerificationAttempt ExtensibilityPointConfiguration `json:"password_verification_attempt" split_words:"true"`
CustomAccessToken ExtensibilityPointConfiguration `json:"custom_access_token" split_words:"true"`
CustomSMSProvider ExtensibilityPointConfiguration `json:"custom_sms_provider" split_words:"true"`
SendEmail ExtensibilityPointConfiguration `json:"custom_email_provider" split_words:"true"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Update JSON tag

@J0
Copy link
Contributor Author

J0 commented Apr 3, 2024

Ah Apologies should have closed this - I actually opened a new PR #1512 as it was easier to refactor starting from scratch.

We can adjust the wording from CustomSMSProvider to SendSMS since it's not merged to infra yet. Do so in a separate PR

@J0 J0 closed this Apr 3, 2024
J0 added a commit that referenced this pull request Apr 3, 2024
## What kind of change does this PR introduce?

We are renaming `CustomEmailProvider` to `SendEmail`. For consistency, I
guess we should rename `CustomSMSProvider` to `SendSMS`. More context in
#1496

This should be fine since [the PR to add env configs for this
hook](https://github.com/supabase/infrastructure/pull/17604) has not
landed in prod
J0 added a commit that referenced this pull request Apr 4, 2024
## What kind of change does this PR introduce?

The send email hook serves as a substitute for the default email client
(e.g. GoMail) across all endpoints.


Supercedes #1496 as it was simpler to visualize the PR when starting
from scratch
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

Moves the mail related refactors from supabase#1496 into a new PR so the hook
related PR is easier to review. This PR moves all EmailActionTypes to
mailer package to establish an explicit link between mailer and the
packages it is used in.

The changes are cosmetic and should not affect underlying functionality.
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

We are renaming `CustomEmailProvider` to `SendEmail`. For consistency, I
guess we should rename `CustomSMSProvider` to `SendSMS`. More context in
supabase#1496

This should be fine since [the PR to add env configs for this
hook](https://github.com/supabase/infrastructure/pull/17604) has not
landed in prod
uxodb pushed a commit to uxodb/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

The send email hook serves as a substitute for the default email client
(e.g. GoMail) across all endpoints.


Supercedes supabase#1496 as it was simpler to visualize the PR when starting
from scratch
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

Moves the mail related refactors from supabase#1496 into a new PR so the hook
related PR is easier to review. This PR moves all EmailActionTypes to
mailer package to establish an explicit link between mailer and the
packages it is used in.

The changes are cosmetic and should not affect underlying functionality.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

We are renaming `CustomEmailProvider` to `SendEmail`. For consistency, I
guess we should rename `CustomSMSProvider` to `SendSMS`. More context in
supabase#1496

This should be fine since [the PR to add env configs for this
hook](https://github.com/supabase/infrastructure/pull/17604) has not
landed in prod
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 13, 2024
## What kind of change does this PR introduce?

The send email hook serves as a substitute for the default email client
(e.g. GoMail) across all endpoints.


Supercedes supabase#1496 as it was simpler to visualize the PR when starting
from scratch
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
## What kind of change does this PR introduce?

Moves the mail related refactors from supabase#1496 into a new PR so the hook
related PR is easier to review. This PR moves all EmailActionTypes to
mailer package to establish an explicit link between mailer and the
packages it is used in.

The changes are cosmetic and should not affect underlying functionality.
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
## What kind of change does this PR introduce?

We are renaming `CustomEmailProvider` to `SendEmail`. For consistency, I
guess we should rename `CustomSMSProvider` to `SendSMS`. More context in
supabase#1496

This should be fine since [the PR to add env configs for this
hook](https://github.com/supabase/infrastructure/pull/17604) has not
landed in prod
LashaJini pushed a commit to LashaJini/auth that referenced this pull request Nov 15, 2024
## What kind of change does this PR introduce?

The send email hook serves as a substitute for the default email client
(e.g. GoMail) across all endpoints.


Supercedes supabase#1496 as it was simpler to visualize the PR when starting
from scratch
@J0 J0 deleted the j0/add_custom_email_sender_hook branch November 21, 2024 08:08
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

3 participants