chore: use SDK http client instead of a separate http-client dependency

[mstahv]

Less dependencies that might cause conflicts in user projects

Fixes #14538

Type of change

• Bugfix
• Feature

Checklist

• I have read the contribution guide: https://vaadin.com/docs/latest/guide/contributing/overview/
• I have added a description following the guideline.
• The issue is created in the corresponding repository and I have referenced it.
• I have added tests to ensure my change is effective and works as intended.
• New and existing tests are passing locally with my change.
• I have performed self-review and corrected misspellings.

Additional for Feature type of change

• Enhancement / new feature was discussed in a corresponding GitHub issue and Acceptance Criteria were created.

[mstahv]

Looks like vaadin-dev-server is expecting to have the http-client dependency via vaadin-server still. That should also be converted to use the JDK version...

[github-actions]

Test Results

   992 files  ±  0     992 suites  ±0   1h 26m 42s ⏱️ + 4m 26s
6 299 tests ±  0  6 258 ✔️ ±  0  41 💤 ±0  0 ❌ ±0 
6 549 runs  +13  6 501 ✔️ +13  48 💤 ±0  0 ❌ ±0 

Results for commit 2c5f45a. ± Comparison against base commit fe1a378.

♻️ This comment has been updated with latest results.

[mcollovati]

Some minor changes.
The hard-coded timeout settings are a bit opinionated.
It would be good to make them configurable.
Maybe trying to get the value from system properties, otherwise fallback to default?

[mstahv]

@mcollovati timeouts were opinionated indeed, copied from some tutorial 😂 I removed them now, going with defaults would be fine I guess? Or were those configurable in the past 🤔

[mcollovati]

I was thinking they were configurable through system properties, as the client builder had useSystemProperties() setting.
But it seems like there are no properties for the timeouts in httpclient.
So removing them from the PR could be fine.

[mstahv]

👌 That was done in 9becd05

[mcollovati]

I'm testing with proxy authentication, but the download fails.
I'll have to verify if it is something on my proxy config, but I'm getting an HTTP 407 error (Proxy Authentication Required) and the authenticator getPasswordAuthentication() is never invoked.

[Artur-]

Please add some proxy tests to the project :)

[mcollovati]

It seems like that basic authentication for https proxy has been deactivated since Java 8u111 (https://www.oracle.com/java/technologies/javase/8u111-relnotes.html)

Setting the -Djdk.http.auth.tunneling.disabledSchemes="" system properties makes the authentication work.
Another suggested workaround is to set the Proxy-Authorization header in the request.
It would be useful to investigate how Apache HTTP client handles authentication, as the current DefaultFileDownloader implementation works fine with the basic auth.

[mcollovati]

And this could be interesting to set up tests with proxy: https://github.com/pan-dora/jdk11-httpclient-tests/blob/master/httpclient/src/test/java/net/httpclient/ProxyAuthTest.java

[knoobie]

Another suggested workaround is to set the Proxy-Authorization header in the request.

keep in mind this old security fix :) apache/httpcomponents-client@a572756

[mstahv]

Would be great if we'd know if and how developers use proxies 😬 Building extensive integration tests for this is probably not worth the investement but some smaller smoke test would be great.

[knoobie]

Couldn't you guys try to get some information about proxy usage based on your usage statistics features? Those could be added til 24.1 and you have those information hopefully in the next months before this hits the road with 24.2/3

[mcollovati]

I propose not adding custom code for handling proxy authentication, but instead updating the documentation at https://vaadin.com/docs/latest/configuration/development-mode/node-js#proxy-settings-for-downloading-the-front-end-toolchain to suggest setting the -Djdk.http.auth.tunneling.disabledSchemes if needed

[mcollovati]

Created vaadin/docs#2475 and #16966

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[platosha]

Would it be feasible to backport this to 24.1? Hilla native compilation is broken by httpclient dependency that brings commons-logging, this would help.