Added PIN-less auth policy.

Added PIN-less auth policy.

BUG=156182
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215355

[Jamie, 2013-07-26 23:25:39]

mnissler: PTAL at everything under chrome/
alexeypa: PTAL at everything under remoting/

[alexeypa (please no reviews), 2013-07-26 23:40:24]

remoting/* - lgtm once the comments are addressed.

https://codereview.chromium.org/20864002/diff/1/remoting/host/policy_hack/pol...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/20864002/diff/1/remoting/host/policy_hack/pol...
remoting/host/policy_hack/policy_watcher.h:58: // The name of the username
policy. This policy is not supported on Windows.
nit#1: "not supported" -> "ignored" or "not used" to emphasize that there will
be no error if it is set.

nit#2: add a comment saying that this policy is not supported publicly at the
moment to explain why it is not mentioned in the policy template.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:488: if (allow_pairing_) {
nit:

if (!allow_pairing_)
  return;

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:491: if (delegate) {
nit: no need for brackets here.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:764:
policy_hack::PolicyWatcher::kHostAllowClientPairing,
nit: The code directly above uses different indentation for the parameters.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:914: // Returns true if the host has to be
restarted after this policy update.
nit: This comment should be next to the method declaration, not here.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:917: if (allow_pairing_ != allow_pairing) {
nit: You can early |return false;| here. It makes the two branches nicely
separated blocks of code.

[Jamie, 2013-07-27 00:26:21]

fyi

https://codereview.chromium.org/20864002/diff/1/remoting/host/policy_hack/pol...
File remoting/host/policy_hack/policy_watcher.h (right):

https://codereview.chromium.org/20864002/diff/1/remoting/host/policy_hack/pol...
remoting/host/policy_hack/policy_watcher.h:58: // The name of the username
policy. This policy is not supported on Windows.
On 2013/07/26 23:40:25, alexeypa wrote:
> nit#1: "not supported" -> "ignored" or "not used" to emphasize that there will
> be no error if it is set.
> 
> nit#2: add a comment saying that this policy is not supported publicly at the
> moment to explain why it is not mentioned in the policy template.

#1 Done.

I'm not convinced of the value of #2, but done anyway. LMKWYT.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:488: if (allow_pairing_) {
On 2013/07/26 23:40:25, alexeypa wrote:
> nit:
> 
> if (!allow_pairing_)
>   return;

We still need to create the authenticator factory.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:491: if (delegate) {
On 2013/07/26 23:40:25, alexeypa wrote:
> nit: no need for brackets here.

Done.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:764:
policy_hack::PolicyWatcher::kHostAllowClientPairing,
On 2013/07/26 23:40:25, alexeypa wrote:
> nit: The code directly above uses different indentation for the parameters.

Done.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:914: // Returns true if the host has to be
restarted after this policy update.
On 2013/07/26 23:40:25, alexeypa wrote:
> nit: This comment should be next to the method declaration, not here.

I've just removed it. I don't think it adds anything.

https://codereview.chromium.org/20864002/diff/1/remoting/host/remoting_me2me_...
remoting/host/remoting_me2me_host.cc:917: if (allow_pairing_ != allow_pairing) {
On 2013/07/26 23:40:25, alexeypa wrote:
> nit: You can early |return false;| here. It makes the two branches nicely
> separated blocks of code.

Done.

[Mattias Nissler (ping if slow), 2013-07-29 08:25:10]

https://codereview.chromium.org/20864002/diff/7001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/20864002/diff/7001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:603: 'desc': '''
nit: No line break here.`

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:230: { "pref":
"remote_access.host_allow_client_pairing" }
Where's the CL that introduces this pref mapping?

[Jamie, 2013-07-29 18:47:29]

mnissler: ptal

https://codereview.chromium.org/20864002/diff/7001/chrome/app/policy/policy_t...
File chrome/app/policy/policy_templates.json (right):

https://codereview.chromium.org/20864002/diff/7001/chrome/app/policy/policy_t...
chrome/app/policy/policy_templates.json:603: 'desc': '''
On 2013/07/29 08:25:10, Mattias Nissler wrote:
> nit: No line break here.`

Done.

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:230: { "pref":
"remote_access.host_allow_client_pairing" }
On 2013/07/29 08:25:10, Mattias Nissler wrote:
> Where's the CL that introduces this pref mapping?

I forgot to add it, thanks. How would I go about verifying that I've added this
everywhere it needs to go?

[Mattias Nissler (ping if slow), 2013-07-30 09:14:37]

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/20864002/diff/7001/chrome/test/data/policy/po...
chrome/test/data/policy/policy_test_cases.json:230: { "pref":
"remote_access.host_allow_client_pairing" }
On 2013/07/29 18:47:29, Jamie wrote:
> On 2013/07/29 08:25:10, Mattias Nissler wrote:
> > Where's the CL that introduces this pref mapping?
> 
> I forgot to add it, thanks. How would I go about verifying that I've added
this
> everywhere it needs to go?

Write a test :) policy_browsertest.cc has examples on how to inject policy.

Also, have you tested this?
http://dev.chromium.org/developers/how-tos/enterprise/adding-new-policies has
pointers to test instructions in the last section.

[Mattias Nissler (ping if slow), 2013-07-30 09:14:57]

Oh, LGTM assuming you have verified that this is working as intended.

[commit-bot: I haz the power, 2013-07-30 23:36:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/16001

[commit-bot: I haz the power, 2013-07-31 01:34:08]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2013-07-31 17:34:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/26001

[commit-bot: I haz the power, 2013-07-31 17:34:30]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/app/policy/policy_templates.json
  Hunk #1 FAILED at 112.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

Patch:       chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
diff --git a/chrome/app/policy/policy_templates.json
b/chrome/app/policy/policy_templates.json
index
3a32920b3d745fb50432cc15a11a45b8f88a95d8..76f93e2fd40cbcab53019cace675009b6d8f7b3b
100644
--- a/chrome/app/policy/policy_templates.json
+++ b/chrome/app/policy/policy_templates.json
@@ -112,7 +112,7 @@
 #   persistent IDs for all fields (but not for groups!) are needed. These are
 #   specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
 #   because doing so would break the deployed wire format!
-#   For your editing convenience: highest ID currently used: 226
+#   For your editing convenience: highest ID currently used: 228
 #
 # Placeholders:
 #   The following placeholder strings are automatically substituted:
@@ -588,6 +588,22 @@
 
           If this setting is disabled or not set, then both local and remote
users can interact with the host when it is being shared.''',
         },
+        {
+          'name': 'RemoteAccessHostAllowClientPairing',
+          'type': 'main',
+          'schema': { 'type': 'boolean' },
+          'supported_on': ['chrome.*:30-'],
+          'features': {
+            'dynamic_refresh': True,
+            'per_profile': False,
+          },
+          'example_value': False,
+          'id': 228,
+          'caption': '''Enable or disable PIN-less authentication''',
+          'desc': '''If this setting is enabled or not configured, then users
can opt to pair clients and hosts at connection time, eliminating the need to
enter a PIN every time.
+
+          If this setting is disabled, then this feature will not be
available.''',
+        },
       ],
     },
     {

[commit-bot: I haz the power, 2013-07-31 20:56:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/31001

[commit-bot: I haz the power, 2013-07-31 21:52:19]

Retried try job too often on linux_rel for step(s) remoting_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Jamie, 2013-08-01 17:46:38]

wez: ptal at the new unit tests.

[Wez, 2013-08-01 18:13:22]

lgtm

[commit-bot: I haz the power, 2013-08-01 18:35:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/52001

[commit-bot: I haz the power, 2013-08-01 18:35:47]

Failed to apply patch for chrome/app/policy/policy_templates.json:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/app/policy/policy_templates.json
  Hunk #1 FAILED at 112.
  1 out of 2 hunks FAILED -- saving rejects to file
chrome/app/policy/policy_templates.json.rej

Patch:       chrome/app/policy/policy_templates.json
Index: chrome/app/policy/policy_templates.json
diff --git a/chrome/app/policy/policy_templates.json
b/chrome/app/policy/policy_templates.json
index
cd70fd2a8666ea1634003f3adc8dcabe85d675ab..75b1c196cb241f5a0da9d5a158d8721be002e1ac
100644
--- a/chrome/app/policy/policy_templates.json
+++ b/chrome/app/policy/policy_templates.json
@@ -112,7 +112,7 @@
 #   persistent IDs for all fields (but not for groups!) are needed. These are
 #   specified by the 'id' keys of each policy. NEVER CHANGE EXISTING IDs,
 #   because doing so would break the deployed wire format!
-#   For your editing convenience: highest ID currently used: 228
+#   For your editing convenience: highest ID currently used: 229
 #
 # Placeholders:
 #   The following placeholder strings are automatically substituted:
@@ -588,6 +588,22 @@
 
           If this setting is disabled or not set, then both local and remote
users can interact with the host when it is being shared.''',
         },
+        {
+          'name': 'RemoteAccessHostAllowClientPairing',
+          'type': 'main',
+          'schema': { 'type': 'boolean' },
+          'supported_on': ['chrome.*:30-'],
+          'features': {
+            'dynamic_refresh': True,
+            'per_profile': False,
+          },
+          'example_value': False,
+          'id': 229,
+          'caption': '''Enable or disable PIN-less authentication''',
+          'desc': '''If this setting is enabled or not configured, then users
can opt to pair clients and hosts at connection time, eliminating the need to
enter a PIN every time.
+
+          If this setting is disabled, then this feature will not be
available.''',
+        },
       ],
     },
     {

[commit-bot: I haz the power, 2013-08-01 19:00:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/58001

[commit-bot: I haz the power, 2013-08-02 04:54:46]

Failed to apply patch for remoting/host/remoting_me2me_host.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/host/remoting_me2me_host.cc
  Hunk #4 FAILED at 486.
  Hunk #5 succeeded at 751 (offset -4 lines).
  Hunk #6 succeeded at 901 (offset -4 lines).
  1 out of 6 hunks FAILED -- saving rejects to file
remoting/host/remoting_me2me_host.cc.rej

Patch:       remoting/host/remoting_me2me_host.cc
Index: remoting/host/remoting_me2me_host.cc
diff --git a/remoting/host/remoting_me2me_host.cc
b/remoting/host/remoting_me2me_host.cc
index
3af312364861ee8e1c456dcc9d0071d7fdcc6059..080d85a5aa858f1c6e069006fd40af69df7eb927
100644
--- a/remoting/host/remoting_me2me_host.cc
+++ b/remoting/host/remoting_me2me_host.cc
@@ -213,6 +213,7 @@ class HostProcess
   bool OnHostTalkGadgetPrefixPolicyUpdate(const std::string&
talkgadget_prefix);
   bool OnHostTokenUrlPolicyUpdate(const GURL& token_url,
                                   const GURL& token_validation_url);
+  bool OnPairingPolicyUpdate(bool pairing_enabled);
 
   void StartHost();
 
@@ -266,6 +267,7 @@ class HostProcess
   scoped_ptr<policy_hack::PolicyWatcher> policy_watcher_;
   bool allow_nat_traversal_;
   std::string talkgadget_prefix_;
+  bool allow_pairing_;
 
   bool curtain_required_;
   GURL token_url_;
@@ -297,6 +299,7 @@ HostProcess::HostProcess(scoped_ptr<ChromotingHostContext>
context,
     : context_(context.Pass()),
       state_(HOST_INITIALIZING),
       allow_nat_traversal_(true),
+      allow_pairing_(true),
       curtain_required_(false),
 #if defined(REMOTING_MULTI_PROCESS)
       desktop_session_connector_(NULL),
@@ -483,10 +486,11 @@ void HostProcess::CreateAuthenticatorFactory() {
   }
 
   scoped_refptr<protocol::PairingRegistry> pairing_registry = NULL;
-  scoped_ptr<protocol::PairingRegistry::Delegate> delegate(
-      CreatePairingRegistryDelegate(context_->file_task_runner()));
-  if (delegate) {
-    pairing_registry = new protocol::PairingRegistry(delegate.Pass());
+  if (allow_pairing_) {
+    scoped_ptr<protocol::PairingRegistry::Delegate> delegate(
+        CreatePairingRegistryDelegate(context_->file_task_runner()));
+    if (delegate)
+      pairing_registry = new protocol::PairingRegistry(delegate.Pass());
   }
 
   scoped_ptr<protocol::AuthenticatorFactory> factory;
@@ -752,6 +756,11 @@ void
HostProcess::OnPolicyUpdate(scoped_ptr<base::DictionaryValue> policies) {
     restart_required |= OnHostTokenUrlPolicyUpdate(
         GURL(token_url_string), GURL(token_validation_url_string));
   }
+  if (policies->GetBoolean(
+          policy_hack::PolicyWatcher::kHostAllowClientPairing,
+          &bool_value)) {
+    restart_required |= OnPairingPolicyUpdate(bool_value);
+  }
 
   if (state_ == HOST_INITIALIZING) {
     StartHost();
@@ -897,6 +906,20 @@ bool HostProcess::OnHostTokenUrlPolicyUpdate(
   return false;
 }
 
+bool HostProcess::OnPairingPolicyUpdate(bool allow_pairing) {
+  DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
+
+  if (allow_pairing_ == allow_pairing)
+    return false;
+
+  if (allow_pairing)
+    LOG(INFO) << "Policy enables client pairing.";
+  else
+    LOG(INFO) << "Policy disables client pairing.";
+  allow_pairing_ = allow_pairing;
+  return true;
+}
+
 void HostProcess::StartHost() {
   DCHECK(context_->network_task_runner()->BelongsToCurrentThread());
   DCHECK(!host_);

[commit-bot: I haz the power, 2013-08-02 17:25:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jamiewalch@chromium.org/20864002/84001

[commit-bot: I haz the power, 2013-08-02 20:13:59]

Change committed as 215355