Mixed Licenses Enrollment

chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.h"
 
 #include "base/bind.h"
+#include "base/command_line.h"
 #include "base/location.h"
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/browser_process_platform_part.h"
 #include "chrome/browser/chromeos/login/enrollment/enrollment_uma.h"
 #include "chrome/browser/chromeos/login/startup_utils.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/device_cloud_policy_initializer.h"
 #include "chrome/browser/chromeos/policy/enrollment_status_chromeos.h"
 #include "chrome/browser/chromeos/policy/policy_oauth2_token_fetcher.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
+#include "chromeos/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "components/policy/core/common/cloud/cloud_policy_constants.h"
 #include "google_apis/gaia/gaia_auth_consumer.h"
 #include "google_apis/gaia/gaia_auth_fetcher.h"
 #include "google_apis/gaia/gaia_constants.h"
 
 namespace {
 
 // A helper class that takes care of asynchronously revoking a given token.
 class TokenRevoker : public GaiaAuthConsumer {
@@ skipping 121 matching lines @@
       connector->GetEnterpriseDomain() != enrolling_user_domain_) {
     LOG(ERROR) << "Trying to re-enroll to a different domain than "
                << connector->GetEnterpriseDomain();
     UMA(policy::kMetricEnrollmentPrecheckDomainMismatch);
     if (oauth_status_ != OAUTH_NOT_STARTED)
       oauth_status_ = OAUTH_FINISHED;
     status_consumer()->OnOtherError(OTHER_ERROR_DOMAIN_MISMATCH);
     return;
   }
 
+  bool check_license_type = false;
+  if (!enrollment_config_.is_mode_attestation()) {

[emaxx, 2017/07/21 12:57:29]

I'm unaware of what the "attestation-based enrollment" is - could you explain
why the license selection shouldn't happen in this case, and why it should
always happen otherwise?

[Denis Kuznetsov (DE-MUC), 2017/07/25 21:51:05]

Attestation-based enrollment is also called "Zero touch", when user
authenticates once on Android device, and then can mass-enroll ChromeOS devices
using that auth instead of entering credentials on each device. As that is done
to avoid interacting with devices, we don't want to show license selection
dialog here.

[emaxx, 2017/08/01 14:41:39]

Could you please leave this hint as a short comment in the code, e.g.:

// The license selection dialog is not used when doing Zero Touch.

[Denis Kuznetsov (DE-MUC), 2017/08/02 18:19:37]

Done.

+    base::CommandLine* command_line = base::CommandLine::ForCurrentProcess();
+    if (command_line->HasSwitch(
+            chromeos::switches::kEnterpriseEnableLicenseTypeSelection)) {
+      check_license_type = true;
+    }
+  }
+
   connector->ScheduleServiceInitialization(0);
   policy::DeviceCloudPolicyInitializer* dcp_initializer =
       connector->GetDeviceCloudPolicyInitializer();
   CHECK(dcp_initializer);
-  dcp_initializer->StartEnrollment(
+  dcp_initializer->PrepareEnrollment(

[emaxx, 2017/07/21 12:57:29]

Introducing this "half-step" of enrollment preparation obscures the overall
logic around policy initializer.
Would it be possible to pass all the parameters to StartEnrollment() instead -
just as it was before? You'll only have to pass one more additional argument -
optional "license". And won't need to have PrepareEnrollment() and
StartEnrollmentWithLicense().

[Denis Kuznetsov (DE-MUC), 2017/07/25 21:51:05]

Still it makes semantics much more clear - instead of yet another boolean flag
that changes behavior significantly, you have method names that improves code
readability.

[emaxx, 2017/08/01 14:41:39]

Hmm, I'm not sure, maybe we're talking about different things?
I was proposing not a boolean flag that "changes behavior significantly", but
instead passing the license:

void StartEnrollment(
    DeviceManagementService* device_management_service,
    chromeos::ActiveDirectoryJoinDelegate* ad_join_delegate,
    const EnrollmentConfig& enrollment_config,
    const std::string& auth_token,
+   policy::LicenseType license_type,
    const EnrollmentCallback& enrollment_callback);

The benefit is that this would preserve this single point of the enrollment
initiation, instead of spreading this logic over three partial methods.
The semantics would be: if license_type is UNKNOWN, then the old behavior is
selected, otherwise this license is used for the enrollment.
I don't think that those two cases are so much different, that they deserve
separate methods: basically, from what I see, the EnrollmentHandlerChromeOS just
stores this flag and then mostly reuses the general logic.

[Denis Kuznetsov (DE-MUC), 2017/08/02 18:19:37]

Picking the license IS the part of enrollment flow, and it happens after
authorization.
If we decide to keep single StartEnrollment, then we would need to keep all
params required for StartEnrollment call until we complete a check. Instead they
are kept inside dcp_initializer. 

[emaxx, 2017/08/03 18:31:49]

OK.

       connector->device_management_service(), ad_join_delegate_,
       enrollment_config_, token,
       base::Bind(&EnterpriseEnrollmentHelperImpl::OnEnrollmentFinished,
                  weak_ptr_factory_.GetWeakPtr()));
+  if (check_license_type) {
+    dcp_initializer->CheckAvailableLicenses(
+        base::Bind(&EnterpriseEnrollmentHelperImpl::OnMultipleLicensesAvailable,
+                   weak_ptr_factory_.GetWeakPtr()));
+  } else {
+    dcp_initializer->StartEnrollment();
+  }
+}
+
+void EnterpriseEnrollmentHelperImpl::UseLicenseType(policy::LicenseType type) {
+  DCHECK(type != policy::LicenseType::UNKNOWN);
+
+  policy::BrowserPolicyConnectorChromeOS* connector =
+      g_browser_process->platform_part()->browser_policy_connector_chromeos();
+  policy::DeviceCloudPolicyInitializer* dcp_initializer =
+      connector->GetDeviceCloudPolicyInitializer();
+  CHECK(dcp_initializer);
+  dcp_initializer->StartEnrollmentWithLicense(type);
 }
 
 void EnterpriseEnrollmentHelperImpl::GetDeviceAttributeUpdatePermission() {
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Get device attribute update permission";
   policy::BrowserPolicyConnectorChromeOS* connector =
       g_browser_process->platform_part()->browser_policy_connector_chromeos();
   // Don't update device attributes for Active Directory management.
   if (connector->IsActiveDirectoryManaged()) {
     OnDeviceAttributeUpdatePermission(false);
@@ skipping 61 matching lines @@
     oauth_status_ = OAUTH_FINISHED;
   if (status.status() == policy::EnrollmentStatus::SUCCESS) {
     success_ = true;
     StartupUtils::MarkOobeCompleted();
     status_consumer()->OnDeviceEnrolled(additional_token_);
   } else {
     status_consumer()->OnEnrollmentError(status);
   }
 }
 
+void EnterpriseEnrollmentHelperImpl::OnMultipleLicensesAvailable(

[emaxx, 2017/07/21 12:57:29]

nit: Maybe rename this to something like "OnLicenseMapObtained"?
The current name sounds like there are always multiple licenses, not 0 or 1 -
which isn't the case.

[Denis Kuznetsov (DE-MUC), 2017/07/25 21:51:05]

Done.

+    const EnrollmentLicenseMap& licenses) {
+  int count = 0;
+  policy::LicenseType license_type = policy::LicenseType::UNKNOWN;
+  for (auto it = licenses.begin(); it != licenses.end(); it++) {

[emaxx, 2017/07/21 12:57:29]

nit: Use range-based for?

[Denis Kuznetsov (DE-MUC), 2017/07/25 21:51:05]

Done.

+    if (it->second > 0) {
+      count++;
+      license_type = it->first;
+    }
+  }
+  if (count == 0) {
+    // No user license type selection allowed, start usual enrollment.
+    policy::BrowserPolicyConnectorChromeOS* connector =
+        g_browser_process->platform_part()->browser_policy_connector_chromeos();
+    policy::DeviceCloudPolicyInitializer* dcp_initializer =
+        connector->GetDeviceCloudPolicyInitializer();
+    CHECK(dcp_initializer);
+    dcp_initializer->StartEnrollment();
+  } else if (count == 1) {
+    UseLicenseType(license_type);
+  } else {
+    status_consumer()->OnMultipleLicensesAvailable(licenses);
+  }
+}
+
 void EnterpriseEnrollmentHelperImpl::OnDeviceAttributeUpdatePermission(
     bool granted) {
   // TODO(pbond): remove this LOG once http://crbug.com/586961 is fixed.
   LOG(WARNING) << "Device attribute update permission granted=" << granted;
   status_consumer()->OnDeviceAttributeUpdatePermission(granted);
 }
 
 void EnterpriseEnrollmentHelperImpl::OnDeviceAttributeUploadCompleted(
     bool success) {
   status_consumer()->OnDeviceAttributeUploadCompleted(success);
@@ skipping 180 matching lines @@
   EnrollmentUMA(sample, enrollment_config_.mode);
 }
 
 void EnterpriseEnrollmentHelperImpl::OnSigninProfileCleared(
     const base::Closure& callback) {
   oauth_data_cleared_ = true;
   callback.Run();
 }
 
 }  // namespace chromeos