On 15.03.22 08:31, Fabian Ebner wrote: > Am 14.03.22 um 14:50 schrieb Oguz Bektas: >> first call $rpcenv->get_user() if user was 'undef'. if that doesn't >> return then we set it to root@pam.
this is just the "whats done" description, that's not really interesting for such a short patch, as it can be read from the code change directly without much effort. A sentence about why (original code reason, change reason to the new behavior) and impact (what are the call sites that could be affected) would be more helpful. >> >> Signed-off-by: Oguz Bektas <[email protected]> >> --- >> v1->v2: >> * do get_user() first, set to 'root@pam' as fallback >> * drop first patch for pve-container (not needed anymore) >> >> src/PVE/RESTEnvironment.pm | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/src/PVE/RESTEnvironment.pm b/src/PVE/RESTEnvironment.pm >> index 1b2af08..bc5b8b5 100644 >> --- a/src/PVE/RESTEnvironment.pm >> +++ b/src/PVE/RESTEnvironment.pm >> @@ -492,7 +492,12 @@ sub fork_worker { >> $dtype = 'unknown' if !defined ($dtype); >> $id = '' if !defined ($id); >> >> - $user = 'root@pve' if !defined ($user); >> + $user = $self->get_user() if !defined($user); > > If you don't set $noerr when calling get_user(), the below if block is > dead code. > >> + >> + if (!defined($user)) { >> + warn 'internal error: Worker user was not specified, defaulting to >> "root@pam"!'; missing newline at the ends means spamming the log more with internal perl module file/line location and I don't really get the warning in the first place, either it's OK to fallback or not. The REST-env should have a valid user for most (all?) API/CLI-handler derived use cases, as only the public API calls have no user set but there we don't use fork_worker at all. So, I'd examine possible call sites that won't have a user passed nor available via get_user(), and dependening from if they even exist I'd * check if we could set a sensible user-id, if possible, there already * make this either a no-warn or just drop the if-block and avoid passing the $noerr to $self->get_user(), making this a usage error. The latter would be cleaner, but has some theoretic breakage potential. As call-site evaluation should be done in any case, as neither breakage nor defaulting to root@pam is something that should be done "blindly" (I mean, root fallback was probably intended originally, just avoided due to the realm typo, but still). _______________________________________________ pve-devel mailing list [email protected] https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
