[PATCH] Allow editing groups from samba-tool

[Alexander Bokovoy]

On ma, 30 huhti 2018, Rowland Penny wrote:
> On Mon, 30 Apr 2018 08:32:14 +0300
> Alexander Bokovoy via samba-technical <samba-technical at lists.samba.org>
> wrote:
> 
> See inline comments.
> 
> > On ma, 30 huhti 2018, William Brown via samba-technical wrote:
> > > Hi there,
> > > 
> > > samba tool allows a command like "user edit" that opens the object
> > > as an ldif, and it can be edited and commited.
> > > 
> > > This modifies the location of that implementation to be a generic
> > > EditCommand that can be derived, and demonstrates how it could be
> > > implemented for group also.
> > > 
> > > One minor detail is how we pass in the "type". Should this be a
> > > "self.attribute" of the EditCommand rather than an argument to
> > > run_edit? filter makes sense to be an argument due to unknown string
> > > formatting for the filter, similar with the samaccount name being an
> > > argument for error messages. But the type could be a self attribute.
> > I think passing filter is more flexible here.
> > 
> > There is one comment I'd like to address but otherwise it looks good.
> > See inline.
> > 
> > > 
> > > Thanks for your review!
> > > 
> > > William
> > 
> > > From 9961efc5f599d043479d2376c6d96a378302f57f Mon Sep 17 00:00:00
> > > 2001 From: William Brown <william at blackhats.net.au>
> > > Date: Mon, 30 Apr 2018 14:28:31 +1200
> > > Subject: [PATCH] python/samba/netcmd/{__init__.py,user.py,group.py}
> > > improve edit command.
> > > 
> > > samba-tool allows editing an ldif of objects which can be then
> > > commited back to the directory server. This makes an EditCommand
> > > type which can be reused, and implemnets the correct values for
> > > group to allow:
> > > 
> > > samba-tool group edit ...
> > > samba-tool user edit ...
> > > 
> > > Signed-off-by: William Brown <william at blackhats.net.au>
> > > ---
> > >  python/samba/netcmd/__init__.py | 108
> > > ++++++++++++++++++++++++++++++++++++++++
> > > python/samba/netcmd/group.py    |  64 +++++++++++++++++++++++-
> > > python/samba/netcmd/user.py     |  87
> > > +++----------------------------- 3 files changed, 179
> > > insertions(+), 80 deletions(-)
> > > 
> > > diff --git a/python/samba/netcmd/__init__.py
> > > b/python/samba/netcmd/__init__.py index 77976780150..65cfbcfc035
> > > 100644 --- a/python/samba/netcmd/__init__.py
> > > +++ b/python/samba/netcmd/__init__.py
> > > @@ -22,6 +22,16 @@ from ldb import LdbError
> > >  import sys, traceback
> > >  import textwrap
> > >  
> > > +# For editCommand
> > > +import re
> > > +import os
> > > +import ldb
> > > +import difflib
> > > +import tempfile
> > > +from samba.samdb import SamDB
> > > +from samba.auth import system_session
> > > +from subprocess import Popen, PIPE, STDOUT, check_call,
> > > CalledProcessError +
> > >  class Option(optparse.Option):
> > >      pass
> > >  
> > > @@ -190,6 +200,104 @@ class Command(object):
> > >          return logger
> > >  
> > >  
> > > +class EditCommand(Command):
> > > +    """A specialise variant of Command for DS object editing."""
> > > +
> > > +    def _run_edit(self, filt, objectname, objecttype,
> > > credopts=None,
> > > +            sambaopts=None, versionopts=None, H=None, editor=None):
> > > +
> > > +        lp = sambaopts.get_loadparm()
> > > +        creds = credopts.get_credentials(lp, fallback_machine=True)
> > > +        samdb = SamDB(url=H, session_info=system_session(),
> > > +                      credentials=creds, lp=lp)
> > > +
> > > +        domaindn = samdb.domain_dn()
> > > +
> > > +        try:
> > > +            res = samdb.search(base=domaindn,
> > > +                               expression=filt,
> > > +                               scope=ldb.SCOPE_SUBTREE)
> > > +            object_dn = res[0].dn
> > > +        except IndexError:
> > > +            raise CommandError('Unable to find %s "%s"' %
> > > +                                  (objecttype.lower(), objectname))
> > > +
> > > +        for msg in res:
> > > +            r_ldif = samdb.write_ldif(msg, 1)
> > > +            # remove 'changetype' line
> > > +            result_ldif = re.sub('changetype: add\n', '', r_ldif)
> > > +            # Remove line wrapping where it exists.
> > > +            # This fixes an issue when you do the diff that
> > > doesn't know how
> > > +            # to handle a multi-line value - or add one ....
> > > +            # In some cases *true* multiline values may need b64
> > > encoding to
> > > +            # preserve the newlines.
> > > +            result_ldif = re.sub('\n ', '', result_ldif)
> > > +
> > > +            if editor is None:
> > > +                editor = os.environ.get('EDITOR')
> > > +                if editor is None:
> > > +                    editor = 'vi'
> > > +
> > > +            # Assert the editor chosen actually exists!
> > > +            if os.path.exists(editor) is False:
> > > +                raise CommandError('Unable to access "%s". Does it
> > > exist?'
> > > +                                   % editor)
> > I know this was partially copied from the existing code but I'd rather
> > pull the editor checks out of the loop and do this first thing in the
> > +run_edit() so that we don't even try opening SamDB and search there
> > for an LDAP record if editor is missing. Original code didn't do 
> > 'raise CommandError' because it assumed 'vi' always exists on the
> > system but if you are adding a terminal path here, better to move it
> > outside of the loop.
> 
> Then why didn't you raise this in the first place ? I would have fixed
> it then.
The original code did not raise an exception, so it was fine as it
backed to an asumingly always existing 'vi'. If we add a hard exception
if an editor does not exist, we'd rather move the code out of a loop
as the code is testing a fact that doesn't change within a loop and can
avoid even reading the data in the first place.

BTW, 'vi' would not pass os.path.exists() check, so if EDITOR is not
set, then the proposed patch would always raise an exception. Original
code relied on a system()'s use of a shell PATH to search the 'vi' or
any other non-fully specified editor binary.

> > > -Example1 shows how to edit a users attributes in the domain
> > > against a remote +Example1 shows how to edit a user's attributes in
> > > the domain against a remote LDAP server.
> 
> Please don't add even more greengrocers apostrophes to Samba, it is
> <users> not <users's>
It is a single user's record to be edited, not multiple users at the
same time. And attributes belong to that record (so to the user, thus
"user's"). I know, I'm venturing into a territory a non-native speaker
would probably need to avoid but that's how I understand the help above
and that's how the command is operating. To me using "users" would be
totally against the meaning of the operation here.


-- 
/ Alexander Bokovoy