Linux

Contributing to diceware tool or 2018’s first Pull Request

Feb 20, 2018 by Bhavin Gandhi

You must have heard about diceware passwords before. If not, then the following image explains about it. It’s all about length of the password!

diceware passwords

I came across this tool called diceware which helps to generate these kind of passwords by using wordlists. Kushal packaged this for Fedora, he wanted people to test his rpm build, so he posted link to the build on #dgplug. While testing, I just passed random string to the command:

(py36env)$ diceware nonexistingfile
Traceback (most recent call last):
  File "/home/bhavin192/diceware/py36/bin/diceware", line 11, in <module>
    load_entry_point('diceware==0.9.4.dev0', 'console_scripts', 'diceware')()
  File "/home/bhavin192/diceware/diceware/__init__.py", line 211, in main
    print(get_passphrase(options))
  File "/home/bhavin192/diceware/diceware/__init__.py", line 184, in get_passphrase
    word_list = WordList(options.infile)
  File "/home/bhavin192/diceware/diceware/wordlist.py", line 120, in __init__
    self.fd = open(self.path, "r")
FileNotFoundError: [Errno 2] No such file or directory: 'nonexistingfile'
Exception ignored in: <bound method WordList.__del__ of <diceware.wordlist.WordList object at 0x7fe7b7b00710>>
Traceback (most recent call last):
  File "/home/bhavin192/diceware/diceware/wordlist.py", line 125, in __del__
    self.fd.close()
AttributeError: 'WordList' object has no attribute 'fd'

It shows whole stack trace instead of simple error message. There are two exceptions here, one is FileNotFoundError and the second one is AttributeError. Filed issue #43 on GitHub for this.

Sending Pull Request to solve the issue

As it fails to open the file it raises the first exception. Also, the variable fd never gets initialized. But the destructor tries to call it’s close() method. Pretty easy fix, right?

After thinking a bit I decided to catch the exception in the main() from __init__.py for first error and the second one got solved by adding one condition in __del__()

The code is compatible with Python 2 and 3. In Python 2, FileNotFoundError is not present. Instead, it raises IOError. Python 3 has OSError which has this exception, IOError is alias of OSError in Python 3. More about this

Created the PR #44 with following changes on 1st Jan,

diff --git a/diceware/__init__.py b/diceware/__init__.py
index 91b633e..1d262e9 100644
--- a/diceware/__init__.py
+++ b/diceware/__init__.py
@@ -208,4 +208,8 @@ def main(args=None):
     if options.version:
         print_version()
         raise SystemExit(0)
-    print(get_passphrase(options))
+    try:
+        print(get_passphrase(options))
+    except IOError as infile_error:
+        print("The file '%s' does not exist." % infile_error.filename)
+        raise SystemExit(1)
diff --git a/diceware/wordlist.py b/diceware/wordlist.py
index 32d80fd..96185d1 100644
--- a/diceware/wordlist.py
+++ b/diceware/wordlist.py
@@ -111,6 +111,7 @@ class WordList(object):
     """
     def __init__(self, path):
         self.path = path
+        self.fd = None
         if self.path == "-":
             self.fd = tempfile.SpooledTemporaryFile(
                     max_size=MAX_IN_MEM_SIZE, mode="w+")
@@ -121,7 +122,7 @@ class WordList(object):
         self.signed = self.is_signed()
 
     def __del__(self):
-        if self.path != "-":
+        if self.path != "-" and self.fd is not None:
             self.fd.close()

This was showing the error and exiting with status code 1. As soon I pushed the code I realized that Travis CI failed. The reason was code coverage. So I started writing simple test case for this scenario, this was first time I wrote test using pytest

diff --git a/tests/test_diceware.py b/tests/test_diceware.py
index d6255bb..30adaa2 100644
--- a/tests/test_diceware.py
+++ b/tests/test_diceware.py
@@ -317,6 +317,15 @@ class TestDicewareModule(object):
         out, err = capsys.readouterr()
         assert out == 'Word1Word1\n'
 
+    def test_main_infile_nonexisting(self, argv_handler, capsys):
+        # main() prints error if file does not exist
+        sys.argv = ['diceware', 'nonexisting']
+        with pytest.raises(SystemExit) as exc_info:
+            main()
+        assert exc_info.value.code == 1
+        out, err = capsys.readouterr()
+        assert out == "The file 'nonexisting' does not exist.\n"
+
     def test_main_delimiters(self, argv_handler, capsys):
         # delimiters are respected on calls to main
         sys.stdin = StringIO("word1\n")

This runs the main function and catches the SystemExit, checks if the status code is 1, uses capsys fixture to read stdout and stderr and checks if the out has correct values.

Improvements in Pull Request

A week later, ulif (author) commented on the PR and he suggested some changes:

It is an error message so it should go to stderr
Possibility of using logger
We should not catch all the IOErrors in main, it may catch other non related errors as well.

If logger (error/critical) is used, then the message goes to stderr. Solution for 3. is to check the errno attribute of the exception we are catching. List of errno symbols

So I modified the code with help of code from this comment

diff --git a/diceware/__init__.py b/diceware/__init__.py
index 91b633e..03b7534 100644
--- a/diceware/__init__.py
+++ b/diceware/__init__.py
@@ -18,6 +18,8 @@
 import argparse
 import pkg_resources
 import sys
+import logging
+from errno import ENOENT
 from random import SystemRandom
 from diceware.config import get_config_dict
 from diceware.logger import configure
@@ -208,4 +210,12 @@ def main(args=None):
     if options.version:
         print_version()
         raise SystemExit(0)
-    print(get_passphrase(options))
+    try:
+        print(get_passphrase(options))
+    except (OSError, IOError) as infile_error:
+        if getattr(infile_error, 'errno', 0) == ENOENT:
+            logging.getLogger('ulif.diceware').error(
+                "The file '%s' does not exist." % infile_error.filename)
+            raise SystemExit(1)
+        else:
+            raise

It checks if errno attribute of the exception matches to ENOENT and logs it as error with proper message. If it’s not ENOENT then it raises that exception as it is.

Now the tests, it was expected that err will have the message “The file nonexisting does not exist.”. It was having something else, which I discovered later. While searching for some way to assert over logs I found a in build fixture of pytest which is caplog.

diff --git a/tests/test_diceware.py b/tests/test_diceware.py
index d6255bb..70f5fb9 100644
--- a/tests/test_diceware.py
+++ b/tests/test_diceware.py
@@ -5,6 +5,7 @@ import pytest
 import re
 import sys
 from io import StringIO
+from errno import EISDIR
 from diceware import (
     get_wordlists_dir, SPECIAL_CHARS, insert_special_char, get_passphrase,
     handle_options, main, __version__, print_version, get_random_sources,
@@ -317,6 +318,25 @@ class TestDicewareModule(object):
         out, err = capsys.readouterr()
         assert out == 'Word1Word1\n'
 
+    def test_main_infile_nonexisting(self, argv_handler, caplog, tmpdir):
+        # main() prints error if file does not exist
+        # raises the exception if it's other that file not exist
+        sys.argv = ['diceware', 'nonexisting']
+        with pytest.raises(SystemExit) as exc_info:
+            main()
+        assert exc_info.value.code == 1
+        assert len(caplog.records) == 1
+        for record in caplog.records:
+            assert record.levelname == 'ERROR'
+            assert record.msg == "The file 'nonexisting' does not exist."
+        # give directory as input
+        tmpdir.mkdir("wordlist")
+        tmpdir.chdir()
+        sys.argv = ['diceware', 'wordlist']
+        with pytest.raises(IOError) as infile_error:
+            main()
+        assert infile_error.value.errno == EISDIR
+
     def test_main_delimiters(self, argv_handler, capsys):
         # delimiters are respected on calls to main
         sys.stdin = StringIO("word1\n")

This checks if number of log records is one. Then checks if the log level is ERROR and then checks the actual error message.

To cover the last raise statement ulif suggested me to generate some other IOError exception and check if it’s raised properly. To pass a directory, I used the tmpdir fixture. Again to maintain the compatibility between Python 2 and 3, it was necessary to catch it as IOError . pytest has something called ExceptionInfo, so to check the errno attribute we have to first use value which returns the actual exception. Read more about this

More changes

Everything seems to be perfect now, isn’t it? But wait, the fixture caplog was introduced in pytest-3.3. This change was introducing a dependency on pytest v3.3 which was causing Travis CI to fail for some Python versions. comment related to this

The err was not empty as I thought at the beginning. It was having really large output. ulif suggested to split the test in two tests as well as to use assert ... in. It worked as the large output was having the message we wanted to assert. This change eliminated dependency on pytest v3.3

diff --git a/tests/test_diceware.py b/tests/test_diceware.py
index d6255bb..9568778 100644
--- a/tests/test_diceware.py
+++ b/tests/test_diceware.py
@@ -5,6 +5,7 @@ import pytest
 import re
 import sys
 from io import StringIO
+from errno import EISDIR
 from diceware import (
     get_wordlists_dir, SPECIAL_CHARS, insert_special_char, get_passphrase,
     handle_options, main, __version__, print_version, get_random_sources,
@@ -317,6 +318,25 @@ class TestDicewareModule(object):
         out, err = capsys.readouterr()
         assert out == 'Word1Word1\n'
 
+    def test_main_infile_nonexisting(self, argv_handler, capsys):
+        # main() prints error if file does not exist
+        # raises the exception if it's other that file not exist
+        sys.argv = ['diceware', 'nonexisting']
+        with pytest.raises(SystemExit) as exc_info:
+            main()
+        assert exc_info.value.code == 1
+        out, err = capsys.readouterr()
+        assert "The file 'nonexisting' does not exist." in err
+
+    def test_main_infile_not_a_file(self, argv_handler, tmpdir):
+        # give directory as input
+        tmpdir.mkdir("wordlist")
+        tmpdir.chdir()
+        sys.argv = ['diceware', 'wordlist']
+        with pytest.raises(IOError) as infile_error:
+            main()
+        assert infile_error.value.errno == EISDIR
+
     def test_main_delimiters(self, argv_handler, capsys):
         # delimiters are respected on calls to main
         sys.stdin = StringIO("word1\n")

And CI passed for all Python versions. PR took 38 days to get merged. Read the whole conversation, PR#44

Thanks to ulif for helping and replying politely even on silly mistakes. ulif also added my name to the Credits section.

Things I learned

diceware passphrases
While contributing I discovered more about Python exceptions
Learned about following pytest fixtures
- capsys
- caplog
- tmpdir
tox

Trying to solve one issue can help you to learn a lot of new things.

Don’t forget to try diceware to generate passwords. sudo dnf install diceware or sudo apt-get install diceware

Tags:

caplog diceware github pytest python

Posted on: Permalink

Comments

Comments are not enabled on this site. The old comments might still be displayed. You can reply on one of the platforms listed in ‘Posted on’ list, or email me.

GeekSocket Plug in and be Geekified