On 08/10/2015 04:27 PM, Michal Židek wrote:
> From 725cb791d43e9f651345ec50419143c4e6bd22b8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?=<mzidek(a)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)
Is there a reason not to use errno_t?
> +{
> + struct ldb_message *msg;
> + int ret;
Same here?
> + 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;
please add an empty line
> +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;
errno_t?
> + TALLOC_CTX *tmpctx;
In function above you used tmp_ctx, I think it would be nice to use the
same name...nitpick
> +
> + 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);
empty line
> +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
>
> 0002-cleanup-task-Expire-all-memberof-targets-when-removi.patch
>
>
> From cccdd4bc3d35e2f0c71f5a1c1c54591e7edb8eac Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?=<mzidek(a)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 removed ?
> 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;
> }
> -
Is this intentional?
> + account_cache_expiration = dp_opt_get_int(opts->basic,
SDAP_ACCOUNT_CACHE_EXPIRATION);
Are these 2 lines same?
> 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) {
would it be nicer to have ret != EOK instead of just ret?
> + 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;
errno_t ?
> + int i;
you might want to move definition of i into for cycle :-)
> + TALLOC_CTX *tmp_ctx;
> +
> + tmp_ctx = talloc_new(NULL);
> + if (tmp_ctx == NULL) {
> + return ENOMEM;
> + }
> +
> + memberof_el = ldb_msg_find_element(user, SYSDB_MEMBEROF);
check for NULL?
> +
> + for (i = 0; i < memberof_el->num_values; i++) {
unsigned int would be better, right?
> + 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;
empty line
> +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
>
> 0003-CI-Add-regression-test-for-2676.patch
>
>
> From 2eec70588ed74e6c6d2322c4fdbd4c738f32d586 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Michal=20=C5=BDidek?=<mzidek(a)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)
> +
> +
Why 2 empty lines? The rest of the module uses just one, right?
It is PEP 8 convention. Actually the reason the rest of the
file does not follow it is my fault (I asked Nick to remove
the additional line because that is how we used it in other
parts of the code - now I know that I was wrong). See the
additional patch that adds the lines back.
> +(a)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(a)lists.fedorahosted.org
>
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Michal, please fix these nitpicks.
Done. Thank you for the review. I added also one patch to
add missing AM_CFLAGS (otherwise it was not possible to
build with the C99 features) and one patch to add the
missing blank lines in the python test in order follow
PEP 8 (at least in this relatively new code).
Michal