[pbs-devel] [PATCH v2 manager 0/2] add user tfa {list, delete} commands

[pbs-devel] [PATCH v2 manager 0/2] add user tfa {list, delete} commands

[Maximiliano Sandoval]

Changes in v2:
  - Rebase on top of master
  - Removed the tfa add command, I might submit it in a separate patch if there
    is interest
  - list commands under user instead of tfa, matching pveum
  - Rename variable defined in config/tfa.rs from json to data
  - Improve doc string for list_user_tfa
  - Move the schema fixup into a separate commit


Maximiliano Sandoval (2):
  api: Fix schema return annotation of tfa_update_auth
  fix #4734: manager: add user tfa {list,delete} commands

 src/api2/access/tfa.rs                 |  9 ++++-
 src/bin/proxmox_backup_manager/user.rs | 53 ++++++++++++++++++++++++++
 src/config/tfa.rs                      | 48 +++++++++++++++++++++++
 3 files changed, 108 insertions(+), 2 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH v2 manager 1/2] api: Fix schema return annotation of tfa_update_auth

[Maximiliano Sandoval]

In the next commit we expose a command to list the tfa methods of a
user. Without this annotation one would get the following error

    unable to format result: got unexpected data (expected null).

when running the proposed cli command.

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/api2/access/tfa.rs | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
index 4b27c0db..89d7e353 100644
--- a/src/api2/access/tfa.rs
+++ b/src/api2/access/tfa.rs
@@ -56,6 +56,11 @@ async fn tfa_update_auth(
     input: {
         properties: { userid: { type: Userid } },
     },
+    returns: {
+        description: "The list of TFA entries.",
+        type: Array,
+        items: { type: methods::TypedTfaInfo }
+    },
     access: {
         permission: &Permission::Or(&[
             &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
-- 
2.39.2

[pbs-devel] applied: [PATCH v2 manager 1/2] api: Fix schema return annotation of tfa_update_auth

[Wolfgang Bumiller]

applied this one

On Tue, Jun 20, 2023 at 03:39:07PM +0200, Maximiliano Sandoval wrote:
> In the next commit we expose a command to list the tfa methods of a
> user. Without this annotation one would get the following error
> 
>     unable to format result: got unexpected data (expected null).
> 
> when running the proposed cli command.
> 
> Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
> ---
>  src/api2/access/tfa.rs | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
> index 4b27c0db..89d7e353 100644
> --- a/src/api2/access/tfa.rs
> +++ b/src/api2/access/tfa.rs
> @@ -56,6 +56,11 @@ async fn tfa_update_auth(
>      input: {
>          properties: { userid: { type: Userid } },
>      },
> +    returns: {
> +        description: "The list of TFA entries.",
> +        type: Array,
> +        items: { type: methods::TypedTfaInfo }
> +    },
>      access: {
>          permission: &Permission::Or(&[
>              &Permission::Privilege(&["access", "users"], PRIV_PERMISSIONS_MODIFY, false),
> -- 
> 2.39.2

[pbs-devel] [PATCH v2 manager 2/2] fix #4734: manager: add user tfa {list, delete} commands

[Maximiliano Sandoval]

Adds the commands

    proxmox-backup-manager user tfa list <userid>
    proxmox-backup-manager user tfa delete <userid> <id>

Signed-off-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
---
 src/api2/access/tfa.rs                 |  4 +-
 src/bin/proxmox_backup_manager/user.rs | 53 ++++++++++++++++++++++++++
 src/config/tfa.rs                      | 48 +++++++++++++++++++++++
 3 files changed, 103 insertions(+), 2 deletions(-)

diff --git a/src/api2/access/tfa.rs b/src/api2/access/tfa.rs
index 89d7e353..589535a6 100644
--- a/src/api2/access/tfa.rs
+++ b/src/api2/access/tfa.rs
@@ -69,7 +69,7 @@ async fn tfa_update_auth(
     },
 )]
 /// Add a TOTP secret to the user.
-fn list_user_tfa(userid: Userid) -> Result<Vec<methods::TypedTfaInfo>, Error> {
+pub fn list_user_tfa(userid: Userid) -> Result<Vec<methods::TypedTfaInfo>, Error> {
     let _lock = crate::config::tfa::read_lock()?;
 
     methods::list_user_tfa(&crate::config::tfa::read()?, userid.as_str())
@@ -122,7 +122,7 @@ fn get_tfa_entry(userid: Userid, id: String) -> Result<methods::TypedTfaInfo, Er
     },
 )]
 /// Delete a single TFA entry.
-async fn delete_tfa(
+pub async fn delete_tfa(
     userid: Userid,
     id: String,
     password: Option<String>,
diff --git a/src/bin/proxmox_backup_manager/user.rs b/src/bin/proxmox_backup_manager/user.rs
index ecaaa554..1a36f393 100644
--- a/src/bin/proxmox_backup_manager/user.rs
+++ b/src/bin/proxmox_backup_manager/user.rs
@@ -157,6 +157,40 @@ fn list_permissions(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Val
     Ok(Value::Null)
 }
 
+#[api(
+    input: {
+        properties: {
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+            userid: {
+                type: Userid,
+            }
+        },
+    }
+)]
+/// List all tfa methods for a user.
+fn list_user_tfa(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = get_output_format(&param);
+
+    let info = &api2::access::tfa::API_METHOD_LIST_USER_TFA;
+    let mut data = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let options = default_table_format_options()
+        .column(ColumnConfig::new("id"))
+        .column(ColumnConfig::new("type"))
+        .column(ColumnConfig::new("description"))
+        .column(ColumnConfig::new("created").renderer(pbs_tools::format::render_epoch));
+
+    format_and_print_result_full(&mut data, &info.returns, &output_format, &options);
+
+    Ok(Value::Null)
+}
+
 pub fn user_commands() -> CommandLineInterface {
     let cmd_def = CliCommandMap::new()
         .insert("list", CliCommand::new(&API_METHOD_LIST_USERS))
@@ -196,6 +230,7 @@ pub fn user_commands() -> CommandLineInterface {
                 .completion_cb("userid", pbs_config::user::complete_userid)
                 .completion_cb("token-name", pbs_config::user::complete_token_name),
         )
+        .insert("tfa", tfa_commands())
         .insert(
             "permissions",
             CliCommand::new(&API_METHOD_LIST_PERMISSIONS)
@@ -206,3 +241,21 @@ pub fn user_commands() -> CommandLineInterface {
 
     cmd_def.into()
 }
+
+fn tfa_commands() -> CommandLineInterface {
+    CliCommandMap::new()
+        .insert(
+            "list",
+            CliCommand::new(&API_METHOD_LIST_USER_TFA)
+                .arg_param(&["userid"])
+                .completion_cb("userid", pbs_config::user::complete_userid),
+        )
+        .insert(
+            "delete",
+            CliCommand::new(&api2::access::tfa::API_METHOD_DELETE_TFA)
+                .arg_param(&["userid", "id"])
+                .completion_cb("userid", pbs_config::user::complete_userid)
+                .completion_cb("id", proxmox_backup::config::tfa::complete_tfa_id),
+        )
+        .into()
+}
diff --git a/src/config/tfa.rs b/src/config/tfa.rs
index 6b1312bb..3cd7f9aa 100644
--- a/src/config/tfa.rs
+++ b/src/config/tfa.rs
@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::fs::File;
 use std::io::{self, Read, Seek, SeekFrom};
 use std::os::unix::fs::OpenOptionsExt;
@@ -302,3 +303,50 @@ impl proxmox_tfa::api::UserChallengeAccess for TfaUserChallengeData {
         TfaUserChallengeData::save(self)
     }
 }
+
+// shell completion helper
+pub fn complete_tfa_id(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
+    let mut results = Vec::new();
+
+    let data = match read() {
+        Ok(data) => data,
+        Err(_err) => return results,
+    };
+    let user = match param
+        .get("userid")
+        .and_then(|user_name| data.users.get(user_name))
+    {
+        Some(user) => user,
+        None => return results,
+    };
+
+    results.extend(
+        user.totp
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.u2f
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.webauthn
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    results.extend(
+        user.yubico
+            .iter()
+            .map(|token| token.info.id.clone())
+            .collect::<Vec<_>>(),
+    );
+    if user.recovery.is_some() {
+        results.push("recovery".to_string());
+    };
+
+    results
+}
-- 
2.39.2

Re: [pbs-devel] [PATCH v2 manager 2/2] fix #4734: manager: add user tfa {list, delete} commands

[Wolfgang Bumiller]

1 minor thing I missed in the 1st round

On Tue, Jun 20, 2023 at 03:39:08PM +0200, Maximiliano Sandoval wrote:
(...)
> diff --git a/src/config/tfa.rs b/src/config/tfa.rs
> index 6b1312bb..3cd7f9aa 100644
> --- a/src/config/tfa.rs
> +++ b/src/config/tfa.rs
> @@ -1,3 +1,4 @@
> +use std::collections::HashMap;
>  use std::fs::File;
>  use std::io::{self, Read, Seek, SeekFrom};
>  use std::os::unix::fs::OpenOptionsExt;
> @@ -302,3 +303,50 @@ impl proxmox_tfa::api::UserChallengeAccess for TfaUserChallengeData {
>          TfaUserChallengeData::save(self)
>      }
>  }
> +
> +// shell completion helper
> +pub fn complete_tfa_id(_arg: &str, param: &HashMap<String, String>) -> Vec<String> {
> +    let mut results = Vec::new();
> +
> +    let data = match read() {
> +        Ok(data) => data,
> +        Err(_err) => return results,
> +    };
> +    let user = match param
> +        .get("userid")
> +        .and_then(|user_name| data.users.get(user_name))
> +    {
> +        Some(user) => user,
> +        None => return results,
> +    };
> +
> +    results.extend(
> +        user.totp
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),

^ .extend() takes an `IntoIterator`, you already have an iterator, you
can drop the `.collect()` call here

> +    );
> +    results.extend(
> +        user.u2f
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),

^ and here

> +    );
> +    results.extend(
> +        user.webauthn
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),

^ and here

> +    );
> +    results.extend(
> +        user.yubico
> +            .iter()
> +            .map(|token| token.info.id.clone())
> +            .collect::<Vec<_>>(),

^ and here

> +    );
> +    if user.recovery.is_some() {
> +        results.push("recovery".to_string());
> +    };
> +
> +    results
> +}
> -- 
> 2.39.2