-
Notifications
You must be signed in to change notification settings - Fork 268
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
[ELY-1466] Http Basic auth - silent mode #1218
[ELY-1466] Http Basic auth - silent mode #1218
Conversation
Can one of the admins verify this patch? |
Haven't reviewed the rest of this PR fully yet but one thing to note is that the commit message and PR title should reference the ELY issue instead of the WFLY issue. |
//if silent we only send a challenge if the request contained auth headers | ||
//otherwise we assume another method will send the challenge | ||
String authHeader = request.getFirstRequestHeaderValue(AUTHORIZATION); | ||
if(authHeader == null) { |
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.
Shouldn't be checked here that Authorization header is of Basic mechanism? Can it happen this mechanism can see Digest mechanism header?
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 if there is Digest mechanism header, it might have successful credentials and so it is better to not send challenge in this case also.
c41480b
to
f4655be
Compare
@@ -107,7 +108,7 @@ public HttpServerAuthenticationMechanism createAuthenticationMechanism(String me | |||
|
|||
switch (mechanismName) { | |||
case BASIC_NAME: | |||
return new BasicAuthenticationMechanism(callbackHandler, (String) properties.get(CONFIG_REALM), false); | |||
return new BasicAuthenticationMechanism(callbackHandler, (String) properties.get(CONFIG_REALM), Boolean.valueOf((String) properties.get(SILENT)), false); |
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.
Just a minor comment, I think you could use Boolean.parseBoolean
here instead.
f4655be
to
cea0d34
Compare
@darranl I think we can go ahead and merge this one if it looks ok to you. (QE has finished pre-checking it.) |
JIRA issues:
https://issues.jboss.org/browse/EAP7-1154
https://issues.jboss.org/browse/ELY-1466
https://issues.jboss.org/browse/WFLY-11479
Analysis doc:
https://github.com/wildfly/wildfly-proposals/blob/b970200ba0662d1c98268977c2f77ca3a941c649/security/WFLY-11479_HTTP_Basic_auth_silent_mode.adoc