Skip to content

Commit

Permalink
Introduce DeviceAttributeAPI class for better test coverage
Browse files Browse the repository at this point in the history 
Replacing the device_attributes_api static methods and replacing it
with the DeviceAttributesAPI class.

Bug: b/333069979
Change-Id: I3123a5164f5f0a7171bd5171e49307508d73e4e7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5522575
Reviewed-by: Jeroen Dhollander <[email protected]>
Reviewed-by: Sergii Bykov <[email protected]>
Commit-Queue: Ashutosh Singhal <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1299318}
  • Loading branch information
Ashutosh Singhal authored and Chromium LUCI CQ committed May 10, 2024
1 parent bbd2eb9 commit 9791d5e
Show file tree
Hide file tree
Showing 6 changed files with 267 additions and 76 deletions.
24 changes: 13 additions & 11 deletions chrome/browser/device_api/device_attribute_api.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
using blink::mojom::DeviceAPIService;
using blink::mojom::DeviceAttributeResultPtr;

namespace device_attribute_api {

namespace {

using Result = blink::mojom::DeviceAttributeResult;
Expand Down Expand Up @@ -61,18 +59,22 @@ void AdaptLacrosResult(

} // namespace

void ReportNotAffiliatedError(
DeviceAttributeApiImpl::DeviceAttributeApiImpl() = default;
DeviceAttributeApiImpl::~DeviceAttributeApiImpl() = default;

void DeviceAttributeApiImpl::ReportNotAffiliatedError(
base::OnceCallback<void(DeviceAttributeResultPtr)> callback) {
std::move(callback).Run(Result::NewErrorMessage(kNotAffiliatedErrorMessage));
}

void ReportNotAllowedError(
void DeviceAttributeApiImpl::ReportNotAllowedError(
base::OnceCallback<void(DeviceAttributeResultPtr)> callback) {
std::move(callback).Run(
Result::NewErrorMessage(kNotAllowedOriginErrorMessage));
}

void GetDirectoryId(DeviceAPIService::GetDirectoryIdCallback callback) {
void DeviceAttributeApiImpl::GetDirectoryId(
DeviceAPIService::GetDirectoryIdCallback callback) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
const std::string attribute = g_browser_process->platform_part()
->browser_policy_connector_ash()
Expand All @@ -94,7 +96,8 @@ void GetDirectoryId(DeviceAPIService::GetDirectoryIdCallback callback) {
#endif
}

void GetHostname(DeviceAPIService::GetHostnameCallback callback) {
void DeviceAttributeApiImpl::GetHostname(
DeviceAPIService::GetHostnameCallback callback) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
const std::optional<std::string> attribute =
g_browser_process->platform_part()
Expand All @@ -114,7 +117,8 @@ void GetHostname(DeviceAPIService::GetHostnameCallback callback) {
#endif
}

void GetSerialNumber(DeviceAPIService::GetSerialNumberCallback callback) {
void DeviceAttributeApiImpl::GetSerialNumber(
DeviceAPIService::GetSerialNumberCallback callback) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
const std::optional<std::string_view> attribute =
ash::system::StatisticsProvider::GetInstance()->GetMachineID();
Expand All @@ -134,7 +138,7 @@ void GetSerialNumber(DeviceAPIService::GetSerialNumberCallback callback) {
#endif
}

void GetAnnotatedAssetId(
void DeviceAttributeApiImpl::GetAnnotatedAssetId(
DeviceAPIService::GetAnnotatedAssetIdCallback callback) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
const std::string attribute = g_browser_process->platform_part()
Expand All @@ -157,7 +161,7 @@ void GetAnnotatedAssetId(
#endif
}

void GetAnnotatedLocation(
void DeviceAttributeApiImpl::GetAnnotatedLocation(
DeviceAPIService::GetAnnotatedLocationCallback callback) {
#if BUILDFLAG(IS_CHROMEOS_ASH)
const std::string attribute = g_browser_process->platform_part()
Expand All @@ -179,5 +183,3 @@ void GetAnnotatedLocation(
Result::NewErrorMessage(kNotSupportedPlatformErrorMessage));
#endif
}

} // namespace device_attribute_api
63 changes: 48 additions & 15 deletions chrome/browser/device_api/device_attribute_api.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,55 @@
#include "base/functional/callback_forward.h"
#include "third_party/blink/public/mojom/device/device.mojom.h"

namespace device_attribute_api {
class DeviceAttributeApi {
public:
DeviceAttributeApi() = default;
DeviceAttributeApi(const DeviceAttributeApi&) = delete;
DeviceAttributeApi& operator=(const DeviceAttributeApi&) = delete;
virtual ~DeviceAttributeApi() = default;

void ReportNotAffiliatedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)> callback);
void ReportNotAllowedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)> callback);
void GetDirectoryId(
blink::mojom::DeviceAPIService::GetDirectoryIdCallback callback);
void GetHostname(blink::mojom::DeviceAPIService::GetHostnameCallback callback);
void GetSerialNumber(
blink::mojom::DeviceAPIService::GetSerialNumberCallback callback);
void GetAnnotatedAssetId(
blink::mojom::DeviceAPIService::GetAnnotatedAssetIdCallback callback);
void GetAnnotatedLocation(
blink::mojom::DeviceAPIService::GetAnnotatedLocationCallback callback);
virtual void ReportNotAffiliatedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)>
callback) = 0;
virtual void ReportNotAllowedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)>
callback) = 0;
virtual void GetDirectoryId(
blink::mojom::DeviceAPIService::GetDirectoryIdCallback callback) = 0;
virtual void GetHostname(
blink::mojom::DeviceAPIService::GetHostnameCallback callback) = 0;
virtual void GetSerialNumber(
blink::mojom::DeviceAPIService::GetSerialNumberCallback callback) = 0;
virtual void GetAnnotatedAssetId(
blink::mojom::DeviceAPIService::GetAnnotatedAssetIdCallback callback) = 0;
virtual void GetAnnotatedLocation(
blink::mojom::DeviceAPIService::GetAnnotatedLocationCallback
callback) = 0;
};

} // namespace device_attribute_api
class DeviceAttributeApiImpl : public DeviceAttributeApi {
public:
DeviceAttributeApiImpl();
~DeviceAttributeApiImpl() override;

void ReportNotAffiliatedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)> callback)
override;
void ReportNotAllowedError(
base::OnceCallback<void(blink::mojom::DeviceAttributeResultPtr)> callback)
override;
void GetDirectoryId(
blink::mojom::DeviceAPIService::GetDirectoryIdCallback callback) override;
void GetHostname(
blink::mojom::DeviceAPIService::GetHostnameCallback callback) override;
void GetSerialNumber(blink::mojom::DeviceAPIService::GetSerialNumberCallback
callback) override;
void GetAnnotatedAssetId(
blink::mojom::DeviceAPIService::GetAnnotatedAssetIdCallback callback)
override;
void GetAnnotatedLocation(
blink::mojom::DeviceAPIService::GetAnnotatedLocationCallback callback)
override;
};

#endif // CHROME_BROWSER_DEVICE_API_DEVICE_ATTRIBUTE_API_H_
26 changes: 16 additions & 10 deletions chrome/browser/device_api/device_attribute_api_browsertest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,29 @@ class DeviceAttributeAPIUnsetTest : public policy::DevicePolicyCrosBrowserTest {
std::string());
}

DeviceAttributeApi& device_attributes_api() { return device_attributes_api_; }

private:
ash::system::ScopedFakeStatisticsProvider fake_statistics_provider_;
DeviceAttributeApiImpl device_attributes_api_;
};

IN_PROC_BROWSER_TEST_F(DeviceAttributeAPIUnsetTest, AllAttributes) {
base::test::TestFuture<blink::mojom::DeviceAttributeResultPtr> future;

device_attribute_api::GetDirectoryId(future.GetRepeatingCallback());
device_attributes_api().GetDirectoryId(future.GetCallback());
EXPECT_FALSE(future.Take()->get_attribute().has_value());

device_attribute_api::GetAnnotatedAssetId(future.GetRepeatingCallback());
device_attributes_api().GetAnnotatedAssetId(future.GetCallback());
EXPECT_FALSE(future.Take()->get_attribute().has_value());

device_attribute_api::GetAnnotatedLocation(future.GetRepeatingCallback());
device_attributes_api().GetAnnotatedLocation(future.GetCallback());
EXPECT_FALSE(future.Take()->get_attribute().has_value());

device_attribute_api::GetSerialNumber(future.GetRepeatingCallback());
device_attributes_api().GetSerialNumber(future.GetCallback());
EXPECT_FALSE(future.Take()->get_attribute().has_value());

device_attribute_api::GetHostname(future.GetRepeatingCallback());
device_attributes_api().GetHostname(future.GetCallback());
EXPECT_FALSE(future.Take()->get_attribute().has_value());
}

Expand All @@ -78,8 +81,11 @@ class DeviceAttributeAPITest : public policy::DevicePolicyCrosBrowserTest {
kSerialNumber);
}

DeviceAttributeApi& device_attributes_api() { return device_attributes_api_; }

private:
ash::system::ScopedFakeStatisticsProvider fake_statistics_provider_;
DeviceAttributeApiImpl device_attributes_api_;
};

IN_PROC_BROWSER_TEST_F(DeviceAttributeAPITest, AllAttributes) {
Expand All @@ -88,18 +94,18 @@ IN_PROC_BROWSER_TEST_F(DeviceAttributeAPITest, AllAttributes) {

base::test::TestFuture<blink::mojom::DeviceAttributeResultPtr> future;

device_attribute_api::GetDirectoryId(future.GetRepeatingCallback());
device_attributes_api().GetDirectoryId(future.GetCallback());
EXPECT_EQ(future.Take()->get_attribute(), kDirectoryApiId);

device_attribute_api::GetAnnotatedAssetId(future.GetRepeatingCallback());
device_attributes_api().GetAnnotatedAssetId(future.GetCallback());
EXPECT_EQ(future.Take()->get_attribute(), kAnnotatedAssetId);

device_attribute_api::GetAnnotatedLocation(future.GetRepeatingCallback());
device_attributes_api().GetAnnotatedLocation(future.GetCallback());
EXPECT_EQ(future.Take()->get_attribute(), kAnnotatedLocation);

device_attribute_api::GetHostname(future.GetRepeatingCallback());
device_attributes_api().GetHostname(future.GetCallback());
EXPECT_EQ(future.Take()->get_attribute(), kHostname);

device_attribute_api::GetSerialNumber(future.GetRepeatingCallback());
device_attributes_api().GetSerialNumber(future.GetCallback());
EXPECT_EQ(future.Take()->get_attribute(), kSerialNumber);
}
51 changes: 35 additions & 16 deletions chrome/browser/device_api/device_service_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@
#include "chrome/browser/device_api/device_service_impl.h"

#include "base/check_deref.h"
#include "base/check_is_test.h"
#include "base/containers/contains.h"
#include "base/functional/bind.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/app_mode/app_mode_utils.h"
#include "chrome/browser/device_api/device_attribute_api.h"
Expand Down Expand Up @@ -149,8 +151,10 @@ bool IsTrustedContext(content::RenderFrameHost& host,

DeviceServiceImpl::DeviceServiceImpl(
content::RenderFrameHost& host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver)
: DocumentService(host, std::move(receiver)) {
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api)
: DocumentService(host, std::move(receiver)),
device_attribute_api_(std::move(device_attribute_api)) {
pref_change_registrar_.Init(
Profile::FromBrowserContext(host.GetBrowserContext())->GetPrefs());
pref_change_registrar_.Add(
Expand All @@ -174,7 +178,8 @@ DeviceServiceImpl::~DeviceServiceImpl() = default;
// static
void DeviceServiceImpl::Create(
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver) {
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api) {
CHECK(host);
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

Expand All @@ -186,7 +191,24 @@ void DeviceServiceImpl::Create(
}
// The object is bound to the lifetime of |host| and the mojo
// connection. See DocumentService for details.
new DeviceServiceImpl(*host, std::move(receiver));
new DeviceServiceImpl(*host, std::move(receiver),
std::move(device_attribute_api));
}

// static
void DeviceServiceImpl::Create(
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver) {
Create(host, std::move(receiver), std::make_unique<DeviceAttributeApiImpl>());
}

// static
void DeviceServiceImpl::CreateForTest(
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api) {
CHECK_IS_TEST();
Create(host, std::move(receiver), std::move(device_attribute_api));
}

// static
Expand All @@ -203,44 +225,41 @@ void DeviceServiceImpl::OnDisposingIfNeeded() {
}

void DeviceServiceImpl::GetDirectoryId(GetDirectoryIdCallback callback) {
GetDeviceAttribute(base::BindOnce(device_attribute_api::GetDirectoryId),
std::move(callback));
GetDeviceAttribute(&DeviceAttributeApi::GetDirectoryId, std::move(callback));
}

void DeviceServiceImpl::GetHostname(GetHostnameCallback callback) {
GetDeviceAttribute(base::BindOnce(device_attribute_api::GetHostname),
std::move(callback));
GetDeviceAttribute(&DeviceAttributeApi::GetHostname, std::move(callback));
}

void DeviceServiceImpl::GetSerialNumber(GetSerialNumberCallback callback) {
GetDeviceAttribute(base::BindOnce(device_attribute_api::GetSerialNumber),
std::move(callback));
GetDeviceAttribute(&DeviceAttributeApi::GetSerialNumber, std::move(callback));
}

void DeviceServiceImpl::GetAnnotatedAssetId(
GetAnnotatedAssetIdCallback callback) {
GetDeviceAttribute(base::BindOnce(device_attribute_api::GetAnnotatedAssetId),
GetDeviceAttribute(&DeviceAttributeApi::GetAnnotatedAssetId,
std::move(callback));
}

void DeviceServiceImpl::GetAnnotatedLocation(
GetAnnotatedLocationCallback callback) {
GetDeviceAttribute(base::BindOnce(device_attribute_api::GetAnnotatedLocation),
GetDeviceAttribute(&DeviceAttributeApi::GetAnnotatedLocation,
std::move(callback));
}

void DeviceServiceImpl::GetDeviceAttribute(
base::OnceCallback<void(DeviceAttributeCallback)> handler,
void (DeviceAttributeApi::*method)(DeviceAttributeCallback callback),
DeviceAttributeCallback callback) {
if (!IsAffiliatedUser()) {
device_attribute_api::ReportNotAffiliatedError(std::move(callback));
device_attribute_api_->ReportNotAffiliatedError(std::move(callback));
return;
}

if (!CanAccessDeviceAttributes(GetPrefs(render_frame_host()), origin())) {
device_attribute_api::ReportNotAllowedError(std::move(callback));
device_attribute_api_->ReportNotAllowedError(std::move(callback));
return;
}

std::move(handler).Run(std::move(callback));
(device_attribute_api_.get()->*method)(std::move(callback));
}
19 changes: 17 additions & 2 deletions chrome/browser/device_api/device_service_impl.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
#ifndef CHROME_BROWSER_DEVICE_API_DEVICE_SERVICE_IMPL_H_
#define CHROME_BROWSER_DEVICE_API_DEVICE_SERVICE_IMPL_H_

#include <memory>

#include "base/functional/callback_forward.h"
#include "chrome/browser/device_api/device_attribute_api.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_registry_simple.h"
#include "content/public/browser/document_service.h"
Expand All @@ -30,6 +33,11 @@ class DeviceServiceImpl final
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver);

static void CreateForTest(
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api);

// Register the user prefs.
static void RegisterProfilePrefs(PrefRegistrySimple* registry);

Expand All @@ -45,17 +53,24 @@ class DeviceServiceImpl final
void GetAnnotatedLocation(GetAnnotatedLocationCallback callback) override;

private:
static void Create(
content::RenderFrameHost* host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api);

DeviceServiceImpl(
content::RenderFrameHost& host,
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver);
mojo::PendingReceiver<blink::mojom::DeviceAPIService> receiver,
std::unique_ptr<DeviceAttributeApi> device_attribute_api);

void GetDeviceAttribute(
base::OnceCallback<void(DeviceAttributeCallback)> handler,
void (DeviceAttributeApi::*method)(DeviceAttributeCallback callback),
DeviceAttributeCallback callback);

void OnDisposingIfNeeded();

PrefChangeRegistrar pref_change_registrar_;
std::unique_ptr<DeviceAttributeApi> device_attribute_api_;
};

#endif // CHROME_BROWSER_DEVICE_API_DEVICE_SERVICE_IMPL_H_
Loading

0 comments on commit 9791d5e

Please sign in to comment.