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

OAuth2 auto configuration support for Eureka Client. #2563

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

daniellavoie
Copy link
Contributor

Provides support for OAuth2 on Eureka Client.

Disclaimer: Support is only provided when Eureka Client is not using Jersey.

@@ -27,6 +27,7 @@
<spring-cloud-commons.version>2.0.0.BUILD-SNAPSHOT</spring-cloud-commons.version>
<spring-cloud-config.version>2.0.0.BUILD-SNAPSHOT</spring-cloud-config.version>
<spring-cloud-stream.version>Elmhurst.BUILD-SNAPSHOT</spring-cloud-stream.version>
<spring-security-oauth2.version>2.2.1.RELEASE</spring-security-oauth2.version>
Copy link
Member

Choose a reason for hiding this comment

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

This isn't managed by boot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if there are plans to make this managed on boot. @rwinch any opinion on that? I don't need autoconfiguration, just the dependencies. We are talking about boot 2.0.

Copy link

Choose a reason for hiding this comment

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

@daniellavoie Thanks for reaching out! There are no plans for the old OAuth project version to be managed by Spring Boot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Had a chat with Rob, best option is to migrate OAuth2RestTemplate to WebClient using Spring Security 5. I'll reword that PR.

Copy link
Contributor Author

@daniellavoie daniellavoie Dec 20, 2017

Choose a reason for hiding this comment

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

Ok, it won't be as easy. Spring Security 5 doesn't support automatic token retrieval for resource servers. The old OAuth2RestTemplate handled that for us. @rwinch plans on integrating that for Security 5.1. We have 3 options:

  • Wait for Security 5.1 - See Spring Security #4921
  • Integrate the old non-managed lib until migration to Security 5.1
  • Write token retrieval logic with WebClient (could be base work for Security 5.1).

I think the most reasonable option is to wait for Security 5.1

@ryanjbaxter @spencergibb Any opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont see any reason why we need this PR right now so I dont see why we cant wait.


=== Custom authentication

For more complex needs you can create a `@Bean` of type `DiscoveryClientOptionalArgs` and
Copy link
Member

Choose a reason for hiding this comment

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

Mention this is for jersey specifically.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well not exactly true, if you use the rest template you can still extend the DiscoveryClientOptionalArgs (that's what we do on SCS)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that's right

public class BasicEurekaRestTemplateFactory implements EurekaRestTemplateFactory {
@Override
public RestTemplate newRestTemplate(String serviceUrl) {
RestTemplate restTemplate = new RestTemplate();
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldnt we use RestTemplateBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have any strong opinion on that. Pushed a new version with the builder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants