Test/rpc whitelistdefault test #29858

naiyoma · 2024-04-11T18:17:46Z

This PR adds tests for rpcwhitelistdefault. The implementation is a continuation of this PR.

Applied suggestions to include the tests in rpc_whitelist.py and to use a single node.

DrahtBot · 2024-04-11T18:17:49Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	tdb3, rkrux

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

tdb3

Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with this security-minded feature.

Concept ACK. Looks like there could be opportunities to enhance the existing approach taken. Some inline comments were left.

Built and ran all functional tests (all passed).

Checked that the following comments were addressed from PR #17805.

The additional tests appear to be integrated into rpc_whitelist.py
Tests were organized into different methods
num_nodes used is 1
Trailing comma included in test_framework.util imports
nit: Looks like the original comment @jonatack to include explicit self.setup_clean_chain = False could be added in

bitcoin/test/functional/rpc_whitelist.py

Lines 35 to 36 in 94836b3

class RPCWhitelistTest(BitcoinTestFramework):

def set_test_params(self):
specififed typo fixed
nit: Seems like @jonatack's original comment to create constants for HTTP codes (e.g. HTTP_FORBIDDEN instead of 403) could increase readability, but I'm not particularly partial either way. If it's decided to factor response codes into constants, there probably should be a follow up PR to refactor response codes used in lines in this file predating this PR. Changing the previous instances of 200/403 in this PR would seem to be combining two separate goals (adding new tests, refactoring).

test/functional/rpc_whitelist.py

tdb3 · 2024-04-17T00:50:04Z

test/functional/rpc_whitelist.py

    url = urllib.parse.urlparse(node.url)
-    headers = {"Authorization": "Basic " + str_to_b64str('{}:{}'.format(user[0], user[3]))}
+    if password is not None:
+        auth_value = str_to_b64str('{}:{}'.format(user, password))


This seems to be using the argument user as either a string or a list depending on whether or not password was provided, which seems less straightforward than using a consistent type. Unless I'm missing something, the password argument might not be needed at all, since the user list contains the plaintext password. See:

bitcoin/test/functional/rpc_whitelist.py

Lines 40 to 45 in 94836b3

# 0 => Username

# 1 => Password (Hashed)

# 2 => Permissions

# 3 => Password Plaintext

self.users = [

["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash,getblockcount,", "12345"],

tdb3 · 2024-04-17T01:15:03Z

test/functional/rpc_whitelist.py

+        self.log.info("Testing rpcwhitelistdefault=0 with no specified permissions")
+        with open(os.path.join(get_datadir_path(self.options.tmpdir, 0), "bitcoin.conf"), 'a', encoding='utf8') as f:
+            f.write("rpcwhitelistdefault=0\n")
+            f.write("rpcauth=user3:50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e\n")


Instead of hardcoding these new rpcauth lines, it might be better to expand the existing users list. This would apply to adjacent methods as well.

@tdb3 I've pushed an update. Adding the users to the existing list is a better approach. However, some tests are now failing because users are whitelisted in the run_test function, while the new tests are designed to test instances when a user is not whitelisted.

I thought clearing the bitcoin.conf file before writing the new test would solve this issue, but it doesn't. Below is a sample output for the bitcoin.conf file in the test_rpcwhitelist_default_0_without_whitelist function:

rpcwhitelistdefault=0 rpcauth=user1:50358aa884c841............................................................ rpcauth=user2:8650ba41296f6.......................................... rpcauth=user3:50358aa8..................................................

Despite having new permissions, users are still whitelisted, and thus this assertion would fail:

assert_equal(200, rpccall(self.nodes[0], user[0], "getbestblockhash", user[3]).status)
Why would this happen? The new tests are still reading permissions from therun_testfunction and not the new bitcoin.conf

naiyoma · 2024-04-17T13:40:21Z

Thank you for picking this up. Enhancing testing for of the whitelist capability is important to notice regressions associated with this security-minded feature.

Concept ACK. Looks like there could be opportunities to enhance the existing approach taken. Some inline comments were left.

Built and ran all functional tests (all passed).

Checked that the following comments were addressed from PR #17805.

The additional tests appear to be integrated into rpc_whitelist.py

Tests were organized into different methods

num_nodes used is 1

Trailing comma included in test_framework.util imports

nit: Looks like the original comment @jonatack to include explicit self.setup_clean_chain = False could be added in

bitcoin/test/functional/rpc_whitelist.py

Lines 35 to 36 in 94836b3

class RPCWhitelistTest(BitcoinTestFramework):

def set_test_params(self):

specififed typo fixed

nit: Seems like @jonatack's original comment to create constants for HTTP codes (e.g. HTTP_FORBIDDEN instead of 403) could increase readability, but I'm not particularly partial either way. If it's decided to factor response codes into constants, there probably should be a follow up PR to refactor response codes used in lines in this file predating this PR. Changing the previous instances of 200/403 in this PR would seem to be combining two separate goals (adding new tests, refactoring).

Thanks a lot for the review,
I didn't implement the requested format for HTTP codes because I wanted to maintain consistency with the already existing tests. However, for readability, I agree with your suggestion. I think the right approach would be to address this in a follow-up PR and also refactor these codes for some of the other tests files as well.

DrahtBot · 2024-05-03T15:47:33Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/24559741310}

rkrux

Thanks for adding these tests, concept ACK.

Seems like this is in progress because there is test failure and a debugger is added.
Added few suggestions for now, will review again when the tests pass.

rkrux · 2024-05-08T10:29:21Z

test/functional/rpc_whitelist.py

+
+
+    def test_whitelistdefault_1_with_specified_permission(self):
+        self.log.info("Testing rpcwhitelistdefault=1 with  specified permissions")


with specified
Extra space between

test/functional/rpc_whitelist.py

rkrux · 2024-05-08T10:32:41Z

test/functional/rpc_whitelist.py

        self.log.info("Strange test 5")
        assert_equal(200, rpccall(self.nodes[0], self.strange_users[4], "getblockcount").status)


Suggestion: The above test ending here can be put in a separate function so that there are mostly other test functions calls inside run_test.

Not sure if I should do this in this PR, since the main scope of this one is to cover the whitelistdefault test case.

rkrux · 2024-05-08T10:35:57Z

test/functional/rpc_whitelist.py

+            permissions = user[2].replace(" ", "").split(",")
+            i = 0
+            while i < len(permissions):
+                if permissions[i] == '':
+                    permissions.pop(i)
+                i += 1


This piece of code is duplicated in every test, good candidate for extracting out in a getPermissions(user) function.

test/functional/rpc_whitelist.py

rkrux · 2024-05-08T10:38:40Z

test/functional/rpc_whitelist.py

+
+    def test_rpcwhitelist_default_0_without_whitelist(self):
+        self.log.info("Testing rpcwhitelistdefault=0 with no specified permissions")
+        with open(os.path.join(self.nodes[0].cwd, "bitcoin.conf"), "w", encoding="utf8") as f:


Why is this path different than in other tests below?

The idea here was to create a separate path for testing whitelisted users and a different path for testing users with no whitelist. The issue is that once a user is whitelisted, clearing the bitcoin.conf and writing new permissions still shows the user as whitelisted. I've provided a detailed explanation here.

DrahtBot · 2024-05-13T07:40:20Z

Converted to draft for now. If this is no longer a WIP, you can move it out of draft.

naiyoma · 2024-05-14T21:57:16Z

Thanks for adding these tests, concept ACK.

Seems like this is in progress because there is test failure and a debugger is added. Added few suggestions for now, will review again when the tests pass.

Thanks for the review! The initial tests passed successfully. But I pushed an update that implements the suggested alternative approach from this discussion

Unfortunately, the tests are currently failing with this new approach, as explained here.

While I'm still working on refining the suggested approach, I've decided to revert the PR to its original state (with passing tests) for now. To keep things functional while I continue working on the alternative approach in a separate branch on my fork

laanwj added RPC/REST/ZMQ Tests labels Apr 13, 2024

tdb3 reviewed Apr 17, 2024

View reviewed changes

test: add rpcwhitelistdefault tests

c9358db

naiyoma force-pushed the test/rpc-whitelistdefault-test branch from 94836b3 to c9358db Compare April 18, 2024 11:48

DrahtBot added the CI failed label May 3, 2024

rkrux reviewed May 8, 2024

View reviewed changes

DrahtBot marked this pull request as draft May 13, 2024 07:39

naiyoma force-pushed the test/rpc-whitelistdefault-test branch from e02af85 to c9358db Compare May 14, 2024 21:55

DrahtBot removed the CI failed label May 15, 2024

fix string formatting

e6ef531

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test/rpc whitelistdefault test #29858

Test/rpc whitelistdefault test #29858

naiyoma commented Apr 11, 2024

DrahtBot commented Apr 11, 2024 •

edited

tdb3 left a comment •

edited

tdb3 Apr 17, 2024

tdb3 Apr 17, 2024

naiyoma May 3, 2024

naiyoma commented Apr 17, 2024

DrahtBot commented May 3, 2024

rkrux left a comment •

edited

rkrux May 8, 2024

rkrux May 8, 2024

naiyoma May 14, 2024

rkrux May 8, 2024

rkrux May 8, 2024

naiyoma May 14, 2024

DrahtBot commented May 13, 2024

naiyoma commented May 14, 2024

	class RPCWhitelistTest(BitcoinTestFramework):
	def set_test_params(self):

	# 0 => Username
	# 1 => Password (Hashed)
	# 2 => Permissions
	# 3 => Password Plaintext
	self.users = [
	["user1", "50358aa884c841648e0700b073c32b2e$b73e95fff0748cc0b517859d2ca47d9bac1aa78231f3e48fa9222b612bd2083e", "getbestblockhash,getblockcount,", "12345"],



		def test_whitelistdefault_1_with_specified_permission(self):
		self.log.info("Testing rpcwhitelistdefault=1 with specified permissions")

		self.log.info("Strange test 5")
		assert_equal(200, rpccall(self.nodes[0], self.strange_users[4], "getblockcount").status)

Test/rpc whitelistdefault test #29858

Are you sure you want to change the base?

Test/rpc whitelistdefault test #29858

Conversation

naiyoma commented Apr 11, 2024

DrahtBot commented Apr 11, 2024 • edited

Code Coverage

Reviews

tdb3 left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naiyoma commented Apr 17, 2024

DrahtBot commented May 3, 2024

rkrux left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented May 13, 2024

naiyoma commented May 14, 2024

DrahtBot commented Apr 11, 2024 •

edited

tdb3 left a comment •

edited

rkrux left a comment •

edited