Remove policy watching from It2MeHost.

remoting/host/it2me/it2me_host_unittest.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 "remoting/host/it2me/it2me_host.h"
 
 #include <memory>
 #include <string>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/callback_helpers.h"
 #include "base/location.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/threading/thread_task_runner_handle.h"
-#include "components/policy/core/common/fake_async_policy_loader.h"
 #include "components/policy/policy_constants.h"
 #include "remoting/base/auto_thread_task_runner.h"
 #include "remoting/host/chromoting_host_context.h"
 #include "remoting/host/it2me/it2me_confirmation_dialog.h"
 #include "remoting/host/policy_watcher.h"
 #include "remoting/signaling/fake_signal_strategy.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace remoting {
 
@@ skipping 129 matching lines @@
  private:
   void StartupHostStateHelper(const base::Closure& quit_closure);
 
   std::unique_ptr<base::MessageLoop> message_loop_;
   std::unique_ptr<base::RunLoop> run_loop_;
 
   scoped_refptr<AutoThreadTaskRunner> network_task_runner_;
   scoped_refptr<AutoThreadTaskRunner> ui_task_runner_;
 
   scoped_refptr<It2MeHost> it2me_host_;
-  // The FakeAsyncPolicyLoader is owned by it2me_host_, but we retain a raw
-  // pointer so we can control the policy contents.
-  policy::FakeAsyncPolicyLoader* policy_loader_ = nullptr;
 
   base::WeakPtrFactory<It2MeHostTest> weak_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(It2MeHostTest);
 };
 
 It2MeHostTest::It2MeHostTest() : weak_factory_(this) {}
 
 It2MeHostTest::~It2MeHostTest() {}
 
 void It2MeHostTest::SetUp() {
   message_loop_.reset(new base::MessageLoop());
   run_loop_.reset(new base::RunLoop());
 
   std::unique_ptr<ChromotingHostContext> host_context(
       ChromotingHostContext::Create(new AutoThreadTaskRunner(
           base::ThreadTaskRunnerHandle::Get(), run_loop_->QuitClosure())));
   network_task_runner_ = host_context->network_task_runner();
   ui_task_runner_ = host_context->ui_task_runner();
 
   std::unique_ptr<FakeIt2MeDialogFactory> dialog_factory(
       new FakeIt2MeDialogFactory());
   dialog_factory_ = dialog_factory.get();
-  policy_loader_ =
-      new policy::FakeAsyncPolicyLoader(base::ThreadTaskRunnerHandle::Get());
   it2me_host_ = new It2MeHost(
       std::move(host_context),
-      PolicyWatcher::CreateFromPolicyLoaderForTesting(
-          base::WrapUnique(policy_loader_)),
       std::move(dialog_factory), weak_factory_.GetWeakPtr(),
       base::WrapUnique(
           new FakeSignalStrategy(SignalingAddress("fake_local_jid"))),
       kTestUserName, "fake_bot_jid");
 }
 
 void It2MeHostTest::TearDown() {
   // Shutdown the host if it hasn't been already. Without this, the call to
   // run_loop_->Run() may never return.
   it2me_host_->Disconnect();
   network_task_runner_ = nullptr;
   ui_task_runner_ = nullptr;
   it2me_host_ = nullptr;
   run_loop_->Run();
 }
 
 void It2MeHostTest::OnValidationComplete(const base::Closure& resume_callback,
                                          ValidationResult validation_result) {
   validation_result_ = validation_result;
 
   ui_task_runner_->PostTask(FROM_HERE, resume_callback);
 }
 
 void It2MeHostTest::SetPolicies(
     std::initializer_list<std::pair<base::StringPiece, const base::Value&>>
         policies) {
-  policy::PolicyBundle bundle;
-  policy::PolicyMap& map = bundle.Get(
-      policy::PolicyNamespace(policy::POLICY_DOMAIN_CHROME, std::string()));
+  std::unique_ptr<base::DictionaryValue> dictionary(new base::DictionaryValue);

[rkjnsn, 2017/04/28 21:15:36]

nit: I think MakeUnique is more readable:
auto dictionary = base::MakeUnique<base::DictionaryValue>();

[Jamie, 2017/04/28 22:41:09]

Done.

   for (const auto& policy : policies) {
-    map.Set(policy.first.as_string(), policy::POLICY_LEVEL_MANDATORY,
-            policy::POLICY_SCOPE_MACHINE, policy::POLICY_SOURCE_PLATFORM,
-            policy.second.CreateDeepCopy(), nullptr);
+    dictionary->Set(policy.first, policy.second.CreateDeepCopy());
   }
-  policy_loader_->SetPolicies(bundle);
-  policy_loader_->PostReloadOnBackgroundThread(true);
-  base::RunLoop().RunUntilIdle();
+  it2me_host_->OnPolicyUpdate(std::move(dictionary));

[rkjnsn, 2017/04/28 21:15:36]

Note that this will bypass normalization and deprecation logic in PolicyWatcher,
which means most of the tests will be broken after
https://codereview.chromium.org/2682473003/ lands.

[joedow, 2017/04/28 21:46:32]

Using the old system (PolicyWatcher + FakeAsyncPolicyLoader) means you get our
default values too so you dpon't have to init once with empty list in
StartHost().

[Jamie, 2017/04/28 22:41:09]

The tests hang without a call to SetPolicies (I think because the It2Me host
needs OnPolicyUpdate to be called before it does anything).

[Jamie, 2017/04/28 22:41:09]

I'll hold off landing until that's in then. I think the right approach will be
just to simplify these tests not to expect that normalization, since it's
already tested in the policy watcher tests.

[rkjnsn, 2017/05/02 19:19:12]

It2MeHost will indeed wait for OnPolicyUpdate to be called the first time. One
possibility would be to keep an actual instance of PolicyWatcher in It2MeTest.
That way, It2MeHost will get the default values as managed by PolicyWatcher
instead of an empty list for the initial call, and deprecation and normalization
will still happen (obviating the need to update the tests).

[Jamie, 2017/05/02 22:07:56]

I think that's mixing up responsibilities too much. This CL allows It2MeHost to
no longer care where its policies are coming from, so the test should provide
them in as simple a way as possible. Adding a PolicyWatcher to the test seems
overly complicated, and we shouldn't test normalization here, since it's not a
feature of It2MeHost.

[rkjnsn, 2017/05/02 23:20:19]

I mostly agree. My main concern is here was the difference in behavior for the
initial call to OnPolicyUpdate. Here we initially call it with an empty
dictionary, whereas PolicyWatcher would call it with a dictionary that had all
of the policies set to there default values. This could potentially lead to
different behavior. E.g., with the approach here, the Update*Policy methods are
never called unless the corresponding policy is set by the test, whereas with
PolicyWatcher they are always called at least once. Also, the defaults might be
different (e.g., It2MeHost defaults nat_traversal_enabled_ to false, but
PolicyWatcher defaults the RemoteAccessHostFirewallTraversal policy to true).

Now, it's probably true that none of these matter, and if they ever do, the
SetPolicies({}) call below can be adjusted as needed. Thus, the current approach
should be fine. It may be worth putting a comment on the SetPolicies({}) call on
244 to call out the differing behavior, though.

[rkjnsn, 2017/05/02 23:27:19]

their*

[Jamie, 2017/05/02 23:49:03]

That's a good point. I was going to argue that It2MeHost should have suitable
defaults set for its member variables, but on reflection I don't think I believe
that, since those will be the same defaults as are set in the policy (or derived
from them) and duplication is never a good idea.

Perhaps the best solution is to instantiate a PolicyWatcher, but just use its
GetCurrentPolicies method to pre-set the defaults, and allow individual tests to
call SetPolicies to override anything they like. That still decouples It2MeHost
from the mechanics of PolicyWatcher, but does ensure that the correct default
values are set. WDYT?

[Jamie, 2017/05/03 20:41:04]

I've taken this approach in the most recent iteration.

 }
 
 void It2MeHostTest::StartupHostStateHelper(const base::Closure& quit_closure) {
   if (last_host_state_ == It2MeHostState::kRequestedAccessCode) {
     network_task_runner_->PostTask(
         FROM_HERE,
         base::Bind(&It2MeHost::SetStateForTesting, it2me_host_.get(),
                    It2MeHostState::kReceivedAccessCode, std::string()));
   } else if (last_host_state_ != It2MeHostState::kStarting) {
     quit_closure.Run();
     return;
   }
   state_change_callback_ = base::Bind(&It2MeHostTest::StartupHostStateHelper,
                                       base::Unretained(this), quit_closure);
 }
 
 void It2MeHostTest::StartHost() {
+  SetPolicies({});  // For tests that don't set any policies
   it2me_host_->Connect();
 
   base::RunLoop run_loop;
   state_change_callback_ =
       base::Bind(&It2MeHostTest::StartupHostStateHelper, base::Unretained(this),
                  run_loop.QuitClosure());
   run_loop.Run();
 }
 
 void It2MeHostTest::RunUntilStateChanged(It2MeHostState expected_state) {
@@ skipping 207 matching lines @@
   ASSERT_EQ(ValidationResult::SUCCESS, validation_result_);
   ASSERT_EQ(It2MeHostState::kConnecting, last_host_state_);
 
   RunValidationCallback(kTestClientJid2);
   ASSERT_EQ(ValidationResult::ERROR_TOO_MANY_CONNECTIONS, validation_result_);
   RunUntilStateChanged(It2MeHostState::kDisconnected);
   ASSERT_EQ(It2MeHostState::kDisconnected, last_host_state_);
 }
 
 }  // namespace remoting