Add basic authentication #967
Conversation
|
Thanks for the PR! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #967 +/- ##
==========================================
+ Coverage 82.05% 82.08% +0.02%
==========================================
Files 18 18
Lines 2636 2662 +26
==========================================
+ Hits 2163 2185 +22
- Misses 386 390 +4
Partials 87 87 ☔ View full report in Codecov by Sentry. |
Pull Request Test Coverage Report for Build 12331988242Details
💛 - Coveralls |
exporter/http.go
Outdated
| @@ -12,6 +14,12 @@ import ( | |||
| ) | |||
|
|
|||
| func (e *Exporter) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| err := e.verifyBasicAuth(r.BasicAuth()) | |||
There was a problem hiding this comment.
nit: you can combine this with line 18 for tighter scoping
if err := verifyBasicAuth(...); err != nil {
There was a problem hiding this comment.
Did so in b0aabba
| @@ -393,6 +393,135 @@ func TestReloadHandlers(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestIsBasicAuthConfigured(t *testing.T) { | |||
| } | ||
|
|
||
| userCorrect := subtle.ConstantTimeCompare([]byte(user), []byte(e.options.BasicAuthUsername)) | ||
| passCorrect := subtle.ConstantTimeCompare([]byte(password), []byte(e.options.BasicAuthPassword)) |
There was a problem hiding this comment.
nit: looks like you're always using these as byte[] instead of string so might as well just move the conversion into the "setter" in main.go:206/207
There was a problem hiding this comment.
I've decided to leave this as it is for now, I hope that's fine.
| BasicAuthPassword: tt.configPass, | ||
| }) | ||
|
|
||
| err := e.verifyBasicAuth(tt.providedUser, tt.providedPass, tt.authHeaderSet) |
There was a problem hiding this comment.
I'm fine with having a test that calls verifyBasicAuth() directly but I'd definitely would like to see tests that use a http client to make a real HTTP request (plenty of examples in this file).
There was a problem hiding this comment.
Thanks for the review!
I've addressed the missing tests and added a proper status code for failed authentication in b0aabba
This PR adds Basic Authentication capabilities in front of redis_exporter.