diff options
20 files changed, 174 insertions, 150 deletions
diff --git a/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc b/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc index 2991d79..fd70ba7 100644 --- a/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc +++ b/chrome/browser/extensions/api/permissions/permissions_api_helpers.cc @@ -71,6 +71,7 @@ scoped_refptr<PermissionSet> UnpackPermissionSet( const Permissions& permissions, bool allow_file_access, std::string* error) { + DCHECK(error); APIPermissionSet apis; std::vector<std::string>* permissions_list = permissions.permissions.get(); if (permissions_list) { @@ -107,7 +108,7 @@ scoped_refptr<PermissionSet> UnpackPermissionSet( } CHECK(permission); - if (!permission->FromValue(permission_json.get())) { + if (!permission->FromValue(permission_json.get(), NULL)) { *error = ErrorUtils::FormatErrorMessage(kInvalidParameter, *it); return NULL; } diff --git a/chrome/browser/extensions/extension_prefs_unittest.cc b/chrome/browser/extensions/extension_prefs_unittest.cc index a7ea6cf..7d81c9f 100644 --- a/chrome/browser/extensions/extension_prefs_unittest.cc +++ b/chrome/browser/extensions/extension_prefs_unittest.cc @@ -200,8 +200,7 @@ class ExtensionPrefsGrantedPermissions : public ExtensionPrefsTest { value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) - NOTREACHED(); + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } api_perm_set1_.insert(permission.release()); diff --git a/chrome/common/extensions/permissions/bluetooth_permission.cc b/chrome/common/extensions/permissions/bluetooth_permission.cc index 4d82868..15289ce 100644 --- a/chrome/common/extensions/permissions/bluetooth_permission.cc +++ b/chrome/common/extensions/permissions/bluetooth_permission.cc @@ -25,7 +25,8 @@ BluetoothPermission::BluetoothPermission(const APIPermissionInfo* info) BluetoothPermission::~BluetoothPermission() { } -bool BluetoothPermission::FromValue(const base::Value* value) { +bool BluetoothPermission::FromValue(const base::Value* value, + std::string* error) { // Value may be omitted to gain access to non-profile functions. if (!value) return true; @@ -36,7 +37,7 @@ bool BluetoothPermission::FromValue(const base::Value* value) { return true; if (!SetDisjunctionPermission<BluetoothPermissionData, - BluetoothPermission>::FromValue(value)) { + BluetoothPermission>::FromValue(value, error)) { return false; } diff --git a/chrome/common/extensions/permissions/bluetooth_permission.h b/chrome/common/extensions/permissions/bluetooth_permission.h index 5eea337..8d0c009 100644 --- a/chrome/common/extensions/permissions/bluetooth_permission.h +++ b/chrome/common/extensions/permissions/bluetooth_permission.h @@ -32,7 +32,8 @@ class BluetoothPermission // SetDisjunctionPermission overrides. // BluetoothPermission permits an empty list for gaining permission to the // Bluetooth APIs without implementing a profile. - virtual bool FromValue(const base::Value* value) OVERRIDE; + virtual bool FromValue(const base::Value* value, + std::string* error) OVERRIDE; // APIPermission overrides virtual PermissionMessages GetMessages() const OVERRIDE; diff --git a/chrome/common/extensions/permissions/media_galleries_permission.cc b/chrome/common/extensions/permissions/media_galleries_permission.cc index b95777b..f6d1689 100644 --- a/chrome/common/extensions/permissions/media_galleries_permission.cc +++ b/chrome/common/extensions/permissions/media_galleries_permission.cc @@ -17,11 +17,22 @@ namespace { // copyTo permission requires delete permission as a prerequisite. // delete permission requires read permission as a prerequisite. -bool IsValidPermissionSet(bool has_read, bool has_copy_to, bool has_delete) { - if (has_copy_to) - return has_read && has_delete; - if (has_delete) - return has_read; +bool IsValidPermissionSet(bool has_read, bool has_copy_to, bool has_delete, + std::string* error) { + if (has_copy_to) { + if (has_read && has_delete) + return true; + if (error) + *error = "copyTo permission requires read and delete permissions"; + return false; + } + if (has_delete) { + if (has_read) + return true; + if (error) + *error = "delete permission requires read permission"; + return false; + } return true; } @@ -44,9 +55,11 @@ MediaGalleriesPermission::MediaGalleriesPermission( MediaGalleriesPermission::~MediaGalleriesPermission() { } -bool MediaGalleriesPermission::FromValue(const base::Value* value) { +bool MediaGalleriesPermission::FromValue(const base::Value* value, + std::string* error) { if (!SetDisjunctionPermission<MediaGalleriesPermissionData, - MediaGalleriesPermission>::FromValue(value)) { + MediaGalleriesPermission>::FromValue(value, + error)) { return false; } @@ -78,7 +91,7 @@ bool MediaGalleriesPermission::FromValue(const base::Value* value) { return false; } - return IsValidPermissionSet(has_read, has_copy_to, has_delete); + return IsValidPermissionSet(has_read, has_copy_to, has_delete, error); } PermissionMessages MediaGalleriesPermission::GetMessages() const { @@ -102,7 +115,7 @@ PermissionMessages MediaGalleriesPermission::GetMessages() const { has_delete = true; } - if (!IsValidPermissionSet(has_read, has_copy_to, has_delete)) { + if (!IsValidPermissionSet(has_read, has_copy_to, has_delete, NULL)) { NOTREACHED(); return result; } diff --git a/chrome/common/extensions/permissions/media_galleries_permission.h b/chrome/common/extensions/permissions/media_galleries_permission.h index e385a24..8fd1a5d 100644 --- a/chrome/common/extensions/permissions/media_galleries_permission.h +++ b/chrome/common/extensions/permissions/media_galleries_permission.h @@ -38,7 +38,7 @@ class MediaGalleriesPermission // SetDisjunctionPermission overrides. // MediaGalleriesPermission does additional checks to make sure the // permissions do not contain unknown values. - virtual bool FromValue(const base::Value* value) OVERRIDE; + virtual bool FromValue(const base::Value* value, std::string* error) OVERRIDE; // APIPermission overrides. virtual PermissionMessages GetMessages() const OVERRIDE; diff --git a/chrome/common/extensions/permissions/media_galleries_permission_unittest.cc b/chrome/common/extensions/permissions/media_galleries_permission_unittest.cc index ddcfc92..1006d3c 100644 --- a/chrome/common/extensions/permissions/media_galleries_permission_unittest.cc +++ b/chrome/common/extensions/permissions/media_galleries_permission_unittest.cc @@ -19,6 +19,7 @@ namespace extensions { namespace { TEST(MediaGalleriesPermissionTest, GoodValues) { + std::string error; const APIPermissionInfo* permission_info = PermissionsInfo::GetInstance()->GetByID(APIPermission::kMediaGalleries); @@ -29,41 +30,55 @@ TEST(MediaGalleriesPermissionTest, GoodValues) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); // all_detected value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); // access_type value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); // Repeats do not make a difference. value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); @@ -72,10 +87,13 @@ TEST(MediaGalleriesPermissionTest, GoodValues) { value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - EXPECT_TRUE(permission->FromValue(value.get())); + EXPECT_TRUE(permission->FromValue(value.get(), &error)); + EXPECT_TRUE(error.empty()); + error.clear(); } TEST(MediaGalleriesPermissionTest, BadValues) { + std::string error; const APIPermissionInfo* permission_info = PermissionsInfo::GetInstance()->GetByID(APIPermission::kMediaGalleries); @@ -83,35 +101,47 @@ TEST(MediaGalleriesPermissionTest, BadValues) { // Empty scoped_ptr<base::ListValue> value(new base::ListValue()); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); // copyTo and delete without read value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kCopyToPermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kDeletePermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); // copyTo without delete value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); // Repeats do not make a difference. value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); @@ -119,7 +149,9 @@ TEST(MediaGalleriesPermissionTest, BadValues) { value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - EXPECT_FALSE(permission->FromValue(value.get())); + EXPECT_FALSE(permission->FromValue(value.get(), &error)); + EXPECT_FALSE(error.empty()); + error.clear(); } TEST(MediaGalleriesPermissionTest, Equal) { @@ -134,43 +166,43 @@ TEST(MediaGalleriesPermissionTest, Equal) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); - ASSERT_TRUE(permission2->FromValue(value.get())); + ASSERT_TRUE(permission2->FromValue(value.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); - ASSERT_TRUE(permission2->FromValue(value.get())); + ASSERT_TRUE(permission2->FromValue(value.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - ASSERT_TRUE(permission2->FromValue(value.get())); + ASSERT_TRUE(permission2->FromValue(value.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - ASSERT_TRUE(permission2->FromValue(value.get())); + ASSERT_TRUE(permission2->FromValue(value.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); } @@ -186,14 +218,14 @@ TEST(MediaGalleriesPermissionTest, NotEqual) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); - ASSERT_TRUE(permission2->FromValue(value.get())); + ASSERT_TRUE(permission2->FromValue(value.get(), NULL)); EXPECT_FALSE(permission1->Equal(permission2.get())); } @@ -209,32 +241,32 @@ TEST(MediaGalleriesPermissionTest, ToFromValue) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kAllAutoDetectedPermission); value->AppendString(MediaGalleriesPermission::kReadPermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); scoped_ptr<base::Value> vtmp(permission1->ToValue()); ASSERT_TRUE(vtmp); - ASSERT_TRUE(permission2->FromValue(vtmp.get())); + ASSERT_TRUE(permission2->FromValue(vtmp.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); value->AppendString(MediaGalleriesPermission::kCopyToPermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); vtmp = permission1->ToValue(); ASSERT_TRUE(vtmp); - ASSERT_TRUE(permission2->FromValue(vtmp.get())); + ASSERT_TRUE(permission2->FromValue(vtmp.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); value.reset(new base::ListValue()); value->AppendString(MediaGalleriesPermission::kReadPermission); value->AppendString(MediaGalleriesPermission::kDeletePermission); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); vtmp = permission1->ToValue(); ASSERT_TRUE(vtmp); - ASSERT_TRUE(permission2->FromValue(vtmp.get())); + ASSERT_TRUE(permission2->FromValue(vtmp.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); } diff --git a/chrome/common/extensions/permissions/permission_set_unittest.cc b/chrome/common/extensions/permissions/permission_set_unittest.cc index 73d02ca..1156ce4 100644 --- a/chrome/common/extensions/permissions/permission_set_unittest.cc +++ b/chrome/common/extensions/permissions/permission_set_unittest.cc @@ -255,9 +255,7 @@ TEST(PermissionsTest, CreateUnion) { base::Value::CreateStringValue("tcp-connect:*.example.com:80")); value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } // Union with an empty set. @@ -302,9 +300,7 @@ TEST(PermissionsTest, CreateUnion) { value->Append( base::Value::CreateStringValue("tcp-connect:*.example.com:80")); value->Append(base::Value::CreateStringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -321,9 +317,7 @@ TEST(PermissionsTest, CreateUnion) { value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); value->Append(base::Value::CreateStringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } // Insert a new permission socket permisssion which will replace the old one. expected_apis.insert(permission); @@ -390,9 +384,7 @@ TEST(PermissionsTest, CreateIntersection) { base::Value::CreateStringValue("tcp-connect:*.example.com:80")); value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis1.insert(permission); @@ -429,9 +421,7 @@ TEST(PermissionsTest, CreateIntersection) { value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); value->Append(base::Value::CreateStringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -441,9 +431,7 @@ TEST(PermissionsTest, CreateIntersection) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } expected_apis.insert(permission); @@ -509,9 +497,7 @@ TEST(PermissionsTest, CreateDifference) { base::Value::CreateStringValue("tcp-connect:*.example.com:80")); value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis1.insert(permission); @@ -536,9 +522,7 @@ TEST(PermissionsTest, CreateDifference) { value->Append( base::Value::CreateStringValue("tcp-connect:*.example.com:80")); value->Append(base::Value::CreateStringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -548,9 +532,7 @@ TEST(PermissionsTest, CreateDifference) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(base::Value::CreateStringValue("udp-bind::8080")); value->Append(base::Value::CreateStringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } expected_apis.insert(permission); diff --git a/chrome/common/extensions/permissions/set_disjunction_permission.h b/chrome/common/extensions/permissions/set_disjunction_permission.h index 97388ba..f018e13 100644 --- a/chrome/common/extensions/permissions/set_disjunction_permission.h +++ b/chrome/common/extensions/permissions/set_disjunction_permission.h @@ -110,24 +110,29 @@ class SetDisjunctionPermission : public APIPermission { return result->data_set_.empty() ? NULL : result.release(); } - virtual bool FromValue(const base::Value* value) OVERRIDE { + virtual bool FromValue(const base::Value* value, + std::string* error) OVERRIDE { data_set_.clear(); const base::ListValue* list = NULL; - if (!value) - return false; - - if (!value->GetAsList(&list) || list->GetSize() == 0) + if (!value || !value->GetAsList(&list) || list->GetSize() == 0) { + if (error) + *error = "NULL or empty permission list"; return false; + } for (size_t i = 0; i < list->GetSize(); ++i) { - const base::Value* item_value; - if (!list->Get(i, &item_value)) - return false; + const base::Value* item_value = NULL; + bool got_item = list->Get(i, &item_value); + DCHECK(got_item); + DCHECK(item_value); PermissionDataType data; - if (!data.FromValue(item_value)) + if (!data.FromValue(item_value)) { + if (error) + *error = "Cannot parse an item from the permission list"; return false; + } data_set_.insert(data); } diff --git a/chrome/common/extensions/permissions/settings_override_permission.cc b/chrome/common/extensions/permissions/settings_override_permission.cc index f5bf946..934bf98 100644 --- a/chrome/common/extensions/permissions/settings_override_permission.cc +++ b/chrome/common/extensions/permissions/settings_override_permission.cc @@ -8,7 +8,6 @@ #include "grit/generated_resources.h" #include "ui/base/l10n/l10n_util.h" - namespace extensions { SettingsOverrideAPIPermission::SettingsOverrideAPIPermission( @@ -53,25 +52,23 @@ PermissionMessages SettingsOverrideAPIPermission::GetMessages() const { bool SettingsOverrideAPIPermission::Check( const APIPermission::CheckParam* param) const { - return !param; + return (param == NULL); } bool SettingsOverrideAPIPermission::Contains(const APIPermission* rhs) const { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return true; } bool SettingsOverrideAPIPermission::Equal(const APIPermission* rhs) const { - if (this == rhs) - return true; - CHECK(info() == rhs->info()); + if (this != rhs) + CHECK_EQ(info(), rhs->info()); return true; } -bool SettingsOverrideAPIPermission::FromValue(const base::Value* value) { - if (value) - return false; - return true; +bool SettingsOverrideAPIPermission::FromValue(const base::Value* value, + std::string* /*error*/) { + return (value == NULL); } scoped_ptr<base::Value> SettingsOverrideAPIPermission::ToValue() const { @@ -84,19 +81,19 @@ APIPermission* SettingsOverrideAPIPermission::Clone() const { APIPermission* SettingsOverrideAPIPermission::Diff( const APIPermission* rhs) const { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return NULL; } APIPermission* SettingsOverrideAPIPermission::Union( const APIPermission* rhs) const { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return new SettingsOverrideAPIPermission(info(), setting_value_); } APIPermission* SettingsOverrideAPIPermission::Intersect( const APIPermission* rhs) const { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return new SettingsOverrideAPIPermission(info(), setting_value_); } diff --git a/chrome/common/extensions/permissions/settings_override_permission.h b/chrome/common/extensions/permissions/settings_override_permission.h index 8f8a4e3..d123f83 100644 --- a/chrome/common/extensions/permissions/settings_override_permission.h +++ b/chrome/common/extensions/permissions/settings_override_permission.h @@ -25,7 +25,7 @@ class SettingsOverrideAPIPermission : public APIPermission { virtual bool Check(const APIPermission::CheckParam* param) const OVERRIDE; virtual bool Contains(const APIPermission* rhs) const OVERRIDE; virtual bool Equal(const APIPermission* rhs) const OVERRIDE; - virtual bool FromValue(const base::Value* value) OVERRIDE; + virtual bool FromValue(const base::Value* value, std::string* error) OVERRIDE; virtual scoped_ptr<base::Value> ToValue() const OVERRIDE; virtual APIPermission* Clone() const OVERRIDE; virtual APIPermission* Diff(const APIPermission* rhs) const OVERRIDE; diff --git a/chrome/common/extensions/permissions/socket_permission_unittest.cc b/chrome/common/extensions/permissions/socket_permission_unittest.cc index 78bd818..a968c46 100644 --- a/chrome/common/extensions/permissions/socket_permission_unittest.cc +++ b/chrome/common/extensions/permissions/socket_permission_unittest.cc @@ -293,7 +293,7 @@ TEST(SocketPermissionTest, IPC) { value->AppendString("tcp-connect:*.example.com:80"); value->AppendString("udp-bind::8080"); value->AppendString("udp-send-to::8888"); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); EXPECT_FALSE(permission1->Equal(permission2.get())); @@ -317,13 +317,13 @@ TEST(SocketPermissionTest, Value) { value->AppendString("tcp-connect:*.example.com:80"); value->AppendString("udp-bind::8080"); value->AppendString("udp-send-to::8888"); - ASSERT_TRUE(permission1->FromValue(value.get())); + ASSERT_TRUE(permission1->FromValue(value.get(), NULL)); EXPECT_FALSE(permission1->Equal(permission2.get())); scoped_ptr<base::Value> vtmp(permission1->ToValue()); ASSERT_TRUE(vtmp); - ASSERT_TRUE(permission2->FromValue(vtmp.get())); + ASSERT_TRUE(permission2->FromValue(vtmp.get(), NULL)); EXPECT_TRUE(permission1->Equal(permission2.get())); } diff --git a/chrome/common/extensions/permissions/usb_device_permission_unittest.cc b/chrome/common/extensions/permissions/usb_device_permission_unittest.cc index bb3ea70e..2aa47f1 100644 --- a/chrome/common/extensions/permissions/usb_device_permission_unittest.cc +++ b/chrome/common/extensions/permissions/usb_device_permission_unittest.cc @@ -51,7 +51,7 @@ TEST(USBDevicePermissionTest, MAYBE_PermissionMessage) { UsbDevicePermission permission( PermissionsInfo::GetInstance()->GetByID(APIPermission::kUsbDevice)); - ASSERT_TRUE(permission.FromValue(permission_list.get())); + ASSERT_TRUE(permission.FromValue(permission_list.get(), NULL)); PermissionMessages messages = permission.GetMessages(); ASSERT_EQ(3U, messages.size()); diff --git a/extensions/common/manifest_constants.cc b/extensions/common/manifest_constants.cc index bfbcc1c..454f5ec 100644 --- a/extensions/common/manifest_constants.cc +++ b/extensions/common/manifest_constants.cc @@ -522,6 +522,8 @@ const char kInvalidPageActionsListSize[] = "Invalid value for 'page_actions'. There can be at most one page action."; const char kInvalidPageActionTypeValue[] = "Invalid value for 'page_actions[*].type', expected 'tab' or 'permanent'."; +const char kInvalidPermissionWithDetail[] = + "Invalid value for 'permissions[*]': *."; const char kInvalidPermission[] = "Invalid value for 'permissions[*]'."; const char kInvalidPermissions[] = diff --git a/extensions/common/manifest_constants.h b/extensions/common/manifest_constants.h index ab207ea..a62583f 100644 --- a/extensions/common/manifest_constants.h +++ b/extensions/common/manifest_constants.h @@ -222,7 +222,7 @@ extern const char kRunAtDocumentEnd[]; extern const char kRunAtDocumentIdle[]; extern const char kRunAtDocumentStart[]; -} // manifest_values +} // namespace manifest_values // Error messages returned from extension installation. namespace manifest_errors { @@ -375,6 +375,7 @@ extern const char kInvalidPageActionPopupPath[]; extern const char kInvalidPageActionsList[]; extern const char kInvalidPageActionsListSize[]; extern const char kInvalidPageActionTypeValue[]; +extern const char kInvalidPermissionWithDetail[]; extern const char kInvalidPermission[]; extern const char kInvalidPermissions[]; extern const char kInvalidPermissionScheme[]; diff --git a/extensions/common/permissions/api_permission.cc b/extensions/common/permissions/api_permission.cc index f183ed2..95728fa 100644 --- a/extensions/common/permissions/api_permission.cc +++ b/extensions/common/permissions/api_permission.cc @@ -37,21 +37,19 @@ class SimpleAPIPermission : public APIPermission { } virtual bool Contains(const APIPermission* rhs) const OVERRIDE { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return true; } virtual bool Equal(const APIPermission* rhs) const OVERRIDE { - if (this == rhs) - return true; - CHECK(info() == rhs->info()); + if (this != rhs) + CHECK_EQ(info(), rhs->info()); return true; } - virtual bool FromValue(const base::Value* value) OVERRIDE { - if (value) - return false; - return true; + virtual bool FromValue(const base::Value* value, + std::string* /*error*/) OVERRIDE { + return (value == NULL); } virtual scoped_ptr<base::Value> ToValue() const OVERRIDE { @@ -63,17 +61,17 @@ class SimpleAPIPermission : public APIPermission { } virtual APIPermission* Diff(const APIPermission* rhs) const OVERRIDE { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return NULL; } virtual APIPermission* Union(const APIPermission* rhs) const OVERRIDE { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return new SimpleAPIPermission(info()); } virtual APIPermission* Intersect(const APIPermission* rhs) const OVERRIDE { - CHECK(info() == rhs->info()); + CHECK_EQ(info(), rhs->info()); return new SimpleAPIPermission(info()); } diff --git a/extensions/common/permissions/api_permission.h b/extensions/common/permissions/api_permission.h index 9fab5e4..b8f23f8 100644 --- a/extensions/common/permissions/api_permission.h +++ b/extensions/common/permissions/api_permission.h @@ -204,8 +204,9 @@ class APIPermission { // Returns true if |rhs| is equal to this. virtual bool Equal(const APIPermission* rhs) const = 0; - // Parses the APIPermission from |value|. Returns false if error happens. - virtual bool FromValue(const base::Value* value) = 0; + // Parses the APIPermission from |value|. Returns false if an error happens + // and optionally set |error| if |error| is not NULL. + virtual bool FromValue(const base::Value* value, std::string* error) = 0; // Stores this into a new created |value|. virtual scoped_ptr<base::Value> ToValue() const = 0; diff --git a/extensions/common/permissions/api_permission_set.cc b/extensions/common/permissions/api_permission_set.cc index 510e7fc..fbbfdc0 100644 --- a/extensions/common/permissions/api_permission_set.cc +++ b/extensions/common/permissions/api_permission_set.cc @@ -41,10 +41,19 @@ bool CreateAPIPermission( return false; } - if (!permission->FromValue(permission_value)) { + std::string error_details; + if (!permission->FromValue(permission_value, &error_details)) { if (error) { - *error = ErrorUtils::FormatErrorMessageUTF16( - errors::kInvalidPermission, permission_info->name()); + if (error_details.empty()) { + *error = ErrorUtils::FormatErrorMessageUTF16( + errors::kInvalidPermission, + permission_info->name()); + } else { + *error = ErrorUtils::FormatErrorMessageUTF16( + errors::kInvalidPermissionWithDetail, + permission_info->name(), + error_details); + } return false; } LOG(WARNING) << "Parse permission failed."; diff --git a/extensions/common/permissions/api_permission_set_unittest.cc b/extensions/common/permissions/api_permission_set_unittest.cc index 85f8ccc..abeb9ab 100644 --- a/extensions/common/permissions/api_permission_set_unittest.cc +++ b/extensions/common/permissions/api_permission_set_unittest.cc @@ -48,9 +48,7 @@ TEST(APIPermissionSetTest, CreateUnion) { value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } // Union with an empty set. @@ -83,9 +81,7 @@ TEST(APIPermissionSetTest, CreateUnion) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -101,9 +97,7 @@ TEST(APIPermissionSetTest, CreateUnion) { value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); value->Append(new base::StringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } // Insert a new socket permission which will replace the old one. expected_apis.insert(permission); @@ -140,9 +134,7 @@ TEST(APIPermissionSetTest, CreateIntersection) { value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis1.insert(permission); @@ -168,9 +160,7 @@ TEST(APIPermissionSetTest, CreateIntersection) { value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); value->Append(new base::StringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -180,9 +170,7 @@ TEST(APIPermissionSetTest, CreateIntersection) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } expected_apis.insert(permission); @@ -218,9 +206,7 @@ TEST(APIPermissionSetTest, CreateDifference) { value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis1.insert(permission); @@ -238,9 +224,7 @@ TEST(APIPermissionSetTest, CreateDifference) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-send-to::8899")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis2.insert(permission); @@ -250,9 +234,7 @@ TEST(APIPermissionSetTest, CreateDifference) { scoped_ptr<base::ListValue> value(new base::ListValue()); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } expected_apis.insert(permission); @@ -286,9 +268,7 @@ TEST(APIPermissionSetTest, IPC) { value->Append(new base::StringValue("tcp-connect:*.example.com:80")); value->Append(new base::StringValue("udp-bind::8080")); value->Append(new base::StringValue("udp-send-to::8888")); - if (!permission->FromValue(value.get())) { - NOTREACHED(); - } + ASSERT_TRUE(permission->FromValue(value.get(), NULL)); } apis.insert(permission); diff --git a/extensions/common/permissions/permissions_data_unittest.cc b/extensions/common/permissions/permissions_data_unittest.cc index 6fed67c..fda57f9 100644 --- a/extensions/common/permissions/permissions_data_unittest.cc +++ b/extensions/common/permissions/permissions_data_unittest.cc @@ -131,9 +131,11 @@ TEST(ExtensionPermissionsTest, SocketPermissions) { Manifest::INTERNAL, Extension::NO_FLAGS, &error); EXPECT_TRUE(extension.get() == NULL); - ASSERT_EQ(ErrorUtils::FormatErrorMessage( - manifest_errors::kInvalidPermission, "socket"), - error); + std::string expected_error_msg_header = ErrorUtils::FormatErrorMessage( + manifest_errors::kInvalidPermissionWithDetail, + "socket", + "NULL or empty permission list"); + EXPECT_EQ(expected_error_msg_header, error); extension = LoadManifest("socket_permissions", "socket2.json"); EXPECT_TRUE(CheckSocketPermission(extension, |