Re: [pdm-devel] [PATCH datacenter-manager 10/13] api: subscription status: add support for view-filter parameter

["Shannon Sterz" <s.sterz@proxmox.com>]

On Wed Oct 29, 2025 at 3:48 PM CET, Lukas Wagner wrote:
> A view filter allows one to get filtered subset of all resources, based
> on filter rules defined in a config file. View filters integrate with
> the permission system - if a user has permissions on
> /view/{view-filter-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 filter.
>
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
>  server/src/api/resources.rs | 58 +++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
>
> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
> index 61960295..89b84c3c 100644
> --- a/server/src/api/resources.rs
> +++ b/server/src/api/resources.rs
> @@ -546,6 +546,10 @@ pub async fn get_status(
>                  default: false,
>                  description: "If true, includes subscription information per node (with enough privileges)",
>              },
> +            "view-filter": {
> +                schema: VIEW_FILTER_ID_SCHEMA,
> +                optional: true,
> +            },
>          },
>      },
>      returns: {
> @@ -560,6 +564,7 @@ pub async fn get_status(
>  pub async fn get_subscription_status(
>      max_age: u64,
>      verbose: bool,
> +    view_filter: Option<String>,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<Vec<RemoteSubscriptions>, Error> {
>      let (remotes_config, _) = pdm_config::remotes::config()?;
> @@ -572,6 +577,14 @@ pub async fn get_subscription_status(
>          .check_privs(&auth_id, &["resources"], PRIV_RESOURCE_AUDIT, false)
>          .is_ok();
>
> +    if let Some(view_filter) = &view_filter {
> +        user_info.check_privs(&auth_id, &["view", view_filter], PRIV_RESOURCE_AUDIT, false)?;
> +    }

this is minor, but maybe consider moving the view check before the
`allow_all` check and skipping the `allow_all` check (by setting it to
false) if a view was found. that should safe us an unecessary traversal
of the acl tree here.

i think wolfgang already noted that tho.

> +
> +    let view_filter = view_filter
> +        .map(|filter_name| views::view_filter::get_view_filter(&filter_name))
> +        .transpose()?;
> +
>      let check_priv = |remote_name: &str| -> bool {
>          user_info
>              .check_privs(
> @@ -584,35 +597,62 @@ pub async fn get_subscription_status(
>      };
>
>      for (remote_name, remote) in remotes_config {
> -        if !allow_all && !check_priv(&remote_name) {
> +        if let Some(filter) = &view_filter {
> +            if filter.can_skip_remote(&remote_name) {
> +                continue;
> +            }
> +        } else if !allow_all && !check_priv(&remote_name) {
>              continue;
>          }
>
> +        let view_filter_clone = view_filter.clone();
> +
>          let future = async move {
>              let (node_status, error) =
>                  match get_subscription_info_for_remote(&remote, max_age).await {
> -                    Ok(node_status) => (Some(node_status), None),
> +                    Ok(mut node_status) => {
> +                        node_status.retain(|node, _| {
> +                            if let Some(filter) = &view_filter_clone {
> +                                filter.is_node_included(&remote.id, node)
> +                            } else {
> +                                true
> +                            }
> +                        });
> +                        (Some(node_status), None)
> +                    }
>                      Err(error) => (None, Some(error.to_string())),
>                  };
>
> -            let mut state = RemoteSubscriptionState::Unknown;
> +            let state = if let Some(node_status) = &node_status {
> +                if error.is_some() {
> +                    // Don't leak the existence of failed remotes, since we cannot apply
> +                    // view-filters here.
> +                    return None;

shouldn't this be gated by checking if view_filters was defined?
otherwise we now return nothing every time we get an error?

also correct me if i'm wrong, but isn't this `return None;` unreachable?
error is assigned `Some` iff the `get_subscription_status` call above
returns an `Err`, but in that case `node_status` is set to `None`, so
the `if let Some(node_status) = &node_status` above should be false?

> +                }
>
> -            if let Some(node_status) = &node_status {
> -                state = map_node_subscription_list_to_state(node_status);
> -            }
> +                if node_status.is_empty() {
> +                    return None;
> +                }
>
> -            RemoteSubscriptions {
> +                map_node_subscription_list_to_state(node_status)
> +            } else {
> +                RemoteSubscriptionState::Unknown
> +            };
> +
> +            Some(RemoteSubscriptions {
>                  remote: remote_name,
>                  error,
>                  state,
>                  node_status: if verbose { node_status } else { None },
> -            }
> +            })
>          };
>
>          futures.push(future);
>      }
>
> -    Ok(join_all(futures).await)
> +    let status = join_all(futures).await.into_iter().flatten().collect();
> +
> +    Ok(status)
>  }
>
>  // FIXME: make timeframe and count parameters?



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