[SSSD] [PATCH] cache_req: support UPN
Jakub Hrozek
jhrozek at redhat.com
Thu Oct 8 18:43:48 UTC 2015
On Thu, Oct 08, 2015 at 08:34:17PM +0200, Jakub Hrozek wrote:
> On Thu, Oct 08, 2015 at 01:21:29PM +0200, Pavel Březina wrote:
> > On 10/07/2015 04:28 PM, Sumit Bose wrote:
> > >On Wed, Oct 07, 2015 at 04:22:31PM +0200, Sumit Bose wrote:
> > >>On Wed, Oct 07, 2015 at 02:42:18PM +0200, Pavel Březina wrote:
> > >>>On 10/06/2015 03:16 PM, Pavel Březina wrote:
> > >>>>On 10/01/2015 05:06 PM, Sumit Bose wrote:
> > >>>>>On Thu, Sep 17, 2015 at 11:06:30AM +0200, Pavel Březina wrote:
> > >>>>>>On 09/17/2015 10:32 AM, Petr Cech wrote:
> > >>>>>>>Hi Pavel!
> > >>>>>>>
> > >>>>>>>There is some code between my last end and this continuation. I was
> > >>>>>>>read
> > >>>>>>>it and did't find anything wrong.
> > >>>>>>
> > >>>>>>Hi Petr,
> > >>>>>>thank you for you review. New patches are attached. All comments from
> > >>>>>>the
> > >>>>>>previous mail should be addressed.
> > >>>>>>
> > >>>>>
> > >>>>>
> > >>>>>[PATCH 2/3] cache_req: add support for UPN
> > >>>>>
> > >>>>>>diff --git a/src/db/sysdb_search.c b/src/db/sysdb_search.c
> > >>>>>>index
> > >>>>>>5f33b225a31e1c1add7f776215d500fb09e127ca..a72a8bac594dc10ff9aa337399f27d9936830a8e
> > >>>>>>100644
> > >>>>>>--- a/src/db/sysdb_search.c
> > >>>>>>+++ b/src/db/sysdb_search.c
> > >>>>>>@@ -295,6 +295,59 @@ static char *enum_filter(TALLOC_CTX *mem_ctx,
> > >>>>>> return filter;
> > >>>>>> }
> > >>>>>>
> > >>>>>>+int sysdb_getpwupn(TALLOC_CTX *mem_ctx,
> > >>>>>>+ struct sss_domain_info *domain,
> > >>>>>>+ const char *upn,
> > >>>>>>+ struct ldb_result **_res)
> > >>>>>>+{
> > >>>>>>+ TALLOC_CTX *tmp_ctx;
> > >>>>>>+ struct ldb_message *msg;
> > >>>>>>+ struct ldb_result *res;
> > >>>>>>+ static const char *attrs[] = SYSDB_PW_ATTRS;
> > >>>>>>+ errno_t ret;
> > >>>>>>+
> > >>>>>>+ tmp_ctx = talloc_new(NULL);
> > >>>>>>+ if (tmp_ctx == NULL) {
> > >>>>>>+ DEBUG(SSSDBG_CRIT_FAILURE, "talloc_new() failed\n");
> > >>>>>>+ return ENOMEM;
> > >>>>>>+ }
> > >>>>>>+
> > >>>>>>+ ret = sysdb_search_user_by_upn(tmp_ctx, domain, upn, attrs, &msg);
> > >>>>>>+ if (ret != EOK && ret != ENOENT) {
> > >>>>>>+ DEBUG(SSSDBG_OP_FAILURE, "sysdb_search_user_by_upn()
> > >>>>>>failed.\n");
> > >>>>>>+ goto done;
> > >>>>>>+ }
> > >>>>>
> > >>>>>The call stack here would be
> > >>>>>
> > >>>>>sysdb_getpwupn()->sysdb_search_user_by_upn()->sysdb_search_entry()->ldb_search()
> > >>>>>
> > >>>>>
> > >>>>>and the returned results, starting from ldb_search()
> > >>>>>
> > >>>>>ldb_result -> ldb_message (array) -> ldb_message -> ldb_result
> > >>>>>
> > >>>>>I think it would be easier and safe some allocations if it would be
> > >>>>>
> > >>>>>sysdb_search_user_by_upn()->sysdb_getpwupn()->ldb_search() .
> > >>>>>
> > >>>>>Do you think this change makes sense?
> > >>>>
> > >>>>Hi, thanks for the review. Done.
> > >>>>
> > >>>>Although I created a new function sysdb_search_user_by_upn_res since you
> > >>>>can't pass attrs into sysdb_getpwnupn (unless we are inconsistent with
> > >>>>api).
> > >>>>
> > >>>>>
> > >>>> const char *orig_name;
> > >>>>>
> > >>>>>Instead of adding a new member which again will hold the originally
> > >>>>>provided name I would suggest to refactor the change added by
> > >>>>>e87b2a6e94c1066b3044fe683825ff5b4f8716c2 (cache_req: parse input name if
> > >>>>>needed) by introducing a member called e.g. parsed_name which keeps the
> > >>>>>name returned by sss_parse_inp_recv() and return it in cache_req_recv().
> > >>>>>
> > >>>>>In e87b2a6e94c1066b3044fe683825ff5b4f8716c2 it made sense to overwrite
> > >>>>>orig_name because no additional member was needed. But now I think the
> > >>>>>code would be more clear if orig_name will be kept unmodified.
> > >>>>
> > >>>>Done in separate patch so the changes are more visible. It can be
> > >>>>squashed to the second patch.
> > >>>
> > >>>Sumit found that test won't pass. New patchset is attached.
> > >>>
> > >>>
> > >>
> > >>Thank you, the tests pass now. I've seen a compiler warning which is
> > >>seen with the first patch I attached.
> > >>
> > >>During testing I found that UPN lookups for sub-domain users do not
> > >>work even with the original nss responder code. I attached fixes for
> > >>your patches and the nss responder since the sysdb change depends on you
> > >>patches.
> > >
> > >and now with patches ...
> >
> > Ack. Thank you!
>
> I pushed both sets to master:
> * 8ded8b2f4a57d1833fd230307218d8b07a571785
> * 374268c5eda35e8bbc2fef30752299199439cffe
> * 391b81f2a78a812a87530e0c50c70d59150f49eb
> * 2fce47f2dadd10d2a2c8bf9f03ab7094bc6c6b3a
> * 3688374991afb34bbaf2b7843683fc13dd77879d
> * 28ebfa4373d1e7ce45b5d70a3619df1c074a661e
> * d8125f0e0d38c6939887a0849a44859d6c498c57
> and sssd-1-13:
> * b1b0abe6223348083be552531f195518b0df3ba5
> * f2c3994c6ffd6e715621d7d7ee580db3cd3e85a9
> * 055248cf20c1ad9dfba06903789e71b88752b810
> * 6bb2a01333bca8a338cb0be6e8d513040b227cb8
> * 44415c5a8ad054555a5e0cc016eec8a125c927ab
> * f04ead531a4e941de1a5bcb695326ca11a3dd145
> * ae3896a12fb4818aac11ccc5d3a57d2cdf61cd43
>
> I hope it's OK to push these all to sssd-1-13 since Sumit's patches
> depend on Pavel's and in 1-13 the cache_req is mostly used for UPN.
~~~~~~~~
For IFP
More information about the sssd-devel
mailing list