New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 876436 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 13
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocked on:
issue 870113


Participants' hotlists:
Better-Together-Launch-Blockers


Sign in to add a comment

Tweak auth requirements for BetterTogetherSuite

Project Member Reported by hansberry@chromium.org, Aug 21

Issue description

Only require authentication for enabling Better Together Suite if the Smart Lock user pref is already enabled. Right now, authentication is always required.
 
Labels: M-70
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
Blockedon: 870113
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
Owner: khorimoto@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Labels: Merge-Request-70
Project Member

Comment 7 by sheriffbot@chromium.org, Sep 1

Labels: -Merge-Request-70 Hotlist-Merge-Approved Merge-Approved-70
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
Project Member

Comment 8 by sheriffbot@chromium.org, 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
Owner: hansberry@chromium.org
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
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Project Member

Comment 12 by sheriffbot@chromium.org, 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
Owner: khorimoto@chromium.org
Cc: hansberry@chromium.org nohle@chromium.org
 Issue 881946  has been merged into this issue.
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Status: Fixed (was: Started)
Project Member

Comment 17 by bugdroid1@chromium.org, Sep 13

Labels: -merge-approved-70 merge-merged-3538
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