Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2920253003:
kiosk: Tighten cert manager UI (Closed)
|
Created:
3 years, 6 months ago by xiyuan Modified:
3 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, alemate+watch_chromium.org, achuith+watch_chromium.org, michaelpg+watch-options_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionkiosk: Tighten cert manager UI
- Make prompt_for_network_when_offline default to false;
- Hide "Servers" and "Authorities" tab in kiosk cert manager;
BUG=719907
TEST=CertificateManagerStandaloneWebUITest.testCertsDisplaying
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2920253003
Cr-Commit-Position: refs/heads/master@{#479108}
Committed: https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20af6109a23c79
Patch Set 1 #
Total comments: 6
Patch Set 2 : for #1 #Patch Set 3 : rebase, base test is removed :( #
Messages
Total messages: 30 (18 generated)
Description was changed from ========== kiosk: Tighten cert manager UI - Make prompt_for_network_when_offline default to false; - Hide "Servers" and "Authorities" tab in kiosk cert manager; BUG=719907 TEST=CertificateManagerStandaloneWebUITest.testCertsDisplaying ========== to ========== kiosk: Tighten cert manager UI - Make prompt_for_network_when_offline default to false; - Hide "Servers" and "Authorities" tab in kiosk cert manager; BUG=719907 TEST=CertificateManagerStandaloneWebUITest.testCertsDisplaying CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by xiyuan@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
xiyuan@chromium.org changed reviewers: + tbarzic@chromium.org
https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/app_launch_controller.cc (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/app_launch_controller.cc:364: // Default to false to allow network configuration if the policy is missing. nit: can you update the comment :) E.g. Network configuration has to be explicitly allowed by the policy. https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:379: // does not have access to the Internet. If the policy is omitted or set to nit: update the comment (no "if the policy is omitted" part) https://codereview.chromium.org/2920253003/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/certificate_manager.js (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/certificate_manager.js:208: $('server-certs-nav-tab').hidden = true; I think you have to update serverCerts and caCerts here, too: https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif...
The CQ bit was checked by xiyuan@chromium.org to run a CQ dry run
https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/log... File chrome/browser/chromeos/login/app_launch_controller.cc (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/log... chrome/browser/chromeos/login/app_launch_controller.cc:364: // Default to false to allow network configuration if the policy is missing. On 2017/06/06 19:02:42, tbarzic wrote: > nit: can you update the comment :) > E.g. > Network configuration has to be explicitly allowed by the policy. Done. https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/pol... File chrome/browser/chromeos/policy/proto/chrome_device_policy.proto (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/chromeos/pol... chrome/browser/chromeos/policy/proto/chrome_device_policy.proto:379: // does not have access to the Internet. If the policy is omitted or set to On 2017/06/06 19:02:42, tbarzic wrote: > nit: update the comment (no "if the policy is omitted" part) Done. https://codereview.chromium.org/2920253003/diff/1/chrome/browser/resources/op... File chrome/browser/resources/options/certificate_manager.js (right): https://codereview.chromium.org/2920253003/diff/1/chrome/browser/resources/op... chrome/browser/resources/options/certificate_manager.js:208: $('server-certs-nav-tab').hidden = true; On 2017/06/06 19:02:42, tbarzic wrote: > I think you have to update serverCerts and caCerts here, too: > https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/certif... emm, kiosk cert manager dialog does not use the MD version yet. Filed http://crbug.com/731219 to track the work.
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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by xiyuan@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
xiyuan@chromium.org changed reviewers: + dbeam@chromium.org
Dan, can I get an owner stamp from you for certificate_manager.js ? Thanks.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by xiyuan@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497375828613580,
"parent_rev": "91b6a2193c69fb067404c809721603070e920d5a", "commit_rev":
"208cf98ca5b1b2bdfdb620b68d20af6109a23c79"}
Message was sent while issue was closed.
Description was changed from ========== kiosk: Tighten cert manager UI - Make prompt_for_network_when_offline default to false; - Hide "Servers" and "Authorities" tab in kiosk cert manager; BUG=719907 TEST=CertificateManagerStandaloneWebUITest.testCertsDisplaying CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== kiosk: Tighten cert manager UI - Make prompt_for_network_when_offline default to false; - Hide "Servers" and "Authorities" tab in kiosk cert manager; BUG=719907 TEST=CertificateManagerStandaloneWebUITest.testCertsDisplaying CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2920253003 Cr-Commit-Position: refs/heads/master@{#479108} Committed: https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20...
Message was sent while issue was closed.
On 2017/06/13 19:50:17, commit-bot: I haz the power wrote: > Committed patchset #3 (id:40001) as > https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20... Xiyuan, how do we enable prompt_for_network_when_offline when testing? I can't launch the cert manager from the kiosk app loading screen anymore. (I also don't have access to https://bugs.chromium.org/p/chromium/issues/detail?id=719907 to ask there.)
Message was sent while issue was closed.
Xiayun, can you please give me some background for this CL? On 2017/08/04 23:37:23, michaelpg wrote: > On 2017/06/13 19:50:17, commit-bot: I haz the power wrote: > > Committed patchset #3 (id:40001) as > > > https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20... > > Xiyuan, how do we enable prompt_for_network_when_offline when testing? I can't > launch the cert manager from the kiosk app loading screen anymore. > > (I also don't have access to > https://bugs.chromium.org/p/chromium/issues/detail?id=719907 to ask there.)
Message was sent while issue was closed.
On 2017/08/04 23:59:05, sduraisamy wrote: > Xiayun, can you please give me some background for this CL? > > On 2017/08/04 23:37:23, michaelpg wrote: > > On 2017/06/13 19:50:17, commit-bot: I haz the power wrote: > > > Committed patchset #3 (id:40001) as > > > > > > https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20... > > > > Xiyuan, how do we enable prompt_for_network_when_offline when testing? I can't > > launch the cert manager from the kiosk app loading screen anymore. > > > > (I also don't have access to > > https://bugs.chromium.org/p/chromium/issues/detail?id=719907 to ask there.) The change is for http://crbug.com/719907. I just realized it is an overkill. Instead of restricting on network config UI as whole, it should focus only at the cert manager part. I'll revert the policy part to make prompt_for_network_when_offline default on again.
Message was sent while issue was closed.
On 2017/08/07 19:56:01, xiyuan wrote: > On 2017/08/04 23:59:05, sduraisamy wrote: > > Xiayun, can you please give me some background for this CL? > > > > On 2017/08/04 23:37:23, michaelpg wrote: > > > On 2017/06/13 19:50:17, commit-bot: I haz the power wrote: > > > > Committed patchset #3 (id:40001) as > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/208cf98ca5b1b2bdfdb620b68d20... > > > > > > Xiyuan, how do we enable prompt_for_network_when_offline when testing? I > can't > > > launch the cert manager from the kiosk app loading screen anymore. > > > > > > (I also don't have access to > > > https://bugs.chromium.org/p/chromium/issues/detail?id=719907 to ask there.) > > The change is for http://crbug.com/719907. I just realized it is an overkill. > Instead of restricting on network config UI as whole, it should focus only at > the cert manager part. I'll revert the policy part to make > prompt_for_network_when_offline default on again. Thanks Xiyuan. Let us not make changes to Cert Manager part as well till we figure out the right approach (policy vs feature in CPanel) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
