From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pmg-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
by lore.proxmox.com (Postfix) with ESMTPS id 9012C1FF173
for <inbox@lore.proxmox.com>; Mon, 10 Mar 2025 15:37:48 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 1A4E51C0B2;
Mon, 10 Mar 2025 15:37:42 +0100 (CET)
Message-ID: <02c92eb4-603a-4c85-9939-6b00ae2ee3ef@proxmox.com>
Date: Mon, 10 Mar 2025 15:37:38 +0100
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird Beta
To: pmg-devel@lists.proxmox.com
References: <20250303084958.2742-1-m.frank@proxmox.com>
Content-Language: en-US
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <20250303084958.2742-1-m.frank@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.022 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
Subject: Re: [pmg-devel] [PATCH pmg-gui v2] add OIDC configuration panel for
PMG
X-BeenThere: pmg-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Mail Gateway development discussion
<pmg-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pmg-devel/>
List-Post: <mailto:pmg-devel@lists.proxmox.com>
List-Help: <mailto:pmg-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel>,
<mailto:pmg-devel-request@lists.proxmox.com?subject=subscribe>
Content-Transfer-Encoding: 7bit
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Errors-To: pmg-devel-bounces@lists.proxmox.com
Sender: "pmg-devel" <pmg-devel-bounces@lists.proxmox.com>
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