On Thu, Feb 16, 2012 at 11:28:00AM +0100, Pavel Březina wrote:
Dne 16.2.2012 10:55, Jakub Hrozek napsal(a):
>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.
Thank you, seems to work fine.
Ack!