Re: [pbs-devel] [PATCH proxmox-backup 4/5] fix #3887: api2: add regenerate token endpoint

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 20/12/2022 15:57, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/access/user.rs | 61 ++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index 40177c8d..c2b563f7 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -628,6 +628,59 @@ pub fn update_token(
>      Ok(())
>  }
>  
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            userid: {
> +                type: Userid,
> +            },
> +            "token-name": {
> +                type: Tokenname,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Or(&[
> +            &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
> +            &Permission::UserParam("userid"),
> +        ]),
> +    },
> +    returns: {
> +        description: "API token identifier + new generated secret.",
> +        properties: {
> +            value: {

"token-secret" ?

> +                type: String,
> +                description: "The API token secret",
> +            },
> +            tokenid: {

"token-id" ?

Or maybe, as we're on a token API call anyway we could drop the "token" completely
and just use "id" and "secret"

> +                type: String,
> +                description: "The API token identifier",
> +            },
> +        },
> +    },
> +)]
> +/// Regenerate an API token's secret, revokes the old secret and create a new one
> +pub fn regenerate_token(userid: Userid, token_name: Tokenname) -> Result<Value, Error> {
> +    let _user_lock = pbs_config::user::lock_config()?;
> +
> +    let tokenid = Authid::from((userid.clone(), Some(token_name.clone())));
> +    let tokenid_string = tokenid.to_string();
> +
> +    let (user_config, _digest) = pbs_config::user::config()?;
> +
> +    // token just has to exist, we don't actually need it
> +    let _data: ApiToken = user_config.lookup("token", &tokenid_string)?;

dumb question: do we want to check the token expiration date, if any, and
bail from regeneration if it expired?

> +
> +    let secret = format!("{:x}", proxmox_uuid::Uuid::generate());

would be IMO nicer to have a central method that generates the secret,
even for things that ain't _that_ likely to change it's just nicer to
avoid having the way its assembled re-done multiple times manually.

maybe that could be moved into token_shadow so that we then only call
something like:

let secret = token_shadow::generate_and_set_secret(&tokenid);

> +    token_shadow::set_secret(&tokenid, &secret)?;
> +
> +    Ok(json!({
> +        "tokenid": tokenid_string,
> +        "value": secret
> +    }))
> +}
> +
>  #[api(
>      protected: true,
>      input: {
> @@ -754,11 +807,17 @@ pub fn list_tokens(
>      Ok(res)
>  }
>  
> +const TOKEN_SUBDIRS: SubdirMap = &[(
> +    "regenerate",
> +    &Router::new().post(&API_METHOD_REGENERATE_TOKEN),
> +)];
> +
>  const TOKEN_ITEM_ROUTER: Router = Router::new()
>      .get(&API_METHOD_READ_TOKEN)
>      .put(&API_METHOD_UPDATE_TOKEN)
>      .post(&API_METHOD_GENERATE_TOKEN)
> -    .delete(&API_METHOD_DELETE_TOKEN);
> +    .delete(&API_METHOD_DELETE_TOKEN)
> +    .subdirs(TOKEN_SUBDIRS);

hmm, but now I cannot get the available subdir's via GET due to that being
already used for reading the token info. Besides the added imperfection, I'm
actually not sure from top of my head about the implications in PBS, but in
PVE this would cause some technical issues in pvesh/api-viewer - did you
check how those (debug api and api-viewer) handle to a shared subdir + "normal"
get on the same API endpoint?

>  
>  const TOKEN_ROUTER: Router = Router::new()
>      .get(&API_METHOD_LIST_TOKENS)