Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367363002:
Update Public Session permissions (manifest features part) (Closed)
|
Created:
4 years, 2 months ago by Ivan Šandrk Modified:
4 years, 2 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org, extensions-reviews_chromium.org, Mattias Nissler (ping if slow) Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate Public Session permissions (manifest features part)
PS permissions have been updated (part pertaining to manifest feature).
Permission features will be updated in a seperate CL.
BUG=649726
Committed: https://crrev.com/06c7a37bf7b8bef067e6fd49d1824f4525cb137b
Cr-Commit-Position: refs/heads/master@{#421228}
Patch Set 1 #
Total comments: 4
Patch Set 2 : Add omnibox to set of allowed features #Patch Set 3 : Rebase #
Total comments: 6
Patch Set 4 : Disabled some manifest features, nits #
Total comments: 18
Patch Set 5 : Thiemo's comments #Patch Set 6 : Fixed failing tests #
Total comments: 4
Patch Set 7 : Added a crbug link to a TODO #
Messages
Total messages: 31 (16 generated)
Description was changed from ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 ========== to ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 ==========
isandrk@chromium.org changed reviewers: + tnagel@chromium.org
Hi Thiemo, ptal! Thanks, Ivan
Could you please rebase once the sorting CL has landed? I'll add more comments then. https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:141: "background", Why is there no string constant for it? Is it even used at all? https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:327: // Only hangouts is whitelisted. Nit: Hangouts is spelled with a capital H.
isandrk@chromium.org changed reviewers: + atwilson@chromium.org
Hi Drew, ptal! Thanks, Ivan
https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:153: emk::kSettingsOverride, This should be blocked! https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:171: emk::kContentScripts, This should be blocked! https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:176: emk::kConvertedFromUserScript, This should be blocked!
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:141: "background", On 2016/09/26 15:10:29, Thiemo Nagel (slow) wrote: > Why is there no string constant for it? Is it even used at all? Needs more investigation, may be the same case as "author". https://codereview.chromium.org/2367363002/diff/1/chrome/browser/chromeos/ext... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:327: // Only hangouts is whitelisted. On 2016/09/26 15:10:29, Thiemo Nagel (slow) wrote: > Nit: Hangouts is spelled with a capital H. Done. https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:153: emk::kSettingsOverride, On 2016/09/26 17:01:57, Thiemo Nagel (slow) wrote: > This should be blocked! Done. https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:171: emk::kContentScripts, On 2016/09/26 17:01:57, Thiemo Nagel (slow) wrote: > This should be blocked! Done. https://codereview.chromium.org/2367363002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:176: emk::kConvertedFromUserScript, On 2016/09/26 17:01:57, Thiemo Nagel (slow) wrote: > This should be blocked! Done.
The CQ bit was checked by tnagel@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:130: // Only allow hosted_app, platform_app. Please wrap comments at 80 characters. Independent of that, I'm not sure this is the right place to specify what is allowed. Far away from the source code doing the actual check, this is likely to become outdated at some point in the future. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:250: // Special-cased in IsSafeForPublicSession() (value == 2). Same as above: Probably not a great idea to comment on the specifics here. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:323: // Update comment. (Note: Using string literal since Is that supposed to be a TODO(isandrk)? https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:514: // List of allowed app strings. I'd prefer "safe" over "allowed" because it is clearer about the intention of the whitelist. "app strings" is not precise. I'd suggest something along the lines of 'entries for the "app" dict in manifest'. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:557: << "Extension type: " << extension->GetType(); Nit: Splitting strings increases code size. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:627: // "app" may contain one of: background, content_security_policy, As above: Please don't add comments that could easily become outdated. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:634: << ": app manifest entry is allowed only for" As above: I'd suggest to avoid splitting strings, also in that case you're missing a blank. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:721: if (extension->GetType() == extensions::Manifest::TYPE_HOSTED_APP) { I think we should drop that now as hosted apps are permitted in IsSafeForPublicSession(). https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:731: // Allow force-installed platform app if all manifest contents are "platform app" isn't the correct description anymore.
https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:130: // Only allow hosted_app, platform_app. On 2016/09/27 10:31:54, Thiemo Nagel (slow) wrote: > Please wrap comments at 80 characters. Independent of that, I'm not sure this > is the right place to specify what is allowed. Far away from the source code > doing the actual check, this is likely to become outdated at some point in the > future. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:250: // Special-cased in IsSafeForPublicSession() (value == 2). On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > Same as above: Probably not a great idea to comment on the specifics here. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:323: // Update comment. (Note: Using string literal since On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > Is that supposed to be a TODO(isandrk)? Added one TODO(isandrk) for all the missing comments. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:514: // List of allowed app strings. On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > I'd prefer "safe" over "allowed" because it is clearer about the intention of > the whitelist. > > "app strings" is not precise. I'd suggest something along the lines of 'entries > for the "app" dict in manifest'. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:557: << "Extension type: " << extension->GetType(); On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > Nit: Splitting strings increases code size. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:627: // "app" may contain one of: background, content_security_policy, On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > As above: Please don't add comments that could easily become outdated. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:634: << ": app manifest entry is allowed only for" On 2016/09/27 10:31:54, Thiemo Nagel (slow) wrote: > As above: I'd suggest to avoid splitting strings, also in that case you're > missing a blank. Done. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:721: if (extension->GetType() == extensions::Manifest::TYPE_HOSTED_APP) { On 2016/09/27 10:31:54, Thiemo Nagel (slow) wrote: > I think we should drop that now as hosted apps are permitted in > IsSafeForPublicSession(). I think we should keeps this part, let us discuss this in person. https://codereview.chromium.org/2367363002/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:731: // Allow force-installed platform app if all manifest contents are On 2016/09/27 10:31:53, Thiemo Nagel (slow) wrote: > "platform app" isn't the correct description anymore. Done.
I've verified the whitelist against the spreadsheet. LGTM, provided that tests and comments will be updated in a follow-up CL and that the issue of blanket-whitelisting of hosted apps also will be addressed in a follow-up CL.
+mnissler fyi
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:140: // TODO(isandrk): Ask Mattias for comments on entries without a comment. Please file a bug for this and put the bug number in this comment. https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc:217: values.Set("chrome_settings_overrides", new base::DictionaryValue()); In your followup CL please make sure you explicitly test that extensions can be installed.
lgtm lgtm https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:140: // TODO(isandrk): Ask Mattias for comments on entries without a comment. Please file a bug for this and put the bug number in this comment. https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc:217: values.Set("chrome_settings_overrides", new base::DictionaryValue()); In your followup CL please make sure you explicitly test that extensions can be installed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc:140: // TODO(isandrk): Ask Mattias for comments on entries without a comment. On 2016/09/27 15:15:07, Andrew T Wilson (Slow) wrote: > Please file a bug for this and put the bug number in this comment. Done. https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc (right): https://codereview.chromium.org/2367363002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/device_local_account_management_policy_provider_unittest.cc:217: values.Set("chrome_settings_overrides", new base::DictionaryValue()); On 2016/09/27 15:15:07, Andrew T Wilson (Slow) wrote: > In your followup CL please make sure you explicitly test that extensions can be > installed. Done.
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tnagel@chromium.org, atwilson@chromium.org Link to the patchset: https://codereview.chromium.org/2367363002/#ps140001 (title: "Added a crbug link to a TODO")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 ========== to ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 ========== to ========== Update Public Session permissions (manifest features part) PS permissions have been updated (part pertaining to manifest feature). Permission features will be updated in a seperate CL. BUG=649726 Committed: https://crrev.com/06c7a37bf7b8bef067e6fd49d1824f4525cb137b Cr-Commit-Position: refs/heads/master@{#421228} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/06c7a37bf7b8bef067e6fd49d1824f4525cb137b Cr-Commit-Position: refs/heads/master@{#421228} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
