Daniel Sahlberg wrote on Fri, Feb 26, 2021 at 13:49:25 +0100:
> +++ contrib/client-side/store-plaintext-password.py (working copy)
> @@ -0,0 +1,127 @@
> +import hashlib
> +import argparse
> +import os
> +import getpass
> +import sys
For bonus points, sort these. PEP 8 doesn't seem to mention this.
> +def readHashFile(filename):
> + """Read a hashfile as written by svn_hash_write2() to a list of tuples
> (key, value)"""
This returns a dict, not a list of tuples.
> + hash = {}
> + with open(authfileName, 'r', encoding='utf-8') as file:
> + while True:
> + # Expecting K [length] or END
> + line = file.readline()
> + if not line:
> + raise Exception('Parse failed, expected K [length] or END,
> got EOF')
Rule of thumb: every error message should contain at least one
replaceable. (Why? To identify the context, the data at fault, etc.)
In this case, it might be easiest to wrap the whole block with
a try/except that just adds the filename to the error message.
Which reminds me: in idiomatic Python, functions are passed open
file-like objects, rather than filenames. This way any file-like object
can be passed (e.g., those returned by gzip.open() or
urllib2.urlopen()). Not asking to change this; just FYI.
> + line = line.strip()
> + if line.strip() == 'END':
> + if file.readline():
> + raise Exception('Parse failed, unexpected data after
> END')
> + return hash
> + elif line[:1] != 'K':
There's line.startswith('K'), but up to you whether to change it.
> + # Read value
> + value = file.readline()
> + if not value:
> + raise Exception('Parse failed, expected value')
> + value = value.strip()
s/strip()/rstrip('\n')/ to allow passwords with trailing whitespace?
> + if len(value.encode('utf-8')) != int(line[2:]):
> + raise Exception('Parse failed, invalid value length {}
> expected {}'.format(len(value.encode('utf-8')), line[2:]))
OK: This length check will cause the script to abort if someone has
a password that ends with whitespace, or a custom key whose value isn't
just one line, and so on.
I'm not sure file.readline().encode('utf-8') is guaranteed to be the
same number of octets as there are octets in the file between the two
successive newlines (even assuming there isn't any leading/trailing
whitespace). This could happen in two cases: if .decode()/.encode() do
silent normalization (to NFC or to NFD), or if there are two different
sequences of octets that UTF-8 maps to the same codepoint.
> + # Store
> + hash[key] = value
> +
> +def writeHashFile(filename, hash):
> + """Write a list of tuples (key, value) to a hashfile of the same format
> as svn_hash_write2()"""
Same issue with the docstring.
> + with open(filename + '.tmp', 'x', encoding='utf-8') as file:
See my previous mail about adding 'w' and ensuring CR isn't emitted on
Windows.
> +# Parse arguments
> +parser = argparse.ArgumentParser(description='''Store plain-text password in
> ~/.subversion/auth/svn.simple/\n\n
> +Existing passwords and authentication realms can be inspected by:\n\n
> + svn auth [--show-passwords]\n\n
> +The authentication realm can also be found using:\n\n
> + svn info URL\n\n
> +Realm will be read from stdin if not provided on the command line.''',
> formatter_class=argparse.RawDescriptionHelpFormatter)
Don't embed multiline string literals this way. Either make it a global
variable, or indent the named actual parameters conventionally:
parser = argparse.ArgumentParser(
description='''\
Store plain-text password in ~/.subversion/auth/svn.simple/
⋮
Realm will be read from stdin if not provided on the command line.
''',
formatter_class=argparse.RawDescriptionHelpFormatter,
)
I'm not sure whether you can indent the string literal's contents there
and still have it output as desired.
Note you have three \n's between every two lines (two escaped and one
literal).
> +parser.add_argument('realm', help='Server authentication real')
> +parser.add_argument('-u', '--user', help='Set username', nargs='?')
> +args = parser.parse_args()
> +
> +# The file name is the md5encoding of the realm
> +m = hashlib.md5()
> +m.update(args.realm.encode('utf-8'))
> +authfileName =
> os.path.join(os.path.expanduser('~/.subversion/auth/svn.simple/'),
> m.hexdigest())
> +
> +# If new file, check that username was given as an argument before prompting
> for password
> +if not os.path.isfile(authfileName) and args.user is None:
«not isfile()» could mean it's a directory or a broken symlink. How
about:
.
if isfile():
elif not exists():
else:
.
?
(The point below about stat() applies to this pseudocode too.)
> + print('New file, username required (see -u)\n', file=sys.stderr)
Error messages should start with the argv[0] of whoever generated them.
> + parser.print_help()
> + quit()
> +
> +# Prompt for password
> +password = getpass.getpass("Enter password: ")
How about "Enter password for {realm}" or "for {user}", etc?
> +# In an existing file, we add/replace password/username/passtype
> +if os.path.isfile(authfileName):
Calling stat() twice has a performance impact and looks like a race
condition. Maybe in this case it doesn't matter, though…
> + hash = readHashFile(authfileName)
> + if args.user is not None:
> + hash['username'] = args.user
> + hash['password'] = password
> + hash['passtype'] = 'simple'
> +
> +# For a new file, set realmstring, username, password and passtype
> +else:
> + hash = {}
> + hash['svn:realmstring'] = args.realm
> + hash['username'] = args.user
> + hash['passtype'] = 'simple'
> + hash['password'] = password
Style: Use a single, multiline initializer rather than multiple
assignments?
Forward cribbing compatibility: «del password» once it's no longer needed?
> +
> +# Write out the resulting file
> +writeHashFile(authfileName, hash)
