Save metadata about the creator of a network.

Previously we were saving a boolean value to the shared profile, so any
user who checked would read true.  The new implementation stores the
user_hash instead.  Also updated the unit tests to catch this situation.

Bug: 966270
Change-Id: If4d2601894ece4eff4180b83979097fb1d47eb05
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2151701
Commit-Queue: Jon Mann <jonmann@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Reviewed-by: Azeem Arshad <azeemarshad@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#766452}

chromeos/components/sync_wifi/local_network_collector_impl_unittest.cc

@@ -94,6 +94,15 @@ class LocalNetworkCollectorImplTest : public testing::Test {
     return local_network_collector_.get();
   }
 
+  void TestGetSyncableNetwork(const std::string& guid,
+                              const std::string& expected_ssid) {
+    local_network_collector()->GetSyncableNetwork(
+        guid,
+        base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
+                       base::Unretained(this), expected_ssid));
+    base::RunLoop().RunUntilIdle();
+  }
+
   NetworkTestHelper* helper() { return local_test_helper_.get(); }
 
  private:
@@ -154,9 +163,7 @@ TEST_F(LocalNetworkCollectorImplTest, TestGetSyncableNetwork) {
   std::string guid = helper()->ConfigureWiFiNetwork(
       kFredSsid, /*is_secured=*/true,
       /*in_profile=*/true, /*has_connected=*/true);
-  local_network_collector()->GetSyncableNetwork(
-      guid, base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
-                           base::Unretained(this), kFredSsid));
+  TestGetSyncableNetwork(guid, kFredSsid);
 }
 
 TEST_F(LocalNetworkCollectorImplTest,
@@ -164,9 +171,7 @@ TEST_F(LocalNetworkCollectorImplTest,
   std::string guid = helper()->ConfigureWiFiNetwork(
       kFredSsid, /*is_secured=*/true,
       /*in_profile=*/false, /*has_connected=*/true, /*owned_by_user=*/true);
-  local_network_collector()->GetSyncableNetwork(
-      guid, base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
-                           base::Unretained(this), kFredSsid));
+  TestGetSyncableNetwork(guid, kFredSsid);
 }
 
 TEST_F(LocalNetworkCollectorImplTest,
@@ -174,26 +179,18 @@ TEST_F(LocalNetworkCollectorImplTest,
   std::string guid = helper()->ConfigureWiFiNetwork(
       kFredSsid, /*is_secured=*/true,
       /*in_profile=*/false, /*has_connected=*/true, /*owned_by_user=*/false);
-  local_network_collector()->GetSyncableNetwork(
-      guid, base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
-                           base::Unretained(this), std::string()));
+  TestGetSyncableNetwork(guid, /*expected_ssid=*/std::string());
 }
 
 TEST_F(LocalNetworkCollectorImplTest, TestGetSyncableNetwork_DoesntExist) {
-  local_network_collector()->GetSyncableNetwork(
-      "test_guid",
-      base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
-                     base::Unretained(this), std::string()));
+  TestGetSyncableNetwork("test_guid", /*expected_ssid=*/std::string());
 }
 
 TEST_F(LocalNetworkCollectorImplTest, TestGetSyncableNetwork_NeverConnected) {
   std::string guid = helper()->ConfigureWiFiNetwork(
       kFredSsid, /*is_secured=*/true,
       /*in_profile=*/true, /*has_connected=*/false);
-
-  local_network_collector()->GetSyncableNetwork(
-      guid, base::BindOnce(&LocalNetworkCollectorImplTest::OnGetSyncableNetwork,
-                           base::Unretained(this), std::string()));
+  TestGetSyncableNetwork(guid, /*expected_ssid=*/std::string());
 }
 
 }  // namespace sync_wifi

chromeos/components/sync_wifi/network_test_helper.cc

@@ -12,10 +12,12 @@
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_metadata_store.h"
 #include "chromeos/services/network_config/in_process_instance.h"
+#include "components/account_id/account_id.h"
 #include "components/onc/onc_pref_names.h"
 #include "components/proxy_config/pref_proxy_config_tracker_impl.h"
 #include "components/proxy_config/proxy_config_pref_names.h"
 #include "components/user_manager/fake_user_manager.h"
+#include "components/user_manager/scoped_user_manager.h"
 
 namespace chromeos {
 
@@ -53,11 +55,13 @@ NetworkTestHelper::NetworkTestHelper()
       /*global_network_config=*/base::DictionaryValue());
 
   auto fake_user_manager = std::make_unique<user_manager::FakeUserManager>();
+  auto primary_account_id = AccountId::FromUserEmail("primary@test.com");
+  auto secondary_account_id = AccountId::FromUserEmail("secondary@test.com");
+  primary_user_ = fake_user_manager->AddUser(primary_account_id);
+  secondary_user_ = fake_user_manager->AddUser(secondary_account_id);
   scoped_user_manager_ = std::make_unique<user_manager::ScopedUserManager>(
       std::move(fake_user_manager));
-
-  LoginState::Get()->SetLoggedInState(LoginState::LOGGED_IN_ACTIVE,
-                                      LoginState::LOGGED_IN_USER_REGULAR);
+  LoginUser(primary_user_);
 
   Initialize(managed_network_configuration_handler_.get());
 }
@@ -77,6 +81,14 @@ void NetworkTestHelper::SetUp() {
   base::RunLoop().RunUntilIdle();
 }
 
+void NetworkTestHelper::LoginUser(const user_manager::User* user) {
+  auto* user_manager = static_cast<user_manager::FakeUserManager*>(
+      user_manager::UserManager::Get());
+  user_manager->UserLoggedIn(user->GetAccountId(), user->username_hash(),
+                             true /* browser_restart */, false /* is_child */);
+  user_manager->SwitchActiveUser(user->GetAccountId());
+}
+
 std::string NetworkTestHelper::ConfigureWiFiNetwork(const std::string& ssid,
                                                     bool is_secured,
                                                     bool in_profile,
@@ -100,9 +112,16 @@ std::string NetworkTestHelper::ConfigureWiFiNetwork(const std::string& ssid,
 
   base::RunLoop().RunUntilIdle();
 
-  if (owned_by_user) {
-    NetworkHandler::Get()->network_metadata_store()->OnConfigurationCreated(
-        service_path, guid);
+  if (!in_profile) {
+    if (owned_by_user) {
+      NetworkHandler::Get()->network_metadata_store()->OnConfigurationCreated(
+          service_path, guid);
+    } else {
+      LoginUser(secondary_user_);
+      NetworkHandler::Get()->network_metadata_store()->OnConfigurationCreated(
+          service_path, guid);
+      LoginUser(primary_user_);
+    }
   }
 
   if (has_connected) {

chromeos/components/sync_wifi/network_test_helper.h

@@ -16,7 +16,11 @@
 #include "chromeos/services/network_config/cros_network_config.h"
 #include "chromeos/services/network_config/public/cpp/cros_network_config_test_helper.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
-#include "components/user_manager/scoped_user_manager.h"
+
+namespace user_manager {
+class ScopedUserManager;
+class User;
+}  // namespace user_manager
 
 namespace chromeos {
 
@@ -41,6 +45,8 @@ class NetworkTestHelper : public network_config::CrosNetworkConfigTestHelper {
   NetworkStateTestHelper* network_state_test_helper();
 
  private:
+  void LoginUser(const user_manager::User* user);
+
   std::unique_ptr<NetworkProfileHandler> network_profile_handler_;
   std::unique_ptr<NetworkConfigurationHandler> network_configuration_handler_;
   std::unique_ptr<ManagedNetworkConfigurationHandler>
@@ -48,6 +54,10 @@ class NetworkTestHelper : public network_config::CrosNetworkConfigTestHelper {
   std::unique_ptr<UIProxyConfigService> ui_proxy_config_service_;
   std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;
   sync_preferences::TestingPrefServiceSyncable user_prefs_;
+
+  const user_manager::User* primary_user_;
+  const user_manager::User* secondary_user_;
+
   TestingPrefServiceSimple local_state_;
 };
 

chromeos/network/BUILD.gn

@@ -227,6 +227,7 @@ source_set("unit_tests") {
     "//components/prefs:test_support",
     "//components/proxy_config",
     "//components/sync_preferences:test_support",
+    "//components/user_manager:test_support",
     "//crypto",
     "//crypto:test_support",
     "//dbus",

chromeos/network/network_metadata_store.cc

@@ -8,12 +8,15 @@
 #include "base/time/time.h"
 #include "chromeos/network/network_configuration_handler.h"
 #include "chromeos/network/network_connection_handler.h"
+#include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_handler.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "components/user_manager/user.h"
+#include "components/user_manager/user_manager.h"
 #include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
 
 namespace chromeos {
@@ -23,7 +26,7 @@ namespace {
 const char kNetworkMetadataPref[] = "network_metadata";
 const char kLastConnectedTimestampPref[] = "last_connected_timestamp";
 const char kIsFromSync[] = "is_from_sync";
-const char kIsCreatedByUser[] = "is_created_by_user";
+const char kOwner[] = "owner";
 
 std::string GetPath(const std::string& guid, const std::string& subkey) {
   return base::StringPrintf("%s.%s", guid.c_str(), subkey.c_str());
@@ -88,7 +91,18 @@ void NetworkMetadataStore::ConnectSucceeded(const std::string& service_path) {
 void NetworkMetadataStore::OnConfigurationCreated(
     const std::string& service_path,
     const std::string& guid) {
-  SetPref(guid, kIsCreatedByUser, base::Value(true));
+  if (!user_manager::UserManager::IsInitialized())
+    return;
+
+  const user_manager::User* user =
+      user_manager::UserManager::Get()->GetActiveUser();
+  if (!user) {
+    NET_LOG(EVENT)
+        << "Network added with no active user, owner metadata not recorded.";
+    return;
+  }
+
+  SetPref(guid, kOwner, base::Value(user->username_hash()));
 }
 
 void NetworkMetadataStore::OnConfigurationModified(
@@ -175,13 +189,18 @@ bool NetworkMetadataStore::GetIsConfiguredBySync(
 }
 
 bool NetworkMetadataStore::GetIsCreatedByUser(const std::string& network_guid) {
-  const base::Value* is_created_by_user =
-      GetPref(network_guid, kIsCreatedByUser);
-  if (!is_created_by_user) {
+  const base::Value* owner = GetPref(network_guid, kOwner);
+  if (!owner) {
+    return false;
+  }
+
+  const user_manager::User* user =
+      user_manager::UserManager::Get()->GetActiveUser();
+  if (!user) {
     return false;
   }
 
-  return is_created_by_user->GetBool();
+  return owner->GetString() == user->username_hash();
 }
 
 void NetworkMetadataStore::SetPref(const std::string& network_guid,

chromeos/network/network_metadata_store_unittest.cc

@@ -16,9 +16,12 @@
 #include "chromeos/network/network_metadata_store.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_state_test_helper.h"
+#include "components/account_id/account_id.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/testing_pref_service.h"
 #include "components/sync_preferences/testing_pref_service_syncable.h"
+#include "components/user_manager/fake_user_manager.h"
+#include "components/user_manager/scoped_user_manager.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
@@ -81,6 +84,15 @@ class NetworkMetadataStoreTest : public ::testing::Test {
     NetworkMetadataStore::RegisterPrefs(user_prefs_->registry());
     NetworkMetadataStore::RegisterPrefs(device_prefs_->registry());
 
+    auto fake_user_manager = std::make_unique<user_manager::FakeUserManager>();
+    auto account_id = AccountId::FromUserEmail("account@test.com");
+    const user_manager::User* user = fake_user_manager->AddUser(account_id);
+    fake_user_manager->UserLoggedIn(user->GetAccountId(), user->username_hash(),
+                                    true /* browser_restart */,
+                                    false /* is_child */);
+    scoped_user_manager_ = std::make_unique<user_manager::ScopedUserManager>(
+        std::move(fake_user_manager));
+
     metadata_store_ = std::make_unique<NetworkMetadataStore>(
         network_configuration_handler_, network_connection_handler_.get(),
         network_state_handler_, user_prefs_.get(), device_prefs_.get());
@@ -96,6 +108,7 @@ class NetworkMetadataStoreTest : public ::testing::Test {
     device_prefs_.reset();
     network_configuration_handler_ = nullptr;
     network_connection_handler_.reset();
+    scoped_user_manager_.reset();
     NetworkHandler::Shutdown();
   }
 
@@ -129,6 +142,7 @@ class NetworkMetadataStoreTest : public ::testing::Test {
   std::unique_ptr<sync_preferences::TestingPrefServiceSyncable> user_prefs_;
   std::unique_ptr<NetworkMetadataStore> metadata_store_;
   std::unique_ptr<TestNetworkMetadataObserver> metadata_observer_;
+  std::unique_ptr<user_manager::ScopedUserManager> scoped_user_manager_;
 
   DISALLOW_COPY_AND_ASSIGN(NetworkMetadataStoreTest);
 };

components/user_manager/fake_user_manager.cc

@@ -67,6 +67,8 @@ const User* FakeUserManager::AddUserWithAffiliation(const AccountId& account_id,
                                                     bool is_affiliated) {
   User* user = User::CreateRegularUser(account_id, USER_TYPE_REGULAR);
   user->SetAffiliation(is_affiliated);
+  // TODO: Merge with ProfileHelper::GetUserIdHashByUserIdForTesting.
+  user->set_username_hash(account_id.GetUserEmail() + "-hash");
   users_.push_back(user);
   return user;
 }
@@ -165,7 +167,15 @@ User* FakeUserManager::GetActiveUser() {
   return GetActiveUserInternal();
 }
 
-void FakeUserManager::SwitchActiveUser(const AccountId& account_id) {}
+void FakeUserManager::SwitchActiveUser(const AccountId& account_id) {
+  for (UserList::const_iterator it = logged_in_users_.begin();
+       it != logged_in_users_.end(); ++it) {
+    if ((*it)->GetAccountId() == account_id) {
+      active_user_ = *it;
+      break;
+    }
+  }
+}
 
 void FakeUserManager::SaveUserDisplayName(const AccountId& account_id,
                                           const base::string16& display_name) {