[SSSD] IPA: resolve ghost members if a non-default view is applied

Jakub Hrozek jhrozek at redhat.com
Tue Jan 13 15:59:18 UTC 2015


On Tue, Jan 13, 2015 at 11:35:32AM +0100, Sumit Bose wrote:
> On Fri, Jan 09, 2015 at 11:54:27AM +0100, Jakub Hrozek wrote:
> > On Wed, Dec 17, 2014 at 11:27:46AM +0100, Sumit Bose wrote:
> > > yes, the check was useless because state->obj_msg is always set if
> > > get_object_from_cache() returns EOK.
> > > 
> > > New version attached.
> > > 
> > > bye,
> > > Sumit
> > > 
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > 
> > 
> > After testing the patches, I can confirm that group resolution of a
> > group that contains a user with overriden login attribute:
> >     ipa idoverrideuser-mod --login=overriden_tuser
> > 
> > Now works OK. But getent group still shows the original user name (b/c
> > the original user name is set in the memberUid attribute, set by the
> > memberof plugin). Is that expected.
> 
> no, the new version contains patches 3 and 4 which should fix it.
> 
> > 
> > There is a compilation warning in the code:
> > src/providers/ipa/ipa_id.c: In function 'ipa_resolve_user_list_get_user_step':
> > src/providers/ipa/ipa_id.c:218:11: warning: pointer targets in passing argument 2 of 'get_be_acct_req_for_user_name' differ in signedness [-Wpointer-sign]
> >      ret = get_be_acct_req_for_user_name(state,
> >            ^
> > In file included from src/providers/ipa/ipa_id.c:31:0:
> > ./src/providers/ipa/ipa_id.h:90:9: note: expected 'const char *' but argument is of type 'uint8_t *'
> 
> fixed
> 
> > 
> > Two nitpicks in the code:
> > 
> > > +static struct tevent_req *
> > > +ipa_resolve_user_list_send(TALLOC_CTX *memctx, struct tevent_context *ev,
> > > +                           struct be_req *be_req,
> > > +                           struct sdap_id_ctx *sdap_id_ctx,
> > > +                           const char *domain_name,
> > > +                           struct ldb_message_element *users)
> > > +{
> > > +    int ret;
> > > +    struct tevent_req *req;
> > > +    struct ipa_resolve_user_list_state *state;
> > > +
> > > +    req = tevent_req_create(memctx, &state,
> > > +                            struct ipa_resolve_user_list_state);
> > > +    if (req == NULL) {
> > > +        DEBUG(SSSDBG_OP_FAILURE, "tevent_req_create failed.\n");
> > > +        return NULL;
> > > +    }
> > > +
> > > +    state->ev = ev;
> > > +    state->sdap_id_ctx = sdap_id_ctx;
> > > +    state->be_req = be_req;
> > > +    state->domain_name = domain_name;
> > > +    state->users = users;
> > > +    state->user_idx = 0;
> > > +    state->dp_error = DP_ERR_FATAL;
> > > +
> > > +    ret = ipa_resolve_user_list_get_user_step(req);
> > > +    if (ret == EAGAIN) {
> > > +        return req;
> > > +    }
> > 
> > Can you add else if into this check:
> >   if EAGAIN
> >   else if ret != EOK
> > 
> > To make it more clear that it's one check, not two.
> 
> fixed
> 
> > 
> > > +    if (ret != EOK) {
> > > +        DEBUG(SSSDBG_OP_FAILURE,
> > > +              "ipa_resolve_user_list_get_user_step failed.\n");
> > > +    }
> > 
> > [...]
> > 
> > > @@ -166,6 +315,7 @@ static errno_t ipa_id_get_account_info_get_original_step(struct tevent_req *req,
> > >                                                          struct be_acct_req *ar);
> > >  static void ipa_id_get_account_info_orig_done(struct tevent_req *subreq);
> > >  static void ipa_id_get_account_info_done(struct tevent_req *subreq);
> > > +static void ipa_id_get_user_list_done(struct tevent_req *subreq);
> > >  
> > >  static struct tevent_req *
> > >  ipa_id_get_account_info_send(TALLOC_CTX *memctx, struct tevent_context *ev,
> > > @@ -405,6 +555,16 @@ static void ipa_id_get_account_info_orig_done(struct tevent_req *subreq)
> > >          goto fail;
> > >      }
> > >  
> > > +    if ((state->ar->entry_type & BE_REQ_TYPE_MASK) == BE_REQ_GROUP
> > > +            && state->ipa_ctx->view_name != NULL
> > > +            && strcmp(state->ipa_ctx->view_name,
> > > +                      SYSDB_DEFAULT_VIEW_NAME) != 0) {
> > > +        /* check for ghost members because shost members are not allowed if a
> > 
> > typo: s/shost/ghost/
> 
> fixed
> 
> New version attached.
> 
> Thank you for the review.

CI passed:
http://sssd-ci.duckdns.org/logs/commit/24/9fefb9b90a4b3acf4107470ac1bf4b47355ceb/574/summary.html

> 
> bye,
> Sumit
> 
> > 
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel at lists.fedorahosted.org
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

> From c88f86f2854bb735c411b59b4638c2fa3dd29928 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 10 Dec 2014 15:02:15 +0100
> Subject: [PATCH 1/4] IPA: add get_be_acct_req_for_user_name()
> 
> Related to https://fedorahosted.org/sssd/ticket/2481

ACK

> From 93514fe4ac86ad6587bab593575f41a25ba07082 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Wed, 10 Dec 2014 15:03:18 +0100
> Subject: [PATCH 2/4] IPA: resolve ghost members if a non-default view is
>  applied
> 

ACK

> Related to https://fedorahosted.org/sssd/ticket/2481
> From f0e658bf310f0af25c9ee0498b75436cba726744 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Mon, 12 Jan 2015 18:36:42 +0100
> Subject: [PATCH 3/4] nss: fix group members with overridden names

ACK, but another call of sss_parse_name() makes me pretty convinced that
we should take a look at always storing FQDNs in the cache instead of
parsing each memberuid..

I will also change the subject prefix from "nss" to "sysdb" if you don't
mind.

> From 1c240e4991050028ccc31c63178de2cd5bdd5637 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose at redhat.com>
> Date: Tue, 13 Jan 2015 11:03:37 +0100
> Subject: [PATCH 4/4] IPA: ipa_resolve_user_list_send() take care of overrides

ACK

I'm waiting on Coverity results before pushing.



More information about the sssd-devel mailing list