[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