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.

Sign up for GitHub

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

Proposed fix for missing WWW-Authenticate header #1877

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Proposed fix for missing WWW-Authenticate header #1877

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
16 changes: 15 additions & 1 deletion ...authorization/config/annotation/web/configurers/OAuth2ClientAuthenticationConfigurer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ public final class OAuth2ClientAuthenticationConfigurer extends AbstractOAuth2Co

private AuthenticationFailureHandler errorResponseHandler;

private String realmName = "oauth";

/**
* Restrict for internal use only.
* @param objectPostProcessor an {@code ObjectPostProcessor}
Expand All @@ -102,6 +104,18 @@ public OAuth2ClientAuthenticationConfigurer authenticationConverter(
return this;
}

/**
* Sets the realm name for Http Basic when returning a WWW-Authenticate header on
* client authentication failure.
* @param realmName the Http Basic realm name
* @return the {@link OAuth2ClientAuthenticationConfigurer} for further configuration
*/
public OAuth2ClientAuthenticationConfigurer realmName(String realmName) {
Assert.hasText(realmName, "realmName cannot be empty");
this.realmName = realmName;
return this;
}

/**
* Sets the {@code Consumer} providing access to the {@code List} of default and
* (optionally) added {@link #authenticationConverter(AuthenticationConverter)
Expand Down Expand Up @@ -213,7 +227,7 @@ void init(HttpSecurity httpSecurity) {
void configure(HttpSecurity httpSecurity) {
AuthenticationManager authenticationManager = httpSecurity.getSharedObject(AuthenticationManager.class);
OAuth2ClientAuthenticationFilter clientAuthenticationFilter = new OAuth2ClientAuthenticationFilter(
authenticationManager, this.requestMatcher);
authenticationManager, this.requestMatcher, this.realmName);
List<AuthenticationConverter> authenticationConverters = createDefaultAuthenticationConverters();
if (!this.authenticationConverters.isEmpty()) {
authenticationConverters.addAll(0, this.authenticationConverters);
Expand Down
26 changes: 12 additions & 14 deletions ...gframework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,8 @@ public final class OAuth2ClientAuthenticationFilter extends OncePerRequestFilter

private final AuthenticationDetailsSource<HttpServletRequest, ?> authenticationDetailsSource = new WebAuthenticationDetailsSource();

private final String realmName;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove realmName as the framework would not make much use of it. Furthermore, the implicit "realm" where the client credentials are authenticated against is the single instance of RegisteredClientRepository.

The "realm" parameter is mainly used for user-based authentication.


private AuthenticationConverter authenticationConverter;

private AuthenticationSuccessHandler authenticationSuccessHandler = this::onAuthenticationSuccess;
Expand All @@ -104,10 +106,12 @@ public final class OAuth2ClientAuthenticationFilter extends OncePerRequestFilter
* @param requestMatcher the {@link RequestMatcher} used for matching against the
* {@code HttpServletRequest}
*/
public OAuth2ClientAuthenticationFilter(AuthenticationManager authenticationManager,
RequestMatcher requestMatcher) {
public OAuth2ClientAuthenticationFilter(AuthenticationManager authenticationManager, RequestMatcher requestMatcher,
String realmName) {
this.realmName = realmName;
Assert.notNull(authenticationManager, "authenticationManager cannot be null");
Assert.notNull(requestMatcher, "requestMatcher cannot be null");
Assert.notNull(realmName, "realmName cannot be null");
this.authenticationManager = authenticationManager;
this.requestMatcher = requestMatcher;
// @formatter:off
Expand Down Expand Up @@ -140,9 +144,12 @@ protected void doFilterInternal(HttpServletRequest request, HttpServletResponse
validateClientIdentifier(authenticationRequest);
Authentication authenticationResult = this.authenticationManager.authenticate(authenticationRequest);
this.authenticationSuccessHandler.onAuthenticationSuccess(request, response, authenticationResult);
filterChain.doFilter(request, response);
}
else {
this.authenticationFailureHandler.onAuthenticationFailure(request, response,
new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_CLIENT));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Define private static final String MISSING_CLIENT_AUTH_ERROR_CODE = "missing_client_auth" and use that as the error code instead of INVALID_CLIENT. Then the authenticationFailureHandler can evaluate that specific error code and default to 401 WWW-Authenticate: Basic.

}
filterChain.doFilter(request, response);

}
catch (OAuth2AuthenticationException ex) {
if (this.logger.isTraceEnabled()) {
Expand Down Expand Up @@ -204,20 +211,11 @@ private void onAuthenticationFailure(HttpServletRequest request, HttpServletResp

SecurityContextHolder.clearContext();

// TODO
// The authorization server MAY return an HTTP 401 (Unauthorized) status code
// to indicate which HTTP authentication schemes are supported.
// If the client attempted to authenticate via the "Authorization" request header
// field,
// the authorization server MUST respond with an HTTP 401 (Unauthorized) status
// code and
// include the "WWW-Authenticate" response header field
// matching the authentication scheme used by the client.

OAuth2Error error = ((OAuth2AuthenticationException) exception).getError();
ServletServerHttpResponse httpResponse = new ServletServerHttpResponse(response);
if (OAuth2ErrorCodes.INVALID_CLIENT.equals(error.getErrorCode())) {
httpResponse.setStatusCode(HttpStatus.UNAUTHORIZED);
httpResponse.getHeaders().set("WWW-Authenticate", "Basic realm=\"" + this.realmName + "\"");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Only return WWW-Authenticate: Basic when error code is MISSING_CLIENT_AUTH_ERROR_CODE OR when the HttpServletRequest contains the header Authorization Basic.

}
else {
httpResponse.setStatusCode(HttpStatus.BAD_REQUEST);
Expand Down
6 changes: 3 additions & 3 deletions ...ework/security/oauth2/server/authorization/web/OAuth2ClientAuthenticationFilterTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public class OAuth2ClientAuthenticationFilterTests {
public void setUp() {
this.authenticationManager = mock(AuthenticationManager.class);
this.requestMatcher = new AntPathRequestMatcher(this.filterProcessesUrl, HttpMethod.POST.name());
this.filter = new OAuth2ClientAuthenticationFilter(this.authenticationManager, this.requestMatcher);
this.filter = new OAuth2ClientAuthenticationFilter(this.authenticationManager, this.requestMatcher, "realm");
this.authenticationConverter = mock(AuthenticationConverter.class);
this.filter.setAuthenticationConverter(this.authenticationConverter);
}
Expand All @@ -93,14 +93,14 @@ public void cleanup() {

@Test
public void constructorWhenAuthenticationManagerNullThenThrowIllegalArgumentException() {
assertThatThrownBy(() -> new OAuth2ClientAuthenticationFilter(null, this.requestMatcher))
assertThatThrownBy(() -> new OAuth2ClientAuthenticationFilter(null, this.requestMatcher, "realm"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("authenticationManager cannot be null");
}

@Test
public void constructorWhenRequestMatcherNullThenThrowIllegalArgumentException() {
assertThatThrownBy(() -> new OAuth2ClientAuthenticationFilter(this.authenticationManager, null))
assertThatThrownBy(() -> new OAuth2ClientAuthenticationFilter(this.authenticationManager, null, "realm"))
.isInstanceOf(IllegalArgumentException.class)
.hasMessage("requestMatcher cannot be null");
}
Expand Down