Move to explicit method call for Basic Auth credentials in push client #242
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sinjo
force-pushed
the
sinjo-fix-basic-auth
branch
from
2ae084f
to
e7111cd
Compare
Sinjo
commented
Jan 9, 2022
|
|
||
| @http = Net::HTTP.new(@uri.host, @uri.port) | ||
| @http.use_ssl = (@uri.scheme == 'https') | ||
| @http.open_timeout = kwargs[:open_timeout] if kwargs[:open_timeout] | ||
| @http.read_timeout = kwargs[:read_timeout] if kwargs[:read_timeout] | ||
| end | ||
|
|
||
| def basic_auth(user, password) |
There was a problem hiding this comment.
I went with the same method name as Net::HTTP as it seems like a reasonable one, though I'd also be open to something like set_basic_auth to make it clearer if you'd prefer.
lib/prometheus/client/push.rb
Outdated
| def validate_no_basic_auth!(uri) | ||
| if uri.user || uri.password | ||
| raise ArgumentError, <<~EOF | ||
| Setting Basic Auth credentials in the gateway URL is not supported, please call the `basic_auth` method |
There was a problem hiding this comment.
Driveby 😄 maybe a quick usage example here that a developer could just copy-paste?
There was a problem hiding this comment.
That's a really good idea. Just added it in!
Also hi Nick 👋🏻
dmagliola
approved these changes
Jan 9, 2022
Sinjo
force-pushed
the
sinjo-fix-basic-auth
branch
2 times, most recently
from
79d119d
to
9ab21cd
Compare
While URLs do support passing Basic Auth credentials using the `http://user:password@example.com/` syntax, the username and password in that syntax have to follow the usual rules for URL encoding of characters per RFC 3986 (https://datatracker.ietf.org/doc/html/rfc3986#section-2.1). Rather than place the burden of correctly performing that encoding on users of this gem, we decided to have a separate method for supplying Basic Auth credentials, with no requirement to URL encode the characters in them. Signed-off-by: Chris Sinjakli <chris@sinjakli.co.uk>
Sinjo
force-pushed
the
sinjo-fix-basic-auth
branch
from
9ab21cd
to
3a70f73
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While URLs do support passing Basic Auth credentials using the
http://user:password@example.com/syntax, the username and password inthat syntax have to follow the usual rules for URL encoding of
characters per RFC 3986
(https://datatracker.ietf.org/doc/html/rfc3986#section-2.1).
Rather than place the burden of correctly performing that encoding on
users of this gem, we decided to have a separate method for supplying
Basic Auth credentials, with no requirement to URL encode the characters
in them.
Since the tests around HTTP requests are extremely heavily mocked, I tested this locally with a real pushgateway configured with one user with
@in the password and another with:in it, as well as a more boring password that only used basic latin alphabet characters. All three worked.Maybe at some point we should have some integration tests, but that feels like its own piece of work.
Fixes #170