Your Tombstone Reanimation branch

[Andrew Bartlett]

On Tue, 2014-12-16 at 04:06 +0200, Kamen Mazdrashki wrote:
> Hi Andrew,
> 
> On Tue, Dec 16, 2014 at 12:47 AM, Andrew Bartlett <abartlet at samba.org>
> wrote:
> >
> > On Tue, 2014-12-16 at 09:25 +1300, Andrew Bartlett wrote:
> > > On Mon, 2014-12-15 at 17:46 +1300, Andrew Bartlett wrote:
> > > > Kamen,
> > > >
> > > > Garming and I did some work to review your tombstone-reanimation-wip
> > > > brnach.  I've pushed it back to
> > > >
> > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/tombstone-reanimation-wip
> > with some TODO markers (metze style).
> > > >
> > >
> > > > We need to go over it again, and to run the tests, but it is an
> > > > impressive lump of work and I look forward to seeing it landed!
> > >
> > > I've run a full make test, and the ldap.secdesc.python test fails.  I
> > > attach the summary file, and the below is the failing subunit.  Can you
> > > look into this please?
> >
> > Kamen,
> >
> > The attached patch fixes the issue.  See the markers in the branch.
> >
> > I'm now re-running a full make test.
> >
> > Nadya,
> >
> > Some of the patches need you to sign off on them again, can you look
> > over:
> >
> >
> > https://git.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/tombstone-reanimation-wip
> >
> >
> Thank you very much for your review and fixing the 'make test' issue!
> I am building a dev machine (kind of) so tomorrow, hopefully, I will be
> able to address your comments.
> 
> As a stretch goal, I will address your concerns for env variable names.
> As you suggested, I will prefix env variables used by tests with TEST_

Garming and I have been looking over the branch again, and between us we
had a few more concerns.

One that both of us were a little bothered by, was this commit:
https://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=dda27fc7dbc69b9203caf57cfa1a7829164e8db5

This is where we have a copy of the code from samldb.  As I think we
discussed much earlier, this really isn't ideal.  My concerns additional
to that are particularly around the userAccountControl -> samAccountType
-> primaryGroupID logic.  This really needs to be kept in one place, in
samldb.

I'm also not really comfortable with:
https://git.samba.org/?p=abartlet/samba.git/.git;a=commitdiff;h=7dfc15b54978ed006a98790db8126d5131dcc3b1

This hunk skips all validation of the primaryGroupID, including if the
group still exists.  Now, the system groups implied by the
userAccountControl should not go away, but I don't like just skipping
checks like this.
-       el = ldb_msg_find_element(ac->msg, "primaryGroupID");
-       if (el != NULL) {
-               ret = samldb_prim_group_trigger(ac);
-               if (ret != LDB_SUCCESS) {
-                       return ret;
+       if (is_undelete == NULL) {
+               el = ldb_msg_find_element(ac->msg, "primaryGroupID");
+               if (el != NULL) {
+                       ret = samldb_prim_group_trigger(ac);
+                       if (ret != LDB_SUCCESS) {
+                               return ret;
+                       }
                }

Instead, it should take the ADD arm of samldb_prim_group_trigger (as
this is what it really looks like to the DB), and at least confirm the
group still exists with samldb_prim_group_set().

Finally, what do you intend to do with _attributes_for_class in your
tombstone_reanimate.py?  It looks like the parts of a test you were
describing early-on with a grand plan for checking each possible
attribute, but it is all un-used right now.  If you want it to be a
utility for schema validation or such, perhaps put it in another file?

Thanks,

Andrew Bartlett

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba