12:04 a.m.
On (07/08/15 20:54), Michal Židek wrote:
Hi,
see the attached patches for ticket
https://fedorahosted.org/sssd/ticket/2676
Removing the user during cleanup task completely
removes that user from local database, so that
user is no longer part of any group in sysdb.
Such user may, or may not have been removed from
LDAP so the only way to figure out is check
on the next refresh otherwise we may get
inconsistent results.
The third patch adds integration test for this
Apply and run the intgcheck without the first 2
patches to see the failed test (you will also see that
all the tests after this failed one will end with
errors as well, that is probably due to incomplete
clean up after the ldap tests and I do not intend to
solve this as part of this patchset but will
investigate it later).
From 9930382acae9eb708c609355131207a0be42d1e9 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 | 10 ++++++-
src/db/sysdb_ops.c | 51 ++++++++++++++++++++++++++++++++++++
src/tests/sysdb-tests.c | 69 +++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 129 insertions(+), 1 deletion(-)
diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 9e28b5c..a48e22d 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -387,6 +387,11 @@ errno_t sysdb_msg2attrs(TALLOC_CTX *mem_ctx, size_t count,
/* convert an ldb error into an errno error */
int sysdb_error_to_errno(int ldberr);
+/* Convert ldb value to ldb dn */
+struct ldb_dn *sysdb_ldb_val_to_ldb_dn(TALLOC_CTX *tctx,
+ struct sss_domain_info *dom,
+ struct ldb_val *ldbval);
+
/* DNs related helper functions */
errno_t sysdb_get_rdn(struct sysdb_ctx *sysdb, TALLOC_CTX *mem_ctx,
const char *dn, char **_name, char **_val);
@@ -717,11 +722,14 @@ 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 (permissive) */
+int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom,
+ struct ldb_dn *ldbdn);
+
/* 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..c582578 100644
--- a/src/db/sysdb_ops.c
+++ b/src/db/sysdb_ops.c
@@ -3875,3 +3875,54 @@ errno_t sysdb_handle_original_uuid(const char *orig_name,
return EOK;
}
+
+struct ldb_dn *sysdb_ldb_val_to_ldb_dn(TALLOC_CTX *tctx,
+ struct sss_domain_info *dom,
+ struct ldb_val *ldbval)
+{
+ return ldb_dn_from_ldb_val(tctx, dom->sysdb->ldb, ldbval);
+}
I would prefer to do not expose such simple functions as sysdb functions.
I do not see a reason why should convert ldb_val -> ldb_dn
in other modules.
If wee need two versions of sysdb_mark_entry_as_expired e can still have two
functions:
sysdb_mark_entry_as_expired_ldb_dn
sysdb_mark_entry_as_expired_ldb_val
The core logic can be in one and second will be wrapper with
ldb_dn_from_ldb_val.
+
+/* Mark entry as expired if that entry exists in database. */
+int sysdb_mark_entry_as_expired_ldb_dn(struct sss_domain_info *dom,
+ struct ldb_dn *ldbdn)
+{
+ struct ldb_message *msg;
+ int ret;
+ 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) {
+ goto done;
+ }
+
+ ret = ldb_msg_add_string(msg, SYSDB_CACHE_EXPIRE, "1");
+ if (ret != LDB_SUCCESS) {
+ goto done;
+ }
+
+ ret = sss_ldb_modify_permissive(dom->sysdb->ldb, msg);
We should avoid
using sss_ldb_modify_permissive.
It should be used in very rare cases.
Could you explain why do we need to it here?
+ if (ret != LDB_SUCCESS) {
+ goto done;
+ }
+
+ ret = EOK;
+done:
+ talloc_free(tmp_ctx);
+ 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 5d6edc83c09b36f34beb9118d1c528b5229600d8 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
his memberof targets as expired to refresh member/ghost
attributes on next request.
---
src/providers/ldap/ldap_id_cleanup.c | 57 ++++++++++++++++++++++++++++++++++--
1 file changed, 55 insertions(+), 2 deletions(-)
diff --git a/src/providers/ldap/ldap_id_cleanup.c b/src/providers/ldap/ldap_id_cleanup.c
index be9496a..93a9362 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;
}
-
+ account_cache_expiration = dp_opt_get_int(opts->basic,
SDAP_ACCOUNT_CACHE_EXPIRATION);
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) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "expire_memberof_target_grps failed: [%d]:%s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
}
done:
@@ -278,6 +292,45 @@ done:
return ret;
}
+static int expire_memberof_target_grps(struct sss_domain_info *dom,
+ struct ldb_message *user)
+{
+ struct ldb_message_element *memberof_el;
+ struct ldb_dn *ldb_dn;
+ int ret;
+ int i;
+ TALLOC_CTX *tmp_ctx;
+
+ tmp_ctx = talloc_new(NULL);
+ if (tmp_ctx == NULL) {
+ return ENOMEM;
+ }
+
+ memberof_el = ldb_msg_find_element(user, SYSDB_MEMBEROF);
+
+ for (i = 0; i < memberof_el->num_values; i++) {
+ ldb_dn = sysdb_ldb_val_to_ldb_dn(tmp_ctx, dom,
+ &memberof_el->values[i]);
+ if (ldb_dn == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+
+ ret = sysdb_mark_entry_as_expired_ldb_dn(dom, ldb_dn);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_CRIT_FAILURE,
+ "sysdb_mark_entry_as_expired_ldb_dn failed: [%d]: %s\n",
+ ret, sss_strerror(ret));
+ goto done;
+ }
+ }
+
+ ret = EOK;
+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 cf61a2037fc3cfb6ddee040e6423463a08e74d2c 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..19968e3 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)
+
+
+(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_timeout = 1
+ entry_cache_group_timeout = 5000
It would be good either to add comment
that
entry_cache_group_timeout should be different to entry_cache_user_timeout
or explicitly use both options in sssd.conf
+ 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(10)
This can cause issuse in CI. The first clean up task
is fired after 10 seconds. It would be be better to
wait a little bit longer. To be sure that clean-up
ask already finished.
+
+ ent.assert_group_by_name(
+ "group2",
+ dict(mem=ent.contains_only("user1")))
LS