On Mon, Feb 13, 2012 at 06:18:57PM +0100, Pavel Březina wrote:
> Dne 13.2.2012 17:32, Jakub Hrozek napsal(a):
>> On Fri, Feb 10, 2012 at 02:32:06PM +0100, Pavel Březina wrote:
>>>
https://fedorahosted.org/sssd/ticket/1173
>>
>> sysdb_sudo_build_sudouser(): Are there any cases where username would be
>> unset but there would be UID and groups?
>
> Probably not.
>
> Can we simplify the logic by
>> always allocating one of the three (or maybe two..uid and username..
>
> Done.
>
>> You should use instead of sysdb_custom_subtree_dn() instead of
>> ldb_dn_new_fmt() to sanitize the username passed from sudo.
>>
>> You probably want to use return EOK rather than goto done in
>> sysdb_sudo_purge_bysudouser() because you test "ret" in the
"done" label
>> and that's uninitialized.
>>
>> + if (sudouser == NULL || sudouser[0] == NULL) {
>> + goto done;
>> + }
>
> Thank you. Done.
>
>> The transaction logic should be to only commit before the "done" label
>> and set in_transaction to false if the commits succeeds. The current
>> code would try to commit the transaction if it's inside one even if
>> there was an error. (And remember, even _commit can fail and need a
>> _cancel).
>
> Done.
> This was copy and paste from somewhere. We should really do a macro for it.
>
>> Is it possible to squash the sysdb_get_sudo_filter() call that deletes
>> by netgroup into sysdb_sudo_build_sudouser() ? It's called
>> unconditionally anyway and we would save a sysdb transaction by doing
>> only one delete.
>
> I assume that you meant to squash
> sysdb_sudo_purge_byfilter(sysdb_ctx, filter /*ngrs*/) to
> sysdb_sudo_purge_bysudouser(sysdb_ctx, sudouser).
>
> No, unless you want to have one function for two things. The
> purge_bysudouser literally searches for all rules that contains at
> least one value from sudouser. So far it would work for netgroups as
> well.
> But now we want to remove the RULE that contains netgroup but remove
> the VALUE that matches sudouser.
>
> It would be theoretically possible if we would do the comparison of
> values with regex (evaluate "+*") but I don't thing it is worth it.
>
> New patch attached.
sysdb_sudo_build_sudouser(): instead of assigning count=2 and then
always doing a realloc, can you allocate count+1 members right from the
start?
This doesn't seem very readable to me:
+ if (ret != EOK&& ret != ENOENT) {
+ DEBUG(SSSDBG_CRIT_FAILURE, ("Error looking up SUDO rules"));
+ goto done;
+ } if (ret == ENOENT) {
+ DEBUG(SSSDBG_TRACE_FUNC, ("No rules matched\n"));
+ ret = EOK;
Can we change it to if (ret == ENOENT) {} else if (ret != EOK) ?
I think it might be nice to add a transaction to
sdap_sudo_purge_sudoers(), especially in the BE_REQ_SUDO_USER we are
performing several steps and I think the whole delete operation should
either succeed or fail. The same applies to sysdb_sudo_purge_byfilter().
Ok, new patch attached. It also takes care of sudoUser=ALL and add debug
message when deleting/modifying rule.