[pbs-devel] [PATCH proxmox-backup 0/5] ACL removal on user/token deletion + token regeneration

[pbs-devel] [PATCH proxmox-backup 0/5] ACL removal on user/token deletion + token regeneration

[Hannes Laimer]

If a user is deleted all its permissions and tokens
will now be deleted with it. If a token is deleted
all its permissions will now be deleted.
Until now neither of those two happened[1].
The last two commits add the possibility to regenerate
tokens, basically revoking the old and generating a
new secret while keeping all the set permissions.

This is all in the same series since just adding the
removal of permissions would kill the currently only
way to keep the permissions but change the secret of
a token(deleting it and creating it again with the
same name[2]).

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4382
[2] https://bugzilla.proxmox.com/show_bug.cgi?id=3887

Hannes Laimer (5):
  pbs-config: add delete_authid to ACL-tree
  fix #4382: api2: remove permissions of token on deletion
  fix #4382: api2: remove permissions and tokens of user on deletion
  fix #3887: api2: add regenerate token endpoint
  fix #3887: ui: add regenerate token button

 pbs-config/src/acl.rs   | 71 +++++++++++++++++++++++++++++++
 src/api2/access/user.rs | 92 +++++++++++++++++++++++++++++++++++++++--
 www/config/TokenView.js | 30 ++++++++++++++
 3 files changed, 190 insertions(+), 3 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 1/5] pbs-config: add delete_authid to ACL-tree

[Hannes Laimer]

... allows the deletion of an authid from the whole tree. Needed
for removing deleted users/tokens.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-config/src/acl.rs | 71 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/pbs-config/src/acl.rs b/pbs-config/src/acl.rs
index 89a54dfc..a4a79755 100644
--- a/pbs-config/src/acl.rs
+++ b/pbs-config/src/acl.rs
@@ -280,6 +280,13 @@ impl AclTreeNode {
         roles.remove(role);
     }
 
+    fn delete_authid(&mut self, auth_id: &Authid) {
+        for (_name, node) in self.children.iter_mut() {
+            node.delete_authid(auth_id);
+        }
+        self.users.remove(auth_id);
+    }
+
     fn insert_group_role(&mut self, group: String, role: String, propagate: bool) {
         let map = self.groups.entry(group).or_default();
         if role == ROLE_NAME_NO_ACCESS {
@@ -411,6 +418,14 @@ impl AclTree {
         }
     }
 
+    /// Deletes a user or token from the ACL-tree
+    ///
+    /// Traverses the tree in-order and removes the given user/token by their Authid
+    /// from every node in the tree.
+    pub fn delete_authid(&mut self, auth_id: &Authid) {
+        self.root.delete_authid(auth_id);
+    }
+
     /// Inserts the specified `role` into the `group` ACL on `path`.
     ///
     /// The [`AclTreeNode`] representing `path` will be created and inserted into the tree if
@@ -1010,4 +1025,60 @@ acl:1:/storage/store1:user1@pbs:DatastoreBackup
 
         Ok(())
     }
+
+    #[test]
+    fn test_delete_authid() -> Result<(), Error> {
+        let mut tree = AclTree::new();
+
+        let user1: Authid = "user1@pbs".parse()?;
+        let user2: Authid = "user2@pbs".parse()?;
+
+        let user1_paths = vec![
+            "/",
+            "/storage",
+            "/storage/a",
+            "/storage/a/b",
+            "/storage/b",
+            "/storage/b/a",
+            "/storage/b/b",
+            "/storage/a/a",
+        ];
+        let user2_paths = vec!["/", "/storage", "/storage/a/b", "/storage/a/a"];
+
+        for path in &user1_paths {
+            tree.insert_user_role(path, &user1, "NoAccess", true);
+        }
+        for path in &user2_paths {
+            tree.insert_user_role(path, &user2, "NoAccess", true);
+        }
+
+        tree.delete_authid(&user1);
+
+        for path in &user1_paths {
+            let node = tree.find_node(path);
+            assert!(node.is_some());
+            if let Some(node) = node {
+                assert!(node.users.get(&user1).is_none());
+            }
+        }
+        for path in &user2_paths {
+            let node = tree.find_node(path);
+            assert!(node.is_some());
+            if let Some(node) = node {
+                assert!(node.users.get(&user2).is_some());
+            }
+        }
+
+        tree.delete_authid(&user2);
+
+        for path in &user2_paths {
+            let node = tree.find_node(path);
+            assert!(node.is_some());
+            if let Some(node) = node {
+                assert!(node.users.get(&user2).is_none());
+            }
+        }
+
+        Ok(())
+    }
 }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/5] fix #4382: api2: remove permissions of token on deletion

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/access/user.rs | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index 2264f8e8..ce676252 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -635,7 +635,8 @@ pub fn delete_token(
     token_name: Tokenname,
     digest: Option<String>,
 ) -> Result<(), Error> {
-    let _lock = pbs_config::user::lock_config()?;
+    let _acl_lock = pbs_config::acl::lock_config()?;
+    let _user_lock = pbs_config::user::lock_config()?;
 
     let (mut config, expected_digest) = pbs_config::user::config()?;
 
@@ -662,6 +663,10 @@ pub fn delete_token(
 
     pbs_config::user::save_config(&config)?;
 
+    let (mut tree, _digest) = pbs_config::acl::config()?;
+    tree.delete_authid(&Authid::from((userid, Some(token_name))));
+    pbs_config::acl::save_config(&tree)?;
+
     Ok(())
 }
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/5] fix #4382: api2: remove permissions and tokens of user on deletion

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/access/user.rs | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
index ce676252..40177c8d 100644
--- a/src/api2/access/user.rs
+++ b/src/api2/access/user.rs
@@ -346,7 +346,7 @@ pub fn update_user(
 )]
 /// Remove a user from the configuration file.
 pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
-    let _lock = pbs_config::user::lock_config()?;
+    let _user_lock = pbs_config::user::lock_config()?;
     let _tfa_lock = crate::config::tfa::write_lock()?;
 
     let (mut config, expected_digest) = pbs_config::user::config()?;
@@ -390,6 +390,28 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
         }
     }
 
+    // delete_token needs the user config lock, therefore we have to drop it here
+    drop(_user_lock);
+
+    let user_tokens: Vec<ApiToken> = config
+        .convert_to_typed_array::<ApiToken>("token")?
+        .into_iter()
+        .filter(|token| token.tokenid.user().eq(&userid))
+        .collect();
+    for token in user_tokens {
+        if let Some(name) = token.tokenid.tokenname() {
+            let user = token.tokenid.user();
+            delete_token(user.clone(), name.to_owned(), None)?
+        }
+    }
+
+    // delete_token needs the acl config lock, that's why it is not aquired at the beginning
+    let _acl_lock = pbs_config::acl::lock_config()?;
+
+    let (mut tree, _digest) = pbs_config::acl::config()?;
+    tree.delete_authid(&Authid::from(userid));
+    pbs_config::acl::save_config(&tree)?;
+
     Ok(())
 }
 
-- 
2.30.2

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

[Hannes Laimer]

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: {
+                type: String,
+                description: "The API token secret",
+            },
+            tokenid: {
+                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)?;
+
+    let secret = format!("{:x}", proxmox_uuid::Uuid::generate());
+    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);
 
 const TOKEN_ROUTER: Router = Router::new()
     .get(&API_METHOD_LIST_TOKENS)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 5/5] fix #3887: ui: add regenerate token button

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/config/TokenView.js | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/www/config/TokenView.js b/www/config/TokenView.js
index 6282a4d4..745f1378 100644
--- a/www/config/TokenView.js
+++ b/www/config/TokenView.js
@@ -100,6 +100,30 @@ Ext.define('PBS.config.TokenView', {
 	    }).show();
 	},
 
+	regenerateToken: function() {
+            let me = this;
+            let view = me.getView();
+            let selection = view.getSelection();
+            if (selection.length < 1) return;
+            let tokenid = selection[0].data.tokenid;
+            let user = PBS.Utils.extractTokenUser(tokenid);
+            let tokenname = PBS.Utils.extractTokenName(tokenid);
+            Proxmox.Utils.API2Request({
+                method: "POST",
+                url: `/access/users/${user}/token/${tokenname}/regenerate`,
+                success: function(res, opt) {
+                    Ext.create("PBS.window.TokenShow", {
+                        autoShow: true,
+                        tokenid: res.result.data.tokenid,
+                        secret: res.result.data.value,
+                    });
+                },
+                failure: function(response, opt) {
+                    Ext.Msg.alert(gettext("Error"), response.htmlStatus);
+                },
+            });
+        },
+
 	showPermissions: function() {
 	    let me = this;
 	    let view = me.getView();
@@ -174,6 +198,12 @@ Ext.define('PBS.config.TokenView', {
 	    handler: 'showPermissions',
 	    disabled: true,
 	},
+	{
+	    xtype: 'proxmoxButton',
+	    text: gettext('Regenerate'),
+	    handler: 'regenerateToken',
+	    disabled: true,
+	},
     ],
 
     viewConfig: {
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #4382: api2: remove permissions and tokens of user on deletion

[Thomas Lamprecht]

On 20/12/2022 15:57, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/access/user.rs | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index ce676252..40177c8d 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -346,7 +346,7 @@ pub fn update_user(
>  )]
>  /// Remove a user from the configuration file.
>  pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
> -    let _lock = pbs_config::user::lock_config()?;
> +    let _user_lock = pbs_config::user::lock_config()?;
>      let _tfa_lock = crate::config::tfa::write_lock()?;
>  
>      let (mut config, expected_digest) = pbs_config::user::config()?;
> @@ -390,6 +390,28 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
>          }
>      }
>  
> +    // delete_token needs the user config lock, therefore we have to drop it here
> +    drop(_user_lock);

opens us up for a data race though. Why not have a private (or marked unsafe)
delete_token_no_lock fn that we can call here and in delete_token?

Alternative but similar: a private do_delete_token that takes the locks as
parameters, probably nicer.

Did not looked at the whole code closely in recent times, so there may even
better ways - possible encapsulating this in a impl on a User/AuthId type
(especially helpful if we want to re-use this for other projects); but for now
I think the do_delete_token taking the locks as parameters would be the
simplest and nicest to avoid races and other issues.

> +
> +    let user_tokens: Vec<ApiToken> = config
> +        .convert_to_typed_array::<ApiToken>("token")?
> +        .into_iter()
> +        .filter(|token| token.tokenid.user().eq(&userid))
> +        .collect();
> +    for token in user_tokens {
> +        if let Some(name) = token.tokenid.tokenname() {
> +            let user = token.tokenid.user();
> +            delete_token(user.clone(), name.to_owned(), None)?
> +        }
> +    }
> +
> +    // delete_token needs the acl config lock, that's why it is not aquired at the beginning

s/aquired/acquired/

> +    let _acl_lock = pbs_config::acl::lock_config()?;
> +
> +    let (mut tree, _digest) = pbs_config::acl::config()?;
> +    tree.delete_authid(&Authid::from(userid));
> +    pbs_config::acl::save_config(&tree)?;
> +
>      Ok(())
>  }
>  

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

[Thomas Lamprecht]

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)

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

[Fabian Grünbichler]

On December 21, 2022 10:56 am, Thomas Lamprecht wrote:
> 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

> [..]

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

yeah, this would break editing tokens (and external calls to that GET API endpoint).

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

it could be folded either into the update_token call (currently no return type,
could return an optional new secret) or the generate_token call, guarded by a
new "regenerate" boolean parameter. for updating it could even be combined with
an actual metadata update, for generate it would probably make more sense to
enforce that any optional parameters match the current one to avoid accidents..

Re: [pbs-devel] [PATCH proxmox-backup 5/5] fix #3887: ui: add regenerate token button

[Thomas Lamprecht]

On 20/12/2022 15:57, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  www/config/TokenView.js | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/www/config/TokenView.js b/www/config/TokenView.js
> index 6282a4d4..745f1378 100644
> --- a/www/config/TokenView.js
> +++ b/www/config/TokenView.js
> @@ -100,6 +100,30 @@ Ext.define('PBS.config.TokenView', {
>  	    }).show();
>  	},
>  
> +	regenerateToken: function() {

note that with using a proxmoxButton this has the signature of

(button, event, rec)

where the record probably should be enough to avoid pulling out the selection manually.

> +            let me = this;
> +            let view = me.getView();
> +            let selection = view.getSelection();
> +            if (selection.length < 1) return;
> +            let tokenid = selection[0].data.tokenid;
> +            let user = PBS.Utils.extractTokenUser(tokenid);
> +            let tokenname = PBS.Utils.extractTokenName(tokenid);
> +            Proxmox.Utils.API2Request({
> +                method: "POST",
> +                url: `/access/users/${user}/token/${tokenname}/regenerate`,
> +                success: function(res, opt) {
> +                    Ext.create("PBS.window.TokenShow", {
> +                        autoShow: true,
> +                        tokenid: res.result.data.tokenid,
> +                        secret: res.result.data.value,
> +                    });
> +                },
> +                failure: function(response, opt) {
> +                    Ext.Msg.alert(gettext("Error"), response.htmlStatus);
> +                },

would prefer a arrow fn here:

failure: res => Ext.Msg.alert(gettext("Error"), res.htmlStatus),

> +            });
> +        },
> +
>  	showPermissions: function() {
>  	    let me = this;
>  	    let view = me.getView();
> @@ -174,6 +198,12 @@ Ext.define('PBS.config.TokenView', {
>  	    handler: 'showPermissions',
>  	    disabled: true,
>  	},
> +	{
> +	    xtype: 'proxmoxButton',
> +	    text: gettext('Regenerate'),
> +	    handler: 'regenerateToken',

I'd really set the proxmoxButton's config for confirming and auto-selecting
the no choice by default to avoid that an "sneeze-and-click" accident revokes
access of an in-use token without any confirmation prompt at all.

E.g., add something like:

    dangerous: true,
    confirmMsg: rec => Ext.String.format(
        gettext("Regenerate the secret of the API token '{0}'? All current use-sites will loose access!"),
        rec.data.tokenid, // <- just a guesstimate, didn't actually test this
    ),

The function could also just be a plain text without referencing the token ID,
but IMO it has some non-negligible value to show such things in confirmations.

> +	    disabled: true,
> +	},
>      ],
>  
>      viewConfig: {
))))

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

[Thomas Lamprecht]

On 21/12/2022 11:53, Fabian Grünbichler wrote:
>>> +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?
> yeah, this would break editing tokens (and external calls to that GET API endpoint).
> 
>>>  
>>>  const TOKEN_ROUTER: Router = Router::new()
>>>      .get(&API_METHOD_LIST_TOKENS)
> it could be folded either into the update_token call (currently no return type,
> could return an optional new secret) or the generate_token call, guarded by a
> new "regenerate" boolean parameter. for updating it could even be combined with
> an actual metadata update, for generate it would probably make more sense to
> enforce that any optional parameters match the current one to avoid accidents..

I'd favor the PUT update one from a REST design POV, as an existing resource
is changed, not a new resource allocated.