public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login
@ 2024-12-03 15:29 Gabriel Goller
  2024-12-03 15:29 ` [pve-devel] [PATCH manager 1/3] show optional consent-banner before login Gabriel Goller
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Gabriel Goller @ 2024-12-03 15:29 UTC (permalink / raw)
  To: pve-devel

The consent text is stored in the datacenter.cfg file and is encoded
using base64. This allows us to support multi-line strings and special
characters. To easily edit the text the existing edit-field
ProxmoxTextAreaField is used. It supports editing and saving
multi-line text and converting it to its base64 representation.

This is the port from the initial PBS implementation and should work
in the same way.

manager:

Gabriel Goller (1):
  show optional consent-banner before login

 PVE/Service/pveproxy.pm            |  4 +++-
 www/index.html.tpl                 |  3 ++-
 www/manager6/dc/OptionView.js      |  5 +++++
 www/manager6/window/LoginWindow.js | 13 ++++++++++++-
 4 files changed, 22 insertions(+), 3 deletions(-)


cluster:

Gabriel Goller (1):
  add consent-text paramter to datacenter config file

 src/PVE/DataCenterConfig.pm | 5 +++++
 1 file changed, 5 insertions(+)


docs:

Gabriel Goller (1):
  add consent-banner description

 pve-gui.adoc | 9 +++++++++
 1 file changed, 9 insertions(+)


Summary over all repositories:
  6 files changed, 36 insertions(+), 3 deletions(-)

-- 
Generated by git-murpp 0.7.1


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pve-devel] [PATCH manager 1/3] show optional consent-banner before login
  2024-12-03 15:29 [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
@ 2024-12-03 15:29 ` Gabriel Goller
  2024-12-03 17:24   ` Thomas Lamprecht
  2024-12-03 15:29 ` [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file Gabriel Goller
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Gabriel Goller @ 2024-12-03 15:29 UTC (permalink / raw)
  To: pve-devel

Add ConsentBanner variable to html template and populate it from the
`datacenter.cfg` config file. Add Datacenter option to set the text and
trigger the popup on login.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 PVE/Service/pveproxy.pm            |  4 +++-
 www/index.html.tpl                 |  3 ++-
 www/manager6/dc/OptionView.js      |  5 +++++
 www/manager6/window/LoginWindow.js | 13 ++++++++++++-
 4 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
index ac1085457f2e..151ba34f8a52 100755
--- a/PVE/Service/pveproxy.pm
+++ b/PVE/Service/pveproxy.pm
@@ -217,9 +217,10 @@ sub get_index {
 	    $token = PVE::AccessControl::assemble_csrf_prevention_token($username);
 	}
     }
+    my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
+    my $consent_text = $dc_conf->{'consent-text'};
 
     if (!$lang) {
-	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
 	$lang = $dc_conf->{language} // 'en';
     }
 
@@ -251,6 +252,7 @@ sub get_index {
 	version => "$version",
 	wtversion => $wtversion,
 	theme => $theme,
+	consenttext => $consent_text
     };
 
     # by default, load the normal index
diff --git a/www/index.html.tpl b/www/index.html.tpl
index 46dc877bcaf2..d53a867522dd 100644
--- a/www/index.html.tpl
+++ b/www/index.html.tpl
@@ -41,7 +41,8 @@
 	defaultLang: '[% lang %]',
 	NodeName: '[% nodename %]',
 	UserName: '[% username %]',
-	CSRFPreventionToken: '[% token %]'
+	CSRFPreventionToken: '[% token %]',
+	ConsentText: '[% consenttext %]'
     };
     </script>
     <script type="text/javascript" src="/proxmoxlib.js?ver=[% wtversion %]"></script>
diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
index b200fd12dcc9..13336f1f6022 100644
--- a/www/manager6/dc/OptionView.js
+++ b/www/manager6/dc/OptionView.js
@@ -516,6 +516,11 @@ Ext.define('PVE.dc.OptionView', {
 	    },
 	};
 
+	me.add_textareafield_row('consent-text', gettext('Consent Text'), {
+	    deleteEmpty: true,
+	    onlineHelp: 'consent_banner',
+	});
+
 	me.selModel = Ext.create('Ext.selection.RowModel', {});
 
 	Ext.apply(me, {
diff --git a/www/manager6/window/LoginWindow.js b/www/manager6/window/LoginWindow.js
index aaeca3550289..bcf9b7be364d 100644
--- a/www/manager6/window/LoginWindow.js
+++ b/www/manager6/window/LoginWindow.js
@@ -18,9 +18,20 @@ Ext.define('PVE.window.LoginWindow', {
     },
 
     controller: {
-
 	xclass: 'Ext.app.ViewController',
 
+	init: async function() {
+	    if (Proxmox.ConsentText) {
+		Ext.create("Proxmox.window.ConsentModal", {
+		    autoShow: true,
+		    consent: Proxmox.Markdown.parse(
+			Ext.htmlEncode(
+			    Proxmox.Utils.base64ToUtf8(
+				Proxmox.ConsentText))),
+		});
+	    }
+	},
+
 	onLogon: async function() {
 	    var me = this;
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file
  2024-12-03 15:29 [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
  2024-12-03 15:29 ` [pve-devel] [PATCH manager 1/3] show optional consent-banner before login Gabriel Goller
@ 2024-12-03 15:29 ` Gabriel Goller
  2024-12-03 17:24   ` Thomas Lamprecht
  2024-12-03 15:29 ` [pve-devel] [PATCH docs 3/3] add consent-banner description Gabriel Goller
  2024-12-06 11:19 ` [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
  3 siblings, 1 reply; 11+ messages in thread
From: Gabriel Goller @ 2024-12-03 15:29 UTC (permalink / raw)
  To: pve-devel

The consent-text paramter is the base64-encoded content of the optional
consent-banner which can be displayed before login.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/PVE/DataCenterConfig.pm | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
index abd0bbfd4532..0347bd5ae19f 100644
--- a/src/PVE/DataCenterConfig.pm
+++ b/src/PVE/DataCenterConfig.pm
@@ -449,6 +449,11 @@ my $datacenter_schema = {
 	    pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
 	    typetext => "<tag>[;<tag>...]",
 	},
+	'consent-text' => {
+	    optional => 1,
+	    type => 'string',
+	    description => "Consent text that is displayed before logging in."
+	},
     },
 };
 
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [pve-devel] [PATCH docs 3/3] add consent-banner description
  2024-12-03 15:29 [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
  2024-12-03 15:29 ` [pve-devel] [PATCH manager 1/3] show optional consent-banner before login Gabriel Goller
  2024-12-03 15:29 ` [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file Gabriel Goller
@ 2024-12-03 15:29 ` Gabriel Goller
  2024-12-06 11:19 ` [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
  3 siblings, 0 replies; 11+ messages in thread
From: Gabriel Goller @ 2024-12-03 15:29 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pve-gui.adoc | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/pve-gui.adoc b/pve-gui.adoc
index 8a04bd977796..4eef5cc1b56d 100644
--- a/pve-gui.adoc
+++ b/pve-gui.adoc
@@ -453,6 +453,15 @@ edited under __Datacenter -> Options -> Registered Tags__ or via the CLI.
 For more details on the exact options and how to invoke them in the CLI, see
 xref:datacenter_configuration_file[Datacenter Configuration].
 
+[[consent_banner]]
+Consent Banner
+------------
+
+A custom consent banner that has to be accepted before login can be configured
+in __Datacenter -> Options -> Consent Text__. If there is no
+content, the consent banner will not be displayed. The text will be stored as a
+base64 string in the `/etc/pve/datacenter.cfg` config file.
+
 ifdef::wiki[]
 
 See Also
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file
  2024-12-03 15:29 ` [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file Gabriel Goller
@ 2024-12-03 17:24   ` Thomas Lamprecht
  2024-12-04  9:16     ` Gabriel Goller
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Lamprecht @ 2024-12-03 17:24 UTC (permalink / raw)
  To: Proxmox VE development discussion, Gabriel Goller

Am 03.12.24 um 16:29 schrieb Gabriel Goller:
> The consent-text paramter is the base64-encoded content of the optional
> consent-banner which can be displayed before login.

This can get quite big, would be good to set some relatively high limit here,
I think, as our pmxcfs files have a max size and not being able to change other
options due to this property being close to the pmxcfs file size limit would
not be ideal.

btw. Did we ever talk about placing this into a dedicated, separate file?
(just out of interest, might be also an option to consider here if we did
not talked already about it)

> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  src/PVE/DataCenterConfig.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
> index abd0bbfd4532..0347bd5ae19f 100644
> --- a/src/PVE/DataCenterConfig.pm
> +++ b/src/PVE/DataCenterConfig.pm
> @@ -449,6 +449,11 @@ my $datacenter_schema = {
>  	    pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
>  	    typetext => "<tag>[;<tag>...]",
>  	},
> +	'consent-text' => {
> +	    optional => 1,
> +	    type => 'string',
> +	    description => "Consent text that is displayed before logging in."
> +	},
>      },
>  };
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH manager 1/3] show optional consent-banner before login
  2024-12-03 15:29 ` [pve-devel] [PATCH manager 1/3] show optional consent-banner before login Gabriel Goller
@ 2024-12-03 17:24   ` Thomas Lamprecht
  2024-12-04  8:48     ` Gabriel Goller
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Lamprecht @ 2024-12-03 17:24 UTC (permalink / raw)
  To: Proxmox VE development discussion, Gabriel Goller

Am 03.12.24 um 16:29 schrieb Gabriel Goller:
> Add ConsentBanner variable to html template and populate it from the
> `datacenter.cfg` config file. Add Datacenter option to set the text and
> trigger the popup on login.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  PVE/Service/pveproxy.pm            |  4 +++-
>  www/index.html.tpl                 |  3 ++-
>  www/manager6/dc/OptionView.js      |  5 +++++
>  www/manager6/window/LoginWindow.js | 13 ++++++++++++-
>  4 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
> index ac1085457f2e..151ba34f8a52 100755
> --- a/PVE/Service/pveproxy.pm
> +++ b/PVE/Service/pveproxy.pm
> @@ -217,9 +217,10 @@ sub get_index {
>  	    $token = PVE::AccessControl::assemble_csrf_prevention_token($username);
>  	}
>      }
> +    my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');

I forgot the details from top of my head, but does this fail if we're not quorate?
If, then I'd wrap it in an eval to avoid breaking start up if not quorate.

> +    my $consent_text = $dc_conf->{'consent-text'};
>  
>      if (!$lang) {
> -	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>  	$lang = $dc_conf->{language} // 'en';
>      }
>  
> @@ -251,6 +252,7 @@ sub get_index {
>  	version => "$version",
>  	wtversion => $wtversion,
>  	theme => $theme,
> +	consenttext => $consent_text
>      };
>  
>      # by default, load the normal index
> diff --git a/www/index.html.tpl b/www/index.html.tpl
> index 46dc877bcaf2..d53a867522dd 100644
> --- a/www/index.html.tpl
> +++ b/www/index.html.tpl
> @@ -41,7 +41,8 @@
>  	defaultLang: '[% lang %]',
>  	NodeName: '[% nodename %]',
>  	UserName: '[% username %]',
> -	CSRFPreventionToken: '[% token %]'
> +	CSRFPreventionToken: '[% token %]',
> +	ConsentText: '[% consenttext %]'
>      };
>      </script>
>      <script type="text/javascript" src="/proxmoxlib.js?ver=[% wtversion %]"></script>
> diff --git a/www/manager6/dc/OptionView.js b/www/manager6/dc/OptionView.js
> index b200fd12dcc9..13336f1f6022 100644
> --- a/www/manager6/dc/OptionView.js
> +++ b/www/manager6/dc/OptionView.js
> @@ -516,6 +516,11 @@ Ext.define('PVE.dc.OptionView', {
>  	    },
>  	};
>  
> +	me.add_textareafield_row('consent-text', gettext('Consent Text'), {
> +	    deleteEmpty: true,
> +	    onlineHelp: 'consent_banner',
> +	});
> +
>  	me.selModel = Ext.create('Ext.selection.RowModel', {});
>  
>  	Ext.apply(me, {
> diff --git a/www/manager6/window/LoginWindow.js b/www/manager6/window/LoginWindow.js
> index aaeca3550289..bcf9b7be364d 100644
> --- a/www/manager6/window/LoginWindow.js
> +++ b/www/manager6/window/LoginWindow.js
> @@ -18,9 +18,20 @@ Ext.define('PVE.window.LoginWindow', {
>      },
>  
>      controller: {
> -
>  	xclass: 'Ext.app.ViewController',
>  
> +	init: async function() {
> +	    if (Proxmox.ConsentText) {
> +		Ext.create("Proxmox.window.ConsentModal", {
> +		    autoShow: true,
> +		    consent: Proxmox.Markdown.parse(
> +			Ext.htmlEncode(
> +			    Proxmox.Utils.base64ToUtf8(
> +				Proxmox.ConsentText))),

IIRC I restructured the indentation here for the PBS side, maybe check that out, IMO
it looks a bit hard to read as is.

> +		});
> +	    }
> +	},
> +
>  	onLogon: async function() {
>  	    var me = this;
>  



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH manager 1/3] show optional consent-banner before login
  2024-12-03 17:24   ` Thomas Lamprecht
@ 2024-12-04  8:48     ` Gabriel Goller
  0 siblings, 0 replies; 11+ messages in thread
From: Gabriel Goller @ 2024-12-04  8:48 UTC (permalink / raw)
  To: Thomas Lamprecht; +Cc: Proxmox VE development discussion

On 03.12.2024 18:24, Thomas Lamprecht wrote:
>Am 03.12.24 um 16:29 schrieb Gabriel Goller:
>> Add ConsentBanner variable to html template and populate it from the
>> `datacenter.cfg` config file. Add Datacenter option to set the text and
>> trigger the popup on login.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>  PVE/Service/pveproxy.pm            |  4 +++-
>>  www/index.html.tpl                 |  3 ++-
>>  www/manager6/dc/OptionView.js      |  5 +++++
>>  www/manager6/window/LoginWindow.js | 13 ++++++++++++-
>>  4 files changed, 22 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/Service/pveproxy.pm b/PVE/Service/pveproxy.pm
>> index ac1085457f2e..151ba34f8a52 100755
>> --- a/PVE/Service/pveproxy.pm
>> +++ b/PVE/Service/pveproxy.pm
>> @@ -217,9 +217,10 @@ sub get_index {
>>  	    $token = PVE::AccessControl::assemble_csrf_prevention_token($username);
>>  	}
>>      }
>> +    my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>
>I forgot the details from top of my head, but does this fail if we're not quorate?
>If, then I'd wrap it in an eval to avoid breaking start up if not quorate.

Hmm I don't think so – no quorum should still allow read access.
Nevertheless it would still be better to wrap it in an eval!

>> +    my $consent_text = $dc_conf->{'consent-text'};
>>
>>      if (!$lang) {
>> -	my $dc_conf = PVE::Cluster::cfs_read_file('datacenter.cfg');
>>  	$lang = $dc_conf->{language} // 'en';
>>      }
>>
>> @@ -251,6 +252,7 @@ sub get_index {
>>  	version => "$version",
>>  	wtversion => $wtversion,
>>  	theme => $theme,
>> +	consenttext => $consent_text
>>      };
>>
>>      # by default, load the normal index
>> diff --git a/www/manager6/window/LoginWindow.js b/www/manager6/window/LoginWindow.js
>> index aaeca3550289..bcf9b7be364d 100644
>> --- a/www/manager6/window/LoginWindow.js
>> +++ b/www/manager6/window/LoginWindow.js
>> @@ -18,9 +18,20 @@ Ext.define('PVE.window.LoginWindow', {
>>      },
>>
>>      controller: {
>> -
>>  	xclass: 'Ext.app.ViewController',
>>
>> +	init: async function() {
>> +	    if (Proxmox.ConsentText) {
>> +		Ext.create("Proxmox.window.ConsentModal", {
>> +		    autoShow: true,
>> +		    consent: Proxmox.Markdown.parse(
>> +			Ext.htmlEncode(
>> +			    Proxmox.Utils.base64ToUtf8(
>> +				Proxmox.ConsentText))),
>
>IIRC I restructured the indentation here for the PBS side, maybe check that out, IMO
>it looks a bit hard to read as is.

Yep, definitely – fixed it.

>
>> +		});
>> +	    }
>> +	},
>> +
>>  	onLogon: async function() {
>>  	    var me = this;
>>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file
  2024-12-03 17:24   ` Thomas Lamprecht
@ 2024-12-04  9:16     ` Gabriel Goller
  2024-12-04 10:16       ` Thomas Lamprecht
  0 siblings, 1 reply; 11+ messages in thread
From: Gabriel Goller @ 2024-12-04  9:16 UTC (permalink / raw)
  To: Thomas Lamprecht; +Cc: Proxmox VE development discussion

On 03.12.2024 18:24, Thomas Lamprecht wrote:
>Am 03.12.24 um 16:29 schrieb Gabriel Goller:
>> The consent-text paramter is the base64-encoded content of the optional
>> consent-banner which can be displayed before login.
>
>This can get quite big, would be good to set some relatively high limit here,
>I think, as our pmxcfs files have a max size and not being able to change other
>options due to this property being close to the pmxcfs file size limit would
>not be ideal.

To keep the textareafield widget generic, I would add the maxLength
parameters to pbs and pve separately.

About the limit itself, the example banner in the bug report [0] has 500
characters and these legal texts can sometimes get quite long – so maybe
a limit of 1000 characters?

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5463

>btw. Did we ever talk about placing this into a dedicated, separate file?
>(just out of interest, might be also an option to consider here if we did
>not talked already about it)

Yep, my first version used a separate file – this was discussed here:
https://lore.proxmox.com/pbs-devel/fc22ec23-a300-488d-821f-bea1285881a8@proxmox.com/

>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>  src/PVE/DataCenterConfig.pm | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/src/PVE/DataCenterConfig.pm b/src/PVE/DataCenterConfig.pm
>> index abd0bbfd4532..0347bd5ae19f 100644
>> --- a/src/PVE/DataCenterConfig.pm
>> +++ b/src/PVE/DataCenterConfig.pm
>> @@ -449,6 +449,11 @@ my $datacenter_schema = {
>>  	    pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
>>  	    typetext => "<tag>[;<tag>...]",
>>  	},
>> +	'consent-text' => {
>> +	    optional => 1,
>> +	    type => 'string',
>> +	    description => "Consent text that is displayed before logging in."
>> +	},
>>      },
>>  };
>>
>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file
  2024-12-04  9:16     ` Gabriel Goller
@ 2024-12-04 10:16       ` Thomas Lamprecht
  2024-12-05 10:45         ` Gabriel Goller
  0 siblings, 1 reply; 11+ messages in thread
From: Thomas Lamprecht @ 2024-12-04 10:16 UTC (permalink / raw)
  To: Gabriel Goller; +Cc: Proxmox VE development discussion

Am 04.12.24 um 10:16 schrieb Gabriel Goller:
> On 03.12.2024 18:24, Thomas Lamprecht wrote:
>> Am 03.12.24 um 16:29 schrieb Gabriel Goller:
>>> The consent-text paramter is the base64-encoded content of the optional
>>> consent-banner which can be displayed before login.
>>
>> This can get quite big, would be good to set some relatively high limit here,
>> I think, as our pmxcfs files have a max size and not being able to change other
>> options due to this property being close to the pmxcfs file size limit would
>> not be ideal.
> 
> To keep the textareafield widget generic, I would add the maxLength
> parameters to pbs and pve separately.
> 
> About the limit itself, the example banner in the bug report [0] has 500
> characters and these legal texts can sometimes get quite long – so maybe
> a limit of 1000 characters?

The one I copied over from some example link [0] comes out as 3733 B in base64,
but I did not encode the image it contained as base64 data-url, so might be
bigger if the text should spot some logo, or patriotic flag image like it was
in my example ^^
To get a real world example for how much bigger I encoded the image from [0]
now, and it comes out as ~ 350 KiB of base64, if converted to WebP format first
it  gets down to ~ 32 KiB.

So maybe go for bigger limit, say 128 KiB, as that still gives quite a bit of
headroom to the pmxcfs per-file limit and allows (efficiently encoded) data-URL
images. Btw. your Ext.htmlEncode() breaks images with data URLs, our wrapper
around the Markdown parser we use already sanitizes problematic HTML, so an
additional HTML encode step might not be warranted here?

[0]: https://businessportal.dla.mil/scon/sys-msg/index-ext.html

> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5463
> 
>> btw. Did we ever talk about placing this into a dedicated, separate file?
>> (just out of interest, might be also an option to consider here if we did
>> not talked already about it)
> 
> Yep, my first version used a separate file – this was discussed here:
> https://lore.proxmox.com/pbs-devel/fc22ec23-a300-488d-821f-bea1285881a8@proxmox.com/

Hmm, I remember now; with the future-proofing this format makes sense, but
we could have also split content and how it's acted on, e.g., use a separate
file for the notes and add the settings for a future enforcement or the like
to the datacenter.cfg once it happens, I could have thought about that variant
back then already, sorry. The reason I asked this is that for PDM we use a
separate file for the generic notes to avoid cluttering config files comments
with that potential bigger encoded text. But for PBS we already went this way,
and it's OK enough (with an explicit limit enforced by the API schema) so fine
by me to keep as is.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file
  2024-12-04 10:16       ` Thomas Lamprecht
@ 2024-12-05 10:45         ` Gabriel Goller
  0 siblings, 0 replies; 11+ messages in thread
From: Gabriel Goller @ 2024-12-05 10:45 UTC (permalink / raw)
  To: Thomas Lamprecht; +Cc: Proxmox VE development discussion

On 04.12.2024 11:16, Thomas Lamprecht wrote:
>Am 04.12.24 um 10:16 schrieb Gabriel Goller:
>> On 03.12.2024 18:24, Thomas Lamprecht wrote:
>>> Am 03.12.24 um 16:29 schrieb Gabriel Goller:
>>>> The consent-text paramter is the base64-encoded content of the optional
>>>> consent-banner which can be displayed before login.
>>>
>>> This can get quite big, would be good to set some relatively high limit here,
>>> I think, as our pmxcfs files have a max size and not being able to change other
>>> options due to this property being close to the pmxcfs file size limit would
>>> not be ideal.
>>
>> To keep the textareafield widget generic, I would add the maxLength
>> parameters to pbs and pve separately.
>>
>> About the limit itself, the example banner in the bug report [0] has 500
>> characters and these legal texts can sometimes get quite long – so maybe
>> a limit of 1000 characters?
>
>The one I copied over from some example link [0] comes out as 3733 B in base64,
>but I did not encode the image it contained as base64 data-url, so might be
>bigger if the text should spot some logo, or patriotic flag image like it was
>in my example ^^
>To get a real world example for how much bigger I encoded the image from [0]
>now, and it comes out as ~ 350 KiB of base64, if converted to WebP format first
>it  gets down to ~ 32 KiB.
>So maybe go for bigger limit, say 128 KiB, as that still gives quite a bit of
>headroom to the pmxcfs per-file limit and allows (efficiently encoded) data-URL
>images. 

According to my conservative calculations that would then be a max input
length (in characters) of circa 24000.

> Btw. your Ext.htmlEncode() breaks images with data URLs, our wrapper
>around the Markdown parser we use already sanitizes problematic HTML, so an
>additional HTML encode step might not be warranted here?

Yep, that's right we do the hmtlEncode in the Markdown.parse function
already so this is not needed. Nice catch!

>[0]: https://businessportal.dla.mil/scon/sys-msg/index-ext.html
>
>> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=5463
>>
>>> btw. Did we ever talk about placing this into a dedicated, separate file?
>>> (just out of interest, might be also an option to consider here if we did
>>> not talked already about it)
>>
>> Yep, my first version used a separate file – this was discussed here:
>> https://lore.proxmox.com/pbs-devel/fc22ec23-a300-488d-821f-bea1285881a8@proxmox.com/
>
>Hmm, I remember now; with the future-proofing this format makes sense, but
>we could have also split content and how it's acted on, e.g., use a separate
>file for the notes and add the settings for a future enforcement or the like
>to the datacenter.cfg once it happens, I could have thought about that variant
>back then already, sorry. The reason I asked this is that for PDM we use a
>separate file for the generic notes to avoid cluttering config files comments
>with that potential bigger encoded text. But for PBS we already went this way,
>and it's OK enough (with an explicit limit enforced by the API schema) so fine
>by me to keep as is.

Yep, I'll include the limit in pbs as well.

Thanks for the review!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login
  2024-12-03 15:29 [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
                   ` (2 preceding siblings ...)
  2024-12-03 15:29 ` [pve-devel] [PATCH docs 3/3] add consent-banner description Gabriel Goller
@ 2024-12-06 11:19 ` Gabriel Goller
  3 siblings, 0 replies; 11+ messages in thread
From: Gabriel Goller @ 2024-12-06 11:19 UTC (permalink / raw)
  To: pve-devel

Sent a v2!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-12-06 11:19 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-03 15:29 [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller
2024-12-03 15:29 ` [pve-devel] [PATCH manager 1/3] show optional consent-banner before login Gabriel Goller
2024-12-03 17:24   ` Thomas Lamprecht
2024-12-04  8:48     ` Gabriel Goller
2024-12-03 15:29 ` [pve-devel] [PATCH cluster 2/3] add consent-text paramter to datacenter config file Gabriel Goller
2024-12-03 17:24   ` Thomas Lamprecht
2024-12-04  9:16     ` Gabriel Goller
2024-12-04 10:16       ` Thomas Lamprecht
2024-12-05 10:45         ` Gabriel Goller
2024-12-03 15:29 ` [pve-devel] [PATCH docs 3/3] add consent-banner description Gabriel Goller
2024-12-06 11:19 ` [pve-devel] [PATCH cluster/docs/manager 0/3] fix #5463: add optional Consent Banner on login Gabriel Goller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal