[SSSD] [PATCH] MEMBEROF: Implement delete operation for ghost users

Jakub Hrozek jhrozek at redhat.com
Sun Nov 25 23:00:44 UTC 2012


https://fedorahosted.org/sssd/ticket/1668

The memberof plugin did only expand the ghost users attribute to
parents when adding a nested group, but didn't implement the reverse
operation.

This bug resulted in users being reported as group members even
after the direct parent went away as the expanded ghost attributes were
never removed from the parent entry.

There seems to be a lot of similarlity between memberuid and ghost
attributes in the memberof plugin. Maybe the code would benefit from
soem more generic functions? But given the time contrainst, I would
prefer the refactoring to happend post-1.9.3.
-------------- next part --------------
>From 1b4cd3a923889257c27d29cd821eea81577458d5 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Sun, 25 Nov 2012 22:25:37 +0100
Subject: [PATCH] MEMBEROF: Implement delete operation for ghost users

https://fedorahosted.org/sssd/ticket/1668

The memberof plugin did only expand the ghost users attribute to
parents when adding a nested group, but didn't implement the reverse
operation.

This bug resulted in users being reported as group members even
after the direct parent went away as the expanded ghost attributes were
never removed from the parent entry.
---
 src/ldb_modules/memberof.c | 214 ++++++++++++++++++++++++++++++++++++++++++++-
 src/tests/sysdb-tests.c    | 107 ++++++++++++++++++++++-
 2 files changed, 316 insertions(+), 5 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index b884228d2ce845a5e52b6f500d47c6ac6dcba42e..5a576edba20a645454f526908e39e49e9b2358e1 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -33,6 +33,7 @@
 #define DB_MEMBERUID "memberuid"
 #define DB_NAME "name"
 #define DB_USER_CLASS "user"
+#define DB_GROUP_CLASS "group"
 #define DB_OC "objectClass"
 
 #ifndef talloc_zfree
@@ -133,6 +134,10 @@ struct mbof_del_ctx {
     int num_muops;
     int cur_muop;
 
+    struct mbof_memberuid_op *ghops;
+    int num_ghops;
+    int cur_ghop;
+
     struct mbof_mod_ctx *follow_mod;
     bool is_mod;
 };
@@ -165,7 +170,8 @@ static struct mbof_ctx *mbof_init(struct ldb_module *module,
     return ctx;
 }
 
-static int entry_is_user_object(struct ldb_message *entry)
+static int entry_has_objectclass(struct ldb_message *entry,
+                                 const char *objectclass)
 {
     struct ldb_message_element *el;
     struct ldb_val *val;
@@ -179,7 +185,7 @@ static int entry_is_user_object(struct ldb_message *entry)
     /* see if this is a user */
     for (i = 0; i < el->num_values; i++) {
         val = &(el->values[i]);
-        if (strncasecmp(DB_USER_CLASS, (char *)val->data, val->length) == 0) {
+        if (strncasecmp(objectclass, (char *)val->data, val->length) == 0) {
             return LDB_SUCCESS;
         }
     }
@@ -187,6 +193,16 @@ static int entry_is_user_object(struct ldb_message *entry)
     return LDB_ERR_NO_SUCH_ATTRIBUTE;
 }
 
+static int entry_is_user_object(struct ldb_message *entry)
+{
+    return entry_has_objectclass(entry, DB_USER_CLASS);
+}
+
+static int entry_is_group_object(struct ldb_message *entry)
+{
+    return entry_has_objectclass(entry, DB_GROUP_CLASS);
+}
+
 static int mbof_append_muop(TALLOC_CTX *memctx,
                             struct mbof_memberuid_op **_muops,
                             int *_num_muops,
@@ -1181,16 +1197,22 @@ static int mbof_del_get_next(struct mbof_del_operation *delop,
                              struct mbof_del_operation **nextop);
 static int mbof_del_fill_muop(struct mbof_del_ctx *del_ctx,
                               struct ldb_message *entry);
+static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
+                              struct ldb_message *entry);
 static int mbof_del_muop(struct mbof_del_ctx *ctx);
 static int mbof_del_muop_callback(struct ldb_request *req,
                                   struct ldb_reply *ares);
+static int mbof_del_ghop(struct mbof_del_ctx *del_ctx);
+static int mbof_del_ghop_callback(struct ldb_request *req,
+                                  struct ldb_reply *ares);
 static void free_delop_contents(struct mbof_del_operation *delop);
 
 
 static int memberof_del(struct ldb_module *module, struct ldb_request *req)
 {
     static const char *attrs[] = { DB_OC, DB_NAME,
-                                   DB_MEMBER, DB_MEMBEROF, NULL };
+                                   DB_MEMBER, DB_MEMBEROF,
+                                   DB_GHOST, NULL };
     struct ldb_context *ldb = ldb_module_get_ctx(module);
     struct mbof_del_operation *first;
     struct ldb_request *search;
@@ -1415,6 +1437,13 @@ static int mbof_orig_del_callback(struct ldb_request *req,
             return ldb_module_done(ctx->req, NULL, NULL, ret);
         }
 
+        /* ..or ghost attributes to remove */
+        ret = mbof_del_fill_ghop(del_ctx, del_ctx->first->entry);
+        if (ret != LDB_SUCCESS) {
+            talloc_zfree(ares);
+            return ldb_module_done(ctx->req, NULL, NULL, ret);
+        }
+
         /* if there are any parents, fire a removal sequence */
         ret = mbof_del_cleanup_parents(del_ctx);
     }
@@ -1426,6 +1455,10 @@ static int mbof_orig_del_callback(struct ldb_request *req,
     else if (del_ctx->muops) {
         return mbof_del_muop(del_ctx);
     }
+    /* see if we need to remove some ghost users */
+    else if (del_ctx->ghops) {
+        return mbof_del_ghop(del_ctx);
+    }
     else {
         /* no parents nor children, end ops */
         return ldb_module_done(ctx->req,
@@ -1537,6 +1570,10 @@ static int mbof_del_clean_par_callback(struct ldb_request *req,
         else if (del_ctx->muops) {
             return mbof_del_muop(del_ctx);
         }
+        /* see if we need to remove some ghost users */
+        else if (del_ctx->ghops) {
+            return mbof_del_ghop(del_ctx);
+        }
         else {
             /* no children, end ops */
             return ldb_module_done(ctx->req,
@@ -2217,6 +2254,10 @@ static int mbof_del_progeny(struct mbof_del_operation *delop)
     if (del_ctx->muops) {
         return mbof_del_muop(del_ctx);
     }
+    /* see if we need to remove some ghost users */
+    else if (del_ctx->ghops) {
+        return mbof_del_ghop(del_ctx);
+    }
     /* see if there are follow functions to run */
     if (del_ctx->follow_mod) {
         return mbof_mod_add(del_ctx->follow_mod,
@@ -2345,7 +2386,80 @@ static int mbof_del_fill_muop(struct mbof_del_ctx *del_ctx,
     return LDB_SUCCESS;
 }
 
-/* del memberuid attributes to parent groups */
+static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
+                              struct ldb_message *entry)
+{
+    struct ldb_message_element *mbof;
+    struct ldb_message_element *ghel;
+    struct ldb_dn *valdn;
+    int ret;
+    int i, j;
+
+    mbof = ldb_msg_find_element(entry, DB_MEMBEROF);
+    if (!mbof || mbof->num_values == 0) {
+        /* no memberof attributes ... */
+        return LDB_SUCCESS;
+    }
+
+    ghel = ldb_msg_find_element(entry, DB_GHOST);
+    if (ghel == NULL || ghel->num_values == 0) {
+        /* No ghel attribute, just return success */
+        return LDB_SUCCESS;
+    }
+
+    ret = entry_is_group_object(entry);
+    switch (ret) {
+    case LDB_SUCCESS:
+        /* it's a group object, continue */
+        break;
+
+    case LDB_ERR_NO_SUCH_ATTRIBUTE:
+        /* it is not a group object, just return */
+        return LDB_SUCCESS;
+
+    default:
+        /* an error occured, return */
+        return ret;
+    }
+
+    ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module),
+              LDB_DEBUG_TRACE,
+              "will delete %d ghost users from %d parents\n",
+              ghel->num_values, mbof->num_values);
+
+    for (i = 0; i < mbof->num_values; i++) {
+        valdn = ldb_dn_from_ldb_val(del_ctx->ghops,
+                                    ldb_module_get_ctx(del_ctx->ctx->module),
+                                    &mbof->values[i]);
+        if (!valdn || !ldb_dn_validate(valdn)) {
+            ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module),
+                      LDB_DEBUG_ERROR,
+                      "Invalid dn value: [%s]",
+                      (const char *)mbof->values[i].data);
+        }
+
+        ldb_debug(ldb_module_get_ctx(del_ctx->ctx->module),
+                  LDB_DEBUG_TRACE,
+                  "processing ghosts in parent [%s]\n",
+                  (const char *) mbof->values[i].data);
+
+        for (j = 0; j < ghel->num_values; j++) {
+            ret = mbof_append_muop(del_ctx, &del_ctx->ghops,
+                                   &del_ctx->num_ghops,
+                                   LDB_FLAG_MOD_DELETE,
+                                   valdn,
+                                   (const char *) ghel->values[j].data,
+                                   DB_GHOST);
+            if (ret != LDB_SUCCESS) {
+                return ret;
+            }
+        }
+    }
+
+    return LDB_SUCCESS;
+}
+
+/* del memberuid attributes from parent groups */
 static int mbof_del_muop(struct mbof_del_ctx *del_ctx)
 {
     struct ldb_context *ldb;
@@ -2411,6 +2525,98 @@ static int mbof_del_muop_callback(struct ldb_request *req,
         if (del_ctx->cur_muop < del_ctx->num_muops) {
             ret = mbof_del_muop(del_ctx);
         }
+        /* see if we need to remove some ghost users */
+        else if (del_ctx->ghops) {
+            return mbof_del_ghop(del_ctx);
+        }
+        /* see if there are follow functions to run */
+        else if (del_ctx->follow_mod) {
+            return mbof_mod_add(del_ctx->follow_mod,
+                                del_ctx->follow_mod->to_add);
+        }
+        else {
+            return ldb_module_done(ctx->req,
+                                   ctx->ret_ctrls,
+                                   ctx->ret_resp,
+                                   LDB_SUCCESS);
+        }
+
+        if (ret != LDB_SUCCESS) {
+            talloc_zfree(ares);
+            return ldb_module_done(ctx->req, NULL, NULL, ret);
+        }
+    }
+
+    talloc_zfree(ares);
+    return LDB_SUCCESS;
+}
+
+/* del ghost attributes from parent groups */
+static int mbof_del_ghop(struct mbof_del_ctx *del_ctx)
+{
+    struct ldb_context *ldb;
+    struct ldb_message *msg;
+    struct ldb_request *mod_req;
+    struct mbof_ctx *ctx;
+    int ret;
+
+    ctx = del_ctx->ctx;
+    ldb = ldb_module_get_ctx(ctx->module);
+
+    msg = ldb_msg_new(del_ctx);
+    if (!msg) return LDB_ERR_OPERATIONS_ERROR;
+
+    msg->dn = del_ctx->ghops[del_ctx->cur_ghop].dn;
+    msg->elements = del_ctx->ghops[del_ctx->cur_ghop].el;
+    msg->num_elements = 1;
+
+    ret = ldb_build_mod_req(&mod_req, ldb, del_ctx,
+                            msg, NULL,
+                            del_ctx, mbof_del_ghop_callback,
+                            ctx->req);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    return ldb_next_request(ctx->module, mod_req);
+}
+
+static int mbof_del_ghop_callback(struct ldb_request *req,
+                                  struct ldb_reply *ares)
+{
+    struct mbof_del_ctx *del_ctx;
+    struct mbof_ctx *ctx;
+    int ret;
+
+    del_ctx = talloc_get_type(req->context, struct mbof_del_ctx);
+    ctx = del_ctx->ctx;
+
+    if (!ares) {
+        return ldb_module_done(ctx->req, NULL, NULL,
+                               LDB_ERR_OPERATIONS_ERROR);
+    }
+    if (ares->error != LDB_SUCCESS) {
+        return ldb_module_done(ctx->req,
+                               ares->controls,
+                               ares->response,
+                               ares->error);
+    }
+
+    switch (ares->type) {
+    case LDB_REPLY_ENTRY:
+        /* shouldn't happen */
+        talloc_zfree(ares);
+        return ldb_module_done(ctx->req, NULL, NULL,
+                               LDB_ERR_OPERATIONS_ERROR);
+    case LDB_REPLY_REFERRAL:
+        /* ignore */
+        break;
+
+    case LDB_REPLY_DONE:
+        del_ctx->cur_ghop++;
+        if (del_ctx->cur_ghop < del_ctx->num_ghops) {
+            ret = mbof_del_ghop(del_ctx);
+        }
         /* see if there are follow functions to run */
         else if (del_ctx->follow_mod) {
             return mbof_mod_add(del_ctx->follow_mod,
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index 283e0845c15905166ad80f5f74061864ddfe8905..e7aa1757d64a6796f89bdbfed566dd9d96366bd2 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -2266,6 +2266,106 @@ START_TEST (test_sysdb_memberof_check_memberuid_loop_without_group_5)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_check_nested_ghosts)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    int ret;
+
+    /* Setup */
+    ret = setup_sysdb_tests(&test_ctx);
+    if (ret != EOK) {
+        fail("Could not set up the test");
+        return;
+    }
+
+    data = talloc_zero(test_ctx, struct test_data);
+    data->ctx = test_ctx;
+    data->ev = test_ctx->ev;
+    data->gid = _i;
+
+    data->attrlist = talloc_array(data, const char *, 2);
+    fail_unless(data->attrlist != NULL, "talloc_array failed.");
+    data->attrlist[0] = SYSDB_GHOST;
+    data->attrlist[1] = NULL;
+
+    ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                    data->gid,
+                                    data->attrlist, &data->msg);
+    fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid);
+
+    fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0,
+                "Wrong attribute name");
+    fail_unless(data->msg->elements[0].num_values == _i - MBO_GROUP_BASE + 1,
+                "Wrong number of attribute values, expected [%d] got [%d]",
+                _i + 1, data->msg->elements[0].num_values);
+
+    talloc_free(test_ctx);
+}
+END_TEST
+
+START_TEST (test_sysdb_memberof_remove_child_group_and_check_ghost)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    int ret;
+    gid_t delgid;
+
+    /* Setup */
+    ret = setup_sysdb_tests(&test_ctx);
+    if (ret != EOK) {
+        fail("Could not set up the test");
+        return;
+    }
+
+    data = talloc_zero(test_ctx, struct test_data);
+    data->ctx = test_ctx;
+    data->ev = test_ctx->ev;
+    data->gid = _i;
+    delgid = data->gid - 1;
+
+    data->attrlist = talloc_array(data, const char *, 2);
+    fail_unless(data->attrlist != NULL, "talloc_array failed.");
+    data->attrlist[0] = SYSDB_GHOST;
+    data->attrlist[1] = NULL;
+
+    ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                    data->gid,
+                                    data->attrlist, &data->msg);
+    fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid);
+
+    fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0,
+                "Wrong attribute name");
+
+    /* Expect our own and our parent's */
+    fail_unless(data->msg->elements[0].num_values == 2,
+                "Wrong number of attribute values, expected [%d] got [%d]",
+                2, data->msg->elements[0].num_values);
+
+    /* Remove the parent */
+    ret = sysdb_delete_group(data->ctx->sysdb, NULL, delgid);
+    fail_if(ret != EOK, "Cannot delete group %llu [%d]: %s\n",
+            (unsigned long long) data->gid, ret, strerror(ret));
+
+    talloc_free(data->msg);
+
+    /* Check the parent again. The inherited ghost user should be gone. */
+    ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                    data->gid, data->attrlist, &data->msg);
+    fail_if(ret != EOK, "Cannot retrieve group %llu\n", (unsigned long long) data->gid);
+
+    fail_unless(strcmp(data->msg->elements[0].name, SYSDB_GHOST) == 0,
+                "Wrong attribute name");
+
+    /* Expect our own now only */
+    fail_unless(data->msg->elements[0].num_values == 1,
+                "Wrong number of attribute values, expected [%d] got [%d]",
+                1, data->msg->elements[0].num_values);
+
+    talloc_free(test_ctx);
+}
+END_TEST
+
 START_TEST (test_sysdb_memberof_check_ghost)
 {
     struct sysdb_test_ctx *test_ctx;
@@ -4324,8 +4424,13 @@ Suite *create_sysdb_suite(void)
     /* Ghost users tests */
     tcase_add_loop_test(tc_memberof, test_sysdb_memberof_store_group_with_ghosts,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
-    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_nested_ghosts,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_remove_child_group_and_check_ghost,
+                        MBO_GROUP_BASE + 1, MBO_GROUP_BASE + 10);
+    /* Only one group should be left now */
+    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+                        MBO_GROUP_BASE + 9 , MBO_GROUP_BASE + 10);
 
     /* ghost users - RFC2307 */
     /* Add groups with ghost users */
-- 
1.8.0



More information about the sssd-devel mailing list