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

Tue Jul 4 15:08:08 UTC 2017

:-)
I know this doesn't count as review but LGTM.
Always wondered how I'd have felt saying it ;-)

Daniele.

On mar, 2017-07-04 at 15:59 +0100, Rowland Penny via samba-technical
wrote:
> On Tue, 04 Jul 2017 16:49:20 +0200
> Daniele Dario <d.dario76 at gmail.com> wrote:
> 
> > Hi Rowland,
> > 
> > 
> > On mar, 2017-07-04 at 15:17 +0100, Rowland Penny via samba-technical
> > wrote:
> > > On Tue, 04 Jul 2017 15:25:50 +0200
> > > Daniele Dario <d.dario76 at gmail.com> wrote:
> > > 
> > > > 
> > > > 
> > > > 
> > > > On mar, 2017-07-04 at 16:10 +0300, Alexander Bokovoy via
> > > > samba-technical wrote:
> > > > > On ti, 04 heinä 2017, Rowland Penny via samba-technical wrote:
> > > > > > On Tue, 04 Jul 2017 14:51:39 +0200
> > > > > > Daniele Dario <d.dario76 at gmail.com> wrote:
> > > > > > 
> > > > > > > Hi Rowland,
> > > > > > > 
> > > > > > > On mar, 2017-07-04 at 13:25 +0100, Rowland Penny via
> > > > > > > samba-technical wrote:
> > > > > > > > On Tue, 4 Jul 2017 14:11:23 +0300
> > > > > > > > Alexander Bokovoy <ab at samba.org> wrote:
> > > > > > > > 
> > > > > > > > > On ti, 04 heinä 2017, Rowland Penny via samba-technical
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, 4 Jul 2017 12:39:00 +0300
> > > > > > > > > > Alexander Bokovoy <ab at samba.org> wrote:
> > > > > > > > > > 
> > > > > > > > > > > On ti, 04 heinä 2017, Rowland Penny via
> > > > > > > > > > > samba-technical wrote:
> > > > > > > > > > > > On Mon, 3 Jul 2017 18:07:49 +0300
> > > > > > > > > > > > Alexander Bokovoy <ab at samba.org> wrote:
> > > > > > > > > > > > 
> > > > > > > > > > > > > Looks like it works! Great!
> > > > > > > > > > > > 
> > > > > > > > > > > > OK, one step forward, three steps back ;-)
> > > > > > > > > > > > 
> > > > > > > > > > > > How do you create a new file ?
> > > > > > > > > > > > 
> > > > > > > > > > > > I have created the edit.sh file in my local git,
> > > > > > > > > > > > but when I run:
> > > > > > > > > > > > 
> > > > > > > > > > > > git commit -s -a
> > > > > > > > > > > > 
> > > > > > > > > > > > I get:
> > > > > > > > > > > > 
> > > > > > > > > > > > On branch master
> > > > > > > > > > > > Your branch is ahead of 'origin/master' by 1
> > > > > > > > > > > > commit. (use "git push" to publish your local
> > > > > > > > > > > > commits) Untracked files:
> > > > > > > > > > > > 	python/samba/tests/samba_tool/edit.sh
> > > > > > > > > > > > 
> > > > > > > > > > > > nothing added to commit but untracked files
> > > > > > > > > > > > present
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't think I should run 'git push' as, from
> > > > > > > > > > > > reading the manpage, it looks like this will try
> > > > > > > > > > > > to create 'edit.sh' in the Samba git.
> > > > > > > > > > > git add <file>
> > > > > > > > > > > https://www.atlassian.com/git/tutorials/saving-changes
> > > > > > > > > > 
> > > > > > > > > > I did wonder about that, but 'man git commit' said
> > > > > > > > > > this:
> > > > > > > > > > 
> > > > > > > > > >        The command git commit -a first looks at your
> > > > > > > > > > working tree, notices that you have modified hello.c
> > > > > > > > > > and removed goodbye.c, and performs necessary git add
> > > > > > > > > > and git rm for you.
> > > > > > > > > > 
> > > > > > > > > > Looks like the manpage lies, because I was using '-a'
> > > > > > > > > > and it didn't add the file ;-)
> > > > > > > > > > 
> > > > > > > > > > I followed the suggestion and it worked, so I have
> > > > > > > > > > attached my new patches for editing users and the test
> > > > > > > > > > for this.
> > > > > > > > > Ok, I have only one comment I'd like to be addressed
> > > > > > > > > before we accept this changeset:
> > > > > > > > > 
> > > > > > > > > +# create editor.sh
> > > > > > > > > +echo "#!/usr/bin/env bash" > /tmp/editor.sh
> > > > > > > > > +echo "user_ldif=\"\$1\"" >> /tmp/editor.sh
> > > > > > > > > +echo "SED=\$(which sed)" >> /tmp/editor.sh
> > > > > > > > > +echo "\$SED -i -e 's/userAccountControl:
> > > > > > > > > 512/userAccountControl: 514/' \$user_ldif"
> > > > > > > > > >> /tmp/editor.sh + +chmod +x /tmp/editor.sh
> > > > > > > > > +
> > > > > > > > > 
> > > > > > > > > Please don't use a static path here. It would prevent
> > > > > > > > > multiple parallel 'make test' runs on the same machine
> > > > > > > > > by different users. And also would allow an attacker
> > > > > > > > > (whoever that be) to rewrite your editor.sh and inject
> > > > > > > > > an execution into your 'make test' run.
> > > > > > > > > 
> > > > > > > > > Instead, create a temporary file:
> > > > > > > > > 
> > > > > > > > > tmpeditor=$(mktemp --suffix .sh
> > > > > > > > > samba-tool-editor-XXXXXXXX)
> > > > > > > > > 
> > > > > > > > > And put the content into it:
> > > > > > > > > 
> > > > > > > > > cat >$tmpeditor <<-'EOF'
> > > > > > > > > 	#!/usr/bin/env bash
> > > > > > > > > 	user_ldif="$1"
> > > > > > > > > 	SED=$(which sed)
> > > > > > > > > 	$SED -i -e 's/userAccountControl:
> > > > > > > > > 512/userAccountControl: 514/' $user_ldif EOF
> > > > > > > > > 
> > > > > > > > > chmod +x $tmpeditor
> > > > > > > > > 
> > > > > > > > > <<-'EOF' means to start here document which ends with
> > > > > > > > > EOF, strip leading tabs, do not perform interpolation
> > > > > > > > > within the document. This allows you to keep the
> > > > > > > > > content of the script clean from shell expansion
> > > > > > > > > protection.
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > OK, here are the new patches ;-)
> > > > > > > > 
> > > > > > > > Rowland
> > > > > > > > 
> > > > > > > 
> > > > > > > I'm not sure but you create the edit.sh script as
> > > > > > > 
> > > > > > > +# create editor.sh
> > > > > > > +tmpeditor=$(mktemp --suffix .sh samba-tool-editor-XXXXXXXX)
> > > > > > > +
> > > > > > > +cat >$tmpeditor <<-'EOF'
> > > > > > > +#!/usr/bin/env bash
> > > > > > > +user_ldif="$1"
> > > > > > > +SED=$(which sed)
> > > > > > > +$SED -i -e 's/userAccountControl: 512/userAccountControl:
> > > > > > > 514/' $user_ldif
> > > > > > > +EOF
> > > > > > > +
> > > > > > > +chmod +x $tmpeditor
> > > > > > > 
> > > > > > > but when you run the test you do
> > > > > > > 
> > > > > > > +# Edit test user
> > > > > > > +subunit_start_test "Edit_User"
> > > > > > > +output=$(${STpath}/source4/scripting/bin/samba-tool user
> > > > > > > edit sambatool1 --editor=/tmp/editor.sh \
> > > > > > > +-H "ldap://$SERVER" "-U$USERNAME" "--password=$PASSWORD")
> > > > > > > 
> > > > > > > Isn't it wrong the --editor part?
> > > > > > > 
> > > > > > > I'd say it would be --editor=$tmpeditor
> > > > > > > 
> > > > > > > Daniele.
> > > > > > > 
> > > > > > 
> > > > > > Note to self: must engage brain ;-)
> > > > > > 
> > > > > > Yes you are probably correct.
> > > > > > I say probably, because I didn't change that line and it
> > > > > > worked, I have just changed it and now it doesn't work.
> > > > > > 
> > > > > > I will try and see why it doesn't work and get back with V4
> > > > > > patches :-(
> > > > > That's one thing I didn't notice, so good to have multiple
> > > > > reviewers. Please also add a code to remove a temporary file at
> > > > > the end of the script.
> > > > > 
> > > > 
> > > > And maybe it worked because you didn't remove /tmp/editor.sh and
> > > > the test was still using that one.
> > > > 
> > > > Daniele.
> > > > 
> > > 
> > > Yes, the old /tmp/editor was still there, so I removed it and also
> > > made the script remove it.
> > > The 'tmpeditor' must be in a fixed place or 'samba-tool used edit'
> > > cannot find it. I suppose I could have put it into somewhere
> > > like /bin or /sbin, but /tmp seems a better place for something
> > > that is temporary ;-)
> > > 
> > > Anyway, here are the version 4 patches.
> > > 
> > > Rowland
> > >  
> > > 
> > 
> > Maybe I have totally misunderstand the goal: you create a 
> > bash script to replace a piece of something and this file ends up to
> > be created in $tmpeditor (see below).
> > 
> > [daniele at dakota:~]$ tmpeditor="/tmp/$(mktemp --suffix .sh
> > samba-tool-editor-XXXXXXXX)"
> > [daniele at dakota:~]$ echo $tmpeditor
> > /tmp/samba-tool-editor-9w3l3NVU.sh
> > 
> > I thought this was the script you needed to pass in to samba-tool
> > using the --editor command but from the second patch I still see
> > you're using --editor=/tmp/editor.sh
> > 
> > Am I totally on the wrong way?
> > 
> > Daniele.
> > 
> 
> AAAARRRHGGGGG, don't know how that happened ;-)
> 
> See attached replacement patch
> 
> Rowland
> 