Skip to content

Commit

Permalink
[Telemetry] Introduce runtime permission
Browse files Browse the repository at this point in the history 
Add the new os.telemetry.serial_number optional permission.

Allow chromeos_system_extension to declare optional_permissions
manifest key.

Allow chromeos_system_extension to access chrome.permissions.* API in
order to request runtime permissions.

Bug: b:188897824, b:197866991
Test: added tests in telemetry_api_browsertest.cc
Test: added tests in chromeos_permission_messages_unittests.cc
Test: added tests in
extension_manifests_chromeos_system_extension_unittest.cc
Test: out/Default/browser_tests
--gtest_filter="*TelemetryExtension*ApiBrowserTest*"
Test: out/Default/unit_tests --gtest_filter="*PermissionMessage*"
Test: out/Default/extensions_unittests
--gtest_filter="*ExtensionAPIPermissionTest*"
Test: out/Default/unit_tests
--gtest_filter="*ExtensionManifestChromeOSSystemExtensionTest*"

Change-Id: I1dafefefc883b4b1cc030cbb86fea6c90fbac260
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3168641
Commit-Queue: Mahmoud Gawad <mgawad@google.com>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Reviewed-by: Oleh Lamzin <lamzin@google.com>
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Cr-Commit-Position: refs/heads/main@{#924083}
  • Loading branch information
Mahmoud Gawad authored and Chromium LUCI CQ committed Sep 22, 2021
1 parent 10b28d7 commit b303ca4
Show file tree
Hide file tree
Showing 20 changed files with 285 additions and 16 deletions.
3 changes: 3 additions & 0 deletions chrome/app/generated_resources.grd
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4469,6 +4469,9 @@ are declared in tools/grit/grit_rule.gni.
<message name="IDS_EXTENSION_PROMPT_WARNING_DEVICE_INFO_MAYBE_SHARED_WITH_MANUFACTURER" desc="Common permission string for chrome.os.{telemetry/diagnostics} APIs.">
This information may be shared with your device manufacturer. Data handled by organizations other than Google will follow their separate privacy policies.
</message>
<message name="IDS_EXTENSION_PROMPT_WARNING_CHROMEOS_TELEMETRY_SERIAL_NUMBER" desc="Permission string for serial number permission.">
Read device and component serial numbers.
</message>
</if>

<!-- Extension/App error messages -->
Expand Down
1 change: 1 addition & 0 deletions ...ated_resources_grd/IDS_EXTENSION_PROMPT_WARNING_CHROMEOS_TELEMETRY_SERIAL_NUMBER.png.sha1
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
08babd791c8ab82412c7fbf955028428c6583fe9
1 change: 1 addition & 0 deletions chrome/browser/chromeos/extensions/telemetry/api/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ source_set("browser_tests") {
"//chromeos/services/cros_healthd/public/cpp",
"//chromeos/services/cros_healthd/public/mojom",
"//extensions:test_support",
"//extensions/browser",
]
}

Expand Down
1 change: 1 addition & 0 deletions chrome/browser/chromeos/extensions/telemetry/api/base_telemetry_extension_browser_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ constexpr char kManifest[] = R"(
"service_worker": "sw.js"
},
"permissions": [ "os.diagnostics", "os.telemetry" ],
"optional_permissions": [ "os.telemetry.serial_number" ],
"externally_connectable": {
"ids": [
"*"
Expand Down
21 changes: 17 additions & 4 deletions chrome/browser/chromeos/extensions/telemetry/api/telemetry_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "base/bind.h"
#include "base/values.h"
#include "chrome/common/chromeos/extensions/api/telemetry.h"
#include "extensions/common/permissions/permissions_data.h"

namespace chromeos {

Expand Down Expand Up @@ -51,15 +52,19 @@ void OsTelemetryGetVpdInfoFunction::OnResult(
result.model_name =
std::make_unique<std::string>(vpd_info->model_name.value());
}
if (vpd_info->serial_number.has_value()) {
result.serial_number =
std::make_unique<std::string>(vpd_info->serial_number.value());
}
if (vpd_info->sku_number.has_value()) {
result.sku_number =
std::make_unique<std::string>(vpd_info->sku_number.value());
}

// Protect accessing the serial number by a runtime permission.
if (extension()->permissions_data()->HasAPIPermission(
extensions::mojom::APIPermissionID::kChromeOSTelemetrySerialNumber) &&
vpd_info->serial_number.has_value()) {
result.serial_number =
std::make_unique<std::string>(vpd_info->serial_number.value());
}

Respond(ArgumentList(api::os_telemetry::GetVpdInfo::Results::Create(result)));
}

Expand All @@ -70,6 +75,14 @@ OsTelemetryGetOemDataFunction::~OsTelemetryGetOemDataFunction() = default;

ExtensionFunction::ResponseAction
OsTelemetryGetOemDataFunction::RunIfAllowed() {
// Protect accessing the serial number by a runtime permission.
if (!extension()->permissions_data()->HasAPIPermission(
extensions::mojom::APIPermissionID::kChromeOSTelemetrySerialNumber)) {
return RespondNow(
Error("Unauthorized access to chrome.os.telemetry.getOemData. Extension"
" doesn't have the permission."));
}

auto cb = base::BindOnce(&OsTelemetryGetOemDataFunction::OnResult, this);

remote_probe_service_->GetOemData(std::move(cb));
Expand Down
119 changes: 116 additions & 3 deletions chrome/browser/chromeos/extensions/telemetry/api/telemetry_api_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@
#include "chromeos/dbus/debug_daemon/fake_debug_daemon_client.h"
#include "chromeos/services/cros_healthd/public/mojom/cros_healthd_probe.mojom.h"
#include "content/public/test/browser_test.h"
#include "extensions/browser/extension_dialog_auto_confirm.h"
#include "extensions/browser/extension_function.h"
#include "testing/gtest/include/gtest/gtest.h"

namespace chromeos {
Expand All @@ -34,7 +36,7 @@ IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
}

IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
GetVpdInfoSuccess) {
GetVpdInfoWithoutSerialNumberPermission) {
// Configure fake cros_healthd response.
{
auto telemetry_info = cros_healthd::mojom::TelemetryInfo::New();
Expand Down Expand Up @@ -63,6 +65,65 @@ IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
CreateExtensionAndRunServiceWorker(R"(
chrome.test.runTests([
async function getVpdInfo() {
const result = await chrome.os.telemetry.getVpdInfo();
chrome.test.assertEq("2021-50", result.activateDate);
chrome.test.assertEq("COOL-LAPTOP-CHROME", result.modelName);
chrome.test.assertEq(null, result.serialNumber);
chrome.test.assertEq("sku15", result.skuNumber);
chrome.test.succeed();
}
]);
)");
}

IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
GetVpdInfoWithSerialNumberPermission) {
// Configure fake cros_healthd response.
{
auto telemetry_info = cros_healthd::mojom::TelemetryInfo::New();

{
auto os_version = cros_healthd::mojom::OsVersion::New();

auto system_info = cros_healthd::mojom::SystemInfo::New();
system_info->first_power_date = "2021-50";
system_info->product_model_name = "COOL-LAPTOP-CHROME";
system_info->product_serial_number = "5CD9132880";
system_info->product_sku_number = "sku15";
system_info->os_version = std::move(os_version);

telemetry_info->system_result =
cros_healthd::mojom::SystemResult::NewSystemInfo(
std::move(system_info));
}

ASSERT_TRUE(cros_healthd::FakeCrosHealthdClient::Get());

cros_healthd::FakeCrosHealthdClient::Get()
->SetProbeTelemetryInfoResponseForTesting(telemetry_info);
}

// TODO(crbug.com/977629): Currently, chrome.test.runWithUserGesture()
// doesn't support Service Worker-based extensions, so this is a workaround.
ExtensionFunction::ScopedUserGestureForTests scoped_user_gesture;

// Auto-confirm the permission dialog.
extensions::ScopedTestDialogAutoConfirm auto_confirm(
extensions::ScopedTestDialogAutoConfirm::ACCEPT);

CreateExtensionAndRunServiceWorker(R"(
chrome.test.runTests([
async function getVpdInfo() {
// Request os.telemetry.serial_number permission
await new Promise((resolve) => {
chrome.permissions.request({
permissions: ['os.telemetry.serial_number']
}, (granted) => {
chrome.test.assertNoLastError();
chrome.test.assertTrue(granted);
resolve();
});
});
const result = await chrome.os.telemetry.getVpdInfo();
chrome.test.assertEq("2021-50", result.activateDate);
chrome.test.assertEq("COOL-LAPTOP-CHROME", result.modelName);
Expand Down Expand Up @@ -91,13 +152,31 @@ class TestDebugDaemonClient : public FakeDebugDaemonClient {
} // namespace

IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
GetOemDataError) {
GetOemDataWithSerialNumberPermission_Error) {
DBusThreadManager::GetSetterForTesting()->SetDebugDaemonClient(
std::make_unique<TestDebugDaemonClient>());

// TODO(crbug.com/977629): Currently, chrome.test.runWithUserGesture()
// doesn't support Service Worker-based extensions, so this is a workaround.
ExtensionFunction::ScopedUserGestureForTests scoped_user_gesture;

// Auto-confirm the permission dialog.
extensions::ScopedTestDialogAutoConfirm auto_confirm(
extensions::ScopedTestDialogAutoConfirm::ACCEPT);

CreateExtensionAndRunServiceWorker(R"(
chrome.test.runTests([
async function getOemData() {
// Request os.telemetry.serial_number permission
await new Promise((resolve) => {
chrome.permissions.request({
permissions: ['os.telemetry.serial_number']
}, (granted) => {
chrome.test.assertNoLastError();
chrome.test.assertTrue(granted);
resolve();
});
});
await chrome.test.assertPromiseRejects(
chrome.os.telemetry.getOemData(),
'Error: API internal error'
Expand All @@ -109,10 +188,28 @@ IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
}

IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
GetOemDataSuccess) {
GetOemDataWithSerialNumberPermission_Success) {
// TODO(crbug.com/977629): Currently, chrome.test.runWithUserGesture()
// doesn't support Service Worker-based extensions, so this is a workaround.
ExtensionFunction::ScopedUserGestureForTests scoped_user_gesture;

// Auto-confirm the permission dialog.
extensions::ScopedTestDialogAutoConfirm auto_confirm(
extensions::ScopedTestDialogAutoConfirm::ACCEPT);

CreateExtensionAndRunServiceWorker(R"(
chrome.test.runTests([
async function getOemData() {
// Request os.telemetry.serial_number permission
await new Promise((resolve) => {
chrome.permissions.request({
permissions: ['os.telemetry.serial_number']
}, (granted) => {
chrome.test.assertNoLastError();
chrome.test.assertTrue(granted);
resolve();
});
});
const result = await chrome.os.telemetry.getOemData();
chrome.test.assertEq(
"oemdata: response from GetLog", result.oemData);
Expand All @@ -122,4 +219,20 @@ IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
)");
}

IN_PROC_BROWSER_TEST_F(TelemetryExtensionTelemetryApiBrowserTest,
GetOemDataWithoutSerialNumberPermission) {
CreateExtensionAndRunServiceWorker(R"(
chrome.test.runTests([
async function getOemData() {
await chrome.test.assertPromiseRejects(
chrome.os.telemetry.getOemData(),
'Error: Unauthorized access to chrome.os.telemetry.getOemData. ' +
'Extension doesn\'t have the permission.'
);
chrome.test.succeed();
}
]);
)");
}

} // namespace chromeos
67 changes: 62 additions & 5 deletions chrome/browser/chromeos/extensions/telemetry/chromeos_permission_messages_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "chrome/browser/extensions/permissions_test_util.h"
#include "chrome/browser/extensions/test_extension_environment.h"
#include "chrome/common/extensions/permissions/chrome_permission_message_provider.h"
#include "chrome/test/base/testing_profile.h"
#include "extensions/common/extension.h"
#include "extensions/common/extension_builder.h"
#include "extensions/common/manifest.h"
Expand All @@ -21,7 +22,6 @@
#include "extensions/common/permissions/permissions_info.h"
#include "extensions/common/value_builder.h"
#include "testing/gtest/include/gtest/gtest.h"
#include "ui/base/l10n/l10n_util.h"

namespace chromeos {

Expand All @@ -47,6 +47,10 @@ const std::u16string kTelemetryAndDiagnosticsPermissionMessage =
u"tests. This information may be shared with your device manufacturer. "
u"Data handled by organizations other than Google will follow their "
u"separate privacy policies.";
const std::u16string kTelemetrySerialNumberPermissionMessage =
u"Read device and component serial numbers. This information may be shared "
u"with your device manufacturer. Data handled by organizations other than "
u"Google will follow their separate privacy policies.";

} // namespace

Expand All @@ -66,11 +70,14 @@ class ChromeOSPermissionMessageUnittest : public testing::Test {

protected:
void CreateAndInstallExtensionWithPermissions(
std::unique_ptr<base::ListValue> required_permissions) {
std::unique_ptr<base::ListValue> required_permissions,
std::unique_ptr<base::ListValue> optional_permissions) {
app_ = extensions::ExtensionBuilder("Test ChromeOS System Extension")
.SetManifestKey("chromeos_system_extension",
extensions::DictionaryBuilder().Build())
.SetManifestKey("permissions", std::move(required_permissions))
.SetManifestKey("optional_permissions",
std::move(optional_permissions))
.SetManifestKey("manifest_version", 3)
.SetID(kChromeOSSystemExtensionId) // only allowlisted id
.SetLocation(ManifestLocation::kInternal)
Expand All @@ -79,6 +86,26 @@ class ChromeOSPermissionMessageUnittest : public testing::Test {
env_.GetExtensionService()->AddExtension(app_.get());
}

// Returns the permission messages that would display in the prompt that
// requests all the optional permissions for the current |app_|.
std::vector<std::u16string> GetInactiveOptionalPermissionMessages() {
std::unique_ptr<const PermissionSet> granted_permissions =
env_.GetExtensionPrefs()->GetGrantedPermissions(app_->id());
const PermissionSet& optional_permissions =
PermissionsParser::GetOptionalPermissions(app_.get());
std::unique_ptr<const PermissionSet> requested_permissions =
PermissionSet::CreateDifference(optional_permissions,
*granted_permissions);
return GetMessages(*requested_permissions);
}

void GrantOptionalPermissions() {
extensions::permissions_test_util::
GrantOptionalPermissionsAndWaitForCompletion(
env_.profile(), *app_,
PermissionsParser::GetOptionalPermissions(app_.get()));
}

std::vector<std::u16string> active_permissions() {
return GetMessages(app_->permissions_data()->active_permissions());
}
Expand All @@ -87,6 +114,10 @@ class ChromeOSPermissionMessageUnittest : public testing::Test {
return GetMessages(PermissionsParser::GetRequiredPermissions(app_.get()));
}

std::vector<std::u16string> optional_permissions() {
return GetMessages(PermissionsParser::GetOptionalPermissions(app_.get()));
}

private:
std::vector<std::u16string> GetMessages(const PermissionSet& permissions) {
std::vector<std::u16string> messages;
Expand All @@ -107,8 +138,10 @@ class ChromeOSPermissionMessageUnittest : public testing::Test {

TEST_F(ChromeOSPermissionMessageUnittest, OsDiagnosticsMessage) {
CreateAndInstallExtensionWithPermissions(
extensions::ListBuilder().Append("os.diagnostics").Build());
extensions::ListBuilder().Append("os.diagnostics").Build(),
extensions::ListBuilder().Build());

ASSERT_EQ(0U, optional_permissions().size());
ASSERT_EQ(1U, required_permissions().size());
EXPECT_EQ(kDiagnosticsPermissionMessage, required_permissions()[0]);
ASSERT_EQ(1U, active_permissions().size());
Expand All @@ -117,8 +150,10 @@ TEST_F(ChromeOSPermissionMessageUnittest, OsDiagnosticsMessage) {

TEST_F(ChromeOSPermissionMessageUnittest, OsTelemetryMessage) {
CreateAndInstallExtensionWithPermissions(
extensions::ListBuilder().Append("os.telemetry").Build());
extensions::ListBuilder().Append("os.telemetry").Build(),
extensions::ListBuilder().Build());

ASSERT_EQ(0U, optional_permissions().size());
ASSERT_EQ(1U, required_permissions().size());
EXPECT_EQ(kTelemetryPermissionMessage, required_permissions()[0]);
ASSERT_EQ(1U, active_permissions().size());
Expand All @@ -129,12 +164,34 @@ TEST_F(ChromeOSPermissionMessageUnittest, OsTelemetryAndOsDiagnosticsMessage) {
CreateAndInstallExtensionWithPermissions(extensions::ListBuilder()
.Append("os.diagnostics")
.Append("os.telemetry")
.Build());
.Build(),
extensions::ListBuilder().Build());
ASSERT_EQ(0U, optional_permissions().size());
ASSERT_EQ(1U, required_permissions().size());
EXPECT_EQ(kTelemetryAndDiagnosticsPermissionMessage,
required_permissions()[0]);
ASSERT_EQ(1U, active_permissions().size());
EXPECT_EQ(kTelemetryAndDiagnosticsPermissionMessage, active_permissions()[0]);
}

TEST_F(ChromeOSPermissionMessageUnittest, OsTelemetrySerialNumber) {
CreateAndInstallExtensionWithPermissions(
extensions::ListBuilder().Build(),
extensions::ListBuilder().Append("os.telemetry.serial_number").Build());

ASSERT_EQ(1U, optional_permissions().size());
EXPECT_EQ(kTelemetrySerialNumberPermissionMessage, optional_permissions()[0]);
ASSERT_EQ(1U, GetInactiveOptionalPermissionMessages().size());
EXPECT_EQ(kTelemetrySerialNumberPermissionMessage,
GetInactiveOptionalPermissionMessages()[0]);
ASSERT_EQ(0U, required_permissions().size());
ASSERT_EQ(0U, active_permissions().size());

GrantOptionalPermissions();

ASSERT_EQ(0U, GetInactiveOptionalPermissionMessages().size());
ASSERT_EQ(1U, active_permissions().size());
EXPECT_EQ(kTelemetrySerialNumberPermissionMessage, active_permissions()[0]);
}

} // namespace chromeos
6 changes: 6 additions & 0 deletions chrome/common/chromeos/extensions/api/_permission_features.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,11 @@
"chromeos_system_extension"
],
"dependencies": [ "manifest:chromeos_system_extension" ]
},
"os.telemetry.serial_number": {
"channel": "stable",
"extension_types": [
"chromeos_system_extension"
]
}
}
Loading

0 comments on commit b303ca4

Please sign in to comment.