Simplified some code and added tests to increase code coverage

4 files changed, 175 insertions, 27 deletions

diff --git a/src/node/ext/channel_credentials.cc b/src/node/ext/channel_credentials.cc
index b77ff80af2..059bc8a890 100644
--- a/src/node/ext/channel_credentials.cc
+++ b/src/node/ext/channel_credentials.cc
@@ -154,6 +154,12 @@ NAN_METHOD(ChannelCredentials::CreateSsl) {
     return Nan::ThrowTypeError(
         "createSSl's third argument must be a Buffer if provided");
   }
+  if ((key_cert_pair.private_key == NULL) !=
+      (key_cert_pair.cert_chain == NULL)) {
+    return Nan::ThrowError(
+        "createSsl's second and third arguments must be"
+        " provided or omitted together");
+  }
   grpc_channel_credentials *creds = grpc_ssl_credentials_create(
       root_certs, key_cert_pair.private_key == NULL ? NULL : &key_cert_pair,
       NULL);
diff --git a/src/node/ext/server_credentials.cc b/src/node/ext/server_credentials.cc
index e6c55e263c..5285d53df4 100644
--- a/src/node/ext/server_credentials.cc
+++ b/src/node/ext/server_credentials.cc
@@ -167,30 +167,18 @@ NAN_METHOD(ServerCredentials::CreateSsl) {
       return Nan::ThrowTypeError("Key/cert pairs must be objects");
     }
     Local<Object> pair_obj = Nan::To<Object>(pair_val).ToLocalChecked();
-    MaybeLocal<Value> maybe_key = Nan::Get(pair_obj, key_key);
-    if (maybe_key.IsEmpty()) {
-      delete key_cert_pairs;
-      return Nan::ThrowTypeError(
-          "Key/cert pairs must have a private_key and a cert_chain");
-    }
-    MaybeLocal<Value> maybe_cert = Nan::Get(pair_obj, cert_key);
-    if (maybe_cert.IsEmpty()) {
-      delete key_cert_pairs;
-      return Nan::ThrowTypeError(
-          "Key/cert pairs must have a private_key and a cert_chain");
-    }
-    if (!::node::Buffer::HasInstance(maybe_key.ToLocalChecked())) {
+    Local<Value> maybe_key = Nan::Get(pair_obj, key_key).ToLocalChecked();
+    Local<Value> maybe_cert = Nan::Get(pair_obj, cert_key).ToLocalChecked();
+    if (!::node::Buffer::HasInstance(maybe_key)) {
       delete key_cert_pairs;
       return Nan::ThrowTypeError("private_key must be a Buffer");
     }
-    if (!::node::Buffer::HasInstance(maybe_cert.ToLocalChecked())) {
+    if (!::node::Buffer::HasInstance(maybe_cert)) {
       delete key_cert_pairs;
       return Nan::ThrowTypeError("cert_chain must be a Buffer");
     }
-    key_cert_pairs[i].private_key = ::node::Buffer::Data(
-        maybe_key.ToLocalChecked());
-    key_cert_pairs[i].cert_chain = ::node::Buffer::Data(
-        maybe_cert.ToLocalChecked());
+    key_cert_pairs[i].private_key = ::node::Buffer::Data(maybe_key);
+    key_cert_pairs[i].cert_chain = ::node::Buffer::Data(maybe_cert);
   }
   grpc_server_credentials *creds = grpc_ssl_server_credentials_create(
       root_certs, key_cert_pairs, key_cert_pair_count, force_client_auth, NULL);
diff --git a/src/node/test/credentials_test.js b/src/node/test/credentials_test.js
index 647f648ca9..294600c85a 100644
--- a/src/node/test/credentials_test.js
+++ b/src/node/test/credentials_test.js
@@ -76,6 +76,146 @@ var fakeFailingGoogleCredentials = {
   }
 };
 
+var key_data, pem_data, ca_data;
+
+before(function() {
+  var key_path = path.join(__dirname, './data/server1.key');
+  var pem_path = path.join(__dirname, './data/server1.pem');
+  var ca_path = path.join(__dirname, '../test/data/ca.pem');
+  key_data = fs.readFileSync(key_path);
+  pem_data = fs.readFileSync(pem_path);
+  ca_data = fs.readFileSync(ca_path);
+});
+
+describe('channel credentials', function() {
+  describe('#createSsl', function() {
+    it('works with no arguments', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.credentials.createSsl();
+      });
+      assert.notEqual(creds, null);
+    });
+    it('works with just one Buffer argument', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.credentials.createSsl(ca_data);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('works with 3 Buffer arguments', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.credentials.createSsl(ca_data, key_data, pem_data);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('works if the first argument is null', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.credentials.createSsl(null, key_data, pem_data);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('fails if the first argument is a non-Buffer value', function() {
+      assert.throws(function() {
+        grpc.credentials.createSsl('test');
+      }, TypeError);
+    });
+    it('fails if the second argument is a non-Buffer value', function() {
+      assert.throws(function() {
+        grpc.credentials.createSsl(null, 'test', pem_data);
+      }, TypeError);
+    });
+    it('fails if the third argument is a non-Buffer value', function() {
+      assert.throws(function() {
+        grpc.credentials.createSsl(null, key_data, 'test');
+      }, TypeError);
+    });
+    it('fails if only 1 of the last 2 arguments is provided', function() {
+      assert.throws(function() {
+        grpc.credentials.createSsl(null, key_data);
+      });
+      assert.throws(function() {
+        grpc.credentials.createSsl(null, null, pem_data);
+      });
+    });
+  });
+});
+
+describe('server credentials', function() {
+  describe('#createSsl', function() {
+    it('accepts a buffer and array as the first 2 arguments', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.ServerCredentials.createSsl(ca_data, []);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('accepts a boolean as the third argument', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.ServerCredentials.createSsl(ca_data, [], true);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('accepts an object with two buffers in the second argument', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.ServerCredentials.createSsl(null,
+                                                 [{private_key: key_data,
+                                                   cert_chain: pem_data}]);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('accepts multiple objects in the second argument', function() {
+      var creds;
+      assert.doesNotThrow(function() {
+        creds = grpc.ServerCredentials.createSsl(null,
+                                                 [{private_key: key_data,
+                                                   cert_chain: pem_data},
+                                                  {private_key: key_data,
+                                                   cert_chain: pem_data}]);
+      });
+      assert.notEqual(creds, null);
+    });
+    it('fails if the second argument is not an Array', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl(ca_data, 'test');
+      }, TypeError);
+    });
+    it('fails if the first argument is a non-Buffer value', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl('test', []);
+      }, TypeError);
+    });
+    it('fails if the third argument is a non-boolean value', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl(ca_data, [], 'test');
+      }, TypeError);
+    });
+    it('fails if the array elements are not objects', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl(ca_data, 'test');
+      }, TypeError);
+    });
+    it('fails if the object does not have a Buffer private_key', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl(null,
+                                         [{private_key: 'test',
+                                           cert_chain: pem_data}]);
+      }, TypeError);
+    });
+    it('fails if the object does not have a Buffer cert_chain', function() {
+      assert.throws(function() {
+        grpc.ServerCredentials.createSsl(null,
+                                         [{private_key: key_data,
+                                           cert_chain: 'test'}]);
+      }, TypeError);
+    });
+  });
+});
+
 describe('client credentials', function() {
   var Client;
   var server;
@@ -109,20 +249,13 @@ describe('client credentials', function() {
         });
       }
     });
-    var key_path = path.join(__dirname, './data/server1.key');
-    var pem_path = path.join(__dirname, './data/server1.pem');
-    var key_data = fs.readFileSync(key_path);
-    var pem_data = fs.readFileSync(pem_path);
     var creds = grpc.ServerCredentials.createSsl(null,
                                                  [{private_key: key_data,
                                                    cert_chain: pem_data}]);
-    //creds = grpc.ServerCredentials.createInsecure();
     port = server.bind('localhost:0', creds);
     server.start();
 
     Client = proto.TestService;
-    var ca_path = path.join(__dirname, '../test/data/ca.pem');
-    var ca_data = fs.readFileSync(ca_path);
     client_ssl_creds = grpc.credentials.createSsl(ca_data);
     var host_override = 'foo.test.google.fr';
     client_options['grpc.ssl_target_name_override'] = host_override;
diff --git a/src/node/test/server_test.js b/src/node/test/server_test.js
index 999a183b3c..592f47e145 100644
--- a/src/node/test/server_test.js
+++ b/src/node/test/server_test.js
@@ -45,9 +45,30 @@ describe('server', function() {
         new grpc.Server();
       });
     });
-    it('should work with an empty list argument', function() {
+    it('should work with an empty object argument', function() {
       assert.doesNotThrow(function() {
-        new grpc.Server([]);
+        new grpc.Server({});
+      });
+    });
+    it('should work without the new keyword', function() {
+      var server;
+      assert.doesNotThrow(function() {
+        server = grpc.Server();
+      });
+      assert(server instanceof grpc.Server);
+    });
+    it('should only accept objects with string or int values', function() {
+      assert.doesNotThrow(function() {
+        new grpc.Server({'key' : 'value'});
+      });
+      assert.doesNotThrow(function() {
+        new grpc.Server({'key' : 5});
+      });
+      assert.throws(function() {
+        new grpc.Server({'key' : null});
+      });
+      assert.throws(function() {
+        new grpc.Server({'key' : new Date()});
       });
     });
   });