On (24/01/14 12:20), Jakub Hrozek wrote: >On Wed, Jan 22, 2014 at 08:11:47PM +0100, Pavel Reichl wrote: >> On Wed, 2014-01-22 at 17:31 +0100, Jakub Hrozek wrote: >> > On Wed, Jan 22, 2014 at 05:13:38PM +0100, Pavel Reichl wrote: >> > > On Fri, 2014-01-17 at 13:02 +0100, Sumit Bose wrote: >> > > > On Fri, Jan 17, 2014 at 12:56:09PM +0100, Jakub Hrozek wrote: >> > > > > On Fri, Jan 17, 2014 at 12:43:04PM +0100, Sumit Bose wrote: >> > > > > > On Fri, Jan 17, 2014 at 12:04:38PM +0100, Jakub Hrozek wrote: >> > > > > > > On Fri, Jan 17, 2014 at 11:55:08AM +0100, Pavel Reichl wrote: >> > > > > > > > >> > > > > > > > > We just have to remember to touch this code, if we start to >> > > > > > > > > support home >> > > > > > > > > directories defined in AD. >> > > > > > > > > >> > > > > > > > >> > > > > > > > I'm sorry that I failed to see what you were implying. My >> > > > > > > > patch will >> > > > > > > > override value of homedir for members of every subdomain by >> > > > > > > > subdomain_homedir. >> > > > > > > > >> > > > > > > > I missed that even if subdomain_homedir is not set explicitly >> > > > > > > > in >> > > > > > > > sssd.conf it has a non NULL value. >> > > > > > > >> > > > > > > Me too and to be honest I failed to test the patch with AD >> > > > > > > subdomains >> > > > > > > with POSIX attributes. Sorry about that. >> > > > > > > >> > > > > > > I think Sumit's suggestion makes the most sense now, but the >> > > > > > > documentation should be amended, too. Currently the man page >> > > > > > > implies >> > > > > > > that the subdomain_homedir parameter will have equal effect for >> > > > > > > all >> > > > > > > kinds of subdomains, while the code would only use it for IPA-AD >> > > > > > > trust >> > > > > > > cases (be it for the server mode or the client). >> > > > > > >> > > > > > I also realized that having the right home directory in sysdb >> > > > > > might be >> > > > > > useful because krb5_ccname_template allows to expand %h to the >> > > > > > user's >> > > > > > home directory. And here the home directory is looked up in sysdb. >> > > > > > >> > > > > > bye, >> > > > > > Sumit >> > > > > >> > > > > But that also affects the other options, right? Sounds like a second >> > > > > problem to me, or did you want to solve both together? >> > > > >> > > > no >> > > > >> > > > > >> > > > > About the subdomain homedir, I think we should: >> > > > > 1) revert the original patch that is commited >> > > > > 2) fix the IPA server mode the way you described in your reply to >> > > > > Pavel's mail >> > > > > 3) Amend the subdomain_homedir documentation so that it's clear >> > > > > where the option has effect >> > > > >> > > > +1 >> > > > >> > > > bye, >> > > > Sumit >> > > > >> > > > > _______________________________________________ >> > > > > sssd-devel mailing list >> > > > > [email protected] >> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >> > > > _______________________________________________ >> > > > sssd-devel mailing list >> > > > [email protected] >> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel >> > > >> > > Hello, >> > > >> > > please see attached patches. >> > > >> > > PR >> > > >> > >> > I haven't tried the patches yet, just read the diff.. >> > >> > > From 4300f985377bfe16c40faf3f373e1875e5f80433 Mon Sep 17 00:00:00 2001 >> > > From: Pavel Reichl <[email protected]> >> > > Date: Tue, 21 Jan 2014 15:06:37 +0000 >> > > Subject: [PATCH 1/2] Revert "NSS: add support for subdomain_homedir" >> > >> > Looks good to me. >> > >> > > From 5eb3fd0cc7a11b0f71fa10a3d41028738382bc8e Mon Sep 17 00:00:00 2001 >> > > From: Pavel Reichl <[email protected]> >> > > Date: Wed, 22 Jan 2014 16:47:22 +0000 >> > > Subject: [PATCH 2/2] AD: support for subdomain_homedir >> > > >> > >> > See two comments inline >> > >> > > If users from AD don't have set homedir then subdomain_homedir is used >> > > as default. >> > > >> > > Resolves: >> > > https://fedorahosted.org/sssd/ticket/2169 >> > > --- >> > > src/providers/ipa/ipa_subdomains_id.c | 89 >> > > +++++++++++++++++++++++++++++++++++ >> > > 1 file changed, 89 insertions(+) >> > > >> > > diff --git a/src/providers/ipa/ipa_subdomains_id.c >> > > b/src/providers/ipa/ipa_subdomains_id.c >> > > index >> > > c29a2a3047af105966b636422105abd15e8a3992..a498a01e9dcc3e3a4c7b59323da9dae017876686 >> > > 100644 >> > > --- a/src/providers/ipa/ipa_subdomains_id.c >> > > +++ b/src/providers/ipa/ipa_subdomains_id.c >> > > @@ -350,6 +350,70 @@ ipa_get_ad_id_ctx(struct ipa_id_ctx *ipa_ctx, >> > > return (iter) ? iter->ad_id_ctx : NULL; >> > > } >> > > >> > > +static errno_t >> > > +use_subdomain_homedir(TALLOC_CTX *mem_ctx, const struct sss_domain_info >> > > *dom, >> > > + const char *name, uint32_t uid) >> > > +{ >> > > + errno_t ret; >> > > + char *fqname = NULL; >> > > + char *homedir = NULL; >> > > + struct sysdb_attrs *attrs = NULL; >> > > + struct sysdb_ctx *sysdb = dom->sysdb; >> > >> > You can create a child context here: >> > TALLOC_CTX *tmp_ctx = NULL; >> > >> > tmp_ctx = talloc_new(mem_ctx); /* or NULL */ >> > if (tmp_ctx == NULL) { >> > return ENOMEM; >> > } >> > > + >> > > + fqname = talloc_asprintf(mem_ctx, "%s@%s", name, dom->name); >> > >> > Then use tmp_ctx instead of mem_ctx through the function >> > >> > > + if (fqname == NULL) { >> > > + ret = ENOMEM; >> > > + goto done; >> > > + } >> > > + >> > > + attrs = sysdb_new_attrs(mem_ctx); >> > > + if (attrs == NULL) { >> > > + ret = ENOMEM; >> > > + goto done; >> > > + } >> > > + >> > > + homedir = expand_homedir_template(mem_ctx, dom->subdomain_homedir, >> > > name, >> > > + uid, NULL, dom->name, >> > > dom->flat_name); >> > > + if (homedir == NULL) { >> > > + DEBUG(SSSDBG_OP_FAILURE, ("expand_homedir_template failed\n")); >> > > + ret = ENOMEM; >> > > + goto done; >> > > + } >> > > + >> > > + ret = sysdb_attrs_add_string(attrs, SYSDB_HOMEDIR, homedir); >> > > + if (ret) { >> > > + DEBUG(SSSDBG_MINOR_FAILURE, ("Error setting homedir: [%s]\n", >> > > + strerror(ret))); >> > > + goto done; >> > > + } >> > > + >> > > + ret = sysdb_transaction_start(sysdb); >> > > + if (ret != EOK) { >> > > + DEBUG(SSSDBG_CRIT_FAILURE, ("Failed to start transaction\n")); >> > > + goto done; >> > > + } >> > > + >> > > + ret = sysdb_set_user_attr(dom, fqname, attrs, SYSDB_MOD_REP); >> > > + if (ret) { >> > > + DEBUG(SSSDBG_CRIT_FAILURE, >> > > + ("Failed to update Login attempt information!\n")); >> > > + } >> > > + >> > > + ret = sysdb_transaction_commit(sysdb); >> > > + if (ret != EOK) { >> > > + DEBUG(SSSDBG_CRIT_FAILURE, >> > > + ("Cannot commit sysdb transaction [%d]: %s\n", >> > > + ret, strerror(ret))); >> > > + goto done; >> > > + } >> > > + >> > > +done: >> > > + talloc_zfree(attrs); >> > > + talloc_zfree(fqname); >> > > + talloc_zfree(homedir); >> > >> > And finally since all three variables are allocated on tmp_ctx, you can >> > just >> > free the context: >> > talloc_free(tmp_ctx); >> > >> > > + return ret; >> > > +} >> > > + >> > > static void >> > > ipa_get_ad_acct_ad_part_done(struct tevent_req *subreq) >> > > { >> > > @@ -358,6 +422,9 @@ ipa_get_ad_acct_ad_part_done(struct tevent_req >> > > *subreq) >> > > struct ipa_get_ad_acct_state *state = tevent_req_data(req, >> > > struct >> > > ipa_get_ad_acct_state); >> > > errno_t ret; >> > > + uint32_t uid; >> > > + char *homedir; >> > > + struct ldb_result *res; >> > > >> > > ret = ad_handle_acct_info_recv(subreq, &state->dp_error, NULL); >> > > talloc_zfree(subreq); >> > > @@ -367,6 +434,28 @@ ipa_get_ad_acct_ad_part_done(struct tevent_req >> > > *subreq) >> > > return; >> > > } >> > > >> > > + ret = sysdb_getpwnam(state, state->user_dom, >> > > state->ar->filter_value, &res); >> > > + if (ret != EOK) { >> > > + DEBUG(SSSDBG_OP_FAILURE, >> > > + ("Failed to make request to our cache: [%d]: [%s]\n", >> > > + ret, sss_strerror(ret))); >> > > + return; >> > > + } >> > >> > I think here ^^^ you also need to branch based on the value of >> > ar->filter_type. The filter_type can also be an ID number, in which case >> > you'd search the user by ID. >> > >> > > + >> > > + homedir = ldb_msg_find_attr_as_string(res->msgs[0], SYSDB_HOMEDIR, >> > > NULL); >> > > + >> > > + if (homedir == NULL) { >> > > + uid = ldb_msg_find_attr_as_uint64(res->msgs[0], SYSDB_UIDNUM, >> > > 0); >> > > + ret = use_subdomain_homedir(state, state->user_dom, >> > > + state->ar->filter_value, uid); >> > > + if (ret != EOK) { >> > > + DEBUG(SSSDBG_OP_FAILURE, >> > > + ("use_subdomain_homedir failed: [%d]: [%s]\n", >> > > + ret, sss_strerror(ret))); >> > > + return; >> > > + } >> > > + } >> > > + >> > > if ((state->ar->entry_type & BE_REQ_TYPE_MASK) != >> > > BE_REQ_INITGROUPS) { >> > > tevent_req_done(req); >> > > return; >> > > -- >> > > 1.8.4.2 >> > > >> Hi Jakub, >> >> new patch addressing your comments is attached. >> >> PR >> > >Looks like you forgot to include some changes? > >/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:393:15: >error: implicit declaration of function 'expand_homedir_template' is invalid >in C99 [-Werror,-Wimplicit-function-declaration] > homedir = expand_homedir_template(tmp_ctx, dom->subdomain_homedir, name, > ^ >/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:393:13: >warning: incompatible integer to pointer conversion assigning to 'char *' from >'int' [-Wint-conversion] > homedir = expand_homedir_template(tmp_ctx, dom->subdomain_homedir, name, > >/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:414:31: >warning: passing 'const struct sss_domain_info *' to parameter of type 'struct >sss_domain_info *' discards qualifiers > [-Wincompatible-pointer-types-discards-qualifiers] > ret = sysdb_set_user_attr(dom, fqname, attrs, SYSDB_MOD_REP); > ^~~ >/home/remote/jhrozek/devel/sssd/src/db/sysdb.h:558:49: note: passing argument >to parameter 'domain' here >int sysdb_set_user_attr(struct sss_domain_info *domain, > ^ >/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:373:9: >warning: variable 'attrs' is uninitialized when used here [-Wuninitialized] > if (attrs == NULL) { > ^~~~~ >/home/remote/jhrozek/devel/sssd/src/providers/ipa/ipa_subdomains_id.c:363:30: >note: initialize the variable 'attrs' to silence this warning > struct sysdb_attrs *attrs; > ^ > = NULL > > It is well know problem with new gcc https://bugzilla.redhat.com/show_bug.cgi?id=986923.
LS _______________________________________________ sssd-devel mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
