Commit 309aa1f2 authored by Patrick Monette's avatar Patrick Monette Committed by Commit Bot

Remove the IAttachmentExecute workaround for enterprise users

This CL is mostly a revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1195682/, and
also adds a DCHECK that ensures that we don't enable third-party
blocking if the QuarantineFile() functions doesn't run out-of-process.

Bug: 870998
Change-Id: I7ff74b1595aa889c12564847d7735825b33f7ae7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1899752Reviewed-by: default avatarChris Hamilton <chrisha@chromium.org>
Reviewed-by: default avatarAvi Drissman <avi@chromium.org>
Reviewed-by: default avatarAsanka Herath <asanka@chromium.org>
Commit-Queue: Patrick Monette <pmonette@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713538}
parent 8cbef17e
...@@ -569,8 +569,7 @@ void ChromeBrowserMainPartsWin::PostProfileInit() { ...@@ -569,8 +569,7 @@ void ChromeBrowserMainPartsWin::PostProfileInit() {
// What truly controls if the blocking is enabled is the presence of the // What truly controls if the blocking is enabled is the presence of the
// module blacklist cache file. This means that to disable the feature, the // module blacklist cache file. This means that to disable the feature, the
// cache must be deleted and the browser relaunched. // cache must be deleted and the browser relaunched.
if (base::IsMachineExternallyManaged() || if (!ModuleDatabase::IsThirdPartyBlockingPolicyEnabled() ||
!ModuleDatabase::IsThirdPartyBlockingPolicyEnabled() ||
!ModuleBlacklistCacheUpdater::IsBlockingEnabled()) !ModuleBlacklistCacheUpdater::IsBlockingEnabled())
ThirdPartyConflictsManager::DisableThirdPartyModuleBlocking( ThirdPartyConflictsManager::DisableThirdPartyModuleBlocking(
base::CreateTaskRunner( base::CreateTaskRunner(
......
...@@ -15,7 +15,6 @@ ...@@ -15,7 +15,6 @@
#include "content/public/browser/browser_thread.h" #include "content/public/browser/browser_thread.h"
#if defined(GOOGLE_CHROME_BUILD) #if defined(GOOGLE_CHROME_BUILD)
#include "base/enterprise_util.h"
#include "base/win/win_util.h" #include "base/win/win_util.h"
#include "chrome/browser/win/conflicts/incompatible_applications_updater.h" #include "chrome/browser/win/conflicts/incompatible_applications_updater.h"
#include "chrome/browser/win/conflicts/module_blacklist_cache_updater.h" #include "chrome/browser/win/conflicts/module_blacklist_cache_updater.h"
...@@ -224,12 +223,6 @@ enum ThirdPartyFeaturesStatus { ...@@ -224,12 +223,6 @@ enum ThirdPartyFeaturesStatus {
kNonGoogleChromeBuild, kNonGoogleChromeBuild,
// The third-party features are not available on Windows 7. // The third-party features are not available on Windows 7.
kNotAvailableWin7, kNotAvailableWin7,
// The third-party features are temporarily disabled on domain-joined
// machines because of a known issue with third-party blocking and the
// IAttachmentExecute::Save() API (https://crbug.com/870998).
// TODO(pmonette): Move IAttachmentExecute::Save() to a utility process and
// remove this.
kEnterpriseManaged,
// The ThirdPartyBlockingEnabled group policy is disabled. // The ThirdPartyBlockingEnabled group policy is disabled.
kPolicyDisabled, kPolicyDisabled,
// Both the IncompatibleApplicationsWarning and the // Both the IncompatibleApplicationsWarning and the
...@@ -284,9 +277,6 @@ ThirdPartyFeaturesStatus GetThirdPartyFeaturesStatus( ...@@ -284,9 +277,6 @@ ThirdPartyFeaturesStatus GetThirdPartyFeaturesStatus(
return kFeatureDisabled; return kFeatureDisabled;
} }
if (base::IsMachineExternallyManaged())
return kEnterpriseManaged;
// The above 3 cases are the only possible reasons why the manager wouldn't // The above 3 cases are the only possible reasons why the manager wouldn't
// exist. // exist.
NOTREACHED(); NOTREACHED();
...@@ -306,9 +296,6 @@ std::string GetThirdPartyFeaturesStatusString(ThirdPartyFeaturesStatus status) { ...@@ -306,9 +296,6 @@ std::string GetThirdPartyFeaturesStatusString(ThirdPartyFeaturesStatus status) {
"builds."; "builds.";
case ThirdPartyFeaturesStatus::kNotAvailableWin7: case ThirdPartyFeaturesStatus::kNotAvailableWin7:
return "The third-party features are not available on Windows 7."; return "The third-party features are not available on Windows 7.";
case ThirdPartyFeaturesStatus::kEnterpriseManaged:
return "The third-party features are temporarily disabled for clients on "
"domain-joined machines.";
case ThirdPartyFeaturesStatus::kPolicyDisabled: case ThirdPartyFeaturesStatus::kPolicyDisabled:
return "The ThirdPartyBlockingEnabled group policy is disabled."; return "The ThirdPartyBlockingEnabled group policy is disabled.";
case ThirdPartyFeaturesStatus::kFeatureDisabled: case ThirdPartyFeaturesStatus::kFeatureDisabled:
......
...@@ -17,7 +17,6 @@ ...@@ -17,7 +17,6 @@
#include "content/public/browser/browser_task_traits.h" #include "content/public/browser/browser_task_traits.h"
#if defined(GOOGLE_CHROME_BUILD) #if defined(GOOGLE_CHROME_BUILD)
#include "base/enterprise_util.h"
#include "base/feature_list.h" #include "base/feature_list.h"
#include "chrome/browser/browser_process.h" #include "chrome/browser/browser_process.h"
#include "chrome/browser/win/conflicts/incompatible_applications_updater.h" #include "chrome/browser/win/conflicts/incompatible_applications_updater.h"
...@@ -29,6 +28,7 @@ ...@@ -29,6 +28,7 @@
#include "components/prefs/pref_change_registrar.h" #include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_registry_simple.h" #include "components/prefs/pref_registry_simple.h"
#include "components/prefs/pref_service.h" #include "components/prefs/pref_service.h"
#include "components/services/quarantine/public/cpp/quarantine_features_win.h"
#endif #endif
namespace { namespace {
...@@ -36,21 +36,6 @@ namespace { ...@@ -36,21 +36,6 @@ namespace {
ModuleDatabase* g_module_database = nullptr; ModuleDatabase* g_module_database = nullptr;
#if defined(GOOGLE_CHROME_BUILD) #if defined(GOOGLE_CHROME_BUILD)
// Returns true if either the IncompatibleApplicationsWarning or
// ThirdPartyModulesBlocking features are enabled via the "enable-features"
// command-line switch.
bool AreThirdPartyFeaturesEnabledViaCommandLine() {
// The FeatureList API is thread-safe.
base::FeatureList* feature_list_instance = base::FeatureList::GetInstance();
return feature_list_instance->IsFeatureOverriddenFromCommandLine(
features::kIncompatibleApplicationsWarning.name,
base::FeatureList::OVERRIDE_ENABLE_FEATURE) ||
feature_list_instance->IsFeatureOverriddenFromCommandLine(
features::kThirdPartyModulesBlocking.name,
base::FeatureList::OVERRIDE_ENABLE_FEATURE);
}
// Callback for the pref change registrar. Is invoked when the // Callback for the pref change registrar. Is invoked when the
// ThirdPartyBlockingEnabled policy is modified. Notifies the ModuleDatabase if // ThirdPartyBlockingEnabled policy is modified. Notifies the ModuleDatabase if
// the policy was disabled. // the policy was disabled.
...@@ -446,23 +431,13 @@ void ModuleDatabase::MaybeInitializeThirdPartyConflictsManager( ...@@ -446,23 +431,13 @@ void ModuleDatabase::MaybeInitializeThirdPartyConflictsManager(
bool third_party_blocking_policy_enabled) { bool third_party_blocking_policy_enabled) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_); DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
// Temporarily disable this class on domain-joined machines because enterprise
// clients depend on IAttachmentExecute::Save() to be invoked for downloaded
// files, but that API call has a known issue (https://crbug.com/870998) with
// third-party modules blocking. Can be Overridden by enabling the feature via
// the command-line.
// TODO(pmonette): Move IAttachmentExecute::Save() to a utility process and
// remove this.
if (base::IsMachineExternallyManaged() &&
!AreThirdPartyFeaturesEnabledViaCommandLine()) {
return;
}
if (!third_party_blocking_policy_enabled) if (!third_party_blocking_policy_enabled)
return; return;
if (IncompatibleApplicationsUpdater::IsWarningEnabled() || if (IncompatibleApplicationsUpdater::IsWarningEnabled() ||
ModuleBlacklistCacheUpdater::IsBlockingEnabled()) { ModuleBlacklistCacheUpdater::IsBlockingEnabled()) {
DCHECK(base::FeatureList::IsEnabled(quarantine::kOutOfProcessQuarantine));
third_party_conflicts_manager_ = third_party_conflicts_manager_ =
std::make_unique<ThirdPartyConflictsManager>(this); std::make_unique<ThirdPartyConflictsManager>(this);
......
...@@ -13,7 +13,6 @@ ...@@ -13,7 +13,6 @@
#include "base/test/scoped_feature_list.h" #include "base/test/scoped_feature_list.h"
#include "base/test/test_reg_util_win.h" #include "base/test/test_reg_util_win.h"
#include "base/win/registry.h" #include "base/win/registry.h"
#include "base/win/win_util.h"
#include "base/win/windows_version.h" #include "base/win/windows_version.h"
#include "chrome/browser/win/conflicts/module_blacklist_cache_updater.h" #include "chrome/browser/win/conflicts/module_blacklist_cache_updater.h"
#include "chrome/browser/win/conflicts/module_blacklist_cache_util.h" #include "chrome/browser/win/conflicts/module_blacklist_cache_util.h"
...@@ -79,8 +78,7 @@ class ThirdPartyRegistryKeyObserver { ...@@ -79,8 +78,7 @@ class ThirdPartyRegistryKeyObserver {
class ThirdPartyBlockingBrowserTest : public InProcessBrowserTest { class ThirdPartyBlockingBrowserTest : public InProcessBrowserTest {
protected: protected:
ThirdPartyBlockingBrowserTest() : scoped_domain_(false) {} ThirdPartyBlockingBrowserTest() = default;
~ThirdPartyBlockingBrowserTest() override = default; ~ThirdPartyBlockingBrowserTest() override = default;
// InProcessBrowserTest: // InProcessBrowserTest:
...@@ -134,9 +132,6 @@ class ThirdPartyBlockingBrowserTest : public InProcessBrowserTest { ...@@ -134,9 +132,6 @@ class ThirdPartyBlockingBrowserTest : public InProcessBrowserTest {
static_cast<int>(contents.size()))); static_cast<int>(contents.size())));
} }
// The feature is always disabled on domain-joined machines.
base::win::ScopedDomainStateForTesting scoped_domain_;
// Enables the ThirdPartyModulesBlocking feature. // Enables the ThirdPartyModulesBlocking feature.
base::test::ScopedFeatureList scoped_feature_list_; base::test::ScopedFeatureList scoped_feature_list_;
......
...@@ -12,10 +12,4 @@ namespace quarantine { ...@@ -12,10 +12,4 @@ namespace quarantine {
const base::Feature kOutOfProcessQuarantine{"OutOfProcessQuarantine", const base::Feature kOutOfProcessQuarantine{"OutOfProcessQuarantine",
base::FEATURE_DISABLED_BY_DEFAULT}; base::FEATURE_DISABLED_BY_DEFAULT};
// This feature controls whether the InvokeAttachmentServices function will be
// called. Has no effect on machines that are domain-joined, where the function
// is always called.
const base::Feature kInvokeAttachmentServices{"InvokeAttachmentServices",
base::FEATURE_ENABLED_BY_DEFAULT};
} // namespace quarantine } // namespace quarantine
...@@ -11,8 +11,6 @@ namespace quarantine { ...@@ -11,8 +11,6 @@ namespace quarantine {
extern const base::Feature kOutOfProcessQuarantine; extern const base::Feature kOutOfProcessQuarantine;
extern const base::Feature kInvokeAttachmentServices;
} // namespace quarantine } // namespace quarantine
#endif // COMPONENTS_SERVICES_QUARANTINE_PUBLIC_CPP_QUARANTINE_FEATURES_WIN_H_ #endif // COMPONENTS_SERVICES_QUARANTINE_PUBLIC_CPP_QUARANTINE_FEATURES_WIN_H_
...@@ -16,7 +16,6 @@ ...@@ -16,7 +16,6 @@
#include <vector> #include <vector>
#include "base/enterprise_util.h"
#include "base/feature_list.h" #include "base/feature_list.h"
#include "base/files/file_util.h" #include "base/files/file_util.h"
#include "base/guid.h" #include "base/guid.h"
...@@ -258,21 +257,8 @@ QuarantineFileResult QuarantineFile(const base::FilePath& file, ...@@ -258,21 +257,8 @@ QuarantineFileResult QuarantineFile(const base::FilePath& file,
return SetInternetZoneIdentifierDirectly(file, source_url, referrer_url); return SetInternetZoneIdentifierDirectly(file, source_url, referrer_url);
} }
// Check if the attachment services should be invoked based on the experiment
// state. Not invoking the attachment services means that the Zone Identifier
// will always be set to 3 (Internet), regardless of URL zones configurations.
//
// Note: The attachment services must always be invoked on domain-joined
// machines.
// TODO(pmonette): Move the InvokeAttachmentServices() call to a utility
// process and remove the feature.
bool should_invoke_attachment_services =
base::IsMachineExternallyManaged() ||
base::FeatureList::IsEnabled(kInvokeAttachmentServices);
QuarantineFileResult attachment_services_result = QuarantineFileResult::OK; QuarantineFileResult attachment_services_result = QuarantineFileResult::OK;
if (should_invoke_attachment_services && if (InvokeAttachmentServices(file, source_url, referrer_url, guid,
InvokeAttachmentServices(file, source_url, referrer_url, guid,
&attachment_services_result)) { &attachment_services_result)) {
return attachment_services_result; return attachment_services_result;
} }
......
...@@ -424,6 +424,7 @@ TEST_F(QuarantineWinTest, MetaData_ApplyMOTW_Directly) { ...@@ -424,6 +424,7 @@ TEST_F(QuarantineWinTest, MetaData_ApplyMOTW_Directly) {
GURL referrer_url_clean = GURL( GURL referrer_url_clean = GURL(
base::StringPrintf(L"https://%ls/folder/index?x#y", GetInternetSite())); base::StringPrintf(L"https://%ls/folder/index?x#y", GetInternetSite()));
// An invalid GUID will cause QuarantineFile() to apply the MOTW directly.
EXPECT_EQ(QuarantineFileResult::OK, EXPECT_EQ(QuarantineFileResult::OK,
QuarantineFile(test_file, host_url, referrer_url, std::string())); QuarantineFile(test_file, host_url, referrer_url, std::string()));
...@@ -431,9 +432,6 @@ TEST_F(QuarantineWinTest, MetaData_ApplyMOTW_Directly) { ...@@ -431,9 +432,6 @@ TEST_F(QuarantineWinTest, MetaData_ApplyMOTW_Directly) {
} }
TEST_F(QuarantineWinTest, MetaData_InvokeAS) { TEST_F(QuarantineWinTest, MetaData_InvokeAS) {
base::test::ScopedFeatureList invoke_as_feature;
invoke_as_feature.InitAndEnableFeature(kInvokeAttachmentServices);
base::FilePath test_file = GetTempDir().AppendASCII("foo.exe"); base::FilePath test_file = GetTempDir().AppendASCII("foo.exe");
ASSERT_TRUE(CreateFile(test_file)); ASSERT_TRUE(CreateFile(test_file));
......
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