[QuickStart][OOBE] Improve cancellation flow

Improve the cancellation flow of QuickStart in OOBE so that it is
properly handled when the user requests via the cancel button, or
when an error occurs.

- Gracefully handle the case when empty WiFi credentials are received.
- Make the NetworkScreen a QuickStartController::UiDelegate so that it
  can react to errors arising from the phone side.
- Expand and improve browser tests with some cancellation cases.

Test: Manual testing.

Bug: b/306480433
Change-Id: I8fdf4da9e14c41dc1be07c963cc895755ccf4037
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4955934
Reviewed-by: Michael Hansen <hansenmichael@google.com>
Commit-Queue: Renato Silva <rrsilva@google.com>
Cr-Commit-Position: refs/heads/main@{#1216371}

chrome/browser/ash/login/oobe_quick_start/target_device_bootstrap_controller.cc

@@ -97,9 +97,6 @@ void TargetDeviceBootstrapController::StartAdvertisingAndMaybeGetQRCode() {
         TargetDeviceConnectionBroker::FeatureSupportStatus::kSupported);
   CHECK_EQ(status_.step, Step::NONE);
 
-  // No pending requests.
-  CHECK(!weak_ptr_factory_.HasWeakPtrs());
-
   bool use_pin_authentication =
       accessibility_manager_wrapper_->IsSpokenFeedbackEnabled();
 
@@ -301,16 +298,15 @@ void TargetDeviceBootstrapController::AttemptWifiCredentialTransfer() {
 void TargetDeviceBootstrapController::OnWifiCredentialsReceived(
     absl::optional<mojom::WifiCredentials> credentials) {
   CHECK_EQ(status_.step, Step::REQUESTING_WIFI_CREDENTIALS);
-  if (!credentials.has_value()) {
-    status_.step = Step::ERROR;
-    status_.payload = ErrorCode::WIFI_CREDENTIALS_NOT_RECEIVED;
-    NotifyObservers();
-    return;
+
+  if (credentials.has_value()) {
+    status_.step = Step::WIFI_CREDENTIALS_RECEIVED;
+    status_.wifi_credentials = credentials.value();
+  } else {
+    status_.step = Step::EMPTY_WIFI_CREDENTIALS_RECEIVED;
   }
 
-  status_.step = Step::WIFI_CREDENTIALS_RECEIVED;
   status_.payload.emplace<absl::monostate>();
-  status_.wifi_credentials = credentials.value();
   NotifyObservers();
 
   // Record successful wifi credentials transfer. Failures will be
@@ -356,6 +352,11 @@ void TargetDeviceBootstrapController::AttemptGoogleAccountTransfer() {
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
+void TargetDeviceBootstrapController::Cleanup() {
+  status_ = Status();
+  CleanupIfNeeded();
+}
+
 void TargetDeviceBootstrapController::OnChallengeBytesReceived(
     SecondDeviceAuthBroker::ChallengeBytesOrError challenge) {
   if (!challenge.has_value()) {
@@ -434,6 +435,9 @@ std::ostream& operator<<(std::ostream& stream,
     case TargetDeviceBootstrapController::Step::WIFI_CREDENTIALS_RECEIVED:
       stream << "[wifi credentials received]";
       break;
+    case TargetDeviceBootstrapController::Step::EMPTY_WIFI_CREDENTIALS_RECEIVED:
+      stream << "[empty wifi credentials received]";
+      break;
     case TargetDeviceBootstrapController::Step::REQUESTING_GOOGLE_ACCOUNT_INFO:
       stream << "[requesting google account info]";
       break;
@@ -453,4 +457,33 @@ std::ostream& operator<<(std::ostream& stream,
   return stream;
 }
 
+std::ostream& operator<<(
+    std::ostream& stream,
+    const TargetDeviceBootstrapController::ErrorCode& error_code) {
+  switch (error_code) {
+    case TargetDeviceBootstrapController::ErrorCode::START_ADVERTISING_FAILED:
+      stream << "[start advertising failed]";
+      break;
+    case TargetDeviceBootstrapController::ErrorCode::CONNECTION_REJECTED:
+      stream << "[connection rejected]";
+      break;
+    case TargetDeviceBootstrapController::ErrorCode::CONNECTION_CLOSED:
+      stream << "[connection closed]";
+      break;
+    case TargetDeviceBootstrapController::ErrorCode::USER_VERIFICATION_FAILED:
+      stream << "[user verification failed]";
+      break;
+    case TargetDeviceBootstrapController::ErrorCode::
+        GAIA_ASSERTION_NOT_RECEIVED:
+      stream << "[Gaia assertion not received]";
+      break;
+    case TargetDeviceBootstrapController::ErrorCode::
+        FETCHING_CHALLENGE_BYTES_FAILED:
+      stream << "[fetching Challenge Bytes failed]";
+      break;
+  }
+
+  return stream;
+}
+
 }  // namespace ash::quick_start

chrome/browser/ash/login/oobe_quick_start/target_device_bootstrap_controller.h

@@ -40,6 +40,7 @@ class TargetDeviceBootstrapController
     PIN_VERIFICATION,
     CONNECTED,
     REQUESTING_WIFI_CREDENTIALS,
+    EMPTY_WIFI_CREDENTIALS_RECEIVED,
     WIFI_CREDENTIALS_RECEIVED,
     REQUESTING_GOOGLE_ACCOUNT_INFO,
     GOOGLE_ACCOUNT_INFO_RECEIVED,
@@ -51,7 +52,6 @@ class TargetDeviceBootstrapController
     START_ADVERTISING_FAILED,
     CONNECTION_REJECTED,
     CONNECTION_CLOSED,
-    WIFI_CREDENTIALS_NOT_RECEIVED,
     USER_VERIFICATION_FAILED,
     GAIA_ASSERTION_NOT_RECEIVED,
     FETCHING_CHALLENGE_BYTES_FAILED,
@@ -152,6 +152,9 @@ class TargetDeviceBootstrapController
   // the two devices in conjunction with Google servers.
   void AttemptGoogleAccountTransfer();
 
+  // Called when the flow is aborted due to an error, or cancelled by the user.
+  void Cleanup();
+
  private:
   friend class TargetDeviceBootstrapControllerTest;
 

chrome/browser/ash/login/oobe_quick_start/target_device_bootstrap_controller_unittest.cc

@@ -456,9 +456,8 @@ TEST_F(TargetDeviceBootstrapControllerTest,
   fake_target_device_connection_broker_->GetFakeConnection()
       ->SendWifiCredentials(absl::nullopt);
 
-  EXPECT_EQ(fake_observer_->last_status.step, Step::ERROR);
-  EXPECT_EQ(absl::get<ErrorCode>(fake_observer_->last_status.payload),
-            ErrorCode::WIFI_CREDENTIALS_NOT_RECEIVED);
+  EXPECT_EQ(fake_observer_->last_status.step,
+            Step::EMPTY_WIFI_CREDENTIALS_RECEIVED);
 }
 
 TEST_F(TargetDeviceBootstrapControllerTest,

chrome/browser/ash/login/quickstart_controller.cc

@@ -13,7 +13,6 @@
 #include "chrome/browser/ui/webui/ash/login/consumer_update_screen_handler.h"
 #include "chrome/browser/ui/webui/ash/login/gaia_screen_handler.h"
 #include "chrome/browser/ui/webui/ash/login/network_screen_handler.h"
-#include "chrome/browser/ui/webui/ash/login/parental_handoff_screen_handler.h"
 #include "chrome/browser/ui/webui/ash/login/quick_start_screen_handler.h"
 #include "chrome/browser/ui/webui/ash/login/user_creation_screen_handler.h"
 #include "chrome/browser/ui/webui/ash/login/welcome_screen_handler.h"
@@ -111,15 +110,15 @@ void QuickStartController::DetermineEntryPointVisibility(
                      weak_ptr_factory_.GetWeakPtr(), std::move(callback)));
 }
 
-void QuickStartController::HandleFlowCancellationRequest() {
+void QuickStartController::AbortFlow() {
   CHECK(bootstrap_controller_);
   bootstrap_controller_->CloseOpenConnections();
   bootstrap_controller_->StopAdvertising();
   ResetState();
 }
 
 QuickStartController::EntryPoint QuickStartController::GetExitPoint() {
-  return entry_point_.value();
+  return exit_point_.value();
 }
 
 void QuickStartController::InitTargetDeviceBootstrapController() {
@@ -173,20 +172,25 @@ void QuickStartController::OnStatusChanged(
       return;
     }
     case Step::ERROR:
-      NOTIMPLEMENTED();
+      AbortFlow();
+      // Triggers a screen exit if there is a UiDelegate driving the UI.
+      if (!ui_delegates_.empty()) {
+        CHECK(current_screen_ == QuickStartScreenHandler::kScreenId ||
+              current_screen_ == NetworkScreenHandler::kScreenId);
+        ui_delegates_.begin()->OnUiUpdateRequested(UiState::EXIT_SCREEN);
+      }
       return;
     case Step::REQUESTING_WIFI_CREDENTIALS:
       UpdateUiState(UiState::CONNECTING_TO_WIFI);
       quick_start::quick_start_metrics::RecordScreenOpened(
           quick_start_metrics::ScreenName::kConnectingToWifi);
       return;
     case Step::WIFI_CREDENTIALS_RECEIVED:
-      LoginDisplayHost::default_host()
-          ->GetWizardContext()
-          ->quick_start_setup_ongoing = true;
       LoginDisplayHost::default_host()
           ->GetWizardContext()
           ->quick_start_wifi_credentials = status.wifi_credentials;
+      ABSL_FALLTHROUGH_INTENDED;
+    case Step::EMPTY_WIFI_CREDENTIALS_RECEIVED:
       UpdateUiState(UiState::WIFI_CREDENTIALS_RECEIVED);
       return;
     case Step::REQUESTING_GOOGLE_ACCOUNT_INFO:
@@ -267,21 +271,29 @@ void QuickStartController::HandleTransitionToQuickStartScreen() {
 
     // Request advertising to start.
     controller_state_ = ControllerState::INITIALIZING;
+    LoginDisplayHost::default_host()
+        ->GetWizardContext()
+        ->quick_start_setup_ongoing = true;
     bootstrap_controller_->StartAdvertisingAndMaybeGetQRCode();
     CHECK(!entry_point_.has_value()) << "Entry point without ongoing setup";
 
     // Keep track of where the flow originated.
     const auto entry_point = EntryPointFromScreen(previous_screen_.value());
     CHECK(entry_point.has_value()) << "Unknown entry point!";
-    entry_point_ = entry_point;
+    exit_point_ = entry_point_ = entry_point;
   } else {
     // The flow must be resuming after reaching the UserCreation screen. Note
     // the the UserCreationScreen is technically never shown when it switches
     // to QuickStart, so |previous_screen_| is one of the many screens that
     // may have appeared up to this point.
     // TODO(b:283965994) - Imrpve the resume logic.
     CHECK(controller_state_ == ControllerState::CONNECTED);
+    CHECK(LoginDisplayHost::default_host()
+              ->GetWizardContext()
+              ->quick_start_setup_ongoing);
     controller_state_ = ControllerState::CONTINUING_AFTER_ENROLLMENT_CHECKS;
+    // OOBE flow cannot go back after enrollment checks, update exit point.
+    exit_point_ = QuickStartController::EntryPoint::GAIA_SCREEN;
 
     bootstrap_controller_->RequestGoogleAccountInfo();
     UpdateUiState(UiState::TRANSFERRING_GAIA_CREDENTIALS);
@@ -314,6 +326,7 @@ void QuickStartController::ResetState() {
   auto* wizard_context = LoginDisplayHost::default_host()->GetWizardContext();
   wizard_context->quick_start_setup_ongoing = false;
   wizard_context->quick_start_wifi_credentials.reset();
+  bootstrap_controller_->Cleanup();
 }
 
 }  // namespace ash::quick_start

chrome/browser/ash/login/quickstart_controller.h

@@ -57,6 +57,8 @@ class QuickStartController : public OobeUI::Observer,
       WIFI_CREDENTIALS_RECEIVED,
       TRANSFERRING_GAIA_CREDENTIALS,
       SHOWING_FIDO,
+      // Exits the screen.
+      EXIT_SCREEN,
     };
 
     virtual void OnUiUpdateRequested(UiState desired_state) = 0;
@@ -84,8 +86,9 @@ class QuickStartController : public OobeUI::Observer,
   void DetermineEntryPointVisibility(
       EntryPointButtonVisibilityCallback callback);
 
-  // Invoked by the frontend whenever the user cancels the flow.
-  void HandleFlowCancellationRequest();
+  // Invoked by the frontend whenever the user cancels the flow or we encounter
+  // an error.
+  void AbortFlow();
 
   // Whether QuickStart is ongoing and orchestrating the flow.
   bool IsSetupOngoing() {
@@ -147,8 +150,8 @@ class QuickStartController : public OobeUI::Observer,
   // Source of truth of OOBE's current state via OobeUI::Observer
   absl::optional<OobeScreenId> current_screen_, previous_screen_;
 
-  // Bookkeeping where the quick start flow started.
-  absl::optional<EntryPoint> entry_point_;
+  // Bookkeeping where the quick start flow started and ended.
+  absl::optional<EntryPoint> entry_point_, exit_point_;
 
   // Discoverable name to be used on the UI. e.g.: Chromebook (123)
   absl::optional<std::string> discoverable_name_;

chrome/browser/ash/login/screens/network_screen.cc

@@ -149,6 +149,10 @@ void NetworkScreen::HideImpl() {
   connection_timer_.Stop();
 
   UnsubscribeNetworkNotification();
+
+  WizardController::default_controller()
+      ->quick_start_controller()
+      ->DetachFrontend(this);
 }
 
 void NetworkScreen::OnUserAction(const base::Value::List& args) {
@@ -200,6 +204,20 @@ void NetworkScreen::DefaultNetworkChanged(const NetworkState* network) {
   UpdateStatus();
 }
 
+void NetworkScreen::OnUiUpdateRequested(
+    quick_start::QuickStartController::UiState state) {
+  if (state == ash::quick_start::QuickStartController::UiState::EXIT_SCREEN) {
+    // Controller requested the flow to be aborted.
+    WizardController::default_controller()
+        ->quick_start_controller()
+        ->DetachFrontend(this);
+    // Show the standard 'Network List'
+    if (view_) {
+      view_->ShowScreenWithData({});
+    }
+  }
+}
+
 void NetworkScreen::Refresh() {
   continue_pressed_ = false;
   SubscribeNetworkNotification();
@@ -379,8 +397,9 @@ void NetworkScreen::ExitQuickStartFlow() {
   auto* quick_start_controller = LoginDisplayHost::default_host()
                                      ->GetWizardController()
                                      ->quick_start_controller();
+  quick_start_controller->DetachFrontend(this);
   const auto entry_point = quick_start_controller->GetExitPoint();
-  quick_start_controller->HandleFlowCancellationRequest();
+  quick_start_controller->AbortFlow();
   if (entry_point ==
       quick_start::QuickStartController::EntryPoint::NETWORK_SCREEN) {
     // Switches to the screen step that shows the list of networks.
@@ -392,6 +411,11 @@ void NetworkScreen::ExitQuickStartFlow() {
 
 void NetworkScreen::ShowStepsWhenQuickStartOngoing() {
   CHECK(context()->quick_start_setup_ongoing);
+  // Attach frontend so that the QuickStartController may notify us in case the
+  // flow is severed from the phone side, or if an error occurs.
+  WizardController::default_controller()
+      ->quick_start_controller()
+      ->AttachFrontend(this);
 
   if (context()->quick_start_wifi_credentials.has_value()) {
     // QuickStart WiFi Transfer Screen Step

chrome/browser/ash/login/screens/network_screen.h

@@ -14,6 +14,7 @@
 #include "base/scoped_observation.h"
 #include "base/timer/timer.h"
 #include "chrome/browser/ash/login/oobe_quick_start/target_device_bootstrap_controller.h"
+#include "chrome/browser/ash/login/quickstart_controller.h"
 #include "chrome/browser/ash/login/screens/base_screen.h"
 #include "chromeos/ash/components/network/network_state_handler_observer.h"
 #include "chromeos/ash/services/nearby/public/mojom/quick_start_decoder_types.mojom.h"
@@ -31,7 +32,9 @@ class NetworkStateHelper;
 }
 
 // Controls network selection screen shown during OOBE.
-class NetworkScreen : public BaseScreen, public NetworkStateHandlerObserver {
+class NetworkScreen : public BaseScreen,
+                      public NetworkStateHandlerObserver,
+                      public quick_start::QuickStartController::UiDelegate {
  public:
   using TView = NetworkScreenView;
 
@@ -86,6 +89,10 @@ class NetworkScreen : public BaseScreen, public NetworkStateHandlerObserver {
   void NetworkConnectionStateChanged(const NetworkState* network) override;
   void DefaultNetworkChanged(const NetworkState* network) override;
 
+  // quick_start::QuickStartController::UiDelegate:
+  void OnUiUpdateRequested(
+      quick_start::QuickStartController::UiState state) final;
+
   // Subscribes NetworkScreen to the network change notification, forces refresh
   // of current network state.
   void Refresh();

chrome/browser/ash/login/screens/quick_start_screen.cc

@@ -82,7 +82,9 @@ void QuickStartScreen::HideImpl() {
 void QuickStartScreen::OnUserAction(const base::Value::List& args) {
   const std::string& action_id = args[0].GetString();
   if (action_id == kUserActionCancelClicked) {
-    CancelAndExitScreen();
+    controller_->DetachFrontend(this);
+    controller_->AbortFlow();
+    ExitScreen();
   } else {
     BaseScreen::OnUserAction(args);
   }
@@ -120,13 +122,16 @@ void QuickStartScreen::OnUiUpdateRequested(
     case ash::quick_start::QuickStartController::UiState::LOADING:
       // TODO(b:283724988) - Add method to view to show the loading spinner.
       break;
+    case ash::quick_start::QuickStartController::UiState::EXIT_SCREEN:
+      // Controller requested the flow to be aborted.
+      controller_->DetachFrontend(this);
+      ExitScreen();
   }
 }
 
-void QuickStartScreen::CancelAndExitScreen() {
+void QuickStartScreen::ExitScreen() {
   // Get exit point before cancelling the whole flow.
   const auto return_entry_point = controller_->GetExitPoint();
-  controller_->HandleFlowCancellationRequest();
   switch (return_entry_point) {
     case ash::quick_start::QuickStartController::EntryPoint::WELCOME_SCREEN:
       exit_callback_.Run(Result::CANCEL_AND_RETURN_TO_WELCOME);

chrome/browser/ash/login/screens/quick_start_screen.h

@@ -55,7 +55,7 @@ class QuickStartScreen : public BaseScreen,
 
   // Exits the screen and returns to the appropriate entry point. This is called
   // whenever the user aborts the flow, or when an error occurs.
-  void CancelAndExitScreen();
+  void ExitScreen();
 
   base::WeakPtr<TView> view_;
   raw_ptr<quick_start::QuickStartController> controller_;