This is a follow-up PR to address review feedback from https://github.com/bitcoin/bitcoin/pull/29858
test_rpcwhitelistdefault_permissions
I am not sure if my approach of adding test_rpcwhitelistdefault_unset is better or if I should just include the assertions in the existing test_rpcwhitelistdefault_permissions
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32079.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky |
| Concept ACK | ismaelsadeeq, musaHaruna, BrandonOdiwuor |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK, nice follow-up! Why is it in draft?
had to force push some changes, but its now open
test_rpcwhitelistdefault_unset is okay, it avoids mixing functionalities into the same function, thereby making test_rpcwhitelistdefault_permissions less complex.
Each function is doing exactly what the function name suggests. IMO
25@@ -26,7 +26,7 @@ def rpccall(node, user, method):
26
27
28 def get_permissions(whitelist):
29- return [perm for perm in whitelist.replace(" ", "").split(",") if perm]
30+ return [perm for perm in whitelist.split(",") if perm]
"getbestblockhash,getblockcount," , it causes the test to fail
self.users = [
["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],
Thanks for following up!
cb5e26d6f2dafa150d87545890afaeeceb5c610a is doing multiple things.
Can you make the commits atomic just as you did in the PR description?
Update get_permissions function to remove unnecessary replace() and
improve password for strangedude6. Change all string concatenation to f-strings.
Replace test_rpcwhitelistdefault_0_no_permissions and
test_rpcwhitelistdefault_1_no_permissions with a single
test_rpcwhitelistdefault_permissions function.
Code review ACK b77485cc7220314edc05adf3b2cb298af556e36c. This is a very good check to add. We should be making sure -rpcwhitelist settings are effective even when -rpcwhitelistdefault is unset.
I think the last commit could be shorter and clearer by reusing existing checks instead of copying them. Would suggest the following change to simplify:
0--- a/test/functional/rpc_whitelist.py
1+++ b/test/functional/rpc_whitelist.py
2@@ -102,7 +102,13 @@ class RPCWhitelistTest(BitcoinTestFramework):
3 self.test_users_permissions()
4 self.test_rpcwhitelistdefault_permissions(1, 403)
5
6- self.test_rpcwhitelistdefault_unset()
7+ # Ensure that not specifying -rpcwhitelistdefault is the same as
8+ # specifying -rpcwhitelistdefault=1. Only explicitly whitelisted users
9+ # should be allowed.
10+ self.nodes[0].replace_in_config([("rpcwhitelistdefault=1", "")])
11+ self.restart_node(0)
12+ self.test_users_permissions()
13+ self.test_rpcwhitelistdefault_permissions(1, 403)
14
15 def test_users_permissions(self):
16 """
17@@ -132,26 +138,6 @@ class RPCWhitelistTest(BitcoinTestFramework):
18 self.log.info(f"[{user[0]}]: Testing rpcwhitelistdefault={default_value} no specified permission ({permission})")
19 assert_equal(expected_status, rpccall(self.nodes[0], user, permission).status)
20
21- def test_rpcwhitelistdefault_unset(self):
22- """
23- * rpcwhitelistdefault is unset
24- Expected result:
25- - Whitelisted users can only access their whitelisted methods
26- - Non-whitelisted users cannot access any methods
27- """
28- self.nodes[0].replace_in_config([("rpcwhitelistdefault=1", "")])
29- self.restart_node(0)
30-
31- # Test whitelisted user (strangedude4)
32- whitelisted_user = self.strange_users[4]
33- self.log.info(f"[{whitelisted_user[0]}]: Testing user with explicit whitelist when rpcwhitelistdefault=unset")
34- assert_equal(200, rpccall(self.nodes[0], whitelisted_user, 'getblockcount').status)
35- assert_equal(403, rpccall(self.nodes[0], whitelisted_user, 'getbestblockhash').status)
36- # Test non-whitelisted user (strangedude6)
37- non_whitelisted_user = self.strange_users[6]
38- self.log.info(f"[{non_whitelisted_user[0]}]: Testing rpcwhitelistdefault=unset no specified permission (getbestblockhash)")
39- assert_equal(403, rpccall(self.nodes[0], non_whitelisted_user, 'getbestblockhash').status)
40-
41
42 if __name__ == "__main__":
43 RPCWhitelistTest(__file__).main()
Current approach is also ok though