Implementation for chrome.certificateProvider.requestPin/stopPinRequest

chrome/browser/chromeos/certificate_provider/pin_dialog_manager.h

+// 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_CERTIFICATE_PROVIDER_PIN_DIALOG_MANAGER_H_
+#define CHROME_BROWSER_CHROMEOS_CERTIFICATE_PROVIDER_PIN_DIALOG_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/memory/weak_ptr.h"
+#include "chrome/browser/chromeos/ui/request_pin_view.h"
+
+namespace chromeos {
+
+enum RequestPinResponse {

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

It's better to either make this "enum class" or move it inside the
PinDialogManager class (probably, the latter is better).

The current definition effectively puts the symbols into the chromeos namespace
with too generic names (e.g. chromeos::SUCCESS).

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

Done.

+  SUCCESS,
+  INVALID_ID,
+  OTHER_FLOW_IN_PROGRESS,
+  DIALOG_DISPLAYED_ALREADY
+};
+
+// Manages the state of the dialog that requests the PIN from user.

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

We should probably also describe the feature a bit more here, since this is the
primary class managing it. Nothing too long but just mention that this is used
by extensions that need to request a pin. Just enough to answer "why does this
class/dialog exist?" Also maybe reference a tracking issue, e.g. crbug.com/1234.

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

Done.

+class PinDialogManager {
+ public:
+  PinDialogManager();
+  virtual ~PinDialogManager();

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

There are no child classes of this class currently, am I correct?
Then instead of this precautious move (I mean adding an empty virtual
destructor) you could just mark the class as "final" now.

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

Done.

+
+  // The user provided input to dialog. |closed| tells whether the dialog was
+  // closed by the user without providing any input.
+  void OnPinDialogInput(const std::string& extension_id, const bool closed);

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

nit: regarding "const bool", "const long long" etc. - I believe usually in
Chromium code base the primitive types are passed just by value.

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

Done.

+
+  // Called by the view when user closes the PIN dialog while the last input is
+  // still processing at extension side. |value| is not used, but checked that
+  // it's empty.
+  void OnFlowInterrupted(const base::string16& value);

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

nit: #include "base/strings/string16.h"

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

Would it be easy to pass the extension_id here too?
Currently, the API looks a bit surprising as all other functions accept the
extension_id, but here it's omitted (and is obtained implicitly).

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

I've cleaned the extension_id from functions that are called internally as 
it's not needed, doesn't accomplish anything and passing a string around is
bad overall.

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

Done.

+
+  // Returns whether the last PIN dialog from this extension was closed by the
+  // user.
+  bool LastPinDialogClosed(const std::string& extension_id);
+
+  // Updates the existing dialog with new error message. Uses |callback| with
+  // empty string when user closes the dialog. Returns whether the provided
+  // |extension_id| matches the extension owning the active dialog.
+  bool UpdatePinDialog(const std::string& extension_id,
+                       RequestPinErrorType error_type,
+                       const bool accept_input,
+                       const RequestPinView::RequestPinCallback& callback);
+
+  // Creates a new RequestPinView object and displays it in a dialog or reuses

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

nit: Maybe it would make sense to reorder the method declarations, so that they
follow the typical work flow (more or less). E.g. first AddSignRequestId, then
ShowPingDialog, etc.

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

Done.

+  // the old dialog if active one exists just updating the parameters.
+  // |extension_id| - the ID of the extension requesting the dialog.
+  // |extension_name| - the name of the extension requesting the dialog.
+  // |sign_request_id| - the ID given by Chrome when the extension was asked to
+  //     sign the data. It should be a valid, not expired ID at the time the
+  //     extension is requesting PIN the first time.
+  // |code_type| - the type of input requested: either "PIN" or "PUK".
+  // |error_type| - the error template to be displayed inside the dialog. If
+  //     NONE, no error is displayed.
+  // |attempts_left| - the number of attempts the user has to try the code. It
+  //     is informational only, and enforced on Chrome side only in case it's
+  //     zero. In that case the textfield is disabled and the user can't provide
+  //     any input to extension. If nullptr the textfield from the dialog is
+  //     enabled but no information about the attepts left is not given to the
+  //     user.
+  // |callback| - used to notify about the user input in the text_field from the
+  //     dialog.
+  // Returns SUCCESS if the dialog is displayed and extension owns it. Otherwise
+  // the specific error is returned.
+  RequestPinResponse 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,

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

Digging through the code, I don't see any reason for this to be a pointer
instead of just an integer. We can use -1 to indicate an unspecified value, i.e.
infinite attempts left. Using a pointer suggests that we might be referencing it
somewhere else, which should not be the case.

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

Yes, the possibility for the parameter to be missing is the only reason I've
used unique_ptr. Changed to int with comment that -1 means the value is missing.

+      const RequestPinView::RequestPinCallback& callback);
+
+  // Called when extension calls the stopPinRequest method. The active dialog is
+  // closed if the |extension_id| matches the |active_dialog_extension_id_|.
+  // Returns whether the dialog was closed.
+  bool CloseDialog(const std::string& extension_id);
+
+  // Stores internally the |signRequestId| along with current timestamp. Also
+  // cleans up the storage from expired IDs. In unlikely case that the ID
+  // exists in the storage, returns false. Otherwise returns true.
+  bool AddSignRequestId(const uint64_t signRequestId);
+
+  // Resets the manager data related to the extension.
+  void ExtensionUnloaded(const std::string& extension_id);
+
+  RequestPinView* active_view_for_testing() { return active_pin_dialog_; }
+  views::Widget* active_window_for_testing() { return active_window_; }

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

nit: #include "ui/views/widget/widget.h".

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

Done.

+
+ private:
+  // Cleans the map of sign request ids, removing the ones that have expired.
+  void RemoveExpiredSignRequests(timeval* tv);
+
+  // State about the last response from user to the requestPin from extension.
+  std::map<std::string, bool> last_response_closed_;

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

nit: #include <map>
(BTW, maybe use std::unordered_map? Not insisting, but I think that using
std::map may make somebody think there's some dependency on the order of the
elements.)

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

Done.

+
+  // The map with sign request ids issued by Chrome as key and the time when the
+  // id was generated as value.
+  std::map<uint64_t, uint64_t> sign_request_times_;

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

Is there a reason to use uint64_t for storing the time instead of base::Time?

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

I've used timeval as that was one example I found in the code base. Changed to
use base::Time now.

+
+  // There can be only one active dialog to request PIN from this extension.
+  // Owned by |active_window_|.
+  chromeos::RequestPinView* active_pin_dialog_ = nullptr;
+  std::string active_dialog_extension_id_;
+  views::Widget* active_window_ = nullptr;
+
+  base::WeakPtrFactory<PinDialogManager> weak_factory_;

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

nit: #include "base/memory/weak_ptr.h".

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

Done.

+};
+
+}  // namespace chromeos
+
+#endif  // CHROME_BROWSER_CHROMEOS_CERTIFICATE_PROVIDER_PIN_DIALOG_MANAGER_H_