Patch for samba-tool user modify subcommand

[Jelmer Vernooij]

Hi,

This patch looks good overall. I have some small comments (added
inline), and as was suggested elsewhere in this thread, it would be
really nice to have tests for this.

On Sun, Jul 22, 2012 at 10:41:03PM +0200, Gémes Géza wrote:
> diff --git a/source4/scripting/python/samba/netcmd/user.py b/source4/scripting/python/samba/netcmd/user.py
> index 6ba6150..91db85e 100644
> --- a/source4/scripting/python/samba/netcmd/user.py
> +++ b/source4/scripting/python/samba/netcmd/user.pya
> @@ -546,6 +546,122 @@ Example3 shows how an administrator would reset TestUser3 user's password to pas
>          self.outf.write("Changed password OK\n")
>  
>  
> +class cmd_user_modify(Command):
> +    """Modifies different attributes of the user
> +
> +This command modifies some properties of an existing user account in the Active Directory domain.  The username specified on the command is the sAMaccountName.
Please keep all lines < 80 characters.

> +        lp = sambaopts.get_loadparm()
> +        creds = credopts.get_credentials(lp)
> +        attributes = {}
> +        attributes["sn"] = surname
> +        attributes["givenName"] = given_name
> +        attributes["initials"] = initials
> +        attributes["profilePath"] = profile_path
> +        attributes["scriptPath"] =  script_path
> +        attributes["homeDrive"] =  home_drive
> +        attributes["homeDirectory"] =  home_directory
> +        attributes["title"] =  job_title
> +        attributes["department"] =  department
> +        attributes["company"] =  company
> +        attributes["description"] =  description
> +        attributes["mail"] =  mail_address
> +        attributes["wWWHomePage"] =  internet_address
> +        attributes["telephoneNumber"] =  telephone_number
> +        attributes["physicalDeliveryOfficeName"] =  physical_delivery_office
It would be more readable and less redundant to write this as:
         
		  attributes = {
		       "description": description,
			   "mail": mail_address,
			   ...

> +
> +        try:
> +            samdb = SamDB(url=H, session_info=system_session(),
> +                          credentials=creds, lp=lp)
> +            domain_dn = samdb.domain_dn()
> +            user_dn = samdb.search(base=domain_dn, scope=ldb.SCOPE_SUBTREE,
> +                                  expression=("(&(objectClass=user)(sAMAccountName=%s))"
> +                                  % (ldb.binary_encode(username))),
> +                                  attrs=['dn'])
> +            m = ldb.Message()
> +            m.dn = user_dn[0]['dn']
> +            for attr_name in attributes.keys():

> +				if attributes[attr_name] is not None:
> +					if attributes[attr_name] != "":

Please don't use tab characters but spaces only (see the coding style
document). This is also enforced by the source code tests.

> +            samdb.modify(m)
> +        except Exception, e:
> +            raise CommandError("Failed to modify user '%s': " % username, e)
Please don't use try/except blocks for catching generic errors -
rather, catch exception you are actually expecting (user errors). If
there is a programmer error we want to get the original backtrace, rather than
the command error.

In this particular case, I imagine you would want to just catch
LdbErrors thrown by samdb.modify().

Cheers,

Jelmer