[pbs-devel] [PATCH v3 proxmox-backup 09/20] server/rest: add ApiAuth trait to make user auth generic

[Thomas Lamprecht]

On 31.03.21 14:55, Wolfgang Bumiller wrote:
>> -pub struct UserAuthData {
>> +pub enum AuthError {
>> +    Generic(Error),
>> +    NoData,
>> +}
>> +
>> +impl From<Error> for AuthError {
>> +    fn from(err: Error) -> Self {
>> +        AuthError::Generic(err)
>> +    }
>> +}
> ^ When you define an Error type you should immediately also derive Debug
> and implement `std::fmt::Display`. While you only "display" it once, I
> don't think the error message should be left up to the caller (see
> further down below).
> 
> In order to make this shorter and easier, I'd propose the inclusion of
> the `thiserror` helper crate, then we'd need only as little code as:
> 
>     #[derive(thiserror::Error, Debug)]
>     pub enum AuthError {
>         #[error(transparent)]
>         Generic(#[from] Error),
> 
>         #[error("no authentication credentials provided")]
>         NoData,
>     }
> 
> And the manual `From<anyhow::Error>` impl can simply be dropped ;-)

+1 for using thiserror in general from my side...

> 
> That is *unless* you explicitly want this to *not* be convertible to
> `anyhow::Error` automagically. Then you don't want this to impl
> `std::error::Error`, but then you should add a comment for this ;-)
> Which would be a valid way to go given that you're already wrapping
> `anyhow::Error` in there... OTOH all the token parsing errors could be
> combined into a single `AuthError::BadToken` instead of multiple
> "slightly different" `format_err!` messages where the minor difference
> in the error message doesn't really provide much value anyway (IMO).
> 
> Anyway, this can easily be left as is and updated up in a later series.
>