Re: A more permanent home for the add-a-password-to-a-cached-username script? (was: Re: using svn cli with --non-interactive (in scripts) securely, without exposing password)

[Daniel Sahlberg <da...@gmail.com>]

Coming back to a thread from long time ago, with the intention to commit
the script in tools/client-side so we can link this from the FAQ based on a
recent question on IRC [1], [2], [3]

[1] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-01
[2] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-04
[3] https://colabti.org/irclogger/irclogger_log/svn-dev?date=2022-07-07

More below...

Den tis 9 mars 2021 kl 21:09 skrev Daniel Shahaf <d....@daniel.shahaf.name>:

> Daniel Sahlberg wrote on Mon, Mar 08, 2021 at 09:15:21 +0100:
> > Attaching version 5, with changes as below.
> >
> > Den sön 7 mars 2021 kl 20:42 skrev Daniel Shahaf <d.s@daniel.shahaf.name
> >:
> >
> > > Daniel Sahlberg wrote on Sun, Mar 07, 2021 at 19:34:15 +0100:
> > > > +    # Parse arguments
> > > > +    import argparse
> > > > +    parser = argparse.ArgumentParser(
> > > ⋮
> > > > +    # The file name is the md5encoding of the realm
> > > > +    import hashlib
> > > > +    m = hashlib.md5()
> > >
> > > Hmm.  Should all function-local imports be grouped at the top of the
> > > function?  I don't remember the convention here.
>

[... long discussion about coding style with the conclusion that even
though PEP-8 says all import should be in the top of the file,
CPython's own source code contains function level imports. And it would be
more PEP-8-ish to have function level imports at the top of the function
...]

I've changed it this way and sorted them alphabetically.

> (Offtopic: When reviewing /trunk/doc, I had a look at
> > /trunk/doc/programmer/WritingChangeLogs.txt. Is it still relevant? It
> seems
> > very CVS-ish and mostly covered in HACKING).
>
> Everything up to the last two section headers is good advice for writing
> log messages.  For those parts, they should be added to HACKING or
> referenced therefrom.
>
> The remaining two sections don't seem applicable to us at all, but as
> the file is "an essay by jimb", I don't think we can change it.
>

I was more thinking about removing it if it doesn't apply to the Subversion
project (anymore).

> +++ contrib/client-side/store-plaintext-password.py   (working copy)
> > @@ -0,0 +1,189 @@
> > +def _read_one_datum(fd, letter):
> ⋮
> > +    # Read the letter and the space
> > +    if fd.read(1) != letter or fd.read(1) != b' ':
> > +        raise ...
>
> (Raising ellipses is discussed elsethread.)
>

Now raising a ValueError(). I'm not experienced in Python but it looked
like the most suitable (standard) exception class.


>
> > +def outputHash(fd, hash):
> > +    """\
> > +    Write a dictionary to an FD in the same format as used by
>
> Say the formal parameter's name somewhere in here?
>
> Calling it "hash" could be confusing, because svn_hash_write2() and
> hashlib use that term in different senses, but I don't have a suggestion
> about this.
>

Renamed the parameter to dict and added the reference in the docstring.


> > +def writeHashFile(filename, hash):
> > +    """\
> > +    Open a temporary file and write the dictionary to the temp
> > +    file in svn_hash_write2() format.
> > +
> > +    If the file is successfully written it is renamed to <filename>.
>
> Please have all docstrings use the same markup for formal parameter
> names.  outputHash() above uses the "capitalize the name" convention the
> C code uses for non-public-API function.  I'll also use that for the
> example I write in the next paragraph.
>
> Describe the contract, not the implementation.  For instance, "Write the
> dictionary HASH to a file named FILENAME in svn_hash_write2() format."
> would be a reasonable first sentence.  The docstring could then promise
> a postcondition that if the file exists upon return, then it's not
> truncated, mention what happens if at entry FILENAME is a regular file
> or an unbroken symlink, etc..
>

Docstring rewritten.


>
> > +    """
> > +    tmpFilename = filename + '.tmp'
> > +    try:
> > +        with open(tmpFilename, 'xb') as fd:
> > +            outputHash(fd, hash)
> > +            os.rename(tmpFilename, filename)
> > +    except FileExistsError:
> > +        print('{}: File {!r} already exist. Is the script already
> running?\n'.format(argv[0], tmpFilename), file=sys.stderr)
>
> s/the/this/
>
> Wrap to 80 columns.
>

done


>
> > +    except:
> > +        os.remove(tmpFilename)
> > +        raise
> > +
> > +def main():
> > +    # Parse arguments
> > +    import argparse
> > +    parser = argparse.ArgumentParser(
> > +        description=PARSERDESCR,
> > +        formatter_class=argparse.RawDescriptionHelpFormatter)
> > +    parser.add_argument('realm', help='Server authentication real')
> > +    parser.add_argument('-u', '--user', help='Set username', nargs='?')
>
> Just delete nargs entirely, for reasons given upthread?  Sorry for
> missing that in my reviews between then and now.
>

done


>
> > +    args = parser.parse_args()
> > +
> > +    # The file name is the md5encoding of the realm
> > +    import hashlib
> > +    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
> > +    existingFile = os.path.exists(authfileName)
> > +    if not existingFile and args.user is None:
> > +        print('{}: New file, username required (see
> -u)\n'.print(argv[0]), file=sys.stderr)
>
> The phrase "new file" in the comment isn't clear, probably because it
> isn't a full sentence.
>
> The phrase "New file" in the error message has the same problem, as well
> as being phrased in implementation terms rather than in user-facing
> terms.
>

Rephrased, and also added an interactive prompt for the username if missing.


>
> > +        parser.print_help()
> > +        return
>
> The exit code should be non-zero whenever an error was printed to
> stderr.
>
> Aren't you reimplementing one of
> https://docs.python.org/3/library/argparse.html#exiting-methods
> ?
>

Changed to parser.exit()

I took the courage to commit as r1902590 and we can work any changes from
there.

Kind regards,
Daniel