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! _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://fedorahosted.org/mailman/listinfo/sssd-devel
