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?
+(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.