[pmg-devel] [PATCH pmg-api/docs/gui] implement and combination and inversion of groups and objects

[pmg-devel] [PATCH pmg-api/docs/gui] implement and combination and inversion of groups and objects

[Dominik Csapak]

This series is the backend part of the and/inversion addition to the
rule system.

The first 4 patches of pmg-api and the first one of pmg-docs are only
preparations and should not change the behaviour.

There is still one WIP patch for the ModGroup explosion, i did not find
a better way to do this for now.

The gui change is rather simple, just one additional drop down for
groups + one for each type in each rule.

changes from rfc:
* added docs + gui
* some minor bugfixes
* fixed the api (forgot to add the info to the GET calls in rfc)
* changed output for `pmgdb dump` so we get that info in the pmgreport too

pmg-api:

Dominik Csapak (12):
  RuleCache: remove unnecessary copying of marks
  RuleCache: reorganize to keep group structure
  RuleCache: reorganize how we  gather marks and spaminfo
  api: refactor rule parameters
  add objectgroup attributes and/invert
  add rule attributes and/invert (for each relevant type)
  RuleCache: load rule/objectgroup attributes from database
  RuleCache: implement and/invert for when/from/to
  MailQueue: return maximum AID
  WIP: ModGroup: add possibility to explode to all targets
  RuleCache: implement and/invert for what matches
  pmgdb: extend dump output to include add/invert

 src/PMG/API2/ObjectGroupHelpers.pm |  51 ++++-
 src/PMG/API2/RuleDB.pm             |  20 +-
 src/PMG/API2/Rules.pm              |  94 ++++++--
 src/PMG/CLI/pmgdb.pm               |  38 +++-
 src/PMG/DBTools.pm                 |  30 +++
 src/PMG/MailQueue.pm               |   4 +-
 src/PMG/ModGroup.pm                |  17 ++
 src/PMG/RuleCache.pm               | 338 +++++++++++++++++++++++------
 src/PMG/RuleDB.pm                  | 314 +++++++++++++++++++++------
 src/PMG/RuleDB/Remove.pm           |  28 ++-
 src/PMG/Utils.pm                   |   2 +
 src/bin/pmg-smtp-filter            |  21 +-
 12 files changed, 754 insertions(+), 203 deletions(-)

pmg-docs:

Dominik Csapak (2):
  rule system: add a small section about matching rules
  rule system: explain new and mode and invert flag

 pmg-mail-filter.adoc | 54 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

pmg-gui:

Dominik Csapak (2):
  rules: use tree panel instead of grouping feature of the grid
  rules/objects: add mode selector dropdown

 css/ext6-pmg.css               |   7 +++
 js/Makefile                    |   1 +
 js/ObjectGroup.js              |  64 +++++++++++++++++++-
 js/ObjectGroupConfiguration.js |   4 ++
 js/RuleInfo.js                 | 103 ++++++++++++++++++++++++++-------
 js/form/ModeSelector.js        |  11 ++++
 6 files changed, 167 insertions(+), 23 deletions(-)
 create mode 100644 js/form/ModeSelector.js

-- 
2.30.2

[pmg-devel] [PATCH pmg-api 01/12] RuleCache: remove unnecessary copying of marks

[Dominik Csapak]

two things that are wrong here
* what_match_targets never returns a non empty list
* we copy the list just returned just to append it to itself again

My guess is that we meant to copy the original list, not the just
acquired one, and append it to the one just received. But that never did
make a difference, since we only ever check for defined-ness on that
exact list, and the only Object that this applies to (Spam) always
returns an empty list with the spaminfo (so it's always defined in that
case).

Since this was always the behavior AFAICT, just remove the unnecessary
copy of the list for now. If we encounter any actual bugs with that, we
can still implement it back in the right way (copy the original list).

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index b8690ea..51d8a07 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -322,9 +322,7 @@ sub what_match {
 	    my $target_info;
 	    if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
 		foreach my $k (keys %$target_info) {
-		    my $cmarks = $target_info->{$k}->{marks}; # make a copy
 		    $res->{$k} = $target_info->{$k};
-		    push @{$res->{$k}->{marks}}, @$cmarks if $cmarks;
 		}
 	    }
 	}
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 02/12] RuleCache: reorganize to keep group structure

[Dominik Csapak]

Currently we 'or' combine all objects of a type (from/to/what/when)
regardless of group, so we only keep a single list of all objects.

Since we want to introduce different logic (and/invert) we want to keep
the configured group structure. This patch does this, wihtout chaning
the current matching logic (still all 'or'-ed).

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm | 115 ++++++++++++++++++++++++-------------------
 1 file changed, 64 insertions(+), 51 deletions(-)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index 51d8a07..fd22a16 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -28,6 +28,14 @@ sub new {
 
     my $sha1 = Digest::SHA->new;
 
+    my $type_map =  {
+	0 => "from",
+	1 => "to",
+	2 => "when",
+	3 => "what",
+	4 => "action",
+    };
+
     eval {
 	$dbh->begin_work;
 
@@ -53,7 +61,11 @@ sub new {
 	    $sha1->add(join(',', $ref->{id}, $ref->{name}, $ref->{priority}, $ref->{active},
 			    $ref->{direction}) . "|");
 
-	    my ($from, $to, $when, $what, $action);
+	    $self->{"$ruleid:from"} = { groups => [] };
+	    $self->{"$ruleid:to"} =  { groups => [] };
+	    $self->{"$ruleid:when"} = { groups => [] };
+	    $self->{"$ruleid:what"} = { groups => [] };
+	    $self->{"$ruleid:action"} = { groups => [] };
 
 	    my $sth1 = $dbh->prepare(
 		"SELECT Objectgroup_ID, Grouptype FROM RuleGroup " .
@@ -64,20 +76,7 @@ sub new {
 	    while (my $ref1 = $sth1->fetchrow_hashref()) {
 		my $gtype = $ref1->{grouptype};
 		my $groupid = $ref1->{objectgroup_id};
-
-		# emtyp groups differ from non-existent groups!
-
-		if ($gtype == 0) {      #from
-		    $from = [] if !defined ($from);
-		} elsif ($gtype == 1) { # to
-		    $to = [] if !defined ($to);
-		} elsif ($gtype == 2) { # when
-		    $when = [] if !defined ($when);
-		} elsif ($gtype == 3) { # what
-		    $what = [] if !defined ($what);
-		} elsif ($gtype == 4) { # action
-		    $action = [] if !defined ($action);
-		}
+		my $objects = [];
 
 		my $sth2 = $dbh->prepare(
 		    "SELECT ID FROM Object where Objectgroup_ID = '$groupid' " .
@@ -90,14 +89,9 @@ sub new {
 		    $sha1->add (join (',', $objid, $gtype, $groupid) . "|");
 		    $sha1->add ($obj->{digest}, "|");
 
-		    if ($gtype == 0) {      #from
-			push @$from, $obj;
-		    } elsif ($gtype == 1) { # to
-			push @$to,  $obj;
-		    } elsif ($gtype == 2) { # when
-			push @$when,  $obj;
-		    } elsif ($gtype == 3) { # what
-			push @$what,  $obj;
+		    push @$objects, $obj;
+
+		    if ($gtype == 3) { # what
 			if ($obj->otype == PMG::RuleDB::ArchiveFilter->otype ||
 			    $obj->otype == PMG::RuleDB::MatchArchiveFilename->otype)
 			{
@@ -111,20 +105,20 @@ sub new {
 			    }
 			}
 		    } elsif ($gtype == 4) { # action
-			push @$action, $obj;
 			$self->{"$ruleid:final"} = 1 if $obj->final();
 		    }
 		}
 		$sth2->finish();
+
+		my $group = {
+		    objects => $objects,
+		};
+
+		my $type = $type_map->{$gtype};
+		push $self->{"$ruleid:$type"}->{groups}->@*, $group;
 	    }
 
 	    $sth1->finish();
-
-	    $self->{"$ruleid:from"} = $from;
-	    $self->{"$ruleid:to"} =  $to;
-	    $self->{"$ruleid:when"} = $when;
-	    $self->{"$ruleid:what"} = $what;
-	    $self->{"$ruleid:action"} = $action;
 	}
 
 	# Cache Greylist Exclusion
@@ -203,7 +197,15 @@ sub get_actions {
 
     defined($ruleid) || die "undefined rule id: ERROR";
 
-    return $self->{"$ruleid:action"};
+    my $actions = $self->{"$ruleid:action"};
+
+    return undef if scalar($actions->{groups}->@*) == 0;
+
+    my $res = [];
+    for my $action ($actions->{groups}->@*) {
+	push $res->@*, $action->{objects}->@*;
+    }
+    return $res;
 }
 
 sub greylist_match {
@@ -239,15 +241,17 @@ sub from_match {
 
     my $from = $self->{"$ruleid:from"};
 
-    return 1 if !defined ($from);
+    return 1 if scalar($from->{groups}->@*) == 0;
 
     # postfix prefixes ipv6 addresses with IPv6:
     if (defined($ip) && $ip =~ /^IPv6:(.*)/) {
 	$ip = $1;
     }
 
-    foreach my $obj (@$from) {
-	return 1 if $obj->who_match($addr, $ip, $ldap);
+    for my $group ($from->{groups}->@*) {
+	for my $obj ($group->{objects}->@*) {
+	    return 1 if $obj->who_match($addr, $ip, $ldap);
+	}
     }
 
     return 0;
@@ -258,12 +262,15 @@ sub to_match {
 
     my $to = $self->{"$ruleid:to"};
 
-    return 1 if !defined ($to);
+    return 1 if scalar($to->{groups}->@*) == 0;
 
-    foreach my $obj (@$to) {
-	return 1 if $obj->who_match($addr, undef, $ldap);
+    for my $group ($to->{groups}->@*) {
+	for my $obj ($group->{objects}->@*) {
+	    return 1 if $obj->who_match($addr, undef, $ldap);
+	}
     }
 
+
     return 0;
 }
 
@@ -272,10 +279,12 @@ sub when_match {
 
     my $when = $self->{"$ruleid:when"};
 
-    return 1 if !defined ($when);
+    return 1 if scalar($when->{groups}->@*) == 0;
 
-    foreach my $obj (@$when) {
-	return 1 if $obj->when_match($time);
+    for my $group ($when->{groups}->@*) {
+	for my $obj ($group->{objects}->@*) {
+	    return 1 if $obj->when_match($time);
+	}
     }
 
     return 0;
@@ -292,7 +301,7 @@ sub what_match {
     # $res->{$target}->{marks} is only used in apply_rules() to exclude some
     # targets (spam blacklist and whitelist)
 
-    if (!defined ($what)) {
+    if (scalar($what->{groups}->@*) == 0) {
 	# match all targets
 	foreach my $target (@{$msginfo->{targets}}) {
 	    $res->{$target}->{marks} = [];
@@ -304,10 +313,12 @@ sub what_match {
 
     my $marks;
 
-    foreach my $obj (@$what) {
-	if (!$obj->can('what_match_targets')) {
-	    if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
-		push @$marks, @$match;
+    for my $group ($what->{groups}->@*) {
+	for my $obj ($group->{objects}->@*) {
+	    if (!$obj->can('what_match_targets')) {
+		if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
+		    push @$marks, @$match;
+		}
 	    }
 	}
     }
@@ -317,12 +328,14 @@ sub what_match {
 	$res->{marks} = $marks;
     }
 
-    foreach my $obj (@$what) {
-	if ($obj->can ("what_match_targets")) {
-	    my $target_info;
-	    if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
-		foreach my $k (keys %$target_info) {
-		    $res->{$k} = $target_info->{$k};
+    for my $group ($what->{groups}->@*) {
+	for my $obj ($group->{objects}->@*) {
+	    if ($obj->can ("what_match_targets")) {
+		my $target_info;
+		if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
+		    foreach my $k (keys %$target_info) {
+			$res->{$k} = $target_info->{$k};
+		    }
 		}
 	    }
 	}
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 03/12] RuleCache: reorganize how we gather marks and spaminfo

[Dominik Csapak]

instead of collecting the spaminfo (+match) seperately, collect this
per target together with the regular marks. With this, we can omit the
'global' marks list, since each target has their own anyway.

We want this, since when we'll implement and/invert for matches, the marks
can differ between targets, since the spamlevel can diverge for them and
that can be and-combined with objects that add marks. For that to be
possible we have to save each match + info per target instead of
globally.

Since we don't change the actual matching behaviour with this patch,
for the remove action, we can simply use the marks from the first target
(as they currently have to be identical).

Conversely, we currently save the spaminfo per target, but later in
pmg-smtp-filter we only ever use the first one we encounter, so instead
save it only the first time and use that.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm     | 32 ++++++++++----------------------
 src/PMG/RuleDB/Remove.pm | 19 +++++++++++++++----
 src/bin/pmg-smtp-filter  | 18 +++++-------------
 3 files changed, 30 insertions(+), 39 deletions(-)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index fd22a16..4f7ebe7 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -304,37 +304,25 @@ sub what_match {
     if (scalar($what->{groups}->@*) == 0) {
 	# match all targets
 	foreach my $target (@{$msginfo->{targets}}) {
-	    $res->{$target}->{marks} = [];
+	    $res->{targets}->{$target}->{marks} = [];
 	}
-
-	$res->{marks} = [];
 	return $res;
     }
 
-    my $marks;
-
     for my $group ($what->{groups}->@*) {
 	for my $obj ($group->{objects}->@*) {
 	    if (!$obj->can('what_match_targets')) {
 		if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
-		    push @$marks, @$match;
+		    for my $target ($msginfo->{targets}->@*) {
+			push $res->{targets}->{$target}->{marks}->@*, $match->@*;
+		    }
 		}
-	    }
-	}
-    }
-
-    foreach my $target (@{$msginfo->{targets}}) {
-	$res->{$target}->{marks} = $marks;
-	$res->{marks} = $marks;
-    }
-
-    for my $group ($what->{groups}->@*) {
-	for my $obj ($group->{objects}->@*) {
-	    if ($obj->can ("what_match_targets")) {
-		my $target_info;
-		if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
-		    foreach my $k (keys %$target_info) {
-			$res->{$k} = $target_info->{$k};
+	    } else {
+		if (my $target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
+		    foreach my $k (keys $target_info->%*) {
+			push $res->{targets}->{$k}->{marks}->@*, $target_info->{$k}->{marks}->@*;
+			# only save spaminfo once
+			$res->{spaminfo} = $target_info->{$k}->{spaminfo} if !defined($res->{spaminfo});
 		    }
 		}
 	    }
diff --git a/src/PMG/RuleDB/Remove.pm b/src/PMG/RuleDB/Remove.pm
index e7c353c..5812602 100644
--- a/src/PMG/RuleDB/Remove.pm
+++ b/src/PMG/RuleDB/Remove.pm
@@ -198,9 +198,15 @@ sub execute {
 
     my $rulename = encode('UTF-8', $vars->{RULE} // 'unknown');
 
-    if (!$self->{all} && ($#$marks == -1)) {
-	# no marks
-	return;
+    if (!$self->{all}) {
+	my $found_mark = 0;
+	for my $target (keys $marks->{targets}->%*) {
+	    if (scalar($marks->{targets}->{$target}->{marks}->@*) > 0) {
+		$found_mark = 1;
+		last;
+	    }
+	}
+	return if !$found_mark;
     }
 
     my $subgroups = $mod_group->subgroups ($targets);
@@ -256,7 +262,12 @@ sub execute {
 	}
 
 	$self->{message_seen} = 0;
-	$self->delete_marked_parts($queue, $entity, $html, $rtype, $marks, $rulename);
+
+	# since all matches are or combinded, marks for all targets must be the same if they exist
+	# so simply use the first one here
+	my $match_marks = $marks->{targets}->{$tg->[0]}->{marks};
+
+	$self->delete_marked_parts($queue, $entity, $html, $rtype, $match_marks, $rulename);
 	delete $self->{message_seen};
 
 	if ($msginfo->{testmode}) {
diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
index 7da3de8..71043b0 100755
--- a/src/bin/pmg-smtp-filter
+++ b/src/bin/pmg-smtp-filter
@@ -276,8 +276,9 @@ sub apply_rules {
 	foreach my $target (@{$msginfo->{targets}}) {
 	    next if $final->{$target};
 	    next if !defined ($rule_marks{$rule->{id}});
-	    next if !defined ($rule_marks{$rule->{id}}->{$target});
-	    next if !defined ($rule_marks{$rule->{id}}->{$target}->{marks});
+	    next if !defined ($rule_marks{$rule->{id}}->{targets});
+	    next if !defined ($rule_marks{$rule->{id}}->{targets}->{$target});
+	    next if !defined ($rule_marks{$rule->{id}}->{targets}->{$target}->{marks});
 	    next if !$rulecache->to_match ($rule->{id}, $target, $ldap);
 
 	    $final->{$target} = $fin;
@@ -320,24 +321,15 @@ sub apply_rules {
 	my $targets = $rule_targets{$rule->{id}};
 	next if !$targets;
 
-	my $spaminfo;
-	foreach my $t (@$targets) {
-	    if ($rule_marks{$rule->{id}}->{$t} && $rule_marks{$rule->{id}}->{$t}->{spaminfo}) {
-		$spaminfo = $rule_marks{$rule->{id}}->{$t}->{spaminfo};
-		# we assume spam info is the same for all matching targets
-		last;
-	    }
-	}
-
 	my $vars = $self->get_prox_vars (
-	    $queue, $entity, $msginfo, $rule, $rule_targets{$rule->{id}}, $spaminfo);
+	    $queue, $entity, $msginfo, $rule, $rule_targets{$rule->{id}}, $rule_marks{$rule->{id}}->{spaminfo});
 
 	my @sorted_actions = sort {$a->priority <=> $b->priority} @{$rule_actions{$rule->{id}}};
 
 	foreach my $action (@sorted_actions) {
 	    $action->execute(
 		$queue, $self->{ruledb}, $mod_group, $rule_targets{$rule->{id}}, $msginfo, $vars,
-		$rule_marks{$rule->{id}}->{marks}, $ldap
+		$rule_marks{$rule->{id}}, $ldap
 	    );
 	    last if $action->final;
 	}
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 04/12] api: refactor rule parameters

[Dominik Csapak]

makes it easier to add new ones

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/API2/RuleDB.pm | 20 ++++++--------------
 src/PMG/API2/Rules.pm  | 41 +++++++++++++++++++++++++++--------------
 2 files changed, 33 insertions(+), 28 deletions(-)

diff --git a/src/PMG/API2/RuleDB.pm b/src/PMG/API2/RuleDB.pm
index 1fddb32..064e72f 100644
--- a/src/PMG/API2/RuleDB.pm
+++ b/src/PMG/API2/RuleDB.pm
@@ -162,7 +162,7 @@ __PACKAGE__->register_method({
     permissions => { check => [ 'admin' ] },
     parameters => {
 	additionalProperties => 0,
-	properties => {
+	properties => PMG::API2::Rules::get_rule_params({
 	    name => {
 		description => "Rule name",
 		type => 'string',
@@ -173,19 +173,7 @@ __PACKAGE__->register_method({
 		minimum => 0,
 		maximum => 100,
 	    },
-	    direction => {
-		description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
-		type => 'integer',
-		minimum => 0,
-		maximum => 2,
-		optional => 1,
-	    },
-	    active => {
-		description => "Flag to activate rule.",
-		type => 'boolean',
-		optional => 1,
-	    },
-	},
+	}),
     },
     returns => { type => 'integer' },
     code => sub {
@@ -196,6 +184,10 @@ __PACKAGE__->register_method({
 	my $rule = PMG::RuleDB::Rule->new (
 	    $param->{name}, $param->{priority}, $param->{active}, $param->{direction});
 
+	for my $key (keys PMG::API2::Rules::get_rule_params()->%*) {
+	    $rule->{$key} = $param->{$key} if defined($param->{$key});
+	}
+
 	return $rdb->save_rule($rule);
     }});
 
diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm
index 4f8c10b..c48370f 100644
--- a/src/PMG/API2/Rules.pm
+++ b/src/PMG/API2/Rules.pm
@@ -136,6 +136,31 @@ __PACKAGE__->register_method ({
 	return $data;
    }});
 
+my $rule_params = {
+    direction => {
+	description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
+	type => 'integer',
+	minimum => 0,
+	maximum => 2,
+	optional => 1,
+    },
+    active => {
+	description => "Flag to activate rule.",
+	type => 'boolean',
+	optional => 1,
+    },
+};
+
+sub get_rule_params {
+    my ($base) = @_;
+    $base //= {};
+    return {
+	$base->%*,
+	$rule_params->%*
+    };
+}
+
+
 __PACKAGE__->register_method ({
     name => 'update_config',
     path => 'config',
@@ -146,7 +171,7 @@ __PACKAGE__->register_method ({
     permissions => { check => [ 'admin' ] },
     parameters => {
 	additionalProperties => 0,
-	properties => {
+	properties => get_rule_params({
 	    id => {
 		description => "Rule ID.",
 		type => 'integer',
@@ -156,18 +181,6 @@ __PACKAGE__->register_method ({
 		type => 'string',
 		optional => 1,
 	    },
-	    active => {
-		description => "Flag to activate rule.",
-		type => 'boolean',
-		optional => 1,
-	    },
-	    direction => {
-		description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
-		type => 'integer',
-		minimum => 0,
-		maximum => 2,
-		optional => 1,
-	    },
 	    priority => {
 		description => "Rule priotity.",
 		type => 'integer',
@@ -175,7 +188,7 @@ __PACKAGE__->register_method ({
 		maximum => 100,
 		optional => 1,
 	    },
-	},
+	}),
     },
     returns => { type => "null" },
     code => sub {
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 05/12] add objectgroup attributes and/invert

[Dominik Csapak]

add a new table Objectgroup_Attributes where we can save additional
attributes for objectgroups (like the Attribut tables for objects).

Adds two new attributes for the groups:
* and
* invert

These will modify the match behaviour for object groups

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/API2/ObjectGroupHelpers.pm |  43 ++++++++-
 src/PMG/DBTools.pm                 |  15 +++
 src/PMG/RuleDB.pm                  | 145 ++++++++++++++++++++++-------
 3 files changed, 162 insertions(+), 41 deletions(-)

diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
index 48078fb..a08a6a3 100644
--- a/src/PMG/API2/ObjectGroupHelpers.pm
+++ b/src/PMG/API2/ObjectGroupHelpers.pm
@@ -46,13 +46,29 @@ sub format_object_group {
 
     my $res = [];
     foreach my $og (@$ogroups) {
-	push @$res, {
-	    id => $og->{id}, name => $og->{name}, info => $og->{info}
-	};
+	my $group = { id => $og->{id}, name => $og->{name}, info => $og->{info} };
+	$group->{and} = $og->{and} if defined($og->{and});
+	$group->{invert} = $og->{invert} if defined($og->{invert});
+	push @$res, $group;
     }
     return $res;
 }
 
+my $group_attributes = {
+    and => {
+	description => "If set to 1, objects in this group are 'and' combined.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    invert => {
+	description => "If set to 1, the resulting match is inverted.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+};
+
 sub register_group_list_api {
     my ($apiclass, $oclass) = @_;
 
@@ -86,6 +102,11 @@ sub register_group_list_api {
 	    return format_object_group($ogroups);
 	}});
 
+    my $additional_parameters = {};
+    if ($oclass =~ /^(?:what|when|who)$/i) {
+	$additional_parameters = { $group_attributes->%* };
+    }
+
     $apiclass->register_method({
 	name => "create_${oclass}_group",
 	path => $oclass,
@@ -108,6 +129,7 @@ sub register_group_list_api {
 		    maxLength => 255,
 		    optional => 1,
 		},
+		$additional_parameters->%*,
 	    },
 	},
 	returns => { type => 'integer' },
@@ -119,6 +141,10 @@ sub register_group_list_api {
 	    my $og = PMG::RuleDB::Group->new(
 		$param->{name}, $param->{info} // '', $oclass);
 
+	    for my $prop (qw(and invert)) {
+		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
+	    }
+
 	    return $rdb->save_group($og);
 	}});
 }
@@ -199,6 +225,11 @@ sub register_object_group_config_api {
 
 	}});
 
+    my $additional_parameters = {};
+    if ($oclass =~ /^(?:what|when|who)$/i) {
+	$additional_parameters = { $group_attributes->%* };
+    }
+
     $apiclass->register_method({
 	name => 'set_config',
 	path => $path,
@@ -226,6 +257,7 @@ sub register_object_group_config_api {
 		    maxLength => 255,
 		    optional => 1,
 		},
+		$additional_parameters->%*,
 	    },
 	},
 	returns => { type => "null" },
@@ -243,8 +275,9 @@ sub register_object_group_config_api {
 	    my $og = shift @$list ||
 		die "$oclass group '$ogroup' not found\n";
 
-	    $og->{name} = $param->{name} if defined($param->{name});
-	    $og->{info} = $param->{info} if defined($param->{info});
+	    for my $prop (qw(name info and invert)) {
+		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
+	    }
 
 	    $rdb->save_group($og);
 
diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
index 9e133bc..0d3d9c3 100644
--- a/src/PMG/DBTools.pm
+++ b/src/PMG/DBTools.pm
@@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
 
 __EOD
 
+my $object_group_attributes_cmd = <<__EOD;
+    CREATE TABLE Objectgroup_Attributes (
+      Objectgroup_ID INTEGER NOT NULL,
+      Name VARCHAR(20) NOT NULL,
+      Value BYTEA NULL,
+      PRIMARY KEY (Objectgroup_ID, Name)
+    );
+
+    CREATE INDEX Objectgroup_Attributes_Objectgroup_ID_Index ON Objectgroup_Attributes(Objectgroup_ID);
+
+__EOD
+
 sub cond_create_dbtable {
     my ($dbh, $name, $ctablecmd) = @_;
 
@@ -439,6 +451,8 @@ sub create_ruledb {
         $userprefs_ctablecmd;
 
         $virusinfo_stat_ctablecmd;
+
+        $object_group_attributes_cmd;
 EOD
     );
 
@@ -494,6 +508,7 @@ sub upgradedb {
 	'CStatistic', $cstatistic_ctablecmd,
 	'ClusterInfo', $clusterinfo_ctablecmd,
 	'VirusInfo', $virusinfo_stat_ctablecmd,
+	'Objectgroup_Attributes', $object_group_attributes_cmd,
     };
 
     foreach my $table (keys %$tables) {
diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
index a6b0b79..df9e526 100644
--- a/src/PMG/RuleDB.pm
+++ b/src/PMG/RuleDB.pm
@@ -160,6 +160,30 @@ sub load_groups_by_name {
     };
 }
 
+sub update_group_attributes {
+    my ($self, $og) = @_;
+
+    my $attributes = [qw(and invert)];
+
+    for my $attribute ($attributes->@*) {
+	# only save the values if they're set to 1
+	if ($og->{$attribute}) {
+	    $self->{dbh}->do(
+		"INSERT INTO Objectgroup_Attributes (Objectgroup_ID, Name, Value) " .
+		"VALUES (?, ?, ?) ".
+		"ON CONFLICT (Objectgroup_ID, Name) DO UPDATE SET Value = ?", undef,
+		$og->{id}, $attribute, $og->{$attribute}, $og->{$attribute},
+	    );
+	} else {
+	    $self->{dbh}->do(
+		"DELETE FROM Objectgroup_Attributes " .
+		"WHERE Objectgroup_ID = ? AND Name = ?", undef,
+		$og->{id}, $attribute,
+	    );
+	}
+    }
+}
+
 sub save_group {
     my ($self, $og) = @_;
 
@@ -171,27 +195,51 @@ sub save_group {
 	die "undefined group attribute - class: ERROR";
 
     if (defined($og->{id})) {
+	$self->{dbh}->begin_work;
+
+	eval {
+	    $self->{dbh}->do("UPDATE Objectgroup " .
+			     "SET Name = ?, Info = ? " .
+			     "WHERE ID = ?", undef,
+			     encode('UTF-8', $og->{name}),
+			     encode('UTF-8', $og->{info}),
+			     $og->{id});
 
-	$self->{dbh}->do("UPDATE Objectgroup " .
-			 "SET Name = ?, Info = ? " .
-			 "WHERE ID = ?", undef,
-			 encode('UTF-8', $og->{name}),
-			 encode('UTF-8', $og->{info}),
-			 $og->{id});
+	    $self->update_group_attributes($og);
 
-	return $og->{id};
+	    $self->{dbh}->commit;
+	};
 
+	if (my $err = $@) {
+	    $self->{dbh}->rollback;
+	    syslog('err', $err);
+	    return undef;
+	}
     } else {
-	my $sth = $self->{dbh}->prepare(
-	    "INSERT INTO Objectgroup (Name, Info, Class) " .
-	    "VALUES (?, ?, ?);");
+	$self->{dbh}->begin_work;
 
-	$sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
+	eval {
+	    my $sth = $self->{dbh}->prepare(
+		"INSERT INTO Objectgroup (Name, Info, Class) " .
+		"VALUES (?, ?, ?);");
 
-	return $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
+	    $sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
+
+	    $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
+
+	    $self->update_group_attributes($og);
+
+	    $self->{dbh}->commit;
+	};
+
+	if (my $err = $@) {
+	    $self->{dbh}->rollback;
+	    syslog('err', $err);
+	    return undef;
+	}
     }
 
-    return undef;
+    return $og->{id};
 }
 
 sub delete_group {
@@ -252,6 +300,18 @@ sub delete_group {
     return undef;
 }
 
+sub load_group_attributes {
+    my ($self, $og) = @_;
+
+    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
+    $attribute_sth->execute($og->{id});
+
+    while (my $ref = $attribute_sth->fetchrow_hashref()) {
+	$og->{and} = $ref->{value} if $ref->{name} eq 'and';
+	$og->{invert} = $ref->{value} if $ref->{name} eq 'invert';
+    }
+}
+
 sub load_objectgroups {
     my ($self, $class, $id) = @_;
 
@@ -259,34 +319,47 @@ sub load_objectgroups {
 
     defined($class) || die "undefined object class";
 
-    if (!(defined($id))) {
-        $sth = $self->{dbh}->prepare(
-	    "SELECT * FROM Objectgroup where Class = ? ORDER BY name");
-        $sth->execute($class);
-
-    } else {
-        $sth = $self->{dbh}->prepare(
-	    "SELECT * FROM Objectgroup where Class like ? and id = ? " .
-	    "order by name");
-        $sth->execute($class,$id);
-    }
+    $self->{dbh}->begin_work;
 
     my $arr_og = ();
-    while (my $ref = $sth->fetchrow_hashref()) {
-    	my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
-					 $ref->{class});
-    	$og->{id} = $ref->{id};
 
-	if ($class eq 'action') {
-	    my $objects = $self->load_group_objects($og->{id});
-	    my $obj = @$objects[0];
-	    defined($obj) || die "undefined action object: ERROR";
-	    $og->{action} = $obj;
+    eval {
+	if (!(defined($id))) {
+	    $sth = $self->{dbh}->prepare(
+		"SELECT * FROM Objectgroup where Class = ? ORDER BY name");
+	    $sth->execute($class);
+
+	} else {
+	    $sth = $self->{dbh}->prepare(
+		"SELECT * FROM Objectgroup where Class like ? and id = ? " .
+		"order by name");
+	    $sth->execute($class,$id);
 	}
-    	push @$arr_og, $og;
-    }
 
-    $sth->finish();
+	while (my $ref = $sth->fetchrow_hashref()) {
+	    my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
+					     $ref->{class});
+	    $og->{id} = $ref->{id};
+
+	    if ($class eq 'action') {
+		my $objects = $self->load_group_objects($og->{id});
+		my $obj = @$objects[0];
+		defined($obj) || die "undefined action object: ERROR";
+		$og->{action} = $obj;
+	    } else {
+		$self->load_group_attributes($og);
+	    }
+	    push @$arr_og, $og;
+	}
+
+	$sth->finish();
+    };
+
+    my $err = $@;
+
+    $self->{dbh}->rollback;
+
+    die $err if $err;
 
     return $arr_og;
 }
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 06/12] add rule attributes and/invert (for each relevant type)

[Dominik Csapak]

like with the objectgroups, add an attributes table for groups, and an
'and'/'invert' attribute for each relevant object type
(what/when/from/to).

This is intended to modify the behaviour for the matching regarding
object groups, so that one has more choice in the logical matching.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/API2/ObjectGroupHelpers.pm |   8 ++
 src/PMG/API2/Rules.pm              |  53 ++++++++-
 src/PMG/DBTools.pm                 |  15 +++
 src/PMG/RuleDB.pm                  | 169 +++++++++++++++++++++++------
 4 files changed, 209 insertions(+), 36 deletions(-)

diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
index a08a6a3..3060157 100644
--- a/src/PMG/API2/ObjectGroupHelpers.pm
+++ b/src/PMG/API2/ObjectGroupHelpers.pm
@@ -31,6 +31,14 @@ sub format_rule {
 	active => $rule->{active},
 	direction => $rule->{direction},
     };
+    my $types = [qw(what when from to)];
+    my $attributes = [qw(and invert)];
+    for my $type ($types->@*) {
+	for my $attribute ($attributes->@*) {
+	    my $opt = "${type}-${attribute}";
+	    $data->{$opt} = $rule->{$opt} if defined($rule->{$opt});
+	}
+    }
 
     $cond_create_group->($data, 'from', $from);
     $cond_create_group->($data, 'to', $to);
diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm
index c48370f..1ebadc2 100644
--- a/src/PMG/API2/Rules.pm
+++ b/src/PMG/API2/Rules.pm
@@ -149,6 +149,54 @@ my $rule_params = {
 	type => 'boolean',
 	optional => 1,
     },
+    'what-and' => {
+	description => "Flag to 'and' combine WHAT group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'what-invert' => {
+	description => "Flag to invert WHAT group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'when-and' => {
+	description => "Flag to 'and' combine WHEN group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'when-invert' => {
+	description => "Flag to invert WHEN group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'from-and' => {
+	description => "Flag to 'and' combine FROM group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'from-invert' => {
+	description => "Flag to invert FROM group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'to-and' => {
+	description => "Flag to 'and' combine TO group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
+    'to-invert' => {
+	description => "Flag to invert TO group matches.",
+	type => 'boolean',
+	default => 0,
+	optional => 1,
+    },
 };
 
 sub get_rule_params {
@@ -203,7 +251,10 @@ __PACKAGE__->register_method ({
 
 	my $rule = $rdb->load_rule($id);
 
-	for my $key (qw(name active direction priority)) {
+	my $keys = ["name", "priority"];
+	push $keys->@*, keys get_rule_params()->%*;
+
+	for my $key ($keys->@*) {
 	    $rule->{$key} = $param->{$key} if defined($param->{$key});
 	}
 
diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
index 0d3d9c3..605eb71 100644
--- a/src/PMG/DBTools.pm
+++ b/src/PMG/DBTools.pm
@@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
 
 __EOD
 
+my $rule_attributes_cmd = <<__EOD;
+    CREATE TABLE Rule_Attributes (
+      Rule_ID INTEGER NOT NULL,
+      Name VARCHAR(20) NOT NULL,
+      Value BYTEA NULL,
+      PRIMARY KEY (Rule_ID, Name)
+    );
+
+    CREATE INDEX Rule_Attributes_Rule_ID_Index ON Rule_Attributes(Rule_ID);
+
+__EOD
+
 my $object_group_attributes_cmd = <<__EOD;
     CREATE TABLE Objectgroup_Attributes (
       Objectgroup_ID INTEGER NOT NULL,
@@ -452,6 +464,8 @@ sub create_ruledb {
 
         $virusinfo_stat_ctablecmd;
 
+        $rule_attributes_cmd;
+
         $object_group_attributes_cmd;
 EOD
     );
@@ -508,6 +522,7 @@ sub upgradedb {
 	'CStatistic', $cstatistic_ctablecmd,
 	'ClusterInfo', $clusterinfo_ctablecmd,
 	'VirusInfo', $virusinfo_stat_ctablecmd,
+	'Rule_Attributes', $rule_attributes_cmd,
 	'Objectgroup_Attributes', $object_group_attributes_cmd,
     };
 
diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
index df9e526..70770a8 100644
--- a/src/PMG/RuleDB.pm
+++ b/src/PMG/RuleDB.pm
@@ -665,6 +665,35 @@ sub delete_object {
     return 1;
 }
 
+sub update_rule_attributes {
+    my ($self, $rule) = @_;
+
+    my $types = [qw(what when from to)];
+    my $attributes = [qw(and invert)];
+
+    for my $type ($types->@*) {
+	for my $attribute ($attributes->@*) {
+	    my $prop = "$type-$attribute";
+
+	    # only save the values if they're set to 1
+	    if ($rule->{$prop}) {
+		$self->{dbh}->do(
+		    "INSERT INTO Rule_Attributes (Rule_ID, Name, Value) " .
+		    "VALUES (?, ?, ?) ".
+		    "ON CONFLICT (Rule_ID, Name) DO UPDATE SET Value = ?", undef,
+		    $rule->{id}, $prop, $rule->{$prop}, $rule->{$prop},
+		);
+	    } else {
+		$self->{dbh}->do(
+		    "DELETE FROM Rule_Attributes " .
+		    "WHERE Rule_ID = ? AND Name = ?", undef,
+		    $rule->{id}, $prop,
+		);
+	    }
+	}
+    }
+}
+
 sub save_rule {
     my ($self, $rule) = @_;
 
@@ -679,28 +708,53 @@ sub save_rule {
 
     my $rulename = encode('UTF-8', $rule->{name});
     if (defined($rule->{id})) {
+	$self->{dbh}->begin_work;
 
-	$self->{dbh}->do(
-	    "UPDATE Rule " .
-	    "SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
-	    "WHERE ID = ?", undef,
-	    $rulename, $rule->{priority}, $rule->{active},
-	    $rule->{direction}, $rule->{id});
+	eval {
+	    $self->{dbh}->do(
+		"UPDATE Rule " .
+		"SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
+		"WHERE ID = ?", undef,
+		$rulename, $rule->{priority}, $rule->{active},
+		$rule->{direction}, $rule->{id});
+
+	    $self->update_rule_attributes($rule);
 
-	return $rule->{id};
+	    $self->{dbh}->commit;
+	};
 
+	if (my $err = $@) {
+	    $self->{dbh}->rollback;
+	    syslog('err', $err);
+	    return undef;
+	}
     } else {
-	my $sth = $self->{dbh}->prepare(
-	    "INSERT INTO Rule (Name, Priority, Active, Direction) " .
-	    "VALUES (?, ?, ?, ?);");
+	$self->{dbh}->begin_work;
 
-	$sth->execute($rulename, $rule->priority, $rule->active,
-		      $rule->direction);
+	eval {
+	    my $sth = $self->{dbh}->prepare(
+		"INSERT INTO Rule (Name, Priority, Active, Direction) " .
+		"VALUES (?, ?, ?, ?);");
+
+	    $sth->execute($rulename, $rule->priority, $rule->active,
+		$rule->direction);
+
+
+	    $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
+
+	    $self->update_rule_attributes($rule);
 
-	return $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
+	    $self->{dbh}->commit;
+	};
+
+	if (my $err = $@) {
+	    $self->{dbh}->rollback;
+	    syslog('err', $err);
+	    return undef;
+	}
     }
 
-    return undef;
+    return $rule->{id};
 }
 
 sub delete_rule {
@@ -826,24 +880,58 @@ sub rule_remove_group {
     return 1;
 }
 
+sub load_rule_attributes {
+    my ($self, $rule) = @_;
+
+    my $types = [qw(what when from to)];
+    my $attributes = [qw(and invert)];
+
+    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?");
+    $attribute_sth->execute($rule->{id});
+
+    ATTRIBUTES_LOOP:
+    while (my $ref = $attribute_sth->fetchrow_hashref()) {
+	for my $type ($types->@*) {
+	    for my $attribute ($attributes->@*) {
+		my $prop = "$type-$attribute";
+		if ($ref->{name} eq $prop) {
+		    $rule->{$prop} = $ref->{value};
+		    next ATTRIBUTES_LOOP;
+		}
+	    }
+	}
+    }
+}
+
 sub load_rule {
     my ($self, $id) = @_;
 
     defined($id) || die "undefined id: ERROR";
 
-    my $sth = $self->{dbh}->prepare(
-	"SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
+    $self->{dbh}->begin_work;
 
-    my $rules = ();
+    my $rule;
 
-    $sth->execute($id);
+    eval {
+	my $sth = $self->{dbh}->prepare(
+	    "SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
 
-    my $ref = $sth->fetchrow_hashref();
-    die "rule '$id' does not exist\n" if !defined($ref);
+	$sth->execute($id);
+
+	my $ref = $sth->fetchrow_hashref();
+	die "rule '$id' does not exist\n" if !defined($ref);
 
-    my $rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
-				      $ref->{active}, $ref->{direction});
-    $rule->{id} = $ref->{id};
+	$rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
+					  $ref->{active}, $ref->{direction});
+	$rule->{id} = $ref->{id};
+
+	$self->load_rule_attributes($rule);
+    };
+    my $err = $@;
+
+    $self->{dbh}->rollback;
+
+    die $err if $err;
 
     return $rule;
 }
@@ -851,22 +939,33 @@ sub load_rule {
 sub load_rules {
     my ($self) = @_;
 
-    my $sth = $self->{dbh}->prepare(
-	"SELECT * FROM Rule ORDER BY Priority DESC");
-
     my $rules = ();
 
-    $sth->execute();
+    $self->{dbh}->begin_work;
 
-    while (my $ref = $sth->fetchrow_hashref()) {
-	my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
-	my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
-					  $ref->{active}, $ref->{direction});
-	$rule->{id} = $ref->{id};
-	push @$rules, $rule;
-    }
+    eval {
+	my $sth = $self->{dbh}->prepare(
+	    "SELECT * FROM Rule ORDER BY Priority DESC");
 
-    $sth->finish();
+	$sth->execute();
+
+	while (my $ref = $sth->fetchrow_hashref()) {
+	    my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
+	    my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
+					      $ref->{active}, $ref->{direction});
+	    $rule->{id} = $ref->{id};
+	    #$self->load_rule_attributes($rule);
+
+	    push @$rules, $rule;
+	}
+
+	$sth->finish();
+    };
+    my $err = $@;
+
+    $self->{dbh}->rollback;
+
+    die $err if $err;
 
     return $rules;
 }
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 07/12] RuleCache: load rule/objectgroup attributes from database

[Dominik Csapak]

so that we can use the 'and' and 'invert' flags set.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index 4f7ebe7..0b62aeb 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -67,6 +67,23 @@ sub new {
 	    $self->{"$ruleid:what"} = { groups => [] };
 	    $self->{"$ruleid:action"} = { groups => [] };
 
+	    my $attribute_sth = $dbh->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?");
+	    $attribute_sth->execute($ruleid);
+
+	    while (my $ref = $attribute_sth->fetchrow_hashref()) {
+		$self->{"$ruleid:from"}->{and} = $ref->{value} if $ref->{name} eq 'from-and';
+		$self->{"$ruleid:from"}->{invert} = $ref->{value} if $ref->{name} eq 'from-invert';
+
+		$self->{"$ruleid:to"}->{and} = $ref->{value} if $ref->{name} eq 'to-and';
+		$self->{"$ruleid:to"}->{invert} = $ref->{value} if $ref->{name} eq 'to-invert';
+
+		$self->{"$ruleid:when"}->{and} = $ref->{value} if $ref->{name} eq 'when-and';
+		$self->{"$ruleid:when"}->{invert} = $ref->{value} if $ref->{name} eq 'when-invert';
+
+		$self->{"$ruleid:what"}->{and} = $ref->{value} if $ref->{name} eq 'what-and';
+		$self->{"$ruleid:what"}->{invert} = $ref->{value} if $ref->{name} eq 'what-invert';
+	    }
+
 	    my $sth1 = $dbh->prepare(
 		"SELECT Objectgroup_ID, Grouptype FROM RuleGroup " .
 		"where RuleGroup.Rule_ID = '$ruleid' " .
@@ -114,6 +131,14 @@ sub new {
 		    objects => $objects,
 		};
 
+		my $objectgroup_sth = $dbh->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
+		$objectgroup_sth->execute($groupid);
+
+		while (my $ref = $objectgroup_sth->fetchrow_hashref()) {
+		    $group->{and} = $ref->{value} if $ref->{name} eq 'and';
+		    $group->{invert} = $ref->{value} if $ref->{name} eq 'invert';
+		}
+
 		my $type = $type_map->{$gtype};
 		push $self->{"$ruleid:$type"}->{groups}->@*, $group;
 	    }
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 08/12] RuleCache: implement and/invert for when/from/to

[Dominik Csapak]

by introducing 'match_list_with_mode' that gets called for each group
and then on the result of each group match result.

This does not work for 'what' matches since they are not a simple
yes/no match (they include the parts) so this will be done seperately.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm | 65 +++++++++++++++++++++++++++++---------------
 1 file changed, 43 insertions(+), 22 deletions(-)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index 0b62aeb..7d08107 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -273,13 +273,14 @@ sub from_match {
 	$ip = $1;
     }
 
-    for my $group ($from->{groups}->@*) {
-	for my $obj ($group->{objects}->@*) {
-	    return 1 if $obj->who_match($addr, $ip, $ldap);
-	}
-    }
-
-    return 0;
+    return match_list_with_mode($from->{groups}, $from->{and}, $from->{invert}, sub {
+	my ($group) = @_;
+	my $list = $group->{objects};
+	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
+	    my ($obj) = @_;
+	    return $obj->who_match($addr, $ip, $ldap);
+	});
+    });
 }
 
 sub to_match {
@@ -289,14 +290,14 @@ sub to_match {
 
     return 1 if scalar($to->{groups}->@*) == 0;
 
-    for my $group ($to->{groups}->@*) {
-	for my $obj ($group->{objects}->@*) {
-	    return 1 if $obj->who_match($addr, undef, $ldap);
-	}
-    }
-
-
-    return 0;
+    return match_list_with_mode($to->{groups}, $to->{and}, $to->{invert}, sub {
+	my ($group) = @_;
+	my $list = $group->{objects};
+	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
+	    my ($obj) = @_;
+	    return $obj->who_match($addr, undef, $ldap);
+	});
+    });
 }
 
 sub when_match {
@@ -306,13 +307,14 @@ sub when_match {
 
     return 1 if scalar($when->{groups}->@*) == 0;
 
-    for my $group ($when->{groups}->@*) {
-	for my $obj ($group->{objects}->@*) {
-	    return 1 if $obj->when_match($time);
-	}
-    }
-
-    return 0;
+    return match_list_with_mode($when->{groups}, $when->{and}, $when->{invert}, sub {
+	my ($group) = @_;
+	my $list = $group->{objects};
+	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
+	    my ($obj) = @_;
+	    return $obj->when_match($time);
+	});
+    });
 }
 
 sub what_match {
@@ -357,4 +359,23 @@ sub what_match {
     return $res;
 }
 
+# calls sub with each element of $list, and and/ors/inverts the result
+sub match_list_with_mode($$$$) {
+    my ($list, $and, $invert, $sub) = @_;
+
+    $and //= 0;
+    $invert //= 0;
+
+    for my $el ($list->@*) {
+	my $res = $sub->($el);
+	if (!$and) {
+	    return !$invert if $res;
+	} else {
+	    return $invert if !$res;
+	}
+    }
+
+    return $and != $invert;
+}
+
 1;
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 09/12] MailQueue: return maximum AID

[Dominik Csapak]

we'll need this in the what_matches to invert mark lists.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/MailQueue.pm    | 4 ++--
 src/PMG/Utils.pm        | 2 ++
 src/bin/pmg-smtp-filter | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/PMG/MailQueue.pm b/src/PMG/MailQueue.pm
index 8355c30..4e37cb9 100644
--- a/src/PMG/MailQueue.pm
+++ b/src/PMG/MailQueue.pm
@@ -406,10 +406,10 @@ sub parse_mail {
     # we also remove all proxmox-marks from the mail and add an unique
     # id to each attachment.
 
-    PMG::Utils::remove_marks ($entity, 1);
+    my $max_aid = PMG::Utils::remove_marks ($entity, 1);
     PMG::Utils::add_ct_marks ($entity);
 
-    return $entity;
+    return ($entity, $max_aid);
 }
 
 sub decode_entities {
diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index d9c9d2d..9f69e7f 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -186,6 +186,8 @@ sub remove_marks {
 
 	$id++;
     });
+
+    return $id - 1; # return max AID
 }
 
 sub subst_values {
diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
index 71043b0..86d633d 100755
--- a/src/bin/pmg-smtp-filter
+++ b/src/bin/pmg-smtp-filter
@@ -648,7 +648,8 @@ sub handle_smtp {
 
 	my $maxfiles = $pmg_cfg->get('clamav', 'archivemaxfiles');
 
-	my $entity = $queue->parse_mail($maxfiles);
+	my ($entity, $max_aid) = $queue->parse_mail($maxfiles);
+	$msginfo->{max_aid} = $max_aid;
 
 	$self->log (3, "$queue->{logid}: new mail message-id=%s", $queue->{msgid});
 
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 10/12] WIP: ModGroup: add possibility to explode to all targets

[Dominik Csapak]

we will need that for the remove action + and/invert

note that this is only a wip commit, there will be a more efficient
implementation later!

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/ModGroup.pm | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/src/PMG/ModGroup.pm b/src/PMG/ModGroup.pm
index b0fa9a6..d4e1cd5 100644
--- a/src/PMG/ModGroup.pm
+++ b/src/PMG/ModGroup.pm
@@ -81,6 +81,23 @@ sub subgroups {
     return $res;
 }
 
+# explode the groups, so we have one for each target we need
+# only to be used by the rRemove action when there was a spaminfo
+sub explode {
+    my ($self, $targets) = @_;
+
+    my $groups = $self->{groups};
+    my $ea = $self->{ea};
+    my $res;
+
+    # TODO: implment it more direclty with less overhead!
+    for my $target ($targets->@*) {
+	$self->subgroups([$target]);
+    }
+
+    return $self->subgroups($targets);
+}
+
 1;
 
 __END__
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 11/12] RuleCache: implement and/invert for what matches

[Dominik Csapak]

Since what matches are not a simple boolean match, but also can contain
"marks" to mark specific parts of the mail, we must implement some
custom logic for and/invert here.

The goal here is to define that groups are on a per part level,
but the rule operates on the whole mail.

To achieve this we have two different and/invert combine functions, one
for the group level and one for the whole what match.

For per group and/inversion we and 'and-combine' and invert the list of
marks, so if it matches part 1,2 of 1,2,3 the inversion would return 3.

For the rule it only matters if the and/inversion part matches at all,
regardless of the marks. If it matches, the marks will be or'ed.

With this, one can represent many different scenarios that were not
possible before.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/RuleCache.pm     | 165 +++++++++++++++++++++++++++++++++++++--
 src/PMG/RuleDB/Remove.pm |  13 ++-
 2 files changed, 168 insertions(+), 10 deletions(-)

diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
index 7d08107..7affa81 100644
--- a/src/PMG/RuleCache.pm
+++ b/src/PMG/RuleCache.pm
@@ -336,29 +336,147 @@ sub what_match {
 	return $res;
     }
 
+    my $what_matches = {};
+
     for my $group ($what->{groups}->@*) {
+	my $group_matches = {};
+	my $and = $group->{and};
+	my $invert = $group->{invert};
 	for my $obj ($group->{objects}->@*) {
 	    if (!$obj->can('what_match_targets')) {
-		if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
-		    for my $target ($msginfo->{targets}->@*) {
-			push $res->{targets}->{$target}->{marks}->@*, $match->@*;
+		my $match = $obj->what_match($queue, $element, $msginfo, $dbh);
+		for my $target ($msginfo->{targets}->@*) {
+		    if (defined($match)) {
+			push $group_matches->{$target}->@*, $match;
+		    } else {
+			push $group_matches->{$target}->@*, undef;
 		    }
 		}
 	    } else {
-		if (my $target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
-		    foreach my $k (keys $target_info->%*) {
-			push $res->{targets}->{$k}->{marks}->@*, $target_info->{$k}->{marks}->@*;
+		my $target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh);
+		for my $target ($msginfo->{targets}->@*) {
+		    my $match = $target_info->{$target};
+		    if (defined($match)) {
+			push $group_matches->{$target}->@*, $match->{marks};
 			# only save spaminfo once
-			$res->{spaminfo} = $target_info->{$k}->{spaminfo} if !defined($res->{spaminfo});
+			$res->{spaminfo} = $match->{spaminfo} if !defined($res->{spaminfo});
+		    } else {
+			push $group_matches->{$target}->@*, undef;
 		    }
 		}
 	    }
 	}
+
+	for my $target (keys $group_matches->%*) {
+	    my $matches = group_match_and_invert($group_matches->{$target}, $and, $invert, $msginfo);
+	    push $what_matches->{$target}->@*, $matches;
+	}
+    }
+
+    for my $target (keys $what_matches->%*) {
+	my $target_marks = what_match_and_invert($what_matches->{$target}, $what->{and}, $what->{invert});
+	next if !defined($target_marks);
+	$res->{targets}->{$target}->{marks} = $target_marks;
     }
 
     return $res;
 }
 
+# combines matches of groups
+# this is only binary, and if it matches, 'or' combines the marks
+# so that all found marks are included
+#
+# this way we can create rules like:
+#
+# ---
+# What is and combined:
+# group1: match filename .*\.pdf
+# group2: spamlevel >= 3
+# ACTION: remove attachments
+# ---
+# which would remove attachments for all *.pdf filenames where
+# the spamlevel is >= 3
+sub what_match_and_invert($$$) {
+    my ($matches, $and, $invert) = @_;
+
+    my $match_result = match_list_with_mode($matches, $and, $invert, sub {
+	my ($match) = @_;
+	return defined($match);
+    });
+
+    if ($match_result) {
+	my $res = [];
+	for my $match ($matches->@*) {
+	    push $res->@*, $match->@* if defined($match);
+	}
+	return $res;
+    } else {
+	return undef;
+    }
+}
+
+# combines group matches according to and/invert
+# since we want match groups per mime part, we must
+# look at the marks and possibly invert them
+sub group_match_and_invert($$$$) {
+    my ($group_matches, $and, $invert, $msginfo) = @_;
+
+    my $encountered_parts = 0;
+    if ($and) {
+	my $set = {};
+	my $count = scalar($group_matches->@*);
+	for my $match ($group_matches->@*) {
+	    if (!defined($match)) {
+		$set = {};
+		last;
+	    }
+
+	    if (scalar($match->@*) > 0) {
+		$encountered_parts = 1;
+		$set->{$_}++ for $match->@*;
+	    } else {
+		$set->{$_}++ for (1..$msginfo->{max_aid});
+	    }
+	}
+
+	$group_matches = undef;
+	for my $key (keys $set->%*) {
+	    if ($set->{$key} == $count) {
+		push $group_matches->@*, $key;
+	    }
+	}
+	if (defined($group_matches) && scalar($group_matches->@*) == $count && !$encountered_parts) {
+	    $group_matches = [];
+	}
+    } else {
+	my $set = {};
+	for my $match ($group_matches->@*) {
+	    next if !defined($match);
+	    if (scalar($match->@*) == 0) {
+		$set->{$_} = 1 for (1..$msginfo->{max_aid});
+	    } else {
+		$encountered_parts = 1;
+		$set->{$_} = 1 for $match->@*;
+	    }
+	}
+
+	my $count = scalar(keys $set->%*);
+	if ($count == $msginfo->{max_aid} && !$encountered_parts) {
+	    $group_matches = [];
+	} elsif ($count == 0) {
+	    $group_matches = undef;
+	} else {
+	    $group_matches = [keys $set->%*];
+	}
+    }
+
+    if ($invert) {
+	$group_matches = invert_mark_list($group_matches, $msginfo->{max_aid});
+    }
+
+    return $group_matches;
+}
+
 # calls sub with each element of $list, and and/ors/inverts the result
 sub match_list_with_mode($$$$) {
     my ($list, $and, $invert, $sub) = @_;
@@ -378,4 +496,37 @@ sub match_list_with_mode($$$$) {
     return $and != $invert;
 }
 
+# inverts a list of marks with the remaining ones of the mail
+# examples:
+# mail has [1,2,3,4,5]
+#
+# undef => [1,2,3,4,5]
+# [1,2] => [3,4,5]
+# [1,2,3,4,5] => undef
+# [] => undef // [] means the whole mail matched
+sub invert_mark_list($$) {
+    my ($list, $max_aid) = @_;
+
+    if (defined($list)) {
+	my $length = scalar($list->@*);
+	if ($length == 0 || $length == ($max_aid - 1)) {
+	    return undef;
+	}
+    }
+
+    $list //= [];
+
+    my $set = {};
+    $set->{$_} = 1 for $list->@*;
+
+    my $new_list = [];
+    for (my $i = 1; $i <= $max_aid; $i++) {
+	if (!$set->{$i}) {
+	    push $new_list->@*, $i;
+	}
+    }
+
+    return $new_list;
+}
+
 1;
diff --git a/src/PMG/RuleDB/Remove.pm b/src/PMG/RuleDB/Remove.pm
index 5812602..c9fd157 100644
--- a/src/PMG/RuleDB/Remove.pm
+++ b/src/PMG/RuleDB/Remove.pm
@@ -209,7 +209,14 @@ sub execute {
 	return if !$found_mark;
     }
 
-    my $subgroups = $mod_group->subgroups ($targets);
+    my $subgroups;
+    if ($marks->{spaminfo}) {
+	# when there was a spam check in the rule, we might have different marks for
+	# different targets, so simply copy the mail for each target that matches
+	$subgroups = $mod_group->explode($targets);
+    } else {
+	$subgroups = $mod_group->subgroups ($targets);
+    }
 
     my $html = PMG::Utils::subst_values($self->{text}, $vars);
 
@@ -263,8 +270,8 @@ sub execute {
 
 	$self->{message_seen} = 0;
 
-	# since all matches are or combinded, marks for all targets must be the same if they exist
-	# so simply use the first one here
+	# if there was spam check in this rule, the marks must always be the same,
+	# otherwise we get a subgroup for each target anyway
 	my $match_marks = $marks->{targets}->{$tg->[0]}->{marks};
 
 	$self->delete_marked_parts($queue, $entity, $html, $rtype, $match_marks, $rulename);
-- 
2.30.2

[pmg-devel] [PATCH pmg-api 12/12] pmgdb: extend dump output to include add/invert

[Dominik Csapak]

if a group type has and/invert set, add a line with that information
and for each object group add its mode too in parenthesis

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PMG/CLI/pmgdb.pm | 38 ++++++++++++++++++++++++++++++++------
 1 file changed, 32 insertions(+), 6 deletions(-)

diff --git a/src/PMG/CLI/pmgdb.pm b/src/PMG/CLI/pmgdb.pm
index 8368af8..cd94c23 100644
--- a/src/PMG/CLI/pmgdb.pm
+++ b/src/PMG/CLI/pmgdb.pm
@@ -40,6 +40,8 @@ sub print_objects {
 sub print_rule {
     my ($ruledb, $rule) = @_;
 
+    $ruledb->load_rule_attributes($rule);
+
     my $direction = {
 	0 => 'in',
 	1 => 'out',
@@ -52,26 +54,50 @@ sub print_rule {
     print "Found RULE $rule->{id} (prio: $rule->{priority}, $dir, $active): $rulename\n";
 
     my $print_group = sub {
-	my ($type, $og) = @_;
+	my ($type, $og, $print_mode) = @_;
 	my $oname = encode('UTF-8', $og->{name});
-	print "  FOUND $type GROUP $og->{id}: $oname\n";
+	my $mode = "";
+	if ($print_mode) {
+	    my $and = $og->{and} // 0;
+	    my $invert = $og->{invert} // 0;
+	    $mode = " (and=$and, invert=$invert)";
+	}
+	print "  FOUND $type GROUP $og->{id}${mode}: $oname\n";
 	print_objects($ruledb, $og);
     };
 
+    my $print_type_mode = sub {
+	my ($type) = @_;
+	my $and = $rule->{"$type-and"};
+	my $invert = $rule->{"$type-invert"};
+	if (defined($and) || defined($invert)) {
+	    my $print_type = uc($type);
+	    print "  $print_type mode: and=" . ($and // 0) . " invert=". ($invert // 0) . "\n";
+	}
+    };
+
     my ($from, $to, $when, $what, $action) =
 	$ruledb->load_groups($rule);
 
+    $print_type_mode->("from") if scalar(@$from);
     foreach my $og (@$from) {
-	$print_group->("FROM", $og);
+	$ruledb->load_group_attributes($og);
+	$print_group->("FROM", $og, 1);
     }
+    $print_type_mode->("to") if scalar(@$to);
     foreach my $og (@$to) {
-	$print_group->("TO", $og);
+	$ruledb->load_group_attributes($og);
+	$print_group->("TO", $og, 1);
     }
+    $print_type_mode->("when") if scalar(@$when);
     foreach my $og (@$when) {
-	$print_group->("WHEN", $og);
+	$ruledb->load_group_attributes($og);
+	$print_group->("WHEN", $og, 1);
     }
+    $print_type_mode->("what") if scalar(@$what);
     foreach my $og (@$what) {
-	$print_group->("WHAT", $og);
+	$ruledb->load_group_attributes($og);
+	$print_group->("WHAT", $og, 1);
     }
     foreach my $og (@$action) {
 	$print_group->("ACTION", $og);
-- 
2.30.2

[pmg-devel] [PATCH pmg-docs 1/2] rule system: add a small section about matching rules

[Dominik Csapak]

this section explains how multiple object categories and multiple
objects interact with the rule matching.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pmg-mail-filter.adoc | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/pmg-mail-filter.adoc b/pmg-mail-filter.adoc
index 3aafe4c..57b8a15 100644
--- a/pmg-mail-filter.adoc
+++ b/pmg-mail-filter.adoc
@@ -58,6 +58,23 @@ You can also disable a rule completely, which is mostly useful for
 testing and debugging. The 'Factory Defaults' button allows you to
 reset the filter rules.
 
+Application of Rules
+--------------------
+
+When there is more than one object category or multiple objects configured
+within a single rule, the following logic is used to determine if the rule
+should be applied:
+
+* Within one category (WHAT/FROM/TO/WHEN), all objects are logical-or linked,
+  meaning that only one object of any one object group from the same category
+  has to match for the whole category to match.
+
+* FROM/TO/WHAT/WHEN category match results are logical-and linked, so all
+  categories that have at least one object in them must match for the rule to
+  match.
+
+When these conditions are met, all configured actions are executed.
+
 
 [[pmg_mailfilter_action]]
 'Action' - objects
-- 
2.30.2

[pmg-devel] [PATCH pmg-docs 2/2] rule system: explain new and mode and invert flag

[Dominik Csapak]

and adding that the previous behaviour is now the default.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pmg-mail-filter.adoc | 39 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/pmg-mail-filter.adoc b/pmg-mail-filter.adoc
index 57b8a15..48bf6c0 100644
--- a/pmg-mail-filter.adoc
+++ b/pmg-mail-filter.adoc
@@ -63,7 +63,7 @@ Application of Rules
 
 When there is more than one object category or multiple objects configured
 within a single rule, the following logic is used to determine if the rule
-should be applied:
+should be applied by default:
 
 * Within one category (WHAT/FROM/TO/WHEN), all objects are logical-or linked,
   meaning that only one object of any one object group from the same category
@@ -75,6 +75,43 @@ should be applied:
 
 When these conditions are met, all configured actions are executed.
 
+Alternatively, one can configure the 'mode' to 'any' (the default) or 'all' and
+set 'invert' (default off) per object group and per object category for each
+rule.
+
+When the mode is 'all' for a group, all objects within must match for the
+object group to count as a match. This can be helpful when one wants to match
+multiple conditions at the same time (e.g. file content-type and filename).
+
+When 'all' is set for a category of a rule, all object groups for that
+type must match for the type to match.
+
+When 'invert' is active on a group, the original result of the group will
+simply be inverted, so a match becomes a non-match and vice versa.
+
+The same is true for the object group types for rules.
+
+Special handling is done for WHAT matches that mark file parts (e.g. filename)
+since that is not a simple logical match, but could be a match for each
+part of the e-mail (e.g. attachments, or parts of a multi-part e-mail).
+
+So for WHAT match object groups, the 'mode' and 'invert' is applied to
+the single parts of the e-mail, not the message as a whole.
+
+This means one has to be very careful with the 'invert' option, as previously
+not matching parts, will match when using 'invert' (e.g. an inverted filename
+matching will also mark non attachment parts of the mail).
+
+On the rule level, these marks of the parts will always be logical-or linked,
+this way even more scenarios can be represented.
+
+To make it a bit easier to understand, the options are combined to a single
+selection in the web ui:
+
+* Any must match => mode: 'any', invert: 'off'
+* All must match => mode: 'all', invert: 'off'
+* At least one must not match => mode: 'all', invert: 'on'
+* None must match => mode: 'any', invert: 'on'
 
 [[pmg_mailfilter_action]]
 'Action' - objects
-- 
2.30.2

[pmg-devel] [PATCH pmg-gui 1/2] rules: use tree panel instead of grouping feature of the grid

[Dominik Csapak]

just in preparation for adding a columnf or the groups
will look similar (though not identical) to before, but this makes
the groups now real entries in the grid, which means we can have
content in additional columns

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 css/ext6-pmg.css |  7 ++++++
 js/RuleInfo.js   | 59 +++++++++++++++++++++++++++++++-----------------
 2 files changed, 45 insertions(+), 21 deletions(-)

diff --git a/css/ext6-pmg.css b/css/ext6-pmg.css
index 2f999f4..1c43b09 100644
--- a/css/ext6-pmg.css
+++ b/css/ext6-pmg.css
@@ -263,3 +263,10 @@ a.download {
     color: inherit;
     text-decoration: none;
 }
+
+.pmx-rule-tree .x-tree-icon,
+.pmx-rule-tree .x-tree-elbow,
+.pmx-rule-tree .x-tree-elbow-end {
+    display: none;
+    width: 0px;
+}
diff --git a/js/RuleInfo.js b/js/RuleInfo.js
index 8f39695..c29c0ca 100644
--- a/js/RuleInfo.js
+++ b/js/RuleInfo.js
@@ -88,7 +88,11 @@ Ext.define('PMG.RuleInfo', {
 	    } else {
 		viewmodel.set('selectedRule', ruledata);
 
-		var data = [];
+		let data = {
+		    leaf: false,
+		    expanded: true,
+		    children: [],
+		};
 		Ext.Array.each(['from', 'to', 'when', 'what', 'action'], function(oc) {
 		    var store = viewmodel.get(oc + 'objects');
 		    if (ruledata[oc] === undefined || store === undefined) { return; }
@@ -111,12 +115,25 @@ Ext.define('PMG.RuleInfo', {
 			},
 		    });
 		    store.load();
+
+		    let group = {
+			name: oc,
+			oclass: oc,
+			type: true,
+			leaf: false,
+			expanded: true,
+			expandable: false,
+			children: [],
+		    };
 		    Ext.Array.each(ruledata[oc], function(og) {
-			data.push({ oclass: oc, name: og.name, typeid: og.id });
+			group.children.push({ oclass: oc, name: og.name, typeid: og.id, leaf: true });
 		    });
-		});
 
-		viewmodel.get('objects').setData(data);
+		    if (group.children.length) {
+			data.children.push(group);
+		    }
+		});
+		viewmodel.get('objects').setRoot(data);
 	    }
 	},
 
@@ -146,7 +163,7 @@ Ext.define('PMG.RuleInfo', {
 	},
 
 	control: {
-	    'grid[reference=usedobjects]': {
+	    'treepanel[reference=usedobjects]': {
 		drop: 'addDrop',
 	    },
 	    'tabpanel[reference=availobjects] > grid': {
@@ -162,6 +179,7 @@ Ext.define('PMG.RuleInfo', {
 
 	stores: {
 	    objects: {
+		type: 'tree',
 		fields: ['oclass', 'name', 'typeid'],
 		groupField: 'oclass',
 		sorters: 'name',
@@ -252,21 +270,16 @@ Ext.define('PMG.RuleInfo', {
 	    ],
 	},
 	{
-	    xtype: 'grid',
+	    xtype: 'treepanel',
 	    reference: 'usedobjects',
 	    hidden: true,
 	    emptyText: gettext('No Objects'),
-	    features: [{
-		id: 'group',
-		ftype: 'grouping',
-		enableGroupingMenu: false,
-		collapsible: false,
-		groupHeaderTpl: [
-		    '{[PMG.Utils.format_oclass(values.name)]}',
-		],
-	    }],
 
 	    title: gettext('Used Objects'),
+	    rootVisible: false,
+	    useArrows: true,
+	    rowLines: true,
+	    userCls: 'pmx-rule-tree',
 
 	    viewConfig: {
 		plugins: {
@@ -285,14 +298,17 @@ Ext.define('PMG.RuleInfo', {
 	    },
 
 	    columns: [
-		{
-		    header: gettext('Type'),
-		    dataIndex: 'oclass',
-		    hidden: true,
-		},
 		{
 		    header: gettext('Name'),
 		    dataIndex: 'name',
+		    xtype: 'treecolumn',
+		    renderer: PMG.Utils.format_oclass,
+		    sorter: function(a, b) {
+			if (a.data.type && b.data.type) {
+			    return a.data.oclass.localeCompare(b.data.oclass);
+			}
+			return a.data.text.localeCompare(b.data.text);
+		    },
 		    flex: 1,
 		},
 		{
@@ -302,8 +318,9 @@ Ext.define('PMG.RuleInfo', {
 		    width: 40,
 		    items: [
 			{
-			    iconCls: 'fa fa-fw fa-minus-circle',
 			    tooltip: gettext('Remove'),
+			    isActionDisabled: (v, rI, cI, i, rec) => rec.data.type,
+			    getClass: (v, mD, { data }) => data.type ? 'pmx-hidden' : 'fa fa-fw fa-minus-circle',
 			    handler: 'removeIconClick',
 			},
 		    ],
-- 
2.30.2

[pmg-devel] [PATCH pmg-gui 2/2] rules/objects: add mode selector dropdown

[Dominik Csapak]

for objects and object types in rules. We add a simple dropdown for the
'and' and 'invert' flags, to be somewhat consistent with the
notification matchers from pve and to make the wording more clear than
simple and/invert.

For What matches add a special warning hint, since that behaves a bit
special because of the mail parts.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 js/Makefile                    |  1 +
 js/ObjectGroup.js              | 64 ++++++++++++++++++++++++++++++++--
 js/ObjectGroupConfiguration.js |  4 +++
 js/RuleInfo.js                 | 44 +++++++++++++++++++++++
 js/form/ModeSelector.js        | 11 ++++++
 5 files changed, 122 insertions(+), 2 deletions(-)
 create mode 100644 js/form/ModeSelector.js

diff --git a/js/Makefile b/js/Makefile
index 78f2b57..4267092 100644
--- a/js/Makefile
+++ b/js/Makefile
@@ -2,6 +2,7 @@ include ../defines.mk
 
 JSSRC=							\
 	Utils.js					\
+	form/ModeSelector.js				\
 	FilterProxy.js					\
 	LoginView.js					\
 	RoleSelector.js					\
diff --git a/js/ObjectGroup.js b/js/ObjectGroup.js
index 2223ffa..b91bf97 100644
--- a/js/ObjectGroup.js
+++ b/js/ObjectGroup.js
@@ -10,6 +10,7 @@ Ext.define('PMG.ObjectGroup', {
     showDirection: false, // only important for SMTP Whitelist
 
     ogdata: undefined,
+    oclass: undefined,
 
     emptyText: gettext('Please select an object.'),
 
@@ -32,10 +33,14 @@ Ext.define('PMG.ObjectGroup', {
     setObjectInfo: function(ogdata) {
 	let me = this;
 
+	let mode = ogdata.invert ? 'not' : '';
+	mode += ogdata.and ? 'all' : 'any';
+
 	me.ogdata = ogdata;
 
 	if (me.ogdata === undefined) {
 	    me.down('#oginfo').update(me.emptyText);
+	    me.down('#modeBox').setHidden(true);
 	} else {
 	    let html = '<b>' + Ext.String.htmlEncode(me.ogdata.name) + '</b>';
 	    html += "<br><br>";
@@ -43,6 +48,12 @@ Ext.define('PMG.ObjectGroup', {
 
 	    me.down('#oginfo').update(html);
 	    me.down('#ogdata').setHidden(false);
+	    let modeSelector = me.down('#modeSelector');
+	    modeSelector.suspendEvents();
+	    me.down('#modeSelector').setValue(mode);
+	    modeSelector.resumeEvents();
+	    me.down('#modeBox').setHidden(false);
+	    me.down('#whatWarning').setHidden(me.oclass !== 'what');
 	}
     },
 
@@ -205,13 +216,62 @@ Ext.define('PMG.ObjectGroup', {
 	me.dockedItems.push({
 	    dock: 'top',
 	    border: 1,
-	    layout: 'anchor',
+	    layout: 'hbox',
 	    hidden: !!me.hideGroupInfo,
 	    itemId: 'ogdata',
 	    items: [
+		{
+		    xtype: 'container',
+		    itemId: 'modeBox',
+		    hidden: true,
+		    width: 220,
+		    padding: 10,
+		    layout: {
+			type: 'vbox',
+			align: 'stretch',
+		    },
+		    items: [
+			{
+			    xtype: 'box',
+			    html: `<b>${gettext("Match if")}</b>`,
+			},
+			{
+			    xtype: 'pmgModeSelector',
+			    itemId: 'modeSelector',
+			    padding: '10 0 0 0',
+			    listeners: {
+				change: function(_field, value) {
+				    let invert = value.startsWith('not');
+				    let and = value.endsWith('all');
+
+				    let url = `${me.baseurl}/config`;
+
+				    Proxmox.Utils.API2Request({
+					url,
+					method: 'PUT',
+					params: {
+					    and: and ? 1 : 0,
+					    invert: invert ? 1 : 0,
+					},
+					success: function() {
+					    me.fireEvent('modeUpdate', me);
+					},
+				    });
+				},
+			    },
+			},
+			{
+			    xtype: 'displayfield',
+			    itemId: 'whatWarning',
+			    hidden: true,
+			    value: gettext("Caution: What Objects match per mail part, so be careful with any option besides 'Any matches'."),
+			    userCls: 'pmx-hint',
+			},
+		    ],
+		},
 		{
 		    xtype: 'component',
-		    anchor: '100%',
+		    flex: 1,
 		    itemId: 'oginfo',
 		    style: { 'white-space': 'pre' },
 		    padding: 10,
diff --git a/js/ObjectGroupConfiguration.js b/js/ObjectGroupConfiguration.js
index 1d72851..f59f5ed 100644
--- a/js/ObjectGroupConfiguration.js
+++ b/js/ObjectGroupConfiguration.js
@@ -30,6 +30,7 @@ Ext.define('PMG.ObjectGroupConfiguration', {
 
 	var right = Ext.create('PMG.ObjectGroup', {
 	    otype_list: me.otype_list,
+	    oclass: me.ogclass,
 	    border: false,
 	    region: 'center',
 	    listeners: {
@@ -40,6 +41,9 @@ Ext.define('PMG.ObjectGroupConfiguration', {
 			left.run_editor();
 		    }
 		},
+		modeUpdate: function() {
+		    left.reload();
+		},
 	    },
 	});
 
diff --git a/js/RuleInfo.js b/js/RuleInfo.js
index c29c0ca..d53c1c5 100644
--- a/js/RuleInfo.js
+++ b/js/RuleInfo.js
@@ -120,6 +120,8 @@ Ext.define('PMG.RuleInfo', {
 			name: oc,
 			oclass: oc,
 			type: true,
+			invert: ruledata[`${oc}-invert`],
+			and: ruledata[`${oc}-and`],
 			leaf: false,
 			expanded: true,
 			expandable: false,
@@ -162,6 +164,25 @@ Ext.define('PMG.RuleInfo', {
 	    return true;
 	},
 
+	updateMode: function(field, value) {
+	    let me = this;
+	    let vm = me.getViewModel();
+	    let oclass = field.getWidgetRecord().data.oclass;
+
+	    let params = {};
+	    params[`${oclass}-invert`] = value.startsWith('not') ? 1 : 0;
+	    params[`${oclass}-and`] = value.endsWith('all') ? 1 : 0;
+
+	    Proxmox.Utils.API2Request({
+		url: `${vm.get('baseurl')}/config`,
+		method: 'PUT',
+		params,
+		success: function() {
+		    me.reload();
+		},
+	    });
+	},
+
 	control: {
 	    'treepanel[reference=usedobjects]': {
 		drop: 'addDrop',
@@ -169,6 +190,9 @@ Ext.define('PMG.RuleInfo', {
 	    'tabpanel[reference=availobjects] > grid': {
 		drop: 'removeDrop',
 	    },
+	    'pmgModeSelector': {
+		change: 'updateMode',
+	    },
 	},
     },
 
@@ -311,6 +335,26 @@ Ext.define('PMG.RuleInfo', {
 		    },
 		    flex: 1,
 		},
+		{
+		    header: gettext('Match if'),
+		    xtype: 'widgetcolumn',
+		    width: 200,
+		    widget: {
+			xtype: 'pmgModeSelector',
+		    },
+		    onWidgetAttach: function(col, widget, rec) {
+			if (rec.data.type && rec.data.oclass !== 'action') {
+			    let mode = rec.data.invert ? 'not' : '';
+			    mode += rec.data.and ? 'all' : 'any';
+			    widget.suspendEvents();
+			    widget.setValue(mode);
+			    widget.resumeEvents();
+			    widget.setHidden(false);
+			} else {
+			    widget.setHidden(true);
+			}
+		    },
+		},
 		{
 		    text: '',
 		    xtype: 'actioncolumn',
diff --git a/js/form/ModeSelector.js b/js/form/ModeSelector.js
new file mode 100644
index 0000000..9c9275b
--- /dev/null
+++ b/js/form/ModeSelector.js
@@ -0,0 +1,11 @@
+Ext.define('PMG.ModeSelector', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: 'widget.pmgModeSelector',
+
+    comboItems: [
+	['all', gettext('All match')],
+	['any', gettext('Any matches')],
+	['notall', gettext('At least one does not match')],
+	['notany', gettext('None matches')],
+    ],
+});
-- 
2.30.2

Re: [pmg-devel] [PATCH pmg-api 03/12] RuleCache: reorganize how we gather marks and spaminfo

[Stoiko Ivanov]

On Fri,  9 Feb 2024 13:54:27 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> instead of collecting the spaminfo (+match) seperately, collect this
> per target together with the regular marks. With this, we can omit the
> 'global' marks list, since each target has their own anyway.
> 
> We want this, since when we'll implement and/invert for matches, the marks
> can differ between targets, since the spamlevel can diverge for them and
> that can be and-combined with objects that add marks. For that to be
> possible we have to save each match + info per target instead of
> globally.
> 
> Since we don't change the actual matching behaviour with this patch,
> for the remove action, we can simply use the marks from the first target
> (as they currently have to be identical).
I don't think this premise holds - or rather the reasoning seems a bit off?

* marks are generated with what_matches
* global (not-per-part) matches are virus, spam - these just mark with an
  empty array-ref [] - indicating they affect the whole mail
* per-part what-matches are MatchField, and the content-type/filename
  matches - they add a list of all parts they match
* the only what_match that might differ per user/target is the spam-match,
  which marks the complete mail

marks are identical per rule across all targets, because the only place
where they could differ just pushes the contents of an empty array to the
list. 

(sorry if this sounds a bit pedantic - but it sadly took me 30 minutes
with Data::Dumper to get my head around this)

> 
> Conversely, we currently save the spaminfo per target, but later in
> pmg-smtp-filter we only ever use the first one we encounter, so instead
> save it only the first time and use that.
we currently get the spaminfo as part of the resulting hashref from
RuleCache::what_match, next to the only other member 'targets'.
Maybe we could return that as second value from what_match and save
ourselves the second level of nesting (see inline)
Please disregard if this becomes obsolete by one of the later patches

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/RuleCache.pm     | 32 ++++++++++----------------------
>  src/PMG/RuleDB/Remove.pm | 19 +++++++++++++++----
>  src/bin/pmg-smtp-filter  | 18 +++++-------------
>  3 files changed, 30 insertions(+), 39 deletions(-)
> 
> diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
> index fd22a16..4f7ebe7 100644
> --- a/src/PMG/RuleCache.pm
> +++ b/src/PMG/RuleCache.pm
> @@ -304,37 +304,25 @@ sub what_match {
>      if (scalar($what->{groups}->@*) == 0) {
>  	# match all targets
>  	foreach my $target (@{$msginfo->{targets}}) {
> -	    $res->{$target}->{marks} = [];
> +	    $res->{targets}->{$target}->{marks} = [];
here this could become $res->{$target}->{marks}
>  	}
> -
> -	$res->{marks} = [];
>  	return $res;
>      }
>  
> -    my $marks;
> -
>      for my $group ($what->{groups}->@*) {
>  	for my $obj ($group->{objects}->@*) {
>  	    if (!$obj->can('what_match_targets')) {
>  		if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
> -		    push @$marks, @$match;
> +		    for my $target ($msginfo->{targets}->@*) {
> +			push $res->{targets}->{$target}->{marks}->@*, $match->@*;
here as well

> +		    }
>  		}
> -	    }
> -	}
> -    }
> -
> -    foreach my $target (@{$msginfo->{targets}}) {
> -	$res->{$target}->{marks} = $marks;
> -	$res->{marks} = $marks;
> -    }
> -
> -    for my $group ($what->{groups}->@*) {
> -	for my $obj ($group->{objects}->@*) {
> -	    if ($obj->can ("what_match_targets")) {
> -		my $target_info;
> -		if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
> -		    foreach my $k (keys %$target_info) {
> -			$res->{$k} = $target_info->{$k};
> +	    } else {
> +		if (my $target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
> +		    foreach my $k (keys $target_info->%*) {
> +			push $res->{targets}->{$k}->{marks}->@*, $target_info->{$k}->{marks}->@*;
and here
> +			# only save spaminfo once
> +			$res->{spaminfo} = $target_info->{$k}->{spaminfo} if !defined($res->{spaminfo});
this would need to be changed (and returned as second value below)

>  		    }
>  		}
>  	    }
> diff --git a/src/PMG/RuleDB/Remove.pm b/src/PMG/RuleDB/Remove.pm
> index e7c353c..5812602 100644
> --- a/src/PMG/RuleDB/Remove.pm
> +++ b/src/PMG/RuleDB/Remove.pm
> @@ -198,9 +198,15 @@ sub execute {
>  
>      my $rulename = encode('UTF-8', $vars->{RULE} // 'unknown');
>  
> -    if (!$self->{all} && ($#$marks == -1)) {
> -	# no marks
> -	return;
> +    if (!$self->{all}) {
> +	my $found_mark = 0;
> +	for my $target (keys $marks->{targets}->%*) {
> +	    if (scalar($marks->{targets}->{$target}->{marks}->@*) > 0) {
> +		$found_mark = 1;
> +		last;
> +	    }
> +	}
> +	return if !$found_mark;
>      }
>  
>      my $subgroups = $mod_group->subgroups ($targets);
> @@ -256,7 +262,12 @@ sub execute {
>  	}
>  
>  	$self->{message_seen} = 0;
> -	$self->delete_marked_parts($queue, $entity, $html, $rtype, $marks, $rulename);
> +
> +	# since all matches are or combinded, marks for all targets must be the same if they exist
> +	# so simply use the first one here
maybe "since currently all marks are equal for all targets, use the first
one"?
> +	my $match_marks = $marks->{targets}->{$tg->[0]}->{marks};
> +
> +	$self->delete_marked_parts($queue, $entity, $html, $rtype, $match_marks, $rulename);
>  	delete $self->{message_seen};
>  
>  	if ($msginfo->{testmode}) {
> diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
> index 7da3de8..71043b0 100755
> --- a/src/bin/pmg-smtp-filter
> +++ b/src/bin/pmg-smtp-filter
> @@ -276,8 +276,9 @@ sub apply_rules {
>  	foreach my $target (@{$msginfo->{targets}}) {
>  	    next if $final->{$target};
>  	    next if !defined ($rule_marks{$rule->{id}});
> -	    next if !defined ($rule_marks{$rule->{id}}->{$target});
> -	    next if !defined ($rule_marks{$rule->{id}}->{$target}->{marks});
> +	    next if !defined ($rule_marks{$rule->{id}}->{targets});
here you could get rid of this line - if the what_match returns the spaminfo as second value.

> +	    next if !defined ($rule_marks{$rule->{id}}->{targets}->{$target});
> +	    next if !defined ($rule_marks{$rule->{id}}->{targets}->{$target}->{marks});
and here get rid of {targets}->
>  	    next if !$rulecache->to_match ($rule->{id}, $target, $ldap);
>  
>  	    $final->{$target} = $fin;
> @@ -320,24 +321,15 @@ sub apply_rules {
>  	my $targets = $rule_targets{$rule->{id}};
>  	next if !$targets;
>  
> -	my $spaminfo;
> -	foreach my $t (@$targets) {
> -	    if ($rule_marks{$rule->{id}}->{$t} && $rule_marks{$rule->{id}}->{$t}->{spaminfo}) {
> -		$spaminfo = $rule_marks{$rule->{id}}->{$t}->{spaminfo};
> -		# we assume spam info is the same for all matching targets
> -		last;
> -	    }
> -	}
> -
>  	my $vars = $self->get_prox_vars (
> -	    $queue, $entity, $msginfo, $rule, $rule_targets{$rule->{id}}, $spaminfo);
> +	    $queue, $entity, $msginfo, $rule, $rule_targets{$rule->{id}}, $rule_marks{$rule->{id}}->{spaminfo});
>  
>  	my @sorted_actions = sort {$a->priority <=> $b->priority} @{$rule_actions{$rule->{id}}};
>  
>  	foreach my $action (@sorted_actions) {
>  	    $action->execute(
>  		$queue, $self->{ruledb}, $mod_group, $rule_targets{$rule->{id}}, $msginfo, $vars,
> -		$rule_marks{$rule->{id}}->{marks}, $ldap
> +		$rule_marks{$rule->{id}}, $ldap
>  	    );
>  	    last if $action->final;
>  	}

Re: [pmg-devel] [PATCH pmg-api 04/12] api: refactor rule parameters

[Stoiko Ivanov]

looks good to me - one nit inline:

On Fri,  9 Feb 2024 13:54:28 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> makes it easier to add new ones
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/RuleDB.pm | 20 ++++++--------------
>  src/PMG/API2/Rules.pm  | 41 +++++++++++++++++++++++++++--------------
>  2 files changed, 33 insertions(+), 28 deletions(-)
> 
> diff --git a/src/PMG/API2/RuleDB.pm b/src/PMG/API2/RuleDB.pm
> index 1fddb32..064e72f 100644
> --- a/src/PMG/API2/RuleDB.pm
> +++ b/src/PMG/API2/RuleDB.pm
> @@ -162,7 +162,7 @@ __PACKAGE__->register_method({
>      permissions => { check => [ 'admin' ] },
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {
> +	properties => PMG::API2::Rules::get_rule_params({
>  	    name => {
>  		description => "Rule name",
>  		type => 'string',
> @@ -173,19 +173,7 @@ __PACKAGE__->register_method({
>  		minimum => 0,
>  		maximum => 100,
>  	    },
> -	    direction => {
> -		description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
> -		type => 'integer',
> -		minimum => 0,
> -		maximum => 2,
> -		optional => 1,
> -	    },
> -	    active => {
> -		description => "Flag to activate rule.",
> -		type => 'boolean',
> -		optional => 1,
> -	    },
> -	},
> +	}),
>      },
>      returns => { type => 'integer' },
>      code => sub {
> @@ -196,6 +184,10 @@ __PACKAGE__->register_method({
>  	my $rule = PMG::RuleDB::Rule->new (
>  	    $param->{name}, $param->{priority}, $param->{active}, $param->{direction});
you could remove active and direction here, as they get set
>  
> +	for my $key (keys PMG::API2::Rules::get_rule_params()->%*) {
> +	    $rule->{$key} = $param->{$key} if defined($param->{$key});
> +	}
> +
here
(also PMG::RuleDB::Rule::new could probably be made shorter (and the
get/set methods could add an explicit 'return'

but as said - only a nit and not really related.

>  	return $rdb->save_rule($rule);
>      }});
>  
> diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm
> index 4f8c10b..c48370f 100644
> --- a/src/PMG/API2/Rules.pm
> +++ b/src/PMG/API2/Rules.pm
> @@ -136,6 +136,31 @@ __PACKAGE__->register_method ({
>  	return $data;
>     }});
>  
> +my $rule_params = {
> +    direction => {
> +	description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
> +	type => 'integer',
> +	minimum => 0,
> +	maximum => 2,
> +	optional => 1,
> +    },
> +    active => {
> +	description => "Flag to activate rule.",
> +	type => 'boolean',
> +	optional => 1,
> +    },
> +};
> +
> +sub get_rule_params {
> +    my ($base) = @_;
> +    $base //= {};
> +    return {
> +	$base->%*,
> +	$rule_params->%*
> +    };
> +}
> +
> +
>  __PACKAGE__->register_method ({
>      name => 'update_config',
>      path => 'config',
> @@ -146,7 +171,7 @@ __PACKAGE__->register_method ({
>      permissions => { check => [ 'admin' ] },
>      parameters => {
>  	additionalProperties => 0,
> -	properties => {
> +	properties => get_rule_params({
>  	    id => {
>  		description => "Rule ID.",
>  		type => 'integer',
> @@ -156,18 +181,6 @@ __PACKAGE__->register_method ({
>  		type => 'string',
>  		optional => 1,
>  	    },
> -	    active => {
> -		description => "Flag to activate rule.",
> -		type => 'boolean',
> -		optional => 1,
> -	    },
> -	    direction => {
> -		description => "Rule direction. Value `0` matches incoming mails, value `1` matches outgoing mails, and value `2` matches both directions.",
> -		type => 'integer',
> -		minimum => 0,
> -		maximum => 2,
> -		optional => 1,
> -	    },
>  	    priority => {
>  		description => "Rule priotity.",
>  		type => 'integer',
> @@ -175,7 +188,7 @@ __PACKAGE__->register_method ({
>  		maximum => 100,
>  		optional => 1,
>  	    },
> -	},
> +	}),
>      },
>      returns => { type => "null" },
>      code => sub {

Re: [pmg-devel] [PATCH pmg-api 05/12] add objectgroup attributes and/invert

[Stoiko Ivanov]

afaict deletion of objectgroup attributes when deleting the object group is
missing

On Fri,  9 Feb 2024 13:54:29 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> add a new table Objectgroup_Attributes where we can save additional
> attributes for objectgroups (like the Attribut tables for objects).
> 
> Adds two new attributes for the groups:
> * and
> * invert
> 
> These will modify the match behaviour for object groups
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/ObjectGroupHelpers.pm |  43 ++++++++-
>  src/PMG/DBTools.pm                 |  15 +++
>  src/PMG/RuleDB.pm                  | 145 ++++++++++++++++++++++-------
>  3 files changed, 162 insertions(+), 41 deletions(-)
> 
> diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
> index 48078fb..a08a6a3 100644
> --- a/src/PMG/API2/ObjectGroupHelpers.pm
> +++ b/src/PMG/API2/ObjectGroupHelpers.pm
> @@ -46,13 +46,29 @@ sub format_object_group {
>  
>      my $res = [];
>      foreach my $og (@$ogroups) {
> -	push @$res, {
> -	    id => $og->{id}, name => $og->{name}, info => $og->{info}
> -	};
> +	my $group = { id => $og->{id}, name => $og->{name}, info => $og->{info} };
> +	$group->{and} = $og->{and} if defined($og->{and});
> +	$group->{invert} = $og->{invert} if defined($og->{invert});
> +	push @$res, $group;
>      }
>      return $res;
>  }
>  
> +my $group_attributes = {
> +    and => {
> +	description => "If set to 1, objects in this group are 'and' combined.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    invert => {
> +	description => "If set to 1, the resulting match is inverted.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +};
> +
>  sub register_group_list_api {
>      my ($apiclass, $oclass) = @_;
>  
> @@ -86,6 +102,11 @@ sub register_group_list_api {
>  	    return format_object_group($ogroups);
>  	}});
>  
> +    my $additional_parameters = {};
> +    if ($oclass =~ /^(?:what|when|who)$/i) {
> +	$additional_parameters = { $group_attributes->%* };
> +    }
> +
>      $apiclass->register_method({
>  	name => "create_${oclass}_group",
>  	path => $oclass,
> @@ -108,6 +129,7 @@ sub register_group_list_api {
>  		    maxLength => 255,
>  		    optional => 1,
>  		},
> +		$additional_parameters->%*,
>  	    },
>  	},
>  	returns => { type => 'integer' },
> @@ -119,6 +141,10 @@ sub register_group_list_api {
>  	    my $og = PMG::RuleDB::Group->new(
>  		$param->{name}, $param->{info} // '', $oclass);
>  
> +	    for my $prop (qw(and invert)) {
> +		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
> +	    }
> +
>  	    return $rdb->save_group($og);
>  	}});
>  }
> @@ -199,6 +225,11 @@ sub register_object_group_config_api {
>  
>  	}});
>  
> +    my $additional_parameters = {};
> +    if ($oclass =~ /^(?:what|when|who)$/i) {
> +	$additional_parameters = { $group_attributes->%* };
> +    }
> +
>      $apiclass->register_method({
>  	name => 'set_config',
>  	path => $path,
> @@ -226,6 +257,7 @@ sub register_object_group_config_api {
>  		    maxLength => 255,
>  		    optional => 1,
>  		},
> +		$additional_parameters->%*,
>  	    },
>  	},
>  	returns => { type => "null" },
> @@ -243,8 +275,9 @@ sub register_object_group_config_api {
>  	    my $og = shift @$list ||
>  		die "$oclass group '$ogroup' not found\n";
>  
> -	    $og->{name} = $param->{name} if defined($param->{name});
> -	    $og->{info} = $param->{info} if defined($param->{info});
> +	    for my $prop (qw(name info and invert)) {
> +		$og->{$prop} = $param->{$prop} if defined($param->{$prop});
> +	    }
>  
>  	    $rdb->save_group($og);
>  
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index 9e133bc..0d3d9c3 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
>  
>  __EOD
>  
> +my $object_group_attributes_cmd = <<__EOD;
> +    CREATE TABLE Objectgroup_Attributes (
> +      Objectgroup_ID INTEGER NOT NULL,
we could create a foreign key constraint on objectgroup.id here
> +      Name VARCHAR(20) NOT NULL,
> +      Value BYTEA NULL,
I know with the current db-schema we use bytea quite extensively - but for
now all the values are actually boolean (and even more so only the true
values are stored) - why not create the column accordingly?

> +      PRIMARY KEY (Objectgroup_ID, Name)
> +    );
> +
> +    CREATE INDEX Objectgroup_Attributes_Objectgroup_ID_Index ON Objectgroup_Attributes(Objectgroup_ID);
> +
> +__EOD
> +
>  sub cond_create_dbtable {
>      my ($dbh, $name, $ctablecmd) = @_;
>  
> @@ -439,6 +451,8 @@ sub create_ruledb {
>          $userprefs_ctablecmd;
>  
>          $virusinfo_stat_ctablecmd;
> +
> +        $object_group_attributes_cmd;
>  EOD
>      );
>  
> @@ -494,6 +508,7 @@ sub upgradedb {
>  	'CStatistic', $cstatistic_ctablecmd,
>  	'ClusterInfo', $clusterinfo_ctablecmd,
>  	'VirusInfo', $virusinfo_stat_ctablecmd,
> +	'Objectgroup_Attributes', $object_group_attributes_cmd,
>      };
>  
>      foreach my $table (keys %$tables) {
> diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
> index a6b0b79..df9e526 100644
> --- a/src/PMG/RuleDB.pm
> +++ b/src/PMG/RuleDB.pm
> @@ -160,6 +160,30 @@ sub load_groups_by_name {
>      };
>  }
>  
> +sub update_group_attributes {
> +    my ($self, $og) = @_;
> +
> +    my $attributes = [qw(and invert)];
> +
> +    for my $attribute ($attributes->@*) {
> +	# only save the values if they're set to 1
> +	if ($og->{$attribute}) {
> +	    $self->{dbh}->do(
> +		"INSERT INTO Objectgroup_Attributes (Objectgroup_ID, Name, Value) " .
> +		"VALUES (?, ?, ?) ".
> +		"ON CONFLICT (Objectgroup_ID, Name) DO UPDATE SET Value = ?", undef,
> +		$og->{id}, $attribute, $og->{$attribute}, $og->{$attribute},
> +	    );
> +	} else {
> +	    $self->{dbh}->do(
> +		"DELETE FROM Objectgroup_Attributes " .
> +		"WHERE Objectgroup_ID = ? AND Name = ?", undef,
> +		$og->{id}, $attribute,
> +	    );
> +	}
> +    }
> +}
> +
>  sub save_group {
>      my ($self, $og) = @_;
>  
> @@ -171,27 +195,51 @@ sub save_group {
>  	die "undefined group attribute - class: ERROR";
>  
>      if (defined($og->{id})) {
> +	$self->{dbh}->begin_work;
> +
> +	eval {
> +	    $self->{dbh}->do("UPDATE Objectgroup " .
> +			     "SET Name = ?, Info = ? " .
> +			     "WHERE ID = ?", undef,
> +			     encode('UTF-8', $og->{name}),
> +			     encode('UTF-8', $og->{info}),
> +			     $og->{id});
>  
> -	$self->{dbh}->do("UPDATE Objectgroup " .
> -			 "SET Name = ?, Info = ? " .
> -			 "WHERE ID = ?", undef,
> -			 encode('UTF-8', $og->{name}),
> -			 encode('UTF-8', $og->{info}),
> -			 $og->{id});
> +	    $self->update_group_attributes($og);
>  
> -	return $og->{id};
> +	    $self->{dbh}->commit;
> +	};
>  
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      } else {
> -	my $sth = $self->{dbh}->prepare(
> -	    "INSERT INTO Objectgroup (Name, Info, Class) " .
> -	    "VALUES (?, ?, ?);");
> +	$self->{dbh}->begin_work;
>  
> -	$sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
> +	eval {
> +	    my $sth = $self->{dbh}->prepare(
> +		"INSERT INTO Objectgroup (Name, Info, Class) " .
> +		"VALUES (?, ?, ?);");
>  
> -	return $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
> +	    $sth->execute(encode('UTF-8', $og->name), encode('UTF-8', $og->info), $og->class);
> +
> +	    $og->{id} = PMG::Utils::lastid($self->{dbh}, 'objectgroup_id_seq');
> +
> +	    $self->update_group_attributes($og);
> +
> +	    $self->{dbh}->commit;
> +	};
> +
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      }
>  
> -    return undef;
> +    return $og->{id};
>  }
>  
>  sub delete_group {
> @@ -252,6 +300,18 @@ sub delete_group {
>      return undef;
>  }
>  
> +sub load_group_attributes {
> +    my ($self, $og) = @_;
> +
> +    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
> +    $attribute_sth->execute($og->{id});
> +
> +    while (my $ref = $attribut<e_sth->fetchrow_hashref()) {
> +	$og->{and} = $ref->{value} if $ref->{name} eq 'and';
> +	$og->{invert} = $ref->{value} if $ref->{name} eq 'invert';
> +    }
> +}
> +
>  sub load_objectgroups {
>      my ($self, $class, $id) = @_;
>  
> @@ -259,34 +319,47 @@ sub load_objectgroups {
>  
>      defined($class) || die "undefined object class";
>  
> -    if (!(defined($id))) {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> -        $sth->execute($class);
> -
> -    } else {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class like ? and id = ? " .
> -	    "order by name");
> -        $sth->execute($class,$id);
> -    }
> +    $self->{dbh}->begin_work;
why running the following SELECTS in a explicit transaction?

>  
>      my $arr_og = ();
> -    while (my $ref = $sth->fetchrow_hashref()) {
> -    	my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> -					 $ref->{class});
> -    	$og->{id} = $ref->{id};
>  
> -	if ($class eq 'action') {
> -	    my $objects = $self->load_group_objects($og->{id});
> -	    my $obj = @$objects[0];
> -	    defined($obj) || die "undefined action object: ERROR";
> -	    $og->{action} = $obj;
> +    eval {
> +	if (!(defined($id))) {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> +	    $sth->execute($class);
> +
> +	} else {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class like ? and id = ? " .
not introduced by you - but why do we use 'like' here and '=' above?


> +		"order by name");
> +	    $sth->execute($class,$id);
>  	}
> -    	push @$arr_og, $og;
> -    }
>  
> -    $sth->finish();
> +	while (my $ref = $sth->fetchrow_hashref()) {
> +	    my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> +					     $ref->{class});
> +	    $og->{id} = $ref->{id};
> +
> +	    if ($class eq 'action') {
> +		my $objects = $self->load_group_objects($og->{id});
> +		my $obj = @$objects[0];
> +		defined($obj) || die "undefined action object: ERROR";
> +		$og->{action} = $obj;
> +	    } else {
> +		$self->load_group_attributes($og);
> +	    }
> +	    push @$arr_og, $og;
> +	}
> +
> +	$sth->finish();
> +    };
> +
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
> +
> +    die $err if $err;
>  
>      return $arr_og;
>  }

Re: [pmg-devel] [PATCH pmg-api 05/12] add objectgroup attributes and/invert

[Stoiko Ivanov]

forgot one point in my last reply (or hid it behind the question of why
running transactions for selects): 
On Fri,  9 Feb 2024 13:54:29 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

>..snip..
>  
> +sub load_group_attributes {
> +    my ($self, $og) = @_;
> +
> +    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
> +    $attribute_sth->execute($og->{id});
> +
> +    while (my $ref = $attribute_sth->fetchrow_hashref()) {
> +	$og->{and} = $ref->{value} if $ref->{name} eq 'and';
> +	$og->{invert} = $ref->{value} if $ref->{name} eq 'invert';
> +    }
> +}
> +
>  sub load_objectgroups {
>      my ($self, $class, $id) = @_;
>  
> @@ -259,34 +319,47 @@ sub load_objectgroups {
>  
>      defined($class) || die "undefined object class";
>  
> -    if (!(defined($id))) {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> -        $sth->execute($class);
> -
> -    } else {
> -        $sth = $self->{dbh}->prepare(
> -	    "SELECT * FROM Objectgroup where Class like ? and id = ? " .
> -	    "order by name");
> -        $sth->execute($class,$id);
> -    }
> +    $self->{dbh}->begin_work;
>  
>      my $arr_og = ();
> -    while (my $ref = $sth->fetchrow_hashref()) {
> -    	my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> -					 $ref->{class});
> -    	$og->{id} = $ref->{id};
>  
> -	if ($class eq 'action') {
> -	    my $objects = $self->load_group_objects($og->{id});
> -	    my $obj = @$objects[0];
> -	    defined($obj) || die "undefined action object: ERROR";
> -	    $og->{action} = $obj;
> +    eval {
> +	if (!(defined($id))) {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class = ? ORDER BY name");
> +	    $sth->execute($class);
> +
> +	} else {
> +	    $sth = $self->{dbh}->prepare(
> +		"SELECT * FROM Objectgroup where Class like ? and id = ? " .
> +		"order by name");
> +	    $sth->execute($class,$id);
>  	}
> -    	push @$arr_og, $og;
> -    }
>  
> -    $sth->finish();
> +	while (my $ref = $sth->fetchrow_hashref()) {
> +	    my $og = PMG::RuleDB::Group->new($ref->{name}, $ref->{info},
> +					     $ref->{class});
> +	    $og->{id} = $ref->{id};
> +
> +	    if ($class eq 'action') {
> +		my $objects = $self->load_group_objects($og->{id});
> +		my $obj = @$objects[0];
> +		defined($obj) || die "undefined action object: ERROR";
> +		$og->{action} = $obj;
> +	    } else {
> +		$self->load_group_attributes($og);
> +	    }
> +	    push @$arr_og, $og;
> +	}
> +
> +	$sth->finish();
> +    };
> +
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
the rollback here is unconditional (not that I think it would ever do
something)
> +
> +    die $err if $err;
also can be dropped, if we drop the eval+transaction block
>  
>      return $arr_og;
>  }

Re: [pmg-devel] [PATCH pmg-api 06/12] add rule attributes and/invert (for each relevant type)

[Stoiko Ivanov]

similarly here deleting the rule-attributes upon rule-deletion seems
missing (also most suggestions from the last patch also apply, but I try
to mention them explicitly inline as well)

comments inline:
On Fri,  9 Feb 2024 13:54:30 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> like with the objectgroups, add an attributes table for groups, and an
> 'and'/'invert' attribute for each relevant object type
> (what/when/from/to).
> 
> This is intended to modify the behaviour for the matching regarding
> object groups, so that one has more choice in the logical matching.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/ObjectGroupHelpers.pm |   8 ++
>  src/PMG/API2/Rules.pm              |  53 ++++++++-
>  src/PMG/DBTools.pm                 |  15 +++
>  src/PMG/RuleDB.pm                  | 169 +++++++++++++++++++++++------
>  4 files changed, 209 insertions(+), 36 deletions(-)
> 
> diff --git a/src/PMG/API2/ObjectGroupHelpers.pm b/src/PMG/API2/ObjectGroupHelpers.pm
> index a08a6a3..3060157 100644
> --- a/src/PMG/API2/ObjectGroupHelpers.pm
> +++ b/src/PMG/API2/ObjectGroupHelpers.pm
> @@ -31,6 +31,14 @@ sub format_rule {
>  	active => $rule->{active},
>  	direction => $rule->{direction},
>      };
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +    for my $type ($types->@*) {
> +	for my $attribute ($attributes->@*) {
> +	    my $opt = "${type}-${attribute}";
> +	    $data->{$opt} = $rule->{$opt} if defined($rule->{$opt});
> +	}
> +    }
>  
>      $cond_create_group->($data, 'from', $from);
>      $cond_create_group->($data, 'to', $to);
> diff --git a/src/PMG/API2/Rules.pm b/src/PMG/API2/Rules.pm
> index c48370f..1ebadc2 100644
> --- a/src/PMG/API2/Rules.pm
> +++ b/src/PMG/API2/Rules.pm
> @@ -149,6 +149,54 @@ my $rule_params = {
>  	type => 'boolean',
>  	optional => 1,
>      },
> +    'what-and' => {
> +	description => "Flag to 'and' combine WHAT group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'what-invert' => {
> +	description => "Flag to invert WHAT group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'when-and' => {
> +	description => "Flag to 'and' combine WHEN group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'when-invert' => {
> +	description => "Flag to invert WHEN group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'from-and' => {
> +	description => "Flag to 'and' combine FROM group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'from-invert' => {
> +	description => "Flag to invert FROM group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'to-and' => {
> +	description => "Flag to 'and' combine TO group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
> +    'to-invert' => {
> +	description => "Flag to invert TO group matches.",
> +	type => 'boolean',
> +	default => 0,
> +	optional => 1,
> +    },
>  };
>  
>  sub get_rule_params {
> @@ -203,7 +251,10 @@ __PACKAGE__->register_method ({
>  
>  	my $rule = $rdb->load_rule($id);
>  
> -	for my $key (qw(name active direction priority)) {
> +	my $keys = ["name", "priority"];
> +	push $keys->@*, keys get_rule_params()->%*;
> +
> +	for my $key ($keys->@*) {
>  	    $rule->{$key} = $param->{$key} if defined($param->{$key});
>  	}
>  
> diff --git a/src/PMG/DBTools.pm b/src/PMG/DBTools.pm
> index 0d3d9c3..605eb71 100644
> --- a/src/PMG/DBTools.pm
> +++ b/src/PMG/DBTools.pm
> @@ -295,6 +295,18 @@ my $userprefs_ctablecmd =  <<__EOD;
>  
>  __EOD
>  
> +my $rule_attributes_cmd = <<__EOD;
> +    CREATE TABLE Rule_Attributes (
> +      Rule_ID INTEGER NOT NULL,
we could consider adding a foreign key constraint on Rule.id here
> +      Name VARCHAR(20) NOT NULL,
> +      Value BYTEA NULL,
if we're only storing booleans, we could use the proper column type for
this

> +      PRIMARY KEY (Rule_ID, Name)
> +    );
> +
> +    CREATE INDEX Rule_Attributes_Rule_ID_Index ON Rule_Attributes(Rule_ID);
> +
> +__EOD
> +
>  my $object_group_attributes_cmd = <<__EOD;
>      CREATE TABLE Objectgroup_Attributes (
>        Objectgroup_ID INTEGER NOT NULL,
> @@ -452,6 +464,8 @@ sub create_ruledb {
>  
>          $virusinfo_stat_ctablecmd;
>  
> +        $rule_attributes_cmd;
> +
>          $object_group_attributes_cmd;
>  EOD
>      );
> @@ -508,6 +522,7 @@ sub upgradedb {
>  	'CStatistic', $cstatistic_ctablecmd,
>  	'ClusterInfo', $clusterinfo_ctablecmd,
>  	'VirusInfo', $virusinfo_stat_ctablecmd,
> +	'Rule_Attributes', $rule_attributes_cmd,
>  	'Objectgroup_Attributes', $object_group_attributes_cmd,
>      };
>  
> diff --git a/src/PMG/RuleDB.pm b/src/PMG/RuleDB.pm
> index df9e526..70770a8 100644
> --- a/src/PMG/RuleDB.pm
> +++ b/src/PMG/RuleDB.pm
> @@ -665,6 +665,35 @@ sub delete_object {
>      return 1;
>  }
>  
> +sub update_rule_attributes {
> +    my ($self, $rule) = @_;
> +
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +
> +    for my $type ($types->@*) {
> +	for my $attribute ($attributes->@*) {
> +	    my $prop = "$type-$attribute";
> +
> +	    # only save the values if they're set to 1
> +	    if ($rule->{$prop}) {
> +		$self->{dbh}->do(
> +		    "INSERT INTO Rule_Attributes (Rule_ID, Name, Value) " .
> +		    "VALUES (?, ?, ?) ".
> +		    "ON CONFLICT (Rule_ID, Name) DO UPDATE SET Value = ?", undef,
> +		    $rule->{id}, $prop, $rule->{$prop}, $rule->{$prop},
> +		);
> +	    } else {
> +		$self->{dbh}->do(
> +		    "DELETE FROM Rule_Attributes " .
> +		    "WHERE Rule_ID = ? AND Name = ?", undef,
> +		    $rule->{id}, $prop,
> +		);
> +	    }
> +	}
> +    }
> +}
> +
>  sub save_rule {
>      my ($self, $rule) = @_;
>  
> @@ -679,28 +708,53 @@ sub save_rule {
>  
>      my $rulename = encode('UTF-8', $rule->{name});
>      if (defined($rule->{id})) {
> +	$self->{dbh}->begin_work;
>  
> -	$self->{dbh}->do(
> -	    "UPDATE Rule " .
> -	    "SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
> -	    "WHERE ID = ?", undef,
> -	    $rulename, $rule->{priority}, $rule->{active},
> -	    $rule->{direction}, $rule->{id});
> +	eval {
> +	    $self->{dbh}->do(
> +		"UPDATE Rule " .
> +		"SET Name = ?, Priority = ?, Active = ?, Direction = ? " .
> +		"WHERE ID = ?", undef,
> +		$rulename, $rule->{priority}, $rule->{active},
> +		$rule->{direction}, $rule->{id});
> +
> +	    $self->update_rule_attributes($rule);
>  
> -	return $rule->{id};
> +	    $self->{dbh}->commit;
> +	};
>  
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      } else {
> -	my $sth = $self->{dbh}->prepare(
> -	    "INSERT INTO Rule (Name, Priority, Active, Direction) " .
> -	    "VALUES (?, ?, ?, ?);");
> +	$self->{dbh}->begin_work;
>  
> -	$sth->execute($rulename, $rule->priority, $rule->active,
> -		      $rule->direction);
> +	eval {
> +	    my $sth = $self->{dbh}->prepare(
> +		"INSERT INTO Rule (Name, Priority, Active, Direction) " .
> +		"VALUES (?, ?, ?, ?);");
> +
> +	    $sth->execute($rulename, $rule->priority, $rule->active,
> +		$rule->direction);
> +
> +
> +	    $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
> +
> +	    $self->update_rule_attributes($rule);
>  
> -	return $rule->{id} = PMG::Utils::lastid($self->{dbh}, 'rule_id_seq');
> +	    $self->{dbh}->commit;
> +	};
> +
> +	if (my $err = $@) {
> +	    $self->{dbh}->rollback;
> +	    syslog('err', $err);
> +	    return undef;
> +	}
>      }
>  
> -    return undef;
> +    return $rule->{id};
>  }
>  
>  sub delete_rule {
> @@ -826,24 +880,58 @@ sub rule_remove_group {
>      return 1;
>  }
>  
> +sub load_rule_attributes {
> +    my ($self, $rule) = @_;
> +
> +    my $types = [qw(what when from to)];
> +    my $attributes = [qw(and invert)];
> +
> +    my $attribute_sth = $self->{dbh}->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?");
> +    $attribute_sth->execute($rule->{id});
> +
> +    ATTRIBUTES_LOOP:
> +    while (my $ref = $attribute_sth->fetchrow_hashref()) {
> +	for my $type ($types->@*) {
> +	    for my $attribute ($attributes->@*) {
> +		my $prop = "$type-$attribute";
> +		if ($ref->{name} eq $prop) {
> +		    $rule->{$prop} = $ref->{value};
> +		    next ATTRIBUTES_LOOP;
would a simple regex match for the attribute here not work equally well,
and prevent the GOTO/next LABEL? 
if ($ref->{name} =~ /(?:what|when|from|to)-(?:and|invert)/)
(also we might consider a die if it does not match the expected pattern?)


> +		}
> +	    }
> +	}
> +    }
> +}
> +
>  sub load_rule {
>      my ($self, $id) = @_;
>  
>      defined($id) || die "undefined id: ERROR";
>  
> -    my $sth = $self->{dbh}->prepare(
> -	"SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
> +    $self->{dbh}->begin_work;
transaction probably not needed for selects
>  
> -    my $rules = ();
> +    my $rule;
>  
> -    $sth->execute($id);
> +    eval {
> +	my $sth = $self->{dbh}->prepare(
> +	    "SELECT * FROM Rule where id = ? ORDER BY Priority DESC");
>  
> -    my $ref = $sth->fetchrow_hashref();
> -    die "rule '$id' does not exist\n" if !defined($ref);
> +	$sth->execute($id);
> +
> +	my $ref = $sth->fetchrow_hashref();
> +	die "rule '$id' does not exist\n" if !defined($ref);
>  
> -    my $rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
> -				      $ref->{active}, $ref->{direction});
> -    $rule->{id} = $ref->{id};
> +	$rule = PMG::RuleDB::Rule->new($ref->{name}, $ref->{priority},
> +					  $ref->{active}, $ref->{direction});
> +	$rule->{id} = $ref->{id};
> +
> +	$self->load_rule_attributes($rule);
> +    };
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
unconditionally rollback
> +
> +    die $err if $err;
not needed if no eval/transaction

>  
>      return $rule;
>  }
> @@ -851,22 +939,33 @@ sub load_rule {
>  sub load_rules {
>      my ($self) = @_;
>  
> -    my $sth = $self->{dbh}->prepare(
> -	"SELECT * FROM Rule ORDER BY Priority DESC");
> -
>      my $rules = ();
>  
> -    $sth->execute();
> +    $self->{dbh}->begin_work;
transaction probably not needed for selects

>  
> -    while (my $ref = $sth->fetchrow_hashref()) {
> -	my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
> -	my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
> -					  $ref->{active}, $ref->{direction});
> -	$rule->{id} = $ref->{id};
> -	push @$rules, $rule;
> -    }
> +    eval {
> +	my $sth = $self->{dbh}->prepare(
> +	    "SELECT * FROM Rule ORDER BY Priority DESC");
>  
> -    $sth->finish();
> +	$sth->execute();
> +
> +	while (my $ref = $sth->fetchrow_hashref()) {
> +	    my $rulename = PMG::Utils::try_decode_utf8($ref->{name});
> +	    my $rule = PMG::RuleDB::Rule->new($rulename, $ref->{priority},
> +					      $ref->{active}, $ref->{direction});
> +	    $rule->{id} = $ref->{id};
> +	    #$self->load_rule_attributes($rule);
this line is commented out?
> +
> +	    push @$rules, $rule;
> +	}
> +
> +	$sth->finish();
> +    };
> +    my $err = $@;
> +
> +    $self->{dbh}->rollback;
unconditionally rollback
> +
> +    die $err if $err;
not needed if no eval/transaction
>  
>      return $rules;
>  }

Re: [pmg-devel] [PATCH pmg-api 08/12] RuleCache: implement and/invert for when/from/to

[Stoiko Ivanov]

LGTM (from a quick glance, and after thinking a bit too long why
match_list_with_mode needs to get called recursively)

Reviewed-by: Stoiko Ivanov <s.ivanov@proxmox.com>

On Fri,  9 Feb 2024 13:54:32 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> by introducing 'match_list_with_mode' that gets called for each group
> and then on the result of each group match result.
> 
> This does not work for 'what' matches since they are not a simple
> yes/no match (they include the parts) so this will be done seperately.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/RuleCache.pm | 65 +++++++++++++++++++++++++++++---------------
>  1 file changed, 43 insertions(+), 22 deletions(-)
> 
> diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
> index 0b62aeb..7d08107 100644
> --- a/src/PMG/RuleCache.pm
> +++ b/src/PMG/RuleCache.pm
> @@ -273,13 +273,14 @@ sub from_match {
>  	$ip = $1;
>      }
>  
> -    for my $group ($from->{groups}->@*) {
> -	for my $obj ($group->{objects}->@*) {
> -	    return 1 if $obj->who_match($addr, $ip, $ldap);
> -	}
> -    }
> -
> -    return 0;
> +    return match_list_with_mode($from->{groups}, $from->{and}, $from->{invert}, sub {
> +	my ($group) = @_;
> +	my $list = $group->{objects};
> +	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
> +	    my ($obj) = @_;
> +	    return $obj->who_match($addr, $ip, $ldap);
> +	});
> +    });
>  }
>  
>  sub to_match {
> @@ -289,14 +290,14 @@ sub to_match {
>  
>      return 1 if scalar($to->{groups}->@*) == 0;
>  
> -    for my $group ($to->{groups}->@*) {
> -	for my $obj ($group->{objects}->@*) {
> -	    return 1 if $obj->who_match($addr, undef, $ldap);
> -	}
> -    }
> -
> -
> -    return 0;
> +    return match_list_with_mode($to->{groups}, $to->{and}, $to->{invert}, sub {
> +	my ($group) = @_;
> +	my $list = $group->{objects};
> +	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
> +	    my ($obj) = @_;
> +	    return $obj->who_match($addr, undef, $ldap);
> +	});
> +    });
>  }
>  
>  sub when_match {
> @@ -306,13 +307,14 @@ sub when_match {
>  
>      return 1 if scalar($when->{groups}->@*) == 0;
>  
> -    for my $group ($when->{groups}->@*) {
> -	for my $obj ($group->{objects}->@*) {
> -	    return 1 if $obj->when_match($time);
> -	}
> -    }
> -
> -    return 0;
> +    return match_list_with_mode($when->{groups}, $when->{and}, $when->{invert}, sub {
> +	my ($group) = @_;
> +	my $list = $group->{objects};
> +	return match_list_with_mode($list, $group->{and}, $group->{invert}, sub {
> +	    my ($obj) = @_;
> +	    return $obj->when_match($time);
> +	});
> +    });
>  }
>  
>  sub what_match {
> @@ -357,4 +359,23 @@ sub what_match {
>      return $res;
>  }
>  
> +# calls sub with each element of $list, and and/ors/inverts the result
> +sub match_list_with_mode($$$$) {
> +    my ($list, $and, $invert, $sub) = @_;
> +
> +    $and //= 0;
> +    $invert //= 0;
> +
> +    for my $el ($list->@*) {
> +	my $res = $sub->($el);
> +	if (!$and) {
> +	    return !$invert if $res;
> +	} else {
> +	    return $invert if !$res;
> +	}
> +    }
> +
> +    return $and != $invert;
> +}
> +
>  1;

Re: [pmg-devel] [PATCH pmg-api 07/12] RuleCache: load rule/objectgroup attributes from database

[Stoiko Ivanov]

On Fri,  9 Feb 2024 13:54:31 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> so that we can use the 'and' and 'invert' flags set.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/RuleCache.pm | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
> index 4f7ebe7..0b62aeb 100644
> --- a/src/PMG/RuleCache.pm
> +++ b/src/PMG/RuleCache.pm
> @@ -67,6 +67,23 @@ sub new {
>  	    $self->{"$ruleid:what"} = { groups => [] };
>  	    $self->{"$ruleid:action"} = { groups => [] };
>  
> +	    my $attribute_sth = $dbh->prepare("SELECT * FROM Rule_Attributes WHERE Rule_ID = ?");
> +	    $attribute_sth->execute($ruleid);
> +
> +	    while (my $ref = $attribute_sth->fetchrow_hashref()) {
> +		$self->{"$ruleid:from"}->{and} = $ref->{value} if $ref->{name} eq 'from-and';
not sure if it's worth it, or makes this less readable ..., but
if ($ref-name =~ /(from|to|when|what)-(and|invert)/) {
    $self->{"$ruleid:$1"}->{$2} = $ref->{value)
}
might work here as well

> +		$self->{"$ruleid:from"}->{invert} = $ref->{value} if $ref->{name} eq 'from-invert';
> +
> +		$self->{"$ruleid:to"}->{and} = $ref->{value} if $ref->{name} eq 'to-and';
> +		$self->{"$ruleid:to"}->{invert} = $ref->{value} if $ref->{name} eq 'to-invert';
> +
> +		$self->{"$ruleid:when"}->{and} = $ref->{value} if $ref->{name} eq 'when-and';
> +		$self->{"$ruleid:when"}->{invert} = $ref->{value} if $ref->{name} eq 'when-invert';
> +
> +		$self->{"$ruleid:what"}->{and} = $ref->{value} if $ref->{name} eq 'what-and';
> +		$self->{"$ruleid:what"}->{invert} = $ref->{value} if $ref->{name} eq 'what-invert';
> +	    }
> +
>  	    my $sth1 = $dbh->prepare(
>  		"SELECT Objectgroup_ID, Grouptype FROM RuleGroup " .
>  		"where RuleGroup.Rule_ID = '$ruleid' " .
> @@ -114,6 +131,14 @@ sub new {
>  		    objects => $objects,
>  		};
>  
> +		my $objectgroup_sth = $dbh->prepare("SELECT * FROM Objectgroup_Attributes WHERE Objectgroup_ID = ?");
> +		$objectgroup_sth->execute($groupid);
> +
> +		while (my $ref = $objectgroup_sth->fetchrow_hashref()) {
> +		    $group->{and} = $ref->{value} if $ref->{name} eq 'and';
> +		    $group->{invert} = $ref->{value} if $ref->{name} eq 'invert';
> +		}
> +
>  		my $type = $type_map->{$gtype};
>  		push $self->{"$ruleid:$type"}->{groups}->@*, $group;
>  	    }

Re: [pmg-devel] [PATCH pmg-api 09/12] MailQueue: return maximum AID

[Stoiko Ivanov]

LGTM

Reviewed-by: Stoiko Ivanov <s.ivanov@proxmox.com>

On Fri,  9 Feb 2024 13:54:33 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> we'll need this in the what_matches to invert mark lists.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/MailQueue.pm    | 4 ++--
>  src/PMG/Utils.pm        | 2 ++
>  src/bin/pmg-smtp-filter | 3 ++-
>  3 files changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/src/PMG/MailQueue.pm b/src/PMG/MailQueue.pm
> index 8355c30..4e37cb9 100644
> --- a/src/PMG/MailQueue.pm
> +++ b/src/PMG/MailQueue.pm
> @@ -406,10 +406,10 @@ sub parse_mail {
>      # we also remove all proxmox-marks from the mail and add an unique
>      # id to each attachment.
>  
> -    PMG::Utils::remove_marks ($entity, 1);
> +    my $max_aid = PMG::Utils::remove_marks ($entity, 1);
>      PMG::Utils::add_ct_marks ($entity);
>  
> -    return $entity;
> +    return ($entity, $max_aid);
>  }
>  
>  sub decode_entities {
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index d9c9d2d..9f69e7f 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -186,6 +186,8 @@ sub remove_marks {
>  
>  	$id++;
>      });
> +
> +    return $id - 1; # return max AID
>  }
>  
>  sub subst_values {
> diff --git a/src/bin/pmg-smtp-filter b/src/bin/pmg-smtp-filter
> index 71043b0..86d633d 100755
> --- a/src/bin/pmg-smtp-filter
> +++ b/src/bin/pmg-smtp-filter
> @@ -648,7 +648,8 @@ sub handle_smtp {
>  
>  	my $maxfiles = $pmg_cfg->get('clamav', 'archivemaxfiles');
>  
> -	my $entity = $queue->parse_mail($maxfiles);
> +	my ($entity, $max_aid) = $queue->parse_mail($maxfiles);
> +	$msginfo->{max_aid} = $max_aid;
>  
>  	$self->log (3, "$queue->{logid}: new mail message-id=%s", $queue->{msgid});
>  

Re: [pmg-devel] [PATCH pmg-docs 2/2] rule system: explain new and mode and invert flag

[Stoiko Ivanov]

two suggestions in-line:
On Fri,  9 Feb 2024 13:54:38 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> and adding that the previous behaviour is now the default.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pmg-mail-filter.adoc | 39 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/pmg-mail-filter.adoc b/pmg-mail-filter.adoc
> index 57b8a15..48bf6c0 100644
> --- a/pmg-mail-filter.adoc
> +++ b/pmg-mail-filter.adoc
> @@ -63,7 +63,7 @@ Application of Rules
>  
>  When there is more than one object category or multiple objects configured
>  within a single rule, the following logic is used to determine if the rule
> -should be applied:
> +should be applied by default:
>  
>  * Within one category (WHAT/FROM/TO/WHEN), all objects are logical-or linked,
>    meaning that only one object of any one object group from the same category
> @@ -75,6 +75,43 @@ should be applied:
>  
>  When these conditions are met, all configured actions are executed.
>  
> +Alternatively, one can configure the 'mode' to 'any' (the default) or 'all' and
> +set 'invert' (default off) per object group and per object category for each
> +rule.
> +
> +When the mode is 'all' for a group, all objects within must match for the
> +object group to count as a match. This can be helpful when one wants to match
> +multiple conditions at the same time (e.g. file content-type and filename).
> +
> +When 'all' is set for a category of a rule, all object groups for that
> +type must match for the type to match.
> +
> +When 'invert' is active on a group, the original result of the group will
> +simply be inverted, so a match becomes a non-match and vice versa.
> +
> +The same is true for the object group types for rules.
> +
> +Special handling is done for WHAT matches that mark file parts (e.g. filename)
that mark mail parts instead of 'that mark file parts'?
> +since that is not a simple logical match, but could be a match for each
is not a simple yes/no match for the complete mail, but... 

> +part of the e-mail (e.g. attachments, or parts of a multi-part e-mail).
> +
> +So for WHAT match object groups, the 'mode' and 'invert' is applied to
> +the single parts of the e-mail, not the message as a whole.
> +
> +This means one has to be very careful with the 'invert' option, as previously
> +not matching parts, will match when using 'invert' (e.g. an inverted filename
> +matching will also mark non attachment parts of the mail).
> +
> +On the rule level, these marks of the parts will always be logical-or linked,
> +this way even more scenarios can be represented.
> +
> +To make it a bit easier to understand, the options are combined to a single
> +selection in the web ui:
> +
> +* Any must match => mode: 'any', invert: 'off'
> +* All must match => mode: 'all', invert: 'off'
> +* At least one must not match => mode: 'all', invert: 'on'
> +* None must match => mode: 'any', invert: 'on'
>  
>  [[pmg_mailfilter_action]]
>  'Action' - objects

[pmg-devel] applied: [PATCH pmg-api 01/12] RuleCache: remove unnecessary copying of marks

[Stoiko Ivanov]

applied this one - Thanks!

On Fri,  9 Feb 2024 13:54:25 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> two things that are wrong here
> * what_match_targets never returns a non empty list
> * we copy the list just returned just to append it to itself again
> 
> My guess is that we meant to copy the original list, not the just
> acquired one, and append it to the one just received. But that never did
> make a difference, since we only ever check for defined-ness on that
> exact list, and the only Object that this applies to (Spam) always
> returns an empty list with the spaminfo (so it's always defined in that
> case).
> 
> Since this was always the behavior AFAICT, just remove the unnecessary
> copy of the list for now. If we encounter any actual bugs with that, we
> can still implement it back in the right way (copy the original list).
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/RuleCache.pm | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
> index b8690ea..51d8a07 100644
> --- a/src/PMG/RuleCache.pm
> +++ b/src/PMG/RuleCache.pm
> @@ -322,9 +322,7 @@ sub what_match {
>  	    my $target_info;
>  	    if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
>  		foreach my $k (keys %$target_info) {
> -		    my $cmarks = $target_info->{$k}->{marks}; # make a copy
>  		    $res->{$k} = $target_info->{$k};
> -		    push @{$res->{$k}->{marks}}, @$cmarks if $cmarks;
>  		}
>  	    }
>  	}

[pmg-devel] applied: [PATCH pmg-api 02/12] RuleCache: reorganize to keep group structure

[Stoiko Ivanov]

applied, with 2 tiny typos in the commit-message fixed up
Thanks!

On Fri,  9 Feb 2024 13:54:26 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> Currently we 'or' combine all objects of a type (from/to/what/when)
> regardless of group, so we only keep a single list of all objects.
> 
> Since we want to introduce different logic (and/invert) we want to keep
> the configured group structure. This patch does this, wihtout chaning
> the current matching logic (still all 'or'-ed).
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/RuleCache.pm | 115 ++++++++++++++++++++++++-------------------
>  1 file changed, 64 insertions(+), 51 deletions(-)
> 
> diff --git a/src/PMG/RuleCache.pm b/src/PMG/RuleCache.pm
> index 51d8a07..fd22a16 100644
> --- a/src/PMG/RuleCache.pm
> +++ b/src/PMG/RuleCache.pm
> @@ -28,6 +28,14 @@ sub new {
>  
>      my $sha1 = Digest::SHA->new;
>  
> +    my $type_map =  {
> +	0 => "from",
> +	1 => "to",
> +	2 => "when",
> +	3 => "what",
> +	4 => "action",
> +    };
> +
>      eval {
>  	$dbh->begin_work;
>  
> @@ -53,7 +61,11 @@ sub new {
>  	    $sha1->add(join(',', $ref->{id}, $ref->{name}, $ref->{priority}, $ref->{active},
>  			    $ref->{direction}) . "|");
>  
> -	    my ($from, $to, $when, $what, $action);
> +	    $self->{"$ruleid:from"} = { groups => [] };
> +	    $self->{"$ruleid:to"} =  { groups => [] };
> +	    $self->{"$ruleid:when"} = { groups => [] };
> +	    $self->{"$ruleid:what"} = { groups => [] };
> +	    $self->{"$ruleid:action"} = { groups => [] };
>  
>  	    my $sth1 = $dbh->prepare(
>  		"SELECT Objectgroup_ID, Grouptype FROM RuleGroup " .
> @@ -64,20 +76,7 @@ sub new {
>  	    while (my $ref1 = $sth1->fetchrow_hashref()) {
>  		my $gtype = $ref1->{grouptype};
>  		my $groupid = $ref1->{objectgroup_id};
> -
> -		# emtyp groups differ from non-existent groups!
> -
> -		if ($gtype == 0) {      #from
> -		    $from = [] if !defined ($from);
> -		} elsif ($gtype == 1) { # to
> -		    $to = [] if !defined ($to);
> -		} elsif ($gtype == 2) { # when
> -		    $when = [] if !defined ($when);
> -		} elsif ($gtype == 3) { # what
> -		    $what = [] if !defined ($what);
> -		} elsif ($gtype == 4) { # action
> -		    $action = [] if !defined ($action);
> -		}
> +		my $objects = [];
>  
>  		my $sth2 = $dbh->prepare(
>  		    "SELECT ID FROM Object where Objectgroup_ID = '$groupid' " .
> @@ -90,14 +89,9 @@ sub new {
>  		    $sha1->add (join (',', $objid, $gtype, $groupid) . "|");
>  		    $sha1->add ($obj->{digest}, "|");
>  
> -		    if ($gtype == 0) {      #from
> -			push @$from, $obj;
> -		    } elsif ($gtype == 1) { # to
> -			push @$to,  $obj;
> -		    } elsif ($gtype == 2) { # when
> -			push @$when,  $obj;
> -		    } elsif ($gtype == 3) { # what
> -			push @$what,  $obj;
> +		    push @$objects, $obj;
> +
> +		    if ($gtype == 3) { # what
>  			if ($obj->otype == PMG::RuleDB::ArchiveFilter->otype ||
>  			    $obj->otype == PMG::RuleDB::MatchArchiveFilename->otype)
>  			{
> @@ -111,20 +105,20 @@ sub new {
>  			    }
>  			}
>  		    } elsif ($gtype == 4) { # action
> -			push @$action, $obj;
>  			$self->{"$ruleid:final"} = 1 if $obj->final();
>  		    }
>  		}
>  		$sth2->finish();
> +
> +		my $group = {
> +		    objects => $objects,
> +		};
> +
> +		my $type = $type_map->{$gtype};
> +		push $self->{"$ruleid:$type"}->{groups}->@*, $group;
>  	    }
>  
>  	    $sth1->finish();
> -
> -	    $self->{"$ruleid:from"} = $from;
> -	    $self->{"$ruleid:to"} =  $to;
> -	    $self->{"$ruleid:when"} = $when;
> -	    $self->{"$ruleid:what"} = $what;
> -	    $self->{"$ruleid:action"} = $action;
>  	}
>  
>  	# Cache Greylist Exclusion
> @@ -203,7 +197,15 @@ sub get_actions {
>  
>      defined($ruleid) || die "undefined rule id: ERROR";
>  
> -    return $self->{"$ruleid:action"};
> +    my $actions = $self->{"$ruleid:action"};
> +
> +    return undef if scalar($actions->{groups}->@*) == 0;
> +
> +    my $res = [];
> +    for my $action ($actions->{groups}->@*) {
> +	push $res->@*, $action->{objects}->@*;
> +    }
> +    return $res;
>  }
>  
>  sub greylist_match {
> @@ -239,15 +241,17 @@ sub from_match {
>  
>      my $from = $self->{"$ruleid:from"};
>  
> -    return 1 if !defined ($from);
> +    return 1 if scalar($from->{groups}->@*) == 0;
>  
>      # postfix prefixes ipv6 addresses with IPv6:
>      if (defined($ip) && $ip =~ /^IPv6:(.*)/) {
>  	$ip = $1;
>      }
>  
> -    foreach my $obj (@$from) {
> -	return 1 if $obj->who_match($addr, $ip, $ldap);
> +    for my $group ($from->{groups}->@*) {
> +	for my $obj ($group->{objects}->@*) {
> +	    return 1 if $obj->who_match($addr, $ip, $ldap);
> +	}
>      }
>  
>      return 0;
> @@ -258,12 +262,15 @@ sub to_match {
>  
>      my $to = $self->{"$ruleid:to"};
>  
> -    return 1 if !defined ($to);
> +    return 1 if scalar($to->{groups}->@*) == 0;
>  
> -    foreach my $obj (@$to) {
> -	return 1 if $obj->who_match($addr, undef, $ldap);
> +    for my $group ($to->{groups}->@*) {
> +	for my $obj ($group->{objects}->@*) {
> +	    return 1 if $obj->who_match($addr, undef, $ldap);
> +	}
>      }
>  
> +
>      return 0;
>  }
>  
> @@ -272,10 +279,12 @@ sub when_match {
>  
>      my $when = $self->{"$ruleid:when"};
>  
> -    return 1 if !defined ($when);
> +    return 1 if scalar($when->{groups}->@*) == 0;
>  
> -    foreach my $obj (@$when) {
> -	return 1 if $obj->when_match($time);
> +    for my $group ($when->{groups}->@*) {
> +	for my $obj ($group->{objects}->@*) {
> +	    return 1 if $obj->when_match($time);
> +	}
>      }
>  
>      return 0;
> @@ -292,7 +301,7 @@ sub what_match {
>      # $res->{$target}->{marks} is only used in apply_rules() to exclude some
>      # targets (spam blacklist and whitelist)
>  
> -    if (!defined ($what)) {
> +    if (scalar($what->{groups}->@*) == 0) {
>  	# match all targets
>  	foreach my $target (@{$msginfo->{targets}}) {
>  	    $res->{$target}->{marks} = [];
> @@ -304,10 +313,12 @@ sub what_match {
>  
>      my $marks;
>  
> -    foreach my $obj (@$what) {
> -	if (!$obj->can('what_match_targets')) {
> -	    if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
> -		push @$marks, @$match;
> +    for my $group ($what->{groups}->@*) {
> +	for my $obj ($group->{objects}->@*) {
> +	    if (!$obj->can('what_match_targets')) {
> +		if (my $match = $obj->what_match($queue, $element, $msginfo, $dbh)) {
> +		    push @$marks, @$match;
> +		}
>  	    }
>  	}
>      }
> @@ -317,12 +328,14 @@ sub what_match {
>  	$res->{marks} = $marks;
>      }
>  
> -    foreach my $obj (@$what) {
> -	if ($obj->can ("what_match_targets")) {
> -	    my $target_info;
> -	    if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
> -		foreach my $k (keys %$target_info) {
> -		    $res->{$k} = $target_info->{$k};
> +    for my $group ($what->{groups}->@*) {
> +	for my $obj ($group->{objects}->@*) {
> +	    if ($obj->can ("what_match_targets")) {
> +		my $target_info;
> +		if ($target_info = $obj->what_match_targets($queue, $element, $msginfo, $dbh)) {
> +		    foreach my $k (keys %$target_info) {
> +			$res->{$k} = $target_info->{$k};
> +		    }
>  		}
>  	    }
>  	}

[pmg-devel] applied: [PATCH pmg-docs 1/2] rule system: add a small section about matching rules

[Stoiko Ivanov]

Thanks for the patch! applied!

On Fri,  9 Feb 2024 13:54:37 +0100
Dominik Csapak <d.csapak@proxmox.com> wrote:

> this section explains how multiple object categories and multiple
> objects interact with the rule matching.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pmg-mail-filter.adoc | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/pmg-mail-filter.adoc b/pmg-mail-filter.adoc
> index 3aafe4c..57b8a15 100644
> --- a/pmg-mail-filter.adoc
> +++ b/pmg-mail-filter.adoc
> @@ -58,6 +58,23 @@ You can also disable a rule completely, which is mostly useful for
>  testing and debugging. The 'Factory Defaults' button allows you to
>  reset the filter rules.
>  
> +Application of Rules
> +--------------------
> +
> +When there is more than one object category or multiple objects configured
> +within a single rule, the following logic is used to determine if the rule
> +should be applied:
> +
> +* Within one category (WHAT/FROM/TO/WHEN), all objects are logical-or linked,
> +  meaning that only one object of any one object group from the same category
> +  has to match for the whole category to match.
> +
> +* FROM/TO/WHAT/WHEN category match results are logical-and linked, so all
> +  categories that have at least one object in them must match for the rule to
> +  match.
> +
> +When these conditions are met, all configured actions are executed.
> +
>  
>  [[pmg_mailfilter_action]]
>  'Action' - objects