Commit 41f21f50 authored by Ryan Hansberry's avatar Ryan Hansberry Committed by Commit Bot

Revert "[CrOS MultiDevice] Update password requirements for Connected Devices."

This reverts commit 28e953d7.

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/1208954Reviewed-by: default avatarRyan Hansberry <hansberry@chromium.org>
Commit-Queue: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589292}
parent e899dc99
...@@ -42,18 +42,12 @@ Polymer({ ...@@ -42,18 +42,12 @@ Polymer({
}, },
/** /**
* Features which the user has requested to be enabled but could not be * The feature which the user wishes to enable, but which requires
* enabled immediately because they require that the user enter a password * authentication. Once |authToken_| is valid, this value will be passed
* first. This array is only set when the password prompt is visible; if the * along to |browserProxy_|, and subsequently cleared.
* user enters a correct password, the features are enabled, and if the user * @private {?settings.MultiDeviceFeature}
* closes the prompt without entering a correct password, these pending
* requests are dropped.
* @private {!Array<!settings.MultiDeviceFeature>}
*/ */
featuresToBeEnabledOnceAuthenticated_: { attemptedEnabledFeature_: {type: Number, value: null},
type: Array,
value: [],
},
/** @private {boolean} */ /** @private {boolean} */
showPasswordPromptDialog_: { showPasswordPromptDialog_: {
...@@ -202,6 +196,18 @@ Polymer({ ...@@ -202,6 +196,18 @@ Polymer({
} }
}, },
/**
* Tell |this.browserProxy_| to enable |this.attemptedEnabledFeature_|.
* @private
*/
enableAttemptedFeature_: function() {
if (this.attemptedEnabledFeature_ != null) {
this.browserProxy_.setFeatureEnabledState(
this.attemptedEnabledFeature_, true /* enabled */, this.authToken_);
this.attemptedEnabledFeature_ = null;
}
},
/** @private */ /** @private */
openPasswordPromptDialog_: function() { openPasswordPromptDialog_: function() {
this.showPasswordPromptDialog_ = true; this.showPasswordPromptDialog_ = true;
...@@ -209,22 +215,6 @@ Polymer({ ...@@ -209,22 +215,6 @@ Polymer({
/** @private */ /** @private */
onPasswordPromptDialogClose_: function() { onPasswordPromptDialogClose_: function() {
// If |this.authToken_| is set when the dialog has been closed, this means
// that the user entered the correct password into the dialog. Thus, send
// all pending features to be enabled.
if (this.authToken_) {
this.featuresToBeEnabledOnceAuthenticated_.forEach((feature) => {
this.browserProxy_.setFeatureEnabledState(
feature, true /* enabled */, this.authToken_);
});
}
// If the features were enabled above, they no longer need to be tracked.
// Likewise, if the features were not enabled above, the user did not enter
// the correct password in the dialog, so the features should not actually
// be enabled.
this.featuresToBeEnabledOnceAuthenticated_ = [];
this.showPasswordPromptDialog_ = false; this.showPasswordPromptDialog_ = false;
}, },
...@@ -240,50 +230,33 @@ Polymer({ ...@@ -240,50 +230,33 @@ Polymer({
let feature = event.detail.feature; let feature = event.detail.feature;
let enabled = event.detail.enabled; let enabled = event.detail.enabled;
if (!this.isAuthenticationRequiredForChange_(feature, enabled)) { // Authentication is never required if disabling a feature; only consider it
this.browserProxy_.setFeatureEnabledState( // if enabling.
feature, enabled, this.authToken_); if (enabled) {
return; // TODO(crbug.com/876436): Actually determine this, when the API is
// available. It's fine to default to true here for now, because it errs
// on the side of more security.
let isSmartLockPrefEnabled = true;
let isPasswordPromptRequired =
(feature === settings.MultiDeviceFeature.SMART_LOCK) ||
(feature === settings.MultiDeviceFeature.BETTER_TOGETHER_SUITE &&
isSmartLockPrefEnabled);
if (isPasswordPromptRequired) {
this.attemptedEnabledFeature_ = feature;
if (this.authToken_) {
this.enableAttemptedFeature_();
} else {
this.openPasswordPromptDialog_();
}
return;
}
} }
this.featuresToBeEnabledOnceAuthenticated_.push(feature); // No authentication is required.
this.openPasswordPromptDialog_(); this.browserProxy_.setFeatureEnabledState(feature, enabled);
},
/**
* @param {!settings.MultiDeviceFeature} feature The feature to change.
* @param {boolean} enabled True if the user has requested to enable the
* feature; false if the user has requested to disable the feature.
* @return {boolean} Whether authentication is required to change the
* feature's state.
* @private
*/
isAuthenticationRequiredForChange_: function(feature, enabled) {
// Disabling a feature never requires authentication.
if (!enabled)
return false;
// Enabling SmartLock always requires authentication.
if (feature == settings.MultiDeviceFeature.SMART_LOCK)
return true;
// Enabling any feature besides SmartLock and the Better Together suite does
// not require authentication.
if (feature != settings.MultiDeviceFeature.BETTER_TOGETHER_SUITE)
return false;
let smartLockState =
this.getFeatureState(settings.MultiDeviceFeature.SMART_LOCK);
// If the user is enabling the Better Together suite and this change would
// result in SmartLock being implicitly enabled, authentication is required.
// SmartLock is implicitly enabled if it is only currently not enabled due
// to the suite being disabled or due to the SmartLock host device not
// having a lock screen set.
return smartLockState ==
settings.MultiDeviceFeatureState.UNAVAILABLE_SUITE_DISABLED ||
smartLockState ==
settings.MultiDeviceFeatureState.UNAVAILABLE_INSUFFICIENT_SECURITY;
}, },
/** @private */ /** @private */
...@@ -291,4 +264,19 @@ Polymer({ ...@@ -291,4 +264,19 @@ Polymer({
this.browserProxy_.removeHostDevice(); this.browserProxy_.removeHostDevice();
settings.navigateTo(settings.routes.MULTIDEVICE); settings.navigateTo(settings.routes.MULTIDEVICE);
}, },
/**
* Called when the authToken_ changes. If the authToken is valid, that
* indicates the user authenticated successfully. If not, cancel the pending
* attempt to enable attemptedEnabledFeature_.
* @param {String} authToken
* @private
*/
authTokenChanged_: function(authToken) {
if (this.authToken_) {
this.enableAttemptedFeature_();
} else {
this.attemptedEnabledFeature_ = null;
}
},
}); });
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment