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.
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.