Re: [RFC PATCH datacenter-manager 2/3] ui: acl: list granular level permission paths for views

Shannon Sterz
2026-03-26T11:16:17Z

some comments in-line.

On Wed Mar 25, 2026 at 4:35 PM CET, Shan Shaji wrote:
> Previously, users were unable to assign permissions to individual views.
> Resolved this by loading the views list and extending permission paths
> to include specific view routes.
>
> Signed-off-by: Shan Shaji <s.shaji@proxmox.com>
> ---
> .../configuration/permission_path_selector.rs | 80 +++++++++++++++----
> 1 file changed, 64 insertions(+), 16 deletions(-)
>
> diff --git a/ui/src/configuration/permission_path_selector.rs b/ui/src/configuration/permission_path_selector.rs
> index 59bdabb..a0fec86 100644
> --- a/ui/src/configuration/permission_path_selector.rs
> +++ b/ui/src/configuration/permission_path_selector.rs
> @@ -1,12 +1,15 @@
> use std::rc::Rc;
>
> +use anyhow::Error;
> use yew::html::IntoPropValue;
>
> -use pwt::prelude::*;
> use pwt::widget::form::Combobox;
> +use pwt::{prelude::*, AsyncPool};
>
> use pwt_macros::{builder, widget};
>
> +use crate::pdm_client;
> +
> static PREDEFINED_PATHS: &[&str] = &[
> "/",
> "/access",
> @@ -44,33 +47,78 @@ impl PermissionPathSelector {
> }
> }
>
> -enum Msg {}
> +enum Msg {
> + Prefetched(Vec<String>),
> + PrefetchFailed,
> +}
>
> struct PdmPermissionPathSelector {
> items: Rc<Vec<AttrValue>>,
> + async_pool: AsyncPool,
> }
>
> -impl PdmPermissionPathSelector {}
> +impl PdmPermissionPathSelector {
> + async fn get_view_paths() -> Result<Vec<String>, Error> {
> + let views = pdm_client().list_views().await?;
> + let paths: Vec<String> = views
> + .iter()
> + .map(|cfg| format!("/view/{}", cfg.id))
> + .collect();
> + Ok(paths)
> + }
> +
> + async fn get_paths() -> Result<Vec<String>, Error> {
> + let paths = Self::get_view_paths().await?;
> + Ok(paths)
> + }
> +}
>
> 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 create(ctx: &Context<Self>) -> Self {
> + let base_items: Vec<AttrValue> = PREDEFINED_PATHS
> + .iter()
> + .map(|i| AttrValue::from(*i))
> + .collect();
> +
> + let this = Self {
> + items: Rc::new(base_items),
> + async_pool: AsyncPool::new(),
> + };
> +
> + let link = ctx.link().clone();
> + this.async_pool.spawn(async move {
> + let paths = Self::get_paths().await;
> + match paths {
> + Ok(paths) => {
> + link.send_message(Msg::Prefetched(paths));
> + }
> + Err(_) => {
> + link.send_message(Msg::PrefetchFailed);
> + }

this is minor, but at least for me cargo fmt will also accept the
following formatting:

match paths {
Ok(paths) => link.send_message(Msg::Prefetched(paths)),
Err(_) => link.send_message(Msg::PrefetchFailed),
}

imo that's a little neater.

> + }
> + });

im generally not too big of a fan of this "this" pattern in rust and
here there is no need for it, you can simply do:

let async_pool = AsyncPool::new();
async_pool.spawn(..);
Self {
items: Rc::new(..),
async_pool,
}

you may need to change `async_pool` in the struct to `_async_pool`
because then this member is never directly accessed.

> +
> + this
> }
>
> - fn update(&mut self, _ctx: &Context<Self>, _msg: Self::Message) -> bool {
> - false
> + fn update(&mut self, _ctx: &Context<Self>, msg: Self::Message) -> bool {
> + match msg {
> + Msg::Prefetched(paths) => {
> + let mut items: Vec<String> = Vec::with_capacity(self.items.len() + paths.len());
> +
> + items.extend(self.items.iter().map(|item| item.to_string()));
> + items.extend(paths.into_iter());
> + items.sort_by(|a, b| a.to_lowercase().cmp(&b.to_lowercase()));
> +
> + self.items = Rc::new(items.into_iter().map(AttrValue::from).collect());

this could be expressed more neatly as:

```
Msg::Prefetched(paths) => {
let items = Rc::make_mut(&mut self.items);
items.extend(paths.into_iter().map(AttrValue::from));
items.sort_by_key(|k| k.to_lowercase());
true
}
```

this has a couple of advantages:

* we don't move the original items from `AttrValue` to a `String` back
to `AttrValue`, saving on allocations.
* `Rc::make_mut()` will only clone if there are other pointers to the
same allocation. since the is an `Rc::clone` in the view method below,
this probably won't actually impact much here from what i can tell,
but it still seems sensible to me to only force a new `Rc<Vec>` if
absolutely needed.
* uses `sort_by_key` instead of `sort_by` is more concise and also
recommended by clippy.

> +
> + true
> + }
> + Msg::PrefetchFailed => false,
> + }
> }
>
> fn view(&self, ctx: &Context<Self>) -> Html {