[pmg-devel] [RFC PATCH pmg-api/gui] add 'view images' checkbox to quarantine view

[pmg-devel] [RFC PATCH pmg-api/gui] add 'view images' checkbox to quarantine view

[Dominik Csapak]

so that the user can control if images should be loaded, even
if the admin set that to true in the backend.

I'm not completely happy with the approach, since there is currently no
general way for us to see if the backend option was set, unless we'd
either weaken the permissions necesary to read the config or introduce
another api call that returns that info, both of which aren't optimal
imho.

Also we always show that checkbox now, since i didn't find a way to
detect if there are images in the mail or not unless we actually load it
with view images on and try to parse the email again in the frontend.
Which also sound not really clean.

For both reasons, sent as RFC.

pmg-api:

Dominik Csapak (1):
  api: quarantine: allow disabling images for content

 src/PMG/API2/Quarantine.pm | 11 +++++++++++
 1 file changed, 11 insertions(+)

pmg-gui:

Dominik Csapak (1):
  quarantine content: add checkbox for controlling image loading

 js/AttachmentQuarantine.js            |  4 +++-
 js/Makefile                           |  1 +
 js/SpamQuarantine.js                  |  4 +++-
 js/VirusQuarantine.js                 |  4 +++-
 js/controller/QuarantineController.js | 19 ++++++++++++++++---
 js/form/ViewImages.js                 | 17 +++++++++++++++++
 6 files changed, 43 insertions(+), 6 deletions(-)
 create mode 100644 js/form/ViewImages.js

-- 
2.30.2

[pmg-devel] [RFC PATCH pmg-api 1/1] api: quarantine: allow disabling images for content

[Dominik Csapak]

So that a client can control if images should be stripped from the
quarantine content, even if the server has 'viewimages' configured to on
in pmg.conf.

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

diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
index a100e11..17f1405 100644
--- a/src/PMG/API2/Quarantine.pm
+++ b/src/PMG/API2/Quarantine.pm
@@ -867,6 +867,13 @@ __PACKAGE__->register_method ({
 		optional => 1,
 		default => 0,
 	    },
+	    'view-images' => {
+		description => "Controls if images are shown. Has no effect if 'viewimages'"
+		    ." is turned off in /etc/pmg/pmg.conf.",
+		type => 'boolean',
+		optional => 1,
+		default => 1,
+	    },
 	},
     },
     returns => {
@@ -944,6 +951,10 @@ __PACKAGE__->register_method ({
 
 	    my $cfg = PMG::Config->new();
 	    my $viewimages = $cfg->get('spamquar', 'viewimages');
+
+	    # only overwrite with the parameter when it's more strict than the config
+	    $viewimages = 0 if !($param->{'view-images'} // 1);
+
 	    my $allowhref = $cfg->get('spamquar', 'allowhrefs');
 
 	    $res->{content} = PMG::HTMLMail::email_to_html($path, $raw, $viewimages, $allowhref) // 'unable to parse mail';
-- 
2.30.2

Re: [pmg-devel] [RFC PATCH pmg-api 1/1] api: quarantine: allow disabling images for content

[Mira Limbeck]

On 2/13/24 12:30, Dominik Csapak wrote:
> So that a client can control if images should be stripped from the
> quarantine content, even if the server has 'viewimages' configured to on
> in pmg.conf.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PMG/API2/Quarantine.pm | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/src/PMG/API2/Quarantine.pm b/src/PMG/API2/Quarantine.pm
> index a100e11..17f1405 100644
> --- a/src/PMG/API2/Quarantine.pm
> +++ b/src/PMG/API2/Quarantine.pm
> @@ -867,6 +867,13 @@ __PACKAGE__->register_method ({
>  		optional => 1,
>  		default => 0,
>  	    },
> +	    'view-images' => {
> +		description => "Controls if images are shown. Has no effect if 'viewimages'"
> +		    ." is turned off in /etc/pmg/pmg.conf.",
> +		type => 'boolean',
> +		optional => 1,
> +		default => 1,
> +	    },
>  	},
>      },
>      returns => {
> @@ -944,6 +951,10 @@ __PACKAGE__->register_method ({
>  
>  	    my $cfg = PMG::Config->new();
>  	    my $viewimages = $cfg->get('spamquar', 'viewimages');
> +
> +	    # only overwrite with the parameter when it's more strict than the config
> +	    $viewimages = 0 if !($param->{'view-images'} // 1);
> +
>  	    my $allowhref = $cfg->get('spamquar', 'allowhrefs');
>  
>  	    $res->{content} = PMG::HTMLMail::email_to_html($path, $raw, $viewimages, $allowhref) // 'unable to parse mail';

Since `view-images` only has an effect when format equals `htmlmail`,
should this be noted in the description?

Works as described, so consider this:

Tested-by: Mira Limbeck <m.limbeck@proxmox.com>

[pmg-devel] [RFC PATCH pmg-gui 1/1] quarantine content: add checkbox for controlling image loading

[Dominik Csapak]

sometimes users don't want to load the images from the quarantine,
even when the admin configured 'viewimages' to on in the config.
(e.g. for privacy reasons)

This checkbox sets/gets the state from the browser local storage
(so it's saved across reloads) to save the users preference of loading
images.

If the backend is set to not load the images, the checkbox doesn't have
any effect.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 js/AttachmentQuarantine.js            |  4 +++-
 js/Makefile                           |  1 +
 js/SpamQuarantine.js                  |  4 +++-
 js/VirusQuarantine.js                 |  4 +++-
 js/controller/QuarantineController.js | 19 ++++++++++++++++---
 js/form/ViewImages.js                 | 17 +++++++++++++++++
 6 files changed, 43 insertions(+), 6 deletions(-)
 create mode 100644 js/form/ViewImages.js

diff --git a/js/AttachmentQuarantine.js b/js/AttachmentQuarantine.js
index 52bda4c..340c18f 100644
--- a/js/AttachmentQuarantine.js
+++ b/js/AttachmentQuarantine.js
@@ -109,7 +109,9 @@ Ext.define('PMG.AttachmentQuarantine', {
 			},
 			{
 			    xtype: 'tbseparator',
-			    reference: 'themeCheckSep',
+			},
+			{
+			    xtype: 'pmgViewImagesCheckbox',
 			},
 			{
 			    xtype: 'proxmoxcheckbox',
diff --git a/js/Makefile b/js/Makefile
index 78f2b57..f2faef7 100644
--- a/js/Makefile
+++ b/js/Makefile
@@ -2,6 +2,7 @@ include ../defines.mk
 
 JSSRC=							\
 	Utils.js					\
+	form/ViewImages.js				\
 	FilterProxy.js					\
 	LoginView.js					\
 	RoleSelector.js					\
diff --git a/js/SpamQuarantine.js b/js/SpamQuarantine.js
index a390dcf..38d6d52 100644
--- a/js/SpamQuarantine.js
+++ b/js/SpamQuarantine.js
@@ -226,7 +226,9 @@ Ext.define('PMG.SpamQuarantine', {
 			},
 			{
 			    xtype: 'tbseparator',
-			    reference: 'themeCheckSep',
+			},
+			{
+			    xtype: 'pmgViewImagesCheckbox',
 			},
 			{
 			    xtype: 'proxmoxcheckbox',
diff --git a/js/VirusQuarantine.js b/js/VirusQuarantine.js
index 9e5a4fb..c21a09f 100644
--- a/js/VirusQuarantine.js
+++ b/js/VirusQuarantine.js
@@ -122,7 +122,9 @@ Ext.define('PMG.VirusQuarantine', {
 			},
 			{
 			    xtype: 'tbseparator',
-			    reference: 'themeCheckSep',
+			},
+			{
+			    xtype: 'pmgViewImagesCheckbox',
 			},
 			{
 			    xtype: 'proxmoxcheckbox',
diff --git a/js/controller/QuarantineController.js b/js/controller/QuarantineController.js
index 2a24389..5d036a6 100644
--- a/js/controller/QuarantineController.js
+++ b/js/controller/QuarantineController.js
@@ -10,8 +10,10 @@ Ext.define('PMG.controller.QuarantineController', {
 	    preview.setDisabled(true);
 	    return;
 	}
+	let sp = Ext.state.Manager.getProvider();
+	let viewImages = sp.get('view-images') ?? 1;
 
-	let url = `/api2/htmlmail/quarantine/content?id=${rec.data.id}`;
+	let url = `/api2/htmlmail/quarantine/content?view-images=${viewImages}&id=${rec.data.id}`;
 	if (raw) {
 	    url += '&raw=1';
 	}
@@ -49,7 +51,6 @@ Ext.define('PMG.controller.QuarantineController', {
 	let themeButton = me.lookup('themeCheck');
 	themeButton.disable();
 	themeButton.hide();
-	me.lookup('themeCheckSep').hide();
 	me.themed = true;
 	me.toggleTheme();
     },
@@ -62,7 +63,6 @@ Ext.define('PMG.controller.QuarantineController', {
 	themeButton.setValue(true);
 	themeButton.enable();
 	themeButton.show();
-	me.lookup('themeCheckSep').show();
     },
 
     toggleRaw: function(button) {
@@ -74,6 +74,16 @@ Ext.define('PMG.controller.QuarantineController', {
 	me.updatePreview(me.raw, rec);
     },
 
+    toggleImages: function(field, value) {
+	let me = this;
+	let sp = Ext.state.Manager.getProvider();
+	sp.set('view-images', value ? 1 : 0);
+
+	let list = me.lookupReference('list');
+	let rec = list.selModel.getSelection()[0];
+	me.updatePreview(me.raw, rec);
+    },
+
     btnHandler: function(button, e) {
 	let me = this;
 	let action = button.reference;
@@ -228,6 +238,9 @@ Ext.define('PMG.controller.QuarantineController', {
 	'button[reference=raw]': {
 	    click: 'toggleRaw',
 	},
+	'proxmoxcheckbox[reference=toggleImages]': {
+	    change: 'toggleImages',
+	},
 	'proxmoxcheckbox[reference=themeCheck]': {
 	    change: 'toggleTheme',
 	},
diff --git a/js/form/ViewImages.js b/js/form/ViewImages.js
new file mode 100644
index 0000000..8fb6321
--- /dev/null
+++ b/js/form/ViewImages.js
@@ -0,0 +1,17 @@
+Ext.define('PMG.form.ViewImagesCheckbox', {
+    extend: 'Proxmox.form.Checkbox',
+    alias: 'widget.pmgViewImagesCheckbox',
+
+    boxLabel: gettext('View Images'),
+    iconCls: 'fa fa-file-photo-o',
+    reference: 'toggleImages',
+
+    initComponent: function() {
+	let me = this;
+
+	let sp = Ext.state.Manager.getProvider();
+	me.checked = sp.get('view-images') ?? 1;
+
+	me.callParent();
+    },
+});
-- 
2.30.2

Re: [pmg-devel] [RFC PATCH pmg-gui 1/1] quarantine content: add checkbox for controlling image loading

[Mira Limbeck]

On 2/13/24 12:30, Dominik Csapak wrote:
> sometimes users don't want to load the images from the quarantine,
> even when the admin configured 'viewimages' to on in the config.
> (e.g. for privacy reasons)
> 
> This checkbox sets/gets the state from the browser local storage
> (so it's saved across reloads) to save the users preference of loading
> images.
> 
> If the backend is set to not load the images, the checkbox doesn't have
> any effect.
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  js/AttachmentQuarantine.js            |  4 +++-
>  js/Makefile                           |  1 +
>  js/SpamQuarantine.js                  |  4 +++-
>  js/VirusQuarantine.js                 |  4 +++-
>  js/controller/QuarantineController.js | 19 ++++++++++++++++---
>  js/form/ViewImages.js                 | 17 +++++++++++++++++
>  6 files changed, 43 insertions(+), 6 deletions(-)
>  create mode 100644 js/form/ViewImages.js
> 

I'd prefer a button similar to the `Toggle Raw` and `Toggle Spam Info`
ones that is off by default and only visible when there are images in
the mail, but that's personal taste.

Since we don't have any info on if there are even images in the mail,
the checkbox seems to be the better option.
Having it on by default doesn't change the behavior after applying the
patch, so seems to be the better option in this case.

Consider this patch:

Tested-by: Mira Limbeck <m.limbeck@proxmox.com>

Re: [pmg-devel] [RFC PATCH pmg-api/gui] add 'view images' checkbox to quarantine view

[Thomas Lamprecht]

Am 13/02/2024 um 12:30 schrieb Dominik Csapak:
> so that the user can control if images should be loaded, even
> if the admin set that to true in the backend.
> 
> I'm not completely happy with the approach, since there is currently no
> general way for us to see if the backend option was set, unless we'd
> either weaken the permissions necesary to read the config or introduce
> another api call that returns that info, both of which aren't optimal
> imho.
> 
> Also we always show that checkbox now, since i didn't find a way to
> detect if there are images in the mail or not unless we actually load it
> with view images on and try to parse the email again in the frontend.
> Which also sound not really clean.
> 
> For both reasons, sent as RFC.


Maybe s/view/load/ but no hard feelings for that..

Otherwise it seems OK for the current API capabilities.

One thought I had is if it might be worth it to place those view options
(this and dark-filter) inside a dropdown menu button, but fine as is too.