Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[wsclient] Allow for Embedded Basic Auth in URLs #111

Conversation

onelapahead
Copy link
Contributor

@onelapahead onelapahead commented Nov 21, 2023

We observed when using the wsclient package that if a WS URL had basic auth embedded in it we'd get the following error:

Kaleido Block IndexerCopyright (C) 2023 Kaleido
Version:  (Build Date: 2023-11-16T19:49:14Z)

time="2023-11-20T20:14:29.999Z" level=info msg="[i] No TLS configuration provided`"
time="2023-11-20T20:14:29.999Z" level=debug msg="Created REST client to https://user:pass@acme.com"
time="2023-11-20T20:14:29.999Z" level=warning msg="WS wss://user:pass@acme.com connect attempt 1 failed [-1]: "
time="2023-11-20T20:14:29.999Z" level=fatal msg="[!] Failed to connect to blockchain : FF00148: Websocket connect failed: malformed ws or wss URL"

In some cases, applied configurations are more difficult to change than a binary version. This change proposes that the credentials in the URL be handled more gracefully, in that 1) the embedded credentials are always removed from the URL string, and that 2) if the config did not have auth credentials provided, the embedded credentials are extracted and used automatically.

onelapahead and others added 3 commits November 21, 2023 00:37
Signed-off-by: hfuss <hayden.fuss@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
Signed-off-by: Nicko Guyer <nicko.guyer@kaleido.io>
@nguyer nguyer marked this pull request as ready for review November 21, 2023 14:24
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (27e89cb) 100.00% compared to head (e3aa13f) 99.98%.
Report is 46 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##              main     #111      +/-   ##
===========================================
- Coverage   100.00%   99.98%   -0.02%     
===========================================
  Files           63       77      +14     
  Lines         5048     6170    +1122     
===========================================
+ Hits          5048     6169    +1121     
- Misses           0        1       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@nguyer nguyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. @peterbroadhurst I don't know if you want to look at the UT changes here that I made (doesn't seem right for me to approve my own changes). Essentially when running in GitHub actions, the file watcher would fail to start because the file didn't exist yet. This seems to work fine running locally, and the way the test was written it seemed like that's exactly what it was designed to test. But it doesn't work. I re-arranged the test so that the file is there when the watcher starts. But I wanted to make sure I wasn't just patching a test to cover up an actual bug.

@peterbroadhurst peterbroadhurst merged commit 43e860e into hyperledger:main Nov 21, 2023
@peterbroadhurst peterbroadhurst deleted the wsclient/handle-embedded-basic-auth branch November 21, 2023 14:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants