From: "Shannon Sterz" <s.sterz@proxmox.com>
To: "Lukas Wagner" <l.wagner@proxmox.com>,
"Proxmox Backup Server development discussion"
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [PATCH proxmox 1/4] sendmail: add sendmail crate
Date: Mon, 02 Dec 2024 12:02:33 +0100 [thread overview]
Message-ID: <D615X0MIPMMY.3JB37Q4Q4W897@proxmox.com> (raw)
In-Reply-To: <47c2f30e-1676-4acf-a55e-ba2b55660eb6@proxmox.com>
On Mon Dec 2, 2024 at 11:20 AM CET, Lukas Wagner wrote:
> Gave these changes a quick test in Proxmox Backup Server as well as in proxmox-mail-forward.
>
> Looks good!
>
> Tested-by: Lukas Wagner <l.wagner@proxmox.com>
> Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>
>
> (the latter iff the minor issues I mentioned are addressed)
>
thanks for the review! addressed some of your comments in-line and will
send a v2 with the rest.
-->8 snip 8<--
> > +
> > + /// Adds a recipient to the mail with a name.
> > + ///
> > + /// Notes:
> > + ///
> > + /// - If the name contains UTF-8 characters it will be encoded. Then the possibly encoded name
> > + /// and non-encoded email address will be passed to the `To:` header in this format:
> > + /// `{encoded_name} <{email}>`
> > + /// - If multiple receivers are specified, they will be masked so as not to disclose them to
>
> Thinking about the main usecase of this new crate, which is to provide the implementation
> to send notification mails from PVE and PBS, I think the masking part should be
> configurable.
>
> In the most common case, a notification mail might go to the members of an infra team of
> a organization, where the identities of other team members is not really sensitive information.
> I'd actually go as far and say that the info "who else was notified" is actually quite valuable
> and useful to have.
>
> Then again, I can see the benefits of masking, e.g. in the case of PBS datastore notifications,
> which might go to non-admin users (e.g. when PBS is offered as a service a la Tuxis).
>
> I don't care that much whether this is opt-in or opt-out at the crate level, but at the
> 'sendmail target' level I'd make this configurable and opt-in (gut feeling and to not
> change the current behavior, I'd be happy to be convinced for another way :) )
>
> What are your thoughts about this?
making this configurable sounds reasonable to me. i'd tend towards
making the masking opt-out, though. at least on a crate level. my
use-case for this crate is to send mails to all participants of a
training, so there disclosing the mail addresses of other participants
could be really bad (even legally actionable, afaict). i can see the
value of this information in a notification scenario. however, i think
the implications of forgetting to disclose this in some scenarios is
much less detrimental than doing so in scenarios where we don't want to
disclose them.
-->8 snip 8<--
> I think with added support for attachments and the other changes it might make sense
> to start breaking the method into smaller sub-methods. Personally I found it a bit
> hard to follow the way it is right now :) At very least I'd try to break this into format_header
> and format_body, in the latter one could probably also break out the attachment part.
>
> What do you think?
yep makes sense, this is starting to spaghettify so yeah, i'll break
these out a bit.
> Also, I think debian packaging should be added in this or a separate commit.
will try to add this in a v2.
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
next prev parent reply other threads:[~2024-12-02 11:03 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-29 10:53 Shannon Sterz
2024-11-29 10:53 ` [pbs-devel] [PATCH proxmox 2/4] notify: switchi sendmail endpoint over to new crate Shannon Sterz
2024-12-02 10:20 ` Lukas Wagner
2024-11-29 10:53 ` [pbs-devel] [PATCH proxmox 3/4] sendmail: add mail-forwarder feature Shannon Sterz
2024-11-29 10:53 ` [pbs-devel] [PATCH proxmox 4/4] notify: use proxmox-sendmail forward implementation Shannon Sterz
2024-11-29 14:38 ` [pbs-devel] [PATCH proxmox 1/4] sendmail: add sendmail crate Thomas Lamprecht
2024-12-02 10:20 ` Lukas Wagner
2024-12-02 11:02 ` Shannon Sterz [this message]
2024-12-02 12:11 ` Lukas Wagner
2024-12-02 12:21 ` Shannon Sterz
2024-12-02 13:00 ` Shannon Sterz
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=D615X0MIPMMY.3JB37Q4Q4W897@proxmox.com \
--to=s.sterz@proxmox.com \
--cc=l.wagner@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal