Discussion:
[PATCH] Allow editing groups from samba-tool
William Brown via samba-technical
2018-04-30 02:34:26 UTC
Permalink
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.

Thanks for your review!

William
Alexander Bokovoy via samba-technical
2018-04-30 05:32:14 UTC
Permalink
Post by William Brown via samba-technical
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.
Post by William Brown via samba-technical
Thanks for your review!
William
From 9961efc5f599d043479d2376c6d96a378302f57f Mon Sep 17 00:00:00 2001
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
samba-tool group edit ...
samba-tool user edit ...
---
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
+
pass
return logger
+ """A specialise variant of Command for DS object editing."""
+
+ def _run_edit(self, filt, objectname, objecttype, credopts=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()
+
+ res = samdb.search(base=domaindn,
+ expression=filt,
+ scope=ldb.SCOPE_SUBTREE)
+ object_dn = res[0].dn
+ raise CommandError('Unable to find %s "%s"' %
+ (objecttype.lower(), objectname))
+
+ 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)
+
+ editor = os.environ.get('EDITOR')
+ editor = 'vi'
+
+ # Assert the editor chosen actually exists!
+ 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.
Post by William Brown via samba-technical
+
+ t_file.write(result_ldif)
+ t_file.flush()
+ check_call([editor, t_file.name])
+ raise CalledProcessError("ERROR: ", e)
+ edited_message = edited_file.read()
+
+ diff = difflib.ndiff(result_ldif.splitlines(),
+ edited_message.splitlines())
+ minus_lines = []
+ plus_lines = []
+ line = line[2:]
+ minus_lines.append(line)
+ line = line[2:]
+ plus_lines.append(line)
+
+ object_ldif="dn: %s\n" % object_dn
+ object_ldif += "changetype: modify\n"
+
+ attr, val = line.split(':', 1)
+ search_attr="%s:" % attr
+ object_ldif += "delete: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+
+ attr, val = line.split(':', 1)
+ search_attr="%s:" % attr
+ object_ldif += "replace: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+ object_ldif += "add: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+
+ samdb.modify_ldif(object_ldif)
+ raise CommandError("Failed to modify %s '%s': " %
+ (objecttype.lower(), objectname), e)
+
+ self.outf.write("Modified %s '%s' successfully\n" %
+ (objecttype, objectname))
+
+
+
+
"""A samba-tool command with subcommands."""
diff --git a/python/samba/netcmd/group.py b/python/samba/netcmd/group.py
index a4969cc6ba9..c2713e1879a 100644
--- a/python/samba/netcmd/group.py
+++ b/python/samba/netcmd/group.py
@@ -17,7 +17,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import samba.getopt as options
-from samba.netcmd import Command, SuperCommand, CommandError, Option
+from samba.netcmd import Command, EditCommand, SuperCommand, CommandError, Option
import ldb
from samba.ndr import ndr_unpack
from samba.dcerpc import security
self.outf.write('Moved group "%s" into "%s"\n' %
(groupname, full_new_parent_dn))
+ """Modify Group AD object.
+
+This command will allow editing of a group object in the Active Directory
+domain. You will then be able to add or change attributes and their values.
+
+The groupname 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.
+
+samba-tool user edit Group1 -H ldap://samba.samdom.example.com \
+-U administrator --password=passw1rd
+
+Example1 shows how to edit a group's attributes in the domain against a remote
+LDAP server.
+
+The -H parameter is used to specify the remote target server.
+
+samba-tool user edit Group2
+
+Example2 shows how to edit a group's attributes in the domain against a local
+LDAP server.
+
+samba-tool user edit Group3 --editor=nano
+
+Example3 shows how to edit a group's attributes in the domain against a local
+LDAP server using the 'nano' editor.
+
+"""
+ synopsis = "%prog <groupname> [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 = ["groupname"]
+ takes_optiongroups = {
+ "sambaopts": options.SambaOptions,
+ "credopts": options.CredentialsOptions,
+ "versionopts": options.VersionOptions,
+ }
+
+ def run(self, groupname, credopts=None, sambaopts=None, versionopts=None,
+
+ filt = ("(&(sAMAccountName=%s)(objectClass=group))" %
+ ldb.binary_encode(groupname))
+
+ self._run_edit(filt, groupname, 'Group', credopts, sambaopts, versionopts,
+ H, editor)
+
+
"""Group management."""
subcommands["list"] = cmd_group_list()
subcommands["listmembers"] = cmd_group_list_members()
subcommands["move"] = cmd_group_move()
+ subcommands["edit"] = cmd_group_edit()
diff --git a/python/samba/netcmd/user.py b/python/samba/netcmd/user.py
index dfe167d8c7d..89bae061c2b 100644
--- a/python/samba/netcmd/user.py
+++ b/python/samba/netcmd/user.py
@@ -51,6 +51,7 @@ from samba.net import Net
from samba.netcmd import (
Command,
CommandError,
+ EditCommand,
SuperCommand,
Option,
)
@@ -2294,7 +2295,7 @@ samba-tool user syncpasswords --terminate \\
update_pid(None)
return
"""Modify User AD object.
This command will allow editing of a user account in the Active Directory
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
+Example1 shows how to edit a user's attributes in the domain against a remote
LDAP server.
The -H parameter is used to specify the remote target server.
@@ -2319,13 +2320,13 @@ The -H parameter is used to specify the remote target server.
samba-tool user edit User2
-Example2 shows how to edit a users attributes in the domain against a local
+Example2 shows how to edit a user's attributes in the domain against a local
LDAP server.
samba-tool user edit User3 --editor=nano
-Example3 shows how to edit a users attributes in the domain against a local
+Example3 shows how to edit a user's attributes in the domain against a local
LDAP server using the 'nano' editor.
"""
@@ -2348,84 +2349,12 @@ LDAP server using the 'nano' editor.
def run(self, username, credopts=None, sambaopts=None, versionopts=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=%d)(sAMAccountName=%s))" %
+ filt = ("(&(sAMAccountType=%d)(sAMAccountName=%s))" %
(dsdb.ATYPE_NORMAL_ACCOUNT, ldb.binary_encode(username)))
- domaindn = samdb.domain_dn()
-
- res = samdb.search(base=domaindn,
- expression=filter,
- scope=ldb.SCOPE_SUBTREE)
- user_dn = res[0].dn
- raise CommandError('Unable to find user "%s"' % (username))
-
- r_ldif = samdb.write_ldif(msg, 1)
- # remove 'changetype' line
- result_ldif = re.sub('changetype: add\n', '', r_ldif)
-
- editor = os.environ.get('EDITOR')
- editor = 'vi'
-
- t_file.write(result_ldif)
- t_file.flush()
- check_call([editor, t_file.name])
- raise CalledProcessError("ERROR: ", e)
- edited_message = edited_file.read()
-
- diff = difflib.ndiff(result_ldif.splitlines(),
- edited_message.splitlines())
- minus_lines = []
- plus_lines = []
- line = line[2:]
- minus_lines.append(line)
- line = line[2:]
- plus_lines.append(line)
-
- user_ldif="dn: %s\n" % user_dn
- user_ldif += "changetype: modify\n"
-
- attr, val = line.split(':', 1)
- search_attr="%s:" % attr
- user_ldif += "delete: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
-
- attr, val = line.split(':', 1)
- search_attr="%s:" % attr
- user_ldif += "replace: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
- user_ldif += "add: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
-
- samdb.modify_ldif(user_ldif)
- raise CommandError("Failed to modify user '%s': " %
- username, e)
+ self._run_edit(filt, username, 'User', credopts, sambaopts, versionopts,
+ H, editor)
- self.outf.write("Modified User '%s' successfully\n" % username)
"""Display a user AD object.
--
2.14.3
--
/ Alexander Bokovoy
Rowland Penny via samba-technical
2018-04-30 06:55:25 UTC
Permalink
On Mon, 30 Apr 2018 08:32:14 +0300
Alexander Bokovoy via samba-technical <samba-***@lists.samba.org>
wrote:

See inline comments.
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
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.
Post by William Brown via samba-technical
Thanks for your review!
William
From 9961efc5f599d043479d2376c6d96a378302f57f Mon Sep 17 00:00:00
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
samba-tool group edit ...
samba-tool user edit ...
---
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 +
pass
return logger
+ """A specialise variant of Command for DS object editing."""
+
+ def _run_edit(self, filt, objectname, objecttype,
credopts=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()
+
+ res = samdb.search(base=domaindn,
+ expression=filt,
+ scope=ldb.SCOPE_SUBTREE)
+ object_dn = res[0].dn
+ raise CommandError('Unable to find %s "%s"' %
+ (objecttype.lower(), objectname))
+
+ 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)
+
+ editor = os.environ.get('EDITOR')
+ editor = 'vi'
+
+ # Assert the editor chosen actually exists!
+ 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.
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
+
+ t_file.write(result_ldif)
+ t_file.flush()
+ check_call([editor, t_file.name])
+ raise CalledProcessError("ERROR: ", e)
+ edited_message = edited_file.read()
+
+ diff = difflib.ndiff(result_ldif.splitlines(),
+ edited_message.splitlines())
+ minus_lines = []
+ plus_lines = []
+ line = line[2:]
+ minus_lines.append(line)
+ line = line[2:]
+ plus_lines.append(line)
+
+ object_ldif="dn: %s\n" % object_dn
+ object_ldif += "changetype: modify\n"
+
+ attr, val = line.split(':', 1)
+ search_attr="%s:" % attr
+ if not re.search(r'^' + search_attr,
+ object_ldif += "delete: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+
+ attr, val = line.split(':', 1)
+ search_attr="%s:" % attr
+ object_ldif += "replace: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+ if not re.search(r'^' + search_attr,
+ object_ldif += "add: %s\n" % attr
+ object_ldif += "%s: %s\n" % (attr, val)
+
+ samdb.modify_ldif(object_ldif)
+ raise CommandError("Failed to modify %s '%s': " %
+ (objecttype.lower(),
objectname), e) +
+ self.outf.write("Modified %s '%s' successfully\n" %
+ (objecttype, objectname))
+
+
+
+
"""A samba-tool command with subcommands."""
diff --git a/python/samba/netcmd/group.py
b/python/samba/netcmd/group.py index a4969cc6ba9..c2713e1879a 100644
--- a/python/samba/netcmd/group.py
+++ b/python/samba/netcmd/group.py
@@ -17,7 +17,7 @@
# along with this program. If not, see
<http://www.gnu.org/licenses/>.
import samba.getopt as options
-from samba.netcmd import Command, SuperCommand, CommandError,
Option +from samba.netcmd import Command, EditCommand,
SuperCommand, CommandError, Option import ldb
from samba.ndr import ndr_unpack
from samba.dcerpc import security
self.outf.write('Moved group "%s" into "%s"\n' %
(groupname, full_new_parent_dn))
+ """Modify Group AD object.
+
+This command will allow editing of a group object in the Active
Directory +domain. You will then be able to add or change
attributes and their values. +
+The groupname 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.
+
+samba-tool user edit Group1 -H ldap://samba.samdom.example.com \
+-U administrator --password=passw1rd
+
+Example1 shows how to edit a group's attributes in the domain
against a remote +LDAP server.
+
+The -H parameter is used to specify the remote target server.
+
+samba-tool user edit Group2
+
+Example2 shows how to edit a group's attributes in the domain
against a local +LDAP server.
+
+samba-tool user edit Group3 --editor=nano
+
+Example3 shows how to edit a group's attributes in the domain
against a local +LDAP server using the 'nano' editor.
+
+"""
+ synopsis = "%prog <groupname> [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 = ["groupname"]
+ takes_optiongroups = {
+ "sambaopts": options.SambaOptions,
+ "credopts": options.CredentialsOptions,
+ "versionopts": options.VersionOptions,
+ }
+
+ def run(self, groupname, credopts=None, sambaopts=None,
versionopts=None,
+
+ filt = ("(&(sAMAccountName=%s)(objectClass=group))" %
+ ldb.binary_encode(groupname))
+
+ self._run_edit(filt, groupname, 'Group', credopts,
sambaopts, versionopts,
+ H, editor)
+
+
"""Group management."""
subcommands["list"] = cmd_group_list()
subcommands["listmembers"] = cmd_group_list_members()
subcommands["move"] = cmd_group_move()
+ subcommands["edit"] = cmd_group_edit()
diff --git a/python/samba/netcmd/user.py
b/python/samba/netcmd/user.py index dfe167d8c7d..89bae061c2b 100644
--- a/python/samba/netcmd/user.py
+++ b/python/samba/netcmd/user.py
@@ -51,6 +51,7 @@ from samba.net import Net
from samba.netcmd import (
Command,
CommandError,
+ EditCommand,
SuperCommand,
Option,
)
@@ -2294,7 +2295,7 @@ samba-tool user syncpasswords --terminate \\
update_pid(None)
return
"""Modify User AD object.
This command will allow editing of a user account in the Active
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 +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>
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
The -H parameter is used to specify the remote target server.
@@ -2319,13 +2320,13 @@ The -H parameter is used to specify the
samba-tool user edit User2
-Example2 shows how to edit a users attributes in the domain
against a local +Example2 shows how to edit a user's attributes in
the domain against a local LDAP server.
samba-tool user edit User3 --editor=nano
-Example3 shows how to edit a users attributes in the domain
against a local +Example3 shows how to edit a user's attributes in
the domain against a local LDAP server using the 'nano' editor.
"""
@@ -2348,84 +2349,12 @@ LDAP server using the 'nano' editor.
def run(self, username, credopts=None, sambaopts=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=%d)(sAMAccountName=%s))" %
+ filt = ("(&(sAMAccountType=%d)(sAMAccountName=%s))" %
Why change 'filter' to 'filt' ? is it so that the line below is less
than 80 characters ?
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
(dsdb.ATYPE_NORMAL_ACCOUNT,
ldb.binary_encode(username)))
- domaindn = samdb.domain_dn()
-
- res = samdb.search(base=domaindn,
- expression=filter,
- scope=ldb.SCOPE_SUBTREE)
- user_dn = res[0].dn
- raise CommandError('Unable to find user "%s"' %
(username)) -
- r_ldif = samdb.write_ldif(msg, 1)
- # remove 'changetype' line
- result_ldif = re.sub('changetype: add\n', '', r_ldif)
-
- editor = os.environ.get('EDITOR')
- editor = 'vi'
-
- t_file.write(result_ldif)
- t_file.flush()
- check_call([editor, t_file.name])
- raise CalledProcessError("ERROR: ", e)
- edited_message = edited_file.read()
-
- diff = difflib.ndiff(result_ldif.splitlines(),
- edited_message.splitlines())
- minus_lines = []
- plus_lines = []
- line = line[2:]
- minus_lines.append(line)
- line = line[2:]
- plus_lines.append(line)
-
- user_ldif="dn: %s\n" % user_dn
- user_ldif += "changetype: modify\n"
-
- attr, val = line.split(':', 1)
- search_attr="%s:" % attr
- if not re.search(r'^' + search_attr,
- user_ldif += "delete: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
-
- attr, val = line.split(':', 1)
- search_attr="%s:" % attr
- user_ldif += "replace: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
- if not re.search(r'^' + search_attr,
- user_ldif += "add: %s\n" % attr
- user_ldif += "%s: %s\n" % (attr, val)
-
- samdb.modify_ldif(user_ldif)
- raise CommandError("Failed to modify user '%s': " %
- username, e)
+ self._run_edit(filt, username, 'User', credopts,
sambaopts, versionopts,
+ H, editor)
- self.outf.write("Modified User '%s' successfully\n" % username)
"""Display a user AD object.
--
2.14.3
Rowland
Alexander Bokovoy via samba-technical
2018-04-30 07:09:40 UTC
Permalink
Post by Rowland Penny via samba-technical
On Mon, 30 Apr 2018 08:32:14 +0300
See inline comments.
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
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.
Post by William Brown via samba-technical
Thanks for your review!
William
From 9961efc5f599d043479d2376c6d96a378302f57f Mon Sep 17 00:00:00
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
samba-tool group edit ...
samba-tool user edit ...
---
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 +
pass
return logger
+ """A specialise variant of Command for DS object editing."""
+
+ def _run_edit(self, filt, objectname, objecttype,
credopts=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()
+
+ res = samdb.search(base=domaindn,
+ expression=filt,
+ scope=ldb.SCOPE_SUBTREE)
+ object_dn = res[0].dn
+ raise CommandError('Unable to find %s "%s"' %
+ (objecttype.lower(), objectname))
+
+ 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)
+
+ editor = os.environ.get('EDITOR')
+ editor = 'vi'
+
+ # Assert the editor chosen actually exists!
+ 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.
Post by Rowland Penny via samba-technical
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
-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
William Brown via samba-technical
2018-04-30 22:03:56 UTC
Permalink
Post by Alexander Bokovoy via samba-technical
Post by Rowland Penny via samba-technical
Post by Alexander Bokovoy via samba-technical
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.
This is also a good point. As mentioned I'll tidy this up today and
resubmit.
Post by Alexander Bokovoy via samba-technical
Post by Rowland Penny via samba-technical
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
-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.
Your knowledge of apostrophes is correct here Alex. This is why I made
the subtle change to "user's" because the attributes belong to the
user, we don't have multiple users to modify.

It's important that we take pride in all parts of our work from the
grammar of our documentation to the whitespacing and formatting of our
code patches. We have code review, so we should have "English review"
also. :)

Thanks,

William
William Brown via samba-technical
2018-04-30 23:05:50 UTC
Permalink
On Mon, 2018-04-30 at 08:32 +0300, Alexander Bokovoy via samba-
Post by Alexander Bokovoy via samba-technical
Post by William Brown via samba-technical
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.
I have addressed your comment in this updated patch. Thanks for
spotting the issue!

Thanks,

William

Loading...