[pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG

[pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG

[Markus Frank]

AuthEditOIDC.js is based on AuthEditOpenId from widget-toolkit and
adds additional configuration options for autocreate-role-assignment.

It uses sub/preferred_username for username-claim instead of the old
names (subject/username/email). Removed email option entirely as it is
incompatible with the username scheme.

Signed-off-by: Markus Frank <m.frank@proxmox.com>
---
v2:
* renamed subject to sub
* renamed username to preferred_username
* removed email entirely

 js/AuthEditOIDC.js | 270 +++++++++++++++++++++++++++++++++++++++++++++
 js/Makefile        |   1 +
 js/Utils.js        |   1 +
 3 files changed, 272 insertions(+)
 create mode 100644 js/AuthEditOIDC.js

diff --git a/js/AuthEditOIDC.js b/js/AuthEditOIDC.js
new file mode 100644
index 0000000..cda9d68
--- /dev/null
+++ b/js/AuthEditOIDC.js
@@ -0,0 +1,270 @@
+Ext.define('PMG.OIDCInputPanel', {
+    extend: 'Proxmox.panel.InputPanel',
+    xtype: 'pmgAuthOIDCPanel',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    showDefaultRealm: false,
+
+    type: 'oidc',
+
+    viewModel: {
+	data: {
+	    roleSource: '__default__',
+	    autocreate: 0,
+	},
+	formulas: {
+	    hideRoleAssignment: function(get) {
+		let autocreate = get('autocreate');
+		if (!autocreate) {
+		    return 1;
+		}
+		return autocreate === 0;
+	    },
+	    hideFixedRoleAssignment: function(get) {
+		return get('roleSource') !== 'fixed' || get('hideRoleAssignment');
+	    },
+	    hideClaimRoleAssignment: function(get) {
+		return get('roleSource') !== 'from-claim' || get('hideRoleAssignment');
+	    },
+	},
+    },
+
+    onGetValues: function(values) {
+	let me = this;
+
+	if (me.isCreate && !me.useTypeInUrl) {
+	    values.type = me.type;
+	}
+
+	if (values.source) {
+	    let autocreateRoleAssignment = {};
+	    autocreateRoleAssignment.source = values.source;
+	    if (values.source === 'fixed') {
+		autocreateRoleAssignment['fixed-role'] = values['fixed-role'];
+	    } else if (values.source === 'from-claim') {
+		autocreateRoleAssignment['role-claim'] = values['role-claim'];
+	    }
+	    values['autocreate-role-assignment'] =
+		Proxmox.Utils.printPropertyString(autocreateRoleAssignment);
+	}
+
+	if ((!values.autocreate || !values.source) && !me.isCreate) {
+	    if (values.delete) {
+		if (Ext.isArray(values.delete)) {
+		    values.delete.push('autocreate-role-assignment');
+		} else {
+		    values.delete += ',autocreate-role-assignment';
+		}
+	    } else {
+		values.delete = 'autocreate-role-assignment';
+	    }
+	}
+	delete values.source;
+	delete values['fixed-role'];
+	delete values['role-claim'];
+
+	return values;
+    },
+
+    setValues: function(values) {
+	let autocreateRoleAssignment =
+	    Proxmox.Utils.parsePropertyString(values['autocreate-role-assignment']);
+
+	if (autocreateRoleAssignment.source) {
+	    values.source = autocreateRoleAssignment.source;
+	} else {
+	    values.source = '__default__';
+	}
+
+	if (autocreateRoleAssignment.source === 'fixed') {
+	    values['fixed-role'] = autocreateRoleAssignment['fixed-role'];
+	}
+	if (autocreateRoleAssignment.source === 'from-claim') {
+	    values['role-claim'] = autocreateRoleAssignment['role-claim'];
+	}
+
+	this.callParent(arguments);
+    },
+
+
+    columnT: [
+	{
+	    xtype: 'textfield',
+	    name: 'issuer-url',
+	    fieldLabel: gettext('Issuer URL'),
+	    allowBlank: false,
+	},
+    ],
+
+    column1: [
+	{
+	    xtype: 'pmxDisplayEditField',
+	    name: 'realm',
+	    cbind: {
+		value: '{realm}',
+		editable: '{isCreate}',
+	    },
+	    fieldLabel: gettext('Realm'),
+	    allowBlank: false,
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Default realm'),
+	    name: 'default',
+	    value: 0,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+		hidden: '{!showDefaultRealm}',
+		disabled: '{!showDefaultRealm}',
+	    },
+	    autoEl: {
+		tag: 'div',
+		'data-qtip': gettext('Set realm as default for login'),
+	    },
+	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    fieldLabel: gettext('Client ID'),
+	    name: 'client-id',
+	    allowBlank: false,
+	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    fieldLabel: gettext('Client Key'),
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	    name: 'client-key',
+	},
+    ],
+
+    column2: [
+	{
+	    xtype: 'pmxDisplayEditField',
+	    name: 'username-claim',
+	    fieldLabel: gettext('Username Claim'),
+	    editConfig: {
+		xtype: 'proxmoxKVComboBox',
+		editable: true,
+		comboItems: [
+		    ['__default__', Proxmox.Utils.defaultText],
+		    ['sub', 'sub (subject)'],
+		    ['preferred_username', 'preferred_username'],
+		],
+	    },
+	    cbind: {
+		value: get => get('isCreate') ? '__default__' : Proxmox.Utils.defaultText,
+		deleteEmpty: '{!isCreate}',
+		editable: '{isCreate}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'scopes',
+	    fieldLabel: gettext('Scopes'),
+	    emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
+	    submitEmpty: false,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'prompt',
+	    fieldLabel: gettext('Prompt'),
+	    editable: true,
+	    emptyText: gettext('Auth-Provider Default'),
+	    comboItems: [
+		['__default__', gettext('Auth-Provider Default')],
+		['none', 'none'],
+		['login', 'login'],
+		['consent', 'consent'],
+		['select_account', 'select_account'],
+	    ],
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+    ],
+
+    columnB: [
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'comment',
+	    fieldLabel: gettext('Comment'),
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+	{
+	    xtype: 'displayfield',
+	    value: gettext('Autocreate Options'),
+	},
+	{
+	    xtype: 'proxmoxcheckbox',
+	    fieldLabel: gettext('Autocreate Users'),
+	    name: 'autocreate',
+	    bind: {
+		value: '{autocreate}',
+	    },
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxKVComboBox',
+	    name: 'source',
+	    fieldLabel: gettext('Source for Role Assignment'),
+	    allowBlank: false,
+	    deleteEmpty: false,
+	    comboItems: [
+		[
+		    '__default__',
+		    Proxmox.Utils.defaultText
+			+ ' (' + gettext('All auto-created users get audit role') + ')',
+		],
+		['fixed', 'Fixed role for all auto-created users'],
+		['from-claim', 'Get role from OIDC claim'],
+	    ],
+	    bind: {
+		value: '{roleSource}',
+		disabled: '{hideRoleAssignment}',
+		hidden: '{hideRoleAssignment}',
+	    },
+	},
+	{
+	    xtype: 'pmgRoleSelector',
+	    name: 'fixed-role',
+	    allowBlank: false,
+	    deleteEmpty: false,
+	    fieldLabel: gettext('Fixed Role'),
+	    bind: {
+		disabled: '{hideFixedRoleAssignment}',
+		hidden: '{hideFixedRoleAssignment}',
+	    },
+	},
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'role-claim',
+	    allowBlank: false,
+	    deleteEmpty: false,
+	    fieldLabel: gettext('Role Claim'),
+	    bind: {
+		disabled: '{hideClaimRoleAssignment}',
+		hidden: '{hideClaimRoleAssignment}',
+	    },
+	},
+    ],
+
+    advancedColumnB: [
+	{
+	    xtype: 'proxmoxtextfield',
+	    name: 'acr-values',
+	    fieldLabel: gettext('ACR Values'),
+	    submitEmpty: false,
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+    ],
+});
diff --git a/js/Makefile b/js/Makefile
index d1fab9b..c984bf3 100644
--- a/js/Makefile
+++ b/js/Makefile
@@ -78,6 +78,7 @@ JSSRC=							\
 	LDAPConfig.js					\
 	UserEdit.js					\
 	UserView.js					\
+	AuthEditOIDC.js					\
 	TFAView.js					\
 	FetchmailEdit.js				\
 	FetchmailView.js				\
diff --git a/js/Utils.js b/js/Utils.js
index d4a55a8..9dbc76f 100644
--- a/js/Utils.js
+++ b/js/Utils.js
@@ -871,6 +871,7 @@ Ext.define('PMG.Utils', {
 	// use oidc instead of openid
 	Proxmox.Schema.authDomains.oidc = Proxmox.Schema.authDomains.openid;
 	Proxmox.Schema.authDomains.oidc.useTypeInUrl = false;
+	Proxmox.Schema.authDomains.oidc.ipanel = 'pmgAuthOIDCPanel';
 	delete Proxmox.Schema.authDomains.openid;
 
 	// Disable LDAP/AD as a realm until LDAP/AD login is implemented
-- 
2.39.5



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

Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG

[Markus Frank]

On  2025-03-03 09:49, Markus Frank wrote:
> AuthEditOIDC.js is based on AuthEditOpenId from widget-toolkit and
> adds additional configuration options for autocreate-role-assignment.
> 
> It uses sub/preferred_username for username-claim instead of the old
> names (subject/username/email). Removed email option entirely as it is
> incompatible with the username scheme.
I am sorry.
This part should be:

Use sub/preferred_username for username-claim instead of the old names
(subject/username/email). Remove the email option entirely as it is
incompatible with the username scheme.

> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v2:
> * renamed subject to sub
> * renamed username to preferred_username
> * removed email entirely
> 
>   js/AuthEditOIDC.js | 270 +++++++++++++++++++++++++++++++++++++++++++++
>   js/Makefile        |   1 +
>   js/Utils.js        |   1 +
>   3 files changed, 272 insertions(+)
>   create mode 100644 js/AuthEditOIDC.js
> 
> diff --git a/js/AuthEditOIDC.js b/js/AuthEditOIDC.js
> new file mode 100644
> index 0000000..cda9d68
> --- /dev/null
> +++ b/js/AuthEditOIDC.js
> @@ -0,0 +1,270 @@
> +Ext.define('PMG.OIDCInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pmgAuthOIDCPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    showDefaultRealm: false,
> +
> +    type: 'oidc',
> +
> +    viewModel: {
> +	data: {
> +	    roleSource: '__default__',
> +	    autocreate: 0,
> +	},
> +	formulas: {
> +	    hideRoleAssignment: function(get) {
> +		let autocreate = get('autocreate');
> +		if (!autocreate) {
> +		    return 1;
> +		}
> +		return autocreate === 0;
> +	    },
> +	    hideFixedRoleAssignment: function(get) {
> +		return get('roleSource') !== 'fixed' || get('hideRoleAssignment');
> +	    },
> +	    hideClaimRoleAssignment: function(get) {
> +		return get('roleSource') !== 'from-claim' || get('hideRoleAssignment');
> +	    },
> +	},
> +    },
> +
> +    onGetValues: function(values) {
> +	let me = this;
> +
> +	if (me.isCreate && !me.useTypeInUrl) {
> +	    values.type = me.type;
> +	}
> +
> +	if (values.source) {
> +	    let autocreateRoleAssignment = {};
> +	    autocreateRoleAssignment.source = values.source;
> +	    if (values.source === 'fixed') {
> +		autocreateRoleAssignment['fixed-role'] = values['fixed-role'];
> +	    } else if (values.source === 'from-claim') {
> +		autocreateRoleAssignment['role-claim'] = values['role-claim'];
> +	    }
> +	    values['autocreate-role-assignment'] =
> +		Proxmox.Utils.printPropertyString(autocreateRoleAssignment);
> +	}
> +
> +	if ((!values.autocreate || !values.source) && !me.isCreate) {
> +	    if (values.delete) {
> +		if (Ext.isArray(values.delete)) {
> +		    values.delete.push('autocreate-role-assignment');
> +		} else {
> +		    values.delete += ',autocreate-role-assignment';
> +		}
> +	    } else {
> +		values.delete = 'autocreate-role-assignment';
> +	    }
> +	}
> +	delete values.source;
> +	delete values['fixed-role'];
> +	delete values['role-claim'];
> +
> +	return values;
> +    },
> +
> +    setValues: function(values) {
> +	let autocreateRoleAssignment =
> +	    Proxmox.Utils.parsePropertyString(values['autocreate-role-assignment']);
> +
> +	if (autocreateRoleAssignment.source) {
> +	    values.source = autocreateRoleAssignment.source;
> +	} else {
> +	    values.source = '__default__';
> +	}
> +
> +	if (autocreateRoleAssignment.source === 'fixed') {
> +	    values['fixed-role'] = autocreateRoleAssignment['fixed-role'];
> +	}
> +	if (autocreateRoleAssignment.source === 'from-claim') {
> +	    values['role-claim'] = autocreateRoleAssignment['role-claim'];
> +	}
> +
> +	this.callParent(arguments);
> +    },
> +
> +
> +    columnT: [
> +	{
> +	    xtype: 'textfield',
> +	    name: 'issuer-url',
> +	    fieldLabel: gettext('Issuer URL'),
> +	    allowBlank: false,
> +	},
> +    ],
> +
> +    column1: [
> +	{
> +	    xtype: 'pmxDisplayEditField',
> +	    name: 'realm',
> +	    cbind: {
> +		value: '{realm}',
> +		editable: '{isCreate}',
> +	    },
> +	    fieldLabel: gettext('Realm'),
> +	    allowBlank: false,
> +	},
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    fieldLabel: gettext('Default realm'),
> +	    name: 'default',
> +	    value: 0,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +		hidden: '{!showDefaultRealm}',
> +		disabled: '{!showDefaultRealm}',
> +	    },
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext('Set realm as default for login'),
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Client ID'),
> +	    name: 'client-id',
> +	    allowBlank: false,
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Client Key'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    name: 'client-key',
> +	},
> +    ],
> +
> +    column2: [
> +	{
> +	    xtype: 'pmxDisplayEditField',
> +	    name: 'username-claim',
> +	    fieldLabel: gettext('Username Claim'),
> +	    editConfig: {
> +		xtype: 'proxmoxKVComboBox',
> +		editable: true,
> +		comboItems: [
> +		    ['__default__', Proxmox.Utils.defaultText],
> +		    ['sub', 'sub (subject)'],
> +		    ['preferred_username', 'preferred_username'],
> +		],
> +	    },
> +	    cbind: {
> +		value: get => get('isCreate') ? '__default__' : Proxmox.Utils.defaultText,
> +		deleteEmpty: '{!isCreate}',
> +		editable: '{isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'scopes',
> +	    fieldLabel: gettext('Scopes'),
> +	    emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
> +	    submitEmpty: false,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    name: 'prompt',
> +	    fieldLabel: gettext('Prompt'),
> +	    editable: true,
> +	    emptyText: gettext('Auth-Provider Default'),
> +	    comboItems: [
> +		['__default__', gettext('Auth-Provider Default')],
> +		['none', 'none'],
> +		['login', 'login'],
> +		['consent', 'consent'],
> +		['select_account', 'select_account'],
> +	    ],
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +
> +    columnB: [
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'comment',
> +	    fieldLabel: gettext('Comment'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    value: gettext('Autocreate Options'),
> +	},
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    fieldLabel: gettext('Autocreate Users'),
> +	    name: 'autocreate',
> +	    bind: {
> +		value: '{autocreate}',
> +	    },
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    name: 'source',
> +	    fieldLabel: gettext('Source for Role Assignment'),
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    comboItems: [
> +		[
> +		    '__default__',
> +		    Proxmox.Utils.defaultText
> +			+ ' (' + gettext('All auto-created users get audit role') + ')',
> +		],
> +		['fixed', 'Fixed role for all auto-created users'],
> +		['from-claim', 'Get role from OIDC claim'],
> +	    ],
> +	    bind: {
> +		value: '{roleSource}',
> +		disabled: '{hideRoleAssignment}',
> +		hidden: '{hideRoleAssignment}',
> +	    },
> +	},
> +	{
> +	    xtype: 'pmgRoleSelector',
> +	    name: 'fixed-role',
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    fieldLabel: gettext('Fixed Role'),
> +	    bind: {
> +		disabled: '{hideFixedRoleAssignment}',
> +		hidden: '{hideFixedRoleAssignment}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'role-claim',
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    fieldLabel: gettext('Role Claim'),
> +	    bind: {
> +		disabled: '{hideClaimRoleAssignment}',
> +		hidden: '{hideClaimRoleAssignment}',
> +	    },
> +	},
> +    ],
> +
> +    advancedColumnB: [
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'acr-values',
> +	    fieldLabel: gettext('ACR Values'),
> +	    submitEmpty: false,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +});
> diff --git a/js/Makefile b/js/Makefile
> index d1fab9b..c984bf3 100644
> --- a/js/Makefile
> +++ b/js/Makefile
> @@ -78,6 +78,7 @@ JSSRC=							\
>   	LDAPConfig.js					\
>   	UserEdit.js					\
>   	UserView.js					\
> +	AuthEditOIDC.js					\
>   	TFAView.js					\
>   	FetchmailEdit.js				\
>   	FetchmailView.js				\
> diff --git a/js/Utils.js b/js/Utils.js
> index d4a55a8..9dbc76f 100644
> --- a/js/Utils.js
> +++ b/js/Utils.js
> @@ -871,6 +871,7 @@ Ext.define('PMG.Utils', {
>   	// use oidc instead of openid
>   	Proxmox.Schema.authDomains.oidc = Proxmox.Schema.authDomains.openid;
>   	Proxmox.Schema.authDomains.oidc.useTypeInUrl = false;
> +	Proxmox.Schema.authDomains.oidc.ipanel = 'pmgAuthOIDCPanel';
>   	delete Proxmox.Schema.authDomains.openid;
>   
>   	// Disable LDAP/AD as a realm until LDAP/AD login is implemented



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

Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG

[Dominik Csapak]

Works fine during testing, but noticed some things in the code,
see comments inline:

On 3/3/25 09:49, Markus Frank wrote:
> AuthEditOIDC.js is based on AuthEditOpenId from widget-toolkit and
> adds additional configuration options for autocreate-role-assignment.
> 
> It uses sub/preferred_username for username-claim instead of the old
> names (subject/username/email). Removed email option entirely as it is
> incompatible with the username scheme.
> 
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v2:
> * renamed subject to sub
> * renamed username to preferred_username
> * removed email entirely
> 
>   js/AuthEditOIDC.js | 270 +++++++++++++++++++++++++++++++++++++++++++++
>   js/Makefile        |   1 +
>   js/Utils.js        |   1 +
>   3 files changed, 272 insertions(+)
>   create mode 100644 js/AuthEditOIDC.js
> 
> diff --git a/js/AuthEditOIDC.js b/js/AuthEditOIDC.js
> new file mode 100644
> index 0000000..cda9d68
> --- /dev/null
> +++ b/js/AuthEditOIDC.js
> @@ -0,0 +1,270 @@
> +Ext.define('PMG.OIDCInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pmgAuthOIDCPanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    showDefaultRealm: false,
> +
> +    type: 'oidc',
> +
> +    viewModel: {
> +	data: {
> +	    roleSource: '__default__',
> +	    autocreate: 0,
> +	},
> +	formulas: {
> +	    hideRoleAssignment: function(get) {
> +		let autocreate = get('autocreate');
> +		if (!autocreate) {
> +		    return 1;
> +		}
> +		return autocreate === 0;

in one branch you return 1, but later true/false
also if 'autocreate' === 0, '!autocreate' is already true..

couldn't you simply use '!autocreate' in the bindings then?


> +	    },
> +	    hideFixedRoleAssignment: function(get) {
> +		return get('roleSource') !== 'fixed' || get('hideRoleAssignment');
> +	    },
> +	    hideClaimRoleAssignment: function(get) {
> +		return get('roleSource') !== 'from-claim' || get('hideRoleAssignment');
> +	    },
> +	},
> +    },
> +
> +    onGetValues: function(values) {
> +	let me = this;
> +
> +	if (me.isCreate && !me.useTypeInUrl) {
> +	    values.type = me.type;
> +	}
> +
> +	if (values.source) {
> +	    let autocreateRoleAssignment = {};
> +	    autocreateRoleAssignment.source = values.source;
> +	    if (values.source === 'fixed') {
> +		autocreateRoleAssignment['fixed-role'] = values['fixed-role'];
> +	    } else if (values.source === 'from-claim') {
> +		autocreateRoleAssignment['role-claim'] = values['role-claim'];
> +	    }

if the content of 'source' would match the indices ('fixed-role' and 'role-claim') you could do
something like this (psuedo-code & untested!)

if (values.source) {
	let assignment = {};
	assignment[values.source] = values[values.source];
	values['aut...'] = printPropertyString(assignment);
}

no?


> +	    values['autocreate-role-assignment'] =
> +		Proxmox.Utils.printPropertyString(autocreateRoleAssignment);
> +	}
> +
> +	if ((!values.autocreate || !values.source) && !me.isCreate) {
> +	    if (values.delete) {
> +		if (Ext.isArray(values.delete)) {
> +		    values.delete.push('autocreate-role-assignment');
> +		} else {
> +		    values.delete += ',autocreate-role-assignment';
> +		}
> +	    } else {
> +		values.delete = 'autocreate-role-assignment';
> +	    }
> +	}
AFAIR, we already have this behavior factored out in proxmox-widget-toolkit:

'delete_if_default'

> +	delete values.source;
> +	delete values['fixed-role'];
> +	delete values['role-claim'];
> +
> +	return values;
> +    },
> +
> +    setValues: function(values) {
> +	let autocreateRoleAssignment =
> +	    Proxmox.Utils.parsePropertyString(values['autocreate-role-assignment']);
> +
> +	if (autocreateRoleAssignment.source) {
> +	    values.source = autocreateRoleAssignment.source;
> +	} else {
> +	    values.source = '__default__';
> +	}

could be simplified to

values.source = autocreateRoleAssignment.source ?? '__default__';

> +
> +	if (autocreateRoleAssignment.source === 'fixed') {
> +	    values['fixed-role'] = autocreateRoleAssignment['fixed-role'];
> +	}
> +	if (autocreateRoleAssignment.source === 'from-claim') {
> +	    values['role-claim'] = autocreateRoleAssignment['role-claim'];
> +	}

same here but reversed:

values[assignment.source] = assignment[assignment.source];

> +
> +	this.callParent(arguments);
> +    },
> +
> +
> +    columnT: [
> +	{
> +	    xtype: 'textfield',
> +	    name: 'issuer-url',
> +	    fieldLabel: gettext('Issuer URL'),
> +	    allowBlank: false,
> +	},
> +    ],
> +
> +    column1: [
> +	{
> +	    xtype: 'pmxDisplayEditField',
> +	    name: 'realm',
> +	    cbind: {
> +		value: '{realm}',
> +		editable: '{isCreate}',
> +	    },
> +	    fieldLabel: gettext('Realm'),
> +	    allowBlank: false,
> +	},
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    fieldLabel: gettext('Default realm'),
> +	    name: 'default',
> +	    value: 0,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +		hidden: '{!showDefaultRealm}',
> +		disabled: '{!showDefaultRealm}',
> +	    },
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext('Set realm as default for login'),
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Client ID'),
> +	    name: 'client-id',
> +	    allowBlank: false,
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    fieldLabel: gettext('Client Key'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    name: 'client-key',
> +	},
> +    ],
> +
> +    column2: [
> +	{
> +	    xtype: 'pmxDisplayEditField',
> +	    name: 'username-claim',
> +	    fieldLabel: gettext('Username Claim'),
> +	    editConfig: {
> +		xtype: 'proxmoxKVComboBox',
> +		editable: true,
> +		comboItems: [
> +		    ['__default__', Proxmox.Utils.defaultText],
> +		    ['sub', 'sub (subject)'],
> +		    ['preferred_username', 'preferred_username'],

if you're changing the text vs widget-toolkit, why not got the full length
and localize the description?

e.g. `gettext('sub (Subject)')`

?

(preexisting) maybe i just overlooked that with the recent patches, but why are the
claims different than the other products (sub vs subject, preferred_username vs username) ?

> +		],
> +	    },
> +	    cbind: {
> +		value: get => get('isCreate') ? '__default__' : Proxmox.Utils.defaultText,
> +		deleteEmpty: '{!isCreate}',
> +		editable: '{isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'scopes',
> +	    fieldLabel: gettext('Scopes'),
> +	    emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
> +	    submitEmpty: false,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    name: 'prompt',
> +	    fieldLabel: gettext('Prompt'),
> +	    editable: true,
> +	    emptyText: gettext('Auth-Provider Default'),
> +	    comboItems: [
> +		['__default__', gettext('Auth-Provider Default')],
> +		['none', 'none'],
> +		['login', 'login'],
> +		['consent', 'consent'],
> +		['select_account', 'select_account'],
> +	    ],
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +
> +    columnB: [
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'comment',
> +	    fieldLabel: gettext('Comment'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    value: gettext('Autocreate Options'),
> +	},
> +	{
> +	    xtype: 'proxmoxcheckbox',
> +	    fieldLabel: gettext('Autocreate Users'),
> +	    name: 'autocreate',
> +	    bind: {
> +		value: '{autocreate}',
> +	    },
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxKVComboBox',
> +	    name: 'source',
> +	    fieldLabel: gettext('Source for Role Assignment'),
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    comboItems: [
> +		[
> +		    '__default__',
> +		    Proxmox.Utils.defaultText
> +			+ ' (' + gettext('All auto-created users get audit role') + ')',
> +		],
> +		['fixed', 'Fixed role for all auto-created users'],
> +		['from-claim', 'Get role from OIDC claim'],

IMHO those should be put in 'gettext'

> +	    ],
> +	    bind: {
> +		value: '{roleSource}',
> +		disabled: '{hideRoleAssignment}',
> +		hidden: '{hideRoleAssignment}',
> +	    },
> +	},
> +	{
> +	    xtype: 'pmgRoleSelector',
> +	    name: 'fixed-role',
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    fieldLabel: gettext('Fixed Role'),
> +	    bind: {
> +		disabled: '{hideFixedRoleAssignment}',
> +		hidden: '{hideFixedRoleAssignment}',
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'role-claim',
> +	    allowBlank: false,
> +	    deleteEmpty: false,
> +	    fieldLabel: gettext('Role Claim'),
> +	    bind: {
> +		disabled: '{hideClaimRoleAssignment}',
> +		hidden: '{hideClaimRoleAssignment}',
> +	    },

side question: did you got that to work with keycloak? i couldn't in my tests...

> +	},
> +    ],
> +
> +    advancedColumnB: [
> +	{
> +	    xtype: 'proxmoxtextfield',
> +	    name: 'acr-values',
> +	    fieldLabel: gettext('ACR Values'),
> +	    submitEmpty: false,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +});
> diff --git a/js/Makefile b/js/Makefile
> index d1fab9b..c984bf3 100644
> --- a/js/Makefile
> +++ b/js/Makefile
> @@ -78,6 +78,7 @@ JSSRC=							\
>   	LDAPConfig.js					\
>   	UserEdit.js					\
>   	UserView.js					\
> +	AuthEditOIDC.js					\
>   	TFAView.js					\
>   	FetchmailEdit.js				\
>   	FetchmailView.js				\
> diff --git a/js/Utils.js b/js/Utils.js
> index d4a55a8..9dbc76f 100644
> --- a/js/Utils.js
> +++ b/js/Utils.js
> @@ -871,6 +871,7 @@ Ext.define('PMG.Utils', {
>   	// use oidc instead of openid
>   	Proxmox.Schema.authDomains.oidc = Proxmox.Schema.authDomains.openid;
>   	Proxmox.Schema.authDomains.oidc.useTypeInUrl = false;
> +	Proxmox.Schema.authDomains.oidc.ipanel = 'pmgAuthOIDCPanel';
>   	delete Proxmox.Schema.authDomains.openid;
>   
>   	// Disable LDAP/AD as a realm until LDAP/AD login is implemented



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

Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG

[Markus Frank]

I will address most of your comments in v3, thanks.

Some comments/answers inline:

On  2025-03-10 15:37, Dominik Csapak wrote:
> Works fine during testing, but noticed some things in the code,
> see comments inline:
> 
> On 3/3/25 09:49, Markus Frank wrote:
>> AuthEditOIDC.js is based on AuthEditOpenId from widget-toolkit and
>> adds additional configuration options for autocreate-role-assignment.
>>
>> It uses sub/preferred_username for username-claim instead of the old
>> names (subject/username/email). Removed email option entirely as it is
>> incompatible with the username scheme.
>>
>> Signed-off-by: Markus Frank <m.frank@proxmox.com>
>> ---
>> v2:
>> * renamed subject to sub
>> * renamed username to preferred_username
>> * removed email entirely
>>
>>   js/AuthEditOIDC.js | 270 +++++++++++++++++++++++++++++++++++++++++++++
>>   js/Makefile        |   1 +
>>   js/Utils.js        |   1 +
>>   3 files changed, 272 insertions(+)
>>   create mode 100644 js/AuthEditOIDC.js
>>
>> diff --git a/js/AuthEditOIDC.js b/js/AuthEditOIDC.js
>> new file mode 100644
>> index 0000000..cda9d68
>> --- /dev/null
>> +++ b/js/AuthEditOIDC.js
>> @@ -0,0 +1,270 @@
>> +Ext.define('PMG.OIDCInputPanel', {
>> +    extend: 'Proxmox.panel.InputPanel',
>> +    xtype: 'pmgAuthOIDCPanel',
>> +    mixins: ['Proxmox.Mixin.CBind'],
>> +
>> +    showDefaultRealm: false,
>> +
>> +    type: 'oidc',
>> +
>> +    viewModel: {
>> +    data: {
>> +        roleSource: '__default__',
>> +        autocreate: 0,
>> +    },
>> +    formulas: {
>> +        hideRoleAssignment: function(get) {
>> +        let autocreate = get('autocreate');
>> +        if (!autocreate) {
>> +            return 1;
>> +        }
>> +        return autocreate === 0;
> 
> in one branch you return 1, but later true/false
> also if 'autocreate' === 0, '!autocreate' is already true..
> 
> couldn't you simply use '!autocreate' in the bindings then?
> 
> 
>> +        },
>> +        hideFixedRoleAssignment: function(get) {
>> +        return get('roleSource') !== 'fixed' || get('hideRoleAssignment');
>> +        },
>> +        hideClaimRoleAssignment: function(get) {
>> +        return get('roleSource') !== 'from-claim' || get('hideRoleAssignment');
>> +        },
>> +    },
>> +    },
>> +
>> +    onGetValues: function(values) {
>> +    let me = this;
>> +
>> +    if (me.isCreate && !me.useTypeInUrl) {
>> +        values.type = me.type;
>> +    }
>> +
>> +    if (values.source) {
>> +        let autocreateRoleAssignment = {};
>> +        autocreateRoleAssignment.source = values.source;
>> +        if (values.source === 'fixed') {
>> +        autocreateRoleAssignment['fixed-role'] = values['fixed-role'];
>> +        } else if (values.source === 'from-claim') {
>> +        autocreateRoleAssignment['role-claim'] = values['role-claim'];
>> +        }
> 
> if the content of 'source' would match the indices ('fixed-role' and 'role-claim') you could do
> something like this (psuedo-code & untested!)
> 
> if (values.source) {
>      let assignment = {};
>      assignment[values.source] = values[values.source];
>      values['aut...'] = printPropertyString(assignment);
> }
> 
> no?
Sadly not.

If the content of 'source' would match the indices ('fixed-role' and 'role-claim'),
I would still need to use an if to change the source variable:

if (values.source) {
       let assignment = {};
       assignment[values.source] = values[values.source];
       if (values.source === 'fixed-role') {
           assignment.source = 'fixed';
       } else if (values.source === 'role-claim') {
           assignment.source = 'from-claim';
       }
       values['aut...'] = printPropertyString(assignment);
}

I am not sure if that would be better.
> 
> 
>> +        values['autocreate-role-assignment'] =
>> +        Proxmox.Utils.printPropertyString(autocreateRoleAssignment);
>> +    }
>> +
>> +    if ((!values.autocreate || !values.source) && !me.isCreate) {
>> +        if (values.delete) {
>> +        if (Ext.isArray(values.delete)) {
>> +            values.delete.push('autocreate-role-assignment');
>> +        } else {
>> +            values.delete += ',autocreate-role-assignment';
>> +        }
>> +        } else {
>> +        values.delete = 'autocreate-role-assignment';
>> +        }
>> +    }
> AFAIR, we already have this behavior factored out in proxmox-widget-toolkit:
> 
> 'delete_if_default'
> 
>> +    delete values.source;
>> +    delete values['fixed-role'];
>> +    delete values['role-claim'];
>> +
>> +    return values;
>> +    },
>> +
>> +    setValues: function(values) {
>> +    let autocreateRoleAssignment =
>> +        Proxmox.Utils.parsePropertyString(values['autocreate-role-assignment']);
>> +
>> +    if (autocreateRoleAssignment.source) {
>> +        values.source = autocreateRoleAssignment.source;
>> +    } else {
>> +        values.source = '__default__';
>> +    }
> 
> could be simplified to
> 
> values.source = autocreateRoleAssignment.source ?? '__default__';
> 
>> +
>> +    if (autocreateRoleAssignment.source === 'fixed') {
>> +        values['fixed-role'] = autocreateRoleAssignment['fixed-role'];
>> +    }
>> +    if (autocreateRoleAssignment.source === 'from-claim') {
>> +        values['role-claim'] = autocreateRoleAssignment['role-claim'];
>> +    }
> 
> same here but reversed:
> 
> values[assignment.source] = assignment[assignment.source];
> 
>> +
>> +    this.callParent(arguments);
>> +    },
>> +
>> +
>> +    columnT: [
>> +    {
>> +        xtype: 'textfield',
>> +        name: 'issuer-url',
>> +        fieldLabel: gettext('Issuer URL'),
>> +        allowBlank: false,
>> +    },
>> +    ],
>> +
>> +    column1: [
>> +    {
>> +        xtype: 'pmxDisplayEditField',
>> +        name: 'realm',
>> +        cbind: {
>> +        value: '{realm}',
>> +        editable: '{isCreate}',
>> +        },
>> +        fieldLabel: gettext('Realm'),
>> +        allowBlank: false,
>> +    },
>> +    {
>> +        xtype: 'proxmoxcheckbox',
>> +        fieldLabel: gettext('Default realm'),
>> +        name: 'default',
>> +        value: 0,
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        hidden: '{!showDefaultRealm}',
>> +        disabled: '{!showDefaultRealm}',
>> +        },
>> +        autoEl: {
>> +        tag: 'div',
>> +        'data-qtip': gettext('Set realm as default for login'),
>> +        },
>> +    },
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        fieldLabel: gettext('Client ID'),
>> +        name: 'client-id',
>> +        allowBlank: false,
>> +    },
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        fieldLabel: gettext('Client Key'),
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +        name: 'client-key',
>> +    },
>> +    ],
>> +
>> +    column2: [
>> +    {
>> +        xtype: 'pmxDisplayEditField',
>> +        name: 'username-claim',
>> +        fieldLabel: gettext('Username Claim'),
>> +        editConfig: {
>> +        xtype: 'proxmoxKVComboBox',
>> +        editable: true,
>> +        comboItems: [
>> +            ['__default__', Proxmox.Utils.defaultText],
>> +            ['sub', 'sub (subject)'],
>> +            ['preferred_username', 'preferred_username'],
> 
> if you're changing the text vs widget-toolkit, why not got the full length
> and localize the description?
> 
> e.g. `gettext('sub (Subject)')`
> 
> ?
> 
> (preexisting) maybe i just overlooked that with the recent patches, but why are the
> claims different than the other products (sub vs subject, preferred_username vs username) ?
subject and username do not exist (anymore).

We use something like this in our backends:

} elsif ($user_attr eq 'subject') { # stay compat with old versions
	$unique_name = $info->{'sub'};
} elsif ($user_attr eq 'username') { # stay compat with old versions
	my $username = $info->{'preferred_username'};
	die "missing claim 'preferred_username'\n" if !defined($username);
	$unique_name =  $username;

I guess it was the plan to update these names in the PVE & PBS WebUIs, but it never happened.

> 
>> +        ],
>> +        },
>> +        cbind: {
>> +        value: get => get('isCreate') ? '__default__' : Proxmox.Utils.defaultText,
>> +        deleteEmpty: '{!isCreate}',
>> +        editable: '{isCreate}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        name: 'scopes',
>> +        fieldLabel: gettext('Scopes'),
>> +        emptyText: `${Proxmox.Utils.defaultText} (email profile)`,
>> +        submitEmpty: false,
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'proxmoxKVComboBox',
>> +        name: 'prompt',
>> +        fieldLabel: gettext('Prompt'),
>> +        editable: true,
>> +        emptyText: gettext('Auth-Provider Default'),
>> +        comboItems: [
>> +        ['__default__', gettext('Auth-Provider Default')],
>> +        ['none', 'none'],
>> +        ['login', 'login'],
>> +        ['consent', 'consent'],
>> +        ['select_account', 'select_account'],
>> +        ],
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +    },
>> +    ],
>> +
>> +    columnB: [
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        name: 'comment',
>> +        fieldLabel: gettext('Comment'),
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'displayfield',
>> +        value: gettext('Autocreate Options'),
>> +    },
>> +    {
>> +        xtype: 'proxmoxcheckbox',
>> +        fieldLabel: gettext('Autocreate Users'),
>> +        name: 'autocreate',
>> +        bind: {
>> +        value: '{autocreate}',
>> +        },
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'proxmoxKVComboBox',
>> +        name: 'source',
>> +        fieldLabel: gettext('Source for Role Assignment'),
>> +        allowBlank: false,
>> +        deleteEmpty: false,
>> +        comboItems: [
>> +        [
>> +            '__default__',
>> +            Proxmox.Utils.defaultText
>> +            + ' (' + gettext('All auto-created users get audit role') + ')',
>> +        ],
>> +        ['fixed', 'Fixed role for all auto-created users'],
>> +        ['from-claim', 'Get role from OIDC claim'],
> 
> IMHO those should be put in 'gettext'
> 
>> +        ],
>> +        bind: {
>> +        value: '{roleSource}',
>> +        disabled: '{hideRoleAssignment}',
>> +        hidden: '{hideRoleAssignment}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'pmgRoleSelector',
>> +        name: 'fixed-role',
>> +        allowBlank: false,
>> +        deleteEmpty: false,
>> +        fieldLabel: gettext('Fixed Role'),
>> +        bind: {
>> +        disabled: '{hideFixedRoleAssignment}',
>> +        hidden: '{hideFixedRoleAssignment}',
>> +        },
>> +    },
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        name: 'role-claim',
>> +        allowBlank: false,
>> +        deleteEmpty: false,
>> +        fieldLabel: gettext('Role Claim'),
>> +        bind: {
>> +        disabled: '{hideClaimRoleAssignment}',
>> +        hidden: '{hideClaimRoleAssignment}',
>> +        },
> 
> side question: did you got that to work with keycloak? i couldn't in my tests...
I tried it with keycloak and authentik. I was also unsuccessful.
> 
>> +    },
>> +    ],
>> +
>> +    advancedColumnB: [
>> +    {
>> +        xtype: 'proxmoxtextfield',
>> +        name: 'acr-values',
>> +        fieldLabel: gettext('ACR Values'),
>> +        submitEmpty: false,
>> +        cbind: {
>> +        deleteEmpty: '{!isCreate}',
>> +        },
>> +    },
>> +    ],
>> +});
>> diff --git a/js/Makefile b/js/Makefile
>> index d1fab9b..c984bf3 100644
>> --- a/js/Makefile
>> +++ b/js/Makefile
>> @@ -78,6 +78,7 @@ JSSRC=                            \
>>       LDAPConfig.js                    \
>>       UserEdit.js                    \
>>       UserView.js                    \
>> +    AuthEditOIDC.js                    \
>>       TFAView.js                    \
>>       FetchmailEdit.js                \
>>       FetchmailView.js                \
>> diff --git a/js/Utils.js b/js/Utils.js
>> index d4a55a8..9dbc76f 100644
>> --- a/js/Utils.js
>> +++ b/js/Utils.js
>> @@ -871,6 +871,7 @@ Ext.define('PMG.Utils', {
>>       // use oidc instead of openid
>>       Proxmox.Schema.authDomains.oidc = Proxmox.Schema.authDomains.openid;
>>       Proxmox.Schema.authDomains.oidc.useTypeInUrl = false;
>> +    Proxmox.Schema.authDomains.oidc.ipanel = 'pmgAuthOIDCPanel';
>>       delete Proxmox.Schema.authDomains.openid;
>>       // Disable LDAP/AD as a realm until LDAP/AD login is implemented
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
> 



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