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(a)redhat.com>
Date: Fri, 22 Aug 2014 19:45:05 +0200
Subject: [PATCH 1/2] SDAP: Immediately finish request for empty array
ACK
From ac7bf888e0f6bd064551bf62922fe8dbf7df8080 Mon Sep 17 00:00:00
2001
From: Lukas Slebodnik <lslebodn(a)redhat.com>
Date: Mon, 25 Aug 2014 10:18:51 +0200
Subject: [PATCH 2/2] 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
Mostly good, see couple of nitpicks inline.
---
src/providers/ldap/sdap_async_initgroups_ad.c | 84 ++++++++++++++++++++-------
1 file changed, 63 insertions(+), 21 deletions(-)
diff --git a/src/providers/ldap/sdap_async_initgroups_ad.c
b/src/providers/ldap/sdap_async_initgroups_ad.c
index a80bccbccb1dd675e2bc29fbd7f527526a85fd6e..989618331b7f426199f858160910d5023afe1e46
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 **valid_groups;
+ size_t num_valid_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;
@@ -1207,7 +1211,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);
This should be a separate patch, it's a problem in the old code as well,
even though not revealed until now.
if (valid_groups[num_valid_groups] == 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 (_missing != _valid_groups) {
+ *_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_valid_groups,
+ &state->valid_groups);
'cached groups' would be better name I think.
if (ret != EOK) {
goto done;
}
@@ -1320,6 +1322,8 @@ 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 **valid_groups;
+ size_t num_valid_groups;
cached groups here as well.
req = tevent_req_callback_data(subreq, struct tevent_req);
state = tevent_req_data(req, struct sdap_ad_tokengroups_initgr_posix_state);
@@ -1332,10 +1336,48 @@ 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_valid_groups,
+ &valid_groups);
+ if (ret != EOK){
+ goto done;
+ }
+
+ if (num_valid_groups > 0){
This whole block should be a separate function.
+ /* we need to extend array of valid groups from state */
+ size_t i,j;
+ size_t new_size = state->num_valid_groups + num_valid_groups + 1;
+ char ** string_array = talloc_realloc(state,
+ state->valid_groups,
+ char *,
+ new_size);
+ if (string_array == NULL) {
+ ret = ENOMEM;
+ goto done;
+ }
+ state->valid_groups = string_array;
+
+ for (i=state->num_valid_groups, j=0; i < new_size; ++i,++j) {
+ state->valid_groups[i] = talloc_steal(state->valid_groups,
+ valid_groups[j]);
+ state->num_valid_groups++;
+ }
+
+ state->valid_groups[state->num_valid_groups] = NULL;
+ }
+
+ /* update membership of existing groups */
+ ret = sdap_ad_tokengroups_update_members(state->username,
+ state->sysdb, state->domain,
+ state->valid_groups);
+ if (ret != EOK) {
+ DEBUG(SSSDBG_MINOR_FAILURE, "Membership update failed [%d]: %s\n",
+ ret, strerror(ret));
+ goto done;
+ }
done:
if (ret != EOK) {
--
2.1.0
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel