[SSSD] [PATCH] SDAP: Update groups for user just once.

Lukas Slebodnik lslebodn at redhat.com
Tue Aug 26 14:43:17 UTC 2014


On (26/08/14 15:35), Jakub Hrozek wrote:
>On Mon, Aug 25, 2014 at 06:21:22PM +0200, Lukas Slebodnik wrote:
>> ehlo,
>> 
>> attached two patches should fix problem with resolving groups
>> from AD child domain. https://fedorahosted.org/sssd/ticket/2407
>> It was caused by change in commit 983983dd1629ab33eab340a40d9ee83965a339c6
>> 
>> LS
>
>> From a94b4d389abc943d3ddc09d87e2f698fb82b1994 Mon Sep 17 00:00:00 2001
>> From: Lukas Slebodnik <lslebodn at redhat.com>
>> Date: Fri, 22 Aug 2014 19:45:05 +0200
>> Subject: [PATCH 1/2] SDAP: Immediately finish request for empty array
>
>ACK
>
Updated version attached.

LS
-------------- next part --------------
>From cb677046c2b90bb7c6439605a0999fa3123550e5 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Fri, 22 Aug 2014 19:45:05 +0200
Subject: [PATCH 1/3] SDAP: Immediately finish request for empty array

If array of sids is empty we needn't try to resolve them
and we can immediately finish request in function sdap_ad_resolve_sids_send
This patch is just a small optimisation.
---
 src/providers/ldap/sdap_async_initgroups_ad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index 08e012d0fa0eb01b45d14a1a190100448641f53b..a80bccbccb1dd675e2bc29fbd7f527526a85fd6e 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -567,7 +567,7 @@ sdap_ad_resolve_sids_send(TALLOC_CTX *mem_ctx,
     state->sids = sids;
     state->index = 0;
 
-    if (state->sids == NULL) {
+    if (state->sids == NULL || state->sids[0] == NULL) {
         ret = EOK;
         goto immediately;
     }
-- 
2.1.0

-------------- next part --------------
>From eb4a5999e9621b9edd91037c358fc13cf2719be7 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Tue, 26 Aug 2014 15:42:58 +0200
Subject: [PATCH 2/3] SDAP: Use different talloc_context for array of names

It will be easier to steal whole array to another talloc context
---
 src/providers/ldap/sdap_async_initgroups_ad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index a80bccbccb1dd675e2bc29fbd7f527526a85fd6e..3e0d585eecdd212e99f9775f09b90bdd1a661475 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -1207,7 +1207,7 @@ sdap_ad_tokengroups_update_posix_members(TALLOC_CTX *mem_ctx,
                 goto done;
             }
 
-            valid_groups[num_valid_groups] = sysdb_group_strdn(tmp_ctx,
+            valid_groups[num_valid_groups] = sysdb_group_strdn(valid_groups,
                                                                domain->name,
                                                                name);
             if (valid_groups[num_valid_groups] == NULL) {
-- 
2.1.0

-------------- next part --------------
>From 80e2bedc4b7358c1b9b4cdbead0d4be9282e29b3 Mon Sep 17 00:00:00 2001
From: Lukas Slebodnik <lslebodn at redhat.com>
Date: Mon, 25 Aug 2014 10:18:51 +0200
Subject: [PATCH 3/3] SDAP: Update groups for user just once.

The function sdap_ad_tokengroups_update_members finds the differences between
list of groups from sysdb and list of groups from LDAP (input argument).
For each new group, connections are created between user and group. The other
connections are removed.

The problem was that in some cases function sdap_ad_tokengroups_update_members
was called twice (sdap_ad_tokengroups_initgr_posix_tg_done and
sdap_ad_tokengroups_initgr_posix_sids_done).

The first call created connection between user and groups resolved from
tokengroups and the second call update groups from missing SIDs, but previously
created connections were removed. The worst case was when there weren't any
missing groups. This behaviour caused missing groups in some cases (for users
in child ad domain)

This patch join array of groups obtained from token group and array of groups
obtained from missing SIDs. The function sdap_ad_tokengroups_update_members
is called just once with single array.

Resolves:
https://fedorahosted.org/sssd/ticket/2407
---
 src/providers/ldap/sdap_async_initgroups_ad.c | 97 +++++++++++++++++++++------
 1 file changed, 77 insertions(+), 20 deletions(-)

diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c b/src/providers/ldap/sdap_async_initgroups_ad.c
index 3e0d585eecdd212e99f9775f09b90bdd1a661475..dee30600f9d8cdbee665755b77341a3e8da58780 100644
--- a/src/providers/ldap/sdap_async_initgroups_ad.c
+++ b/src/providers/ldap/sdap_async_initgroups_ad.c
@@ -1002,6 +1002,8 @@ struct sdap_ad_tokengroups_initgr_posix_state {
     struct sdap_id_op *op;
     char **missing_sids;
     size_t num_missing_sids;
+    char **cached_groups;
+    size_t num_cached_groups;
 };
 
 static void
@@ -1135,12 +1137,14 @@ sdap_ad_tokengroups_initgr_posix_sids_connect_done(struct tevent_req *subreq)
 }
 
 static errno_t
-sdap_ad_tokengroups_update_posix_members(TALLOC_CTX *mem_ctx,
+sdap_ad_tokengroups_get_posix_members(TALLOC_CTX *mem_ctx,
                          struct sdap_ad_tokengroups_initgr_posix_state *state,
                          size_t num_sids,
                          char **sids,
                          size_t *_num_missing,
-                         char ***_missing)
+                         char ***_missing,
+                         size_t *_num_valid,
+                         char ***_valid_groups)
 {
     TALLOC_CTX *tmp_ctx = NULL;
     struct sss_domain_info *domain = NULL;
@@ -1239,22 +1243,18 @@ sdap_ad_tokengroups_update_posix_members(TALLOC_CTX *mem_ctx,
     valid_groups[num_valid_groups] = NULL;
     missing_sids[num_missing_sids] = NULL;
 
-    /* update membership of existing groups */
-    ret = sdap_ad_tokengroups_update_members(state->username,
-                                             state->sysdb, state->domain,
-                                             valid_groups);
-    if (ret != EOK) {
-        DEBUG(SSSDBG_MINOR_FAILURE, "Membership update failed [%d]: %s\n",
-                                     ret, strerror(ret));
-        goto done;
-    }
-
     /* return list of missing groups */
     if (_missing != NULL) {
         *_missing = talloc_steal(mem_ctx, missing_sids);
         *_num_missing = num_missing_sids;
     }
 
+    /* return list of missing groups */
+    if (_valid_groups != NULL) {
+        *_valid_groups = talloc_steal(mem_ctx, valid_groups);
+        *_num_valid = num_valid_groups;
+    }
+
     ret = EOK;
 
 done:
@@ -1282,10 +1282,12 @@ sdap_ad_tokengroups_initgr_posix_tg_done(struct tevent_req *subreq)
         goto done;
     }
 
-    ret = sdap_ad_tokengroups_update_posix_members(state, state,
-                                                   num_sids, sids,
-                                                   &state->num_missing_sids,
-                                                   &state->missing_sids);
+    ret = sdap_ad_tokengroups_get_posix_members(state, state,
+                                                num_sids, sids,
+                                                &state->num_missing_sids,
+                                                &state->missing_sids,
+                                                &state->num_cached_groups,
+                                                &state->cached_groups);
     if (ret != EOK) {
         goto done;
     }
@@ -1314,12 +1316,18 @@ done:
     tevent_req_done(req);
 }
 
+static char **concatenate_string_array(TALLOC_CTX *mem_ctx,
+                                       char **arr1, size_t len1,
+                                       char **arr2, size_t len2);
+
 static void
 sdap_ad_tokengroups_initgr_posix_sids_done(struct tevent_req *subreq)
 {
     struct sdap_ad_tokengroups_initgr_posix_state *state = NULL;
     struct tevent_req *req = NULL;
     errno_t ret;
+    char **cached_groups;
+    size_t num_cached_groups;
 
     req = tevent_req_callback_data(subreq, struct tevent_req);
     state = tevent_req_data(req, struct sdap_ad_tokengroups_initgr_posix_state);
@@ -1332,10 +1340,38 @@ sdap_ad_tokengroups_initgr_posix_sids_done(struct tevent_req *subreq)
         goto done;
     }
 
-    ret = sdap_ad_tokengroups_update_posix_members(state, state,
-                                                   state->num_missing_sids,
-                                                   state->missing_sids,
-                                                   NULL, NULL);
+    ret = sdap_ad_tokengroups_get_posix_members(state, state,
+                                                state->num_missing_sids,
+                                                state->missing_sids,
+                                                NULL, NULL,
+                                                &num_cached_groups,
+                                                &cached_groups);
+    if (ret != EOK){
+        DEBUG(SSSDBG_MINOR_FAILURE,
+              "sdap_ad_tokengroups_get_posix_members failed [%d]: %s\n",
+              ret, strerror(ret));
+        goto done;
+    }
+
+    state->cached_groups = concatenate_string_array(state,
+                                                    state->cached_groups,
+                                                    state->num_cached_groups,
+                                                    cached_groups,
+                                                    num_cached_groups);
+    if (state->cached_groups == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+
+    /* update membership of existing groups */
+    ret = sdap_ad_tokengroups_update_members(state->username,
+                                             state->sysdb, state->domain,
+                                             state->cached_groups);
+    if (ret != EOK) {
+        DEBUG(SSSDBG_MINOR_FAILURE, "Membership update failed [%d]: %s\n",
+                                     ret, strerror(ret));
+        goto done;
+    }
 
 done:
     if (ret != EOK) {
@@ -1346,6 +1382,27 @@ done:
     tevent_req_done(req);
 }
 
+static char **concatenate_string_array(TALLOC_CTX *mem_ctx,
+                                       char **arr1, size_t len1,
+                                       char **arr2, size_t len2)
+{
+    size_t i, j;
+    size_t new_size = len1 + len2;
+    char ** string_array = talloc_realloc(mem_ctx, arr1, char *, new_size + 1);
+    if (string_array == NULL) {
+        return NULL;
+    }
+
+    for (i=len1, j=0; i < new_size; ++i,++j) {
+        string_array[i] = talloc_steal(string_array,
+                                       arr2[j]);
+    }
+
+    string_array[i] = NULL;
+
+    return string_array;
+}
+
 static errno_t sdap_ad_tokengroups_initgr_posix_recv(struct tevent_req *req)
 {
     TEVENT_REQ_RETURN_ON_ERROR(req);
-- 
2.1.0



More information about the sssd-devel mailing list