New issue
Advanced search Search tips

Issue 834804 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Hook up schema validator for JSON policies

Project Member Reported by ljusten@chromium.org, Apr 19 2018

Issue description

Would be nice to actually see JSON parsing errors on chrome://policy for JSON policies. We have a SchemaValidatingPolicyHandler class. I haven't looked into it deeply, but maybe we can hook that up with JSON policies (maybe even do it automatically for user policy?).

 
Project Member

Comment 1 by bugdroid1@chromium.org, May 30 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9469f9fd5450bc5c767fe91abca335820dda7318

commit 9469f9fd5450bc5c767fe91abca335820dda7318
Author: A Olsen <olsen@chromium.org>
Date: Wed May 30 09:47:31 2018

Surface errors for invalid JSON string policies

Certain policies contain strings which are required to be valid
JSON which should conform to a certain schema. Without changing how
the policy is stored (which could have far-reaching changes), we can
still validate the policy while keeping it a string - by validating
that it is a string, that it is valid JSON, and that it conforms to
the schema. This change adds the code for validating either a
single JSON string or a list of JSON strings, but doesn't actually
enable JSON validation for any policy.

Bug: 834804
Change-Id: I2ca96a93bd771c6fb7276dcca67bf4a65da99bc8
Reviewed-on: https://chromium-review.googlesource.com/1070274
Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562778}
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy/core/browser/configuration_policy_handler.cc
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy/core/browser/configuration_policy_handler.h
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy/resources/policy_templates.json
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy/tools/generate_policy_source.py
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy/tools/syntax_check_policy_template_json.py
[modify] https://crrev.com/9469f9fd5450bc5c767fe91abca335820dda7318/components/policy_strings.grdp

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e

commit 7cc0b3761a8a90c578220a86dd9b34887ebf7c0e
Author: A Olsen <olsen@chromium.org>
Date: Fri Jun 01 09:50:34 2018

Revert "Surface errors for invalid JSON string policies"

This reverts commit 9469f9fd5450bc5c767fe91abca335820dda7318.

Reason for revert: 
Adding a validation_schema (and storing it where schema is normally stored in an attempt to avoid more complexity) had an unintended side effect - it caused some strings to turn into dicts on windows and so fail validation.

Original change's description:
> Surface errors for invalid JSON string policies
> 
> Certain policies contain strings which are required to be valid
> JSON which should conform to a certain schema. Without changing how
> the policy is stored (which could have far-reaching changes), we can
> still validate the policy while keeping it a string - by validating
> that it is a string, that it is valid JSON, and that it conforms to
> the schema. This change adds the code for validating either a
> single JSON string or a list of JSON strings, but doesn't actually
> enable JSON validation for any policy.
> 
> Bug: 834804
> Change-Id: I2ca96a93bd771c6fb7276dcca67bf4a65da99bc8
> Reviewed-on: https://chromium-review.googlesource.com/1070274
> Reviewed-by: Julian Pastarmov <pastarmovj@chromium.org>
> Reviewed-by: Lutz Justen <ljusten@chromium.org>
> Commit-Queue: A Olsen <olsen@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#562778}

TBR=pastarmovj@chromium.org,ljusten@chromium.org,olsen@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 834804
Change-Id: I6e248ec0c1314636e85f1da03d059aeb621e3f37
Reviewed-on: https://chromium-review.googlesource.com/1082191
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#563574}
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy/core/browser/configuration_policy_handler.cc
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy/core/browser/configuration_policy_handler.h
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy/resources/policy_templates.json
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy/tools/generate_policy_source.py
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy/tools/syntax_check_policy_template_json.py
[modify] https://crrev.com/7cc0b3761a8a90c578220a86dd9b34887ebf7c0e/components/policy_strings.grdp

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 5 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949

commit bc71bafcc84fe618a8ba0a08adbea0f77a5ab949
Author: A Olsen <olsen@chromium.org>
Date: Tue Jun 05 14:42:44 2018

RF:Surface errors for invalid JSON string policies

Rollforward explanation:
Rollforward of CL 1070274 - that CL made the mistake of overwriting
schema metadata with validation_schema - see rollback CL 1082191.
This CL has a modification in generate_policy_source.py - it now stores
both original schema metadata and the new validation_schema metadata
so as not to break anything relying on the existing metadata.

Original description:
Certain policies contain strings which are required to be valid
JSON which should conform to a certain schema. Without changing how
the policy is stored (which could have far-reaching changes), we can
still validate the policy while keeping it a string - by validating
that it is a string, that it is valid JSON, and that it conforms to
the schema. This change adds the code for validating either a
single JSON string or a list of JSON strings, but doesn't actually
enable JSON validation for any policy.


Bug: 834804
Change-Id: Ib189e1d52f622b678d640df27cb9f7f7afafb839
Reviewed-on: https://chromium-review.googlesource.com/1082291
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Commit-Queue: A Olsen <olsen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#564490}
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/core/browser/configuration_policy_handler.cc
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/core/browser/configuration_policy_handler.h
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/core/common/schema.cc
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/core/common/schema_internal.h
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/resources/policy_templates.json
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/tools/generate_policy_source.py
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy/tools/syntax_check_policy_template_json.py
[modify] https://crrev.com/bc71bafcc84fe618a8ba0a08adbea0f77a5ab949/components/policy_strings.grdp

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e92d1d030c45602e6366ed5201c8ad644b3aafc7

commit e92d1d030c45602e6366ed5201c8ad644b3aafc7
Author: A Olsen <olsen@chromium.org>
Date: Tue Jun 12 09:00:21 2018

Implement validators for JSON policies

Certain policies contain JSON - that is, for the admin to use them
properly, they should set them to a JSON string that conforms to a
particular schema. Until this change, that JSON was not being
validated, and errors were not getting reported to the user.
This change adds validation schemas for two JSON  policies -
DefaultPrinterSelection and AutoSelectCertificateForUrls -
and adds a histogram for tracking how often JSON policies fail
validation.

However, it doesn't actually enable validation of these schemas yet.

== More details:

SimpleJsonStringSchemaValidatingPolicyHandler changes:
Type mismatches are always rejected - that is, if the value is not a
string or a list of strings (whichever is appropriate) it is rejected.
SimpleJsonStringSchemaValidatingPolicyHandler is changed slightly in this
CL so that type errors are validated first, then JSON errors second,
with some leniency if the allow_embedded_json_errors_ flag is set.

Unit tests:
Added some unit tests for SimpleJsonSchemaValidatingPolicyHandler.
(I should have added these last CL).


Bug: 834804
Change-Id: I8e7fd2097e5aad37636862543958f4a803fa018d
Reviewed-on: https://chromium-review.googlesource.com/1078751
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Brian White <bcwhite@chromium.org>
Reviewed-by: Pavol Marko <pmarko@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#566360}
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/components/policy/core/browser/configuration_policy_handler.cc
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/components/policy/core/browser/configuration_policy_handler.h
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/components/policy/core/browser/configuration_policy_handler_unittest.cc
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/components/policy/core/common/schema.cc
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/components/policy/core/common/schema.h
[modify] https://crrev.com/e92d1d030c45602e6366ed5201c8ad644b3aafc7/tools/metrics/histograms/histograms.xml

Comment 5 by olsen@chromium.org, Jun 19 2018

Status: Started (was: Assigned)
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 27 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2b24d355f98eb7c3690a546033ced70f78244581

commit 2b24d355f98eb7c3690a546033ced70f78244581
Author: A Olsen <olsen@chromium.org>
Date: Wed Jun 27 14:16:07 2018

Enable two embedded-JSON policy validators

Certain policies contain JSON - that is, for the admin to use them
properly, they should set them to a JSON string that conforms to a
particular schema. Until this change, that JSON was not being
validated, and errors were not getting reported to the user.
This CL enables validation for two JSON  policies -
DefaultPrinterSelection and AutoSelectCertificateForUrls.
It uses a histogram to track how often these policies contain
any errors, and it reports any errors encountered to the user in
chrome://policy. However, it does not actually reject any policies
unless they have the wrong root type. This is by design since the
code that reads these policies is already robust.

Didn't work on Android at first since these policies don't exist
there - this is fixed by guarding these handlers with an #ifdef.

Bug: 834804
Change-Id: I64100ab3bcab07c6f4c2c53117cc4e9225a609e8
Reviewed-on: https://chromium-review.googlesource.com/1106158
Commit-Queue: A Olsen <olsen@chromium.org>
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Lutz Justen <ljusten@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570757}
[modify] https://crrev.com/2b24d355f98eb7c3690a546033ced70f78244581/chrome/browser/policy/configuration_policy_handler_list_factory.cc
[modify] https://crrev.com/2b24d355f98eb7c3690a546033ced70f78244581/components/policy/core/browser/configuration_policy_handler.cc
[modify] https://crrev.com/2b24d355f98eb7c3690a546033ced70f78244581/components/policy/core/browser/configuration_policy_handler.h
[modify] https://crrev.com/2b24d355f98eb7c3690a546033ced70f78244581/components/policy/core/browser/configuration_policy_handler_unittest.cc

Done for the following policies:

AutoSelectCertificateForUrls
DefaultPrinterSelection

Not done for the following policies, for various reasons:

Device policies:
DeviceOpenNetworkConfiguration (device policy)
DeviceLoginScreenAutoSelectCertificateForUrls (device policy)

Complex / hard to validate policies:
ArcPolicy
DeviceOpenNetworkConfiguration
DefaultPrinterSelection
OpenNetworkConfiguration
RemoteAccessHostDebugOverridePolicies

Unique policy that is set using a 'list' element but has a complex schema:
NativePrinters

Sign in to add a comment