On Wed, 21 Feb 2024 06:56:01 GMT, Jaikiran Pai <[email protected]> wrote:
> Can I please get a review of this change which proposes to fix > https://bugs.openjdk.org/browse/JDK-8326381? > > As noted in the JBS issue, the implementation in `setNeedClientAuth()` and > `setWantClientAuth()` of `com.sun.net.httpserver.HttpsParameters` wasn't > matching the API specification. The commit in this PR fixes that issue and it > now matches the API specification as well as what is done in > `javax.net.ssl.SSLParameters` class. > > Additionally, as noted in the JBS issue, the (internal class) > `sun.net.httpserver.SSLStreams` had a bug where it could end up resetting the > `needClientAuth` flag on the `SSLEngine` because of the way the > `setNeedClientAuth()` and `setWantClientAuth()` methods were being called on > the `SSLEngine`. This too has been fixed in this PR. > > A new jtreg test has been introduced to reproduce the issue in the > `HttpsParameters` class and verify this fix. src/jdk.httpserver/share/classes/sun/net/httpserver/SSLStreams.java line 91: > 89: engine.setNeedClientAuth(true); > 90: } > 91: if (params.getWantClientAuth()) { Assume the states of `wantClientAuth` and `needClientAuth` are correctly maintained with the changes in `HttpsParameters`, or it's impossible both of them are `true`. Could here use `if-else if` clause, like the below? if (params.getNeedClientAuth()) { engine.setNeedClientAuth(true); } else if (params.getWantClientAuth()) { engine.setWantClientAuth(true); } test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 49: > 47: */ > 48: @Test > 49: public void testClientAuth() throws Exception { Just a suggestion. Now that this test uses JUnit, why doesn't it define multiple test methods for the different HttpsParameters instances? A single test method just focus on only one HttpsParameters instance (or test case). If the checking on a HttpsParameters instance (or test case) fails, the remaining test cases still can be executed. test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 72: > 70: assertFalse(wantClientAuthParams.getNeedClientAuth(), > 71: "needClientAuth was expected to be false but wasn't"); > 72: } Is it necessary to check the states on the following cases? HttpsParameters params = new Params(); params.setNeedClientAuth(true); params.setWantClientAuth(false); HttpsParameters params = new Params(); params.setWantClientAuth(true); params.setNeedClientAuth(false); test/jdk/com/sun/net/httpserver/HttpsParametersClientAuthTest.java line 74: > 72: } > 73: > 74: Nit: this blank line could be removed if you want. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497073469 PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497091356 PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497109813 PR Review Comment: https://git.openjdk.org/jdk/pull/17940#discussion_r1497097393
