Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
820133002:
Reusing names of policy keys from policy_constants.h (Closed)
|
Created:
6 years ago by Łukasz Anforowicz Modified:
5 years, 11 months ago CC:
chromium-reviews, asvitkine+watch_chromium.org, chromoting-reviews_chromium.org, kelvinp, jar (doing other things), brettw Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionReusing names of policy keys from policy_constants.h
Before this change, Chromoting-specific code under
src/remoting/host/policy_hack was introducing its own constants for
names of Chromoting policies, rather than reusing the constants already
generated into policy_constants.h from
src/components/policy/resources/policy_templates.json. This change
fixes this (as asked for by crbug.com/368321).
Removing names of policy keys from policy_watcher.cc is good because:
- It enourages keeping policy_templates.json in-sync with remoting code
(i.e. RemoteAccessHostTokenUrl was introduced to policy_watcher.cc but
not to policy_templates.json; after this changes this should be less
likely).
- Code removal is good.
Updating and keeping in-sync policy_templates.json is good because:
- This is where rest of Chromium infrastructure looks for policy
descriptions, examples, supported-since, etc.
- Up-to-date policy_templates.json is a pre-requisite for removing
src/remoting/host/policy_hack and instead reusing PolicyService and/or
ConfigurationPolicyProvider(s) for Win/Linux/Mac.
While working on the change, I've noticed that 5 out of 15 Chromoting
policies are not covered in policy_templates.json (i.e.
src/components/policy and src/remoting/host/policy_hack got out of
sync). This change fixes this as well. Notes:
- Making src/components/policy and src/remoting/host/policy_hack in-sync
again is good for the short-term.
- In the long-term we might want to somehow separate Chromoting-specific
policies out of policy_templates.json (separate
chromoting_policy_templates.json? separate supported_on?). This
should be done in a separate changelist because
1) the separation concerns should not block current changelist and
more importantly crrev.com/830193002 which gets rid of most code
under policy_hack and
2) the separation is quite involved in itself (i.e. need to account
for it2me/chromeos differences and make sure that tools for
enterprise admins continue to support chromoting policies).
BUG=368321
R=asvitkine@chromium.org, bartfab@chromium.org, mnissler@chromium.org, sergeyu@chromium.org
Committed: https://chromium.googlesource.com/chromium/src/+/2b91a480c5ea6fc6e20703058b1d1748dd0de332
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebasing... #
Total comments: 8
Patch Set 3 : Removed all Chromoting policies from Chrome prefs. #
Total comments: 1
Patch Set 4 : Completed changes to policy_templates.json and related files. #
Total comments: 5
Patch Set 5 : Tweaked description of RemoteAccessHostMatchUsername policy. #
Total comments: 14
Patch Set 6 : Tweaks to policy_templates.json as suggested by Sergey and Bartosz. #
Total comments: 4
Patch Set 7 : Addressed code review feedback from Bartosz. #
Total comments: 10
Patch Set 8 : Addressed code review feedback from Bartosz. #Patch Set 9 : Rebasing... #Patch Set 10 : Marked the 5 "new" policies as chrome_os:42- #
Messages
Total messages: 45 (14 generated)
lukasza@chromium.org changed reviewers: + kelvinp@chromium.org
Hi Kelvin, Could you please review a *draft* of changes to reuse policy names from policy_constants.h and to remove the policy names from policy_watcher.h? I'll work on the remaining changes / TODOs after we can agree that the current changes are going in the right direction and are desirable. Also - for now I have not included all the necessary reviewers (i.e. for policy_templates.h). Thanks, Lukasz https://codereview.chromium.org/820133002/diff/1/chrome/browser/policy/config... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/820133002/diff/1/chrome/browser/policy/config... chrome/browser/policy/configuration_policy_handler_list_factory.cc:291: base::Value::TYPE_STRING }, All the other chromoting *policies* are also duplicated as *preferences*. I assume I want to duplicate the 5 new policies as well. https://codereview.chromium.org/820133002/diff/1/components/policy/resources/... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:767: This policy is ignored on Windows.''', policy_watcher.h used to say: // This policy is currently considered 'internal only' and so is not // documented in policy_templates.json. I don't understand what 'internal only' means in this context. The policy name is visible publicly in src/remoting/host/policy_hack/policy_watcher.h and elsewhere under src/remoting. I think that even if the policy is meant for limited consumption, then it might still make sense to include it in policy_templates.json: - This will help get rid of src/remoting/host/policy_hack and reuse PolicyService in the future, - This will unify how policy names are defined and exposed to src/remoting today. https://codereview.chromium.org/820133002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:774: 'supported_on DO NOT SUBMIT': 'rmsousa@chromium.org 2013-04-06 04:50:43 4386f0a', Can I please get some help or hints with mapping a git commit id into a chrome version? https://codereview.chromium.org/820133002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:781: 'caption': '''DO NOT SUBMIT''', For the 3 RemoteAccessHostTokenXxx policies, I will need Renato's help for filling out the caption, description and examples. Let's first review this draft and agree that inclusion of Chromoting policies in policy_templates.js is the right thing to do, and I can work with Renato on the captions/descriptions afterwards. https://codereview.chromium.org/820133002/diff/1/components/policy/resources/... components/policy/resources/policy_templates.json:836: }, I think it is ok to include RemoteAccessHostDebugOverridePolicies here. I think that even if the policy is meant for limited consumption, then it might still make sense to include it in policy_templates.json: - This will help get rid of src/remoting/host/policy_hack and reuse PolicyService in the future, - This will unify how policy names are defined and exposed to src/remoting today. https://codereview.chromium.org/820133002/diff/1/tools/metrics/histograms/his... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/1/tools/metrics/histograms/his... tools/metrics/histograms/histograms.xml:44201: + <int value="284" label="DO NOT SUBMIT - RemoteAccessHostMatchUsername"/> I am not sure what should be used as a value here. This doesn't seem to be something that is end-user visible, so I think I would like to just use the policy name here (i.e. "RemoteAccessHostMatchUsername").
I am not sure if I am the best person to review this CL, as I haven't changed any policy values before. Considering contacting someone from the policy_component instead?
lukasza@chromium.org changed reviewers: + dconnelly@chromium.org - kelvinp@chromium.org
Hi Daniel, Let me start by trying to give a little bit of context. I am working on getting rid of copy&pasted policy code from src/remoting/host/policy_hack. I wanted to start with just reusing policy names from policy_constants.h, since 1) this is what crbug.com/368321 is explicitly asking for and 2) I think covering all of Chromoting policies in policy_templates.json is a prerequisite for switching to a PolicyService later on (i.e. PolicyService won't be able to validate the schema of Chromoting policies without including them in policy_templates.json). Could you please review a *draft* of changes to reuse policy names from policy_constants.h and to remove the policy names from policy_watcher.h? I'll work on the remaining changes / TODOs (and I'll add other necessary reviewers) after we can agree that the current changes are going in the right direction and are desirable. Thanks, Lukasz
mnissler@chromium.org changed reviewers: + mnissler@chromium.org
https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/co... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/co... chrome/browser/policy/configuration_policy_handler_list_factory.cc:292: base::Value::TYPE_STRING }, If I'm not mistaken, you're not using Chrome's PrefService instances to read policy. If that is the case you don't need the changes here and you don't need to introduce pref names either. The same probably goes for the other remoting policy->pref mappings already present in this file? Maybe I'm missing something, you may want to try to remove all of them and see whether things still work or not ;)
sergeyu@chromium.org changed reviewers: + sergeyu@chromium.org
src/remoting looks good, but as Mattias I'm not sure why you need any changes in src/chrome https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:760: 'caption': '''Requires that username of the local user and the host owner match''', Caption should make it clear that this is a policy for the CRD host, and it doesn't. https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:774: 'supported_on DO NOT SUBMIT': 'rmsousa@chromium.org 2013-04-06 04:50:43 4386f0a', There are bunch of "DO NO SUBMIT" in this file. Were you planning to double check them? https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:830: 'example_value': '{ "RemoteAccessHostMatchUsername": true }', Please fix.
lukasza@chromium.org changed reviewers: - dconnelly@chromium.org
I've removed Chromoting-related preferences altogether. I'll start working on removing the remaining "DO NOT SUBMIT"-s. https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/co... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/820133002/diff/20001/chrome/browser/policy/co... chrome/browser/policy/configuration_policy_handler_list_factory.cc:292: base::Value::TYPE_STRING }, On 2015/01/05 09:45:33, Mattias Nissler wrote: > If I'm not mistaken, you're not using Chrome's PrefService instances to read > policy. If that is the case you don't need the changes here and you don't need > to introduce pref names either. The same probably goes for the other remoting > policy->pref mappings already present in this file? Maybe I'm missing something, > you may want to try to remove all of them and see whether things still work or > not ;) Correct - current code doesn't use Chrome's PrefService and instead directly reads content of json files from under /etc/opt/chrome/policies/managed/ (this will hopefully be replaced by using ConfigDirPolicyLoader in crrev.com/830193002). In PatchSet #3, I've removed the old Chromoting entries from current file and from pref_names.cc and pref_names.h. I also removed "pref_mappings" property from Chromoting policy entries in policy_test_cases.json. It seems that try bots are happy so far (the 3 failures seem unrelated to my changes). I hope that "git cl try" includes whatever testcases cover these 4 files. https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:760: 'caption': '''Requires that username of the local user and the host owner match''', On 2015/01/05 17:51:56, Sergey Ulanov wrote: > Caption should make it clear that this is a policy for the CRD host, and it > doesn't. Good point. I'll fix this in a later patchset (when fixing the other new entries in this file). https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:774: 'supported_on DO NOT SUBMIT': 'rmsousa@chromium.org 2013-04-06 04:50:43 4386f0a', On 2015/01/05 17:51:56, Sergey Ulanov wrote: > There are bunch of "DO NO SUBMIT" in this file. Were you planning to double > check them? I wanted to get an ACK on the general direction of the changes, before spending time on researching Chrome versions + looping in Renato for helping with captions and descriptions of the policies introduced by him. I guess I hear that I should fix the "DO NOT SUBMITS", but other than them things look more or less okay (both from Chromoting and from Policy perspective). https://codereview.chromium.org/820133002/diff/20001/components/policy/resour... components/policy/resources/policy_templates.json:830: 'example_value': '{ "RemoteAccessHostMatchUsername": true }', On 2015/01/05 17:51:56, Sergey Ulanov wrote: > Please fix. Why? What is wrong with this example? The example is a string that contains JSON representation of Chromoting policies. I picked RemoteAccessHostMatchUsername as an example. Should I include 2 policies in the JSON example to make it more obvious that this is not a copy&paste error / that I didn't use RemoteAccessHostMatchUsername example instead of RemoteAccessHostDebugOverridePolicies example?
policy and prefs bits LGTM, except for policy_templates.json (which you need to get separate OWNERS approval for anyways).
https://codereview.chromium.org/820133002/diff/40001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/40001/components/policy/resour... components/policy/resources/policy_templates.json:774: 'supported_on DO NOT SUBMIT': 'rmsousa@chromium.org 2013-04-06 04:50:43 4386f0a', RemoteAccessHostTokenUrl This feature is currently disabled server-side. If this policy is set, the host will require authenticating clients to obtain an authentication token from this URL in order to connect. Must be used in conjunction with RemoteAccessHostTokenValidationUrl. Example: https://example.com/issue RemoteAccessHostTokenValidationUrl This feature is currently disabled server-side. If this policy is set, the host will use this URL to validate authentication tokens from clients, in order to accept connections. Must be used in conjunction with RemoteAccessHostTokenUrl. Example: https://example.com/validate RemoteAccessHostTokenValidationCertificateIssuer This feature is currently disabled server-side. If this policy is set, the host will use a client certificate with the given issuer CN to authenticate to RemoteAccessHostTokenValidationUrl. Set it to "*" to use any available client certificate. Example: "Example Certificate Authority"
lukasza@chromium.org changed reviewers: + atwilson@chromium.org, jar@chromium.org
Thanks for the feedback Mattias. With Renato's help, I've completed the changes to policy_templates.json and related files (histograms.xml and policy_test_cases.json). Sergey - could you please take another look from src/remoting perspective? Andrew - could you please take a look at the policy_templates.json changes? Jim - could you please take a look at histograms.xml changes?
jar@chromium.org changed reviewers: + asvitkine@chromium.org - jar@chromium.org
+asvitkine for histograms.xml https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> Question (which I had trouble tracking down): What kind of histogram is recording this? Is this recorded in Chromium? If you are already using a SPARSE histogram... then great.... but if not... This enum value is getting large... so perhaps this is part of a UMA_HISTOGRAM_ENUMERATION.... and perhaps it is also sparse (for a single user?) and *might* benefit (data space usage) to use a sparse histogram. 289 buckets in a histogram is not "the end of the world," but it is getting larger than the common 50-100p
https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> On 2015/01/07 18:13:56, jar wrote: > Question (which I had trouble tracking down): What kind of histogram is > recording this? Is this recorded in Chromium? If you are already using a SPARSE > histogram... then great.... but if not... > > This enum value is getting large... so perhaps this is part of a > UMA_HISTOGRAM_ENUMERATION.... and perhaps it is also sparse (for a single user?) > and *might* benefit (data space usage) to use a sparse histogram. > > 289 buckets in a histogram is not "the end of the world," but it is getting > larger than the common 50-100p I am sorry, but I am not able to answer the questions above. The reason why I am modifying histograms.xml is that after changing policy_templates.json, I was getting errors from "git cl presubmit" that were asking me to add the new policy entries to histograms.xml as well: $ git cl presubmit ... Running presubmit commit checks ... ... ** Presubmit ERRORS ** ... Policy 'RemoteAccessHostMatchUsername' was added to policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. Please update both files. Policy 'RemoteAccessHostTokenUrl' was added to policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. Please update both files. Policy 'RemoteAccessHostTokenValidationUrl' was added to policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. Please update both files. Policy 'RemoteAccessHostTokenValidationCertificateIssuer' was added to policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. Please update both files. Policy 'RemoteAccessHostDebugOverridePolicies' was added to policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. Please update both files. Does that help?
histograms lgtm https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/60001/tools/metrics/histograms... tools/metrics/histograms/histograms.xml:45095: + <int value="289" label="RemoteAccessHostDebugOverridePolicies"/> On 2015/01/07 18:25:45, lukasza wrote: > On 2015/01/07 18:13:56, jar wrote: > > Question (which I had trouble tracking down): What kind of histogram is > > recording this? Is this recorded in Chromium? If you are already using a > SPARSE > > histogram... then great.... but if not... > > > > This enum value is getting large... so perhaps this is part of a > > UMA_HISTOGRAM_ENUMERATION.... and perhaps it is also sparse (for a single > user?) > > and *might* benefit (data space usage) to use a sparse histogram. > > > > 289 buckets in a histogram is not "the end of the world," but it is getting > > larger than the common 50-100p > > I am sorry, but I am not able to answer the questions above. The reason why I > am modifying histograms.xml is that after changing policy_templates.json, I was > getting errors from "git cl presubmit" that were asking me to add the new policy > entries to histograms.xml as well: > > $ git cl presubmit > ... > Running presubmit commit checks ... > ... > ** Presubmit ERRORS ** > ... > Policy 'RemoteAccessHostMatchUsername' was added to policy_templates.json but > not to src/tools/metrics/histograms/histograms.xml. Please update both files. > > Policy 'RemoteAccessHostTokenUrl' was added to policy_templates.json but not to > src/tools/metrics/histograms/histograms.xml. Please update both files. > > Policy 'RemoteAccessHostTokenValidationUrl' was added to policy_templates.json > but not to src/tools/metrics/histograms/histograms.xml. Please update both > files. > > Policy 'RemoteAccessHostTokenValidationCertificateIssuer' was added to > policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. > Please update both files. > > Policy 'RemoteAccessHostDebugOverridePolicies' was added to > policy_templates.json but not to src/tools/metrics/histograms/histograms.xml. > Please update both files. > > > Does that help? The two histograms that log this enum (Enterprise.Policies and EnterpriseCheck.InvalidPolicies) are already logged using the sparse macro, so it's already all good.
https://codereview.chromium.org/820133002/diff/60001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access host will not start if the name of the host owner is different from the name of the local user that the host is associated with. It's not clear from this description what "name of the host owner" means. It's the local part of the main user's e-mail address used to authenticate to the Google account when starting the host (e.g. sergeyu if the host is registered for sergeyu@chromium.org).
https://codereview.chromium.org/820133002/diff/60001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/60001/components/policy/resour... components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access host will not start if the name of the host owner is different from the name of the local user that the host is associated with. On 2015/01/07 20:41:14, Sergey Ulanov wrote: > It's not clear from this description what "name of the host owner" means. It's > the local part of the main user's e-mail address used to authenticate to the > Google account when starting the host (e.g. sergeyu if the host is registered > for mailto:sergeyu@chromium.org). I tried to improve the description in patchset #5.
lgtm https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access host compares the name of the local user (that the host is associated with) and the name of the Google account registered as the host owner (i.e. "lukasza" if the host is owned by "lukasza@chromium.org" Google account). The remote access host will not start if the name of the host owner is different from the name of the local user that the host is associated with. RemoteAccessHostMatchUsername policy should be used together with RemoteAccessHostDomain to also enforce that the Google account of the host owner is associated with a specific domain (i.e. "chromium.org"). nit: Maybe use something like johndoe@example.com instead of your account in the example
bartfab@chromium.org changed reviewers: + bartfab@chromium.org
https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], So these policies were actually working as of Chrome 25, Chrome 28, etc., but were not listed in policy_templates.json? If so, thanks for fixing that. All policies should be listed in this file. https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:766: This policy is ignored on Windows.''', Please set 'supported_on' to the actual supported platforms instead of documenting this here. https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', I presume the "policy" here has nothing to do with Chrome policy? Could you clarify this for posterity's sake?
I've addressed pending comment from Sergey and most of comments from Bartosz. Bartosz - could you please take another look (and possibly suggest what to do with the description of RemoteAccessHostDebugOverridePolicies). Thanks, Lukasz https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], On 2015/01/09 19:20:15, bartfab wrote: > So these policies were actually working as of Chrome 25, Chrome 28, etc., but > were not listed in policy_templates.json? If so, thanks for fixing that. All > policies should be listed in this file. Yes, but this assumes that when you say "Chrome 25" then it also covers Chromoting host (separate [daemon/service] process, same project). I've been discussing with mnissler@ if we should consider moving Chromoting-specific policies to a separate policy_templates.json (generating separate, Chromoting-specific chromoting_policy_constants.h/.cc etc.). It's not yet clear what more has to be involved in such move (i.e. to make sure that Chrome tools for enterprise admins continue to cover Chromoting policies). But, given that all the other Chromoting policies are listed here, I think listing the 5 missing ones is the right thing to do (at the very least this enables getting rid of code under src/remoting/host/policy_hack and reusing src/components/policy - this is under review at crrev.com/830193002 but depends on the current changelist being checked-in first). https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:762: If this setting is enabled, then the remote access host compares the name of the local user (that the host is associated with) and the name of the Google account registered as the host owner (i.e. "lukasza" if the host is owned by "lukasza@chromium.org" Google account). The remote access host will not start if the name of the host owner is different from the name of the local user that the host is associated with. RemoteAccessHostMatchUsername policy should be used together with RemoteAccessHostDomain to also enforce that the Google account of the host owner is associated with a specific domain (i.e. "chromium.org"). On 2015/01/09 00:46:56, Sergey Ulanov wrote: > nit: Maybe use something like mailto:johndoe@example.com instead of your account in the > example Done. https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:766: This policy is ignored on Windows.''', On 2015/01/09 19:20:15, bartfab wrote: > Please set 'supported_on' to the actual supported platforms instead of > documenting this here. Thanks for pointing this out. Done. https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', On 2015/01/09 19:20:15, bartfab wrote: > I presume the "policy" here has nothing to do with Chrome policy? Could you > clarify this for posterity's sake? I tried to point this out by mentioning "remote access host" above (this is what it's been called in all the other chromoting-related elements in this file). Could you please suggest a wording that you think would work better? BTW: let me bring your attention to a code review comment here, where we are discussing desire to potentially yank out this policy in the future: https://codereview.chromium.org/830193002/diff/20001/remoting/host/policy_hac... (next to: FILE_PATH_LITERAL("/etc/opt/chrome/policies"))
lukasza@chromium.org changed reviewers: - atwilson@chromium.org
https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], On 2015/01/09 19:47:27, lukasza wrote: > On 2015/01/09 19:20:15, bartfab wrote: > > So these policies were actually working as of Chrome 25, Chrome 28, etc., but > > were not listed in policy_templates.json? If so, thanks for fixing that. All > > policies should be listed in this file. > > Yes, but this assumes that when you say "Chrome 25" then it also covers > Chromoting host (separate [daemon/service] process, same project). I've been > discussing with mnissler@ if we should consider moving Chromoting-specific > policies to a separate policy_templates.json (generating separate, > Chromoting-specific chromoting_policy_constants.h/.cc etc.). It's not yet clear > what more has to be involved in such move (i.e. to make sure that Chrome tools > for enterprise admins continue to cover Chromoting policies). But, given that > all the other Chromoting policies are listed here, I think listing the 5 missing > ones is the right thing to do (at the very least this enables getting rid of > code under src/remoting/host/policy_hack and reusing src/components/policy - > this is under review at crrev.com/830193002 but depends on the current > changelist being checked-in first). The CL description provides no context, so I had no idea where these policies came from and what they were for. With your additional explanations, I understand what they are for now. I agree that using "chrome" when you mean "chromoting host" is strange. How about this middle ground: Do not introduce separate json files and processing mechanisms but add new supported_on constants like "chromoting.linux," "chromoting.win," etc. You could then tweak grit to output constants for these policies (making it easy to use them in code) but leave them out from e.g. the ADM templates we generate. https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', On 2015/01/09 19:47:27, lukasza wrote: > On 2015/01/09 19:20:15, bartfab wrote: > > I presume the "policy" here has nothing to do with Chrome policy? Could you > > clarify this for posterity's sake? > > I tried to point this out by mentioning "remote access host" above (this is what > it's been called in all the other chromoting-related elements in this file). > > Could you please suggest a wording that you think would work better? > > BTW: let me bring your attention to a code review comment here, where we are > discussing desire to potentially yank out this policy in the future: > https://codereview.chromium.org/830193002/diff/20001/remoting/host/policy_hac... > (next to: FILE_PATH_LITERAL("/etc/opt/chrome/policies")) I think the key problem is that these policies are intermixed with the 250+ Chrome policies. It is totally unexpected for the reader that four out of 250 policies apply to the chromoting host, not Chrome itself. It is OK (and arguably good) to put them in this file. But it should be really clear that these policies are different. They should be differentiated by supported_on constants, probably even clearer wording, possibly an additional flag like 'product': 'chromoting_host' and maybe by moving them to a dedicated section of the file, e.g. the very end, after Chrome OS device policies.
Bartosz - I've tweaked the CL description + tried to address your concerns in the comments. Can you please take another look? Thanks, Lukasz https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome.*:25-'], On 2015/01/12 13:16:33, bartfab wrote: > On 2015/01/09 19:47:27, lukasza wrote: > > On 2015/01/09 19:20:15, bartfab wrote: > > > So these policies were actually working as of Chrome 25, Chrome 28, etc., > but > > > were not listed in policy_templates.json? If so, thanks for fixing that. All > > > policies should be listed in this file. > > > > Yes, but this assumes that when you say "Chrome 25" then it also covers > > Chromoting host (separate [daemon/service] process, same project). I've been > > discussing with mnissler@ if we should consider moving Chromoting-specific > > policies to a separate policy_templates.json (generating separate, > > Chromoting-specific chromoting_policy_constants.h/.cc etc.). It's not yet > clear > > what more has to be involved in such move (i.e. to make sure that Chrome tools > > for enterprise admins continue to cover Chromoting policies). But, given that > > all the other Chromoting policies are listed here, I think listing the 5 > missing > > ones is the right thing to do (at the very least this enables getting rid of > > code under src/remoting/host/policy_hack and reusing src/components/policy - > > this is under review at crrev.com/830193002 but depends on the current > > changelist being checked-in first). > > The CL description provides no context, so I had no idea where these policies > came from and what they were for. Good point. I've added this to the CL description. > With your additional explanations, I > understand what they are for now. I agree that using "chrome" when you mean > "chromoting host" is strange. How about this middle ground: Do not introduce > separate json files and processing mechanisms but add new supported_on constants > like "chromoting.linux," "chromoting.win," etc. You could then tweak grit to > output constants for these policies (making it easy to use them in code) but > leave them out from e.g. the ADM templates we generate. > I am not sure what you mean by "ADM templates", but continuing to support Chromoting policies by tools for enterprise admins is an explicit requirement here (and one of the reasons to keep using the policy_templates.json from under components/policy). At any rate, I agree with the desire to move Chromoting policies to a separate location (either a separate chromoting_policy_templates.json or a separate supported_on flags), but I think this should be done in a separate changelist: - The current changelist just preserves the status quo (i.e. existance of 10 chromoting-specific policies under policy_templates.json). - The current changelist is needed to remove platform-specific code under src/remoting/host/policy_hack that duplicates the functionality of src/components/policy (in fact it got copy&pasted from there long time ago before this was a sharable component). - Separationg Chromoting policies is quite involved in itself and therefore belongs in a separate changelist: need to account for it2me/chromeos differences, need to make sure that tools for enterprise admins continue to support chromoting policies, need to discuss and identify mechanics of the move (i.e supported_on VS separate chromoting_policy_templates.json VS just filter the Schema in code). https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', On 2015/01/12 13:16:33, bartfab wrote: > On 2015/01/09 19:47:27, lukasza wrote: > > On 2015/01/09 19:20:15, bartfab wrote: > > > I presume the "policy" here has nothing to do with Chrome policy? Could you > > > clarify this for posterity's sake? > > > > I tried to point this out by mentioning "remote access host" above (this is > what > > it's been called in all the other chromoting-related elements in this file). > > > > Could you please suggest a wording that you think would work better? > > > > BTW: let me bring your attention to a code review comment here, where we are > > discussing desire to potentially yank out this policy in the future: > > > https://codereview.chromium.org/830193002/diff/20001/remoting/host/policy_hac... > > (next to: FILE_PATH_LITERAL("/etc/opt/chrome/policies")) > > I think the key problem is that these policies are intermixed with the 250+ > Chrome policies. It is totally unexpected for the reader that four out of 250 > policies apply to the chromoting host, not Chrome itself. It is OK (and arguably > good) to put them in this file. But it should be really clear that these > policies are different. They should be differentiated by supported_on constants, > probably even clearer wording, possibly an additional flag like 'product': > 'chromoting_host' and maybe by moving them to a dedicated section of the file, > e.g. the very end, after Chrome OS device policies. 10 Chromoting policies have been present in policy_templates.json forever (well, since 2011 :-). I am just adding 5 policies that are missing (and that prevent removal of the platform-specific code under src/remoting/host/policy_hack that got copy&pasted from src/components/policy before it became a shareable component). $ grep name.*:.*RemoteAccess components/policy/resources/policy_templates.json 'name': 'RemoteAccess', 'name': 'RemoteAccessClientFirewallTraversal', 'name': 'RemoteAccessHostFirewallTraversal', 'name': 'RemoteAccessHostDomain', 'name': 'RemoteAccessHostRequireTwoFactor', 'name': 'RemoteAccessHostTalkGadgetPrefix', 'name': 'RemoteAccessHostRequireCurtain', 'name': 'RemoteAccessHostAllowClientPairing', 'name': 'RemoteAccessHostAllowGnubbyAuth', 'name': 'RemoteAccessHostAllowRelayedConnection', 'name': 'RemoteAccessHostUdpPortRange', 'name': 'RemoteAccessHostMatchUsername', 'name': 'RemoteAccessHostTokenUrl', 'name': 'RemoteAccessHostTokenValidationUrl', 'name': 'RemoteAccessHostTokenValidationCertificateIssuer', 'name': 'RemoteAccessHostDebugOverridePolicies', $ git blame components/policy/resources/policy_templates.json | grep name.*:.*RemoteAccess lists the following (really old) commits: 268954f65c (garykac@chromium.org 2011-07-26 18:56:10 +0000 585) 48428b844d (garykac@chromium.org 2012-08-06 21:49:42 +0000 605) 60e2ac0880 (jamiewalch@chromium.org 2012-08-13 21:17:54 +0000 663) 562378fd4f (jamiewalch@chromium.org 2013-08-02 20:13:55 +0000 681) 3a5b82c105 (psj@chromium.org 2014-02-20 13:28:14 +0000 697) 11f9efbe87 (noamsml@google.com 2014-04-30 17:04:23 +0000 713)
components/policy/resources/policy_templates.json lgtm https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', On 2015/01/12 18:32:59, lukasza wrote: > On 2015/01/12 13:16:33, bartfab wrote: > > On 2015/01/09 19:47:27, lukasza wrote: > > > On 2015/01/09 19:20:15, bartfab wrote: > > > > I presume the "policy" here has nothing to do with Chrome policy? Could > you > > > > clarify this for posterity's sake? > > > > > > I tried to point this out by mentioning "remote access host" above (this is > > what > > > it's been called in all the other chromoting-related elements in this file). > > > > > > Could you please suggest a wording that you think would work better? > > > > > > BTW: let me bring your attention to a code review comment here, where we are > > > discussing desire to potentially yank out this policy in the future: > > > > > > https://codereview.chromium.org/830193002/diff/20001/remoting/host/policy_hac... > > > (next to: FILE_PATH_LITERAL("/etc/opt/chrome/policies")) > > > > I think the key problem is that these policies are intermixed with the 250+ > > Chrome policies. It is totally unexpected for the reader that four out of 250 > > policies apply to the chromoting host, not Chrome itself. It is OK (and > arguably > > good) to put them in this file. But it should be really clear that these > > policies are different. They should be differentiated by supported_on > constants, > > probably even clearer wording, possibly an additional flag like 'product': > > 'chromoting_host' and maybe by moving them to a dedicated section of the file, > > e.g. the very end, after Chrome OS device policies. > > 10 Chromoting policies have been present in policy_templates.json forever (well, > since 2011 :-). I am just adding 5 policies that are missing (and that prevent > removal of the platform-specific code under src/remoting/host/policy_hack that > got copy&pasted from src/components/policy before it became a shareable > component). > > $ grep name.*:.*RemoteAccess components/policy/resources/policy_templates.json > 'name': 'RemoteAccess', > 'name': 'RemoteAccessClientFirewallTraversal', > 'name': 'RemoteAccessHostFirewallTraversal', > 'name': 'RemoteAccessHostDomain', > 'name': 'RemoteAccessHostRequireTwoFactor', > 'name': 'RemoteAccessHostTalkGadgetPrefix', > 'name': 'RemoteAccessHostRequireCurtain', > 'name': 'RemoteAccessHostAllowClientPairing', > 'name': 'RemoteAccessHostAllowGnubbyAuth', > 'name': 'RemoteAccessHostAllowRelayedConnection', > 'name': 'RemoteAccessHostUdpPortRange', > 'name': 'RemoteAccessHostMatchUsername', > 'name': 'RemoteAccessHostTokenUrl', > 'name': 'RemoteAccessHostTokenValidationUrl', > 'name': 'RemoteAccessHostTokenValidationCertificateIssuer', > 'name': 'RemoteAccessHostDebugOverridePolicies', > > $ git blame components/policy/resources/policy_templates.json | grep > name.*:.*RemoteAccess > lists the following (really old) commits: > > 268954f65c (mailto:garykac@chromium.org 2011-07-26 18:56:10 +0000 585) > 48428b844d (mailto:garykac@chromium.org 2012-08-06 21:49:42 +0000 605) > 60e2ac0880 (mailto:jamiewalch@chromium.org 2012-08-13 21:17:54 +0000 663) > 562378fd4f (mailto:jamiewalch@chromium.org 2013-08-02 20:13:55 +0000 681) > 3a5b82c105 (mailto:psj@chromium.org 2014-02-20 13:28:14 +0000 697) > 11f9efbe87 (mailto:noamsml@google.com 2014-04-30 17:04:23 +0000 713) An interesting thing you said above is that you are just preserving the status quo. But as long as these five policies were not listed in policy_templates.json, they did not show up in any of our admin templates. So by adding them here, you are changing the status quo. After your CL lands, the admin templates used to manage Chrome will show five new policies, none of which actually affect Chrome. You pointed out that there are 10 Chromoting host policies in policy_templates.json already. Given that precedent, I think it is OK to move the remaining 5 policies in here too for consistency. It would be great to figure out where these policies should live in the longer term and move them there eventually (in a follow-up CL) though. https://codereview.chromium.org/820133002/diff/100001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/100001/components/policy/resou... components/policy/resources/policy_templates.json:770: 'supported_on': ['chrome.*:28-'], Note that |chrome.*| does not include Chrome OS. If the policy is supported on Chrome OS as well, you need to also set |chrome_os:28-|. https://codereview.chromium.org/820133002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45091: + <int value="285" label="RemoteAccessHostMatchUsername"/> You should copy the policy |caption|s, not the |name|s, to this file.
Bartosz - I tried addressing your code review feedback in patchset #7. Could you please take another look? Thanks, Lukasz https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/80001/components/policy/resour... components/policy/resources/policy_templates.json:836: The value is parsed as a JSON dictionary of policy name to policy value mappings.''', On 2015/01/13 18:49:45, bartfab wrote: > On 2015/01/12 18:32:59, lukasza wrote: > > On 2015/01/12 13:16:33, bartfab wrote: > > > On 2015/01/09 19:47:27, lukasza wrote: > > > > On 2015/01/09 19:20:15, bartfab wrote: > > > > > I presume the "policy" here has nothing to do with Chrome policy? Could > > you > > > > > clarify this for posterity's sake? > > > > > > > > I tried to point this out by mentioning "remote access host" above (this > is > > > what > > > > it's been called in all the other chromoting-related elements in this > file). > > > > > > > > Could you please suggest a wording that you think would work better? > > > > > > > > BTW: let me bring your attention to a code review comment here, where we > are > > > > discussing desire to potentially yank out this policy in the future: > > > > > > > > > > https://codereview.chromium.org/830193002/diff/20001/remoting/host/policy_hac... > > > > (next to: FILE_PATH_LITERAL("/etc/opt/chrome/policies")) > > > > > > I think the key problem is that these policies are intermixed with the 250+ > > > Chrome policies. It is totally unexpected for the reader that four out of > 250 > > > policies apply to the chromoting host, not Chrome itself. It is OK (and > > arguably > > > good) to put them in this file. But it should be really clear that these > > > policies are different. They should be differentiated by supported_on > > constants, > > > probably even clearer wording, possibly an additional flag like 'product': > > > 'chromoting_host' and maybe by moving them to a dedicated section of the > file, > > > e.g. the very end, after Chrome OS device policies. > > > > 10 Chromoting policies have been present in policy_templates.json forever > (well, > > since 2011 :-). I am just adding 5 policies that are missing (and that > prevent > > removal of the platform-specific code under src/remoting/host/policy_hack that > > got copy&pasted from src/components/policy before it became a shareable > > component). > > > > $ grep name.*:.*RemoteAccess components/policy/resources/policy_templates.json > > > 'name': 'RemoteAccess', > > 'name': 'RemoteAccessClientFirewallTraversal', > > 'name': 'RemoteAccessHostFirewallTraversal', > > 'name': 'RemoteAccessHostDomain', > > 'name': 'RemoteAccessHostRequireTwoFactor', > > 'name': 'RemoteAccessHostTalkGadgetPrefix', > > 'name': 'RemoteAccessHostRequireCurtain', > > 'name': 'RemoteAccessHostAllowClientPairing', > > 'name': 'RemoteAccessHostAllowGnubbyAuth', > > 'name': 'RemoteAccessHostAllowRelayedConnection', > > 'name': 'RemoteAccessHostUdpPortRange', > > 'name': 'RemoteAccessHostMatchUsername', > > 'name': 'RemoteAccessHostTokenUrl', > > 'name': 'RemoteAccessHostTokenValidationUrl', > > 'name': 'RemoteAccessHostTokenValidationCertificateIssuer', > > 'name': 'RemoteAccessHostDebugOverridePolicies', > > > > $ git blame components/policy/resources/policy_templates.json | grep > > name.*:.*RemoteAccess > > lists the following (really old) commits: > > > > 268954f65c (mailto:garykac@chromium.org 2011-07-26 18:56:10 +0000 585) > > 48428b844d (mailto:garykac@chromium.org 2012-08-06 21:49:42 +0000 605) > > 60e2ac0880 (mailto:jamiewalch@chromium.org 2012-08-13 21:17:54 +0000 663) > > 562378fd4f (mailto:jamiewalch@chromium.org 2013-08-02 20:13:55 +0000 681) > > 3a5b82c105 (mailto:psj@chromium.org 2014-02-20 13:28:14 +0000 697) > > 11f9efbe87 (mailto:noamsml@google.com 2014-04-30 17:04:23 +0000 713) > > An interesting thing you said above is that you are just preserving the status > quo. But as long as these five policies were not listed in > policy_templates.json, they did not show up in any of our admin templates. So by > adding them here, you are changing the status quo. After your CL lands, the > admin templates used to manage Chrome will show five new policies, none of which > actually affect Chrome. > > You pointed out that there are 10 Chromoting host policies in > policy_templates.json already. Given that precedent, I think it is OK to move > the remaining 5 policies in here too for consistency. It would be great to > figure out where these policies should live in the longer term and move them > there eventually (in a follow-up CL) though. Yes. This is a bit painful, but probably the right thing to do. I've added you to an email conversation where I am trying to figure out the pieces needed to make this happen. https://codereview.chromium.org/820133002/diff/100001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/100001/components/policy/resou... components/policy/resources/policy_templates.json:770: 'supported_on': ['chrome.*:28-'], On 2015/01/13 18:49:45, bartfab wrote: > Note that |chrome.*| does not include Chrome OS. If the policy is supported on > Chrome OS as well, you need to also set |chrome_os:28-|. Thanks for pointing this out. Fixed/done. I noticed that this was also missing for the old policies, because Chromoting *into* Chrome OS was not supported until 41 (thanks to Kelvin for helping me understand the version/support here). I've fixed this for the old policies as well (and fixed version number in one of the new policies). Full disclosure: Chromoting *into* Chrome OS is only supported in "it2me" / "Remote Assistance" mode. We still do not support "me2me" / "My Computers" mode for Chromoting into Chrome OS. https://codereview.chromium.org/820133002/diff/100001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/820133002/diff/100001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:45091: + <int value="285" label="RemoteAccessHostMatchUsername"/> On 2015/01/13 18:49:45, bartfab wrote: > You should copy the policy |caption|s, not the |name|s, to this file. Errr... ok. The |name|s looked cleaner and more descriptive without extra context... I guess I should have tackled this by fixing captions, not by using names. Fixed.
lukasza@chromium.org changed reviewers: + brettw@chromium.org
Brett, Could you please review the DEPS changes here? The changes have been already LGTM-ed by the owner of src/remoting (sergeyu@) and the owner of src/components/policy (mnissler@), but need a sign-off from somebody from top-level src/OWNERS, because of a quirk in PRESUBMIT.py. PRESUBMIT.py is confused and cannot tell who is the owner of the generated policy/policy_constants.h. The confusion is caused by the fact that policy/policy_constants.h does not exist as a path rooted in chromium/src (only as out/Debug/gen/policy/policy/policy_constants.h). I've forwarded to you the email with more detailed discussion of this issue. Thanks, Lukasz
Still LGTM, with a few more nits. https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (left): https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:204: "RemoteAccessHostFirewallTraversal": { Since none of these policies are actually processed by Chrome, there is no need to specify |os| or |test_policy|. Please just leave all the dictionaries empty, e.g.: "RemoteAccessHostFirewallTraversal": { }, https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], Here and below: 41 just branched. This should be 42- now. https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome_os:41-', 'chrome.linux:25-', 'chrome.mac:25-'], Nit: It is customary to list |chrome| before |chrome_os| in this file.
lukasza@chromium.org changed reviewers: - brettw@chromium.org
I've addressed the nits pointed out by Bartosz. I talked with Sergey and he has kindly offered to help with manually landing the changes (I don't have chromium commiter rights yet). Because of that I've removed Brett from "reviewers" list and moved him to CC. https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (left): https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:204: "RemoteAccessHostFirewallTraversal": { On 2015/01/14 11:09:02, bartfab wrote: > Since none of these policies are actually processed by Chrome, there is no need > to specify |os| or |test_policy|. Please just leave all the dictionaries empty, > e.g.: > > "RemoteAccessHostFirewallTraversal": { > }, Done. https://codereview.chromium.org/820133002/diff/120001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:211: "note": "TODO(frankf): Enable on all OS after crbug.com/121066 is fixed." I will remove this comment as well - crbug.com/121066 complains about a test failing (and being disabled) because of no mapping to preferences, but this is no longer applicable, because the current changelist is removing all Chromoting-related preferences from Chrome. https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/14 11:09:02, bartfab wrote: > Here and below: 41 just branched. This should be 42- now. I think this is okay - support for Chromoting into Chrome OS (in it2me / remote support mode) is enabled in 41 (this includes Chromoting policies). https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:752: 'supported_on': ['chrome_os:41-', 'chrome.linux:25-', 'chrome.mac:25-'], On 2015/01/14 11:09:02, bartfab wrote: > Nit: It is customary to list |chrome| before |chrome_os| in this file. Done.
https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/14 19:35:12, lukasza wrote: > On 2015/01/14 11:09:02, bartfab wrote: > > Here and below: 41 just branched. This should be 42- now. > > I think this is okay - support for Chromoting into Chrome OS (in it2me / remote > support mode) is enabled in 41 (this includes Chromoting policies). Chrome OS supports cloud policy only. The cloud policy protobuf is generated from policy_templates.json. This means that only policies listed in this file will be part of the protobuf. The existing chromoting policies did make it into the protobuf, even if they were not marked as supported on Chrome OS 41. So it is OK to retroactively mark them as supported. But the new policies you are adding are not part of the protobuf that gets compiled into Chrome OS 41. How does your code access e.g. the RemoteAccessHostTokenUrl policy on M41? You could use raw protobuf to but I doubt that this how your code works. I would rather expect that the five new policies are simply unsupported on Chrome OS until this CL lands and thus, should be marked as 42-.
Patchset #9 (id:160001) has been deleted
Thanks for the feedback Bartosz! I've applied the suggested fix to patchset #10. https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/15 09:41:44, bartfab wrote: > On 2015/01/14 19:35:12, lukasza wrote: > > On 2015/01/14 11:09:02, bartfab wrote: > > > Here and below: 41 just branched. This should be 42- now. > > > > I think this is okay - support for Chromoting into Chrome OS (in it2me / > remote > > support mode) is enabled in 41 (this includes Chromoting policies). > > Chrome OS supports cloud policy only. The cloud policy protobuf is generated > from policy_templates.json. This means that only policies listed in this file > will be part of the protobuf. > > The existing chromoting policies did make it into the protobuf, even if they > were not marked as supported on Chrome OS 41. So it is OK to retroactively mark > them as supported. > > But the new policies you are adding are not part of the protobuf that gets > compiled into Chrome OS 41. How does your code access e.g. the > RemoteAccessHostTokenUrl policy on M41? You could use raw protobuf to but I > doubt that this how your code works. I would rather expect that the five new > policies are simply unsupported on Chrome OS until this CL lands and thus, > should be marked as 42-. You're right. On non-Chrome-OS Chromoting didn't depend on policy_templates.json at all, but on Chrome-OS it does use the PolicyService from g_browser_process (which, as I infer from your explanation above, means that it depends on policy_templates.json generating the right protobufs). I changed 41 to 42 in the 5 new policies.
Still LGTM. https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/820133002/diff/120001/components/policy/resou... components/policy/resources/policy_templates.json:588: 'supported_on': ['chrome.*:14-', 'chrome_os:41-'], On 2015/01/15 17:35:22, lukasza wrote: > On 2015/01/15 09:41:44, bartfab wrote: > > On 2015/01/14 19:35:12, lukasza wrote: > > > On 2015/01/14 11:09:02, bartfab wrote: > > > > Here and below: 41 just branched. This should be 42- now. > > > > > > I think this is okay - support for Chromoting into Chrome OS (in it2me / > > remote > > > support mode) is enabled in 41 (this includes Chromoting policies). > > > > Chrome OS supports cloud policy only. The cloud policy protobuf is generated > > from policy_templates.json. This means that only policies listed in this file > > will be part of the protobuf. > > > > The existing chromoting policies did make it into the protobuf, even if they > > were not marked as supported on Chrome OS 41. So it is OK to retroactively > mark > > them as supported. > > > > But the new policies you are adding are not part of the protobuf that gets > > compiled into Chrome OS 41. How does your code access e.g. the > > RemoteAccessHostTokenUrl policy on M41? You could use raw protobuf to but I > > doubt that this how your code works. I would rather expect that the five new > > policies are simply unsupported on Chrome OS until this CL lands and thus, > > should be marked as 42-. > > You're right. On non-Chrome-OS Chromoting didn't depend on > policy_templates.json at all, but on Chrome-OS it does use the PolicyService > from g_browser_process (which, as I infer from your explanation above, means > that it depends on policy_templates.json generating the right protobufs). > > I changed 41 to 42 in the 5 new policies. We generate the cloud policy protobuf from policy_templates.json. Since your policies were not in policy_templates.json, they were not in the protobuf in M41. No code (other than raw protobuf) would have been able to parse these policies in M41 - neither the shared PolicyService, nor any other PolicyService or custom policy implementation.
The CQ bit was checked by sergeyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/820133002/200001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/2b91a480c5ea6fc6e20703058b1d1748dd0de332 Cr-Commit-Position: refs/heads/master@{#311968}
Message was sent while issue was closed.
Committed patchset #10 (id:200001) manually as 2b91a480c5ea6fc6e20703058b1d1748dd0de332. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
