[SSSD] [PATCHES] Improve group handling for subdomain users

Sumit Bose sbose at redhat.com
Fri Nov 9 11:09:54 UTC 2012


On Thu, Nov 08, 2012 at 09:34:19AM -0500, Simo Sorce wrote:
> On Wed, 2012-11-07 at 13:13 +0100, Sumit Bose wrote:
> > 
> 
> Can you change the function name to diff_gid_lists() ?
> 
> 
> Also may be nice to change the following arg names:
> current_gid_count -> cur_gid_num
> current_gids -> cur_gid_list
> gid_count -> new_gid_num
> gids -> new_gid_list
> _add_gid_count -> _add_gid_num
> _add_gids -> _add_gid_list
> _del_gid_count -> _del_gid_num
> _del_gids -> _del_gid_list

done

> 
> > 
> 
> 
> > +    if (current_gid_count == 0 && gid_count == 0) {
> > +        ret = EOK;
> > +        goto done;
> > +    }
> 
> Also noticed that for this condition you probably want to just assign
> and return EOK directly, otherwise ...
> 
> > +done:
> > +
> > +    if (ret == EOK) {
> > +        *_add_gid_count = add_gid_count;
> > +        *_add_gids = talloc_steal(mem_ctx, add_gids);
> > +        *_del_gid_count = del_gid_count;
> > +        *_del_gids = talloc_steal(mem_ctx, del_gids);
> > +    }
> > +
> > +    talloc_free(tmp_ctx);
> > +
> > +    return ret;
> > +} 
> 
> ... here you try to steal NULL pointers and free a NULL context.
> 
> talloc_steal and talloc_free do not crash if you pass NULL, but it is
> strange to see all this done when clearly you never set anything in
> there.
> 
> Not fatal, if you feel strongly that jumping to done is better I am ok
> with it too.

Having NULLs for the group lists here would be a typical case if the
group memberships of does not change too often. I would prefer to have a
single exit point where the returned data is set.

> 
> > +static errno_t
> > +pac_save_memberships_delete(struct pac_save_memberships_state *state)
> > +{
> 
> [...]
> 
> > +    for (c = 0; c < pr_ctx->del_gid_count; c++) {
> > +        ret = sysdb_search_group_by_gid(state,
> > state->group_dom->sysdb,
> > +                                        pr_ctx->del_gids[c], NULL,
> > &group);
> 
> Wouldn't it be more efficient to create a search filter that lists all
> GIDs we are interested in and do a single search rather than looping
> over and over ?

I skipped the search her completely and kept the needed attributes after
the sysdb_initgroups() call.

> 
> 
> 
> Here it seems like you are overloading save_pac_user() with also
> returning the current group memberships, but I do not see any real
> advantage in doing it as part of pac_sve_user, there is no optimization
> involved.
> 
> I think it would be more readable if you created a pac_user_get_gids()
> that does the initgroups call and just called it before/after
> save_pac_user()
> 
> Also this patch seem to make save_pac_user() skip storing the user if
> sysdb_initgroups() is successful. This means that if the PAC has new
> information (like different fullname) that information is simply
> discarded and not updated.
> 
> I thik keeping these 2 separate makes it more readable and avoids this
> bug.

done

> 
> 
> Can you please ad a comment on why you are also removing code that sets
> group_attrs ?
> 
> I can't see why at a glance.

The old version added SYSDB_MEMBER manually with the help of
group_attrs, the new version just calls sysdb_mod_group_member()
instead.


> 
> Simo.
> 

New series attached.

bye,
Sumit
-------------- next part --------------
From aa97c61c5a5314cdc5fddc224e89119288250059 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 7 Nov 2012 11:29:51 +0100
Subject: [PATCH 1/5] Do not remove a group if it has members from subdomains

Currently it is only checked if an expired group still has members of
the local domain. If not, the group is delete from the cache. With this
patch the whole cache, i.e. including subdomains, is searched for
members.
---
 src/providers/ldap/ldap_id_cleanup.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/src/providers/ldap/ldap_id_cleanup.c b/src/providers/ldap/ldap_id_cleanup.c
index e65356d..9c2faab 100644
--- a/src/providers/ldap/ldap_id_cleanup.c
+++ b/src/providers/ldap/ldap_id_cleanup.c
@@ -395,6 +395,7 @@ static int cleanup_groups(TALLOC_CTX *memctx,
     int ret;
     int i;
     const char *posix;
+    struct ldb_dn *base_dn;
 
     tmpctx = talloc_new(memctx);
     if (!tmpctx) {
@@ -436,10 +437,12 @@ static int cleanup_groups(TALLOC_CTX *memctx,
         posix = ldb_msg_find_attr_as_string(msgs[i], SYSDB_POSIX, NULL);
         if (!posix || strcmp(posix, "TRUE") == 0) {
             /* Search for users that are members of this group, or
-             * that have this group as their primary GID
+             * that have this group as their primary GID.
+             * Include subdomain users as well.
              */
             gid = (gid_t) ldb_msg_find_attr_as_uint(msgs[i], SYSDB_GIDNUM, 0);
-            subfilter = talloc_asprintf(tmpctx, "(|(%s=%s)(%s=%lu))",
+            subfilter = talloc_asprintf(tmpctx, "(&(%s=%s)(|(%s=%s)(%s=%lu)))",
+                                        SYSDB_OBJECTCLASS, SYSDB_USER_CLASS,
                                         SYSDB_MEMBEROF, dn,
                                         SYSDB_GIDNUM, (long unsigned) gid);
         } else {
@@ -451,8 +454,16 @@ static int cleanup_groups(TALLOC_CTX *memctx,
             goto done;
         }
 
-        ret = sysdb_search_users(tmpctx, sysdb,
-                                 subfilter, NULL, &u_count, &u_msgs);
+        base_dn = sysdb_base_dn(sysdb, tmpctx);
+        if (base_dn == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, ("Failed to build base dn\n"));
+            ret = ENOMEM;
+            goto done;
+        }
+
+        ret = sysdb_search_entry(tmpctx, sysdb, base_dn,
+                                 LDB_SCOPE_SUBTREE, subfilter, NULL,
+                                 &u_count, &u_msgs);
         if (ret == ENOENT) {
             const char *name;
 
-- 
1.7.7.6

-------------- next part --------------
From c631b0d58c7c10819c26f9586c64b0a71462a8a8 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 7 Nov 2012 11:34:09 +0100
Subject: [PATCH 2/5] Add diff_gid_lists() with test

This patch adds a new call which compares a list of current GIDs with a
list of new GIDs and return a list of GIDs which are currently missing
and must be added and another list of GIDs which are not used anymore
and must be deleted. The method is the same as used by
diff_string_lists().
---
 src/responder/pac/pacsrv.h       |   16 ++++
 src/responder/pac/pacsrv_utils.c |  156 ++++++++++++++++++++++++++++++++++++++
 src/tests/pac_responder-tests.c  |  109 ++++++++++++++++++++++++++-
 3 files changed, 279 insertions(+), 2 deletions(-)

diff --git a/src/responder/pac/pacsrv.h b/src/responder/pac/pacsrv.h
index e088e21..c0a13a3 100644
--- a/src/responder/pac/pacsrv.h
+++ b/src/responder/pac/pacsrv.h
@@ -67,6 +67,12 @@ struct local_mapping_ranges {
     struct range secondary_rids;
 };
 
+struct grp_info {
+    gid_t gid;
+    char *orig_dn;
+    struct ldb_dn *dn;
+};
+
 int pac_cmd_execute(struct cli_ctx *cctx);
 
 struct sss_cmd_table *get_pac_cmds(void);
@@ -106,4 +112,14 @@ errno_t get_pwd_from_pac(TALLOC_CTX *mem_ctx,
                          struct PAC_LOGON_INFO *logon_info,
                          struct passwd **_pwd,
                          struct sysdb_attrs **_attrs);
+
+errno_t diff_gid_lists(TALLOC_CTX *mem_ctx,
+                       size_t cur_grp_num,
+                       struct grp_info *cur_gid_list,
+                       size_t new_gid_num,
+                       gid_t *new_gid_list,
+                       size_t *_add_gid_num,
+                       gid_t **_add_gid_list,
+                       size_t *_del_gid_num,
+                       struct grp_info ***_del_gid_list);
 #endif /* __PACSRV_H__ */
diff --git a/src/responder/pac/pacsrv_utils.c b/src/responder/pac/pacsrv_utils.c
index 101960f..c9551c9 100644
--- a/src/responder/pac/pacsrv_utils.c
+++ b/src/responder/pac/pacsrv_utils.c
@@ -616,3 +616,159 @@ done:
 
     return ret;
 }
+
+errno_t diff_gid_lists(TALLOC_CTX *mem_ctx,
+                       size_t cur_grp_num,
+                       struct grp_info *cur_grp_list,
+                       size_t new_gid_num,
+                       gid_t *new_gid_list,
+                       size_t *_add_gid_num,
+                       gid_t **_add_gid_list,
+                       size_t *_del_grp_num,
+                       struct grp_info ***_del_grp_list)
+{
+    int ret;
+    size_t c;
+    hash_table_t *table;
+    hash_key_t key;
+    hash_value_t value;
+    size_t add_gid_num = 0;
+    gid_t *add_gid_list = NULL;
+    size_t del_grp_num = 0;
+    struct grp_info **del_grp_list = NULL;
+    TALLOC_CTX *tmp_ctx = NULL;
+    unsigned long value_count;
+    hash_value_t *values;
+
+    if ((cur_grp_num != 0 && cur_grp_list == NULL) ||
+        (new_gid_num != 0 && new_gid_list == NULL)) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Missing group array.\n"));
+        return EINVAL;
+    }
+
+    if (cur_grp_num == 0 && new_gid_num == 0) {
+        ret = EOK;
+        goto done;
+    }
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
+
+    if (cur_grp_num == 0 && new_gid_num != 0) {
+        add_gid_num = new_gid_num;
+        add_gid_list = talloc_array(tmp_ctx, gid_t, add_gid_num);
+        if (add_gid_list == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
+            ret = ENOMEM;
+            goto done;
+        }
+
+        for (c = 0; c < add_gid_num; c++) {
+            add_gid_list[c] = new_gid_list[c];
+        }
+
+        ret = EOK;
+        goto done;
+    }
+
+    if (cur_grp_num != 0 && new_gid_num == 0) {
+        del_grp_num = cur_grp_num;
+        del_grp_list = talloc_array(tmp_ctx, struct grp_info *, del_grp_num);
+        if (del_grp_list == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
+            ret = ENOMEM;
+            goto done;
+        }
+
+        for (c = 0; c < del_grp_num; c++) {
+            del_grp_list[c] = &cur_grp_list[c];
+        }
+
+        ret = EOK;
+        goto done;
+    }
+
+    /* Add all current GIDs to a hash and then compare with the new ones in a
+     * single loop */
+    ret = sss_hash_create(tmp_ctx, cur_grp_num, &table);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sss_hash_create failed.\n"));
+        goto done;
+    }
+
+    key.type = HASH_KEY_ULONG;
+    value.type = HASH_VALUE_PTR;
+    for (c = 0; c < cur_grp_num; c++) {
+        key.ul = (unsigned long) cur_grp_list[c].gid;
+        value.ptr = &cur_grp_list[c];
+
+        ret = hash_enter(table, &key, &value);
+        if (ret != HASH_SUCCESS) {
+            DEBUG(SSSDBG_OP_FAILURE, ("hash_enter failed.\n"));
+            ret = EIO;
+            goto done;
+        }
+    }
+
+    for (c = 0; c < new_gid_num; c++) {
+        key.ul = (unsigned long) new_gid_list[c];
+
+        ret = hash_delete(table, &key);
+        if (ret == HASH_ERROR_KEY_NOT_FOUND) {
+            /* gid not found, must be added */
+            add_gid_num++;
+            add_gid_list = talloc_realloc(tmp_ctx, add_gid_list, gid_t, add_gid_num);
+            if (add_gid_list == NULL) {
+                DEBUG(SSSDBG_OP_FAILURE, ("talloc_realloc failed.\n"));
+                ret = ENOMEM;
+                goto done;
+            }
+
+            add_gid_list[add_gid_num - 1] = new_gid_list[c];
+        } else if (ret != HASH_SUCCESS) {
+            DEBUG(SSSDBG_OP_FAILURE, ("hash_delete failed.\n"));
+            ret = EIO;
+            goto done;
+        }
+    }
+
+    /* the remaining entries in the hash are not in the new list anymore and
+     * must be deleted */
+    ret = hash_values(table, &value_count, &values);
+    if (ret != HASH_SUCCESS) {
+        DEBUG(SSSDBG_OP_FAILURE, ("hash_keys failed.\n"));
+        ret = EIO;
+        goto done;
+    }
+
+    del_grp_num = value_count;
+    del_grp_list = talloc_array(tmp_ctx, struct grp_info *, del_grp_num);
+    if (del_grp_list == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
+
+    for (c = 0; c < del_grp_num; c++) {
+        del_grp_list[c] = (struct grp_info *) values[c].ptr;
+    }
+
+    ret = EOK;
+
+done:
+
+    if (ret == EOK) {
+        *_add_gid_num = add_gid_num;
+        *_add_gid_list = talloc_steal(mem_ctx, add_gid_list);
+        *_del_grp_num = del_grp_num;
+        *_del_grp_list = talloc_steal(mem_ctx, del_grp_list);
+    }
+
+    talloc_free(tmp_ctx);
+
+    return ret;
+}
diff --git a/src/tests/pac_responder-tests.c b/src/tests/pac_responder-tests.c
index 720793c..02cc242 100644
--- a/src/tests/pac_responder-tests.c
+++ b/src/tests/pac_responder-tests.c
@@ -70,18 +70,123 @@ START_TEST(pac_test_seondary_local_sid_to_id)
 }
 END_TEST
 
+START_TEST(pac_test_get_gids_to_add_and_remove)
+{
+    TALLOC_CTX *mem_ctx;
+    int ret;
+    size_t c;
+    size_t add_gid_count = 0;
+    gid_t *add_gids = NULL;
+    size_t del_gid_count = 0;
+    struct grp_info **del_gids = NULL;
+
+    gid_t gid_list_2[] = {2};
+    gid_t gid_list_3[] = {3};
+    gid_t gid_list_23[] = {2, 3};
+
+    struct grp_info grp_info_1 = {1, NULL, NULL};
+    struct grp_info grp_info_2 = {2, NULL, NULL};
+    struct grp_info  grp_list_1[] = {grp_info_1};
+    struct grp_info  grp_list_12[] = {grp_info_1, grp_info_2};
+
+    struct a_and_r_data {
+        size_t cur_gid_count;
+        struct grp_info *cur_gids;
+        size_t gid_count;
+        gid_t *gids;
+        int exp_ret;
+        size_t exp_add_gid_count;
+        gid_t *exp_add_gids;
+        size_t exp_del_gid_count;
+        struct grp_info *exp_del_gids;
+    } a_and_r_data[] = {
+            {1, grp_list_1, 1, gid_list_2, EOK, 1, gid_list_2, 1, grp_list_1},
+            {1, grp_list_1, 0, NULL, EOK, 0, NULL, 1, grp_list_1},
+            {0, NULL, 1, gid_list_2, EOK, 1, gid_list_2, 0, NULL},
+            {2, grp_list_12, 1, gid_list_2, EOK,  0, NULL, 1, grp_list_1},
+            {2, grp_list_12, 2, gid_list_23, EOK, 1, gid_list_3, 1, grp_list_1},
+            {0, NULL, 0, NULL, 0, 0, NULL, 0, NULL}
+    };
+
+    mem_ctx = talloc_new(NULL);
+    fail_unless(mem_ctx != NULL, "talloc_new failed.");
+
+    ret = diff_gid_lists(mem_ctx, 0, NULL, 0, NULL,
+                         &add_gid_count, &add_gids,
+                         &del_gid_count, &del_gids);
+    fail_unless(ret == EOK, "get_gids_to_add_and_remove failed with empty " \
+                            "groups.");
+
+    ret = diff_gid_lists(mem_ctx, 1, NULL, 0, NULL,
+                         &add_gid_count, &add_gids,
+                         &del_gid_count, &del_gids);
+    fail_unless(ret == EINVAL, "get_gids_to_add_and_remove failed with " \
+                               "invalid current groups.");
+
+    ret = diff_gid_lists(mem_ctx, 0, NULL, 1, NULL,
+                         &add_gid_count, &add_gids,
+                         &del_gid_count, &del_gids);
+    fail_unless(ret == EINVAL, "get_gids_to_add_and_remove failed with " \
+                               "invalid new groups.");
+
+    for (c = 0; a_and_r_data[c].cur_gids != NULL ||
+                a_and_r_data[c].gids != NULL; c++) {
+        ret = diff_gid_lists(mem_ctx,
+                             a_and_r_data[c].cur_gid_count,
+                             a_and_r_data[c].cur_gids,
+                             a_and_r_data[c].gid_count,
+                             a_and_r_data[c].gids,
+                             &add_gid_count, &add_gids,
+                             &del_gid_count, &del_gids);
+        fail_unless(ret == a_and_r_data[c].exp_ret,
+                    "Unexpected return value for test data #%d, " \
+                    "expected [%d], got [%d]",
+                    c, a_and_r_data[c].exp_ret, ret);
+        fail_unless(add_gid_count ==  a_and_r_data[c].exp_add_gid_count,
+                    "Unexpected numer of groups to add for test data #%d, " \
+                    "expected [%d], got [%d]",
+                    c, a_and_r_data[c].exp_add_gid_count, add_gid_count);
+        fail_unless(del_gid_count ==  a_and_r_data[c].exp_del_gid_count,
+                    "Unexpected numer of groups to delete for test data #%d, " \
+                    "expected [%d], got [%d]",
+                    c, a_and_r_data[c].exp_del_gid_count, del_gid_count);
+
+        /* The lists might be returned in any order, to make tests simple we
+         * only look at lists with 1 element. TODO: add code to compare lists
+         * with more than 1 member. */
+        if (add_gid_count == 1) {
+            fail_unless(add_gids[0] ==  a_and_r_data[c].exp_add_gids[0],
+                        "Unexpected gid to add for test data #%d, " \
+                        "expected [%d], got [%d]",
+                        c, a_and_r_data[c].exp_add_gids[0], add_gids[0]);
+        }
+
+        if (del_gid_count == 1) {
+            fail_unless(del_gids[0]->gid == a_and_r_data[c].exp_del_gids[0].gid,
+                        "Unexpected gid to delete for test data #%d, " \
+                        "expected [%d], got [%d]",
+                        c, a_and_r_data[c].exp_del_gids[0].gid,
+                        del_gids[0]->gid);
+        }
+    }
+
+    talloc_free(mem_ctx);
+}
+END_TEST
+
 
 Suite *idmap_test_suite (void)
 {
     Suite *s = suite_create ("PAC responder");
 
     TCase *tc_pac = tcase_create("PAC responder tests");
-    /*tcase_add_checked_fixture(tc_init,
+    tcase_add_checked_fixture(tc_pac,
                               leak_check_setup,
-                              leak_check_teardown);*/
+                              leak_check_teardown);
 
     tcase_add_test(tc_pac, pac_test_local_sid_to_id);
     tcase_add_test(tc_pac, pac_test_seondary_local_sid_to_id);
+    tcase_add_test(tc_pac, pac_test_get_gids_to_add_and_remove);
 
     suite_add_tcase(s, tc_pac);
 
-- 
1.7.7.6

-------------- next part --------------
From 3d29380712ce3e593a7022fb1c92f5e0e5311d65 Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 7 Nov 2012 11:53:13 +0100
Subject: [PATCH 3/5] Add pac_user_get_grp_info() to read current group
 memberships

To be able to efficiently store group memberships we need to know the
current memberships of a user. sysdb_initgroups() is used to read the
user entry together with all groups the user is a member of. Some of the
group attributes are kept to avoid additional lookups and speed up
further processing.

Currently sysdb_initgroups() does not return the original DN of the
group. Since it is needed to remove memberships later on it is added to
the list of requested attributes
---
 src/db/sysdb.h                 |    1 +
 src/responder/pac/pacsrv_cmd.c |  106 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 107 insertions(+), 0 deletions(-)

diff --git a/src/db/sysdb.h b/src/db/sysdb.h
index 8fe0e81..39e0f9f 100644
--- a/src/db/sysdb.h
+++ b/src/db/sysdb.h
@@ -200,6 +200,7 @@
 #define SYSDB_INITGR_ATTR SYSDB_MEMBEROF
 #define SYSDB_INITGR_ATTRS {SYSDB_GIDNUM, SYSDB_POSIX, \
                             SYSDB_DEFAULT_ATTRS, \
+                            SYSDB_ORIG_DN, \
                             NULL}
 
 #define SYSDB_TMPL_USER SYSDB_NAME"=%s,"SYSDB_TMPL_USER_BASE
diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index 7777983..0e76acf 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -60,10 +60,17 @@ struct pac_req_ctx {
 
     size_t gid_count;
     gid_t *gids;
+
+    size_t current_grp_count;
+    struct grp_info *current_grp_list;
 };
 
 static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx);
 static void pac_get_domains_done(struct tevent_req *req);
+static errno_t pac_user_get_grp_info(TALLOC_CTX *mem_ctx,
+                                     struct pac_req_ctx *pr_ctx,
+                                     size_t *_current_grp_count,
+                                     struct grp_info **_current_grp_list);
 static errno_t save_pac_user(struct pac_req_ctx *pr_ctx);
 static void pac_get_group_done(struct tevent_req *subreq);
 static errno_t pac_save_memberships_next(struct tevent_req *req);
@@ -194,6 +201,13 @@ static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx)
         goto done;
     }
 
+    ret = pac_user_get_grp_info(pr_ctx, pr_ctx, &pr_ctx->current_grp_count,
+                                &pr_ctx->current_grp_list);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("pac_user_get_grp_info failed.\n"));
+        goto done;
+    }
+
     ret = get_my_domain_data(pr_ctx->pac_ctx, pr_ctx->dom,
                              &my_dom_sid, &my_range_map);
     if (ret != EOK) {
@@ -223,6 +237,98 @@ done:
     return ret;
 }
 
+static errno_t pac_user_get_grp_info(TALLOC_CTX *mem_ctx,
+                                     struct pac_req_ctx *pr_ctx,
+                                     size_t *_current_grp_count,
+                                     struct grp_info **_current_grp_list)
+{
+    struct sysdb_ctx *sysdb;
+    int ret;
+    TALLOC_CTX *tmp_ctx = NULL;
+    struct ldb_result *res = NULL;
+    struct grp_info *current_grp_list = NULL;
+    size_t current_grp_count = 0;
+    size_t c;
+    const char *tmp_str;
+
+    sysdb = pr_ctx->dom->sysdb;
+    if (sysdb == NULL) {
+        ret = EINVAL;
+        DEBUG(SSSDBG_FATAL_FAILURE, ("Fatal: Sysdb CTX not found for this domain!\n"));
+        goto done;
+    }
+
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        ret = ENOMEM;
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
+        goto done;
+    }
+
+    ret = sysdb_initgroups(tmp_ctx, sysdb, pr_ctx->user_name, &res);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_initgroups failed.\n"));
+        goto done;
+    }
+
+    /* First result is the user entry then the groups follow */
+    if (res->count > 1) {
+        current_grp_count = res->count - 1;
+        current_grp_list = talloc_array(tmp_ctx, struct grp_info,
+                                        current_grp_count);
+        if (current_grp_list == NULL) {
+            DEBUG(SSSDBG_OP_FAILURE, ("talloc_array failed.\n"));
+            ret = ENOMEM;
+            goto done;
+        }
+
+        for (c = 0; c < current_grp_count; c++) {
+            current_grp_list[c].gid =
+                                  ldb_msg_find_attr_as_uint64(res->msgs[c + 1],
+                                                              SYSDB_GIDNUM, 0);
+            if (current_grp_list[c].gid == 0) {
+                DEBUG(SSSDBG_OP_FAILURE, ("Missing GID.\n"));
+                ret = EINVAL;
+                goto done;
+            }
+
+            tmp_str = ldb_msg_find_attr_as_string(res->msgs[c + 1],
+                                                  SYSDB_ORIG_DN, NULL);
+            if (tmp_str == NULL) {
+                DEBUG(SSSDBG_OP_FAILURE, ("Missing original DN.\n"));
+                ret = EINVAL;
+                goto done;
+            }
+
+            current_grp_list[c].orig_dn = talloc_strdup(current_grp_list,
+                                                        tmp_str);
+            if (current_grp_list[c].orig_dn == NULL) {
+                DEBUG(SSSDBG_OP_FAILURE, ("talloc_strdup failed.\n"));
+                ret = ENOMEM;
+                goto done;
+            }
+
+            current_grp_list[c].dn = ldb_dn_copy(current_grp_list,
+                                                 res->msgs[c + 1]->dn);
+            if (current_grp_list[c].dn == NULL) {
+                DEBUG(SSSDBG_OP_FAILURE, ("ldb_dn_copy failed.\n"));
+                ret = ENOMEM;
+                goto done;
+            }
+        }
+    }
+
+    *_current_grp_count = current_grp_count;
+    *_current_grp_list = talloc_steal(mem_ctx, current_grp_list);
+
+    ret = EOK;
+
+done:
+    talloc_free(tmp_ctx);
+
+    return ret;
+}
+
 static errno_t save_pac_user(struct pac_req_ctx *pr_ctx)
 {
     struct sysdb_ctx *sysdb;
-- 
1.7.7.6

-------------- next part --------------
From 039a75335b60edd8e9211db5d3f0dcc23eaa187f Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 7 Nov 2012 12:01:27 +0100
Subject: [PATCH 4/5] Get lists of GIDs to be added and deleted and use them

Currently the user was just added to all local groups which are given in
the PAC. With this patch the user is added only to groups he is
currently not a member of and deleted from groups which are not found in
the PAC anymore.
---
 src/responder/pac/pacsrv_cmd.c |   92 ++++++++++++++++++++++++++++++++++++++-
 1 files changed, 89 insertions(+), 3 deletions(-)

diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index 0e76acf..d0091dd 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -63,6 +63,12 @@ struct pac_req_ctx {
 
     size_t current_grp_count;
     struct grp_info *current_grp_list;
+
+    size_t add_gid_count;
+    gid_t *add_gids;
+
+    size_t del_grp_count;
+    struct grp_info **del_grp_list;
 };
 
 static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx);
@@ -223,6 +229,12 @@ static errno_t pac_add_user_next(struct pac_req_ctx *pr_ctx)
         goto done;
     }
 
+    ret = diff_gid_lists(pr_ctx,
+                         pr_ctx->current_grp_count, pr_ctx->current_grp_list,
+                         pr_ctx->gid_count, pr_ctx->gids,
+                         &pr_ctx->add_gid_count, &pr_ctx->add_gids,
+                         &pr_ctx->del_grp_count, &pr_ctx->del_grp_list);
+
     req = pac_save_memberships_send(pr_ctx);
     if (req == NULL) {
         ret = ENOMEM;
@@ -395,6 +407,9 @@ struct pac_save_memberships_state {
     struct sss_domain_info *group_dom;
 };
 
+static errno_t
+pac_save_memberships_delete(struct pac_save_memberships_state *state);
+
 struct tevent_req *pac_save_memberships_send(struct pac_req_ctx *pr_ctx)
 {
     struct pac_save_memberships_state *state;
@@ -422,6 +437,12 @@ struct tevent_req *pac_save_memberships_send(struct pac_req_ctx *pr_ctx)
         state->group_dom = pr_ctx->dom;
     }
 
+    ret = pac_save_memberships_delete(state);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("pac_save_memberships_delete failed.\n"));
+        goto done;
+    }
+
     ret = pac_save_memberships_next(req);
     if (ret == EOK) {
         tevent_req_done(req);
@@ -437,6 +458,62 @@ done:
     return req;
 }
 
+static errno_t
+pac_save_memberships_delete(struct pac_save_memberships_state *state)
+{
+    int ret;
+    int sret;
+    size_t c;
+    struct pac_req_ctx *pr_ctx;
+    bool in_transaction = false;
+
+    pr_ctx = state->pr_ctx;
+
+    if (pr_ctx->del_grp_count == 0) {
+        return EOK;
+    }
+
+    if (pr_ctx->del_grp_list == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Missing group list.\n"));
+        return EINVAL;
+    }
+
+    ret = sysdb_transaction_start(state->group_dom->sysdb);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_transaction_start failed.\n"));
+        goto done;
+    }
+    in_transaction = true;
+
+    for (c = 0; c < pr_ctx->del_grp_count; c++) {
+        ret = sysdb_mod_group_member(state->group_dom->sysdb, state->user_dn,
+                                     pr_ctx->del_grp_list[c]->dn,
+                                     LDB_FLAG_MOD_DELETE);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE, ("sysdb_mod_group_member failed.\n"));
+            goto done;
+        }
+    }
+
+    ret = sysdb_transaction_commit(state->group_dom->sysdb);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_transaction_commit failed.\n"));
+        goto done;
+    }
+    in_transaction = false;
+
+    ret = EOK;
+done:
+    if (in_transaction) {
+        sret = sysdb_transaction_cancel(state->group_dom->sysdb);
+        if (sret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE, ("sysdb_transaction_cancel failed.\n"));
+        }
+    }
+
+    return ret;
+}
+
 static errno_t pac_save_memberships_next(struct tevent_req *req)
 {
     errno_t ret;
@@ -448,8 +525,17 @@ static errno_t pac_save_memberships_next(struct tevent_req *req)
     state = tevent_req_data(req, struct pac_save_memberships_state);
     pr_ctx = state->pr_ctx;
 
-    while (state->gid_iter < pr_ctx->gid_count) {
-        gid = pr_ctx->gids[state->gid_iter];
+    if (pr_ctx->add_gid_count == 0) {
+        return EOK;
+    }
+
+    if (pr_ctx->add_gids == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Missing group list.\n"));
+        return EINVAL;
+    }
+
+    while (state->gid_iter < pr_ctx->add_gid_count) {
+        gid = pr_ctx->add_gids[state->gid_iter];
 
         ret = pac_store_membership(state->pr_ctx, state->group_dom->sysdb,
                                     state->user_dn, state->gid_iter);
@@ -538,7 +624,7 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
         return ENOMEM;
     }
 
-    gid = pr_ctx->gids[gid_iter];
+    gid = pr_ctx->add_gids[gid_iter];
 
     ret = sysdb_search_group_by_gid(tmp_ctx, group_sysdb,
                                     gid, NULL, &group);
-- 
1.7.7.6

-------------- next part --------------
From 1b1c02095737c5ce1c0783eca4817d5ab834aa6b Mon Sep 17 00:00:00 2001
From: Sumit Bose <sbose at redhat.com>
Date: Wed, 7 Nov 2012 12:09:55 +0100
Subject: [PATCH 5/5] Store the original group DN in the subdomain user object

For user of the local domain the server-side DN of the groups the user
is a member of is stored with the user object in the cache and used to
improve performance e.g. by the HBAC code. Since subdomain users should
be handled by HBAC as well the group DN is stored in the same way as for
users of the local domain.

This patch also adds code to remove the attribute from the user object
if the user is removed from the group.
---
 src/responder/pac/pacsrv_cmd.c |   84 +++++++++++++++++++++++++++------------
 1 files changed, 58 insertions(+), 26 deletions(-)

diff --git a/src/responder/pac/pacsrv_cmd.c b/src/responder/pac/pacsrv_cmd.c
index d0091dd..5721d92 100644
--- a/src/responder/pac/pacsrv_cmd.c
+++ b/src/responder/pac/pacsrv_cmd.c
@@ -466,6 +466,8 @@ pac_save_memberships_delete(struct pac_save_memberships_state *state)
     size_t c;
     struct pac_req_ctx *pr_ctx;
     bool in_transaction = false;
+    TALLOC_CTX *tmp_ctx;
+    struct sysdb_attrs *user_attrs = NULL;
 
     pr_ctx = state->pr_ctx;
 
@@ -478,6 +480,19 @@ pac_save_memberships_delete(struct pac_save_memberships_state *state)
         return EINVAL;
     }
 
+    tmp_ctx = talloc_new(NULL);
+    if (tmp_ctx == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("talloc_new failed.\n"));
+        return ENOMEM;
+    }
+
+    user_attrs = sysdb_new_attrs(tmp_ctx);
+    if (user_attrs == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_new_attrs failed.\n"));
+        ret = ENOMEM;
+        goto done;
+    }
+
     ret = sysdb_transaction_start(state->group_dom->sysdb);
     if (ret != EOK) {
         DEBUG(SSSDBG_OP_FAILURE, ("sysdb_transaction_start failed.\n"));
@@ -493,6 +508,20 @@ pac_save_memberships_delete(struct pac_save_memberships_state *state)
             DEBUG(SSSDBG_OP_FAILURE, ("sysdb_mod_group_member failed.\n"));
             goto done;
         }
+
+        ret = sysdb_attrs_add_string(user_attrs, SYSDB_ORIG_MEMBEROF,
+                                     pr_ctx->del_grp_list[c]->orig_dn);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_add_string failed.\n"));
+            goto done;
+        }
+    }
+
+    ret = sysdb_set_entry_attr(pr_ctx->dom->sysdb, state->user_dn, user_attrs,
+                               LDB_FLAG_MOD_DELETE);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_set_entry_attr failed.\n"));
+        goto done;
     }
 
     ret = sysdb_transaction_commit(state->group_dom->sysdb);
@@ -511,6 +540,8 @@ done:
         }
     }
 
+    talloc_free(tmp_ctx);
+
     return ret;
 }
 
@@ -613,11 +644,12 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
                       int gid_iter)
 {
     TALLOC_CTX *tmp_ctx;
-    const char *group_name;
-    struct sysdb_attrs *group_attrs;
+    struct sysdb_attrs *user_attrs;
     struct ldb_message *group;
     uint32_t gid;
     errno_t ret;
+    const char *orig_group_dn;
+    const char *group_attrs[] = { SYSDB_ORIG_DN, NULL };
 
     tmp_ctx = talloc_new(NULL);
     if (tmp_ctx == NULL) {
@@ -627,46 +659,46 @@ pac_store_membership(struct pac_req_ctx *pr_ctx,
     gid = pr_ctx->add_gids[gid_iter];
 
     ret = sysdb_search_group_by_gid(tmp_ctx, group_sysdb,
-                                    gid, NULL, &group);
+                                    gid, group_attrs, &group);
     if (ret != EOK) {
+        DEBUG(SSSDBG_TRACE_INTERNAL, ("sysdb_search_group_by_gid for gid [%d]" \
+                                      "failed [%d][%s].\n",
+                                      gid, ret, strerror(ret)));
         goto done;
     }
 
-    group_name = ldb_msg_find_attr_as_string(group, SYSDB_NAME, NULL);
-    if (group_name == NULL) {
-        ret = EIO;
+    ret = sysdb_mod_group_member(group_sysdb, user_dn, group->dn,
+                                 LDB_FLAG_MOD_ADD);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_mod_group_member failed.\n"));
         goto done;
     }
 
-    group_attrs = talloc_zero(tmp_ctx, struct sysdb_attrs);
-    if (group_attrs == NULL) {
-        ret = ENOMEM;
+    orig_group_dn = ldb_msg_find_attr_as_string(group, SYSDB_ORIG_DN, NULL);
+    if (orig_group_dn == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("Original DN not found.\n"));
+        ret = EINVAL;
         goto done;
     }
-    group_attrs->num = 1;
-    group_attrs->a = ldb_msg_find_element(group, SYSDB_MEMBER);
-    if (group_attrs->a == NULL) {
-        group_attrs->a = talloc_zero(group_attrs, struct ldb_message_element);
-        if (group_attrs->a == NULL) {
-            ret = ENOMEM;
-            goto done;
-        }
-        group_attrs->a[0].name = talloc_strdup(group_attrs->a, SYSDB_MEMBER);
-        if (group_attrs->a[0].name == NULL) {
-            ret = ENOMEM;
-            goto done;
-        }
+
+    user_attrs = sysdb_new_attrs(tmp_ctx);
+    if (user_attrs == NULL) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_new_attrs failed.\n"));
+        ret = ENOMEM;
+        goto done;
     }
 
-    ret = sysdb_attrs_add_string(group_attrs, SYSDB_MEMBER,
-                                 ldb_dn_get_linearized(user_dn));
+    ret = sysdb_attrs_add_string(user_attrs, SYSDB_ORIG_MEMBEROF,
+                                 orig_group_dn);
     if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_attrs_add_string failed.\n"));
         goto done;
     }
 
-    ret = sysdb_store_group(group_sysdb, group_name, gid,
-                            group_attrs, pr_ctx->dom->group_timeout, 0);
+    ret = sysdb_set_entry_attr(pr_ctx->dom->sysdb, user_dn, user_attrs,
+                              LDB_FLAG_MOD_ADD);
     if (ret != EOK) {
+        DEBUG(SSSDBG_OP_FAILURE, ("sysdb_set_entry_attr failed.\n"));
         goto done;
     }
 
-- 
1.7.7.6



More information about the sssd-devel mailing list