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
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32079.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | w0xlt, ryanofsky, achow101 |
| 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?
Concept ACK, nice follow-up! Why is it in draft?
had to force push some changes, but its now open
Concept ACK
Creating a seperate functiontest_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]
nit: I think the previous implementation is catching all the edge cases including unintended white spaces.
e.g when I add a space between "getbestblockhash,getblockcount," , it causes the test to fail
self.users = [ ["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash, getblockcount,", "12345"],
There is no unintended white space in the current list hence it is redundant not needed see my comment here #29858 (review)
Thanks for the clarification! Yeah the previous is making it redundant. Please @naiyoma ignore my nit
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.
@musaHaruna, thanks for the review, @ismaelsadeeq I've broken down commits to match description
Concept ACK
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:
<details><summary>diff</summary> <p>
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -102,7 +102,13 @@ class RPCWhitelistTest(BitcoinTestFramework):
self.test_users_permissions()
self.test_rpcwhitelistdefault_permissions(1, 403)
- self.test_rpcwhitelistdefault_unset()
+ # Ensure that not specifying -rpcwhitelistdefault is the same as
+ # specifying -rpcwhitelistdefault=1. Only explicitly whitelisted users
+ # should be allowed.
+ self.nodes[0].replace_in_config([("rpcwhitelistdefault=1", "")])
+ self.restart_node(0)
+ self.test_users_permissions()
+ self.test_rpcwhitelistdefault_permissions(1, 403)
def test_users_permissions(self):
"""
@@ -132,26 +138,6 @@ class RPCWhitelistTest(BitcoinTestFramework):
self.log.info(f"[{user[0]}]: Testing rpcwhitelistdefault={default_value} no specified permission ({permission})")
assert_equal(expected_status, rpccall(self.nodes[0], user, permission).status)
- def test_rpcwhitelistdefault_unset(self):
- """
- * rpcwhitelistdefault is unset
- Expected result:
- - Whitelisted users can only access their whitelisted methods
- - Non-whitelisted users cannot access any methods
- """
- self.nodes[0].replace_in_config([("rpcwhitelistdefault=1", "")])
- self.restart_node(0)
-
- # Test whitelisted user (strangedude4)
- whitelisted_user = self.strange_users[4]
- self.log.info(f"[{whitelisted_user[0]}]: Testing user with explicit whitelist when rpcwhitelistdefault=unset")
- assert_equal(200, rpccall(self.nodes[0], whitelisted_user, 'getblockcount').status)
- assert_equal(403, rpccall(self.nodes[0], whitelisted_user, 'getbestblockhash').status)
- # Test non-whitelisted user (strangedude6)
- non_whitelisted_user = self.strange_users[6]
- self.log.info(f"[{non_whitelisted_user[0]}]: Testing rpcwhitelistdefault=unset no specified permission (getbestblockhash)")
- assert_equal(403, rpccall(self.nodes[0], non_whitelisted_user, 'getbestblockhash').status)
-
if __name__ == "__main__":
RPCWhitelistTest(__file__).main()
</p> </details>
Current approach is also ok though
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:
Thanks for the review, I've added the suggestion here 2929da1dd592da79e0afa6834a82c1bc54fbcf18
Code review ACK https://github.com/bitcoin/bitcoin/pull/32079/commits/2929da1dd592da79e0afa6834a82c1bc54fbcf18 A welcome addition to test cases
Code review ACK 2929da1dd592da79e0afa6834a82c1bc54fbcf18. Only change since last review was simplifying the last commit as suggested
ACK 2929da1dd592da79e0afa6834a82c1bc54fbcf18