Skip to content

client_secret like "Abc123++" get token failed #568

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

Closed
funs690 opened this issue Jan 6, 2022 · 4 comments
Closed

client_secret like "Abc123++" get token failed #568

funs690 opened this issue Jan 6, 2022 · 4 comments
Assignees
Labels
status: invalid An issue that we don't feel is valid

Comments

@funs690
Copy link

funs690 commented Jan 6, 2022

ClientSecretBasicAuthenticationConverter line 88-89
"Abc123++" after URLDecoder.decode change to "Abc123 "

###############
Basic Authentication set client_id and client_secret in the header. So no need to URLDecoder.decode again!

@funs690 funs690 added the type: bug A general bug label Jan 6, 2022
@sjohnr sjohnr self-assigned this Jan 7, 2022
@sjohnr
Copy link
Member

sjohnr commented Jan 7, 2022

Hi @funs690. The converter is implemented to spec, per RFC 6749, Section 2.3.1. You can provide your own converter if you wish to deviate from the url encoding/decoding specified by the spec. For example:

OAuth2AuthorizationServerConfigurer<HttpSecurity> authorizationServerConfigurer =
		new OAuth2AuthorizationServerConfigurer<>();
authorizationServerConfigurer.clientAuthentication(clientAuthentication -> clientAuthentication
		.authenticationConverter(new MyClientSecretBasicAuthenticationConverter()));

If you are interested in learning more about customizing client authentication, please up-vote #540, as we are currently prioritizing what guides we work on for our reference documentation based on community feedback.

I'm going to close this issue, as the implementation is working according to the specification.

@sjohnr sjohnr closed this as completed Jan 7, 2022
@sjohnr sjohnr added status: invalid An issue that we don't feel is valid and removed type: bug A general bug labels Jan 7, 2022
@shmonika
Copy link

.authenticationConverter

Hi @sjohnr, I am facing the same issue and I tried to implement my own AuthenticationConverter with the changes as you suggested

    @Bean
    @Order(HIGHEST_PRECEDENCE)
    fun defaultSecurityFilterChain(http: HttpSecurity): SecurityFilterChain {
        val authorizationServerConfigurer = OAuth2AuthorizationServerConfigurer<HttpSecurity>()
            .clientAuthentication { clientAuthentication: OAuth2ClientAuthenticationConfigurer ->
                clientAuthentication.authenticationConverter(CustomClientSecretBasicAuthenticationConverter())
            }

        http.authorizeRequests { authorizeRequests ->
            authorizeRequests.antMatchers(
                ACTUATOR_PATH,
                TOKEN_VALIDATION_PATH
            ).permitAll()
            authorizeRequests.anyRequest().authenticated()
        }.apply(authorizationServerConfigurer)
        return http.build()
    }

However, it always defaults to ClientSecretBasicAuthenticationConverter.
Any feedback on this is appreciated. Thanks!

@sjohnr
Copy link
Member

sjohnr commented Feb 28, 2022

@shmonika, did you intend to put this code in the defaultSecurityFilterChain() method? In our samples, there are two filter chains, and the one you want to be modifying would be authorizationServerSecurityFilterChain().

@shmonika
Copy link

shmonika commented Mar 1, 2022

@shmonika, did you intend to put this code in the defaultSecurityFilterChain() method? In our samples, there are two filter chains, and the one you want to be modifying would be authorizationServerSecurityFilterChain().

@sjohnr Yeah, that was it. Thank you for your quick response :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: invalid An issue that we don't feel is valid
Projects
None yet
Development

No branches or pull requests

3 participants