Re: [PATCH datacenter-manager v3 2/6] server: api: add support to optionally delete token from remote

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

Hi Shan, thanks for the patch!

Some notes inline.

On Wed Feb 11, 2026 at 4:20 PM CET, Shan Shaji wrote:
> Previously, when removing a remote, the token was still present in the
> remote configuration. When users tried to add the remote again, they
> received an error because a token with the same name already existed.
> To support deleting the token from the remote, add an optional
> parameter to the API endpoint.
>
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
>
> changes since v2: No changes. 
> changes since v1:
>  - nit: inlined the id argument using the format string.
>  - used `get` instead of `get_mut` inorder to access remote.
>  - removed unnecessary `&` operator use.
>
>  server/src/api/remotes.rs | 46 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/server/src/api/remotes.rs b/server/src/api/remotes.rs
> index 298ad13..82b8469 100644
> --- a/server/src/api/remotes.rs
> +++ b/server/src/api/remotes.rs
> @@ -27,6 +27,7 @@ use crate::api::remote_updates;
>  use crate::metric_collection;
>  use crate::{connection, pbs_client};
>  
> +use super::pbs;
>  use super::pve;
>  use super::rrd_common;
>  use super::rrd_common::DataPoint;
> @@ -292,16 +293,57 @@ pub fn update_remote(
>      input: {
>          properties: {
>              id: { schema: REMOTE_ID_SCHEMA },
> +            "delete-token": {
> +                type: bool,
> +                description: "Optional boolean value to delete the token from remote.",
> +                optional: true,

You can set 'default: false' and also remove 'type: bool' (since the
type is already clear from the handler function signature).

Also, I'd slightly rephrase the description to something like:

  "Remove the API token used by Proxmox Datacenter Manager from the remote"

Both, the fact that the value is optional and that it is a bool should
be evident from the API schema, so there is no need to mention it again
in the description.

> +            }
>          },
>      },
>      access: {
>          permission: &Permission::Privilege(&["resource"], PRIV_RESOURCE_MODIFY, false),
>      },
>  )]
> -/// List all the remotes this instance is managing.
> -pub fn remove_remote(id: String) -> Result<(), Error> {
> +/// Remove a remote that this instance is managing.
> +pub async fn remove_remote(id: String, delete_token: Option<bool>) -> Result<(), Error> {

With 'default: false', the parameter just becomes a `bool` instead of
`Option<bool>`

> +    let _lock = pdm_config::remotes::lock_config()?;
>      let (mut remotes, _) = pdm_config::remotes::config()?;
>  
> +    if delete_token.unwrap_or(false) {

... and you can remove this `.unwrap_or(false)`

The benefit of setting the default via the schema is that then it is
also automatically documented in the API viewer.

> +        let remote = remotes
> +            .get(&id)
> +            .ok_or_else(|| http_err!(NOT_FOUND, "no such remote {id:?}"))?;
> +
> +        let user = remote.authid.user();
> +
> +        let short_delete_err = |err: proxmox_client::Error| {
> +            format_err!("error deleting token: {}", err.source().unwrap_or(&err))
> +        };

Since you copied this helper from `add_remote`, maybe also copy the doc
comment from there? Since it somewhat explains why this helper exists in
the first place.

> +
> +        let token_name = remote
> +            .authid
> +            .tokenname()
> +            .ok_or_else(|| format_err!("Unable to find the token for the remote {id:?}"))?;
> +
> +        // connect to remote and delete the already existing token.
> +        match remote.ty {
> +            RemoteType::Pve => {
> +                let client = pve::connect_or_login(remote).await?;

I don't think there is a benefit of using the `_or_login` variant here.
It's really only used when initially adding the remote, when one only
provides username/password in the wizard. 

When removing the remote, we should have a token anyways (that we are
about to remove) - so rather use `connection::make_{pve,pbs}_client(remote)` here.

(unless I'm missing something)

> +                client
> +                    .delete_token(user.as_str(), token_name.as_str())
> +                    .await
> +                    .map_err(short_delete_err)?
> +            }
> +            RemoteType::Pbs => {
> +                let client = pbs::connect_or_login(remote).await?;
> +                client
> +                    .delete_admin_token(user, token_name.as_str())
> +                    .await
> +                    .map_err(short_delete_err)?
> +            }
> +        };
> +    }
> +
>      if remotes.remove(&id).is_none() {
>          http_bail!(NOT_FOUND, "no such entry {id:?}");
>      }