all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left
@ 2023-05-17 11:59 Wolfgang Bumiller
  2023-05-17 13:22 ` Wolfgang Bumiller
  2023-05-25  7:28 ` Dominik Csapak
  0 siblings, 2 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2023-05-17 11:59 UTC (permalink / raw)
  To: pve-devel

If we get an empty challenge, tell the user to contact an
administrator as it means no 2nd factors and no recovery
keys are available.

Currently if only 1 key was available and it had a high ID,
we'd show something like: "Recovery keys available: 9,
Warning, less than 4 keys available."
Let's start off with the warning, and then be explicit about
the IDs.

Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
---
Changes to v1 address Dominik's remarks:
  - Fix typos
  - Make generated ID list format string right-to-left compatible
  - Replace the pair of `hasAny2nd` and `hasNonRecovery2nd` with a
    counter and a `hasRecovery` boolean which allows expressing the
    condition that "either no 2nd factors, or only an already used up set of
    recovery keys exists" with less cognitive overhead.

 src/window/TfaWindow.js | 74 ++++++++++++++++++++++++++++++++---------
 1 file changed, 58 insertions(+), 16 deletions(-)

diff --git a/src/window/TfaWindow.js b/src/window/TfaWindow.js
index 22ac50d..a622ce1 100644
--- a/src/window/TfaWindow.js
+++ b/src/window/TfaWindow.js
@@ -45,11 +45,17 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
 
 	    let lastTabId = me.getLastTabUsed();
 	    let initialTab = -1, i = 0;
+	    let count2nd = 0;
+	    let hasRecovery = false;
 	    for (const k of ['webauthn', 'totp', 'recovery', 'u2f', 'yubico']) {
 		const available = !!challenge[k];
 		vm.set(`availableChallenge.${k}`, available);
 
 		if (available) {
+		    count2nd++;
+		    if (k !== 'recovery') {
+			hasRecovery = true;
+		    }
 		    if (i === lastTabId) {
 			initialTab = i;
 		    } else if (initialTab < 0) {
@@ -58,15 +64,31 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
 		}
 		i++;
 	    }
+	    if (!count2nd || (count2nd === 1 && hasRecovery && !challenge.recovery.length)) {
+		// no 2nd factors available (and if recovery keys are configured they're empty)
+		me.lookup('cannotLogin').setVisible(true);
+		me.lookup('recoveryKey').setVisible(false);
+		view.down('tabpanel').setActiveTab(2); // recovery
+		return;
+	    }
 	    view.down('tabpanel').setActiveTab(initialTab);
 
 	    if (challenge.recovery) {
-		me.lookup('availableRecovery').update(Ext.String.htmlEncode(
-		    gettext('Available recovery keys: ') + view.challenge.recovery.join(', '),
-		));
-		me.lookup('availableRecovery').setVisible(true);
-		if (view.challenge.recovery.length <= 3) {
-		    me.lookup('recoveryLow').setVisible(true);
+		if (!view.challenge.recovery.length) {
+		    me.lookup('recoveryEmpty').setVisible(true);
+		} else {
+		    let idList = view
+			    .challenge
+			    .recovery
+			    .map((id) => Ext.String.format(gettext('ID {0}'), id))
+			    .join(', ');
+		    me.lookup('availableRecovery').update(Ext.String.htmlEncode(
+			Ext.String.format(gettext('Available recovery keys: {0}'), idList),
+		    ));
+		    me.lookup('availableRecovery').setVisible(true);
+		    if (view.challenge.recovery.length <= 3) {
+			me.lookup('recoveryLow').setVisible(true);
+		    }
 		}
 	    }
 
@@ -365,19 +387,23 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
 		items: [
 		    {
 			xtype: 'box',
-			reference: 'availableRecovery',
+			reference: 'cannotLogin',
 			hidden: true,
+			html: '<i class="fa fa-exclamation-triangle warning"></i>'
+			    + Ext.String.format(
+				gettext('No second factor left! Please contact an administrator!'),
+				4,
+			    ),
 		    },
 		    {
-			xtype: 'textfield',
-			fieldLabel: gettext('Please enter one of your single-use recovery keys'),
-			labelWidth: 300,
-			name: 'recoveryKey',
-			disabled: true,
-			reference: 'recoveryKey',
-			allowBlank: false,
-			regex: /^[0-9a-f]{4}(-[0-9a-f]{4}){3}$/,
-			regexText: gettext('Does not look like a valid recovery key'),
+			xtype: 'box',
+			reference: 'recoveryEmpty',
+			hidden: true,
+			html: '<i class="fa fa-exclamation-triangle warning"></i>'
+			    + Ext.String.format(
+				gettext('No more recovery keys left! Please generate a new set!'),
+				4,
+			    ),
 		    },
 		    {
 			xtype: 'box',
@@ -389,6 +415,22 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
 				4,
 			    ),
 		    },
+		    {
+			xtype: 'box',
+			reference: 'availableRecovery',
+			hidden: true,
+		    },
+		    {
+			xtype: 'textfield',
+			fieldLabel: gettext('Please enter one of your single-use recovery keys'),
+			labelWidth: 300,
+			name: 'recoveryKey',
+			disabled: true,
+			reference: 'recoveryKey',
+			allowBlank: false,
+			regex: /^[0-9a-f]{4}(-[0-9a-f]{4}){3}$/,
+			regexText: gettext('Does not look like a valid recovery key'),
+		    },
 		],
 	    },
 	    {
-- 
2.30.2





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

* Re: [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left
  2023-05-17 11:59 [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left Wolfgang Bumiller
@ 2023-05-17 13:22 ` Wolfgang Bumiller
  2023-05-25  7:28 ` Dominik Csapak
  1 sibling, 0 replies; 3+ messages in thread
From: Wolfgang Bumiller @ 2023-05-17 13:22 UTC (permalink / raw)
  To: pve-devel

On Wed, May 17, 2023 at 01:59:28PM +0200, Wolfgang Bumiller wrote:
> If we get an empty challenge, tell the user to contact an
> administrator as it means no 2nd factors and no recovery
> keys are available.
> 
> Currently if only 1 key was available and it had a high ID,
> we'd show something like: "Recovery keys available: 9,
> Warning, less than 4 keys available."
> Let's start off with the warning, and then be explicit about
> the IDs.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> Changes to v1 address Dominik's remarks:
>   - Fix typos
>   - Make generated ID list format string right-to-left compatible
>   - Replace the pair of `hasAny2nd` and `hasNonRecovery2nd` with a
>     counter and a `hasRecovery` boolean which allows expressing the
>     condition that "either no 2nd factors, or only an already used up set of
>     recovery keys exists" with less cognitive overhead.
> 
>  src/window/TfaWindow.js | 74 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/src/window/TfaWindow.js b/src/window/TfaWindow.js
> index 22ac50d..a622ce1 100644
> --- a/src/window/TfaWindow.js
> +++ b/src/window/TfaWindow.js
> @@ -45,11 +45,17 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
>  
>  	    let lastTabId = me.getLastTabUsed();
>  	    let initialTab = -1, i = 0;
> +	    let count2nd = 0;
> +	    let hasRecovery = false;
>  	    for (const k of ['webauthn', 'totp', 'recovery', 'u2f', 'yubico']) {
>  		const available = !!challenge[k];
>  		vm.set(`availableChallenge.${k}`, available);
>  
>  		if (available) {
> +		    count2nd++;
> +		    if (k !== 'recovery') {

^ Dominik noted that is a typo btw., needs to be `===`.




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

* Re: [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left
  2023-05-17 11:59 [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left Wolfgang Bumiller
  2023-05-17 13:22 ` Wolfgang Bumiller
@ 2023-05-25  7:28 ` Dominik Csapak
  1 sibling, 0 replies; 3+ messages in thread
From: Dominik Csapak @ 2023-05-25  7:28 UTC (permalink / raw)
  To: Proxmox VE development discussion, Wolfgang Bumiller

Aside from what wolfgang already answered, one minor thing inline

On 5/17/23 13:59, Wolfgang Bumiller wrote:
> If we get an empty challenge, tell the user to contact an
> administrator as it means no 2nd factors and no recovery
> keys are available.
> 
> Currently if only 1 key was available and it had a high ID,
> we'd show something like: "Recovery keys available: 9,
> Warning, less than 4 keys available."
> Let's start off with the warning, and then be explicit about
> the IDs.
> 
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> ---
> Changes to v1 address Dominik's remarks:
>    - Fix typos
>    - Make generated ID list format string right-to-left compatible
>    - Replace the pair of `hasAny2nd` and `hasNonRecovery2nd` with a
>      counter and a `hasRecovery` boolean which allows expressing the
>      condition that "either no 2nd factors, or only an already used up set of
>      recovery keys exists" with less cognitive overhead.
> 
>   src/window/TfaWindow.js | 74 ++++++++++++++++++++++++++++++++---------
>   1 file changed, 58 insertions(+), 16 deletions(-)
> 
> diff --git a/src/window/TfaWindow.js b/src/window/TfaWindow.js
> index 22ac50d..a622ce1 100644
> --- a/src/window/TfaWindow.js
> +++ b/src/window/TfaWindow.js
> @@ -45,11 +45,17 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
>   
>   	    let lastTabId = me.getLastTabUsed();
>   	    let initialTab = -1, i = 0;
> +	    let count2nd = 0;
> +	    let hasRecovery = false;
>   	    for (const k of ['webauthn', 'totp', 'recovery', 'u2f', 'yubico']) {
>   		const available = !!challenge[k];
>   		vm.set(`availableChallenge.${k}`, available);
>   
>   		if (available) {
> +		    count2nd++;
> +		    if (k !== 'recovery') {
> +			hasRecovery = true;
> +		    }
>   		    if (i === lastTabId) {
>   			initialTab = i;
>   		    } else if (initialTab < 0) {
> @@ -58,15 +64,31 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
>   		}
>   		i++;
>   	    }
> +	    if (!count2nd || (count2nd === 1 && hasRecovery && !challenge.recovery.length)) {
> +		// no 2nd factors available (and if recovery keys are configured they're empty)
> +		me.lookup('cannotLogin').setVisible(true);
> +		me.lookup('recoveryKey').setVisible(false);
> +		view.down('tabpanel').setActiveTab(2); // recovery
> +		return;
> +	    }
>   	    view.down('tabpanel').setActiveTab(initialTab);
>   
>   	    if (challenge.recovery) {
> -		me.lookup('availableRecovery').update(Ext.String.htmlEncode(
> -		    gettext('Available recovery keys: ') + view.challenge.recovery.join(', '),
> -		));
> -		me.lookup('availableRecovery').setVisible(true);
> -		if (view.challenge.recovery.length <= 3) {
> -		    me.lookup('recoveryLow').setVisible(true);
> +		if (!view.challenge.recovery.length) {
> +		    me.lookup('recoveryEmpty').setVisible(true);

i'd hide the recovery input field here too, otherwise you get the warning
that no recovery keys are left, but get an input field that the user
cannot fill out...

> +		} else {
> +		    let idList = view
> +			    .challenge
> +			    .recovery
> +			    .map((id) => Ext.String.format(gettext('ID {0}'), id))
> +			    .join(', ');
> +		    me.lookup('availableRecovery').update(Ext.String.htmlEncode(
> +			Ext.String.format(gettext('Available recovery keys: {0}'), idList),
> +		    ));
> +		    me.lookup('availableRecovery').setVisible(true);
> +		    if (view.challenge.recovery.length <= 3) {
> +			me.lookup('recoveryLow').setVisible(true);
> +		    }
>   		}
>   	    }
>   
> @@ -365,19 +387,23 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
>   		items: [
>   		    {
>   			xtype: 'box',
> -			reference: 'availableRecovery',
> +			reference: 'cannotLogin',
>   			hidden: true,
> +			html: '<i class="fa fa-exclamation-triangle warning"></i>'
> +			    + Ext.String.format(
> +				gettext('No second factor left! Please contact an administrator!'),
> +				4,
> +			    ),
>   		    },
>   		    {
> -			xtype: 'textfield',
> -			fieldLabel: gettext('Please enter one of your single-use recovery keys'),
> -			labelWidth: 300,
> -			name: 'recoveryKey',
> -			disabled: true,
> -			reference: 'recoveryKey',
> -			allowBlank: false,
> -			regex: /^[0-9a-f]{4}(-[0-9a-f]{4}){3}$/,
> -			regexText: gettext('Does not look like a valid recovery key'),
> +			xtype: 'box',
> +			reference: 'recoveryEmpty',
> +			hidden: true,
> +			html: '<i class="fa fa-exclamation-triangle warning"></i>'
> +			    + Ext.String.format(
> +				gettext('No more recovery keys left! Please generate a new set!'),
> +				4,
> +			    ),
>   		    },
>   		    {
>   			xtype: 'box',
> @@ -389,6 +415,22 @@ Ext.define('Proxmox.window.TfaLoginWindow', {
>   				4,
>   			    ),
>   		    },
> +		    {
> +			xtype: 'box',
> +			reference: 'availableRecovery',
> +			hidden: true,
> +		    },
> +		    {
> +			xtype: 'textfield',
> +			fieldLabel: gettext('Please enter one of your single-use recovery keys'),
> +			labelWidth: 300,
> +			name: 'recoveryKey',
> +			disabled: true,
> +			reference: 'recoveryKey',
> +			allowBlank: false,
> +			regex: /^[0-9a-f]{4}(-[0-9a-f]{4}){3}$/,
> +			regexText: gettext('Does not look like a valid recovery key'),
> +		    },
>   		],
>   	    },
>   	    {





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

end of thread, other threads:[~2023-05-25  7:28 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-17 11:59 [pve-devel] [PATCH v2 widget-toolkit] tfa: improve UX for recovery keys and when none are left Wolfgang Bumiller
2023-05-17 13:22 ` Wolfgang Bumiller
2023-05-25  7:28 ` Dominik Csapak

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal