Re: [pbs-devel] [PATCH proxmox-backup 03/12] api-types: implement `LdapMode` -> `ConnectionMode` conversion

[Christoph Heiss <c.heiss@proxmox.com>]

Thanks for the review!

On Fri, Aug 11, 2023 at 12:36:41PM +0200, Wolfgang Bumiller wrote:
>
> On Tue, Aug 08, 2023 at 02:22:05PM +0200, Christoph Heiss wrote:
> > No functional changes.
> >
> > Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> > ---
> >  pbs-api-types/Cargo.toml  |  1 +
> >  pbs-api-types/src/ldap.rs | 11 +++++++++++
> >  src/auth.rs               | 12 +++---------
> >  3 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/pbs-api-types/Cargo.toml b/pbs-api-types/Cargo.toml
> > index 31b69f62..cb584cb5 100644
> > --- a/pbs-api-types/Cargo.toml
> > +++ b/pbs-api-types/Cargo.toml
> > @@ -17,6 +17,7 @@ serde_plain.workspace = true
> >  proxmox-auth-api = { workspace = true, features = [ "api-types" ] }
> >  proxmox-human-byte.workspace = true
> >  proxmox-lang.workspace=true
> > +proxmox-ldap.workspace = true
>
> The api type crate should strive to be somewhat lightweight, as it will
> also end up being used in with wasm at some point where we definitely
> can't pull this in.
>
> If it really makes sense to have this locally, it should be
> feature-guarded.
Ack, I'll drop this then and go with a simple, local function in
src/auth.rs instead. It is only needed in two places there anyway, and
the feature-gating isn't worth it just to be able to use `.into()` in
two places IMO ..

>
> >  proxmox-schema = { workspace = true, features = [ "api-macro" ] }
> >  proxmox-serde.workspace = true
> >  proxmox-time.workspace = true
> > diff --git a/pbs-api-types/src/ldap.rs b/pbs-api-types/src/ldap.rs
> > index f3df90a0..e1f7c452 100644
> > --- a/pbs-api-types/src/ldap.rs
> > +++ b/pbs-api-types/src/ldap.rs
> > @@ -1,5 +1,6 @@
> >  use serde::{Deserialize, Serialize};
> >
> > +use proxmox_ldap::ConnectionMode;
> >  use proxmox_schema::{api, ApiStringFormat, ApiType, ArraySchema, Schema, StringSchema, Updater};
> >
> >  use super::{REALM_ID_SCHEMA, SINGLE_LINE_COMMENT_SCHEMA};
> > @@ -20,6 +21,16 @@ pub enum LdapMode {
> >      Ldaps,
> >  }
> >
> > +impl From<LdapMode> for ConnectionMode {
> > +    fn from(value: LdapMode) -> ConnectionMode {
> > +        match value {
> > +            LdapMode::Ldap => ConnectionMode::Ldap,
> > +            LdapMode::StartTls => ConnectionMode::StartTls,
> > +            LdapMode::Ldaps => ConnectionMode::Ldaps,
> > +        }
> > +    }
> > +}
> > +
> >  #[api(
> >      properties: {
> >          "realm": {
> > diff --git a/src/auth.rs b/src/auth.rs
> > index 318d1ff2..e375ebc4 100644
> > --- a/src/auth.rs
> > +++ b/src/auth.rs
> > @@ -16,10 +16,10 @@ use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
> >  use proxmox_auth_api::ticket::{Empty, Ticket};
> >  use proxmox_auth_api::types::Authid;
> >  use proxmox_auth_api::Keyring;
> > -use proxmox_ldap::{Config, Connection, ConnectionMode};
> > +use proxmox_ldap::{Config, Connection};
> >  use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
> >
> > -use pbs_api_types::{LdapMode, LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
> > +use pbs_api_types::{LdapRealmConfig, OpenIdRealmConfig, RealmRef, Userid, UsernameRef};
> >  use pbs_buildcfg::configdir;
> >
> >  use crate::auth_helpers;
> > @@ -185,12 +185,6 @@ impl LdapAuthenticator {
> >              servers.push(server.clone());
> >          }
> >
> > -        let tls_mode = match config.mode.unwrap_or_default() {
> > -            LdapMode::Ldap => ConnectionMode::Ldap,
> > -            LdapMode::StartTls => ConnectionMode::StartTls,
> > -            LdapMode::Ldaps => ConnectionMode::Ldaps,
> > -        };
> > -
> >          let (ca_store, trusted_cert) = if let Some(capath) = config.capath.as_deref() {
> >              let path = PathBuf::from(capath);
> >              if path.is_dir() {
> > @@ -209,7 +203,7 @@ impl LdapAuthenticator {
> >              base_dn: config.base_dn.clone(),
> >              bind_dn: config.bind_dn.clone(),
> >              bind_password: password,
> > -            tls_mode,
> > +            tls_mode: config.mode.unwrap_or_default().into(),
> >              verify_certificate: config.verify.unwrap_or_default(),
> >              additional_trusted_certificates: trusted_cert,
> >              certificate_store_path: ca_store,
> > --
> > 2.41.0