[pbs-devel] [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

[pbs-devel] [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

[Fabian Grünbichler]

document all public things, add some doc links and make some
previously-public things only available for test cases or within the
crate:

previously public, now private:
- AclTreeNode::extract_user_roles (we have extract_roles())
- AclTreeNode::extract_group_roles (same)
- AclTreeNode::delete_group_role (exists on AclTree)
- AclTreeNode::delete_user_role (same)
- AclTreeNode::insert_group_role (same)
- AclTreeNode::insert_user_role (same)
- AclTree::write_config (we have save_config())
- AclTree::load (we have config()/cached_config())

previously public, now crate-internal:
- AclTree::from_raw (only used by tests)
- split_acl_path (used by some test binaries)

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
RFC to get some feedback on which rustdoc features we actually want to
use here, and which we want to leave out for now.

 src/config/acl.rs | 113 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 94 insertions(+), 19 deletions(-)

diff --git a/src/config/acl.rs b/src/config/acl.rs
index 8cdce8bf..8503a2ab 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -20,10 +20,16 @@ use crate::api2::types::{Authid,Userid};
 // define Privilege bitfield
 
 constnamedbitmap! {
-    /// Contains a list of Privileges
+    /// Contains a list of privilege name to privilege value mappings.
+    ///
+    /// The names are used when displaying/persisting privileges anywhere, the values are used to
+    /// allow easy matching of privileges as bitflags.
     PRIVILEGES: u64 => {
+        /// Sys.Audit allows knowing about the system and its status
         PRIV_SYS_AUDIT("Sys.Audit");
+        /// Sys.Modify allows modifying system-level configuration
         PRIV_SYS_MODIFY("Sys.Modify");
+        /// Sys.Modify allows to poweroff/reboot/.. the system
         PRIV_SYS_POWER_MANAGEMENT("Sys.PowerManagement");
 
         /// Datastore.Audit allows knowing about a datastore,
@@ -45,12 +51,17 @@ constnamedbitmap! {
         /// but also requires backup ownership
         PRIV_DATASTORE_PRUNE("Datastore.Prune");
 
+        /// Permissions.Modify allows modifying ACLs
         PRIV_PERMISSIONS_MODIFY("Permissions.Modify");
 
+        /// Remote.Audit allows reading remote.cfg and sync.cfg entries
         PRIV_REMOTE_AUDIT("Remote.Audit");
+        /// Remote.Modify allows modifying remote.cfg
         PRIV_REMOTE_MODIFY("Remote.Modify");
+        /// Remote.Read allows reading data from a configured `Remote`
         PRIV_REMOTE_READ("Remote.Read");
 
+        /// Sys.Console allows access to the system's console
         PRIV_SYS_CONSOLE("Sys.Console");
     }
 }
@@ -60,9 +71,10 @@ constnamedbitmap! {
 /// which are limited to the 'root@pam` superuser
 pub const ROLE_ADMIN: u64 = std::u64::MAX;
 
-/// NoAccess can be used to remove privileges from specific paths
+/// NoAccess can be used to remove privileges from specific (sub-)paths
 pub const ROLE_NO_ACCESS: u64 = 0;
 
+/// Audit can view configuration and status information, but not modify it.
 pub const ROLE_AUDIT: u64 =
 PRIV_SYS_AUDIT |
 PRIV_DATASTORE_AUDIT;
@@ -110,12 +122,16 @@ pub const ROLE_REMOTE_SYNC_OPERATOR: u64 =
 PRIV_REMOTE_AUDIT |
 PRIV_REMOTE_READ;
 
+/// NoAccess can be used to remove privileges from specific (sub-)paths
 pub const ROLE_NAME_NO_ACCESS: &str ="NoAccess";
 
 #[api()]
 #[repr(u64)]
 #[derive(Serialize, Deserialize)]
-/// Role
+/// Enum representing roles via their [PRIVILEGES] combination.
+///
+/// Since privileges are implemented as bitflags, each unique combination of privileges maps to a
+/// single, unique `u64` value that is used in this enum definition.
 pub enum Role {
     /// Administrator
     Admin = ROLE_ADMIN,
@@ -150,6 +166,8 @@ impl FromStr for Role {
 }
 
 lazy_static! {
+    /// Map of pre-defined [Roles](Role) to their associated [privileges](PRIVILEGES) combination and
+    /// description.
     pub static ref ROLE_NAMES: HashMap<&'static str, (u64, &'static str)> = {
         let mut map = HashMap::new();
 
@@ -167,8 +185,7 @@ lazy_static! {
     };
 }
 
-pub fn split_acl_path(path: &str) -> Vec<&str> {
-
+pub(crate) fn split_acl_path(path: &str) -> Vec<&str> {
     let items = path.split('/');
 
     let mut components = vec![];
@@ -181,6 +198,9 @@ pub fn split_acl_path(path: &str) -> Vec<&str> {
     components
 }
 
+/// Check whether a given ACL `path` conforms to the expected schema.
+///
+/// Currently this just checks for the number of components for various sub-trees.
 pub fn check_acl_path(path: &str) -> Result<(), Error> {
 
     let components = split_acl_path(path);
@@ -234,18 +254,29 @@ pub fn check_acl_path(path: &str) -> Result<(), Error> {
     bail!("invalid acl path '{}'.", path);
 }
 
+/// Tree representing a parsed acl.cfg
 pub struct AclTree {
+    /// Root node of the tree.
+    ///
+    /// The rest of the tree is available via [find_node()](AclTree::find_node()) or an
+    /// [AclTreeNode]'s [children](AclTreeNode::children) member.
     pub root: AclTreeNode,
 }
 
+/// Node representing ACLs for a certain ACL path.
 pub struct AclTreeNode {
+    /// [User](crate::config::user::User) or
+    /// [Token](crate::config::user::ApiToken) ACLs for this node.
     pub users: HashMap<Authid, HashMap<String, bool>>,
+    /// `Group` ACLs for this node (not yet implemented)
     pub groups: HashMap<String, HashMap<String, bool>>,
+    /// `AclTreeNodes` representing ACL paths directly below the current one.
     pub children: BTreeMap<String, AclTreeNode>,
 }
 
 impl AclTreeNode {
 
+    /// Creates a new, empty AclTreeNode.
     pub fn new() -> Self {
         Self {
             users: HashMap::new(),
@@ -254,17 +285,25 @@ impl AclTreeNode {
         }
     }
 
-    pub fn extract_roles(&self, auth_id: &Authid, all: bool) -> HashMap<String, bool> {
-        let user_roles = self.extract_user_roles(auth_id, all);
+    /// Returns applicable [Role] and their propagation status for a given
+    /// [Authid](crate::api2::types::Authid).
+    ///
+    /// If the `Authid` is a [User](crate::config::user::User) that has no specific `Roles` configured on this node,
+    /// applicable `Group` roles will be returned instead.
+    ///
+    /// If `leaf` is `false`, only those roles where the propagate flag in the ACL is set to `true`
+    /// are returned. Otherwise, all roles will be returned.
+    pub fn extract_roles(&self, auth_id: &Authid, leaf: bool) -> HashMap<String, bool> {
+        let user_roles = self.extract_user_roles(auth_id, leaf);
         if !user_roles.is_empty() || auth_id.is_token() {
             // user privs always override group privs
             return user_roles
         };
 
-        self.extract_group_roles(auth_id.user(), all)
+        self.extract_group_roles(auth_id.user(), leaf)
     }
 
-    pub fn extract_user_roles(&self, auth_id: &Authid, all: bool) -> HashMap<String, bool> {
+    fn extract_user_roles(&self, auth_id: &Authid, leaf: bool) -> HashMap<String, bool> {
 
         let mut map = HashMap::new();
 
@@ -274,7 +313,7 @@ impl AclTreeNode {
         };
 
         for (role, propagate) in roles {
-            if *propagate || all {
+            if *propagate || leaf {
                 if role == ROLE_NAME_NO_ACCESS {
                     // return a map with a single role 'NoAccess'
                     let mut map = HashMap::new();
@@ -288,7 +327,7 @@ impl AclTreeNode {
         map
     }
 
-    pub fn extract_group_roles(&self, _user: &Userid, all: bool) -> HashMap<String, bool> {
+    fn extract_group_roles(&self, _user: &Userid, leaf: bool) -> HashMap<String, bool> {
 
         let mut map = HashMap::new();
 
@@ -297,7 +336,7 @@ impl AclTreeNode {
             if !is_member { continue; }
 
             for (role, propagate) in roles {
-                if *propagate || all {
+                if *propagate || leaf {
                     if role == ROLE_NAME_NO_ACCESS {
                         // return a map with a single role 'NoAccess'
                         let mut map = HashMap::new();
@@ -312,7 +351,7 @@ impl AclTreeNode {
         map
     }
 
-    pub fn delete_group_role(&mut self, group: &str, role: &str) {
+    fn delete_group_role(&mut self, group: &str, role: &str) {
         let roles = match self.groups.get_mut(group) {
             Some(r) => r,
             None => return,
@@ -320,7 +359,7 @@ impl AclTreeNode {
         roles.remove(role);
     }
 
-    pub fn delete_user_role(&mut self, auth_id: &Authid, role: &str) {
+    fn delete_user_role(&mut self, auth_id: &Authid, role: &str) {
         let roles = match self.users.get_mut(auth_id) {
             Some(r) => r,
             None => return,
@@ -328,7 +367,7 @@ impl AclTreeNode {
         roles.remove(role);
     }
 
-    pub fn insert_group_role(&mut self, group: String, role: String, propagate: bool) {
+    fn insert_group_role(&mut self, group: String, role: String, propagate: bool) {
         let map = self.groups.entry(group).or_insert_with(|| HashMap::new());
         if role == ROLE_NAME_NO_ACCESS {
             map.clear();
@@ -339,7 +378,7 @@ impl AclTreeNode {
         }
     }
 
-    pub fn insert_user_role(&mut self, auth_id: Authid, role: String, propagate: bool) {
+    fn insert_user_role(&mut self, auth_id: Authid, role: String, propagate: bool) {
         let map = self.users.entry(auth_id).or_insert_with(|| HashMap::new());
         if role == ROLE_NAME_NO_ACCESS {
             map.clear();
@@ -353,12 +392,14 @@ impl AclTreeNode {
 
 impl AclTree {
 
+    /// Create a new, empty ACL tree with a single, empty root [node](AclTreeNode)
     pub fn new() -> Self {
         Self {
             root: AclTreeNode::new(),
         }
     }
 
+    /// Iterates over the tree looking for a node matching `path`.
     pub fn find_node(&mut self, path: &str) -> Option<&mut AclTreeNode> {
         let path = split_acl_path(path);
         return self.get_node(&path);
@@ -384,6 +425,10 @@ impl AclTree {
         node
     }
 
+    /// Deletes the specified `role` from the `group`'s ACL on `path`.
+    ///
+    /// Never fails, even if the `path` has no ACLs configured, or the `group`/`role` combination
+    /// does not exist on `path`.
     pub fn delete_group_role(&mut self, path: &str, group: &str, role: &str) {
         let path = split_acl_path(path);
         let node = match self.get_node(&path) {
@@ -393,6 +438,10 @@ impl AclTree {
         node.delete_group_role(group, role);
     }
 
+    /// Deletes the specified `role` from the `user`'s ACL on `path`.
+    ///
+    /// Never fails, even if the `path` has no ACLs configured, or the `user`/`role` combination
+    /// does not exist on `path`.
     pub fn delete_user_role(&mut self, path: &str, auth_id: &Authid, role: &str) {
         let path = split_acl_path(path);
         let node = match self.get_node(&path) {
@@ -402,12 +451,20 @@ impl AclTree {
         node.delete_user_role(auth_id, role);
     }
 
+    /// Inserts the specified `role` into the `group` ACL on `path`.
+    ///
+    /// The [AclTreeNode] representing `path` will be created and inserted into the tree if
+    /// necessary.
     pub fn insert_group_role(&mut self, path: &str, group: &str, role: &str, propagate: bool) {
         let path = split_acl_path(path);
         let node = self.get_or_insert_node(&path);
         node.insert_group_role(group.to_string(), role.to_string(), propagate);
     }
 
+    /// Inserts the specified `role` into the `user` ACL on `path`.
+    ///
+    /// The [AclTreeNode] representing `path` will be created and inserted into the tree if
+    /// necessary.
     pub fn insert_user_role(&mut self, path: &str, auth_id: &Authid, role: &str, propagate: bool) {
         let path = split_acl_path(path);
         let node = self.get_or_insert_node(&path);
@@ -498,7 +555,7 @@ impl AclTree {
         Ok(())
     }
 
-    pub fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> {
+    fn write_config(&self, w: &mut dyn Write) -> Result<(), Error> {
         Self::write_node_config(&self.root, "", w)
     }
 
@@ -547,7 +604,7 @@ impl AclTree {
         Ok(())
     }
 
-    pub fn load(filename: &Path) -> Result<(Self, [u8;32]), Error> {
+    fn load(filename: &Path) -> Result<(Self, [u8;32]), Error> {
         let mut tree = Self::new();
 
         let raw = match std::fs::read_to_string(filename) {
@@ -575,7 +632,8 @@ impl AclTree {
         Ok((tree, digest))
     }
 
-    pub fn from_raw(raw: &str) -> Result<Self, Error> {
+    #[cfg(test)]
+    pub(crate) fn from_raw(raw: &str) -> Result<Self, Error> {
         let mut tree = Self::new();
         for (linenr, line) in raw.lines().enumerate() {
             let line = line.trim();
@@ -587,6 +645,14 @@ impl AclTree {
         Ok(tree)
     }
 
+    /// Returns a map of role name and propagation status for a given `auth_id` and `path`.
+    ///
+    /// This will collect role mappings according to the following algorithm:
+    /// - iterate over all intermediate nodes along `path` and collect roles with `propagate` set
+    /// - get all (propagating and non-propagating) roles for last component of path
+    /// - more specific role maps replace less specific role maps
+    /// -- user/token is more specific than group at each level
+    /// -- roles lower in the tree are more specific than those higher up along the path
     pub fn roles(&self, auth_id: &Authid, path: &[&str]) -> HashMap<String, bool> {
 
         let mut node = &self.root;
@@ -610,14 +676,21 @@ impl AclTree {
     }
 }
 
+/// Filename where [AclTree] is stored.
 pub const ACL_CFG_FILENAME: &str = "/etc/proxmox-backup/acl.cfg";
+/// Path used to lock the [AclTree] when modifying.
 pub const ACL_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.acl.lck";
 
+/// Reads the [AclTree] from the [default path](ACL_CFG_FILENAME).
 pub fn config() -> Result<(AclTree, [u8; 32]), Error> {
     let path = PathBuf::from(ACL_CFG_FILENAME);
     AclTree::load(&path)
 }
 
+/// Returns a cached [AclTree] or fresh copy read directly from the [default path](ACL_CFG_FILENAME)
+///
+/// Since the AclTree is used for every API request's permission check, this caching mechanism
+/// allows to skip reading and parsing the file again if it is unchanged.
 pub fn cached_config() -> Result<Arc<AclTree>, Error> {
 
     struct ConfigCache {
@@ -663,6 +736,8 @@ pub fn cached_config() -> Result<Arc<AclTree>, Error> {
     Ok(config)
 }
 
+/// Saves an [AclTree] to the [default path](ACL_CFG_FILENAME), ensuring proper ownership and
+/// file permissions.
 pub fn save_config(acl: &AclTree) -> Result<(), Error> {
     let mut raw: Vec<u8> = Vec::new();
 
-- 
2.20.1

[pbs-devel] [RFC proxmox-backup 2/3] acl: reformat privileges

[Fabian Grünbichler]

for better readability, and tell rustfmt to leave those definitions
alone.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/config/acl.rs | 73 ++++++++++++++++++++++++++---------------------
 1 file changed, 41 insertions(+), 32 deletions(-)

diff --git a/src/config/acl.rs b/src/config/acl.rs
index 8503a2ab..04a01b51 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -74,56 +74,65 @@ pub const ROLE_ADMIN: u64 = std::u64::MAX;
 /// NoAccess can be used to remove privileges from specific (sub-)paths
 pub const ROLE_NO_ACCESS: u64 = 0;
 
+#[rustfmt::skip]
 /// Audit can view configuration and status information, but not modify it.
-pub const ROLE_AUDIT: u64 =
-PRIV_SYS_AUDIT |
-PRIV_DATASTORE_AUDIT;
+pub const ROLE_AUDIT: u64 = 0
+    | PRIV_SYS_AUDIT
+    | PRIV_DATASTORE_AUDIT;
 
+#[rustfmt::skip]
 /// Datastore.Admin can do anything on the datastore.
-pub const ROLE_DATASTORE_ADMIN: u64 =
-PRIV_DATASTORE_AUDIT |
-PRIV_DATASTORE_MODIFY |
-PRIV_DATASTORE_READ |
-PRIV_DATASTORE_VERIFY |
-PRIV_DATASTORE_BACKUP |
-PRIV_DATASTORE_PRUNE;
-
+pub const ROLE_DATASTORE_ADMIN: u64 = 0
+    | PRIV_DATASTORE_AUDIT
+    | PRIV_DATASTORE_MODIFY
+    | PRIV_DATASTORE_READ
+    | PRIV_DATASTORE_VERIFY
+    | PRIV_DATASTORE_BACKUP
+    | PRIV_DATASTORE_PRUNE;
+
+#[rustfmt::skip]
 /// Datastore.Reader can read/verify datastore content and do restore
-pub const ROLE_DATASTORE_READER: u64 =
-PRIV_DATASTORE_AUDIT |
-PRIV_DATASTORE_VERIFY |
-PRIV_DATASTORE_READ;
+pub const ROLE_DATASTORE_READER: u64 = 0
+    | PRIV_DATASTORE_AUDIT
+    | PRIV_DATASTORE_VERIFY
+    | PRIV_DATASTORE_READ;
 
+#[rustfmt::skip]
 /// Datastore.Backup can do backup and restore, but no prune.
-pub const ROLE_DATASTORE_BACKUP: u64 =
-PRIV_DATASTORE_BACKUP;
+pub const ROLE_DATASTORE_BACKUP: u64 = 0
+    | PRIV_DATASTORE_BACKUP;
 
+#[rustfmt::skip]
 /// Datastore.PowerUser can do backup, restore, and prune.
-pub const ROLE_DATASTORE_POWERUSER: u64 =
-PRIV_DATASTORE_PRUNE |
-PRIV_DATASTORE_BACKUP;
+pub const ROLE_DATASTORE_POWERUSER: u64 = 0
+    | PRIV_DATASTORE_PRUNE
+    | PRIV_DATASTORE_BACKUP;
 
+#[rustfmt::skip]
 /// Datastore.Audit can audit the datastore.
-pub const ROLE_DATASTORE_AUDIT: u64 =
-PRIV_DATASTORE_AUDIT;
+pub const ROLE_DATASTORE_AUDIT: u64 = 0
+    | PRIV_DATASTORE_AUDIT;
 
+#[rustfmt::skip]
 /// Remote.Audit can audit the remote
-pub const ROLE_REMOTE_AUDIT: u64 =
-PRIV_REMOTE_AUDIT;
+pub const ROLE_REMOTE_AUDIT: u64 = 0
+    | PRIV_REMOTE_AUDIT;
 
+#[rustfmt::skip]
 /// Remote.Admin can do anything on the remote.
-pub const ROLE_REMOTE_ADMIN: u64 =
-PRIV_REMOTE_AUDIT |
-PRIV_REMOTE_MODIFY |
-PRIV_REMOTE_READ;
+pub const ROLE_REMOTE_ADMIN: u64 = 0
+    | PRIV_REMOTE_AUDIT
+    | PRIV_REMOTE_MODIFY
+    | PRIV_REMOTE_READ;
 
+#[rustfmt::skip]
 /// Remote.SyncOperator can do read and prune on the remote.
-pub const ROLE_REMOTE_SYNC_OPERATOR: u64 =
-PRIV_REMOTE_AUDIT |
-PRIV_REMOTE_READ;
+pub const ROLE_REMOTE_SYNC_OPERATOR: u64 = 0
+    | PRIV_REMOTE_AUDIT
+    | PRIV_REMOTE_READ;
 
 /// NoAccess can be used to remove privileges from specific (sub-)paths
-pub const ROLE_NAME_NO_ACCESS: &str ="NoAccess";
+pub const ROLE_NAME_NO_ACCESS: &str = "NoAccess";
 
 #[api()]
 #[repr(u64)]
-- 
2.20.1

[pbs-devel] [RFC proxmox-backup 3/3] acl: rustfmt module

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/config/acl.rs | 243 ++++++++++++++++++++++++++++++----------------
 1 file changed, 157 insertions(+), 86 deletions(-)

diff --git a/src/config/acl.rs b/src/config/acl.rs
index 04a01b51..3ad17eb5 100644
--- a/src/config/acl.rs
+++ b/src/config/acl.rs
@@ -1,8 +1,8 @@
+use std::collections::{BTreeMap, BTreeSet, HashMap};
 use std::io::Write;
-use std::collections::{HashMap, BTreeMap, BTreeSet};
-use std::path::{PathBuf, Path};
-use std::sync::{Arc, RwLock};
+use std::path::{Path, PathBuf};
 use std::str::FromStr;
+use std::sync::{Arc, RwLock};
 
 use anyhow::{bail, Error};
 
@@ -11,11 +11,11 @@ use lazy_static::lazy_static;
 use ::serde::{Deserialize, Serialize};
 use serde::de::{value, IntoDeserializer};
 
-use proxmox::tools::{fs::replace_file, fs::CreateOptions};
-use proxmox::constnamedbitmap;
 use proxmox::api::{api, schema::*};
+use proxmox::constnamedbitmap;
+use proxmox::tools::{fs::replace_file, fs::CreateOptions};
 
-use crate::api2::types::{Authid,Userid};
+use crate::api2::types::{Authid, Userid};
 
 // define Privilege bitfield
 
@@ -66,7 +66,6 @@ constnamedbitmap! {
     }
 }
 
-
 /// Admin always has all privileges. It can do everything except a few actions
 /// which are limited to the 'root@pam` superuser
 pub const ROLE_ADMIN: u64 = std::u64::MAX;
@@ -200,7 +199,9 @@ pub(crate) fn split_acl_path(path: &str) -> Vec<&str> {
     let mut components = vec![];
 
     for name in items {
-        if name.is_empty() { continue; }
+        if name.is_empty() {
+            continue;
+        }
         components.push(name);
     }
 
@@ -211,45 +212,70 @@ pub(crate) fn split_acl_path(path: &str) -> Vec<&str> {
 ///
 /// Currently this just checks for the number of components for various sub-trees.
 pub fn check_acl_path(path: &str) -> Result<(), Error> {
-
     let components = split_acl_path(path);
 
     let components_len = components.len();
 
-    if components_len == 0 { return Ok(()); }
+    if components_len == 0 {
+        return Ok(());
+    }
     match components[0] {
         "access" => {
-            if components_len == 1 { return Ok(()); }
+            if components_len == 1 {
+                return Ok(());
+            }
             match components[1] {
                 "acl" | "users" => {
-                    if components_len == 2 { return Ok(()); }
+                    if components_len == 2 {
+                        return Ok(());
+                    }
                 }
-                _ => {},
+                _ => {}
             }
         }
-        "datastore" => {  // /datastore/{store}
-            if components_len <= 2 { return Ok(()); }
+        "datastore" => {
+            // /datastore/{store}
+            if components_len <= 2 {
+                return Ok(());
+            }
         }
-        "remote" => { // /remote/{remote}/{store}
-            if components_len <= 3 { return Ok(()); }
+        "remote" => {
+            // /remote/{remote}/{store}
+            if components_len <= 3 {
+                return Ok(());
+            }
         }
         "system" => {
-            if components_len == 1 { return Ok(()); }
+            if components_len == 1 {
+                return Ok(());
+            }
             match components[1] {
                 "disks" | "log" | "status" | "tasks" | "time" => {
-                    if components_len == 2 { return Ok(()); }
+                    if components_len == 2 {
+                        return Ok(());
+                    }
                 }
-                "services" => { // /system/services/{service}
-                    if components_len <= 3 { return Ok(()); }
+                "services" => {
+                    // /system/services/{service}
+                    if components_len <= 3 {
+                        return Ok(());
+                    }
                 }
                 "network" => {
-                    if components_len == 2 { return Ok(()); }
+                    if components_len == 2 {
+                        return Ok(());
+                    }
                     match components[2] {
                         "dns" => {
-                            if components_len == 3 { return Ok(()); }
+                            if components_len == 3 {
+                                return Ok(());
+                            }
                         }
-                        "interfaces" => { // /system/network/interfaces/{iface}
-                            if components_len <= 4 { return Ok(()); }
+                        "interfaces" => {
+                            // /system/network/interfaces/{iface}
+                            if components_len <= 4 {
+                                return Ok(());
+                            }
                         }
                         _ => {}
                     }
@@ -284,7 +310,6 @@ pub struct AclTreeNode {
 }
 
 impl AclTreeNode {
-
     /// Creates a new, empty AclTreeNode.
     pub fn new() -> Self {
         Self {
@@ -306,14 +331,13 @@ impl AclTreeNode {
         let user_roles = self.extract_user_roles(auth_id, leaf);
         if !user_roles.is_empty() || auth_id.is_token() {
             // user privs always override group privs
-            return user_roles
+            return user_roles;
         };
 
         self.extract_group_roles(auth_id.user(), leaf)
     }
 
     fn extract_user_roles(&self, auth_id: &Authid, leaf: bool) -> HashMap<String, bool> {
-
         let mut map = HashMap::new();
 
         let roles = match self.users.get(auth_id) {
@@ -337,12 +361,13 @@ impl AclTreeNode {
     }
 
     fn extract_group_roles(&self, _user: &Userid, leaf: bool) -> HashMap<String, bool> {
-
         let mut map = HashMap::new();
 
         for (_group, roles) in &self.groups {
             let is_member = false; // fixme: check if user is member of the group
-            if !is_member { continue; }
+            if !is_member {
+                continue;
+            }
 
             for (role, propagate) in roles {
                 if *propagate || leaf {
@@ -400,7 +425,6 @@ impl AclTreeNode {
 }
 
 impl AclTree {
-
     /// Create a new, empty ACL tree with a single, empty root [node](AclTreeNode)
     pub fn new() -> Self {
         Self {
@@ -428,7 +452,9 @@ impl AclTree {
     fn get_or_insert_node(&mut self, path: &[&str]) -> &mut AclTreeNode {
         let mut node = &mut self.root;
         for comp in path {
-            node = node.children.entry(String::from(*comp))
+            node = node
+                .children
+                .entry(String::from(*comp))
                 .or_insert_with(|| AclTreeNode::new());
         }
         node
@@ -480,26 +506,27 @@ impl AclTree {
         node.insert_user_role(auth_id.to_owned(), role.to_string(), propagate);
     }
 
-    fn write_node_config(
-        node: &AclTreeNode,
-        path: &str,
-        w: &mut dyn Write,
-    ) -> Result<(), Error> {
-
+    fn write_node_config(node: &AclTreeNode, path: &str, w: &mut dyn Write) -> Result<(), Error> {
         let mut role_ug_map0 = HashMap::new();
         let mut role_ug_map1 = HashMap::new();
 
         for (auth_id, roles) in &node.users {
             // no need to save, because root is always 'Administrator'
-            if !auth_id.is_token() && auth_id.user() == "root@pam" { continue; }
+            if !auth_id.is_token() && auth_id.user() == "root@pam" {
+                continue;
+            }
             for (role, propagate) in roles {
                 let role = role.as_str();
                 let auth_id = auth_id.to_string();
                 if *propagate {
-                    role_ug_map1.entry(role).or_insert_with(|| BTreeSet::new())
+                    role_ug_map1
+                        .entry(role)
+                        .or_insert_with(|| BTreeSet::new())
                         .insert(auth_id);
                 } else {
-                    role_ug_map0.entry(role).or_insert_with(|| BTreeSet::new())
+                    role_ug_map0
+                        .entry(role)
+                        .or_insert_with(|| BTreeSet::new())
                         .insert(auth_id);
                 }
             }
@@ -509,10 +536,14 @@ impl AclTree {
             for (role, propagate) in roles {
                 let group = format!("@{}", group);
                 if *propagate {
-                    role_ug_map1.entry(role).or_insert_with(|| BTreeSet::new())
+                    role_ug_map1
+                        .entry(role)
+                        .or_insert_with(|| BTreeSet::new())
                         .insert(group);
                 } else {
-                    role_ug_map0.entry(role).or_insert_with(|| BTreeSet::new())
+                    role_ug_map0
+                        .entry(role)
+                        .or_insert_with(|| BTreeSet::new())
                         .insert(group);
                 }
             }
@@ -524,11 +555,15 @@ impl AclTree {
             let mut result_map = BTreeMap::new();
             for (item, property_map) in item_property_map {
                 let item_list = property_map.iter().fold(String::new(), |mut acc, v| {
-                    if !acc.is_empty() { acc.push(','); }
+                    if !acc.is_empty() {
+                        acc.push(',');
+                    }
                     acc.push_str(v);
                     acc
                 });
-                result_map.entry(item_list).or_insert_with(|| BTreeSet::new())
+                result_map
+                    .entry(item_list)
+                    .or_insert_with(|| BTreeSet::new())
                     .insert(item.to_string());
             }
             result_map
@@ -538,9 +573,13 @@ impl AclTree {
         let uglist_role_map1 = group_by_property_list(&role_ug_map1);
 
         fn role_list(roles: &BTreeSet<String>) -> String {
-            if roles.contains(ROLE_NAME_NO_ACCESS) { return String::from(ROLE_NAME_NO_ACCESS); }
+            if roles.contains(ROLE_NAME_NO_ACCESS) {
+                return String::from(ROLE_NAME_NO_ACCESS);
+            }
             roles.iter().fold(String::new(), |mut acc, v| {
-                if !acc.is_empty() { acc.push(','); }
+                if !acc.is_empty() {
+                    acc.push(',');
+                }
                 acc.push_str(v);
                 acc
             })
@@ -548,12 +587,24 @@ impl AclTree {
 
         for (uglist, roles) in &uglist_role_map0 {
             let role_list = role_list(roles);
-            writeln!(w, "acl:0:{}:{}:{}", if path.is_empty() { "/" } else { path }, uglist, role_list)?;
+            writeln!(
+                w,
+                "acl:0:{}:{}:{}",
+                if path.is_empty() { "/" } else { path },
+                uglist,
+                role_list
+            )?;
         }
 
         for (uglist, roles) in &uglist_role_map1 {
             let role_list = role_list(roles);
-            writeln!(w, "acl:1:{}:{}:{}", if path.is_empty() { "/" } else { path }, uglist, role_list)?;
+            writeln!(
+                w,
+                "acl:1:{}:{}:{}",
+                if path.is_empty() { "/" } else { path },
+                uglist,
+                role_list
+            )?;
         }
 
         for (name, child) in node.children.iter() {
@@ -569,7 +620,6 @@ impl AclTree {
     }
 
     fn parse_acl_line(&mut self, line: &str) -> Result<(), Error> {
-
         let items: Vec<&str> = line.split(':').collect();
 
         if items.len() != 5 {
@@ -613,7 +663,7 @@ impl AclTree {
         Ok(())
     }
 
-    fn load(filename: &Path) -> Result<(Self, [u8;32]), Error> {
+    fn load(filename: &Path) -> Result<(Self, [u8; 32]), Error> {
         let mut tree = Self::new();
 
         let raw = match std::fs::read_to_string(filename) {
@@ -631,10 +681,16 @@ impl AclTree {
 
         for (linenr, line) in raw.lines().enumerate() {
             let line = line.trim();
-            if line.is_empty() { continue; }
+            if line.is_empty() {
+                continue;
+            }
             if let Err(err) = tree.parse_acl_line(line) {
-                bail!("unable to parse acl config {:?}, line {} - {}",
-                      filename, linenr+1, err);
+                bail!(
+                    "unable to parse acl config {:?}, line {} - {}",
+                    filename,
+                    linenr + 1,
+                    err
+                );
             }
         }
 
@@ -646,9 +702,15 @@ impl AclTree {
         let mut tree = Self::new();
         for (linenr, line) in raw.lines().enumerate() {
             let line = line.trim();
-            if line.is_empty() { continue; }
+            if line.is_empty() {
+                continue;
+            }
             if let Err(err) = tree.parse_acl_line(line) {
-                bail!("unable to parse acl config data, line {} - {}", linenr+1, err);
+                bail!(
+                    "unable to parse acl config data, line {} - {}",
+                    linenr + 1,
+                    err
+                );
             }
         }
         Ok(tree)
@@ -663,7 +725,6 @@ impl AclTree {
     /// -- user/token is more specific than group at each level
     /// -- roles lower in the tree are more specific than those higher up along the path
     pub fn roles(&self, auth_id: &Authid, path: &[&str]) -> HashMap<String, bool> {
-
         let mut node = &self.root;
         let mut role_map = node.extract_roles(auth_id, path.is_empty());
 
@@ -701,7 +762,6 @@ pub fn config() -> Result<(AclTree, [u8; 32]), Error> {
 /// Since the AclTree is used for every API request's permission check, this caching mechanism
 /// allows to skip reading and parsing the file again if it is unchanged.
 pub fn cached_config() -> Result<Arc<AclTree>, Error> {
-
     struct ConfigCache {
         data: Option<Arc<AclTree>>,
         last_mtime: i64,
@@ -709,8 +769,11 @@ pub fn cached_config() -> Result<Arc<AclTree>, Error> {
     }
 
     lazy_static! {
-        static ref CACHED_CONFIG: RwLock<ConfigCache> = RwLock::new(
-            ConfigCache { data: None, last_mtime: 0, last_mtime_nsec: 0 });
+        static ref CACHED_CONFIG: RwLock<ConfigCache> = RwLock::new(ConfigCache {
+            data: None,
+            last_mtime: 0,
+            last_mtime_nsec: 0
+        });
     }
 
     let stat = match nix::sys::stat::stat(ACL_CFG_FILENAME) {
@@ -719,11 +782,13 @@ pub fn cached_config() -> Result<Arc<AclTree>, Error> {
         Err(err) => bail!("unable to stat '{}' - {}", ACL_CFG_FILENAME, err),
     };
 
-    { // limit scope
+    {
+        // limit scope
         let cache = CACHED_CONFIG.read().unwrap();
         if let Some(ref config) = cache.data {
             if let Some(stat) = stat {
-                if stat.st_mtime == cache.last_mtime && stat.st_mtime_nsec == cache.last_mtime_nsec {
+                if stat.st_mtime == cache.last_mtime && stat.st_mtime_nsec == cache.last_mtime_nsec
+                {
                     return Ok(config.clone());
                 }
             } else if cache.last_mtime == 0 && cache.last_mtime_nsec == 0 {
@@ -768,30 +833,30 @@ pub fn save_config(acl: &AclTree) -> Result<(), Error> {
 
 #[cfg(test)]
 mod test {
-    use anyhow::{Error};
     use super::AclTree;
+    use anyhow::Error;
 
     use crate::api2::types::Authid;
 
-    fn check_roles(
-        tree: &AclTree,
-        auth_id: &Authid,
-        path: &str,
-        expected_roles: &str,
-    ) {
-
+    fn check_roles(tree: &AclTree, auth_id: &Authid, path: &str, expected_roles: &str) {
         let path_vec = super::split_acl_path(path);
-        let mut roles = tree.roles(auth_id, &path_vec)
-            .iter().map(|(v, _)| v.clone()).collect::<Vec<String>>();
+        let mut roles = tree
+            .roles(auth_id, &path_vec)
+            .iter()
+            .map(|(v, _)| v.clone())
+            .collect::<Vec<String>>();
         roles.sort();
         let roles = roles.join(",");
 
-        assert_eq!(roles, expected_roles, "\nat check_roles for '{}' on '{}'", auth_id, path);
+        assert_eq!(
+            roles, expected_roles,
+            "\nat check_roles for '{}' on '{}'",
+            auth_id, path
+        );
     }
 
     #[test]
     fn test_acl_line_compression() {
-
         let tree = AclTree::from_raw(
             "\
             acl:0:/store/store2:user1@pbs:Admin\n\
@@ -803,20 +868,25 @@ mod test {
         .expect("failed to parse acl tree");
 
         let mut raw: Vec<u8> = Vec::new();
-        tree.write_config(&mut raw).expect("failed to write acl tree");
+        tree.write_config(&mut raw)
+            .expect("failed to write acl tree");
         let raw = std::str::from_utf8(&raw).expect("acl tree is not valid utf8");
 
-        assert_eq!(raw, "acl:0:/store/store2:user1@pbs,user2@pbs:Admin,DatastoreBackup\n");
+        assert_eq!(
+            raw,
+            "acl:0:/store/store2:user1@pbs,user2@pbs:Admin,DatastoreBackup\n"
+        );
     }
 
     #[test]
     fn test_roles_1() -> Result<(), Error> {
-
-        let tree = AclTree::from_raw(r###"
+        let tree = AclTree::from_raw(
+            r###"
 acl:1:/storage:user1@pbs:Admin
 acl:1:/storage/store1:user1@pbs:DatastoreBackup
 acl:1:/storage/store2:user2@pbs:DatastoreBackup
-"###)?;
+"###,
+        )?;
         let user1: Authid = "user1@pbs".parse()?;
         check_roles(&tree, &user1, "/", "");
         check_roles(&tree, &user1, "/storage", "Admin");
@@ -834,12 +904,13 @@ acl:1:/storage/store2:user2@pbs:DatastoreBackup
 
     #[test]
     fn test_role_no_access() -> Result<(), Error> {
-
-        let tree = AclTree::from_raw(r###"
+        let tree = AclTree::from_raw(
+            r###"
 acl:1:/:user1@pbs:Admin
 acl:1:/storage:user1@pbs:NoAccess
 acl:1:/storage/store1:user1@pbs:DatastoreBackup
-"###)?;
+"###,
+        )?;
         let user1: Authid = "user1@pbs".parse()?;
         check_roles(&tree, &user1, "/", "Admin");
         check_roles(&tree, &user1, "/storage", "NoAccess");
@@ -847,11 +918,13 @@ acl:1:/storage/store1:user1@pbs:DatastoreBackup
         check_roles(&tree, &user1, "/storage/store2", "NoAccess");
         check_roles(&tree, &user1, "/system", "Admin");
 
-        let tree = AclTree::from_raw(r###"
+        let tree = AclTree::from_raw(
+            r###"
 acl:1:/:user1@pbs:Admin
 acl:0:/storage:user1@pbs:NoAccess
 acl:1:/storage/store1:user1@pbs:DatastoreBackup
-"###)?;
+"###,
+        )?;
         check_roles(&tree, &user1, "/", "Admin");
         check_roles(&tree, &user1, "/storage", "NoAccess");
         check_roles(&tree, &user1, "/storage/store1", "DatastoreBackup");
@@ -863,7 +936,6 @@ acl:1:/storage/store1:user1@pbs:DatastoreBackup
 
     #[test]
     fn test_role_add_delete() -> Result<(), Error> {
-
         let mut tree = AclTree::new();
 
         let user1: Authid = "user1@pbs".parse()?;
@@ -887,7 +959,6 @@ acl:1:/storage/store1:user1@pbs:DatastoreBackup
 
     #[test]
     fn test_no_access_overwrite() -> Result<(), Error> {
-
         let mut tree = AclTree::new();
 
         let user1: Authid = "user1@pbs".parse()?;
-- 
2.20.1

Re: [pbs-devel] [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

[Thomas Lamprecht]

On 17/12/2020 15:27, Fabian Grünbichler wrote:
> document all public things, add some doc links and make some
> previously-public things only available for test cases or within the
> crate:
> 
> previously public, now private:
> - AclTreeNode::extract_user_roles (we have extract_roles())
> - AclTreeNode::extract_group_roles (same)
> - AclTreeNode::delete_group_role (exists on AclTree)
> - AclTreeNode::delete_user_role (same)
> - AclTreeNode::insert_group_role (same)
> - AclTreeNode::insert_user_role (same)
> - AclTree::write_config (we have save_config())
> - AclTree::load (we have config()/cached_config())
> 
> previously public, now crate-internal:
> - AclTree::from_raw (only used by tests)
> - split_acl_path (used by some test binaries)
> 

thanks, thats makes it cleaner

> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> RFC to get some feedback on which rustdoc features we actually want to
> use here, and which we want to leave out for now.
> 

Anything specific you want comments for, I mean I only skimmed through it quickly 
so may have overlooked stuff, but it seems like its mostly links which are new
in usage?

As long its a stable supported rustdoc feature available by the rust version
we use I'm all for being able to use it.

[pbs-devel] applied: [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

[Dietmar Maurer]

applied all 3 patches

Re: [pbs-devel] [RFC proxmox-backup 1/3] acl: add docs and adapt visibility

[Fabian Grünbichler]

On December 17, 2020 5:12 pm, Thomas Lamprecht wrote:
> On 17/12/2020 15:27, Fabian Grünbichler wrote:
>> document all public things, add some doc links and make some
>> previously-public things only available for test cases or within the
>> crate:
>> 
>> previously public, now private:
>> - AclTreeNode::extract_user_roles (we have extract_roles())
>> - AclTreeNode::extract_group_roles (same)
>> - AclTreeNode::delete_group_role (exists on AclTree)
>> - AclTreeNode::delete_user_role (same)
>> - AclTreeNode::insert_group_role (same)
>> - AclTreeNode::insert_user_role (same)
>> - AclTree::write_config (we have save_config())
>> - AclTree::load (we have config()/cached_config())
>> 
>> previously public, now crate-internal:
>> - AclTree::from_raw (only used by tests)
>> - split_acl_path (used by some test binaries)
>> 
> 
> thanks, thats makes it cleaner
> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>> RFC to get some feedback on which rustdoc features we actually want to
>> use here, and which we want to leave out for now.
>> 
> 
> Anything specific you want comments for, I mean I only skimmed through it quickly 
> so may have overlooked stuff, but it seems like its mostly links which are new
> in usage?
> 
> As long its a stable supported rustdoc feature available by the rust version
> we use I'm all for being able to use it.
> 

yes, mainly the linking (rust 1.48 now supports proper intra-crate 
linking without having to guess the HTML structure it creates, so this 
should work nicely now).

my main concern were the following (sorry for not taking the time to 
write them up):

nice to read links in the code (i.e., as plain as possible) don't look 
nice in the generated docs, and vice-versa => which part gets priority?

e.g., the following:

    /// Returns applicable [Role] and their propagation status for a given
    /// [Authid](crate::api2::types::Authid).
    ///
    /// If the `Authid` is a [User](crate::config::user::User) that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.

is not nice to parse while reading the code, but it will read much 
better in the generated HTML than

    /// Returns applicable [Role] and their propagation status for a given
    /// [crate::api2::types::Authid]
    ///
    /// If the `Authid` is a [crate::config::user::User] that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.

does. according to the docs the following should also work, and might 
actually be the nicest variant:

    /// Returns applicable [Role] and their propagation status for a given
    /// [Authid].
    ///
    /// If the `Authid` is a [User] that has no specific `Roles` configured on this node,
    /// applicable `Group` roles will be returned instead.
    ///
    /// [Authid]: crate::api2::types::Authid
    /// [User]: crate::config::user::User

a second point was that for structs we have to watch out that not too 
much rustdoc special syntax slips into the generated API schema 
descriptions (thankfully, examples are usually limited to functions). we 
could try some heuristics to run on our api dump (e.g., complaining if 
we add special syntax compared to the last tracked api dump) to catch 
that, since we just need to override the #[api] description which takes 
precedence over the auto-generated one to fix it. or, we could try to 
automagically strip some stuff when using the auto-generated one.