ONC: add support for non-utf-8 SSIDs

ONC: add support for non-utf-8 SSIDs

ONC currently uses strings to represent SSIDs. Since JSON-Schema strings are restricted to Unicode and ONC is encoding these as UTF-8, this means that ONC cannot be used to represent SSIDs that include non-UTF-8 strings.

This change adds a separate HexSSID field to the WiFi section of the ONC that expects an SSID byte sequence encoded in hex.

At least one of the fields HexSSID or SSID must be present. If both HexSSID and SSID are set, the values must be consistent.

BUG=432546
Committed: https://crrev.com/decce902f12dde8b6abdbfe625905be8fcb27ab0
Cr-Commit-Position: refs/heads/master@{#306838}

[pneubeck (no reviews), 2014-11-26 10:15:32]

pneubeck@chromium.org changed reviewers:
+ pneubeck@chromium.org

[pneubeck (no reviews), 2014-11-26 10:15:34]

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_nor...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_nor...
chromeos/network/onc/onc_normalizer.cc:236: FillInHexSSIDField(wifi);
check whether this should also be called after every other ONCValidator usage
that is not followed by a normalizer call.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_sig...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_sig...
chromeos/network/onc/onc_signature.cc:206:
{::onc::wifi::kAllowGatewayARPPolling, &kBoolSignature},
nit: keep the formatting as in the rest of the file.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
chromeos/network/onc/onc_utils.cc:249: std::string hex_ssid_string;
nit: merge both lines and use c'tor arguments instead of assignment.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
chromeos/network/onc/onc_utils.h:86: // If the UTF-8 encoded SSID field is set,
but HexSSID is not, this function
The encoding _in_ the SSID field doesn't matter. The semantics of the field are
that we _use it with the UTF-8 encoding_.

I would change the comment to:

If the SSID field is set, but HexSSID is not, converts the contents of the SSID
field to UTF-8 encoding, creates the hex representation and assigns the result
to HexSSID.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:23: const int kMaximumSSIDLengthInBytes =
32;
nit: add a comment where this comes from (802.11 standard)

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:402: const std::string& ssid_field_name,
as you always pass the same strings for the field names, you could inline kSSID
and kHexSSID

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:404: // SSID validity
nit: use whole sentences (ending with fullstop).

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:408: if
(!value->GetAsString(&ssid_string)) {
type is verified already by the generic/default verification _before_ the custom
verification code runs.

you can change line 407 to 'GetStringWithoutPathExpansion'

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:425: if
(object.GetWithoutPathExpansion(hex_ssid_field_name, &value)) {
use GetStringWithoutPathExpansion

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:426: if
(!value->GetAsString(&hex_ssid_string)) {
again not required

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:449: // both set
nit: make the comment a bit more verbose:

"If both SSID and HexSSID are set, verify that they are consistent."

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:639: // validate SSID and HexSSID fields
nit: start with capital case and end with fullstop

append a ', if present.' or 'if existent'

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:644: if (!result->HasKey(kSSID))
prepend a comment that says that "One of {kSSID, kHexSSID} must be present".

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:158:
OncParams("toplevel_wifi_hexssid.onc",
I wouldn't test both managed=true and =false if we know already that the code
does not depend on it.

Instead ensure that the different valid cases are covered:
  SSID only
  HexSSID only
  both present and consistent

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:458:
std::make_pair(OncParams("network-wifi-invalid-hexssid",
nit: you could also test for invalid length

https://codereview.chromium.org/759663004/diff/1/chromeos/test/data/network/i...
File chromeos/test/data/network/invalid_settings_with_repairs.json (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:167: "HexSSID":
"notvalid",
nit: notvalid -> invalidhex

[cschuet (SLOW), 2014-11-27 11:03:58]

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_nor...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_nor...
chromeos/network/onc/onc_normalizer.cc:236: FillInHexSSIDField(wifi);
On 2014/11/26 10:15:33, pneubeck wrote:
> check whether this should also be called after every other ONCValidator usage
> that is not followed by a normalizer call.

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_sig...
File chromeos/network/onc/onc_signature.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_sig...
chromeos/network/onc/onc_signature.cc:206:
{::onc::wifi::kAllowGatewayARPPolling, &kBoolSignature},
On 2014/11/26 10:15:33, pneubeck wrote:
> nit: keep the formatting as in the rest of the file.

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
chromeos/network/onc/onc_utils.cc:249: std::string hex_ssid_string;
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: merge both lines and use c'tor arguments instead of assignment.

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
File chromeos/network/onc/onc_utils.h (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_uti...
chromeos/network/onc/onc_utils.h:86: // If the UTF-8 encoded SSID field is set,
but HexSSID is not, this function
On 2014/11/26 10:15:34, pneubeck wrote:
> The encoding _in_ the SSID field doesn't matter. The semantics of the field
are
> that we _use it with the UTF-8 encoding_.
> 
> I would change the comment to:
> 
> If the SSID field is set, but HexSSID is not, converts the contents of the
SSID
> field to UTF-8 encoding, creates the hex representation and assigns the result
> to HexSSID.

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:23: const int kMaximumSSIDLengthInBytes =
32;
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: add a comment where this comes from (802.11 standard)

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:402: const std::string& ssid_field_name,
On 2014/11/26 10:15:34, pneubeck wrote:
> as you always pass the same strings for the field names, you could inline
kSSID
> and kHexSSID

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:404: // SSID validity
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: use whole sentences (ending with fullstop).

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:408: if
(!value->GetAsString(&ssid_string)) {
On 2014/11/26 10:15:34, pneubeck wrote:
> type is verified already by the generic/default verification _before_ the
custom
> verification code runs.
> 
> you can change line 407 to 'GetStringWithoutPathExpansion'

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:425: if
(object.GetWithoutPathExpansion(hex_ssid_field_name, &value)) {
On 2014/11/26 10:15:34, pneubeck wrote:
> use GetStringWithoutPathExpansion

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:426: if
(!value->GetAsString(&hex_ssid_string)) {
On 2014/11/26 10:15:34, pneubeck wrote:
> again not required

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:449: // both set
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: make the comment a bit more verbose:
> 
> "If both SSID and HexSSID are set, verify that they are consistent."

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:639: // validate SSID and HexSSID fields
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: start with capital case and end with fullstop
> 
> append a ', if present.' or 'if existent'

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator.cc:644: if (!result->HasKey(kSSID))
On 2014/11/26 10:15:34, pneubeck wrote:
> prepend a comment that says that "One of {kSSID, kHexSSID} must be present".

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
File chromeos/network/onc/onc_validator_unittest.cc (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:158:
OncParams("toplevel_wifi_hexssid.onc",
On 2014/11/26 10:15:34, pneubeck wrote:
> I wouldn't test both managed=true and =false if we know already that the code
> does not depend on it.
> 
> Instead ensure that the different valid cases are covered:
>   SSID only
>   HexSSID only
>   both present and consistent

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/network/onc/onc_val...
chromeos/network/onc/onc_validator_unittest.cc:458:
std::make_pair(OncParams("network-wifi-invalid-hexssid",
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: you could also test for invalid length

Done.

https://codereview.chromium.org/759663004/diff/1/chromeos/test/data/network/i...
File chromeos/test/data/network/invalid_settings_with_repairs.json (right):

https://codereview.chromium.org/759663004/diff/1/chromeos/test/data/network/i...
chromeos/test/data/network/invalid_settings_with_repairs.json:167: "HexSSID":
"notvalid",
On 2014/11/26 10:15:34, pneubeck wrote:
> nit: notvalid -> invalidhex

Done.

[pneubeck (no reviews), 2014-11-27 14:49:03]

+Steven,Mukesh FYI

https://codereview.chromium.org/759663004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/759663004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:227: //
fill in HexSSID field from contents of SSID field if not set already
ah. In this case, you don't have to fix the HexSSID because the result
|root_dict| will not be used and thrown away.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:294: if
(user_settings_copy.get() != NULL)
nit: no .get()!=NULL and missing braces
 if (user_settings_copy) {
   ...
 }

nit: please check all comments to follow the scheme,
whole sentence starting with capital letter and ending with fullstop. :-)

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:296:
onc::kToplevelConfigurationSignature, user_settings_copy.get());
bug:
user_settings_copy -> validated_user_settings

validated_user_settings is an onc::kNetworkConfigurationSignature not a toplevel
ONC.

please check whether you can add a SetProperties test to
managed_network_configuration_unittest that covers this case, i.e. that it
checks for the HexSSID to be sent to Shill.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.cc:252: base::DictionaryValue* inner_object =
NULL;
nit: NULL -> nullptr (please check in the whole CL)

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:734: SSID of the network. If both
mention here already / highlight, that this field can only be used for unicode
strings and will be used in the UTF-8 encoding.
If non-UTF8 should be used, HexSSID must be used.

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:750: </section>
add a rule section (just search for 'rule') that states that either SSID or
HexSSID must be present.

[pneubeck (no reviews), 2014-11-27 14:56:46]

explain a little bit more what this CL is about in the commit message.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:333: }
when we adapt the policy matching, you will have to call 
onc::FillInHexSSIDFieldsInOncObject
here before we do the policy matching.
We'll have to ensure that this works in a unit test of the
managed_network_configuration_handler (MNCH).

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_normalizer.cc:236: FillInHexSSIDField(wifi);
also mention this bheavior in the class comment in the header.

[cschuet (SLOW), 2014-11-27 15:44:31]

https://codereview.chromium.org/759663004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc
(right):

https://codereview.chromium.org/759663004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/configuration_policy_handler_chromeos.cc:227: //
fill in HexSSID field from contents of SSID field if not set already
On 2014/11/27 14:49:02, pneubeck wrote:
> ah. In this case, you don't have to fix the HexSSID because the result
> |root_dict| will not be used and thrown away.

Done.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
File chromeos/network/managed_network_configuration_handler_impl.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:294: if
(user_settings_copy.get() != NULL)
On 2014/11/27 14:49:02, pneubeck wrote:
> nit: no .get()!=NULL and missing braces
>  if (user_settings_copy) {
>    ...
>  }
> 
> nit: please check all comments to follow the scheme,
> whole sentence starting with capital letter and ending with fullstop. :-)

Done.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:296:
onc::kToplevelConfigurationSignature, user_settings_copy.get());
On 2014/11/27 14:49:02, pneubeck wrote:
> bug:
> user_settings_copy -> validated_user_settings
> 
> validated_user_settings is an onc::kNetworkConfigurationSignature not a
toplevel
> ONC.
> 
> please check whether you can add a SetProperties test to
> managed_network_configuration_unittest that covers this case, i.e. that it
> checks for the HexSSID to be sent to Shill.

Acknowledged.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/managed...
chromeos/network/managed_network_configuration_handler_impl.cc:333: }
On 2014/11/27 14:56:45, pneubeck wrote:
> when we adapt the policy matching, you will have to call 
> onc::FillInHexSSIDFieldsInOncObject
> here before we do the policy matching.
> We'll have to ensure that this works in a unit test of the
> managed_network_configuration_handler (MNCH).

Acknowledged.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_normalizer.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_normalizer.cc:236: FillInHexSSIDField(wifi);
On 2014/11/27 14:56:46, pneubeck wrote:
> also mention this bheavior in the class comment in the header.

Done.

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
File chromeos/network/onc/onc_utils.cc (right):

https://codereview.chromium.org/759663004/diff/40001/chromeos/network/onc/onc...
chromeos/network/onc/onc_utils.cc:252: base::DictionaryValue* inner_object =
NULL;
On 2014/11/27 14:49:02, pneubeck wrote:
> nit: NULL -> nullptr (please check in the whole CL)

Done.

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
File components/onc/docs/onc_spec.html (right):

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:734: SSID of the network. If both
On 2014/11/27 14:49:02, pneubeck wrote:
> mention here already / highlight, that this field can only be used for unicode
> strings and will be used in the UTF-8 encoding.
> If non-UTF8 should be used, HexSSID must be used.

Done.

https://codereview.chromium.org/759663004/diff/40001/components/onc/docs/onc_...
components/onc/docs/onc_spec.html:750: </section>
On 2014/11/27 14:49:02, pneubeck wrote:
> add a rule section (just search for 'rule') that states that either SSID or
> HexSSID must be present.

Done.

[pneubeck (no reviews), 2014-11-29 12:17:53]

lgtm

[pneubeck (no reviews), 2014-11-29 12:18:19]

On 2014/11/29 12:17:53, pneubeck wrote:
> lgtm

please still update the CL's commit message to be a bit more verbose.

[cschuet (SLOW), 2014-12-02 12:38:26]

On 2014/11/29 12:18:19, pneubeck wrote:
> On 2014/11/29 12:17:53, pneubeck wrote:
> > lgtm
> 
> please still update the CL's commit message to be a bit more verbose.

Done.

[pneubeck (no reviews), 2014-12-02 13:14:24]

On 2014/12/02 12:38:26, cschuet wrote:
> On 2014/11/29 12:18:19, pneubeck wrote:
> > On 2014/11/29 12:17:53, pneubeck wrote:
> > > lgtm
> > 
> > please still update the CL's commit message to be a bit more verbose.
> 
> Done.

the subject line is usually copied to be the first line of the message as well.
I've fixed that for you.

[pneubeck (no reviews), 2014-12-04 16:15:31]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-12-04 16:16:11]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/759663004/60001

[commit-bot: I haz the power, 2014-12-04 16:58:31]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2014-12-04 16:59:34]

Patchset 4 (id:??) landed as
https://crrev.com/decce902f12dde8b6abdbfe625905be8fcb27ab0
Cr-Commit-Position: refs/heads/master@{#306838}