Chrome OS: Fix SIM PIN change logic.

On Chrome OS, the settings UI supports enabling and disabling SIM
locking on a cellular modem, and changing the SIM PIN. In the current
implementation, all these actions eventually triggers
NetworkingPrivateChromeOS::SetCellularSimState, which then calls the
org.chromium.flimflam.Device.RequirePIN and
org.chromium.flimflam.Device.ChangePIN DBus API exposed by shill.

The "Change Pin" action is available after SIM locking is enabled.
RequirePIN is called once when SIM locking is enabled. After that,
ChangePIN should be used to change the PIN. However,
NetworkingPrivateChromeOS::SetCellularSimState currently handles the
"Change Pin" action by first calling RequirePIN first and then ChangePIN
upon the success completion of RequirePIN. That results in two
successive RequirePIN calls, and the second RequirePIN isn't necessary
and often fails as the modem complains about SIM locking is already
enabled. This CL changes NetworkingPrivateChromeOS::SetCellularSimState
to simply call ChangePIN for the "Change Pin" action.

BUG=b:63803092
BUG=chromium:751792
TEST=Manually tested the following with a few cellular modems:

Under the expanded view of "Settings > Network > Mobile data":

1. Enable and disable SIM locking:
   a. Turn on "Enable SIM locking". A SIM PIN dialog is shown and asks
      for a previously set PIN.
   b. Enter a wrong PIN. The action fails.
   c. Enter the correct PIN. The action succeeds.
   d. Turn off "Enable SIM locking".
   e. Enter a wrong PIN. The action fails.
   f. Enter the correct PIN. The action succeeds.

2. Enable SIM locking, change the PIN, and disable SIM locking:
   a. Turn on "Enable SIM locking". A SIM PIN dialog is shown and asks
      for a previously set PIN.
   b. Enter the correct PIN. The action succeeds.
   c. Click "Change Pin". A SIM PIN dialog is shown and asks for the
      current PIN, the new PIN and the confirmation of the new PIN.
   d. Enter a wrong current PIN. The action fails.
   e. Enter the correct current PIN, but the confirmation of the new PIN
      not matching the new PIN. The action fails.
   f. Enter the correct current PIN, the new PIN and the confirmation of
      the new PIN that matches the new PIN. The action succeeds.
   g. Turn off "Enable SIM locking".
   h. Enter the old PIN. The action fails.
   i. Enter the new PIN. The action succeeds.

3. Enable SIM locking, reboot the system, and enter PIN to unlock the
   SIM:
   a. Turn on "Enable SIM locking". A SIM PIN dialog is shown and asks
      for a previously set PIN.
   b. Enter the correct PIN. The action succeeds.
   c. Reboot the system.
   d. A "SIM card is locked" message is shown under "Mobile data".
   e. Click "Unlock". A SIM PIN dialog is shown and asks for a PIN.
   f. Enter a wrong PIN. The action fails.
   g. Enter the correct PIN. The action succeeds.
   h. The cellular connection can be established after the SIM is unlocked.

Change-Id: Ia095c4243ffd777fcfc822a1dcf2b32003257c78
Reviewed-on: https://chromium-review.googlesource.com/595327Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Ben Chan <benchan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491842}

extensions/browser/api/networking_private/networking_private_api.cc

@@ -128,6 +128,7 @@ namespace networking_private {
 
 // static
 const char kErrorAccessToSharedConfig[] = "Error.CannotChangeSharedConfig";
+const char kErrorInvalidArguments[] = "Error.InvalidArguments";
 const char kErrorInvalidNetworkGuid[] = "Error.InvalidNetworkGuid";
 const char kErrorInvalidNetworkOperation[] = "Error.InvalidNetworkOperation";
 const char kErrorNetworkUnavailable[] = "Error.NetworkUnavailable";

extensions/browser/api/networking_private/networking_private_api.h

@@ -17,6 +17,7 @@ namespace extensions {
 namespace networking_private {
 
 extern const char kErrorAccessToSharedConfig[];
+extern const char kErrorInvalidArguments[];
 extern const char kErrorInvalidNetworkGuid[];
 extern const char kErrorInvalidNetworkOperation[];
 extern const char kErrorNetworkUnavailable[];

extensions/browser/api/networking_private/networking_private_chromeos.cc

@@ -165,24 +165,6 @@ void NetworkHandlerFailureCallback(
   callback.Run(error_name);
 }
 
-void RequirePinSuccess(
-    const std::string& device_path,
-    const std::string& current_pin,
-    const std::string& new_pin,
-    const extensions::NetworkingPrivateChromeOS::VoidCallback& success_callback,
-    const extensions::NetworkingPrivateChromeOS::FailureCallback&
-        failure_callback) {
-  // After RequirePin succeeds, call ChangePIN iff a different new_pin is
-  // provided.
-  if (new_pin.empty() || new_pin == current_pin) {
-    success_callback.Run();
-    return;
-  }
-  NetworkHandler::Get()->network_device_handler()->ChangePin(
-      device_path, current_pin, new_pin, success_callback,
-      base::Bind(&NetworkHandlerFailureCallback, failure_callback));
-}
-
 // Returns the string corresponding to |key|. If the property is a managed
 // dictionary, returns the active value. If the property does not exist or
 // has no active value, returns an empty string.
@@ -696,12 +678,29 @@ void NetworkingPrivateChromeOS::SetCellularSimState(
     return;
   }
 
-  // Only set a new pin if require_pin is true.
-  std::string set_new_pin = require_pin ? new_pin : "";
-  NetworkHandler::Get()->network_device_handler()->RequirePin(
-      device_state->path(), require_pin, current_pin,
-      base::Bind(&RequirePinSuccess, device_state->path(), current_pin,
-                 set_new_pin, success_callback, failure_callback),
+  // TODO(benchan): Add more checks to validate the parameters of this method
+  // and the state of the SIM lock on the cellular device. Consider refactoring
+  // some of the code by moving the logic into shill instead.
+
+  // If |new_pin| is empty, we're trying to enable (require_pin == true) or
+  // disable (require_pin == false) SIM locking.
+  if (new_pin.empty()) {
+    NetworkHandler::Get()->network_device_handler()->RequirePin(
+        device_state->path(), require_pin, current_pin, success_callback,
+        base::Bind(&NetworkHandlerFailureCallback, failure_callback));
+    return;
+  }
+
+  // Otherwise, we're trying to change the PIN from |current_pin| to
+  // |new_pin|, which also requires SIM locking to be enabled, i.e.
+  // require_pin == true.
+  if (!require_pin) {
+    failure_callback.Run(networking_private::kErrorInvalidArguments);
+    return;
+  }
+
+  NetworkHandler::Get()->network_device_handler()->ChangePin(
+      device_state->path(), current_pin, new_pin, success_callback,
       base::Bind(&NetworkHandlerFailureCallback, failure_callback));
 }