[pbs-devel] [PATCH proxmox-backup 0/6] tfa fixups

[pbs-devel] [PATCH proxmox-backup 0/6] tfa fixups

[Dominik Csapak]

fixed some of the more weird spacing/style issues i saw

there are still some issues, but maybe someone else can look at it:

* there is no gui for initial webauthn setup (which is mandatory)
* The text 'Copy Secret Value' is weird, but i could not come up with
  something better for now
* on login, the text should not be 'OTP' for recovery keys, it should be
  distinct, else it is very confusing
* the spacing on the login windows and deletion confirm window is still
  wrong
* the deletion confirm window has the title 'Confirm Password', but as
  root@pam, there is no password field (better only "Conifrm"?)
  we maybe can reuse our 'Safe Destroy Window' here?
* when i enter a wrong password, the error message is

AUTH_ERR (7)

  which is not informational, and not consistent with our login.
* Some edit/add windows can be refactored using the Edit Window,
  currently the functionality is duplicated across them
  (addWebauthn, addTotp, etc.)
* The warning that there are only few recovery codes left has too many
  warning icons (one should be enough)
* Sometimes i ran into an issue, where, if i have the recovery login
  window open for too long, the code is not accepted. It was not very
  clear to me if i can reuse that code again (i could) but that seems
  to be bad ux, since normally a recovery code is not something i have
  on hand, and it'll take a while until i found one.


Dominik Csapak (6):
  ui: LoginView: remove not used viewModel
  ui: config/TfaView: disable Remove button by default
  ui: window/AddTfaRecovery: rewrite to a Proxmox.window.Edit
  ui: window/AddTfaRecovery: fix style of TfaRecoveryShow window
  ui: window/AddTotp: fix spacing styling of form fields
  ui: window/{AddWebauthn,TfaEdit}: fix spacing/border of the windows

 www/LoginView.js             |   8 ---
 www/config/TfaView.js        |   1 +
 www/window/AddTfaRecovery.js | 136 ++++++++++++-----------------------
 www/window/AddTotp.js        |  81 ++++++++++-----------
 www/window/AddWebauthn.js    |   2 +-
 www/window/TfaEdit.js        |   1 -
 6 files changed, 86 insertions(+), 143 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/6] ui: LoginView: remove not used viewModel

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/LoginView.js | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/www/LoginView.js b/www/LoginView.js
index 305f4c0d..fa8772b5 100644
--- a/www/LoginView.js
+++ b/www/LoginView.js
@@ -255,14 +255,6 @@ Ext.define('PBS.login.TfaWindow', {
 
     defaultButton: 'totpButton',
 
-    viewModel: {
-	data: {
-	    userid: undefined,
-	    ticket: undefined,
-	    challenge: undefined,
-	},
-    },
-
     controller: {
 	xclass: 'Ext.app.ViewController',
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/6] ui: config/TfaView: disable Remove button by default

[Dominik Csapak]

gets enabled when an item is clicked

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/config/TfaView.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/config/TfaView.js b/www/config/TfaView.js
index 6d7e862a..c112a132 100644
--- a/www/config/TfaView.js
+++ b/www/config/TfaView.js
@@ -250,6 +250,7 @@ Ext.define('PBS.config.TfaView', {
 	},
 	{
 	    xtype: 'proxmoxButton',
+	    disabled: true,
 	    text: gettext('Remove'),
 	    getRecordName: rec => rec.data.description,
 	    handler: 'onRemoveButton',
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/6] ui: window/AddTfaRecovery: rewrite to a Proxmox.window.Edit

[Dominik Csapak]

we can reuse the edit window from widget toolkit for the most part
this solves some spacing and layout issues and is less code

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/window/AddTfaRecovery.js | 116 +++++++++++------------------------
 1 file changed, 36 insertions(+), 80 deletions(-)

diff --git a/www/window/AddTfaRecovery.js b/www/window/AddTfaRecovery.js
index 710c243f..a260c0ca 100644
--- a/www/window/AddTfaRecovery.js
+++ b/www/window/AddTfaRecovery.js
@@ -1,23 +1,34 @@
 Ext.define('PBS.window.AddTfaRecovery', {
-    extend: 'Ext.window.Window',
+    extend: 'Proxmox.window.Edit',
     alias: 'widget.pbsAddTfaRecovery',
     mixins: ['Proxmox.Mixin.CBind'],
 
     onlineHelp: 'user_mgmt',
-
-    modal: true,
-    resizable: false,
-    title: gettext('Add TFA recovery keys'),
+    isCreate: true,
+    isAdd: true,
+    subject: gettext('TFA recovery keys'),
     width: 512,
+    method: 'POST',
 
     fixedUser: false,
 
-    baseurl: '/api2/extjs/access/tfa',
+    url: '/api2/extjs/access/tfa',
+    submitUrl: function(url, values) {
+	let userid = values.userid;
+	delete values.userid;
+	return `${url}/${userid}`;
+    },
+
+    apiCallDone: function(success, response) {
+	if (!success) {
+	    return;
+	}
 
-    initComponent: function() {
-	let me = this;
-	me.callParent();
-	Ext.GlobalEvents.fireEvent('proxmoxShowHelp', me.onlineHelp);
+	let values = response.result.data.recovery.join("\n");
+	Ext.create('PBS.window.TfaRecoveryShow', {
+	    autoShow: true,
+	    values,
+	});
     },
 
     viewModel: {
@@ -28,27 +39,13 @@ Ext.define('PBS.window.AddTfaRecovery', {
 
     controller: {
 	xclass: 'Ext.app.ViewController',
-	control: {
-	    '#': {
-		show: function() {
-		    let me = this;
-		    let view = me.getView();
-
-		    if (Proxmox.UserName === 'root@pam') {
-			view.lookup('password').setVisible(false);
-			view.lookup('password').setDisabled(true);
-		    }
-		},
-	    },
-	},
-
 	hasEntry: async function(userid) {
 	    let me = this;
 	    let view = me.getView();
 
 	    try {
 		await PBS.Async.api2({
-		    url: `${view.baseurl}/${userid}/recovery`,
+		    url: `${view.url}/${userid}/recovery`,
 		    method: 'GET',
 		});
 		return true;
@@ -57,11 +54,11 @@ Ext.define('PBS.window.AddTfaRecovery', {
 	    }
 	},
 
-	init: function() {
+	init: function(view) {
 	    this.onUseridChange(null, Proxmox.UserName);
 	},
 
-	onUseridChange: async function(_field, userid) {
+	onUseridChange: async function(field, userid) {
 	    let me = this;
 
 	    me.userid = userid;
@@ -69,43 +66,6 @@ Ext.define('PBS.window.AddTfaRecovery', {
 	    let has_entry = await me.hasEntry(userid);
 	    me.getViewModel().set('has_entry', has_entry);
 	},
-
-	onAdd: async function() {
-	    let me = this;
-	    let view = me.getView();
-
-	    view.mask(gettext('Please wait...'), 'x-mask-loading');
-
-	    let baseurl = view.baseurl;
-
-	    let userid = me.userid;
-	    if (userid === undefined) {
-		throw "no userid set";
-	    }
-
-	    let params = { type: 'recovery' };
-
-	    if (Proxmox.UserName !== 'root@pam') {
-		params.password = me.lookup('password').getValue();
-	    }
-
-	    try {
-		let response = await PBS.Async.api2({
-		    url: `${baseurl}/${userid}`,
-		    method: 'POST',
-		    params,
-		});
-		let values = response.result.data.recovery.join("\n");
-		Ext.create('PBS.window.TfaRecoveryShow', {
-		    autoShow: true,
-		    values,
-		});
-	    } catch (ex) {
-		Ext.Msg.alert(gettext('Error'), ex);
-	    } finally {
-		view.close();
-	    }
-	},
     },
 
     items: [
@@ -119,6 +79,9 @@ Ext.define('PBS.window.AddTfaRecovery', {
 	    editConfig: {
 		xtype: 'pbsUserSelector',
 		allowBlank: false,
+		validator: function(_value) {
+		    return !this.up('window').getViewModel().get('has_entry');
+		},
 	    },
 	    renderer: Ext.String.htmlEncode,
 	    value: Proxmox.UserName,
@@ -126,11 +89,18 @@ Ext.define('PBS.window.AddTfaRecovery', {
 		change: 'onUseridChange',
 	    },
 	},
+	{
+	    xtype: 'hiddenfield',
+	    name: 'type',
+	    value: 'recovery',
+	},
 	{
 	    xtype: 'displayfield',
 	    bind: {
 		hidden: '{!has_entry}',
 	    },
+	    hidden: true,
+	    userCls: 'pmx-hint',
 	    value: gettext('User already has recovery keys.'),
 	},
 	{
@@ -142,25 +112,11 @@ Ext.define('PBS.window.AddTfaRecovery', {
 	    name: 'password',
 	    allowBlank: false,
 	    validateBlank: true,
-	    padding: '0 0 5 5',
+	    hidden: Proxmox.UserName === 'root@pam',
+	    disabled: Proxmox.UserName === 'root@pam',
 	    emptyText: gettext('verify current password'),
 	},
     ],
-
-    buttons: [
-	{
-	    xtype: 'proxmoxHelpButton',
-	},
-	'->',
-	{
-	    xtype: 'button',
-	    text: gettext('Add'),
-	    handler: 'onAdd',
-	    bind: {
-		disabled: '{has_entry}',
-	    },
-	},
-    ],
 });
 
 Ext.define('PBS.window.TfaRecoveryShow', {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/6] ui: window/AddTfaRecovery: fix style of TfaRecoveryShow window

[Dominik Csapak]

to have a more similar layout/spacing to our other windows

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/window/AddTfaRecovery.js | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/www/window/AddTfaRecovery.js b/www/window/AddTfaRecovery.js
index a260c0ca..1b63031f 100644
--- a/www/window/AddTfaRecovery.js
+++ b/www/window/AddTfaRecovery.js
@@ -131,15 +131,13 @@ Ext.define('PBS.window.TfaRecoveryShow', {
 
     items: [
 	{
-	    xtype: 'container',
-	    layout: 'form',
+	    xtype: 'form',
+	    layout: 'anchor',
 	    bodyPadding: 10,
 	    border: false,
 	    fieldDefaults: {
-		labelWidth: 100,
 		anchor: '100%',
             },
-	    padding: '0 10 10 10',
 	    items: [
 		{
 		    xtype: 'textarea',
@@ -153,15 +151,15 @@ Ext.define('PBS.window.TfaRecoveryShow', {
 		    },
 		    height: '160px',
 		},
+		{
+		    xtype: 'displayfield',
+		    border: false,
+		    padding: '5 0 0 0',
+		    userCls: 'pmx-hint',
+		    value: gettext('Please record recovery keys - they will only be displayed now'),
+		},
 	    ],
 	},
-	{
-	    xtype: 'component',
-	    border: false,
-	    padding: '10 10 10 10',
-	    userCls: 'pmx-hint',
-	    html: gettext('Please record recovery keys - they will only be displayed now'),
-	},
     ],
     buttons: [
 	{
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/6] ui: window/AddTotp: fix spacing styling of form fields

[Dominik Csapak]

by moving the lower fields into the form itself and dropping the padding

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/window/AddTotp.js | 81 +++++++++++++++++++++----------------------
 1 file changed, 39 insertions(+), 42 deletions(-)

diff --git a/www/window/AddTotp.js b/www/window/AddTotp.js
index 40417340..414147e7 100644
--- a/www/window/AddTotp.js
+++ b/www/window/AddTotp.js
@@ -125,7 +125,6 @@ Ext.define('PBS.window.AddTotp', {
 	    reference: 'totp_form',
 	    fieldDefaults: {
 		anchor: '100%',
-		padding: '0 5',
 	    },
 	    items: [
 		{
@@ -205,49 +204,47 @@ Ext.define('PBS.window.AddTotp', {
 		    value: `Proxmox Backup Server - ${Proxmox.NodeName}`,
 		    qrupdate: true,
 		},
+		{
+		    xtype: 'box',
+		    itemId: 'qrbox',
+		    visible: false, // will be enabled when generating a qr code
+		    bind: {
+			visible: '{!secretEmpty}',
+		    },
+		    style: {
+			'background-color': 'white',
+			'margin-left': 'auto',
+			'margin-right': 'auto',
+			padding: '5px',
+			width: '266px',
+			height: '266px',
+		    },
+		},
+		{
+		    xtype: 'textfield',
+		    fieldLabel: gettext('Verification Code'),
+		    allowBlank: false,
+		    reference: 'challenge',
+		    name: 'challenge',
+		    bind: {
+			disabled: '{!showTOTPVerifiction}',
+			visible: '{showTOTPVerifiction}',
+		    },
+		    emptyText: gettext('Scan QR code and enter TOTP auth. code to verify'),
+		},
+		{
+		    xtype: 'textfield',
+		    inputType: 'password',
+		    fieldLabel: gettext('Password'),
+		    minLength: 5,
+		    reference: 'password',
+		    name: 'password',
+		    allowBlank: false,
+		    validateBlank: true,
+		    emptyText: gettext('verify current password'),
+		},
 	    ],
 	},
-	{
-	    xtype: 'box',
-	    itemId: 'qrbox',
-	    visible: false, // will be enabled when generating a qr code
-	    bind: {
-		visible: '{!secretEmpty}',
-	    },
-	    style: {
-		'background-color': 'white',
-		'margin-left': 'auto',
-		'margin-right': 'auto',
-		padding: '5px',
-		width: '266px',
-		height: '266px',
-	    },
-	},
-	{
-	    xtype: 'textfield',
-	    fieldLabel: gettext('Verification Code'),
-	    allowBlank: false,
-	    reference: 'challenge',
-	    name: 'challenge',
-	    bind: {
-		disabled: '{!showTOTPVerifiction}',
-		visible: '{showTOTPVerifiction}',
-	    },
-	    padding: '0 5',
-	    emptyText: gettext('Scan QR code and enter TOTP auth. code to verify'),
-	},
-	{
-	    xtype: 'textfield',
-	    inputType: 'password',
-	    fieldLabel: gettext('Password'),
-	    minLength: 5,
-	    reference: 'password',
-	    name: 'password',
-	    allowBlank: false,
-	    validateBlank: true,
-	    padding: '0 0 5 5',
-	    emptyText: gettext('verify current password'),
-	},
     ],
 
     initComponent: function() {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/6] ui: window/{AddWebauthn, TfaEdit}: fix spacing/border of the windows

[Dominik Csapak]

the password field should not be indented differently than the rest of
the fields, and we never have a border on the panels

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/window/AddWebauthn.js | 2 +-
 www/window/TfaEdit.js     | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/www/window/AddWebauthn.js b/www/window/AddWebauthn.js
index 7c5489e3..195a756d 100644
--- a/www/window/AddWebauthn.js
+++ b/www/window/AddWebauthn.js
@@ -135,6 +135,7 @@ Ext.define('PBS.window.AddWebauthn', {
 	    xtype: 'form',
 	    reference: 'webauthn_form',
 	    layout: 'anchor',
+	    border: false,
 	    bodyPadding: 10,
 	    fieldDefaults: {
 		anchor: '100%',
@@ -171,7 +172,6 @@ Ext.define('PBS.window.AddWebauthn', {
 		    name: 'password',
 		    allowBlank: false,
 		    validateBlank: true,
-		    padding: '0 0 5 5',
 		    emptyText: gettext('verify current password'),
 		},
 	    ],
diff --git a/www/window/TfaEdit.js b/www/window/TfaEdit.js
index 182da33b..abec1335 100644
--- a/www/window/TfaEdit.js
+++ b/www/window/TfaEdit.js
@@ -75,7 +75,6 @@ Ext.define('PBS.window.TfaEdit', {
 	    name: 'password',
 	    allowBlank: false,
 	    validateBlank: true,
-	    padding: '0 0 5 5',
 	    emptyText: gettext('verify current password'),
 	},
     ],
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 0/6] tfa fixups

[Dominik Csapak]

On 1/13/21 12:06 PM, Dominik Csapak wrote:
> fixed some of the more weird spacing/style issues i saw
> 
> there are still some issues, but maybe someone else can look at it:
> 
> 

of course i forgot one:

i'd like to add recovery keys even if i have some, since
if there are only a few left, i do not want to remove them first
and add then new ones (what if my computer crashes in between?)

[pbs-devel] applied-series: [PATCH proxmox-backup 0/6] tfa fixups

[Thomas Lamprecht]

On 13.01.21 12:06, Dominik Csapak wrote:
> fixed some of the more weird spacing/style issues i saw
> 
> there are still some issues, but maybe someone else can look at it:
> 
> * there is no gui for initial webauthn setup (which is mandatory)
> * The text 'Copy Secret Value' is weird, but i could not come up with
>   something better for now
> * on login, the text should not be 'OTP' for recovery keys, it should be
>   distinct, else it is very confusing
> * the spacing on the login windows and deletion confirm window is still
>   wrong
> * the deletion confirm window has the title 'Confirm Password', but as
>   root@pam, there is no password field (better only "Conifrm"?)

it is also weirdly formatted..

>   we maybe can reuse our 'Safe Destroy Window' here?
> * when i enter a wrong password, the error message is
> 
> AUTH_ERR (7)
> 
>   which is not informational, and not consistent with our login.
> * Some edit/add windows can be refactored using the Edit Window,
>   currently the functionality is duplicated across them
>   (addWebauthn, addTotp, etc.)
> * The warning that there are only few recovery codes left has too many
>   warning icons (one should be enough)
> * Sometimes i ran into an issue, where, if i have the recovery login
>   window open for too long, the code is not accepted. It was not very
>   clear to me if i can reuse that code again (i could) but that seems
>   to be bad ux, since normally a recovery code is not something i have
>   on hand, and it'll take a while until i found one.
> 
> 
> Dominik Csapak (6):
>   ui: LoginView: remove not used viewModel
>   ui: config/TfaView: disable Remove button by default
>   ui: window/AddTfaRecovery: rewrite to a Proxmox.window.Edit
>   ui: window/AddTfaRecovery: fix style of TfaRecoveryShow window
>   ui: window/AddTotp: fix spacing styling of form fields
>   ui: window/{AddWebauthn,TfaEdit}: fix spacing/border of the windows
> 
>  www/LoginView.js             |   8 ---
>  www/config/TfaView.js        |   1 +
>  www/window/AddTfaRecovery.js | 136 ++++++++++++-----------------------
>  www/window/AddTotp.js        |  81 ++++++++++-----------
>  www/window/AddWebauthn.js    |   2 +-
>  www/window/TfaEdit.js        |   1 -
>  6 files changed, 86 insertions(+), 143 deletions(-)
> 


applied series, much thanks for those initial fixes!

@Wolfgang, maybe you can also tackle a few of those when sending patches for adding
the initial setup part - after all the JS/gui part got pushed without any real review
by you ;P