test: rpc_users: Also test rpcauth.py with password. #16334

pull dongcarl wants to merge 4 commits into bitcoin:master from dongcarl:2019-07-rpcauth-passwd-specified-case changing 1 files +41 −141

dongcarl commented at 7:04 PM on July 3, 2019: member

Fixes #14758

First two commits are tidy-ups which I feel are worthwhile as they are very straightforward, cut down the file by 50%, and made the final diff more minimal. Happy to squash after review.
dongcarl added the label Tests on Jul 3, 2019

in test/functional/rpc_users.py:91 in 30b12a286e outdated

 178 |  
 179 | -        #Wrong password for randomly generated user
 180 | -        self.log.info('Wrong...')
 181 | -        authpairnew = self.user+":"+self.password+"Wrong"
 182 | -        headers = {"Authorization": "Basic " + str_to_b64str(authpairnew)}
 183 | +        password = "cA773lm788buwYe4g4WT+05pKyNruVKjQ25x3n0DQcM="

qmma70 commented at 3:26 AM on July 4, 2019:

Minor: maybe rename this rtpassword or something similar to avoid confusion with self.password?

dongcarl commented at 4:42 PM on July 4, 2019:

Fixed.

in test/functional/rpc_users.py:104 in 30b12a286e outdated
```
 233 | -        conn.request('POST', '/', '{"method": "getbestblockhash"}', headers)
 234 | -        resp = conn.getresponse()
 235 | -        assert_equal(resp.status, 401)
 236 | -        conn.close()
 237 | -
 238 | +        self.test_auth(self.nodes[1], "rpcuser💻", "rpcpassword🔑")
```
fqlx commented at 3:57 AM on July 4, 2019:

Why do we have emojis in the code?

laanwj commented at 10:45 AM on July 4, 2019:

it's arbitrary, but the thinking is: if it can handle emoji, it can handle any UTF symbol

fqlx commented at 1:30 PM on July 4, 2019:

Just because it can handle emojis doesn't mean it's a good idea to use. Can we remove them?

fanquake commented at 1:37 PM on July 4, 2019:

We are already using emojis elsewhere in the functional test suite.

https://github.com/bitcoin/bitcoin/blob/413e438ea9767710d4810c4400fd1242ca52cd1c/test/functional/test_runner.py#L255

achow101 commented at 4:05 PM on July 4, 2019:

Emojis are UTF symbols. It is good to test that we are able to handle UTF symbols, especially in places that people may have entered them such as the configuration files where there may be UTF symbols in passwords or file paths. As such, it would be detrimental to remove them as otherwise we are losing the tests for these.

fjahr commented at 9:45 PM on July 5, 2019:

You don't clarify why it is not a good idea, so I am not sure what you are getting at. But in general, I would add that it would be better to have an explicit test for the usage of UTF symbols and not an implicit test within another test.

dongcarl commented at 10:00 PM on July 5, 2019:

While I agree that separating tests for "if rpcuser/rpcpassword works at all" from "if rpcuser/rpcpassword works with UTF-8" is worthwhile, it is beyond the scope of this PR.

However, this separation looks like an easy enough change, and perhaps if @fjahr or @fqlx feel strongly about this they can take this up in a separate PR.

laanwj commented at 1:36 PM on July 8, 2019:

UTF handling is not optional in this day and age. I think we should keep this as-is.
fqlx changes_requested

in test/functional/rpc_users.py:59 in 1d5312a9ad outdated

  58 | +        p = subprocess.Popen([sys.executable, gen_rpcauth, 'rt2', self.rt2password], stdout=subprocess.PIPE, universal_newlines=True)
  59 | +        lines = p.stdout.read().splitlines()
  60 | +        rpcauth2 = lines[1]
  61 | +
  62 | +        # Generate RPCAUTH without specifying password
  63 | +        self.user = ''.join(SystemRandom().choice(string.ascii_letters + string.digits) for _ in range(10))

fjahr commented at 7:54 PM on July 6, 2019:

Nit: Why do you generate a random username here instead of just hardcoding one?

dongcarl commented at 1:55 AM on July 7, 2019:

Cuz it was originally done this way. Not really within scope but good catch!

in test/functional/rpc_users.py:94 in 1d5312a9ad outdated

 183 | -        #Wrong password for randomly generated user
 184 | -        self.log.info('Wrong...')
 185 | -        authpairnew = self.user+":"+self.password+"Wrong"
 186 | -        headers = {"Authorization": "Basic " + str_to_b64str(authpairnew)}
 187 | +        self.log.info('Correct...')
 188 | +        assert_equal(200, call_with_auth(self.nodes[0], url.username, url.password).status)

fjahr commented at 7:55 PM on July 6, 2019:

Nit: Could this not also use the test_auth function?

dongcarl commented at 1:54 AM on July 7, 2019:

Done!

fjahr commented at 7:58 PM on July 6, 2019: member

ACK 1d5312a Good refactoring! Ran test locally, checked that they could fail, reviewed code.
dongcarl force-pushed on Jul 7, 2019
laanwj commented at 1:39 PM on July 8, 2019: member

Looks like the "tidy up run_test" commit was duplicated!

Good refactoring! Ran test locally, checked that they could fail, reviewed code.

Thanks for reviewing/testing.
dongcarl commented at 2:05 PM on July 8, 2019: member

Looks like the "tidy up run_test" commit was duplicated!

Good refactoring! Ran test locally, checked that they could fail, reviewed code.

Thanks for reviewing/testing.

Oh they're not duplicates, both tidy-ups but easier to review when separate. :-) I'm gunna squash everything together after review.
MarcoFalke commented at 5:02 PM on July 8, 2019: member

I think they should be kept separate commits, to make review after merge easier and preserve history for future reference.
test: rpc_users: Add function for auth'd requests. 604e2a997f
test: rpc_users: Add function for testing auth params. c73d871799
test: rpc_users: Also test rpcauth.py with specified password. 830dc2dd0f
test: rpc_users: Make variable names more clear. e263a343d4
dongcarl force-pushed on Jul 8, 2019
dongcarl commented at 8:14 PM on July 8, 2019: member

Since we're not squashing, I reworded the commit messages to be more descriptive and rebased.
laanwj commented at 7:28 PM on July 12, 2019: member

ACK e263a343d4b6a2622df6bb734cd9d51a0d20a663
laanwj merged this on Jul 12, 2019
laanwj closed this on Jul 12, 2019
laanwj referenced this in commit 536590f358 on Jul 12, 2019
sidhujag referenced this in commit 0f7e2c5ab1 on Jul 12, 2019
sidhujag referenced this in commit b80033163a on Jul 29, 2019
jasonbcox referenced this in commit 6bcfe65667 on Oct 9, 2020
jasonbcox referenced this in commit 69e6c7d65c on Oct 9, 2020
jasonbcox referenced this in commit fade28c891 on Oct 9, 2020
jasonbcox referenced this in commit 2d524721fe on Oct 9, 2020
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#14758 Integration tests that use rpcauth.py