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

security:client-registrations doesn't take propertyconfigurer properties #8453

Closed
richardcs opened this issue May 2, 2020 · 8 comments
Closed
Assignees
Labels
in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement
Milestone

Comments

@richardcs
Copy link

richardcs commented May 2, 2020

Using Spring Security 5.3.1.RELEASE

I use XML based configuration for most security setup as I have customizations that need to be dynamically processed.

Using <security:client-registrations... Everything works fine if I hardcode the client-id and client-secret but using a propertyConfigurer the values aren't used.

I have this setup for the propertyConfigurer

<context:property-placeholder location="file:${app.properties}"/>

app.properties contains these values:

oauth2.client.id=<redacted>
oauth2.client.secret=<redacted>

client-registrations is setup like this:

<security:client-registrations>
    <security:client-registration registration-id="github"
                         client-id="${oauth2.client.id}"
                         client-secret="${oauth2.client.secret}"
                         provider-id="github"/>
  </security:client-registrations>

Values are not propagated even though in other beans (not in the security namespace) are propagated through the propertyConfigurer.

@richardcs richardcs added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels May 2, 2020
@jgrandja
Copy link
Contributor

jgrandja commented May 4, 2020

@richardcs Property placeholders are not taken into account with the initial support for xml config. This would be an enhancement we can certainly add.

Would you be interested in submitting a PR for this?

@jgrandja jgrandja added in: oauth2 An issue in OAuth2 modules (oauth2-core, oauth2-client, oauth2-resource-server, oauth2-jose) type: enhancement A general enhancement status: ideal-for-contribution An issue that we actively are looking for someone to help us with and removed status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels May 4, 2020
@richardcs
Copy link
Author

I can definitely take a look. Could you give me some pointers about how that enhancement would be added?

@jgrandja
Copy link
Contributor

jgrandja commented May 5, 2020

@richardcs I would have to spend some time myself to figure out how it can be done. Please see if you can figure it out and if you're having issues then I can jump in and help out.

@rwinch
Copy link
Member

rwinch commented May 6, 2020

@richardcs Thanks for looking into this See if this commit helps you 4542f00

@evgeniycheban
Copy link
Contributor

I can work on it.

@jgrandja
Copy link
Contributor

Thanks @evgeniycheban ! The issue is yours.

@jgrandja jgrandja removed the status: ideal-for-contribution An issue that we actively are looking for someone to help us with label Jul 23, 2020
@evgeniycheban
Copy link
Contributor

@jgrandja I've submitted PR.
Please take a look when you have a moment.

evgeniycheban added a commit to evgeniycheban/spring-security that referenced this issue Aug 28, 2020
@jgrandja jgrandja added this to the 5.4.0 milestone Sep 1, 2020
@abhishek-bafna-amdhan
Copy link

@jzheaux I am wondering if a similar issue might also apply to SAML integration within Spring Security. I have encountered the same issue and created an issue #14645

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) type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants