From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 3261AB9CD for ; Thu, 10 Aug 2023 10:35:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0C6EF1E345 for ; Thu, 10 Aug 2023 10:35:16 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 10 Aug 2023 10:35:15 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5313F448F6 for ; Thu, 10 Aug 2023 10:35:15 +0200 (CEST) Date: Thu, 10 Aug 2023 10:35:14 +0200 From: Christoph Heiss To: Wolfgang Bumiller Cc: pve-devel@lists.proxmox.com Message-ID: <4rsergs6kzodeqxtd5ztxmvr2opzzrrh4nnpt3iysotol2gztr@v2scx4ajllge> References: <20230801123734.446797-1-c.heiss@proxmox.com> <20230801123734.446797-3-c.heiss@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-SPAM-LEVEL: Spam detection results: 0 AWL -0.044 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: [pve-devel] [PATCH access-control v2 2/3] ldap: add opt-in `check-connection` param to perform a bind check X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Aug 2023 08:35:46 -0000 On Thu, Aug 10, 2023 at 09:55:51AM +0200, Wolfgang Bumiller wrote: > On Tue, Aug 01, 2023 at 02:37:18PM +0200, Christoph Heiss wrote: [..] > > @@ -137,7 +131,13 @@ sub properties { > > type => 'boolean', > > optional => 1, > > default => 1, > > - } > > + }, > > + 'check-connection' => { > > + description => 'Check bind connection to LDAP server.', > > + type => 'boolean', > > + optional => 1, > > + default => 0, > > + }, > > While there's special handling for how we store the password, this > schema here should still actually describe the stored config. > Since this is a parameter specifically for the add/update API methods we > should declare it in those functions as parameter. > > Some of our methods to get schemas have an optional hash parameter to > include an extra set of base properties in its returned contents (see > `get_standard_option` as an example), but `createSchema` and > `updateSchema` do not. Right, I was unsure anyway if this was the right way anyway to add this, at least I did not see any other way - that explains why :^) > > We could either add this, or, since this is currently only required > once, just move the `{create,update}Schema` calls over the > `register_method()` calls and modify them right there before use... > Since this series already touches pve-common, I have a *slight* > preference to extending the `create/updateSchema` subs in > `PVE::SectionConfig`, Seems like the right thing - I'd also rather do it properly once than to introduce a hack that sticks around .. > although AFAICT the common patch does not strictly > require a dependency bump inside pve-access-control as it mostly about > how errors are presented to end-users (?), so either way is fine with Exactly, the changes in pve-common are purely cosmectic. > me. If we update the SectionConfig we'll definitely need a versioned > dependency bump. If it's OK for you I will go this route, extending {create,update}Schema() as needed for this, in the same way get_standard_option() works.