[pmg-devel] [PATCH pmg-api v2 08/10] RuleDB/Remove: add attachment quarantine option

[Thomas Lamprecht]

On 9/30/19 2:55 PM, Dominik Csapak wrote:
> So that users can choose to copy the mail to the attachment quarantine,
> if they remove some (or all attachments)
> 
> Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
> ---
>  src/PMG/RuleDB/Remove.pm | 34 ++++++++++++++++++++++++++++++----
>  1 file changed, 30 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/RuleDB/Remove.pm b/src/PMG/RuleDB/Remove.pm
> index ee6e37b..248dad6 100644
> --- a/src/PMG/RuleDB/Remove.pm
> +++ b/src/PMG/RuleDB/Remove.pm
> @@ -41,7 +41,7 @@ sub priority {
>  }
>  
>  sub new {
> -    my ($type, $all, $text, $ogroup) = @_;
> +    my ($type, $all, $text, $ogroup, $quar) = @_;

why not just use $quarantine written out??
No gain in those strange to read abbreviations here.

>  
>      my $class = ref($type) || $type;
>  
> @@ -51,6 +51,7 @@ sub new {
>  
>      $self->{all} = $all;
>      $self->{text} = $text;
> +    $self->{quar} = $quar;
..
>  
>      return $self;
>  }
> @@ -64,7 +65,9 @@ sub load_attr {
>  
>      my $obj;
>  
> -    if ($value =~ m/^([01])(\:(.*))?$/s) {
> +    if ($value =~ m/^([01])\,([01])(\:(.*))?$/s) {
> +	$obj = $class->new($1, $4, $ogroup, $2);
> +    } elsif ($value =~ m/^([01])(\:(.*))?$/s) {
>  	$obj = $class->new($1, $3, $ogroup);
>      } else {
>  	$obj = $class->new(0, undef, $ogroup);
> @@ -83,6 +86,7 @@ sub save {
>      defined($self->{ogroup}) || die "undefined ogroup: ERROR";
>  
>      my $value = $self->{all} ? '1' : '0';
> +    $value .= ','. ($self->{quar} ? '1' : '0');
>  
>      if ($self->{text}) {
>  	$value .= ":$self->{text}";
> @@ -188,7 +192,7 @@ sub delete_marked_parts {
>  
>  sub execute {
>      my ($self, $queue, $ruledb, $mod_group, $targets,
> -	$msginfo, $vars, $marks) = @_;
> +	$msginfo, $vars, $marks, $ldap) = @_;

the $ldap param is not documented in the POD of RuleDB::Object, between all
the DB $sth->execute stamements it was a bit hard to track down the call
site in pmg-smtp-filter ^^

unrelated to the patch, but could be nice to mention that this is already
always passed to all RuleDB executes. We often have the method signature
also in the abstract "implement me in subclass" like methods, but they lack
here - could be good to add this information someday ^^

>  
>      my $rulename = $vars->{RULE} // 'unknown';
>  
> @@ -201,7 +205,10 @@ sub execute {
>  
>      my $html = PMG::Utils::subst_values($self->{text}, $vars);
>  
> -    $html = "This attachment was removed: __FILENAME__\n" if !$html;
> +    if (!$html) {
> +	$html = "This attachment was removed: __FILENAME__\n";
> +	$html .= "It was put into the Attachment Quarantine, please contact your Administrator\n" if $self->{quar};
> +    }
>  
>      my $rtype = "text/plain";
>  
> @@ -212,6 +219,17 @@ sub execute {
>      foreach my $ta (@$subgroups) {
>  	my ($tg, $entity) = (@$ta[0], @$ta[1]);
>  
> +	# move in attachment quarantine if configured

you mix "move" and "copy" here a few times, IMO this makes it slightly
confusing, effectively you do:
full copy     -> att. quarantine
filtered mail -> continue

> +	if ($self->{quar}) {
> +	    my $quarentity = $entity->dup;

$quarentity -> $entity_copy (or $original_entity ?)

making it more clear that this is a copy of the original untouched
entity, it isn't in the quarantine here (yet) and the name would be
a bit hard to read anyway, so if you object to above then at least I'd
add underscore, e.g., quarantine_entity

> +	    PMG::Utils::remove_marks($quarentity);
> +	    if (my $qid = $queue->quarantine_mail($ruledb, 'A', $quarentity, $tg, $msginfo, $vars, $ldap)) {
> +		foreach (@$tg) {
> +		    syslog ('info', "$queue->{logid}: moved mail for <%s> to attachment quarantine - %s (rule: %s)", $_, $qid, $rulename);
> +		}
> +	    }
> +	}
> +
>  	# handle singlepart mails
>  	my $ctype = $entity->head->mime_type;
>  	if (!$entity->is_multipart && (!$self->{all} || $ctype !~ m|text/.*|i)) {
> @@ -250,6 +268,12 @@ sub properties {
>  	    type => 'boolean',
>  	    optional => 1,
>  	},
> +	quar => {

s/quar/quarantine/

to hint that it's copied over there we could also say:
'quarantine_cc' but such a hint is then possible solved by documenting
and UI, albeit user (API) visible property names should be named with
at least some thoughts about the fact that some users always will interpret
them a bit literal.

> +	    description => "Copy to attachement Quarantine.",

Copy what? Maybe: "Copy original mail to attachment quarantine."

(note also the s/attachement/attachment/ typo fix)

> +	    type => 'boolean',
> +	    default => 0,
> +	    optional => 1,
> +	},
>  	text => {
>  	    description => "The replacement text.",
>  	    type => 'string',
> @@ -264,6 +288,7 @@ sub get {
>      return {
>  	text => $self->{text},
>  	all => $self->{all},
> +	quar => $self->{quar},

s/quar/quarantine/

>      };
>  }
>  
> @@ -272,6 +297,7 @@ sub update {
>  
>      $self->{text} = $param->{text};
>      $self->{all} = $param->{all} ? 1 : 0;
> +    $self->{quar} = $param->{quar} ? 1 : 0;

s/quar/quarantine/

>  }
>  
>  1;
>