Extract file system interaction from SonarWebSocketImpl

Summary:
SonarWebSocketImpl has got pretty bloated. So I'm extracting all the file interaction out of it into
ConnectionContextStore. The purpose of this class is to provide all the context needed to establish a connection.
This makes SonarWebSocketImpl more functional and therefore testable.

Reviewed By: priteshrnandgaonkar

Differential Revision: D9540089

fbshipit-source-id: 0cd1d69f2b11eaf9f569245a2da14f85cc140427

xplat/Sonar/ConnectionContextStore.cpp

@@ -0,0 +1,134 @@
+#include "ConnectionContextStore.h"
+#include "CertificateUtils.h"
+#include <sys/stat.h>
+#include <iostream>
+#include <fstream>
+#include <folly/json.h>
+
+#define CSR_FILE_NAME "app.csr"
+#define SONAR_CA_FILE_NAME "sonarCA.crt"
+#define CLIENT_CERT_FILE_NAME "device.crt"
+#define PRIVATE_KEY_FILE "privateKey.pem"
+#define CONNECTION_CONFIG_FILE "connection_config.json"
+
+#ifdef __ANDROID__
+#include <android/log.h>
+#define SONAR_LOG(message) \
+  __android_log_print(ANDROID_LOG_INFO, "sonar", "sonar: %s", message)
+#else
+#define SONAR_LOG(message) printf("sonar: %s\n", message)
+#endif
+
+using namespace facebook::sonar;
+
+bool fileExists(std::string fileName);
+std::string loadStringFromFile(std::string fileName);
+void writeStringToFile(std::string content, std::string fileName);
+
+ConnectionContextStore::ConnectionContextStore(DeviceData deviceData): deviceData_(deviceData) {}
+
+bool ConnectionContextStore::hasRequiredFiles() {
+  std::string caCert = loadStringFromFile(absoluteFilePath(SONAR_CA_FILE_NAME));
+  std::string clientCert =
+      loadStringFromFile(absoluteFilePath(CLIENT_CERT_FILE_NAME));
+  std::string privateKey =
+      loadStringFromFile(absoluteFilePath(PRIVATE_KEY_FILE));
+
+  if (caCert == "" || clientCert == "" || privateKey == "") {
+    return false;
+  }
+  return true;
+}
+
+std::string ConnectionContextStore::createCertificateSigningRequest() {
+  ensureSonarDirExists();
+  generateCertSigningRequest(
+      deviceData_.appId.c_str(),
+      absoluteFilePath(CSR_FILE_NAME).c_str(),
+      absoluteFilePath(PRIVATE_KEY_FILE).c_str());
+      std::string csr = loadStringFromFile(absoluteFilePath(CSR_FILE_NAME));
+
+  return csr;
+}
+
+std::shared_ptr<SSLContext> ConnectionContextStore::getSSLContext() {
+  std::shared_ptr<folly::SSLContext> sslContext =
+      std::make_shared<folly::SSLContext>();
+  sslContext->loadTrustedCertificates(
+      absoluteFilePath(SONAR_CA_FILE_NAME).c_str());
+  sslContext->setVerificationOption(
+      folly::SSLContext::SSLVerifyPeerEnum::VERIFY);
+  sslContext->loadCertKeyPairFromFiles(
+      absoluteFilePath(CLIENT_CERT_FILE_NAME).c_str(),
+      absoluteFilePath(PRIVATE_KEY_FILE).c_str());
+  sslContext->authenticate(true, false);
+  return sslContext;
+}
+
+std::string ConnectionContextStore::getDeviceId() {
+  /* On android we can't reliably get the serial of the current device
+     So rely on our locally written config, which is provided by the
+     desktop app.
+     For backwards compatibility, when this isn't present, fall back to the
+     unreliable source. */
+  std::string config = loadStringFromFile(absoluteFilePath(CONNECTION_CONFIG_FILE));
+  auto maybeDeviceId = folly::parseJson(config)["deviceId"];
+  return maybeDeviceId.isString() ? maybeDeviceId.getString() : deviceData_.deviceId;
+}
+
+void ConnectionContextStore::storeConnectionConfig(folly::dynamic& config) {
+  std::string json = folly::toJson(config);
+  writeStringToFile(json, absoluteFilePath(CONNECTION_CONFIG_FILE));
+}
+
+std::string ConnectionContextStore::absoluteFilePath(const char* filename) {
+  return std::string(deviceData_.privateAppDirectory + "/sonar/" + filename);
+}
+
+std::string ConnectionContextStore::getCertificateDirectoryPath() {
+  return absoluteFilePath("");
+}
+
+bool ConnectionContextStore::ensureSonarDirExists() {
+  std::string dirPath = absoluteFilePath("");
+  struct stat info;
+  if (stat(dirPath.c_str(), &info) != 0) {
+    int ret = mkdir(dirPath.c_str(), S_IRUSR | S_IWUSR | S_IXUSR);
+    return ret == 0;
+  } else if (info.st_mode & S_IFDIR) {
+    return true;
+  } else {
+    SONAR_LOG(std::string(
+                  "ERROR: Sonar path exists but is not a directory: " + dirPath)
+                  .c_str());
+    return false;
+  }
+}
+
+std::string loadStringFromFile(std::string fileName) {
+  if (!fileExists(fileName)) {
+    return "";
+  }
+  std::stringstream buffer;
+  std::ifstream stream;
+  std::string line;
+  stream.open(fileName.c_str());
+  if (!stream) {
+    SONAR_LOG(
+        std::string("ERROR: Unable to open ifstream: " + fileName).c_str());
+    return "";
+  }
+  buffer << stream.rdbuf();
+  std::string s = buffer.str();
+  return s;
+}
+
+void writeStringToFile(std::string content, std::string fileName) {
+  std::ofstream out(fileName);
+  out << content;
+}
+
+bool fileExists(std::string fileName) {
+  struct stat buffer;
+  return stat(fileName.c_str(), &buffer) == 0;
+}

xplat/Sonar/ConnectionContextStore.h

@@ -0,0 +1,33 @@
+#pragma once
+
+#include <string>
+#include <folly/io/async/SSLContext.h>
+#include <folly/dynamic.h>
+#include "SonarInitConfig.h"
+
+using namespace folly;
+
+namespace facebook {
+namespace sonar {
+
+class ConnectionContextStore {
+
+public:
+  ConnectionContextStore(DeviceData deviceData);
+  bool hasRequiredFiles();
+  std::string createCertificateSigningRequest();
+  std::shared_ptr<SSLContext> getSSLContext();
+  std::string getCertificateDirectoryPath();
+  std::string getDeviceId();
+  void storeConnectionConfig(folly::dynamic& config);
+
+private:
+  DeviceData deviceData_;
+
+  std::string absoluteFilePath(const char* filename);
+  bool ensureSonarDirExists();
+
+};
+
+} // namespace sonar
+} //namespace facebook

xplat/Sonar/SonarClient.cpp

@@ -12,6 +12,7 @@
 #include "SonarState.h"
 #include "SonarStep.h"
 #include "SonarWebSocketImpl.h"
+#include "ConnectionContextStore.h"
 #include <vector>
 
 #ifdef __ANDROID__
@@ -33,8 +34,9 @@ using folly::dynamic;
 
 void SonarClient::init(SonarInitConfig config) {
   auto state = std::make_shared<SonarState>();
+  auto context = std::make_shared<ConnectionContextStore>(config.deviceData);
   kInstance =
-      new SonarClient(std::make_unique<SonarWebSocketImpl>(std::move(config), state), state);
+      new SonarClient(std::make_unique<SonarWebSocketImpl>(std::move(config), state, context), state);
 }
 
 SonarClient* SonarClient::instance() {

xplat/Sonar/SonarWebSocketImpl.cpp

@@ -8,6 +8,7 @@
 
 #include "SonarWebSocketImpl.h"
 #include "SonarStep.h"
+#include "ConnectionContextStore.h"
 #include <folly/String.h>
 #include <folly/futures/Future.h>
 #include <folly/io/async/SSLContext.h>
@@ -15,14 +16,9 @@
 #include <rsocket/Payload.h>
 #include <rsocket/RSocket.h>
 #include <rsocket/transports/tcp/TcpConnectionFactory.h>
-#include <sys/stat.h>
-#include <sys/types.h>
-#include <fstream>
-#include <iostream>
 #include <thread>
 #include <folly/io/async/AsyncSocketException.h>
 #include <stdexcept>
-#include "CertificateUtils.h"
 
 #ifdef __ANDROID__
 #include <android/log.h>
@@ -32,11 +28,6 @@
 #define SONAR_LOG(message) printf("sonar: %s\n", message)
 #endif
 
-#define CSR_FILE_NAME "app.csr"
-#define SONAR_CA_FILE_NAME "sonarCA.crt"
-#define CLIENT_CERT_FILE_NAME "device.crt"
-#define PRIVATE_KEY_FILE "privateKey.pem"
-#define CONNECTION_CONFIG_FILE "connection_config.json"
 #define WRONG_THREAD_EXIT_MSG \
   "ERROR: Aborting sonar initialization because it's not running in the sonar thread."
 
@@ -48,9 +39,6 @@ static constexpr int insecurePort = 8089;
 namespace facebook {
 namespace sonar {
 
-bool fileExists(std::string fileName);
-void writeStringToFile(std::string content, std::string fileName);
-
 class ConnectionEvents : public rsocket::RSocketConnectionEvents {
  private:
   SonarWebSocketImpl* websocket_;
@@ -96,8 +84,8 @@ class Responder : public rsocket::RSocketResponder {
   }
 };
 
-SonarWebSocketImpl::SonarWebSocketImpl(SonarInitConfig config, std::shared_ptr<SonarState> state)
+SonarWebSocketImpl::SonarWebSocketImpl(SonarInitConfig config, std::shared_ptr<SonarState> state, std::shared_ptr<ConnectionContextStore> contextStore)
-    : deviceData_(config.deviceData), sonarState_(state), sonarEventBase_(config.callbackWorker), connectionEventBase_(config.connectionWorker) {
+    : deviceData_(config.deviceData), sonarState_(state), sonarEventBase_(config.callbackWorker), connectionEventBase_(config.connectionWorker), contextStore_(contextStore) {
       CHECK_THROW(config.callbackWorker, std::invalid_argument);
       CHECK_THROW(config.connectionWorker, std::invalid_argument);
     }
@@ -175,7 +163,6 @@ void SonarWebSocketImpl::doCertificateExchange() {
           .get();
   connectingInsecurely->complete();
 
-  ensureSonarDirExists();
   requestSignedCertFromSonar();
 }
 
@@ -183,24 +170,17 @@ void SonarWebSocketImpl::connectSecurely() {
   rsocket::SetupParameters parameters;
   folly::SocketAddress address;
 
-  auto deviceId = getDeviceId();
+  auto loadingDeviceId = sonarState_->start("Load Device Id");
-
+  auto deviceId = contextStore_->getDeviceId();
+  if (deviceId.compare("unknown")) {
+    loadingDeviceId->complete();
+  }
   parameters.payload = rsocket::Payload(folly::toJson(folly::dynamic::object(
       "os", deviceData_.os)("device", deviceData_.device)(
       "device_id", deviceId)("app", deviceData_.app)));
   address.setFromHostPort(deviceData_.host, securePort);
 
-  std::shared_ptr<folly::SSLContext> sslContext =
+  std::shared_ptr<folly::SSLContext> sslContext = contextStore_->getSSLContext();
-      std::make_shared<folly::SSLContext>();
-  sslContext->loadTrustedCertificates(
-      absoluteFilePath(SONAR_CA_FILE_NAME).c_str());
-  sslContext->setVerificationOption(
-      folly::SSLContext::SSLVerifyPeerEnum::VERIFY);
-  sslContext->loadCertKeyPairFromFiles(
-      absoluteFilePath(CLIENT_CERT_FILE_NAME).c_str(),
-      absoluteFilePath(PRIVATE_KEY_FILE).c_str());
-  sslContext->authenticate(true, false);
-
   auto connectingSecurely = sonarState_->start("Connect securely");
   connectionIsTrusted_ = true;
   client_ =
@@ -251,27 +231,6 @@ void SonarWebSocketImpl::sendMessage(const folly::dynamic& message) {
   });
 }
 
-std::string SonarWebSocketImpl::getDeviceId() {
-  /* On android we can't reliably get the serial of the current device
-     So rely on our locally written config, which is provided by the
-     desktop app.
-     For backwards compatibility, when this isn't present, fall back to the
-     unreliable source. */
-  auto gettingDeviceId = sonarState_->start("Get deviceId");
-  std::string config = loadStringFromFile(absoluteFilePath(CONNECTION_CONFIG_FILE));
-  auto maybeDeviceId = folly::parseJson(config)["deviceId"];
-  std::string deviceId;
-  if (maybeDeviceId.isString()) {
-    deviceId = maybeDeviceId.getString();
-  } else {
-    deviceId = deviceData_.deviceId;
-  }
-  if (deviceId.compare("unknown")) {
-    gettingDeviceId->complete();
-  }
-  return deviceId;
-}
-
 bool SonarWebSocketImpl::isCertificateExchangeNeeded() {
 
   if (failedConnectionAttempts_ >= 2) {
@@ -279,32 +238,20 @@ bool SonarWebSocketImpl::isCertificateExchangeNeeded() {
   }
 
   auto step = sonarState_->start("Check required certificates are present");
-  std::string caCert = loadStringFromFile(absoluteFilePath(SONAR_CA_FILE_NAME));
+  bool hasRequiredFiles = contextStore_->hasRequiredFiles();
-  std::string clientCert =
+  if (hasRequiredFiles) {
-      loadStringFromFile(absoluteFilePath(CLIENT_CERT_FILE_NAME));
-  std::string privateKey =
-      loadStringFromFile(absoluteFilePath(PRIVATE_KEY_FILE));
-
-  if (caCert == "" || clientCert == "" || privateKey == "") {
-    return true;
-  }
     step->complete();
-  return false;
+  }
+  return !hasRequiredFiles;
 }
 
 void SonarWebSocketImpl::requestSignedCertFromSonar() {
   auto generatingCSR = sonarState_->start("Generate CSR");
-  generateCertSigningRequest(
+  std::string csr = contextStore_->createCertificateSigningRequest();
-      deviceData_.appId.c_str(),
-      absoluteFilePath(CSR_FILE_NAME).c_str(),
-      absoluteFilePath(PRIVATE_KEY_FILE).c_str());
   generatingCSR->complete();
-  auto loadingCSR = sonarState_->start("Load CSR");
-  std::string csr = loadStringFromFile(absoluteFilePath(CSR_FILE_NAME));
-  loadingCSR->complete();
 
   folly::dynamic message = folly::dynamic::object("method", "signCertificate")(
-      "csr", csr.c_str())("destination", absoluteFilePath("").c_str());
+      "csr", csr.c_str())("destination", contextStore_->getCertificateDirectoryPath().c_str());
   auto gettingCert = sonarState_->start("Getting cert from desktop");
 
   sonarEventBase_->add([this, message, gettingCert]() {
@@ -312,7 +259,10 @@ void SonarWebSocketImpl::requestSignedCertFromSonar() {
         ->requestResponse(rsocket::Payload(folly::toJson(message)))
         ->subscribe([this, gettingCert](rsocket::Payload p) {
           auto response = p.moveDataToString();
-          writeStringToFile(response, absoluteFilePath(CONNECTION_CONFIG_FILE));
+          if (!response.empty()) {
+            folly::dynamic config = folly::parseJson(response);
+            contextStore_->storeConnectionConfig(config);
+          }
           gettingCert->complete();
           SONAR_LOG("Certificate exchange complete.");
           // Disconnect after message sending is complete.
@@ -352,57 +302,9 @@ void SonarWebSocketImpl::sendLegacyCertificateRequest(folly::dynamic message) {
    });
 }
 
-std::string SonarWebSocketImpl::loadStringFromFile(std::string fileName) {
-  if (!fileExists(fileName)) {
-    return "";
-  }
-  std::stringstream buffer;
-  std::ifstream stream;
-  std::string line;
-  stream.open(fileName.c_str());
-  if (!stream) {
-    SONAR_LOG(
-        std::string("ERROR: Unable to open ifstream: " + fileName).c_str());
-    return "";
-  }
-  buffer << stream.rdbuf();
-  std::string s = buffer.str();
-  return s;
-}
-
-void writeStringToFile(std::string content, std::string fileName) {
-  std::ofstream out(fileName);
-  out << content;
-}
-
-std::string SonarWebSocketImpl::absoluteFilePath(const char* filename) {
-  return std::string(deviceData_.privateAppDirectory + "/sonar/" + filename);
-}
-
-bool SonarWebSocketImpl::ensureSonarDirExists() {
-  std::string dirPath = absoluteFilePath("");
-  struct stat info;
-  if (stat(dirPath.c_str(), &info) != 0) {
-    int ret = mkdir(dirPath.c_str(), S_IRUSR | S_IWUSR | S_IXUSR);
-    return ret == 0;
-  } else if (info.st_mode & S_IFDIR) {
-    return true;
-  } else {
-    SONAR_LOG(std::string(
-                  "ERROR: Sonar path exists but is not a directory: " + dirPath)
-                  .c_str());
-    return false;
-  }
-}
-
 bool SonarWebSocketImpl::isRunningInOwnThread() {
   return sonarEventBase_->isInEventBaseThread();
 }
 
-bool fileExists(std::string fileName) {
-  struct stat buffer;
-  return stat(fileName.c_str(), &buffer) == 0;
-}
-
 } // namespace sonar
 } // namespace facebook

xplat/Sonar/SonarWebSocketImpl.h

@@ -20,6 +20,7 @@ namespace facebook {
 namespace sonar {
 
 class ConnectionEvents;
+class ConnectionContextStore;
 class Responder;
 
 class SonarWebSocketImpl : public SonarWebSocket {
@@ -27,7 +28,7 @@ class SonarWebSocketImpl : public SonarWebSocket {
   friend Responder;
 
  public:
-  SonarWebSocketImpl(SonarInitConfig config, std::shared_ptr<SonarState> state);
+  SonarWebSocketImpl(SonarInitConfig config, std::shared_ptr<SonarState> state, std::shared_ptr<ConnectionContextStore> contextStore);
 
   ~SonarWebSocketImpl();
 
@@ -54,16 +55,13 @@ class SonarWebSocketImpl : public SonarWebSocket {
   std::unique_ptr<rsocket::RSocketClient> client_;
   bool connectionIsTrusted_;
   int failedConnectionAttempts_ = 0;
+  std::shared_ptr<ConnectionContextStore> contextStore_;
 
   void startSync();
   void doCertificateExchange();
   void connectSecurely();
-  std::string loadCSRFromFile();
-  std::string loadStringFromFile(std::string fileName);
-  std::string absoluteFilePath(const char* relativeFilePath);
   bool isCertificateExchangeNeeded();
   void requestSignedCertFromSonar();
-  bool ensureSonarDirExists();
   bool isRunningInOwnThread();
   void sendLegacyCertificateRequest(folly::dynamic message);
   std::string getDeviceId();

xplat/SonarTestLib/ConnectionContextStoreMock.h

@@ -0,0 +1,28 @@
+#include <Sonar/ConnectionContextStore.h>
+
+namespace facebook {
+namespace sonar {
+namespace test {
+
+  class ConnectionContextStoreMock : public ConnectionContextStore {
+    public:
+      ConnectionContextStoreMock() : ConnectionContextStore(DeviceData()) {
+      }
+      bool hasRequiredFiles() {
+        return true;
+      }
+      std::string createCertificateSigningRequest() {
+        return "thisIsACsr";
+      }
+      std::shared_ptr<SSLContext> getSSLContext() {
+        return nullptr;
+      }
+      dynamic getConnectionConfig() {
+        return nullptr;
+      }
+      std::string getCertificateDirectoryPath() {
+        return "/something/sonar/";
+      }
+  };
+
+}}}

xplat/SonarTests/SonarWebSocketImplTerminationTests.cpp

@@ -7,6 +7,7 @@
  */
 
 #include <Sonar/SonarWebSocketImpl.h>
+#include <SonarTestLib/ConnectionContextStoreMock.h>
 
 #include <gtest/gtest.h>
 
@@ -18,10 +19,14 @@ using folly::EventBase;
 
 class SonarWebSocketImplTerminationTest : public ::testing::Test {
 protected:
+  std::shared_ptr<SonarState> state;
+  std::shared_ptr<ConnectionContextStore> contextStore;
   void SetUp() override {
     // Folly singletons must be registered before they are used.
     // Without this, test fails in phabricator.
     folly::SingletonVault::singleton()->registrationComplete();
+    state = std::make_shared<SonarState>();
+    contextStore = std::make_shared<ConnectionContextStoreMock>();
   }
 };
 
@@ -31,7 +36,10 @@ TEST_F(SonarWebSocketImplTerminationTest, testNullEventBaseGetsRejected) {
       DeviceData {},
       nullptr,
       new EventBase()
-    }, std::make_shared<SonarState>());
+    },
+    state,
+    contextStore
+    );
     FAIL();
   } catch (std::invalid_argument& e) {
     // Pass test
@@ -41,7 +49,10 @@ TEST_F(SonarWebSocketImplTerminationTest, testNullEventBaseGetsRejected) {
       DeviceData {},
       new EventBase(),
       nullptr
-    }, std::make_shared<SonarState>());
+    },
+    state,
+    contextStore
+    );
     FAIL();
   } catch (std::invalid_argument& e) {
     // Pass test
@@ -54,8 +65,7 @@ TEST_F(SonarWebSocketImplTerminationTest, testNonStartedEventBaseDoesntHang) {
     new EventBase(),
     new EventBase()
   };
-  auto state = std::make_shared<SonarState>();
+  auto instance = std::make_shared<SonarWebSocketImpl>(config, state, contextStore);
-  auto instance = std::make_shared<SonarWebSocketImpl>(config, state);
   instance->start();
 }
 
@@ -73,8 +83,7 @@ TEST_F(SonarWebSocketImplTerminationTest, testStartedEventBaseDoesntHang) {
     sonarEventBase,
     connectionEventBase
   };
-  auto state = std::make_shared<SonarState>();
+  auto instance = std::make_shared<SonarWebSocketImpl>(config, state, contextStore);
-  auto instance = std::make_shared<SonarWebSocketImpl>(config, state);
 
   instance->start();