Implement OpenID client registration endpoint

[ghost]

See: https://openid.net/specs/openid-connect-registration-1_0.html#ClientRegistration

Fixes gh-57

[ghost]

Hi @jgrandja . Happy New Year!

I opened a draft PR as I still have to finish the unit tests, javadoc and some polishing to double check that I did not missed anything.

The implementation should be more or less ready and I think it would be a good to have a fast feedback. Looking forward to your comments :)

[jgrandja]

Happy new year @ovidiupopa91 ! I hope you had a great holiday break :)

I took a quick glance at the changes and it looks good to me - it follows the implementation patterns of OidcProviderConfigurationEndpointFilter.

The one minor thing I noticed in OAuth2AuthorizationServerConfigurer is this was prepended to this.postProcess(... FYI, instance members should be prefixed with this but NOT instance methods. Also, try to limit the changes to the code you are implementing only. This is very minor but wanted to mention it now.

I also noticed that the build is failing but I'm guessing because this is a draft anyway and you're aware of it.

Just a heads up that I likely won't be doing a detailed review for at least 2 weeks as I'm trying to ramp up on a couple of higher priority items for 0.1.0 release later this month. This will likely get merged into 0.1.1.

Thanks for your work!

[dfcoffin]

The PR build failures are all due to the same issues:

FAILURE: Build failed with an exception.

• What went wrong:
  Execution failed for task ':spring-security-oauth2-authorization-server:checkstyleMain'.
  Deprecated Gradle features were used in this build, making it incompatible with Gradle 7.0.

Checkstyle rule violations were found. See the report at: file:///home/runner/work/spring-authorization-server/spring-authorization-server/oauth2-authorization-server/build/reports/checkstyle/main.html
Checkstyle files with violations: 2
Checkstyle violations by severity: [error:2]

[ghost]

Yep, I am aware that the build is failing. There are two issues (copyright and a whitespace). I will fix both of them with the next push.

[joshuatcasey]

Hi @ovidiupopa91 ! Glad to see you are working on this. I think it would be cool if this functionality were enabled/disabled via configuration. I can see cases where the owner of the authorization server might not want to allow new client registration via this endpoint.

[jgrandja]

@joshuatcasey The default configuration will NOT enable the registration endpoint. You have to opt-in via custom configuration. This will reside in OAuth2AuthorizationServerConfigurer but we haven't gotten that far yet.

[jgrandja]

@ovidiupopa91 I'm finally circling back to this now!

FYI, I updated the main issue #57 comment with requirements for the "Initial Access Token". Let me know if you have any questions.

[ghost]

hi @jgrandja. Only one thing that is not very clear to me: (initial) access token that was obtained using the client_credentials grant.

This means that when calling the token endpoint (client_credentials grant_type), OAuth2AccessTokenAuthenticationToken \ OAuth2AccessTokenResponse will be extended with a new property initial access token ?
Or a different call will be made with scope client.create to get the initial access token?

[jgrandja]

@ovidiupopa91

Or a different call will be made with scope client.create to get the initial access token?

Correct. An existing registered client will obtain an access token via the client_credentials grant and request the client.create scope - simply leveraging the logic that exists. After the client receives the access token, it will call the registration endpoint to register the new client. After registration is successful, the access token should be revoked as it can only be used once. The "initial" access token has restricted / limited privileges as it can only be used at the registration endpoint and only one time.

[ghost]

hi @jgrandja I will try yo wrap everything up today (convert the PR from draft to ready for review)
There's one thing that I want to double check before pushing the changes. As the client registration is a protected resource, is it safe to assume that your expectation would be to protect it by creating a new AuthenticationProvider?

[jgrandja]

@ovidiupopa91

the client registration is a protected resource, is it safe to assume that your expectation would be to protect it by creating a new AuthenticationProvider.

No, there is no need to create a new AuthenticationProvider. When enabling HttpSecurity.oauth2ResourceServer().jwt(), this will add BearerTokenAuthenticationFilter and JwtAuthenticationProvider to authenticate the Jwt.

We just need to ensure that OidcClientRegistrationEndpointFilter sits behind FilterSecurityInterceptor, just like OAuth2TokenEndpointFilter and OAuth2TokenRevocationEndpointFilter - see OAuth2AuthorizationServerConfigurer.configure().

As well, in OAuth2AuthorizationServerConfiguration.applyDefaultSecurity(), we will need to update to something like this:

http
	...
	.authorizeRequests(authorizeRequests ->
		authorizeRequests
			.antMatchers("/connect/register").access("hasAuthority('SCOPE_client.create')")
			.anyRequest().authenticated()
	)
	.oauth2ResourceServer(OAuth2ResourceServerConfigurer::jwt)
	...

We will need to take into account that the Client Registration endpoint URI may be customized via ProviderSettings so the hardcoding won't work in the above configuration.

[ghost]

@jgrandja awesome this is how I did it. But I had second thoughts right at the end and wanted to check with you that I am not missing anything. Thank you.

[jgrandja]

Excellent @ovidiupopa91 !

[ghost]

@jgrandja I was quite busy in the last couple of days -> had not time to finish the PR.

After it's used, the access token must be invalidated, as it can be used only once. There's one small issue here. It's not possible to call OAuth2AuthenticationProviderUtils.invalidate because the method is package private and the OidcClientRegistrationFilter is located in the oidc package.
Do you have any preference on how to implement this change/requirement?

[jgrandja]

@ovidiupopa91 No rush. I'm quite back logged anyway.

Do you have any preference on how to implement this change/requirement?

For now, just duplicate the logic in the Filter. We can re-factor and share the logic at a later point. Please put a FIXME note as a reminder to come back to it. Thanks!

[jgrandja]

@ovidiupopa91 Just a reminder to let me know when this is ready for review. Thanks!

UPDATE: I just saw that you marked it for review. I will start it first thing tomorrow.

[jgrandja]

Thanks for the PR @ovidiupopa91 ! Please see review comments.

[ghost]

Hi @jgrandja. This is now ready for a new round of review.
All comments should be addressed and marked as resolved.

Looking forward to the next comments 👍

[jgrandja]

@ovidiupopa91 Apologies for the delay in the review. I'll be circling back to this later this week.

[jgrandja]

Thanks for all the work on this feature @ovidiupopa91. This is now merged!

FYI, I needed to move some of the logic in the Filter to the AuthenticationProvider.
I also applied a few other updates in the polish commit.

Please let me know if you have any questions.
Thanks!

[ghost]

Hi there @jgrandja. It looks a lot better after the polish commit. I have only one question. I noticed that you removed the oidcClientRegistrationenabled flag from ProviderSettings. Is there any other way to implement this feature?

Also let me know if there is any other feature that I could start looking at. Thanks!

[jgrandja]

@ovidiupopa91 The next release will be focusing on some of the items in #139 and this is where the equivalent of oidcClientRegistrationenabled will be added. It won't be implemented as a ProviderSettings but instead will be integrated indirectly through OAuth2AuthorizationServerConfigurer. I'm still thinking of the design but both OAuth2AuthorizationServerConfigurer and OAuth2AuthorizationServerConfiguration will be enhanced to enable greater customization.

I'm going to plan out the features for 0.1.2 this week. As soon as I have a plan, I'll reach out to you to see which one you're interested on taking on. Thanks!

[jgrandja]

@ovidiupopa91 Would you be interested in gh-245? If not, I can find another one.

[ghost]

Hi @jgrandja . Sure, I will take a look at that one.

[jgrandja]

Thanks @ovidiupopa91. Can you please respond in that issue asking for it so I can assign it to you.