From 9498199923402eb14c4a248914bb4cc84d29ac10 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 15 Aug 2024 12:53:02 -0700 Subject: [PATCH 1/3] fix: handle protected attributes correctly --- .../launchdarkly/attributes_builder.hpp | 28 ++-- libs/common/src/attributes_builder.cpp | 93 ++++++++++-- libs/common/tests/context_builder_test.cpp | 140 +++++++++++++----- 3 files changed, 197 insertions(+), 64 deletions(-) diff --git a/libs/common/include/launchdarkly/attributes_builder.hpp b/libs/common/include/launchdarkly/attributes_builder.hpp index bc0dc82c9..e28021003 100644 --- a/libs/common/include/launchdarkly/attributes_builder.hpp +++ b/libs/common/include/launchdarkly/attributes_builder.hpp @@ -1,13 +1,13 @@ #pragma once -#include - #include #include #include -namespace launchdarkly { +#include +#include +namespace launchdarkly { class ContextBuilder; /** @@ -22,7 +22,7 @@ template class AttributesBuilder final { friend class ContextBuilder; - public: +public: /** * Create an attributes builder with the given kind and key. * @param builder The context builder associated with this attributes @@ -31,7 +31,8 @@ class AttributesBuilder final { * @param key The key for the kind. */ AttributesBuilder(BuilderReturn& builder, std::string kind, std::string key) - : key_(std::move(key)), kind_(std::move(kind)), builder_(builder) {} + : key_(std::move(key)), kind_(std::move(kind)), builder_(builder) { + } /** * Crate an attributes builder with the specified kind, and pre-populated @@ -96,11 +97,9 @@ class AttributesBuilder final { * * @param name The name of the attribute. * @param value The value for the attribute. - * @param private_attribute If the attribute should be considered private: - * that is, the value will not be sent to LaunchDarkly in analytics events. * @return A reference to the current builder. */ - AttributesBuilder& Set(std::string name, launchdarkly::Value value); + AttributesBuilder& Set(std::string name, Value value); /** * Add or update a private attribute in the context. @@ -115,11 +114,9 @@ class AttributesBuilder final { * * @param name The name of the attribute. * @param value The value for the attribute. - * @param private_attribute If the attribute should be considered private: - * that is, the value will not be sent to LaunchDarkly in analytics events. * @return A reference to the current builder. */ - AttributesBuilder& SetPrivate(std::string name, launchdarkly::Value value); + AttributesBuilder& SetPrivate(std::string name, Value value); /** * Designate a context attribute, or properties within them, as private: @@ -216,7 +213,7 @@ class AttributesBuilder final { */ [[nodiscard]] BuildType Build() const { return builder_.Build(); } - private: +private: BuilderReturn& builder_; /** @@ -229,15 +226,16 @@ class AttributesBuilder final { Attributes BuildAttributes() const; AttributesBuilder& Set(std::string name, - launchdarkly::Value value, + Value value, bool private_attribute); + std::string kind_; std::string key_; std::string name_; bool anonymous_ = false; - std::map values_; + std::map values_; AttributeReference::SetType private_attributes_; }; -} // namespace launchdarkly +} // namespace launchdarkly diff --git a/libs/common/src/attributes_builder.cpp b/libs/common/src/attributes_builder.cpp index 0f8d2a6d2..3b74298e1 100644 --- a/libs/common/src/attributes_builder.cpp +++ b/libs/common/src/attributes_builder.cpp @@ -1,8 +1,10 @@ #include #include -namespace launchdarkly { +#include +#include +namespace launchdarkly { template <> AttributesBuilder& AttributesBuilder::Name(std::string name) { @@ -17,33 +19,104 @@ AttributesBuilder::Anonymous(bool anonymous) { return *this; } +// These attributes values cannot be set via the public Set/SetPrivate methods. +// 'key' and 'kind' are defined in the AttributesBuilder constructor, whereas +// '_meta' is set internally by the SDK. +std::set const& ProtectedAttributes() { + static std::set protectedAttrs = { + "key", + "kind", + "_meta" + }; + return protectedAttrs; +} + +// Utility struct to enforce the type-safe setting of a particular attribute +// via a method on AttributesBuilder. +struct TypedAttribute { + enum Value::Type type; + std::function& builder, + Value)> setter; + + TypedAttribute(enum Value::Type type, + std::function& builder, + Value const&)> setter) + : type(type), setter(std::move(setter)) { + } +}; + +// These attribute values, while able to be set via public Set/SetPrivate methods, +// are regarded as special and are type-enforced. Instead of being stored in the +// internal AttributesBuilder::values map, they are stored as individual fields. +// This is because they have special meaning/usage within the LaunchDarkly Platform. +std::unordered_map const& +TypedAttributes() { + static std::unordered_map typedAttrs = { + { + "anonymous", {Value::Type::kBool, + [](AttributesBuilder< + ContextBuilder, Context> + & builder, + Value const& value) { + builder.Anonymous( + value.AsBool()); + }} + }, + {"name", {Value::Type::kString, + [](AttributesBuilder< + ContextBuilder, Context>& + builder, + Value const& value) { + builder.Name(value.AsString()); + }}} + }; + return typedAttrs; +} + + template <> AttributesBuilder& AttributesBuilder::Set(std::string name, Value value, bool private_attribute) { - if (name == "key" || name == "kind" || name == "anonymous" || - name == "name" || name == "_meta") { + // Protected attributes cannot be set at all. + if (ProtectedAttributes().count(name) > 0) { + return *this; + } + + // Typed attributes can be set only if the value is of the correct type; + // the setting is forwarded to one of AttributeBuilder's dedicated methods. + auto const typed_attr = TypedAttributes().find(name); + const bool is_typed = typed_attr != TypedAttributes().end(); + if (is_typed && value.Type() != typed_attr->second.type) { return *this; } + if (is_typed) { + typed_attr->second.setter(*this, value); + } else { + values_[name] = std::move(value); + } + if (private_attribute) { - this->private_attributes_.insert(name); + this->private_attributes_.insert(std::move(name)); } - values_[std::move(name)] = std::move(value); return *this; } template <> AttributesBuilder& -AttributesBuilder::Set(std::string name, Value value) { +AttributesBuilder::Set( + std::string name, + Value value) { return Set(std::move(name), std::move(value), false); } template <> AttributesBuilder& AttributesBuilder::SetPrivate(std::string name, - Value value) { + Value value) { return Set(std::move(name), std::move(value), true); } @@ -56,8 +129,8 @@ AttributesBuilder::AddPrivateAttribute( } template <> -Attributes AttributesBuilder::BuildAttributes() const { +Attributes AttributesBuilder< + ContextBuilder, Context>::BuildAttributes() const { return {key_, name_, anonymous_, values_, private_attributes_}; } - -} // namespace launchdarkly +} // namespace launchdarkly diff --git a/libs/common/tests/context_builder_test.cpp b/libs/common/tests/context_builder_test.cpp index f5a46fa57..5f61ff6d6 100644 --- a/libs/common/tests/context_builder_test.cpp +++ b/libs/common/tests/context_builder_test.cpp @@ -29,14 +29,14 @@ TEST(ContextBuilderTests, CanMakeBasicContext) { TEST(ContextBuilderTests, CanMakeSingleContextWithCustomAttributes) { auto context = ContextBuilder() - .Kind("user", "bobby-bobberson") - .Name("Bob") - .Anonymous(true) - // Set a custom attribute. - .Set("likesCats", true) - // Set a private custom attribute. - .SetPrivate("email", "email@email.email") - .Build(); + .Kind("user", "bobby-bobberson") + .Name("Bob") + .Anonymous(true) + // Set a custom attribute. + .Set("likesCats", true) + // Set a private custom attribute. + .SetPrivate("email", "email@email.email") + .Build(); EXPECT_TRUE(context.Valid()); @@ -56,25 +56,25 @@ TEST(ContextBuilderTests, CanMakeSingleContextWithCustomAttributes) { TEST(ContextBuilderTests, CanBuildComplexMultiContext) { auto context = ContextBuilder() - .Kind("user", "user-key") - .Anonymous(true) - .Name("test") - .Set("string", "potato") - .Set("int", 42) - .Set("double", 3.14) - .Set("array", {false, true, 42}) - - .SetPrivate("private", "this is private") - .AddPrivateAttribute("double") - .AddPrivateAttributes(std::vector{"string", "int"}) - .AddPrivateAttributes( - std::vector{"explicitArray"}) - // Start the org kind. - .Kind("org", "org-key") - .Name("Macdonwalds") - .Set("explicitArray", Array{"egg", "ham"}) - .Set("object", Object{{"string", "bacon"}, {"boolean", false}}) - .Build(); + .Kind("user", "user-key") + .Anonymous(true) + .Name("test") + .Set("string", "potato") + .Set("int", 42) + .Set("double", 3.14) + .Set("array", {false, true, 42}) + + .SetPrivate("private", "this is private") + .AddPrivateAttribute("double") + .AddPrivateAttributes(std::vector{"string", "int"}) + .AddPrivateAttributes( + std::vector{"explicitArray"}) + // Start the org kind. + .Kind("org", "org-key") + .Name("Macdonwalds") + .Set("explicitArray", Array{"egg", "ham"}) + .Set("object", Object{{"string", "bacon"}, {"boolean", false}}) + .Build(); EXPECT_TRUE(context.Valid()); @@ -94,7 +94,7 @@ TEST(ContextBuilderTests, CanBuildComplexMultiContext) { EXPECT_EQ(1, context.Attributes("user").PrivateAttributes().count("double")); EXPECT_EQ(1, context.Attributes("user").PrivateAttributes().count( - "explicitArray")); + "explicitArray")); EXPECT_EQ(1, context.Attributes("user").PrivateAttributes().count("string")); EXPECT_EQ(1, @@ -177,11 +177,11 @@ TEST(ContextBuilderTests, AccessKindBuilderMultipleTimes) { TEST(ContextBuilderTests, AddAttributeToExistingContext) { auto context = ContextBuilder() - .Kind("user", "potato") - .Name("Bob") - .Set("city", "Reno") - .SetPrivate("private", "a") - .Build(); + .Kind("user", "potato") + .Name("Bob") + .Set("city", "Reno") + .SetPrivate("private", "a") + .Build(); auto builder = ContextBuilder(context); if (auto updater = builder.Kind("user")) { @@ -196,16 +196,20 @@ TEST(ContextBuilderTests, AddAttributeToExistingContext) { EXPECT_EQ("Nevada", updated_context.Get("user", "state").AsString()); EXPECT_EQ(2, updated_context.Attributes("user").PrivateAttributes().size()); - EXPECT_EQ(1, updated_context.Attributes("user").PrivateAttributes().count("private")); - EXPECT_EQ(1, updated_context.Attributes("user").PrivateAttributes().count("sneaky")); + EXPECT_EQ( + 1, updated_context.Attributes("user").PrivateAttributes().count( + "private")); + EXPECT_EQ( + 1, updated_context.Attributes("user").PrivateAttributes().count("sneaky" + )); } TEST(ContextBuilderTests, AddKindToExistingContext) { auto context = ContextBuilder() - .Kind("user", "potato") - .Name("Bob") - .Set("city", "Reno") - .Build(); + .Kind("user", "potato") + .Name("Bob") + .Set("city", "Reno") + .Build(); auto updated_context = ContextBuilder(context).Kind("org", "org-key").Build(); @@ -241,4 +245,62 @@ TEST(ContextBuilderTests, UseTheSameBuilderToBuildMultipleContexts) { EXPECT_EQ("tomato", context_c.Get("bob", "key").AsString()); } +TEST(ContextBuilderTests, CannotSetProtectedAttributes) { + auto builder = ContextBuilder(); + auto context = builder.Kind("user", "potato").Set("_meta", "a"). + Set("kind", "b").Set("key", "c").Build(); + ASSERT_TRUE(context.Valid()); + EXPECT_EQ(context.Get("user", "_meta"), Value::Null()); + EXPECT_EQ(context.Get("user", "kind"), Value::Null()); + EXPECT_EQ(context.Get("user", "key"), "potato"); +} + +TEST(ContextBuilderTests, CannotMarkProtectedAttributesAsPrivate) { + auto builder = ContextBuilder(); + auto context = builder.Kind("user", "potato").SetPrivate("_meta", "a"). + SetPrivate("kind", "b").SetPrivate("key", "c"). + Build(); + ASSERT_TRUE(context.Valid()); + EXPECT_EQ(context.Get("user", "_meta"), Value::Null()); + EXPECT_EQ(context.Get("user", "kind"), Value::Null()); + EXPECT_EQ(context.Get("user", "key"), "potato"); + EXPECT_EQ(context.Attributes("user").PrivateAttributes().size(), 0); +} + +TEST(ContextBuilderTests, CannotSetIncorrectTypedAttributes) { + auto builder = ContextBuilder(); + auto context = builder.Kind("user", "potato").Set("anonymous", "hello"). + Set("name", 42).Build(); + ASSERT_TRUE(context.Valid()); + // By default, anonymous is false. + EXPECT_EQ(context.Get("user", "anonymous"), Value(false)); + // By default, name is the empty string. + EXPECT_EQ(context.Get("user", "name"), Value("")); +} + +TEST(ContextBuilderTests, CanSetCorrectTypedAttributes) { + auto builder = ContextBuilder(); + auto context = builder.Kind("user", "potato").Set("anonymous", true). + Set("name", "Bob").Build(); + ASSERT_TRUE(context.Valid()); + EXPECT_EQ(context.Get("user", "anonymous"), Value(true)); + EXPECT_EQ(context.Get("user", "name"), Value("Bob")); +} + +TEST(ContextBuilderTests, CanMarkTypedAttributesAsPrivate) { + auto builder = ContextBuilder(); + auto context = builder.Kind("user", "potato").SetPrivate( + "anonymous", Value(true)).SetPrivate("name", Value("Bob")).Build(); + ASSERT_TRUE(context.Valid()); + // Even if these attributes cannot *actually* be redacted, the attributes builder + // still allows them to be marked private. Whether this should be allowed or not is + // up for debate (ideally we'd log it, but we don't have access to a logger. We could + // silently not allow it to be added to the private attributes list instead.) + EXPECT_EQ(context.Get("user", "anonymous"), Value(true)); + EXPECT_EQ(context.Attributes("user").PrivateAttributes().count("anonymous"), + 1); + EXPECT_EQ(context.Get("user", "name"), Value("Bob")); + EXPECT_EQ(context.Attributes("user").PrivateAttributes().count("name"), 1); +} + // NOLINTEND cppcoreguidelines-avoid-magic-numbers From 717b35df5c941d9fddbc4465e9ff17c6496a74a3 Mon Sep 17 00:00:00 2001 From: Casey Waldren Date: Thu, 15 Aug 2024 13:27:10 -0700 Subject: [PATCH 2/3] add missing functional header, and re-arrange AttribtueBuilder constructor init list --- libs/common/include/launchdarkly/attributes_builder.hpp | 6 +++--- libs/common/src/attributes_builder.cpp | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/libs/common/include/launchdarkly/attributes_builder.hpp b/libs/common/include/launchdarkly/attributes_builder.hpp index e28021003..3c7765507 100644 --- a/libs/common/include/launchdarkly/attributes_builder.hpp +++ b/libs/common/include/launchdarkly/attributes_builder.hpp @@ -31,7 +31,7 @@ class AttributesBuilder final { * @param key The key for the kind. */ AttributesBuilder(BuilderReturn& builder, std::string kind, std::string key) - : key_(std::move(key)), kind_(std::move(kind)), builder_(builder) { + : builder_(builder), kind_(std::move(kind)), key_(std::move(key)) { } /** @@ -45,9 +45,9 @@ class AttributesBuilder final { AttributesBuilder(BuilderReturn& builder, std::string kind, Attributes const& attributes) - : key_(attributes.Key()), + : builder_(builder), kind_(std::move(kind)), - builder_(builder), + key_(attributes.Key()), name_(attributes.Name()), anonymous_(attributes.Anonymous()), private_attributes_(attributes.PrivateAttributes()) { diff --git a/libs/common/src/attributes_builder.cpp b/libs/common/src/attributes_builder.cpp index 3b74298e1..adb5aef73 100644 --- a/libs/common/src/attributes_builder.cpp +++ b/libs/common/src/attributes_builder.cpp @@ -3,6 +3,7 @@ #include #include +#include namespace launchdarkly { template <> From fa48b49a707de4be9673c84001d53c8dab5b89e4 Mon Sep 17 00:00:00 2001 From: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Tue, 29 Apr 2025 14:36:00 +0000 Subject: [PATCH 3/3] docs: update documentation to reflect new behavior for Set method and private attributes Co-Authored-By: mkeeler@launchdarkly.com --- .../launchdarkly/attributes_builder.hpp | 18 +++++++++++------- .../launchdarkly/bindings/c/context_builder.h | 6 +++--- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/libs/common/include/launchdarkly/attributes_builder.hpp b/libs/common/include/launchdarkly/attributes_builder.hpp index 3c7765507..de525a64e 100644 --- a/libs/common/include/launchdarkly/attributes_builder.hpp +++ b/libs/common/include/launchdarkly/attributes_builder.hpp @@ -91,9 +91,11 @@ class AttributesBuilder final { /** * Add or update an attribute in the context. * - * This method cannot be used to set the key, kind, name, anonymous, or - * _meta property of a context. The specific methods on the context builder, - * or attributes builder, should be used. + * This method cannot be used to set the key, kind, or _meta property + * of a context. The anonymous and name properties can be set, but must + * be of the correct types (boolean and string respectively). The specific + * methods on the context builder or attributes builder can also be used + * for these properties. * * @param name The name of the attribute. * @param value The value for the attribute. @@ -104,9 +106,11 @@ class AttributesBuilder final { /** * Add or update a private attribute in the context. * - * This method cannot be used to set the key, kind, name, or anonymous - * property of a context. The specific methods on the context builder, or - * attributes builder, should be used. + * This method cannot be used to set the key, kind, or _meta property + * of a context. The anonymous and name properties can be set, but must + * be of the correct types (boolean and string respectively). The specific + * methods on the context builder or attributes builder can also be used + * for these properties. * * Once you have set an attribute private it will remain in the private * list even if you call `set` afterward. This method is just a convenience @@ -147,7 +151,7 @@ class AttributesBuilder final { * launchdarkly::config::shared::builders::EventsBuilder::PrivateAttribute. * * The attributes "kind" and "key", and the "_meta" attributes cannot be - * made private. + * made private. The "anonymous" attribute can be marked private, but will not be redacted. * * In this example, firstName is marked as private, but lastName is not: * diff --git a/libs/common/include/launchdarkly/bindings/c/context_builder.h b/libs/common/include/launchdarkly/bindings/c/context_builder.h index 9c29dc3e2..f38b5ecb2 100644 --- a/libs/common/include/launchdarkly/bindings/c/context_builder.h +++ b/libs/common/include/launchdarkly/bindings/c/context_builder.h @@ -92,8 +92,8 @@ LDContextBuilder_Attributes_Set(LDContextBuilder builder, * A subsequent call to @ref LDContextBuilder_Attributes_Set, for the same * attribute, will not remove the private status. * - * This method cannot be used to set the `key`, `kind`, `name`, or `anonymous` - * property of a context. + * This method cannot be used to set the `key`, `kind`, or `_meta` + * property of a context. The `anonymous` and `name` properties can be set, but must be of the correct types. * * Adding a @ref LDValue to the builder will consume that value. * @@ -177,7 +177,7 @@ LDContextBuilder_Attributes_SetAnonymous(LDContextBuilder builder, * - @ref LDClientConfigBuilder_Events_PrivateAttribute * * The attributes "kind" and "key", and the "_meta" attributes cannot be - * made private. + * made private. The "anonymous" attribute can be marked private, but will not be redacted. * * @param builder The builder. Must not be NULL. * @param kind The kind to set the attribute as private for. Must not be NULL.