-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
basic_auth: add WWW-Authenticate header for 401 response #34214
Conversation
Hi @sdwbgn, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
/retest |
decoder_callbacks_->sendLocalReply( | ||
Http::Code::Unauthorized, body, | ||
[uri = this->original_uri_](Http::ResponseHeaderMap& headers) { | ||
std::string value = absl::StrCat("Basic realm=\"", uri, "\""); | ||
headers.setCopy(Http::Headers::get().WWWAuthenticate, value); | ||
}, | ||
absl::nullopt, response_code_details); |
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.
Thanks for the contribution. LGTM overall.
But I don't think we need to create and store the original_uri_
as a member of filter. You could simply capture this
in the lambda and build the value in the lambda.
Tips: You can get request headers by the decoder_callbacks_->requestHeaders()
in the lambda.
/wait |
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
6fa48d8
to
be5a3e0
Compare
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
828c929
to
21fb89d
Compare
/retest |
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.
Thanks. LGTM with one nit comment. :)
const auto request_headers = this->decoder_callbacks_->requestHeaders(); | ||
const std::string uri = Http::Utility::buildOriginalUri(*request_headers, MaximumUriLength); | ||
const std::string value = absl::StrCat("Basic realm=\"", uri, "\""); | ||
headers.setCopy(Http::Headers::get().WWWAuthenticate, value); |
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.
nit: setReferenceKey(...)
Signed-off-by: Vasilii Bulatov <sdwbgn@gmail.com>
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.
LGTM. Thanks.
Commit Message: Adds valid WWW-Authenticate header for 401 response in Basic Auth filter
Additional Description:
Risk Level: Low
Testing: unit test, integration
Docs Changes: N/A
Release Notes: Set
WWW-Authenticate
header for 401 responses from the Basic Auth filter.Platform Specific Features: N/A
Fixes #34015