Re: [pdm-devel] [PATCH datacenter-manager/proxmox/yew-comp v2 00/11] ACL edit api and ui components

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

On Thu Apr 17, 2025 at 5:46 PM CEST, Thomas Lamprecht wrote:
> Am 11.04.25 um 15:44 schrieb Shannon Sterz:
>> 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. it
>> also tries to make the code here easier to understand as the checks
>> could be hard to grasp previously.
>>
>> 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.
>>
>
> I double-checked, so I hope I did not just overlook it, but I could not
> find any (higher level) changelog since v1, neither a series wide one
> nor a per patch one. At least one would be really nice to have, and
> having both is always great from a reviewer POV IMO.

sorry yeah, a lot of the review here kind of happened off list and i
forgot to add it in the end, let me add it here:

- added a patch that refactors the top-level privilege checking logic
  for the acl endpoints to use `CachedUserInfo`'s `check_privs` and to
  allow for configuring partial permission matches per product. this
  ideally also makes the permission checks easier to understand (patch
  proxmox 5)
- added a patch that refactors the `extract_acl_node_data` helper
  function to be non-recursive. this should improve the memory footprint
  of this function, especially when dealing with deeper trees (patch
  proxmox 6)

hopefully this won't have cause to much trouble as both changes are part
of their own additional patches.


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