Adding policy support to the new network configuration stack.

chromeos/network/managed_network_configuration_handler.h

 // Copyright (c) 2013 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.
 
 #ifndef CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 #define CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_
 
+#include <map>
 #include <string>
-#include <vector>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
 #include "base/gtest_prod_util.h"
+#include "base/memory/weak_ptr.h"
 #include "chromeos/chromeos_export.h"
 #include "chromeos/network/network_handler_callbacks.h"
+#include "chromeos/network/onc/onc_constants.h"
 
 namespace base {
 class DictionaryValue;
 }
 
 namespace chromeos {
 
 // The ManagedNetworkConfigurationHandler class is used to create and configure
 // networks in ChromeOS using ONC.
 //
@@ skipping 34 matching lines @@
 
   // Initialize() must be called before this.
   static ManagedNetworkConfigurationHandler* Get();
 
   // Provides the properties of the network with |service_path| to |callback|.
   void GetProperties(
       const std::string& service_path,
       const network_handler::DictionaryResultCallback& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
+  // Provides the managed properties of the network with |service_path| to
+  // |callback|.
+  void GetManagedProperties(
+      const std::string& service_path,
+      const network_handler::DictionaryResultCallback& callback,
+      const network_handler::ErrorCallback& error_callback);
+
   // Sets the user's settings of an already configured network with
   // |service_path|. A network can be initially configured by calling
   // CreateConfiguration or if it is managed by a policy. The given properties
   // will be merged with the existing settings, and it won't clear any existing
   // properties.
   void SetProperties(
       const std::string& service_path,
-      const base::DictionaryValue& properties,
+      const base::DictionaryValue& user_settings,
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
   // Initiates a connection with network that has |service_path|. |callback| is
   // called if the connection request was successfully handled. That doesn't
   // mean that the connection was successfully established.
   void Connect(const std::string& service_path,
                const base::Closure& callback,
                const network_handler::ErrorCallback& error_callback) const;
 
@@ skipping 14 matching lines @@
       const network_handler::ErrorCallback& error_callback) const;
 
   // Removes the user's configuration from the network with |service_path|. The
   // network may still show up in the visible networks after this, but no user
   // configuration will remain. If it was managed, it will still be configured.
   void RemoveConfiguration(
       const std::string& service_path,
       const base::Closure& callback,
       const network_handler::ErrorCallback& error_callback) const;
 
+  // Only to be called by NetworkConfigurationUpdater.

[stevenjb, 2013/04/11 18:20:08]

Also describe what the method does.

[pneubeck (no reviews), 2013/04/15 12:16:24]

Done.

+  void SetPolicy(onc::ONCSource onc_source,
+                 const base::DictionaryValue& onc_dict);
+
  private:
+  typedef std::map<std::string, const base::DictionaryValue*> PolicyMap;
+
+  class PolicyApplicator;
+
   ManagedNetworkConfigurationHandler();
   ~ManagedNetworkConfigurationHandler();
 
+  void GetManagedPropertiesCallback(
+      const network_handler::DictionaryResultCallback& callback,
+      const network_handler::ErrorCallback& error_callback,
+      const std::string& service_path,
+      const base::DictionaryValue& shill_properties);
+
+  const PolicyMap* GetPoliciesForProfile(const std::string& profile) const;
+
+  scoped_ptr<PolicyMap> user_policies_by_guid_;
+  scoped_ptr<PolicyMap> device_policies_by_guid_;

[stevenjb, 2013/04/11 18:20:08]

This is pretty confusing; it's a pointer to a map of pointers.

Do we actually need to pass ownership of these maps elsewhere, or could we just
make these maps, and convert:

user_policies_by_guid_ = GetFoo(..)
to
SetFoo(..., &user_policies_by_guid_);

And

!user_policies_by_guid_ to user_policies_by_guid_.empty()?

Or do we need to differentiate between an empty map and an unset one?

There is also the question of who owns the Dictionary*'s in the maps. A scoped
pointer implies that destroying the member will clean up any contents, but it
appears that is not the case. If we need to differentiate between empty and
NULL, then I would use raw *'s here, not scoped_ptr.

[pneubeck (no reviews), 2013/04/15 12:16:24]

Yes, that must be differentiated.
In the previous patch I used a separate boolean "...is_initialized". Which
Julian thought not to be ideal.
What I like about the current usage of "pointer to map" is that every user of
the map is reminded by the type that the pointer might be NULL.
What I don't like is the bloated syntax in some of the functions.
As in other cases of map<*> usages of the code base, I added a clarifying
comment about the ownership.
Alternatively, I could change the Map to DictionaryValue, which however drops
the type information about it's content (the entries will be of type Value
instead of DictionaryValue).


I think, we'll have to come up with a compromise here.
Reverting to the booleans "...initialized_" as in the previous patch? At least
the semantics are more obvious then and don't infer with ownership.

[stevenjb, 2013/04/15 17:38:54]

How about raw *'s here then, instead of scoped_ptr? scoped_ptr is great when a
member fully cleans itself up, but that is not the case here.

Alternatively, make PolicyMap a class with a destructor that does the cleanup
instead of a typedef.

+
+  // For Shill client callbacks
+  base::WeakPtrFactory<ManagedNetworkConfigurationHandler> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(ManagedNetworkConfigurationHandler);
 };
 
 }  // namespace chromeos
 
 #endif  // CHROMEOS_NETWORK_MANAGED_NETWORK_CONFIGURATION_HANDLER_H_