test: Add test for rpcwhitelistdefault

emilengler commented at 8:09 pm on December 27, 2019: contributor

A test for rpcwhitelistdefault.

fanquake added the label Tests on Dec 27, 2019

emilengler force-pushed on Dec 27, 2019

in test/functional/rpc_whitelistdefault.py:25 in 5450f6fb1d outdated

20+def rpccall(node, user, password, method):
21+    url = urllib.parse.urlparse(node.url)
22+    headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user, password))}
23+    conn = http.client.HTTPConnection(url.hostname, url.port)
24+    conn.connect()
25+    conn.request('POST', '/', '{"method": "' + method + '"}', headers)

promag commented at 11:03 pm on December 27, 2019:

nit, use .format(method). Same above.

in test/functional/rpc_whitelistdefault.py:7 in 5450f6fb1d outdated

0@@ -0,0 +1,100 @@
1+#!/usr/bin/env python3
2+# Copyright (c) 2019 The Bitcoin Core developers
3+# Distributed under the MIT software license, see the accompanying
4+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
5+
6+"""
7+A test for nrpcwhitelistdefault states.

promag commented at 11:05 pm on December 27, 2019:

typo.

in test/functional/rpc_whitelistdefault.py:13 in 5450f6fb1d outdated

 8+"""
 9+
10+from test_framework.test_framework import BitcoinTestFramework
11+import os
12+from test_framework.util import (
13+    get_datadir_path,

promag commented at 11:05 pm on December 27, 2019:

nit, sort

promag commented at 11:15 pm on December 27, 2019: member

Concept ACK.

emilengler force-pushed on Dec 28, 2019

practicalswift commented at 1:02 am on December 28, 2019: contributor

Concept ACK

emilengler force-pushed on Dec 28, 2019

in test/functional/rpc_whitelistdefault.py:11 in d76a516f30 outdated

 6+"""
 7+A test for rpcwhitelistdefault states.
 8+"""
 9+
10+from test_framework.test_framework import BitcoinTestFramework
11+import os

jonatack commented at 6:46 pm on December 28, 2019:

nit: os, http and urllib imports above project-level imports with a line between them (look at the other functional tests)

emilengler commented at 7:20 pm on December 28, 2019:

Done

in test/functional/rpc_whitelistdefault.py:14 in d76a516f30 outdated

10+from test_framework.test_framework import BitcoinTestFramework
11+import os
12+from test_framework.util import (
13+    assert_equal,
14+    get_datadir_path,
15+    str_to_b64str

jonatack commented at 6:47 pm on December 28, 2019:

nit: comma at EOL so it doesn’t need to be touched if adding a method below it

emilengler commented at 7:21 pm on December 28, 2019:

The comma is at the end of every line or am I getting it wrong?

jonatack commented at 7:42 pm on December 28, 2019:

Have a look at example_test.py

in test/functional/rpc_whitelistdefault.py:84 in d76a516f30 outdated

79+            f.write("rpcwhitelist=user1:getbestblockhash\n")
80+            # We need to permit the __cookie__ user for the Bitcoin Test Framework
81+            f.write("rpcwhitelist=__cookie__:getblockcount,getwalletinfo,importprivkey,getblockchaininfo,submitblock,addnode,getpeerinfo,getbestblockhash,getrawmempool,syncwithvalidationinterfacequeue,stop\n")
82+
83+    def run_test(self):
84+        self.log.info("Testing rpcwhitelistdefault=0 with no specififed permissions")

jonatack commented at 6:49 pm on December 28, 2019:

“specified” – typo here and lines 91 and 94 below

in test/functional/rpc_whitelistdefault.py:101 in d76a516f30 outdated

 95+        assert_equal(200, rpccall(self.nodes[3], "user1", "12345", "getbestblockhash"))
 96+        assert_equal(403, rpccall(self.nodes[3], "user2", "12345", "getbestblockhash"))
 97+
 98+if __name__ == "__main__":
 99+    RPCWhitelistDefaultTest().main()
100+

jonatack commented at 6:50 pm on December 28, 2019:

remove the extra line at EOF, and insert a line between lines 96 and 98

emilengler commented at 7:22 pm on December 28, 2019:

The linter cries if there is no new line ant the end of the file. See PEP 8

in test/functional/rpc_whitelistdefault.py:86 in d76a516f30 outdated

80+            # We need to permit the __cookie__ user for the Bitcoin Test Framework
81+            f.write("rpcwhitelist=__cookie__:getblockcount,getwalletinfo,importprivkey,getblockchaininfo,submitblock,addnode,getpeerinfo,getbestblockhash,getrawmempool,syncwithvalidationinterfacequeue,stop\n")
82+
83+    def run_test(self):
84+        self.log.info("Testing rpcwhitelistdefault=0 with no specififed permissions")
85+        assert_equal(200, rpccall(self.nodes[0], "user1", "12345", "getbestblockhash"))

jonatack commented at 6:57 pm on December 28, 2019:

perhaps hoist 200 and 403 to constants like HTTP_OK and HTTP_FORBIDDEN

emilengler commented at 7:23 pm on December 28, 2019:

This is a featrue for another PR. Other tests use the plain codes as well

in test/functional/rpc_whitelistdefault.py:33 in d76a516f30 outdated

27+    conn.close()
28+    return resp.code
29+
30+class RPCWhitelistDefaultTest(BitcoinTestFramework):
31+    def set_test_params(self):
32+        self.num_nodes = 4

jonatack commented at 7:00 pm on December 28, 2019:

perhaps add self.setup_clean_chain = False for explicitness

emilengler commented at 7:24 pm on December 28, 2019:

It doesn’t matter anyway as this isn’t a chain related test

jonatack commented at 7:42 pm on December 28, 2019:

It is false by default (see test_framework.py::L96), but most tests set it explicitly even when false. It matters a bit for test speed as it controls whether or not to use the cached data directories for the test setup.

jonatack commented at 7:05 pm on December 28, 2019: contributor

I think this test should be added to the existing rpc_whitelist.py test with which this test shares the same imports and rpccall code.

emilengler commented at 7:19 pm on December 28, 2019: contributor

I think this test should be added to the existing rpc_whitelist.py test with which this test shares the same imports and rpccall code.

They are two different options. The test would be to big to test both.

test: Add test for rpcwhitelistdefault a478f6d6cb

emilengler force-pushed on Dec 28, 2019

jonatack commented at 7:52 pm on December 28, 2019: contributor

I think this test should be added to the existing rpc_whitelist.py test with which this test shares the same imports and rpccall code.

They are two different options. The test would be to big to test both.

The tests can be organised into different methods within the same test class and file. Many functional test files are much larger than this one would become. Have a look at the size at p2p_segwit.py, feature_block.py, etc. It may be a bit faster in test run time to group similar tests into the same testfile setup where possible. And otherwise, rpccall could be extracted to a common helper method (if one does not already exist). I could be wrong but this is what I’d try to do. It’s up to you.

emilengler commented at 8:18 pm on December 28, 2019: contributor

Even if the featrue is very similar, they test both two different things. rpc_whitelist.py tests permissions, invalid syntaxes, etc. This test tests if users can even do something but is not limited to specific permissions

promag commented at 10:56 pm on December 29, 2019: member

I think the new file is fine.

However have you considered to use just one node? Like

change config
restart
test calls/access
repeat

I prefer this way because:

new test cases don’t imply messing with more nodes;
testing the calls/access is immediately after the config.

emilengler commented at 11:32 pm on December 29, 2019: contributor

However have you considered to use just one node? Like

Yes but after all I believe this would make the code much messier because I would need to re-implement some functions. This is probably the most cleanest way on how to do it

DrahtBot commented at 0:02 am on January 4, 2020: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

fanquake added the label Up for grabs on Aug 18, 2021

fanquake commented at 8:05 am on August 18, 2021: member

Closing as “Up for grabs”. If these tests are going to be added, I think adding them to the existing rpc_whitelist.py is the better approach, as well as using a single node as suggested.

fanquake closed this on Aug 18, 2021

bitcoin locked this on Aug 18, 2022

maflcko removed the label Up for grabs on Apr 12, 2024

test: Add test for rpcwhitelistdefault #17805

Conflicts