Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc

Index: chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc
diff --git a/chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc b/chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2dfce248d7af122f5abf1db5a7f3aafb44978aeb
--- /dev/null
+++ b/chrome/browser/chromeos/certificate_provider/pin_dialog_manager.cc
@@ -0,0 +1,184 @@
+// 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
+// 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_rejected_[extension_id] = closed;
+  if (closed) {
+    active_pin_dialog_ = nullptr;

[stevenjb, 2016/08/11 01:58:54]

Leaky?

[igorcov1, 2016/08/11 16:15:23]

No. The active_window_ manages that object, sorry for confusion from the bad
comment in header file. Added a comment here too.

+    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)));
+}
+
+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_rejected_[extension_id];

[stevenjb, 2016/08/11 01:58:54]

We should probably actually name this last_response_closed_.

[igorcov1, 2016/08/11 16:15:23]

Done.

+}
+
+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);
+  // Don't allow the extension to create anything if an active dialog already
+  // exists. Still if the dialog is there, initiated by the extension then skip
+  // the signRequestId check as it's possibly expired.

[stevenjb, 2016/08/11 01:58:54]

This is confusing and I'm not sure I understand it. Can you clarify the logic
below and place smaller comments appropriately, e.g:

if (active_pin_dialog_ != nullptr) {
  DCHECK(!active_dialog_extension_id_.empty());  // That should never happen,
right? 
  if (extension_id != active_dialog_extension_id_)
    return RequestPinResponse::OTHER_FLOW_IN_PROGRESS;
  if (active_pin_dialog_->IsLocked()) {
    // Don't create a new dialog, instead update the callback and dialog
parameters
    ...
    return RequestPinResponse::SUCCESS;
  } else {
    ??? If the dialog is not locked do we actually want to return
OTHER_FLOW_IN_PROGRESS? Regardless we should add a comment to explain this state
  }
}

[igorcov1, 2016/08/11 16:15:22]

Added another state in the enum, for the case when dialog is not locked. This
should help the extension developers debug if a problem occurs. Also changed the
code structure and comments. Thanks.

+  if (active_pin_dialog_ != nullptr) {
+    if (!active_dialog_extension_id_.empty() &&
+        extension_id == active_dialog_extension_id_ &&
+        active_pin_dialog_->IsLocked()) {
+      // 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;
+    }
+
+    return RequestPinResponse::OTHER_FLOW_IN_PROGRESS;
+  }
+
+  // Check the validity of sign_request_id
+  if (sign_request_ids_.find(sign_request_id) == sign_request_ids_.end()) {
+    return RequestPinResponse::INVALID_ID;
+  }
+
+  struct timeval tv;
+  int result = gettimeofday(&tv, NULL);

[stevenjb, 2016/08/11 01:58:54]

nullptr throughout

[igorcov1, 2016/08/11 16:15:23]

Done.

+  DCHECK_EQ(0, result);
+  if (tv.tv_sec - sign_request_ids_[sign_request_id] > SIGN_REQUEST_ID_TIMEOUT)
+    return RequestPinResponse::INVALID_ID;
+
+  active_dialog_extension_id_ = extension_id;
+  active_pin_dialog_ = new chromeos::RequestPinView(
+      extension_name, code_type, std::move(attempts_left), callback);
+  gfx::NativeWindow context = ash::Shell::GetPrimaryRootWindow();
+  if (chromeos::LoginDisplayHost::default_host()) {
+    active_window_ = views::DialogDelegate::CreateDialogWidget(
+        active_pin_dialog_, context,
+        chromeos::LoginDisplayHost::default_host()->GetNativeWindow());
+    active_window_->Show();
+  } else {
+    Browser* browser = chrome::FindTabbedBrowser(
+        ProfileManager::GetPrimaryUserProfile(), true);
+    if (browser) {
+      gfx::NativeWindow native_window = browser->window()->GetNativeWindow();
+      active_window_ = views::DialogDelegate::CreateDialogWidget(
+          active_pin_dialog_, context, native_window);
+      active_window_->Show();
+    }

[stevenjb, 2016/08/11 01:58:55]

We should test and comment what happens if there is no browser here. As near as
I can tell we always call window->Show() for WifiConfigView. Where exactly is
the example you were following?

[igorcov1, 2016/08/11 16:15:23]

I took the code from here:
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/options/network_...

But looking where it is called,
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/options/network_...

you are right that the window is always showing. I've changed the code to have
the same behavior. Thanks.

+  }
+
+  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;

[stevenjb, 2016/08/11 01:58:54]

If active_pin_dialog_ is owned by acitve_window_, we need to comment that
clearly where active_pin_dialog_ is declared and add a similar comment above
where active_pin_dialog_ is set to null.

[igorcov1, 2016/08/11 16:15:23]

Done.

+
+  return true;
+}
+
+bool PinDialogManager::AddSignRequestId(const uint64_t sign_request_id) {
+  if (sign_request_ids_[sign_request_id]) {
+    return false;
+  }
+  // Cache the ID with current timestamp.
+  struct timeval tv;
+  int result = gettimeofday(&tv, NULL);
+  DCHECK_EQ(0, result);
+  sign_request_ids_[sign_request_id] = tv.tv_sec;
+  if (sign_request_ids_.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_rejected_[extension_id] = false;
+}
+
+void PinDialogManager::RemoveExpiredSignRequests(timeval* tv) {
+  for (auto it = sign_request_ids_.cbegin(); it != sign_request_ids_.cend();) {
+    if (tv->tv_sec - it->second > SIGN_REQUEST_ID_TIMEOUT) {
+      sign_request_ids_.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+}
+
+}  // namespace chromeos