[PATCH] samba-tool: Easily edit a users object in AD

classic Classic list List threaded Threaded
37 messages Options
12
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

[PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list

Hi, the attached patch is something I created to use instead of running
ldbedit when I just wanted to alter one of a users attributes.

Rowland

samba-tool-Easily-edit-a-users-object-in-AD.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
hi Rowland,

On 24/06/17 02:27, Rowland Penny via samba-technical wrote:
> Hi, the attached patch is something I created to use instead of running
> ldbedit when I just wanted to alter one of a users attributes.

Nice documentation, and it seems to avoid some ldbedit pitfalls (e.g.
--show-deleted), but I see a few issues.

1: tests.

2..n: interspersed below.

>
> Rowland
>
>
> samba-tool-Easily-edit-a-users-object-in-AD.patch
>
>
> From 540f2bced8c8a6172fdef6aea6d74ef6584c4455 Mon Sep 17 00:00:00 2001
> From: Rowland Penny <[hidden email]>
> Date: Fri, 23 Jun 2017 14:27:54 +0100
> Subject: [PATCH] samba-tool: Easily edit a users object in AD, as if using
>  ldbedit.
>
> Signed-off-by: Rowland Penny <[hidden email]>
> ---
>  python/samba/netcmd/user.py | 137 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 136 insertions(+), 1 deletion(-)
>
> diff --git a/python/samba/netcmd/user.py b/python/samba/netcmd/user.py
> index 53ac39f..6b12dde 100644
> --- a/python/samba/netcmd/user.py
> +++ b/python/samba/netcmd/user.py
> @@ -21,6 +21,9 @@ import samba.getopt as options
>  import ldb
>  import pwd
>  import os
> +import re
> +import tempfile
> +import difflib
>  import sys
>  import fcntl
>  import signal
> @@ -28,7 +31,7 @@ import errno
>  import time
>  import base64
>  import binascii
> -from subprocess import Popen, PIPE, STDOUT
> +from subprocess import Popen, PIPE, STDOUT, call, CalledProcessError

You want check_call, not call if you're expecting a CalledProcessError.
call returns the status code.

>  from getpass import getpass
>  from samba.auth import system_session
>  from samba.samdb import SamDB
> @@ -2283,6 +2286,137 @@ samba-tool user syncpasswords --terminate \\
>          update_pid(None)
>          return
>  
> +class cmd_user_edit(Command):
> +    """Modify User AD object.
> +
> +This command will allow editing of a user account in the Active Directory
> +domain. You will then be able to add or change attributes and their values.
> +
> +The username specified on the command is the sAMAccountName.
> +
> +The command may be run from the root userid or another authorized userid.
> +
> +The -H or --URL= option can be used to execute the command against a remote
> +server.
> +
> +Example1:
> +samba-tool user edit User1 -H ldap://samba.samdom.example.com \
> +-U administrator --password=passw1rd
> +
> +Example1 shows how to edit a users attributes in the domain against a remote
> +LDAP server.
> +
> +The -H parameter is used to specify the remote target server.
> +
> +Example2:
> +samba-tool user edit User2
> +
> +Example2 shows how to edit a users attributes in the domain against a local
> +LDAP server.
> +
> +Example3:
> +samba-tool user edit User3 --editor=nano
> +
> +Example3 shows how to edit a users attributes in the domain against a local
> +LDAP server using the 'nano' editor.
> +
> +"""
> +    synopsis = "%prog <username> [options]"
> +
> +    takes_options = [
> +        Option("-H", "--URL", help="LDB URL for database or target server",
> +               type=str, metavar="URL", dest="H"),
> +        Option("--editor", help="Editor to use instead of the system default,"
> +               " or 'vi' if no system default is set.", type=str),
> +    ]
> +
> +    takes_args = ["username"]
> +    takes_optiongroups = {
> +        "sambaopts": options.SambaOptions,
> +        "credopts": options.CredentialsOptions,
> +        "versionopts": options.VersionOptions,
> +        }
> +
> +    def run(self, username, 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)
> +
> +        filter = ("(&(sAMAccountType=805306368)(sAMAccountName=%s))" %
                                        ^^^
It would be easier to read with

    "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...

> +                  (ldb.binary_encode(username)))
> +
> +        domaindn = samdb.domain_dn()
> +
> +        try:
> +            res = samdb.search(base=domaindn,
> +                               expression=filter,
> +                               scope=ldb.SCOPE_SUBTREE)
> +            user_dn = res[0].dn
> +        except IndexError:
> +            raise CommandError('Unable to find user "%s"' % (username))
> +
> +        for msg in res:
> +            r_ldif = samdb.write_ldif(msg, 1)
> +            # remove 'changetype' line
> +            result_ldif = re.sub('changetype: add\n', '', r_ldif)
> +
> +            if editor is None:
> +                editor = os.environ.get('EDITOR')
> +                if editor is None:
> +                    editor = 'vi'
> +
> +            with tempfile.NamedTemporaryFile(suffix=".tmp") as t_file:
> +                t_file.write(result_ldif)
> +                t_file.flush()
> +                try:
> +                    call([editor, t_file.name])
> +                except CalledProcessError, e:
> +                    raise IOError("ERROR: ", e)

Is there are reason to prefer IOError over CalledProcessError?

> +                with open(t_file.name) as edited_file:
> +                    edited_message = edited_file.read()
> +
> +        if result_ldif != edited_message:
> +            diff = difflib.ndiff(result_ldif.splitlines(),
> +                                 edited_message.splitlines())
> +            minus_lines = []
> +            plus_lines = []
> +            for line in diff:
> +                if line.startswith('-'):
> +                    line = line[2:]
> +                    minus_lines.append(line)
> +                elif line.startswith('+'):
> +                    line = line[2:]
> +                    plus_lines.append(line)
> +
> +            user_ldif="dn: %s\n" % user_dn
> +            user_ldif += "changetype: modify\n"
> +
> +            for line in minus_lines:
> +                attr, val = line.split(':')

Here you want line.split(':', 1), or you'll get an error when a value
contains a colon (like "url: http://example.com").

> +                if attr not in str(plus_lines):
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^

This is NOT going to work well, because it matches whenever a value
happens to contain the attribute's name. Contriving an example:

cn: foo
- street: 17 Picnic street.
+ streetAddress: 17 Picnic Street

`str(plus_lines)` contains "street" (and incidentally "cn"), resulting
in weird changes.

What you need to do is something more like:

minus_attrs = set()
for line in diff:
    op = line[:2]
    attr, value = line[2:].split(':', 1)
    if change == '- ':
        minus_attrs.add(attr)
    ...

    if attr not in minus_attrs:


> +                    user_ldif += "delete: %s\n" % attr
> +                    user_ldif += "%s: %s\n" % (attr, val)
> +
> +            for line in plus_lines:
> +                attr, val = line.split(':')
> +                if attr in str(minus_lines):
> +                    user_ldif += "replace: %s\n" % attr
> +                    user_ldif += "%s: %s\n" % (attr, val)
> +                if attr not in str(minus_lines):
> +                    user_ldif += "add: %s\n" % attr
> +                    user_ldif += "%s: %s\n" % (attr, val)
> +
> +            try:
> +                samdb.modify_ldif(user_ldif)
> +            except Exception, e:
> +                raise CommandError("Failed to modify user '%s': " %
> +                                   username, e)
> +
   ^^^^^^^^
Git tells me there's loose whitespace here.

> +            self.outf.write("Modified User '%s' successfully\n" % username)
> +
>  class cmd_user(SuperCommand):
>      """User management."""
>  
> @@ -2298,3 +2432,4 @@ class cmd_user(SuperCommand):
>      subcommands["setpassword"] = cmd_user_setpassword()
>      subcommands["getpassword"] = cmd_user_getpassword()
>      subcommands["syncpasswords"] = cmd_user_syncpasswords()
> +    subcommands["edit"] = cmd_user_edit()
> -- 2.1.4
>

cheers,
Douglas

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Tue, 27 Jun 2017 14:35:42 +1200
Douglas Bagnall <[hidden email]> wrote:

> hi Rowland,
>
> On 24/06/17 02:27, Rowland Penny via samba-technical wrote:
> > Hi, the attached patch is something I created to use instead of
> > running ldbedit when I just wanted to alter one of a users
> > attributes.
>
> Nice documentation, and it seems to avoid some ldbedit pitfalls (e.g.
> --show-deleted), but I see a few issues.
>
> 1: tests.
>
> 2..n: interspersed below.
> >
> > Rowland
> >
> >
> > samba-tool-Easily-edit-a-users-object-in-AD.patch
> >
> >
> > From 540f2bced8c8a6172fdef6aea6d74ef6584c4455 Mon Sep 17 00:00:00
> > 2001 From: Rowland Penny <[hidden email]>
> > Date: Fri, 23 Jun 2017 14:27:54 +0100
> > Subject: [PATCH] samba-tool: Easily edit a users object in AD, as
> > if using ldbedit.
> >
> > Signed-off-by: Rowland Penny <[hidden email]>
> > ---
> >  python/samba/netcmd/user.py | 137
> > +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 136
> > insertions(+), 1 deletion(-)
> >
> > diff --git a/python/samba/netcmd/user.py
> > b/python/samba/netcmd/user.py index 53ac39f..6b12dde 100644
> > --- a/python/samba/netcmd/user.py
> > +++ b/python/samba/netcmd/user.py
> > @@ -21,6 +21,9 @@ import samba.getopt as options
> >  import ldb
> >  import pwd
> >  import os
> > +import re
> > +import tempfile
> > +import difflib
> >  import sys
> >  import fcntl
> >  import signal
> > @@ -28,7 +31,7 @@ import errno
> >  import time
> >  import base64
> >  import binascii
> > -from subprocess import Popen, PIPE, STDOUT
> > +from subprocess import Popen, PIPE, STDOUT, call,
> > CalledProcessError
>
> You want check_call, not call if you're expecting a
> CalledProcessError. call returns the status code.
>
> >  from getpass import getpass
> >  from samba.auth import system_session
> >  from samba.samdb import SamDB
> > @@ -2283,6 +2286,137 @@ samba-tool user syncpasswords --terminate \\
> >          update_pid(None)
> >          return
> >  
> > +class cmd_user_edit(Command):
> > +    """Modify User AD object.
> > +
> > +This command will allow editing of a user account in the Active
> > Directory +domain. You will then be able to add or change
> > attributes and their values. +
> > +The username specified on the command is the sAMAccountName.
> > +
> > +The command may be run from the root userid or another authorized
> > userid. +
> > +The -H or --URL= option can be used to execute the command against
> > a remote +server.
> > +
> > +Example1:
> > +samba-tool user edit User1 -H ldap://samba.samdom.example.com \
> > +-U administrator --password=passw1rd
> > +
> > +Example1 shows how to edit a users attributes in the domain
> > against a remote +LDAP server.
> > +
> > +The -H parameter is used to specify the remote target server.
> > +
> > +Example2:
> > +samba-tool user edit User2
> > +
> > +Example2 shows how to edit a users attributes in the domain
> > against a local +LDAP server.
> > +
> > +Example3:
> > +samba-tool user edit User3 --editor=nano
> > +
> > +Example3 shows how to edit a users attributes in the domain
> > against a local +LDAP server using the 'nano' editor.
> > +
> > +"""
> > +    synopsis = "%prog <username> [options]"
> > +
> > +    takes_options = [
> > +        Option("-H", "--URL", help="LDB URL for database or target
> > server",
> > +               type=str, metavar="URL", dest="H"),
> > +        Option("--editor", help="Editor to use instead of the
> > system default,"
> > +               " or 'vi' if no system default is set.", type=str),
> > +    ]
> > +
> > +    takes_args = ["username"]
> > +    takes_optiongroups = {
> > +        "sambaopts": options.SambaOptions,
> > +        "credopts": options.CredentialsOptions,
> > +        "versionopts": options.VersionOptions,
> > +        }
> > +
> > +    def run(self, username, 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)
> > +
> > +        filter =
> > ("(&(sAMAccountType=805306368)(sAMAccountName=%s))" %
>                                         ^^^
> It would be easier to read with
>
>     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
>
> > +                  (ldb.binary_encode(username)))
> > +
> > +        domaindn = samdb.domain_dn()
> > +
> > +        try:
> > +            res = samdb.search(base=domaindn,
> > +                               expression=filter,
> > +                               scope=ldb.SCOPE_SUBTREE)
> > +            user_dn = res[0].dn
> > +        except IndexError:
> > +            raise CommandError('Unable to find user "%s"' %
> > (username)) +
> > +        for msg in res:
> > +            r_ldif = samdb.write_ldif(msg, 1)
> > +            # remove 'changetype' line
> > +            result_ldif = re.sub('changetype: add\n', '', r_ldif)
> > +
> > +            if editor is None:
> > +                editor = os.environ.get('EDITOR')
> > +                if editor is None:
> > +                    editor = 'vi'
> > +
> > +            with tempfile.NamedTemporaryFile(suffix=".tmp") as
> > t_file:
> > +                t_file.write(result_ldif)
> > +                t_file.flush()
> > +                try:
> > +                    call([editor, t_file.name])
> > +                except CalledProcessError, e:
> > +                    raise IOError("ERROR: ", e)
>
> Is there are reason to prefer IOError over CalledProcessError?
>
> > +                with open(t_file.name) as edited_file:
> > +                    edited_message = edited_file.read()
> > +
> > +        if result_ldif != edited_message:
> > +            diff = difflib.ndiff(result_ldif.splitlines(),
> > +                                 edited_message.splitlines())
> > +            minus_lines = []
> > +            plus_lines = []
> > +            for line in diff:
> > +                if line.startswith('-'):
> > +                    line = line[2:]
> > +                    minus_lines.append(line)
> > +                elif line.startswith('+'):
> > +                    line = line[2:]
> > +                    plus_lines.append(line)
> > +
> > +            user_ldif="dn: %s\n" % user_dn
> > +            user_ldif += "changetype: modify\n"
> > +
> > +            for line in minus_lines:
> > +                attr, val = line.split(':')
>
> Here you want line.split(':', 1), or you'll get an error when a value
> contains a colon (like "url: http://example.com").
>
> > +                if attr not in str(plus_lines):
>                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> This is NOT going to work well, because it matches whenever a value
> happens to contain the attribute's name. Contriving an example:
>
> cn: foo
> - street: 17 Picnic street.
> + streetAddress: 17 Picnic Street
>
> `str(plus_lines)` contains "street" (and incidentally "cn"), resulting
> in weird changes.
>
> What you need to do is something more like:
>
> minus_attrs = set()
> for line in diff:
>     op = line[:2]
>     attr, value = line[2:].split(':', 1)
>     if change == '- ':
>         minus_attrs.add(attr)
>     ...
>
>     if attr not in minus_attrs:
>
>
> > +                    user_ldif += "delete: %s\n" % attr
> > +                    user_ldif += "%s: %s\n" % (attr, val)
> > +
> > +            for line in plus_lines:
> > +                attr, val = line.split(':')
> > +                if attr in str(minus_lines):
> > +                    user_ldif += "replace: %s\n" % attr
> > +                    user_ldif += "%s: %s\n" % (attr, val)
> > +                if attr not in str(minus_lines):
> > +                    user_ldif += "add: %s\n" % attr
> > +                    user_ldif += "%s: %s\n" % (attr, val)
> > +
> > +            try:
> > +                samdb.modify_ldif(user_ldif)
> > +            except Exception, e:
> > +                raise CommandError("Failed to modify user '%s': " %
> > +                                   username, e)
> > +
>    ^^^^^^^^
> Git tells me there's loose whitespace here.
>
> > +            self.outf.write("Modified User '%s' successfully\n" %
> > username) +
> >  class cmd_user(SuperCommand):
> >      """User management."""
> >  
> > @@ -2298,3 +2432,4 @@ class cmd_user(SuperCommand):
> >      subcommands["setpassword"] = cmd_user_setpassword()
> >      subcommands["getpassword"] = cmd_user_getpassword()
> >      subcommands["syncpasswords"] = cmd_user_syncpasswords()
> > +    subcommands["edit"] = cmd_user_edit()
> > -- 2.1.4
> >
>
> cheers,
> Douglas

Hi Douglas, thanks for looking at my patch and I take on board most of
your comments and will work on the patch. The only comment I have a
problem with is this one:

It would be easier to read with

    "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...

'pydoc samba.dsdb' shows this:

    UF_NORMAL_ACCOUNT = 512

This means that the filter will become:

sAMAccountType=512

Don't think that is a valid number for sAMAccountType, it is for
userAccountControl though.

My way seems to be the accepted filter to just get a user.

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:

> On Tue, 27 Jun 2017 14:35:42 +1200
> Douglas Bagnall <[hidden email]> wrote:
>
> > hi Rowland,
> >
> > On 24/06/17 02:27, Rowland Penny via samba-technical wrote:
> > > Hi, the attached patch is something I created to use instead of
> > > running ldbedit when I just wanted to alter one of a users
> > > attributes.
> >
> > Nice documentation, and it seems to avoid some ldbedit pitfalls (e.g.
> > --show-deleted), but I see a few issues.
> >
> > 1: tests.
> >
> > 2..n: interspersed below.
> > >
> > > Rowland
> > >
> > >
> > > samba-tool-Easily-edit-a-users-object-in-AD.patch
> > >
> > >
> > > From 540f2bced8c8a6172fdef6aea6d74ef6584c4455 Mon Sep 17 00:00:00
> > > 2001 From: Rowland Penny <[hidden email]>
> > > Date: Fri, 23 Jun 2017 14:27:54 +0100
> > > Subject: [PATCH] samba-tool: Easily edit a users object in AD, as
> > > if using ldbedit.
> > >
> > > Signed-off-by: Rowland Penny <[hidden email]>
> > > ---
> > >  python/samba/netcmd/user.py | 137
> > > +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 136
> > > insertions(+), 1 deletion(-)
> > >
> > > diff --git a/python/samba/netcmd/user.py
> > > b/python/samba/netcmd/user.py index 53ac39f..6b12dde 100644
> > > --- a/python/samba/netcmd/user.py
> > > +++ b/python/samba/netcmd/user.py
> > > @@ -21,6 +21,9 @@ import samba.getopt as options
> > >  import ldb
> > >  import pwd
> > >  import os
> > > +import re
> > > +import tempfile
> > > +import difflib
> > >  import sys
> > >  import fcntl
> > >  import signal
> > > @@ -28,7 +31,7 @@ import errno
> > >  import time
> > >  import base64
> > >  import binascii
> > > -from subprocess import Popen, PIPE, STDOUT
> > > +from subprocess import Popen, PIPE, STDOUT, call,
> > > CalledProcessError
> >
> > You want check_call, not call if you're expecting a
> > CalledProcessError. call returns the status code.
> >
> > >  from getpass import getpass
> > >  from samba.auth import system_session
> > >  from samba.samdb import SamDB
> > > @@ -2283,6 +2286,137 @@ samba-tool user syncpasswords --terminate \\
> > >          update_pid(None)
> > >          return
> > >  
> > > +class cmd_user_edit(Command):
> > > +    """Modify User AD object.
> > > +
> > > +This command will allow editing of a user account in the Active
> > > Directory +domain. You will then be able to add or change
> > > attributes and their values. +
> > > +The username specified on the command is the sAMAccountName.
> > > +
> > > +The command may be run from the root userid or another authorized
> > > userid. +
> > > +The -H or --URL= option can be used to execute the command against
> > > a remote +server.
> > > +
> > > +Example1:
> > > +samba-tool user edit User1 -H ldap://samba.samdom.example.com \
> > > +-U administrator --password=passw1rd
> > > +
> > > +Example1 shows how to edit a users attributes in the domain
> > > against a remote +LDAP server.
> > > +
> > > +The -H parameter is used to specify the remote target server.
> > > +
> > > +Example2:
> > > +samba-tool user edit User2
> > > +
> > > +Example2 shows how to edit a users attributes in the domain
> > > against a local +LDAP server.
> > > +
> > > +Example3:
> > > +samba-tool user edit User3 --editor=nano
> > > +
> > > +Example3 shows how to edit a users attributes in the domain
> > > against a local +LDAP server using the 'nano' editor.
> > > +
> > > +"""
> > > +    synopsis = "%prog <username> [options]"
> > > +
> > > +    takes_options = [
> > > +        Option("-H", "--URL", help="LDB URL for database or target
> > > server",
> > > +               type=str, metavar="URL", dest="H"),
> > > +        Option("--editor", help="Editor to use instead of the
> > > system default,"
> > > +               " or 'vi' if no system default is set.", type=str),
> > > +    ]
> > > +
> > > +    takes_args = ["username"]
> > > +    takes_optiongroups = {
> > > +        "sambaopts": options.SambaOptions,
> > > +        "credopts": options.CredentialsOptions,
> > > +        "versionopts": options.VersionOptions,
> > > +        }
> > > +
> > > +    def run(self, username, 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)
> > > +
> > > +        filter =
> > > ("(&(sAMAccountType=805306368)(sAMAccountName=%s))" %
> >                                         ^^^
> > It would be easier to read with
> >
> >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> >
> > > +                  (ldb.binary_encode(username)))
> > > +
> > > +        domaindn = samdb.domain_dn()
> > > +
> > > +        try:
> > > +            res = samdb.search(base=domaindn,
> > > +                               expression=filter,
> > > +                               scope=ldb.SCOPE_SUBTREE)
> > > +            user_dn = res[0].dn
> > > +        except IndexError:
> > > +            raise CommandError('Unable to find user "%s"' %
> > > (username)) +
> > > +        for msg in res:
> > > +            r_ldif = samdb.write_ldif(msg, 1)
> > > +            # remove 'changetype' line
> > > +            result_ldif = re.sub('changetype: add\n', '', r_ldif)
> > > +
> > > +            if editor is None:
> > > +                editor = os.environ.get('EDITOR')
> > > +                if editor is None:
> > > +                    editor = 'vi'
> > > +
> > > +            with tempfile.NamedTemporaryFile(suffix=".tmp") as
> > > t_file:
> > > +                t_file.write(result_ldif)
> > > +                t_file.flush()
> > > +                try:
> > > +                    call([editor, t_file.name])
> > > +                except CalledProcessError, e:
> > > +                    raise IOError("ERROR: ", e)
> >
> > Is there are reason to prefer IOError over CalledProcessError?
> >
> > > +                with open(t_file.name) as edited_file:
> > > +                    edited_message = edited_file.read()
> > > +
> > > +        if result_ldif != edited_message:
> > > +            diff = difflib.ndiff(result_ldif.splitlines(),
> > > +                                 edited_message.splitlines())
> > > +            minus_lines = []
> > > +            plus_lines = []
> > > +            for line in diff:
> > > +                if line.startswith('-'):
> > > +                    line = line[2:]
> > > +                    minus_lines.append(line)
> > > +                elif line.startswith('+'):
> > > +                    line = line[2:]
> > > +                    plus_lines.append(line)
> > > +
> > > +            user_ldif="dn: %s\n" % user_dn
> > > +            user_ldif += "changetype: modify\n"
> > > +
> > > +            for line in minus_lines:
> > > +                attr, val = line.split(':')
> >
> > Here you want line.split(':', 1), or you'll get an error when a value
> > contains a colon (like "url: http://example.com").
> >
> > > +                if attr not in str(plus_lines):
> >                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
> >
> > This is NOT going to work well, because it matches whenever a value
> > happens to contain the attribute's name. Contriving an example:
> >
> > cn: foo
> > - street: 17 Picnic street.
> > + streetAddress: 17 Picnic Street
> >
> > `str(plus_lines)` contains "street" (and incidentally "cn"), resulting
> > in weird changes.
> >
> > What you need to do is something more like:
> >
> > minus_attrs = set()
> > for line in diff:
> >     op = line[:2]
> >     attr, value = line[2:].split(':', 1)
> >     if change == '- ':
> >         minus_attrs.add(attr)
> >     ...
> >
> >     if attr not in minus_attrs:
> >
> >
> > > +                    user_ldif += "delete: %s\n" % attr
> > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > +
> > > +            for line in plus_lines:
> > > +                attr, val = line.split(':')
> > > +                if attr in str(minus_lines):
> > > +                    user_ldif += "replace: %s\n" % attr
> > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > +                if attr not in str(minus_lines):
> > > +                    user_ldif += "add: %s\n" % attr
> > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > +
> > > +            try:
> > > +                samdb.modify_ldif(user_ldif)
> > > +            except Exception, e:
> > > +                raise CommandError("Failed to modify user '%s': " %
> > > +                                   username, e)
> > > +
> >    ^^^^^^^^
> > Git tells me there's loose whitespace here.
> >
> > > +            self.outf.write("Modified User '%s' successfully\n" %
> > > username) +
> > >  class cmd_user(SuperCommand):
> > >      """User management."""
> > >  
> > > @@ -2298,3 +2432,4 @@ class cmd_user(SuperCommand):
> > >      subcommands["setpassword"] = cmd_user_setpassword()
> > >      subcommands["getpassword"] = cmd_user_getpassword()
> > >      subcommands["syncpasswords"] = cmd_user_syncpasswords()
> > > +    subcommands["edit"] = cmd_user_edit()
> > > -- 2.1.4
> > >
> >
> > cheers,
> > Douglas
>
> Hi Douglas, thanks for looking at my patch and I take on board most of
> your comments and will work on the patch. The only comment I have a
> problem with is this one:
>
> It would be easier to read with
>
>     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
>
> 'pydoc samba.dsdb' shows this:
>
>     UF_NORMAL_ACCOUNT = 512
>
> This means that the filter will become:
>
> sAMAccountType=512
>
> Don't think that is a valid number for sAMAccountType, it is for
> userAccountControl though.
>
> My way seems to be the accepted filter to just get a user.
It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT. Please use
this one.

Also please fix 'except FooBar:' to be 'except Foobar as e:'. The former
is not compatible with Python 3. See
http://portingguide.readthedocs.io/en/latest/index.html for more details
on Python 3 compatibility. I suspect, though, we'll have some issues
with string processing too but it would be nice to reduce amount of
Python 3 incompatibilities going forward.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Tue, 27 Jun 2017 12:44:14 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:
> > On Tue, 27 Jun 2017 14:35:42 +1200
> > Douglas Bagnall <[hidden email]> wrote:
> >
> > > hi Rowland,
> > >
> > > On 24/06/17 02:27, Rowland Penny via samba-technical wrote:
> > > > Hi, the attached patch is something I created to use instead of
> > > > running ldbedit when I just wanted to alter one of a users
> > > > attributes.
> > >
> > > Nice documentation, and it seems to avoid some ldbedit pitfalls
> > > (e.g. --show-deleted), but I see a few issues.
> > >
> > > 1: tests.
> > >
> > > 2..n: interspersed below.
> > > >
> > > > Rowland
> > > >
> > > >
> > > > samba-tool-Easily-edit-a-users-object-in-AD.patch
> > > >
> > > >
> > > > From 540f2bced8c8a6172fdef6aea6d74ef6584c4455 Mon Sep 17
> > > > 00:00:00 2001 From: Rowland Penny <[hidden email]>
> > > > Date: Fri, 23 Jun 2017 14:27:54 +0100
> > > > Subject: [PATCH] samba-tool: Easily edit a users object in AD,
> > > > as if using ldbedit.
> > > >
> > > > Signed-off-by: Rowland Penny <[hidden email]>
> > > > ---
> > > >  python/samba/netcmd/user.py | 137
> > > > +++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 136
> > > > insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/python/samba/netcmd/user.py
> > > > b/python/samba/netcmd/user.py index 53ac39f..6b12dde 100644
> > > > --- a/python/samba/netcmd/user.py
> > > > +++ b/python/samba/netcmd/user.py
> > > > @@ -21,6 +21,9 @@ import samba.getopt as options
> > > >  import ldb
> > > >  import pwd
> > > >  import os
> > > > +import re
> > > > +import tempfile
> > > > +import difflib
> > > >  import sys
> > > >  import fcntl
> > > >  import signal
> > > > @@ -28,7 +31,7 @@ import errno
> > > >  import time
> > > >  import base64
> > > >  import binascii
> > > > -from subprocess import Popen, PIPE, STDOUT
> > > > +from subprocess import Popen, PIPE, STDOUT, call,
> > > > CalledProcessError
> > >
> > > You want check_call, not call if you're expecting a
> > > CalledProcessError. call returns the status code.
> > >
> > > >  from getpass import getpass
> > > >  from samba.auth import system_session
> > > >  from samba.samdb import SamDB
> > > > @@ -2283,6 +2286,137 @@ samba-tool user syncpasswords
> > > > --terminate \\ update_pid(None)
> > > >          return
> > > >  
> > > > +class cmd_user_edit(Command):
> > > > +    """Modify User AD object.
> > > > +
> > > > +This command will allow editing of a user account in the Active
> > > > Directory +domain. You will then be able to add or change
> > > > attributes and their values. +
> > > > +The username specified on the command is the sAMAccountName.
> > > > +
> > > > +The command may be run from the root userid or another
> > > > authorized userid. +
> > > > +The -H or --URL= option can be used to execute the command
> > > > against a remote +server.
> > > > +
> > > > +Example1:
> > > > +samba-tool user edit User1 -H ldap://samba.samdom.example.com \
> > > > +-U administrator --password=passw1rd
> > > > +
> > > > +Example1 shows how to edit a users attributes in the domain
> > > > against a remote +LDAP server.
> > > > +
> > > > +The -H parameter is used to specify the remote target server.
> > > > +
> > > > +Example2:
> > > > +samba-tool user edit User2
> > > > +
> > > > +Example2 shows how to edit a users attributes in the domain
> > > > against a local +LDAP server.
> > > > +
> > > > +Example3:
> > > > +samba-tool user edit User3 --editor=nano
> > > > +
> > > > +Example3 shows how to edit a users attributes in the domain
> > > > against a local +LDAP server using the 'nano' editor.
> > > > +
> > > > +"""
> > > > +    synopsis = "%prog <username> [options]"
> > > > +
> > > > +    takes_options = [
> > > > +        Option("-H", "--URL", help="LDB URL for database or
> > > > target server",
> > > > +               type=str, metavar="URL", dest="H"),
> > > > +        Option("--editor", help="Editor to use instead of the
> > > > system default,"
> > > > +               " or 'vi' if no system default is set.",
> > > > type=str),
> > > > +    ]
> > > > +
> > > > +    takes_args = ["username"]
> > > > +    takes_optiongroups = {
> > > > +        "sambaopts": options.SambaOptions,
> > > > +        "credopts": options.CredentialsOptions,
> > > > +        "versionopts": options.VersionOptions,
> > > > +        }
> > > > +
> > > > +    def run(self, username, 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)
> > > > +
> > > > +        filter =
> > > > ("(&(sAMAccountType=805306368)(sAMAccountName=%s))" %
> > >                                         ^^^
> > > It would be easier to read with
> > >
> > >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> > >
> > > > +                  (ldb.binary_encode(username)))
> > > > +
> > > > +        domaindn = samdb.domain_dn()
> > > > +
> > > > +        try:
> > > > +            res = samdb.search(base=domaindn,
> > > > +                               expression=filter,
> > > > +                               scope=ldb.SCOPE_SUBTREE)
> > > > +            user_dn = res[0].dn
> > > > +        except IndexError:
> > > > +            raise CommandError('Unable to find user "%s"' %
> > > > (username)) +
> > > > +        for msg in res:
> > > > +            r_ldif = samdb.write_ldif(msg, 1)
> > > > +            # remove 'changetype' line
> > > > +            result_ldif = re.sub('changetype: add\n', '',
> > > > r_ldif) +
> > > > +            if editor is None:
> > > > +                editor = os.environ.get('EDITOR')
> > > > +                if editor is None:
> > > > +                    editor = 'vi'
> > > > +
> > > > +            with tempfile.NamedTemporaryFile(suffix=".tmp") as
> > > > t_file:
> > > > +                t_file.write(result_ldif)
> > > > +                t_file.flush()
> > > > +                try:
> > > > +                    call([editor, t_file.name])
> > > > +                except CalledProcessError, e:
> > > > +                    raise IOError("ERROR: ", e)
> > >
> > > Is there are reason to prefer IOError over CalledProcessError?
> > >
> > > > +                with open(t_file.name) as edited_file:
> > > > +                    edited_message = edited_file.read()
> > > > +
> > > > +        if result_ldif != edited_message:
> > > > +            diff = difflib.ndiff(result_ldif.splitlines(),
> > > > +                                 edited_message.splitlines())
> > > > +            minus_lines = []
> > > > +            plus_lines = []
> > > > +            for line in diff:
> > > > +                if line.startswith('-'):
> > > > +                    line = line[2:]
> > > > +                    minus_lines.append(line)
> > > > +                elif line.startswith('+'):
> > > > +                    line = line[2:]
> > > > +                    plus_lines.append(line)
> > > > +
> > > > +            user_ldif="dn: %s\n" % user_dn
> > > > +            user_ldif += "changetype: modify\n"
> > > > +
> > > > +            for line in minus_lines:
> > > > +                attr, val = line.split(':')
> > >
> > > Here you want line.split(':', 1), or you'll get an error when a
> > > value contains a colon (like "url: http://example.com").
> > >
> > > > +                if attr not in str(plus_lines):
> > >                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >
> > > This is NOT going to work well, because it matches whenever a
> > > value happens to contain the attribute's name. Contriving an
> > > example:
> > >
> > > cn: foo
> > > - street: 17 Picnic street.
> > > + streetAddress: 17 Picnic Street
> > >
> > > `str(plus_lines)` contains "street" (and incidentally "cn"),
> > > resulting in weird changes.
> > >
> > > What you need to do is something more like:
> > >
> > > minus_attrs = set()
> > > for line in diff:
> > >     op = line[:2]
> > >     attr, value = line[2:].split(':', 1)
> > >     if change == '- ':
> > >         minus_attrs.add(attr)
> > >     ...
> > >
> > >     if attr not in minus_attrs:
> > >
> > >
> > > > +                    user_ldif += "delete: %s\n" % attr
> > > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > > +
> > > > +            for line in plus_lines:
> > > > +                attr, val = line.split(':')
> > > > +                if attr in str(minus_lines):
> > > > +                    user_ldif += "replace: %s\n" % attr
> > > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > > +                if attr not in str(minus_lines):
> > > > +                    user_ldif += "add: %s\n" % attr
> > > > +                    user_ldif += "%s: %s\n" % (attr, val)
> > > > +
> > > > +            try:
> > > > +                samdb.modify_ldif(user_ldif)
> > > > +            except Exception, e:
> > > > +                raise CommandError("Failed to modify user
> > > > '%s': " %
> > > > +                                   username, e)
> > > > +
> > >    ^^^^^^^^
> > > Git tells me there's loose whitespace here.
> > >
> > > > +            self.outf.write("Modified User '%s'
> > > > successfully\n" % username) +
> > > >  class cmd_user(SuperCommand):
> > > >      """User management."""
> > > >  
> > > > @@ -2298,3 +2432,4 @@ class cmd_user(SuperCommand):
> > > >      subcommands["setpassword"] = cmd_user_setpassword()
> > > >      subcommands["getpassword"] = cmd_user_getpassword()
> > > >      subcommands["syncpasswords"] = cmd_user_syncpasswords()
> > > > +    subcommands["edit"] = cmd_user_edit()
> > > > -- 2.1.4
> > > >
> > >
> > > cheers,
> > > Douglas
> >
> > Hi Douglas, thanks for looking at my patch and I take on board most
> > of your comments and will work on the patch. The only comment I
> > have a problem with is this one:
> >
> > It would be easier to read with
> >
> >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> >
> > 'pydoc samba.dsdb' shows this:
> >
> >     UF_NORMAL_ACCOUNT = 512
> >
> > This means that the filter will become:
> >
> > sAMAccountType=512
> >
> > Don't think that is a valid number for sAMAccountType, it is for
> > userAccountControl though.
> >
> > My way seems to be the accepted filter to just get a user.
> It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT. Please
> use this one.

I knew it wasn't correct, but for the ignorant (i.e. me), why use
'ATYPE_NORMAL_ACCOUNT' instead of '805306368' ?

>
> Also please fix 'except FooBar:' to be 'except Foobar as e:'. The
> former is not compatible with Python 3. See
> http://portingguide.readthedocs.io/en/latest/index.html for more
> details on Python 3 compatibility. I suspect, though, we'll have some
> issues with string processing too but it would be nice to reduce
> amount of Python 3 incompatibilities going forward.
>

Will do, this is the least of my worries ;-)

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
hi Rowland,

> Hi Douglas, thanks for looking at my patch and I take on board most of
> your comments and will work on the patch. The only comment I have a
> problem with is this one:
>
> It would be easier to read with
>
>     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
>
> 'pydoc samba.dsdb' shows this:
>
>     UF_NORMAL_ACCOUNT = 512

Right. Sorry. I should have said dsdb.ATYPE_NORMAL_ACCOUNT.

It is the same number you had (0x30000000) but documents itself better
for people like me who don't know the numbers.

Douglas

>
> This means that the filter will become:
>
> sAMAccountType=512
>
> Don't think that is a valid number for sAMAccountType, it is for
> userAccountControl though.
>
> My way seems to be the accepted filter to just get a user.
>
> Rowland
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:

> > >
> > >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> > >
> > > 'pydoc samba.dsdb' shows this:
> > >
> > >     UF_NORMAL_ACCOUNT = 512
> > >
> > > This means that the filter will become:
> > >
> > > sAMAccountType=512
> > >
> > > Don't think that is a valid number for sAMAccountType, it is for
> > > userAccountControl though.
> > >
> > > My way seems to be the accepted filter to just get a user.
> > It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT. Please
> > use this one.
>
> I knew it wasn't correct, but for the ignorant (i.e. me), why use
> 'ATYPE_NORMAL_ACCOUNT' instead of '805306368' ?
There are few reasons:
 - using symbolic constants is more readable. For me, at least, as I
   don't remember all constants by heart.
 - using symbolic constants avoids subtle typo issues. Errors in typing
   are not unknown.

We have few more places in the Python code that refer to a normal account
type by its numeric value. Using constants helps to reduce likeability
of an error that would be very hard to spot otherwise.


--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Tue, 27 Jun 2017 17:57:10 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:
> > > >
> > > >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> > > >
> > > > 'pydoc samba.dsdb' shows this:
> > > >
> > > >     UF_NORMAL_ACCOUNT = 512
> > > >
> > > > This means that the filter will become:
> > > >
> > > > sAMAccountType=512
> > > >
> > > > Don't think that is a valid number for sAMAccountType, it is for
> > > > userAccountControl though.
> > > >
> > > > My way seems to be the accepted filter to just get a user.
> > > It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT.
> > > Please use this one.
> >
> > I knew it wasn't correct, but for the ignorant (i.e. me), why use
> > 'ATYPE_NORMAL_ACCOUNT' instead of '805306368' ?
> There are few reasons:
>  - using symbolic constants is more readable. For me, at least, as I
>    don't remember all constants by heart.

Valid reason, I usually have to look things like this up.

>  - using symbolic constants avoids subtle typo issues. Errors in
> typing are not unknown.

Really understand this, I could be the king of typo's, never mind
incorrectly spelt words, I can completely miss out entire words ;-)

>
> We have few more places in the Python code that refer to a normal
> account type by its numeric value. Using constants helps to reduce
> likeability of an error that would be very hard to spot otherwise.

Sorry, but I had to read that last sentence several times before I
understood it. I think you meant 'Using constants helps to increase the
likelihood of seeing hard to spot errors.'. If this the case, then I
totally agree with you ;-)

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:

> On Tue, 27 Jun 2017 17:57:10 +0300
> Alexander Bokovoy <[hidden email]> wrote:
>
> > On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:
> > > > >
> > > > >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> > > > >
> > > > > 'pydoc samba.dsdb' shows this:
> > > > >
> > > > >     UF_NORMAL_ACCOUNT = 512
> > > > >
> > > > > This means that the filter will become:
> > > > >
> > > > > sAMAccountType=512
> > > > >
> > > > > Don't think that is a valid number for sAMAccountType, it is for
> > > > > userAccountControl though.
> > > > >
> > > > > My way seems to be the accepted filter to just get a user.
> > > > It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT.
> > > > Please use this one.
> > >
> > > I knew it wasn't correct, but for the ignorant (i.e. me), why use
> > > 'ATYPE_NORMAL_ACCOUNT' instead of '805306368' ?
> > There are few reasons:
> >  - using symbolic constants is more readable. For me, at least, as I
> >    don't remember all constants by heart.
>
> Valid reason, I usually have to look things like this up.
>
> >  - using symbolic constants avoids subtle typo issues. Errors in
> > typing are not unknown.
>
> Really understand this, I could be the king of typo's, never mind
> incorrectly spelt words, I can completely miss out entire words ;-)
>
> >
> > We have few more places in the Python code that refer to a normal
> > account type by its numeric value. Using constants helps to reduce
> > likeability of an error that would be very hard to spot otherwise.
>
> Sorry, but I had to read that last sentence several times before I
> understood it. I think you meant 'Using constants helps to increase the
> likelihood of seeing hard to spot errors.'. If this the case, then I
> totally agree with you ;-)
:) If you do a typo and use 805306360 instead of 805306368, it will be
hard to detect as the code would still work. If you'd do a typo in
dsdb.ATYPE_NORMAL_ACCOUNT it would fail with 'unknown variable' which is
easy to see because the code execution will break. That was my point.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Tue, 27 Jun 2017 18:55:04 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:
> > On Tue, 27 Jun 2017 17:57:10 +0300
> > Alexander Bokovoy <[hidden email]> wrote:
> >
> > > On ti, 27 kesä 2017, Rowland Penny via samba-technical wrote:
> > > > > >
> > > > > >     "sAMAccountType=%d"... % (dsdb.UF_NORMAL_ACCOUNT,...
> > > > > >
> > > > > > 'pydoc samba.dsdb' shows this:
> > > > > >
> > > > > >     UF_NORMAL_ACCOUNT = 512
> > > > > >
> > > > > > This means that the filter will become:
> > > > > >
> > > > > > sAMAccountType=512
> > > > > >
> > > > > > Don't think that is a valid number for sAMAccountType, it
> > > > > > is for userAccountControl though.
> > > > > >
> > > > > > My way seems to be the accepted filter to just get a user.
> > > > > It is samba.dsdb.ATYPE_NORMAL_ACCOUNT, not UF_NORMAL_ACCOUNT.
> > > > > Please use this one.
> > > >
> > > > I knew it wasn't correct, but for the ignorant (i.e. me), why
> > > > use 'ATYPE_NORMAL_ACCOUNT' instead of '805306368' ?
> > > There are few reasons:
> > >  - using symbolic constants is more readable. For me, at least,
> > > as I don't remember all constants by heart.
> >
> > Valid reason, I usually have to look things like this up.
> >
> > >  - using symbolic constants avoids subtle typo issues. Errors in
> > > typing are not unknown.
> >
> > Really understand this, I could be the king of typo's, never mind
> > incorrectly spelt words, I can completely miss out entire words ;-)
> >
> > >
> > > We have few more places in the Python code that refer to a normal
> > > account type by its numeric value. Using constants helps to reduce
> > > likeability of an error that would be very hard to spot otherwise.
> >
> > Sorry, but I had to read that last sentence several times before I
> > understood it. I think you meant 'Using constants helps to increase
> > the likelihood of seeing hard to spot errors.'. If this the case,
> > then I totally agree with you ;-)
> :) If you do a typo and use 805306360 instead of 805306368, it will be
> hard to detect as the code would still work. If you'd do a typo in
> dsdb.ATYPE_NORMAL_ACCOUNT it would fail with 'unknown variable' which
> is easy to see because the code execution will break. That was my
> point.
>
OK, the attached patch should meet all problems raised against the
first one, except a test.

I am struggling with the test. I can see how I could write a test that
would:

Create a user to use in the test.
Run 'samba-tool user edit USER'

I cannot then see how I can write code to simulate a sysadmin
altering the users object and/or saving and closing the editor.

Rowland
 

samba-tool-Easily-edit-a-users-object-in-AD.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Wed, 2017-06-28 at 09:32 +0100, Rowland Penny via samba-technical
wrote:

>
> OK, the attached patch should meet all problems raised against the
> first one, except a test.
>
> I am struggling with the test. I can see how I could write a test that
> would:
>
> Create a user to use in the test.
> Run 'samba-tool user edit USER'
>
> I cannot then see how I can write code to simulate a sysadmin
> altering the users object and/or saving and closing the editor.

You replace the EDITOR with a script that modifies the object's LDIF.

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


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:

> On Wed, 2017-06-28 at 09:32 +0100, Rowland Penny via samba-technical
> wrote:
> >
> > OK, the attached patch should meet all problems raised against the
> > first one, except a test.
> >
> > I am struggling with the test. I can see how I could write a test that
> > would:
> >
> > Create a user to use in the test.
> > Run 'samba-tool user edit USER'
> >
> > I cannot then see how I can write code to simulate a sysadmin
> > altering the users object and/or saving and closing the editor.
>
> You replace the EDITOR with a script that modifies the object's LDIF.
Yes, something like
EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Wed, 28 Jun 2017 12:27:53 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:
> > On Wed, 2017-06-28 at 09:32 +0100, Rowland Penny via samba-technical
> > wrote:
> > >
> > > OK, the attached patch should meet all problems raised against the
> > > first one, except a test.
> > >
> > > I am struggling with the test. I can see how I could write a test
> > > that would:
> > >
> > > Create a user to use in the test.
> > > Run 'samba-tool user edit USER'
> > >
> > > I cannot then see how I can write code to simulate a sysadmin
> > > altering the users object and/or saving and closing the editor.
> >
> > You replace the EDITOR with a script that modifies the object's
> > LDIF.
> Yes, something like
> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
>

That is even easier, I was thinking something a lot more complex, just
need to identify an attribute created by 'samba-tool user create' that
will be there and can be altered.

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
In reply to this post by Samba - samba-technical mailing list
On Wed, 28 Jun 2017 12:27:53 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:
> > On Wed, 2017-06-28 at 09:32 +0100, Rowland Penny via samba-technical
> > wrote:
> > >
> > > OK, the attached patch should meet all problems raised against the
> > > first one, except a test.
> > >
> > > I am struggling with the test. I can see how I could write a test
> > > that would:
> > >
> > > Create a user to use in the test.
> > > Run 'samba-tool user edit USER'
> > >
> > > I cannot then see how I can write code to simulate a sysadmin
> > > altering the users object and/or saving and closing the editor.
> >
> > You replace the EDITOR with a script that modifies the object's
> > LDIF.
> Yes, something like
> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
>

OK, this doesn't work, but if I create a dummy editor script, it does.

This is what I came up with:

#!/bin/sh
user_ldif="$1"
SED=$(which sed)
$SED -i -e \'s/userAccountControl: 512/userAccountControl: 514/\' $user_ldif

If I create the above script as /tmp/editor.sh, make it executable and
then pass it to 'samba-tool user USER' with '--editor=/tmp/editor.sh'
it works.

Problem is, how do I either get the python test script to create the
bash script and then use it, or how do I create the bash script
somewhere in the source (if so where?) and then get the python test
script to use it ?

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> On Wed, 28 Jun 2017 12:27:53 +0300
> Alexander Bokovoy <[hidden email]> wrote:
>
>> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:

>>> You replace the EDITOR with a script that modifies the object's
>>> LDIF.
>> Yes, something like
>> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
>>
>
> OK, this doesn't work, but if I create a dummy editor script, it does.

Yes, that isn't going to work because you are calling the editor using a
list, exactly like so

       call([editor, t_file.name])

which is interpreted as an exec-style argument list, meaning it will look
for an editor called "sed -i -e ...", not one called "sed".  In this case
it is probably mostly safe to change it to

       call("%s %s" % (editor, t_file.name), shell=True)

which will use a shell to split the string.

> This is what I came up with:
>
> #!/bin/sh
> user_ldif="$1"
> SED=$(which sed)
> $SED -i -e \'s/userAccountControl: 512/userAccountControl: 514/\' $user_ldif
>
> If I create the above script as /tmp/editor.sh, make it executable and
> then pass it to 'samba-tool user USER' with '--editor=/tmp/editor.sh'
> it works.
>
> Problem is, how do I either get the python test script to create the
> bash script and then use it, or how do I create the bash script
> somewhere in the source (if so where?) and then get the python test
> script to use it ?

Why not use tempfile.NamedTemporaryFile() again? and subprocess.call()?

Douglas

>
> Rowland
>


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Sat, 1 Jul 2017 13:27:07 +1200
Douglas Bagnall <[hidden email]> wrote:

> On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> > On Wed, 28 Jun 2017 12:27:53 +0300
> > Alexander Bokovoy <[hidden email]> wrote:
> >
> >> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:
>
> >>> You replace the EDITOR with a script that modifies the object's
> >>> LDIF.
> >> Yes, something like
> >> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
> >>
> >
> > OK, this doesn't work, but if I create a dummy editor script, it
> > does.
>
> Yes, that isn't going to work because you are calling the editor
> using a list, exactly like so
>
>        call([editor, t_file.name])
>
> which is interpreted as an exec-style argument list, meaning it will
> look for an editor called "sed -i -e ...", not one called "sed".  In
> this case it is probably mostly safe to change it to
>
>        call("%s %s" % (editor, t_file.name), shell=True)
>
> which will use a shell to split the string.
>
> > This is what I came up with:
> >
> > #!/bin/sh
> > user_ldif="$1"
> > SED=$(which sed)
> > $SED -i -e \'s/userAccountControl: 512/userAccountControl: 514/\'
> > $user_ldif
> >
> > If I create the above script as /tmp/editor.sh, make it executable
> > and then pass it to 'samba-tool user USER' with
> > '--editor=/tmp/editor.sh' it works.
> >
> > Problem is, how do I either get the python test script to create the
> > bash script and then use it, or how do I create the bash script
> > somewhere in the source (if so where?) and then get the python test
> > script to use it ?
>
> Why not use tempfile.NamedTemporaryFile() again? and
> subprocess.call()?
>
> Douglas
>
> >
> > Rowland
> >
>

I have tried what Douglas suggested, but I am getting nowhere, I do not
want to alter the 'samba-tool user edit', I just want to get a python
test script to run it and hopefully pass.

edit.py uses an editor and a human intervention to work, I can automate
this with a simple bash script. What I cannot do is to get a python test
script to fully automate this.

I think if I was to create the bash script in the source code it
would work, but where would be the best place to put it ? and how
would I obtain the path to it ?

Rowland
 

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Sat, 1 Jul 2017 18:08:10 +0100
Rowland Penny via samba-technical <[hidden email]>
wrote:

> On Sat, 1 Jul 2017 13:27:07 +1200
> Douglas Bagnall <[hidden email]> wrote:
>
> > On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> > > On Wed, 28 Jun 2017 12:27:53 +0300
> > > Alexander Bokovoy <[hidden email]> wrote:
> > >
> > >> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:
> >
> > >>> You replace the EDITOR with a script that modifies the object's
> > >>> LDIF.
> > >> Yes, something like
> > >> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
> > >>
> > >
> > > OK, this doesn't work, but if I create a dummy editor script, it
> > > does.
> >
> > Yes, that isn't going to work because you are calling the editor
> > using a list, exactly like so
> >
> >        call([editor, t_file.name])
> >
> > which is interpreted as an exec-style argument list, meaning it will
> > look for an editor called "sed -i -e ...", not one called "sed".  In
> > this case it is probably mostly safe to change it to
> >
> >        call("%s %s" % (editor, t_file.name), shell=True)
> >
> > which will use a shell to split the string.
> >
> > > This is what I came up with:
> > >
> > > #!/bin/sh
> > > user_ldif="$1"
> > > SED=$(which sed)
> > > $SED -i -e \'s/userAccountControl: 512/userAccountControl: 514/\'
> > > $user_ldif
> > >
> > > If I create the above script as /tmp/editor.sh, make it executable
> > > and then pass it to 'samba-tool user USER' with
> > > '--editor=/tmp/editor.sh' it works.
> > >
> > > Problem is, how do I either get the python test script to create
> > > the bash script and then use it, or how do I create the bash
> > > script somewhere in the source (if so where?) and then get the
> > > python test script to use it ?
> >
> > Why not use tempfile.NamedTemporaryFile() again? and
> > subprocess.call()?
> >
> > Douglas
> >
> > >
> > > Rowland
> > >
> >
>
> I have tried what Douglas suggested, but I am getting nowhere, I do
> not want to alter the 'samba-tool user edit', I just want to get a
> python test script to run it and hopefully pass.
>
> edit.py uses an editor and a human intervention to work, I can
> automate this with a simple bash script. What I cannot do is to get a
> python test script to fully automate this.
>
> I think if I was to create the bash script in the source code it
> would work, but where would be the best place to put it ? and how
> would I obtain the path to it ?
>
> Rowland
>  
>

I couldn't get the python script to work, so I dumped it, in favour of
a bash script and this seems to work, but I don't fully understand the
output, here is a sample:

testsuite: samba.tests.samba_tool.edit(fl2008r2dc)
progress: push
time: 2017-07-03 13:27:14.000000Z
User 'sambatool1' created successfully
Modified User 'sambatool1' successfully
Deleted user sambatool1
teardown_env(fl2008r2dc)
samba: EOF on stdin - PID 2302 terminating
time: 2017-07-03 13:27:14.000000Z
progress: pop
command: /root/samba-master/python/samba/tests/samba_tool/edit.sh $SERVER $USERNAME $PASSWORD 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
expanded command: /root/samba-master/python/samba/tests/samba_tool/edit.sh dc7 Administrator locDCpass7 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
Exit code was 1

]

samba child process 2302 exited with value 0
samba: EOF on stdin - PID 2234 terminating
SAMBA LOG of: DC7 pid 2302
samba: EOF on stdin - PID 2302 terminating
TOP 10 slowest tests
samba.tests.samba_tool.edit(fl2008r2dc) -> 0
'testonly' finished successfully (29.037s)

The script creates the user, modifies the user and then deletes the
user, the script then exits with the exit code '0', but there is this:

testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
Exit code was 1

]

Is this something to worry about ?
Where does the '1' exit code come from ?

Rowland

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On ma, 03 heinä 2017, Rowland Penny via samba-technical wrote:

> On Sat, 1 Jul 2017 18:08:10 +0100
> Rowland Penny via samba-technical <[hidden email]>
> wrote:
>
> > On Sat, 1 Jul 2017 13:27:07 +1200
> > Douglas Bagnall <[hidden email]> wrote:
> >
> > > On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> > > > On Wed, 28 Jun 2017 12:27:53 +0300
> > > > Alexander Bokovoy <[hidden email]> wrote:
> > > >
> > > >> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical wrote:
> > >
> > > >>> You replace the EDITOR with a script that modifies the object's
> > > >>> LDIF.
> > > >> Yes, something like
> > > >> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
> > > >>
> > > >
> > > > OK, this doesn't work, but if I create a dummy editor script, it
> > > > does.
> > >
> > > Yes, that isn't going to work because you are calling the editor
> > > using a list, exactly like so
> > >
> > >        call([editor, t_file.name])
> > >
> > > which is interpreted as an exec-style argument list, meaning it will
> > > look for an editor called "sed -i -e ...", not one called "sed".  In
> > > this case it is probably mostly safe to change it to
> > >
> > >        call("%s %s" % (editor, t_file.name), shell=True)
> > >
> > > which will use a shell to split the string.
> > >
> > > > This is what I came up with:
> > > >
> > > > #!/bin/sh
> > > > user_ldif="$1"
> > > > SED=$(which sed)
> > > > $SED -i -e \'s/userAccountControl: 512/userAccountControl: 514/\'
> > > > $user_ldif
> > > >
> > > > If I create the above script as /tmp/editor.sh, make it executable
> > > > and then pass it to 'samba-tool user USER' with
> > > > '--editor=/tmp/editor.sh' it works.
> > > >
> > > > Problem is, how do I either get the python test script to create
> > > > the bash script and then use it, or how do I create the bash
> > > > script somewhere in the source (if so where?) and then get the
> > > > python test script to use it ?
> > >
> > > Why not use tempfile.NamedTemporaryFile() again? and
> > > subprocess.call()?
> > >
> > > Douglas
> > >
> > > >
> > > > Rowland
> > > >
> > >
> >
> > I have tried what Douglas suggested, but I am getting nowhere, I do
> > not want to alter the 'samba-tool user edit', I just want to get a
> > python test script to run it and hopefully pass.
> >
> > edit.py uses an editor and a human intervention to work, I can
> > automate this with a simple bash script. What I cannot do is to get a
> > python test script to fully automate this.
> >
> > I think if I was to create the bash script in the source code it
> > would work, but where would be the best place to put it ? and how
> > would I obtain the path to it ?
> >
> > Rowland
> >  
> >
>
> I couldn't get the python script to work, so I dumped it, in favour of
> a bash script and this seems to work, but I don't fully understand the
> output, here is a sample:
>
> testsuite: samba.tests.samba_tool.edit(fl2008r2dc)
> progress: push
> time: 2017-07-03 13:27:14.000000Z
> User 'sambatool1' created successfully
> Modified User 'sambatool1' successfully
> Deleted user sambatool1
> teardown_env(fl2008r2dc)
> samba: EOF on stdin - PID 2302 terminating
> time: 2017-07-03 13:27:14.000000Z
> progress: pop
> command: /root/samba-master/python/samba/tests/samba_tool/edit.sh $SERVER $USERNAME $PASSWORD 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> expanded command: /root/samba-master/python/samba/tests/samba_tool/edit.sh dc7 Administrator locDCpass7 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
> Exit code was 1
>
> ]
>
> samba child process 2302 exited with value 0
> samba: EOF on stdin - PID 2234 terminating
> SAMBA LOG of: DC7 pid 2302
> samba: EOF on stdin - PID 2302 terminating
> TOP 10 slowest tests
> samba.tests.samba_tool.edit(fl2008r2dc) -> 0
> 'testonly' finished successfully (29.037s)
>
> The script creates the user, modifies the user and then deletes the
> user, the script then exits with the exit code '0', but there is this:
>
> testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
> Exit code was 1
>
> ]
>
> Is this something to worry about ?
> Where does the '1' exit code come from ?
I think the script 'edit.sh' needs to return its output in a format
expected by selftest/filter-subunit which is a normal subunit format.

source3/script/tests/test_preserve_case.sh is one of examples on how we
do that in shell.

--
/ Alexander Bokovoy

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Mon, 3 Jul 2017 16:50:28 +0300
Alexander Bokovoy <[hidden email]> wrote:

> On ma, 03 heinä 2017, Rowland Penny via samba-technical wrote:
> > On Sat, 1 Jul 2017 18:08:10 +0100
> > Rowland Penny via samba-technical <[hidden email]>
> > wrote:
> >
> > > On Sat, 1 Jul 2017 13:27:07 +1200
> > > Douglas Bagnall <[hidden email]> wrote:
> > >
> > > > On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> > > > > On Wed, 28 Jun 2017 12:27:53 +0300
> > > > > Alexander Bokovoy <[hidden email]> wrote:
> > > > >
> > > > >> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical
> > > > >> wrote:
> > > >
> > > > >>> You replace the EDITOR with a script that modifies the
> > > > >>> object's LDIF.
> > > > >> Yes, something like
> > > > >> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
> > > > >>
> > > > >
> > > > > OK, this doesn't work, but if I create a dummy editor script,
> > > > > it does.
> > > >
> > > > Yes, that isn't going to work because you are calling the editor
> > > > using a list, exactly like so
> > > >
> > > >        call([editor, t_file.name])
> > > >
> > > > which is interpreted as an exec-style argument list, meaning it
> > > > will look for an editor called "sed -i -e ...", not one called
> > > > "sed".  In this case it is probably mostly safe to change it to
> > > >
> > > >        call("%s %s" % (editor, t_file.name), shell=True)
> > > >
> > > > which will use a shell to split the string.
> > > >
> > > > > This is what I came up with:
> > > > >
> > > > > #!/bin/sh
> > > > > user_ldif="$1"
> > > > > SED=$(which sed)
> > > > > $SED -i -e \'s/userAccountControl: 512/userAccountControl:
> > > > > 514/\' $user_ldif
> > > > >
> > > > > If I create the above script as /tmp/editor.sh, make it
> > > > > executable and then pass it to 'samba-tool user USER' with
> > > > > '--editor=/tmp/editor.sh' it works.
> > > > >
> > > > > Problem is, how do I either get the python test script to
> > > > > create the bash script and then use it, or how do I create
> > > > > the bash script somewhere in the source (if so where?) and
> > > > > then get the python test script to use it ?
> > > >
> > > > Why not use tempfile.NamedTemporaryFile() again? and
> > > > subprocess.call()?
> > > >
> > > > Douglas
> > > >
> > > > >
> > > > > Rowland
> > > > >
> > > >
> > >
> > > I have tried what Douglas suggested, but I am getting nowhere, I
> > > do not want to alter the 'samba-tool user edit', I just want to
> > > get a python test script to run it and hopefully pass.
> > >
> > > edit.py uses an editor and a human intervention to work, I can
> > > automate this with a simple bash script. What I cannot do is to
> > > get a python test script to fully automate this.
> > >
> > > I think if I was to create the bash script in the source code it
> > > would work, but where would be the best place to put it ? and how
> > > would I obtain the path to it ?
> > >
> > > Rowland
> > >  
> > >
> >
> > I couldn't get the python script to work, so I dumped it, in favour
> > of a bash script and this seems to work, but I don't fully
> > understand the output, here is a sample:
> >
> > testsuite: samba.tests.samba_tool.edit(fl2008r2dc)
> > progress: push
> > time: 2017-07-03 13:27:14.000000Z
> > User 'sambatool1' created successfully
> > Modified User 'sambatool1' successfully
> > Deleted user sambatool1
> > teardown_env(fl2008r2dc)
> > samba: EOF on stdin - PID 2302 terminating
> > time: 2017-07-03 13:27:14.000000Z
> > progress: pop
> > command: /root/samba-master/python/samba/tests/samba_tool/edit.sh
> > $SERVER $USERNAME $PASSWORD 2>&1
> > | /root/samba-master/selftest/filter-subunit --fail-on-empty
> > --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> > expanded
> > command: /root/samba-master/python/samba/tests/samba_tool/edit.sh
> > dc7 Administrator locDCpass7 2>&1
> > | /root/samba-master/selftest/filter-subunit --fail-on-empty
> > --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> > testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [ Exit
> > code was 1
> >
> > ]
> >
> > samba child process 2302 exited with value 0
> > samba: EOF on stdin - PID 2234 terminating
> > SAMBA LOG of: DC7 pid 2302
> > samba: EOF on stdin - PID 2302 terminating
> > TOP 10 slowest tests
> > samba.tests.samba_tool.edit(fl2008r2dc) -> 0
> > 'testonly' finished successfully (29.037s)
> >
> > The script creates the user, modifies the user and then deletes the
> > user, the script then exits with the exit code '0', but there is
> > this:
> >
> > testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
> > Exit code was 1
> >
> > ]
> >
> > Is this something to worry about ?
> > Where does the '1' exit code come from ?
> I think the script 'edit.sh' needs to return its output in a format
> expected by selftest/filter-subunit which is a normal subunit format.
>
> source3/script/tests/test_preserve_case.sh is one of examples on how
> we do that in shell.
>

Thanks, that helps a lot ;-)

Just need to rewrite my bash script using that as a template.

Rowland


Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: [PATCH] samba-tool: Easily edit a users object in AD

Samba - samba-technical mailing list
On Mon, 3 Jul 2017 14:57:06 +0100
Rowland Penny via samba-technical <[hidden email]>
wrote:

> On Mon, 3 Jul 2017 16:50:28 +0300
> Alexander Bokovoy <[hidden email]> wrote:
>
> > On ma, 03 heinä 2017, Rowland Penny via samba-technical wrote:
> > > On Sat, 1 Jul 2017 18:08:10 +0100
> > > Rowland Penny via samba-technical
> > > <[hidden email]> wrote:
> > >
> > > > On Sat, 1 Jul 2017 13:27:07 +1200
> > > > Douglas Bagnall <[hidden email]> wrote:
> > > >
> > > > > On 01/07/17 00:43, Rowland Penny via samba-technical wrote:
> > > > > > On Wed, 28 Jun 2017 12:27:53 +0300
> > > > > > Alexander Bokovoy <[hidden email]> wrote:
> > > > > >
> > > > > >> On ke, 28 kesä 2017, Andrew Bartlett via samba-technical
> > > > > >> wrote:
> > > > >
> > > > > >>> You replace the EDITOR with a script that modifies the
> > > > > >>> object's LDIF.
> > > > > >> Yes, something like
> > > > > >> EDITOR="sed -i -e 's/attribute: foo/attribute: bar/'"
> > > > > >>
> > > > > >
> > > > > > OK, this doesn't work, but if I create a dummy editor
> > > > > > script, it does.
> > > > >
> > > > > Yes, that isn't going to work because you are calling the
> > > > > editor using a list, exactly like so
> > > > >
> > > > >        call([editor, t_file.name])
> > > > >
> > > > > which is interpreted as an exec-style argument list, meaning
> > > > > it will look for an editor called "sed -i -e ...", not one
> > > > > called "sed".  In this case it is probably mostly safe to
> > > > > change it to
> > > > >
> > > > >        call("%s %s" % (editor, t_file.name), shell=True)
> > > > >
> > > > > which will use a shell to split the string.
> > > > >
> > > > > > This is what I came up with:
> > > > > >
> > > > > > #!/bin/sh
> > > > > > user_ldif="$1"
> > > > > > SED=$(which sed)
> > > > > > $SED -i -e \'s/userAccountControl: 512/userAccountControl:
> > > > > > 514/\' $user_ldif
> > > > > >
> > > > > > If I create the above script as /tmp/editor.sh, make it
> > > > > > executable and then pass it to 'samba-tool user USER' with
> > > > > > '--editor=/tmp/editor.sh' it works.
> > > > > >
> > > > > > Problem is, how do I either get the python test script to
> > > > > > create the bash script and then use it, or how do I create
> > > > > > the bash script somewhere in the source (if so where?) and
> > > > > > then get the python test script to use it ?
> > > > >
> > > > > Why not use tempfile.NamedTemporaryFile() again? and
> > > > > subprocess.call()?
> > > > >
> > > > > Douglas
> > > > >
> > > > > >
> > > > > > Rowland
> > > > > >
> > > > >
> > > >
> > > > I have tried what Douglas suggested, but I am getting nowhere, I
> > > > do not want to alter the 'samba-tool user edit', I just want to
> > > > get a python test script to run it and hopefully pass.
> > > >
> > > > edit.py uses an editor and a human intervention to work, I can
> > > > automate this with a simple bash script. What I cannot do is to
> > > > get a python test script to fully automate this.
> > > >
> > > > I think if I was to create the bash script in the source code it
> > > > would work, but where would be the best place to put it ? and
> > > > how would I obtain the path to it ?
> > > >
> > > > Rowland
> > > >  
> > > >
> > >
> > > I couldn't get the python script to work, so I dumped it, in
> > > favour of a bash script and this seems to work, but I don't fully
> > > understand the output, here is a sample:
> > >
> > > testsuite: samba.tests.samba_tool.edit(fl2008r2dc)
> > > progress: push
> > > time: 2017-07-03 13:27:14.000000Z
> > > User 'sambatool1' created successfully
> > > Modified User 'sambatool1' successfully
> > > Deleted user sambatool1
> > > teardown_env(fl2008r2dc)
> > > samba: EOF on stdin - PID 2302 terminating
> > > time: 2017-07-03 13:27:14.000000Z
> > > progress: pop
> > > command: /root/samba-master/python/samba/tests/samba_tool/edit.sh
> > > $SERVER $USERNAME $PASSWORD 2>&1
> > > | /root/samba-master/selftest/filter-subunit --fail-on-empty
> > > --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> > > expanded
> > > command: /root/samba-master/python/samba/tests/samba_tool/edit.sh
> > > dc7 Administrator locDCpass7 2>&1
> > > | /root/samba-master/selftest/filter-subunit --fail-on-empty
> > > --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
> > > testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [ Exit
> > > code was 1
> > >
> > > ]
> > >
> > > samba child process 2302 exited with value 0
> > > samba: EOF on stdin - PID 2234 terminating
> > > SAMBA LOG of: DC7 pid 2302
> > > samba: EOF on stdin - PID 2302 terminating
> > > TOP 10 slowest tests
> > > samba.tests.samba_tool.edit(fl2008r2dc) -> 0
> > > 'testonly' finished successfully (29.037s)
> > >
> > > The script creates the user, modifies the user and then deletes
> > > the user, the script then exits with the exit code '0', but there
> > > is this:
> > >
> > > testsuite-failure: samba.tests.samba_tool.edit(fl2008r2dc) [
> > > Exit code was 1
> > >
> > > ]
> > >
> > > Is this something to worry about ?
> > > Where does the '1' exit code come from ?
> > I think the script 'edit.sh' needs to return its output in a format
> > expected by selftest/filter-subunit which is a normal subunit
> > format.
> >
> > source3/script/tests/test_preserve_case.sh is one of examples on how
> > we do that in shell.
> >
>
> Thanks, that helps a lot ;-)
>
> Just need to rewrite my bash script using that as a template.
>
> Rowland
>
>

OK, does this mean what I think it does:

testsuite: samba.tests.samba_tool.edit(fl2008r2dc)
progress: push
time: 2017-07-03 15:47:59.000000Z
time: 2017-07-03 14:47:59.339430Z
test: samba.tests.samba_tool.edit.Create_User(fl2008r2dc)
time: 2017-07-03 14:47:59.619598Z
successful: samba.tests.samba_tool.edit.Create_User(fl2008r2dc)
time: 2017-07-03 14:47:59.629681Z
test: samba.tests.samba_tool.edit.Edit_User(fl2008r2dc)
time: 2017-07-03 14:47:59.817088Z
successful: samba.tests.samba_tool.edit.Edit_User(fl2008r2dc)
time: 2017-07-03 14:47:59.825323Z
test: samba.tests.samba_tool.edit.Delete_User(fl2008r2dc)
time: 2017-07-03 14:48:00.009445Z
teardown_env(fl2008r2dc)
samba: EOF on stdin - PID 4836 terminating
successful: samba.tests.samba_tool.edit.Delete_User(fl2008r2dc)
time: 2017-07-03 15:48:00.000000Z
progress: pop
command: /root/samba-master/python/samba/tests/samba_tool/edit.sh $SERVER $USERNAME $PASSWORD 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
expanded command: /root/samba-master/python/samba/tests/samba_tool/edit.sh dc7 Administrator locDCpass7 2>&1  | /root/samba-master/selftest/filter-subunit --fail-on-empty --prefix="samba.tests.samba_tool.edit." --suffix="(fl2008r2dc)"
testsuite-success: samba.tests.samba_tool.edit(fl2008r2dc)

samba child process 4836 exited with value 0
samba: EOF on stdin - PID 4767 terminating
SAMBA LOG of: DC7 pid 4836
samba: EOF on stdin - PID 4836 terminating
TOP 10 slowest tests
samba.tests.samba_tool.edit(fl2008r2dc) -> 1
'testonly' finished successfully (29.323s)

If it does, I will produce new patches ;-)

Rowland

12
Loading...