[Security Settings] Implement ABH settings bundle backend [chromium/src : main]
Awad Osman (Gerrit)
Awad Osman added 1 comment![]( "Open in Gerrit")
safe_browsing::kBundledSecuritySettingsAskBeforeHttp);Should this be added here or in a separate function? Balanced mode can exist without BundledSecuritySettings (BSS) right?
I'm wondering because this function is used elsewhere so if the BSS flag is true and the BalancedMode flag is false we would end up affecting those functions.
Related details
- Zack Han
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zack Han (Gerrit)
Zack Han added 2 comments![]( "Open in Gerrit")
Thanks Awad, I decouple the logic, and added some new stuff to address the design doc change. Can you PTAL again?
Should this be added here or in a separate function? Balanced mode can exist without BundledSecuritySettings (BSS) right?
I'm wondering because this function is used elsewhere so if the BSS flag is true and the BalancedMode flag is false we would end up affecting those functions.
- Awad Osman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Awad Osman (Gerrit)
Awad Osman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Awad Osman
| Code-Review | +1 |
2 comments
#include "components/safe_browsing/core/common/features.h"Is this import used anywhere?
Related details
- Zack Han
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Zack Han (Gerrit)
Zack Han added 1 comment![]( "Open in Gerrit")
Hi jacastro@ can you please take a look at the chrome/browser/safe_browsing/metrics/... related changes? Thanks
Related details
- Chris Thompson
- Javier Castro
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson added 3 comments![]( "Open in Gerrit")
Change-Id: I9d898d478c4e141050587faa9ea84e9b6a1c5b8eAdd bug link here
```suggestion
Bug: 515782410
Change-Id: I9d898d478c4e141050587faa9ea84e9b6a1c5b8e
```
void GeneratedHttpsFirstModePref::OnSettingsBundleChanged() {
if (!base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp)) {
return;
}
auto bundle = safe_browsing::GetSecurityBundleSetting(*profile_->GetPrefs());
switch (bundle) {
case safe_browsing::SecuritySettingsBundleSetting::STANDARD:
SetPref(std::make_unique<base::Value>(
static_cast<int>(HttpsFirstModeSetting::kDisabled))
.get());
break;
case safe_browsing::SecuritySettingsBundleSetting::ENHANCED:
SetPref(std::make_unique<base::Value>(
static_cast<int>(IsBalancedModeAvailable()
? HttpsFirstModeSetting::kEnabledBalanced
: HttpsFirstModeSetting::kDisabled))
.get());
break;
}
}If this can trigger for cases other than "user clicked on the bundle in settings", then this might be setting the pref too aggressively. I'm checking on this now.
// If the Toast has already been shown or HFM features are not enabled, abort.
if (prefs->GetBoolean(prefs::kHttpsFirstModeBundleToastShown) ||
!base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) ||
!IsBalancedModeAvailable()) {
return;
}
One case where we want to make sure we don't set the underlying prefs: if the user is in the Enhanced bundle because of Advanced Protection Program. (This has caused issues where Android Device Protection, which should be device scoped, escaped containment and caused the ABH pref to get synced across profiles.)
Related details
- Javier Castro
- Zack Han
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Thompson (Gerrit)
Chris Thompson added 2 comments![]( "Open in Gerrit")
void GeneratedHttpsFirstModePref::OnSettingsBundleChanged() {
if (!base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp)) {
return;
}
auto bundle = safe_browsing::GetSecurityBundleSetting(*profile_->GetPrefs());
switch (bundle) {
case safe_browsing::SecuritySettingsBundleSetting::STANDARD:
SetPref(std::make_unique<base::Value>(
static_cast<int>(HttpsFirstModeSetting::kDisabled))
.get());
break;
case safe_browsing::SecuritySettingsBundleSetting::ENHANCED:
SetPref(std::make_unique<base::Value>(
static_cast<int>(IsBalancedModeAvailable()
? HttpsFirstModeSetting::kEnabledBalanced
: HttpsFirstModeSetting::kDisabled))
.get());
break;
}
}If this can trigger for cases other than "user clicked on the bundle in settings", then this might be setting the pref too aggressively. I'm checking on this now.
This calls SetPref() which early returns if the user is in APP, so I think this will be okay.
// If the Toast has already been shown or HFM features are not enabled, abort.
if (prefs->GetBoolean(prefs::kHttpsFirstModeBundleToastShown) ||
!base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) ||
!IsBalancedModeAvailable()) {
return;
}
One case where we want to make sure we don't set the underlying prefs: if the user is in the Enhanced bundle because of Advanced Protection Program. (This has caused issues where Android Device Protection, which should be device scoped, escaped containment and caused the ABH pref to get synced across profiles.)
Checking with ackermanb@ on this, it sounds like APP users would have the enhanced bundle set by default, and that this migration path would include users who had previously chosen ESB in settings. In cases where the user _didn't_ explicitly choose the bundle in settings, can we early return before setting the underlying prefs?
(For comparison in generated_https_first_mode_pref.cc: in SetPref() if the user is APP mode we treat it as "force enabled" and don't actually route through to setting the underlying pref values. I think similarly skipping this for APP users here would be sufficient.)
Javier Castro (Gerrit)
Javier Castro added 4 comments![]( "Open in Gerrit")
!base::FeatureList::IsEnabled(
safe_browsing::kBundledSecuritySettingsAskBeforeHttp) ||I think you'd want this check to be last in this OR clause so that a user is only marked as active in the study if they can actually enable balanced mode.
prefs->SetBoolean(prefs::kHttpsFirstModeBundleToastShown, true);Wouldn't we only want to record that the toast was shown if it was shown? Otherwise, it seems that this preference is being overloaded into tracking the state of whether the migration is complete (so maybe it needs a different name).
prefs->SetBoolean(prefs::kHttpsFirstModeBundleToastShown, true);If we set this here, and the migration toast doesn't end up getting shown to the user, then this boolean is not representing that the toast was shown. Maybe we just need a better name for the preference, like HttpsFirstModeBundleToastQueued?
prefs->SetInteger(
prefs::kSecuritySettingsBundleMigrationToastState,
static_cast<int>(
safe_browsing::SecuritySettingsBundleToastState::kPending));Where does the ToastController's MaybeShowToast(ToastParams(ToastId::kEnhancedBundledSecuritySettings)) end up getting triggered?
For the overall ESB to enhanced bundle migration we trigger it over here:
I think if this code doesn't ask the toast controller to run, then the toast may get shown on the next run of Chrome. Ideally the user would see the toast on this current run of Chrome though (since this would be when we update the preference value).
Related details
- Zack Han
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |