Mixed Licenses Enrollment

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

Index: chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
diff --git a/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc b/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
index 747c21fae520d630629ad4850516f9746e2f396e..8f3783ace9fd978c42dac9c5d78e497060753a49 100644
--- a/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
+++ b/chrome/browser/chromeos/login/enrollment/enterprise_enrollment_helper_impl.cc
@@ -5,6 +5,7 @@
 #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"
@@ -19,6 +20,7 @@
 #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"
@@ -160,15 +162,42 @@ void EnterpriseEnrollmentHelperImpl::DoEnroll(const std::string& token) {
     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() {
@@ -250,6 +279,31 @@ void EnterpriseEnrollmentHelperImpl::OnEnrollmentFinished(
   }
 }
 
+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.