Re: [pmg-devel] [PATCH pmg-api] fix #3924: ldap: skip non-smtp proxyAddresses

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 18.03.22 13:37, Markus Frank wrote:
> When an entry in proxyAddresses starts with SIP (or everthing
> that is not smtp) no e-mail gets listed on the webpage.
> Therefore no quarantine notification messages are sent.
> 

1. that statement doesn't really tells me any rationale or the actual
   problem, also, which webpage? do you mean the web-interface (GUI)?

2. this loops not only over proxyAddresses but *all* mail attributes
   (user configurable, default: mail, userPrincipalName, proxyAddresses,
   othermailbox, mailAlternativeAddress) so unconditionally skipping all
   that doesn't starts with smtp seems odd

3. the bug report shows that there is a smtp address, it just isn't on
   the first line, should this rather cope with such multi-line stuff
   and better extract the (any?) mail addresses there?

> To change this, every entry with another protocol than smtp,
> should be skipped.
> 
> removed "SMTP" because of lowercase function is called before.

nit: could be a separate patch as its rather unrelated with the
skipping.

> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/PMG/LDAPCache.pm | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PMG/LDAPCache.pm b/src/PMG/LDAPCache.pm
> index df61454..8172329 100755
> --- a/src/PMG/LDAPCache.pm
> +++ b/src/PMG/LDAPCache.pm
> @@ -159,12 +159,17 @@ sub queryusers {
>  	    next if !$user->{attributes}->{$attr};
>  	    foreach my $mail (@{$user->{attributes}->{$attr}}) {
>  		$mail = lc($mail);
> +
> +		# Check if proxyAddresses entry starts with anything

but it's not only 'proxyAddresses' that's accessed here (see 2. above).

> +		# other than smtp and skip if true
> +		next if ($mail =~ m/^(?!.*smtp).*[\:\$]/gs);

iff we'd do it this way a negated match (`!~` vs. `=~`) could be favorable over a
negative-lookahead readability wise.

> +
>  		# Test if the Line starts with one of the following lines:
> -		# proxyAddresses: [smtp|SMTP]:
> +		# proxyAddresses: [smtp]:
>  		# and also discard this starting string, so that $mail is only the
>  		# address without any other characters...
>  
> -		$mail =~ s/^(smtp|SMTP)[\:\$]//gs;
> +		$mail =~ s/^smtp[\:\$]//gs;
>  
>  		if ($mail !~ m/[\{\}\\\/]/ && $mail =~ m/^\S+\@\S+$/) {
>  		    $umails->{$mail} = 1;