[SSSD] [PATCHES] cleanup task: Expire all memberof targets when removing user

Pavel Reichl preichl at redhat.com
Mon Aug 31 13:08:03 UTC 2015



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 at 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 at 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 at 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?
> + at 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Michal, please fix these nitpicks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20150831/40b42bd5/attachment-0001.html>


More information about the sssd-devel mailing list