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

feat: Log invalid credentials on info level instead of error/warning #517

Merged
merged 2 commits into from
Sep 17, 2020
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Log invalid credentials on info level instead of error/warning
catper committed Sep 15, 2020
commit abba8e6dcffc156c2d226e01456061cee22973e1
2 changes: 1 addition & 1 deletion api/decision.go
Original file line number Diff line number Diff line change
@@ -110,7 +110,7 @@ func (h *DecisionHandler) decisions(w http.ResponseWriter, r *http.Request) {
h.r.Logger().WithError(err).
WithFields(fields).
WithField("granted", false).
Warn("Access request denied")
Info("Access request denied")

h.r.ProxyRequestHandler().HandleError(w, r, rl, err)
return
16 changes: 11 additions & 5 deletions proxy/request_handler.go
Original file line number Diff line number Diff line change
@@ -223,6 +223,8 @@ func (d *RequestHandler) HandleRequest(r *http.Request, rl *rule.Rule) (session
// The authentication handler says that no further authentication/authorization is required, and the request should
// be forwarded to its final destination.
// return nil
case helper.ErrUnauthorized.ErrorField:
d.r.Logger().Info(err)
default:
d.r.Logger().WithError(err).
WithFields(fields).
@@ -242,11 +244,15 @@ func (d *RequestHandler) HandleRequest(r *http.Request, rl *rule.Rule) (session

if !found {
err := errors.WithStack(helper.ErrUnauthorized)
d.r.Logger().WithError(err).
WithFields(fields).
WithField("granted", false).
WithField("reason_id", "authentication_handler_no_match").
Warn("No authentication handler was responsible for handling the authentication request")
// found will only be true if an authenticator was found AND authorisation was granted so check
// that this wasn't just a problem with the request
if err.Error() != helper.ErrUnauthorized.ErrorField {
Copy link
Member

Choose a reason for hiding this comment

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

This is always false because we set the error to err := errors.WithStack(helper.ErrUnauthorized) right before - so this can be removed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i see your point :), guess i need to think about this a bit more.

the problem is, if this if statement is removed, then invalid credentials or an expired token will result in level=warning msg=No authentication handler was responsible for handling the authentication request getting logged, which is inaccurate and misleading.

d.r.Logger().WithError(err).
WithFields(fields).
WithField("granted", false).
WithField("reason_id", "authentication_handler_no_match").
Warn("No authentication handler was responsible for handling the authentication request")
}
return nil, err
}