Merge pull request #32 from jagerman/fix-empty-set-bug

Fix assigning empty dicts/sets dirtying
oxen-io · May 12, 2023 · 97084c6 · 97084c6
2 parents 53c824d + b4e7c5d
commit 97084c6
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 2 deletions.
diff --git a/include/session/config/base.hpp b/include/session/config/base.hpp
@@ -189,6 +189,16 @@ class ConfigBase {
 
         template <typename T>
         void assign_if_changed(T value) {
+            if constexpr (is_one_of<T, config::set, config::dict>) {
+                // If we're assigning an empty set or dict then that's really the same as deleting
+                // the element, since empty sets/dicts get pruned.  If we *don't* do this, then
+                // assigning an empty value will dirty even though, ultimately, we aren't changing
+                // anything.
+                if (value.empty()) {
+                    erase();
+                    return;
+                }
+            }
             // Try to avoiding dirtying the config if this assignment isn't changing anything
             if (!_conf.is_dirty())
                 if (auto current = get_clean<T>(); current && *current == value)

diff --git a/include/session/config/user_groups.hpp b/include/session/config/user_groups.hpp
@@ -219,8 +219,6 @@ class UserGroups : public ConfigBase {
     ///
     void set(const community_info& info);
     void set(const legacy_group_info& info);
-    /// Takes a variant of either group type to set:
-    void set(const any_group_info& info);
 
   protected:
     // Drills into the nested dicts to access open group details

diff --git a/tests/test_config_user_groups.cpp b/tests/test_config_user_groups.cpp
@@ -552,6 +552,92 @@ TEST_CASE("User Groups members C API", "[config][groups][c]") {
     CHECK(grp->joined_at == created_ts);
 }
 
+TEST_CASE("User groups empty member bug", "[config][groups][bug]") {
+    // Tests a bug where setting legacy group with empty members (or empty admin) list would dirty
+    // the config, even when the current members (or admin) list is empty.  (This isn't strictly
+    // specific to user groups, but that's where the bug is easily encountered).
+
+    const auto seed = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa000000000000000000000000000000000"_hexbytes;
+    std::array<unsigned char, 32> ed_pk, curve_pk;
+    std::array<unsigned char, 64> ed_sk;
+    crypto_sign_ed25519_seed_keypair(
+            ed_pk.data(), ed_sk.data(), reinterpret_cast<const unsigned char*>(seed.data()));
+    int rc = crypto_sign_ed25519_pk_to_curve25519(curve_pk.data(), ed_pk.data());
+    REQUIRE(rc == 0);
+
+    CHECK(oxenc::to_hex(seed.begin(), seed.end()) ==
+          oxenc::to_hex(ed_sk.begin(), ed_sk.begin() + 32));
+
+    session::config::UserGroups c{ustring_view{seed}, std::nullopt};
+
+    CHECK_FALSE(c.needs_push());
+
+    {
+        auto lg = c.get_or_construct_legacy_group(
+                "051234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+
+        lg.insert("050000000000000000000000000000000000000000000000000000000000000000", true);
+        lg.insert("051111111111111111111111111111111111111111111111111111111111111111", true);
+        lg.insert("052222222222222222222222222222222222222222222222222222222222222222", true);
+
+        c.set(lg);
+    }
+
+    CHECK(c.needs_push());
+    auto [seqno, data, obs] = c.push();
+    CHECK(seqno == 1);
+    auto d = c.dump();
+    c.confirm_pushed(seqno, "fakehash1");
+    CHECK_FALSE(c.needs_push());
+
+    {
+        auto lg = c.get_or_construct_legacy_group(
+                "051234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+
+        lg.insert("050000000000000000000000000000000000000000000000000000000000000000", true);
+        lg.insert("051111111111111111111111111111111111111111111111111111111111111111", true);
+        lg.insert("052222222222222222222222222222222222222222222222222222222222222222", true);
+
+        // The bug was here: because *members* is empty, we would assign an empty set, which would
+        // set the dirty flag, even though an empty set gets pruned away, so end up with an empty
+        // diff.
+        c.set(lg);
+    }
+
+    {
+        auto lg = c.get_or_construct_legacy_group(
+                "051234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+
+        // Now add an actual member:
+        lg.insert("053333333333333333333333333333333333333333333333333333333333333333", false);
+        c.set(lg);
+    }
+
+    CHECK(c.needs_push());
+    c.push();
+
+    {
+        auto lg = c.get_or_construct_legacy_group(
+                "051234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+
+        // Remove them again, so that members is empty again (to make sure it gets emptied out in
+        // storage):
+        lg.erase("053333333333333333333333333333333333333333333333333333333333333333");
+        c.set(lg);
+    }
+    CHECK(c.needs_push());
+    c.push();
+
+    {
+        auto lg = c.get_or_construct_legacy_group(
+                "051234567890abcdef1234567890abcdef1234567890abcdef1234567890abcdef");
+
+        auto [admins, members] = lg.counts();
+        CHECK(admins == 3);
+        CHECK(members == 0);
+    }
+}
+
 namespace Catch {
 template <>
 struct StringMaker<std::pair<const std::string, bool>> {