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(server): email notifications #8447

Merged
merged 23 commits into from
May 2, 2024
Merged

Conversation

hitech95
Copy link
Contributor

@hitech95 hitech95 commented Apr 2, 2024

  • SMTP transport is validated when SMTP settings are changed.
  • SMTP settings available in the WEB-UI
  • React-Mail template engine. It has built in components to provide modern web to "old" table based layout common in emails and text/plain email.
  • It is possible to view the rendered templates in real-time by running npm run email:dev in the server directory. This just ned the dependencies to be installed and does not require the full server to be running.
  • This can also be integrated with tailwind and generate email templates from already available classes.
  • Simple notification triggered when an user is created from the POST request to /user endpoint.
  • Option to skip sending welcome email when creating a new user

Known issues:

  • To generate server side URLs it is required to have the Server url configured. There is no handler right now if this fails the job also does.
Screenshots

Admin settings
image

Create user form
image

Welcome email template
image

This adapter render the React-Email into HTML and plain/text email.
The output is set as the body of the email.
Allow to use the NestJS-modules-mailer module to send SMTP emails.
This is the base transport for the `NotificationRepository`
This allows to queue email sending jobs for the `EmailService`.
This act as a middleware to properly route the notification to the right transport.
As POC I've only implemented a simple SMTP transport.
…ice`

This trigger an event for the `NotificationRepository` that once processes
by using the global config and per-user config will carry the payload to the right notification transport.
@hitech95 hitech95 marked this pull request as ready for review April 18, 2024 12:26
@hitech95 hitech95 changed the title Simple SMTP Notification POC [SERVER] SMTP Notification POC Apr 18, 2024
@jrasm91 jrasm91 changed the title [SERVER] SMTP Notification POC feat(server): email notifications Apr 30, 2024
@jrasm91 jrasm91 requested a review from bo0tzz April 30, 2024 22:28
Copy link
Contributor

@zackpollard zackpollard left a comment

Choose a reason for hiding this comment

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

Overall looks solid, a few comments but nothing major. One thing I did want to check though is do we currently support encrypted SMTP servers with this? Generally when setting up SMTP you specify whether the server supports encryption and what kind (example from nextcloud)
image

server/src/interfaces/notification.interface.ts Outdated Show resolved Hide resolved
server/src/services/user.service.ts Show resolved Hide resolved
server/src/interfaces/notification.interface.ts Outdated Show resolved Hide resolved
@jrasm91
Copy link
Contributor

jrasm91 commented May 1, 2024

The library we're using will automatically upgrade to smtps if the mail server indicates it support the STARTTLS extension.

https://www.nodemailer.com/smtp/#tls-options

@bo0tzz
Copy link
Member

bo0tzz commented May 1, 2024

automatically upgrade to smtps

This feels prone to breakage or MITM attacks. Is there a way to enforce it, something like a toggle to reject plaintext connections or such?

@danieldietzler
Copy link
Member

automatically upgrade to smtps

This feels prone to breakage or MITM attacks. Is there a way to enforce it, something like a toggle to reject plaintext connections or such?

Yes there is. So maybe enable that by default and have a toggle that optionally allows for insecure connections?

@zackpollard
Copy link
Contributor

FWIW it seems nextcloud does the same thing, there option is None/STARTTLS, so presumably they also auto upgrade

@zackpollard zackpollard merged commit 9bce341 into immich-app:main May 2, 2024
22 checks passed
@aviv926
Copy link
Contributor

aviv926 commented May 2, 2024

I would be happy if we could add documentation to this.
I hope to have some time to document this, can you put your name on Discord for me in case I have questions?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants