On 03/15/2016 04:40 PM, Petr Cech wrote:
On 03/10/2016 02:36 PM, Pavel Březina wrote:
> On 03/09/2016 05:49 PM, Petr Cech wrote:
>> On 03/08/2016 05:57 PM, Petr Cech wrote:
>>> On 03/08/2016 05:09 PM, Pavel Reichl wrote:
>>>> Hello Petr,
>>>>
>>>> I just run through the code and I have some code style suggestions,
>>>> feel
>>>> free to disagree :-).
>>>
>>> I will address some suggestions tomorrow. I am doing tests for
>>> sysdb_sudo_rules now. Thanks for comments.
>>>
>>> Petr
>>>
>>
>> Hello,
>>
>> all comments addressed. New patches attached. I am still working on
>> tests, so I will read all the patches again.
>
> Hi, just a quick review.
>
> Patch 1:
>
>> 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)
>> {
> ...
>> if (sub_filter == NULL) {
>> filter = talloc_asprintf(tmp_ctx, "(&%s)",
SUDO_ALL_FILTER);
>
> Remove ampersand please, it is not necessary. (objectClass=sudoRule) is
> sufficient.
Addressed.
>> } else {
>> filter = talloc_asprintf(tmp_ctx, "(&%s%s)",
>> SUDO_ALL_FILTER, sub_filter);
>> }
>> if (filter == NULL) {
>> DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
>> ret = ENOMEM;
>> goto done;
>> }
>>
>> DEBUG(SSSDBG_TRACE_INTERNAL,
>> "Search sudo rules with filter: %s\n", filter);
>>
>> ret = sysdb_search_entry(tmp_ctx, domain->sysdb, dn,
>> LDB_SCOPE_SUBTREE, filter, attrs,
>> &msgs_count, &msgs);
>>
>> if (ret == ENOENT) {
>> DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
>
> The code says the ENOENT may be returned but msgs_count and msgs remain
> not set. You need to set it here explicitly and goto done.
Addressed.
>> } else if (ret != EOK) {
>> DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
>> sss_strerror(ret));
>
> You need to goto done here.
Adrressed.
>> }
>>
>> *_msgs_count = msgs_count;
>> *_msgs = talloc_steal(mem_ctx, msgs);
>>
>> ret = EOK;
>>
>> done:
>> talloc_zfree(tmp_ctx);
>> return ret;
>> }
>>
>> static struct ldb_dn *
>> sysdb_sudo_rule_dn(TALLOC_CTX *mem_ctx,
>> struct sss_domain_info *domain,
>> const char *name)
>> {
>> return sysdb_custom_dn(mem_ctx, domain, name, SUDORULE_SUBDIR);
>> }
>>
>> 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);
>> NULL_CHECK(tmp_ctx, ret, done);
>>
>> dn = sysdb_sudo_rule_dn(tmp_ctx, domain, name);
>
> 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.
>> NULL_CHECK(dn, ret, done);
>>
>> ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
>>
>> done:
>> talloc_free(tmp_ctx);
>> return ret;
>> }
>
> Patch 3:
>
> case 'e':
> idb = INVALIDATE_EVERYTHING;
> + idb |= INVALIDATE_SUDO_RULES;
> break;
>
> #ifdef BUILD_SUDO should be here... it is not necessary but we should
> remain clear.
Addressed.
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, 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".
*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;
}
*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.
#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.
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.
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.
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.
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.
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.
}
/* 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)
^
*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};'
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;
}
if (!is_filter_valid(ctx, &values, idb)) { // NOT is_filter_valid()
DEBUG(SSSDBG_CRIT_FAILURE, "Construction of filters failed\n");
ret = ENOMEM;
goto fini;
}
*Patch 5: TOOL: Invalidation of sudo rules at sss_cache*
This looks good. I did not test it so far though.