[tests] Make rpcauth.py testable and add unit tests #13056

pull nixbox wants to merge 1 commits into bitcoin:master from nixbox:rpcauth changing 4 files +81 −19

nixbox commented at 7:47 AM on April 23, 2018: contributor

refs #12995
fanquake added the label Tests on Apr 23, 2018

laanwj commented at 2:59 PM on April 23, 2018: member

Some lint issues:

./test/util/rpcauth-test.py:19:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:20:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:21:1: E402 module level import not at top of file
./test/util/rpcauth-test.py:22:1: E402 module level import not at top of file
^---- failure generated from contrib/devtools/lint-python.sh

Also the test fails to launch:

/usr/bin/python3.4 ../test/util/rpcauth-test.py
/usr/bin/python3.4: can't open file '../test/util/rpcauth-test.py': [Errno 2] No such file or directory

nixbox commented at 1:27 AM on April 24, 2018: contributor

Fixed the lint issues.

Fixed the "No such file or directory" problem as well.
laanwj commented at 12:09 PM on April 24, 2018: member

utACK ebff14786b7addd4bcc6e74691394a77019f2bf6

in Makefile.am:228 in ebff14786b outdated

 224 | @@ -225,6 +225,7 @@ EXTRA_DIST += \
 225 |      test/util/data/blanktxv1.json \
 226 |      test/util/data/blanktxv2.hex \
 227 |      test/util/data/blanktxv2.json \
 228 | +    test/util/rpcauth-test.py \

jnewbery commented at 3:00 PM on April 24, 2018:

nit: seems like somewhat random placement. Why not alphabetically sorted like the rest?

nixbox commented at 5:16 PM on April 24, 2018:

Done

in share/rpcauth/rpcauth.py:17 in ebff14786b outdated

  16 | +    # This uses os.urandom() underneath
  17 | +    cryptogen = SystemRandom()
  18 |  
  19 | -username = sys.argv[1]
  20 | +    # Create 16 byte hex salt
  21 | +    salt_sequence = [cryptogen.randrange(256) for i in range(16)]

jnewbery commented at 3:02 PM on April 24, 2018:

nit: prefer _ for unused loop variables.

nixbox commented at 5:16 PM on April 24, 2018:

Done

in share/rpcauth/rpcauth.py:21 in ebff14786b outdated

  22 | +    return ''.join([format(r, 'x') for r in salt_sequence])
  23 |  
  24 | -#This uses os.urandom() underneath
  25 | -cryptogen = SystemRandom()
  26 | +def generate_password(salt):
  27 | +    # Create 32 byte b64 password

jnewbery commented at 3:03 PM on April 24, 2018:

nit: prefer docstrings for function-level comments.

nixbox commented at 5:16 PM on April 24, 2018:

Done

in share/rpcauth/rpcauth.py:37 in ebff14786b outdated
```
  49 | -print("rpcauth="+username+":"+salt+"$"+result)
  50 | -print("Your password:\n"+password)
  51 | +    username = sys.argv[1]
  52 | +
  53 | +    salt = generate_salt()
  54 | +    (password, password_hmac) = generate_password(salt)
```
jnewbery commented at 3:12 PM on April 24, 2018:

No need to use parens here. Just:

password, password_hmac = generate_password(salt)

(you could also change the return statement in generate_password() to return password, password_hmac, but under the covers Python will construct a tuple to return anyway, so it's equivalent.)
nixbox commented at 5:16 PM on April 24, 2018:

Done
in share/rpcauth/rpcauth.py:40 in ebff14786b outdated
```
  52 | +
  53 | +    salt = generate_salt()
  54 | +    (password, password_hmac) = generate_password(salt)
  55 | +
  56 | +    print('String to be appended to bitcoin.conf:')
  57 | +    print('rpcauth='+username+':'+salt+'$'+password_hmac)
```
jnewbery commented at 3:14 PM on April 24, 2018:

nit: prefer string formatters ({} or % - we can't use f-strings yet).

If you must concatenate strings, please surround the + with spaces so the code isn't so difficult to parse.

nixbox commented at 5:16 PM on April 24, 2018:

Done
in test/util/rpcauth-test.py:10 in ebff14786b outdated
```
   5 | +"""Test share/rpcauth/rpcauth.py
   6 | +"""
   7 | +import base64
   8 | +try:
   9 | +    import configparser
  10 | +except ImportError:
```
jnewbery commented at 3:16 PM on April 24, 2018:

I think this is unnecessary - this is for python2 compatibility. We now no longer support python2 across the project.

nixbox commented at 5:16 PM on April 24, 2018:

Updated

in test/util/rpcauth-test.py:22 in ebff14786b outdated

  17 | +
  18 | +class TestRPCAuth(unittest.TestCase):
  19 | +    def setUp(self):
  20 | +        config = configparser.ConfigParser()
  21 | +        file_path = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
  22 | +        config_file = open(file_path)

jnewbery commented at 3:23 PM on April 24, 2018:

nit: prefer with open() as: for opening files.

jnewbery commented at 3:25 PM on April 24, 2018: member

A few nits inline after testing.
nixbox commented at 5:18 PM on April 24, 2018: contributor

Mistakenly did a force push. Addressed all of your nits.

Thanks for the feedback!

in share/rpcauth/rpcauth.py:20 in 3714cef800 outdated

  21 | +    salt_sequence = [cryptogen.randrange(256) for _ in range(16)]
  22 | +    return ''.join([format(r, 'x') for r in salt_sequence])
  23 |  
  24 | -#This uses os.urandom() underneath
  25 | -cryptogen = SystemRandom()
  26 | +"""Create 32 byte b64 password"""

jnewbery commented at 5:26 PM on April 24, 2018:

Docstrings should be the first statement inside the function (https://www.python.org/dev/peps/pep-0257/#what-is-a-docstring)

nixbox commented at 6:29 PM on April 24, 2018:

Done

in test/util/rpcauth-test.py:18 in 3714cef800 outdated
```
  13 | +import unittest
  14 | +
  15 | +class TestRPCAuth(unittest.TestCase):
  16 | +    def setUp(self):
  17 | +        config = configparser.ConfigParser()
  18 | +        file_path = os.path.abspath(os.path.dirname(__file__)) + "/../config.ini"
```
jnewbery commented at 5:28 PM on April 24, 2018:

nit: use os.path.join() for maximum portability (instead of concat'ing path strings)

MarcoFalke commented at 6:13 PM on April 24, 2018:

Also, should probably say config_path

nixbox commented at 6:39 PM on April 24, 2018:

Done
jnewbery commented at 6:00 PM on April 24, 2018: member

couple more nits inline
[tests] Make rpcauth.py testable and add unit tests
refs #12995
6674a75bfb
in test/config.ini.in:12 in 3714cef800 outdated
```
   8 | @@ -9,6 +9,7 @@
   9 |  SRCDIR=@abs_top_srcdir@
  10 |  BUILDDIR=@abs_top_builddir@
  11 |  EXEEXT=@EXEEXT@
  12 | +RPCAUTH=@abs_top_srcdir@/share/rpcauth/rpcauth.py
```
MarcoFalke commented at 6:14 PM on April 24, 2018:

Could do a rebase, since this is already in master?

nixbox commented at 6:39 PM on April 24, 2018:

Done
jnewbery commented at 7:15 PM on April 24, 2018: member

Tested ACK 6674a75bfbdf680d0a3648f44c6591ff5fc983cf.

Travis failure was unrelated failure in mining_prioritisetransaction.py. I've restarted the job.
nixbox commented at 12:07 AM on April 25, 2018: contributor

All green, ready to go?
laanwj merged this on Apr 25, 2018
laanwj closed this on Apr 25, 2018
laanwj referenced this in commit 106d929780 on Apr 25, 2018
laanwj referenced this in commit 3e53004339 on Apr 25, 2018
MarcoFalke referenced this in commit a785bc3667 on Apr 25, 2018
stamhe referenced this in commit add46c6b4b on Jun 27, 2018
HashUnlimited referenced this in commit 3c2a999a4d on Sep 6, 2018
lionello referenced this in commit e2ef82d212 on Nov 7, 2018
joemphilips referenced this in commit e2eec24874 on Nov 9, 2018
PastaPastaPasta referenced this in commit 69651c36a0 on Jul 19, 2020
PastaPastaPasta referenced this in commit 1396dc674a on Jul 19, 2020
PastaPastaPasta referenced this in commit 46c368b8f1 on Jul 22, 2020
PastaPastaPasta referenced this in commit 7f32576e0b on Jul 22, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#12995 Need test for rpcauth.py #13073 test: add rpcauth-test to AC_CONFIG_LINKS to fix out-of-tree make check #14758 Integration tests that use rpcauth.py