Skip to content

Commit

Permalink
[ONC] Provide missing defaults for editable+policy.
Browse files Browse the repository at this point in the history
This change updates how the augmented ONC is generated to provide
default defaults for properties that are device editable and are
affected by device policy (e.g. recommended) but do not have a default
recommended value provided. This directly fixes an issue where the
device policy provides a recommended default value but the UI does not
show any indication that there is a recommended value.

Not using recommended: https://screenshot.googleplex.com/4cxzd6AX7EyPpbD.png
Using recommended:     https://screenshot.googleplex.com/9HhBcSfdpbJj96X.png

Bug: b/228334471
Test: Unit tests and manually on-device.
Change-Id: I0a27d507dfb0a8afd91e183b01a3be0feb4537d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3805464
Reviewed-by: Steven Bennetts <stevenjb@chromium.org>
Commit-Queue: Chad Duffin <chadduffin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1037809}
  • Loading branch information
Chad Duffin authored and Chromium LUCI CQ committed Aug 22, 2022
1 parent fb2fe19 commit 99f1ff2
Show file tree
Hide file tree
Showing 6 changed files with 76 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -632,32 +632,35 @@ var availableTests = [
assertEq({
ConnectionState: ConnectionStateType.NOT_CONNECTED,
GUID: 'stub_wifi2',
IPAddressConfigType: {
Active: 'DHCP',
Effective: 'UserPolicy'
},
Name: {
Active: 'wifi2_PSK',
Effective: 'UserPolicy',
UserPolicy: 'My WiFi Network'
},
NameServersConfigType: {
Active: 'DHCP',
Effective: 'UserPolicy'
},
ProxySettings: {
Type: {
Active: 'Direct',
Effective: 'UserPolicy',
UserPolicy: 'Direct'
}
},
IPAddressConfigType: {
Active: 'DHCP',
Effective: 'UserPolicy'
},
NameServersConfigType: {
Active: 'DHCP',
Effective: 'UserPolicy'
},
Source: 'UserPolicy',
Type: NetworkType.WI_FI,
WiFi: {
AutoConnect: {
UserEditable: true
UserEditable: true,
UserPolicy: false
},
Frequency: 5000,
FrequencyList: [2400, 5000],
HexSSID: {
Active: '77696669325F50534B', // 'wifi2_PSK'
Effective: 'UserPolicy',
Expand All @@ -668,8 +671,6 @@ var availableTests = [
Effective: 'UserPolicy',
UserPolicy: false,
},
Frequency: 5000,
FrequencyList: [2400, 5000],
Passphrase: {
Effective: 'UserSetting',
UserEditable: true,
Expand Down
33 changes: 33 additions & 0 deletions chromeos/ash/components/network/onc/onc_merger.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,15 @@
#include <utility>
#include <vector>

#include "base/check.h"
#include "base/logging.h"
#include "base/memory/ptr_util.h"
#include "base/notreached.h"
#include "base/values.h"
#include "chromeos/ash/components/network/policy_util.h"
#include "chromeos/components/onc/onc_signature.h"
#include "components/onc/onc_constants.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace ash::onc {
namespace {
Expand Down Expand Up @@ -57,6 +60,17 @@ void MarkRecommendedFieldnames(const base::Value& policy, base::Value* result) {
}
}

// Returns the default value for ONC field specified by |field|.
base::Value GetDefaultValue(const OncFieldSignature* field) {
if (field->default_value_setter)
return field->default_value_setter();

DCHECK(field->value_signature);

// Return the default base::Value for the field type.
return base::Value(field->value_signature->onc_type);
}

// Returns a dictionary which contains |true| at each path that is editable by
// the user. No other fields are set.
base::Value GetEditableFlags(const base::Value& policy) {
Expand Down Expand Up @@ -427,13 +441,32 @@ class MergeToAugmented : public MergeToEffective {
augmented_value.SetKey(::onc::kAugmentationSharedSetting,
values.shared_setting->Clone());
}

base::Value::Dict& augmented_value_dict = augmented_value.GetDict();

if (HasUserPolicy() && values.user_editable) {
augmented_value.SetKey(::onc::kAugmentationUserEditable,
base::Value(true));

// Ensure that a property that is editable and has a user policy (which
// indicates that the policy recommends a value) always has the
// appropriate default user policy value provided.
if (!augmented_value_dict.Find(::onc::kAugmentationUserPolicy)) {
augmented_value_dict.Set(::onc::kAugmentationUserPolicy,
GetDefaultValue(field));
}
}
if (HasDevicePolicy() && values.device_editable) {
augmented_value.SetKey(::onc::kAugmentationDeviceEditable,
base::Value(true));

// Ensure that a property that is editable and has a device policy (which
// indicates that the policy recommends a value) always has the
// appropriate default device policy value provided.
if (!augmented_value_dict.Find(::onc::kAugmentationDevicePolicy)) {
augmented_value_dict.Set(::onc::kAugmentationDevicePolicy,
GetDefaultValue(field));
}
}
if (!augmented_value.DictEmpty())
return augmented_value;
Expand Down
16 changes: 15 additions & 1 deletion chromeos/components/test/data/onc/augmented_merge.json
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,23 @@
"Effective": "UserPolicy",
"UserSetting": "Static"
},
"NameServersConfigType": {
"DeviceEditable": true,
"DevicePolicy": "DHCP",
"UserEditable": true,
"UserPolicy": "DHCP"
},
"StaticIPConfig": {
"IPAddress": {
"DevicePolicy": "127.0.0.1",
"Effective": "UserPolicy",
"UserPolicy": "127.0.0.1",
"UserSetting": "1.2.3.4"
},
"NameServers": {
"Effective": "UserPolicy",
"UserPolicy": [ ]
},
"RoutingPrefix": {
"Effective": "UserPolicy",
"UserPolicy": 32
Expand Down Expand Up @@ -84,7 +94,9 @@
"ClientCertPattern": {
"EnrollmentURI": {
"DeviceEditable": true,
"UserEditable": true
"DevicePolicy": [ ],
"UserEditable": true,
"UserPolicy": [ ]
},
"IssuerCARef": {
"DeviceEditable": true,
Expand All @@ -101,8 +113,10 @@
},
"Password": {
"DeviceEditable": true,
"DevicePolicy": "",
"Effective": "UserSetting",
"UserEditable": true,
"UserPolicy": "",
"UserSetting": "users password"
},
"Port": {
Expand Down
7 changes: 6 additions & 1 deletion chromeos/components/test/data/onc/device_policy.onc
Original file line number Diff line number Diff line change
Expand Up @@ -23,5 +23,10 @@
"IPsec": {
"PSK": "sharedkey"
}
}
},
// We explicitly do not provide a policy value for this property, but still
// consider it recommended, to verify that we still provide the correct
// default (which is different than the default value for a string) in the
// final augmented ONC.
"Recommended": [ "NameServersConfigType" ]
}
12 changes: 9 additions & 3 deletions chromeos/components/test/data/onc/managed_vpn.onc
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
"StaticIPConfig": {
"Type": "IPv4",
"IPAddress": "127.0.0.1",
"NameServers": [ ],
"RoutingPrefix": 32
},
"VPN": {
"Host": "policys host",
"Recommended": ["Host"],
"Recommended": [ "Host" ],
"Type": "OpenVPN",
"OpenVPN": {
"Port": 1194,
Expand All @@ -19,12 +20,17 @@
"IssuerCARef": [ "openvpn-test-ca" ],
"Recommended": [ "EnrollmentURI", "IssuerCARef" ]
},
"ServerCARefs": ["ref1", "ref2"]
"ServerCARefs": [ "ref1", "ref2" ]
},
"IPsec": {
"AuthenticationType": "PSK",
"PSK": "sharedkey",
"IKEVersion": 1
}
}
},
// We explicitly do not provide a policy value for this property, but still
// consider it recommended, to verify that we still provide the correct
// default (which is different than the default value for a string) in the
// final augmented ONC.
"Recommended": [ "NameServersConfigType" ]
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
"StaticIPConfig": {
"Type": "IPv4",
"IPAddress": "127.0.0.1",
"NameServers": [ ],
"RoutingPrefix": 32
},
"VPN": {
Expand Down

0 comments on commit 99f1ff2

Please sign in to comment.