spring-attic / spring-security-oauth

Support for adding OAuth1(a) and OAuth2 features (consumer and provider) for Spring web applications.
http://github.com/spring-projects/spring-security-oauth
Apache License 2.0
4.69k stars 4.04k forks source link

Customizing the error response of unauthenticated clients #483

Closed nucatus closed 4 years ago

nucatus commented 9 years ago

I really can't figure out a way to handle the errors thrown during the client authentication process. For instance, if the jdbc connection is down, I don't want this message to make its way to the client while it is trying to authenticate. Of course, I could catch that jdbc exception in the ClientDetailService implementation, but that is not elegant.

My research concluded that I don't have much control over the BasicAuthenticationFilter configured by AuthorizationServerSecurityConfigurer, consequently, I don't have a hook to plug in a custom BasicAuthenticaitonEntryPoint to handle the failure and the error handling falls back to a default implementation where the HttpServletResponse gets committed with a sendError() call.

Here is, I think, the closest I could get to solving the issue, although, my guess is that the Oauth2AuthenticationEntryPoint is not used for the BasicAuthenticationFilter in that filter stack, even though it's marked as defaultAuthenticationEntryPointFor http://stackoverflow.com/questions/30262600/customize-oauth2-error-response-on-client-authentication-with-spring-security

dsyer commented 9 years ago

The authentication entry point looks a bit messed up in the Java config, but I guess I never had to change it before (except to set the realm name which I believe is tested somewhere). I'm quite surprised you need to change it really.

Do you have some evidence that a JDBC exception leaks through to the authentication entry point as things stand?

nucatus commented 9 years ago

If we don't catch SQL exceptions in the ClientDetailsService#loadClientByClientId(String clientId), they will leak.

{"timestamp":1432051018896,"status":401,"error":"Unauthorized","message":"PreparedStatementCallback; bad SQL grammar [select \n client.CLIENT_ID,\n client.CLIENT_SECRET,\n client.AUTHORIZATION_GRANT_TYPE,\n client.REDIRECT_URI,\n client.ACCESS_TOKEN_TTL,\n client.REFRESH_TOKEN_TTL,\n application.NAME,\n client.white_listed\nfrom \n oauth_client_details client \njoin application_key \n on client.CLIENT_IDD=application_key.APP_KEY \njoin application \n on application_key.APPLICATION_ID=application.APPLICATION_ID \nwhere client.CLIENT_ID=?]; nested exception is java.sql.SQLException: ORA-00904: \"CLIENT\".\"CLIENT_IDD\": invalid identifier\n","path":"/oauth/token"}

We actually have to specifically handle this in the mentioned method to avoid this situation.

    try
    {
        // load the client
    }
    catch (EmptyResultDataAccessException e)
    {
        throw new NoSuchClientException("No client with requested id: " + clientId);
    }
    catch (Exception e)
    {
        if(e.getCause() instanceof SQLException)
        {
            LOG.error("Caught database exception which would expose SQL", e);
            throw new DataAccessResourceFailureException("There was an internal issue processing the request");
        }
        throw e;
    }
maxsap commented 9 years ago

Any plans to fix this?

dsyer commented 9 years ago

A pull request would go a long way.

EmilIfrim commented 8 years ago

Facing the same issue when sending a wrong client_id or client_secret. I'm using jdbc client details service and when NoSuchClientException is thrown, it's handled by the BasicAuthenticationEntryPoint. This adds WWW-Authenticate header and I don't want that. The recommended way is to provide a custom AuthenticationEntryPoint but this is not posible with @EnableAuthorizationServer.

Below is a sample request:

POST http://localhost:8080/rator-selfcare-rest-api/oauth/token HTTP/1.1 Accept-Encoding: gzip,deflate Content-Type: application/x-www-form-urlencoded Authorization: Basic cmF0b3Itc2VsZmNhcmUtd2ViLWFwcXpyYXRvcnNlbGZjYXJl Content-Length: 58 Host: localhost:8080 Connection: Keep-Alive User-Agent: Apache-HttpClient/4.1.1 (java 1.5)

prakashsagadevan commented 8 years ago

Hi @dsyer

Please let me know how can we customize the all JDBC Exception in Authorization server?

@nucatus , Can you please share your code to catch SQL exceptions

romanwozniak commented 8 years ago

@EmilIfrim Thanks for sharing this. I have exactly the same problem – I can't see any way to have a control over AuthenticationEntryPoint for BasicAuthenticationFilter, that was created to protect calls to /oauth/token

In case when client_id or client_secret are invalid, it uses BasicAuthenticationEntryPoint which just send error Unauthorized to the client. I need to have ability to send JSON response with an information, that client used invalid credentials.

romanwozniak commented 8 years ago

Here is my solution for this issue:

@Configuration
@EnableAuthorizationServer
protected static class AuthorizationServerConfiguration extends AuthorizationServerConfigurerAdapter {

  //...

    @Override
    public void configure(AuthorizationServerSecurityConfigurer oauthServer) throws Exception {
        oauthServer
                    .realm(RESOURCE_ID + "/client")
                    .accessDeniedHandler(accessDeniedHandler)
                    .authenticationEntryPoint(entryPoint);

        // This allows you to replace default filter for Basic authentication and customize error responses
        oauthServer.addTokenEndpointAuthenticationFilter(
                new BasicAuthenticationFilter(authenticationManager, entryPoint));
        }
}
MyGitUserAcc commented 8 years ago

@romanwozniak How can i do this in xml configuration instead of annotation based???

mohsinkerai commented 6 years ago

Hello, It is long open, any plans to fix this?

AmitParmar0530 commented 5 years ago

If we don't catch SQL exceptions in the ClientDetailsService#loadClientByClientId(String clientId), they will leak.

{"timestamp":1432051018896,"status":401,"error":"Unauthorized","message":"PreparedStatementCallback; bad SQL grammar [select \n client.CLIENT_ID,\n client.CLIENT_SECRET,\n client.AUTHORIZATION_GRANT_TYPE,\n client.REDIRECT_URI,\n client.ACCESS_TOKEN_TTL,\n client.REFRESH_TOKEN_TTL,\n application.NAME,\n client.white_listed\nfrom \n oauth_client_details client \njoin application_key \n on client.CLIENT_IDD=application_key.APP_KEY \njoin application \n on application_key.APPLICATION_ID=application.APPLICATION_ID \nwhere client.CLIENT_ID=?]; nested exception is java.sql.SQLException: ORA-00904: \"CLIENT\".\"CLIENT_IDD\": invalid identifier\n","path":"/oauth/token"}

We actually have to specifically handle this in the mentioned method to avoid this situation.

    try
    {
        // load the client
    }
    catch (EmptyResultDataAccessException e)
    {
        throw new NoSuchClientException("No client with requested id: " + clientId);
    }
    catch (Exception e)
    {
        if(e.getCause() instanceof SQLException)
        {
            LOG.error("Caught database exception which would expose SQL", e);
            throw new DataAccessResourceFailureException("There was an internal issue processing the request");
        }
        throw e;
    }

I tried to handle the SQLException in UserDetailsService.java, However, it calls the commence() method of BasicAuthenticationEntryPoint.java instead of spring Exception handler class, Hence it throws an unauthorized exception. I want to throw it as server error as my database connection is lost.

murilo8812 commented 5 years ago

The simple accessDeniedHandler() and authenticationEntryPoint() build methods from AuthorizationServerSecurityConfigurer should solve the problem. Why is it so hard to implement this?

SeunMatt commented 5 years ago

Hi guys,

Here are my findings:

I'm using SpringBoot: 2.1.4.RELEASE spring-security-oauth2: 2.3.5.RELEASE

What I want: I want to be able to customize the message format when there's an authentication error on my AuthorizationServer and see some helpful logs when an authentication error happens.

What I did:

I followed @romanwozniak's suggestion and hope it'll work but it didn't. I did this:

@Override
public void configure(AuthorizationServerSecurityConfigurer security) {

    security.checkTokenAccess("isAuthenticated()")
            .tokenKeyAccess("permitAll()")
            .passwordEncoder(passwordEncoder);

    security.authenticationEntryPoint(myBasicAuthenticationEntryPoint);
    security.accessDeniedHandler(((request, response, accessDeniedException) -> {
        logger.debug("access denied handler called");
    }));
    BasicAuthenticationFilter basicAuthenticationFilter = new BasicAuthenticationFilter(authenticationManager, myBasicAuthenticationEntryPoint);
    security.addTokenEndpointAuthenticationFilter(basicAuthenticationFilter);

}

In summary, calling security.tokenEndpointAuthenticationFilters() or security.addTokenEndpointAuthenticationFilter() will disable/ignore the configured ClientDetailsService in the ClientDetailsServiceConfigurer:

@Override
public void configure(ClientDetailsServiceConfigurer clients) throws Exception {

      //clients.jdbc(dataSource());

    clients.withClientDetails((clientId -> {
        logger.debug("client details service 1 called: " + clientId);
        OAuthClient oAuthClient = oAuthClientRepository.findById(clientId).orElse(null);
        if(Objects.isNull(oAuthClient)) {
            logger.debug("OAuth Client record not found for " + clientId);
            throw new ClientRegistrationException("Client " + clientId + " does not exist");
        }
        BaseClientDetails clientDetails = new BaseClientDetails(clientId, oAuthClient.getResourceIds(), oAuthClient.getScope(), oAuthClient.getAuthorizedGrantTypes(),
                oAuthClient.getAuthorities());
        clientDetails.setClientSecret(oAuthClient.getClientSecret());
        clientDetails.setAccessTokenValiditySeconds(oAuthClient.getAccessTokenValidity());
        clientDetails.setRefreshTokenValiditySeconds(oAuthClient.getRefreshTokenValidity());
        logger.debug("client details: " + clientDetails);
        return clientDetails;
    }));
}

Rather it'll be using the UserDetailsService configured in a class that extends the WebSecurityConfigurerAdapter:

@Override
protected void configure(AuthenticationManagerBuilder auth) throws Exception {
    String userSecret = passwordEncoder().encode("secret");
    auth.inMemoryAuthentication().passwordEncoder(passwordEncoder())
            .withUser("user").password(userSecret).roles("USER");
}

By my guess, this is so because I'm passing in the autowired AuthenticationManager to the constructor of the BasicAuthenticationFilter. Remember that the AuthenticationManager was originally created in the WebSecurityConfigurerAdapter.

if I did not configure/override the AuthenticationManagerBuilder, as shown above, in the WebSecurityConfigurerAdapter i.e. protected void configure(AuthenticationManagerBuilder auth) throws Exception { ... } I'll get an Internal Server Error due to java.lang.StackOverflowError: null

if I override it without configuring it, I will get No AuthenticationProvider found for org.springframework.security.authentication.UsernamePasswordAuthenticationToken

So the problem seems to lie in calling security.tokenEndpointAuthenticationFilters(). It seems to cause the BasicAuthenticationFilter to ignore any configured ClientDetailsService.

Also note, from my PasswordEncoder logging, the database-password supplied for matching the user-supplied secret is userNotFoundPassword when security.tokenEndpointAuthenticationFilters() or security.addTokenEndpointAuthenticationFilter() is called.

Moreover, calling security.authenticationEntryPoint(myBasicAuthenticationEntryPoint); alone does not invoke myBasicAuthenticationEntryPoint when an error such as authentication error occurs.

Moreover configuring endpoints.setClientDetailsService() does not have effect as the provided implementation is not evoked. Same goes for endpoints.userDetailsService() in the method override below:

@Override
public void configure(AuthorizationServerEndpointsConfigurer endpoints) {
  //...
}

What led me to all this is because I'm supplying the correct client credentials, same as I used on my local machine and it worked, to my remote server and it didn't work. I'm not getting any logs whatsoever and didn't even know where to start. Now, I'm going to provide a custom password encoder and a custom clientDetails so I can see what's going on.

However, if anyone by any chance is able to figure out how to make custom BasicAuthenticationFilter work please do a brother a favour 🙏🏽and ping me. Thanks

@dsyer I'm pinging you because I can see your name as an author all over the configuration classes 😃

OrangeDog commented 5 years ago

To summarise, AuthorizationServerSecurityConfigurer#authenticationEntryPoint is non-functional. The final BasicAuthenticationFilter always has the default DelegatingAuthenticationEntryPoint.

A common case where someone wants to replace it is to handle the InternalAuthenticationServiceExceptions coming out of JdbcClientDetailsService.

@dsyer should the label change from question to bug?

OrangeDog commented 5 years ago

It looks like #1698 might fix this?

jgrandja commented 4 years ago

It feels like this is a question that would be better suited to Stack Overflow. As mentioned in the guidelines for contributing, we prefer to use GitHub issues only for bugs and enhancements. Feel free to update this issue with a link to the re-posted question (so that other people can find it) or add some more details if you feel this is a genuine bug.

Please also see this comment for further details.