[SSSD] [PATCH] Ghost user related fixes to the memberof plugin

Jakub Hrozek jhrozek at redhat.com
Wed Dec 5 16:41:50 UTC 2012


On Wed, Dec 05, 2012 at 08:30:01AM -0500, Simo Sorce wrote:
> On Wed, 2012-12-05 at 10:18 +0100, Jakub Hrozek wrote:
> > On Wed, Dec 05, 2012 at 12:39:33AM -0500, Simo Sorce wrote:
> > > On Tue, 2012-12-04 at 21:39 +0100, Jakub Hrozek wrote:
> > > > Hi,
> > > > 
> > > > this thread obsoletes the previous memberof related threads into a single one
> > > > to make it easier for the reviewers to apply the patches without having to
> > > > guess the dependencies. The patches should apply cleanly on origin/master. In
> > > > addition to bringing all the patches together, this set also addresses some
> > > > of Simo's concerns, in particular the ghost users are better explained in
> > > > the big comment blocks above each major memberof operation.
> > > > 
> > > > The patches implement the missing operations of the memberof plugin
> > > > related to ghost users - delete and modify. The mod operation is given
> > > > some special care because not only we need to propagate the modification
> > > > to parent groups, but we also need to retain the inherited ghost attributes.
> > > > 
> > > > The intent is to fix issues such as #1652. Most patches also include
> > > > unit tests to make sure we don't break existing memberof plugin
> > > > functionality.
> > > > 
> > > > More information is in the patches themselves or in the commit messages.
> > > 
> > > The first 5 patches get a tentative ack from me.
> > > They look good and they compile fine and a basic smoke test seem to show
> > > no issue whatsoever, also make check works as expected.
> > > 
> > > On the last patch I have a question. I see that you do a number of base
> > > searches for each hild of a group in order to source ghost users. But
> > > that seem a bit wasteful.
> > > 
> > > Why don't you simply do a subtree search on the whole tree with
> > > (&(objectclass=group)(memberof=<dn of the group you are handling>)) (you
> > > might also add a &(ghost=*) ) ?
> > > 
> > > This would give you all groups that are 'children' of the group you are
> > > actually handling with a single search and avoids searching user entries
> > > and then immediately discarding them as they have no ghost members.
> > > 
> > > Simo.
> > 
> > Mostly because memberof is transitive so I thought I might get too
> > many "ghost" duplicates in case the nesting went too deep which would
> > need to be filtered later (using the muop operations).
> 
> Sorry not sure I understand what you mean here, the number of users in
> the typical case is much higher than groups, I wouldn't worry in
> processing groups that add only duplicates.
> 
> > On the other hand, in the typical case, there would probably *many* more
> > user entries to discard when traversing direct members rather than
> > memberofs, so you might be right..
> 
> IT really depends on how many users are actually fetched, if you have
> many ghost you'll probably have few users, but then if you have few
> ghost it may be because a lot of users were actually resolved.
> 
> On the filtering of ghosts I was wondering if it wouldn't be faster to
> sort ghosts by name in the structure where you gather them. It would
> make searching for a match faster if you then use a bisection algorithm,
> or use a simple hash table :)
> This optimization doesn't need to be done right now, but maybe we open a
> ticket for future improvement.

As it turned out, it is really needed to filter out duplicated at the
time we gather them. The attached new iteration of patches uses hash
table to accomplish this.
-------------- next part --------------
>From 523afa453c722b8883680f1f8573e0391bc8c525 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 1/6] 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.

When a ghost entry is removed from a group, all its parent groups are
expired from the cache by setting the expire timestamp to 1. Doing so
would force the SSSD to re-read the group next time it is requested in
order to make sure its members are really up-to-date.
---
 src/ldb_modules/memberof.c | 262 +++++++++++++++++++++++++++++++++++++++++++--
 src/tests/sysdb-tests.c    | 107 +++++++++++++++++-
 2 files changed, 362 insertions(+), 7 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index 2c0eac0dadf9b1762ad13c45f3792bf7dbf622f7..f7b4fddbe9c4a636d78d966be292b65a30dd093f 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -33,6 +33,8 @@
 #define DB_MEMBERUID "memberuid"
 #define DB_NAME "name"
 #define DB_USER_CLASS "user"
+#define DB_GROUP_CLASS "group"
+#define DB_CACHE_EXPIRE "dataExpireTimestamp"
 #define DB_OC "objectClass"
 
 #ifndef MAX
@@ -129,6 +131,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;
 };
@@ -161,7 +167,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;
@@ -175,7 +182,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;
         }
     }
@@ -183,6 +190,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,
@@ -285,7 +302,19 @@ static int mbof_append_muop(TALLOC_CTX *memctx,
  * we proceed to store the user name.
  *
  * At the end we will add a memberuid attribute to our new object that
- * includes all direct and indeirect user members names.
+ * includes all direct and indirect user members names.
+ *
+ * Group objects can also contain a "ghost" attribute. A ghost attribute
+ * represents a user that is a member of the group but has not yet been
+ * looked up so there is no real user entry with member/memberof links.
+ *
+ * If an object being added contains a "ghost" attribute, the ghost attribute
+ * is in turn copied to all parents of that object so that retrieving a
+ * group returns both its direct and indirect members. The ghost attribute is
+ * similar to the memberuid attribute in many respects. One difference is that
+ * the memberuid attribute is completely generated and managed by the memberof
+ * plugin - in contrast, the ghost attribute is added to the entry that "owns"
+ * it and only propagated to parent groups.
  */
 
 static int mbof_append_addop(struct mbof_add_ctx *add_ctx,
@@ -1148,7 +1177,14 @@ static int mbof_add_muop_callback(struct ldb_request *req,
  * understand how we proceed to select which new operation to process.
  *
  * As a final operation remove any memberuid corresponding to a removal of
- * a memberof field from a user entry
+ * a memberof field from a user entry. Also if the original entry had a ghost
+ * attribute, we need to remove that attribute from all its parents as well.
+ *
+ * There is one catch though - at the memberof level, we can't know if the
+ * attribute being removed from a parent group is just inherited from the group
+ * being removed or also a direct member of the parent group. To make sure
+ * that the attribute is displayed next time the group is requested, we also
+ * set expire the parent group at the same time.
  */
 
 static int mbof_del_search_callback(struct ldb_request *req,
@@ -1177,16 +1213,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;
@@ -1411,6 +1453,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);
     }
@@ -1422,6 +1471,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,
@@ -1533,6 +1586,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,
@@ -2213,6 +2270,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,
@@ -2341,7 +2402,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;
@@ -2407,6 +2541,122 @@ 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;
+
+    ret = ldb_msg_add(msg, del_ctx->ghops[del_ctx->cur_ghop].el,
+                      LDB_FLAG_MOD_DELETE);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    /* Also expire any parent groups to force reloading direct members in
+     * case the ghost users we remove now were actually *also* direct members
+     * of the parent groups
+     */
+    ret = ldb_msg_add_empty(msg, DB_CACHE_EXPIRE, LDB_FLAG_MOD_REPLACE, NULL);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    ret = ldb_msg_add_string(msg, DB_CACHE_EXPIRE, "1");
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    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);
+    }
+
+    /* We must treat no such attribute as non-fatal b/c the entry
+     * might have been directly nested in the parent as well and
+     * updated with another replace operation.
+     */
+    if (ares->error != LDB_SUCCESS &&  \
+        ares->error != LDB_ERR_NO_SUCH_ATTRIBUTE) {
+        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 74b75233d7a5cf027ffdf581cbc9942ba2cc2b95..7b69876ff0dc3dba5168d6f3ac096526bf7ff576 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -2267,6 +2267,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;
@@ -4331,8 +4431,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.1

-------------- next part --------------
>From 147cc43cbeba39252aaed3cb5882f59417732cf8 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 26 Nov 2012 13:18:59 +0100
Subject: [PATCH 2/6] MEMBEROF: split processing the member modify into a
 separate function

This will allow to process ghost users in a similar fashion
---
 src/ldb_modules/memberof.c | 120 +++++++++++++++++++++++++++------------------
 1 file changed, 73 insertions(+), 47 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index f7b4fddbe9c4a636d78d966be292b65a30dd093f..68b340cfd03714ad2f4192eccf6fb832486c644a 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -145,7 +145,8 @@ struct mbof_mod_ctx {
     const struct ldb_message_element *membel;
     struct ldb_message *entry;
 
-    struct mbof_dn_array *to_add;
+    struct mbof_dn_array *mb_add;
+    struct mbof_dn_array *mb_remove;
 
     struct ldb_message *msg;
     bool terminate;
@@ -2277,7 +2278,7 @@ static int mbof_del_progeny(struct mbof_del_operation *delop)
     /* see if there are follow functions to run */
     if (del_ctx->follow_mod) {
         return mbof_mod_add(del_ctx->follow_mod,
-                            del_ctx->follow_mod->to_add);
+                            del_ctx->follow_mod->mb_add);
     }
 
     /* ok, no more ops, this means our job is done */
@@ -2548,7 +2549,7 @@ static int mbof_del_muop_callback(struct ldb_request *req,
         /* 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);
+                                del_ctx->follow_mod->mb_add);
         }
         else {
             return ldb_module_done(ctx->req,
@@ -2660,7 +2661,7 @@ static int mbof_del_ghop_callback(struct ldb_request *req,
         /* 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);
+                                del_ctx->follow_mod->mb_add);
         }
         else {
             return ldb_module_done(ctx->req,
@@ -2708,6 +2709,11 @@ static int mbof_orig_mod(struct mbof_mod_ctx *mod_ctx);
 static int mbof_orig_mod_callback(struct ldb_request *req,
                                   struct ldb_reply *ares);
 static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done);
+static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
+                                   struct ldb_message *entry,
+                                   const struct ldb_message_element *membel,
+                                   struct mbof_dn_array **_added,
+                                   struct mbof_dn_array **_removed);
 static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
                            struct mbof_dn_array *del);
 static int mbof_fill_dn_array(TALLOC_CTX *memctx,
@@ -2931,63 +2937,99 @@ static int mbof_orig_mod_callback(struct ldb_request *req,
 
 static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done)
 {
-    const struct ldb_message_element *el;
     struct ldb_context *ldb;
     struct mbof_ctx *ctx;
-    struct mbof_dn_array *removed;
-    struct mbof_dn_array *added;
-    int i, j, ret;
+    int ret;
 
     ctx = mod_ctx->ctx;
     ldb = ldb_module_get_ctx(ctx->module);
 
-    switch (mod_ctx->membel->flags) {
+    ret = mbof_mod_process_membel(mod_ctx, ldb, mod_ctx->entry, mod_ctx->membel,
+                                  &mod_ctx->mb_add, &mod_ctx->mb_remove);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    /* Process the operations */
+    /* if we have something to remove do it first */
+    if (mod_ctx->mb_remove && mod_ctx->mb_remove->num) {
+        return mbof_mod_delete(mod_ctx, mod_ctx->mb_remove);
+    }
+
+    /* if there is nothing to remove and we have stuff to add
+     * do it right away */
+    if (mod_ctx->mb_add && mod_ctx->mb_add->num) {
+        return mbof_mod_add(mod_ctx, mod_ctx->mb_add);
+    }
+
+    /* the replacement function resulted in a null op,
+     * nothing to do, return happily */
+    *done = true;
+    return LDB_SUCCESS;
+}
+
+static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx,
+                                   struct ldb_context *ldb,
+                                   struct ldb_message *entry,
+                                   const struct ldb_message_element *membel,
+                                   struct mbof_dn_array **_added,
+                                   struct mbof_dn_array **_removed)
+{
+    const struct ldb_message_element *el;
+    struct mbof_dn_array *removed = NULL;
+    struct mbof_dn_array *added = NULL;
+    int i, j, ret;
+
+    if (!membel) {
+        /* Nothing to do.. */
+        return LDB_SUCCESS;
+    }
+
+    switch (membel->flags) {
     case LDB_FLAG_MOD_ADD:
 
-        ret = mbof_fill_dn_array(mod_ctx, ldb, mod_ctx->membel, &added);
+        ret = mbof_fill_dn_array(mem_ctx, ldb, membel, &added);
         if (ret != LDB_SUCCESS) {
             return ret;
         }
-
-        return mbof_mod_add(mod_ctx, added);
+        break;
 
     case LDB_FLAG_MOD_DELETE:
 
-        if (mod_ctx->membel->num_values == 0) {
-            el = ldb_msg_find_element(mod_ctx->entry, DB_MEMBER);
+        if (membel->num_values == 0) {
+            el = ldb_msg_find_element(entry, DB_MEMBER);
         } else {
-            el = mod_ctx->membel;
+            el = membel;
         }
 
         if (!el) {
             /* nothing to do really */
-            *done = true;
-            return LDB_SUCCESS;
+            break;
         }
 
-        ret = mbof_fill_dn_array(mod_ctx, ldb, el, &removed);
+        ret = mbof_fill_dn_array(mem_ctx, ldb, el, &removed);
         if (ret != LDB_SUCCESS) {
             return ret;
         }
-
-        return mbof_mod_delete(mod_ctx, removed);
+        break;
 
     case LDB_FLAG_MOD_REPLACE:
 
         removed = NULL;
-        el = ldb_msg_find_element(mod_ctx->entry, DB_MEMBER);
+        el = ldb_msg_find_element(entry, DB_MEMBER);
         if (el) {
-            ret = mbof_fill_dn_array(mod_ctx, ldb, el, &removed);
+            ret = mbof_fill_dn_array(mem_ctx, ldb, el, &removed);
             if (ret != LDB_SUCCESS) {
                 return ret;
             }
         }
 
         added = NULL;
-        el = mod_ctx->membel;
+        el = membel;
         if (el) {
-            ret = mbof_fill_dn_array(mod_ctx, ldb, el, &added);
+            ret = mbof_fill_dn_array(mem_ctx, ldb, el, &added);
             if (ret != LDB_SUCCESS) {
+                talloc_free(removed);
                 return ret;
             }
         }
@@ -3014,31 +3056,15 @@ static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done)
                 }
             }
         }
+        break;
 
-        /* if we need to add something put it away so that it
-         * can be done after all delete operations are over */
-        if (added && added->num) {
-            mod_ctx->to_add = added;
-        }
-
-        /* if we have something to remove do it first */
-        if (removed && removed->num) {
-            return mbof_mod_delete(mod_ctx, removed);
-        }
-
-        /* if there is nothing to remove and we have stuff to add
-         * do it right away */
-        if (mod_ctx->to_add) {
-            return mbof_mod_add(mod_ctx, added);
-        }
-
-        /* the replacement function resulted in a null op,
-         * nothing to do, return happily */
-        *done = true;
-        return LDB_SUCCESS;
+    default:
+        return LDB_ERR_OPERATIONS_ERROR;
     }
 
-    return LDB_ERR_OPERATIONS_ERROR;
+    *_added = added;
+    *_removed = removed;
+    return LDB_SUCCESS;
 }
 
 static int mbof_mod_add(struct mbof_mod_ctx *mod_ctx,
@@ -3126,7 +3152,7 @@ static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
     }
 
     /* add followup function if we also have stuff to add */
-    if (mod_ctx->to_add) {
+    if (mod_ctx->mb_add) {
         del_ctx->follow_mod = mod_ctx;
     }
 
-- 
1.8.0.1

-------------- next part --------------
>From ac922c8bfa5c7702054c3566d57de14370175693 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 26 Nov 2012 18:22:12 +0100
Subject: [PATCH 3/6] MEMBEROF: Split the del ghost attribute op into a
 reusable function

This new function is going to be reused by the modify operation
---
 src/ldb_modules/memberof.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index 68b340cfd03714ad2f4192eccf6fb832486c644a..57336191084f878f2ee7384126828a7baddf299d 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -2403,11 +2403,12 @@ static int mbof_del_fill_muop(struct mbof_del_ctx *del_ctx,
     return LDB_SUCCESS;
 }
 
-static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
-                              struct ldb_message *entry)
+static int mbof_del_fill_ghop_ex(struct mbof_del_ctx *del_ctx,
+                                 struct ldb_message *entry,
+                                 struct ldb_val *ghvals,
+                                 unsigned int num_gh_vals)
 {
     struct ldb_message_element *mbof;
-    struct ldb_message_element *ghel;
     struct ldb_dn *valdn;
     int ret;
     int i, j;
@@ -2418,12 +2419,6 @@ static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
         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:
@@ -2442,7 +2437,7 @@ static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
     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);
+              num_gh_vals, mbof->num_values);
 
     for (i = 0; i < mbof->num_values; i++) {
         valdn = ldb_dn_from_ldb_val(del_ctx->ghops,
@@ -2460,12 +2455,12 @@ static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
                   "processing ghosts in parent [%s]\n",
                   (const char *) mbof->values[i].data);
 
-        for (j = 0; j < ghel->num_values; j++) {
+        for (j = 0; j < num_gh_vals; 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,
+                                   (const char *) ghvals[j].data,
                                    DB_GHOST);
             if (ret != LDB_SUCCESS) {
                 return ret;
@@ -2476,6 +2471,21 @@ static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
     return LDB_SUCCESS;
 }
 
+static int mbof_del_fill_ghop(struct mbof_del_ctx *del_ctx,
+                              struct ldb_message *entry)
+{
+    struct ldb_message_element *ghel;
+
+    ghel = ldb_msg_find_element(entry, DB_GHOST);
+    if (ghel == NULL || ghel->num_values == 0) {
+        /* No ghel attribute, just return success */
+        return LDB_SUCCESS;
+    }
+
+    return mbof_del_fill_ghop_ex(del_ctx, entry,
+                                 ghel->values, ghel->num_values);
+}
+
 /* del memberuid attributes from parent groups */
 static int mbof_del_muop(struct mbof_del_ctx *del_ctx)
 {
-- 
1.8.0.1

-------------- next part --------------
>From 4cf74f862a3cfe44340acc91ea6296a9244f2594 Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 27 Nov 2012 16:06:42 +0100
Subject: [PATCH 4/6] MEMBEROF: Split the add ghost operation into a separate
 function

This new function will be reused by the modify operation later
---
 src/ldb_modules/memberof.c | 90 +++++++++++++++++++++++++++++++++++++---------
 1 file changed, 73 insertions(+), 17 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index 57336191084f878f2ee7384126828a7baddf299d..4899b03c67183db2081cca593a677856d1a07e4b 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -361,6 +361,57 @@ static int mbof_append_addop(struct mbof_add_ctx *add_ctx,
     return LDB_SUCCESS;
 }
 
+static int mbof_add_fill_ghop_ex(struct mbof_add_ctx *add_ctx,
+                                 struct ldb_message *entry,
+                                 struct mbof_dn_array *parents,
+                                 struct ldb_val *ghvals,
+                                 unsigned int num_gh_vals)
+{
+    int ret;
+    int i, j;
+
+    if (!parents || parents->num == 0) {
+        /* no parents attributes ... */
+        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(add_ctx->ctx->module),
+              LDB_DEBUG_TRACE,
+              "will add %d ghost users to %d parents\n",
+              num_gh_vals, parents->num);
+
+    for (i = 0; i < parents->num; i++) {
+        for (j = 0; j < num_gh_vals; j++) {
+            ret = mbof_append_muop(add_ctx, &add_ctx->muops,
+                                   &add_ctx->num_muops,
+                                   LDB_FLAG_MOD_ADD,
+                                   parents->dns[i],
+                                   (const char *) ghvals[j].data,
+                                   DB_GHOST);
+            if (ret != LDB_SUCCESS) {
+                return ret;
+            }
+        }
+    }
+
+    return LDB_SUCCESS;
+}
+
 static int memberof_recompute_task(struct ldb_module *module,
                                    struct ldb_request *req);
 
@@ -370,6 +421,9 @@ static int mbof_next_add(struct mbof_add_operation *addop);
 static int mbof_next_add_callback(struct ldb_request *req,
                                   struct ldb_reply *ares);
 static int mbof_add_operation(struct mbof_add_operation *addop);
+static int mbof_add_fill_ghop(struct mbof_add_ctx *add_ctx,
+                              struct ldb_message *entry,
+                              struct mbof_dn_array *parents);
 static int mbof_add_missing(struct mbof_add_ctx *add_ctx, struct ldb_dn *dn);
 static int mbof_add_cleanup(struct mbof_add_ctx *add_ctx);
 static int mbof_add_cleanup_callback(struct ldb_request *req,
@@ -836,23 +890,9 @@ static int mbof_add_operation(struct mbof_add_operation *addop)
         return ret;
     }
 
-    el = ldb_msg_find_element(addop->entry, DB_GHOST);
-    if (el) {
-        for (i = 0; i < el->num_values; i++) {
-            /* add ghost to all group's parents */
-            for (j = 0; j < parents->num; j++) {
-                ret = mbof_append_muop(add_ctx, &add_ctx->muops,
-                                       &add_ctx->num_muops,
-                                       LDB_FLAG_MOD_ADD,
-                                       parents->dns[j],
-                                       (char *)el->values[i].data,
-                                       DB_GHOST);
-                if (ret != LDB_SUCCESS) {
-                    return ret;
-                }
-            }
-
-        }
+    ret = mbof_add_fill_ghop(add_ctx, addop->entry, parents);
+    if (ret != LDB_SUCCESS) {
+        return ret;
     }
 
     /* we are done with the entry now */
@@ -897,6 +937,22 @@ static int mbof_add_operation(struct mbof_add_operation *addop)
     return ldb_next_request(ctx->module, mod_req);
 }
 
+static int mbof_add_fill_ghop(struct mbof_add_ctx *add_ctx,
+                              struct ldb_message *entry,
+                              struct mbof_dn_array *parents)
+{
+    struct ldb_message_element *ghel;
+
+    ghel = ldb_msg_find_element(entry, DB_GHOST);
+    if (ghel == NULL || ghel->num_values == 0) {
+        /* No ghel attribute, just return success */
+        return LDB_SUCCESS;
+    }
+
+    return mbof_add_fill_ghop_ex(add_ctx, entry, parents,
+                                 ghel->values, ghel->num_values);
+}
+
 static int mbof_add_missing(struct mbof_add_ctx *add_ctx, struct ldb_dn *dn)
 {
     struct mbof_dn *mdn;
-- 
1.8.0.1

-------------- next part --------------
>From 47b772b68553bd516ceb9a3a75615d49149b6add Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Tue, 27 Nov 2012 16:09:23 +0100
Subject: [PATCH 5/6] MEMBEROF: Implement the modify operation for ghost users

Similar to the add and delete operation, we also need to propagate the
changes of the ghost user attribute to the parent groups so that if a
nested group updates memberships, its parents also get the membership
updated.
---
 src/ldb_modules/memberof.c | 285 +++++++++++++++++++++++----
 src/tests/sysdb-tests.c    | 480 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 722 insertions(+), 43 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index 4899b03c67183db2081cca593a677856d1a07e4b..cb6f8113f2b863cc6f8758f7de3baa5b38e053e8 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -41,6 +41,11 @@
 #define MAX(a,b) (((a) > (b)) ? (a) : (b))
 #endif
 
+struct mbof_val_array {
+    struct ldb_val *vals;
+    int num;
+};
+
 struct mbof_dn_array {
     struct ldb_dn **dns;
     int num;
@@ -143,11 +148,15 @@ struct mbof_mod_ctx {
     struct mbof_ctx *ctx;
 
     const struct ldb_message_element *membel;
+    const struct ldb_message_element *ghel;
     struct ldb_message *entry;
 
     struct mbof_dn_array *mb_add;
     struct mbof_dn_array *mb_remove;
 
+    struct mbof_val_array *gh_add;
+    struct mbof_val_array *gh_remove;
+
     struct ldb_message *msg;
     bool terminate;
 };
@@ -2275,7 +2284,8 @@ static int mbof_del_mod_callback(struct ldb_request *req,
 }
 
 static int mbof_mod_add(struct mbof_mod_ctx *mod_ctx,
-                        struct mbof_dn_array *ael);
+                        struct mbof_dn_array *ael,
+                        struct mbof_val_array *addgh);
 
 static int mbof_del_progeny(struct mbof_del_operation *delop)
 {
@@ -2334,7 +2344,8 @@ static int mbof_del_progeny(struct mbof_del_operation *delop)
     /* see if there are follow functions to run */
     if (del_ctx->follow_mod) {
         return mbof_mod_add(del_ctx->follow_mod,
-                            del_ctx->follow_mod->mb_add);
+                            del_ctx->follow_mod->mb_add,
+                            del_ctx->follow_mod->gh_add);
     }
 
     /* ok, no more ops, this means our job is done */
@@ -2615,7 +2626,8 @@ static int mbof_del_muop_callback(struct ldb_request *req,
         /* 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->mb_add);
+                                del_ctx->follow_mod->mb_add,
+                                del_ctx->follow_mod->gh_add);
         }
         else {
             return ldb_module_done(ctx->req,
@@ -2727,7 +2739,8 @@ static int mbof_del_ghop_callback(struct ldb_request *req,
         /* 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->mb_add);
+                                del_ctx->follow_mod->mb_add,
+                                del_ctx->follow_mod->gh_add);
         }
         else {
             return ldb_module_done(ctx->req,
@@ -2780,19 +2793,30 @@ static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
                                    const struct ldb_message_element *membel,
                                    struct mbof_dn_array **_added,
                                    struct mbof_dn_array **_removed);
+static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
+                                 struct ldb_message *entry,
+                                 const struct ldb_message_element *membel,
+                                 struct mbof_val_array **_added,
+                                 struct mbof_val_array **_removed);
 static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
-                           struct mbof_dn_array *del);
+                           struct mbof_dn_array *del,
+                           struct mbof_val_array *delgh);
 static int mbof_fill_dn_array(TALLOC_CTX *memctx,
                               struct ldb_context *ldb,
                               const struct ldb_message_element *el,
                               struct mbof_dn_array **dn_array);
+static int mbof_fill_vals_array(TALLOC_CTX *memctx,
+                                struct ldb_context *ldb,
+                                const struct ldb_message_element *el,
+                                struct mbof_val_array **val_array);
 
 static int memberof_mod(struct ldb_module *module, struct ldb_request *req)
 {
     struct ldb_message_element *el;
     struct mbof_mod_ctx *mod_ctx;
     struct mbof_ctx *ctx;
-    static const char *attrs[] = {DB_MEMBER, DB_MEMBEROF, NULL};
+    static const char *attrs[] = { DB_OC, DB_GHOST,
+                                   DB_MEMBER, DB_MEMBEROF, NULL};
     struct ldb_context *ldb = ldb_module_get_ctx(module);
     struct ldb_request *search;
     int ret;
@@ -2835,15 +2859,15 @@ static int memberof_mod(struct ldb_module *module, struct ldb_request *req)
         return LDB_ERR_OPERATIONS_ERROR;
     }
 
-    /* continue with normal ops if there are no members */
-    el = ldb_msg_find_element(mod_ctx->msg, DB_MEMBER);
-    if (!el) {
+    mod_ctx->membel = ldb_msg_find_element(mod_ctx->msg, DB_MEMBER);
+    mod_ctx->ghel = ldb_msg_find_element(mod_ctx->msg, DB_GHOST);
+
+    /* continue with normal ops if there are no members and no ghosts */
+    if (mod_ctx->membel == NULL && mod_ctx->ghel == NULL) {
         mod_ctx->terminate = true;
         return mbof_orig_mod(mod_ctx);
     }
 
-    mod_ctx->membel = el;
-
     /* can't do anything,
      * must check first what's on the entry */
     ret = ldb_build_search_req(&search, ldb, mod_ctx,
@@ -3016,16 +3040,24 @@ static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done)
         return ret;
     }
 
+    ret = mbof_mod_process_ghel(mod_ctx, ldb, mod_ctx->entry, mod_ctx->ghel,
+                                &mod_ctx->gh_add, &mod_ctx->gh_remove);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
     /* Process the operations */
     /* if we have something to remove do it first */
-    if (mod_ctx->mb_remove && mod_ctx->mb_remove->num) {
-        return mbof_mod_delete(mod_ctx, mod_ctx->mb_remove);
+    if ((mod_ctx->mb_remove && mod_ctx->mb_remove->num) ||
+        (mod_ctx->gh_remove && mod_ctx->gh_remove->num)) {
+        return mbof_mod_delete(mod_ctx, mod_ctx->mb_remove, mod_ctx->gh_remove);
     }
 
     /* if there is nothing to remove and we have stuff to add
      * do it right away */
-    if (mod_ctx->mb_add && mod_ctx->mb_add->num) {
-        return mbof_mod_add(mod_ctx, mod_ctx->mb_add);
+    if ((mod_ctx->mb_add && mod_ctx->mb_add->num) ||
+        (mod_ctx->gh_add && mod_ctx->gh_add->num)) {
+        return mbof_mod_add(mod_ctx, mod_ctx->mb_add, mod_ctx->gh_add);
     }
 
     /* the replacement function resulted in a null op,
@@ -3133,8 +3165,109 @@ static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx,
     return LDB_SUCCESS;
 }
 
+static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
+                                 struct ldb_message *entry,
+                                 const struct ldb_message_element *ghel,
+                                 struct mbof_val_array **_added,
+                                 struct mbof_val_array **_removed)
+{
+    const struct ldb_message_element *el;
+    struct mbof_val_array *removed = NULL;
+    struct mbof_val_array *added = NULL;
+    int i, j, ret;
+
+    if (!ghel) {
+        /* Nothing to do.. */
+        return LDB_SUCCESS;
+    }
+
+    el = ldb_msg_find_element(entry, DB_MEMBEROF);
+    if (!el || el->num_values == 0) {
+        /* no memberof attributes ... */
+        return LDB_SUCCESS;
+    }
+
+    switch (ghel->flags) {
+    case LDB_FLAG_MOD_ADD:
+        ret = mbof_fill_vals_array(mem_ctx, ldb, ghel, &added);
+        if (ret != LDB_SUCCESS) {
+            return ret;
+        }
+        break;
+
+    case LDB_FLAG_MOD_DELETE:
+        if (ghel->num_values == 0) {
+            el = ldb_msg_find_element(entry, DB_GHOST);
+        } else {
+            el = ghel;
+        }
+
+        if (!el) {
+            /* nothing to do really */
+            break;
+        }
+
+        ret = mbof_fill_vals_array(mem_ctx, ldb, ghel, &removed);
+        if (ret != LDB_SUCCESS) {
+            return ret;
+        }
+        break;
+
+    case LDB_FLAG_MOD_REPLACE:
+        el = ldb_msg_find_element(entry, DB_GHOST);
+        if (el) {
+            ret = mbof_fill_vals_array(mem_ctx, ldb, el, &removed);
+            if (ret != LDB_SUCCESS) {
+                return ret;
+            }
+        }
+
+        el = ghel;
+        if (el) {
+            ret = mbof_fill_vals_array(mem_ctx, ldb, el, &added);
+            if (ret != LDB_SUCCESS) {
+                talloc_free(removed);
+                return ret;
+            }
+        }
+
+        /* remove from arrays values that ended up unchanged */
+        if (removed && removed->num && added && added->num) {
+            for (i = 0; i < added->num; i++) {
+                for (j = 0; j < removed->num; j++) {
+                    if (strcmp((const char *) added->vals[i].data,
+                               (const char *) removed->vals[j].data) == 0) {
+                        break;
+                    }
+                }
+                if (j < removed->num) {
+                    /* preexisting one, not removed, nor added */
+                    for (; j+1 < removed->num; j++) {
+                        removed->vals[j] = removed->vals[j+1];
+                    }
+                    removed->num--;
+                    for (j = i; j+1 < added->num; j++) {
+                        added->vals[j] = added->vals[j+1];
+                    }
+                    added->num--;
+                    i--;
+                }
+            }
+        }
+        break;
+
+    default:
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+    *_added = added;
+    *_removed = removed;
+    return LDB_SUCCESS;
+}
+
 static int mbof_mod_add(struct mbof_mod_ctx *mod_ctx,
-                        struct mbof_dn_array *ael)
+                        struct mbof_dn_array *ael,
+                        struct mbof_val_array *addgh)
 {
     const struct ldb_message_element *el;
     struct mbof_dn_array *parents;
@@ -3154,14 +3287,6 @@ static int mbof_mod_add(struct mbof_mod_ctx *mod_ctx,
         return ret;
     }
 
-    parents->dns = talloc_realloc(parents, parents->dns,
-                                  struct ldb_dn *, parents->num + 1);
-    if (!parents->dns) {
-        return LDB_ERR_OPERATIONS_ERROR;
-    }
-    parents->dns[parents->num] = mod_ctx->entry->dn;
-    parents->num++;
-
     add_ctx = talloc_zero(mod_ctx, struct mbof_add_ctx);
     if (!add_ctx) {
         return LDB_ERR_OPERATIONS_ERROR;
@@ -3169,18 +3294,42 @@ static int mbof_mod_add(struct mbof_mod_ctx *mod_ctx,
     add_ctx->ctx = ctx;
     add_ctx->msg_dn = mod_ctx->msg->dn;
 
-    for (i = 0; i < ael->num; i++) {
-        ret = mbof_append_addop(add_ctx, parents, ael->dns[i]);
+    if (addgh != NULL) {
+        /* Build the memberuid add op */
+        ret =  mbof_add_fill_ghop_ex(add_ctx, mod_ctx->entry,
+                                     parents, addgh->vals, addgh->num);
         if (ret != LDB_SUCCESS) {
             return ret;
         }
     }
 
-    return mbof_next_add(add_ctx->add_list);
+    if (ael != NULL) {
+        /* Add itself to the list of the parents to also get the memberuid */
+        parents->dns = talloc_realloc(parents, parents->dns,
+                                    struct ldb_dn *, parents->num + 1);
+        if (!parents->dns) {
+            return LDB_ERR_OPERATIONS_ERROR;
+        }
+        parents->dns[parents->num] = mod_ctx->entry->dn;
+        parents->num++;
+
+        /* Build the member-add array */
+        for (i = 0; i < ael->num; i++) {
+            ret = mbof_append_addop(add_ctx, parents, ael->dns[i]);
+            if (ret != LDB_SUCCESS) {
+                return ret;
+            }
+        }
+
+        return mbof_next_add(add_ctx->add_list);
+    }
+
+    return mbof_add_muop(add_ctx);
 }
 
 static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
-                           struct mbof_dn_array *del)
+                           struct mbof_dn_array *del,
+                           struct mbof_val_array *delgh)
 {
     struct mbof_del_operation *first;
     struct mbof_del_ctx *del_ctx;
@@ -3205,25 +3354,39 @@ static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
     }
     del_ctx->first = first;
 
-    first->del_ctx = del_ctx;
-    first->entry = mod_ctx->entry;
-    first->entry_dn = mod_ctx->entry->dn;
-
-    /* prepare del sets */
-    for (i = 0; i < del->num; i++) {
-        ret = mbof_append_delop(first, del->dns[i]);
-        if (ret != LDB_SUCCESS) {
-            return ret;
-        }
-    }
-
     /* add followup function if we also have stuff to add */
-    if (mod_ctx->mb_add) {
+    if ((mod_ctx->mb_add && mod_ctx->mb_add->num > 0) ||
+        (mod_ctx->gh_add && mod_ctx->gh_add->num > 0)) {
         del_ctx->follow_mod = mod_ctx;
     }
 
-    /* now that sets are built, start processing */
-    return mbof_del_execute_op(first->children[0]);
+    first->del_ctx = del_ctx;
+    first->entry = mod_ctx->entry;
+    first->entry_dn = mod_ctx->entry->dn;
+
+    if (delgh != NULL) {
+        ret = mbof_del_fill_ghop_ex(del_ctx, del_ctx->first->entry,
+                                    delgh->vals, delgh->num);
+        if (ret != LDB_SUCCESS) {
+            return ret;
+        }
+    }
+
+    /* prepare del sets */
+    if (del != NULL) {
+        for (i = 0; i < del->num; i++) {
+            ret = mbof_append_delop(first, del->dns[i]);
+            if (ret != LDB_SUCCESS) {
+                return ret;
+            }
+        }
+
+        /* now that sets are built, start processing */
+        return mbof_del_execute_op(first->children[0]);
+    }
+
+    /* No member processing, just delete ghosts */
+    return mbof_del_ghop(del_ctx);
 }
 
 static int mbof_fill_dn_array(TALLOC_CTX *memctx,
@@ -3264,6 +3427,42 @@ static int mbof_fill_dn_array(TALLOC_CTX *memctx,
     return LDB_SUCCESS;
 }
 
+static int mbof_fill_vals_array(TALLOC_CTX *memctx,
+                                struct ldb_context *ldb,
+                                const struct ldb_message_element *el,
+                                struct mbof_val_array **val_array)
+{
+    struct mbof_val_array *var;
+    int i;
+
+    var = talloc_zero(memctx, struct mbof_val_array);
+    if (!var) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+    *val_array = var;
+
+    if (!el || el->num_values == 0) {
+        return LDB_SUCCESS;
+    }
+
+    var->vals = talloc_array(var, struct ldb_val, el->num_values);
+    if (!var->vals) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+    var->num = el->num_values;
+
+    for (i = 0; i < var->num; i++) {
+        var->vals[i].length = strlen((const char *) el->values[i].data);
+        var->vals[i].data = (uint8_t *) talloc_strdup(var,
+                                          (const char *) el->values[i].data);
+        if (var->vals[i].data == NULL) {
+            return LDB_ERR_OPERATIONS_ERROR;
+        }
+    }
+
+    return LDB_SUCCESS;
+}
+
 
 /*************************
  * Cleanup task routines *
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index 7b69876ff0dc3dba5168d6f3ac096526bf7ff576..d513c30f9b370d030bea72901eacdec8eaeb0e87 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -1996,6 +1996,190 @@ START_TEST (test_sysdb_memberof_store_group_with_ghosts)
 }
 END_TEST
 
+
+START_TEST (test_sysdb_memberof_mod_add)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+    gid_t itergid;
+
+    /* 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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    ghostname = talloc_asprintf(data, "testghost%d", _i);
+    fail_unless(ghostname != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    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;
+
+    /* Before the add, the groups should not contain the ghost attribute */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname;
+        gv.length = strlen(ghostname);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        if (data->gid > MBO_GROUP_BASE) {
+            /* The first group would have the ghost attribute gone completely */
+            fail_if(el == NULL, "Cannot find ghost element\n");
+            test_gv = ldb_msg_find_val(el, &gv);
+            fail_unless(test_gv == NULL,
+                        "Ghost user %s unexpectedly found\n", ghostname);
+        } else {
+            fail_unless(el == NULL, "Stray values in ghost element?\n");
+        }
+    }
+
+    /* Perform the add operation */
+    ret =  sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                                data->attrs, SYSDB_MOD_ADD);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* Before the delete, all groups with gid >= _i have the testuser%_i
+     * as a member
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname;
+        gv.length = strlen(ghostname);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname);
+    }
+    talloc_free(test_ctx);
+}
+END_TEST
+
+START_TEST (test_sysdb_memberof_mod_replace)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname_del;
+    char *ghostname_add;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+    gid_t itergid;
+
+    /* 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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    /* The test replaces the testuser%i attribute with testghost%i */
+    ghostname_del = talloc_asprintf(data, "testuser%d", _i);
+    fail_unless(ghostname_del != NULL, "Out of memory\n");
+
+    ghostname_add = talloc_asprintf(data, "testghost%d", _i);
+    fail_unless(ghostname_add != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname_add);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    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;
+
+    /* Before the replace, all groups with gid >= _i have the testuser%_i
+     * as a member
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname_del;
+        gv.length = strlen(ghostname_del);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_del);
+    }
+
+    /* Perform the replace operation */
+    ret =  sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                                data->attrs, SYSDB_MOD_REP);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* After the replace, all groups with gid >= _i have the testghost%_i
+     * as a member
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname_add;
+        gv.length = strlen(ghostname_add);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_add);
+    }
+
+    talloc_free(test_ctx);
+}
+END_TEST
+
 START_TEST (test_sysdb_memberof_close_loop)
 {
     struct sysdb_test_ctx *test_ctx;
@@ -2367,6 +2551,97 @@ START_TEST (test_sysdb_memberof_remove_child_group_and_check_ghost)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_mod_del)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+    gid_t itergid;
+
+    /* 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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    ghostname = talloc_asprintf(data, "testuser%d", _i);
+    fail_unless(ghostname != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    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;
+
+    /* Before the delete, all groups with gid >= _i have the testuser%_i
+     * as a member
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname;
+        gv.length = strlen(ghostname);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname);
+    }
+
+    /* Delete the attribute */
+    ret = sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                               data->attrs, SYSDB_MOD_DEL);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* After the delete, we shouldn't be able to find the ghost attribute */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname;
+        gv.length = strlen(ghostname);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        if (itergid > data->gid) {
+            /* The first group would have the ghost attribute gone completely */
+            fail_if(el == NULL, "Cannot find ghost element\n");
+            test_gv = ldb_msg_find_val(el, &gv);
+            fail_unless(test_gv == NULL,
+                        "Ghost user %s unexpectedly found\n", ghostname);
+        } else {
+            fail_unless(el == NULL, "Stray values in ghost element?\n");
+        }
+    }
+
+    talloc_free(test_ctx);
+}
+END_TEST
+
 START_TEST (test_sysdb_memberof_check_ghost)
 {
     struct sysdb_test_ctx *test_ctx;
@@ -2521,6 +2796,170 @@ START_TEST (test_sysdb_memberof_check_convert)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_ghost_replace)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname_del;
+    char *ghostname_add;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+
+    /* 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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    /* The test replaces the testghost%i attribute with testuser%i */
+    ghostname_del = talloc_asprintf(data, "testghost%d", _i - 1);
+    fail_unless(ghostname_del != NULL, "Out of memory\n");
+
+    ghostname_add = talloc_asprintf(data, "testuser%d", _i - 1);
+    fail_unless(ghostname_add != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname_add);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    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;
+
+    /* Before the replace, the group has the testghost%_i as a member */
+    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);
+
+    gv.data = (uint8_t *) ghostname_del;
+    gv.length = strlen(ghostname_del);
+
+    el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+    fail_if(el == NULL, "Cannot find ghost element\n");
+
+    test_gv = ldb_msg_find_val(el, &gv);
+    fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_del);
+
+    /* Perform the replace operation */
+    ret =  sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                                data->attrs, SYSDB_MOD_REP);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* After the replace, the group has the testghost%_i as a member */
+    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);
+
+    gv.data = (uint8_t *) ghostname_add;
+    gv.length = strlen(ghostname_add);
+
+    el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+    fail_if(el == NULL, "Cannot find ghost element\n");
+
+    test_gv = ldb_msg_find_val(el, &gv);
+    fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_add);
+}
+END_TEST
+
+START_TEST (test_sysdb_memberof_ghost_replace_noop)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname_del;
+    char *ghostname_add;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+
+    /* 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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    /* The test replaces the testghost%i attribute with testuser%i */
+    ghostname_del = talloc_asprintf(data, "testuser%d", _i - 1);
+    fail_unless(ghostname_del != NULL, "Out of memory\n");
+
+    ghostname_add = talloc_asprintf(data, "testuser%d", _i - 1);
+    fail_unless(ghostname_add != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname_add);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    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;
+
+    /* Before the replace, the group has the testghost%_i as a member */
+    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);
+
+    gv.data = (uint8_t *) ghostname_del;
+    gv.length = strlen(ghostname_del);
+
+    el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+    fail_if(el == NULL, "Cannot find ghost element\n");
+
+    test_gv = ldb_msg_find_val(el, &gv);
+    fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_del);
+
+    /* Perform the replace operation */
+    ret =  sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                                data->attrs, SYSDB_MOD_REP);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* After the replace, the group has the testghost%_i as a member */
+    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);
+
+    gv.data = (uint8_t *) ghostname_add;
+    gv.length = strlen(ghostname_add);
+
+    el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+    fail_if(el == NULL, "Cannot find ghost element\n");
+
+    test_gv = ldb_msg_find_val(el, &gv);
+    fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_add);
+}
+END_TEST
+
 START_TEST (test_sysdb_memberof_user_cleanup)
 {
     struct sysdb_test_ctx *test_ctx;
@@ -4456,12 +4895,53 @@ Suite *create_sysdb_suite(void)
     /* Check the members and ghosts are there as appropriate */
     tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_convert,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + NUM_GHOSTS);
+    /* Rename the other half */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_ghost_replace,
+                        MBO_GROUP_BASE + NUM_GHOSTS/2 + 1,
+                        MBO_GROUP_BASE + NUM_GHOSTS);
+    /* Attempt to replace with the same data to check if noop works correctly */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_ghost_replace_noop,
+                        MBO_GROUP_BASE + NUM_GHOSTS/2 + 1,
+                        MBO_GROUP_BASE + NUM_GHOSTS);
     /* Remove the real users */
     tcase_add_loop_test(tc_memberof, test_sysdb_memberof_user_cleanup,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + NUM_GHOSTS/2);
     tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + NUM_GHOSTS);
 
+    /* ghost users - memberof mod_del */
+    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_memberof_check_nested_ghosts,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_mod_del,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + NUM_GHOSTS);
+
+    /* ghost users - memberof mod_add */
+    /* Add groups without ghosts first */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_store_group, 0, 10);
+    /* Add ghosts to groups so that they propagate */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_mod_add,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    /* Check if the ghosts in fact propagated */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_nested_ghosts,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    /* Clean up */
+    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+
+    /* ghost users - replace */
+    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_memberof_check_nested_ghosts,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_mod_replace,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+
     suite_add_tcase(s, tc_memberof);
 
     TCase *tc_subdomain = tcase_create("SYSDB sub-domain Tests");
-- 
1.8.0.1

-------------- next part --------------
>From 8a71718b2c6486dcf638b064ed9aae63b2b34abb Mon Sep 17 00:00:00 2001
From: Jakub Hrozek <jhrozek at redhat.com>
Date: Mon, 3 Dec 2012 23:03:35 +0100
Subject: [PATCH 6/6] MEMBEROF: Keep inherited ghost users around on modify
 operation

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

It is possible to simply reset the list of ghost users to a different one
during a modify operation. It is also actually how we update entries that
are expired in the SSSD cache.

In this case, we must be careful and retain the ghost users that are not
native to the group we are processing but are rather inherited from child
groups. The intention of the replace operation after all is to set the
list of direct members of that group, not direct and indirect.
---
 src/ldb_modules/memberof.c | 423 +++++++++++++++++++++++++++++++++++++++++----
 src/tests/sysdb-tests.c    | 248 ++++++++++++++++++++++++++
 2 files changed, 637 insertions(+), 34 deletions(-)

diff --git a/src/ldb_modules/memberof.c b/src/ldb_modules/memberof.c
index cb6f8113f2b863cc6f8758f7de3baa5b38e053e8..3367bd754c2102dd44e33d2c912a41f389101017 100644
--- a/src/ldb_modules/memberof.c
+++ b/src/ldb_modules/memberof.c
@@ -144,6 +144,15 @@ struct mbof_del_ctx {
     bool is_mod;
 };
 
+struct mbof_mod_del_op {
+    struct mbof_mod_ctx *mod_ctx;
+
+    struct ldb_message *mod_msg;
+    struct ldb_message_element *el;
+
+    hash_table_t *inherited_gh;
+};
+
 struct mbof_mod_ctx {
     struct mbof_ctx *ctx;
 
@@ -156,6 +165,7 @@ struct mbof_mod_ctx {
 
     struct mbof_val_array *gh_add;
     struct mbof_val_array *gh_remove;
+    struct mbof_mod_del_op *igh;
 
     struct ldb_message *msg;
     bool terminate;
@@ -177,6 +187,16 @@ static struct mbof_ctx *mbof_init(struct ldb_module *module,
     return ctx;
 }
 
+static void *hash_alloc(const size_t size, void *pvt)
+{
+    return talloc_size(pvt, size);
+}
+
+static void hash_free(void *ptr, void *pvt)
+{
+    talloc_free(ptr);
+}
+
 static int entry_has_objectclass(struct ldb_message *entry,
                                  const char *objectclass)
 {
@@ -2774,19 +2794,37 @@ static void free_delop_contents(struct mbof_del_operation *delop)
 
 /* A modify operation just implements either an add operation, or a delete
  * operation or both (replace) in turn.
- * The only difference between a modify and a pure add or a pure delete is that
+ * One difference between a modify and a pure add or a pure delete is that
  * the object is not created a new or not completely removed, but the setup just
  * treats it in the same way children objects are treated in a pure add or delete
  * operation. A list of appropriate parents and objects to modify is built, then
  * we jump directly in the add or delete code.
  * If both add and delete are necessary, delete operations are performed first
- * and then a followup add operation is concatenated */
+ * and then a followup add operation is concatenated
+ *
+ * Another difference is the ghost users. Because of its semi-managed nature,
+ * the ghost attribute requires some special care. During a modify operation, the
+ * ghost attribute can be set to a new list. That list coming, from an
+ * application, would typically only include the direct ghost
+ * members. However, we want to keep both direct and indirect ghost members
+ * in the cache to be able to return them all in a single call. To solve
+ * that problem, we also iterate over members of the group being modified,
+ * collect all ghost entries and add them back in case the original modify
+ * operation wiped them out.
+ */
 
 static int mbof_mod_callback(struct ldb_request *req,
                              struct ldb_reply *ares);
+static int mbof_collect_child_ghosts(struct mbof_mod_ctx *mod_ctx);
+static int mbof_get_ghost_from_parent(struct mbof_mod_del_op *igh);
+static int mbof_get_ghost_from_parent_cb(struct ldb_request *req,
+                                         struct ldb_reply *ares);
 static int mbof_orig_mod(struct mbof_mod_ctx *mod_ctx);
 static int mbof_orig_mod_callback(struct ldb_request *req,
                                   struct ldb_reply *ares);
+static int mbof_inherited_mod(struct mbof_mod_ctx *mod_ctx);
+static int mbof_inherited_mod_callback(struct ldb_request *req,
+                                       struct ldb_reply *ares);
 static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done);
 static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
                                    struct ldb_message *entry,
@@ -2795,7 +2833,8 @@ static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
                                    struct mbof_dn_array **_removed);
 static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
                                  struct ldb_message *entry,
-                                 const struct ldb_message_element *membel,
+                                 const struct ldb_message_element *ghel,
+                                 const struct ldb_message_element *inherited,
                                  struct mbof_val_array **_added,
                                  struct mbof_val_array **_removed);
 static int mbof_mod_delete(struct mbof_mod_ctx *mod_ctx,
@@ -2807,8 +2846,13 @@ static int mbof_fill_dn_array(TALLOC_CTX *memctx,
                               struct mbof_dn_array **dn_array);
 static int mbof_fill_vals_array(TALLOC_CTX *memctx,
                                 struct ldb_context *ldb,
-                                const struct ldb_message_element *el,
+                                unsigned int num_values,
+                                struct ldb_val *values,
                                 struct mbof_val_array **val_array);
+static int mbof_fill_vals_array_el(TALLOC_CTX *memctx,
+                                   struct ldb_context *ldb,
+                                   const struct ldb_message_element *el,
+                                   struct mbof_val_array **val_array);
 
 static int memberof_mod(struct ldb_module *module, struct ldb_request *req)
 {
@@ -2937,11 +2981,173 @@ static int mbof_mod_callback(struct ldb_request *req,
                                    LDB_ERR_NO_SUCH_OBJECT);
         }
 
+        ret = mbof_collect_child_ghosts(mod_ctx);
+        if (ret != LDB_SUCCESS) {
+            talloc_zfree(ares);
+            return ldb_module_done(ctx->req, NULL, NULL, ret);
+        }
+    }
+
+    talloc_zfree(ares);
+    return LDB_SUCCESS;
+}
+
+static int mbof_collect_child_ghosts(struct mbof_mod_ctx *mod_ctx)
+{
+    int ret;
+    const struct ldb_message_element *member;
+
+    member = ldb_msg_find_element(mod_ctx->entry, DB_MEMBER);
+
+    if (member == NULL || member->num_values == 0 ||
+        mod_ctx->ghel == NULL || mod_ctx->ghel->flags != LDB_FLAG_MOD_REPLACE) {
         ret = mbof_orig_mod(mod_ctx);
         if (ret != LDB_SUCCESS) {
+            return ret;
+        }
+
+        return LDB_SUCCESS;
+    }
+
+    mod_ctx->igh = talloc_zero(mod_ctx, struct mbof_mod_del_op);
+    if (mod_ctx == NULL) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+    mod_ctx->igh->mod_ctx = mod_ctx;
+
+    ret = hash_create_ex(1024, &mod_ctx->igh->inherited_gh, 0, 0, 0, 0,
+                         hash_alloc, hash_free, mod_ctx, NULL, NULL);
+    if (ret != HASH_SUCCESS) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+
+    return mbof_get_ghost_from_parent(mod_ctx->igh);
+}
+
+static int mbof_get_ghost_from_parent(struct mbof_mod_del_op *igh)
+{
+    struct ldb_request *search;
+    struct ldb_context *ldb;
+    struct mbof_ctx *ctx;
+    int ret;
+    static const char *attrs[] = { DB_GHOST, NULL };
+    char *expression;
+    char *clean_dn;
+    const char *dn;
+
+    ctx = igh->mod_ctx->ctx;
+    ldb = ldb_module_get_ctx(ctx->module);
+
+    dn = ldb_dn_get_linearized(igh->mod_ctx->entry->dn);
+    if (!dn) {
+        talloc_free(ctx);
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+    ret = sss_filter_sanitize(igh, dn, &clean_dn);
+    if (ret != 0) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+    expression = talloc_asprintf(igh,
+                                 "(&(%s=%s)(%s=%s))",
+                                 DB_OC, DB_GROUP_CLASS,
+                                 DB_MEMBEROF, clean_dn);
+    if (!expression) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+    talloc_zfree(clean_dn);
+
+    ret = ldb_build_search_req(&search, ldb, igh,
+                               NULL,
+                               LDB_SCOPE_SUBTREE,
+                               expression, attrs, NULL,
+                               igh, mbof_get_ghost_from_parent_cb,
+                               ctx->req);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    return ldb_request(ldb, search);
+}
+
+static int mbof_get_ghost_from_parent_cb(struct ldb_request *req,
+                                         struct ldb_reply *ares)
+{
+    struct mbof_mod_del_op *igh;
+    struct mbof_ctx *ctx;
+    struct ldb_message_element *el;
+    struct ldb_val *dup;
+    int ret;
+    hash_value_t value;
+    hash_key_t key;
+    int i;
+
+    igh = talloc_get_type(req->context, struct mbof_mod_del_op);
+    ctx = igh->mod_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:
+        el = ldb_msg_find_element(ares->message, DB_GHOST);
+        if (!el) {
+            break;
+        }
+
+        for (i=0; i < el->num_values; i++) {
+            key.type = HASH_KEY_STRING;
+            key.str = (char *) el->values[i].data;
+
+            if (hash_has_key(igh->inherited_gh, &key)) {
+                /* We already have this user. Don't re-add him */
+                continue;
+            }
+
+            dup = talloc_zero(igh->inherited_gh, struct ldb_val);
+            if (dup == NULL) {
+                return LDB_ERR_OPERATIONS_ERROR;
+            }
+
+            *dup = ldb_val_dup(igh->inherited_gh, &el->values[i]);
+            if (dup->data == NULL) {
+                return LDB_ERR_OPERATIONS_ERROR;
+            }
+
+            value.type = HASH_VALUE_PTR;
+            value.ptr = dup;
+
+            ret = hash_enter(igh->inherited_gh, &key, &value);
+            if (ret != HASH_SUCCESS) {
+                return LDB_ERR_OPERATIONS_ERROR;
+            }
+        }
+        break;
+
+    case LDB_REPLY_REFERRAL:
+        /* ignore */
+        break;
+
+    case LDB_REPLY_DONE:
+        /* All the children are gathered, let's do the real
+         * modify operation
+         */
+        ret = mbof_orig_mod(igh->mod_ctx);
+        if (ret != LDB_SUCCESS) {
             talloc_zfree(ares);
             return ldb_module_done(ctx->req, NULL, NULL, ret);
         }
+        break;
     }
 
     talloc_zfree(ares);
@@ -3006,7 +3212,13 @@ static int mbof_orig_mod_callback(struct ldb_request *req,
 
     if (!mod_ctx->terminate) {
         /* next step */
-        ret = mbof_mod_process(mod_ctx, &mod_ctx->terminate);
+        if (mod_ctx->igh && mod_ctx->igh->inherited_gh &&
+            hash_count(mod_ctx->igh->inherited_gh) > 0) {
+            ret = mbof_inherited_mod(mod_ctx);
+        } else {
+            ret = mbof_mod_process(mod_ctx, &mod_ctx->terminate);
+        }
+
         if (ret != LDB_SUCCESS) {
             talloc_zfree(ares);
             return ldb_module_done(ctx->req, NULL, NULL, ret);
@@ -3025,6 +3237,128 @@ static int mbof_orig_mod_callback(struct ldb_request *req,
     return LDB_SUCCESS;
 }
 
+static int mbof_inherited_mod(struct mbof_mod_ctx *mod_ctx)
+{
+    struct ldb_request *mod_req;
+    struct ldb_context *ldb;
+    struct mbof_ctx *ctx;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_message *msg;
+    struct ldb_val *val;
+    struct ldb_val *dup;
+    hash_value_t *values;
+    unsigned long num_values;
+    int i, j;
+
+    ctx = mod_ctx->ctx;
+    ldb = ldb_module_get_ctx(ctx->module);
+
+    /* add back the inherited children to entry */
+    msg = ldb_msg_new(mod_ctx);
+    if (!msg) return LDB_ERR_OPERATIONS_ERROR;
+
+    msg->dn = mod_ctx->entry->dn;
+
+    /* We only inherit during replaces, so it's safe to only look
+     * at the replaced set
+     */
+    ret = ldb_msg_add_empty(msg, DB_GHOST, LDB_FLAG_MOD_ADD, &el);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    ret = hash_values(mod_ctx->igh->inherited_gh, &num_values, &values);
+    if (ret != HASH_SUCCESS) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+    el->values = talloc_array(msg, struct ldb_val, num_values);
+    if (!el->values) {
+        return LDB_ERR_OPERATIONS_ERROR;
+    }
+
+    for (i = 0, j = 0; i < num_values; i++) {
+        val = talloc_get_type(values[i].ptr, struct ldb_val);
+
+        dup = ldb_msg_find_val(mod_ctx->ghel, val);
+        if (dup) {
+            continue;
+        }
+
+        el->values[j].length = strlen((const char *) val->data);
+        el->values[j].data = (uint8_t *) talloc_strdup(el->values,
+                                                    (const char *) val->data);
+        if (!el->values[j].data) {
+            return LDB_ERR_OPERATIONS_ERROR;
+        }
+        j++;
+    }
+    el->num_values = j;
+
+    mod_ctx->igh->mod_msg = msg;
+    mod_ctx->igh->el = el;
+
+    ret = ldb_build_mod_req(&mod_req, ldb, ctx->req,
+                            msg, ctx->req->controls,
+                            mod_ctx, mbof_inherited_mod_callback,
+                            ctx->req);
+    if (ret != LDB_SUCCESS) {
+        return ret;
+    }
+
+    return ldb_next_request(ctx->module, mod_req);
+}
+
+static int mbof_inherited_mod_callback(struct ldb_request *req,
+                                       struct ldb_reply *ares)
+{
+    struct ldb_context *ldb;
+    struct mbof_mod_ctx *mod_ctx;
+    struct mbof_ctx *ctx;
+    int ret;
+
+    mod_ctx = talloc_get_type(req->context, struct mbof_mod_ctx);
+    ctx = mod_ctx->ctx;
+    ldb = ldb_module_get_ctx(ctx->module);
+
+    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);
+    }
+
+    if (ares->type != LDB_REPLY_DONE) {
+        talloc_zfree(ares);
+        ldb_debug(ldb, LDB_DEBUG_TRACE, "Invalid reply type!");
+        ldb_set_errstring(ldb, "Invalid reply type!");
+        return ldb_module_done(ctx->req, NULL, NULL,
+                               LDB_ERR_OPERATIONS_ERROR);
+    }
+
+    ret = mbof_mod_process(mod_ctx, &mod_ctx->terminate);
+    if (ret != LDB_SUCCESS) {
+        talloc_zfree(ares);
+        return ldb_module_done(ctx->req, NULL, NULL, ret);
+    }
+
+    if (mod_ctx->terminate) {
+        talloc_zfree(ares);
+        return ldb_module_done(ctx->req,
+                               ctx->ret_ctrls,
+                               ctx->ret_resp,
+                               LDB_SUCCESS);
+    }
+
+    talloc_zfree(ares);
+    return LDB_SUCCESS;
+}
+
 static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done)
 {
     struct ldb_context *ldb;
@@ -3041,6 +3375,7 @@ static int mbof_mod_process(struct mbof_mod_ctx *mod_ctx, bool *done)
     }
 
     ret = mbof_mod_process_ghel(mod_ctx, ldb, mod_ctx->entry, mod_ctx->ghel,
+                                mod_ctx->igh ? mod_ctx->igh->el : NULL,
                                 &mod_ctx->gh_add, &mod_ctx->gh_remove);
     if (ret != LDB_SUCCESS) {
         return ret;
@@ -3168,6 +3503,7 @@ static int mbof_mod_process_membel(TALLOC_CTX *mem_ctx,
 static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
                                  struct ldb_message *entry,
                                  const struct ldb_message_element *ghel,
+                                 const struct ldb_message_element *inherited,
                                  struct mbof_val_array **_added,
                                  struct mbof_val_array **_removed)
 {
@@ -3189,7 +3525,7 @@ static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
 
     switch (ghel->flags) {
     case LDB_FLAG_MOD_ADD:
-        ret = mbof_fill_vals_array(mem_ctx, ldb, ghel, &added);
+        ret = mbof_fill_vals_array_el(mem_ctx, ldb, ghel, &added);
         if (ret != LDB_SUCCESS) {
             return ret;
         }
@@ -3207,7 +3543,7 @@ static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
             break;
         }
 
-        ret = mbof_fill_vals_array(mem_ctx, ldb, ghel, &removed);
+        ret = mbof_fill_vals_array_el(mem_ctx, ldb, ghel, &removed);
         if (ret != LDB_SUCCESS) {
             return ret;
         }
@@ -3216,7 +3552,7 @@ static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
     case LDB_FLAG_MOD_REPLACE:
         el = ldb_msg_find_element(entry, DB_GHOST);
         if (el) {
-            ret = mbof_fill_vals_array(mem_ctx, ldb, el, &removed);
+            ret = mbof_fill_vals_array_el(mem_ctx, ldb, el, &removed);
             if (ret != LDB_SUCCESS) {
                 return ret;
             }
@@ -3224,13 +3560,22 @@ static int mbof_mod_process_ghel(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
 
         el = ghel;
         if (el) {
-            ret = mbof_fill_vals_array(mem_ctx, ldb, el, &added);
+            ret = mbof_fill_vals_array_el(mem_ctx, ldb, el, &added);
             if (ret != LDB_SUCCESS) {
                 talloc_free(removed);
                 return ret;
             }
         }
 
+        if (inherited) {
+            ret = mbof_fill_vals_array_el(mem_ctx, ldb, inherited, &added);
+            if (ret != LDB_SUCCESS) {
+                talloc_free(added);
+                talloc_free(removed);
+                return ret;
+            }
+        }
+
         /* remove from arrays values that ended up unchanged */
         if (removed && removed->num && added && added->num) {
             for (i = 0; i < added->num; i++) {
@@ -3429,40 +3774,60 @@ static int mbof_fill_dn_array(TALLOC_CTX *memctx,
 
 static int mbof_fill_vals_array(TALLOC_CTX *memctx,
                                 struct ldb_context *ldb,
-                                const struct ldb_message_element *el,
+                                unsigned int num_values,
+                                struct ldb_val *values,
                                 struct mbof_val_array **val_array)
 {
-    struct mbof_val_array *var;
-    int i;
+    struct mbof_val_array *var = *val_array;
+    int i, index;
 
-    var = talloc_zero(memctx, struct mbof_val_array);
-    if (!var) {
-        return LDB_ERR_OPERATIONS_ERROR;
+    if (var == NULL) {
+        var = talloc_zero(memctx, struct mbof_val_array);
+        if (!var) {
+            return LDB_ERR_OPERATIONS_ERROR;
+        }
+        *val_array = var;
     }
-    *val_array = var;
 
-    if (!el || el->num_values == 0) {
+    if (values == NULL || num_values == 0) {
         return LDB_SUCCESS;
     }
 
-    var->vals = talloc_array(var, struct ldb_val, el->num_values);
+    /* We do not care about duplicate values now.
+     * They will be filtered later */
+    index = var->num;
+    var->num += num_values;
+    var->vals = talloc_realloc(memctx, var->vals, struct ldb_val, var->num);
     if (!var->vals) {
         return LDB_ERR_OPERATIONS_ERROR;
     }
-    var->num = el->num_values;
 
-    for (i = 0; i < var->num; i++) {
-        var->vals[i].length = strlen((const char *) el->values[i].data);
-        var->vals[i].data = (uint8_t *) talloc_strdup(var,
-                                          (const char *) el->values[i].data);
-        if (var->vals[i].data == NULL) {
+    /* FIXME - use ldb_val_dup() */
+    for (i = 0; i < num_values; i++) {
+        var->vals[index].length = strlen((const char *) values[i].data);
+        var->vals[index].data = (uint8_t *) talloc_strdup(var,
+                                          (const char *) values[i].data);
+        if (var->vals[index].data == NULL) {
             return LDB_ERR_OPERATIONS_ERROR;
         }
+        index++;
     }
 
     return LDB_SUCCESS;
 }
 
+static int mbof_fill_vals_array_el(TALLOC_CTX *memctx,
+                                   struct ldb_context *ldb,
+                                   const struct ldb_message_element *el,
+                                   struct mbof_val_array **val_array)
+{
+    if (el == NULL) {
+        return LDB_SUCCESS;
+    }
+
+    return mbof_fill_vals_array(memctx, ldb, el->num_values, el->values,
+                                val_array);
+}
 
 /*************************
  * Cleanup task routines *
@@ -3501,16 +3866,6 @@ struct mbof_rcmp_context {
     hash_table_t *group_table;
 };
 
-static void *hash_alloc(const size_t size, void *pvt)
-{
-    return talloc_size(pvt, size);
-}
-
-static void hash_free(void *ptr, void *pvt)
-{
-    talloc_free(ptr);
-}
-
 static int mbof_steal_msg_el(TALLOC_CTX *memctx,
                              const char *name,
                              struct ldb_message *msg,
diff --git a/src/tests/sysdb-tests.c b/src/tests/sysdb-tests.c
index d513c30f9b370d030bea72901eacdec8eaeb0e87..1db907b5a74bd7cd9a258fc8634a5c271a0386ca 100644
--- a/src/tests/sysdb-tests.c
+++ b/src/tests/sysdb-tests.c
@@ -1996,6 +1996,46 @@ START_TEST (test_sysdb_memberof_store_group_with_ghosts)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_store_group_with_double_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->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    if (_i == 0) {
+        data->attrlist = NULL;
+    } else {
+        data->attrlist = talloc_array(data, const char *, 2);
+        fail_unless(data->attrlist != NULL, "talloc_array failed.");
+        data->attrlist[0] = talloc_asprintf(data, "testgroup%d", data->gid - 1);
+        data->attrlist[1] = NULL;
+    }
+
+    data->memberlist = talloc_array(data, char *, 3);
+    fail_unless(data->memberlist != NULL, "talloc_array failed.");
+    data->memberlist[0] = talloc_asprintf(data, "testusera%d", data->gid);
+    data->memberlist[1] = talloc_asprintf(data, "testuserb%d", data->gid);
+    data->memberlist[2] = NULL;
+
+    ret = test_memberof_store_group_with_ghosts(data);
+
+    fail_if(ret != EOK, "Could not store POSIX group #%d", data->gid);
+    talloc_free(test_ctx);
+}
+END_TEST
 
 START_TEST (test_sysdb_memberof_mod_add)
 {
@@ -2180,6 +2220,164 @@ START_TEST (test_sysdb_memberof_mod_replace)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_mod_replace_keep)
+{
+    struct sysdb_test_ctx *test_ctx;
+    struct test_data *data;
+    char *ghostname_rep;
+    char *ghostname_del;
+    char *ghostname_check;
+    int ret;
+    struct ldb_message_element *el;
+    struct ldb_val gv, *test_gv;
+    gid_t itergid;
+    uid_t iteruid;
+
+    /* 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 = MBO_GROUP_BASE + 10 - _i;
+    data->groupname = talloc_asprintf(data, "testgroup%d", data->gid);
+
+    data->attrs = sysdb_new_attrs(data);
+    if (ret != EOK) {
+        fail("Could not create the changeset");
+        return;
+    }
+
+    /* The test replaces the attributes (testusera$gid, testuserb$gid) with
+     * just testusera$gid. The result should be not only testusera, but also
+     * all ghost users inherited from child groups
+     */
+    ghostname_rep = talloc_asprintf(data, "testusera%d", data->gid);
+    fail_unless(ghostname_rep != NULL, "Out of memory\n");
+    ret = sysdb_attrs_steal_string(data->attrs, SYSDB_GHOST, ghostname_rep);
+    fail_unless(ret == EOK, "Cannot add attr\n");
+
+    ghostname_del = talloc_asprintf(data, "testuserb%d", data->gid);
+    fail_unless(ghostname_del != NULL, "Out of memory\n");
+
+    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;
+
+    /* Before the replace, all groups with gid >= _i have both testuser a
+     * and testuserb as a member
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname_rep;
+        gv.length = strlen(ghostname_rep);
+
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_rep);
+
+        gv.data = (uint8_t *) ghostname_del;
+        gv.length = strlen(ghostname_rep);
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_del);
+
+        /* inherited users must be there */
+        for (iteruid = MBO_GROUP_BASE ; iteruid < itergid ; iteruid++) {
+            ghostname_check = talloc_asprintf(data, "testusera%d", iteruid);
+            gv.data = (uint8_t *) ghostname_check;
+            gv.length = strlen(ghostname_check);
+
+            test_gv = ldb_msg_find_val(el, &gv);
+            fail_if(test_gv == NULL, "Cannot find inherited ghost user %s\n",
+                    ghostname_check);
+
+            if (iteruid < data->gid) {
+                /* Also check the B user if it hasn't been deleted yet */
+                ghostname_check = talloc_asprintf(data, "testuserb%d", iteruid);
+                gv.data = (uint8_t *) ghostname_check;
+                gv.length = strlen(ghostname_check);
+
+                test_gv = ldb_msg_find_val(el, &gv);
+                fail_if(test_gv == NULL, "Cannot find inherited ghost user %s\n",
+                        ghostname_check);
+            }
+            talloc_zfree(ghostname_check);
+        }
+    }
+
+    /* Perform the replace operation */
+    ret =  sysdb_set_group_attr(test_ctx->sysdb, data->groupname,
+                                data->attrs, SYSDB_MOD_REP);
+    fail_unless(ret == EOK, "Cannot set group attrs\n");
+
+    /* After the replace, testusera should still be there, but we also need
+     * to keep ghost users inherited from other groups
+     */
+    for (itergid = data->gid ; itergid < MBO_GROUP_BASE + NUM_GHOSTS; itergid++) {
+        ret = sysdb_search_group_by_gid(data, test_ctx->sysdb,
+                                        itergid,
+                                        data->attrlist, &data->msg);
+        fail_if(ret != EOK, "Cannot retrieve group %llu\n",
+                (unsigned long long) data->gid);
+
+        gv.data = (uint8_t *) ghostname_rep;
+        gv.length = strlen(ghostname_rep);
+
+        /* testusera must still be there */
+        el = ldb_msg_find_element(data->msg, SYSDB_GHOST);
+        fail_if(el == NULL, "Cannot find ghost element\n");
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_if(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_rep);
+
+        /* testuserb must be gone */
+        gv.data = (uint8_t *) ghostname_del;
+        gv.length = strlen(ghostname_rep);
+
+        test_gv = ldb_msg_find_val(el, &gv);
+        fail_unless(test_gv == NULL, "Cannot find ghost user %s\n", ghostname_del);
+
+        /* inherited users must still be there */
+        for (iteruid = MBO_GROUP_BASE ; iteruid < itergid ; iteruid++) {
+            ghostname_check = talloc_asprintf(data, "testusera%d", iteruid);
+            gv.data = (uint8_t *) ghostname_check;
+            gv.length = strlen(ghostname_check);
+
+            test_gv = ldb_msg_find_val(el, &gv);
+            fail_if(test_gv == NULL, "Cannot find inherited ghost user %s\n",
+                    ghostname_check);
+
+            if (iteruid < data->gid) {
+                /* Also check the B user if it hasn't been deleted yet */
+                ghostname_check = talloc_asprintf(data, "testuserb%d", iteruid);
+                gv.data = (uint8_t *) ghostname_check;
+                gv.length = strlen(ghostname_check);
+
+                test_gv = ldb_msg_find_val(el, &gv);
+                fail_if(test_gv == NULL, "Cannot find inherited ghost user %s\n",
+                        ghostname_check);
+            }
+            talloc_zfree(ghostname_check);
+        }
+    }
+
+    talloc_free(test_ctx);
+}
+END_TEST
+
 START_TEST (test_sysdb_memberof_close_loop)
 {
     struct sysdb_test_ctx *test_ctx;
@@ -2489,6 +2687,45 @@ START_TEST (test_sysdb_memberof_check_nested_ghosts)
 }
 END_TEST
 
+START_TEST (test_sysdb_memberof_check_nested_double_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)*2,
+                "Wrong number of attribute values, expected [%d] got [%d]",
+                (_i - MBO_GROUP_BASE + 1)*2,
+                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;
@@ -4942,6 +5179,17 @@ Suite *create_sysdb_suite(void)
     tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
                         MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
 
+    /* ghost users - replace but retain inherited */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_store_group_with_double_ghosts,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_check_nested_double_ghosts,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+    /* This loop counts backwards so the indexing is a little odd */
+    tcase_add_loop_test(tc_memberof, test_sysdb_memberof_mod_replace_keep,
+                        1 , 11);
+    tcase_add_loop_test(tc_memberof, test_sysdb_remove_local_group_by_gid,
+                        MBO_GROUP_BASE , MBO_GROUP_BASE + 10);
+
     suite_add_tcase(s, tc_memberof);
 
     TCase *tc_subdomain = tcase_create("SYSDB sub-domain Tests");
-- 
1.8.0.1



More information about the sssd-devel mailing list