Site Isolation enterprise policy should be able to disable field trial |
|||||||||||||||||
Issue descriptionWe added enterprise policies for SitePerProcess and IsolateOrigins in r518725 ( issue 783842 and issue 760761 ). https://www.chromium.org/administrators/policy-list-3#SitePerProcess https://www.chromium.org/administrators/policy-list-3#IsolateOrigins The documentation for both polices says: "If the policy is disabled, the per-Site Isolation process management logic will take effect. If the policy is not configured, the user will be able to change this setting." However, the current implementation does not actually disable either SitePerProcess or IsolateOrigins if a field trial turns them on. It should be possible for enterprises to turn off these field trials if they cause problems within the enterprise, similar to the --disable-site-isolation-trials flag we added in r542288 for issue 817137 . (It's my understanding that there's no way for an enterprise to deploy the --disable-site-isolation-trials flag directly, and that would have to be done on a per-machine basis today.) We should make the implementation of the policy match the description a little more closely: 1) Enabling the SitePerProcess policy turns on SitePerProcess (regardless of other settings). Specifying a string for the IsolateOrigins policy turns on isolation for those origins (regardless of other settings). This already works today. 2) Disabling the SitePerProcess policy should override SitePerProcess field trials (like issue 817137 ), but not about:flags or the command line. We could use --disable-site-isolation-trials to implement this, although that would also disable IsolateOrigin field trials. (Maybe that's ok.) Similarly, disabling the IsolateOrigins policy should override IsolateOrigin field trials. (Again, it might disable both SitePerProcess and IsolateOrigins field trials if we implement it with --disable-site-isolation-trials.) 3) Leaving either policy unspecified should allow field trials to work. This already works today, though we'll have to be careful to still allow it by distinguishing between "default" and "disabled" states for the enterprise policy. I'm not sure how to do that in ChromeBrowserMainParts::PreCreateThreadsImpl() where the policy is currently implemented. We should also update the description of the policies while we're at it (describing the field trial effects similar to the language used for kSiteIsolationTrialOptOutDescription). We should also remove or rephrase the "NOTE: This policy is experimental and may break functionality!" warning, now that the behavior is largely ready to launch in M66/M67.
,
Apr 2 2018
I will take a look this week. I think it makes sense to implement 2) while keeping 1) and 3) as is.
,
Apr 5 2018
Thanks! Friendly ping, just to see if you still have a chance to look this week.
,
Apr 5 2018
Hey, I did have time to look into that today. The CL is in review here https://chromium-review.googlesource.com/c/chromium/src/+/997737 Adding pmarko to make the necessary changes to session_manager to enable this feature on ChromeOS as well and to verify it works as intended there.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d0cde58276022e8f88b83152a423b7bbc9cd1b2 commit 1d0cde58276022e8f88b83152a423b7bbc9cd1b2 Author: Julian Pastarmov <pastarmovj@chromium.org> Date: Thu Apr 12 09:35:21 2018 Allow the Site Isolation policies to override related trials. The site isolation policies until now could only enable site isolation but being set to disabled was equivalent to not being set at all. With this change their disabled state is going to prevent trials from turning on site isolation per default. The user can still decide to enable site isolation by using the command line flags directly. Additionally fixes the problem of adding the isolated origins flag twice if the browser is restarted programmatically. BUG= 827821 , 826624 Change-Id: I754e1d8837b5afb0e7a70acbdb2e823733ad9663 Reviewed-on: https://chromium-review.googlesource.com/997737 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Julian Pastarmov <pastarmovj@chromium.org> Cr-Commit-Position: refs/heads/master@{#550124} [modify] https://crrev.com/1d0cde58276022e8f88b83152a423b7bbc9cd1b2/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/1d0cde58276022e8f88b83152a423b7bbc9cd1b2/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/1d0cde58276022e8f88b83152a423b7bbc9cd1b2/chrome/browser/policy/site_isolation_policy_browsertest.cc [modify] https://crrev.com/1d0cde58276022e8f88b83152a423b7bbc9cd1b2/components/policy/resources/policy_templates.json
,
Apr 12 2018
Thanks for the quick work to get this in for M67!
,
Apr 12 2018
[Auto-generated comment by a script] We noticed that this issue is targeted for M-67; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-67 label, otherwise remove Merge-TBD label. Thanks.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/eadfad93b548d2dad2ed0d8d4ec96ff1f0fa9d44 commit eadfad93b548d2dad2ed0d8d4ec96ff1f0fa9d44 Author: Pavol Marko <pmarko@chromium.org> Date: Thu Apr 12 17:01:04 2018 login: Allow explicit disabling of site isolation When site isolation device policies are set to a value that should disable site isolation (DeviceLoginScreenSitePerProcess is false, or DeviceLoginScreenIsolateOrigins is empty), pass an additional flag to prevent trials from enabling site isolation. Note: Please see also the respective user policy evaluation change on the chrome side in CL:997737. BUG= chromium:827821 TEST=cros_run_unit_tests --board=${BOARD} --packages \ chromeos-base/chromeos-login Change-Id: I286d0187ff79dc60e463a2971430cff4620c4cd8 Reviewed-on: https://chromium-review.googlesource.com/1005155 Commit-Ready: Pavol Marko <pmarko@chromium.org> Tested-by: Pavol Marko <pmarko@chromium.org> Reviewed-by: Mattias Nissler <mnissler@chromium.org> [modify] https://crrev.com/eadfad93b548d2dad2ed0d8d4ec96ff1f0fa9d44/login_manager/device_policy_service.cc [modify] https://crrev.com/eadfad93b548d2dad2ed0d8d4ec96ff1f0fa9d44/login_manager/device_policy_service_unittest.cc
,
Apr 12 2018
Julian/Pavol: Would you be able to verify these fixes in tonight's Canary and request merge to M66 if they're safe? We were just planning to have them for M67, but Abdul raised a good point that they may be important to have for the M66 stable experiment as well, in case any enterprises encounter functional problems. It's my understanding that the CLs should be pretty safe to merge-- they just append the switch to disable trials if the policy is explicitly disabled, and they're pretty well tested. The flag they depend on was added in r542288 for issue 817137 , and it was already merged to M66. Thanks!
,
Apr 13 2018
Requesting merge for https://chromium-review.googlesource.com/997737. I verified that Chrome Canary on windows 67.0.3396.0 contains the patch and works as expected. When SitePerProcess is set to false it causes the flag to disable SI trials to appear. When IsolateOrigins is set an empty strings as well (in this case also an empty --isolate-origins is appended but I think this should ok).
,
Apr 13 2018
This bug requires manual review: We are only 3 days from stable. Please contact the milestone owner if you have questions. Owners: cmasso@(Android), cmasso@(iOS), josafat@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 13 2018
For Chrome OS, also requesting merge for https://chromium-review.googlesource.com/1005155 after manually verifying on Chrome OS 10574.0.0-rc1 (Chrome 67.0.3396.0) caroline image from the PFQ (pre-flight queue).
,
Apr 13 2018
Thanks for verifying on short notice! I can try to help with the merges if Abdul approves the request, giving the timing.
,
Apr 13 2018
Well, I can help with Julian's CL, anyway, which Gerrit is able to cleanly merge. Pavol's CL might require some different steps to merge than I'm familiar with, since it's on ChromeOS and refs/branch-heads/3359 isn't recognized by Gerrit's Cherry-pick feature.
,
Apr 13 2018
Approving Julian's change in #10 for merge to M66. Branch:3359 Josafat@: can you please review change in #12?
,
Apr 13 2018
,
Apr 13 2018
release-R66-10452.B is the M66 branch for chrome OS
,
Apr 13 2018
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/764fa1d241653d7b37d7201389efac90c69b1359 commit 764fa1d241653d7b37d7201389efac90c69b1359 Author: Julian Pastarmov <pastarmovj@chromium.org> Date: Fri Apr 13 18:10:00 2018 Allow the Site Isolation policies to override related trials. The site isolation policies until now could only enable site isolation but being set to disabled was equivalent to not being set at all. With this change their disabled state is going to prevent trials from turning on site isolation per default. The user can still decide to enable site isolation by using the command line flags directly. Additionally fixes the problem of adding the isolated origins flag twice if the browser is restarted programmatically. BUG= 827821 , 826624 Change-Id: I754e1d8837b5afb0e7a70acbdb2e823733ad9663 Reviewed-on: https://chromium-review.googlesource.com/997737 Reviewed-by: Jochen Eisinger <jochen@chromium.org> Reviewed-by: Achuith Bhandarkar <achuith@chromium.org> Reviewed-by: Charlie Reis <creis@chromium.org> Commit-Queue: Julian Pastarmov <pastarmovj@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#550124}(cherry picked from commit 1d0cde58276022e8f88b83152a423b7bbc9cd1b2) Reviewed-on: https://chromium-review.googlesource.com/1012137 Cr-Commit-Position: refs/branch-heads/3359@{#704} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/764fa1d241653d7b37d7201389efac90c69b1359/chrome/browser/chrome_browser_main.cc [modify] https://crrev.com/764fa1d241653d7b37d7201389efac90c69b1359/chrome/browser/chromeos/login/session/user_session_manager.cc [modify] https://crrev.com/764fa1d241653d7b37d7201389efac90c69b1359/chrome/browser/policy/site_isolation_policy_browsertest.cc [modify] https://crrev.com/764fa1d241653d7b37d7201389efac90c69b1359/components/policy/resources/policy_templates.json
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromiumos/platform2/+/bda965bc263e873323e68a09628fddced620f810 commit bda965bc263e873323e68a09628fddced620f810 Author: Pavol Marko <pmarko@chromium.org> Date: Fri Apr 13 18:21:43 2018 login: Allow explicit disabling of site isolation When site isolation device policies are set to a value that should disable site isolation (DeviceLoginScreenSitePerProcess is false, or DeviceLoginScreenIsolateOrigins is empty), pass an additional flag to prevent trials from enabling site isolation. Note: Please see also the respective user policy evaluation change on the chrome side in CL:997737. BUG= chromium:827821 TEST=cros_run_unit_tests --board=${BOARD} --packages \ chromeos-base/chromeos-login Change-Id: I286d0187ff79dc60e463a2971430cff4620c4cd8 Reviewed-on: https://chromium-review.googlesource.com/1005155 Commit-Ready: Pavol Marko <pmarko@chromium.org> Tested-by: Pavol Marko <pmarko@chromium.org> Reviewed-by: Mattias Nissler <mnissler@chromium.org> (cherry picked from commit eadfad93b548d2dad2ed0d8d4ec96ff1f0fa9d44) Reviewed-on: https://chromium-review.googlesource.com/1012602 Reviewed-by: Charlie Reis <creis@chromium.org> Reviewed-by: Josafat Garcia <josafat@chromium.org> Commit-Queue: Charlie Reis <creis@chromium.org> Commit-Queue: Josafat Garcia <josafat@chromium.org> Tested-by: Charlie Reis <creis@chromium.org> Tested-by: Josafat Garcia <josafat@chromium.org> Trybot-Ready: Josafat Garcia <josafat@chromium.org> [modify] https://crrev.com/bda965bc263e873323e68a09628fddced620f810/login_manager/device_policy_service.cc [modify] https://crrev.com/bda965bc263e873323e68a09628fddced620f810/login_manager/device_policy_service_unittest.cc
,
Apr 13 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/06de8e2145bd47ff76ae5b047348101a58c62d96 commit 06de8e2145bd47ff76ae5b047348101a58c62d96 Author: Charlie Reis <creis@chromium.org> Date: Fri Apr 13 21:31:36 2018 Fix M66 compile error in site_isolation_policy_browsertest.cc. The merge in https://chromium-review.googlesource.com/c/chromium/src/+/1012137 caused a compile error on the M66 branch due to kSitePerProcess moving from content/public to chrome/ in r545649 (67.0.3381.0). BUG= 832898 , 827821 TBR=lukasza, pastarmovj Change-Id: I1e339cac100d9fcec24015e0ba8364da9e1b7140 Reviewed-on: https://chromium-review.googlesource.com/1012572 Reviewed-by: Charlie Reis <creis@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#707} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/06de8e2145bd47ff76ae5b047348101a58c62d96/chrome/browser/policy/site_isolation_policy_browsertest.cc
,
Apr 17 2018
Is there any merge needed to M67? Pls note M67 (branch 3396)was branched at chromium revision 550428.
,
Apr 18 2018
I think no additional merges are needed, since the Chrome fix landed in r550124, before branch. (Comment 7 was mistaken.) pmarko@, can you confirm that your https://chromium-review.googlesource.com/1005155 ChromeOS CL is in the M67 branch? We merged it to M66 in comment 20, but I'm not sure how to check that it landed in M67 rather than M68.
,
Apr 18 2018
Confirming that all is fine - CL 1005155 is included in ChromeOS M67.
,
Apr 19 2018
Verified with flag turned on/off in M67.0.3396.8 10575.6.0 dev kefka.
,
Apr 19 2018
Reopened to be verified by platforms other than ChromeOS.
,
Apr 26 2018
pastarmovj@ Could you please help us with repro steps to verify the fix from TE end Thank You...
,
Apr 26 2018
pastormovj@, can you please help us to verify this fix on today's stable RC# 66.0.3359.139 ? Binary location is here: https://goto.google.com/zpyxpb Thank you!
,
Apr 26 2018
pastormovj@ confirmed the fix is working as intended on latest stable RC#66.0.3359.139. Thank you so much for the quick help!
,
May 1 2018
Marked as "Verified" based on c#25, c#29. |
|||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||
Comment 1 by creis@chromium.org
, Mar 31 2018