[EnterprisePolicy] Migrate policies to AutofillSettings. [chromium/src : main]
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
Reviewer source(s):
nico...@chromium.org is from context(chrome/enterprise/gwsq/enterprise-policy-review.gwsq)
Related details
- Christoph Schwering
- Nicolas Ouellet-Payeur
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 7 comments![]( "Open in Gerrit")
// Deprecate legacy boolean policies AutofillAddressEnabled and
// AutofillCreditCardEnabled in favor of the unified AutofillSettings list
// policy (N-to-1 migration). If AutofillSettings is unset, legacy disabled
// booleans are synthesized into global wildcard block rules ('*') inside
// autofill::prefs::kAutofillTypesBlocked.Should this comment go near the LegacyAutofillSettingsMigrationPolicyHandler class definition?
legacy_autofill_migration;
legacy_autofill_migration.push_back(
std::make_unique<
autofill::LegacyAutofillSettingsMigrationPolicyHandler>());nit: golf
```cpp
vector<unique_ptr<...>> legacy_autofill_migration = {make_unique<...>()};
```
or inline
handlers->AddHandler(std::make_unique<LegacyPoliciesDeprecatingPolicyHandler>(To clarify, `AutofillAddressEnabled` and `AutofillCreditCardEnabled` will do *literally nothing* if `AutofillSettings` is configured. I think this is gonna be really confusing for admins
// Do NOT synthesize legacy list rules if the admin is already using the new
// policy.
if (policies.GetValue(policy::key::kAutofillSettings,
base::Value::Type::LIST)) {
return;
}I don't think this can ever happen? Because of how `LegacyPoliciesDeprecatingPolicyHandler` works
if (address_enabled && address_enabled->is_bool() &&
!address_enabled->GetBool()) {Code is repeated 2 times, this could be a helper function
```cpp
base::DictValue CreateMigratedRule(const char* blocked_type) {
return ...;
}
```
base::DictValue rule;
rule.Set("url_pattern", "*");
rule.Set("blocked_types", std::move(blocked_types));nit: golf
```cpp
base::DictValue rule = base::DictValue()
.Set("url_pattern", "*")
.Set("blocked_types", base::ListValue().Append("contact_info"));
```
and/or inline.
blocked_types.Append("payments");Would it make sense to create a single rule with 1-2 blocked_types, instead of 2 rules with 1 blocked_type each? I guess it doesn't really matter
Related details
- Christoph Schwering
- Luchen Peng
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. |
Luchen Peng (Gerrit)
Luchen Peng added 7 comments![]( "Open in Gerrit")
// Deprecate legacy boolean policies AutofillAddressEnabled and
// AutofillCreditCardEnabled in favor of the unified AutofillSettings list
// policy (N-to-1 migration). If AutofillSettings is unset, legacy disabled
// booleans are synthesized into global wildcard block rules ('*') inside
// autofill::prefs::kAutofillTypesBlocked.Should this comment go near the LegacyAutofillSettingsMigrationPolicyHandler class definition?
Sure, Moved this to autofill_policy_handler.h
legacy_autofill_migration;
legacy_autofill_migration.push_back(
std::make_unique<
autofill::LegacyAutofillSettingsMigrationPolicyHandler>());nit: golf
```cpp
vector<unique_ptr<...>> legacy_autofill_migration = {make_unique<...>()};
```or inline
I tried `vector<unique_ptr<...>> legacy_autofill_migration = {make_unique<...>()};` but it gives a compile error. Because std::unique_ptr is move-only and std::initializer_list only provides const access (forcing a deleted copy-constructor call), { make_unique<...>() } doesn't compile.
```
../../third_party/libc++/src/include/__vector/vector.h:273:5: note: in instantiation of function template specialization 'std::vector<std::unique_ptr<policy::ConfigurationPolicyHandler>>::__init_with_size<const std::unique_ptr<policy::ConfigurationPolicyHandler> *, const std::unique_ptr<policy::ConfigurationPolicyHandler> *>' requested here
273 | __init_with_size(__il.begin(), __il.end(), __il.size());
| ^
../../chrome/browser/policy/configuration_policy_handler_list_factory.cc:2638:35: note: in instantiation of member function 'std::vector<std::unique_ptr<policy::ConfigurationPolicyHandler>>::vector' requested here
2638 | legacy_autofill_migration = {
| ^
../../third_party/libc++/src/include/__memory/construct_at.h:43:58: note: candidate template ignored: substitution failure [with _Tp = std::unique_ptr<policy::ConfigurationPolicyHandler>, _Args = <const std::unique_ptr<policy::ConfigurationPolicyHandler> &>]: call to implicitly-deleted copy constructor of 'std::unique_ptr<policy::ConfigurationPolicyHandler>'
42 | template <class _Tp, class... _Args, class = decltype(::new(std::declval<void*>()) _Tp(std::declval<_Args>()...))>
| ~~~
43 | _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp* __construct_at(_Tp* __location, _Args&&... __args) {
| ^
1 error generated.
```
handlers->AddHandler(std::make_unique<LegacyPoliciesDeprecatingPolicyHandler>(To clarify, `AutofillAddressEnabled` and `AutofillCreditCardEnabled` will do *literally nothing* if `AutofillSettings` is configured. I think this is gonna be really confusing for admins
The goal is to replace `AutofillAddressEnabled` and `AutofillCreditCardEnabled` with the new `AutofillSettings` policy. I'm not sure what is the usual convention for enterprise policy for migration?
I can also keep the behavior for `AutofillAddressEnabled` and `AutofillCreditCardEnabled` in addition to `AutofillSettings`, but then admins will have three settings to manage and keep in sync.
// Do NOT synthesize legacy list rules if the admin is already using the new
// policy.
if (policies.GetValue(policy::key::kAutofillSettings,
base::Value::Type::LIST)) {
return;
}I don't think this can ever happen? Because of how `LegacyPoliciesDeprecatingPolicyHandler` works
You are right, this shouldn't happen, removed it.
if (address_enabled && address_enabled->is_bool() &&
!address_enabled->GetBool()) {Code is repeated 2 times, this could be a helper function
```cpp
base::DictValue CreateMigratedRule(const char* blocked_type) {
return ...;
}
```
Created helper method IsPolicySetToFalse() to check for both address and credit card policies.
base::DictValue rule;
rule.Set("url_pattern", "*");
rule.Set("blocked_types", std::move(blocked_types));nit: golf
```cpp
base::DictValue rule = base::DictValue()
.Set("url_pattern", "*")
.Set("blocked_types", base::ListValue().Append("contact_info"));
```and/or inline.
Done
Would it make sense to create a single rule with 1-2 blocked_types, instead of 2 rules with 1 blocked_type each? I guess it doesn't really matter
Makes sense to have it with 1 rule and 1-2 blocked_types. Update the logic that way.
Related details
- Christoph Schwering
- Nicolas Ouellet-Payeur
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. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur added 2 comments![]( "Open in Gerrit")
legacy_autofill_migration;
legacy_autofill_migration.push_back(
std::make_unique<
autofill::LegacyAutofillSettingsMigrationPolicyHandler>());Luchen Pengnit: golf
```cpp
vector<unique_ptr<...>> legacy_autofill_migration = {make_unique<...>()};
```or inline
I tried `vector<unique_ptr<...>> legacy_autofill_migration = {make_unique<...>()};` but it gives a compile error. Because std::unique_ptr is move-only and std::initializer_list only provides const access (forcing a deleted copy-constructor call), { make_unique<...>() } doesn't compile.
```
../../third_party/libc++/src/include/__vector/vector.h:273:5: note: in instantiation of function template specialization 'std::vector<std::unique_ptr<policy::ConfigurationPolicyHandler>>::__init_with_size<const std::unique_ptr<policy::ConfigurationPolicyHandler> *, const std::unique_ptr<policy::ConfigurationPolicyHandler> *>' requested here
273 | __init_with_size(__il.begin(), __il.end(), __il.size());
| ^
../../chrome/browser/policy/configuration_policy_handler_list_factory.cc:2638:35: note: in instantiation of member function 'std::vector<std::unique_ptr<policy::ConfigurationPolicyHandler>>::vector' requested here
2638 | legacy_autofill_migration = {
| ^
../../third_party/libc++/src/include/__memory/construct_at.h:43:58: note: candidate template ignored: substitution failure [with _Tp = std::unique_ptr<policy::ConfigurationPolicyHandler>, _Args = <const std::unique_ptr<policy::ConfigurationPolicyHandler> &>]: call to implicitly-deleted copy constructor of 'std::unique_ptr<policy::ConfigurationPolicyHandler>'
42 | template <class _Tp, class... _Args, class = decltype(::new(std::declval<void*>()) _Tp(std::declval<_Args>()...))>
| ~~~
43 | _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 _Tp* __construct_at(_Tp* __location, _Args&&... __args) {
| ^
1 error generated.```
Gotcha. I hate C++ sometimes
handlers->AddHandler(std::make_unique<LegacyPoliciesDeprecatingPolicyHandler>(Luchen PengTo clarify, `AutofillAddressEnabled` and `AutofillCreditCardEnabled` will do *literally nothing* if `AutofillSettings` is configured. I think this is gonna be really confusing for admins
The goal is to replace `AutofillAddressEnabled` and `AutofillCreditCardEnabled` with the new `AutofillSettings` policy. I'm not sure what is the usual convention for enterprise policy for migration?
I can also keep the behavior for `AutofillAddressEnabled` and `AutofillCreditCardEnabled` in addition to `AutofillSettings`, but then admins will have three settings to manage and keep in sync.
The common solution would be to deprecate Autofill{Address,Card}Enabled but keep them working (your CL does this). More importantly, if both {Address,Card}Enabled *and* AutofillSettings are both set, they could "merge" in a way that makes sense (your CL doesn't do this right now).
For example, look at ExtensionInstall{Force,Allow,Block}list vs. ExtensionSettings. If you set them, the force/allow/blocklist effectively gets merged into the ExtensionSettings dict. This is the more common pattern, and it's less confusing for admins.
I think(?) you could do something similar here: let the admin define rules in AutofillSettings, and if they set Autofill{Address,Card}Enabled we *also* add a wildcard rule in `kAutofillTypesBlocked`. Or maybe that doesn't make sense for this policy?
(for context: `LegacyPoliciesDeprecatingPolicyHandler` is useful when the policies can't be combined, so it doesn't make sense to set both. e.g. SigninEnabled (bool) vs. BrowserSignin (enum: disabled, enabled, forced) we can't really reconcile conflicting values. So when you have `SigninEnabled=false` and `BrowserSignin=enabled`, BrowserSignin always "wins" the conflict)
Related details
- Christoph Schwering
- Luchen Peng
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. |
Luchen Peng (Gerrit)
Luchen Peng added 1 comment![]( "Open in Gerrit")
handlers->AddHandler(std::make_unique<LegacyPoliciesDeprecatingPolicyHandler>(Luchen PengTo clarify, `AutofillAddressEnabled` and `AutofillCreditCardEnabled` will do *literally nothing* if `AutofillSettings` is configured. I think this is gonna be really confusing for admins
Nicolas Ouellet-PayeurThe goal is to replace `AutofillAddressEnabled` and `AutofillCreditCardEnabled` with the new `AutofillSettings` policy. I'm not sure what is the usual convention for enterprise policy for migration?
I can also keep the behavior for `AutofillAddressEnabled` and `AutofillCreditCardEnabled` in addition to `AutofillSettings`, but then admins will have three settings to manage and keep in sync.
The common solution would be to deprecate Autofill{Address,Card}Enabled but keep them working (your CL does this). More importantly, if both {Address,Card}Enabled *and* AutofillSettings are both set, they could "merge" in a way that makes sense (your CL doesn't do this right now).
For example, look at ExtensionInstall{Force,Allow,Block}list vs. ExtensionSettings. If you set them, the force/allow/blocklist effectively gets merged into the ExtensionSettings dict. This is the more common pattern, and it's less confusing for admins.
I think(?) you could do something similar here: let the admin define rules in AutofillSettings, and if they set Autofill{Address,Card}Enabled we *also* add a wildcard rule in `kAutofillTypesBlocked`. Or maybe that doesn't make sense for this policy?
(for context: `LegacyPoliciesDeprecatingPolicyHandler` is useful when the policies can't be combined, so it doesn't make sense to set both. e.g. SigninEnabled (bool) vs. BrowserSignin (enum: disabled, enabled, forced) we can't really reconcile conflicting values. So when you have `SigninEnabled=false` and `BrowserSignin=enabled`, BrowserSignin always "wins" the conflict)
I see, in this case I think merging them makes more sense. When this policy is checked on the client, we match all the rules, so if there are conflict rules, e.g. AutofillAddressEnabled is set to false and in AutofillSettings address is disabled in a specific domain, we would respect the overall address disabled policy. I think this is fine.
I removed the usage of `LegacyPoliciesDeprecatingPolicyHandler`, I think that is not needed in this case.
Related details
- Christoph Schwering
- Nicolas Ouellet-Payeur
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. |
Nicolas Ouellet-Payeur (Gerrit)
Nicolas Ouellet-Payeur voted and added 4 comments![]( "Open in Gerrit")
Votes added by Nicolas Ouellet-Payeur
| Code-Review | +1 |
4 comments
handlers->AddHandler(std::make_unique<LegacyPoliciesDeprecatingPolicyHandler>(Luchen PengTo clarify, `AutofillAddressEnabled` and `AutofillCreditCardEnabled` will do *literally nothing* if `AutofillSettings` is configured. I think this is gonna be really confusing for admins
Nicolas Ouellet-PayeurThe goal is to replace `AutofillAddressEnabled` and `AutofillCreditCardEnabled` with the new `AutofillSettings` policy. I'm not sure what is the usual convention for enterprise policy for migration?
I can also keep the behavior for `AutofillAddressEnabled` and `AutofillCreditCardEnabled` in addition to `AutofillSettings`, but then admins will have three settings to manage and keep in sync.
Luchen PengThe common solution would be to deprecate Autofill{Address,Card}Enabled but keep them working (your CL does this). More importantly, if both {Address,Card}Enabled *and* AutofillSettings are both set, they could "merge" in a way that makes sense (your CL doesn't do this right now).
For example, look at ExtensionInstall{Force,Allow,Block}list vs. ExtensionSettings. If you set them, the force/allow/blocklist effectively gets merged into the ExtensionSettings dict. This is the more common pattern, and it's less confusing for admins.
I think(?) you could do something similar here: let the admin define rules in AutofillSettings, and if they set Autofill{Address,Card}Enabled we *also* add a wildcard rule in `kAutofillTypesBlocked`. Or maybe that doesn't make sense for this policy?
(for context: `LegacyPoliciesDeprecatingPolicyHandler` is useful when the policies can't be combined, so it doesn't make sense to set both. e.g. SigninEnabled (bool) vs. BrowserSignin (enum: disabled, enabled, forced) we can't really reconcile conflicting values. So when you have `SigninEnabled=false` and `BrowserSignin=enabled`, BrowserSignin always "wins" the conflict)
I see, in this case I think merging them makes more sense. When this policy is checked on the client, we match all the rules, so if there are conflict rules, e.g. AutofillAddressEnabled is set to false and in AutofillSettings address is disabled in a specific domain, we would respect the overall address disabled policy. I think this is fine.
I removed the usage of `LegacyPoliciesDeprecatingPolicyHandler`, I think that is not needed in this case.
Nice, thanks
bool AutofillSettingsPolicyHandler::CheckPolicySettings(It should probably show a warning if the legacy policies aren't the correct type
if (const base::Value* autofill_settings =
policies.GetValue(policy_name(), base::Value::Type::LIST)) {
for (const auto& item : autofill_settings->GetList()) {
merged_rules.Append(item.Clone());
}
}You can simplify by removing the guard on lines 107-115:
```
base::ListValue merged_rules;
if (const base::Value* autofill_settings =
policies.GetValue(policy_name(), base::Value::Type::LIST)) {
merged_rules = autofill_settings->GetList().Clone();
}
if (!blocked_types.empty()) {
merged_rules.Append(base::DictValue()
.Set(kUrlPatternKey, "*")
.Set(kBlockedTypesKey, std::move(blocked_types)))
}
```
Related details
- Christoph Schwering
- Luchen Peng
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. |
Christoph Schwering (Gerrit)
Christoph Schwering added 4 comments![]( "Open in Gerrit")
The code looks good to me % the open comments, but perhaps a reviewer of the doc should review this CL.
// If no legacy booleans are disabled, let the validating handler applynit: "boolean policies" or so?
if (blocked_types.empty()) {Why is it OK to enter the `if` block if, for example, `kAutofillAddressEnabled` is disabled and `kAutofillCreditCardEnabled` is enabled?
// If legacy booleans are disabled, build the consolidated list
// directly in memory (configured rules + wildcard block rule) and write tonit: join lines
Related details
- Luchen Peng
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. |
Luchen Peng (Gerrit)
Luchen Peng voted and added 5 comments![]( "Open in Gerrit")
Votes added by Luchen Peng
| Commit-Queue | +1 |
5 comments
bool AutofillSettingsPolicyHandler::CheckPolicySettings(It should probably show a warning if the legacy policies aren't the correct type
Added checks for legacy policy type.
// If no legacy booleans are disabled, let the validating handler applynit: "boolean policies" or so?
Updated to "boolean policies".
if (blocked_types.empty()) {Why is it OK to enter the `if` block if, for example, `kAutofillAddressEnabled` is disabled and `kAutofillCreditCardEnabled` is enabled?
No, it is checking both in the previous block, for `legacy_address_enabled` and `legacy_credit_card_enabled`, and appending to the `blocked_types`.
// If legacy booleans are disabled, build the consolidated list
// directly in memory (configured rules + wildcard block rule) and write toLuchen Pengnit: join lines
Done
if (const base::Value* autofill_settings =
policies.GetValue(policy_name(), base::Value::Type::LIST)) {
for (const auto& item : autofill_settings->GetList()) {
merged_rules.Append(item.Clone());
}
}You can simplify by removing the guard on lines 107-115:
```
base::ListValue merged_rules;
if (const base::Value* autofill_settings =
policies.GetValue(policy_name(), base::Value::Type::LIST)) {
merged_rules = autofill_settings->GetList().Clone();
}
if (!blocked_types.empty()) {
merged_rules.Append(base::DictValue()
.Set(kUrlPatternKey, "*")
.Set(kBlockedTypesKey, std::move(blocked_types)))
}
```
- Christoph Schwering
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. |
Christoph Schwering (Gerrit)
Christoph Schwering voted and added 2 comments![]( "Open in Gerrit")
Votes added by Christoph Schwering
| Code-Review | +1 |
2 comments
constexpr char kContactInfoBlockedType[] = "contact_info";
constexpr char kPaymentsBlockedType[] = "payments";
constexpr char kUrlPatternKey[] = "url_pattern";
constexpr char kBlockedTypesKey[] = "blocked_types";Should we ensure that these are identical to https://source.chromium.org/chromium/chromium/src/+/main:components/autofill/core/browser/permissions/enterprise_policy/autofill_enterprise_policy_service.cc;l=23;drc=54e68f01b6df87060477349a42b3dc60c83fff2a?
I think at least documentation would be helpful to understand where those prefs that are written here will be read.
if (blocked_types.empty()) {Luchen PengWhy is it OK to enter the `if` block if, for example, `kAutofillAddressEnabled` is disabled and `kAutofillCreditCardEnabled` is enabled?
No, it is checking both in the previous block, for `legacy_address_enabled` and `legacy_credit_card_enabled`, and appending to the `blocked_types`.
I think I got it now: IIUC the `policy::SimpleSchemaValidatingPolicyHandler::ApplyPolicySettings()` call inside the `if` statement is equivalent to the code after the `if` block if `blocked_types` were empty, except that the merging logic in that case would add useless `blocked_types: []` to the pref.
Related details
- Luchen Peng
- Norge Vizcay
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. |