Re: [pbs-devel] [PATCH proxmox 1/4] sendmail: add sendmail crate

["Shannon Sterz" <s.sterz@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