Update FWMP in TPM

chrome/browser/chromeos/login/enrollment/auto_enrollment_controller.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/auto_enrollment_controller.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h"
 #include "chrome/browser/chromeos/policy/server_backed_state_keys_broker.h"
+#include "chrome/browser/chromeos/settings/install_attributes.h"
 #include "chromeos/chromeos_switches.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/system/statistics_provider.h"
 #include "components/policy/core/common/cloud/device_management_service.h"
 #include "net/url_request/url_request_context_getter.h"
 
 namespace chromeos {
 
 namespace {
 
 // Maximum time to wait before forcing a decision.  Note that download time for
 // state key buckets can be non-negligible, especially on 2G connections.
@@ skipping 258 matching lines @@
   state_ = new_state;
 
   // Stop the safeguard timer once a result comes in.
   switch (state_) {
     case policy::AUTO_ENROLLMENT_STATE_IDLE:
     case policy::AUTO_ENROLLMENT_STATE_PENDING:
       break;
     case policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR:
     case policy::AUTO_ENROLLMENT_STATE_SERVER_ERROR:
     case policy::AUTO_ENROLLMENT_STATE_TRIGGER_ENROLLMENT:
-    case policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT:
       safeguard_timer_.Stop();
       break;
+    case policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT:
+      RemoveFwmp();

[Andrew T Wilson (Slow), 2017/03/07 11:16:30]

Document that RemoveFwmp() will handle calling the various callbacks and
stopping safeguard_timer_ since it's not documented that these are side effects.
What happens if the safeguard_timer_ triggers while we are in the middle of
removing FWMP?

[igorcov, 2017/03/09 12:22:56]

It would possibly request to remove FWMP twice, which is not nice. I've fixed
now the behavior to catch this case. Thanks.

+      return;
   }
 
   progress_callbacks_.Notify(state_);
 }
 
+void AutoEnrollmentController::RemoveFwmp() {
+  DCHECK(state_ == policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);

[Daniel Erat, 2017/03/06 21:18:25]

use DCHECK_EQ so the actual value is logged on failure

[Andrew T Wilson (Slow), 2017/03/07 11:16:30]

DCHECK_EQ

[igorcov, 2017/03/09 12:22:56]

Done.

+
+  std::unique_ptr<chromeos::InstallAttributes> install_attributes =

[Daniel Erat, 2017/03/06 21:18:25]

nit: auto (since the type is obvious from the MakeUnique call)

+      base::MakeUnique<chromeos::InstallAttributes>(

[Andrew T Wilson (Slow), 2017/03/07 11:16:30]

This is something Thiemo should look at - I don't know the right pattern for
using InstallAttributes (are we really supposed to free it right after calling?)
so I can't check if it's OK.

[igorcov, 2017/03/09 12:22:56]

I've changed this to use directly the CryptohomeClient.

+          chromeos::DBusThreadManager::Get()->GetCryptohomeClient());
+  install_attributes->RemoveFwmpInTpm(
+      base::Bind(&AutoEnrollmentController::OnFwmpRemoved,
+                 client_start_weak_factory_.GetWeakPtr()));

[Andrew T Wilson (Slow), 2017/03/07 11:16:30]

Why do we only call progress callbacks and stop the safeguard timer in
OnFwmpRemoved, instead of here? We should explain this somewhere.

[igorcov, 2017/03/09 12:22:56]

Included in the comments of RemoveFirmwareManagementParameters() function.
Thanks.

+}
+
+void AutoEnrollmentController::OnFwmpRemoved(
+    chromeos::DBusMethodCallStatus call_status,
+    bool result,
+    const cryptohome::BaseReply& reply) {
+  if (!result)
+    LOG(ERROR) << "Failed to remove firmware management parameters";

[Andrew T Wilson (Slow), 2017/03/07 11:16:30]

Any other info we can log here (does it return a more detailed error?)

[igorcov, 2017/03/09 12:22:56]

Added the reply error. We can't use much from call_status as that is a two value
enum (success/fail). Hopefully reply error has more details, but normally
cryptohomed logs details about the failed operation.

+  safeguard_timer_.Stop();
+  progress_callbacks_.Notify(state_);
+}
+
 void AutoEnrollmentController::Timeout() {
   // TODO(mnissler): Add UMA to track results of auto-enrollment checks.
   if (client_start_weak_factory_.HasWeakPtrs() &&
       fre_requirement_ != EXPLICITLY_REQUIRED) {
     // If the callbacks to check ownership status or state keys are still
     // pending, there's a bug in the code running on the device. No use in
     // retrying anything, need to fix that bug.
     LOG(ERROR) << "Failed to start auto-enrollment check, fix the code!";
     UpdateState(policy::AUTO_ENROLLMENT_STATE_NO_ENROLLMENT);
   } else {
     // This can actually happen in some cases, for example when state key
     // generation is waiting for time sync or the server just doesn't reply and
     // keeps the connection open.
     LOG(ERROR) << "AutoEnrollmentClient didn't complete within time limit.";
     UpdateState(policy::AUTO_ENROLLMENT_STATE_CONNECTION_ERROR);
   }
 
   // Reset state.
   Cancel();
 }
 
 }  // namespace chromeos