Support user info in Elasticsearch URIs #21381

evgeniycheban · 2020-05-11T00:47:32Z

wilkinsona · 2020-05-11T07:01:05Z

Thanks very much for the PR, @evgeniycheban

Could you please take a look at adding some tests for the new functionality? I think they could be implemented as additions to org.springframework.boot.autoconfigure.elasticsearch.ElasticsearchRestClientAutoConfigurationTests. It looks like you can get the nodes from the RestClient to verify that the configuration has been applied correctly. As part of the tests, it would be good to verify the behaviour of URIs with and without any user info and how the separate username and password properties are used.

evgeniycheban · 2020-05-11T16:13:25Z

Thanks very much for the PR, @evgeniycheban

Could you please take a look at adding some tests for the new functionality? I think they could be implemented as additions to org.springframework.boot.autoconfigure.elasticsearch.ElasticsearchRestClientAutoConfigurationTests. It looks like you can get the nodes from the RestClient to verify that the configuration has been applied correctly. As part of the tests, it would be good to verify the behaviour of URIs with and without any user info and how the separate username and password properties are used.

Thanks for the review, @wilkinsona

Sure, I will add tests.

evgeniycheban · 2020-05-12T18:27:43Z

@wilkinsona I added tests.

When you have a moment, please take a look.

wilkinsona

Thanks for the updates. I've left a few more comments. Could you please take a look when you have a moment?

wilkinsona · 2020-05-13T11:32:21Z

.../springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientConfigurations.java

@@ -114,25 +123,48 @@ RestClient elasticsearchRestClient(RestClientBuilder builder) {

 		private final ElasticsearchRestClientProperties properties;

+		private CredentialsProvider credentialsProvider = new BasicCredentialsProvider();


Creating a field for the CredentialsProvider and allowing it to be injected doesn't seem to be related to supporting user info in the URI. Can you please revert this part of the changes?

I added this to make testing easer, because we can not access credentialsProvider field from InternalHttpAsyncClient. It can also be useful in some cases when the user needs to use a custom CredentialsProvider.

I don't think it's safe to expose a CredentialProvider as a bean. In this case the credentials that it provides are specific to Elasticsearch, but there's nothing about the bean that indicates that's the case. It could be accidentally consumed and used anywhere that's working with an Apache HTTP client, potentially leaking credentials to another service.

For this sort of situation, we prefer to use AssertJ's support for extracting fields. In this case it would look something like this:

assertThat(client).extracting("client") .extracting("credentialsProvider", InstanceOfAssertFactories.type(CredentialsProvider.class)) .satisfies((credentialsProvider) -> { Credentials uriCredentials = credentialsProvider.getCredentials(new AuthScope("localhost", 9200)); assertThat(uriCredentials.getUserPrincipal().getName()).isEqualTo("user"); assertThat(uriCredentials.getPassword()).isEqualTo("password"); Credentials defaultCredentials = credentialsProvider.getCredentials(new AuthScope("localhost", 9201)); assertThat(defaultCredentials.getUserPrincipal().getName()).isEqualTo("admin"); assertThat(defaultCredentials.getPassword()).isEqualTo("admin"); });

Thank you for the AssertJ example, @wilkinsona
I will update tests.

wilkinsona · 2020-05-13T11:34:58Z

...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java

+
+					Credentials defaultCredentials = credentialsProvider
+							.getCredentials(new AuthScope("localhost", 9201));
+					assertThat(defaultCredentials.getUserPrincipal().getName()).isEqualTo("admin");


I think the username in the URI should win so the name of the principal should be user.

Yes, that is right, in this assert we are checking that if the URI does not contain credentials then spring.elasticsearch.rest.username and spring.elasticsearch.rest.password will be used.

Sorry, I'd missed that there are two URIs being configured, one with credentials and one without.

wilkinsona · 2020-05-13T11:35:29Z

...ramework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientAutoConfigurationTests.java

+					Credentials defaultCredentials = credentialsProvider
+							.getCredentials(new AuthScope("localhost", 9201));
+					assertThat(defaultCredentials.getUserPrincipal().getName()).isEqualTo("admin");
+					assertThat(defaultCredentials.getPassword()).isEqualTo("admin");


Similarly, I think the password in the URI should win so it should be password.

Sorry, ignore this. I'd missed that there are two URIs being configured, one with credentials and one without.

wilkinsona · 2020-05-13T14:17:33Z

.../springframework/boot/autoconfigure/elasticsearch/ElasticsearchRestClientConfigurations.java

@@ -114,25 +123,48 @@ RestClient elasticsearchRestClient(RestClientBuilder builder) {

 		private final ElasticsearchRestClientProperties properties;

+		private CredentialsProvider credentialsProvider = new BasicCredentialsProvider();


I don't think it's safe to expose a CredentialProvider as a bean. In this case the credentials that it provides are specific to Elasticsearch, but there's nothing about the bean that indicates that's the case. It could be accidentally consumed and used anywhere that's working with an Apache HTTP client, potentially leaking credentials to another service.

For this sort of situation, we prefer to use AssertJ's support for extracting fields. In this case it would look something like this:

assertThat(client).extracting("client") .extracting("credentialsProvider", InstanceOfAssertFactories.type(CredentialsProvider.class)) .satisfies((credentialsProvider) -> { Credentials uriCredentials = credentialsProvider.getCredentials(new AuthScope("localhost", 9200)); assertThat(uriCredentials.getUserPrincipal().getName()).isEqualTo("user"); assertThat(uriCredentials.getPassword()).isEqualTo("password"); Credentials defaultCredentials = credentialsProvider.getCredentials(new AuthScope("localhost", 9201)); assertThat(defaultCredentials.getUserPrincipal().getName()).isEqualTo("admin"); assertThat(defaultCredentials.getPassword()).isEqualTo("admin"); });

Fixes spring-projectsgh-21291

evgeniycheban · 2020-05-19T14:16:33Z

@wilkinsona Could you please take a look at the latest changes when you have a moment?

murdos · 2020-06-02T20:18:04Z

@wilkinsona : is there any chance to get this in 2.3.x ?
Indeed this feature was available with Jest, but support has been removed in 2.3.

wilkinsona · 2020-06-06T07:45:38Z

Yes. Given the situation with Jest, it seems reasonable to include this in 2.3.x.

See gh-21381

philwebb · 2020-06-07T18:59:17Z

Thanks very much for the PR @evgeniycheban, this is now in 2.3.x and master. I made some minor tweaks in f8982bd (mainly to protect against URL parse exceptions).

evgeniycheban marked this pull request as draft May 11, 2020 00:47

spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 11, 2020

evgeniycheban force-pushed the gh-21291 branch from b04d7b3 to f399b12 Compare May 11, 2020 01:39

wilkinsona added the status: waiting-for-feedback We need additional information before we can continue label May 11, 2020

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 May 11, 2020

evgeniycheban force-pushed the gh-21291 branch from f399b12 to 45c6a16 Compare May 12, 2020 01:37

evgeniycheban marked this pull request as ready for review May 12, 2020 01:38

evgeniycheban force-pushed the gh-21291 branch 2 times, most recently from 6da09b1 to 03c7ca3 Compare May 12, 2020 02:27

wilkinsona requested changes May 13, 2020

View reviewed changes

evgeniycheban requested a review from wilkinsona May 13, 2020 12:10

wilkinsona requested changes May 13, 2020

View reviewed changes

Support userInfo in elasticsearch uri

964e712

Fixes spring-projectsgh-21291

evgeniycheban force-pushed the gh-21291 branch from 03c7ca3 to 964e712 Compare May 13, 2020 15:38

evgeniycheban requested a review from wilkinsona May 13, 2020 16:06

wilkinsona changed the title ~~Support userInfo in elasticsearch uri~~ Support user info in Elasticsearch URIs May 19, 2020

wilkinsona approved these changes May 19, 2020

View reviewed changes

wilkinsona added type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels May 19, 2020

wilkinsona added this to the 2.4.x milestone May 19, 2020

philwebb added the for: team-attention An issue we'd like other members of the team to review label Jun 4, 2020

philwebb modified the milestones: 2.4.x, 2.3.x Jun 5, 2020

philwebb removed the for: team-attention An issue we'd like other members of the team to review label Jun 5, 2020

philwebb self-assigned this Jun 7, 2020

This was referenced Jun 7, 2020

Elasticsearch: Support REST URIs including userInfo #21291

Closed

Support user info in Elasticsearch URIs #21747

Closed

philwebb modified the milestones: 2.3.x, 2.3.1 Jun 7, 2020

philwebb pushed a commit that referenced this pull request Jun 7, 2020

Support userInfo in elasticsearch URI

a72179a

See gh-21381

philwebb added a commit that referenced this pull request Jun 7, 2020

Polish 'Support userInfo in elasticsearch URI'

f8982bd

See gh-21381

philwebb closed this in 8ac8329 Jun 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support user info in Elasticsearch URIs #21381

Support user info in Elasticsearch URIs #21381

evgeniycheban commented May 11, 2020

wilkinsona commented May 11, 2020

evgeniycheban commented May 11, 2020

evgeniycheban commented May 12, 2020

wilkinsona left a comment

wilkinsona May 13, 2020 •

edited

evgeniycheban May 13, 2020 •

edited

wilkinsona May 13, 2020

evgeniycheban May 13, 2020 •

edited

evgeniycheban May 13, 2020

wilkinsona May 13, 2020

evgeniycheban May 13, 2020

wilkinsona May 13, 2020

wilkinsona May 13, 2020

wilkinsona May 13, 2020

wilkinsona May 13, 2020

evgeniycheban commented May 19, 2020

murdos commented Jun 2, 2020

wilkinsona commented Jun 6, 2020

philwebb commented Jun 7, 2020

		@@ -114,25 +123,48 @@ RestClient elasticsearchRestClient(RestClientBuilder builder) {

		private final ElasticsearchRestClientProperties properties;

		private CredentialsProvider credentialsProvider = new BasicCredentialsProvider();

Support user info in Elasticsearch URIs #21381

Support user info in Elasticsearch URIs #21381

Conversation

evgeniycheban commented May 11, 2020

wilkinsona commented May 11, 2020

evgeniycheban commented May 11, 2020

evgeniycheban commented May 12, 2020

wilkinsona left a comment

Choose a reason for hiding this comment

wilkinsona May 13, 2020 • edited

Choose a reason for hiding this comment

evgeniycheban May 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban May 13, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeniycheban commented May 19, 2020

murdos commented Jun 2, 2020

wilkinsona commented Jun 6, 2020

philwebb commented Jun 7, 2020

wilkinsona May 13, 2020 •

edited

evgeniycheban May 13, 2020 •

edited

evgeniycheban May 13, 2020 •

edited