[PATCH] passdb: handle UPN in lookup_name correctly

[Andreas Schneider]

On Tuesday, February 12, 2019 1:38:09 PM CET Ralph Wuerthner via samba-
technical wrote:
> Hi Andreas!
> 
> On 12.02.19 10:39, Andreas Schneider wrote:
> > On Monday, February 11, 2019 5:39:33 PM CET Ralph Wuerthner wrote:
> >> Hi Andreas!
> >> 
> >> On 11.02.19 16:07, Andreas Schneider wrote:
> >>> Hi Ralph,
> >>> 
> >>>> Please see attached patchset:
> >>>> The fix for Samba bugzilla 13312 (commit 1775ac8aa4) caused a
> >>>> regression
> >>>> when looking up names in UPN notation: Because winbind_lookup_name is
> >>>> called with lp_workgroup as domain name the lookup is now failing and
> >>>> the SID for an unmapped Unix user is returned by lookup_name. Fixed by
> >>>> calling winbind_lookup_name with an empty domain name in case the name
> >>>> is in UPN notation.
> >>>> 
> >>>> The patchset already passed a CI run:
> >>>> https://gitlab.com/samba-team/devel/samba/pipelines/46689980
> >>> 
> >>> Thanks for your contribution!
> >>> 
> >>> Please use DGB_DEBUG() instead of DEBUG(10, ...)
> >>> 
> >>> In lookup_upn() use a helper variable 'bool ok'. And check
> >>> talloc_strdup()
> >>> for NULL.
> >> 
> >> Thanks for your feedback! I prepared a new version of the patchset with
> >> the following changes:
> >> - using a helper variable in lookup_upn()
> >> - use DBG_DEBUG() instead of DEBUG(10, ...)
> >> 
> >> I didn't add a NULL check for talloc_strdup() because there is already a
> >> NULL check right after the ok: label. This check is used by other
> >> sequence steps in lookup_name() too.
> > 
> > But then the function would need documentation that the caller is
> > responsible for the NULL check.
> > 
> > I think this is strange code and we should do the check in place and not
> > defer it to later.
> 
> Ahh, I got your point. I thought you were referring to the
> talloc_strdup() call from patch #2 in lookup_name(), instead you are
> referring to the talloc_strdup() call in lookup_upn(). You are right,
> there should be a check in lookup_upn(). I didn't pay much attention to
> this because patch #3 is elimination the talloc_strdup() call in
> lookup_upn(). Please see the attached patchset with the missing check
> (though it will be gone after applying patch #3).

This looks good. Now we need tests which verify this :-)



	Andreas


-- 
Andreas Schneider                      asn at samba.org
Samba Team                             www.samba.org
GPG-ID:     8DFF53E18F2ABC8D8F3C92237EE0FC4DCC014E3D