Re: [pbs-devel] [PATCH proxmox-backup 06/17] auth: add LDAP realm authenticator

[Lukas Wagner <l.wagner@proxmox.com>]

On 1/4/23 14:32, Wolfgang Bumiller wrote:
>> +impl ProxmoxAuthenticator for LDAP {
>> +    /// Authenticate user in LDAP realm
>> +    fn authenticate_user(&self, username: &UsernameRef, password: &str) -> Result<(), Error> {
>> +        let ldap_config = ldap_api_type_to_ldap_config(&self.config)?;
>> +
>> +        let ldap = LdapConnection::new(ldap_config);
>> +
>> +        proxmox_async::runtime::block_on(ldap.authenticate_user(username.as_str(), password))
>> +    }
>> +
>> +    fn store_password(&self, _username: &UsernameRef, _password: &str) -> Result<(), Error> {
>> +        // do not store password for LDAP users
>> +        Ok(())
> Actually this should fail.
> Otherwise this will make change-password API calls "succeed" without
> actually doing anything, but IMO it makes more sense to return a
> meaningful error there.
> 
> (Perhaps even a http_bail!(NOT_IMPLEMENTED) though I'm not really sure
> how the GUI would deal with that 😉 )
> 

Good point. I considered returning a failure here as well, but in the end I decided against it because
the PAM authenticator also returns no failure if one attempts to `remove_password`.
I guess it would make sense then to return a failure there as well?


-- 
- Lukas