From 9a1050d29b65e2f740631fa2b3514848b89ddd06 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Wed, 10 May 2023 22:09:48 -0300 Subject: [PATCH 1/2] Remove unimplemented declaration --- include/session/config/user_groups.hpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/include/session/config/user_groups.hpp b/include/session/config/user_groups.hpp index 9ab69117..eb63e4fa 100644 --- 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 From b4e7c5deb89c2370dac7356aae861701bff97ad9 Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Wed, 10 May 2023 22:10:14 -0300 Subject: [PATCH 2/2] Fix assigning empty dicts/sets dirtying When assigning an empty set/dict to a location that doesn't currently exist, we'd dirty the config, assign the empty dict/set, but then when we produce the update it is unchanged because empty dicts/sets get pruned. This fixes it so that such assignment becomes an *erase* instead, since that what the final result will be. --- include/session/config/base.hpp | 10 ++++ tests/test_config_user_groups.cpp | 86 +++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/include/session/config/base.hpp b/include/session/config/base.hpp index 46b71c2f..091184a7 100644 --- a/include/session/config/base.hpp +++ b/include/session/config/base.hpp @@ -189,6 +189,16 @@ class ConfigBase { template void assign_if_changed(T value) { + if constexpr (is_one_of) { + // 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(); current && *current == value) diff --git a/tests/test_config_user_groups.cpp b/tests/test_config_user_groups.cpp index 0d2e298e..cccf18bf 100644 --- 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 ed_pk, curve_pk; + std::array ed_sk; + crypto_sign_ed25519_seed_keypair( + ed_pk.data(), ed_sk.data(), reinterpret_cast(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> {