Revise firewalld unittests

Previously, unit tests for firewalld in iptables_unittest.cc
were tightly coupled with the implementation of IpTables::ApplyVpnSetup
making them difficult to maintain when changes to the daemon were
introduced. This commit augments the MockIpTables class by mocking
ExecvNonRoot and keeping track of the ip/iptables shell commands that
are issued during a test. This makes it possible to test that error
cases properly handle undo-ing the addition of any firewall rules
that are added during IpTables::ApplyVpnSetup.

BUG=None
TEST=None
Change-Id: Ibfd581ebaad55826d95bdc3b5ad98795610aee9f
Reviewed-on: https://chromium-review.googlesource.com/370862
Commit-Ready: Kevin Cernekee <cernekee@chromium.org>
Tested-by: Ian Wyszynski <wyszynski@google.com>
Reviewed-by: Ian Wyszynski <wyszynski@google.com>
Reviewed-by: Kevin Cernekee <cernekee@chromium.org>
diff --git a/iptables.cc b/iptables.cc
index 7fc96da..992df58 100644
--- a/iptables.cc
+++ b/iptables.cc
@@ -32,17 +32,6 @@
 
 using IpTablesCallback = base::Callback<bool(const std::string&, bool)>;
 
-#if defined(__ANDROID__)
-const char kIpTablesPath[] = "/system/bin/iptables";
-const char kIp6TablesPath[] = "/system/bin/ip6tables";
-const char kIpPath[] = "/system/bin/ip";
-#else
-const char kIpTablesPath[] = "/sbin/iptables";
-const char kIp6TablesPath[] = "/sbin/ip6tables";
-const char kIpPath[] = "/bin/ip";
-const char kUnprivilegedUser[] = "nobody";
-#endif  // __ANDROID__
-
 const char kIPv4[] = "IPv4";
 const char kIPv6[] = "IPv6";
 
diff --git a/iptables.h b/iptables.h
index 74b9acb..beca79c 100644
--- a/iptables.h
+++ b/iptables.h
@@ -27,6 +27,17 @@
 
 #include "dbus_bindings/org.chromium.Firewalld.h"
 
+#if defined(__ANDROID__)
+const char kIpTablesPath[] = "/system/bin/iptables";
+const char kIp6TablesPath[] = "/system/bin/ip6tables";
+const char kIpPath[] = "/system/bin/ip";
+#else
+const char kIpTablesPath[] = "/sbin/iptables";
+const char kIp6TablesPath[] = "/sbin/ip6tables";
+const char kIpPath[] = "/bin/ip";
+const char kUnprivilegedUser[] = "nobody";
+#endif  // __ANDROID__
+
 namespace firewalld {
 
 enum ProtocolEnum { kProtocolTcp, kProtocolUdp };
@@ -104,7 +115,8 @@
   bool ApplyRuleForUserTrafficWithVersion(const std::string& ip_version,
                                           bool add);
 
-  int ExecvNonRoot(const std::vector<std::string>& argv, uint64_t capmask);
+  virtual int ExecvNonRoot(const std::vector<std::string>& argv,
+                           uint64_t capmask);
 
   // 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 615b4f8..bb3cff6 100644
--- a/iptables_unittest.cc
+++ b/iptables_unittest.cc
@@ -18,16 +18,6 @@
 
 #include "mock_iptables.h"
 
-namespace {
-#if defined(__ANDROID__)
-const char kIpTablesPath[] = "/system/bin/iptables";
-const char kIp6TablesPath[] = "/system/bin/ip6tables";
-#else
-const char kIpTablesPath[] = "/sbin/iptables";
-const char kIp6TablesPath[] = "/sbin/ip6tables";
-#endif  // __ANDROID__
-}  // namespace
-
 namespace firewalld {
 
 using testing::_;
@@ -40,23 +30,21 @@
 
  protected:
   void SetMockExpectations(MockIpTables* iptables, bool success) {
-    EXPECT_CALL(*iptables, AddAcceptRule(_, _, _, _))
-        .WillRepeatedly(Return(success));
-    EXPECT_CALL(*iptables, DeleteAcceptRule(_, _, _, _))
-        .WillRepeatedly(Return(success));
+    // Empty criterion matches all commands.
+    iptables->SetExecvNonRootFailCriterion(std::vector<std::string>(),
+                                           true, success);
   }
 
   void SetMockExpectationsPerExecutable(MockIpTables* iptables,
                                         bool ip4_success,
                                         bool ip6_success) {
-    EXPECT_CALL(*iptables, AddAcceptRule(kIpTablesPath, _, _, _))
-        .WillRepeatedly(Return(ip4_success));
-    EXPECT_CALL(*iptables, AddAcceptRule(kIp6TablesPath, _, _, _))
-        .WillRepeatedly(Return(ip6_success));
-    EXPECT_CALL(*iptables, DeleteAcceptRule(kIpTablesPath, _, _, _))
-        .WillRepeatedly(Return(ip4_success));
-    EXPECT_CALL(*iptables, DeleteAcceptRule(kIp6TablesPath, _, _, _))
-        .WillRepeatedly(Return(ip6_success));
+    if (!ip4_success)
+      iptables->SetExecvNonRootFailCriterion(
+          std::vector<std::string>({kIpTablesPath}), true /* repeat */,
+                                   false /* omit_failure */);
+    if (!ip6_success)
+      iptables->SetExecvNonRootFailCriterion(
+          std::vector<std::string>({kIp6TablesPath}), true, false);
   }
 
  private:
@@ -65,13 +53,17 @@
 
 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);
+  // Don't fail on anything.
+  int id = mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>(), true, true);
+
   // Try to punch hole for TCP port 0, port 0 is not a valid port.
   EXPECT_FALSE(mock_iptables.PunchTcpHole(0, "iface"));
   // Try to punch hole for UDP port 0, port 0 is not a valid port.
   EXPECT_FALSE(mock_iptables.PunchUdpHole(0, "iface"));
+
+  // We should not be adding any rules for port 0.
+  EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 0);
 }
 
 TEST_F(IpTablesTest, ValidInterfaceName) {
@@ -88,9 +80,8 @@
 
 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);
+  int id = mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>(), true, true);
 
   EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "reallylonginterfacename"));
   EXPECT_FALSE(mock_iptables.PunchTcpHole(80, "with spaces"));
@@ -107,6 +98,9 @@
   EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "enddash-"));
   EXPECT_FALSE(mock_iptables.PunchUdpHole(53, ".startdot"));
   EXPECT_FALSE(mock_iptables.PunchUdpHole(53, "enddot."));
+
+  // We should not be adding any rules for invalid interface names.
+  EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 0);
 }
 
 TEST_F(IpTablesTest, PunchTcpHoleSucceeds) {
@@ -193,17 +187,6 @@
   const bool add = true;
 
   MockIpTables mock_iptables;
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
-      .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(true));
-
   ASSERT_TRUE(
       mock_iptables.ApplyVpnSetup(usernames, interface, add));
 }
@@ -211,114 +194,67 @@
 TEST_F(IpTablesTest, ApplyVpnSetupAdd_FailureInUsername) {
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
   const std::string interface = "ifc0";
-  const bool remove = false;
   const bool add = true;
 
   MockIpTables mock_iptables;
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  EXPECT_CALL(mock_iptables,
-              ApplyMarkForUserTraffic(usernames[0], add))
-      .Times(1)
-      .WillOnce(Return(true));
-  EXPECT_CALL(mock_iptables,
-              ApplyMarkForUserTraffic(usernames[1], add))
-      .Times(1)
-      .WillOnce(Return(false));
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  EXPECT_CALL(mock_iptables,
-              ApplyMarkForUserTraffic(usernames[0], remove))
-      .Times(1)
-      .WillOnce(Return(false));
-  EXPECT_CALL(mock_iptables,
-              ApplyMarkForUserTraffic(usernames[1], remove))
-              .Times(0);
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
-      .Times(1)
-      .WillOnce(Return(false));
-
+  // Fail on all commands issued for testuser1.
+  int id = mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({usernames[1], "-A"}), true, false);
+  // Fail on all the calls to ExecvNonRoot issued by the
+  // attempt to apply marks for the last user.
   ASSERT_FALSE(
       mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
+
+  // Should have failed only on the first testuser1 command.
+  EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAdd_FailureInMasquerade) {
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
+  // First two calls are issued by ApplyRuleForUserTraffic, next two
+  // are issued by ApplyMasquerade. We make both of those fail.
   const std::string interface = "ifc0";
-  const bool remove = false;
   const bool add = true;
 
   MockIpTables mock_iptables;
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, add))
-      .Times(1)
-      .WillOnce(Return(false));
-
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, _)).Times(0);
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(add))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
-      .Times(1)
-      .WillOnce(Return(true));
-
+  // Fail on calls adding MASQUERADE.
+  int id = mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"MASQUERADE", "-A"}), true, false);
   ASSERT_FALSE(
       mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
+  EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupAdd_FailureInRuleForUserTraffic) {
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
   const std::string interface = "ifc0";
-  const bool remove = false;
   const bool add = true;
 
   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(false));
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove)).Times(1);
-
+  int id = mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"rule", "add", "fwmark"}), true, false);
   ASSERT_FALSE(mock_iptables.ApplyVpnSetup(usernames, interface, add));
+  EXPECT_TRUE(mock_iptables.CheckCommandsUndone());
+  EXPECT_TRUE(mock_iptables.GetExecvNonRootCriterionMatchCount(id) == 1);
 }
 
 TEST_F(IpTablesTest, ApplyVpnSetupRemove_Success) {
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
   const std::string interface = "ifc0";
   const bool remove = false;
-  const bool add = true;
 
   MockIpTables mock_iptables;
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
-      .Times(1)
-      .WillOnce(Return(true));
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, remove))
-      .Times(2)
-      .WillRepeatedly(Return(true));
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
-      .Times(1)
-      .WillOnce(Return(true));
-
-  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);
+  // The fail criterions ensure that the ApplyVpnSetup function will delete
+  // the rules that are added. In addition to this, test that when all of
+  // these commands succeed, ApplyVpnSetup returns true.
+  mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"MASQUERADE", "-D"}), true, true);
+  mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"--uid-owner", "-D"}), true, true);
+  mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"rule", "delete", "fwmark"}), true, true);
 
   ASSERT_TRUE(mock_iptables.ApplyVpnSetup(usernames, interface, remove));
 }
@@ -327,26 +263,13 @@
   const std::vector<std::string> usernames = {"testuser0", "testuser1"};
   const std::string interface = "ifc0";
   const bool remove = false;
-  const bool add = true;
 
   MockIpTables mock_iptables;
-  EXPECT_CALL(mock_iptables, ApplyMasquerade(interface, remove))
-      .Times(1)
-      .WillRepeatedly(Return(false));
-
-  EXPECT_CALL(mock_iptables, ApplyMarkForUserTraffic(_, remove))
-      .Times(2)
-      .WillRepeatedly(Return(false));
-
-  EXPECT_CALL(mock_iptables, ApplyRuleForUserTraffic(remove))
-      .Times(1)
-      .WillRepeatedly(Return(false));
-
-  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);
-
+  // Make all removing commands fail.
+  mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"delete"}), true, false);
+  mock_iptables.SetExecvNonRootFailCriterion(
+      std::vector<std::string>({"-D"}), true, false);
   ASSERT_FALSE(mock_iptables.ApplyVpnSetup(usernames, interface, remove));
 }
 
diff --git a/mock_iptables.cc b/mock_iptables.cc
index 767637f..8fe817b 100644
--- a/mock_iptables.cc
+++ b/mock_iptables.cc
@@ -12,6 +12,8 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
+#include <algorithm>
+
 #include "mock_iptables.h"
 
 namespace firewalld {
@@ -20,4 +22,99 @@
   PlugAllHoles();
 }
 
+int MockIpTables::SetExecvNonRootFailCriterion(
+    const std::vector<std::string>& keywords, bool repeat,
+    bool omit_failure) {
+  match_criteria_.push_back(Criterion{keywords, repeat, omit_failure, 0});
+  return match_criteria_.size() - 1;
+}
+
+int MockIpTables::GetExecvNonRootCriterionMatchCount(int id) {
+  if (id < 0 || id >= (int) match_criteria_.size())
+    return -1;
+  return match_criteria_[id].match_count;
+}
+
+bool MockIpTables::MatchAndUpdate(const std::vector<std::string>& argv) {
+  for (auto& criterion : match_criteria_) {
+    bool match = true;
+    // Empty criterion is a catch all -- fail on any ExecvNonRoot.
+    for (const std::string& keyword : criterion.keywords) {
+      match = match &&
+        (std::find(argv.begin(), argv.end(), keyword) != argv.end());
+    }
+    if (match) {
+      criterion.match_count++;
+      if (!criterion.repeat) {
+        match_criteria_.erase(std::remove(match_criteria_.begin(),
+                                          match_criteria_.end(),
+                                          criterion),
+                              match_criteria_.end());
+      }
+      // If not negating, treat as fail criterion,
+      // otherwise ignore (return false).
+      return !criterion.omit_failure;
+    }
+  }
+  return false;
+}
+
+int MockIpTables::ExecvNonRoot(
+    const std::vector<std::string>& argv, uint64_t capmask) {
+  if (MatchAndUpdate(argv))
+    return 1;
+  commands_.push_back(argv);
+  return 0;
+}
+
+std::vector<std::string> MockIpTables::GetInverseCommand(
+    const std::vector<std::string>& argv) {
+  std::vector<std::string> inverse;
+  bool isIpTablesCommand = (argv.size() > 0) && argv[0] == kIpTablesPath;
+  bool isIpCommand = (argv.size() > 0) && argv[0] == kIpPath;
+  for (const auto& arg : argv) {
+    if (arg == "-A" && isIpTablesCommand)
+      inverse.push_back("-D");
+    else if (arg == "-D" && isIpTablesCommand)
+      inverse.push_back("-A");
+    else if (arg == "add" && isIpCommand)
+      inverse.push_back("delete");
+    else if (arg == "delete" && isIpCommand)
+      inverse.push_back("add");
+    else
+      inverse.push_back(arg);
+  }
+  return inverse;
+}
+
+// This check does not enforce ordering. It only checks
+// that for each command that adds a rule/mark with
+// ip/iptables, there is a later match command that
+// deletes that same rule/mark.
+bool MockIpTables::CheckCommandsUndone() {
+  for (std::vector<std::vector<std::string>>::iterator it = commands_.begin();
+       it != commands_.end(); it++) {
+    // For each command that adds a rule, check that its inverse
+    // exists later in the log of commands.
+    if ((std::find(it->begin(), it->end(), "-A") != it->end()) ||
+        (std::find(it->begin(), it->end(), "add") != it->end())) {
+      bool found = false;
+      std::vector<std::string> inverse = GetInverseCommand(*it);
+      // If GetInverseCommand returns the same command, then it was
+      // not an ip/iptables command that added/removed a rule/mark.
+      if (std::equal(it->begin(), it->end(), inverse.begin()))
+        continue;
+      for (auto it_next = it + 1; it_next != commands_.end(); it_next++) {
+        if (*it_next == inverse) {
+          found = true;
+          break;
+        }
+      }
+      if (!found)
+        return false;
+    }
+  }
+  return true;
+}
+
 }  // namespace firewalld
diff --git a/mock_iptables.h b/mock_iptables.h
index 54aaa25..cb0b5df 100644
--- a/mock_iptables.h
+++ b/mock_iptables.h
@@ -29,20 +29,51 @@
   MockIpTables() = default;
   ~MockIpTables() override;
 
-  MOCK_METHOD4(
-      AddAcceptRule,
-      bool(const std::string&, ProtocolEnum, uint16_t, const std::string&));
+  int SetExecvNonRootFailCriterion(
+      const std::vector<std::string>& keywords, bool repeat, bool omit_failure);
+  int GetExecvNonRootCriterionMatchCount(int id);
+  bool CheckCommandsUndone();
 
-  MOCK_METHOD4(
-      DeleteAcceptRule,
-      bool(const std::string&, ProtocolEnum, uint16_t, const std::string&));
-
-  MOCK_METHOD2(ApplyMasquerade, bool(const std::string&, bool));
-  MOCK_METHOD2(ApplyMarkForUserTraffic, bool(const std::string&, bool));
-  MOCK_METHOD1(ApplyRuleForUserTraffic, bool(bool));
+  // Check if the current command matches a failure rule,
+  // if the failure rule is not a repeat rule, remove it
+  // from the match criteria.
+  bool MatchAndUpdate(const std::vector<std::string>& argv);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockIpTables);
+
+  struct Criterion {
+    std::vector<std::string> keywords;
+    // If false, remove the criterion after it's matched.
+    bool repeat;
+    // If false, treat matching commands as failures, otherwise,
+    // omit failing.
+    bool omit_failure;
+    // Count the number of times the criterion is matched.
+    int match_count;
+
+    // Don't take into account match_count.
+    bool operator==(const Criterion& c) const {
+      return (std::equal(keywords.begin(), keywords.end(),
+                         c.keywords.begin()) &&
+              (repeat == c.repeat) &&
+              (c.omit_failure == omit_failure));
+    }
+  };
+
+  // A list of collections of keywords that a command
+  // must have in order for it to fail.
+  std::vector<Criterion> match_criteria_;
+
+  // A log of commands issued with ExecvNonRoot during the test.
+  std::vector<std::vector<std::string>> commands_;
+
+  // The mock's implementation simply logs the command issued if
+  // successful.
+  int ExecvNonRoot(const std::vector<std::string>& argv, uint64_t capmask) override;
+
+  // Given an ip/iptables command, return the command that undoes it's effect.
+  std::vector<std::string> GetInverseCommand(const std::vector<std::string>& command);
 };
 
 }  // namespace firewalld