Re: [pdm-devel] [PATCH datacenter-manager v3 05/11] api: resources: list: add support for view parameter

["Lukas Wagner" <l.wagner@proxmox.com>]

On Tue Nov 11, 2025 at 3:31 PM CET, Michael Köppl wrote:
> 2 comments inline
>
> On Thu Nov 6, 2025 at 2:43 PM CET, Lukas Wagner wrote:
>> A view allows one to get filtered subset of all resources, based on
>> filter rules defined in a config file. View integrate with the
>> permission system - if a user has permissions on /view/{view-id}, then
>> these privileges are transitively applied to all resources which are
>> matched by the rules. All other permission checks are replaced if
>> requesting data through a view.
>>
>> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>> ---
>>  server/src/api/resources.rs  | 56 ++++++++++++++++++++++++++++++------
>>  server/src/resource_cache.rs |  3 +-
>>  2 files changed, 50 insertions(+), 9 deletions(-)
>>
>> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
>> index dad3e6b6..1b3bfda2 100644
>> --- a/server/src/api/resources.rs
>> +++ b/server/src/api/resources.rs
>> @@ -18,7 +18,7 @@ use pdm_api_types::resource::{
>>  use pdm_api_types::subscription::{
>>      NodeSubscriptionInfo, RemoteSubscriptionState, RemoteSubscriptions, SubscriptionLevel,
>>  };
>> -use pdm_api_types::{Authid, PRIV_RESOURCE_AUDIT};
>> +use pdm_api_types::{Authid, PRIV_RESOURCE_AUDIT, VIEW_ID_SCHEMA};
>>  use pdm_search::{Search, SearchTerm};
>>  use proxmox_access_control::CachedUserInfo;
>>  use proxmox_router::{
>> @@ -30,8 +30,8 @@ use proxmox_sortable_macro::sortable;
>>  use proxmox_subscription::SubscriptionStatus;
>>  use pve_api_types::{ClusterResource, ClusterResourceType};
>>  
>> -use crate::connection;
>>  use crate::metric_collection::top_entities;
>> +use crate::{connection, views};
>>  
>>  pub const ROUTER: Router = Router::new()
>>      .get(&list_subdirs_api_method!(SUBDIRS))
>> @@ -221,6 +221,10 @@ impl From<RemoteWithResources> for RemoteResources {
>>                  type: ResourceType,
>>                  optional: true,
>>              },
>> +            view: {
>> +                schema: VIEW_ID_SCHEMA,
>> +                optional: true,
>> +            },
>>          }
>>      },
>>      returns: {
>> @@ -236,10 +240,17 @@ pub async fn get_resources(
>>      max_age: u64,
>>      resource_type: Option<ResourceType>,
>>      search: Option<String>,
>> +    view: Option<String>,
>>      rpcenv: &mut dyn RpcEnvironment,
>>  ) -> Result<Vec<RemoteResources>, Error> {
>> -    let remotes_with_resources =
>> -        get_resources_impl(max_age, search, resource_type, Some(rpcenv)).await?;
>> +    let remotes_with_resources = get_resources_impl(
>> +        max_age,
>> +        search,
>> +        resource_type,
>> +        view.as_deref(),
>> +        Some(rpcenv),
>> +    )
>> +    .await?;
>>      let resources = remotes_with_resources.into_iter().map(Into::into).collect();
>>      Ok(resources)
>>  }
>> @@ -276,6 +287,7 @@ pub(crate) async fn get_resources_impl(
>>      max_age: u64,
>>      search: Option<String>,
>>      resource_type: Option<ResourceType>,
>> +    view: Option<&str>,
>>      rpcenv: Option<&mut dyn RpcEnvironment>,
>>  ) -> Result<Vec<RemoteWithResources>, Error> {
>>      let user_info = CachedUserInfo::new()?;
>> @@ -285,9 +297,15 @@ pub(crate) async fn get_resources_impl(
>>              .get_auth_id()
>>              .ok_or_else(|| format_err!("no authid available"))?
>>              .parse()?;
>> -        if !user_info.any_privs_below(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT)? {
>> +
>> +        // NOTE: Assumption is that the regular permission check is completely replaced by a check
>> +        // on the view ACL object *if* a view parameter is passed.
>> +        if let Some(view) = &view {
>> +            user_info.check_privs(&auth_id, &["view", view], PRIV_RESOURCE_AUDIT, false)?;
>> +        } else if !user_info.any_privs_below(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT)? {
>>              http_bail!(FORBIDDEN, "user has no access to resources");
>>          }
>> +
>>          opt_auth_id = Some(auth_id);
>>      }
>>  
>> @@ -296,12 +314,22 @@ pub(crate) async fn get_resources_impl(
>>  
>>      let filters = search.map(Search::from).unwrap_or_default();
>>  
>> +    let view = views::get_optional_view(view.as_deref())?;
>
> Is this .as_deref() needed? `view` already is a Option<&str>.
>

Good catch!

>> +
>>      let remotes_only = is_remotes_only(&filters);
>>  
>>      for (remote_name, remote) in remotes_config {
>>          if let Some(ref auth_id) = opt_auth_id {
>> -            let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
>> -            if remote_privs & PRIV_RESOURCE_AUDIT == 0 {
>> +            if view.is_none() {
>> +                let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
>> +                if remote_privs & PRIV_RESOURCE_AUDIT == 0 {
>> +                    continue;
>> +                }
>> +            }
>> +        }
>> +
>> +        if let Some(view) = &view {
>> +            if view.can_skip_remote(&remote_name) {
>>                  continue;
>>              }
>>          }
>
> Wouldn't it make more sense to switch the two if-blocks above?
>
>     if let Some(view) = &view {
>         if view.can_skip_remote(&remote_name) {
>             continue;
>         }
>     }
>
>     if let Some(ref auth_id) = opt_auth_id {
>         if view.is_none() {
>             let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
>             if remote_privs & PRIV_RESOURCE_AUDIT == 0 {
>                 continue;
>             }
>         }
>     }
>
> Then view filtering takes precedence over checking the permissions, same
> as in get_subscription_status. Might be that I'm misinterpreting this,
> though.
>

I think the order does not really make a difference, no? Sine the
regular check is gated by the `if view.is_none()`.

Anyways, I changed it to the following, since it is a bit nicer to read:

        if let Some(view) = &view {
            if view.can_skip_remote(&remote_name) {
                continue;
            }
        } else if let Some(ref auth_id) = opt_auth_id {
            let remote_privs = user_info.lookup_privs(auth_id, &["resource", &remote_name]);
            if remote_privs & PRIV_RESOURCE_AUDIT == 0 {
                continue;
            }
        }





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