Chromad: added AD Join ui, authpolicy_client

chromeos/dbus/authpolicy_client.cc

+// Copyright (c) 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 "chromeos/dbus/authpolicy_client.h"
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/worker_pool.h"
+#include "chromeos/dbus/pipe_string_writer.h"
+#include "dbus/bus.h"
+#include "dbus/message.h"
+#include "dbus/object_proxy.h"
+
+namespace chromeos {
+
+namespace {
+
+// TODO(rsorokin): switch to service constants when it's landed (crbug/658655/)

[hashimoto, 2016/10/26 06:11:12]

nit: crbug.com is the publicly accessible URL of our bug tracker.

Can't you land the service constants change first?
Are you working on a project which requires internal code access?

[Thiemo Nagel, 2016/10/26 09:12:13]

I think a nice way to write this would be:

TODO(rsorokin, 658655): Switch to service constants when they have landed.

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Done.

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Done.

+const char kAuthPolicyInterface[] = "org.chromium.AuthPolicy";
+const char kAuthPolicyServicePath[] = "/org/chromium/AuthPolicy";
+const char kAuthPolicyServiceName[] = "org.chromium.AuthPolicy";
+// Method
+const char kAuthPolicyJoinADDomain[] = "JoinADDomain";
+
+class AuthPolicyClientImpl : public AuthPolicyClient {
+ public:
+  AuthPolicyClientImpl() : bus_(nullptr), weak_ptr_factory_(this) {}
+
+  ~AuthPolicyClientImpl() override {}
+
+  void JoinADDomain(const std::string& machine_name,
+                    const std::string& user,
+                    const std::string& password,
+                    const JoinCallback& callback) override {
+    scoped_refptr<base::TaskRunner> task_runner =
+        base::WorkerPool::GetTaskRunner(true /* tasks_are_slow */);

[hashimoto, 2016/10/26 06:11:11]

Generally speaking, you should avoid using WorkerPool.
https://www.chromium.org/developers/coding-style/important-abstractions-and-d...

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Done.

+    pipe_string_writer_.reset(new PipeStringWriter(

[hashimoto, 2016/10/26 06:11:12]

Sorry for not being clear, please put the pipe related logic to somewhere close
to the user code (i.e. enrollment_screen_handler.cc), and make this client just
receive its FD.

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Done.

+        password, task_runner,
+        base::Bind(&AuthPolicyClientImpl::OnPipeReadComplete,
+                   weak_ptr_factory_.GetWeakPtr())));
+
+    dbus::ObjectPath objectPath(kAuthPolicyServicePath);
+    dbus::ObjectProxy* proxy =
+        bus_->GetObjectProxy(kAuthPolicyServiceName, objectPath);
+    dbus::MethodCall method_call(kAuthPolicyInterface, kAuthPolicyJoinADDomain);
+    dbus::MessageWriter writer(&method_call);
+    writer.AppendString(machine_name);
+    writer.AppendString(user);
+    writer.AppendFileDescriptor(pipe_string_writer_->StartIO().get());
+    proxy->CallMethod(&method_call, dbus::ObjectProxy::TIMEOUT_USE_DEFAULT,
+                      base::Bind(&AuthPolicyClientImpl::HandleJoinCallback,
+                                 weak_ptr_factory_.GetWeakPtr(), callback));
+  }
+
+ protected:
+  void Init(dbus::Bus* bus) override { bus_ = bus; }
+
+ private:
+  void HandleJoinCallback(const JoinCallback& callback,
+                          dbus::Response* response) {
+    if (!response) {
+      LOG(ERROR) << "Join: Couldn't call to authpolicy";
+      // TODO(rsorokin): make proper call, after defining possible errors codes.

[hashimoto, 2016/10/26 06:11:11]

Before this change, can't you land error code constants together with the
service constants above?

Even when not, please make the "1" a constant and put it next to the service
constants above.

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Constants cl is in the pipeline, left TODO

+      callback.Run(1);
+      return;
+    }
+
+    dbus::MessageReader reader(response);
+    int res;

[hashimoto, 2016/10/26 06:11:12]

Please don't leave a variable uninitialized.

[Roman Sorokin (ftl), 2016/10/26 18:45:35]

Done.

+    if (!reader.PopInt32(&res)) {
+      LOG(ERROR) << "Join: Couldn't get an error from the response";
+      // TODO(rsorokin): make proper call, after defining possible errors codes.
+      callback.Run(1);
+      return;
+    }
+
+    callback.Run(res);
+  }
+
+  void OnPipeReadComplete(int error_code) {
+    if (error_code != 0) {
+      LOG(ERROR) << "Failed to stream password to AuthPolicy: " << error_code;
+    }
+  }
+
+  dbus::Bus* bus_;
+
+  std::unique_ptr<PipeStringWriter> pipe_string_writer_;
+  // Note: This should remain the last member so it'll be destroyed and
+  // invalidate its weak pointers before any other members are destroyed.
+  base::WeakPtrFactory<AuthPolicyClientImpl> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(AuthPolicyClientImpl);
+};
+
+}  // namespace
+
+AuthPolicyClient::AuthPolicyClient() {}
+
+AuthPolicyClient::~AuthPolicyClient() {}
+
+// static
+AuthPolicyClient* AuthPolicyClient::Create() {
+  return new AuthPolicyClientImpl();
+}
+
+}  // namespace chromeos