-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Allow token propagation if token type is not specified #1685
Allow token propagation if token type is not specified #1685
Conversation
2e59817
to
7379625
Compare
Codecov Report
@@ Coverage Diff @@
## master #1685 +/- ##
==========================================
+ Coverage 98.49% 98.49% +<.01%
==========================================
Files 193 193
Lines 9284 9286 +2
==========================================
+ Hits 9144 9146 +2
Misses 111 111
Partials 29 29
Continue to review full report at Codecov.
|
7379625
to
b4a7714
Compare
lgtm but I don't think we want all those log statements |
@yurishkuro ok I can remove it |
b4a7714
to
9f4ba33
Compare
I remove one, and change to debug others. Is that ok? or do you prefer to get rid of it? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all log statements except for warning on unparseable header should be removed. They provide little value in production.
if authHeaderValue != "" { | ||
headerValue := strings.Split(authHeaderValue, " ") | ||
token := "" | ||
if len(headerValue) == 2 { | ||
// Make sure we only capture bearer token , not other types like Basic auth. | ||
if headerValue[0] == "Bearer" { | ||
token = headerValue[1] | ||
} else { | ||
logger.Debug("Unsupported type of token " + headerValue[0] + " skipping token propagation") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to log this?
@@ -26,21 +27,30 @@ import ( | |||
func bearerTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler { | |||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
ctx := r.Context() | |||
logger.Info("Propagating bearer token") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to log this?
@@ -26,21 +27,30 @@ import ( | |||
func bearerTokenPropagationHandler(logger *zap.Logger, h http.Handler) http.Handler { | |||
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |||
ctx := r.Context() | |||
logger.Info("Propagating bearer token") | |||
log.Print(r) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use log package
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups it was not my intend to have this here.
if authHeaderValue == "" { | ||
authHeaderValue = r.Header.Get("X-Forwarded-Access-Token") | ||
} | ||
logger.Info("Token: " + authHeaderValue) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to log this?
9f4ba33
to
ed35e2e
Compare
Done, logs removed, except warning log for unparseable header. |
Signed-off-by: Ruben Vargas <[email protected]>
ed35e2e
to
3852889
Compare
Which problem is this PR solving?
Authorization: <type> <token>
, they put the token in the formAuthorization: <token>
and assumed that the token there is a bearer token.Short description of the changes