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

["Michael Köppl" <m.koeppl@proxmox.com>]

On Fri Dec 5, 2025 at 7:04 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>
> ---
>  server/src/api/remotes.rs | 45 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/server/src/api/remotes.rs b/server/src/api/remotes.rs
> index 298ad13..9f9786c 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,56 @@ 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,
> +            }
>          },
>      },
>      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> {
>      let (mut remotes, _) = pdm_config::remotes::config()?;

Isn't that a bit racy, though, considering you're making asynchronous
network calls if delete_token is true? By the time you arrive at the
save_config(...) call, the list of remotes you fetched here might have
changed because another call to remove a remote was made pretty much at
the same time, causing you to overwrite those changes with the list of
remotes you fetched here. Not too familiar with the code here, but we
use pdm_config::remotes::lock_config()?; in other parts to avoid this,
it seems.

>  
> +    if delete_token.unwrap_or(false) {
> +        let remote = remotes
> +            .get_mut(&id)

nit: .get(&id) should be sufficient

> +            .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))
> +        };
> +
> +        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?;

nit: this could just be pve::connect_or_login(remote).await?

> +                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?;

nit: same here

> +                client
> +                    .delete_admin_token(&user, token_name.as_str())

nit: same here for user

> +                    .await
> +                    .map_err(short_delete_err)?
> +            }
> +        };
> +    }
> +
>      if remotes.remove(&id).is_none() {
>          http_bail!(NOT_FOUND, "no such entry {id:?}");
>      }



_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel