Add app push-install log uploading to CloudPolicyClient

This CL adds a method for uploading app push-install logs to the
CloudPolicyClient. It contains a trivial amount of boilerplate
code and a non-trivial amount of boilerplate tests.

Bug: b/73277923
Test: components_unittests and browser_tests
Change-Id: Ib2ca2f562e7cf974e7536e612d0b6a72bdcebcf7
Reviewed-on: https://chromium-review.googlesource.com/919485
Commit-Queue: Bartosz Fabianowski <bartfab@chromium.org>
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537766}

chrome/browser/policy/cloud/device_management_service_browsertest.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/stl_util.h"
@@ -60,6 +61,8 @@ void ConstructResponse(const char* request_data,
     response.mutable_policy_response()->add_response();
   } else if (request.has_auto_enrollment_request()) {
     response.mutable_auto_enrollment_response();
+  } else if (request.has_app_install_report_request()) {
+    response.mutable_app_install_report_response();
   } else {
     FAIL() << "Failed to parse request.";
   }
@@ -261,6 +264,26 @@ IN_PROC_BROWSER_TEST_P(DeviceManagementServiceIntegrationTest, AutoEnrollment) {
   run_loop.Run();
 }
 
+IN_PROC_BROWSER_TEST_P(DeviceManagementServiceIntegrationTest,
+                       AppInstallReport) {
+  PerformRegistration();
+
+  ExpectRequest();
+  base::RunLoop run_loop;
+  EXPECT_CALL(*this, OnJobDone(DM_STATUS_SUCCESS, _, _))
+      .WillOnce(InvokeWithoutArgs(&run_loop, &base::RunLoop::QuitWhenIdle));
+  std::unique_ptr<DeviceManagementRequestJob> job(service_->CreateJob(
+      DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT,
+      g_browser_process->system_request_context()));
+  job->SetDMToken(token_);
+  job->SetClientID("testid");
+  job->GetRequest()->mutable_app_install_report_request();
+  job->Start(base::AdaptCallbackForRepeating(
+      base::BindOnce(&DeviceManagementServiceIntegrationTest::OnJobDone,
+                     base::Unretained(this))));
+  run_loop.Run();
+}
+
 INSTANTIATE_TEST_CASE_P(
     DeviceManagementServiceIntegrationTestInstance,
     DeviceManagementServiceIntegrationTest,

chrome/browser/policy/test/policy_testserver.py

@@ -330,6 +330,9 @@ class PolicyRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
       response = self.ProcessCheckAndroidManagementRequest(
           rmsg.check_android_management_request,
           str(self.GetUniqueParam('oauth_token')))
+    elif request_type == 'app_install_report':
+      response = self.ProcessAppInstallReportRequest(
+          rmsg.app_install_report_request)
     else:
       return (400, 'Invalid request parameter')
 
@@ -725,6 +728,18 @@ class PolicyRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
     else:
       return (403, response)
 
+  def ProcessAppInstallReportRequest(self, app_install_report):
+    """Handles a push-installed app report upload request.
+
+    Returns:
+      A tuple of HTTP status code and response data to send to the client.
+    """
+    app_install_report_response = dm.AppInstallReportResponse()
+    response = dm.DeviceManagementResponse()
+    response.app_install_report_response.CopyFrom(app_install_report_response)
+
+    return (200, response)
+
   def SetProtoRepeatedField(self, group_message, field, field_value):
     assert type(field_value) == list
     entries = group_message.__getattribute__(field.name)

components/policy/core/common/cloud/cloud_policy_client.cc

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
+#include "base/callback_helpers.h"
 #include "base/guid.h"
 #include "base/logging.h"
 #include "base/stl_util.h"
@@ -114,6 +115,7 @@ void CloudPolicyClient::SetupRegistration(const std::string& dm_token,
   dm_token_ = dm_token;
   client_id_ = client_id;
   request_jobs_.clear();
+  app_install_report_request_job_ = nullptr;
   policy_fetch_request_job_.reset();
   responses_.clear();
 
@@ -383,13 +385,46 @@ void CloudPolicyClient::UploadDeviceStatus(
     *request->mutable_session_status_report_request() = *session_status;
 
   const DeviceManagementRequestJob::Callback job_callback =
-      base::Bind(&CloudPolicyClient::OnStatusUploadCompleted,
-                 weak_ptr_factory_.GetWeakPtr(), request_job.get(), callback);
+      base::AdaptCallbackForRepeating(base::BindOnce(
+          &CloudPolicyClient::OnReportUploadCompleted,
+          weak_ptr_factory_.GetWeakPtr(), request_job.get(), callback));
 
   request_jobs_.push_back(std::move(request_job));
   request_jobs_.back()->Start(job_callback);
 }
 
+void CloudPolicyClient::UploadAppInstallReport(
+    const em::AppInstallReportRequest* app_install_report,
+    const StatusCallback& callback) {
+  CHECK(is_registered());
+  DCHECK(app_install_report);
+
+  std::unique_ptr<DeviceManagementRequestJob> request_job(service_->CreateJob(
+      DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT,
+      GetRequestContext()));
+  request_job->SetDMToken(dm_token_);
+  request_job->SetClientID(client_id_);
+
+  *request_job->GetRequest()->mutable_app_install_report_request() =
+      *app_install_report;
+
+  const DeviceManagementRequestJob::Callback job_callback =
+      base::AdaptCallbackForRepeating(base::BindOnce(
+          &CloudPolicyClient::OnReportUploadCompleted,
+          weak_ptr_factory_.GetWeakPtr(), request_job.get(), callback));
+
+  CancelAppInstallReportUpload();
+  app_install_report_request_job_ = request_job.get();
+  request_jobs_.push_back(std::move(request_job));
+  request_jobs_.back()->Start(job_callback);
+}
+
+void CloudPolicyClient::CancelAppInstallReportUpload() {
+  if (app_install_report_request_job_) {
+    RemoveJob(app_install_report_request_job_);
+  }
+}
+
 void CloudPolicyClient::FetchRemoteCommands(
     std::unique_ptr<RemoteCommandJob::UniqueIDType> last_command_id,
     const std::vector<em::RemoteCommandResult>& command_results,
@@ -702,6 +737,7 @@ void CloudPolicyClient::OnUnregisterCompleted(
     dm_token_.clear();
     // Cancel all outstanding jobs.
     request_jobs_.clear();
+    app_install_report_request_job_ = nullptr;
     NotifyRegistrationStateChanged();
   } else {
     NotifyClientError();
@@ -819,6 +855,9 @@ void CloudPolicyClient::OnAvailableLicensesRequested(
 }
 
 void CloudPolicyClient::RemoveJob(const DeviceManagementRequestJob* job) {
+  if (app_install_report_request_job_ == job) {
+    app_install_report_request_job_ = nullptr;
+  }
   for (auto it = request_jobs_.begin(); it != request_jobs_.end(); ++it) {
     if (it->get() == job) {
       request_jobs_.erase(it);
@@ -830,7 +869,7 @@ void CloudPolicyClient::RemoveJob(const DeviceManagementRequestJob* job) {
   NOTREACHED();
 }
 
-void CloudPolicyClient::OnStatusUploadCompleted(
+void CloudPolicyClient::OnReportUploadCompleted(
     const DeviceManagementRequestJob* job,
     const CloudPolicyClient::StatusCallback& callback,
     DeviceManagementStatus status,

components/policy/core/common/cloud/cloud_policy_client.h

@@ -180,6 +180,16 @@ class POLICY_EXPORT CloudPolicyClient {
       const enterprise_management::SessionStatusReportRequest* session_status,
       const StatusCallback& callback);
 
+  // Uploads a report on the status of app push-installs. The client must be in
+  // a registered state. The |callback| will be called when the operation
+  // completes.
+  virtual void UploadAppInstallReport(
+      const enterprise_management::AppInstallReportRequest* app_install_report,
+      const StatusCallback& callback);
+
+  // Cancels the pending app push-install status report upload, if an.
+  virtual void CancelAppInstallReportUpload();
+
   // Attempts to fetch remote commands, with |last_command_id| being the ID of
   // the last command that finished execution and |command_results| being
   // results for previous commands which have not been reported yet. The
@@ -357,8 +367,8 @@ class POLICY_EXPORT CloudPolicyClient {
       int net_error,
       const enterprise_management::DeviceManagementResponse& response);
 
-  // Callback for status upload requests.
-  void OnStatusUploadCompleted(
+  // Callback for several types of status/report upload requests.
+  void OnReportUploadCompleted(
       const DeviceManagementRequestJob* job,
       const StatusCallback& callback,
       DeviceManagementStatus status,
@@ -449,6 +459,10 @@ class POLICY_EXPORT CloudPolicyClient {
   // silently cancelled if Unregister() is called.
   std::vector<std::unique_ptr<DeviceManagementRequestJob>> request_jobs_;
 
+  // Only one outstanding app push-install report upload is allowed, and it must
+  // be accessible so that it can be canceled.
+  DeviceManagementRequestJob* app_install_report_request_job_ = nullptr;
+
   // The policy responses returned by the last policy fetch operation.
   ResponseMap responses_;
   DeviceManagementStatus status_ = DM_STATUS_SUCCESS;

components/policy/core/common/cloud/cloud_policy_client_unittest.cc

@@ -53,6 +53,7 @@ const char kResultPayload[] = "output_payload";
 const char kAssetId[] = "fake-asset-id";
 const char kLocation[] = "fake-location";
 const char kGcmID[] = "fake-gcm-id";
+const char kPackageName[] = "com.example.app";
 const int64_t kAgeOfCommand = 123123123;
 const int64_t kLastCommandId = 123456789;
 const int64_t kTimestamp = 987654321;
@@ -206,6 +207,8 @@ class CloudPolicyClientTest : public testing::Test {
     license_two->mutable_license_type()->set_license_type(
         em::LicenseType_LicenseTypeEnum_KIOSK);
     license_two->set_available_licenses(0);
+
+    upload_app_install_report_response_.mutable_app_install_report_response();
   }
 
   void SetUp() override {
@@ -361,6 +364,26 @@ class CloudPolicyClientTest : public testing::Test {
                                    MatchProto(check_device_license_request_)));
   }
 
+  void ExpectUploadAppInstallReport(const em::DeviceManagementRequest& request,
+                                    MockDeviceManagementJob** async_job) {
+    if (async_job) {
+      EXPECT_CALL(
+          service_,
+          CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT,
+                    request_context_))
+          .WillOnce(service_.CreateAsyncJob(async_job));
+    } else {
+      EXPECT_CALL(
+          service_,
+          CreateJob(DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT,
+                    request_context_))
+          .WillOnce(service_.SucceedJob(upload_app_install_report_response_));
+    }
+    EXPECT_CALL(service_, StartJob(dm_protocol::kValueRequestAppInstallReport,
+                                   std::string(), std::string(), kDMToken,
+                                   client_id_, MatchProto(request)));
+  }
+
   void CheckPolicyResponse() {
     ASSERT_TRUE(client_->GetPolicyFor(policy_type_, std::string()));
     EXPECT_THAT(*client_->GetPolicyFor(policy_type_, std::string()),
@@ -400,6 +423,7 @@ class CloudPolicyClientTest : public testing::Test {
   em::DeviceManagementResponse gcm_id_update_response_;
   em::DeviceManagementResponse check_device_license_response_;
   em::DeviceManagementResponse check_device_license_broken_response_;
+  em::DeviceManagementResponse upload_app_install_report_response_;
 
   base::MessageLoop loop_;
   std::string client_id_;
@@ -1076,4 +1100,76 @@ TEST_F(CloudPolicyClientTest, RequestAvailableLicensesBrokenResponse) {
   EXPECT_EQ(DM_STATUS_RESPONSE_DECODING_ERROR, client_->status());
 }
 
+TEST_F(CloudPolicyClientTest, UploadAppInstallReport) {
+  Register();
+  em::DeviceManagementRequest request;
+  request.mutable_app_install_report_request();
+  ExpectUploadAppInstallReport(request, nullptr /* async_job */);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(1);
+
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+
+  em::AppInstallReportRequest app_install_report;
+  client_->UploadAppInstallReport(&app_install_report, callback);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+}
+
+TEST_F(CloudPolicyClientTest, CancelUploadAppInstallReport) {
+  Register();
+  em::DeviceManagementRequest request;
+  request.mutable_app_install_report_request();
+  MockDeviceManagementJob* async_job = nullptr;
+  ExpectUploadAppInstallReport(request, &async_job);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(0);
+
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+
+  em::AppInstallReportRequest app_install_report;
+  client_->UploadAppInstallReport(&app_install_report, callback);
+  EXPECT_EQ(1, client_->GetActiveRequestCountForTest());
+
+  client_->CancelAppInstallReportUpload();
+  EXPECT_EQ(0, client_->GetActiveRequestCountForTest());
+}
+
+TEST_F(CloudPolicyClientTest, UploadAppInstallReportSupersedesPending) {
+  Register();
+  em::DeviceManagementRequest request;
+  request.mutable_app_install_report_request();
+  MockDeviceManagementJob* async_job = nullptr;
+  ExpectUploadAppInstallReport(request, &async_job);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(0);
+
+  CloudPolicyClient::StatusCallback callback =
+      base::BindRepeating(&MockStatusCallbackObserver::OnCallbackComplete,
+                          base::Unretained(&callback_observer_));
+
+  em::AppInstallReportRequest app_install_report;
+  client_->UploadAppInstallReport(&app_install_report, callback);
+  EXPECT_EQ(1, client_->GetActiveRequestCountForTest());
+  Mock::VerifyAndClearExpectations(&service_);
+  Mock::VerifyAndClearExpectations(&callback_observer_);
+
+  // Starting another app push-install report upload should cancel the pending
+  // one.
+  request.mutable_app_install_report_request()
+      ->add_app_install_report()
+      ->set_package(kPackageName);
+  ExpectUploadAppInstallReport(request, &async_job);
+  EXPECT_CALL(callback_observer_, OnCallbackComplete(true)).Times(1);
+
+  app_install_report.CopyFrom(request.app_install_report_request());
+  client_->UploadAppInstallReport(&app_install_report, callback);
+  EXPECT_EQ(1, client_->GetActiveRequestCountForTest());
+
+  async_job->SendResponse(DM_STATUS_SUCCESS,
+                          upload_app_install_report_response_);
+  EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
+  EXPECT_EQ(0, client_->GetActiveRequestCountForTest());
+}
+
 }  // namespace policy

components/policy/core/common/cloud/cloud_policy_constants.cc

@@ -54,6 +54,7 @@ const char kValueRequestActiveDirectoryEnrollPlayUser[] =
 const char kValueRequestActiveDirectoryPlayActivity[] =
     "active_directory_play_activity";
 const char kValueRequestCheckDeviceLicense[] = "check_device_license";
+const char kValueRequestAppInstallReport[] = "app_install_report";
 
 const char kChromeDevicePolicyType[] = "google/chromeos/device";
 #if defined(OS_CHROMEOS)

components/policy/core/common/cloud/cloud_policy_constants.h

@@ -46,6 +46,7 @@ POLICY_EXPORT extern const char kValueRequestCertBasedRegister[];
 POLICY_EXPORT extern const char kValueRequestActiveDirectoryEnrollPlayUser[];
 POLICY_EXPORT extern const char kValueRequestActiveDirectoryPlayActivity[];
 POLICY_EXPORT extern const char kValueRequestCheckDeviceLicense[];
+POLICY_EXPORT extern const char kValueRequestAppInstallReport[];
 
 // Policy type strings for the policy_type field in PolicyFetchRequest.
 POLICY_EXPORT extern const char kChromeDevicePolicyType[];

components/policy/core/common/cloud/device_management_service.cc

@@ -158,6 +158,8 @@ const char* JobTypeToRequestType(DeviceManagementRequestJob::JobType type) {
       return dm_protocol::kValueRequestActiveDirectoryPlayActivity;
     case DeviceManagementRequestJob::TYPE_REQUEST_LICENSE_TYPES:
       return dm_protocol::kValueRequestCheckDeviceLicense;
+    case DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT:
+      return dm_protocol::kValueRequestAppInstallReport;
   }
   NOTREACHED() << "Invalid job type " << type;
   return "";

components/policy/core/common/cloud/device_management_service.h

@@ -63,6 +63,7 @@ class POLICY_EXPORT DeviceManagementRequestJob {
     TYPE_ACTIVE_DIRECTORY_ENROLL_PLAY_USER = 14,
     TYPE_ACTIVE_DIRECTORY_PLAY_ACTIVITY = 15,
     TYPE_REQUEST_LICENSE_TYPES = 16,
+    TYPE_UPLOAD_APP_INSTALL_REPORT = 17,
   };
 
   typedef base::Callback<

components/policy/core/common/cloud/device_management_service_unittest.cc

@@ -9,6 +9,7 @@
 #include <vector>
 
 #include "base/bind.h"
+#include "base/callback_helpers.h"
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
@@ -187,6 +188,20 @@ class DeviceManagementServiceTestBase : public testing::Test {
     return job;
   }
 
+  DeviceManagementRequestJob* StartAppInstallReportJob() {
+    DeviceManagementRequestJob* job = service_->CreateJob(
+        DeviceManagementRequestJob::TYPE_UPLOAD_APP_INSTALL_REPORT,
+        request_context_.get());
+    job->SetDMToken(kDMToken);
+    job->SetClientID(kClientID);
+    job->GetRequest()->mutable_app_install_report_request();
+    job->SetRetryCallback(base::BindRepeating(
+        &DeviceManagementServiceTestBase::OnJobRetry, base::Unretained(this)));
+    job->Start(base::AdaptCallbackForRepeating(base::BindOnce(
+        &DeviceManagementServiceTestBase::OnJobDone, base::Unretained(this))));
+    return job;
+  }
+
   void SendResponse(net::TestURLFetcher* fetcher,
                     net::Error error,
                     int http_status,
@@ -310,6 +325,18 @@ TEST_P(DeviceManagementServiceFailedRequestTest, AutoEnrollmentRequest) {
                GetParam().response_);
 }
 
+TEST_P(DeviceManagementServiceFailedRequestTest, AppInstallReportRequest) {
+  EXPECT_CALL(*this, OnJobDone(GetParam().expected_status_, _, _));
+  EXPECT_CALL(*this, OnJobRetry(_)).Times(0);
+  std::unique_ptr<DeviceManagementRequestJob> request_job(
+      StartAppInstallReportJob());
+  net::TestURLFetcher* fetcher = GetFetcher();
+  ASSERT_TRUE(fetcher);
+
+  SendResponse(fetcher, GetParam().error_, GetParam().http_status_,
+               GetParam().response_);
+}
+
 INSTANTIATE_TEST_CASE_P(
     DeviceManagementServiceFailedRequestTestInstance,
     DeviceManagementServiceFailedRequestTest,
@@ -574,6 +601,31 @@ TEST_F(DeviceManagementServiceTest, UnregisterRequest) {
   SendResponse(fetcher, net::OK, 200, response_data);
 }
 
+TEST_F(DeviceManagementServiceTest, AppInstallReportRequest) {
+  em::DeviceManagementResponse expected_response;
+  expected_response.mutable_app_install_report_response();
+  EXPECT_CALL(
+      *this, OnJobDone(DM_STATUS_SUCCESS, _, MessageEquals(expected_response)));
+  EXPECT_CALL(*this, OnJobRetry(_)).Times(0);
+  std::unique_ptr<DeviceManagementRequestJob> request_job(
+      StartAppInstallReportJob());
+  net::TestURLFetcher* fetcher = GetFetcher();
+  ASSERT_TRUE(fetcher);
+
+  CheckURLAndQueryParams(fetcher->GetOriginalURL(),
+                         dm_protocol::kValueRequestAppInstallReport, kClientID,
+                         "");
+
+  std::string expected_data;
+  ASSERT_TRUE(request_job->GetRequest()->SerializeToString(&expected_data));
+  EXPECT_EQ(expected_data, fetcher->upload_data());
+
+  // Generate the response.
+  std::string response_data;
+  ASSERT_TRUE(expected_response.SerializeToString(&response_data));
+  SendResponse(fetcher, net::OK, 200, response_data);
+}
+
 TEST_F(DeviceManagementServiceTest, CancelRegisterRequest) {
   EXPECT_CALL(*this, OnJobDone(_, _, _)).Times(0);
   EXPECT_CALL(*this, OnJobRetry(_)).Times(0);
@@ -634,6 +686,18 @@ TEST_F(DeviceManagementServiceTest, CancelPolicyRequest) {
   request_job.reset();
 }
 
+TEST_F(DeviceManagementServiceTest, CancelAppInstallReportRequest) {
+  EXPECT_CALL(*this, OnJobDone(_, _, _)).Times(0);
+  EXPECT_CALL(*this, OnJobRetry(_)).Times(0);
+  std::unique_ptr<DeviceManagementRequestJob> request_job(
+      StartAppInstallReportJob());
+  net::TestURLFetcher* fetcher = GetFetcher();
+  ASSERT_TRUE(fetcher);
+
+  // There shouldn't be any callbacks.
+  request_job.reset();
+}
+
 TEST_F(DeviceManagementServiceTest, JobQueueing) {
   // Start with a non-initialized service.
   ResetService();