* [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG @ 2025-03-03 8:49 Markus Frank 2025-03-03 8:58 ` Markus Frank 2025-03-10 14:37 ` Dominik Csapak 0 siblings, 2 replies; 4+ messages in thread From: Markus Frank @ 2025-03-03 8:49 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG 2025-03-03 8:49 [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG Markus Frank @ 2025-03-03 8:58 ` Markus Frank 2025-03-10 14:37 ` Dominik Csapak 1 sibling, 0 replies; 4+ messages in thread From: Markus Frank @ 2025-03-03 8:58 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG 2025-03-03 8:49 [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG Markus Frank 2025-03-03 8:58 ` Markus Frank @ 2025-03-10 14:37 ` Dominik Csapak 2025-03-11 10:22 ` Markus Frank 1 sibling, 1 reply; 4+ messages in thread From: Dominik Csapak @ 2025-03-10 14:37 UTC (permalink / raw) To: pmg-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG 2025-03-10 14:37 ` Dominik Csapak @ 2025-03-11 10:22 ` Markus Frank 0 siblings, 0 replies; 4+ messages in thread From: Markus Frank @ 2025-03-11 10:22 UTC (permalink / raw) To: Dominik Csapak, pmg-devel 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2025-03-11 10:22 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2025-03-03 8:49 [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for PMG Markus Frank 2025-03-03 8:58 ` Markus Frank 2025-03-10 14:37 ` Dominik Csapak 2025-03-11 10:22 ` Markus Frank
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