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

["Shan Shaji" <s.shaji@proxmox.com>]

On Tue Dec 9, 2025 at 1:34 PM CET, Michael Köppl wrote:
> 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.

You are right. There is a possibility of race condition. thanks
for pointing it out. I will lock the config here. 

>>  
>> +    if delete_token.unwrap_or(false) {
>> +        let remote = remotes
>> +            .get_mut(&id)
>
> nit: .get(&id) should be sufficient

Thanks! Will update it.

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

Will update it. Thanks!

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



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