Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc

+// Copyright 2016 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/certificate_provider/pin_dialog_manager.h"
+
+#include "ash/shell.h"
+#include "chrome/browser/chromeos/login/ui/login_display_host.h"
+#include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/ui/browser_finder.h"
+#include "chrome/browser/ui/browser_window.h"
+#include "ui/views/widget/widget.h"
+#include "ui/views/window/dialog_client_view.h"
+
+namespace chromeos {
+
+// Define timeout for issued sign_request_id.
+const uint64_t SIGN_REQUEST_ID_TIMEOUT = 10 * 60;  // 10 minutes
+
+// The limit of the sign_request_id map size, after which the sanitizing process

[emaxx, 2016/09/06 15:02:10]

This looks a bit hacky, and may be not needed at all if the code would be
changed to use the storage in the SignRequests class (which always has the
up-to-date information regarding the currently running requests).

[igorcov, 2016/09/07 09:12:28]

Implemented the same way the storage is in SignRequests class. Now besides
the ID, the extension_id it was generated for is also kept in the map. And
the IDs are disposed from the map only when the extension is unloaded.

+// will be applied to remove expired entries.
+const int MAX_SIGN_REQUESTS_MAP_SIZE = 10;
+
+PinDialogManager::PinDialogManager() : weak_factory_(this) {}
+
+PinDialogManager::~PinDialogManager() {}
+
+void PinDialogManager::OnPinDialogInput(const std::string& extension_id,
+                                        const bool closed) {
+  last_response_closed_[extension_id] = closed;
+  if (closed) {
+    // |active_pin_dialog_| is managed by |active_window_|. This local copy of
+    // the pointer is reset here to allow a new dialog to be created when a new
+    // request comes.
+    active_pin_dialog_ = nullptr;
+    return;
+  }
+
+  // Set the temporary callback to be called if user closes the dialog while
+  // request is processing by the extension.
+  active_pin_dialog_->SetCallback(
+      base::Bind(&PinDialogManager::OnFlowInterrupted, base::Unretained(this)));

[stevenjb, 2016/08/11 18:12:02]

base::Unretained is unsafe here. The view (dialog) is not owned by this class,
so on shutdown this could be destroyed while a callback is queued, creating an
unlikely but subtle shutdown crash.

Also see my other comment, the notion of a 'temporary callback' is confusing.
What happens to the original callback and why do we no longer care about it?

[igorcov, 2016/09/06 13:22:02]

Included in destructor of the class to close the active dialog if present.

I've also implemented your suggestion to have two separate callbacks, and
changed
code so that temporary callback is always called.

+}
+
+void PinDialogManager::OnFlowInterrupted(const base::string16& value) {
+  DCHECK(value.empty());
+  OnPinDialogInput(active_dialog_extension_id_, true);
+}
+
+bool PinDialogManager::LastPinDialogClosed(const std::string& extension_id) {
+  return last_response_closed_[extension_id];
+}
+
+bool PinDialogManager::UpdatePinDialog(
+    const std::string& extension_id,
+    RequestPinErrorType error_type,
+    const bool accept_input,
+    const RequestPinView::RequestPinCallback& callback) {
+  if (active_pin_dialog_ == nullptr) {
+    return false;
+  }
+
+  if (extension_id != active_dialog_extension_id_ ||
+      !active_pin_dialog_->IsLocked()) {
+    return false;
+  }
+
+  active_pin_dialog_->SetCallback(callback);
+  active_pin_dialog_->SetDialogParameters(RequestPinCodeType::UNCHANGED,
+                                          error_type, nullptr, accept_input);
+  active_pin_dialog_->GetDialogClientView()->UpdateDialogButtons();
+  return true;
+}
+
+RequestPinResponse PinDialogManager::ShowPinDialog(
+    const std::string& extension_id,
+    const std::string& extension_name,
+    const long long sign_request_id,
+    RequestPinCodeType code_type,
+    RequestPinErrorType error_type,
+    std::unique_ptr<int> attempts_left,
+    const RequestPinView::RequestPinCallback& callback) {
+  const bool accept_input = (!attempts_left || *(attempts_left.get()) > 0);
+  // If active dialog exists already, we need to make sure it belongs to the
+  // same extension and the user submitted some input.
+  if (active_pin_dialog_ != nullptr) {
+    DCHECK(!active_dialog_extension_id_.empty());
+    if (extension_id != active_dialog_extension_id_) {
+      return RequestPinResponse::OTHER_FLOW_IN_PROGRESS;
+    }
+
+    // Extension requests a PIN without having received any input from its
+    // previous request. Reject the new request.
+    if (!active_pin_dialog_->IsLocked()) {
+      return RequestPinResponse::DIALOG_DISPLAYED_ALREADY;
+    }
+
+    // Set the new callback to be used by the view.
+    active_pin_dialog_->SetCallback(callback);
+    active_pin_dialog_->SetDialogParameters(
+        code_type, error_type, std::move(attempts_left), accept_input);
+    active_pin_dialog_->GetDialogClientView()->UpdateDialogButtons();
+    return RequestPinResponse::SUCCESS;
+  }
+
+  // Check the validity of sign_request_id
+  if (sign_request_times_.find(sign_request_id) == sign_request_times_.end()) {
+    return RequestPinResponse::INVALID_ID;
+  }
+
+  struct timeval tv;
+  int result = gettimeofday(&tv, nullptr);

[emaxx, 2016/09/06 15:02:10]

The same question as in the header - would it be possible to use the convenient
utils provided at base/time/?

[igorcov, 2016/09/07 09:12:28]

Done.

+  DCHECK_EQ(0, result);
+  if (tv.tv_sec - sign_request_times_[sign_request_id] >
+      SIGN_REQUEST_ID_TIMEOUT)
+    return RequestPinResponse::INVALID_ID;

[stevenjb, 2016/08/11 18:12:02]

{}

[igorcov, 2016/09/06 13:22:02]

Done.

+
+  active_dialog_extension_id_ = extension_id;
+  active_pin_dialog_ = new chromeos::RequestPinView(
+      extension_name, code_type, std::move(attempts_left), callback);
+
+  gfx::NativeWindow parent = nullptr;
+  if (chromeos::LoginDisplayHost::default_host()) {
+    parent = chromeos::LoginDisplayHost::default_host()->GetNativeWindow();
+  } else {
+    Browser* browser = chrome::FindTabbedBrowser(
+        ProfileManager::GetPrimaryUserProfile(), true);
+    if (browser) {
+      parent = browser->window()->GetNativeWindow();
+    }
+  }
+  gfx::NativeWindow context =
+      parent ? nullptr : ash::Shell::GetPrimaryRootWindow();
+  active_window_ = views::DialogDelegate::CreateDialogWidget(active_pin_dialog_,
+                                                             context, parent);
+  active_window_->Show();
+
+  return RequestPinResponse::SUCCESS;
+}
+
+bool PinDialogManager::CloseDialog(const std::string& extension_id) {
+  if (extension_id != active_dialog_extension_id_ ||
+      active_pin_dialog_ == nullptr) {
+    LOG(ERROR) << "StopPinRequest called by wrong extension";
+    return false;
+  }
+
+  // Notifying the old callback if present.
+  active_pin_dialog_->Cancel();
+
+  // Close the window. |active_pin_dialog_| gets deleted inside Close().
+  active_window_->Close();
+  active_pin_dialog_ = nullptr;
+
+  return true;
+}
+
+bool PinDialogManager::AddSignRequestId(const uint64_t sign_request_id) {
+  if (sign_request_times_[sign_request_id]) {
+    return false;
+  }
+  // Cache the ID with current timestamp.
+  struct timeval tv;
+  int result = gettimeofday(&tv, nullptr);
+  DCHECK_EQ(0, result);
+  sign_request_times_[sign_request_id] = tv.tv_sec;
+  if (sign_request_times_.size() > MAX_SIGN_REQUESTS_MAP_SIZE) {
+    RemoveExpiredSignRequests(&tv);
+  }
+
+  return true;
+}
+
+void PinDialogManager::ExtensionUnloaded(const std::string& extension_id) {
+  if (active_pin_dialog_ && active_dialog_extension_id_ == extension_id) {
+    CloseDialog(extension_id);
+  }
+
+  last_response_closed_[extension_id] = false;
+}
+
+void PinDialogManager::RemoveExpiredSignRequests(timeval* tv) {
+  for (auto it = sign_request_times_.cbegin();
+       it != sign_request_times_.cend();) {
+    if (tv->tv_sec - it->second > SIGN_REQUEST_ID_TIMEOUT) {
+      sign_request_times_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+}
+
+}  // namespace chromeos