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

Add OAuth2-Client ConnectionDetails and Keycloak Docker-Compose support #39391

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

PhilKes
Copy link
Contributor

@PhilKes PhilKes commented Feb 3, 2024

Closes #36777
This PR includes:

  • Adding the ConnectionDetails abstraction for OAuth2 Registration + Provider
  • Docker-Compose Support for the keycloak/keycloak Image

Open Questions:

  • Since the Registration has a lot fields that cant simply be extracted from any Keycloak env. variables, I added Docker service labels to set e.g. the scope of the client, or the client secret. Is this the best way to handle this?

    I also thought about not ignoring the PropertiesOAuth2ClientConnectionDetails if the Docker-Compose support already created an OAuth2ClientConnectionDetails Bean, but rather merge them together, so that the registration-id, provider are extracted from the Keycloak env. variables and the other settings (scope, client-secret, etc.) could be set in the application.properties. I am not sure which to prefer.

When the open questions are answered, I would also add the TestContainer-Support for Keycloak.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 3, 2024
@philwebb philwebb added for: team-meeting An issue we'd like to discuss as a team to make progress type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged for: team-meeting An issue we'd like to discuss as a team to make progress labels Apr 19, 2024
@philwebb philwebb added this to the 3.4.x milestone Apr 24, 2024
@PhilKes
Copy link
Contributor Author

PhilKes commented May 7, 2024

Any opinions on this @philwebb , @mhalbritter? 😀

@mhalbritter
Copy link
Contributor

We're currently preparing the Spring Boot 3.3 release. This PR is planned as an enhancement for 3.4 and we'll look into it in detail when 3.3 is out.

@wilkinsona
Copy link
Member

Thanks for the PR, @PhilKes. Can you please describe how you identified the properties that should be part of OAuth2ClientConnectionDetails? My first impression is that there are too many of them but I may be overlooking something.

The four different labels that are being used to supply some of the values feels like a smell to me as it tells me that these settings (probably) aren't coming from the Docker Compose-managed service but from user configuration. Typically, we'd want the user's standard configuration to be reused (so that their use of Docker Compose matches as closely as possible to running their application "properly") and where test-specific configuration is needed, it can be provided through application properties rather than Docker Compose labels.

@wilkinsona wilkinsona added status: waiting-for-feedback We need additional information before we can continue labels Jun 5, 2024
@PhilKes
Copy link
Contributor Author

PhilKes commented Jun 6, 2024

Thanks for the PR, @PhilKes. Can you please describe how you identified the properties that should be part of OAuth2ClientConnectionDetails? My first impression is that there are too many of them but I may be overlooking something.

The four different labels that are being used to supply some of the values feels like a smell to me as it tells me that these settings (probably) aren't coming from the Docker Compose-managed service but from user configuration. Typically, we'd want the user's standard configuration to be reused (so that their use of Docker Compose matches as closely as possible to running their application "properly") and where test-specific configuration is needed, it can be provided through application properties rather than Docker Compose labels.

Thanks for your remarks @wilkinsona
I wasnt really sure what the minimum amount of properties were, since it's not as trivial as e.g. host + user + password, so I simply copied the attributes from OAuth2ClientProperties for a first draft.
Now I suppose the bare minimum for OAuth2ConnectionDetails would be:

  • Registration:
    • clientId
    • clientSecret
    • redirectUri
    • provider
  • Provider:
    • issuerUri?

All other properties should then only be set via OAuth2ClientProperties?

About the labels, the clientId and clientSecret are set for a Registration which has a specified name. If the user defined a Registration in the application.properties, how do we match that Registration to the Provider in e.g. KeycloakDockerComposeConnectionDetailsFactory? In theory there could be more than 1 Registration or Provider, so how do we know which Provider and Registation name should be used in KeycloakDockerComposeConnectionDetailsFactory?

Thank you for your feedback.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jun 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: feedback-provided Feedback has been provided type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ConnectionDetails for external providers in Spring Security modules
5 participants