Use ONC for Cellular APN and SimLock in Settings

chrome/browser/resources/options/chromeos/internet_detail.js

Index: chrome/browser/resources/options/chromeos/internet_detail.js
diff --git a/chrome/browser/resources/options/chromeos/internet_detail.js b/chrome/browser/resources/options/chromeos/internet_detail.js
index 431e7d25a24a0161d69226d9aa46b9032a006a60..70984d3767bbb9cad47073264e9e9fbe389e8840 100644
--- a/chrome/browser/resources/options/chromeos/internet_detail.js
+++ b/chrome/browser/resources/options/chromeos/internet_detail.js
@@ -36,7 +36,7 @@ cr.define('options.internet', function() {
    */
   function getManagedValue(data, key, type) {
     var property = getManagedProperty(data, key);
-    if (typeof property != 'object')
+    if (Array.isArray(property) || typeof property != 'object')
       return property;
     if (type == GetManagedTypes.RECOMMENDED)
       return getRecommendedValue(property);
@@ -45,8 +45,12 @@ cr.define('options.internet', function() {
     // Otherwise get the Active value (defalt behavior).
     if ('Active' in property)
       return property['Active'];
-    // If no Active value is defined, return the effective value.
-    return getEffectiveValue(property);
+    // If no Active value is defined, return the effective value if present.
+    var effective = getEffectiveValue(property);
+    if (effective != undefined)
+      return effective;
+    // Otherwise this is an Object but not a Managed one.
+    return property;
   }
 
   /**
@@ -101,6 +105,35 @@ cr.define('options.internet', function() {
   }
 
   /**
+   * Set the value of a property in dictionary |data| that includes ONC
+   * managed properties, e.g. setManagedValue(data, 'Name', 'MyNetwork').
+   * See notes for getManagedProperty.
+   * @param {object} data The properties dictionary.
+   * @param {string} key The property key.
+   * @param {string} value The property value to set.
+   */
+  function setManagedProperty(data, key, value) {
+    while (true) {
+      var index = key.indexOf('.');
+      if (index < 0)
+        break;
+      var keyComponent = key.substr(0, index);
+      if (!(keyComponent in data))
+        data[keyComponent] = {};
+      data = data[keyComponent];
+      key = key.substr(index + 1);
+    }
+    if (!(key in data) ||
+        (typeof data[key] != 'object') || !('Active' in data[key])) {

[pneubeck (no reviews), 2014/08/22 08:55:32]

Active might not be present if the value from Shill is not translated back,
which should be legitimate in some cases.

Checking 'Active' and 'Effective' should be sufficiently safe.

[stevenjb, 2014/08/22 17:08:43]

Done.

+      data[key] = value;
+    } else {
+      // For now, just uodare the active value. TODO(stevenjb): Possibly check

[pneubeck (no reviews), 2014/08/22 08:55:32]

typo: uodare -> update

[stevenjb, 2014/08/22 17:08:43]

Done.

+      // the policy attributes and update appropriately.
+      data[key]['Active'] = value;

[pneubeck (no reviews), 2014/08/22 08:55:32]

If I get it right, then you want to use the ONC properties in |data| to store
the transient state of the UI?

To be consistent with the data structure as it is received initially from C++,
this should update:

'Active' = value
if (value != undefined) {
  'UserSetting' = value
  'Effective' = 'UserSetting'
} else {
  remove 'UserSetting'
  if ('Effective' == 'UserSetting') {
    // In general, this case is problematic, as 'Effective' should now be set to
something else than 'UserSetting'.
    // For Cellular where you know that there is no policy, you could just
remove the 'Effective' field.
  }
}
Maybe you want to double-check that 'UserEditable' or 'DeviceEditable' was true
and fail otherwise.


A more reliable approach might be to send the update request to C++, and let C++
send a new dictionary (the whole one or only for this value) to JS. That way you
would get information about whether the setting could be actually applied and
you would not have to fake the data structure.

[stevenjb, 2014/08/22 17:08:43]

My intention was to do something better than clobber any ONC properties, but not
as thorough as you describe (thus the TODO). This is only designed to update the
locally cached value.

I will add a couple of asserts, but we certainly don't want to do a round-trip
here, that's a level of refactoring I think we should avoid until we have
eliminated the C++ glue code.

+    }
+  }
+
+  /**
    * Helper function to set hidden attribute for elements matching a selector.
    * @param {string} selector CSS selector for extracting a list of elements.
    * @param {bool} hidden New hidden value.
@@ -272,25 +305,24 @@ cr.define('options.internet', function() {
           data.userApnIndex = -1;
         }
 
-        if (data.providerApnList.value.length > 0) {
-          var iApn = 0;
-          var defaultApn = data.providerApnList.value[iApn];
-          data.apn.apn = stringFromValue(defaultApn.apn);
-          data.apn.username = stringFromValue(defaultApn.username);
-          data.apn.password = stringFromValue(defaultApn.password);
+        var activeApn;
+        var iApn = -1;
+        var apnList = getManagedValue(data, 'Cellular.APNList');
+        if (apnList != undefined && apnList.length > 0) {
+          iApn = 0;
+          var defaultApn = apnList[iApn];
+          activeApn['APN'] = stringFromValue(defaultApn['APN']);
+          activeApn['Username'] = stringFromValue(defaultApn['Username']);
+          activeApn['Password'] = stringFromValue(defaultApn['Password']);
           chrome.send('setApn', [data.servicePath,
-                                 data.apn.apn,
-                                 data.apn.username,
-                                 data.apn.password]);
-          apnSelector.selectedIndex = iApn;
-          data.selectedApn = iApn;
-        } else {
-          data.apn.apn = '';
-          data.apn.username = '';
-          data.apn.password = '';
-          apnSelector.selectedIndex = -1;
-          data.selectedApn = -1;
+                                 activeApn['APN'],
+                                 activeApn['Username'],
+                                 activeApn['Password']]);
         }
+        setManagedProperty(data, 'Cellular.APN', activeApn);
+        apnSelector.selectedIndex = iApn;
+        data.selectedApn = iApn;
+
         updateHidden('.apn-list-view', false);
         updateHidden('.apn-details-view', true);
       });
@@ -302,18 +334,18 @@ cr.define('options.internet', function() {
         var data = $('connection-state').data;
         var apnSelector = $('select-apn');
 
-        data.apn.apn = stringFromValue($('cellular-apn').value);
-        data.apn.username = stringFromValue($('cellular-apn-username').value);
-        data.apn.password = stringFromValue($('cellular-apn-password').value);
-        data.userApn = {
-          'apn': data.apn.apn,
-          'username': data.apn.username,
-          'password': data.apn.password
-        };
+        var activeApn = {};
+        activeApn['APN'] = stringFromValue($('cellular-apn').value);
+        activeApn['Username'] =
+            stringFromValue($('cellular-apn-username').value);
+        activeApn['Password'] =
+            stringFromValue($('cellular-apn-password').value);
+        setManagedProperty(data, 'Cellular.APN', activeApn);
+        data.userApn = activeApn;
         chrome.send('setApn', [data.servicePath,
-                               data.apn.apn,
-                               data.apn.username,
-                               data.apn.password]);
+                               activeApn['APN'],
+                               activeApn['Username'],
+                               activeApn['Password']]);
 
         if (data.userApnIndex != -1) {
           apnSelector.remove(data.userApnIndex);
@@ -321,7 +353,7 @@ cr.define('options.internet', function() {
         }
 
         var option = document.createElement('option');
-        option.textContent = data.apn.apn;
+        option.textContent = activeApn['APN'];
         option.value = -1;
         option.selected = true;
         apnSelector.add(option, apnSelector[apnSelector.length - 1]);
@@ -342,13 +374,15 @@ cr.define('options.internet', function() {
         var data = $('connection-state').data;
         var apnSelector = $('select-apn');
         if (apnSelector[apnSelector.selectedIndex].value != -1) {
-          var apnList = data.providerApnList.value;
+          var apnList = getManagedValue(data, 'Cellular.APNList');
+          var apnIndex = apnSelector.selectedIndex;
+          assert(apnIndex < apnList.length);
+          var apnDict = apnList[apnIndex];
           chrome.send('setApn', [data.servicePath,
-              stringFromValue(apnList[apnSelector.selectedIndex].apn),
-              stringFromValue(apnList[apnSelector.selectedIndex].username),
-              stringFromValue(apnList[apnSelector.selectedIndex].password)]
-          );
-          data.selectedApn = apnSelector.selectedIndex;
+                                 stringFromValue(apnDict['Name']),

[armansito, 2014/08/22 00:56:45]

Shouldn't this be 'APN' rather than 'Name'?

[stevenjb, 2014/08/22 17:08:43]

Done.

+                                 stringFromValue(apnDict['Username']),
+                                 stringFromValue(apnDict['Password'])]);
+          data.selectedApn = apnIndex;
         } else if (apnSelector.selectedIndex == data.userApnIndex) {
           chrome.send('setApn', [data.servicePath,
                                  stringFromValue(data.userApn.apn),
@@ -356,9 +390,12 @@ cr.define('options.internet', function() {
                                  stringFromValue(data.userApn.password)]);
           data.selectedApn = apnSelector.selectedIndex;
         } else {
-          $('cellular-apn').value = stringFromValue(data.apn.apn);
-          $('cellular-apn-username').value = stringFromValue(data.apn.username);
-          $('cellular-apn-password').value = stringFromValue(data.apn.password);
+          var activeApn = getManagedValue(data, 'Cellular.APN');
+          $('cellular-apn').value = stringFromValue(activeApn['APN']);
+          $('cellular-apn-username').value =
+              stringFromValue(activeApn['Username']);
+          $('cellular-apn-password').value =
+              stringFromValue(activeApn['Password']);
 
           updateHidden('.apn-list-view', true);
           updateHidden('.apn-details-view', false);
@@ -1017,8 +1054,8 @@ cr.define('options.internet', function() {
         $('details-internet-login').hidden = true;
 
       if (detailsPage.gsm) {
-        // TODO(stevenjb): Use managed properties for policy controlled values.
-        var lockEnabled = data.simCardLockEnabled.value;
+        var lockEnabled =
+            getManagedValue(data, 'Cellular.SIMLockStatus.LockEnabled');
         $('sim-card-lock-enabled').checked = lockEnabled;
         $('change-pin').hidden = !lockEnabled;
       }
@@ -1315,7 +1352,7 @@ cr.define('options.internet', function() {
       setOrHideParent('prl-version',
                       getManagedValue(data, 'Cellular.PRLVersion'));
 
-      var family = getManagedValue(data, 'Cellular.GSM');
+      var family = getManagedValue(data, 'Cellular.Family');
       detailsPage.gsm = family == 'GSM';
       if (detailsPage.gsm) {
         $('iccid').textContent = getManagedValue(data, 'Cellular.ICCID');
@@ -1328,33 +1365,37 @@ cr.define('options.internet', function() {
         var otherOption = apnSelector[0];
         data.selectedApn = -1;
         data.userApnIndex = -1;
-        var apnList = data.providerApnList.value;
+        var activeApn = getManagedValue(data, 'Cellular.APN');
+        var lastGoodApn = getManagedValue(data, 'Cellular.LastGoodAPN');
+        var apnList = getManagedValue(data, 'Cellular.APNList');
         for (var i = 0; i < apnList.length; i++) {
+          var apnDict = apnList[i];
           var option = document.createElement('option');
-          var localizedName = apnList[i].localizedName;
-          var name = localizedName ? localizedName : apnList[i].name;
-          var apn = apnList[i].apn;
+          var localizedName = apnDict['LocalizedName'];
+          var name = localizedName ? localizedName : apnDict['Name'];
+          var apn = apnDict['APN'];
           option.textContent = name ? (name + ' (' + apn + ')') : apn;
           option.value = i;
-          // data.apn and data.lastGoodApn will always be defined, however
-          // data.apn.apn and data.lastGoodApn.apn may not be. This is not a
-          // problem, as apnList[i].apn will always be defined and the
-          // comparisons below will work as expected.
-          if ((data.apn.apn == apn &&
-               data.apn.username == apnList[i].username &&
-               data.apn.password == apnList[i].password) ||
-              (!data.apn.apn &&
-               data.lastGoodApn.apn == apn &&
-               data.lastGoodApn.username == apnList[i].username &&
-               data.lastGoodApn.password == apnList[i].password)) {
+          // If this matches the active Apn, or LastGoodApn, set it as the
+          // selected Apn.
+          if ((activeApn != undefined &&
+               activeApn['APN'] == apn &&
+               activeApn['Username'] == apnDict['Username'] &&
+               activeApn['Password'] == apnDict['Password']) ||
+              ((activeApn == undefined || !activeApn['APN']) &&
+                lastGoodApn != undefined &&
+                lastGoodApn['APN'] == apn &&
+                lastGoodApn['Username'] == apnDict['Username'] &&
+                lastGoodApn['Password'] == apnDict['Password'])) {
             data.selectedApn = i;
           }
           // Insert new option before "other" option.
           apnSelector.add(option, otherOption);
         }
-        if (data.selectedApn == -1 && data.apn.apn) {
+        if (data.selectedApn == -1 &&
+            activeApn != undefined && activeApn['APN']) {
           var option = document.createElement('option');
-          option.textContent = data.apn.apn;
+          option.textContent = activeApn['APN'];
           option.value = -1;
           apnSelector.add(option, otherOption);
           data.selectedApn = apnSelector.length - 2;
@@ -1363,8 +1404,8 @@ cr.define('options.internet', function() {
         apnSelector.selectedIndex = data.selectedApn;
         updateHidden('.apn-list-view', false);
         updateHidden('.apn-details-view', true);
-        // TODO(stevenjb): Used managed properties for policy controlled value.
-        var lockEnabled = data.simCardLockEnabled.value;
+        var lockEnabled =
+            getManagedValue(data, 'Cellular.SIMLockStatus.LockEnabled');
         $('sim-card-lock-enabled').checked = lockEnabled;
         $('change-pin').hidden = !lockEnabled;
       }