firewalld: Mock IpTables::{Add|Delete}AcceptRule methods.

This CL paves the way to launch 'ip(6)tables' using Minijail. We cannot
use the current approach of providing test-only binaries because Minijail
will not work when running as non-root (such as in unit tests). Therefore,
we need to mock {Add|Delete}Accept.

Also add an Exec() method to wrap the Minijail invocation in the future,
and clean up some of the unit tests.

BUG=chromium:487019
TEST=Existing unit tests.

Change-Id: I6ddf41bf5c2e8e7fa8f6369d08a3fb37ad2edeb6
Reviewed-on: https://chromium-review.googlesource.com/270341
Trybot-Ready: Jorge Lucangeli Obes <jorgelo@chromium.org>
Tested-by: Jorge Lucangeli Obes <jorgelo@chromium.org>
Reviewed-by: Bartosz Fabianowski <bartfab@chromium.org>
Commit-Queue: Jorge Lucangeli Obes <jorgelo@chromium.org>

5 files changed, 241 insertions, 181 deletions

diff --git a/iptables.cc b/iptables.cc
index 73d373d..ffddaa4 100644
--- a/iptables.cc
+++ b/iptables.cc
@@ -50,11 +50,6 @@ bool IsValidInterfaceName(const std::string& iface) {
 
 namespace firewalld {
 
-IpTables::IpTables() : IpTables{kIpTablesPath, kIp6TablesPath} {}
-
-IpTables::IpTables(const std::string& ip4_path, const std::string& ip6_path)
-    : ip4_exec_path_{ip4_path}, ip6_exec_path_{ip6_path} {}
-
 IpTables::~IpTables() {
   // Plug all holes when destructed.
   PlugAllHoles();
@@ -178,13 +173,13 @@ void IpTables::PlugAllHoles() {
 bool IpTables::AddAcceptRules(ProtocolEnum protocol,
                               uint16_t port,
                               const std::string& interface) {
-  if (!AddAcceptRule(ip4_exec_path_, protocol, port, interface)) {
-    LOG(ERROR) << "Could not add ACCEPT rule using '" << ip4_exec_path_ << "'";
+  if (!AddAcceptRule(kIpTablesPath, protocol, port, interface)) {
+    LOG(ERROR) << "Could not add ACCEPT rule using '" << kIpTablesPath << "'";
     return false;
   }
-  if (!AddAcceptRule(ip6_exec_path_, protocol, port, interface)) {
-    LOG(ERROR) << "Could not add ACCEPT rule using '" << ip6_exec_path_ << "'";
-    DeleteAcceptRule(ip4_exec_path_, protocol, port, interface);
+  if (!AddAcceptRule(kIp6TablesPath, protocol, port, interface)) {
+    LOG(ERROR) << "Could not add ACCEPT rule using '" << kIp6TablesPath << "'";
+    DeleteAcceptRule(kIpTablesPath, protocol, port, interface);
     return false;
   }
   return true;
@@ -193,9 +188,9 @@ bool IpTables::AddAcceptRules(ProtocolEnum protocol,
 bool IpTables::DeleteAcceptRules(ProtocolEnum protocol,
                                  uint16_t port,
                                  const std::string& interface) {
-  bool ip4_success = DeleteAcceptRule(ip4_exec_path_, protocol, port,
+  bool ip4_success = DeleteAcceptRule(kIpTablesPath, protocol, port,
                                       interface);
-  bool ip6_success = DeleteAcceptRule(ip6_exec_path_, protocol, port,
+  bool ip6_success = DeleteAcceptRule(kIp6TablesPath, protocol, port,
                                       interface);
   return ip4_success && ip6_success;
 }
@@ -204,45 +199,44 @@ bool IpTables::AddAcceptRule(const std::string& executable_path,
                              ProtocolEnum protocol,
                              uint16_t port,
                              const std::string& interface) {
-  chromeos::ProcessImpl iptables;
-  iptables.AddArg(executable_path);
-  iptables.AddArg("-I");  // insert
-  iptables.AddArg("INPUT");
-  iptables.AddArg("-p");  // protocol
-  iptables.AddArg(protocol == kProtocolTcp ? "tcp" : "udp");
-  iptables.AddArg("--dport");  // destination port
-  iptables.AddArg(std::to_string(port));
+  std::vector<std::string> argv;
+  argv.push_back(executable_path);
+  argv.push_back("-I");  // insert
+  argv.push_back("INPUT");
+  argv.push_back("-p");  // protocol
+  argv.push_back(protocol == kProtocolTcp ? "tcp" : "udp");
+  argv.push_back("--dport");  // destination port
+  argv.push_back(std::to_string(port));
   if (!interface.empty()) {
-    iptables.AddArg("-i");  // interface
-    iptables.AddArg(interface);
+    argv.push_back("-i");  // interface
+    argv.push_back(interface);
   }
-  iptables.AddArg("-j");
-  iptables.AddArg("ACCEPT");
+  argv.push_back("-j");
+  argv.push_back("ACCEPT");
 
-  return iptables.Run() == 0;
+  return Execv(argv) == 0;
 }
 
 bool IpTables::DeleteAcceptRule(const std::string& executable_path,
                                 ProtocolEnum protocol,
                                 uint16_t port,
                                 const std::string& interface) {
-  chromeos::ProcessImpl iptables;
-  iptables.AddArg(executable_path);
-  iptables.AddArg("-D");  // delete
-  iptables.AddArg("INPUT");
-  iptables.AddArg("-p");  // protocol
-  iptables.AddArg(protocol == kProtocolTcp ? "tcp" : "udp");
-  iptables.AddArg("--dport");  // destination port
-  std::string port_number = base::StringPrintf("%d", port);
-  iptables.AddArg(port_number.c_str());
+  std::vector<std::string> argv;
+  argv.push_back(executable_path);
+  argv.push_back("-D");  // delete
+  argv.push_back("INPUT");
+  argv.push_back("-p");  // protocol
+  argv.push_back(protocol == kProtocolTcp ? "tcp" : "udp");
+  argv.push_back("--dport");  // destination port
+  argv.push_back(std::to_string(port));
   if (interface != "") {
-    iptables.AddArg("-i");  // interface
-    iptables.AddArg(interface);
+    argv.push_back("-i");  // interface
+    argv.push_back(interface);
   }
-  iptables.AddArg("-j");
-  iptables.AddArg("ACCEPT");
+  argv.push_back("-j");
+  argv.push_back("ACCEPT");
 
-  return iptables.Run() == 0;
+  return Execv(argv) == 0;
 }
 
 bool IpTables::ApplyVpnSetup(const std::vector<std::string>& usernames,
@@ -290,38 +284,38 @@ bool IpTables::ApplyVpnSetup(const std::vector<std::string>& usernames,
 }
 
 bool IpTables::ApplyMasquerade(const std::string& interface, bool add) {
-  chromeos::ProcessImpl iptables;
-  iptables.AddArg(ip4_exec_path_);
-  iptables.AddArg("-t");  // table
-  iptables.AddArg("nat");
-  iptables.AddArg(add ? "-A" : "-D");  // rule
-  iptables.AddArg("POSTROUTING");
-  iptables.AddArg("-o");  // output interface
-  iptables.AddArg(interface);
-  iptables.AddArg("-j");
-  iptables.AddArg("MASQUERADE");
-
-  return iptables.Run() == 0;
+  std::vector<std::string> argv;
+  argv.push_back(kIpTablesPath);
+  argv.push_back("-t");  // table
+  argv.push_back("nat");
+  argv.push_back(add ? "-A" : "-D");  // rule
+  argv.push_back("POSTROUTING");
+  argv.push_back("-o");  // output interface
+  argv.push_back(interface);
+  argv.push_back("-j");
+  argv.push_back("MASQUERADE");
+
+  return Execv(argv) == 0;
 }
 
 bool IpTables::ApplyMarkForUserTraffic(const std::string& user_name,
                                        bool add) {
-  chromeos::ProcessImpl iptables;
-  iptables.AddArg(ip4_exec_path_);
-  iptables.AddArg("-t");  // table
-  iptables.AddArg("mangle");
-  iptables.AddArg(add ? "-A" : "-D");  // rule
-  iptables.AddArg("OUTPUT");
-  iptables.AddArg("-m");
-  iptables.AddArg("owner");
-  iptables.AddArg("--uid-owner");
-  iptables.AddArg(user_name);
-  iptables.AddArg("-j");
-  iptables.AddArg("MARK");
-  iptables.AddArg("--set-mark");
-  iptables.AddArg(kMarkForUserTraffic);
-
-  return iptables.Run() == 0;
+  std::vector<std::string> argv;
+  argv.push_back(kIpTablesPath);
+  argv.push_back("-t");  // table
+  argv.push_back("mangle");
+  argv.push_back(add ? "-A" : "-D");  // rule
+  argv.push_back("OUTPUT");
+  argv.push_back("-m");
+  argv.push_back("owner");
+  argv.push_back("--uid-owner");
+  argv.push_back(user_name);
+  argv.push_back("-j");
+  argv.push_back("MARK");
+  argv.push_back("--set-mark");
+  argv.push_back(kMarkForUserTraffic);
+
+  return Execv(argv) == 0;
 }
 
 bool IpTables::ApplyRuleForUserTraffic(bool add) {
@@ -337,4 +331,12 @@ bool IpTables::ApplyRuleForUserTraffic(bool add) {
   return ip.Run() == 0;
 }
 
+int IpTables::Execv(const std::vector<std::string>& argv) {
+  chromeos::ProcessImpl proc;
+  for (const auto& arg : argv) {
+    proc.AddArg(arg);
+  }
+  return proc.Run();
+}
+
 }  // namespace firewalld
diff --git a/iptables.h b/iptables.h
index 02edce5..4023ff6 100644
--- a/iptables.h
+++ b/iptables.h
@@ -25,7 +25,7 @@ class IpTables : public org::chromium::FirewalldInterface {
  public:
   typedef std::pair<uint16_t, std::string> Hole;
 
-  IpTables();
+  IpTables() = default;
   ~IpTables();
 
   // D-Bus methods.
@@ -42,10 +42,6 @@ class IpTables : public org::chromium::FirewalldInterface {
   // Close all outstanding firewall holes.
   void PlugAllHoles();
 
- protected:
-  // Test-only.
-  explicit IpTables(const std::string& ip4_path, const std::string& ip6_path);
-
  private:
   friend class IpTablesTest;
   FRIEND_TEST(IpTablesTest, ApplyVpnSetupAddSuccess);
@@ -70,29 +66,25 @@ class IpTables : public org::chromium::FirewalldInterface {
   bool DeleteAcceptRules(ProtocolEnum protocol,
                          uint16_t port,
                          const std::string& interface);
-  bool AddAcceptRule(const std::string& executable_path,
-                     ProtocolEnum protocol,
-                     uint16_t port,
-                     const std::string& interface);
-  bool DeleteAcceptRule(const std::string& executable_path,
-                        ProtocolEnum protocol,
-                        uint16_t port,
-                        const std::string& interface);
+
+  virtual bool AddAcceptRule(const std::string& executable_path,
+                             ProtocolEnum protocol,
+                             uint16_t port,
+                             const std::string& interface);
+  virtual bool DeleteAcceptRule(const std::string& executable_path,
+                                ProtocolEnum protocol,
+                                uint16_t port,
+                                const std::string& interface);
 
   bool ApplyVpnSetup(const std::vector<std::string>& usernames,
                      const std::string& interface,
                      bool add);
 
-  virtual bool ApplyMasquerade(const std::string& interface,
-                               bool add);
-
-  virtual bool ApplyMarkForUserTraffic(const std::string& user_name,
-                                       bool add);
-
+  virtual bool ApplyMasquerade(const std::string& interface, bool add);
+  virtual bool ApplyMarkForUserTraffic(const std::string& user_name, bool add);
   virtual bool ApplyRuleForUserTraffic(bool add);
 
-  std::string ip4_exec_path_;
-  std::string ip6_exec_path_;
+  int Execv(const std::vector<std::string>& argv);
 
   // Keep track of firewall holes to avoid adding redundant firewall rules.
   std::set<Hole> tcp_holes_;
diff --git a/iptables_unittest.cc b/iptables_unittest.cc
index 5b8c02d..8c95414 100644
--- a/iptables_unittest.cc
+++ b/iptables_unittest.cc
@@ -15,117 +15,171 @@ using testing::Return;
 
 class IpTablesTest : public testing::Test {
  public:
-  IpTablesTest()
-      : iptables_succeeds{"/bin/true", "/bin/true"},
-        iptables_fails{"/bin/false", "/bin/false"},
-        ip4succeeds_ip6fails{"/bin/true", "/bin/false"} {}
+  IpTablesTest() = default;
   ~IpTablesTest() override = default;
 
  protected:
-  IpTables iptables_succeeds;
-  IpTables iptables_fails;
-  IpTables ip4succeeds_ip6fails;
+  void SetMockExpectations(MockIpTables* iptables, bool success) {
+    EXPECT_CALL(*iptables, AddAcceptRule(_, _, _, _))
+        .WillRepeatedly(Return(success));
+    EXPECT_CALL(*iptables, DeleteAcceptRule(_, _, _, _))
+        .WillRepeatedly(Return(success));
+  }
+
+  void SetMockExpectationsPerExecutable(MockIpTables* iptables,
+                                        bool ip4_success,
+                                        bool ip6_success) {
+    EXPECT_CALL(*iptables, AddAcceptRule("/sbin/iptables", _, _, _))
+        .WillRepeatedly(Return(ip4_success));
+    EXPECT_CALL(*iptables, AddAcceptRule("/sbin/ip6tables", _, _, _))
+        .WillRepeatedly(Return(ip6_success));
+    EXPECT_CALL(*iptables, DeleteAcceptRule("/sbin/iptables", _, _, _))
+        .WillRepeatedly(Return(ip4_success));
+    EXPECT_CALL(*iptables, DeleteAcceptRule("/sbin/ip6tables", _, _, _))
+        .WillRepeatedly(Return(ip6_success));
+  }
 
  private:
   DISALLOW_COPY_AND_ASSIGN(IpTablesTest);
 };
 
 TEST_F(IpTablesTest, Port0Fails) {
+  MockIpTables mock_iptables;
+  // We should not be adding any rules for port 0.
+  EXPECT_CALL(mock_iptables, AddAcceptRule(_, _, _, _)).Times(0);
+  EXPECT_CALL(mock_iptables, DeleteAcceptRule(_, _, _, _)).Times(0);
   // Try to punch hole for TCP port 0, port 0 is not a valid port.
-  ASSERT_FALSE(iptables_succeeds.PunchTcpHole(0, "iface"));
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(0, "iface"));
   // Try to punch hole for UDP port 0, port 0 is not a valid port.
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(0, "iface"));
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(0, "iface"));
 }
 
-TEST_F(IpTablesTest, InvalidInterfaceName) {
-  ASSERT_FALSE(iptables_succeeds.PunchTcpHole(80, "reallylonginterfacename"));
-  ASSERT_FALSE(iptables_succeeds.PunchTcpHole(80, "with spaces"));
-  ASSERT_FALSE(iptables_succeeds.PunchTcpHole(80, "with$ymbols"));
-
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(53, "reallylonginterfacename"));
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(53, "with spaces"));
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(53, "with$ymbols"));
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(53, "-startdash"));
-  ASSERT_FALSE(iptables_succeeds.PunchUdpHole(53, "enddash-"));
+TEST_F(IpTablesTest, ValidInterfaceName) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, true /* success */);
+
+  EXPECT_TRUE(mock_iptables.PunchTcpHole(80, "shortname"));
+  EXPECT_TRUE(mock_iptables.PunchUdpHole(53, "shortname"));
+  EXPECT_TRUE(mock_iptables.PunchTcpHole(80, "middle-dash"));
+  EXPECT_TRUE(mock_iptables.PunchUdpHole(53, "middle-dash"));
 }
 
-TEST_F(IpTablesTest, ValidInterfaceName) {
-  ASSERT_TRUE(iptables_succeeds.PunchUdpHole(53, "middle-dash"));
+TEST_F(IpTablesTest, InvalidInterfaceName) {
+  MockIpTables mock_iptables;
+  // We should not be adding any rules for invalid interface names.
+  EXPECT_CALL(mock_iptables, AddAcceptRule(_, _, _, _)).Times(0);
+  EXPECT_CALL(mock_iptables, DeleteAcceptRule(_, _, _, _)).Times(0);
+
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "reallylonginterfacename"));
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "with spaces"));
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "with$ymbols"));
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "-startdash"));
+  EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "enddash-"));
+
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "reallylonginterfacename"));
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "with spaces"));
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "with$ymbols"));
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "-startdash"));
+  EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "enddash-"));
 }
 
 TEST_F(IpTablesTest, PunchTcpHoleSucceeds) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, true /* success */);
+
   // Punch hole for TCP port 80, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchTcpHole(80, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchTcpHole(80, "iface"));
   // Punch again, should still succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchTcpHole(80, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchTcpHole(80, "iface"));
   // Plug the hole, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PlugTcpHole(80, "iface"));
+  EXPECT_TRUE(mock_iptables.PlugTcpHole(80, "iface"));
 }
 
 TEST_F(IpTablesTest, PlugTcpHoleSucceeds) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, true /* success */);
+
   // Punch hole for TCP port 80, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchTcpHole(80, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchTcpHole(80, "iface"));
   // Plug the hole, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PlugTcpHole(80, "iface"));
+  EXPECT_TRUE(mock_iptables.PlugTcpHole(80, "iface"));
   // Plug again, should fail.
-  ASSERT_FALSE(iptables_succeeds.PlugTcpHole(80, "iface"));
+  EXPECT_FALSE(mock_iptables.PlugTcpHole(80, "iface"));
 }
 
 TEST_F(IpTablesTest, PunchUdpHoleSucceeds) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, true /* success */);
+
   // Punch hole for UDP port 53, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchUdpHole(53, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchUdpHole(53, "iface"));
   // Punch again, should still succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchUdpHole(53, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchUdpHole(53, "iface"));
   // Plug the hole, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PlugUdpHole(53, "iface"));
+  EXPECT_TRUE(mock_iptables.PlugUdpHole(53, "iface"));
 }
 
 TEST_F(IpTablesTest, PlugUdpHoleSucceeds) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, true /* success */);
+
   // Punch hole for UDP port 53, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PunchUdpHole(53, "iface"));
+  EXPECT_TRUE(mock_iptables.PunchUdpHole(53, "iface"));
   // Plug the hole, should succeed.
-  ASSERT_TRUE(iptables_succeeds.PlugUdpHole(53, "iface"));
+  EXPECT_TRUE(mock_iptables.PlugUdpHole(53, "iface"));
   // Plug again, should fail.
-  ASSERT_FALSE(iptables_succeeds.PlugUdpHole(53, "iface"));
+  EXPECT_FALSE(mock_iptables.PlugUdpHole(53, "iface"));
 }
 
 TEST_F(IpTablesTest, PunchTcpHoleFails) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, false /* success */);
   // Punch hole for TCP port 80, should fail.
-  ASSERT_FALSE(iptables_fails.PunchTcpHole(80, "iface"));
+  ASSERT_FALSE(mock_iptables.PunchTcpHole(80, "iface"));
 }
 
 TEST_F(IpTablesTest, PunchUdpHoleFails) {
+  MockIpTables mock_iptables;
+  SetMockExpectations(&mock_iptables, false /* success */);
   // Punch hole for UDP port 53, should fail.
-  ASSERT_FALSE(iptables_fails.PunchUdpHole(53, "iface"));
+  ASSERT_FALSE(mock_iptables.PunchUdpHole(53, "iface"));
 }
 
 TEST_F(IpTablesTest, PunchTcpHoleIpv6Fails) {
+  MockIpTables mock_iptables;
+  SetMockExpectationsPerExecutable(&mock_iptables, true /* ip4_success */,
+                                   false /* ip6_success */);
   // Punch hole for TCP port 80, should fail because 'ip6tables' fails.
-  ASSERT_FALSE(ip4succeeds_ip6fails.PunchTcpHole(80, "iface"));
+  ASSERT_FALSE(mock_iptables.PunchTcpHole(80, "iface"));
 }
 
 TEST_F(IpTablesTest, PunchUdpHoleIpv6Fails) {
+  MockIpTables mock_iptables;
+  SetMockExpectationsPerExecutable(&mock_iptables, true /* ip4_success */,
+                                   false /* ip6_success */);
   // Punch hole for UDP port 53, should fail because 'ip6tables' fails.
-  ASSERT_FALSE(ip4succeeds_ip6fails.PunchUdpHole(53, "iface"));
+  ASSERT_FALSE(mock_iptables.PunchUdpHole(53, "iface"));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAddSuccess) {
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
   const std::string interface = "ifc0";
   const bool add = true;
-  const bool success = true;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
-      .WillOnce(Return(success));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[0], add))
-      .WillOnce(Return(success));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[1], add))
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[0], add))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[1], add))
+      .WillOnce(Return(true));
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
 
-  EXPECT_EQ(success, mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  ASSERT_TRUE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, add));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInUsername) {
@@ -133,36 +187,38 @@ TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInUsername) {
   const std::string interface = "ifc0";
   const bool remove = false;
   const bool add = true;
-  const bool failure = false;
-  const bool success = true;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
       .Times(1)
-      .WillOnce(Return(success));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[0], add))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[0], add))
       .Times(1)
-      .WillOnce(Return(success));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[1], add))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[1], add))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
       .Times(1)
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
 
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
       .Times(1)
-      .WillOnce(Return(success));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[0], remove))
+      .WillOnce(Return(true));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[0], remove))
       .Times(1)
-      .WillOnce(Return(failure));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(usernames[1], remove))
-      .Times(0);
+      .WillOnce(Return(false));
+  EXPECT_CALL(mock_iptables,
+              ApplyMarkForUserTraffic(usernames[1], remove)).Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
 
-  EXPECT_EQ(failure, mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  ASSERT_FALSE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, add));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInMasquerade) {
@@ -170,24 +226,24 @@ TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInMasquerade) {
   const std::string interface = "ifc0";
   const bool remove = false;
   const bool add = true;
-  const bool failure = false;
-  const bool success = true;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
   EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, _)).Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
       .Times(1)
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
 
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove)).Times(0);
+  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
+      .Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
       .Times(1)
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
 
-  EXPECT_EQ(failure, mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  ASSERT_FALSE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, add));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInRuleForUserTraffic) {
@@ -195,18 +251,18 @@ TEST_F(IpTablesTest, ApplyVpnSetupAddFailureInRuleForUserTraffic) {
   const std::string interface = "ifc0";
   const bool remove = false;
   const bool add = true;
-  const bool failure = false;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, _)).Times(0);
   EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, _)).Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
 
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove)).Times(0);
 
-  EXPECT_EQ(failure, mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  ASSERT_FALSE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, add));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupRemoveSuccess) {
@@ -214,24 +270,26 @@ TEST_F(IpTablesTest, ApplyVpnSetupRemoveSuccess) {
   const std::string interface = "ifc0";
   const bool remove = false;
   const bool add = true;
-  const bool success = true;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
       .Times(1)
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
   EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, remove))
       .Times(2)
-      .WillRepeatedly(Return(success));
+      .WillRepeatedly(Return(true));
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
       .Times(1)
-      .WillOnce(Return(success));
+      .WillOnce(Return(true));
 
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add)).Times(0);
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, add)).Times(0);
+  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
+      .Times(0);
+  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, add))
+      .Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add)).Times(0);
 
-  EXPECT_EQ(success, mock_iptables.ApplyVpnSetup(usernames, interface, remove));
+  ASSERT_TRUE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, remove));
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupRemoveFailure) {
@@ -239,24 +297,26 @@ TEST_F(IpTablesTest, ApplyVpnSetupRemoveFailure) {
   const std::string interface = "ifc0";
   const bool remove = false;
   const bool add = true;
-  const bool failure = false;
 
   MockIpTables mock_iptables;
   EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
   EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, remove))
       .Times(2)
-      .WillRepeatedly(Return(failure));
+      .WillRepeatedly(Return(false));
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
       .Times(1)
-      .WillOnce(Return(failure));
+      .WillOnce(Return(false));
 
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add)).Times(0);
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, add)).Times(0);
+  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
+      .Times(0);
+  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, add))
+      .Times(0);
   EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add)).Times(0);
 
-  EXPECT_EQ(failure, mock_iptables.ApplyVpnSetup(usernames, interface, remove));
+  ASSERT_FALSE(
+      mock_iptables.ApplyVpnSetup(usernames, interface, remove));
 }
 
 }  // namespace firewalld
diff --git a/mock_iptables.cc b/mock_iptables.cc
index 13d800b..d9cd7f6 100644
--- a/mock_iptables.cc
+++ b/mock_iptables.cc
@@ -6,8 +6,8 @@
 
 namespace firewalld {
 
-MockIpTables::MockIpTables() : IpTables("", "") {}
-
-MockIpTables::~MockIpTables() {}
+MockIpTables::~MockIpTables() {
+  PlugAllHoles();
+}
 
 }  // namespace firewalld
diff --git a/mock_iptables.h b/mock_iptables.h
index 8f7e132..1ce291a 100644
--- a/mock_iptables.h
+++ b/mock_iptables.h
@@ -16,16 +16,22 @@ namespace firewalld {
 
 class MockIpTables : public IpTables {
  public:
-  MockIpTables();
+  MockIpTables() = default;
   ~MockIpTables() override;
 
-  MOCK_METHOD2(ApplyMasquerade, bool(const std::string& interface,
-                                     bool add));
+  MOCK_METHOD4(
+      AddAcceptRule,
+      bool(const std::string&, ProtocolEnum, uint16_t, const std::string&));
 
-  MOCK_METHOD2(ApplyMarkForUserTraffic, bool(const std::string& user_name,
-                                             bool add));
+  MOCK_METHOD4(
+      DeleteAcceptRule,
+      bool(const std::string&, ProtocolEnum, uint16_t, const std::string&));
 
-  MOCK_METHOD1(ApplyRuleForUserTraffic, bool(bool add));
+  MOCK_METHOD2(ApplyMasquerade, bool(const std::string&, bool));
+
+  MOCK_METHOD2(ApplyMarkForUserTraffic, bool(const std::string&, bool));
+
+  MOCK_METHOD1(ApplyRuleForUserTraffic, bool(bool));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockIpTables);