Hide Apps button after a user session has started

Previously, during the post login steps the Apps button stays visible,
and starting an app crashes the recently started user session.

This is fixed by hiding the Apps button in all post login dialogs.
Hide Apps button after a user session has started

Bug: 1124319
Change-Id: Ie1c1f72c4dd473b928c3f93dc5fbeaf8f03cb30e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2407312
Commit-Queue: Ossama Mahmoud <osamafathy@google.com>
Reviewed-by: Roman Sorokin [CET] <rsorokin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813709}

ash/login/login_screen_controller.cc

@@ -320,11 +320,11 @@ void LoginScreenController::EnableShutdownButton(bool enable) {
       ->SetShutdownButtonEnabled(enable);
 }
 
-void LoginScreenController::ShowGuestButtonInOobe(bool show) {
+void LoginScreenController::SetIsFirstSigninStep(bool is_first) {
   Shelf::ForWindow(Shell::Get()->GetPrimaryRootWindow())
       ->shelf_widget()
       ->login_shelf_view()
-      ->ShowGuestButtonInOobe(show);
+      ->SetIsFirstSigninStep(is_first);
 }
 
 void LoginScreenController::ShowParentAccessButton(bool show) {

ash/login/login_screen_controller.h

@@ -112,7 +112,7 @@ class ASH_EXPORT LoginScreenController : public LoginScreen,
   bool IsReadyForPassword() override;
   void EnableAddUserButton(bool enable) override;
   void EnableShutdownButton(bool enable) override;
-  void ShowGuestButtonInOobe(bool show) override;
+  void SetIsFirstSigninStep(bool is_first) override;
   void ShowParentAccessButton(bool show) override;
   void SetAllowLoginAsGuest(bool allow_guest) override;
   std::unique_ptr<ScopedGuestButtonBlocker> GetScopedGuestButtonBlocker()

ash/public/cpp/login_screen.h

@@ -51,8 +51,9 @@ class ASH_PUBLIC_EXPORT LoginScreen {
   // Sets whether shutdown button is enabled in the login screen.
   virtual void EnableShutdownButton(bool enable) = 0;
 
-  // Shows or hides the guest button on the login shelf during OOBE.
-  virtual void ShowGuestButtonInOobe(bool show) = 0;
+  // Used to show or hide apps the guest and buttons on the login shelf during
+  // OOBE.
+  virtual void SetIsFirstSigninStep(bool is_first) = 0;
 
   // Shows or hides the parent access button on the login shelf.
   virtual void ShowParentAccessButton(bool show) = 0;

ash/shelf/login_shelf_view.cc

@@ -275,12 +275,6 @@ void StartAddUser() {
       EmptyAccountId() /*prefilled_account*/);
 }
 
-bool DialogStateGuestAllowed(OobeDialogState state) {
-  return state == OobeDialogState::GAIA_SIGNIN ||
-         state == OobeDialogState::ERROR || state == OobeDialogState::HIDDEN ||
-         state == OobeDialogState::USER_CREATION;
-}
-
 bool ShutdownButtonHidden(OobeDialogState state) {
   return state == OobeDialogState::MIGRATION ||
          state == OobeDialogState::ENROLLMENT ||
@@ -668,8 +662,8 @@ void LoginShelfView::ShowParentAccessButton(bool show) {
   UpdateUi();
 }
 
-void LoginShelfView::ShowGuestButtonInOobe(bool show) {
-  allow_guest_in_oobe_ = show;
+void LoginShelfView::SetIsFirstSigninStep(bool is_first) {
+  is_first_signin_step_ = is_first;
   UpdateUi();
 }
 
@@ -802,13 +796,9 @@ void LoginShelfView::UpdateUi() {
   // visible. The button should not appear if the device is not connected to a
   // network.
   GetViewByID(kAddUser)->SetVisible(!dialog_visible && is_login_primary);
-  // Show kiosk apps button if:
-  // 1. It's in login screen.
-  // 2. There are Kiosk apps available.
-  // 3. Device is not currently blocked.
   kiosk_apps_button_->SetVisible(kiosk_apps_button_->HasApps() &&
-                                 (is_login_primary || is_oobe) &&
-                                 (dialog_state_ != OobeDialogState::BLOCKING));
+                                 ShouldShowAppsButton());
+
   // If there is no visible (and thus focusable) buttons, we shouldn't focus
   // LoginShelfView. We update it here, so we don't need to check visibility
   // every time we move focus to system tray.
@@ -866,6 +856,22 @@ void LoginShelfView::UpdateButtonUnionBounds() {
   }
 }
 
+bool LoginShelfView::ShouldShowGuestAndAppsButtons() const {
+  bool dialog_state_allowed = false;
+  if (dialog_state_ == OobeDialogState::USER_CREATION ||
+      dialog_state_ == OobeDialogState::GAIA_SIGNIN) {
+    dialog_state_allowed = !login_screen_has_users_ && is_first_signin_step_;
+  } else if (dialog_state_ == OobeDialogState::ERROR ||
+             dialog_state_ == OobeDialogState::HIDDEN) {
+    dialog_state_allowed = true;
+  }
+
+  const bool user_session_started =
+      Shell::Get()->session_controller()->NumberOfLoggedInUsers() != 0;
+
+  return dialog_state_allowed && !user_session_started;
+}
+
 // Show guest button if:
 // 1. Guest login is allowed.
 // 2. OOBE UI dialog is currently showing the login UI or error.
@@ -885,27 +891,18 @@ bool LoginShelfView::ShouldShowGuestButton() const {
   if (scoped_guest_button_blockers_ > 0)
     return false;
 
-  if (!DialogStateGuestAllowed(dialog_state_))
-    return false;
-
-  const bool user_session_started =
-      Shell::Get()->session_controller()->NumberOfLoggedInUsers() != 0;
-  if (user_session_started)
+  if (!ShouldShowGuestAndAppsButtons())
     return false;
 
   const SessionState session_state =
       Shell::Get()->session_controller()->GetSessionState();
 
   if (session_state == SessionState::OOBE)
-    return allow_guest_in_oobe_;
+    return is_first_signin_step_;
 
   if (session_state != SessionState::LOGIN_PRIMARY)
     return false;
 
-  if (dialog_state_ == OobeDialogState::USER_CREATION ||
-      dialog_state_ == OobeDialogState::GAIA_SIGNIN)
-    return !login_screen_has_users_ && allow_guest_in_oobe_;
-
   return true;
 }
 
@@ -916,4 +913,16 @@ bool LoginShelfView::ShouldShowEnterpriseEnrollmentButton() const {
          dialog_state_ == OobeDialogState::USER_CREATION;
 }
 
+bool LoginShelfView::ShouldShowAppsButton() const {
+  if (!ShouldShowGuestAndAppsButtons())
+    return false;
+
+  const SessionState session_state =
+      Shell::Get()->session_controller()->GetSessionState();
+  if (session_state != SessionState::LOGIN_PRIMARY)
+    return false;
+
+  return true;
+}
+
 }  // namespace ash

ash/shelf/login_shelf_view.h

@@ -93,9 +93,9 @@ class ASH_EXPORT LoginShelfView : public views::View,
   // Sets whether parent access button can be shown on the login shelf.
   void ShowParentAccessButton(bool show);
 
-  // Sets if the guest button on the login shelf can be shown during gaia
-  // signin screen.
-  void ShowGuestButtonInOobe(bool show);
+  // Sets if the guest button and apps button on the login shelf can be
+  // shown during gaia signin screen.
+  void SetIsFirstSigninStep(bool is_first);
 
   // Sets whether users can be added from the login screen.
   void SetAddUserButtonEnabled(bool enable_add_user);
@@ -172,9 +172,13 @@ class ASH_EXPORT LoginShelfView : public views::View,
 
   bool ShouldShowEnterpriseEnrollmentButton() const;
 
+  bool ShouldShowAppsButton() const;
+
+  bool ShouldShowGuestAndAppsButtons() const;
+
   OobeDialogState dialog_state_ = OobeDialogState::HIDDEN;
   bool allow_guest_ = true;
-  bool allow_guest_in_oobe_ = false;
+  bool is_first_signin_step_ = false;
   bool show_parent_access_ = false;
   // When the Gaia screen is active during Login, the guest-login button should
   // appear if there are no user views.

ash/shelf/login_shelf_view_unittest.cc

@@ -239,24 +239,39 @@ TEST_F(LoginShelfViewTest, ShouldUpdateUiBasedOnShutdownPolicyInActiveSession) {
       AreButtonsInOrder(LoginShelfView::kRestart, LoginShelfView::kSignOut));
 }
 
+// Checks that the Apps button is hidden if a session has started
+TEST_F(LoginShelfViewTest, ShouldNotShowAppsButtonAfterSessionStarted) {
+  NotifySessionStateChanged(SessionState::LOGIN_PRIMARY);
+
+  std::vector<KioskAppMenuEntry> kiosk_apps(1);
+  login_shelf_view_->SetKioskApps(kiosk_apps, {}, {});
+  EXPECT_TRUE(
+      login_shelf_view_->GetViewByID(LoginShelfView::kApps)->GetVisible());
+
+  CreateUserSessions(1);
+  EXPECT_FALSE(
+      login_shelf_view_->GetViewByID(LoginShelfView::kApps)->GetVisible());
+}
+
 // Checks that the shutdown or restart buttons shown before the Apps button when
-// kiosk mode is enabled at GAIA_SIGNIN state
+// kiosk mode is enabled
 TEST_F(LoginShelfViewTest, ShouldShowShutdownOrRestartButtonsBeforeApps){
   NotifySessionStateChanged(SessionState::LOGIN_PRIMARY);
 
   std::vector<KioskAppMenuEntry> kiosk_apps(1);
   login_shelf_view_->SetKioskApps(kiosk_apps, {}, {});
-  login_shelf_view_->SetLoginDialogState(OobeDialogState::GAIA_SIGNIN);
 
   // |reboot_on_shutdown| is initially off
-  EXPECT_TRUE(
-      ShowsShelfButtons({LoginShelfView::kShutdown, LoginShelfView::kApps}));
+  EXPECT_TRUE(ShowsShelfButtons(
+      {LoginShelfView::kShutdown, LoginShelfView::kBrowseAsGuest,
+       LoginShelfView::kAddUser, LoginShelfView::kApps}));
   EXPECT_TRUE(
       AreButtonsInOrder(LoginShelfView::kShutdown, LoginShelfView::kApps));
 
   NotifyShutdownPolicyChanged(true /*reboot_on_shutdown*/);
-  EXPECT_TRUE(
-      ShowsShelfButtons({LoginShelfView::kRestart, LoginShelfView::kApps}));
+  EXPECT_TRUE(ShowsShelfButtons(
+      {LoginShelfView::kRestart, LoginShelfView::kBrowseAsGuest,
+       LoginShelfView::kAddUser, LoginShelfView::kApps}));
   EXPECT_TRUE(
       AreButtonsInOrder(LoginShelfView::kRestart, LoginShelfView::kApps));
 }
@@ -394,23 +409,18 @@ TEST_F(LoginShelfViewTest, ShouldUpdateUiAfterDialogStateChange) {
   login_shelf_view_->SetAllowLoginAsGuest(false /*allow_guest*/);
   EXPECT_TRUE(ShowsShelfButtons({LoginShelfView::kShutdown}));
 
-  // Kiosk app button is visible when dialog state == OobeDialogState::HIDDEN
-  // or GAIA_SIGNIN.
+  // By default apps button is hidden during gaia sign in
   login_shelf_view_->SetLoginDialogState(OobeDialogState::GAIA_SIGNIN);
   std::vector<KioskAppMenuEntry> kiosk_apps(1);
   login_shelf_view_->SetKioskApps(kiosk_apps, {}, {});
-  EXPECT_TRUE(
-      ShowsShelfButtons({LoginShelfView::kShutdown, LoginShelfView::kApps}));
-  EXPECT_TRUE(
-      AreButtonsInOrder(LoginShelfView::kShutdown, LoginShelfView::kApps));
+  EXPECT_TRUE(ShowsShelfButtons({LoginShelfView::kShutdown}));
 
+  // Apps button is hidden during SAML_PASSWORD_CONFIRM STATE
   login_shelf_view_->SetLoginDialogState(
       OobeDialogState::SAML_PASSWORD_CONFIRM);
-  EXPECT_TRUE(
-      ShowsShelfButtons({LoginShelfView::kShutdown, LoginShelfView::kApps}));
-  EXPECT_TRUE(
-      AreButtonsInOrder(LoginShelfView::kShutdown, LoginShelfView::kApps));
+  EXPECT_TRUE(ShowsShelfButtons({LoginShelfView::kShutdown}));
 
+  // Kiosk apps button is visible when dialog state == OobeDialogState::HIDDEN
   login_shelf_view_->SetLoginDialogState(OobeDialogState::HIDDEN);
   EXPECT_TRUE(
       ShowsShelfButtons({LoginShelfView::kShutdown, LoginShelfView::kAddUser,
@@ -450,7 +460,7 @@ TEST_F(LoginShelfViewTest, ShouldUpdateUiAfterDialogStateChange) {
 
 TEST_F(LoginShelfViewTest, ShouldShowGuestButtonWhenNoUserPods) {
   login_shelf_view_->SetAllowLoginAsGuest(/*allow_guest=*/true);
-  login_shelf_view_->ShowGuestButtonInOobe(/*show=*/true);
+  login_shelf_view_->SetIsFirstSigninStep(/*is_first=*/true);
   SetUserCount(0);
 
   NotifySessionStateChanged(SessionState::LOGIN_PRIMARY);

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

@@ -822,10 +822,10 @@ IN_PROC_BROWSER_TEST_P(OobeGuestButtonPolicy, VisibilityAfterEnrollment) {
             user_manager::UserManager::Get()->IsGuestSessionAllowed());
   EXPECT_EQ(GetParam(), ash::LoginScreenTestApi::IsGuestButtonShown());
 
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [false]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [false]);");
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [true]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [true]);");
   EXPECT_EQ(GetParam(), ash::LoginScreenTestApi::IsGuestButtonShown());
 }
 

chrome/browser/chromeos/login/login_screen_policy_browsertest.cc

@@ -104,16 +104,16 @@ IN_PROC_BROWSER_TEST_F(LoginScreenGuestButtonPolicyTest, NoUsers) {
   EXPECT_TRUE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
   // When there are no users - should be the same as OOBE.
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [false]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [false]);");
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [true]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [true]);");
   EXPECT_TRUE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
   SetGuestModePolicy(false);
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [true]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [true]);");
   // Should not affect.
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
@@ -131,13 +131,13 @@ IN_PROC_BROWSER_TEST_F(LoginScreenGuestButtonPolicyTest, HasUsers) {
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
   // Should not affect.
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [true]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [true]);");
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
   ash::LoginScreen::Get()->GetModel()->SetUserList({});
   EXPECT_TRUE(ash::LoginScreenTestApi::IsGuestButtonShown());
 
-  test::ExecuteOobeJS("chrome.send('showGuestInOobe', [false]);");
+  test::ExecuteOobeJS("chrome.send('setIsFirstSigninStep', [false]);");
   EXPECT_FALSE(ash::LoginScreenTestApi::IsGuestButtonShown());
 }
 

chrome/browser/chromeos/login/screens/user_creation_screen.cc

@@ -88,7 +88,7 @@ void UserCreationScreen::ShowImpl() {
       ScopedObserver<NetworkStateInformer, NetworkStateInformerObserver>>(this);
   scoped_observer_->Add(network_state_informer_.get());
 
-  ash::LoginScreen::Get()->ShowGuestButtonInOobe(true);
+  ash::LoginScreen::Get()->SetIsFirstSigninStep(true);
 
   // Back button is only available in login screen (add user flow) which is
   // indicated by if the device has users. Back button is hidden in the oobe

chrome/browser/resources/chromeos/login/screen_gaia_signin.js

@@ -459,15 +459,14 @@ Polymer({
   },
 
   /**
-   * Updates whether the Guest button is allowed to be shown. (Note that the
-   * C++ side contains additional logic that decides whether the Guest button
-   * should be shown.)
+   * Updates whether the Guest and Apps button is allowed to be shown. (Note
+   * that the C++ side contains additional logic that decides whether the
+   * Guest button should be shown.)
    * @private
    */
-  updateGuestButtonVisibility_() {
-    let showGuestInOobe = !this.isClosable_() && this.isAtTheBeginning_();
-    // TODO(rsorokin): Rename message string to reflect the meaning.
-    chrome.send('showGuestInOobe', [showGuestInOobe]);
+  updateButtonsVisibilityAtFirstSigingStep_() {
+    let isFristSigninStep = !this.isClosable_() && this.isAtTheBeginning_();
+    chrome.send('setIsFirstSigninStep', [isFristSigninStep]);
   },
 
   /**
@@ -545,7 +544,7 @@ Polymer({
       return;
     }
     chrome.send('updateOfflineLogin', [this.isOffline_()]);
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
   },
 
   /**
@@ -713,7 +712,7 @@ Polymer({
     this.navigationEnabled_ = true;
 
     this.lastBackMessageValue_ = false;
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
 
     cr.ui.login.invokePolymerMethod(
         this.$['signin-frame-dialog'], 'onBeforeShow');
@@ -823,7 +822,7 @@ Polymer({
         this.loadingFrameContents_ = false;
         break;
     }
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
     chrome.send('authExtensionLoaded');
   },
 
@@ -916,7 +915,7 @@ Polymer({
         Oobe.getInstance().updateScreenSize(this);
       }
 
-      this.updateGuestButtonVisibility_();
+      this.updateButtonsVisibilityAtFirstSigingStep_();
     }
   },
 
@@ -987,7 +986,7 @@ Polymer({
   onBackButton_(e) {
     this.getActiveFrame_().focus();
     this.lastBackMessageValue_ = !!e.detail;
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
   },
   /**
    * Invoked when the auth host emits 'setPrimaryActionEnabled'  event
@@ -1219,7 +1218,7 @@ Polymer({
 
     this.clearVideoTimer_();
     this.authCompleted_ = true;
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
   },
 
   /**
@@ -1284,7 +1283,7 @@ Polymer({
     this.startLoadingTimer_();
     this.lastBackMessageValue_ = false;
     this.authCompleted_ = false;
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
   },
 
   /**
@@ -1387,7 +1386,7 @@ Polymer({
     else
       Oobe.showSigninUI();
 
-    this.updateGuestButtonVisibility_();
+    this.updateButtonsVisibilityAtFirstSigingStep_();
   },
 
   /**

chrome/browser/ui/ash/test_login_screen.cc

@@ -43,7 +43,7 @@ void TestLoginScreen::EnableAddUserButton(bool enable) {}
 
 void TestLoginScreen::EnableShutdownButton(bool enable) {}
 
-void TestLoginScreen::ShowGuestButtonInOobe(bool show) {}
+void TestLoginScreen::SetIsFirstSigninStep(bool is_first) {}
 
 void TestLoginScreen::ShowParentAccessButton(bool show) {}
 

chrome/browser/ui/ash/test_login_screen.h

@@ -33,7 +33,7 @@ class TestLoginScreen : public ash::LoginScreen {
   bool IsReadyForPassword() override;
   void EnableAddUserButton(bool enable) override;
   void EnableShutdownButton(bool enable) override;
-  void ShowGuestButtonInOobe(bool show) override;
+  void SetIsFirstSigninStep(bool is_first) override;
   void ShowParentAccessButton(bool show) override;
   void SetAllowLoginAsGuest(bool allow_guest) override;
   std::unique_ptr<ash::ScopedGuestButtonBlocker> GetScopedGuestButtonBlocker()

chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc

@@ -723,7 +723,8 @@ void GaiaScreenHandler::RegisterMessages() {
   AddRawCallback("showAddUser", &GaiaScreenHandler::HandleShowAddUser);
   AddCallback("getIsSamlUserPasswordless",
               &GaiaScreenHandler::HandleGetIsSamlUserPasswordless);
-  AddCallback("showGuestInOobe", &GaiaScreenHandler::HandleShowGuestInOobe);
+  AddCallback("setIsFirstSigninStep",
+              &GaiaScreenHandler::HandleIsFirstSigninStep);
   AddCallback("samlStateChanged", &GaiaScreenHandler::HandleSamlStateChanged);
   AddCallback("securityTokenPinEntered",
               &GaiaScreenHandler::HandleSecurityTokenPinEntered);
@@ -1031,8 +1032,8 @@ void GaiaScreenHandler::HandleGetIsSamlUserPasswordless(
                             base::Value(is_saml_user_passwordless));
 }
 
-void GaiaScreenHandler::HandleShowGuestInOobe(bool show) {
-  ash::LoginScreen::Get()->ShowGuestButtonInOobe(show);
+void GaiaScreenHandler::HandleIsFirstSigninStep(bool is_first) {
+  ash::LoginScreen::Get()->SetIsFirstSigninStep(is_first);
 }
 
 void GaiaScreenHandler::HandleSamlStateChanged(bool is_saml) {

chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h

@@ -241,9 +241,9 @@ class GaiaScreenHandler : public BaseScreenHandler,
                                        const std::string& typed_email,
                                        const std::string& gaia_id);
 
-  // Allows WebUI to control the login shelf's guest button visibility during
-  // OOBE.
-  void HandleShowGuestInOobe(bool show);
+  // Allows WebUI to control the login shelf's guest and apps buttons visibility
+  // during OOBE.
+  void HandleIsFirstSigninStep(bool is_first);
 
   // Called to notify whether the SAML sign-in is currently happening.
   void HandleSamlStateChanged(bool is_saml);

chrome/browser/ui/webui/chromeos/login/supervision_transition_screen_handler.cc

@@ -106,7 +106,7 @@ void SupervisionTransitionScreenHandler::Show() {
   SystemTrayClient::Get()->SetPrimaryTrayEnabled(false);
   ash::LoginScreen::Get()->EnableShutdownButton(false);
   ash::LoginScreen::Get()->SetAllowLoginAsGuest(false);
-  ash::LoginScreen::Get()->ShowGuestButtonInOobe(false);
+  ash::LoginScreen::Get()->SetIsFirstSigninStep(false);
 
   base::DictionaryValue data;
   data.SetBoolean("isRemovingSupervision",