* [pdm-devel] [PATCH proxmox 1/4] access-control: add more types to prepare for api feature
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
@ 2025-04-03 14:17 ` Shannon Sterz
2025-04-03 14:17 ` [pdm-devel] [PATCH proxmox 2/4] access-control: add acl " Shannon Sterz
` (8 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:17 UTC (permalink / raw)
To: pdm-devel
this includes:
- `ACL_PATH_SCHEMA`: describes the format of valid acl paths
- `ACL_PROPAGATE_SCHEMA`: describes whether an acl entry propagates to
its child paths
- `AclUgidType`: which type an acl entry refers to, either a user or a
group
- `AclListItem`: describes an entry of the ACL
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
proxmox-access-control/Cargo.toml | 3 ++
proxmox-access-control/src/types.rs | 59 ++++++++++++++++++++++++++++-
2 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/proxmox-access-control/Cargo.toml b/proxmox-access-control/Cargo.toml
index 9c355344..23be7fcb 100644
--- a/proxmox-access-control/Cargo.toml
+++ b/proxmox-access-control/Cargo.toml
@@ -13,10 +13,13 @@ rust-version.workspace = true
[dependencies]
anyhow.workspace = true
+const_format.workspace = true
nix = { workspace = true, optional = true }
openssl = { workspace = true, optional = true }
+regex.workspace = true
serde.workspace = true
serde_json = { workspace = true, optional = true }
+serde_plain.workspace = true
proxmox-auth-api = { workspace = true, features = [ "api-types" ] }
proxmox-config-digest = { workspace = true, optional = true, features = [ "openssl" ] }
diff --git a/proxmox-access-control/src/types.rs b/proxmox-access-control/src/types.rs
index ae6de7cf..01d078de 100644
--- a/proxmox-access-control/src/types.rs
+++ b/proxmox-access-control/src/types.rs
@@ -1,10 +1,12 @@
use serde::{Deserialize, Serialize};
+use const_format::concatcp;
+
use proxmox_auth_api::types::{Authid, Userid, PROXMOX_TOKEN_ID_SCHEMA};
use proxmox_schema::{
api,
- api_types::{COMMENT_SCHEMA, SINGLE_LINE_COMMENT_FORMAT},
- BooleanSchema, IntegerSchema, Schema, StringSchema, Updater,
+ api_types::{COMMENT_SCHEMA, SAFE_ID_REGEX_STR, SINGLE_LINE_COMMENT_FORMAT},
+ const_regex, ApiStringFormat, BooleanSchema, IntegerSchema, Schema, StringSchema, Updater,
};
pub const ENABLE_USER_SCHEMA: Schema = BooleanSchema::new(
@@ -38,6 +40,23 @@ pub const EMAIL_SCHEMA: Schema = StringSchema::new("E-Mail Address.")
.max_length(64)
.schema();
+const_regex! {
+ pub ACL_PATH_REGEX = concatcp!(r"^(?:/|", r"(?:/", SAFE_ID_REGEX_STR, ")+", r")$");
+}
+
+pub const ACL_PATH_FORMAT: ApiStringFormat = ApiStringFormat::Pattern(&ACL_PATH_REGEX);
+
+pub const ACL_PATH_SCHEMA: Schema = StringSchema::new("Access control path.")
+ .format(&ACL_PATH_FORMAT)
+ .min_length(1)
+ .max_length(128)
+ .schema();
+
+pub const ACL_PROPAGATE_SCHEMA: Schema =
+ BooleanSchema::new("Allow to propagate (inherit) permissions.")
+ .default(true)
+ .schema();
+
#[api(
properties: {
user: {
@@ -192,3 +211,39 @@ impl User {
true
}
}
+
+#[api]
+/// Type of the 'ugid' property in the ACL entry list.
+#[derive(Clone, Copy, Debug, Eq, PartialEq, Deserialize, Serialize, Hash)]
+#[serde(rename_all = "lowercase")]
+pub enum AclUgidType {
+ /// An entry for a user (or token).
+ User,
+ /// An entry for a group.
+ Group,
+}
+
+serde_plain::derive_display_from_serialize!(AclUgidType);
+serde_plain::derive_fromstr_from_deserialize!(AclUgidType);
+
+#[api(
+ properties: {
+ propagate: { schema: ACL_PROPAGATE_SCHEMA, },
+ path: { schema: ACL_PATH_SCHEMA, },
+ ugid_type: { type: AclUgidType },
+ ugid: {
+ type: String,
+ description: "User or Group ID.",
+ },
+ }
+)]
+#[derive(Serialize, Deserialize, PartialEq, Clone, Hash)]
+/// Access control list entry.
+pub struct AclListItem {
+ pub path: String,
+ pub ugid: String,
+ pub ugid_type: AclUgidType,
+ pub propagate: bool,
+ /// A role represented as a string.
+ pub roleid: String,
+}
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
2025-04-03 14:17 ` [pdm-devel] [PATCH proxmox 1/4] access-control: add more types to prepare for api feature Shannon Sterz
@ 2025-04-03 14:17 ` Shannon Sterz
2025-04-09 11:01 ` Dietmar Maurer
2025-04-03 14:18 ` [pdm-devel] [PATCH proxmox 3/4] access-control: add comments to roles function of AccessControlConfig Shannon Sterz
` (7 subsequent siblings)
9 siblings, 1 reply; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:17 UTC (permalink / raw)
To: pdm-devel
this moves this commonly re-implemented api endpoints to this shared
crate so they can be easily re-used.
for this to function a user of the crate needs to extend the
`AccessControlConfig` with the following three functions:
- `acl_audit_privilege()`: returns the privilege necessary to see all
acl entries
- `acl_modify_privilege()`: returns the privilege necessary to edit
the acl beyond a user's api token privileges
- `check_acl_path()`: checks whether a path is a valid acl path in the
context of the product using this crate. by default all paths are
considered valid.
all three provide default implementations so that users that only use
the `impl` feature don't need to change anything.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
proxmox-access-control/Cargo.toml | 5 +
proxmox-access-control/src/api.rs | 278 +++++++++++++++++++++++++++++
proxmox-access-control/src/init.rs | 22 +++
proxmox-access-control/src/lib.rs | 3 +
4 files changed, 308 insertions(+)
create mode 100644 proxmox-access-control/src/api.rs
diff --git a/proxmox-access-control/Cargo.toml b/proxmox-access-control/Cargo.toml
index 23be7fcb..03f5c9fd 100644
--- a/proxmox-access-control/Cargo.toml
+++ b/proxmox-access-control/Cargo.toml
@@ -17,6 +17,7 @@ const_format.workspace = true
nix = { workspace = true, optional = true }
openssl = { workspace = true, optional = true }
regex.workspace = true
+hex = { workspace = true, optional = true }
serde.workspace = true
serde_json = { workspace = true, optional = true }
serde_plain.workspace = true
@@ -33,6 +34,10 @@ proxmox-time = { workspace = true }
[features]
default = []
+api = [
+ "impl",
+ "dep:hex",
+]
impl = [
"dep:nix",
"dep:openssl",
diff --git a/proxmox-access-control/src/api.rs b/proxmox-access-control/src/api.rs
new file mode 100644
index 00000000..4a6aabf5
--- /dev/null
+++ b/proxmox-access-control/src/api.rs
@@ -0,0 +1,278 @@
+use anyhow::{bail, format_err, Error};
+
+use proxmox_auth_api::types::{Authid, PROXMOX_GROUP_ID_SCHEMA};
+use proxmox_config_digest::{ConfigDigest, PROXMOX_CONFIG_DIGEST_SCHEMA};
+use proxmox_router::{Permission, Router, RpcEnvironment};
+use proxmox_schema::api;
+
+use crate::acl::AclTreeNode;
+use crate::init::access_conf;
+use crate::types::{AclListItem, AclUgidType, ACL_PATH_SCHEMA, ACL_PROPAGATE_SCHEMA};
+use crate::CachedUserInfo;
+
+#[api(
+ input: {
+ properties: {
+ path: {
+ schema: ACL_PATH_SCHEMA,
+ optional: true,
+ },
+ exact: {
+ description: "If set, returns only ACL for the exact path.",
+ type: bool,
+ optional: true,
+ default: false,
+ },
+ },
+ },
+ returns: {
+ description: "ACL entry list.",
+ type: Array,
+ items: {
+ type: AclListItem,
+ }
+ },
+ access: {
+ permission: &Permission::Anybody,
+ description: "Returns all ACLs if user has sufficient privileges on this endpoint, otherwise it is limited to the user's API tokens.",
+ },
+)]
+/// Get ACL entries, can be filter by path.
+pub fn read_acl(
+ path: Option<String>,
+ exact: bool,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Vec<AclListItem>, Error> {
+ let auth_id = rpcenv
+ .get_auth_id()
+ .ok_or_else(|| format_err!("endpoint called without an auth id"))?
+ .parse()?;
+
+ let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
+
+ let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
+ Some(auth_id)
+ } else {
+ None
+ };
+
+ let (mut tree, digest) = crate::acl::config()?;
+
+ let node = if let Some(path) = &path {
+ if let Some(node) = tree.find_node(path) {
+ node
+ } else {
+ return Ok(Vec::new());
+ }
+ } else {
+ &tree.root
+ };
+
+ rpcenv["digest"] = hex::encode(digest).into();
+
+ Ok(extract_acl_node_data(node, path.as_deref(), exact, &filter))
+}
+
+#[api(
+ protected: true,
+ input: {
+ properties: {
+ path: {
+ schema: ACL_PATH_SCHEMA,
+ },
+ role: {
+ type: String,
+ description: "Name of a role that the auth id will be granted.",
+ },
+ propagate: {
+ optional: true,
+ schema: ACL_PROPAGATE_SCHEMA,
+ },
+ "auth-id": {
+ optional: true,
+ type: Authid,
+ },
+ group: {
+ optional: true,
+ schema: PROXMOX_GROUP_ID_SCHEMA,
+ },
+ delete: {
+ optional: true,
+ description: "Remove permissions (instead of adding it).",
+ type: bool,
+ default: false,
+ },
+ digest: {
+ optional: true,
+ schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
+ },
+ },
+ },
+ access: {
+ permission: &Permission::Anybody,
+ description: "Requires sufficient permissions to edit the ACL, otherwise only editing the current user's API token permissions is allowed."
+ },
+)]
+/// Update ACL
+#[allow(clippy::too_many_arguments)]
+pub fn update_acl(
+ path: String,
+ role: String,
+ propagate: Option<bool>,
+ auth_id: Option<Authid>,
+ group: Option<String>,
+ delete: bool,
+ digest: Option<ConfigDigest>,
+ rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+ let access_conf = access_conf();
+
+ if !access_conf.roles().contains_key(role.as_str()) {
+ bail!("Role does not exist, please make sure to specify a valid role!")
+ }
+
+ let current_auth_id: Authid = rpcenv
+ .get_auth_id()
+ .expect("auth id could not be determined")
+ .parse()?;
+
+ let user_info = CachedUserInfo::new()?;
+ let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
+
+ if top_level_privs & access_conf.acl_modify_privileges() == 0 {
+ if group.is_some() {
+ bail!("Unprivileged users are not allowed to create group ACL item.");
+ }
+
+ match &auth_id {
+ Some(auth_id) => {
+ if current_auth_id.is_token() {
+ bail!("Unprivileged API tokens can't set ACL items.");
+ } else if !auth_id.is_token() {
+ bail!("Unprivileged users can only set ACL items for API tokens.");
+ } else if auth_id.user() != current_auth_id.user() {
+ bail!("Unprivileged users can only set ACL items for their own API tokens.");
+ }
+ }
+ None => {
+ bail!("Unprivileged user needs to provide auth_id to update ACL item.");
+ }
+ };
+ }
+
+ // FIXME: add support for group
+ if group.is_some() {
+ bail!("parameter 'group' - groups are currently not supported");
+ } else if let Some(auth_id) = &auth_id {
+ // only allow deleting non-existing auth id's, not adding them
+ if !delete {
+ let exists = crate::user::cached_config()?
+ .sections
+ .contains_key(&auth_id.to_string());
+
+ if !exists {
+ if auth_id.is_token() {
+ bail!("no such API token");
+ } else {
+ bail!("no such user.")
+ }
+ }
+ }
+ } else {
+ // FIXME: suggest groups here once they exist
+ bail!("missing 'userid' parameter");
+ }
+
+ // allow deleting invalid acl paths
+ if !delete {
+ access_conf.check_acl_path(&path)?;
+ }
+
+ let _guard = crate::acl::lock_config()?;
+ let (mut tree, expected_digest) = crate::acl::config()?;
+ expected_digest.detect_modification(digest.as_ref())?;
+
+ let propagate = propagate.unwrap_or(true);
+
+ if let Some(auth_id) = &auth_id {
+ if delete {
+ tree.delete_user_role(&path, auth_id, &role);
+ } else {
+ tree.insert_user_role(&path, auth_id, &role, propagate);
+ }
+ } else if let Some(group) = &group {
+ if delete {
+ tree.delete_group_role(&path, group, &role);
+ } else {
+ tree.insert_group_role(&path, group, &role, propagate);
+ }
+ }
+
+ crate::acl::save_config(&tree)?;
+
+ Ok(())
+}
+
+fn extract_acl_node_data(
+ node: &AclTreeNode,
+ path: Option<&str>,
+ exact: bool,
+ auth_id_filter: &Option<Authid>,
+) -> Vec<AclListItem> {
+ // tokens can't have tokens, so we can early return
+ if let Some(auth_id_filter) = auth_id_filter {
+ if auth_id_filter.is_token() {
+ return Vec::new();
+ }
+ }
+
+ let mut list = Vec::new();
+ let path_str = path.unwrap_or("/");
+
+ for (user, roles) in &node.users {
+ if let Some(auth_id_filter) = auth_id_filter {
+ if !user.is_token() || user.user() != auth_id_filter.user() {
+ continue;
+ }
+ }
+
+ for (role, propagate) in roles {
+ list.push(AclListItem {
+ path: path_str.to_owned(),
+ propagate: *propagate,
+ ugid_type: AclUgidType::User,
+ ugid: user.to_string(),
+ roleid: role.to_string(),
+ });
+ }
+ }
+
+ for (group, roles) in &node.groups {
+ if auth_id_filter.is_some() {
+ continue;
+ }
+
+ for (role, propagate) in roles {
+ list.push(AclListItem {
+ path: path_str.to_owned(),
+ propagate: *propagate,
+ ugid_type: AclUgidType::Group,
+ ugid: group.to_string(),
+ roleid: role.to_string(),
+ });
+ }
+ }
+
+ if !exact {
+ list.extend(node.children.iter().flat_map(|(comp, child)| {
+ let new_path = format!("{}/{comp}", path.unwrap_or(""));
+ extract_acl_node_data(child, Some(&new_path), exact, auth_id_filter)
+ }));
+ }
+
+ list
+}
+
+pub const ACL_ROUTER: Router = Router::new()
+ .get(&API_METHOD_READ_ACL)
+ .put(&API_METHOD_UPDATE_ACL);
diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs
index b0cf1a3e..a6d36780 100644
--- a/proxmox-access-control/src/init.rs
+++ b/proxmox-access-control/src/init.rs
@@ -72,6 +72,28 @@ pub trait AccessControlConfig: Send + Sync {
let _ = config;
Ok(())
}
+
+ /// This is used to determined what access control list entries a user is allowed to read.
+ ///
+ /// Override this if you want to use the `api` feature.
+ fn acl_audit_privileges(&self) -> u64 {
+ 0
+ }
+
+ /// This is used to determine what privileges are needed to modify the access control list.
+ ///
+ /// Override this if you want to use the `api` feature.
+ fn acl_modify_privileges(&self) -> u64 {
+ 0
+ }
+
+ /// Used to determine which paths are valid in a given `AclTree`.
+ ///
+ /// Override this if you want to use the `api` feature.
+ fn check_acl_path(&self, path: &str) -> Result<(), Error> {
+ let _ = path;
+ Ok(())
+ }
}
pub fn init<P: AsRef<Path>>(
diff --git a/proxmox-access-control/src/lib.rs b/proxmox-access-control/src/lib.rs
index c3aeb9db..62683924 100644
--- a/proxmox-access-control/src/lib.rs
+++ b/proxmox-access-control/src/lib.rs
@@ -5,6 +5,9 @@ pub mod types;
#[cfg(feature = "impl")]
pub mod acl;
+#[cfg(feature = "api")]
+pub mod api;
+
#[cfg(feature = "impl")]
pub mod init;
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-03 14:17 ` [pdm-devel] [PATCH proxmox 2/4] access-control: add acl " Shannon Sterz
@ 2025-04-09 11:01 ` Dietmar Maurer
2025-04-09 11:39 ` Dominik Csapak
0 siblings, 1 reply; 20+ messages in thread
From: Dietmar Maurer @ 2025-04-09 11:01 UTC (permalink / raw)
To: Proxmox Datacenter Manager development discussion, Shannon Sterz
> +/// Get ACL entries, can be filter by path.
> +pub fn read_acl(
> + path: Option<String>,
> + exact: bool,
> + rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Vec<AclListItem>, Error> {
> + let auth_id = rpcenv
> + .get_auth_id()
> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
> + .parse()?;
> +
> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
> +
> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
> + Some(auth_id)
> + } else {
> + None
> + };
As discussed offline, maybe we can use CachedUserInfo::check_privs here?
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-09 11:01 ` Dietmar Maurer
@ 2025-04-09 11:39 ` Dominik Csapak
2025-04-09 12:58 ` Shannon Sterz
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2025-04-09 11:39 UTC (permalink / raw)
To: Proxmox Datacenter Manager development discussion,
Dietmar Maurer, Shannon Sterz
On 4/9/25 13:01, Dietmar Maurer wrote:
>
>> +/// Get ACL entries, can be filter by path.
>> +pub fn read_acl(
>> + path: Option<String>,
>> + exact: bool,
>> + rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<Vec<AclListItem>, Error> {
>> + let auth_id = rpcenv
>> + .get_auth_id()
>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>> + .parse()?;
>> +
>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>> +
>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>> + Some(auth_id)
>> + } else {
>> + None
>> + };
>
> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>
>
maybe something like this for the update case (untested, please verify before using this!):
(the diff is for pbs, where the code was copied from)
this also includes a reformatted check for the token,non-token, same user checks
that are IMHO more readable than what we currently have
with the match, i think it's much more obvious that all cases are handled
---
let user_info = CachedUserInfo::new()?;
- let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
- if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
+ let has_modify_permission = user_info
+ .check_privs(
+ ¤t_auth_id,
+ &["access", "acl"],
+ PRIV_PERMISSIONS_MODIFY,
+ false,
+ )
+ .is_ok();
+
+ if !has_modify_permission {
if group.is_some() {
bail!("Unprivileged users are not allowed to create group ACL item.");
}
match &auth_id {
Some(auth_id) => {
- if current_auth_id.is_token() {
- bail!("Unprivileged API tokens can't set ACL items.");
- } else if !auth_id.is_token() {
- bail!("Unprivileged users can only set ACL items for API tokens.");
- } else if auth_id.user() != current_auth_id.user() {
- bail!("Unprivileged users can only set ACL items for their own API tokens.");
+ let same_user = auth_id.user() == current_auth_id.user();
+ match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
+ (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
+ (false, false, _) => {
+ bail!("Unprivileged users can only set ACL items for API tokens.")
+ }
+ (false, true, true) => {
+ // users are always allowed to modify ACLs for their own tokens
+ }
+ (false, true, false) => {
+ bail!("Unprivileged users can only set ACL items for their own API tokens.")
+ }
}
}
None => {
---
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-09 11:39 ` Dominik Csapak
@ 2025-04-09 12:58 ` Shannon Sterz
2025-04-10 6:28 ` Dominik Csapak
2025-04-11 10:29 ` Shannon Sterz
0 siblings, 2 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-09 12:58 UTC (permalink / raw)
To: Dominik Csapak,
Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
> On 4/9/25 13:01, Dietmar Maurer wrote:
>>
>>> +/// Get ACL entries, can be filter by path.
>>> +pub fn read_acl(
>>> + path: Option<String>,
>>> + exact: bool,
>>> + rpcenv: &mut dyn RpcEnvironment,
>>> +) -> Result<Vec<AclListItem>, Error> {
>>> + let auth_id = rpcenv
>>> + .get_auth_id()
>>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>>> + .parse()?;
>>> +
>>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>>> +
>>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>>> + Some(auth_id)
>>> + } else {
>>> + None
>>> + };
>>
>> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>>
>>
>
> maybe something like this for the update case (untested, please verify before using this!):
> (the diff is for pbs, where the code was copied from)
>
> this also includes a reformatted check for the token,non-token, same user checks
> that are IMHO more readable than what we currently have
> with the match, i think it's much more obvious that all cases are handled
>
> ---
> let user_info = CachedUserInfo::new()?;
>
> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
> + let has_modify_permission = user_info
> + .check_privs(
> + ¤t_auth_id,
> + &["access", "acl"],
> + PRIV_PERMISSIONS_MODIFY,
> + false,
the false here means that partial matches are discounted. i'm not sure
this is correct as at least in pbs and pdm, we do use a partial check as
that is equivalent to the check i ported over.
imo, we'd need to discuss what the proper semantics are here and at
least up until now, we decided for partial semantics.
> + )
> + .is_ok();
> +
> + if !has_modify_permission {
> if group.is_some() {
> bail!("Unprivileged users are not allowed to create group ACL item.");
> }
>
> match &auth_id {
> Some(auth_id) => {
> - if current_auth_id.is_token() {
> - bail!("Unprivileged API tokens can't set ACL items.");
> - } else if !auth_id.is_token() {
> - bail!("Unprivileged users can only set ACL items for API tokens.");
> - } else if auth_id.user() != current_auth_id.user() {
> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
> + let same_user = auth_id.user() == current_auth_id.user();
> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
> + (false, false, _) => {
> + bail!("Unprivileged users can only set ACL items for API tokens.")
> + }
> + (false, true, true) => {
> + // users are always allowed to modify ACLs for their own tokens
> + }
> + (false, true, false) => {
> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
> + }
> }
> }
> None => {
> ---
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-09 12:58 ` Shannon Sterz
@ 2025-04-10 6:28 ` Dominik Csapak
2025-04-10 8:17 ` Shannon Sterz
2025-04-11 10:29 ` Shannon Sterz
1 sibling, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2025-04-10 6:28 UTC (permalink / raw)
To: Shannon Sterz, Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On 4/9/25 14:58, Shannon Sterz wrote:
> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>>
>>>> +/// Get ACL entries, can be filter by path.
>>>> +pub fn read_acl(
>>>> + path: Option<String>,
>>>> + exact: bool,
>>>> + rpcenv: &mut dyn RpcEnvironment,
>>>> +) -> Result<Vec<AclListItem>, Error> {
>>>> + let auth_id = rpcenv
>>>> + .get_auth_id()
>>>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>>>> + .parse()?;
>>>> +
>>>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>>>> +
>>>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>>>> + Some(auth_id)
>>>> + } else {
>>>> + None
>>>> + };
>>>
>>> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>>>
>>>
>>
>> maybe something like this for the update case (untested, please verify before using this!):
>> (the diff is for pbs, where the code was copied from)
>>
>> this also includes a reformatted check for the token,non-token, same user checks
>> that are IMHO more readable than what we currently have
>> with the match, i think it's much more obvious that all cases are handled
>>
>> ---
>> let user_info = CachedUserInfo::new()?;
>>
>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>> + let has_modify_permission = user_info
>> + .check_privs(
>> + ¤t_auth_id,
>> + &["access", "acl"],
>> + PRIV_PERMISSIONS_MODIFY,
>> + false,
>
> the false here means that partial matches are discounted. i'm not sure
> this is correct as at least in pbs and pdm, we do use a partial check as
> that is equivalent to the check i ported over.
>
> imo, we'd need to discuss what the proper semantics are here and at
> least up until now, we decided for partial semantics.
IIUC the PRIV_PERMISSIONS_MODIFY is only a single bit, so partial/not partial makes
no difference in this diff here.
but yeah sure, if we have multiple privileges that would all allow setting
ACL individually, we would have to match with `partial = true`
>
>> + )
>> + .is_ok();
>> +
>> + if !has_modify_permission {
>> if group.is_some() {
>> bail!("Unprivileged users are not allowed to create group ACL item.");
>> }
>>
>> match &auth_id {
>> Some(auth_id) => {
>> - if current_auth_id.is_token() {
>> - bail!("Unprivileged API tokens can't set ACL items.");
>> - } else if !auth_id.is_token() {
>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>> - } else if auth_id.user() != current_auth_id.user() {
>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>> + let same_user = auth_id.user() == current_auth_id.user();
>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>> + (false, false, _) => {
>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>> + }
>> + (false, true, true) => {
>> + // users are always allowed to modify ACLs for their own tokens
>> + }
>> + (false, true, false) => {
>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>> + }
>> }
>> }
>> None => {
>> ---
>
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-10 6:28 ` Dominik Csapak
@ 2025-04-10 8:17 ` Shannon Sterz
2025-04-10 10:09 ` Dominik Csapak
0 siblings, 1 reply; 20+ messages in thread
From: Shannon Sterz @ 2025-04-10 8:17 UTC (permalink / raw)
To: Dominik Csapak,
Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On Thu Apr 10, 2025 at 8:28 AM CEST, Dominik Csapak wrote:
> On 4/9/25 14:58, Shannon Sterz wrote:
>> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>>>
>>>>> +/// Get ACL entries, can be filter by path.
>>>>> +pub fn read_acl(
>>>>> + path: Option<String>,
>>>>> + exact: bool,
>>>>> + rpcenv: &mut dyn RpcEnvironment,
>>>>> +) -> Result<Vec<AclListItem>, Error> {
>>>>> + let auth_id = rpcenv
>>>>> + .get_auth_id()
>>>>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>>>>> + .parse()?;
>>>>> +
>>>>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>>>>> +
>>>>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>>>>> + Some(auth_id)
>>>>> + } else {
>>>>> + None
>>>>> + };
>>>>
>>>> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>>>>
>>>>
>>>
>>> maybe something like this for the update case (untested, please verify before using this!):
>>> (the diff is for pbs, where the code was copied from)
>>>
>>> this also includes a reformatted check for the token,non-token, same user checks
>>> that are IMHO more readable than what we currently have
>>> with the match, i think it's much more obvious that all cases are handled
>>>
>>> ---
>>> let user_info = CachedUserInfo::new()?;
>>>
>>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>> + let has_modify_permission = user_info
>>> + .check_privs(
>>> + ¤t_auth_id,
>>> + &["access", "acl"],
>>> + PRIV_PERMISSIONS_MODIFY,
>>> + false,
>>
>> the false here means that partial matches are discounted. i'm not sure
>> this is correct as at least in pbs and pdm, we do use a partial check as
>> that is equivalent to the check i ported over.
>>
>> imo, we'd need to discuss what the proper semantics are here and at
>> least up until now, we decided for partial semantics.
>
> IIUC the PRIV_PERMISSIONS_MODIFY is only a single bit, so partial/not partial makes
> no difference in this diff here.
>
> but yeah sure, if we have multiple privileges that would all allow setting
> ACL individually, we would have to match with `partial = true`
well, except your code stems from the pre-existing code (in pbs
presumably?). in my moved implementation PRIV_PERMISSIONS_MODIFY isn't
used anymore. the value is parameterized by the product via the
AccessControlConfig. which is necessary, otherwise we need to create
some kind of minimal set of common privileges between all products.
keeping that clean and conflict free sounds more tedious to me, though.
we could also expose whether this should allow partial matches or not
there (in the AccessControlConfig) too. for now i'd stick with keeping
the pre-existing behaviour where we can (and we can do this easily here)
in order to avoid possibly confusing bugs. unless there is a good reason
to forbid partial matches at this point already.
but yes, as mentioned yesterday, all current products define the
privileges as a bitmap (via the `constnamedbitmap!` macro). therefore no
privileges *should* use more than one bit and making that change
*should* be safe.
>>> + )
>>> + .is_ok();
>>> +
>>> + if !has_modify_permission {
>>> if group.is_some() {
>>> bail!("Unprivileged users are not allowed to create group ACL item.");
>>> }
>>>
>>> match &auth_id {
>>> Some(auth_id) => {
>>> - if current_auth_id.is_token() {
>>> - bail!("Unprivileged API tokens can't set ACL items.");
>>> - } else if !auth_id.is_token() {
>>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>>> - } else if auth_id.user() != current_auth_id.user() {
>>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>>> + let same_user = auth_id.user() == current_auth_id.user();
>>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>>> + (false, false, _) => {
>>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>>> + }
>>> + (false, true, true) => {
>>> + // users are always allowed to modify ACLs for their own tokens
>>> + }
>>> + (false, true, false) => {
>>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>>> + }
>>> }
>>> }
>>> None => {
>>> ---
>>
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-10 8:17 ` Shannon Sterz
@ 2025-04-10 10:09 ` Dominik Csapak
0 siblings, 0 replies; 20+ messages in thread
From: Dominik Csapak @ 2025-04-10 10:09 UTC (permalink / raw)
To: Shannon Sterz, Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On 4/10/25 10:17, Shannon Sterz wrote:
> On Thu Apr 10, 2025 at 8:28 AM CEST, Dominik Csapak wrote:
>> On 4/9/25 14:58, Shannon Sterz wrote:
>>> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>>>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>>>>
>>>>>> +/// Get ACL entries, can be filter by path.
>>>>>> +pub fn read_acl(
>>>>>> + path: Option<String>,
>>>>>> + exact: bool,
>>>>>> + rpcenv: &mut dyn RpcEnvironment,
>>>>>> +) -> Result<Vec<AclListItem>, Error> {
>>>>>> + let auth_id = rpcenv
>>>>>> + .get_auth_id()
>>>>>> + .ok_or_else(|| format_err!("endpoint called without an auth id"))?
>>>>>> + .parse()?;
>>>>>> +
>>>>>> + let top_level_privs = CachedUserInfo::new()?.lookup_privs(&auth_id, &["access", "acl"]);
>>>>>> +
>>>>>> + let filter = if top_level_privs & access_conf().acl_audit_privileges() == 0 {
>>>>>> + Some(auth_id)
>>>>>> + } else {
>>>>>> + None
>>>>>> + };
>>>>>
>>>>> As discussed offline, maybe we can use CachedUserInfo::check_privs here?
>>>>>
>>>>>
>>>>
>>>> maybe something like this for the update case (untested, please verify before using this!):
>>>> (the diff is for pbs, where the code was copied from)
>>>>
>>>> this also includes a reformatted check for the token,non-token, same user checks
>>>> that are IMHO more readable than what we currently have
>>>> with the match, i think it's much more obvious that all cases are handled
>>>>
>>>> ---
>>>> let user_info = CachedUserInfo::new()?;
>>>>
>>>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>>>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>>> + let has_modify_permission = user_info
>>>> + .check_privs(
>>>> + ¤t_auth_id,
>>>> + &["access", "acl"],
>>>> + PRIV_PERMISSIONS_MODIFY,
>>>> + false,
>>>
>>> the false here means that partial matches are discounted. i'm not sure
>>> this is correct as at least in pbs and pdm, we do use a partial check as
>>> that is equivalent to the check i ported over.
>>>
>>> imo, we'd need to discuss what the proper semantics are here and at
>>> least up until now, we decided for partial semantics.
>>
>> IIUC the PRIV_PERMISSIONS_MODIFY is only a single bit, so partial/not partial makes
>> no difference in this diff here.
>>
>> but yeah sure, if we have multiple privileges that would all allow setting
>> ACL individually, we would have to match with `partial = true`
>
> well, except your code stems from the pre-existing code (in pbs
> presumably?). in my moved implementation PRIV_PERMISSIONS_MODIFY isn't
> used anymore. the value is parameterized by the product via the
> AccessControlConfig. which is necessary, otherwise we need to create
> some kind of minimal set of common privileges between all products.
> keeping that clean and conflict free sounds more tedious to me, though.
>
> we could also expose whether this should allow partial matches or not
> there (in the AccessControlConfig) too. for now i'd stick with keeping
> the pre-existing behaviour where we can (and we can do this easily here)
> in order to avoid possibly confusing bugs. unless there is a good reason
> to forbid partial matches at this point already.
i don't think returning an extra boolean to configure is too much work
but err'ing on the safe side to require all permissions defined by default
does not sound too bad to me to start (we can always loosen it)
>
> but yes, as mentioned yesterday, all current products define the
> privileges as a bitmap (via the `constnamedbitmap!` macro). therefore no
> privileges *should* use more than one bit and making that change
> *should* be safe.
as i wrote my change is only an example and probably should not be used
as is without thinking more about it
also I wanted to put more emphasis on the token/user issue below, than
on the check_privs call
>
>>>> + )
>>>> + .is_ok();
>>>> +
>>>> + if !has_modify_permission {
>>>> if group.is_some() {
>>>> bail!("Unprivileged users are not allowed to create group ACL item.");
>>>> }
>>>>
>>>> match &auth_id {
>>>> Some(auth_id) => {
>>>> - if current_auth_id.is_token() {
>>>> - bail!("Unprivileged API tokens can't set ACL items.");
>>>> - } else if !auth_id.is_token() {
>>>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>>>> - } else if auth_id.user() != current_auth_id.user() {
>>>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>>>> + let same_user = auth_id.user() == current_auth_id.user();
>>>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>>>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>>>> + (false, false, _) => {
>>>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>>>> + }
>>>> + (false, true, true) => {
>>>> + // users are always allowed to modify ACLs for their own tokens
>>>> + }
>>>> + (false, true, false) => {
>>>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>>>> + }
>>>> }
>>>> }
>>>> None => {
>>>> ---
>>>
>
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-09 12:58 ` Shannon Sterz
2025-04-10 6:28 ` Dominik Csapak
@ 2025-04-11 10:29 ` Shannon Sterz
2025-04-11 10:53 ` Dominik Csapak
1 sibling, 1 reply; 20+ messages in thread
From: Shannon Sterz @ 2025-04-11 10:29 UTC (permalink / raw)
To: Shannon Sterz, Dominik Csapak,
Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On Wed Apr 9, 2025 at 2:58 PM CEST, Shannon Sterz wrote:
> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>> On 4/9/25 13:01, Dietmar Maurer wrote:
>> maybe something like this for the update case (untested, please verify before using this!):
>> (the diff is for pbs, where the code was copied from)
>>
>> this also includes a reformatted check for the token,non-token, same user checks
>> that are IMHO more readable than what we currently have
>> with the match, i think it's much more obvious that all cases are handled
>>
>> ---
>> let user_info = CachedUserInfo::new()?;
>>
>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>> + let has_modify_permission = user_info
>> + .check_privs(
>> + ¤t_auth_id,
>> + &["access", "acl"],
>> + PRIV_PERMISSIONS_MODIFY,
>> + false,
>> + )
>> + .is_ok();
>> +
>> + if !has_modify_permission {
>> if group.is_some() {
>> bail!("Unprivileged users are not allowed to create group ACL item.");
>> }
>>
>> match &auth_id {
>> Some(auth_id) => {
>> - if current_auth_id.is_token() {
>> - bail!("Unprivileged API tokens can't set ACL items.");
>> - } else if !auth_id.is_token() {
>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>> - } else if auth_id.user() != current_auth_id.user() {
>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>> + let same_user = auth_id.user() == current_auth_id.user();
>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>> + (false, false, _) => {
>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>> + }
>> + (false, true, true) => {
>> + // users are always allowed to modify ACLs for their own tokens
>> + }
>> + (false, true, false) => {
>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>> + }
>> }
>> }
>> None => {
>> ---
had another think about this, i'd tend towards something like the below.
the match statement is a nice idea, but it couples together things that
aren't really related. for example, why pull in the
`current_auth_id.is_token()` check, but not the `group.is_some()` check?
having match statements with tuples like this is making the code more
complex. imo, this is simpler:
```rs
let unprivileged_user = CachedUserInfo::new()?
.check_privs(
¤t_auth_id,
&["access", "acl"],
access_conf.acl_modify_privileges(),
access_conf.allow_partial_permission_match(),
)
.is_err();
if unprivileged_user {
if group.is_some() {
bail!("Unprivileged users are not allowed to create group ACL item.");
}
let auth_id = auth_id.as_ref().ok_or_else(|| {
format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
})?;
if current_auth_id.is_token() {
bail!("Unprivileged API tokens can't set ACL items.");
}
if !auth_id.is_token() {
bail!("Unprivileged users can only set ACL items for API tokens.");
}
if current_auth_id != *auth_id {
bail!("Unprivileged users can only set ACL items for their own API tokens.");
}
}
```
what do you think?
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-11 10:29 ` Shannon Sterz
@ 2025-04-11 10:53 ` Dominik Csapak
2025-04-11 11:40 ` Shannon Sterz
0 siblings, 1 reply; 20+ messages in thread
From: Dominik Csapak @ 2025-04-11 10:53 UTC (permalink / raw)
To: Shannon Sterz, Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On 4/11/25 12:29, Shannon Sterz wrote:
> On Wed Apr 9, 2025 at 2:58 PM CEST, Shannon Sterz wrote:
>> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>> maybe something like this for the update case (untested, please verify before using this!):
>>> (the diff is for pbs, where the code was copied from)
>>>
>>> this also includes a reformatted check for the token,non-token, same user checks
>>> that are IMHO more readable than what we currently have
>>> with the match, i think it's much more obvious that all cases are handled
>>>
>>> ---
>>> let user_info = CachedUserInfo::new()?;
>>>
>>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>> + let has_modify_permission = user_info
>>> + .check_privs(
>>> + ¤t_auth_id,
>>> + &["access", "acl"],
>>> + PRIV_PERMISSIONS_MODIFY,
>>> + false,
>>> + )
>>> + .is_ok();
>>> +
>>> + if !has_modify_permission {
>>> if group.is_some() {
>>> bail!("Unprivileged users are not allowed to create group ACL item.");
>>> }
>>>
>>> match &auth_id {
>>> Some(auth_id) => {
>>> - if current_auth_id.is_token() {
>>> - bail!("Unprivileged API tokens can't set ACL items.");
>>> - } else if !auth_id.is_token() {
>>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>>> - } else if auth_id.user() != current_auth_id.user() {
>>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>>> + let same_user = auth_id.user() == current_auth_id.user();
>>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>>> + (false, false, _) => {
>>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>>> + }
>>> + (false, true, true) => {
>>> + // users are always allowed to modify ACLs for their own tokens
>>> + }
>>> + (false, true, false) => {
>>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>>> + }
>>> }
>>> }
>>> None => {
>>> ---
>
> had another think about this, i'd tend towards something like the below.
> the match statement is a nice idea, but it couples together things that
> aren't really related. for example, why pull in the
> `current_auth_id.is_token()` check, but not the `group.is_some()` check?
> having match statements with tuples like this is making the code more
> complex. imo, this is simpler:
>
> ```rs
> let unprivileged_user = CachedUserInfo::new()?
> .check_privs(
> ¤t_auth_id,
> &["access", "acl"],
> access_conf.acl_modify_privileges(),
> access_conf.allow_partial_permission_match(),
> )
> .is_err();
>
> if unprivileged_user {
> if group.is_some() {
> bail!("Unprivileged users are not allowed to create group ACL item.");
> }
>
> let auth_id = auth_id.as_ref().ok_or_else(|| {
> format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
> })?;
>
> if current_auth_id.is_token() {
> bail!("Unprivileged API tokens can't set ACL items.");
> }
>
> if !auth_id.is_token() {
> bail!("Unprivileged users can only set ACL items for API tokens.");
> }
>
> if current_auth_id != *auth_id {
> bail!("Unprivileged users can only set ACL items for their own API tokens.");
> }
> }
> ```
>
> what do you think?
I see what you mean, and yes i think it's more readable, but what I really wanted to convey with my
approach was to clarify which condition is ok
we currently try to filter out all invalid states, and it is not really obvious what
condition makes the code continue at first glance
Maybe we have to approach it completely different, for example check only the valid cases first
and let that pass through, and then fail with the specific errors and have a fallback error
for all other cases. that way we can't come into a situation where we forget/overlook some edge
case.
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature
2025-04-11 10:53 ` Dominik Csapak
@ 2025-04-11 11:40 ` Shannon Sterz
0 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-11 11:40 UTC (permalink / raw)
To: Dominik Csapak,
Proxmox Datacenter Manager development discussion,
Dietmar Maurer
On Fri Apr 11, 2025 at 12:53 PM CEST, Dominik Csapak wrote:
> On 4/11/25 12:29, Shannon Sterz wrote:
>> On Wed Apr 9, 2025 at 2:58 PM CEST, Shannon Sterz wrote:
>>> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>>>> On 4/9/25 13:01, Dietmar Maurer wrote:
>>>> maybe something like this for the update case (untested, please verify before using this!):
>>>> (the diff is for pbs, where the code was copied from)
>>>>
>>>> this also includes a reformatted check for the token,non-token, same user checks
>>>> that are IMHO more readable than what we currently have
>>>> with the match, i think it's much more obvious that all cases are handled
>>>>
>>>> ---
>>>> let user_info = CachedUserInfo::new()?;
>>>>
>>>> - let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
>>>> - if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>>>> + let has_modify_permission = user_info
>>>> + .check_privs(
>>>> + ¤t_auth_id,
>>>> + &["access", "acl"],
>>>> + PRIV_PERMISSIONS_MODIFY,
>>>> + false,
>>>> + )
>>>> + .is_ok();
>>>> +
>>>> + if !has_modify_permission {
>>>> if group.is_some() {
>>>> bail!("Unprivileged users are not allowed to create group ACL item.");
>>>> }
>>>>
>>>> match &auth_id {
>>>> Some(auth_id) => {
>>>> - if current_auth_id.is_token() {
>>>> - bail!("Unprivileged API tokens can't set ACL items.");
>>>> - } else if !auth_id.is_token() {
>>>> - bail!("Unprivileged users can only set ACL items for API tokens.");
>>>> - } else if auth_id.user() != current_auth_id.user() {
>>>> - bail!("Unprivileged users can only set ACL items for their own API tokens.");
>>>> + let same_user = auth_id.user() == current_auth_id.user();
>>>> + match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>>>> + (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>>>> + (false, false, _) => {
>>>> + bail!("Unprivileged users can only set ACL items for API tokens.")
>>>> + }
>>>> + (false, true, true) => {
>>>> + // users are always allowed to modify ACLs for their own tokens
>>>> + }
>>>> + (false, true, false) => {
>>>> + bail!("Unprivileged users can only set ACL items for their own API tokens.")
>>>> + }
>>>> }
>>>> }
>>>> None => {
>>>> ---
>>
>> had another think about this, i'd tend towards something like the below.
>> the match statement is a nice idea, but it couples together things that
>> aren't really related. for example, why pull in the
>> `current_auth_id.is_token()` check, but not the `group.is_some()` check?
>> having match statements with tuples like this is making the code more
>> complex. imo, this is simpler:
>>
>> ```rs
>> let unprivileged_user = CachedUserInfo::new()?
>> .check_privs(
>> ¤t_auth_id,
>> &["access", "acl"],
>> access_conf.acl_modify_privileges(),
>> access_conf.allow_partial_permission_match(),
>> )
>> .is_err();
>>
>> if unprivileged_user {
>> if group.is_some() {
>> bail!("Unprivileged users are not allowed to create group ACL item.");
>> }
>>
>> let auth_id = auth_id.as_ref().ok_or_else(|| {
>> format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
>> })?;
>>
>> if current_auth_id.is_token() {
>> bail!("Unprivileged API tokens can't set ACL items.");
>> }
>>
>> if !auth_id.is_token() {
>> bail!("Unprivileged users can only set ACL items for API tokens.");
>> }
>>
>> if current_auth_id != *auth_id {
>> bail!("Unprivileged users can only set ACL items for their own API tokens.");
>> }
>> }
>> ```
>>
>> what do you think?
>
> I see what you mean, and yes i think it's more readable, but what I really wanted to convey with my
> approach was to clarify which condition is ok
>
> we currently try to filter out all invalid states, and it is not really obvious what
> condition makes the code continue at first glance
>
> Maybe we have to approach it completely different, for example check only the valid cases first
> and let that pass through, and then fail with the specific errors and have a fallback error
> for all other cases. that way we can't come into a situation where we forget/overlook some edge
> case.
oh, i mean we could just add a comment to the first if statement there
something like:
```rs
// check that if a user with insufficient permissions is changing acl
// entries, that they only modify their own api tokens' entries.
// unprivileged api tokens are not allowed to modify anything.
if unprivileged_user {
...
```
alternatively, we could do this which is closer to your suggestion in
the last comment
```rs
if unprivileged_user {
if group.is_none()
&& !current_auth_id.is_token()
// check that an entry for an auth_id is being edited and
// that it is a token for the user that is making the edit
&& auth_id
.as_ref()
.map(|id| id.is_token() && current_auth_id.user() == id.user())
.unwrap_or_default()
{
// a user is directly editing the privileges of their own token, this is always
// allowed
} else {
if group.is_some() {
bail!("Unprivileged users are not allowed to create group ACL item.");
}
let auth_id = auth_id.as_ref().ok_or_else(|| {
format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
})?;
if current_auth_id.is_token() {
bail!("Unprivileged API tokens can't set ACL items.");
}
if !auth_id.is_token() {
bail!("Unprivileged users can only set ACL items for API tokens.");
}
if current_auth_id.user() != auth_id.user() {
bail!("Unprivileged users can only set ACL items for their own API tokens.");
}
// this should not be reachable, but just in case, bail here
bail!("Unprivileged user is trying to set an invalid ACL item.")
}
}
```
i think that respects your initial intend and also has a fail-safe just
in case something got overlooked or is changed later on.
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread
* [pdm-devel] [PATCH proxmox 3/4] access-control: add comments to roles function of AccessControlConfig
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
2025-04-03 14:17 ` [pdm-devel] [PATCH proxmox 1/4] access-control: add more types to prepare for api feature Shannon Sterz
2025-04-03 14:17 ` [pdm-devel] [PATCH proxmox 2/4] access-control: add acl " Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH proxmox 4/4] access-control: add generic roles endpoint to `api` feature Shannon Sterz
` (6 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
this will allow us to implement a generic `roles()` endpoint similar
to the one proxmox backup server already has at `/access/roles` and
that proxmox-yew-comp's `RoleSelector` already relies on.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
proxmox-access-control/src/acl.rs | 12 ++++++------
proxmox-access-control/src/cached_user_info.rs | 4 ++--
proxmox-access-control/src/init.rs | 5 +++--
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/proxmox-access-control/src/acl.rs b/proxmox-access-control/src/acl.rs
index b8041688..270292ac 100644
--- a/proxmox-access-control/src/acl.rs
+++ b/proxmox-access-control/src/acl.rs
@@ -647,11 +647,11 @@ mod test {
#[derive(Debug)]
struct TestAcmConfig<'a> {
- roles: HashMap<&'a str, u64>,
+ roles: HashMap<&'a str, (u64, &'a str)>,
}
impl AccessControlConfig for TestAcmConfig<'_> {
- fn roles(&self) -> &HashMap<&str, u64> {
+ fn roles(&self) -> &HashMap<&str, (u64, &str)> {
&self.roles
}
@@ -672,10 +672,10 @@ mod test {
static ACL_CONFIG: OnceLock<TestAcmConfig> = OnceLock::new();
let config = ACL_CONFIG.get_or_init(|| {
let mut roles = HashMap::new();
- roles.insert("NoAccess", 0);
- roles.insert("Admin", u64::MAX);
- roles.insert("DatastoreBackup", 4);
- roles.insert("DatastoreReader", 8);
+ roles.insert("NoAccess", (0, "comment"));
+ roles.insert("Admin", (u64::MAX, "comment"));
+ roles.insert("DatastoreBackup", (4, "comment"));
+ roles.insert("DatastoreReader", (8, "comment"));
TestAcmConfig { roles }
});
diff --git a/proxmox-access-control/src/cached_user_info.rs b/proxmox-access-control/src/cached_user_info.rs
index 4d011f00..f5ed2858 100644
--- a/proxmox-access-control/src/cached_user_info.rs
+++ b/proxmox-access-control/src/cached_user_info.rs
@@ -150,7 +150,7 @@ impl CachedUserInfo {
if self.is_superuser(auth_id) {
let acm_config = access_conf();
if let Some(admin) = acm_config.role_admin() {
- if let Some(admin) = acm_config.roles().get(admin) {
+ if let Some((admin, _)) = acm_config.roles().get(admin) {
return (*admin, *admin);
}
}
@@ -160,7 +160,7 @@ impl CachedUserInfo {
let mut privs: u64 = 0;
let mut propagated_privs: u64 = 0;
for (role, propagate) in roles {
- if let Some(role_privs) = access_conf().roles().get(role.as_str()) {
+ if let Some((role_privs, _)) = access_conf().roles().get(role.as_str()) {
if propagate {
propagated_privs |= role_privs;
}
diff --git a/proxmox-access-control/src/init.rs b/proxmox-access-control/src/init.rs
index a6d36780..c219a78e 100644
--- a/proxmox-access-control/src/init.rs
+++ b/proxmox-access-control/src/init.rs
@@ -16,8 +16,9 @@ pub trait AccessControlConfig: Send + Sync {
/// Returns a mapping of all recognized privileges and their corresponding `u64` value.
fn privileges(&self) -> &HashMap<&str, u64>;
- /// Returns a mapping of all recognized roles and their corresponding `u64` value.
- fn roles(&self) -> &HashMap<&str, u64>;
+ /// Returns a mapping of all recognized roles and their corresponding `u64` value as well as
+ /// a comment.
+ fn roles(&self) -> &HashMap<&str, (u64, &str)>;
/// Checks whether an `Authid` has super user privileges or not.
///
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH proxmox 4/4] access-control: add generic roles endpoint to `api` feature
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (2 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH proxmox 3/4] access-control: add comments to roles function of AccessControlConfig Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 1/3] api-types/role_selector: depend on common `RoleInfo` type Shannon Sterz
` (5 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
since this is always the same between most products and we already
have access to all the relevant information
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
proxmox-access-control/src/api.rs | 45 ++++++++++++++++++++++++++++-
proxmox-access-control/src/types.rs | 28 ++++++++++++++++++
2 files changed, 72 insertions(+), 1 deletion(-)
diff --git a/proxmox-access-control/src/api.rs b/proxmox-access-control/src/api.rs
index 4a6aabf5..3c62fbcf 100644
--- a/proxmox-access-control/src/api.rs
+++ b/proxmox-access-control/src/api.rs
@@ -7,7 +7,9 @@ use proxmox_schema::api;
use crate::acl::AclTreeNode;
use crate::init::access_conf;
-use crate::types::{AclListItem, AclUgidType, ACL_PATH_SCHEMA, ACL_PROPAGATE_SCHEMA};
+use crate::types::{
+ AclListItem, AclUgidType, RoleInfo, ACL_PATH_SCHEMA, ACL_PROPAGATE_SCHEMA,
+};
use crate::CachedUserInfo;
#[api(
@@ -276,3 +278,44 @@ fn extract_acl_node_data(
pub const ACL_ROUTER: Router = Router::new()
.get(&API_METHOD_READ_ACL)
.put(&API_METHOD_UPDATE_ACL);
+
+#[api(
+ returns: {
+ description: "List of roles.",
+ type: Array,
+ items: {
+ type: RoleInfo,
+ }
+ },
+ access: {
+ permission: &Permission::Anybody,
+ }
+)]
+/// A list of available roles
+fn list_roles() -> Result<Vec<RoleInfo>, Error> {
+ let list = access_conf()
+ .roles()
+ .iter()
+ .map(|(role, (privs, comment))| {
+ let priv_list = access_conf()
+ .privileges()
+ .iter()
+ .filter_map(|(name, privilege)| {
+ if privs & privilege > 0 {
+ Some(name.to_string())
+ } else {
+ None
+ }
+ });
+
+ RoleInfo {
+ roleid: role.to_string(),
+ privs: priv_list.collect(),
+ comment: Some(comment.to_string()),
+ }
+ });
+
+ Ok(list.collect())
+}
+
+pub const ROLE_ROUTER: Router = Router::new().get(&API_METHOD_LIST_ROLES);
diff --git a/proxmox-access-control/src/types.rs b/proxmox-access-control/src/types.rs
index 01d078de..ea64d333 100644
--- a/proxmox-access-control/src/types.rs
+++ b/proxmox-access-control/src/types.rs
@@ -247,3 +247,31 @@ pub struct AclListItem {
/// A role represented as a string.
pub roleid: String,
}
+
+#[api(
+ properties: {
+ privs: {
+ type: Array,
+ description: "List of Privileges",
+ items: {
+ type: String,
+ description: "A Privilege",
+ },
+ },
+ comment: {
+ schema: COMMENT_SCHEMA,
+ optional: true,
+ }
+ }
+)]
+/// A struct that the describes a role and shows the associated privileges.
+#[derive(Serialize, Deserialize, PartialEq, Clone)]
+pub struct RoleInfo {
+ /// The id of the role
+ pub roleid: String,
+ /// The privileges the role holds
+ pub privs: Vec<String>,
+ /// A comment describing the role
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub comment: Option<String>,
+}
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH yew-comp 1/3] api-types/role_selector: depend on common `RoleInfo` type
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (3 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH proxmox 4/4] access-control: add generic roles endpoint to `api` feature Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 2/3] acl: add a view and semi-generic `EditWindow` for acl entries Shannon Sterz
` (4 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
instead of redefining it here. this should make sure that we stay in
sync with the common api definition in `proxmox-access-control`.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
src/common_api_types.rs | 8 --------
src/role_selector.rs | 2 +-
2 files changed, 1 insertion(+), 9 deletions(-)
diff --git a/src/common_api_types.rs b/src/common_api_types.rs
index 58aec58..7b22bb3 100644
--- a/src/common_api_types.rs
+++ b/src/common_api_types.rs
@@ -24,14 +24,6 @@ impl ExtractPrimaryKey for BasicRealmInfo {
}
}
-#[derive(Serialize, Deserialize, PartialEq, Clone)]
-pub struct RoleInfo {
- pub roleid: String,
- pub privs: Vec<String>,
- #[serde(skip_serializing_if = "Option::is_none")]
- pub comment: Option<String>,
-}
-
// copied from pbs_api_types::TaskListItem;
#[derive(Serialize, Deserialize, Clone, PartialEq)]
/// Task properties.
diff --git a/src/role_selector.rs b/src/role_selector.rs
index 7bde09f..20a8483 100644
--- a/src/role_selector.rs
+++ b/src/role_selector.rs
@@ -10,7 +10,7 @@ use pwt::widget::data_table::{DataTable, DataTableColumn, DataTableHeader};
use pwt::widget::form::{Selector, SelectorRenderArgs, ValidateFn};
use pwt::widget::GridPicker;
-use crate::common_api_types::RoleInfo;
+use proxmox_access_control::types::RoleInfo;
thread_local! {
static COLUMNS: Rc<Vec<DataTableHeader<RoleInfo>>> = Rc::new(vec![
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH yew-comp 2/3] acl: add a view and semi-generic `EditWindow` for acl entries
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (4 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 1/3] api-types/role_selector: depend on common `RoleInfo` type Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 3/3] role_selector/acl_edit: make api endpoint and default role configurable Shannon Sterz
` (3 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
since each product will always have different acl paths, editing them
cannot be made completelly generic. however, this does try to make
displaying them generic based on the common api implementations in
`proxmox-access-control`.
furthermore, a the `AclEditWindow` trait makes it possible for each
product to adapt the editing panels and the number of them that are
displayed in the ACL View configurable. allowing for greater
flexibility while trying to reduce duplicate code accross multiple
products.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
src/acl/acl_edit.rs | 92 +++++++++++++++
src/acl/acl_view.rs | 270 ++++++++++++++++++++++++++++++++++++++++++++
src/acl/mod.rs | 5 +
src/lib.rs | 3 +
4 files changed, 370 insertions(+)
create mode 100644 src/acl/acl_edit.rs
create mode 100644 src/acl/acl_view.rs
create mode 100644 src/acl/mod.rs
diff --git a/src/acl/acl_edit.rs b/src/acl/acl_edit.rs
new file mode 100644
index 0000000..b8bbd89
--- /dev/null
+++ b/src/acl/acl_edit.rs
@@ -0,0 +1,92 @@
+use yew::html::IntoPropValue;
+
+use pwt::prelude::*;
+use pwt::widget::form::{Checkbox, FormContext};
+use pwt::widget::{FieldLabel, InputPanel};
+
+use pwt_macros::builder;
+
+use crate::EditWindow;
+use crate::{AuthidSelector, RoleSelector};
+
+pub trait AclEditWindow: Into<EditWindow> {}
+
+#[derive(Clone, PartialEq, Properties)]
+#[builder]
+pub struct AclEdit {
+ /// Use API Tokens instead of Users.
+ #[prop_or_default]
+ #[builder]
+ use_tokens: bool,
+
+ /// The endpoint which will be used to create new ACL entries via a PUT request.
+ #[prop_or(String::from("/access/acl"))]
+ #[builder(IntoPropValue, into_prop_value)]
+ acl_api_endpoint: String,
+
+ #[prop_or_default]
+ input_panel: InputPanel,
+}
+
+impl AclEdit {
+ /// Create a new `AclEdit` that takes as input a field and its label which are used to select
+ /// the ACL path for a new ACL entry.
+ pub fn new(
+ path_selector_label: impl Into<FieldLabel>,
+ path_selector: impl FieldBuilder,
+ ) -> Self {
+ let path_selector = path_selector.name("path").required(true);
+ let input_panel = InputPanel::new().with_field(path_selector_label, path_selector);
+ yew::props!(Self { input_panel })
+ }
+}
+
+impl From<AclEdit> for EditWindow {
+ fn from(value: AclEdit) -> Self {
+ let field = AuthidSelector::new().name("auth-id").required(true);
+
+ let (title, authid_label, authid_field) = if value.use_tokens {
+ (
+ tr!("API Token Permission"),
+ tr!("API Token"),
+ field.include_users(false),
+ )
+ } else {
+ (
+ tr!("User Permission"),
+ tr!("User"),
+ field.include_tokens(false),
+ )
+ };
+
+ let input_panel = value
+ .input_panel
+ .clone()
+ .padding(4)
+ .with_field(authid_label, authid_field)
+ .with_field(tr!("Role"), RoleSelector::new().name("role").required(true))
+ .with_field(
+ tr!("Propagate"),
+ Checkbox::new().name("propagate").required(true),
+ );
+
+ let url = value.acl_api_endpoint.to_owned();
+
+ let on_submit = {
+ let url = url.clone();
+ move |form_ctx: FormContext| {
+ let url = url.clone();
+ async move {
+ let data = form_ctx.get_submit_data();
+ crate::http_put(url.as_str(), Some(data)).await
+ }
+ }
+ };
+
+ EditWindow::new(title)
+ .renderer(move |_form_ctx: &FormContext| input_panel.clone().into())
+ .on_submit(on_submit)
+ }
+}
+
+impl AclEditWindow for AclEdit {}
diff --git a/src/acl/acl_view.rs b/src/acl/acl_view.rs
new file mode 100644
index 0000000..58da3fd
--- /dev/null
+++ b/src/acl/acl_view.rs
@@ -0,0 +1,270 @@
+use std::borrow::BorrowMut;
+use std::future::Future;
+use std::pin::Pin;
+use std::rc::Rc;
+
+use anyhow::Error;
+use indexmap::IndexMap;
+use serde_json::json;
+
+use yew::html::IntoPropValue;
+use yew::virtual_dom::{Key, VComp, VNode};
+
+use pwt::css;
+use pwt::prelude::*;
+use pwt::state::{Selection, Store};
+use pwt::widget::data_table::{DataTable, DataTableColumn, DataTableHeader};
+use pwt::widget::menu::{Menu, MenuButton, MenuItem};
+use pwt::widget::Toolbar;
+
+use pwt_macros::builder;
+
+use proxmox_access_control::types::{AclListItem, AclUgidType};
+
+use crate::percent_encoding::percent_encode_component;
+use crate::utils::render_boolean;
+use crate::{
+ ConfirmButton, EditWindow, LoadableComponent, LoadableComponentContext, LoadableComponentMaster,
+};
+
+use super::acl_edit::AclEditWindow;
+
+#[derive(PartialEq, Properties)]
+#[builder]
+pub struct AclView {
+ /// Show the ACL entries for the specified API path and sub-paths only.
+ #[builder(IntoPropValue, into_prop_value)]
+ #[prop_or_default]
+ acl_path: Option<AttrValue>,
+
+ /// Specifies the endpoint from which to fetch the ACL entries from via GET and to update them
+ /// via PUT requests.
+ #[builder(IntoPropValue, into_prop_value)]
+ #[prop_or(String::from("/access/acl"))]
+ acl_api_endpoint: String,
+
+ /// Menu entries for editing the ACL. The key is used as the menu label while the value should
+ /// be a tuple containing icon class and the dialog for editing ACL entries.
+ #[prop_or_default]
+ // using an index map here preserves the insertion order
+ edit_acl_menu: IndexMap<AttrValue, (Classes, EditWindow)>,
+}
+
+impl AclView {
+ pub fn new() -> Self {
+ yew::props!(Self {})
+ }
+
+ pub fn with_acl_edit_menu_entry(
+ mut self,
+ lable: impl Into<AttrValue>,
+ icon: impl Into<Classes>,
+ dialog: impl AclEditWindow,
+ ) -> Self {
+ self.add_acl_edit_menu_entry(lable, icon, dialog);
+ self
+ }
+
+ pub fn add_acl_edit_menu_entry(
+ &mut self,
+ lable: impl Into<AttrValue>,
+ icon: impl Into<Classes>,
+ dialog: impl AclEditWindow,
+ ) {
+ self.edit_acl_menu
+ .borrow_mut()
+ .insert(lable.into(), (icon.into(), dialog.into()));
+ }
+}
+
+impl Default for AclView {
+ fn default() -> Self {
+ AclView::new()
+ }
+}
+
+impl From<AclView> for VNode {
+ fn from(value: AclView) -> Self {
+ VComp::new::<LoadableComponentMaster<ProxmoxAclView>>(Rc::new(value), None).into()
+ }
+}
+
+#[derive(Clone, PartialEq)]
+enum ViewState {
+ AddAcl(AttrValue),
+}
+
+enum Msg {
+ Reload,
+ Remove,
+}
+
+struct ProxmoxAclView {
+ selection: Selection,
+ store: Store<AclListItem>,
+}
+
+impl ProxmoxAclView {
+ fn colmuns() -> Rc<Vec<DataTableHeader<AclListItem>>> {
+ Rc::new(vec![
+ DataTableColumn::new(tr!("Path"))
+ .flex(1)
+ .render(|item: &AclListItem| item.path.as_str().into())
+ .sorter(|a: &AclListItem, b: &AclListItem| a.path.cmp(&b.path))
+ .sort_order(true)
+ .into(),
+ DataTableColumn::new(tr!("User/Group/API Token"))
+ .flex(1)
+ .render(|item: &AclListItem| item.ugid.as_str().into())
+ .sorter(|a: &AclListItem, b: &AclListItem| a.ugid.cmp(&b.ugid))
+ .sort_order(true)
+ .into(),
+ DataTableColumn::new(tr!("Role"))
+ .flex(1)
+ .render(|item: &AclListItem| item.roleid.as_str().into())
+ .sorter(|a: &AclListItem, b: &AclListItem| a.roleid.cmp(&b.roleid))
+ .into(),
+ DataTableColumn::new(tr!("Propagate"))
+ .render(|item: &AclListItem| render_boolean(item.propagate).as_str().into())
+ .sorter(|a: &AclListItem, b: &AclListItem| a.propagate.cmp(&b.propagate))
+ .into(),
+ ])
+ }
+}
+
+impl LoadableComponent for ProxmoxAclView {
+ type Properties = AclView;
+ type Message = Msg;
+ type ViewState = ViewState;
+
+ fn create(ctx: &LoadableComponentContext<Self>) -> Self {
+ let link = ctx.link();
+ link.repeated_load(5000);
+
+ let selection = Selection::new().on_select(link.callback(|_| Msg::Reload));
+
+ let store = Store::with_extract_key(|record: &AclListItem| {
+ let acl_id = format!("{} for {} - {}", record.path, record.ugid, record.roleid);
+ Key::from(acl_id)
+ });
+
+ Self { selection, store }
+ }
+
+ fn load(
+ &self,
+ ctx: &LoadableComponentContext<Self>,
+ ) -> Pin<Box<dyn Future<Output = Result<(), Error>>>> {
+ let store = self.store.clone();
+ let url = &ctx.props().acl_api_endpoint;
+
+ let path = if let Some(path) = &ctx.props().acl_path {
+ format!("{url}&path={}", percent_encode_component(path))
+ } else {
+ url.to_owned()
+ };
+
+ Box::pin(async move {
+ let data = crate::http_get(&path, None).await?;
+ store.write().set_data(data);
+ Ok(())
+ })
+ }
+
+ fn toolbar(&self, ctx: &LoadableComponentContext<Self>) -> Option<Html> {
+ let selected_id = self.selection.selected_key().map(|k| k.to_string());
+ let disabled = selected_id.is_none();
+
+ let mut toolbar = Toolbar::new()
+ .class("pwt-w-100")
+ .class("pwt-overflow-hidden")
+ .border_bottom(true);
+
+ if !ctx.props().edit_acl_menu.is_empty() {
+ let add_menu = ctx.props().edit_acl_menu.iter().fold(
+ Menu::new(),
+ |add_menu, (label, (icon, _))| {
+ let msg = label.to_owned();
+
+ add_menu.with_item(
+ MenuItem::new(label.to_owned())
+ .icon_class(icon.to_owned())
+ .on_select(ctx.link().change_view_callback(move |_| {
+ Some(ViewState::AddAcl(msg.clone()))
+ })),
+ )
+ },
+ );
+
+ toolbar.add_child(MenuButton::new(tr!("Add")).show_arrow(true).menu(add_menu));
+ }
+
+ toolbar.add_child(
+ ConfirmButton::new(tr!("Remove ACL Entry"))
+ .confirm_message(tr!("Are you sure you want to remove this ACL entry?"))
+ .disabled(disabled)
+ .on_activate(ctx.link().callback(|_| Msg::Remove)),
+ );
+
+ Some(toolbar.into())
+ }
+
+ fn update(&mut self, ctx: &LoadableComponentContext<Self>, msg: Self::Message) -> bool {
+ match msg {
+ Msg::Reload => true,
+ Msg::Remove => {
+ if let Some(key) = self.selection.selected_key() {
+ if let Some(record) = self.store.read().lookup_record(&key).cloned() {
+ let link = ctx.link();
+ let url = ctx.props().acl_api_endpoint.to_owned();
+
+ link.clone().spawn(async move {
+ let data = match record.ugid_type {
+ AclUgidType::User => json!({
+ "delete": true,
+ "path": record.path,
+ "role": record.roleid,
+ "auth-id": record.ugid,
+ }),
+ AclUgidType::Group => json!({
+ "delete": true,
+ "path": record.path,
+ "role": record.roleid,
+ "group": record.ugid,
+ }),
+ };
+
+ match crate::http_put(url, Some(data)).await {
+ Ok(()) => link.send_reload(),
+ Err(err) => link.show_error("Removing ACL failed", err, true),
+ }
+ });
+ }
+ }
+ false
+ }
+ }
+ }
+
+ fn main_view(&self, _ctx: &LoadableComponentContext<Self>) -> Html {
+ DataTable::new(Self::colmuns(), self.store.clone())
+ .class(css::FlexFit)
+ .selection(self.selection.clone())
+ .into()
+ }
+
+ fn dialog_view(
+ &self,
+ ctx: &LoadableComponentContext<Self>,
+ view_state: &Self::ViewState,
+ ) -> Option<Html> {
+ match view_state {
+ ViewState::AddAcl(key) => ctx.props().edit_acl_menu.get(key).map(|(_, dialog)| {
+ dialog
+ .clone()
+ .on_done(ctx.link().change_view_callback(|_| None))
+ .into()
+ }),
+ }
+ }
+}
diff --git a/src/acl/mod.rs b/src/acl/mod.rs
new file mode 100644
index 0000000..cfe6aa6
--- /dev/null
+++ b/src/acl/mod.rs
@@ -0,0 +1,5 @@
+pub(crate) mod acl_edit;
+pub use acl_edit::AclEdit;
+
+pub(crate) mod acl_view;
+pub use acl_view::AclView;
diff --git a/src/lib.rs b/src/lib.rs
index 091cb72..3dacc20 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -25,6 +25,9 @@ pub use auth_edit_ldap::{AuthEditLDAP, ProxmoxAuthEditLDAP};
mod authid_selector;
pub use authid_selector::AuthidSelector;
+mod acl;
+pub use acl::{AclEdit, AclView};
+
mod bandwidth_selector;
pub use bandwidth_selector::{BandwidthSelector, ProxmoxBandwidthSelector};
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH yew-comp 3/3] role_selector/acl_edit: make api endpoint and default role configurable
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (5 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 2/3] acl: add a view and semi-generic `EditWindow` for acl entries Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH datacenter-manager 1/2] server: use proxmox-access-control api implementations Shannon Sterz
` (2 subsequent siblings)
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
and expose it via the AclEdit component too so that users can more
easily adapt the components to their needs.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
src/acl/acl_edit.rs | 22 +++++++++++++++++++++-
src/role_selector.rs | 20 ++++++++++++++++----
2 files changed, 37 insertions(+), 5 deletions(-)
diff --git a/src/acl/acl_edit.rs b/src/acl/acl_edit.rs
index b8bbd89..03421b0 100644
--- a/src/acl/acl_edit.rs
+++ b/src/acl/acl_edit.rs
@@ -24,6 +24,16 @@ pub struct AclEdit {
#[builder(IntoPropValue, into_prop_value)]
acl_api_endpoint: String,
+ /// The endpoint which will be used to create new ACL entries via a PUT request.
+ #[prop_or_default]
+ #[builder(IntoPropValue, into_prop_value)]
+ role_api_endpoint: Option<String>,
+
+ /// The role that will be pre-selected in the role selector, `NoAccess` by default.
+ #[prop_or_default]
+ #[builder(IntoPropValue, into_prop_value)]
+ default_role: Option<AttrValue>,
+
#[prop_or_default]
input_panel: InputPanel,
}
@@ -59,12 +69,22 @@ impl From<AclEdit> for EditWindow {
)
};
+ let mut role_selector = RoleSelector::new().name("role").required(true);
+
+ if let Some(role_api_endpoint) = value.role_api_endpoint {
+ role_selector.set_role_api_endpoint(role_api_endpoint);
+ }
+
+ if let Some(default_role) = value.default_role {
+ role_selector.set_default_role(default_role.clone());
+ }
+
let input_panel = value
.input_panel
.clone()
.padding(4)
.with_field(authid_label, authid_field)
- .with_field(tr!("Role"), RoleSelector::new().name("role").required(true))
+ .with_field(tr!("Role"), role_selector)
.with_field(
tr!("Propagate"),
Checkbox::new().name("propagate").required(true),
diff --git a/src/role_selector.rs b/src/role_selector.rs
index 20a8483..429f58c 100644
--- a/src/role_selector.rs
+++ b/src/role_selector.rs
@@ -1,5 +1,6 @@
use anyhow::format_err;
use std::rc::Rc;
+use yew::html::IntoPropValue;
use yew::virtual_dom::Key;
@@ -37,11 +38,22 @@ thread_local! {
}
use pwt::props::{FieldBuilder, WidgetBuilder};
-use pwt_macros::widget;
+use pwt_macros::{builder, widget};
#[widget(comp=ProxmoxRoleSelector, @input)]
#[derive(Clone, Properties, PartialEq)]
-pub struct RoleSelector {}
+#[builder]
+pub struct RoleSelector {
+ /// The default role.
+ #[builder(IntoPropValue, into_prop_value)]
+ #[prop_or(Some(AttrValue::from("NoAccess")))]
+ default_role: Option<AttrValue>,
+
+ /// The API endpoint from which to fetch the existing roles from.
+ #[builder(IntoPropValue, into_prop_value)]
+ #[prop_or(String::from("/access/roles"))]
+ role_api_endpoint: String,
+}
impl Default for RoleSelector {
fn default() -> Self {
@@ -104,8 +116,8 @@ impl Component for ProxmoxRoleSelector {
.with_std_props(&props.std_props)
.with_input_props(&props.input_props)
.required(true)
- .default("NoAccess")
- .loader("/access/roles")
+ .default(ctx.props().default_role.clone())
+ .loader(ctx.props().role_api_endpoint.clone())
.validate(self.validate.clone())
.into()
}
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH datacenter-manager 1/2] server: use proxmox-access-control api implementations
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (6 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH yew-comp 3/3] role_selector/acl_edit: make api endpoint and default role configurable Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-03 14:18 ` [pdm-devel] [PATCH datacenter-manager 2/2] ui: configuration: add panel for viewing and editing acl entries Shannon Sterz
2025-04-11 13:45 ` [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
instead of re-implementing most of the code here, take advantage of
the new `api` feature that allows for sharing the common api
implementations.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
server/Cargo.toml | 2 +-
server/src/acl.rs | 102 +++++++++-
server/src/api/access/acl.rs | 357 -----------------------------------
server/src/api/access/mod.rs | 4 +-
4 files changed, 100 insertions(+), 365 deletions(-)
delete mode 100644 server/src/api/access/acl.rs
diff --git a/server/Cargo.toml b/server/Cargo.toml
index 7b0058e..2d58cea 100644
--- a/server/Cargo.toml
+++ b/server/Cargo.toml
@@ -32,7 +32,7 @@ tokio-stream.workspace = true
tracing.workspace = true
url.workspace = true
-proxmox-access-control = { workspace = true, features = [ "impl" ] }
+proxmox-access-control = { workspace = true, features = [ "api" ] }
proxmox-async.workspace = true
proxmox-auth-api = { workspace = true, features = [ "api", "ticket", "pam-authenticator", "password-authenticator" ] }
proxmox-daemon.workspace = true
diff --git a/server/src/acl.rs b/server/src/acl.rs
index a99eede..52a1f97 100644
--- a/server/src/acl.rs
+++ b/server/src/acl.rs
@@ -1,7 +1,7 @@
use std::collections::HashMap;
use std::sync::OnceLock;
-use anyhow::{Context as _, Error};
+use anyhow::{format_err, Context as _, Error};
use proxmox_access_control::types::User;
use proxmox_auth_api::types::Authid;
@@ -10,7 +10,7 @@ use proxmox_section_config::SectionConfigData;
struct AccessControlConfig;
static PRIVILEGES: OnceLock<HashMap<&str, u64>> = OnceLock::new();
-static ROLES: OnceLock<HashMap<&str, u64>> = OnceLock::new();
+static ROLES: OnceLock<HashMap<&str, (u64, &str)>> = OnceLock::new();
impl proxmox_access_control::init::AccessControlConfig for AccessControlConfig {
fn privileges(&self) -> &HashMap<&str, u64> {
@@ -18,11 +18,11 @@ impl proxmox_access_control::init::AccessControlConfig for AccessControlConfig {
}
#[rustfmt::skip]
- fn roles(&self) -> &HashMap<&str, u64> {
+ fn roles(&self) -> &HashMap<&str, (u64, &str)> {
ROLES.get_or_init(|| {
[
- ("Administrator", pdm_api_types::ROLE_ADMINISTRATOR),
- ("Auditor", pdm_api_types::ROLE_AUDITOR),
+ ("Administrator", (pdm_api_types::ROLE_ADMINISTRATOR, "Administrators can inspect and modify the system.")),
+ ("Auditor", (pdm_api_types::ROLE_AUDITOR, "An Auditor can inspect many aspects of the system, but not change them.")),
//("SystemAdministrator", pdm_api_types::ROLE_SYS_ADMINISTRATOR),
//("SystemAuditor", pdm_api_types::ROLE_SYS_AUDITOR),
//("ResourceAdministrator", pdm_api_types::ROLE_RESOURCE_ADMINISTRATOR),
@@ -63,6 +63,98 @@ impl proxmox_access_control::init::AccessControlConfig for AccessControlConfig {
Ok(())
}
+
+ fn acl_audit_privileges(&self) -> u64 {
+ pdm_api_types::PRIV_ACCESS_AUDIT
+ }
+
+ fn acl_modify_privileges(&self) -> u64 {
+ pdm_api_types::PRIV_ACCESS_MODIFY
+ }
+
+ fn check_acl_path(&self, path: &str) -> Result<(), Error> {
+ let components = proxmox_access_control::acl::split_acl_path(path);
+
+ let components_len = components.len();
+
+ if components_len == 0 {
+ return Ok(());
+ }
+ match components[0] {
+ "access" => {
+ if components_len == 1 {
+ return Ok(());
+ }
+ match components[1] {
+ "acl" | "users" | "realm" => {
+ if components_len == 2 {
+ return Ok(());
+ }
+ }
+ _ => {}
+ }
+ }
+ "resource" => {
+ // `/resource` and `/resource/{remote}`
+ if components_len <= 2 {
+ return Ok(());
+ }
+ // `/resource/{remote-id}/{resource-type=guest,storage}/{resource-id}`
+ match components[2] {
+ "guest" | "storage" => {
+ // /resource/{remote-id}/{resource-type}
+ // /resource/{remote-id}/{resource-type}/{resource-id}
+ if components_len <= 4 {
+ return Ok(());
+ }
+ }
+ _ => {}
+ }
+ }
+ "system" => {
+ if components_len == 1 {
+ return Ok(());
+ }
+ match components[1] {
+ "certificates" | "disks" | "log" | "notifications" | "status" | "tasks"
+ | "time" => {
+ if components_len == 2 {
+ return Ok(());
+ }
+ }
+ "services" => {
+ // /system/services/{service}
+ if components_len <= 3 {
+ return Ok(());
+ }
+ }
+ "network" => {
+ if components_len == 2 {
+ return Ok(());
+ }
+ match components[2] {
+ "dns" => {
+ if components_len == 3 {
+ return Ok(());
+ }
+ }
+ "interfaces" => {
+ // /system/network/interfaces/{iface}
+ if components_len <= 4 {
+ return Ok(());
+ }
+ }
+ _ => {}
+ }
+ }
+ _ => {}
+ }
+ }
+ _ => {}
+ }
+
+ Err(format_err!("invalid acl path '{}'.", path))
+ }
}
pub(crate) fn init() {
diff --git a/server/src/api/access/acl.rs b/server/src/api/access/acl.rs
deleted file mode 100644
index a51899b..0000000
--- a/server/src/api/access/acl.rs
+++ /dev/null
@@ -1,357 +0,0 @@
-use anyhow::{bail, Error};
-
-use proxmox_access_control::acl::AclTreeNode;
-use proxmox_access_control::CachedUserInfo;
-use proxmox_config_digest::{ConfigDigest, PROXMOX_CONFIG_DIGEST_SCHEMA};
-use proxmox_router::{Permission, Router, RpcEnvironment};
-use proxmox_schema::api;
-
-use pdm_api_types::{
- AclListItem, AclUgidType, Authid, Role, ACL_PATH_SCHEMA, ACL_PROPAGATE_SCHEMA,
- PRIV_ACCESS_AUDIT, PRIV_ACCESS_MODIFY, PROXMOX_GROUP_ID_SCHEMA,
-};
-
-pub(super) const ROUTER: Router = Router::new()
- .get(&API_METHOD_READ_ACL)
- .put(&API_METHOD_UPDATE_ACL);
-
-// FIXME: copied from PBS
-fn extract_acl_node_data(
- node: &AclTreeNode,
- path: &str,
- list: &mut Vec<AclListItem>,
- exact: bool,
- auth_id_filter: &Option<Authid>,
-) {
- // tokens can't have tokens, so we can early return
- if let Some(auth_id_filter) = auth_id_filter {
- if auth_id_filter.is_token() {
- return;
- }
- }
-
- for (user, roles) in &node.users {
- if let Some(auth_id_filter) = auth_id_filter {
- if !user.is_token() || user.user() != auth_id_filter.user() {
- continue;
- }
- }
-
- for (role, propagate) in roles {
- list.push(AclListItem {
- path: if path.is_empty() {
- String::from("/")
- } else {
- path.to_string()
- },
- propagate: *propagate,
- ugid_type: AclUgidType::User,
- ugid: user.to_string(),
- roleid: role.to_string(),
- });
- }
- }
- for (group, roles) in &node.groups {
- if auth_id_filter.is_some() {
- continue;
- }
-
- for (role, propagate) in roles {
- list.push(AclListItem {
- path: if path.is_empty() {
- String::from("/")
- } else {
- path.to_string()
- },
- propagate: *propagate,
- ugid_type: AclUgidType::Group,
- ugid: group.to_string(),
- roleid: role.to_string(),
- });
- }
- }
- if exact {
- return;
- }
- for (comp, child) in &node.children {
- let new_path = format!("{}/{}", path, comp);
- extract_acl_node_data(child, &new_path, list, exact, auth_id_filter);
- }
-}
-
-#[api(
- input: {
- properties: {
- path: {
- schema: ACL_PATH_SCHEMA,
- optional: true,
- },
- exact: {
- description: "If set, returns only ACL for the exact path.",
- type: bool,
- optional: true,
- default: false,
- },
- },
- },
- returns: {
- description: "ACL entry list.",
- type: Array,
- items: {
- type: AclListItem,
- }
- },
- access: {
- permission: &Permission::Anybody,
- description: "Returns all ACLs if user has Sys.Audit on '/access/acl', or just the ACLs containing the user's API tokens.",
- },
-)]
-/// Read Access Control List (ACLs).
-fn read_acl(
- path: Option<String>,
- exact: bool,
- rpcenv: &mut dyn RpcEnvironment,
-) -> Result<Vec<AclListItem>, Error> {
- let auth_id = rpcenv.get_auth_id().unwrap().parse()?;
-
- let user_info = CachedUserInfo::new()?;
-
- let top_level_privs = user_info.lookup_privs(&auth_id, &["access", "acl"]);
- let auth_id_filter = if (top_level_privs & PRIV_ACCESS_AUDIT) == 0 {
- Some(auth_id)
- } else {
- None
- };
-
- let (mut tree, digest) = proxmox_access_control::acl::config()?;
-
- let mut list: Vec<AclListItem> = Vec::new();
- if let Some(path) = &path {
- if let Some(node) = &tree.find_node(path) {
- extract_acl_node_data(node, path, &mut list, exact, &auth_id_filter);
- }
- } else {
- extract_acl_node_data(&tree.root, "", &mut list, exact, &auth_id_filter);
- }
-
- rpcenv["digest"] = hex::encode(digest).into();
-
- Ok(list)
-}
-
-#[api(
- protected: true,
- input: {
- properties: {
- path: {
- schema: ACL_PATH_SCHEMA,
- },
- role: {
- type: Role,
- },
- propagate: {
- optional: true,
- schema: ACL_PROPAGATE_SCHEMA,
- },
- "auth-id": {
- optional: true,
- type: Authid,
- },
- group: {
- optional: true,
- schema: PROXMOX_GROUP_ID_SCHEMA,
- },
- delete: {
- optional: true,
- description: "Remove permissions (instead of adding it).",
- type: bool,
- default: false,
- },
- digest: {
- optional: true,
- schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
- },
- },
- },
- access: {
- permission: &Permission::Anybody,
- description: "Requires Permissions.Modify on '/access/acl', limited to updating ACLs of the user's API tokens otherwise."
- },
-)]
-/// Update Access Control List (ACLs).
-#[allow(clippy::too_many_arguments)]
-fn update_acl(
- path: String,
- role: String,
- propagate: Option<bool>,
- auth_id: Option<Authid>,
- group: Option<String>,
- delete: bool,
- digest: Option<ConfigDigest>,
- rpcenv: &mut dyn RpcEnvironment,
-) -> Result<(), Error> {
- let current_auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
-
- let user_info = CachedUserInfo::new()?;
-
- let top_level_privs = user_info.lookup_privs(¤t_auth_id, &["access", "acl"]);
- if top_level_privs & PRIV_ACCESS_MODIFY == 0 {
- if group.is_some() {
- bail!("Unprivileged users are not allowed to create group ACL item.");
- }
-
- match &auth_id {
- Some(auth_id) => {
- if current_auth_id.is_token() {
- bail!("Unprivileged API tokens can't set ACL items.");
- } else if !auth_id.is_token() {
- bail!("Unprivileged users can only set ACL items for API tokens.");
- } else if auth_id.user() != current_auth_id.user() {
- bail!("Unprivileged users can only set ACL items for their own API tokens.");
- }
- }
- None => {
- bail!("Unprivileged user needs to provide auth_id to update ACL item.");
- }
- };
- }
-
- let _lock = proxmox_access_control::acl::lock_config()?;
-
- let (mut tree, expected_digest) = proxmox_access_control::acl::config()?;
-
- expected_digest.detect_modification(digest.as_ref())?;
-
- let propagate = propagate.unwrap_or(true);
-
- if let Some(ref _group) = group {
- bail!("parameter 'group' - groups are currently not supported.");
- } else if let Some(ref auth_id) = auth_id {
- if !delete {
- // Note: we allow to delete non-existent users
- let user_cfg = proxmox_access_control::user::cached_config()?;
- if !user_cfg.sections.contains_key(&auth_id.to_string()) {
- bail!(format!(
- "no such {}.",
- if auth_id.is_token() {
- "API token"
- } else {
- "user"
- }
- ));
- }
- }
- } else {
- bail!("missing 'userid' or 'group' parameter.");
- }
-
- if !delete {
- // Note: we allow to delete entries with invalid path
- check_acl_path(&path)?;
- }
-
- if let Some(auth_id) = auth_id {
- if delete {
- tree.delete_user_role(&path, &auth_id, &role);
- } else {
- tree.insert_user_role(&path, &auth_id, &role, propagate);
- }
- } else if let Some(group) = group {
- if delete {
- tree.delete_group_role(&path, &group, &role);
- } else {
- tree.insert_group_role(&path, &group, &role, propagate);
- }
- }
-
- proxmox_access_control::acl::save_config(&tree)?;
-
- Ok(())
-}
-///
-/// Check whether a given ACL `path` conforms to the expected schema.
-///
-/// Currently this just checks for the number of components for various sub-trees.
-fn check_acl_path(path: &str) -> Result<(), Error> {
- let components = proxmox_access_control::acl::split_acl_path(path);
-
- let components_len = components.len();
-
- if components_len == 0 {
- return Ok(());
- }
- match components[0] {
- "access" => {
- if components_len == 1 {
- return Ok(());
- }
- match components[1] {
- "acl" | "users" | "realm" => {
- if components_len == 2 {
- return Ok(());
- }
- }
- _ => {}
- }
- }
- "resource" => {
- // `/resource` and `/resource/{remote}`
- if components_len <= 2 {
- return Ok(());
- }
- // `/resource/{remote-id}/{resource-type=guest,storage}/{resource-id}`
- match components[2] {
- "guest" | "storage" => {
- // /resource/{remote-id}/{resource-type}
- // /resource/{remote-id}/{resource-type}/{resource-id}
- if components_len <= 4 {
- return Ok(());
- }
- }
- _ => {}
- }
- }
- "system" => {
- if components_len == 1 {
- return Ok(());
- }
- match components[1] {
- "certificates" | "disks" | "log" | "notifications" | "status" | "tasks"
- | "time" => {
- if components_len == 2 {
- return Ok(());
- }
- }
- "services" => {
- // /system/services/{service}
- if components_len <= 3 {
- return Ok(());
- }
- }
- "network" => {
- if components_len == 2 {
- return Ok(());
- }
- match components[2] {
- "dns" => {
- if components_len == 3 {
- return Ok(());
- }
- }
- "interfaces" => {
- // /system/network/interfaces/{iface}
- if components_len <= 4 {
- return Ok(());
- }
- }
- _ => {}
- }
- }
- _ => {}
- }
- }
- _ => {}
- }
-
- bail!("invalid acl path '{}'.", path);
-}
diff --git a/server/src/api/access/mod.rs b/server/src/api/access/mod.rs
index befe2f1..345b22f 100644
--- a/server/src/api/access/mod.rs
+++ b/server/src/api/access/mod.rs
@@ -13,19 +13,19 @@ use proxmox_sortable_macro::sortable;
use pdm_api_types::{Authid, ACL_PATH_SCHEMA, PRIVILEGES, PRIV_ACCESS_AUDIT};
-mod acl;
mod domains;
mod tfa;
mod users;
#[sortable]
const SUBDIRS: SubdirMap = &sorted!([
- ("acl", &acl::ROUTER),
+ ("acl", &proxmox_access_control::api::ACL_ROUTER),
("domains", &domains::ROUTER),
(
"permissions",
&Router::new().get(&API_METHOD_LIST_PERMISSIONS)
),
+ ("roles", &proxmox_access_control::api::ROLE_ROUTER),
("tfa", &tfa::ROUTER),
(
"ticket",
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* [pdm-devel] [PATCH datacenter-manager 2/2] ui: configuration: add panel for viewing and editing acl entries
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (7 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH datacenter-manager 1/2] server: use proxmox-access-control api implementations Shannon Sterz
@ 2025-04-03 14:18 ` Shannon Sterz
2025-04-11 13:45 ` [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-03 14:18 UTC (permalink / raw)
To: pdm-devel
using the new generic acl viewing and editing components from
proxmox-yew-comp make it possible for users to edit acl entries
note that the path selector is based on the current definition of
acceptable paths in the `AccessControlConfig`, since those are the
only valid paths. however, the convenience could be improved by, for
example, adding entries for specific resources. however, the acl paths
here don't seem fixed yet, so leave this as-is for now.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
ui/src/configuration/mod.rs | 23 ++++-
.../configuration/permission_path_selector.rs | 88 +++++++++++++++++++
2 files changed, 110 insertions(+), 1 deletion(-)
create mode 100644 ui/src/configuration/permission_path_selector.rs
diff --git a/ui/src/configuration/mod.rs b/ui/src/configuration/mod.rs
index ca16685..2a8b991 100644
--- a/ui/src/configuration/mod.rs
+++ b/ui/src/configuration/mod.rs
@@ -1,3 +1,4 @@
+use permission_path_selector::PermissionPathSelector;
use pwt::prelude::*;
use pwt::props::StorageLocation;
use pwt::state::NavigationContainer;
@@ -6,8 +7,9 @@ use pwt::widget::{Container, MiniScrollMode, Panel, TabBarItem, TabPanel};
use proxmox_yew_comp::configuration::TimePanel;
use proxmox_yew_comp::configuration::{DnsPanel, NetworkView};
use proxmox_yew_comp::tfa::TfaView;
-use proxmox_yew_comp::UserPanel;
+use proxmox_yew_comp::{AclEdit, AclView, UserPanel};
+mod permission_path_selector;
mod webauthn;
pub use webauthn::WebauthnPanel;
@@ -40,6 +42,8 @@ pub fn system_configuration() -> Html {
#[function_component(AccessControl)]
pub fn access_control() -> Html {
+ let acl_edit = AclEdit::new(tr!("Path"), PermissionPathSelector::new()).default_role("Auditor");
+
let panel = TabPanel::new()
.state_id(StorageLocation::session("AccessControlState"))
//.title(tr!("Configuration") + ": " + &tr!("Access Control"))
@@ -71,6 +75,23 @@ pub fn access_control() -> Html {
.with_child(TfaView::new())
.into()
},
+ )
+ .with_item_builder(
+ TabBarItem::new()
+ .key("permissions")
+ .icon_class("fa fa-unlock")
+ .label(tr!("Permissions")),
+ move |_| {
+ Container::new()
+ .class("pwt-content-spacer")
+ .class(pwt::css::FlexFit)
+ .with_child(AclView::new().with_acl_edit_menu_entry(
+ tr!("User Permission"),
+ "fa fa-fw fa-user",
+ acl_edit.clone().use_tokens(false),
+ ))
+ .into()
+ },
);
NavigationContainer::new().with_child(panel).into()
diff --git a/ui/src/configuration/permission_path_selector.rs b/ui/src/configuration/permission_path_selector.rs
new file mode 100644
index 0000000..68e38a9
--- /dev/null
+++ b/ui/src/configuration/permission_path_selector.rs
@@ -0,0 +1,88 @@
+use std::rc::Rc;
+
+use yew::html::IntoPropValue;
+
+use pwt::widget::form::Combobox;
+use pwt::{prelude::*, AsyncPool};
+
+use pwt_macros::{builder, widget};
+
+static PREDEFINED_PATHS: &[&str] = &[
+ "/",
+ "/access",
+ "/access/acl",
+ "/access/users",
+ "/access/domains",
+ "/access/openid",
+ "/resource",
+ "/system",
+ "/system/certificates",
+ "/system/disks",
+ "/system/log",
+ "/system/notifications",
+ "/system/network",
+ "/system/network/dns",
+ "/system/network/interfaces",
+ "/system/services",
+ "/system/status",
+ "/system/tasks",
+ "/system/time",
+ "/system/services",
+];
+
+#[widget(comp=PdmPermissionPathSelector, @input, @element)]
+#[derive(Clone, PartialEq, Properties)]
+#[builder]
+pub struct PermissionPathSelector {
+ /// Default value
+ #[builder(IntoPropValue, into_prop_value)]
+ #[prop_or_default]
+ default: Option<AttrValue>,
+}
+
+impl PermissionPathSelector {
+ pub(super) fn new() -> Self {
+ yew::props!(Self {})
+ }
+}
+
+enum Msg {}
+
+struct PdmPermissionPathSelector {
+ items: Rc<Vec<AttrValue>>,
+}
+
+impl PdmPermissionPathSelector {}
+
+impl Component for PdmPermissionPathSelector {
+ type Message = Msg;
+ type Properties = PermissionPathSelector;
+
+ fn create(_ctx: &Context<Self>) -> Self {
+ // TODO: fetch resources & remotes from the backend to improve the pre-defined selection of
+ // acl paths
+ Self {
+ items: Rc::new(
+ PREDEFINED_PATHS
+ .iter()
+ .map(|i| AttrValue::from(*i))
+ .collect(),
+ ),
+ }
+ }
+
+ fn update(&mut self, _ctx: &Context<Self>, _msg: Self::Message) -> bool {
+ false
+ }
+
+ fn view(&self, ctx: &Context<Self>) -> Html {
+ let props = ctx.props();
+ Combobox::new()
+ .with_std_props(&props.std_props)
+ .with_input_props(&props.input_props)
+ .default(props.default.clone())
+ .items(Rc::clone(&self.items))
+ .editable(true)
+ .into()
+ }
+}
--
2.39.5
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components
2025-04-03 14:17 [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp 0/9] ACL edit api and ui components Shannon Sterz
` (8 preceding siblings ...)
2025-04-03 14:18 ` [pdm-devel] [PATCH datacenter-manager 2/2] ui: configuration: add panel for viewing and editing acl entries Shannon Sterz
@ 2025-04-11 13:45 ` Shannon Sterz
9 siblings, 0 replies; 20+ messages in thread
From: Shannon Sterz @ 2025-04-11 13:45 UTC (permalink / raw)
To: Shannon Sterz, pdm-devel
Superseeded-by: https://lore.proxmox.com/pdm-devel/20250411134435.269524-2-s.sterz@proxmox.com/T/#t
On Thu Apr 3, 2025 at 4:17 PM CEST, Shannon Sterz wrote:
> this series aims to make more parts of our access control list
> implementation re-usable between products. in a first step most of the
> relevant api endpoints and api types are moved to
> `proxmox-access-control`. this is done by adding a new `api` feature
> that includes the necessary api endpoints. the `AccessControlConfig`
> trait is also expanded to make the api endpoints more adaptable to
> different products. by providing default implementations for the newly
> added trait functions existing users don't need to change anything.
>
> next the series adds components to proxmox-yew-comp to provide a panel
> for inspecting the current acl and adding or removing entries. this is
> done by using the existing `RoleSelector` and `AuthidSelector`
> components. the later is also slightly adapted to make it possible to
> change the api endpoint that roles are fetched from as well as the
> default role. the `AclView` component allows users of the crate to add
> more options for adding ACL entries. meaning they can configure distinct
> components for adding user, token or group permissions. this is done in
> a generic fashion so that extending this menu does not require changing
> the component again.
>
> finally proxmox-datacenter-manager is adapted to use the new api
> endpoints in `proxmox-access-control` and a permissions panel is
> implemented. note that this would benefit from some clean-up once
> permission path and such are cleaned up.
>
> proxmox:
>
> Shannon Sterz (4):
> access-control: add more types to prepare for api feature
> access-control: add acl api feature
> access-control: add comments to roles function of AccessControlConfig
> access-control: add generic roles endpoint to `api` feature
>
> proxmox-access-control/Cargo.toml | 8 +
> proxmox-access-control/src/acl.rs | 12 +-
> proxmox-access-control/src/api.rs | 321 ++++++++++++++++++
> .../src/cached_user_info.rs | 4 +-
> proxmox-access-control/src/init.rs | 27 +-
> proxmox-access-control/src/lib.rs | 3 +
> proxmox-access-control/src/types.rs | 87 ++++-
> 7 files changed, 450 insertions(+), 12 deletions(-)
> create mode 100644 proxmox-access-control/src/api.rs
>
>
> proxmox-yew-comp:
>
> Shannon Sterz (3):
> api-types/role_selector: depend on common `RoleInfo` type
> acl: add a view and semi-generic `EditWindow` for acl entries
> role_selector/acl_edit: make api endpoint and default role
> configurable
>
> src/acl/acl_edit.rs | 112 +++++++++++++++++
> src/acl/acl_view.rs | 270 ++++++++++++++++++++++++++++++++++++++++
> src/acl/mod.rs | 5 +
> src/common_api_types.rs | 8 --
> src/lib.rs | 3 +
> src/role_selector.rs | 22 +++-
> 6 files changed, 407 insertions(+), 13 deletions(-)
> create mode 100644 src/acl/acl_edit.rs
> create mode 100644 src/acl/acl_view.rs
> create mode 100644 src/acl/mod.rs
>
>
> proxmox-datacenter-manager:
>
> Shannon Sterz (2):
> server: use proxmox-access-control api implementations
> ui: configuration: add panel for viewing and editing acl entries
>
> server/Cargo.toml | 2 +-
> server/src/acl.rs | 102 ++++-
> server/src/api/access/acl.rs | 357 ------------------
> server/src/api/access/mod.rs | 4 +-
> ui/src/configuration/mod.rs | 23 +-
> .../configuration/permission_path_selector.rs | 88 +++++
> 6 files changed, 210 insertions(+), 366 deletions(-)
> delete mode 100644 server/src/api/access/acl.rs
> create mode 100644 ui/src/configuration/permission_path_selector.rs
>
>
> Summary over all repositories:
> 19 files changed, 1067 insertions(+), 391 deletions(-)
>
> --
> Generated by git-murpp 0.8.0
_______________________________________________
pdm-devel mailing list
pdm-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pdm-devel
^ permalink raw reply [flat|nested] 20+ messages in thread