QA: Support making RPC calls with different authentication #19120

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:qa_rpc_with_auth changing 2 files +15 −1

luke-jr commented at 6:45 AM on May 31, 2020: member

Needed to refactor call_rpc out of the test for #19117
QA: authproxy: Support making calls with different authentication 402b34647a
QA: TestNodeCLI: Support making calls with different authentication 72e4160b50
fanquake added the label Tests on May 31, 2020
in test/functional/test_framework/authproxy.py:87 in 402b34647a outdated
```
  79 | @@ -79,6 +80,14 @@ def __init__(self, service_url, service_name=None, timeout=HTTP_TIMEOUT, connect
  80 |          self.timeout = timeout
  81 |          self._set_conn(connection)
  82 |  
  83 | +    def with_auth(self, user, password):
  84 | +        authpair = user + ':' + password
  85 | +        authpair.replace('\\', '\\\\')
  86 | +        new_url = '%s' % (self.__service_url,)
  87 | +        # Based on RFC 3986 (I can't believe there isn't a better way to do this!)
```
promag commented at 12:20 AM on June 2, 2020:

Untested:

u = urlparse(self.__service_url) u = u._replace(netloc="{}:{}@{}".format(user, password, u.hostname)) return AuthServiceProxy(u.geturl(), ...)
luke-jr commented at 12:34 AM on June 2, 2020:

That looks worse (readability, undocumented/undefined behaviour, etc)

promag commented at 12:39 AM on June 2, 2020:

It's documented in https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urlparse

As is the case with all named tuples, the subclass has a few additional methods and attributes that are particularly useful. One such method is _replace(). The _replace() method will return a new ParseResult object replacing specified fields with new values.

https://docs.python.org/3/library/urllib.parse.html#urllib.parse.urllib.parse.SplitResult.geturl

luke-jr commented at 1:01 AM on June 2, 2020:

Hmm, okay...

Might still need a regex, though, since hostname/port are not necessarily obvious.

Consider https://a:b@[ffff:aaaa]:1234

in test/functional/test_framework/authproxy.py:85 in 72e4160b50

  79 | @@ -79,6 +80,14 @@ def __init__(self, service_url, service_name=None, timeout=HTTP_TIMEOUT, connect
  80 |          self.timeout = timeout
  81 |          self._set_conn(connection)
  82 |  
  83 | +    def with_auth(self, user, password):
  84 | +        authpair = user + ':' + password
  85 | +        authpair.replace('\\', '\\\\')

practicalswift commented at 7:45 AM on June 2, 2020:

>>> authpair = "\\"
>>> authpair
'\\'
>>> authpair.replace('\\', '\\\\')
'\\\\'
>>> authpair
'\\'

Should be authpair = authpair.replace('\\', '\\\\')? :)

fanquake commented at 12:14 PM on September 19, 2020: member

#19117 is no-longer open, is this still relevant?
luke-jr commented at 3:20 PM on December 10, 2020: member

It might make the existing rpc_users test nicer?
fanquake commented at 6:10 AM on April 8, 2021: member

It might make the existing rpc_users test nicer?

Going to close this for now, as this seems like fairly weak motivation, without any specific examples/explanation. If this change was going to be made, rather than standalone, the improvements to the tests should probably be included, and the PR description updated now that #19117 is no longer relevant.
fanquake closed this on Apr 8, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels

Linked (view graph)

#19117 RPC: Add getrpcwhitelist method