Tweak auth requirements for BetterTogetherSuite |
|||||||||
Issue descriptionOnly require authentication for enabling Better Together Suite if the Smart Lock user pref is already enabled. Right now, authentication is always required.
,
Aug 22
As per Kyle's comment in [1], also revisit changing how features are stored as a member variable of multidevice_page before enabling it. 1) https://chromium-review.googlesource.com/c/chromium/src/+/1184025/5/chrome/browser/resources/settings/multidevice_page/multidevice_page.js#51
,
Aug 22
Can't implement this until there's a DISBALED_BY_BETTER_TOGETHER_SUITE in [1]. Tracked in crbug.com/870113 . I'll loop back once unblocked. 1) https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/multidevice_page/multidevice_constants.js?q=multidevice_constants&sq=package:chromium&dr&l=47
,
Aug 31
,
Aug 31
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/28e953d7d5bd94c1ab319b185eba612f7456df90 commit 28e953d7d5bd94c1ab319b185eba612f7456df90 Author: Kyle Horimoto <khorimoto@google.com> Date: Fri Aug 31 20:48:59 2018 [CrOS MultiDevice] Update password requirements for Connected Devices. It should be required that users enter their password to enable SmartLock, since it is a security-sensitive feature. As a corollary, it should also be required that users enter their password to enable the Better Together suite if doing so would implicitly cause SmartLock to become enabled. Previously, entering a password was required for enabling the Better Together suite under any circumstances, but this was unnecessary, since no password should be required if SmartLock were turned off. This CL fixes this issue; additionally, it updates the authentication code here, which was previously not as robust. Bug: 876436 , 824568 Change-Id: Ia61dd112675485972361a51b406482a0bb0a1f05 Reviewed-on: https://chromium-review.googlesource.com/1200148 Reviewed-by: Tommy Li <tommycli@chromium.org> Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#588158} [modify] https://crrev.com/28e953d7d5bd94c1ab319b185eba612f7456df90/chrome/browser/resources/settings/multidevice_page/multidevice_page.js
,
Aug 31
,
Sep 1
Your change meets the bar and is auto-approved for M70. Please go ahead and merge the CL to branch 3538 manually. Please contact milestone owner if you have questions. Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 5
,
Sep 6
The change that Kyle landed is not complete. It should have also fixed this this TODO in multidevice_setup_impl [1]. If Kyle's change as-is lands in 70, users will not be prompted for a password in some cases where the backend multidevice_setup_impl does expect a password, resulting in a broken state where toggles do not enable when the user expects that they should. Do not merge into 70 until that fix has been added. 1) https://cs.chromium.org/chromium/src/chromeos/services/multidevice_setup/multidevice_setup_impl.cc?q=multidevicesetupimpl&sq=package:chromium&dr=CSs&l=193
,
Sep 6
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41f21f5031da1e7cdbdc0440f103e2006e20f669 commit 41f21f5031da1e7cdbdc0440f103e2006e20f669 Author: Ryan Hansberry <hansberry@chromium.org> Date: Thu Sep 06 20:53:38 2018 Revert "[CrOS MultiDevice] Update password requirements for Connected Devices." This reverts commit 28e953d7d5bd94c1ab319b185eba612f7456df90. Reason for revert: This CL introduces 2 bugs: 1. The new |featuresToBeEnabledOnceAuthenticated_| queue has features pushed to it regardless of whether the user wants to enable or disable the feature. This means the features intended to be disabled are actually enabled when the dialog is closed. 2. The required corresponding backend change required in multidevice_setup_impl.cc is not implemented, meaning the frontend does not prompt the user for a password in situations where the backend expects it. This creates a jarring breakage for the user. The issue that this is attempting to address should be fixed in M70; instead of merging this broken CL into 70 and then merging a followup, it will be cleaner to revert this CL and then merge a single correct CL into 70. Original change's description: > [CrOS MultiDevice] Update password requirements for Connected Devices. > > It should be required that users enter their password to enable > SmartLock, since it is a security-sensitive feature. As a corollary, it > should also be required that users enter their password to enable the > Better Together suite if doing so would implicitly cause SmartLock to > become enabled. > > Previously, entering a password was required for enabling the Better > Together suite under any circumstances, but this was unnecessary, since > no password should be required if SmartLock were turned off. > > This CL fixes this issue; additionally, it updates the authentication > code here, which was previously not as robust. > > Bug: 876436 , 824568 > Change-Id: Ia61dd112675485972361a51b406482a0bb0a1f05 > Reviewed-on: https://chromium-review.googlesource.com/1200148 > Reviewed-by: Tommy Li <tommycli@chromium.org> > Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> > Cr-Commit-Position: refs/heads/master@{#588158} TBR=khorimoto@chromium.org,tommycli@chromium.org # Not skipping CQ checks because original CL landed > 1 day ago. Bug: 876436 , 824568 Change-Id: Iec397181ac06c31fd911569c9bb4c22eec4a787a Reviewed-on: https://chromium-review.googlesource.com/1208954 Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Commit-Queue: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/heads/master@{#589292} [modify] https://crrev.com/41f21f5031da1e7cdbdc0440f103e2006e20f669/chrome/browser/resources/settings/multidevice_page/multidevice_page.js
,
Sep 10
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 10
,
Sep 11
,
Sep 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/988990dafd8b5d7a91b86c6aa6204791ecb1db29 commit 988990dafd8b5d7a91b86c6aa6204791ecb1db29 Author: Kyle Horimoto <khorimoto@google.com> Date: Wed Sep 12 23:21:52 2018 [CrOS MultiDevice] Update password requirements for Connected Devices. This CL originally landed as: https://chromium-review.googlesource.com/c/chromium/src/+/1200148 It was reverted as: https://chromium-review.googlesource.com/c/chromium/src/+/1208954 The original change has been uploaded as patchset 1 and additional changes are uploaded as additional patchsets. The original CL had two issues: (1) It did not update the C++ auth token checks, so it would reject some requests that should not have been rejected. (2) It did not clear |this.authToken_| in multidevice_page.js, which made it possible to send multiple authenticated requests without re-authenticating each time. Original description below: It should be required that users enter their password to enable SmartLock, since it is a security-sensitive feature. As a corollary, it should also be required that users enter their password to enable the Better Together suite if doing so would implicitly cause SmartLock to become enabled. Previously, entering a password was required for enabling the Better Together suite under any circumstances, but this was unnecessary, since no password should be required if SmartLock were turned off. This CL fixes this issue; additionally, it updates the authentication code here, which was previously not as robust. Bug: 876436 Change-Id: I3548d835c9c3da4bbdc3694387c72df480b6556a Reviewed-on: https://chromium-review.googlesource.com/1217325 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Cr-Commit-Position: refs/heads/master@{#590854} [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chrome/test/data/webui/settings/multidevice_page_tests.js [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chromeos/services/multidevice_setup/multidevice_setup_impl.cc [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chromeos/services/multidevice_setup/multidevice_setup_impl.h [modify] https://crrev.com/988990dafd8b5d7a91b86c6aa6204791ecb1db29/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc
,
Sep 13
,
Sep 13
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262 commit dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262 Author: Kyle Horimoto <khorimoto@google.com> Date: Thu Sep 13 00:05:06 2018 [CrOS MultiDevice] Update password requirements for Connected Devices. This CL originally landed as: https://chromium-review.googlesource.com/c/chromium/src/+/1200148 It was reverted as: https://chromium-review.googlesource.com/c/chromium/src/+/1208954 The original change has been uploaded as patchset 1 and additional changes are uploaded as additional patchsets. The original CL had two issues: (1) It did not update the C++ auth token checks, so it would reject some requests that should not have been rejected. (2) It did not clear |this.authToken_| in multidevice_page.js, which made it possible to send multiple authenticated requests without re-authenticating each time. Original description below: It should be required that users enter their password to enable SmartLock, since it is a security-sensitive feature. As a corollary, it should also be required that users enter their password to enable the Better Together suite if doing so would implicitly cause SmartLock to become enabled. Previously, entering a password was required for enabling the Better Together suite under any circumstances, but this was unnecessary, since no password should be required if SmartLock were turned off. This CL fixes this issue; additionally, it updates the authentication code here, which was previously not as robust. TBR=khorimoto@google.com (cherry picked from commit 988990dafd8b5d7a91b86c6aa6204791ecb1db29) Bug: 876436 Change-Id: I3548d835c9c3da4bbdc3694387c72df480b6556a Reviewed-on: https://chromium-review.googlesource.com/1217325 Commit-Queue: Kyle Horimoto <khorimoto@chromium.org> Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org> Reviewed-by: Ryan Hansberry <hansberry@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#590854} Reviewed-on: https://chromium-review.googlesource.com/1222988 Reviewed-by: Kyle Horimoto <khorimoto@chromium.org> Cr-Commit-Position: refs/branch-heads/3538@{#354} Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811} [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/browser/chromeos/tether/tether_service.cc [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/browser/chromeos/tether/tether_service.h [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/browser/chromeos/tether/tether_service_unittest.cc [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/browser/resources/settings/multidevice_page/multidevice_page.html [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/browser/resources/settings/multidevice_page/multidevice_page.js [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chrome/test/data/webui/settings/multidevice_page_tests.js [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chromeos/services/multidevice_setup/multidevice_setup_impl.cc [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chromeos/services/multidevice_setup/multidevice_setup_impl.h [modify] https://crrev.com/dfdbc5f13ff5a5462146f1748d2cc4eee1dc4262/chromeos/services/multidevice_setup/multidevice_setup_impl_unittest.cc |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by khorimoto@chromium.org
, Aug 21