[SSSD] [PATCH] SDAP: Update groups for user just once.
Jakub Hrozek
jhrozek at redhat.com
Tue Aug 26 13:35:38 UTC 2014
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
> From ac7bf888e0f6bd064551bf62922fe8dbf7df8080 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 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 at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
More information about the sssd-devel
mailing list