[pmg-devel] [PATCH pmg-api v2 0/5] allow / in local part of pmg-email-address

[pmg-devel] [PATCH pmg-api v2 0/5] allow / in local part of pmg-email-address

[Stoiko Ivanov]

v1->v2:
* Had a chat with Thomas off-list - who (righfully) pointed out that the paths
'contact' and 'contacts' seem a bit shoddy and counterintuitive
* reimplemented the change so that it still uses '/contact' as prefix,
but instead of providing the address as next component uses 'detail' (and
provides the address in the detailaddress parameter)
(the two points above apply to 'sender' and 'receiver' as well)

* individual notes below the individual patches (where applicable)

original cover-letter:
The following patchset is a result of investigating a thread in our
community forum [0].

While I'd consider e-mail addresses with '/' in the local-part quite odd,
as so often smtp RFCs are quite liberal with what smtp servers (have to)
accept. Postfix happily accepts mails with '/' in the local-part, and
should they end up in the quarantine, it is currently not possible to
remove them (short of waiting for quarantine retention period).

the 4 places where we use the pmg-email-address format are:
* quarantine (pmail parameter)
* statistics (contact, receiver, sender detail views)
* fetchmail
* pbsconfig (username)

the first two are problematic in the sense that external sources might
cause a mail-address with '/' to get stored. pbsconfig stores a username
with '/' (which then fails since the repository verification does not
expect a '/' in the username/token), fetchmail accepts it (and stores its
config with all special characters escaped)
only the statistics api calls are problematic, since the detail views pass
the mail-address as path component (and the decoding inside pve-http-server
breaks the api call resolution) - addressing this is the main part of the
patchset.

It follows a similar series by Dominik for the user blocklists [1].

patch 1/5 for the api is a cleanup that caught my eye
patches 2+3 for the api could probably be squashed (happy to send a v2 for
this - but feel free to squash them if this gets applied as is)

tested it a bit on my setup (with a limited set of addresses in the
statistics database).

[0]
https://forum.proxmox.com/threads/pmg-error-parameter-verification-failed-400.82353/
[1] https://lists.proxmox.com/pipermail/pmg-devel/2020-March/000952.html

pmg-api:
Stoiko Ivanov (5):
  api: statistics: remove unneeded RESTEnvironment
  api: statistics: refactor return for detail calls
  api: statistics: refactor detail calls
  api: statistics: make email a parameter
  utils: allow '/' inside email address localpart

 src/PMG/API2/Statistics.pm | 278 ++++++++++++++++---------------------
 src/PMG/Utils.pm           |   2 +-
 2 files changed, 122 insertions(+), 158 deletions(-)

pmg-gui:
Stoiko Ivanov (1):
  statistics: use new api structure for detailed stats

 js/ContactStatistics.js  |  9 ++++-----
 js/ReceiverStatistics.js |  9 ++++-----
 js/SenderStatistics.js   |  9 ++++-----
 js/StatStore.js          | 11 +++++++++--
 4 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2 1/5] api: statistics: remove unneeded RESTEnvironment

[Stoiko Ivanov]

none of the API calls in PMG::API2::Statistics use the
RESTEnvironment - so remove the unused code.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/Statistics.pm | 34 ----------------------------------
 1 file changed, 34 deletions(-)

diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
index 7eb8d18..adc7650 100644
--- a/src/PMG/API2/Statistics.pm
+++ b/src/PMG/API2/Statistics.pm
@@ -238,9 +238,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $cfg = PMG::Config->new();
@@ -383,9 +380,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -530,9 +524,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $cfg = PMG::Config->new();
@@ -684,9 +675,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -791,9 +779,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -910,8 +895,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-
 	my $hours = $param->{hours} // 12;
 	my $span = $param->{timespan} // 1800;
 
@@ -972,8 +955,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-
 	my $hours = $param->{hours} // 12;
 
 	my $limit = $param->{limit} // 5;
@@ -1071,9 +1052,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $span = $param->{timespan} // 3600;
@@ -1138,9 +1116,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -1185,9 +1160,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -1284,9 +1256,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $stat = PMG::Statistic->new($start, $end);
@@ -1345,9 +1314,6 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
 	my ($start, $end) = $extract_start_end->($param);
 
 	my $span = $param->{timespan} // 3600;
-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2 2/5] api: statistics: refactor return for detail calls

[Stoiko Ivanov]

all api methods returning information for a particular sender,
receiver or contact have similar returns.

This commit pulls the common ones out into a sub like the common method
parameters in $default_properties.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
v1->v2:
* keep the common properties in a sub instead of a hash (for consistency
  with $default_properties)
 src/PMG/API2/Statistics.pm | 109 ++++++++++++++-----------------------
 1 file changed, 40 insertions(+), 69 deletions(-)

diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
index adc7650..064865d 100644
--- a/src/PMG/API2/Statistics.pm
+++ b/src/PMG/API2/Statistics.pm
@@ -257,6 +257,40 @@ __PACKAGE__->register_method ({
 	return $res;
     }});
 
+my $detail_return_properties = sub {
+    my ($prop) = @_;
+
+    $prop //= {};
+
+    $prop->{time} = {
+	description => "Receive time stamp",
+	type => 'integer',
+    };
+
+    $prop->{bytes} = {
+	description => "Mail traffic (Bytes).",
+	type => 'number',
+    };
+
+    $prop->{blocked} = {
+	description => "Mail was blocked.",
+	type => 'boolean',
+    };
+
+    $prop->{spamlevel} = {
+	description => "Spam score.",
+	type => 'number',
+    };
+
+    $prop->{virusinfo} = {
+	description => "Virus name.",
+	type => 'string',
+	optional => 1,
+    };
+
+    return $prop;
+};
+
 __PACKAGE__->register_method ({
     name => 'contactdetails',
     path => 'contact/{contact}',
@@ -282,33 +316,12 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		time => {
-		    description => "Receive time stamp",
-		    type => 'integer',
-		},
+	    properties => $detail_return_properties->({
 		sender => {
 		    description => "Sender email.",
 		    type => 'string',
 		},
-		bytes => {
-		    description => "Mail traffic (Bytes).",
-		    type => 'number',
-		},
-		blocked => {
-		    description => "Mail was blocked.",
-		    type => 'boolean',
-		},
-		spamlevel => {
-		    description => "Spam score.",
-		    type => 'number',
-		},
-		virusinfo => {
-		    description => "Virus name.",
-		    type => 'string',
-		    optional => 1,
-		},
-	    },
+	    }),
 	},
     },
     code => sub {
@@ -421,33 +434,12 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		time => {
-		    description => "Receive time stamp",
-		    type => 'integer',
-		},
+	    properties => $detail_return_properties->({
 		receiver => {
 		    description => "Receiver email.",
 		    type => 'string',
 		},
-		bytes => {
-		    description => "Mail traffic (Bytes).",
-		    type => 'number',
-		},
-		blocked => {
-		    description => "Mail was blocked.",
-		    type => 'boolean',
-		},
-		spamlevel => {
-		    description => "Spam score.",
-		    type => 'number',
-		},
-		virusinfo => {
-		    description => "Virus name.",
-		    type => 'string',
-		    optional => 1,
-		},
-	    },
+	    }),
 	},
     },
     code => sub {
@@ -568,33 +560,12 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		time => {
-		    description => "Receive time stamp",
-		    type => 'integer',
-		},
+	    properties => $detail_return_properties->({
 		sender => {
 		    description => "Sender email.",
 		    type => 'string',
 		},
-		bytes => {
-		    description => "Mail traffic (Bytes).",
-		    type => 'number',
-		},
-		blocked => {
-		    description => "Mail was blocked.",
-		    type => 'boolean',
-		},
-		spamlevel => {
-		    description => "Spam score.",
-		    type => 'number',
-		},
-		virusinfo => {
-		    description => "Virus name.",
-		    type => 'string',
-		    optional => 1,
-		},
-	    },
+	    }),
 	},
     },
     code => sub {
-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2 3/5] api: statistics: refactor detail calls

[Stoiko Ivanov]

the API calls returning the detailed statistics for a particular
email use much common code.
This patch introduces a sub to be used in all detail api calls.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/API2/Statistics.pm | 82 ++++++++++++++++----------------------
 1 file changed, 34 insertions(+), 48 deletions(-)

diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
index 064865d..72bfeb5 100644
--- a/src/PMG/API2/Statistics.pm
+++ b/src/PMG/API2/Statistics.pm
@@ -291,6 +291,37 @@ my $detail_return_properties = sub {
     return $prop;
 };
 
+sub get_detail_statistics {
+    my ($type, $param) =@_;
+
+    my ($start, $end) = $extract_start_end->($param);
+
+    my $stat = PMG::Statistic->new($start, $end);
+    my $rdb = PMG::RuleDB->new();
+
+    my $sorters = [];
+    if ($param->{orderby}) {
+	my $props = ['time', 'sender', 'bytes', 'blocked', 'spamlevel', 'virusinfo'];
+	$props->[1] = 'receiver' if $type eq 'sender';
+	$sorters = $decode_orderby->($param->{orderby}, $props);
+    }
+
+    my $res = [];
+    if ($type eq 'contact') {
+	$res = $stat->user_stat_contact_details(
+	    $rdb, $param->{contact}, $userstat_limit, $sorters, $param->{filter});
+    } elsif ($type eq 'sender') {
+	$res = $stat->user_stat_sender_details(
+	    $rdb, $param->{sender}, $userstat_limit, $sorters, $param->{filter});
+    } elsif ($type eq 'receiver') {
+	$res = $stat->user_stat_receiver_details(
+	    $rdb, $param->{receiver}, $userstat_limit, $sorters, $param->{filter});
+    } else {
+	die "invalid type provided (not 'contact', 'sender', 'receiver')\n";
+    }
+    return $res;
+}
+
 __PACKAGE__->register_method ({
     name => 'contactdetails',
     path => 'contact/{contact}',
@@ -327,22 +358,7 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
-	my ($start, $end) = $extract_start_end->($param);
-
-	my $stat = PMG::Statistic->new($start, $end);
-	my $rdb = PMG::RuleDB->new();
-
-	my $sorters = [];
-	if ($param->{orderby}) {
-	    my $props = ['time', 'sender', 'bytes', 'blocked', 'spamlevel', 'virusinfo'];
-	    $sorters = $decode_orderby->($param->{orderby}, $props);
-	}
-
-	return $stat->user_stat_contact_details(
-	    $rdb, $param->{contact}, $userstat_limit, $sorters, $param->{filter});
+	return get_detail_statistics('contact', $param);
     }});
 
 __PACKAGE__->register_method ({
@@ -445,22 +461,7 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
-	my ($start, $end) = $extract_start_end->($param);
-
-	my $stat = PMG::Statistic->new($start, $end);
-	my $rdb = PMG::RuleDB->new();
-
-	my $sorters = [];
-	if ($param->{orderby}) {
-	    my $props = ['time', 'receiver', 'bytes', 'blocked', 'spamlevel', 'virusinfo'];
-	    $sorters = $decode_orderby->($param->{orderby}, $props);
-	}
-
-	return $stat->user_stat_sender_details(
-	    $rdb, $param->{sender}, $userstat_limit, $sorters, $param->{filter});
+	return get_detail_statistics('sender', $param);
     }});
 
 __PACKAGE__->register_method ({
@@ -571,22 +572,7 @@ __PACKAGE__->register_method ({
     code => sub {
 	my ($param) = @_;
 
-	my $restenv = PMG::RESTEnvironment->get();
-	my $cinfo = $restenv->{cinfo};
-
-	my ($start, $end) = $extract_start_end->($param);
-
-	my $stat = PMG::Statistic->new($start, $end);
-	my $rdb = PMG::RuleDB->new();
-
-	my $sorters = [];
-	if ($param->{orderby}) {
-	    my $props = ['time', 'sender', 'bytes', 'blocked', 'spamlevel', 'virusinfo'];
-	    $sorters = $decode_orderby->($param->{orderby}, $props);
-	}
-
-	return $stat->user_stat_receiver_details(
-	    $rdb, $param->{receiver}, $userstat_limit, $sorters, $param->{filter});
+	return get_detail_statistics('receiver', $param);
     }});
 
 __PACKAGE__->register_method ({
-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2] api: statistics: make email a parameter

[Stoiko Ivanov]

This patch changes the semantics for the detailed contact, sender and
receiver statistics call:
additionally to the current way of providing the e-mail address as
path component, it is now also possible to provide 'detail' as path
and the address in the additional parameter 'detailaddress'.

This allows the statistics to be displayed for addresses containing
'/' in their localpart, once this is permitted in our api schema.

the idea follows a similar change for the user blocklists in
e8d909c11faeb5a4f84f39ef50e0eaf8ea65046d

The verification of the paramter now happens inside the common sub
for detail statistics instead of the API decomposer.

Backward compatibility is ensured, since the only additional value
'detail' was not allowed as value for 'pmg-email-address'.

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
v1->v2:
* instead of adding the new paths contacts, senders, receivers in addition
  to their singular counterparts, handle the detail calls in the old path
  with the special value 'detail' (instead of an e-mail address)

 src/PMG/API2/Statistics.pm | 59 ++++++++++++++++++++++++++++++++------
 1 file changed, 50 insertions(+), 9 deletions(-)
 src/PMG/API2/Statistics.pm | 58 ++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
index 72bfeb5..732a3b4 100644
--- a/src/PMG/API2/Statistics.pm
+++ b/src/PMG/API2/Statistics.pm
@@ -294,6 +294,19 @@ my $detail_return_properties = sub {
 sub get_detail_statistics {
     my ($type, $param) =@_;
 
+    my $detail_address;
+    if ($param->{$type} eq 'detail') {
+	if (!defined($param->{detailaddress})) {
+	    raise_param_exc({ $type => 'need detailaddress'});
+	}
+	$detail_address = $param->{detailaddress};
+    } else {
+	$detail_address = $param->{$type};
+    }
+
+    PVE::JSONSchema::validate($detail_address, get_standard_option('pmg-email-address'),
+	'Address parameter verification failed');
+
     my ($start, $end) = $extract_start_end->($param);
 
     my $stat = PMG::Statistic->new($start, $end);
@@ -309,19 +322,22 @@ sub get_detail_statistics {
     my $res = [];
     if ($type eq 'contact') {
 	$res = $stat->user_stat_contact_details(
-	    $rdb, $param->{contact}, $userstat_limit, $sorters, $param->{filter});
+	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
     } elsif ($type eq 'sender') {
 	$res = $stat->user_stat_sender_details(
-	    $rdb, $param->{sender}, $userstat_limit, $sorters, $param->{filter});
+	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
     } elsif ($type eq 'receiver') {
 	$res = $stat->user_stat_receiver_details(
-	    $rdb, $param->{receiver}, $userstat_limit, $sorters, $param->{filter});
+	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
     } else {
 	die "invalid type provided (not 'contact', 'sender', 'receiver')\n";
     }
     return $res;
 }
 
+# FIXME: change for PMG 7.0 - remove support for providing addresses as path
+# addresses can contain '/' which breaks API path resolution - hardcode
+# 'detail' for the path pattern.
 __PACKAGE__->register_method ({
     name => 'contactdetails',
     path => 'contact/{contact}',
@@ -331,8 +347,14 @@ __PACKAGE__->register_method ({
     parameters => {
 	additionalProperties => 0,
 	properties => $default_properties->({
-	    contact => get_standard_option('pmg-email-address', {
-		description => "Contact email address.",
+	    contact => {
+		type => 'string',
+		description => "'detail', with the address provided as 'detailaddress' or"
+		. "the address - the latter is deprecated.",
+	    },
+	    detailaddress => get_standard_option('pmg-email-address', {
+		description => "The e-mail address if contact is 'detail'.",
+		optional => 1,
 	    }),
 	    filter => {
 		description => "Sender address filter.",
@@ -425,6 +447,9 @@ __PACKAGE__->register_method ({
 	return $res;
     }});
 
+# FIXME: change for PMG 7.0 - remove support for providing addresses as path
+# addresses can contain '/' which breaks API path resolution - hardcode
+# 'detail' for the path pattern.
 __PACKAGE__->register_method ({
     name => 'senderdetails',
     path => 'sender/{sender}',
@@ -434,8 +459,14 @@ __PACKAGE__->register_method ({
     parameters => {
 	additionalProperties => 0,
 	properties => $default_properties->({
-	    sender => get_standard_option('pmg-email-address', {
-		description => "Sender email address.",
+	    sender => {
+		type => 'string',
+		description => "'detail', with the address provided as 'detailaddress' or"
+		. "the address - the latter is deprecated.",
+	    },
+	    detailaddress => get_standard_option('pmg-email-address', {
+		description => "The e-mail address if sender is 'detail'.",
+		optional => 1,
 	    }),
 	    filter => {
 		description => "Receiver address filter.",
@@ -536,6 +567,9 @@ __PACKAGE__->register_method ({
 	return $res;
     }});
 
+# FIXME: change for PMG 7.0 - remove support for providing addresses as path
+# addresses can contain '/' which breaks API path resolution - hardcode
+# 'detail' for the path pattern.
 __PACKAGE__->register_method ({
     name => 'receiverdetails',
     path => 'receiver/{receiver}',
@@ -545,8 +579,14 @@ __PACKAGE__->register_method ({
     parameters => {
 	additionalProperties => 0,
 	properties => $default_properties->({
-	    receiver => get_standard_option('pmg-email-address', {
-		description => "Receiver email address.",
+	    receiver => {
+		type => 'string',
+		description => "'detail', with the address provided as 'detailaddress' or"
+		. "the address - the latter is deprecated.",
+	    },
+	    detailaddress => get_standard_option('pmg-email-address', {
+		description => "The e-mail address if contact is 'detail'.",
+		optional => 1,
 	    }),
 	    filter => {
 		description => "Sender address filter.",
-- 
2.20.1

[pmg-devel] [PATCH pmg-api v2 5/5] utils: allow '/' inside email address localpart

[Stoiko Ivanov]

The change is motivated by a report in our community forum [0], where
a mail addressed to an address containing '/' in its local-part ended
up in the quarantine.
This is permitted by RFC5322 ([1]), and, probably more relevant,
happily accepted and processed by postfix.

Once inside the quarantine (or the statistic database) the records cannot
be displayed (due to the parameter verification failure).

This leaves the user unable to delete the quarantined mail.

Apart from the quarantine and statistics the 'pmg-email-address'
format is only used in the PBSConfig and the fetchmail configuration
(both of which are available only to the admin and can be still be
edited irrespective of the set localpart).

[0]
https://forum.proxmox.com/threads/pmg-error-parameter-verification-failed-400.82353/
[1] https://tools.ietf.org/html/rfc5322

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
 src/PMG/Utils.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
index 149bcdc..4341dbd 100644
--- a/src/PMG/Utils.pm
+++ b/src/PMG/Utils.pm
@@ -117,7 +117,7 @@ PVE::JSONSchema::register_standard_option('username', {
 PVE::JSONSchema::register_standard_option('pmg-email-address', {
     description => "Email Address (allow most characters).",
     type => 'string',
-    pattern => '(?:[^\s\/\\\@]+\@[^\s\/\\\@]+)',
+    pattern => '(?:[^\s\\\@]+\@[^\s\/\\\@]+)',
     maxLength => 512,
     minLength => 3,
 });
-- 
2.20.1

[pmg-devel] [PATCH pmg-gui v2 1/1] statistics: use new api structure for detailed stats

[Stoiko Ivanov]

the detail api calls take the address for which to display the statistics
as explicit parameter instead of component of the path if the path
component is set to 'detail'.

This makes it possible to accept '/' as part of an e-mail address
which is allowed (in the local-part by RFC5322 [0], and accepted by
postfix.

[0] https://tools.ietf.org/html/rfc5322

Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
---
v1->v2:
* adapt to new api-structure
* still pass url as parameter - passing undefined is handled specially
  (huge thanks to Dominik for noticing this after a 2 second glance :)

 js/ContactStatistics.js  |  9 ++++-----
 js/ReceiverStatistics.js |  9 ++++-----
 js/SenderStatistics.js   |  9 ++++-----
 js/StatStore.js          | 11 +++++++++--
 4 files changed, 21 insertions(+), 17 deletions(-)

diff --git a/js/ContactStatistics.js b/js/ContactStatistics.js
index 6c9d2d4..8fc58b8 100644
--- a/js/ContactStatistics.js
+++ b/js/ContactStatistics.js
@@ -16,10 +16,10 @@ Ext.define('PMG.ContactDetails', {
 
     plugins: 'gridfilters',
 
-    setUrl: function(url, title) {
+    setUrl: function(url, detailaddress, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, { detailaddress: detailaddress });
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -201,9 +201,8 @@ Ext.define('PMG.ContactStatistics', {
 	    var details = this.lookupReference('details');
 	    if (selected.length > 0) {
 		var contact = selected[0].data.contact;
-		var url = "/api2/json/statistics/contact/" +
-		    encodeURIComponent(contact);
-		details.setUrl(url, '<b>' + gettext('Contact') + ':</b> ' + Ext.htmlEncode(contact));
+		var url = "/api2/json/statistics/contact/detail";
+		details.setUrl(url, contact, '<b>' + gettext('Contact') + ':</b> ' + Ext.htmlEncode(contact));
 	    } else {
 		details.setUrl();
 	    }
diff --git a/js/ReceiverStatistics.js b/js/ReceiverStatistics.js
index 6378e06..069c2bf 100644
--- a/js/ReceiverStatistics.js
+++ b/js/ReceiverStatistics.js
@@ -16,10 +16,10 @@ Ext.define('PMG.ReceiverDetails', {
 
     plugins: 'gridfilters',
 
-    setUrl: function(url, title) {
+    setUrl: function(url, detailaddress, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, { detailaddress: detailaddress });
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -212,10 +212,9 @@ Ext.define('PMG.ReceiverStatistics', {
 	selectionChange: function(grid, selected, eOpts) {
 	    var details = this.lookupReference('details');
 	    if (selected.length > 0) {
+		var url = "/api2/json/statistics/receiver/detail";
 		var receiver = selected[0].data.receiver;
-		var url = "/api2/json/statistics/receiver/" +
-		    encodeURIComponent(receiver);
-		details.setUrl(url, '<b>' + gettext('Receiver') + ':</b> ' + Ext.htmlEncode(receiver));
+		details.setUrl(url, receiver, '<b>' + gettext('Receiver') + ':</b> ' + Ext.htmlEncode(receiver));
 	    } else {
 		details.setUrl();
 	    }
diff --git a/js/SenderStatistics.js b/js/SenderStatistics.js
index 43c5438..9b9f48d 100644
--- a/js/SenderStatistics.js
+++ b/js/SenderStatistics.js
@@ -16,10 +16,10 @@ Ext.define('PMG.SenderDetails', {
 
     plugins: 'gridfilters',
 
-    setUrl: function(url, title) {
+    setUrl: function(url, detailaddress, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, { detailaddress: detailaddress });
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -201,9 +201,8 @@ Ext.define('PMG.SenderStatistics', {
 	    var details = this.lookupReference('details');
 	    if (selected.length > 0) {
 		var sender = selected[0].data.sender;
-		var url = "/api2/json/statistics/sender/" +
-		    encodeURIComponent(sender);
-		details.setUrl(url, '<b>' + gettext('Sender') + ':</b> ' + Ext.htmlEncode(sender));
+		var url = "/api2/json/statistics/sender/detail";
+		details.setUrl(url, sender, '<b>' + gettext('Sender') + ':</b> ' + Ext.htmlEncode(sender));
 	    } else {
 		details.setUrl();
 	    }
diff --git a/js/StatStore.js b/js/StatStore.js
index ec11777..0fc1d31 100644
--- a/js/StatStore.js
+++ b/js/StatStore.js
@@ -8,13 +8,17 @@ Ext.define('PMG.data.StatStore', {
 
     includeTimeSpan: false,
 
-    setUrl: function(url) {
+    setUrl: function(url, extraparam) {
 	var me = this;
 
 	me.proxy.abort(); // abort pending requests
 
 	me.staturl = url;
 	me.proxy.extraParams = {};
+	if (extraparam !== undefined) {
+	    me.proxy.extraParams = extraparam;
+	}
+
 	me.setData([]);
     },
 
@@ -38,7 +42,10 @@ Ext.define('PMG.data.StatStore', {
 	}
 
 	me.proxy.url = me.staturl;
-	me.proxy.extraParams = { starttime: ts.starttime, endtime: ts.endtime };
+	Ext.apply(me.proxy.extraParams, {
+	    starttime: ts.starttime,
+	    endtime: ts.endtime,
+	});
 
 	var timespan = 3600;
 	if (me.includeTimeSpan) {
-- 
2.20.1

[pmg-devel] applied: [PATCH pmg-api v2 1/5] api: statistics: remove unneeded RESTEnvironment

[Thomas Lamprecht]

On 21.01.21 16:51, Stoiko Ivanov wrote:
> none of the API calls in PMG::API2::Statistics use the
> RESTEnvironment - so remove the unused code.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/Statistics.pm | 34 ----------------------------------
>  1 file changed, 34 deletions(-)
> 
>

applied, thanks!

[pmg-devel] applied: [PATCH pmg-api v2 2/5] api: statistics: refactor return for detail calls

[Thomas Lamprecht]

On 21.01.21 16:51, Stoiko Ivanov wrote:
> all api methods returning information for a particular sender,
> receiver or contact have similar returns.
> 
> This commit pulls the common ones out into a sub like the common method
> parameters in $default_properties.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> v1->v2:
> * keep the common properties in a sub instead of a hash (for consistency
>   with $default_properties)
>  src/PMG/API2/Statistics.pm | 109 ++++++++++++++-----------------------
>  1 file changed, 40 insertions(+), 69 deletions(-)
> 
>

applied, thanks!

[pmg-devel] applied: [PATCH pmg-api v2 3/5] api: statistics: refactor detail calls

[Thomas Lamprecht]

On 21.01.21 16:51, Stoiko Ivanov wrote:
> the API calls returning the detailed statistics for a particular
> email use much common code.
> This patch introduces a sub to be used in all detail api calls.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/Statistics.pm | 82 ++++++++++++++++----------------------
>  1 file changed, 34 insertions(+), 48 deletions(-)
> 
>

applied, thanks!

Re: [pmg-devel] [PATCH pmg-api v2] api: statistics: make email a parameter

[Thomas Lamprecht]

On 21.01.21 16:51, Stoiko Ivanov wrote:
> This patch changes the semantics for the detailed contact, sender and
> receiver statistics call:
> additionally to the current way of providing the e-mail address as
> path component, it is now also possible to provide 'detail' as path
> and the address in the additional parameter 'detailaddress'.
> 
> This allows the statistics to be displayed for addresses containing
> '/' in their localpart, once this is permitted in our api schema.

hmm, but this won't really work, or at least play nicely, when trying
to deprecate the old API path {template} and mount /detail there as
the index of the 'receiver', 'sender'  and 'contact' API endpoints still
needs to return unfiltered, undetailed statistics for all senders.

Could be hacked in, would only work with keeping a dynamic {templated}
API path as else those indexes would need to return a pseudo entry for
linking to the /detail sub-endpoint.

Alternative to the old two proposals:

(1) Add new API endpoint "detailed" API endpoint with the types as
child-endpoint, or maybe even better as common parameter, as then only
one new API point would be required.

(2) move now also the unfiltered info a level down, i.e.:

         /sender
             /detailed
             /all
         /receiver
             /detailed
             /all
         /contact
             /detailed
             /all

This way we can move the parent sender, receiver, contact endpoints
to return statically the /all and /detailed children hrefs and the
children be leaf-nodes and just return data.

Both are about the same work and in general fine to me. The decision is
IMO only over what fits the rest of the API better and deemed to be
Better/Nicer/Cleaner Way™.


> the idea follows a similar change for the user blocklists in
> e8d909c11faeb5a4f84f39ef50e0eaf8ea65046d
> 
> The verification of the paramter now happens inside the common sub
> for detail statistics instead of the API decomposer.
> 
> Backward compatibility is ensured, since the only additional value
> 'detail' was not allowed as value for 'pmg-email-address'.
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
> v1->v2:
> * instead of adding the new paths contacts, senders, receivers in addition
>   to their singular counterparts, handle the detail calls in the old path
>   with the special value 'detail' (instead of an e-mail address)
> 
>  src/PMG/API2/Statistics.pm | 59 ++++++++++++++++++++++++++++++++------
>  1 file changed, 50 insertions(+), 9 deletions(-)
>  src/PMG/API2/Statistics.pm | 58 ++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
> index 72bfeb5..732a3b4 100644
> --- a/src/PMG/API2/Statistics.pm
> +++ b/src/PMG/API2/Statistics.pm
> @@ -294,6 +294,19 @@ my $detail_return_properties = sub {
>  sub get_detail_statistics {
>      my ($type, $param) =@_;
>  
> +    my $detail_address;
> +    if ($param->{$type} eq 'detail') {
> +	if (!defined($param->{detailaddress})) {
> +	    raise_param_exc({ $type => 'need detailaddress'});
> +	}
> +	$detail_address = $param->{detailaddress};
> +    } else {
> +	$detail_address = $param->{$type};
> +    }
> +
> +    PVE::JSONSchema::validate($detail_address, get_standard_option('pmg-email-address'),
> +	'Address parameter verification failed');
> +
>      my ($start, $end) = $extract_start_end->($param);
>  
>      my $stat = PMG::Statistic->new($start, $end);
> @@ -309,19 +322,22 @@ sub get_detail_statistics {
>      my $res = [];
>      if ($type eq 'contact') {
>  	$res = $stat->user_stat_contact_details(
> -	    $rdb, $param->{contact}, $userstat_limit, $sorters, $param->{filter});
> +	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
>      } elsif ($type eq 'sender') {
>  	$res = $stat->user_stat_sender_details(
> -	    $rdb, $param->{sender}, $userstat_limit, $sorters, $param->{filter});
> +	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
>      } elsif ($type eq 'receiver') {
>  	$res = $stat->user_stat_receiver_details(
> -	    $rdb, $param->{receiver}, $userstat_limit, $sorters, $param->{filter});
> +	    $rdb, $detail_address, $userstat_limit, $sorters, $param->{filter});
>      } else {
>  	die "invalid type provided (not 'contact', 'sender', 'receiver')\n";
>      }
>      return $res;
>  }
>  
> +# FIXME: change for PMG 7.0 - remove support for providing addresses as path
> +# addresses can contain '/' which breaks API path resolution - hardcode
> +# 'detail' for the path pattern.
>  __PACKAGE__->register_method ({
>      name => 'contactdetails',
>      path => 'contact/{contact}',
> @@ -331,8 +347,14 @@ __PACKAGE__->register_method ({
>      parameters => {
>  	additionalProperties => 0,
>  	properties => $default_properties->({
> -	    contact => get_standard_option('pmg-email-address', {
> -		description => "Contact email address.",
> +	    contact => {
> +		type => 'string',
> +		description => "'detail', with the address provided as 'detailaddress' or"
> +		. "the address - the latter is deprecated.",
> +	    },
> +	    detailaddress => get_standard_option('pmg-email-address', {
> +		description => "The e-mail address if contact is 'detail'.",
> +		optional => 1,
>  	    }),
>  	    filter => {
>  		description => "Sender address filter.",
> @@ -425,6 +447,9 @@ __PACKAGE__->register_method ({
>  	return $res;
>      }});
>  
> +# FIXME: change for PMG 7.0 - remove support for providing addresses as path
> +# addresses can contain '/' which breaks API path resolution - hardcode
> +# 'detail' for the path pattern.
>  __PACKAGE__->register_method ({
>      name => 'senderdetails',
>      path => 'sender/{sender}',
> @@ -434,8 +459,14 @@ __PACKAGE__->register_method ({
>      parameters => {
>  	additionalProperties => 0,
>  	properties => $default_properties->({
> -	    sender => get_standard_option('pmg-email-address', {
> -		description => "Sender email address.",
> +	    sender => {
> +		type => 'string',
> +		description => "'detail', with the address provided as 'detailaddress' or"
> +		. "the address - the latter is deprecated.",
> +	    },
> +	    detailaddress => get_standard_option('pmg-email-address', {
> +		description => "The e-mail address if sender is 'detail'.",
> +		optional => 1,
>  	    }),
>  	    filter => {
>  		description => "Receiver address filter.",
> @@ -536,6 +567,9 @@ __PACKAGE__->register_method ({
>  	return $res;
>      }});
>  
> +# FIXME: change for PMG 7.0 - remove support for providing addresses as path
> +# addresses can contain '/' which breaks API path resolution - hardcode
> +# 'detail' for the path pattern.
>  __PACKAGE__->register_method ({
>      name => 'receiverdetails',
>      path => 'receiver/{receiver}',
> @@ -545,8 +579,14 @@ __PACKAGE__->register_method ({
>      parameters => {
>  	additionalProperties => 0,
>  	properties => $default_properties->({
> -	    receiver => get_standard_option('pmg-email-address', {
> -		description => "Receiver email address.",
> +	    receiver => {
> +		type => 'string',
> +		description => "'detail', with the address provided as 'detailaddress' or"
> +		. "the address - the latter is deprecated.",
> +	    },
> +	    detailaddress => get_standard_option('pmg-email-address', {
> +		description => "The e-mail address if contact is 'detail'.",
> +		optional => 1,
>  	    }),
>  	    filter => {
>  		description => "Sender address filter.",
>