libweave: Validate constraints in FromJson methods
Also fixed public TypedValueBase::SetValue method that allows to create
invalid values.
BUG=brillo:1246
TEST='FEATURES=test emerge-gizmo buffet'
Change-Id: Ia60354c3daf2f150fe3e8d1781d0f37420e03563
Reviewed-on: https://chromium-review.googlesource.com/289641
Trybot-Ready: Vitaly Buka <vitalybuka@chromium.org>
Tested-by: Vitaly Buka <vitalybuka@chromium.org>
Reviewed-by: Alex Vakulenko <avakulenko@chromium.org>
Commit-Queue: Vitaly Buka <vitalybuka@chromium.org>
diff --git a/libweave/src/commands/prop_types.cc b/libweave/src/commands/prop_types.cc
index ffec8f4..a5db4cb 100644
--- a/libweave/src/commands/prop_types.cc
+++ b/libweave/src/commands/prop_types.cc
@@ -187,8 +187,7 @@
const base::Value* defval = nullptr; // Owned by value
if (value->GetWithoutPathExpansion(commands::attributes::kDefault, &defval)) {
std::unique_ptr<PropValue> prop_value = CreateValue();
- if (!prop_value->FromJson(defval, error) ||
- !ValidateValue(prop_value->GetValueAsAny(), error)) {
+ if (!prop_value->FromJson(defval, error)) {
chromeos::Error::AddToPrintf(error, FROM_HERE, errors::commands::kDomain,
errors::commands::kInvalidPropValue,
"Invalid value for property '%s'",
@@ -237,7 +236,7 @@
chromeos::ErrorPtr* error) const {
std::unique_ptr<PropValue> val = CreateValue();
CHECK(val) << "Failed to create value object";
- return val->FromJson(value, error) && ValidateConstraints(*val, error);
+ return val->FromJson(value, error);
}
bool PropType::ValidateValue(const chromeos::Any& value,
diff --git a/libweave/src/commands/prop_types.h b/libweave/src/commands/prop_types.h
index eb605a7..6b364f3 100644
--- a/libweave/src/commands/prop_types.h
+++ b/libweave/src/commands/prop_types.h
@@ -225,16 +225,14 @@
std::unique_ptr<PropValue> CreateValue(
const chromeos::Any& v,
chromeos::ErrorPtr* error) const override {
- std::unique_ptr<PropValue> prop_value;
- if (v.IsTypeCompatible<T>()) {
- std::unique_ptr<Value> value{new Value{Clone()}};
- value->SetValue(v.Get<T>());
- if (ValidateConstraints(*value, error))
- prop_value = std::move(value);
- } else {
+ if (!v.IsTypeCompatible<T>()) {
GenerateErrorValueTypeMismatch(error);
+ return nullptr;
}
- return prop_value;
+ std::unique_ptr<Value> value{new Value{Clone()}};
+ if (!value->SetValue(v.Get<T>(), error))
+ return nullptr;
+ return std::move(value);
}
chromeos::Any ConvertArrayToDBusVariant(
diff --git a/libweave/src/commands/prop_values.h b/libweave/src/commands/prop_values.h
index d0b2dc7..fa9963a 100644
--- a/libweave/src/commands/prop_values.h
+++ b/libweave/src/commands/prop_values.h
@@ -156,7 +156,10 @@
}
bool FromJson(const base::Value* value, chromeos::ErrorPtr* error) override {
- return TypedValueFromJson(value, GetPropType(), &value_, error);
+ T tmp_value;
+ if (!TypedValueFromJson(value, GetPropType(), &tmp_value, error))
+ return false;
+ return SetValue(tmp_value, error);
}
bool IsEqual(const PropValue* value) const override {
@@ -169,7 +172,13 @@
// Helper methods to get and set the C++ representation of the value.
chromeos::Any GetValueAsAny() const override { return value_; }
const T& GetValue() const { return value_; }
- void SetValue(T value) { value_ = std::move(value); }
+ bool SetValue(T value, chromeos::ErrorPtr* error) {
+ std::swap(value_, value); // Backup.
+ if (GetPropType()->ValidateConstraints(*this, error))
+ return true;
+ std::swap(value_, value); // Restore.
+ return false;
+ }
protected:
T value_{}; // The value of the parameter in C++ data representation.
diff --git a/libweave/src/commands/schema_utils.cc b/libweave/src/commands/schema_utils.cc
index 04b8bcb..034eca9 100644
--- a/libweave/src/commands/schema_utils.cc
+++ b/libweave/src/commands/schema_utils.cc
@@ -147,8 +147,7 @@
const base::Value* param_value = nullptr;
CHECK(dict->GetWithoutPathExpansion(pair.first, ¶m_value))
<< "Unable to get parameter";
- if (!value->FromJson(param_value, error) ||
- !pair.second->ValidateValue(value->GetValueAsAny(), error)) {
+ if (!value->FromJson(param_value, error)) {
chromeos::Error::AddToPrintf(
error, FROM_HERE, errors::commands::kDomain,
errors::commands::kInvalidPropValue,
@@ -215,8 +214,7 @@
value_out->reserve(list->GetSize());
for (const base::Value* item : *list) {
std::unique_ptr<PropValue> prop_value = item_type->CreateValue();
- if (!prop_value->FromJson(item, error) ||
- !item_type->ValidateValue(prop_value->GetValueAsAny(), error)) {
+ if (!prop_value->FromJson(item, error)) {
return false;
}
value_out->push_back(std::move(prop_value));
diff --git a/libweave/src/commands/schema_utils.h b/libweave/src/commands/schema_utils.h
index ea1639f..f00a89f 100644
--- a/libweave/src/commands/schema_utils.h
+++ b/libweave/src/commands/schema_utils.h
@@ -75,6 +75,9 @@
// Similarly to TypedValueToJson() function above, the following overloaded
// helper methods allow to extract specific C++ data types from base::Value.
// Also used in template classes below to simplify specialization logic.
+// TODO(vitalybuka): Fix this. Interface is misleading. Seeing PropType internal
+// type validation is expected. In reality only ValueMap and ValueVector do
+// validation.
bool TypedValueFromJson(const base::Value* value_in,
const PropType* type,
bool* value_out,
diff --git a/libweave/src/commands/unittest_utils.h b/libweave/src/commands/unittest_utils.h
index 345e92d..d0d39ee 100644
--- a/libweave/src/commands/unittest_utils.h
+++ b/libweave/src/commands/unittest_utils.h
@@ -34,7 +34,8 @@
std::unique_ptr<const PropVal> make_prop_value(const T& value) {
std::unique_ptr<PropVal> result{
new PropVal{PropType::Create(GetValueType<T>())}};
- result->SetValue(value);
+ if (!result->SetValue(value, nullptr))
+ return nullptr;
return std::move(result);
}
