On 03/02/2016 05:04 PM, Petr Cech wrote:
> Hi all,
>
> attached two patches resolve [1]. This ticket has design page [2].
>
> In my opinion it could be fine to have tests on sysdb_sudo. I have
> started write some, but there were troubles with memory leak. Maybe I
> haven't understood necessary logic properly.
>
> However I could continue with tests or create new ticket for it.
>
> And little question. Is the name of sudo rule case sensitive? If yes, I
> have to do one little change.
I think we are good if we use domain settings so no change is needed.
>
> [1]
https://fedorahosted.org/sssd/ticket/2081
> [2]
>
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRulesInvalidate
>
> Regards
Hi,
see comments inline.
> errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
> struct sss_domain_info *domain,
> const char *sub_filter,
> const char **attrs,
> size_t *msgs_count,
> struct ldb_message ***msgs)
> {
> TALLOC_CTX *tmp_ctx;
> struct ldb_dn *dn;
> char *filter;
> int ret;
>
> tmp_ctx = talloc_new(NULL);
> if (!tmp_ctx) {
> return ENOMEM;
> }
>
> dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
> SYSDB_TMPL_CUSTOM_SUBTREE,
> SUDORULE_SUBDIR, domain->name);
> if (!dn) {
> DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
> ret = ENOMEM;
> goto fail;
> }
>
> filter = talloc_asprintf(tmp_ctx, "(&%s%s)", SUDO_ALL_FILTER,
> sub_filter);
You also need to add case where sub_filter is NULL.
> if (!filter) {
> DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
> ret = ENOMEM;
> goto fail;
> }
>
> DEBUG(SSSDBG_TRACE_INTERNAL,
> "Search services with filter: %s\n", filter);
sudo rule, not "services"
>
> ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
> LDB_SCOPE_SUBTREE, filter, attrs,
> msgs_count, msgs);
> if (ret) {
> goto fail;
> }
>
> talloc_zfree(tmp_ctx);
> return EOK;
>
> fail:
> if (ret == ENOENT) {
> DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
> }
> else if (ret) {
> DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
> strerror(ret));
> }
> talloc_zfree(tmp_ctx);
There is already good debug message per dn and filter case. Move this to
sysdb_search_entry and use done scheme instead of fail, please.
> return ret;
> }
Also atm -E won't trigger invalidation of sudo rules which I think is
also desired. Instead of modifying the current #ifdefs thingy, I'd
suggest using |= operator in init_context. I.e.:
case 'e':
idb = INVALIDATE_EVERYTHING;
idb |= SUDO...
break;