public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days
@ 2023-09-22 14:36 Philipp Hufnagl
  2023-09-22 14:36 ` [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon Philipp Hufnagl
                   ` (4 more replies)
  0 siblings, 5 replies; 15+ messages in thread
From: Philipp Hufnagl @ 2023-09-22 14:36 UTC (permalink / raw)
  To: pve-devel

Currently, when an user account expires, it catches the users by
surprise. It would be helpful to notify the the user as well as the
administrator.

This patch highlights such accounts in the user account pannel and also
shows for regular user an exclamation mark in the user info pannel. It
also adds in this case a new entry briefly saing that the account
expires. When this entry is clicked it states this in more detail
including the experation date.

Sending this for PVE only now to get feedback before implementing this
for PMG and PBS


Philipp Hufnagl (1):
  fix #4546: ui: notify user if there usser account expires soon

 www/manager6/Workspace.js | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Philipp Hufnagl (1):
  fix #4546: api: Return user expiration date on access/ticket API call

 src/PVE/API2/AccessControl.pm | 8 ++++++++
 src/PVE/AccessControl.pm      | 8 ++++++++
 2 files changed, 16 insertions(+)

Philipp Hufnagl (2):
  fix #4546: css: Inform user administrator about user accounts expiring
    soon
  fix #4546: utils: save expiring date of user account for UI

 src/Utils.js                                    | 9 ++++++++-
 src/css/ext6-pmx.css                            | 4 ++++
 src/proxmox-dark/scss/abstracts/_variables.scss | 2 ++
 src/proxmox-dark/scss/extjs/_menu.scss          | 4 ++++
 src/proxmox-dark/scss/proxmox/_general.scss     | 5 +++++
 5 files changed, 23 insertions(+), 1 deletion(-)

-- 
2.39.2





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

* [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon
  2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
@ 2023-09-22 14:36 ` Philipp Hufnagl
  2023-10-06 13:16   ` Lukas Wagner
  2023-09-22 14:36 ` [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call Philipp Hufnagl
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Philipp Hufnagl @ 2023-09-22 14:36 UTC (permalink / raw)
  To: pve-devel

When the user account that is currently logged in will expire soon, the
user icon will turn into a yellow exclamation mark. In the user menu
there will be a new element informing the user briefly about it. If the
element is clicked, a popup will appear informing the user in detail
about it

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 www/manager6/Workspace.js | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
index 18d574b7..9d166493 100644
--- a/www/manager6/Workspace.js
+++ b/www/manager6/Workspace.js
@@ -42,6 +42,7 @@ Ext.define('PVE.Workspace', {
 	Proxmox.Utils.authClear();
 	Ext.state.Manager.clear('GuiCap');
 	Proxmox.UserName = null;
+	Proxmox.UserExpires = null;
 	me.loginData = null;
 
 	if (!me.login) {
@@ -198,6 +199,18 @@ Ext.define('PVE.StdWorkspace', {
 	let me = this;
 	let ui = me.query('#userinfo')[0];
 	ui.setText(Ext.String.htmlEncode(Proxmox.UserName || ''));
+	let label = me.query('#expirewarning')[0];
+	if (Proxmox.UserExpires !== null) {
+	    let expieWariningThreshold = Ext.Date.add(new Date(), Ext.Date.DAY, 7);
+	    let expireDate = new Date(Proxmox.UserExpires * 1000);
+	    if (expieWariningThreshold >= expireDate) {
+	        ui.setIconCls('fa fa-exclamation-triangle warning');
+		label.setHidden(false);
+	    }
+	} else {
+		label.setHidden(true);
+		ui.setIconCls('fa fa-user');
+	}
 	ui.updateLayout();
     },
 
@@ -367,6 +380,21 @@ Ext.define('PVE.StdWorkspace', {
 			    },
 			    iconCls: 'fa fa-user',
 			    menu: [
+				{
+				    iconCls: 'fa fa-exclamation-triangle warning',
+				    itemId: 'expirewarning',
+				    text: gettext('Account expiring soon!'),
+				    hidden: true,
+				    handler: function() {
+					let expireDate = new Date(Proxmox.UserExpires * 1000);
+					Ext.Msg.show({
+					    title: gettext('Account expiring soon!'),
+					    icon: Ext.Msg.WARNING,
+					    message: Ext.String.format(gettext("Your account is expiring on {0} . After that you won't be able to log in!"), expireDate),
+					    buttons: Ext.Msg.OK,
+					});
+				    },
+				},
 				{
 				    iconCls: 'fa fa-gear',
 				    text: gettext('My Settings'),
-- 
2.39.2





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

* [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call
  2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
  2023-09-22 14:36 ` [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon Philipp Hufnagl
@ 2023-09-22 14:36 ` Philipp Hufnagl
  2023-10-06 13:16   ` Lukas Wagner
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon Philipp Hufnagl
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 15+ messages in thread
From: Philipp Hufnagl @ 2023-09-22 14:36 UTC (permalink / raw)
  To: pve-devel

Adds an additional, optional parameter to the access/tickets api call
which tells when the currently used user account will expire. If it will
not expire, the parameter will not be added.

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/PVE/API2/AccessControl.pm | 8 ++++++++
 src/PVE/AccessControl.pm      | 8 ++++++++
 2 files changed, 16 insertions(+)

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 74b3910..e562a97 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -267,6 +267,11 @@ __PACKAGE__->register_method ({
 	    ticket => { type => 'string', optional => 1},
 	    CSRFPreventionToken => { type => 'string', optional => 1 },
 	    clustername => { type => 'string', optional => 1 },
+	    user_expires => {
+		type => 'number',
+                description => "When the user account expires.",
+		optional => 1 ,
+	    },
 	    # cap => computed api permissions, unless there's a u2f challenge
 	}
     },
@@ -304,6 +309,9 @@ __PACKAGE__->register_method ({
 	    die PVE::Exception->new("authentication failure\n", code => 401);
 	}
 
+	my $exp = PVE::AccessControl::lookup_user_expiration($username);
+	$res->{user_expieres} = $exp if defined($exp);
+
 	$res->{cap} = $rpcenv->compute_api_permission($username)
 	    if !defined($res->{NeedTFA});
 
diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index cc0f00b..471cc92 100644
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -1234,6 +1234,14 @@ sub lookup_username {
     return $username;
 }
 
+sub lookup_user_expiration {
+    my ($username) = @_;
+    my $usercfg = cfs_read_file('user.cfg');
+    my $exp = $usercfg->{users}->{$username}->{expire};
+    return undef if $exp == 0;
+    return $exp;
+}
+
 sub normalize_path {
     my $path = shift;
 
-- 
2.39.2





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

* [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon
  2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
  2023-09-22 14:36 ` [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon Philipp Hufnagl
  2023-09-22 14:36 ` [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call Philipp Hufnagl
@ 2023-09-22 14:36 ` Philipp Hufnagl
  2023-10-06 13:16   ` Lukas Wagner
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI Philipp Hufnagl
  2023-10-06 13:16 ` [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Lukas Wagner
  4 siblings, 1 reply; 15+ messages in thread
From: Philipp Hufnagl @ 2023-09-22 14:36 UTC (permalink / raw)
  To: pve-devel

Adds a new css class to underlay information urgency in table columns
for dark and light mode. This underlay color then is used to notifiy
user administrators about user accounts that will expire soon

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/Utils.js                                    | 6 +++++-
 src/css/ext6-pmx.css                            | 4 ++++
 src/proxmox-dark/scss/abstracts/_variables.scss | 1 +
 src/proxmox-dark/scss/proxmox/_general.scss     | 5 +++++
 4 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/src/Utils.js b/src/Utils.js
index f269607..a7ded2a 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -174,10 +174,14 @@ utilities: {
 	return value ? Proxmox.Utils.enabledText : Proxmox.Utils.disabledText;
     },
 
-    format_expire: function(date) {
+    format_expire: function(date, meta) {
 	if (!date) {
 	    return Proxmox.Utils.neverText;
 	}
+	let expieWariningThreshold = Ext.Date.add(new Date(), Ext.Date.DAY, 7);
+	if (expieWariningThreshold >= date) {
+	    meta.tdCls += 'proxmox-hint-row';
+	}
 	return Ext.Date.format(date, "Y-m-d");
     },
 
diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
index 2ffd2a8..439f3a0 100644
--- a/src/css/ext6-pmx.css
+++ b/src/css/ext6-pmx.css
@@ -76,6 +76,10 @@
     background-color: #f3d6d7;
 }
 
+.proxmox-hint-row {
+    background-color: #5eb9ff;
+}
+
 .proxmox-warning-row {
     background-color: #f5e5d8;
 }
diff --git a/src/proxmox-dark/scss/abstracts/_variables.scss b/src/proxmox-dark/scss/abstracts/_variables.scss
index cac51eb..8bcae09 100644
--- a/src/proxmox-dark/scss/abstracts/_variables.scss
+++ b/src/proxmox-dark/scss/abstracts/_variables.scss
@@ -26,6 +26,7 @@ $background-darker: hsl(0deg, 0%, 15%);
 $background-darkest: hsl(0deg, 0%, 10%);
 $background-invalid: hsl(360deg, 60%, 20%);
 $background-warning: hsl(40deg, 100%, 20%);
+$background-hint: hsl(233deg, 99%, 60%);
 
 // Buttons
 $neutral-button-color: hsl(0deg, 0%, 25%);
diff --git a/src/proxmox-dark/scss/proxmox/_general.scss b/src/proxmox-dark/scss/proxmox/_general.scss
index c319f6d..8b2a4d0 100644
--- a/src/proxmox-dark/scss/proxmox/_general.scss
+++ b/src/proxmox-dark/scss/proxmox/_general.scss
@@ -25,6 +25,11 @@ div[id^="versioninfo-"] + div[id^="panel-"] > div[id^="panel-"][id$="-bodyWrap"]
   background-color: $background-warning;
 }
 
+// Info rows, e.g. when an user account expires soon
+.proxmox-hint-row {
+  background-color: $background-hint;
+}
+
 // Disabled rows (e.g. disabled repos in Repository view)
 .proxmox-disabled-row td {
   color: $text-color-inactive;
-- 
2.39.2





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

* [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI
  2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
                   ` (2 preceding siblings ...)
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon Philipp Hufnagl
@ 2023-09-22 14:36 ` Philipp Hufnagl
  2023-10-06 13:16   ` Lukas Wagner
  2023-10-06 14:41   ` Stefan Sterz
  2023-10-06 13:16 ` [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Lukas Wagner
  4 siblings, 2 replies; 15+ messages in thread
From: Philipp Hufnagl @ 2023-09-22 14:36 UTC (permalink / raw)
  To: pve-devel

When an user experation date is send with the /accesss/tickets POST API
call, it will be stored in a global variable like the username

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 src/Utils.js                                    | 3 +++
 src/proxmox-dark/scss/abstracts/_variables.scss | 1 +
 src/proxmox-dark/scss/extjs/_menu.scss          | 4 ++++
 3 files changed, 8 insertions(+)

diff --git a/src/Utils.js b/src/Utils.js
index a7ded2a..5481a32 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -309,6 +309,9 @@ utilities: {
 
     setAuthData: function(data) {
 	Proxmox.UserName = data.username;
+	if (data.user_expieres !== '') {
+	    Proxmox.UserExpires = data.user_expieres;
+	}
 	Proxmox.LoggedOut = data.LoggedOut;
 	// creates a session cookie (expire = null)
 	// that way the cookie gets deleted after the browser window is closed
diff --git a/src/proxmox-dark/scss/abstracts/_variables.scss b/src/proxmox-dark/scss/abstracts/_variables.scss
index 8bcae09..b079654 100644
--- a/src/proxmox-dark/scss/abstracts/_variables.scss
+++ b/src/proxmox-dark/scss/abstracts/_variables.scss
@@ -13,6 +13,7 @@ $text-color: hsl(0deg, 0%, 95%);
 $text-color-inactive: hsl(0deg, 0%, 60%);
 $icon-color: hsl(0deg, 0%, 90%);
 $icon-color-alt: hsl(0deg, 0%, 55%);
+$text-color-warning: hsl(48deg, 100%, 50%);
 
 // Borders
 $border-color: hsl(0deg, 0%, 40%);
diff --git a/src/proxmox-dark/scss/extjs/_menu.scss b/src/proxmox-dark/scss/extjs/_menu.scss
index 2983f60..aa51260 100644
--- a/src/proxmox-dark/scss/extjs/_menu.scss
+++ b/src/proxmox-dark/scss/extjs/_menu.scss
@@ -33,6 +33,10 @@
   color: $icon-color;
 }
 
+.x-menu-item-icon-default.warning {
+  color: $text-color-warning;
+}
+
 // Vertical divider (e.g. in UserInfo between icons and text)
 .x-menu-icon-separator-default {
   background-color: $background-dark;
-- 
2.39.2





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

* Re: [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon
  2023-09-22 14:36 ` [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon Philipp Hufnagl
@ 2023-10-06 13:16   ` Lukas Wagner
  2023-10-10 10:15     ` Philipp Hufnagl
  0 siblings, 1 reply; 15+ messages in thread
From: Lukas Wagner @ 2023-10-06 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Philipp Hufnagl

Comments inline.

On 9/22/23 16:36, Philipp Hufnagl wrote:
> When the user account that is currently logged in will expire soon, the
> user icon will turn into a yellow exclamation mark. In the user menu
> there will be a new element informing the user briefly about it. If the
                                                           ^
Well, the warning is permanent, so I wouldn't call it 'briefly' ;)
> element is clicked, a popup will appear informing the user in detail
> about it
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>   www/manager6/Workspace.js | 28 ++++++++++++++++++++++++++++
>   1 file changed, 28 insertions(+)
> 
> diff --git a/www/manager6/Workspace.js b/www/manager6/Workspace.js
> index 18d574b7..9d166493 100644
> --- a/www/manager6/Workspace.js
> +++ b/www/manager6/Workspace.js
> @@ -42,6 +42,7 @@ Ext.define('PVE.Workspace', {
>   	Proxmox.Utils.authClear();
>   	Ext.state.Manager.clear('GuiCap');
>   	Proxmox.UserName = null;
> +	Proxmox.UserExpires = null;

Another variable name might convey the meaning better, as this name
somewhat reads booleanesque.
How about Proxmox.AccountExpiresAt or Proxmox.AccountExpirationDate?

>   	me.loginData = null;
>   
>   	if (!me.login) {
> @@ -198,6 +199,18 @@ Ext.define('PVE.StdWorkspace', {
>   	let me = this;
>   	let ui = me.query('#userinfo')[0];
>   	ui.setText(Ext.String.htmlEncode(Proxmox.UserName || ''));
> +	let label = me.query('#expirewarning')[0];
> +	if (Proxmox.UserExpires !== null) {
> +	    let expieWariningThreshold = Ext.Date.add(new Date(), Ext.Date.DAY, 7);
                            ^
Typo in 'Warning'
Nit: Also I'd not abbreviate: expiryWarningThreshold

> +	    let expireDate = new Date(Proxmox.UserExpires * 1000);
> +	    if (expieWariningThreshold >= expireDate) {
> +	        ui.setIconCls('fa fa-exclamation-triangle warning');
> +		label.setHidden(false);
> +	    }
> +	} else {
> +		label.setHidden(true);
> +		ui.setIconCls('fa fa-user');
> +	}
>   	ui.updateLayout();
>       },
>   
> @@ -367,6 +380,21 @@ Ext.define('PVE.StdWorkspace', {
>   			    },
>   			    iconCls: 'fa fa-user',
>   			    menu: [
> +				{
> +				    iconCls: 'fa fa-exclamation-triangle warning',
> +				    itemId: 'expirewarning',
> +				    text: gettext('Account expiring soon!'),
> +				    hidden: true,
> +				    handler: function() {
> +					let expireDate = new Date(Proxmox.UserExpires * 1000);
> +					Ext.Msg.show({
> +					    title: gettext('Account expiring soon!'),
> +					    icon: Ext.Msg.WARNING,
> +					    message: Ext.String.format(gettext("Your account is expiring on {0} . After that you won't be able to log in!"), expireDate),
> +					    buttons: Ext.Msg.OK,
> +					});
> +				    },
> +				},
>   				{
>   				    iconCls: 'fa fa-gear',
>   				    text: gettext('My Settings'),

-- 
- Lukas




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

* Re: [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon Philipp Hufnagl
@ 2023-10-06 13:16   ` Lukas Wagner
  2023-10-06 14:41     ` Stefan Sterz
  0 siblings, 1 reply; 15+ messages in thread
From: Lukas Wagner @ 2023-10-06 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Philipp Hufnagl

Comments inline.

On 9/22/23 16:36, Philipp Hufnagl wrote:
> Adds a new css class to underlay information urgency in table columns
> for dark and light mode. This underlay color then is used to notifiy
Typo in 'notify'
> user administrators about user accounts that will expire soon
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>   src/Utils.js                                    | 6 +++++-
>   src/css/ext6-pmx.css                            | 4 ++++
>   src/proxmox-dark/scss/abstracts/_variables.scss | 1 +
>   src/proxmox-dark/scss/proxmox/_general.scss     | 5 +++++
>   4 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/src/Utils.js b/src/Utils.js
> index f269607..a7ded2a 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -174,10 +174,14 @@ utilities: {
>   	return value ? Proxmox.Utils.enabledText : Proxmox.Utils.disabledText;
>       },
>   
> -    format_expire: function(date) {
> +    format_expire: function(date, meta) {
>   	if (!date) {
>   	    return Proxmox.Utils.neverText;
>   	}
> +	let expieWariningThreshold = Ext.Date.add(new Date(), Ext.Date.DAY, 7);
                       ^
Typo, also same remarks as in the other patch.
> +	if (expieWariningThreshold >= date) {
> +	    meta.tdCls += 'proxmox-hint-row';
> +	}
>   	return Ext.Date.format(date, "Y-m-d");
>       },
>   
> diff --git a/src/css/ext6-pmx.css b/src/css/ext6-pmx.css
> index 2ffd2a8..439f3a0 100644
> --- a/src/css/ext6-pmx.css
> +++ b/src/css/ext6-pmx.css
> @@ -76,6 +76,10 @@
>       background-color: #f3d6d7;
>   }
>   
> +.proxmox-hint-row {
> +    background-color: #5eb9ff;
> +}
> +
>   .proxmox-warning-row {
>       background-color: #f5e5d8;
>   }
> diff --git a/src/proxmox-dark/scss/abstracts/_variables.scss b/src/proxmox-dark/scss/abstracts/_variables.scss
> index cac51eb..8bcae09 100644
> --- a/src/proxmox-dark/scss/abstracts/_variables.scss
> +++ b/src/proxmox-dark/scss/abstracts/_variables.scss
> @@ -26,6 +26,7 @@ $background-darker: hsl(0deg, 0%, 15%);
>   $background-darkest: hsl(0deg, 0%, 10%);
>   $background-invalid: hsl(360deg, 60%, 20%);
>   $background-warning: hsl(40deg, 100%, 20%);
> +$background-hint: hsl(233deg, 99%, 60%);

That particular color tone looks pretty out of place to me in dark mode.
In light mode, you use the same hue as other interface elements, is 
there a reason why you use a different color in dark mode?

Playing around a bit, hsl(205, 100%, 40%) or hsl(205, 100%, 45%) would 
look about right for me, that's the same hue as other elements,
while being a bit toned down (reduced lightness).

@sterzy, maybe you can provide some feedback as well?

Also, small bug: the hint-color seems to disappear when the 'expiry'
is clicked, only seems to affect dark mode.

>   
>   // Buttons
>   $neutral-button-color: hsl(0deg, 0%, 25%);
> diff --git a/src/proxmox-dark/scss/proxmox/_general.scss b/src/proxmox-dark/scss/proxmox/_general.scss
> index c319f6d..8b2a4d0 100644
> --- a/src/proxmox-dark/scss/proxmox/_general.scss
> +++ b/src/proxmox-dark/scss/proxmox/_general.scss
> @@ -25,6 +25,11 @@ div[id^="versioninfo-"] + div[id^="panel-"] > div[id^="panel-"][id$="-bodyWrap"]
>     background-color: $background-warning;
>   }
>   
> +// Info rows, e.g. when an user account expires soon
> +.proxmox-hint-row {
> +  background-color: $background-hint;
> +}
> +
>   // Disabled rows (e.g. disabled repos in Repository view)
>   .proxmox-disabled-row td {
>     color: $text-color-inactive;

-- 
- Lukas




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

* Re: [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call
  2023-09-22 14:36 ` [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call Philipp Hufnagl
@ 2023-10-06 13:16   ` Lukas Wagner
  0 siblings, 0 replies; 15+ messages in thread
From: Lukas Wagner @ 2023-10-06 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Philipp Hufnagl

Comments inline.

On 9/22/23 16:36, Philipp Hufnagl wrote:
> Adds an additional, optional parameter to the access/tickets api call
> which tells when the currently used user account will expire. If it will
> not expire, the parameter will not be added.
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>   src/PVE/API2/AccessControl.pm | 8 ++++++++
>   src/PVE/AccessControl.pm      | 8 ++++++++
>   2 files changed, 16 insertions(+)
> 
> diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
> index 74b3910..e562a97 100644
> --- a/src/PVE/API2/AccessControl.pm
> +++ b/src/PVE/API2/AccessControl.pm
> @@ -267,6 +267,11 @@ __PACKAGE__->register_method ({
>   	    ticket => { type => 'string', optional => 1},
>   	    CSRFPreventionToken => { type => 'string', optional => 1 },
>   	    clustername => { type => 'string', optional => 1 },
> +	    user_expires => {

I'd prefer a different property name. `user_expires` kinda suggests it
to be a boolean variable (e.g. whether the user expires at *some point*)
How about `account-expiry-date`? Also note that prefer to use hyphens
(-) instead of underscores (_) for new properties.

> +		type => 'number',
> +                description => "When the user account expires.",
I'd maybe elaborate a bit: "Account expiration date as a UNIX timestamp"
or something like that.

> +		optional => 1 ,
> +	    },
>   	    # cap => computed api permissions, unless there's a u2f challenge
>   	}
>       },
> @@ -304,6 +309,9 @@ __PACKAGE__->register_method ({
>   	    die PVE::Exception->new("authentication failure\n", code => 401);
>   	}
>   
> +	my $exp = PVE::AccessControl::lookup_user_expiration($username);
> +	$res->{user_expieres} = $exp if defined($exp);

Typo in `user_expieres`
Nit: maybe call it $expires/$expiry instead of $exp. Not much longer
but easier to follow.

> +
>   	$res->{cap} = $rpcenv->compute_api_permission($username)
>   	    if !defined($res->{NeedTFA});
>   
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index cc0f00b..471cc92 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1234,6 +1234,14 @@ sub lookup_username {
>       return $username;
>   }
>   
> +sub lookup_user_expiration {
> +    my ($username) = @_;
> +    my $usercfg = cfs_read_file('user.cfg');
> +    my $exp = $usercfg->{users}->{$username}->{expire};
> +    return undef if $exp == 0;
> +    return $exp;
> +}

Nit: Same as above


-- 
- Lukas




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

* Re: [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days
  2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
                   ` (3 preceding siblings ...)
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI Philipp Hufnagl
@ 2023-10-06 13:16 ` Lukas Wagner
  2023-10-09 13:07   ` Philipp Hufnagl
  4 siblings, 1 reply; 15+ messages in thread
From: Lukas Wagner @ 2023-10-06 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Philipp Hufnagl



On 9/22/23 16:36, Philipp Hufnagl wrote:
> Currently, when an user account expires, it catches the users by
> surprise. It would be helpful to notify the the user as well as the
> administrator.
> 
> This patch highlights such accounts in the user account pannel and also
> shows for regular user an exclamation mark in the user info pannel. It
> also adds in this case a new entry briefly saing that the account
> expires. When this entry is clicked it states this in more detail
> including the experation date.
> 
> Sending this for PVE only now to get feedback before implementing this
> for PMG and PBS
> 

Tested this on the latest master. Seems to work as adverised with the
exception of a small rendering bug (decribed in one of the other messages).



-- 
- Lukas




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

* Re: [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI Philipp Hufnagl
@ 2023-10-06 13:16   ` Lukas Wagner
  2023-10-06 14:41   ` Stefan Sterz
  1 sibling, 0 replies; 15+ messages in thread
From: Lukas Wagner @ 2023-10-06 13:16 UTC (permalink / raw)
  To: Proxmox VE development discussion, Philipp Hufnagl

Comments inline.

On 9/22/23 16:36, Philipp Hufnagl wrote:
> When an user experation date is send with the /accesss/tickets POST API
                     ^                   ^               ^
some minor typos: expiration          sent             access
> call, it will be stored in a global variable like the username
> 
> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
(...)
> diff --git a/src/Utils.js b/src/Utils.js
> index a7ded2a..5481a32 100644
> --- a/src/Utils.js
> +++ b/src/Utils.js
> @@ -309,6 +309,9 @@ utilities: {
>   
>       setAuthData: function(data) {
>   	Proxmox.UserName = data.username;
> +	if (data.user_expieres !== '') {
Shouldn't this be !== null?

So maybe just do a
if (data['account-expiry-date']) {
     ...
}

> +	    Proxmox.UserExpires = data.user_expieres;
typo, and same general remark regarding the naming as in the
`access-control` patch.> +	}
>   	Proxmox.LoggedOut = data.LoggedOut;
>   	// creates a session cookie (expire = null)
>   	// that way the cookie gets deleted after the browser window is closed

Also, the CSS changes found in this commit should probably be in another 
commit.

Furthermore, I'd probably send the widget-toolkit patches before the
pve-manager patches, since you require the `Proxmox.UserExpires`
variable to be set in your changes for `pve-manager`.

-- 
- Lukas




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

* Re: [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon
  2023-10-06 13:16   ` Lukas Wagner
@ 2023-10-06 14:41     ` Stefan Sterz
  2023-10-10 10:18       ` Philipp Hufnagl
  0 siblings, 1 reply; 15+ messages in thread
From: Stefan Sterz @ 2023-10-06 14:41 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox VE development discussion, Philipp Hufnagl

On Fri Oct 6, 2023 at 3:16 PM CEST, Lukas Wagner wrote:
-- snip 8< --

> > +$background-hint: hsl(233deg, 99%, 60%);
>
> That particular color tone looks pretty out of place to me in dark mode.
> In light mode, you use the same hue as other interface elements, is
> there a reason why you use a different color in dark mode?
>
> Playing around a bit, hsl(205, 100%, 40%) or hsl(205, 100%, 45%) would
> look about right for me, that's the same hue as other elements,
> while being a bit toned down (reduced lightness).
>
> @sterzy, maybe you can provide some feedback as well?
>
> Also, small bug: the hint-color seems to disappear when the 'expiry'
> is clicked, only seems to affect dark mode.

in general: please refrain from defining new colors. you are very likely
not the first person that needs to highlight something in a row.

in this case: why not use either `$primary-dark` if it needs to be blue.
or in my opinion: use `$background-warning`. this would be semantically
fitting as you want to warn an admin that this user is about to expire.

for the light theme you could simply use the `.warning` class instead
then you can simply overwrite that with a more specific selector in the
dark theme again, like we already do in a couple of places.

defining new colors each time you want to highlight something in the ui
quikly leads to a visual mess, instead try to re-use colors that already
have some kind of semantic meaning. this will make it easier to visually
parse the ui quikly.

-- snip 8< --




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

* Re: [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI
  2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI Philipp Hufnagl
  2023-10-06 13:16   ` Lukas Wagner
@ 2023-10-06 14:41   ` Stefan Sterz
  1 sibling, 0 replies; 15+ messages in thread
From: Stefan Sterz @ 2023-10-06 14:41 UTC (permalink / raw)
  To: Proxmox VE development discussion

On Fri Sep 22, 2023 at 4:36 PM CEST, Philipp Hufnagl wrote:

-- snip 8< --

> +$text-color-warning: hsl(48deg, 100%, 50%);
>
>  // Borders
>  $border-color: hsl(0deg, 0%, 40%);
> diff --git a/src/proxmox-dark/scss/extjs/_menu.scss b/src/proxmox-dark/scss/extjs/_menu.scss
> index 2983f60..aa51260 100644
> --- a/src/proxmox-dark/scss/extjs/_menu.scss
> +++ b/src/proxmox-dark/scss/extjs/_menu.scss
> @@ -33,6 +33,10 @@
>    color: $icon-color;
>  }
>
> +.x-menu-item-icon-default.warning {
> +  color: $text-color-warning;
> +}
> +

same here, please don't define new colors unless you must. in this case
there is an argument to be made that this needs to be a new color as we
don't have a text color for warnings.

except, we kind of do: gauges use a brigther version of the warning and
invalid background colors. please factor these out into their own
variables and use those here and in `src/proxmox-dark/scss/_charts.scss`,
which is where we curretnly define the brigther colors.

-- snip 8< --




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

* Re: [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days
  2023-10-06 13:16 ` [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Lukas Wagner
@ 2023-10-09 13:07   ` Philipp Hufnagl
  0 siblings, 0 replies; 15+ messages in thread
From: Philipp Hufnagl @ 2023-10-09 13:07 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox VE development discussion



On 10/6/23 15:16, Lukas Wagner wrote:
> 
> 
> On 9/22/23 16:36, Philipp Hufnagl wrote:
>> Currently, when an user account expires, it catches the users by
>> surprise. It would be helpful to notify the the user as well as the
>> administrator.
>>
>> This patch highlights such accounts in the user account pannel and also
>> shows for regular user an exclamation mark in the user info pannel. It
>> also adds in this case a new entry briefly saing that the account
>> expires. When this entry is clicked it states this in more detail
>> including the experation date.
>>
>> Sending this for PVE only now to get feedback before implementing this
>> for PMG and PBS
>>
> 
> Tested this on the latest master. Seems to work as adverised with the
> exception of a small rendering bug (decribed in one of the other
> messages).
> 
> 
> 

Thanks for all the valuable feedback! I am currently working on a v2.
Please don't merge!




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

* Re: [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon
  2023-10-06 13:16   ` Lukas Wagner
@ 2023-10-10 10:15     ` Philipp Hufnagl
  0 siblings, 0 replies; 15+ messages in thread
From: Philipp Hufnagl @ 2023-10-10 10:15 UTC (permalink / raw)
  To: Lukas Wagner, Proxmox VE development discussion



On 10/6/23 15:16, Lukas Wagner wrote:
> Comments inline.
> 
> On 9/22/23 16:36, Philipp Hufnagl wrote:
>> When the user account that is currently logged in will expire soon, the
>> user icon will turn into a yellow exclamation mark. In the user menu
>> there will be a new element informing the user briefly about it. If the
>                                                           ^
> Well, the warning is permanent, so I wouldn't call it 'briefly' ;)
>> element is clicked, a popup will appear informing the user in detail
>> about it

I meant briefly as in "in brief words" :)




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

* Re: [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon
  2023-10-06 14:41     ` Stefan Sterz
@ 2023-10-10 10:18       ` Philipp Hufnagl
  0 siblings, 0 replies; 15+ messages in thread
From: Philipp Hufnagl @ 2023-10-10 10:18 UTC (permalink / raw)
  To: Stefan Sterz, Lukas Wagner, Proxmox VE development discussion



> for the light theme you could simply use the `.warning` class instead
> then you can simply overwrite that with a more specific selector in the
> dark theme again, like we already do in a couple of places.

Thank you. I tried using .warning. Unfortunately the result was yellow
on white background, with very poor readability. Therefore I decided
to use the warning highlighting for columns in both themes.




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

end of thread, other threads:[~2023-10-10 10:18 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-22 14:36 [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Philipp Hufnagl
2023-09-22 14:36 ` [pve-devel] [PATCH manager v1 1/1] fix #4546: ui: notify user if their user account expires soon Philipp Hufnagl
2023-10-06 13:16   ` Lukas Wagner
2023-10-10 10:15     ` Philipp Hufnagl
2023-09-22 14:36 ` [pve-devel] [PATCH access-control v1 1/1] fix #4546: api: Return user expiration date on access/ticket API call Philipp Hufnagl
2023-10-06 13:16   ` Lukas Wagner
2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 1/2] fix #4546: css: Inform user administrator about user accounts expiring soon Philipp Hufnagl
2023-10-06 13:16   ` Lukas Wagner
2023-10-06 14:41     ` Stefan Sterz
2023-10-10 10:18       ` Philipp Hufnagl
2023-09-22 14:36 ` [pve-devel] [PATCH proxmox-widget-toolkit v1 2/2] fix #4546: utils: save expiring date of user account for UI Philipp Hufnagl
2023-10-06 13:16   ` Lukas Wagner
2023-10-06 14:41   ` Stefan Sterz
2023-10-06 13:16 ` [pve-devel] [PATCH manager/access-control/proxmox-widget-toolkit v1 0/4] fix #4546: Show warning hint/badge if user account is expiring in next few days Lukas Wagner
2023-10-09 13:07   ` Philipp Hufnagl

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