On 05/06/2011 04:05 PM, Stephen Gallagher wrote:
On Fri, 2011-05-06 at 13:56 +0200, Jakub Hrozek wrote:
> [PATCH 1/2] Add a confdb_get_uint32_t convenience function
> A convenience function to fetch the gid override from confdb
>
Ack
> [PATCH 2/2] Add options to override GID, homedir and shell
> The functionality to override the shell is a little different than
> described in the ticket -- it was adjusted after a discussion on #sssd
> on IRC.
Nack.
In the future, please provide separate patches for logically different
features (this patch contains three different changes).
The home directory template should also support UID (I mentioned this in
ticket 742). We should cover all our bases.
Done, sorry, I implemented this particular part based on the discussion
in ticket #551 that does not list UID.
Please reorder the manpage entry for allowed_shells. It should be
clear
that the order of evaluation is:
1. If it's in /etc/shells, use it.
2. If it's NOT in /etc/shells, but it is in allowed_shells, use the
value of shell_fallback.
3. If it's NOT in /etc/shells and it's not in allowed_shells, it
becomes nologin
We should also make it clear that having an empty string for shell is
passed as-is to glibc for it to make the decision.
Done.
In expand_homedir_template(), you allocate memory for
"copy" that you
never free. Please create a tmp_ctx for copy and free it on all exits
(success or failure). It would be preferable to also allocate "result"
on tmp_ctx and only talloc_steal() it onto mem_ctx at the end of
successful execution.
Done. I copied this bug when I stole the code from
expand_ccname_template(). I'll send a separate patch to address the same
issue there, too.
get_shell_override() should not shortcut out if there are no allowed
shells. It should still check /etc/shells because if it's not listed, we
want to convert it to nologin.
This is done in the attached patch, but I don't agree with the change.
It differs from the current behaviour and moreover allowed_shells is
unset in the default setting, so the change would affect anyone running
with defaults.
Also, get_shell_override() isn't handling the empty shell special
case,
which should be passed back as-is.
Done.
Finally, getusershell() is an expensive system call. I'd prefer
if we
only did so once and cached it in the nss_ctx, rather than invoking it
for every user request. I think it's reasonable to require the SSSD to
be restarted if /etc/shells changes. (Might want to put that in the
manpage though.)
Done. I was relying on VFS, I guess, but I agree that restarting SSSD is
acceptable.