Fix incorrect error when cancelling pin request

Previously, the UI could not distinguish between a failed log in attempt
and the user cancelling the attempt. By saving that the user cancelled
the request, we suppress the error that would otherwise be displayed.

Fixed: 1045029
Change-Id: I758d2934db7da7206d13b6036c0723677ada33ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2017351
Commit-Queue: Fabian Sommer <fabiansommer@google.com>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738164}

ash/login/login_screen_controller.cc

@@ -207,6 +207,15 @@ bool LoginScreenController::ValidateParentAccessCode(
   return client_->ValidateParentAccessCode(account_id, code, validation_time);
 }
 
+void LoginScreenController::OnSecurityTokenPinRequestCancelledByUser() {
+  security_token_pin_request_cancelled_ = true;
+  std::move(on_request_security_token_ui_closed_).Run();
+}
+
+bool LoginScreenController::GetSecurityTokenPinRequestCancelled() const {
+  return security_token_pin_request_cancelled_;
+}
+
 void LoginScreenController::HardlockPod(const AccountId& account_id) {
   if (!client_)
     return;
@@ -397,13 +406,24 @@ LoginScreenController::GetScopedGuestButtonBlocker() {
 
 void LoginScreenController::RequestSecurityTokenPin(
     SecurityTokenPinRequest request) {
-  if (!LockScreen::HasInstance()) {
-    // Corner case: the PIN request is made at inappropriate time, racing with
-    // the lock screen showing/hiding.
+  if (LockScreen::HasInstance() && !security_token_pin_request_cancelled_) {
+    // The caller must ensure that there is no unresolved pin request currently
+    // in progress.
+    on_request_security_token_ui_closed_ =
+        std::move(request.pin_ui_closed_callback);
+    // base::Unretained(this) could lead to errors if this controller is
+    // destroyed before the callback happens. This will be fixed by
+    // crbug.com/1001288 by using a UI owned by the controller.
+    request.pin_ui_closed_callback = base::BindOnce(
+        &LoginScreenController::OnSecurityTokenPinRequestCancelledByUser,
+        base::Unretained(this));
+    LockScreen::Get()->RequestSecurityTokenPin(std::move(request));
+  } else {
+    // The user closed the PIN UI on a previous request that was part of the
+    // same smart card login attempt, or the PIN request is made at an
+    // inappropriate time, racing with the lock screen showing/hiding.
     std::move(request.pin_ui_closed_callback).Run();
-    return;
   }
-  LockScreen::Get()->RequestSecurityTokenPin(std::move(request));
 }
 
 void LoginScreenController::ClearSecurityTokenPinRequest() {
@@ -477,6 +497,8 @@ void LoginScreenController::OnAuthenticateComplete(
     bool success) {
   authentication_stage_ = AuthenticationStage::kUserCallback;
   std::move(callback).Run(base::make_optional<bool>(success));
+  security_token_pin_request_cancelled_ = false;
+  on_request_security_token_ui_closed_.Reset();
   authentication_stage_ = AuthenticationStage::kIdle;
 }
 

ash/login/login_screen_controller.h

@@ -73,6 +73,8 @@ class ASH_EXPORT LoginScreenController : public LoginScreen,
   bool ValidateParentAccessCode(const AccountId& account_id,
                                 const std::string& code,
                                 base::Time validation_time);
+  void OnSecurityTokenPinRequestCancelledByUser();
+  bool GetSecurityTokenPinRequestCancelled() const;
   void HardlockPod(const AccountId& account_id);
   void OnFocusPod(const AccountId& account_id);
   void OnNoPodFocused();
@@ -159,6 +161,12 @@ class ASH_EXPORT LoginScreenController : public LoginScreen,
   // If set to false, all auth requests will forcibly fail.
   ForceFailAuth force_fail_auth_for_debug_overlay_ = ForceFailAuth::kOff;
 
+  // Original OnUiClosed callback of a pending security token pin request.
+  // Invoked by OnSecurityTokenPinRequestCancelledByUser.
+  SecurityTokenPinRequest::OnUiClosed on_request_security_token_ui_closed_;
+
+  bool security_token_pin_request_cancelled_ = false;
+
   base::WeakPtrFactory<LoginScreenController> weak_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(LoginScreenController);

ash/login/ui/login_auth_user_view.cc

@@ -1314,7 +1314,18 @@ void LoginAuthUserView::OnChallengeResponseAuthComplete(
   if (!auth_success.has_value() || !auth_success.value()) {
     password_view_->Clear();
     password_view_->SetReadOnly(false);
-    challenge_response_view_->SetState(ChallengeResponseView::State::kFailure);
+    // If the user cancelled the PIN request during ChallengeResponse,
+    // ChallengeResponse will fail with an unknown error. Since this is
+    // expected, we do not show this error.
+    if (Shell::Get()
+            ->login_screen_controller()
+            ->GetSecurityTokenPinRequestCancelled()) {
+      challenge_response_view_->SetState(
+          ChallengeResponseView::State::kInitial);
+    } else {
+      challenge_response_view_->SetState(
+          ChallengeResponseView::State::kFailure);
+    }
   }
 
   on_auth_.Run(auth_success.value_or(false), /*display_error_messages=*/false);

ash/public/cpp/login_types.h

@@ -345,7 +345,8 @@ struct ASH_PUBLIC_EXPORT SecurityTokenPinRequest {
 
   // Called when the PIN request UI gets closed. Will not be called when the
   // browser itself requests the UI to be closed.
-  base::OnceClosure pin_ui_closed_callback;
+  using OnUiClosed = base::OnceClosure;
+  OnUiClosed pin_ui_closed_callback;
 };
 
 }  // namespace ash