Enhancement for the contrib/htpasswd.py script

[thomasbilk@…]

I have updated the htpasswd.py from contrib.

[thomasbilk@…]

Patch for htpasswd.py

[Ryan J Ollos <ryan.j.ollos@…>]

It would be helpful if you described how you've updated and enhanced the script. That is generally considered good practice when creating a ticket and submitting a patch.

[thomasbilk@…]

Oh yes. Sorry. So what I did was that:

• Improve the salt method by adding an optional parameter length and using two constants from the string package
• Improve the load and save methods of the HtpasswdFile class by using the with statement for proper closing of the file
• Rename the -b parameter to -a for adding new users to the file and changing the deletion of users to -d (lower case)
• Improved the adding of users by prompting for the password in a save way instead of reading it as a command line arg

[Ryan J Ollos]

Replying to thomasbilk@…:

Oh yes. Sorry. So what I did was that:

• Improve the salt method by adding an optional parameter length and using two constants from the string package
• Improve the load and save methods of the HtpasswdFile class by using the with statement for proper closing of the file

Those changes look good.

• Rename the -b parameter to -a for adding new users to the file and changing the deletion of users to -d (lower case)

Those options mirror the options for the htpasswd command line tool, so we should keep them as-is for consistency.

$ htpasswd
Usage:
	htpasswd [-cimBdpsDv] [-C cost] passwordfile username
	htpasswd -b[cmBdpsDv] [-C cost] passwordfile username password

	htpasswd -n[imBdps] [-C cost] username
	htpasswd -nb[mBdps] [-C cost] username password
 -c  Create a new file.
 -n  Don't update file; display results on stdout.
 -b  Use the password from the command line rather than prompting for it.
 -i  Read password from stdin without verification (for script usage).
 -m  Force MD5 encryption of the password (default).
 -B  Force bcrypt encryption of the password (very secure).
 -C  Set the computing time used for the bcrypt algorithm
     (higher is more secure but slower, default: 5, valid: 4 to 31).
 -d  Force CRYPT encryption of the password (8 chars max, insecure).
 -s  Force SHA encryption of the password (insecure).
 -p  Do not encrypt the password (plaintext, insecure).
 -D  Delete the specified user.
 -v  Verify password for the specified user.
On other systems than Windows and NetWare the '-p' flag will probably not work.
The SHA algorithm does not use a salt and is less secure than the MD5 algorithm.

• Improved the adding of users by prompting for the password in a save way instead of reading it as a command line arg

It could be okay to prompt for a password when the user hasn't specified it.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t10975. The changes make htpasswd.py behave more like htpasswd.

[Ryan J Ollos]

I'll add unit tests before committing the changes.

[Ryan J Ollos]

Replying to rjollos:

I'll add unit tests before committing the changes.

It would be nice to have tests since I've already discovered a minor issue with the proposed changes. Also, I'm considering some refactoring of htdigest.py. However, should the tests in contrib be invoked from trac.test? contrib is not a package and I haven't explored whether that will effect the ability to easily write a tests package in contrib/tests. Or perhaps it makes more sense to just have test modules in a tests directory, rather than a tests package.

A more drastic approach would be to move htpasswd.py and htdigest.py from contrib into the trac package.

[Ryan J Ollos]

Replying to rjollos:

A more drastic approach would be to move htpasswd.py and htdigest.py from contrib into the trac package.

Perhaps trac.auth could be a new package? I'm considering integrating some ​AccountManagerPlugin functionality in Trac 1.3.x, and will think about revisiting this.

For now, latest proposed changes in log:rjollos.git:t10975.2.

[Jun Omae]

Undefined name 'self' is used in trac/util/compat.py.

     except ImportError:
-        self.crypt = None
+        crypt = None

[Peter Suter]

In salt(), ascii_letters + digits does not include / and . while in the previous set 0123456789/. they were included. Was that intentional?

[Ryan J Ollos]

According to multiple sources in a Google search, including ​here, valid characters include . and /. It was an oversight that they were removed. I'll add back. Thanks for noticing.

[Ryan J Ollos]

Committed to trunk in [14014:14015] after fixing noted issues.

[Jun Omae]

Sorry, I missed the following issue. When fcrypt isn't available on Windows, tracd with basic auth option crashes.

C> python.exe -c "import fcrypt"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
ImportError: No module named fcrypt

C> python.exe trac/web/standalone.py --basic-auth="*,C:/usr/var/trac/htpasswd.txt,auth" -s C:/usr/var/trac/1.1.5dev
Traceback (most recent call last):
  File "trac/web/standalone.py", line 341, in <module>
    main()
  File "trac/web/standalone.py", line 236, in main
    options, args = parser.parse_args()
  File "C:\usr\apps\python26\lib\optparse.py", line 1394, in parse_args
    stop = self._process_args(largs, rargs, values)
  File "C:\usr\apps\python26\lib\optparse.py", line 1434, in _process_args
    self._process_long_opt(rargs, values)
  File "C:\usr\apps\python26\lib\optparse.py", line 1509, in _process_long_opt
    option.process(opt, value, values, self)
  File "C:\usr\apps\python26\lib\optparse.py", line 788, in process
    self.action, self.dest, opt, value, values, parser)
  File "C:\usr\apps\python26\lib\optparse.py", line 808, in take_action
    self.callback(self, opt, value, parser, *args, **kwargs)
  File "trac/web/standalone.py", line 136, in _auth_callback
    auths[env_name] = cls(os.path.abspath(filename), realm)
  File "C:\usr\src\trac\trac.git\trac\web\auth.py", line 303, in __init__
    self.crypt = crypt.crypt
AttributeError: 'NoneType' object has no attribute 'crypt'

I think it would be simple to import crypt method rather than crypt module.

contrib/htpasswd.py

@@ -55 +55 @@
 
     def update(self, username, password):
         """Replace the entry for the given user, or add it if new."""
-        pwhash = crypt.crypt(password, salt())
+        pwhash = crypt(password, salt())
         matching_entries = [entry for entry in self.entries
                             if entry[0] == username]
         if matching_entries:

trac/util/compat.py

@@ -25 +25 @@
 
 # Windows doesn't have a crypt module by default.
 try:
-    import crypt
+    from crypt import crypt
 except ImportError:
     try:
-        import fcrypt as crypt
+        from fcrypt import crypt
     except ImportError:
         crypt = None
 

trac/web/auth.py

@@ -300 +300 @@
     def __init__(self, htpasswd, realm):
         # FIXME pass a logger
         self.realm = realm
-        self.crypt = crypt.crypt
+        self.crypt = crypt
         PasswordFileAuthentication.__init__(self, htpasswd)
 
     def load(self, filename):

[Ryan J Ollos]

I should have tested on Windows, but I don't have easy access to a Windows system this weekend. Would you mind pushing the change?

[Jun Omae]

Okay. I've pushed the changes in [14023].