Skip to content

Return registration_endpoint in OidcProviderConfigurationEndpointFilter #762

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Return registration_endpoint in OidcProviderConfigurationEndpointFilter #762

wants to merge 1 commit into from

Conversation

sahariardev
Copy link
Contributor

Before: client registration endpoint was not retuned in oidc
Provider Configuration response

After: Returns client registration endpoint in oidc provider configuration
response if client registration is enabled

Fixes gh-370

@sahariardev sahariardev mentioned this pull request Jun 3, 2022
Closed
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jun 3, 2022
@jgrandja jgrandja self-assigned this Jun 14, 2022
@jgrandja jgrandja added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 14, 2022
jgrandja
jgrandja reviewed Jun 14, 2022
View reviewed changes
...k/security/oauth2/server/authorization/oidc/web/OidcProviderConfigurationEndpointFilter.java Outdated
this(providerSettings, false);
}

public OidcProviderConfigurationEndpointFilter(ProviderSettings providerSettings, boolean dynamicClientRegistrationEnabled) {
Copy link
Collaborator

@jgrandja jgrandja Jun 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@sahariardev Adding the flag dynamicClientRegistrationEnabled to the constructor will not provide us the flexibility we need. There will be other features that are enabled/disabled and adding more flags to constructor provides limited flexibility and API will constantly change.

Copy link
Contributor Author

@sahariardev sahariardev Jun 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jgrandja,
Thanks for your feedback.
One thing we can do is keep this flag inside provider settings as this is a provider related settings. What do you think? Or do you have any better idea for keeping this flag?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sahariardev

One thing we can do is keep this flag inside provider settings

I'd prefer to find an approach that doesn't introduce an "enabled" setting in ProviderSettings. The less settings there are the easier it will be to use and maintain. Let's try to find a way to implement this without a setting. We know it's disabled by default so we just need to know when it's enabled (based on custom configuration) and then apply the logic in OidcProviderConfigurationEndpointFilter. Please give it some further thought. No rush.

FYI, I'm on PTO starting this coming Wed and away for just over 2 weeks.

Copy link
Contributor Author

@sahariardev sahariardev Jun 24, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your feedback, will try to find a way out

Copy link
Contributor Author

@sahariardev sahariardev Jul 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jgrandja,
I have updated the PR. Please have a look and provider your feedback.

@jgrandja jgrandja added the status: duplicate A duplicate of another issue label Jun 14, 2022
jgrandja
jgrandja reviewed Jul 18, 2022
View reviewed changes
...k/security/oauth2/server/authorization/oidc/web/OidcProviderConfigurationEndpointFilter.java Outdated

private boolean isDynamicClientRegistrationEnabled() {
DefaultSecurityFilterChain defaultSecurityFilterChain = (DefaultSecurityFilterChain) WebApplicationContextUtils
.getRequiredWebApplicationContext(getServletContext()).getBean("authorizationServerSecurityFilterChain");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the updates @sahariardev.

We cannot rely on the bean being called authorizationServerSecurityFilterChain. The consuming application could name the bean to something different and then this logic would not work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sahariardev I might have an idea on how to implement this as part of the work involved for gh-616. Let me think about it for a bit and I'll get back to you.

Copy link
Contributor Author

@sahariardev sahariardev Jul 20, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Okay thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sahariardev Sorry for the delayed response. I haven't had a chance to revisit this yet. I have some re-factoring tasks that are higher priority at the moment. I will touch base as soon as I get a chance to revisit this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, no problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for your patience @sahariardev. I just merged gh-616 via 0994a1e.

Based on the updates in that commit, the following code (add to OidcConfigurer.configure()) will allow you to determine when oidc.clientRegistrationEndpoint() is enabled so you can apply the registration_endpoint configuration:

OidcClientRegistrationEndpointConfigurer clientRegistrationEndpointConfigurer =
		getConfigurer(OidcClientRegistrationEndpointConfigurer.class);
if (clientRegistrationEndpointConfigurer != null) {
	OidcProviderConfigurationEndpointConfigurer providerConfigurationEndpointConfigurer =
			getConfigurer(OidcProviderConfigurationEndpointConfigurer.class);
	providerConfigurationEndpointConfigurer.addDefaultProviderConfigurationCustomizer(
			builder -> builder.clientRegistrationEndpoint("https://localhost/connect/register"));
}

Let me know if you have any questions. Thanks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, please rebase the PR off of 0.4.x branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jgrandja. I will update those ASAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @jgrandja,
I have started working on this. As I was facing some trouble to rebase with 0.4.x, I had do close this one and opening a new PR. Will let you know the updates ASAP.

Copy link
Contributor Author

@sahariardev sahariardev Sep 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Here is the new PR link. let's continue our discussion there.

#881

@sahariardev
Return registration_endpoint in OidcProviderConfigurationEndpointFilter
22622ad 
Before: client registration endpoint was not retuned in oidc
Provider Configuration response

After: Returns client registration endpoint in oidc provider configuration
response if client registration is enabled

Fixes gh-370
@sahariardev sahariardev closed this by deleting the head repository Sep 2, 2022
@sahariardev sahariardev mentioned this pull request Sep 2, 2022
Closed
@jgrandja jgrandja added status: declined A suggestion or change that we don't feel we should currently apply and removed status: duplicate A duplicate of another issue type: enhancement A general enhancement labels Sep 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgrandja jgrandja jgrandja left review comments

Assignees

@jgrandja jgrandja

Labels
status: declined A suggestion or change that we don't feel we should currently apply
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return registration_endpoint in OidcProviderConfigurationEndpointFilter
3 participants
@sahariardev @jgrandja @spring-projects-issues