Re: [pdm-devel] [PATCH datacenter-manager v3 08/11] api: subscription status: add support for view parameter

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

On Tue Nov 11, 2025 at 3:46 PM CET, Michael Köppl wrote:
> 1 comment 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. Views 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>
>> ---
>>
>> Notes:
>>     Changes since v2:
>>       - make sure to not filter out a remote if it has been explicitly
>>         included
>>
>>  server/src/api/resources.rs | 66 ++++++++++++++++++++++++++++++-------
>>  1 file changed, 54 insertions(+), 12 deletions(-)
>>
>> diff --git a/server/src/api/resources.rs b/server/src/api/resources.rs
>> index 143335fe..2930da8b 100644
>> --- a/server/src/api/resources.rs
>> +++ b/server/src/api/resources.rs
>> @@ -552,6 +552,10 @@ pub async fn get_status(
>>                  default: false,
>>                  description: "If true, includes subscription information per node (with enough privileges)",
>>              },
>> +            view: {
>> +                schema: VIEW_ID_SCHEMA,
>> +                optional: true,
>> +            },
>>          },
>>      },
>>      returns: {
>> @@ -566,6 +570,7 @@ pub async fn get_status(
>>  pub async fn get_subscription_status(
>>      max_age: u64,
>>      verbose: bool,
>> +    view: Option<String>,
>>      rpcenv: &mut dyn RpcEnvironment,
>>  ) -> Result<Vec<RemoteSubscriptions>, Error> {
>>      let (remotes_config, _) = pdm_config::remotes::config()?;
>> @@ -574,9 +579,17 @@ pub async fn get_subscription_status(
>>
>>      let auth_id = rpcenv.get_auth_id().unwrap().parse()?;
>>      let user_info = CachedUserInfo::new()?;
>> -    let allow_all = user_info
>> -        .check_privs(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT, false)
>> -        .is_ok();
>> +
>> +    let allow_all = if let Some(view) = &view {
>> +        user_info.check_privs(&auth_id, &["view", view], PRIV_RESOURCE_AUDIT, false)?;
>> +        false
>> +    } else {
>> +        user_info
>> +            .check_privs(&auth_id, &["resource"], PRIV_RESOURCE_AUDIT, false)
>> +            .is_ok()
>> +    };
>> +
>> +    let view = views::get_optional_view(view.as_deref())?;
>>
>>      let check_priv = |remote_name: &str| -> bool {
>>          user_info
>> @@ -590,35 +603,64 @@ pub async fn get_subscription_status(
>>      };
>>
>>      for (remote_name, remote) in remotes_config {
>> -        if !allow_all && !check_priv(&remote_name) {
>> +        if let Some(view) = &view {
>> +            if view.can_skip_remote(&remote_name) {
>> +                continue;
>> +            }
>> +        } else if !allow_all && !check_priv(&remote_name) {
>>              continue;
>>          }
>>
>> +        let view = view.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(view) = &view {
>> +                                view.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 let Some(view) = view {
>> +                    if error.is_some() && !view.is_remote_explicitly_included(&remote.id) {
>
> Can this ever be be true? From the code above, you can never have the
> case that node_status and error are both Some(...) at the same time, so
> error.is_some() would always evaluate to false here, unless I'm missing
> something.
>
> I think you'd have to put the check outside of this block.
>
>     if let Some(view) = &view {
>         if error.is_some() && !view.is_remote_explicitly_included(&remote.id) {
>             return None;
>         }
>     }
>
>     let state = if let Some(node_status) = &node_status {
>         ...
>     }
>

i thikn this is right, at least i noted something similar in a previous
version of this patch series already. thanks for spotting it again!

https://lore.proxmox.com/all/DDVMQ0KGW0IP.50W15XZS8TQ@proxmox.com/

>> +                        // Don't leak the existence of failed remotes unless they were explicitly
>> +                        // pulled in by a `include remote:<id>` rule.
>> +                        return None;
>> +                    }
>> +                }
>>
>> -            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



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