On 03/29/2016 03:17 PM, Petr Cech wrote:
My big challenge is test_filter_rules_by_time(). I recognize that we
don't have function for load sudo rule by name. I mean how to obtain
struct sysdb_attrs *rule, for example by name of rule.
If I understand it rigt, we just prepare filter and we say (in attrs)
which fields of rule we want load. Is it right?
Well... write one or just use (name=$name) as filter :-)
Of course, I can do that. :-)
I have hoped there is way how to obtain such filter from
sysdb_get_sudo_filter()
For example:
# ret = sysdb_get_sudo_filter(test_ctx, TEST_RULE_NAME2,
# uid, groupnames, SYSDB_NAME,
# &filter);
# printf(">>> %s\n", filter);
Prints:
# >>>
(&(objectClass=sudoRule)(|(sudoUser=ALL)(name=defaults))(&(dataExpireTimestamp<=1458136945)))
I am little confused from that output. Does it mean that function
sysdb_get_sudo_filter() is only for user, uid and groupnames connected
filters?
Yes, it was design to create a filter for responder. If you want to
change it, go ahead.
Hi,
*Patch 1: SYSDB: Add documentation to sysdb_sudo_purge()*
Yea, we don't do this, unless it is a public interface. The best
documentation is the code itself since maintaining documentation
together with active development is a pain in (you know where) and you
can't rely on it since it's often sloppy and quite often it doesn't
mirror recent changes in the code.
But if you insist on this patch, then you should use xor i.e. "either a
or b".
I understand documentation is pain.
Actually I think that the behaviour of function sysdb_sudo_purge() is
little confusing.
I have read the code of this function and I've recognized there are
other private functions:
sysdb_sudo_purge_byfiler()
sysdb_sudo_purge_byname()
sysdb_sudo_purge_byrules()
Public function sysdb_sudo_purge() combines some of them but
exclusively. That's confusing, isn't it?
I don't see the confusion but I'd rather change the name than
documenting it. So if you want to change the name or you need to change
the behaviour, go ahead.
*Patch 1: SYSDB: Add new funtions into sysdb_sudo*
> +static struct ldb_dn *
> +sysdb_sudo_rule_dn(TALLOC_CTX *mem_ctx,
> + struct sss_domain_info *domain,
> + const char *name)
^ indentation
> +{
> + return sysdb_custom_dn(mem_ctx, domain, name, SUDORULE_SUBDIR);
> +}
Ack otherwise.
*Patch 2: TESTS: Test of sysdb_search_sudo_rules*
static int test_sysdb_teardown(void **state)
{
struct sysdb_test_ctx *test_ctx = talloc_get_type_abort(*state,
struct sysdb_test_ctx);
remove_rules(test_ctx);
talloc_zfree(test_ctx->rules);
assert_true(check_leaks_pop(test_ctx));
remove_users(test_ctx->tctx->dom);
remove_groups(test_ctx->tctx->dom);
talloc_zfree(test_ctx);
assert_true(check_leaks_pop(global_talloc_context));
assert_true(leak_check_teardown());
return 0;
}
I suggest the following modification of setup and teardown. This will
simplify the leak check and allows you to remove sysdb cleanup functions:
1) You already create new sysdb with each test so we just need to remove
it in teardown.
2) The confdb values you are using are not needed for tests. Remove them.
3) leak_check_setup/teardown already push/pop global_talloc_context, you
don't need to do it yourself.
4) push(test_ctx) at the end of setup so you don't have to take care of
test_ctx->rules (and you will actually see that you are leaking memory
from tests by manipulating with these attrs)
5) please, move creating/storing rules into the tests itself. I think
renaming create_rule() to create_rule_attrs() and creating store_rule()
that creates and stores attrs will make it simple and clear when called
inside tests. And you will only have rules in sysdb that you need in
test. Remove test_ctx->rules altogether.
6) create_dom_test_ctx() needs to take test_ctx as memory context
(btw it's funny to use sysdb_sudo_store in _setup and then have a unit
test for it)
static int test_sysdb_setup(void **state)
{
struct sysdb_test_ctx *test_ctx;
assert_true(leak_check_setup());
test_ctx = talloc_zero(global_talloc_context,
struct sysdb_test_ctx);
assert_non_null(test_ctx);
test_dom_suite_setup(TESTS_PATH);
test_ctx->tctx = create_dom_test_ctx(test_ctx, TESTS_PATH,
TEST_CONF_DB, TEST_DOM_NAME, "ipa", NULL);
assert_non_null(test_ctx->tctx);
create_groups(test_ctx->tctx->dom);
create_users(test_ctx->tctx->dom);
check_leaks_push(test_ctx);
*state = (void *) test_ctx;
return 0;
}
static int test_sysdb_teardown(void **state)
{
struct sysdb_test_ctx *test_ctx;
test_ctx = talloc_get_type_abort(*state, struct sysdb_test_ctx);
assert_true(check_leaks_pop(test_ctx));
talloc_zfree(test_ctx);
test_dom_suite_cleanup(TESTS_PATH, TEST_CONF_DB, TEST_DOM_NAME);
assert_true(leak_check_teardown());
return 0;
}
main:
7) You need to use TEST_DOM_NAME instead of LOCAL_SYSDB_FILE in cleanup
8) you want to use SSSDBG_INVALID instead of TRACE_LIBS as initial level
9) remove test_dom_suite_setup(TESTS_PATH); from main since you are
creating it in setup
Comments to test cases:
test_store_sudo
test_sudo_purge*
test_set_sudo_rule_attr_exist
This doesn't test anything. You need to check that the rules was stored,
removed and that the attribute has actually changed.
test_sudo_set_get_last_full_refresh -- you don't need to you sleep() nor
time() it's just number so don't slowdown the test with this... and you
also should test set and get separately. Unit test in general should
test only one thing...
test_get_sudo_user_info_nonexist_group -- I think _nogroups is a better
name, what do you think?
test_get_sudo_user_info_no_exist -- _nouser?
test_filter_rules_by_time -- you also want to test that the correct rule
was returned.
*Patch 3: REFACTOR: sss_cache*
Ack.
*Patch 4: TOOL: Invalidation of sudo rules at sss_cache*
Ack.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
