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.

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

Resolve oauth2 client-id, client-secret placeholders #8880

Closed
wants to merge 1 commit into from

Conversation

evgeniycheban
Copy link
Contributor

Closes gh-8453

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Jul 26, 2020
Copy link
Contributor

@jgrandja jgrandja left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @evgeniycheban. Please see my one comment.

@@ -104,8 +105,10 @@ public BeanDefinition parse(Element element, ParserContext parserContext) {
}
}
getOptionalIfNotEmpty(clientRegistrationElt.getAttribute(ATT_CLIENT_ID))
.map(parserContext.getReaderContext().getEnvironment()::resolvePlaceholders)
Copy link
Contributor

Choose a reason for hiding this comment

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

There may be other placeholders that need to be resolved other than client-id and client-secret and I would like to avoid adding parserContext.getReaderContext().getEnvironment()::resolvePlaceholders to every element.

If you look at this commit, could we simply register a org.springframework.beans.factory.config.PropertyPlaceholderConfigurer @Bean to take care of it?

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 for the review @jgrandja.
In this case, we do not register ClientRegistration as a bean, so we need to manually resolve property placeholders.

Maybe it would be better to move parserContext.getReaderContext().getEnvironment()::resolvePlaceholders to getOptionalIfNotEmpty method?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes you are right @evgeniycheban - ClientRegistration are not beans so that won't work.

Maybe it would be better to move parserContext.getReaderContext().getEnvironment()::resolvePlaceholders to getOptionalIfNotEmpty method?

Agreed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Aug 4, 2020
@jgrandja jgrandja self-assigned this Aug 4, 2020
@evgeniycheban evgeniycheban force-pushed the gh-8453 branch 2 times, most recently from 2f9069c to bca066d Compare August 21, 2020 22:39
@evgeniycheban evgeniycheban requested a review from jgrandja August 21, 2020 22:54
@evgeniycheban evgeniycheban force-pushed the gh-8453 branch 2 times, most recently from 4482e8a to b0f45d8 Compare August 28, 2020 12:29
@jgrandja jgrandja added this to the 5.4.0 milestone Sep 1, 2020
@jgrandja jgrandja added the status: duplicate A duplicate of another issue label Sep 1, 2020
@jgrandja
Copy link
Contributor

jgrandja commented Sep 1, 2020

Thanks for the PR @evgeniycheban ! This is now in master.

@jgrandja jgrandja closed this Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) status: duplicate A duplicate of another issue type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

security:client-registrations doesn't take propertyconfigurer properties
3 participants