add https support #47

shzcore · 2020-06-08T05:51:10Z

add https support
implement https://github.com/hugegraph/hugegraph/issues/445

CLAassistant · 2020-06-08T05:51:17Z

All committers have signed the CLA.

javeme · 2020-06-08T05:52:54Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                .config(user, password)
+                .config(maxTotal, maxPerRoute)
+                .config(protocol, trustStoreFile, trustStorePassword)
+                .build());


javeme · 2020-06-08T05:53:17Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                .config(protocol, trustStoreFile, trustStorePassword)
+                .build());
+    }
+    private TrustManager[] getTrustManager() {


add empty line

javeme · 2020-06-08T05:54:51Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+    }
+    private TrustManager[] getTrustManager() {
+        return new TrustManager[]{
+                new X509TrustManager() {


add class define NonCheckTrustManager

javeme · 2020-06-08T05:55:07Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+            try {
+                SslConfigurator sslConfig = SslConfigurator.newInstance();
+                sslConfig.trustStoreFile(config.getProperties().get("trustStoreFile").toString())
+                        .trustStorePassword(config.getProperties().get("trustStorePassword").toString());


javeme · 2020-06-08T05:57:00Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                        .build();
+            } catch (Exception e) {
+                e.printStackTrace();
+            }


add a method

javeme · 2020-06-08T05:57:25Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                        .sslContext(sc)
+                        .build();
+            } catch (Exception e) {
+                e.printStackTrace();


don't swallow exception

javeme · 2020-06-08T05:58:33Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+                              int timeout, int maxTotal, int maxPerRoute,
+                              String protocol, String trustStoreFile,
+                              String trustStorePassword,int status) {
+            super(url, user, password, timeout, maxTotal, maxPerRoute,protocol,


add space before protocol

javeme · 2020-06-08T05:58:45Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+                              String protocol, String trustStoreFile,
+                              String trustStorePassword,int status) {
+            super(url, user, password, timeout, maxTotal, maxPerRoute,protocol,
+                    trustStoreFile,trustStorePassword);


add space before trustStorePassword

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

src/main/java/com/baidu/hugegraph/rest/RestClient.java

javeme · 2020-06-08T07:19:46Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+import static org.glassfish.jersey.apache.connector.ApacheClientProperties.CONNECTION_MANAGER;


keep origin order

javeme · 2020-06-08T07:20:57Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                        .build();
+            } catch (Exception e) {
+                e.printStackTrace();
+            }


javeme · 2020-06-08T07:22:14Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

-            } catch (Exception e) {
-                e.printStackTrace();
+            } catch (KeyManagementException e) {
+                throw new ClientException("security key management exception", e);


improve message

javeme · 2020-06-08T07:27:20Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                TrustManager[] trustAllCerts = NoCheckTrustManager();
+                sc.init(null, trustAllCerts, new java.security.SecureRandom());
+                client = ClientBuilder.newBuilder()
+                        .hostnameVerifier(new HostNameVerifier(url))


align with .newBuilder

javeme · 2020-06-08T07:28:04Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+    private static class X509TrustManager implements javax.net.ssl.X509TrustManager {
+        @Override
+        public void checkClientTrusted(X509Certificate[] chain, String authType)
+                throws CertificateException {


align with X509Certificate

javeme · 2020-06-08T07:28:20Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+    private TrustManager[] NoCheckTrustManager() {
+        return new TrustManager[]{
+               new X509TrustManager()
+        };


put in one line

javeme · 2020-06-08T07:28:35Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                return hv.verify(hostname, session);
+            }
+        }
+


codecov · 2020-06-08T09:13:50Z

Codecov Report

Merging #47 into master will increase coverage by 0.01%.
The diff coverage is 82.60%.

@@             Coverage Diff              @@
##             master      #47      +/-   ##
============================================
+ Coverage     81.58%   81.60%   +0.01%     
- Complexity      671      675       +4     
============================================
  Files            56       56              
  Lines          2004     2049      +45     
  Branches        297      300       +3     
============================================
+ Hits           1635     1672      +37     
- Misses          244      249       +5     
- Partials        125      128       +3

Impacted Files	Coverage Δ	Complexity Δ
...ava/com/baidu/hugegraph/version/CommonVersion.java	`50.00% <ø> (ø)`	`1.00 <0.00> (ø)`
...main/java/com/baidu/hugegraph/rest/RestClient.java	`87.57% <82.60%> (-1.95%)`	`38.00 <3.00> (+4.00)`	⬇️
.../main/java/com/baidu/hugegraph/event/EventHub.java	`80.55% <0.00%> (-1.39%)`	`26.00% <0.00%> (-1.00%)`
.../main/java/com/baidu/hugegraph/perf/Stopwatch.java	`87.75% <0.00%> (+2.04%)`	`21.00% <0.00%> (+1.00%)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d90ae4...ca326a6. Read the comment docs.

javeme · 2020-06-08T13:36:47Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        try {
+            sc.init(null, trustAllCerts, new java.security.SecureRandom());
+        } catch (KeyManagementException e) {
+            throw new ClientException("security key init management exception", e);


"Failed to init security key management"

javeme · 2020-06-08T13:37:35Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

-            } catch (KeyManagementException e) {
-                throw new ClientException("security key management exception", e);
-            }
+            client = TrustConfig(url, config);


wrapTrustConfig()

javeme · 2020-06-08T14:10:44Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+    }
+
+    private Client wrapTrustConfig(String url, ClientConfig config) {
+        Client client = null;


move to line 142

javeme · 2020-06-08T14:10:56Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+
+import static org.glassfish.jersey.apache.connector.ApacheClientProperties.CONNECTION_MANAGER;


javeme · 2020-06-08T14:11:54Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        SSLContext sc = sslConfig.createSSLContext();
+        TrustManager[] trustAllCerts = NoCheckTrustManager();
+        try {
+            sc.init(null, trustAllCerts, new java.security.SecureRandom());


import java.security.SecureRandom

javeme · 2020-06-08T14:12:15Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        return new TrustManager[]{new X509TrustManager()};
+    }
+
+    private static class X509TrustManager implements javax.net.ssl.X509TrustManager {


rename to NoCheckTrustManager

javeme · 2020-06-08T14:13:25Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+                                     .build());
+    }
+
+    private TrustManager[] NoCheckTrustManager() {


method name must be started with lower case, rename to createNoCheckTrustManager

javeme · 2020-06-08T14:17:17Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+                              String protocol, String trustStoreFile,
+                              String trustStorePassword, int status) {
+            super(url, user, password, timeout, maxTotal, maxPerRoute, protocol,
+                              trustStoreFile, trustStorePassword);


javeme · 2020-06-08T14:26:24Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+    private Client wrapTrustConfig(String url, ClientConfig config) {
+        Client client = null;
+        SslConfigurator sslConfig = SslConfigurator.newInstance();
+        sslConfig.trustStoreFile(config.getProperties().get("trustStoreFile").toString())


use config.getProperty("trustStoreFile") instead

javeme · 2020-06-08T14:26:38Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        Client client = null;
+        SslConfigurator sslConfig = SslConfigurator.newInstance();
+        sslConfig.trustStoreFile(config.getProperties().get("trustStoreFile").toString())
+                 .trustStorePassword(config.getProperties().get("trustStorePassword").toString());


javeme · 2020-06-08T14:26:52Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

    public RestClient(String url, ClientConfig config) {
-        this.client = ClientBuilder.newClient(config);
+        Client client = null;
+        Object protocol = config.getProperties().get("protocol");


Linary · 2020-06-09T02:40:38Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        this(url, new ConfigBuilder().config(timeout)
+                                     .config(user, password)
+                                     .config(maxTotal, maxPerRoute)
+                                     .config(protocol, trustStoreFile, trustStorePassword)


line 109 seems exceed 80 chars, wrap line like

.config(protocol, trustStoreFile, trustStorePassword)

also pay attention to other places

Linary · 2020-06-09T02:47:31Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+        } catch (KeyManagementException e) {
+            throw new ClientException("Failed to init security key management", e);
+        }
+        client = ClientBuilder.newBuilder()


just return ClientBuilder.newBuilder().... is ok, no need client

javeme · 2020-06-10T17:40:26Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+            this.headers = ImmutableMultivaluedMap.empty();
+            this.content = "";
+        }
+


just add a test case: testHttpsGet:

public void testHttpsGet() { RestClient client = new RestClientImpl(...); RestResult restResult = client.get("path", "id1"); Assert.assertEquals(200, restResult.status()); }

javeme · 2020-06-11T05:20:56Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+        String trustStorePassword = "changeit";
+        RestClient client = new RestClientImpl("/test", "user", "", 1000,
+                                               10, 5, "https", trustStoreFile,
+                                                trustStorePassword, 200);


javeme · 2020-06-11T05:21:26Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+            if (!this.url.equals("") && this.url.contains(hostname)) {
+                return true;
+            } else {
+                HostnameVerifier hv = HttpsURLConnection.getDefaultHostnameVerifier();


rename hv to verifier

javeme · 2020-06-11T05:22:26Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

+
+        @Override
+        public boolean verify(String hostname, SSLSession session) {
+            if (!this.url.equals("") && this.url.contains(hostname)) {


use this.url.isEmpty() instead of this.url.equals("")

javeme · 2020-06-11T05:22:37Z

src/main/java/com/baidu/hugegraph/rest/RestClient.java

@@ -284,6 +356,25 @@ private static String encode(String raw) {
        return UriComponent.encode(raw, UriComponent.Type.PATH_SEGMENT);
    }

+    private static class HostNameVerifier implements HostnameVerifier {
+
+        private String url;


javeme · 2020-06-11T05:23:36Z

src/test/java/com/baidu/hugegraph/unit/rest/RestClientTest.java

+        RestResult restResult = client.post("path", "body");
+        Assert.assertEquals(200, restResult.status());
+    }
+


add test for HostNameVerifier

add https support

fd7c0df

javeme requested changes Jun 8, 2020

View reviewed changes

coding Standards

e055386

javeme reviewed Jun 8, 2020

View reviewed changes

fix coding standards

4c2ee03

javeme reviewed Jun 8, 2020

View reviewed changes

shzcore requested a review from javeme June 8, 2020 13:41

fix icoding standards

82d4c9d

javeme reviewed Jun 8, 2020

View reviewed changes

coding standards

9fc4cc4

javeme reviewed Jun 8, 2020

View reviewed changes

shzcore added 2 commits June 8, 2020 22:49

coding standards

c294456

coding standards

7ed3f36

Linary requested changes Jun 9, 2020

View reviewed changes

shzcore added 3 commits June 9, 2020 22:38

coding standards

87ed884

coding standards

b4d1134

coding standards

2a38b12

javeme previously approved these changes Jun 9, 2020

View reviewed changes

Linary previously approved these changes Jun 10, 2020

View reviewed changes

javeme reviewed Jun 10, 2020

View reviewed changes

add test

648cefb

shzcore dismissed stale reviews from Linary and javeme via 648cefb June 11, 2020 01:00

javeme requested changes Jun 11, 2020

View reviewed changes

shzcore added 2 commits June 11, 2020 19:49

coding standards

4e57191

coding standards

83551d8

shzcore added 3 commits June 11, 2020 20:39

add test HostNameVerifier

bf90c31

add test HostNameVerifier

0e14e94

add test HostNameVerifier

ca326a6

javeme approved these changes Jun 11, 2020

View reviewed changes

Linary approved these changes Jun 11, 2020

View reviewed changes

Linary merged commit c2c5ba4 into apache:master Jun 11, 2020

add https support #47

add https support #47

Conversation

shzcore commented Jun 8, 2020 • edited by javeme Loading

CLAassistant commented Jun 8, 2020 • edited Loading

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

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

Choose a reason for hiding this comment

codecov bot commented Jun 8, 2020 • edited Loading

Codecov Report

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

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

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

shzcore commented Jun 8, 2020 •

edited by javeme

Loading

CLAassistant commented Jun 8, 2020 •

edited

Loading

codecov bot commented Jun 8, 2020 •

edited

Loading