[pmg-devel] [PATCH pmg-api/pmg-gui v3] allow / in local part of pmg-email-address

[pmg-devel] [PATCH pmg-api/pmg-gui v3] allow / in local part of pmg-email-address

[Stoiko Ivanov]

v2->v3:
* addressed Thomas' feedback (huge thx!!)
* changed the API to have a dedicated /statistics/detail call, which handles
  all 3 detail-statistics calls (based on the required type parameter)
* dropped the already applied patches

cover-letter for previous series:
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 (2):
  api: statistics: add common method for details
  utils: allow '/' inside email address localpart

 src/PMG/API2/Statistics.pm | 63 ++++++++++++++++++++++++++++++++++++++
 src/PMG/Utils.pm           |  2 +-
 2 files changed, 64 insertions(+), 1 deletion(-)

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

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

-- 
2.20.1

[pmg-devel] [PATCH pmg-api v3 1/2] api: statistics: add common method for details

[Stoiko Ivanov]

This patch changes adds a new method, which yields the detail statistics
for a particular address.

Depending on the provided type argument it returns the same information as
the contact/sender/receiver detail calls.

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

By adding a new API method we can eventually drop the old methods with 7.0

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

diff --git a/src/PMG/API2/Statistics.pm b/src/PMG/API2/Statistics.pm
index 72bfeb5..7de6d78 100644
--- a/src/PMG/API2/Statistics.pm
+++ b/src/PMG/API2/Statistics.pm
@@ -44,6 +44,7 @@ __PACKAGE__->register_method ({
 
 	return [
 	    { name => "contact" },
+	    { name => "detail" },
 	    { name => "domains" },
 	    { name => "mail" },
 	    { name => "mailcount" },
@@ -322,6 +323,62 @@ sub get_detail_statistics {
     return $res;
 }
 
+__PACKAGE__->register_method ({
+    name => 'detailstats',
+    path => 'detail',
+    method => 'GET',
+    description => "Detailed Statistics.",
+    permissions => { check => [ 'admin', 'qmanager', 'audit'] },
+    parameters => {
+	additionalProperties => 0,
+	properties => $default_properties->({
+	    type => {
+		description => "Type of statistics",
+		type => 'string',
+		enum => [ 'contact', 'sender', 'receiver' ],
+	    },
+	    address => get_standard_option('pmg-email-address', {
+		description => "Email address.",
+	    }),
+	    filter => {
+		description => "Address filter.",
+		type => 'string',
+		maxLength => 512,
+		optional => 1,
+	    },
+	    orderby => $api_properties->{orderby},
+	}),
+    },
+    returns => {
+	type => 'array',
+	items => {
+	    type => "object",
+	    properties => $detail_return_properties->({
+		sender => {
+		    description => "Sender email. (for contact and receiver statistics)",
+		    type => 'string',
+		    optional => 1,
+		},
+		receiver => {
+		    description => "Receiver email. (for sender statistics)",
+		    type => 'string',
+		    optional => 1,
+		},
+	    }),
+	},
+    },
+    code => sub {
+	my ($param) = @_;
+
+	my $type = $param->{type};
+	$param->{$type} = $param->{address};
+
+	return get_detail_statistics($param->{type}, $param);
+    }});
+
+# 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}',
@@ -425,6 +482,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}',
@@ -536,6 +596,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}',
-- 
2.20.1

[pmg-devel] [PATCH pmg-api v3 2/2] 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 v3 1/1] statistics: use new api call for detailed stats

[Stoiko Ivanov]

the new /statistics/detail api calls takes the type (contact, sender,
receiver) and address for which to display the statistics as explicit
parameter instead of path-component.

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>
---
 js/ContactStatistics.js  | 10 +++++-----
 js/ReceiverStatistics.js | 10 +++++-----
 js/SenderStatistics.js   | 10 +++++-----
 js/StatStore.js          | 11 +++++++++--
 4 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/js/ContactStatistics.js b/js/ContactStatistics.js
index 6c9d2d4..7f448af 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, extraparam, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, extraparam);
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -201,9 +201,9 @@ 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 extraparam = { address: contact, type: 'contact' };
+		var url = "/api2/json/statistics/detail";
+		details.setUrl(url, extraparam, '<b>' + gettext('Contact') + ':</b> ' + Ext.htmlEncode(contact));
 	    } else {
 		details.setUrl();
 	    }
diff --git a/js/ReceiverStatistics.js b/js/ReceiverStatistics.js
index 6378e06..bf86745 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, extraparam, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, extraparam);
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -212,10 +212,10 @@ Ext.define('PMG.ReceiverStatistics', {
 	selectionChange: function(grid, selected, eOpts) {
 	    var details = this.lookupReference('details');
 	    if (selected.length > 0) {
+		var url = "/api2/json/statistics/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));
+		var extraparam = { address: receiver, type: 'receiver' };
+		details.setUrl(url, extraparam, '<b>' + gettext('Receiver') + ':</b> ' + Ext.htmlEncode(receiver));
 	    } else {
 		details.setUrl();
 	    }
diff --git a/js/SenderStatistics.js b/js/SenderStatistics.js
index 43c5438..830f5fb 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, extraparam, title) {
 	var me = this;
 
-	me.store.setUrl(url);
+	me.store.setUrl(url, extraparam);
 	me.store.setRemoteFilter(url !== undefined);
 	Proxmox.Utils.setErrorMask(me, false);
 	me.store.reload();
@@ -201,9 +201,9 @@ 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 extraparam = { address: sender, type: 'sender' };
+		var url = "/api2/json/statistics/detail";
+		details.setUrl(url, extraparam, '<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 v3 1/2] api: statistics: add common method for details

[Thomas Lamprecht]

On 02.02.21 14:03, Stoiko Ivanov wrote:
> This patch changes adds a new method, which yields the detail statistics
> for a particular address.
> 
> Depending on the provided type argument it returns the same information as
> the contact/sender/receiver detail calls.
> 
> 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
> 
> By adding a new API method we can eventually drop the old methods with 7.0
> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  src/PMG/API2/Statistics.pm | 63 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
>

applied, thanks!

Ammended the commit to adapt the FIXME comments as we actually can drop those
three API endpoints in 7.0

[pmg-devel] applied: [PATCH pmg-api v3 2/2] utils: allow '/' inside email address localpart

[Thomas Lamprecht]

On 02.02.21 14:03, Stoiko Ivanov wrote:
> 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(-)
> 
>

applied, thanks!

[pmg-devel] applied: [PATCH pmg-gui v3 1/1] statistics: use new api call for detailed stats

[Thomas Lamprecht]

On 02.02.21 14:03, Stoiko Ivanov wrote:
> the new /statistics/detail api calls takes the type (contact, sender,
> receiver) and address for which to display the statistics as explicit
> parameter instead of path-component.
> 
> 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>
> ---
>  js/ContactStatistics.js  | 10 +++++-----
>  js/ReceiverStatistics.js | 10 +++++-----
>  js/SenderStatistics.js   | 10 +++++-----
>  js/StatStore.js          | 11 +++++++++--
>  4 files changed, 24 insertions(+), 17 deletions(-)
> 
>

applied, thanks!