Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12147004:
Disable/enable echo for enterprise device. (Closed)
|
Created:
7 years, 10 months ago by oscarpan Modified:
5 years, 1 month ago CC:
chromium-reviews, nkostylev+watch_chromium.org, Aaron Boodman, oshima+watch_chromium.org, chromium-apps-reviews_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, stephenlin, Mattias Nissler (ping if slow) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionDisable/enable echo for enterprise device.
Create a new policy to controll disable/enable Echo and make the policy value
avaialbe to Echo extension.
BUG=chromium-os:38500
TEST=unittest and manually check the flow is working.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=181649
Patch Set 1 #Patch Set 2 : Update inline comments. #
Total comments: 38
Patch Set 3 : Address Mattias and Xiyuan's comments, except one unit test Mattias required, which will be added i… #Patch Set 4 : Add browser test. #Patch Set 5 : Update some inline comments. #
Total comments: 12
Patch Set 6 : My efforts to implement the browser test for echo. #
Total comments: 17
Patch Set 7 : Address comments. #
Total comments: 6
Patch Set 8 : Address Mattias comments. #Patch Set 9 : Rebase #
Total comments: 2
Patch Set 10 : Address Antony's comment. #Patch Set 11 : Debugging. #Patch Set 12 : fix test. #Patch Set 13 : Add the missing SendResponse call to the async extension method. #Messages
Total messages: 29 (0 generated)
https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3880: 'name': 'DeviceChromeOsRegistrationEnabled', The policy was named ***Disabled, but presubmit checks complained about the naming convention to be XYZEnabled. So I updated accordingly. However, the extension api is still left as ***Disabled b/c it fits the logic better. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:114: // GetBoolean. I assume this comment is correct and seems so from my manual test, but reviewers please convince me. Thanks a lot! https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:243: } I don't know when this default value is being used! Is it used only when a device is enrolled in enterprise and but the value is not set?
+mnissler https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3880: 'name': 'DeviceChromeOsRegistrationEnabled', On 2013/02/02 01:50:56, oscarpan wrote: > The policy was named ***Disabled, but presubmit checks complained about the > naming convention to be XYZEnabled. So I updated accordingly. However, the > extension api is still left as ***Disabled b/c it fits the logic better. Please update extension api as well. I found it's a bit confusing to read through the CL. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3897: If this policy is left not set, Chrome OS Registration will not be disabled.''', I don't understand this. "will no be disabled" means "will be enabled"? https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:114: // GetBoolean. On 2013/02/02 01:50:56, oscarpan wrote: > I assume this comment is correct and seems so from my manual test, but reviewers > please convince me. Thanks a lot! I think the comment is correct. If not confortable, you can use return value of GetBoolean to set the fallback value. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); nit: 4-space indent https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); Do we need to worry that this value might not be trusted?
https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3880: 'name': 'DeviceChromeOsRegistrationEnabled', On 2013/02/02 07:49:27, xiyuan wrote: > On 2013/02/02 01:50:56, oscarpan wrote: > > The policy was named ***Disabled, but presubmit checks complained about the > > naming convention to be XYZEnabled. So I updated accordingly. However, the > > extension api is still left as ***Disabled b/c it fits the logic better. > > Please update extension api as well. I found it's a bit confusing to read > through the CL. Ok, I will fix it in my new patch. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3897: If this policy is left not set, Chrome OS Registration will not be disabled.''', On 2013/02/02 07:49:27, xiyuan wrote: > I don't understand this. "will no be disabled" means "will be enabled"? Yes, I mean "will be enabled", will fix it in my new patch. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); On 2013/02/02 07:49:27, xiyuan wrote: > Do we need to worry that this value might not be trusted? I'm not sure when this value is unreliable. Is there any way a user can modify the policy value? This reminds me another thing, right now I create this separate method to check whether Echo is disabled. But if a user can modify the extension code, he can still bypass this check and claim offers using Echo. Is that even possible to modify private extension? If so, it seems safer to remove this new api and add the check inside GetRegistrationCodeFunction, and never return the registration code if Echo is disabled.
https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); On 2013/02/02 21:52:35, oscarpan wrote: > On 2013/02/02 07:49:27, xiyuan wrote: > > Do we need to worry that this value might not be trusted? > I'm not sure when this value is unreliable. Is there any way a user can modify > the policy value? This reminds me another thing, right now I create this > separate method to check whether Echo is disabled. But if a user can modify the > extension code, he can still bypass this check and claim offers using Echo. Is > that even possible to modify private extension? If so, it seems safer to remove > this new api and add the check inside GetRegistrationCodeFunction, and never > return the registration code if Echo is disabled. I am not worrying about extension code being changed. It's more about the value returned from GetBoolean call. I have an impression that the settings values are cached locally somewhere to speed things up. The cached values are not safe because user might temper with it. If this is a problem, then you need to call CrsoSettings::PrepareTrustedValues to ensure GetBoolean will return a trusted value (value that passed signature check etc). It's quite simple to use. You simple add a PrepareTrustedValues call before you call GetBoolean and put your function as the callback when trusted values are available. Of course, if you do this, you might need to change it to be an AsyncExtensionFunction. One example could be found here: https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/polic...
Regarding the commit messages: It has typos, and the TEST= line isn't very helpful: "the flow is working" doesn't make sense to anybody not already intimately familiar with the feature. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3883: 'supported_on': ['chrome.*:26-'], Shouldn't this be 'chrome_os.*:26-'? https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3891: 'desc': '''Enable Chrome OS Registration. So "Chrome OS Registration" is the official term for ECHO? I think that's kind of unfortunate naming, as an admin I'd have no idea what this thing is. Can you add one sentence to the description that recaps the high-level idea behind ECHO? https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3895: If this policy is set to false, Chrome OS Registration will be disabled, and user will not be able to use Goodies to redeem offers. Playing dump admin again: What are Goodies and offers? https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3897: If this policy is left not set, Chrome OS Registration will not be disabled.''', On 2013/02/02 21:52:35, oscarpan wrote: > On 2013/02/02 07:49:27, xiyuan wrote: > > I don't understand this. "will no be disabled" means "will be enabled"? > Yes, I mean "will be enabled", will fix it in my new patch. If there's no difference between set to true and unset, merge the two sentences. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:38: if (!chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) { You should check the policy value here too - we want some confidence that there's no way to retrieve coupon codes when the policy says echo is disabled. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:114: // GetBoolean. On 2013/02/02 01:50:56, oscarpan wrote: > I assume this comment is correct and seems so from my manual test, but reviewers > please convince me. Thanks a lot! It should be you convincing the reviewers ;) In these cases, please make the effort to check the source whether the assumption is justified. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.h (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.h:38: class IsEchoDisabledFunction : public SyncExtensionFunction { I don't see why this shouldn't better be named IsEchoEnabled? https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/cros_settings_names.cc:87: const char kChromeOsRegistrationEnabled[] = document. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/device_settings_provider.cc:565: DecodeGenericPolicies(settings, &new_values_cache); You need to add some decoding, otherwise you'll never see the policy value. Have you tested this? https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:241: // Determines if a device can use Chrome OS Registration service. Again, needs better docs. Pretend you hear "Chrome OS Registration service" the first time and need to make sense of it - then write a message that'd be helpful to you in that situation. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:243: } On 2013/02/02 01:50:56, oscarpan wrote: > I don't know when this default value is being used! Is it used only when a > device is enrolled in enterprise and but the value is not set? Device settings apply to both enterprise-enrolled and consumer machines. In the latter context, the device owner account is in control of them. The default value determines what value the generated protobuf class returns if the value is not present. Thus it is only really relevant in decoding, and even then you can implement the right behavior regardless of the default value by calling has_chrome_os_registration_enabled() before making decisions based on the value. See the code in DeviceSettingsProvider::DecodeLoginPolicies() for examples. I think it still makes sense to leave the default = true here to document that the feature is default-on. https://codereview.chromium.org/12147004/diff/3001/chrome/test/data/extension... File chrome/test/data/extensions/api_test/echo/component_extension/test.js (right): https://codereview.chromium.org/12147004/diff/3001/chrome/test/data/extension... chrome/test/data/extensions/api_test/echo/component_extension/test.js:29: })); You should also write a browser_test that checks whether the extension API function(s) affected by the policy behave as expected if the policy is set. See chrome/browser/policy/policy_browsertest.cc for examples.
The commit message has lots of typos, and the second half of the TEST= instructions is useless = "the flow is working" doesn't make sense to anybody who isn't intimately familiar with the feature already.
On Mon, Feb 4, 2013 at 10:06 AM, <mnissler@chromium.org> wrote: > The commit message has lots of typos, and the second half of the TEST= > instructions is useless = "the flow is working" doesn't make sense to > anybody > who isn't intimately familiar with the feature already. > Now you have it twice ;) Sorry, I submitted the form in a tab I had abandoned.
https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... File chrome/app/policy/policy_templates.json (right): https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3883: 'supported_on': ['chrome.*:26-'], On 2013/02/04 09:06:19, Mattias Nissler wrote: > Shouldn't this be 'chrome_os.*:26-'? Done. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3891: 'desc': '''Enable Chrome OS Registration. On 2013/02/04 09:06:19, Mattias Nissler wrote: > So "Chrome OS Registration" is the official term for ECHO? I think that's kind > of unfortunate naming, as an admin I'd have no idea what this thing is. Can you > add one sentence to the description that recaps the high-level idea behind ECHO? Done. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3895: If this policy is set to false, Chrome OS Registration will be disabled, and user will not be able to use Goodies to redeem offers. On 2013/02/04 09:06:19, Mattias Nissler wrote: > Playing dump admin again: What are Goodies and offers? Done. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3897: If this policy is left not set, Chrome OS Registration will not be disabled.''', On 2013/02/04 09:06:19, Mattias Nissler wrote: > On 2013/02/02 21:52:35, oscarpan wrote: > > On 2013/02/02 07:49:27, xiyuan wrote: > > > I don't understand this. "will no be disabled" means "will be enabled"? > > Yes, I mean "will be enabled", will fix it in my new patch. > > If there's no difference between set to true and unset, merge the two sentences. Done. https://codereview.chromium.org/12147004/diff/3001/chrome/app/policy/policy_t... chrome/app/policy/policy_templates.json:3897: If this policy is left not set, Chrome OS Registration will not be disabled.''', On 2013/02/04 09:06:19, Mattias Nissler wrote: > On 2013/02/02 21:52:35, oscarpan wrote: > > On 2013/02/02 07:49:27, xiyuan wrote: > > > I don't understand this. "will no be disabled" means "will be enabled"? > > Yes, I mean "will be enabled", will fix it in my new patch. > > If there's no difference between set to true and unset, merge the two sentences. Done. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); On 2013/02/02 07:49:27, xiyuan wrote: > nit: 4-space indent Done. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.cc:116: chromeos::kChromeOsRegistrationEnabled, &isEnabled); On 2013/02/02 22:44:16, xiyuan wrote: > On 2013/02/02 21:52:35, oscarpan wrote: > > On 2013/02/02 07:49:27, xiyuan wrote: > > > Do we need to worry that this value might not be trusted? > > I'm not sure when this value is unreliable. Is there any way a user can modify > > the policy value? This reminds me another thing, right now I create this > > separate method to check whether Echo is disabled. But if a user can modify > the > > extension code, he can still bypass this check and claim offers using Echo. Is > > that even possible to modify private extension? If so, it seems safer to > remove > > this new api and add the check inside GetRegistrationCodeFunction, and never > > return the registration code if Echo is disabled. > > I am not worrying about extension code being changed. It's more about the value > returned from GetBoolean call. I have an impression that the settings values are > cached locally somewhere to speed things up. The cached values are not safe > because user might temper with it. If this is a problem, then you need to call > CrsoSettings::PrepareTrustedValues to ensure GetBoolean will return a trusted > value (value that passed signature check etc). > > It's quite simple to use. You simple add a PrepareTrustedValues call before > you call GetBoolean and put your function as the callback when trusted values > are available. Of course, if you do this, you might need to change it to be an > AsyncExtensionFunction. > > One example could be found here: > https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/polic... How about we postpone this feature to later cls. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... File chrome/browser/chromeos/extensions/echo_private_api.h (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/ex... chrome/browser/chromeos/extensions/echo_private_api.h:38: class IsEchoDisabledFunction : public SyncExtensionFunction { On 2013/02/04 09:06:19, Mattias Nissler wrote: > I don't see why this shouldn't better be named IsEchoEnabled? Done. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/cros_settings_names.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/cros_settings_names.cc:87: const char kChromeOsRegistrationEnabled[] = On 2013/02/04 09:06:19, Mattias Nissler wrote: > document. Done. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/device_settings_provider.cc:565: DecodeGenericPolicies(settings, &new_values_cache); On 2013/02/04 09:06:19, Mattias Nissler wrote: > You need to add some decoding, otherwise you'll never see the policy value. Have > you tested this? Do you mean the decoder I added to "device_policy_decoder_chromeos.cc" ? https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... File chrome/browser/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:241: // Determines if a device can use Chrome OS Registration service. On 2013/02/04 09:06:19, Mattias Nissler wrote: > Again, needs better docs. Pretend you hear "Chrome OS Registration service" the > first time and need to make sense of it - then write a message that'd be helpful > to you in that situation. Done. https://codereview.chromium.org/12147004/diff/3001/chrome/browser/policy/prot... chrome/browser/policy/proto/chrome_device_policy.proto:243: } I verified that without setting the policy value, the default value is used and the echo flow works properly. On 2013/02/04 09:06:19, Mattias Nissler wrote: > On 2013/02/02 01:50:56, oscarpan wrote: > > I don't know when this default value is being used! Is it used only when a > > device is enrolled in enterprise and but the value is not set? > > Device settings apply to both enterprise-enrolled and consumer machines. In the > latter context, the device owner account is in control of them. The default > value determines what value the generated protobuf class returns if the value is > not present. Thus it is only really relevant in decoding, and even then you can > implement the right behavior regardless of the default value by calling > has_chrome_os_registration_enabled() before making decisions based on the value. > See the code in DeviceSettingsProvider::DecodeLoginPolicies() for examples. I > think it still makes sense to leave the default = true here to document that the > feature is default-on. https://codereview.chromium.org/12147004/diff/3001/chrome/test/data/extension... File chrome/test/data/extensions/api_test/echo/component_extension/test.js (right): https://codereview.chromium.org/12147004/diff/3001/chrome/test/data/extension... chrome/test/data/extensions/api_test/echo/component_extension/test.js:29: })); Working on it. On 2013/02/04 09:06:19, Mattias Nissler wrote: > You should also write a browser_test that checks whether the extension API > function(s) affected by the policy behave as expected if the policy is set. See > chrome/browser/policy/policy_browsertest.cc for examples.
https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:312: } Apparently my test failed. It complained "Cannot call method 'allowRedeemOffers' of undefined". Because echoPrivate.allowRedeemOffers is a private api and can only run in extension (instead of web page). I don't know how to test the new policy in this case... Should I not use javascript at all?
https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12147004/diff/3001/chrome/browser/chromeos/se... chrome/browser/chromeos/settings/device_settings_provider.cc:565: DecodeGenericPolicies(settings, &new_values_cache); On 2013/02/05 00:17:47, oscarpan wrote: > On 2013/02/04 09:06:19, Mattias Nissler wrote: > > You need to add some decoding, otherwise you'll never see the policy value. > Have > > you tested this? > Do you mean the decoder I added to "device_policy_decoder_chromeos.cc" ? No, that's a different beast which is only relevant to decoding the values into the format used by chrome://policy https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:38: if (!chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) { Reminder: Introduce a check here as well. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:116: // TODO(oscarpan): Check if we need to use PreparedTrustedValues. You should be using this. As pointed out by Xiyuan, it's not that hard - I don't see why to postpone that bit. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:48: kAllowRedeemChromeOsRegistrationOffers, nit: alphabetize https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:565: DecodeGenericPolicies(settings, &new_values_cache); Decoding still missing here - you'll never see the policy value in CrosSettings if you don't put code here. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:312: } On 2013/02/05 05:39:22, oscarpan wrote: > Apparently my test failed. It complained "Cannot call method 'allowRedeemOffers' > of undefined". Because echoPrivate.allowRedeemOffers is a private api and can > only run in extension (instead of web page). I don't know how to test the new > policy in this case... Should I not use javascript at all? Have you made sure the extension is actually loaded for browser_tests? You may have to do that manually. Another thing to try would be to have the tab navigate to a URL that's within that extension's origin. Also, aren't there browser tests for the echo private API already? If they exist, they probably tell you what you need to do (or you can add your test case there).
https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:38: if (!chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) { On 2013/02/05 13:06:53, Mattias Nissler wrote: > Reminder: Introduce a check here as well. Added a TODO, we'd like to get this cl in first, so let's revisit this check in later cl! https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:116: // TODO(oscarpan): Check if we need to use PreparedTrustedValues. On 2013/02/05 13:06:53, Mattias Nissler wrote: > You should be using this. As pointed out by Xiyuan, it's not that hard - I don't > see why to postpone that bit. Done. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... File chrome/browser/chromeos/settings/device_settings_provider.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:48: kAllowRedeemChromeOsRegistrationOffers, On 2013/02/05 13:06:53, Mattias Nissler wrote: > nit: alphabetize Done. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/s... chrome/browser/chromeos/settings/device_settings_provider.cc:565: DecodeGenericPolicies(settings, &new_values_cache); On 2013/02/05 13:06:53, Mattias Nissler wrote: > Decoding still missing here - you'll never see the policy value in CrosSettings > if you don't put code here. Done. https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:312: } There are some tricky parts. First our extension is a component extension, which supposed to be loaded automatically (not certain, someone told me). I indeed tried to install our extension again (including adding a new crx file to the test data dir), but still I'm having the same issue. Even I installed the extension, I cannot navigate to it (but I can on the chrome on my linux desktop). For our echo extension, we do have an extension browser test at https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom..., But that's parallel to policy browser tests. I was trying to copy some of the codes from policy_browsertest.cc to echo_private_apitest.cc but that seems to involve too much work, and I've not yet suceeded. I'm thinking if we revisit this test case in a latter cl, so that we are not blocked on it. I'm very welcome to work on within this cl if there are some obvious solution that i'm not realize. What do you think? On 2013/02/05 13:06:53, Mattias Nissler wrote: > On 2013/02/05 05:39:22, oscarpan wrote: > > Apparently my test failed. It complained "Cannot call method > 'allowRedeemOffers' > > of undefined". Because echoPrivate.allowRedeemOffers is a private api and can > > only run in extension (instead of web page). I don't know how to test the new > > policy in this case... Should I not use javascript at all? > > Have you made sure the extension is actually loaded for browser_tests? You may > have to do that manually. Another thing to try would be to have the tab navigate > to a URL that's within that extension's origin. > > Also, aren't there browser tests for the echo private API already? If they > exist, they probably tell you what you need to do (or you can add your test case > there).
https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:106: #include "chrome/browser/extensions/component_loader.h" Either remove or reorder the imports in future, as this version is not finalized. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:557: This seems a bit ugly. I'm trying to load my extension as a component extension. This cannot be done by InstallExtension() https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:1356: content::Referrer(), Although it seems it can access chrome.echoPrivate api now, but i'm getting the following error when running the test. [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified request id: 0 ../../chrome/browser/policy/policy_browsertest.cc:1368: Failure Value of: CheckAllowRedeemCrosOffers(contents) Actual: true Expected: false [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified request id: 1 [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified request id: 2 [ FAILED ] PolicyTest.AllowRedeemChromeOsRegistrationOffers, where TypeParam = and GetParam() = (3579 ms) [----------] 1 test from PolicyTest (3580 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (3580 ms total) [ PASSED ] 0 tests. [ FAILED ] 1 test, listed below: [ FAILED ] PolicyTest.AllowRedeemChromeOsRegistrationOffers, where TypeParam = and GetParam() = 1 FAILED TEST [0205/202054:ERROR:process_util_posix.cc(297)] Unable to terminate process group 17702: No such process
https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:115: return; Let's add a AddRef() before return here to ensure this object does not go away before the callback happens. And balance it by adding a Release() after SendResponse below. I wonder if that would fix the request_sender.cc error you saw in your test. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:131: return false; It does not matter result for AsyncExtensionFunction. But we should return true here to indicate the call succeeds to be consistent with other code. My bad. :p
https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/25001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:38: if (!chromeos::KioskModeSettings::Get()->IsKioskModeEnabled()) { On 2013/02/06 04:34:54, oscarpan wrote: > On 2013/02/05 13:06:53, Mattias Nissler wrote: > > Reminder: Introduce a check here as well. > > Added a TODO, we'd like to get this cl in first, so let's revisit this check in > later cl! Why? Checking whether the policy is on or off is the same code as below. The concern I have is that people can still call getRegistrationCode(), even if the allowRedeemOffers returns false. What's your plan to prevent this? I'm not a fan of committing half-finished known-broken code, in particular if the fix is as simple as adding a check that you already have. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:111: if (chromeos::CrosSettingsProvider::TRUSTED != There's an edge case here: Devices that don't have an owner yet (for example, guest sessions immediately after OOBE), will return PERMANENTLY_UNTRUSTED. Is allowing ECHO the right call for these cases? I guess so, just checking. However, in these cases you don't get a callback, so you may want to change the logic to only post the callback if the return value is TEMPORARILY_UNTRUSTED. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:127: // value. This policy is used to controll whether user can redeem offers using *control https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:557: On 2013/02/06 04:38:45, oscarpan wrote: > This seems a bit ugly. I'm trying to load my extension as a component extension. > This cannot be done by InstallExtension() That's fine. The alternative would be to add the test to echo_private_apitest.cc and use RunComponentExtensionTest, but then you'll have to find a way to inject policy into the extension testing framework - I'm not familiar enough with that to know whether that's easily possible. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:1356: content::Referrer(), On 2013/02/06 04:38:45, oscarpan wrote: > Although it seems it can access chrome.echoPrivate api now, but i'm getting the > following error when running the test. > > [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified > request id: 0 > ../../chrome/browser/policy/policy_browsertest.cc:1368: Failure > Value of: CheckAllowRedeemCrosOffers(contents) > Actual: true > Expected: false > [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified > request id: 1 > [17747:17747:0205/202052:ERROR:request_sender.cc(133)] Could not find specified > request id: 2 > [ FAILED ] PolicyTest.AllowRedeemChromeOsRegistrationOffers, where TypeParam = > and GetParam() = (3579 ms) > [----------] 1 test from PolicyTest (3580 ms total) > > [----------] Global test environment tear-down > [==========] 1 test from 1 test case ran. (3580 ms total) > [ PASSED ] 0 tests. > [ FAILED ] 1 test, listed below: > [ FAILED ] PolicyTest.AllowRedeemChromeOsRegistrationOffers, where TypeParam = > and GetParam() = > > 1 FAILED TEST > [0205/202054:ERROR:process_util_posix.cc(297)] Unable to terminate process group > 17702: No such process Seems like you want to attach a debugger and check why the policy value is not taking effect in the extension function. https://codereview.chromium.org/12147004/diff/29001/chrome/test/data/extensio... File chrome/test/data/extensions/echo_component_extension/main.html (right): https://codereview.chromium.org/12147004/diff/29001/chrome/test/data/extensio... chrome/test/data/extensions/echo_component_extension/main.html:4: <div id="dialog"></div> any reason for this change?
https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/dev... File chrome/browser/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/dev... chrome/browser/policy/device_policy_decoder_chromeos.cc:420: if (container.allow_redeem_offers()) { This is the reason why you don't see the false value in about:policy. You should set the value of the policy in |policies| when the policy is present, hence you need an has_allow_redeem_offers() check here, not the allow_redeem_offers() check you have which just reads the value. Sorry for not catching it earlier, 4 missing characters are hard to spot :)
https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:111: if (chromeos::CrosSettingsProvider::TRUSTED != I think we should only disable echo when we are certain about the value. So yes, i think we should allow echo in those cases. On 2013/02/06 10:03:26, Mattias Nissler wrote: > There's an edge case here: Devices that don't have an owner yet (for example, > guest sessions immediately after OOBE), will return PERMANENTLY_UNTRUSTED. > > Is allowing ECHO the right call for these cases? I guess so, just checking. > > However, in these cases you don't get a callback, so you may want to change the > logic to only post the callback if the return value is TEMPORARILY_UNTRUSTED. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:115: return; This seems fixed by other my other changes. No longer needed. On 2013/02/06 05:28:11, xiyuan wrote: > Let's add a AddRef() before return here to ensure this object does not go away > before the callback happens. And balance it by adding a Release() after > SendResponse below. > > I wonder if that would fix the request_sender.cc error you saw in your test. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:127: // value. This policy is used to controll whether user can redeem offers using On 2013/02/06 10:03:26, Mattias Nissler wrote: > *control Done. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:131: return false; On 2013/02/06 05:28:11, xiyuan wrote: > It does not matter result for AsyncExtensionFunction. But we should return true > here to indicate the call succeeds to be consistent with other code. My bad. :p Done. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/dev... File chrome/browser/policy/device_policy_decoder_chromeos.cc (right): https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/dev... chrome/browser/policy/device_policy_decoder_chromeos.cc:420: if (container.allow_redeem_offers()) { On 2013/02/06 10:25:22, Mattias Nissler wrote: > This is the reason why you don't see the false value in about:policy. You should > set the value of the policy in |policies| when the policy is present, hence you > need an has_allow_redeem_offers() check here, not the allow_redeem_offers() > check you have which just reads the value. Sorry for not catching it earlier, 4 > missing characters are hard to spot :) Done. https://codereview.chromium.org/12147004/diff/29001/chrome/browser/policy/dev... chrome/browser/policy/device_policy_decoder_chromeos.cc:420: if (container.allow_redeem_offers()) { On 2013/02/06 10:25:22, Mattias Nissler wrote: > This is the reason why you don't see the false value in about:policy. You should > set the value of the policy in |policies| when the policy is present, hence you > need an has_allow_redeem_offers() check here, not the allow_redeem_offers() > check you have which just reads the value. Sorry for not catching it earlier, 4 > missing characters are hard to spot :) Done.
https://codereview.chromium.org/12147004/diff/32002/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/32002/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:1367: UpdateProviderPolicy(policies); So I confirmed that all other codes are running as expected. The only issue is UpdateProviderPolicy will never update my policy. To update the device policy, SetInPolicy(https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chromeos/settings/device_settings_provider.cc&sq=package:chrome&type=cs&rcl=1360158437&l=697) has to be triggered, but the condition {if (trusted_status_ == TRUSTED && !pending_changes_.empty())} is never satisfied, because the flow always goes here (case DeviceSettingsService::STORE_KEY_UNAVAILABLE:) https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom.... I think the reason is our policy is device policy and it is not handled properly in this policy_browsertest.cc. So do you think we could postpone adding this test given that the complexity is beyond the goal of this cl?
I didn't find an answer to my comment regarding checking the policy also in GetRegistrationCode(), and the code still doesn't have the check. Any update on that?` https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:38: // TODO(oscarpan) update to use PrepareTrustedValues. still missing a policy check here? https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:115: if (status == chromeos::CrosSettingsProvider::TEMPORARILY_UNTRUSTED) { nit: no need for curly braces. https://codereview.chromium.org/12147004/diff/32002/chrome/browser/policy/pol... File chrome/browser/policy/policy_browsertest.cc (right): https://codereview.chromium.org/12147004/diff/32002/chrome/browser/policy/pol... chrome/browser/policy/policy_browsertest.cc:1367: UpdateProviderPolicy(policies); On 2013/02/07 00:40:39, oscarpan wrote: > So I confirmed that all other codes are running as expected. The only issue is > UpdateProviderPolicy will never update my policy. > To update the device policy, > SetInPolicy(https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chromeos/settings/device_settings_provider.cc&sq=package:chrome&type=cs&rcl=1360158437&l=697) > has to be triggered, but the condition {if (trusted_status_ == TRUSTED && > !pending_changes_.empty())} is never satisfied, because the flow always goes > here (case DeviceSettingsService::STORE_KEY_UNAVAILABLE:) > https://code.google.com/p/chromium/codesearch#chrome/src/chrome/browser/chrom.... > > I think the reason is our policy is device policy and it is not handled properly > in this policy_browsertest.cc. Ah, that makes sense! > So do you think we could postpone adding this > test given that the complexity is beyond the goal of this cl? Sounds fair to me.
https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... File chrome/browser/chromeos/extensions/echo_private_api.cc (right): https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:38: // TODO(oscarpan) update to use PrepareTrustedValues. On 2013/02/07 10:45:57, Mattias Nissler wrote: > still missing a policy check here? Done. https://codereview.chromium.org/12147004/diff/32002/chrome/browser/chromeos/e... chrome/browser/chromeos/extensions/echo_private_api.cc:115: if (status == chromeos::CrosSettingsProvider::TEMPORARILY_UNTRUSTED) { On 2013/02/07 10:45:57, Mattias Nissler wrote: > nit: no need for curly braces. Done.
LGTM
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oscarpan@google.com/12147004/29018
Presubmit check for 12147004-29018 failed and returned exit status 1.
INFO:root:Found 13 file(s).
Running presubmit commit checks ...
Running /b/commit-queue/workdir/chromium/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/app/policy/PRESUBMIT.py
Finished checking
/b/commit-queue/workdir/chromium/chrome/app/policy/policy_templates.json. 0
errors, 0 warnings.
Running /b/commit-queue/workdir/chromium/chrome/browser/extensions/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/browser/policy/PRESUBMIT.py
Running /b/commit-queue/workdir/chromium/chrome/common/extensions/PRESUBMIT.py
** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
chrome/common/extensions/api
chrome/browser/extensions
Presubmit checks took 2.6s to calculate.
chrome/{browser,common}/extensions LGTM
https://chromiumcodereview.appspot.com/12147004/diff/29018/chrome/common/exte...
File chrome/common/extensions/api/echo_private.json (right):
https://chromiumcodereview.appspot.com/12147004/diff/29018/chrome/common/exte...
chrome/common/extensions/api/echo_private.json:54: "name": "allowRedeemOffers",
optional naming suggestion: it looks like the other functions here begin with
"get", so the fact that this one does not makes it sound a little more like this
would let you toggle whether redeeming offers was allowed or not. No big deal to
leave it as-is, but you might consider adding a "get" prefix to the name for
consistency.
https://chromiumcodereview.appspot.com/12147004/diff/29018/chrome/common/exte... File chrome/common/extensions/api/echo_private.json (right): https://chromiumcodereview.appspot.com/12147004/diff/29018/chrome/common/exte... chrome/common/extensions/api/echo_private.json:54: "name": "allowRedeemOffers", Maybe checkAllowRedeemOffers? On 2013/02/09 00:19:33, Antony Sargent wrote: > optional naming suggestion: it looks like the other functions here begin with > "get", so the fact that this one does not makes it sound a little more like this > would let you toggle whether redeeming offers was allowed or not. No big deal to > leave it as-is, but you might consider adding a "get" prefix to the name for > consistency.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oscarpan@google.com/12147004/19006
Retried try job too often on linux_chromeos for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/oscarpan@google.com/12147004/32009
Message was sent while issue was closed.
Change committed as 181649 |
