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 EFE8DBA59 for ; Thu, 10 Aug 2023 10:49:18 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D13921E626 for ; Thu, 10 Aug 2023 10:49:18 +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:49:17 +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 99579448F6 for ; Thu, 10 Aug 2023 10:49:17 +0200 (CEST) Date: Thu, 10 Aug 2023 10:49:16 +0200 From: Wolfgang Bumiller To: Christoph Heiss Cc: pve-devel@lists.proxmox.com Message-ID: References: <20230801123734.446797-1-c.heiss@proxmox.com> <20230801123734.446797-3-c.heiss@proxmox.com> <4rsergs6kzodeqxtd5ztxmvr2opzzrrh4nnpt3iysotol2gztr@v2scx4ajllge> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4rsergs6kzodeqxtd5ztxmvr2opzzrrh4nnpt3iysotol2gztr@v2scx4ajllge> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.107 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:49:19 -0000 On Thu, Aug 10, 2023 at 10:35:14AM +0200, Christoph Heiss wrote: > > 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. Sure, just be aware that `get_standard_option()` only needs 1 level, whereas for the section config ones it works one level above, as in it returns a full schema like: { type => 'object', properties => { ... } } It's unlikely that we'll need to modify anything at the top level, so I think it would be fine for the extra parameter to only affect the contained properties hash, so no need to do any multi-level merging.