[pbs-devel] [PATCH proxmox-backup] fix: config: remove duplicate privilege lookup in cached_user_info

[pbs-devel] [PATCH proxmox-backup] fix: config: remove duplicate privilege lookup in cached_user_info

[Stefan Sterz]

`lookup_privs` just uses `lookup_privs_details` but ignores the
propagated privileges it returns. thus, the lookup here is redundant
as it is immediately followed by a call to `lookup_privs_details` with
the same parameters.

Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
 pbs-config/src/cached_user_info.rs | 1 -
 1 file changed, 1 deletion(-)

diff --git a/pbs-config/src/cached_user_info.rs b/pbs-config/src/cached_user_info.rs
index 8dd2375a..b9534b80 100644
--- a/pbs-config/src/cached_user_info.rs
+++ b/pbs-config/src/cached_user_info.rs
@@ -170,7 +170,6 @@ impl CachedUserInfo {
         if auth_id.is_token() {
             // limit privs to that of owning user
             let user_auth_id = Authid::from(auth_id.user().clone());
-            privs &= self.lookup_privs(&user_auth_id, path);
             let (owner_privs, owner_propagated_privs) =
                 self.lookup_privs_details(&user_auth_id, path);
             privs &= owner_privs;
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] fix: config: remove duplicate privilege lookup in cached_user_info

[Wolfgang Bumiller]

Any reason for the "fix: " prefix in the commit message, though? This
just seems to remove something redundant and not actually fix an issue?
Or am I missing something?

Code-wise it seems fine, so I'd apply it, but I'd drop the 'fix' prefix?

On Fri, Jun 10, 2022 at 10:13:25AM +0200, Stefan Sterz wrote:
> `lookup_privs` just uses `lookup_privs_details` but ignores the
> propagated privileges it returns. thus, the lookup here is redundant
> as it is immediately followed by a call to `lookup_privs_details` with
> the same parameters.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
>  pbs-config/src/cached_user_info.rs | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/pbs-config/src/cached_user_info.rs b/pbs-config/src/cached_user_info.rs
> index 8dd2375a..b9534b80 100644
> --- a/pbs-config/src/cached_user_info.rs
> +++ b/pbs-config/src/cached_user_info.rs
> @@ -170,7 +170,6 @@ impl CachedUserInfo {
>          if auth_id.is_token() {
>              // limit privs to that of owning user
>              let user_auth_id = Authid::from(auth_id.user().clone());
> -            privs &= self.lookup_privs(&user_auth_id, path);
>              let (owner_privs, owner_propagated_privs) =
>                  self.lookup_privs_details(&user_auth_id, path);
>              privs &= owner_privs;
> -- 
> 2.30.2

Re: [pbs-devel] [PATCH proxmox-backup] fix: config: remove duplicate privilege lookup in cached_user_info

[Stefan Sterz]

On 6/10/22 10:52, Wolfgang Bumiller wrote:
> Any reason for the "fix: " prefix in the commit message, though? This
> just seems to remove something redundant and not actually fix an issue?
> Or am I missing something?
> 
> Code-wise it seems fine, so I'd apply it, but I'd drop the 'fix' prefix?
> 

i can see your point in that this technically doesn't fix a bug. you
can remove it.

my reasoning was that it removes a performance loss (albeit a rather
minor one) and also potential confusion when reviewing this code in
the future. it tripped me up when i looked at it. at the very least it
makes the code here more concise.

-- snip --

[pbs-devel] applied: [PATCH proxmox-backup] fix: config: remove duplicate privilege lookup in cached_user_info

[Wolfgang Bumiller]

applied w/o the 'fix:' prefix in the commit message
thanks