-
Notifications
You must be signed in to change notification settings - Fork 25
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
Saving generated emails locally #29
base: main
Are you sure you want to change the base?
Conversation
You then just hand these to postfix or sendmail or whatever? I'd been asked for a similar feature, but had been planning it differently - write out the signatures and some metadata, and then have another option to slurp that in and send them off... I'm not entirely sure which I like better. Your approach is definitely simpler and less code. Less to keep track of, less to refactor. The downside is that it requires more know-how on the part of the user to then send those messages off... for example, at the very least you have to have a mailserver configured to send properly - which many don't (which is why Let me ponder it it for a bit. I'll get back to you. :) |
Yup, I transferred the emails to my mail server and handed each of them off to sendmail. I see your point that a fair number of people don't have an MTA setup for this to work. If you don't want to add another mode to Pius for just sending the generated emails, perhaps we can get mailx to send the emails in a fashion similar to sendmail. This is what I have so far but it doesn't seem to be pulling the recipients out of the email metadata like sendmail does. |
# must re-ehlo after STARTTLS | ||
smtp.ehlo() | ||
# Don't want to send auth information unless we're TLS'd | ||
if self.user: |
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 more I think about this approach the more I like it.
However... how do we handle the email_override and the BCC-the-user bit here?
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.
Ping?
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.
Sorry for the extremely late reply. I think these options can be handled by moving their logic to the beginning of the add_options method. Then they will be printed out to the email file or passed to sendmail.
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 you aren't actually writing it to the file... env_to isn't in the object you write out.
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.
Yeah, keeping py2 compat for now. BUt also as mentioned I think we should only add that Bcc header to the message when we're saving it as a proxy for a proper envelope header.
@mborger - you still interested in finishing this? |
Sure. What's next on your mind to look into? |
I think all that's left is:
And then we're good. |
df7299c
to
fb7b273
Compare
Check out my changes. I combined the two address override conditionals to the beginning of the _send_mail method. |
libpius/mailer.py
Outdated
if not os.path.isdir(self.local_mail_dir): | ||
os.mkdir(self.local_mail_dir) | ||
email = open(os.path.join(self.local_mail_dir, msg['To']), 'w') | ||
email.write(str(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.
the env_to isn't being written here...
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.
Good catch but this opens another question. Do we need the copy to yourself to be blind? There is no standard for a BCC header within an email that a sender is supposed to strip before the SMTP submission. We could look into finding a client that does that or add another command to pius to handle sending these emails. If we don't really need the self copy to be blind then we can just add the extra To header to the message and mail senders should send it to both addresses.
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, sendmail's -t
does this. It reads all address headers from the message (to,cc,bcc), acts on them and strips out BCC from the message.
I just tried postfix'q sendmail compat mode, and it also does this.
So... that's probably sufficient.
I would say that if the mailqueue mode is activated, take any env_to
s that are not in the standard to
and add them as Bcc
in the message.
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 added a Bcc header and included a helper function to generate the to_addrs parameter of the send_mail method. Newer versions of python have a send_message method which will extract the headers from the message. I included it with some comments but I think you're still trying to support Python2.
doc/pius.1
Outdated
@@ -47,6 +47,8 @@ Hostname of SMTP server. [default: \fIlocalhost\fP] | |||
Use the pexpect module for signing and drop to the gpg shell for entering the passphrase. [default: false] | |||
.IP "\fB\-I\fP, \fB\-\-import\fP" | |||
Also import the unsigned keys from the keyring into the default keyring. Ignored if \fB\-r\fP is not specified, or if it's the same as the default keyring. | |||
.IP "\fB\-L\fP, \fB\-\-local\-mail\-dir\fP" |
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.
We should probably call this --save-to-mail-dir
or something
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.
Changed to --save-to-mail-dir
libpius/mailer.py
Outdated
if not os.path.isdir(self.local_mail_dir): | ||
os.mkdir(self.local_mail_dir) | ||
email = open(os.path.join(self.local_mail_dir, msg['To']), 'w') | ||
email.write(str(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.
Well, sendmail's -t
does this. It reads all address headers from the message (to,cc,bcc), acts on them and strips out BCC from the message.
I just tried postfix'q sendmail compat mode, and it also does this.
So... that's probably sufficient.
I would say that if the mailqueue mode is activated, take any env_to
s that are not in the standard to
and add them as Bcc
in the message.
libpius/mailer.py
Outdated
else: | ||
msg['To'] = to | ||
env_to = [to, self.mail] | ||
msg['BCC'] = self.mail |
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 think that should be Bcc
libpius/mailer.py
Outdated
@@ -256,38 +258,50 @@ def _send_mail(self, to, msg): | |||
msg['To'] = self.address_override | |||
else: | |||
msg['To'] = to | |||
msg['Bcc'] = self.mail |
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.
we should only do this in the case of self.local_mail_dir
e2562c9
to
6987bca
Compare
@mborger - This code looks great! Thanks for working through the long review process on this one. The only thing left before merging is some docs. Probably a simple command people can run to send off all the emails, in the help for |
to a given folder instead of sending via SMTP.
Hey @mborger - wanna rebase and add some simple comments to the README on this and I can merge it? |
@mborger ? |
Looks like this PR grew a bit stale. Should it be closed? |
one last call, @mborger ? |
Added an option to save the generated emails locally instead of sending through an SMTP server. This is useful if you're signing keys from an air gapped machine.