Disable some API calls in networkingPrivate for non-primary user

chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc

Index: chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
diff --git a/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc b/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
index 81bda9eec2e36073a7b70b72fede42f2283161c2..56c6284e53be1e9373c4938b43ec9da29ce9d11a 100644
--- a/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
+++ b/chrome/browser/extensions/api/networking_private/networking_private_api_chromeos.cc
@@ -7,17 +7,16 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
-#include "chrome/browser/browser_process.h"
-#include "chrome/browser/browser_process_platform_part_chromeos.h"
 #include "chrome/browser/chromeos/net/network_portal_detector.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
-#include "chrome/browser/profiles/profiles_state.h"
 #include "chrome/common/extensions/api/networking_private.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_manager_client.h"
+#include "chromeos/login/login_state.h"
 #include "chromeos/network/managed_network_configuration_handler.h"
 #include "chromeos/network/network_connection_handler.h"
 #include "chromeos/network/network_device_handler.h"
+#include "chromeos/network/network_event_log.h"
 #include "chromeos/network/network_state.h"
 #include "chromeos/network/network_state_handler.h"
 #include "chromeos/network/network_util.h"
@@ -25,6 +24,7 @@
 #include "chromeos/network/onc/onc_translator.h"
 #include "chromeos/network/onc/onc_utils.h"
 #include "components/onc/onc_constants.h"
+#include "content/public/browser/browser_context.h"
 #include "extensions/browser/extension_function_registry.h"
 
 namespace api = extensions::api::networking_private;
@@ -62,9 +62,20 @@ ShillManagerClient::VerificationProperties ConvertVerificationProperties(
   return output;
 }
 
-std::string GetUserIdHash(Profile* profile) {
-  return g_browser_process->platform_part()->
-      profile_helper()->GetUserIdHashFromProfile(profile);
+bool GetUserIdHash(content::BrowserContext* browser_context,
+                   std::string* user_hash) {
+  // Currently Chrome OS only configures networks for the primary user.
+  // Configuration attempts from other profiles should fail. TODO(stevenjb):
+  // use an ExtensionsBrowserClient method to access ProfileHelper when moving
+  // this to src/extensions.
+  std::string current_user_hash =
+      chromeos::ProfileHelper::GetUserIdHashFromProfile(
+          static_cast<Profile*>(browser_context));
+
+  if (current_user_hash != chromeos::LoginState::Get()->primary_user_hash())
+    return false;
+  *user_hash = current_user_hash;
+  return true;
 }
 
 bool GetServicePathFromGuid(const std::string& guid,
@@ -137,7 +148,15 @@ bool NetworkingPrivateGetManagedPropertiesFunction::RunAsync() {
     return false;
 
   std::string user_id_hash;
-  GetUserIdHash(GetProfile());
+  if (!GetUserIdHash(browser_context(), &user_id_hash)) {
+    // It is OK to call getManagedProperties from a non-primary user context
+    // for the purposes of displaying network properites. |user_id_hash| will

[pneubeck (no reviews), 2014/06/12 09:51:00]

typo: properites -> properties

[stevenjb, 2014/06/12 20:27:51]

Done.

+    // be empty, so no properties from the profile (e.g. proxy config) will be

[pneubeck (no reviews), 2014/06/12 18:26:18]

this comment is still wrong / misleading and brings up a good point.
Is 'profile' a Shill profile or a Chrome profile?
proxy config is so far not handled at all by MNCH and the extension API.
As mentioned, MNCH was written with the assumption of the user hash not empty.

But again, we can fix that after this commit if you like that better (then
please add a todo).

[stevenjb, 2014/06/12 20:27:51]

Disallowing for now to simplify.

+    // included.

[pneubeck (no reviews), 2014/06/12 09:51:00]

the MNCH wasn't written with empty user hashes in mind. This usage will require
an audit / more tests / updated comments of MNCH.

[pneubeck (no reviews), 2014/06/12 10:17:47]

Thinking more about it, MNCH shouldn't ever get an empty user hash so that we
can ensure consistent policy enforcement.
All restrictions that we want to apply in the extension API should happen here
and shouldn't be able to undo what MNCH decides (what policies enforced).

That means, we can either pass a boolean primary_user (together with the a
non-empty hash) to the MNCH and make it multi-profile aware.
Or, we add the additional restrictions here (I think I prefer this, as these
restrictions seem a very extension-API specific to me and might not apply to
other consumers).


E.g. I think we should enforce if called from secondary profile:
- GetState: always ok
- CreateNetwork, SetProperties: always fail as it might lead to surprising
effects because of different user policies.
- GetProperties, GetManagedProperties: fail if not shared in order to not leak
sensitive information / to preserve privacy
- Get*Networks: maybe should drop all disconnected/invisible private networks as
these are useless anyways with the above restrictions

Considering how subtle this is and how much effort it would take, I would just
disable the whole API for secondary profiles if  there's no strong reason to do
otherwise.

[stevenjb, 2014/06/12 20:27:51]

For now I am only disallowing calls that require a user id hash so that we can
avoid an immediate audit of MNCH. If/when we expose the API publically (or
expand its usage) we will want to do a more thorough security/privacy review.

That said, I think that MNCSH should Do The Right Thing if an empty user id hash
is passed to it (see note below).

+    NET_LOG_DEBUG("getManagedProperties called from non primary user context",
+                  browser_context()->GetPath().value());
+  }
+
   NetworkHandler::Get()->managed_network_configuration_handler()->
       GetManagedProperties(
           user_id_hash,
@@ -249,8 +268,15 @@ bool NetworkingPrivateCreateNetworkFunction::RunAsync() {
   EXTENSION_FUNCTION_VALIDATE(params);
 
   std::string user_id_hash;

[stevenjb, 2014/06/12 20:27:51]

We are already currently passing an empty user_id_hash to MNCH to indicate a
shared network configuration. I think this should be handled (which may include
failure if policy should disallow it) by MNCH.

[pneubeck (no reviews), 2014/06/13 08:51:12]

Whether empty user_id_hash is the right way to communicate the shared bit is
debatable. But anyways, yes, this must be handled correctly by MNCH soon.

-  if (!params->shared)
-    user_id_hash = GetUserIdHash(GetProfile());
+  if (!params->shared &&
+      !GetUserIdHash(browser_context(), &user_id_hash)) {
+    // Do not allow configuring a non-shared network from a non-primary user
+    // context.
+    NET_LOG_ERROR("createNetwork called from non primary user.",
+                  browser_context()->GetPath().value());
+    error_ = "Error.NonPrimaryUser";
+    return false;
+  }
 
   scoped_ptr<base::DictionaryValue> properties_dict(
       params->properties.ToValue());