Re: [pmg-devel] SPAM: [PATCH pmg-api 1/1] fix: 2971, to DKIM signing for OOO messages by using postmaster@<domain> as a sender if it is not present.

["Shannon Sterz" <s.sterz@proxmox.com>]

hey thanks for contributing to PMG!

i just wanted to provide some quick feedback on the code you sent in,
but please make sure to follow our development guidelines and also note
that if you haven't already, you need to sign a harmony CLA [1].

some notes on the commit message:

a) it is to long, we generally enforce a limit of 70 characters per
   line. please provide a short summary in the first line and if needed
   more context in a second paragraph.
b) i am assumig OOO stands for "out of office", however your patch
   applies to all mails as long as they don't have a sender. so this
   commit message is somewhat confusing.

On Sat May 18, 2024 at 4:58 PM CEST, Nigel van Keulen wrote:
> ---
>  src/PMG/RuleDB/Accept.pm | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/src/PMG/RuleDB/Accept.pm b/src/PMG/RuleDB/Accept.pm
> index e3e39a7..8a6261f 100644
> --- a/src/PMG/RuleDB/Accept.pm
> +++ b/src/PMG/RuleDB/Accept.pm
> @@ -101,8 +101,16 @@ sub execute {
>  	PMG::Utils::remove_marks($entity);
>
>  	if ($dkim->{sign}) {
> +
> +		my $mailSender = $msginfo->{sender};
> +		if ($mailSender eq '') {
> +			my $mailDomain = $msginfo->{domain};
> +			$mailSender = "postmaster\@$mailDomain";

i wonder if more or less hardcoding a sender for mails here makes sense.
it might be neater to make this configurable somehow.

i also wonder if this adds much functionality in reality. most users may
be able to just use the `dkim-use-domain` to use the sender from the
envelop instead.

> +			syslog('info', "%s: No sender found, using default sender: %s", $queue->{logid}, $mailSender);

note that this line is too long. lines should not be longer than 100
characters. you can wrap this line like so to fit that limit

```pl
            syslog(
                'info',
                "%s: No sender found, using default sender: %s",
                $queue->{logid},
                $mailSender
            );
```

this would be in accordance with our style guide [2]. also note that
you did not correctly indent your code. please follow the indentation
guidelines in the style guide as well.

> +		}
> +
>  	    eval {
> -		$entity = PMG::DKIMSign::sign_entity($entity, $dkim, $msginfo->{sender});
> +		$entity = PMG::DKIMSign::sign_entity($entity, $dkim, $mailSender);
>  	    };
>  	    if ($@) {
>  		syslog('warning',

[1]: https://pmg.proxmox.com/wiki/index.php/Developer_Documentation#Software_License_and_Copyright
[2]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Breaking_long_lines_and_strings


_______________________________________________
pmg-devel mailing list
pmg-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel