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(a)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/9fefb9b90a4b3acf4107470ac1bf4b4...
>
> bye,
> Sumit
>
> >
> > _______________________________________________
> > sssd-devel mailing list
> > sssd-devel(a)lists.fedorahosted.org
> >
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> From c88f86f2854bb735c411b59b4638c2fa3dd29928 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)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(a)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(a)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.
sure. go ahead
bye,
Sumit
> From 1c240e4991050028ccc31c63178de2cd5bdd5637 Mon Sep 17 00:00:00 2001
> From: Sumit Bose <sbose(a)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.
_______________________________________________
sssd-devel mailing list
sssd-devel(a)lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel