fix: handle setting protected/type-enforced attributes correctly (#433)

This addresses a bug where you couldn't set the `name` attribute as
private using the `AttributesBuilder::SetPrivate(key, value)` API.

The issue was that this API forwards to an internal
`AttributesBuilder::Set(key, value, is_private)` method, which
disallowed setting of "protected" attributes.

In this PR, I've re-designated attributes as:
1. "protected": they cannot be set using the Set APIs at all
    -  This includes `_meta`, `kind`, and `key`. 
3. "typed": they can be set using Set, but must have the correct type
    - This includes `anonymous` and `name`
5. All other attributes, which don't have any type checking

Within (2), the current behavior is that both of these can be marked
private, which isn't really correct. In reality only `name` can be
redacted.

So currently, you could say `SetPrivate("anonymous, true)`, and it'd set
it and add it to the private attributes list. Come redaction time, it
wouldn't be redacted.

To make this totally consistent, we'd need to disallow
`SetPrivate("anonymous", ..)` as well as modify the
`AddPrivateAttribute` and `AddPrivateAttributes` APIs.

Perhaps the behavior in this PR is sufficient, given some documentation
that mentions that setting `anonymous` private isn't meaningful.

Author: cwaldren-ld

libs/common/include/launchdarkly/attributes_builder.hpp

@@ -1,13 +1,13 @@
 #pragma once
 
-#include <string>
-
 #include <launchdarkly/attribute_reference.hpp>
 #include <launchdarkly/attributes.hpp>
 #include <launchdarkly/value.hpp>
 
-namespace launchdarkly {
+#include <map>
+#include <string>
 
+namespace launchdarkly {
 class ContextBuilder;
 
 /**
@@ -22,7 +22,7 @@ template <class BuilderReturn, class BuildType>
 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) {}
+        : builder_(builder), kind_(std::move(kind)), key_(std::move(key)) {
+    }
 
     /**
      * Crate an attributes builder with the specified kind, and pre-populated
@@ -44,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()) {
@@ -90,36 +91,36 @@ 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.
-     * @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.
      *
-     * 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
      * which also adds the attribute to the `PrivateAttributes`.
      *
      * @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:
@@ -150,7 +151,7 @@ class AttributesBuilder final {
      * launchdarkly::config::shared::builders::EventsBuilder<SDK>::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:
      *
@@ -216,7 +217,7 @@ class AttributesBuilder final {
      */
     [[nodiscard]] BuildType Build() const { return builder_.Build(); }
 
-   private:
+private:
     BuilderReturn& builder_;
 
     /**
@@ -229,15 +230,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<std::string, launchdarkly::Value> values_;
+    std::map<std::string, Value> values_;
     AttributeReference::SetType private_attributes_;
 };
-}  // namespace launchdarkly
+} // namespace launchdarkly

libs/common/include/launchdarkly/bindings/c/context_builder.h

@@ -94,8 +94,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.
  *
@@ -179,7 +179,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.

libs/common/src/attributes_builder.cpp

@@ -1,8 +1,11 @@
 #include <launchdarkly/attributes_builder.hpp>
 #include <launchdarkly/context_builder.hpp>
 
-namespace launchdarkly {
+#include <set>
+#include <unordered_map>
+#include <functional>
 
+namespace launchdarkly {
 template <>
 AttributesBuilder<ContextBuilder, Context>&
 AttributesBuilder<ContextBuilder, Context>::Name(std::string name) {
@@ -17,33 +20,104 @@ AttributesBuilder<ContextBuilder, Context>::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<std::string> const& ProtectedAttributes() {
+    static std::set<std::string> 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<void(AttributesBuilder<ContextBuilder, Context>& builder,
+                       Value)> setter;
+
+    TypedAttribute(enum Value::Type type,
+                   std::function<void(
+                       AttributesBuilder<ContextBuilder, Context>& 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<std::string, TypedAttribute> const&
+TypedAttributes() {
+    static std::unordered_map<std::string, TypedAttribute> 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<ContextBuilder, Context>&
 AttributesBuilder<ContextBuilder, Context>::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<ContextBuilder, Context>&
-AttributesBuilder<ContextBuilder, Context>::Set(std::string name, Value value) {
+AttributesBuilder<ContextBuilder, Context>::Set(
+    std::string name,
+    Value value) {
     return Set(std::move(name), std::move(value), false);
 }
 
 template <>
 AttributesBuilder<ContextBuilder, Context>&
 AttributesBuilder<ContextBuilder, Context>::SetPrivate(std::string name,
-                                                       Value value) {
+    Value value) {
     return Set(std::move(name), std::move(value), true);
 }
 
@@ -56,8 +130,8 @@ AttributesBuilder<ContextBuilder, Context>::AddPrivateAttribute(
 }
 
 template <>
-Attributes AttributesBuilder<ContextBuilder, Context>::BuildAttributes() const {
+Attributes AttributesBuilder<
+    ContextBuilder, Context>::BuildAttributes() const {
     return {key_, name_, anonymous_, values_, private_attributes_};
 }
-
-}  // namespace launchdarkly
+} // namespace launchdarkly