-
Notifications
You must be signed in to change notification settings - Fork 611
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
SOLR-17247: Fix bug - 'WWW-Authenticate' headers missing in MultiAuthPlugin #2416
Conversation
Hi @janhoy If you have some time, would you please take a look at this? |
@@ -105,6 +110,10 @@ public void testMultiAuthEditAPI() throws Exception { | |||
new SecurityConfHandler.SecurityConfig() | |||
.setData(Utils.fromJSONString(multiAuthPluginSecurityJson))); | |||
securityConfHandler.securityConfEdited(); | |||
|
|||
// verify "WWW-Authenticate" headers are returned | |||
testWWWAuthenticateHeaders(httpClient, baseUrl); |
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.
should this test actually be starting with verify
? Instead of test
? like verifySecurityStatus
??
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.
Yes I think that make sense as testWWWAuthenticateHeaders is not a UnitTest method. Will change 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 wish I knew more about this space, overall the changes make sense...
Hi @epugh |
I'm going to ping @janhoy on this ticket... If he isnt' able to review it, I can look some more early next week... Would you mind pinging me early next week, say tuesday if you don't get another set of eyes? I have assigned the PR to me to remind me when I check my list of PR's to review that I have this one! |
Thank you very much, Eric, for your prompt answer. Yes will ping you if it doesn't get a review by then. |
…Plugin (#2416) Co-authored-by: Lamine Idjeraoui <lidjeraoui@apple.com> Co-authored-by: Eric Pugh <epugh@opensourceconnections.com>
https://issues.apache.org/jira/browse/SOLR-17247
Description
MultiAuthPlugin does not return WWW-Authenticate' headers
When returning a 401 response a Web application needs to indicate to the client what authentication challenges it supports, otherwise an exception like "HTTP protocol violation: Authentication challenge without WWW-Authenticate header“ is raised.
Solr’s MultiAuthPlugin does not supports this. With this PR Solr would return the list of supported schemes (challenges).
According to HTTP's RFC 7235:
The 401 (Unauthorized) status code indicates that the request has not
been applied because it lacks valid authentication credentials for
the target resource. The server generating a 401 response MUST send
a WWW-Authenticate header field (Section 4.1) containing at least one
challenge applicable to the target resource.
Solution
Add WWW-Authenticate' headers to error responses
Tests
Added new test case for missing WWW-Authenticate' headers
Checklist
Please review the following and check all that apply:
main
branch../gradlew check
.