Is there a reason not to use errno_t?From 725cb791d43e9f651345ec50419143c4e6bd22b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek@redhat.com> Date: Thu, 6 Aug 2015 09:16:03 +0200 Subject: [PATCH 1/3] SYSDB: Add function to expire entry Ticket: https://fedorahosted.org/sssd/ticket/2676 Added function to expire entry in sysdb using its DN. --- src/db/sysdb.h | 7 ++++- src/db/sysdb_ops.c | 70 +++++++++++++++++++++++++++++++++++++++++++++++++ src/tests/sysdb-tests.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) diff --git a/src/db/sysdb.h b/src/db/sysdb.h index 9e28b5c..75d7318 100644 --- a/src/db/sysdb.h +++ b/src/db/sysdb.h @@ -717,11 +717,16 @@ int sysdb_delete_entry(struct sysdb_ctx *sysdb, struct ldb_dn *dn, bool ignore_not_found); - int sysdb_delete_recursive(struct sysdb_ctx *sysdb, struct ldb_dn *dn, bool ignore_not_found); +/* Mark entry as expired */ +int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom, + struct ldb_dn *ldbdn); +int sysdb_mark_entry_as_expired_ldb_val(struct sss_domain_info *dom, + struct ldb_val *dn_val); + /* Search Entry */ int sysdb_search_entry(TALLOC_CTX *mem_ctx, struct sysdb_ctx *sysdb, diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c index d1d43eb..6d4070a 100644 --- a/src/db/sysdb_ops.c +++ b/src/db/sysdb_ops.c @@ -3875,3 +3875,73 @@ errno_t sysdb_handle_original_uuid(const char *orig_name, return EOK; } + +/* Mark entry as expired */ +int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom, + struct ldb_dn *ldbdn)
Same here?+{ + struct ldb_message *msg; + int ret;
please add an empty line+ TALLOC_CTX *tmp_ctx; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + msg = ldb_msg_new(tmp_ctx); + if (msg == NULL) { + ret = ENOMEM; + goto done; + } + + msg->dn = ldbdn; + + ret = ldb_msg_add_empty(msg, SYSDB_CACHE_EXPIRE, + LDB_FLAG_MOD_REPLACE, NULL); + if (ret != LDB_SUCCESS) { + ret = sysdb_error_to_errno(ret); + goto done; + } + + ret = ldb_msg_add_string(msg, SYSDB_CACHE_EXPIRE, "1"); + if (ret != LDB_SUCCESS) { + ret = sysdb_error_to_errno(ret); + goto done; + } + + ret = ldb_modify(dom->sysdb->ldb, msg); + if (ret != LDB_SUCCESS) { + ret = sysdb_error_to_errno(ret); + goto done; + } + + ret = EOK;
errno_t?+done: + talloc_free(tmp_ctx); + return ret; +} + +int sysdb_mark_entry_as_expired_ldb_val(struct sss_domain_info *dom, + struct ldb_val *dn_val) +{ + struct ldb_dn *ldbdn; + int ret;
In function above you used tmp_ctx, I think it would be nice to use the same name...nitpick+ TALLOC_CTX *tmpctx;
empty line+ + tmpctx = talloc_new(NULL); + if (tmpctx == NULL) { + return ENOMEM; + } + + ldbdn = ldb_dn_from_ldb_val(tmpctx, dom->sysdb->ldb, dn_val); + if (ldbdn == NULL) { + ret = ENOMEM; + goto done; + } + + ret = sysdb_mark_entry_as_expired_ldb_dn(dom, ldbdn);
is removed ?+done: + talloc_free(tmpctx); + return ret; +} diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c index 24d1527..3bd03b1 100644 --- a/src/tests/sysdb-tests.c +++ b/src/tests/sysdb-tests.c @@ -6212,6 +6212,74 @@ START_TEST(test_confdb_list_all_domain_names_multi_dom) } END_TEST +START_TEST(test_sysdb_mark_entry_as_expired_ldb_dn) +{ + errno_t ret; + struct sysdb_test_ctx *test_ctx; + const char *attrs[] = { SYSDB_CACHE_EXPIRE, NULL }; + size_t count; + struct ldb_message **msgs; + uint64_t expire; + struct ldb_dn *userdn; + + ret = setup_sysdb_tests(&test_ctx); + fail_if(ret != EOK, "Could not setup the test"); + + /* Add something to database to test against */ + + ret = sysdb_transaction_start(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_add_user(test_ctx->domain, "testuser", + 2000, 0, "Test User", "/home/testuser", + "/bin/bash", + NULL, NULL, 500, 0); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_transaction_commit(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_search_users(test_ctx, test_ctx->domain, + "("SYSDB_UIDNUM"=2000)", attrs, &count, &msgs); + ck_assert_int_eq(ret, EOK); + ck_assert_int_eq(count, 1); + + expire = ldb_msg_find_attr_as_uint64(msgs[0], SYSDB_CACHE_EXPIRE, 0); + ck_assert(expire != 1); + + userdn = sysdb_user_dn(test_ctx, test_ctx->domain, "testuser"); + ck_assert(userdn != NULL); + + ret = sysdb_transaction_start(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); + + /* Expire entry */ + ret = sysdb_mark_entry_as_expired_ldb_dn(test_ctx->domain, userdn); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_transaction_commit(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_search_users(test_ctx, test_ctx->domain, + "("SYSDB_UIDNUM"=2000)", attrs, &count, &msgs); + ck_assert_int_eq(ret, EOK); + ck_assert_int_eq(count, 1); + + expire = ldb_msg_find_attr_as_uint64(msgs[0], SYSDB_CACHE_EXPIRE, 0); + ck_assert_int_eq(expire, 1); + + /* Attempt to mark missing entry should be OK */ + ret = sysdb_transaction_start(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_mark_entry_as_expired_ldb_dn(test_ctx->domain, userdn); + ck_assert_int_eq(ret, EOK); + + ret = sysdb_transaction_commit(test_ctx->sysdb); + ck_assert_int_eq(ret, EOK); +} +END_TEST + Suite *create_sysdb_suite(void) { Suite *s = suite_create("sysdb"); @@ -6424,6 +6492,7 @@ Suite *create_sysdb_suite(void) /* ===== Misc ===== */ tcase_add_test(tc_sysdb, test_sysdb_set_get_bool); + tcase_add_test(tc_sysdb, test_sysdb_mark_entry_as_expired_ldb_dn); /* Add all test cases to the test suite */ suite_add_tcase(s, tc_sysdb);-- 2.1.0
From cccdd4bc3d35e2f0c71f5a1c1c54591e7edb8eac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek@redhat.com> Date: Wed, 29 Jul 2015 17:18:06 +0200 Subject: [PATCH 2/3] cleanup task: Expire all memberof targets when removing user Ticket: https://fedorahosted.org/sssd/ticket/2676 When user removed from cache during cleanup task, mark all
Is this intentional?his memberof targets as expired to refresh member/ghost attributes on next request. --- src/providers/ldap/ldap_id_cleanup.c | 50 ++++++++++++++++++++++++++++++++++-- 1 file changed, 48 insertions(+), 2 deletions(-) diff --git a/src/providers/ldap/ldap_id_cleanup.c b/src/providers/ldap/ldap_id_cleanup.c index be9496a..86c8ec4 100644 --- a/src/providers/ldap/ldap_id_cleanup.c +++ b/src/providers/ldap/ldap_id_cleanup.c @@ -169,11 +169,14 @@ done: static int cleanup_users_logged_in(hash_table_t *table, const struct ldb_message *msg); +static int expire_memberof_target_grps(struct sss_domain_info *dom, + struct ldb_message *user); + static int cleanup_users(struct sdap_options *opts, struct sss_domain_info *dom) { TALLOC_CTX *tmpctx; - const char *attrs[] = { SYSDB_NAME, SYSDB_UIDNUM, NULL }; + const char *attrs[] = { SYSDB_NAME, SYSDB_UIDNUM, SYSDB_MEMBEROF, NULL }; time_t now = time(NULL); char *subfilter = NULL; int account_cache_expiration; @@ -188,7 +191,7 @@ static int cleanup_users(struct sdap_options *opts, if (!tmpctx) { return ENOMEM; } -
Are these 2 lines same?+ account_cache_expiration = dp_opt_get_int(opts->basic, SDAP_ACCOUNT_CACHE_EXPIRATION);
would it be nicer to have ret != EOK instead of just ret?account_cache_expiration = dp_opt_get_int(opts->basic, SDAP_ACCOUNT_CACHE_EXPIRATION); DEBUG(SSSDBG_TRACE_ALL, "Cache expiration is set to %d days\n", account_cache_expiration); @@ -271,6 +274,17 @@ static int cleanup_users(struct sdap_options *opts, DEBUG(SSSDBG_CRIT_FAILURE, "sysdb_delete_user failed: %d\n", ret); goto done; } + + /* Mark all groups of which user was a member as expired in cache, + * so that its ghost/member attributes are refreshed on next + * request. */ + ret = expire_memberof_target_grps(dom, msgs[i]); + if (ret && ret != ENOENT) {
errno_t ?+ DEBUG(SSSDBG_CRIT_FAILURE, + "expire_memberof_target_grps failed: [%d]:%s\n", + ret, sss_strerror(ret)); + goto done; + } } done: @@ -278,6 +292,38 @@ done: return ret; } +static int expire_memberof_target_grps(struct sss_domain_info *dom, + struct ldb_message *user) +{ + struct ldb_message_element *memberof_el; + int ret;
you might want to move definition of i into for cycle :-)+ int i;
check for NULL?+ TALLOC_CTX *tmp_ctx; + + tmp_ctx = talloc_new(NULL); + if (tmp_ctx == NULL) { + return ENOMEM; + } + + memberof_el = ldb_msg_find_element(user, SYSDB_MEMBEROF);
unsigned int would be better, right?+ + for (i = 0; i < memberof_el->num_values; i++) {
empty line+ ret = sysdb_mark_entry_as_expired_ldb_val(dom, + &memberof_el->values[i]); + if (ret != EOK && ret != ENOENT) { + DEBUG(SSSDBG_CRIT_FAILURE, + "sysdb_mark_entry_as_expired_ldb_val failed: [%d]: %s\n", + ret, sss_strerror(ret)); + goto done; + } + } + + ret = EOK;
Why 2 empty lines? The rest of the module uses just one, right?+done: + talloc_free(tmp_ctx); + return ret; +} + static int cleanup_users_logged_in(hash_table_t *table, const struct ldb_message *msg) {-- 2.1.0
From 2eec70588ed74e6c6d2322c4fdbd4c738f32d586 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michal=20=C5=BDidek?= <mzidek@redhat.com> Date: Thu, 6 Aug 2015 13:16:48 +0200 Subject: [PATCH 3/3] CI: Add regression test for #2676 Ticket: https://fedorahosted.org/sssd/ticket/2676 Regression test for the above ticket. --- src/tests/intg/ldap_test.py | 61 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+) diff --git a/src/tests/intg/ldap_test.py b/src/tests/intg/ldap_test.py index bfe4e65..b0a4dc6 100644 --- a/src/tests/intg/ldap_test.py +++ b/src/tests/intg/ldap_test.py @@ -260,3 +260,64 @@ def test_sanity_rfc2307_bis(ldap_conn, sanity_rfc2307_bis): grp.getgrnam("non_existent_group") with pytest.raises(KeyError): grp.getgrgid(1) + +
Michal, please fix these nitpicks.+@pytest.fixture +def refresh_after_cleanup_task(request, ldap_conn): + ent_list = ldap_ent.List(LDAP_BASE_DN) + ent_list.add_user("user1", 1001, 2001) + + ent_list.add_group_bis("group1", 2001, ["user1"]) + ent_list.add_group_bis("group2", 2002, [], ["group1"]) + + create_ldap_fixture(request, ldap_conn, ent_list) + + conf = unindent("""\ + [sssd] + config_file_version = 2 + domains = LDAP + services = nss + + [nss] + memcache_timeout = 0 + + [domain/LDAP] + ldap_auth_disable_tls_never_use_in_production = true + debug_level = 0xffff + ldap_schema = rfc2307bis + ldap_group_object_class = groupOfNames + id_provider = ldap + auth_provider = ldap + sudo_provider = ldap + ldap_uri = {ldap_conn.ds_inst.ldap_url} + ldap_search_base = {ldap_conn.ds_inst.base_dn} + + entry_cache_user_timeout = 1 + entry_cache_group_timeout = 5000 + ldap_purge_cache_timeout = 3 + + """).format(**locals()) + create_conf_fixture(request, conf) + create_sssd_fixture(request) + return None + + +def test_refresh_after_cleanup_task(ldap_conn, refresh_after_cleanup_task): + """ + Regression test for ticket: + https://fedorahosted.org/sssd/ticket/2676 + """ + ent.assert_group_by_name( + "group2", + dict(mem=ent.contains_only("user1"))) + + ent.assert_passwd_by_name( + 'user1', + dict(name='user1', passwd='*', uid=1001, gid=2001, + gecos='1001', shell='/bin/bash')) + + time.sleep(15) + + ent.assert_group_by_name( + "group2", + dict(mem=ent.contains_only("user1")))-- 2.1.0
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel