Configure Libassistant through mojom service

Bug: b/176851446
Test: chromeos_unittests and deployed
Change-Id: I1463132c4e545a1ca8aa7b62880b6c6c4125b00f
Cq-Include-Trybots: luci.chrome.try:linux-chromeos-chrome
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2615504
Commit-Queue: Sam McNally <sammc@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Reviewed-by: Xiaohui Chen <xiaohuic@chromium.org>
Auto-Submit: Jeroen Dhollander <jeroendh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#843228}

chromeos/services/assistant/assistant_manager_service_impl.cc

@@ -1024,6 +1024,10 @@ void AssistantManagerServiceImpl::InitAssistant(
                      weak_factory_.GetWeakPtr()));
 }
 
+base::Thread& AssistantManagerServiceImpl::GetBackgroundThreadForTesting() {
+  return background_thread();
+}
+
 void AssistantManagerServiceImpl::PostInitAssistant() {
   DCHECK(main_task_runner()->RunsTasksInCurrentSequence());
   DCHECK_EQ(GetState(), State::STARTING);

chromeos/services/assistant/assistant_manager_service_impl.h

@@ -247,6 +247,8 @@ class COMPONENT_EXPORT(ASSISTANT_SERVICE) AssistantManagerServiceImpl
     return action_module_.get();
   }
 
+  base::Thread& GetBackgroundThreadForTesting();
+
  private:
   void InitAssistant(const base::Optional<UserInfo>& user,
                      const std::string& locale);

chromeos/services/assistant/assistant_manager_service_impl_unittest.cc

@@ -10,6 +10,7 @@
 #include "ash/public/cpp/assistant/controller/assistant_alarm_timer_controller.h"
 #include "base/json/json_reader.h"
 #include "base/optional.h"
+#include "base/run_loop.h"
 #include "base/test/bind.h"
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
@@ -258,7 +259,12 @@ class AssistantManagerServiceImplTest : public testing::Test {
                                        /*enable_hotword=*/false);
   }
 
-  void RunUntilIdle() { base::RunLoop().RunUntilIdle(); }
+  void RunUntilIdle() {
+    // First ensure our mojom thread is finished.
+    background_thread().FlushForTesting();
+    // Then handle any callbacks.
+    base::RunLoop().RunUntilIdle();
+  }
 
   // Adds a state observer mock, and add the expectation for the fact that it
   // auto-fires the observer.
@@ -321,6 +327,10 @@ class AssistantManagerServiceImplTest : public testing::Test {
   }
 
  private:
+  base::Thread& background_thread() {
+    return assistant_manager_service()->GetBackgroundThreadForTesting();
+  }
+
   base::test::SingleThreadTaskEnvironment task_environment_;
 
   ScopedAssistantClient assistant_client_;
@@ -430,8 +440,8 @@ TEST_F(AssistantManagerServiceImplTest,
 
   WaitForState(AssistantManagerService::STARTED);
 
-  EXPECT_EQ("<user-id>", fake_assistant_manager()->user_id());
-  EXPECT_EQ("<access-token>", fake_assistant_manager()->access_token());
+  EXPECT_EQ("<user-id>", mojom_service_controller().gaia_id());
+  EXPECT_EQ("<access-token>", mojom_service_controller().access_token());
 }
 
 TEST_F(AssistantManagerServiceImplTest, ShouldPassUserInfoToAssistantManager) {
@@ -440,9 +450,10 @@ TEST_F(AssistantManagerServiceImplTest, ShouldPassUserInfoToAssistantManager) {
 
   assistant_manager_service()->SetUser(
       UserInfo("<new-user-id>", "<new-access-token>"));
+  RunUntilIdle();
 
-  EXPECT_EQ("<new-user-id>", fake_assistant_manager()->user_id());
-  EXPECT_EQ("<new-access-token>", fake_assistant_manager()->access_token());
+  EXPECT_EQ("<new-user-id>", mojom_service_controller().gaia_id());
+  EXPECT_EQ("<new-access-token>", mojom_service_controller().access_token());
 }
 
 TEST_F(AssistantManagerServiceImplTest,
@@ -451,9 +462,10 @@ TEST_F(AssistantManagerServiceImplTest,
   WaitForState(AssistantManagerService::STARTED);
 
   assistant_manager_service()->SetUser(base::nullopt);
+  RunUntilIdle();
 
-  EXPECT_EQ(kNoValue, fake_assistant_manager()->user_id());
-  EXPECT_EQ(kNoValue, fake_assistant_manager()->access_token());
+  EXPECT_EQ(kNoValue, mojom_service_controller().gaia_id());
+  EXPECT_EQ(kNoValue, mojom_service_controller().access_token());
 }
 
 TEST_F(AssistantManagerServiceImplTest,

chromeos/services/assistant/proxy/service_controller_proxy.cc

@@ -49,11 +49,6 @@ struct StartArguments {
   assistant_client::ConversationStateListener* conversation_state_listener;
   assistant_client::DeviceStateListener* device_state_listener;
   CrosDisplayConnection* display_connection;
-  std::string libassistant_config;
-  std::string locale;
-  std::string locale_override;
-  bool spoken_feedback_enabled;
-  ServiceControllerProxy::AuthTokens auth_tokens;
 };
 
 void FillServerExperimentIds(std::vector<std::string>* server_experiment_ids) {
@@ -78,25 +73,6 @@ void SetServerExperiments(
   }
 }
 
-void SetInternalOptions(
-    assistant_client::AssistantManagerInternal* assistant_manager_internal,
-    const std::string& locale,
-    bool spoken_feedback_enabled) {
-  auto* internal_options =
-      assistant_manager_internal->CreateDefaultInternalOptions();
-  SetAssistantOptions(internal_options, locale, spoken_feedback_enabled);
-
-  internal_options->SetClientControlEnabled(
-      assistant::features::IsRoutinesEnabled());
-
-  if (!features::IsVoiceMatchDisabled())
-    internal_options->EnableRequireVoiceMatchVerification();
-
-  assistant_manager_internal->SetOptions(*internal_options, [](bool success) {
-    DVLOG(2) << "set options: " << success;
-  });
-}
-
 // TODO(b/171748795): This should all be migrated to the mojom service, which
 // should be responsible for the complete creation of the Libassistant
 // objects.
@@ -105,11 +81,8 @@ void InitializeAssistantManager(
     StartArguments arguments,
     assistant_client::AssistantManager* assistant_manager,
     assistant_client::AssistantManagerInternal* assistant_manager_internal) {
-  SetInternalOptions(assistant_manager_internal, arguments.locale,
-                     arguments.spoken_feedback_enabled);
   assistant_manager_internal->SetDisplayConnection(
       arguments.display_connection);
-  assistant_manager_internal->SetLocaleOverride(arguments.locale_override);
   assistant_manager_internal->RegisterActionModule(arguments.action_module);
   assistant_manager_internal->SetAssistantManagerDelegate(
       arguments.assistant_manager_delegate);
@@ -119,7 +92,19 @@ void InitializeAssistantManager(
       arguments.conversation_state_listener);
   assistant_manager->AddDeviceStateListener(arguments.device_state_listener);
   SetServerExperiments(assistant_manager_internal);
-  assistant_manager->SetAuthTokens(arguments.auth_tokens);
+}
+
+std::vector<libassistant::mojom::AuthenticationTokenPtr>
+ToMojomAuthenticationTokens(const ServiceControllerProxy::AuthTokens& tokens) {
+  std::vector<libassistant::mojom::AuthenticationTokenPtr> result;
+
+  for (const std::pair<std::string, std::string>& token : tokens) {
+    result.emplace_back(libassistant::mojom::AuthenticationTokenPtr(
+        base::in_place, /*gaia_id=*/token.first,
+        /*access_token=*/token.second));
+  }
+
+  return result;
 }
 
 }  // namespace
@@ -160,6 +145,9 @@ void ServiceControllerProxy::Start(
 
   // The mojom service will create the |AssistantManager|.
   service_controller_remote_->Initialize(libassistant_config);
+  service_controller_remote_->SetLocaleOverride(locale_override);
+  UpdateInternalOptions(locale, spoken_feedback_enabled);
+  SetAuthTokens(auth_tokens);
   service_controller_remote_->Start();
 
   // We need to initialize the |AssistantManager| once it's created and before
@@ -171,10 +159,6 @@ void ServiceControllerProxy::Start(
   arguments.conversation_state_listener = conversation_state_listener;
   arguments.device_state_listener = device_state_listener;
   arguments.display_connection = pending_display_connection_.get();
-  arguments.locale = locale;
-  arguments.locale_override = locale_override;
-  arguments.spoken_feedback_enabled = spoken_feedback_enabled;
-  arguments.auth_tokens = auth_tokens;
 
   host_->SetInitializeCallback(
       base::BindOnce(InitializeAssistantManager, std::move(arguments)));
@@ -196,12 +180,13 @@ void ServiceControllerProxy::Stop() {
 void ServiceControllerProxy::UpdateInternalOptions(
     const std::string& locale,
     bool spoken_feedback_enabled) {
-  SetInternalOptions(assistant_manager_internal(), locale,
+  service_controller_remote_->SetInternalOptions(locale,
                                                  spoken_feedback_enabled);
 }
 
 void ServiceControllerProxy::SetAuthTokens(const AuthTokens& tokens) {
-  assistant_manager()->SetAuthTokens(tokens);
+  service_controller_remote_->SetAuthenticationTokens(
+      ToMojomAuthenticationTokens(tokens));
 }
 
 bool ServiceControllerProxy::IsStarted() const {

chromeos/services/assistant/test_support/fake_service_controller.cc

@@ -49,6 +49,20 @@ void FakeServiceController::UnblockStartCalls() {
   start_mutex_.unlock();
 }
 
+std::string FakeServiceController::access_token() {
+  if (authentication_tokens_.size())
+    return authentication_tokens_[0]->access_token;
+  else
+    return kNoValue;
+}
+
+std::string FakeServiceController::gaia_id() {
+  if (authentication_tokens_.size())
+    return authentication_tokens_[0]->gaia_id;
+  else
+    return kNoValue;
+}
+
 void FakeServiceController::Initialize(const std::string& libassistant_config) {
   libassistant_config_ = libassistant_config;
 }
@@ -80,6 +94,11 @@ void FakeServiceController::AddAndFireStateObserver(
   state_observers_.Add(std::move(observer));
 }
 
+void FakeServiceController::SetAuthenticationTokens(
+    std::vector<libassistant::mojom::AuthenticationTokenPtr> tokens) {
+  authentication_tokens_ = std::move(tokens);
+}
+
 }  // namespace assistant
 
 }  // namespace chromeos

chromeos/services/assistant/test_support/fake_service_controller.h

@@ -25,6 +25,15 @@ namespace assistant {
 // any state change, just like the real implementation.
 class FakeServiceController : public libassistant::mojom::ServiceController {
  public:
+  // Value returned when optional fields |access_token| or |user_id| are
+  // missing. Note we use this instead of a |base::Optional| because this
+  // results in a much nicer error message if the test fails. (otherwise you get
+  // a message like this:
+  //     Expected equality of these values:
+  //           "<new-user-id-wrong>"
+  //     with 32-byte object <01-00 snip 00-00>
+  static constexpr const char* kNoValue = "<no-value>";
+
   using State = libassistant::mojom::ServiceState;
   using InitializeCallback =
       base::OnceCallback<void(assistant_client::AssistantManager*,
@@ -56,6 +65,13 @@ class FakeServiceController : public libassistant::mojom::ServiceController {
   void BlockStartCalls();
   void UnblockStartCalls();
 
+  // Return the access-token that was passed to |SetAuthenticationTokens|, or
+  // |kNoValue| if an empty vector was passed in.
+  std::string access_token();
+  // Return the user-id that was passed to |SetAuthenticationTokens|, or
+  // |kNoValue| if an empty vector was passed in.
+  std::string gaia_id();
+
   // mojom::ServiceController implementation:
   void Initialize(const std::string& libassistant_config) override;
   void Start() override;
@@ -63,6 +79,11 @@ class FakeServiceController : public libassistant::mojom::ServiceController {
   void AddAndFireStateObserver(
       mojo::PendingRemote<libassistant::mojom::StateObserver> pending_observer)
       override;
+  void SetLocaleOverride(const std::string&) override {}
+  void SetInternalOptions(const std::string& locale,
+                          bool spoken_feedback_enabled) override {}
+  void SetAuthenticationTokens(
+      std::vector<libassistant::mojom::AuthenticationTokenPtr> tokens) override;
 
  private:
   // Mutex taken in |Start| to allow the calls to block if |BlockStartCalls| was
@@ -72,6 +93,10 @@ class FakeServiceController : public libassistant::mojom::ServiceController {
   // Config passed to LibAssistant when it was started.
   std::string libassistant_config_;
 
+  // Authentication tokens passed to SetAuthenticationTokens().
+  std::vector<libassistant::mojom::AuthenticationTokenPtr>
+      authentication_tokens_;
+
   InitializeCallback initialize_callback_;
 
   State state_ = State::kStopped;

chromeos/services/libassistant/public/mojom/service_controller.mojom

@@ -32,6 +32,17 @@ interface ServiceController {
   // Stop the service. Will be a noop if the service is already stopped.
   Stop();
 
+  // Sets the locale override of the device. This field overrides the
+  // value obtained from the user's assistant settings.
+  SetLocaleOverride(string locale_override);
+
+  // Sets the internal Libassistant options.
+  SetInternalOptions(string locale, bool spoken_feedback_enabled);
+
+  // Sets the authentication tokens used by Libassistant.
+  // Passing in an empty array will restart Libassistant in signed-out mode.
+  SetAuthenticationTokens(array<AuthenticationToken> tokens);
+
   // Add a state observer. It will immediately be called with the current state,
   // and then once for each state change.
   AddAndFireStateObserver(pending_remote<StateObserver> observer);
@@ -44,3 +55,9 @@ interface StateObserver {
   // Will never be called twice consecutively with the same state.
   OnStateChanged(ServiceState new_state);
 };
+
+// A token used by Libassistant to authenticate the user.
+struct AuthenticationToken {
+  string gaia_id;
+  string access_token;
+};

chromeos/services/libassistant/service_controller.cc

@@ -7,14 +7,31 @@
 #include <memory>
 
 #include "base/check.h"
+#include "chromeos/assistant/internal/internal_util.h"
+#include "chromeos/services/assistant/public/cpp/features.h"
 #include "chromeos/services/assistant/public/cpp/migration/assistant_manager_service_delegate.h"
 #include "chromeos/services/assistant/public/cpp/migration/libassistant_v1_api.h"
+#include "libassistant/shared/internal_api/assistant_manager_internal.h"
 
 namespace chromeos {
 namespace libassistant {
 
+namespace {
+
 using mojom::ServiceState;
 
+std::vector<std::pair<std::string, std::string>> ToAuthTokens(
+    const std::vector<mojom::AuthenticationTokenPtr>& mojo_tokens) {
+  std::vector<std::pair<std::string, std::string>> result;
+
+  for (const auto& token : mojo_tokens)
+    result.emplace_back(token->gaia_id, token->access_token);
+
+  return result;
+}
+
+}  // namespace
+
 ServiceController::ServiceController(
     assistant::AssistantManagerServiceDelegate* delegate,
     assistant_client::PlatformApi* platform_api)
@@ -102,6 +119,39 @@ void ServiceController::AddAndFireStateObserver(
   state_observers_.Add(std::move(observer));
 }
 
+void ServiceController::SetLocaleOverride(const std::string& value) {
+  DCHECK(IsInitialized());
+
+  assistant_manager_internal()->SetLocaleOverride(value);
+}
+
+void ServiceController::SetInternalOptions(const std::string& locale,
+                                           bool spoken_feedback_enabled) {
+  DCHECK(IsInitialized());
+
+  auto* internal_options =
+      assistant_manager_internal()->CreateDefaultInternalOptions();
+  assistant::SetAssistantOptions(internal_options, locale,
+                                 spoken_feedback_enabled);
+
+  internal_options->SetClientControlEnabled(
+      assistant::features::IsRoutinesEnabled());
+
+  if (!assistant::features::IsVoiceMatchDisabled())
+    internal_options->EnableRequireVoiceMatchVerification();
+
+  assistant_manager_internal()->SetOptions(*internal_options, [](bool success) {
+    DVLOG(2) << "set options: " << success;
+  });
+}
+
+void ServiceController::SetAuthenticationTokens(
+    std::vector<mojom::AuthenticationTokenPtr> tokens) {
+  DCHECK(IsInitialized());
+
+  assistant_manager()->SetAuthTokens(ToAuthTokens(tokens));
+}
+
 void ServiceController::AddAndFireAssistantManagerObserver(
     AssistantManagerObserver* observer) {
   DCHECK(observer);

chromeos/services/libassistant/service_controller.h

@@ -67,6 +67,11 @@ class COMPONENT_EXPORT(LIBASSISTANT_SERVICE) ServiceController
   void Stop() override;
   void AddAndFireStateObserver(
       mojo::PendingRemote<mojom::StateObserver> observer) override;
+  void SetLocaleOverride(const std::string& value) override;
+  void SetInternalOptions(const std::string& locale,
+                          bool spoken_feedback_enabled) override;
+  void SetAuthenticationTokens(
+      std::vector<mojom::AuthenticationTokenPtr> tokens) override;
 
   void AddAndFireAssistantManagerObserver(AssistantManagerObserver* observer);
   void RemoveAssistantManagerObserver(AssistantManagerObserver* observer);