From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id ED09E93847 for ; Tue, 20 Feb 2024 13:36:29 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D02226B86 for ; Tue, 20 Feb 2024 13:35:59 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Tue, 20 Feb 2024 13:35:58 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 8176F43F83 for ; Tue, 20 Feb 2024 13:35:58 +0100 (CET) Date: Tue, 20 Feb 2024 13:35:56 +0100 From: Stoiko Ivanov To: Dominik Csapak Cc: pmg-devel@lists.proxmox.com Message-ID: <20240220133556.76ef39fa@rosa.proxmox.com> In-Reply-To: <20240209125440.2572239-6-d.csapak@proxmox.com> References: <20240209125440.2572239-1-d.csapak@proxmox.com> <20240209125440.2572239-6-d.csapak@proxmox.com> X-Mailer: Claws Mail 4.1.1 (GTK 3.24.38; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.086 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [objectgrouphelpers.pm, dbtools.pm, ruledb.pm, objectgroup.id] Subject: Re: [pmg-devel] [PATCH pmg-api 05/12] add objectgroup attributes and/invert X-BeenThere: pmg-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Mail Gateway development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 20 Feb 2024 12:36:30 -0000 afaict deletion of objectgroup attributes when deleting the object group is missing On Fri, 9 Feb 2024 13:54:29 +0100 Dominik Csapak 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 > --- > 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 = $attributfetchrow_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; > }