On 03/16/2016 12:37 PM, Pavel Březina wrote:
> On 03/15/2016 04:40 PM, Petr Cech wrote:
>> On 03/10/2016 02:36 PM, Pavel Březina wrote:
[...]
>> There is whole patch set attached. I worked on tests (003 patch) last
>> couple days. I know that there are some test cases missing.
>>
>> 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?
>> Of course, if you see any missing important test case, please write me
>> :-)
>>
>> Regards
>
> 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 do not insist on the use of patch (with documentation).
> *Patch 2: SYSDB: Add new funtions into sysdb_sudo*
>
>> +errno_t
>> +sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
>> + const char *name,
>> + struct sysdb_attrs *attrs,
>> + int mod_op)
>> +{
>> + errno_t ret;
>> + struct ldb_dn *dn;
>> + TALLOC_CTX *tmp_ctx;
>> +
>> + tmp_ctx = talloc_new(NULL);
>> + if (tmp_ctx == NULL) {
>> + return ENOMEM;
>> + }
>> +
>> + dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
>> + NULL_CHECK(dn, ret, done);
>> +
>> + ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
>> +
>> +done:
>> + talloc_free(tmp_ctx);
>> + return ret;
>>>> Temporary context is not needed here, you can use NULL instead and
>>>> talloc_free(dn) in the end.
>>>
>>> I am not sure about it. I have tried it but it didn't work for me.
>>>
>
> What didn't work for you?
>
> errno_t
> sysdb_set_sudo_rule_attr(struct sss_domain_info *domain,
> const char *name,
> struct sysdb_attrs *attrs,
> int mod_op)
> {
> struct ldb_dn *dn;
> errno_t ret;
>
> dn = sysdb_sudo_rule_dn(NULL, domain, name);
> if (dn == NULL) {
> return ENOMEM;
> }
>
> ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
> talloc_free(dn);
> return ret;
> }
Comments to this issue is in another mail in this thread.
I have sent patch to samba project.
https://lists.samba.org/archive/samba-technical/2016-March/113068.html
I use my original workaround with tmp_ctx for now.
I would like to wait to answer from samba project and patch
this situation later.
> *Patch 3: WIP: TESTS: Test of sysdb_search_sudo_rules*
>
>> #define TESTS_PATH "tp_" BASE_FILE_STEM
>> #define TEST_CONF_DB "test_sysdb_sudorules.ldb"
>>
>> #define TEST_DOM_NAME "test_domain.test"
>>
>> #define TEST_CACHE_SUDO_TIMEOUT 20
>>
>> #define TEST_USER1 "young_user"
>> #define TEST_UID1 1001
>> #define TEST_HOMEDIR1 "/home/" TEST_USER1
>>
>> #define TEST_USER2 "little_user"
>> #define TEST_UID2 1002
>> #define TEST_HOMEDIR2 "/home/" TEST_USER2
>>
>> #define TEST_USER3 "big_user"
>> #define TEST_UID3 1003
>> #define TEST_HOMEDIR3 "/home/" TEST_USER3
>>
>> #define TEST_USER_NON_EXIST "no_user"
>
> Unused.
If you mean TEST_USER_NON_EXIST, it is used in function
test_get_sudo_user_info_no_exist().
>> #define TEST_GROUP_NAME "test_sudo_group"
>> #define TEST_GID 10001
>>
>> #define TEST_SHELL "/bin/bash"
>>
>> #define TEST_RULE_NAME1 "first_rule"
>> #define TEST_SUDO_HOST1 "blue_host." TEST_DOM_NAME
>> #define TEST_SUDO_USER1 TEST_USER1
>> #define TEST_RUN_AS_USER1 "user_like_root_1"
>>
>> #define TEST_RULE_NAME2 "second_rule"
>> #define TEST_SUDO_HOST2 "red_host." TEST_DOM_NAME
>> #define TEST_SUDO_USER2 TEST_USER2
>> #define TEST_RUN_AS_USER2 "user_like_root_2"
>>
>> #define TEST_RULE_NAME3 "third_rule"
>> #define TEST_SUDO_HOST3 "green_host." TEST_DOM_NAME
>> #define TEST_SUDO_USER3 TEST_USER3
>> #define TEST_RUN_AS_USER3 "user_like_root_3"
>
> There is no need to #define homedir and shell, since it doesn't have to
> be unique and it's useless in test anyway. It would be nice to avoid
> these macros and group it into structure (users/groups/rules) like:
>
> struct test_user {
> const char *name;
> uid_t uid;
> gid_t gid;
> } users[] = {{"test-user1", 1001, 1001},
> {"test-user2", 1002, 1002}};
>
> It reads better when there are multiple objects.
Addressed.
>> static void create_set_of_rules(struct sysdb_test_ctx *test_ctx)
>> {
>> errno_t ret;
>>
>> test_ctx->rules = talloc_array(test_ctx, struct sysdb_attrs *, 2);
>>
>> test_ctx->rules[0] = sysdb_new_attrs(test_ctx);
>> assert_non_null(test_ctx->rules[0]);
>> create_rule(test_ctx->rules[0], test_ctx->tctx->dom,
>> TEST_RULE_NAME2, TEST_SUDO_HOST2,
>> TEST_RUN_AS_USER2, TEST_SUDO_USER2);
>>
>> test_ctx->rules[1] = sysdb_new_attrs(test_ctx);
>> assert_non_null(test_ctx->rules[1]);
>> create_rule(test_ctx->rules[1], test_ctx->tctx->dom,
>> TEST_RULE_NAME3, TEST_SUDO_HOST3,
>> TEST_RUN_AS_USER3, TEST_SUDO_USER3);
>>
>> ret = sysdb_sudo_store(test_ctx->tctx->dom, test_ctx->rules, 2);
>> assert_int_equal(ret, EOK);
>> }
>
> You are breaking talloc hierarchy here.
>
>
> What you want is
> test_ctx->rules = talloc_zero_array(test_ctx, ...)
> test_ctx->rules[0] = sysdb_new_attrs(test_ctx->rules) // see the context
> change
>
> This is on multiple places, please check and fix it. I suggest you to
> also read the Talloc tutorial, especially:
>
https://talloc.samba.org/talloc/doc/html/libtalloc__context.html
>
> ...to see why it is important to keep the hierarchy when possible.
Thanks, I understood now what talloc hierarchy is. I hope I've fixed all
places with broken talloc hierarchy.
> To remove_* functions -- if setup/teardown is done right (sysdb/test_ctx
> is created in setup and destroyed in teardown which it is) you don't
> need them.
>
>> static int test_sysdb_setup(void **state, bool rules_exist)
>> {
>> int ret;
>> struct sysdb_test_ctx *test_ctx;
>> struct sss_test_conf_param params[] = {
>> { NULL, NULL }, /* Sentinel */
>> };
>> const char *val[2];
>> val[1] = NULL;
>>
>> assert_true(leak_check_setup());
>> check_leaks_push(global_talloc_context);
>>
>> 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", params);
>
> Remove params and pass NULL instead.
Addressed.
>> assert_non_null(test_ctx->tctx);
>>
>> /* config */
>> val[0] = "1";
>> ret = confdb_add_param(test_ctx->tctx->confdb, true,
>> "config/domain/" TEST_DOM_NAME,
>> "pwd_expiration_warning", val);
>> assert_int_equal(ret, EOK);
>>
>> /* SUDO RULES Configuration */
>> val[0] = "0xFFF0";
>> ret = confdb_add_param(test_ctx->tctx->confdb, true,
>> "config/sudo", "debug_level",
val);
>> assert_int_equal(ret, EOK);
>>
>> val[0] = "TEST_CACHE_SUDO_TIMEOUT";
>
> This won't work :-) It would just set entry_cache_sudo_timeout =
> TEST_CACHE... but you want an actual value there.
Addressed.
>> ret = confdb_add_param(test_ctx->tctx->confdb, true,
>> "config/sudo",
"entry_cache_sudo_timeout",
>> val);
>> assert_int_equal(ret, EOK);
>>
>> create_groups(test_ctx->tctx->dom);
>> create_users(test_ctx->tctx->dom);
>>
>> if (rules_exist) {
>>
>
> Additional line... and I believe 'create_rules' is a better name.
Addressed.
>> create_set_of_rules(test_ctx);
>>
>> ret = sysdb_sudo_set_last_full_refresh(test_ctx->tctx->dom,
>> time(NULL));
>> assert_int_equal(ret, EOK);
>>
>> talloc_zfree(test_ctx->rules[1]);
>> talloc_zfree(test_ctx->rules[0]);
>> talloc_zfree(test_ctx->rules);
>
> If hierarchy is done, talloc_zfree(test_ctx->rules) is enough. Why do
> you free it anyway? And it seems pretty much useless but I may have
> missed something.
/* TODO */
Test patch is still WIP. I will slightly change logic that I will no
need to destroy test_ctx->rules[0-2]. There is still one commented test
and I would like write one more for filtering by name.
>> }
>>
>> /* Rules */
>> test_ctx->rules = NULL;
>>
>> check_leaks_push(test_ctx);
>> *state = (void *) test_ctx;
>> return 0;
>> }
>
> I'm getting this compiler error:
>
> /home/pbrezina/workspace/sssd/src/tests/cmocka/test_sysdb_sudo.c:85:1:
> error: ‘static’ is not at beginning of declaration
> [-Werror=old-style-declaration]
> void static remove_groups(struct sss_domain_info *domain)
> ^
> /home/pbrezina/workspace/sssd/src/tests/cmocka/test_sysdb_sudo.c:114:1:
> error: ‘static’ is not at beginning of declaration
> [-Werror=old-style-declaration]
> void static remove_users(struct sss_domain_info *domain)
> ^
Addressed.
> *Patch 4: REFACTOR: sss_cache*
>
> Use memset in init_input_values(). Or even better drop the function and
> use initializer 'struct input_values input = {0};'
Addressed.
>> static bool is_filter_valid(struct cache_tool_ctx *ctx,
>> struct input_values *values, int idb)
>> {
>> bool ret = ((idb & INVALIDATE_USERS) && !ctx->user_filter) ||
>> ((idb & INVALIDATE_GROUPS) && !ctx->group_filter) ||
>> ((idb & INVALIDATE_NETGROUPS) && !ctx->netgroup_filter)
||
>> ((idb & INVALIDATE_SERVICES) && !ctx->service_filter) ||
>> ((idb & INVALIDATE_AUTOFSMAPS) && !ctx->autofs_filter) ||
>> ((idb & INVALIDATE_SSH_HOSTS) && !ctx->ssh_host_filter)
||
>> ((idb & INVALIDATE_SUDO_RULES) && !ctx->sudo_rule_filter)
||
>> (values->user && !ctx->user_name) ||
>> (values->group && !ctx->group_name) ||
>> (values->netgroup && !ctx->netgroup_name) ||
>> (values->service && !ctx->service_name) ||
>> (values->map && !ctx->autofs_name) ||
>> (values->ssh_host && !ctx->ssh_host_name) ||
>> (values->sudo_rule && !ctx->sudo_rule_name);
>>
>> return ret;
>> }
>
> Yak, there must be a way how to split this into multiple if's. Can you
> try please? What is it supposed to return anyway? From the function name
> I suppose you want a negation of ret because ret contains true if the
> filter is actually not valid.
>
> What I propose is:
>
> bool is_filter_valid()
> {
> if ((idb & INVALIDATE_USERS) && ctx->user_filter == NULL) {
> return false;
> }
>
> if ((idb & INVALIDATE_GROUPS) && ctx->group_filter == NULL) {
> return false;
> }
>
> ...
>
> if (values->sudo_rule != NULL && ctx->sudo_rule_name == NULL) {
> return false;
> }
>
> return true;
> }
Addressed.
> if (!is_filter_valid(ctx, &values, idb)) { // NOT is_filter_valid()
> DEBUG(SSSDBG_CRIT_FAILURE, "Construction of filters failed\n");
> ret = ENOMEM;
> goto fini;
> }
Addressed.
>
> *Patch 5: TOOL: Invalidation of sudo rules at sss_cache*
>
> This looks good. I did not test it so far though.
> _______________________________________________
> sssd-devel mailing list
> sssd-devel(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
>
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org