[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