-
-
Notifications
You must be signed in to change notification settings - Fork 430
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
Send via SMTP support for aliases #869
base: master
Are you sure you want to change the base?
Conversation
Also change way of importing nanoid
This reverts commit 42c4c80.
Great job! Thanks @sahilph. I think this is definitely a valuable feature. The only point I currently find somewhat awkward is that each alias would get their own credentials. My thought for this feature is that users should be able to connect to the SMTP server using a generic credential, and the SMTP server would automatically detect which specific alias should be used to send the email by looking at the address the user is replying to, and other information that available. This way, users would only have to add their credentials once to Gmail's send-as feature or local email client, and reply to emails sent to any alias with the generic credentials. |
@developStorm Thanks for you feedback. One of the main reasons I developed this was to use Gmail "Send mail as:" feature. And It requires users to add "From:", and username and password for every alias. Also consider one more scenario. Suppose the aliases are getting generated on-the-fly and I want to give a email to website say In the current scenario, since we can enable SMTP per-alias-basis, a reverse-alias will automatically get generated and replying will be easier as I can just reply via reverse-alias. Also I'm not sure how to authenticate and validate, if there was only a single generic credential present. If you have any ideas please let me know and I can implement it. |
@sahilph The idea is that the SMTP server does not need to honor the |
@developStorm While it would be very convenient to have such a feature, I can't really think of a way of implementing this without reading |
@sahilph We just have to guess ;-) For example, if the incoming email to SL SMTP server looks like:
Then now SL SMTP server can guess: "hmm the authenticated user has a recent interaction with |
@developStorm I don't think we should rely on guesswork while sending emails. Also what about sending to someone who we have never sent before ? |
@sahilph Yeah guess is not a perfect idea, but I think it's worse to have user add every single alias they have, when they have let's say 50+ aliases and need to be able to reply as all those aliases. For new recipients, user can manually add them on SL portal before they proceed, as what they have to do now to obtain reply address. |
User don't have to add every single alias. They only need to add the ones they usually send from. For other aliases those who dont have SMTP enabled, sending via reverse-alias is still possible
Then what's the point of SMTP? users can do that already do that using reverse-alias. The whole point of SMTP access (and this PR) is that, if users want to send emails from the aliases they most often send from, there would be no need of creating reverse-alias. Users can just the emails using their aliases via SMTP using their favorite email client. No need to manually login to SL portal and create reverse aliases everytime. For other non-frequently used aliases, use the reverse-alias method as that would still work. TL;DR: For sending via most-frequently used alias -> Use SMTP ; otherwise -> use the regular-reverse alias method. |
@developStorm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks really good! I haven't run it though, it'd be great if you can include how to test this new service in the CONTRBUTING file.
I left some comments, lots of them are rather nits though.
app/config.py
Outdated
@@ -444,3 +444,7 @@ def setup_nameservers(): | |||
|
|||
|
|||
NAMESERVERS = setup_nameservers() | |||
|
|||
# For SMTP support | |||
SMTP_SSL_CERT_FILE = os.environ.get("SMTP_SSL_CERT_FILE") or "/smtp_ssl_cert.pem" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add PATH
suffix to be explicit, so SMTP_SSL_CERT_FILEPATH
instead of SMTP_SSL_CERT_FILE
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And as existing installations don't have the certificates and keys file, should this new service disabled unless the env variables are set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add
PATH
suffix to be explicit, soSMTP_SSL_CERT_FILEPATH
instead ofSMTP_SSL_CERT_FILE
?
I have changed this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And as existing installations don't have the certificates and keys file, should this new service disabled unless the env variables are set?
The service won't start at all, if the files cert/key files do not exist and thus would remain disabled.
return [(False, status.E515)] | ||
|
||
user = alias.user | ||
mailbox = Mailbox.get_by(id=alias.mailbox_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the mailbox should come from the envelope.mail_from as an alias can have several mailboxes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if user can have multiple inboxes, it won't be relevant to include mailbox here.
I added it only for logging as I thought Email Log requires it. But it doesn't, so it can be removed. What are your thoughts. ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mailbox_id
is needed by EmailLog. In this reply phase, this is the mailbox (or a mailbox's authorized address) that sends the email.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for SMTP phase, the envelope.mail_from
, will be the alias itself.
So how to get the relevant mailbox in case of multiple mailboxes ?
I think it just just be removed as we are already setting is_SMTP=True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realized that I haven't done the follow up on the discussion on SMTP_handler file.
SMTP_handler.py
Outdated
|
||
contact = Contact.get_by(website_email=website_email) | ||
if not contact: | ||
LOG.w(f"No contact with {website_email} as website email") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I meant is this is a rather normal case so we shouldn't use logging warning level here. info or debug level is more suitable.
SMTP_handler.py
Outdated
LOG.w(f"No contact with {website_email} as website email") | ||
try: | ||
LOG.d("Create or get contact for website_email:%s", website_email) | ||
contact = get_or_create_contact("", website_email, alias, msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the current get_or_create_contact
handles a lot of cases that aren't necessary for this usage. I think it's better to have a smaller method, which is a stripped down version of the get_or_create_contact
to be used here.
return [(False, status.E515)] | ||
|
||
user = alias.user | ||
mailbox = Mailbox.get_by(id=alias.mailbox_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mailbox_id
is needed by EmailLog. In this reply phase, this is the mailbox (or a mailbox's authorized address) that sends the email.
LOG.e("%s domain isn't known", alias) | ||
return False, status.E503 | ||
|
||
contact = Contact.get_by(website_email=website_email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in another comment, I think we can also handle the case where website_email is a reverse alias here. This should be relatively easy IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some code to handle this case.
- Also create a stripped down function `get_or_create_contact_for_SMTP_phase` - Also change the log to debug instead of warning
This reverts commit 25a56ec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nguyenkims I have made some changes and left some comments.
LOG.e("%s domain isn't known", alias) | ||
return False, status.E503 | ||
|
||
contact = Contact.get_by(website_email=website_email) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added some code to handle this case.
SMTP_handler.py
Outdated
|
||
contact = Contact.get_by(website_email=website_email) | ||
if not contact: | ||
LOG.w(f"No contact with {website_email} as website email") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have changed this to debug level.
SMTP_handler.py
Outdated
LOG.w(f"No contact with {website_email} as website email") | ||
try: | ||
LOG.d("Create or get contact for website_email:%s", website_email) | ||
contact = get_or_create_contact("", website_email, alias, msg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created a similar stripped down version get_or_create_contact_for_SMTP_phase
please check if its fine.
return [(False, status.E515)] | ||
|
||
user = alias.user | ||
mailbox = Mailbox.get_by(id=alias.mailbox_id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for SMTP phase, the envelope.mail_from
, will be the alias itself.
So how to get the relevant mailbox in case of multiple mailboxes ?
I think it just just be removed as we are already setting is_SMTP=True
app/models.py
Outdated
@@ -2557,6 +2575,45 @@ class AliasMailbox(Base, ModelMixin): | |||
alias = orm.relationship(Alias) | |||
|
|||
|
|||
class SMTPCredentials(Base, ModelMixin, PasswordOracle): | |||
__tablename__ = "SMTP_credentials" | |||
__table_args__ = (sa.UniqueConstraint("alias_id", name="uq_alias"),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have removed the __table_args__
email_handler.py
Outdated
add_or_replace_header(msg, "Reply-To", new_reply_to_header) | ||
LOG.d("Reply-To header, new:%s, old:%s", new_reply_to_header, reply_to_header) | ||
# Don't Change Headers when SMTP is enabled for alias | ||
if not is_SMTP_enabled_for_alias: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have reverted this part.
else: | ||
SMTPCredentials.delete_by_alias_id(alias_id=alias.id) | ||
alias.enable_SMTP = enable_SMTP | ||
changed = True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of generating the password in the client it'd be better to generate it server-side and show it once to the client on successful activation:
- No need to add extra dependencies in python and in js (ie. nanoid)
- No need to have js code in the front
- No strange 21 character validation
- Other clients have no need to add nanoid dependencies.
Just generate a random secret, store it in the db and in the next page display show the credentials warning the user that the credentials won't be able to be displayed any more times.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nguyenkims Is this really needed at this stage ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sahilph generally I think it's better to generate secret on server as we can't control client environment. Adding this change should be quite simple right as the server generates the password and send this back to the client to display?
Hey, nice PR. But as a general comment, there aren't any tests added. I would suggest to add some:
|
@acasajus Thanks for the feedback. |
@sahilph @acasajus Having tests for every PR is indeed a good idea. We actually set a limit in terms of code coverage under which a PR won't pass the CI. And tests should be written by the PR author :). It happens several times that writing tests help discover cases that we don't realize during the development. We are not extreme in terms of test coverage though but there should be enough tests to cover the most frequent cases. |
any update? I really need this asap |
Yeah, I am following this with great interest as well. I purchased a year of SL and love much about the service and have brought 2 of my domain on board. After using for a couple of weeks though and mucking with the reverse alias system and clogging my contacts up with extra redundant emails in their profiles, this is the one part that feels clunky. Really hoping this is solved with this above proposed solution. If no way to "send as" SL aliases from email client arrives in a year when renewal arrives, I would have to consider pulling my domains and finding a service for email support and relegate SL to just throwaway emails on their domains. I am hoping a way will be found, as I love SL so far and would love for it to fit. |
I'm not sure i get the point of this function, seen as SimpleLogin is indeed not meant to be a transactional email service. But if there are other use cases for this function, then i would say, "why not". |
What exactly is holding this back? This has been sitting in a completed state for so long, how can we get it over the line? |
Agreed, disappointing to see such a valuable PR just sitting here. |
@nephalemsec @edsimpsons83 the PR isn't complete. We asked for adding tests into it (among other things) and haven't received any news yet. And please keep in mind that this PR is rather for self hosting. To make it available on SL.io, we need to adapt the infra and work on an anti-abuse system first. |
Understood. As @sahilph has stated he believes there would be a bias with the PR author making unit tests is there a plan for SL team to assist? |
@nephalemsec providing unit tests should actually be part of the PR and it is up to the author to write tests. |
Sadly i didn't get a reply from someone explaining the purpose of this function to me. :( |
To be able to send from an alias without needing to log into the SL interface comes to mind. |
So, is this PR just dead now after 6 months? Is there any intent on supporting this feature? Would be nice to actually be able to use SL as a way to send emails from an alias similar to the way iCloud Plus currently lets you do it. |
It's a shame this PR has been left to die. Time to look for a better alternative to SL. |
This reverse-Alias thing really needs a rework asap. This is a core functionality which lacks usability really hard. |
Any luck on hunt for alternatives. Have my own domains, thinking of alternatives to move to once current year sub at SL ends... |
I changed to Fastmail. Handles all the alias stuff I could ever want. |
Was it possible to transfer all this easily without significant interruption? |
yeah you setup a wildcard and it's barely any work. Just keep in mind you can't disable aliases in the same way. It's basically making a rule to send anything to the alias to the trash. So depending on how many you have disabled, that could take a bit to setup. |
Actually you can block an alias. The sender will get a notification that alias doesn't exist. You just create any alias you want to block and then disable it. |
This PR add ability to send via SMTP for aliases. With this emails can be sent directly from aliases via SMTP without the need for creation of reverse-alias.
If enabled, Every alias will have a unique password (generated client side) for SMTP access. Username will be same as alias.
This adds a global option in Settings to enable this feature, without enabling this option, SMTP access will remain disabled for the user.
Note: Even after enabling this option in settings, you would still need to enable SMTP individually per alias. This will also disable receiving via reverse-alias for that particular alias. i.e. Sender's original address will be displayed in "From:".
SMTP server address should ideally be the same where your app is hosted (but this depends on your configuration)
SMTP port: 465
This would need SSL cert/key file as SMTP would be over (implicit) SSL.
This should resolve #679
The screenshots below will make it more clear on how it would operate: