RFR: 8344228: Revisit SecurityManager usage in java.net.http after JEP 486 integration

[Daniel Fuchs]

On Thu, 14 Nov 2024 20:53:02 GMT, Michael McMahon <michaelm at openjdk.org> wrote:

>> Please find here a patch that cleans up the java.net.http module code to remove permission checks and doPriviliged calls.
>> This was mostly mechanical.
>
> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientBuilderImpl.java line 49:
> 
>> 47:     HttpClient.Version version;
>> 48:     Executor executor;
>> 49:     SSLContext sslContext;
> 
> I think this line was referring to the SSLContext and SSLParameters rather than the security manager

Yes - I toyed with replacing `Security` with `SSL`, but decided to remove it instead because there's other field declarations following that have nothing to do with SSL, and this comment doesn't explain anything: it's obvious that SSLContext and SSLParameters are for SSL... Plus, it's the only comment in the whole field declaration list...

> src/java.net.http/share/classes/jdk/internal/net/http/HttpClientImpl.java line 323:
> 
>> 321:     private final DelegatingExecutor delegatingExecutor;
>> 322:     private final boolean isDefaultExecutor;
>> 323:     private final SSLContext sslContext;
> 
> Same here. I guess it's okay to remove them either way

Yes - same rationale for simply removing it.

> src/java.net.http/share/classes/jdk/internal/net/http/RequestPublishers.java line 245:
> 
>> 243: 
>> 244:             try {
>> 245:                 pathForDefaultFSCheck(path);
> 
> Is this intended, calling the method but ignoring the return value? If so, a comment might be in order.

The method might throw UOE if the path doesn't come from the default file system. I didn't want to investigate and change that logic now - I wasn't sure I fully understood why we have special cases here, so decided to keep it.

> src/java.net.http/share/classes/jdk/internal/net/http/ResponseBodyHandlers.java line 55:
> 
>> 53:     private ResponseBodyHandlers() { }
>> 54: 
>> 55:     private static final String pathForDefaultFSCheck(Path path) {
> 
> maybe this method and the equivalent in the previous file could be renamed `checkPathForDefaultFS` which would suggest an exception being thrown in error cases.

Good idea for the renaming. I will change that - and above too.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843419538
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843421547
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843424520
PR Review Comment: https://git.openjdk.org/jdk/pull/22118#discussion_r1843425880