Skip to content

Commit

Permalink
[iOS][UserPolicy]🏢 Integrate and allow UserPolicy
Browse files Browse the repository at this point in the history
This is the follow up CL http://crrev.com/c/3497515 (where we
introduced the UserPolicySigninService for iOS).

This CL does the necessary integration to be able to enable the
UserPolicy feature. Is only viable for testing at this stage (need a
few follow-up CLs to have the thing polished).

At this stage, UserPolicy have to be enabled from Experimental Settings.

Tested:
 > User policies are fetched when enabling sync for the managed account.
 > User policies aren't fetched when the user only signs in.
 > User policies are deleted when signing out.
 > If the user policy cache is empty and sync enabled for the managed
   account at startup, user policies will be fetched.
 > User Policy is only enabled when the feature is toggled.
 > User policies are deleted when the feature is toggled off
   (at restart).

Change-Id: Id49e12b269df31d2714f6e6c59ed8c8390e04658
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3573186
Reviewed-by: Fabio Tirelo <ftirelo@chromium.org>
Reviewed-by: Jérôme Lebel <jlebel@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Vincent Boisselle <vincb@google.com>
Cr-Commit-Position: refs/heads/main@{#990685}
  • Loading branch information
vincb2 authored and Chromium LUCI CQ committed Apr 9, 2022
1 parent d11d1cf commit d042d55
Show file tree
Hide file tree
Showing 22 changed files with 492 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ namespace {
#if BUILDFLAG(IS_ANDROID)
const em::DeviceRegisterRequest::Type kCloudPolicyRegistrationType =
em::DeviceRegisterRequest::ANDROID_BROWSER;
#elif BUILDFLAG(IS_IOS)
// TODO(crbug.com/1312263): Use em::DeviceRegisterRequest::IOS_BROWSER when
// supported in the dmserver. The type for Desktop is temporarily used on iOS
// to allow early testing of the feature before the DMServer can support iOS
// User Policy.
const em::DeviceRegisterRequest::Type kCloudPolicyRegistrationType =
em::DeviceRegisterRequest::BROWSER;
#else
const em::DeviceRegisterRequest::Type kCloudPolicyRegistrationType =
em::DeviceRegisterRequest::BROWSER;
Expand Down
45 changes: 21 additions & 24 deletions components/policy/core/common/cloud/cloud_policy_client_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -562,6 +562,13 @@ class CloudPolicyClientTest : public testing::Test {
base::RunLoop().RunUntilIdle();
}

void VerifyQueryParameter() {
#if !BUILDFLAG(IS_IOS)
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
#endif
}

base::test::SingleThreadTaskEnvironment task_environment_;
DeviceManagementService::JobConfiguration::JobType job_type_;
DeviceManagementService::JobConfiguration::ParameterMap query_params_;
Expand Down Expand Up @@ -639,8 +646,7 @@ TEST_F(CloudPolicyClientTest, SetupRegistrationAndPolicyFetchWithOAuthToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_POLICY_FETCH,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::FromDMToken(kDMToken));
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetPolicyRequest().SerializePartialAsString());
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand Down Expand Up @@ -708,8 +714,7 @@ TEST_F(CloudPolicyClientTest, RegistrationAndPolicyFetch) {
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetRegistrationRequest().SerializePartialAsString());
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_TRUE(client_->is_registered());
EXPECT_FALSE(client_->GetPolicyFor(policy_type_, std::string()));
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand Down Expand Up @@ -748,8 +753,7 @@ TEST_F(CloudPolicyClientTest, RegistrationAndPolicyFetchWithOAuthToken) {
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetRegistrationRequest().SerializePartialAsString());
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_TRUE(client_->is_registered());
EXPECT_FALSE(client_->GetPolicyFor(policy_type_, std::string()));
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand All @@ -761,8 +765,7 @@ TEST_F(CloudPolicyClientTest, RegistrationAndPolicyFetchWithOAuthToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_POLICY_FETCH,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::FromDMToken(kDMToken));
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetPolicyRequest().SerializePartialAsString());
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand Down Expand Up @@ -855,8 +858,7 @@ TEST_F(CloudPolicyClientTest, RegistrationParametersPassedThrough) {
EXPECT_EQ(job_request_.SerializePartialAsString(),
registration_request.SerializePartialAsString());
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(kClientID, client_id_);
}

Expand All @@ -878,8 +880,7 @@ TEST_F(CloudPolicyClientTest, RegistrationNoToken) {
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetRegistrationRequest().SerializePartialAsString());
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_FALSE(client_->is_registered());
EXPECT_FALSE(client_->GetPolicyFor(policy_type_, std::string()));
EXPECT_EQ(DM_STATUS_RESPONSE_DECODING_ERROR, client_->status());
Expand Down Expand Up @@ -1144,8 +1145,7 @@ TEST_F(CloudPolicyClientTest, PolicyFetchClearOAuthToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_POLICY_FETCH,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::FromDMToken(kDMToken));
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
policy_request.SerializePartialAsString());
CheckPolicyResponse(policy_response);
Expand Down Expand Up @@ -1503,8 +1503,7 @@ TEST_F(CloudPolicyClientTest, UploadStatusWithOAuthToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_UPLOAD_STATUS,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::FromDMToken(kDMToken));
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetUploadStatusRequest().SerializePartialAsString());
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand All @@ -1523,8 +1522,10 @@ TEST_F(CloudPolicyClientTest, UploadStatusWithOAuthToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_UPLOAD_STATUS,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::FromDMToken(kDMToken));
#if !BUILDFLAG(IS_IOS)
EXPECT_THAT(query_params_,
Not(Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken))));
#endif
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetUploadStatusRequest().SerializePartialAsString());
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand Down Expand Up @@ -2358,8 +2359,7 @@ TEST_F(CloudPolicyClientTest, RequestDeviceAttributeUpdatePermission) {
TYPE_ATTRIBUTE_UPDATE_PERMISSION,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
attribute_update_permission_request.SerializePartialAsString());
EXPECT_EQ(DM_STATUS_SUCCESS, client_->status());
Expand Down Expand Up @@ -2390,8 +2390,7 @@ TEST_F(CloudPolicyClientTest, RequestDeviceAttributeUpdate) {
base::RunLoop().RunUntilIdle();
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_ATTRIBUTE_UPDATE,
job_type_);
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_EQ(job_request_.SerializePartialAsString(),
attribute_update_request.SerializePartialAsString());
Expand Down Expand Up @@ -2456,8 +2455,7 @@ TEST_F(CloudPolicyClientTest, PolicyReregistration) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_REGISTRATION,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetReregistrationRequest().SerializePartialAsString());
EXPECT_TRUE(client_->is_registered());
Expand Down Expand Up @@ -2500,8 +2498,7 @@ TEST_F(CloudPolicyClientTest, PolicyReregistrationFailsWithNonMatchingDMToken) {
EXPECT_EQ(DeviceManagementService::JobConfiguration::TYPE_REGISTRATION,
job_type_);
EXPECT_EQ(auth_data_, DMAuth::NoAuth());
EXPECT_THAT(query_params_,
Contains(Pair(dm_protocol::kParamOAuthToken, kOAuthToken)));
VerifyQueryParameter();
EXPECT_EQ(job_request_.SerializePartialAsString(),
GetReregistrationRequest().SerializePartialAsString());
EXPECT_FALSE(client_->is_registered());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,11 @@ const char kChromeUserPolicyType[] = "google/chromeos/user";
#elif BUILDFLAG(IS_ANDROID)
const char kChromeUserPolicyType[] = "google/android/user";
#elif BUILDFLAG(IS_IOS)
const char kChromeUserPolicyType[] = "google/ios/user";
// TODO(crbug.com/1312263): Change this for "google/ios/user" once supported
// by the dmserver. The type for Desktop is temporarily used on iOS to allow
// early testing of the feature before the DMServer can support iOS User
// Policy.
const char kChromeUserPolicyType[] = "google/chrome/user";
#else
const char kChromeUserPolicyType[] = "google/chrome/user";
#endif
Expand Down
24 changes: 23 additions & 1 deletion components/policy/core/common/cloud/device_management_service.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
#include "base/location.h"
#include "base/logging.h"
#include "base/metrics/histogram_functions.h"
#include "base/strings/strcat.h"
#include "base/strings/string_number_conversions.h"
#include "base/task/sequenced_task_runner.h"
#include "base/threading/sequenced_task_runner_handle.h"
#include "build/build_config.h"
#include "components/policy/proto/device_management_backend.pb.h"
#include "net/base/escape.h"
#include "net/base/load_flags.h"
Expand Down Expand Up @@ -222,8 +224,15 @@ JobConfigurationBase::JobConfigurationBase(
oauth_token_(std::move(oauth_token)) {
CHECK(!auth_data_.has_oauth_token()) << "Use |oauth_token| instead";

if (oauth_token_)
#if !BUILDFLAG(IS_IOS)
if (oauth_token_) {
// Put the oauth token in the query parameters for platforms that are not
// iOS. On iOS we are trying the oauth token in the request headers
// (crbug.com/1312158). We might want to use the iOS approach on all
// platforms at some point.
AddParameter(dm_protocol::kParamOAuthToken, *oauth_token_);
}
#endif
}

JobConfigurationBase::~JobConfigurationBase() = default;
Expand Down Expand Up @@ -308,6 +317,19 @@ JobConfigurationBase::GetResourceRequest(bool bypass_proxy, int last_error) {
rr->trusted_params = network::ResourceRequest::TrustedParams();
rr->trusted_params->disable_secure_dns = true;

#if BUILDFLAG(IS_IOS)
// Put the oauth token in the request headers on iOS. We might want
// to use this approach on the other platforms at some point. This approach
// will be tried first on iOS (crbug.com/1312158). Technically, the
// DMServer should already be able to handle the oauth token in the
// request headers, but we prefer to try the approach on iOS first to
// avoid breaking the other platforms with unexpected issues.
if (oauth_token_ && !oauth_token_->empty()) {
rr->headers.SetHeader(dm_protocol::kAuthHeader,
base::StrCat({"OAuth ", *oauth_token_}));
}
#endif

// If auth data is specified, use it to build the request.
switch (auth_data_.token_type()) {
case DMAuthTokenType::kNoAuth:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "base/strings/stringprintf.h"
#include "base/test/metrics/histogram_tester.h"
#include "base/test/task_environment.h"
#include "build/build_config.h"
#include "components/policy/core/common/cloud/cloud_policy_constants.h"
#include "components/policy/core/common/cloud/dm_auth.h"
#include "components/policy/core/common/cloud/mock_device_management_service.h"
Expand Down Expand Up @@ -57,6 +58,9 @@ const char kDMToken[] = "device-management-token";
const char kClientID[] = "device-id";
const char kRobotAuthCode[] = "robot-oauth-auth-code";
const char kEnrollmentToken[] = "enrollment_token";
#if BUILDFLAG(IS_IOS)
const char kOAuthAuthorizationHeaderPrefix[] = "OAuth ";
#endif

// Unit tests for the device management policy service. The tests are run
// against a TestURLLoaderFactory that is used to short-circuit the request
Expand Down Expand Up @@ -1112,10 +1116,16 @@ TEST_F(DeviceManagementRequestAuthTest, OnlyOAuthToken) {
GetPendingRequest();
ASSERT_TRUE(request);

#if BUILDFLAG(IS_IOS)
EXPECT_EQ(base::StrCat({kOAuthAuthorizationHeaderPrefix, kOAuthToken}),
GetAuthHeader(*request));
EXPECT_TRUE(GetOAuthParams(*request).empty());
#else
const std::vector<std::string> params = GetOAuthParams(*request);
ASSERT_EQ(1u, params.size());
EXPECT_EQ(kOAuthToken, params[0]);
EXPECT_FALSE(GetAuthHeader(*request));
#endif

SendResponse(net::OK, 200, std::string());
}
Expand Down Expand Up @@ -1157,6 +1167,10 @@ TEST_F(DeviceManagementRequestAuthTest, OnlyEnrollmentToken) {
SendResponse(net::OK, 200, std::string());
}

#if !BUILDFLAG(IS_IOS)
// Cannot test requests with an oauth token and another authorization token on
// iOS because they both use the "Authorization" header.

TEST_F(DeviceManagementRequestAuthTest, OAuthAndDMToken) {
std::unique_ptr<DeviceManagementService::Job> request_job(
StartJobWithAuthData(DMAuth::FromDMToken(kDMToken), kOAuthToken));
Expand Down Expand Up @@ -1195,6 +1209,8 @@ TEST_F(DeviceManagementRequestAuthTest, OAuthAndEnrollmentToken) {
SendResponse(net::OK, 200, std::string());
}

#endif

#if defined(GTEST_HAS_DEATH_TEST)
TEST_F(DeviceManagementRequestAuthTest, CannotUseOAuthTokenAsAuthData) {
// Job type is not really relevant for the test.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "ios/chrome/browser/optimization_guide/optimization_guide_service_factory.h"
#include "ios/chrome/browser/passwords/ios_chrome_password_check_manager_factory.h"
#include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
#include "ios/chrome/browser/policy/cloud/user_policy_signin_service_factory.h"
#import "ios/chrome/browser/policy/policy_features.h"
#include "ios/chrome/browser/policy_url_blocking/policy_url_blocking_service.h"
#include "ios/chrome/browser/reading_list/reading_list_model_factory.h"
Expand Down Expand Up @@ -143,6 +144,7 @@ void EnsureBrowserStateKeyedServiceFactoriesBuilt() {
ManagedBookmarkServiceFactory::GetInstance();
ModelTypeStoreServiceFactory::GetInstance();
OptimizationGuideServiceFactory::GetInstance();
policy::UserPolicySigninServiceFactory::GetInstance();
TabsSearchServiceFactory::GetInstance();
SyncServiceFactory::GetInstance();
ReadingListModelFactory::GetInstance();
Expand Down
1 change: 1 addition & 0 deletions ios/chrome/browser/flags/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ source_set("flags") {
"//ios/chrome/browser/browsing_data:feature_flags",
"//ios/chrome/browser/crash_report",
"//ios/chrome/browser/drag_and_drop",
"//ios/chrome/browser/policy",
"//ios/chrome/browser/policy:feature_flags",
"//ios/chrome/browser/policy:policy_util",
"//ios/chrome/browser/screen_time:buildflags",
Expand Down
5 changes: 5 additions & 0 deletions ios/chrome/browser/flags/about_flags.mm
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@
#include "ios/chrome/browser/chrome_switches.h"
#include "ios/chrome/browser/crash_report/features.h"
#include "ios/chrome/browser/flags/ios_chrome_flag_descriptions.h"
#include "ios/chrome/browser/policy/cloud/user_policy_switch.h"
#include "ios/chrome/browser/policy/policy_features.h"
#include "ios/chrome/browser/policy/policy_util.h"
#include "ios/chrome/browser/screen_time/screen_time_buildflags.h"
Expand Down Expand Up @@ -1060,6 +1061,10 @@ void AppendSwitchesFromExperimentalSettings(base::CommandLine* command_line) {
[testing_policies setValue:token forKey:token_key];
}

if ([defaults boolForKey:@"EnableUserPolicy"]) {
policy::EnableUserPolicy();
}

// If some policies were set, commit them to the app's registration defaults.
if ([testing_policies count] > 0) {
NSDictionary* registration_defaults =
Expand Down
2 changes: 2 additions & 0 deletions ios/chrome/browser/policy/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ source_set("policy") {
"cloud/user_policy_signin_service.mm",
"cloud/user_policy_signin_service_factory.h",
"cloud/user_policy_signin_service_factory.mm",
"cloud/user_policy_switch.h",
"cloud/user_policy_switch.mm",
"cloud_policy_client_observer_bridge.h",
"cloud_policy_client_observer_bridge.mm",
"configuration_policy_handler_list_factory.h",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "google_apis/gaia/core_account_id.h"
#include "google_apis/gaia/gaia_auth_util.h"
#include "ios/chrome/browser/browser_state/chrome_browser_state.h"
#include "ios/chrome/browser/policy/cloud/user_policy_switch.h"
#include "services/network/public/cpp/shared_url_loader_factory.h"

#if !defined(__has_feature) || !__has_feature(objc_arc)
Expand Down Expand Up @@ -86,7 +87,10 @@ AccountId AccountIdFromAccountInfo(const CoreAccountInfo& account_info) {
// (http://crbug.com/316229).
scoped_identity_manager_observation_.Observe(identity_manager());

if (!CanApplyPolicies(/*check_for_refresh_token=*/false)) {
if (!IsUserPolicyEnabled() ||
!CanApplyPolicies(/*check_for_refresh_token=*/false)) {
// Clear existing user policies if the feature is disabled or if policies
// can no longer be applied.
ShutdownUserCloudPolicyManager();
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ class UserPolicySigninService;

// Singleton that owns all UserPolicySigninServices and creates/deletes them as
// new BrowserStates are created/shutdown.
//
// Warning: ONLY use the service when Enterprise Policy is enabled where
// the policy system objects are enabled (eg. the BrowserPolicyConnector object
// was instantiated).
class UserPolicySigninServiceFactory : public BrowserStateKeyedServiceFactory {
public:
// Returns an instance of the UserPolicySigninServiceFactory singleton.
Expand Down Expand Up @@ -51,6 +55,9 @@ class UserPolicySigninServiceFactory : public BrowserStateKeyedServiceFactory {
user_prefs::PrefRegistrySyncable* registry) override;

private:
bool ServiceIsCreatedWithBrowserState() const override;
bool ServiceIsNULLWhileTesting() const override;

friend struct base::DefaultSingletonTraits<UserPolicySigninServiceFactory>;

UserPolicySigninServiceFactory();
Expand Down
Loading

0 comments on commit d042d55

Please sign in to comment.