[PATCH] samba-tool: make 'samba-tool user create' work like ADUC

Alexander Bokovoy ab at samba.org
Thu Jun 25 05:35:23 MDT 2015


On Thu, Jun 25, 2015 at 11:29:45AM +0100, Rowland Penny wrote:
> >>to use, what is its gidNumber and they finally type  ' --gid-number=10000'
> >>on the end. They then press 'Enter' and create the user, they then go back
> >>and update their records with the uidNumber they just used.
> >>
> >>Would you like to explain how this is different from the way my patch works
> >>?
> >You patch ignores ID allocation schemas that are configured on Samba AD
> >DC with Winbindd, completely. We need to have a single place where logic
> >to allocate IDs is done and that is winbindd. Do not add other logic,
> >please, use the one that is there already.
> 
> Taking a member server as an example, windbind can be setup to work in at
> least two ways, the 'ad' and 'rid' back ends. The 'rid' backend maps the
> users AD RID to a number, this number should be the same on all member
> servers, but not on a DC (at the moment). The 'ad' backend has nothing to do
> with winbind apart from the fact winbind extracts the info from AD.
> I am *not* adding logic, I am just using what is already there, but in a
> different way. If you come right down to it, the two main ways of using
> windbind do not allocate IDs, they are either mapped or extracted.
You are increasing number of code paths that have logic to create IDs.
We have that logic managed in idmap backens already so please use that.
If backend returns you (uint32_t)-1 value, it means SID is not mapped or
could not be mapped automatically so you would be able to tell that to
the admin. However, incrementing on your own represents just one
specific way of allocating the ID. I would rather ask the admin to
confirm it.

Additionally, Samba AD DC has idmap module that allocates
uidNumber/gidNumber, see idmap_sid_to_xid() in source4/winbind/idmap.c.
At the very least you want to be compatible with it.

Also, the code
+        idmax = 0
+        res = self.search(domain_dn,
+                          scope=ldb.SCOPE_SUBTREE,
+                          expression=idexpr,
+                          attrs=[idattr])
+        for msg in res:
+            if idattr in msg:
+                n = str("%s" % msg[idattr])
+                idn = int(n)
+                if idn > idmax:
+                    idmax = idn

is wrong -- it will pull either whole user database or just a page (1000
entries or so) depending on how samdb is setup. 

> 
> >
> >>>This is what I meant by a heritage that we would need to deal with
> >>>later.
> >>>
> >>>Also, I don't like the mess that is created by combining formatting
> >>>changes with functional ones. It is going to beat us in future when any
> >>>specific bug arises and we'll be doing searches for the offending
> >>>commit. The way we deal with it is that formatting commits are separate
> >>>from functional changes.
> >>
> >>Could you explain what you mean by 'formatting' and 'functional' , I think I
> >>understand the later, but do not have a clue about the former.
> >More than half of your patch is reformatting existing code rather than
> >adding new feature. Split that into a separate patch, rather than
> >combining them together.
> 
> OK, I will try and split the patch up, but quite a lot of what I altered was
> because it wouldn't work unless I did alter it i.e one depends on the other.
If you need to reformat, do it first, then add your changes to the
formatted code, then do nisadd part. There is no requirement that your
changes have to be in the single patch.

-- 
/ Alexander Bokovoy


More information about the samba-technical mailing list