Skip to content

Commit

Permalink
[Phone Hub] Update settings ui based on phone's enterprise policy.
Browse files Browse the repository at this point in the history
- Defines new proto for apps streaming policy.
- Based on phone's apps streaming policy to update setting UI.
- If CrOS's and Phone's enterprise policy are prohibited, CrOS's enterprise policy has higher priority.

Bug: b/213314358
Test:
- ./out/Default/browser_tests --gtest_filter="OSSettingsMultideviceSubPageV3Test.*"
- ./out/Default/ash_webui_unittests

Screenshot:
- phone's apps streaming policy is disabled.
https://screenshot.googleplex.com/9x5MnxADfzscbad
- before: CrOS's and Phone's enterprise policy are prohibited, we will show two icons.
https://screenshot.googleplex.com/AowVuP28TRjarKo

- after: CrOS's and Phone's enterprise policy are prohibited, we will only show one icon for CrOS's enterprise policy.
https://screenshot.googleplex.com/A867xMH35XTXbw4

Change-Id: I4352fa4683018d5725a8f69efdb8c242665c22b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3613042
Reviewed-by: Daniel Nishi <dhnishi@chromium.org>
Reviewed-by: Jon Mann <jonmann@chromium.org>
Commit-Queue: Mavis Hsu <mavishsu@google.com>
Cr-Commit-Position: refs/heads/main@{#1001373}
  • Loading branch information
mavishsu authored and Chromium LUCI CQ committed May 10, 2022
1 parent 8524364 commit 7e9e6df
Show file tree
Hide file tree
Showing 22 changed files with 342 additions and 42 deletions.
27 changes: 20 additions & 7 deletions ash/webui/eche_app_ui/apps_access_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,8 @@ void AppsAccessManagerImpl::OnSetupRequested() {
void AppsAccessManagerImpl::OnGetAppsAccessStateResponseReceived(
proto::GetAppsAccessStateResponse apps_access_state_response) {
if (apps_access_state_response.result() == proto::Result::RESULT_NO_ERROR) {
AccessStatus access_status =
ComputeAppsAccessState(apps_access_state_response.apps_access_state());
current_apps_access_state_ = apps_access_state_response.apps_access_state();
AccessStatus access_status = ComputeAppsAccessState();
UpdateFeatureEnabledState(access_status);
SetAccessStatusInternal(access_status);
}
Expand All @@ -110,8 +110,8 @@ void AppsAccessManagerImpl::OnGetAppsAccessStateResponseReceived(
void AppsAccessManagerImpl::OnSendAppsSetupResponseReceived(
proto::SendAppsSetupResponse apps_setup_response) {
if (apps_setup_response.result() == proto::Result::RESULT_NO_ERROR) {
AccessStatus access_status =
ComputeAppsAccessState(apps_setup_response.apps_access_state());
current_apps_access_state_ = apps_setup_response.apps_access_state();
AccessStatus access_status = ComputeAppsAccessState();
SetAccessStatusInternal(access_status);

if (access_status == AccessStatus::kAccessGranted) {
Expand All @@ -122,6 +122,16 @@ void AppsAccessManagerImpl::OnSendAppsSetupResponseReceived(
}
}

void AppsAccessManagerImpl::OnAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) {
if (current_app_policy_state_ == app_policy_state)
return;
current_app_policy_state_ = app_policy_state;
AccessStatus access_status = ComputeAppsAccessState();
UpdateFeatureEnabledState(access_status);
SetAccessStatusInternal(access_status);
}

void AppsAccessManagerImpl::OnFeatureStatusChanged() {
UpdateSetupOperationState();
}
Expand Down Expand Up @@ -218,9 +228,12 @@ void AppsAccessManagerImpl::SetAccessStatusInternal(
}
}

AccessStatus AppsAccessManagerImpl::ComputeAppsAccessState(
proto::AppsAccessState apps_access_state) {
if (apps_access_state == proto::AppsAccessState::ACCESS_GRANTED) {
AccessStatus AppsAccessManagerImpl::ComputeAppsAccessState() {
if (current_app_policy_state_ ==
proto::AppStreamingPolicy::APP_POLICY_DISABLED)
return AccessStatus::kProhibited;

if (current_apps_access_state_ == proto::AppsAccessState::ACCESS_GRANTED) {
return AccessStatus::kAccessGranted;
}
return AccessStatus::kAvailableButNotGranted;
Expand Down
8 changes: 7 additions & 1 deletion ash/webui/eche_app_ui/apps_access_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ class AppsAccessManagerImpl
void OnSendAppsSetupResponseReceived(
proto::SendAppsSetupResponse apps_setup_response) override;
void OnStatusChange(proto::StatusChangeType status_change_type) override {}
void OnAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) override;

// FeatureStatusProvider::Observer:
void OnFeatureStatusChanged() override;
Expand All @@ -74,7 +76,7 @@ class AppsAccessManagerImpl
bool IsEligibleForOnboarding(FeatureStatus feature_status) const;
void UpdateSetupOperationState();

AccessStatus ComputeAppsAccessState(proto::AppsAccessState apps_access_state);
AccessStatus ComputeAppsAccessState();

FeatureStatus current_feature_status_;
ConnectionStatus current_connection_status_;
Expand All @@ -85,6 +87,10 @@ class AppsAccessManagerImpl
multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
secure_channel::ConnectionManager* connection_manager_;
bool initialized_ = false;
proto::AppStreamingPolicy current_app_policy_state_ =
proto::AppStreamingPolicy::APP_POLICY_UNKNOWN;
proto::AppsAccessState current_apps_access_state_ =
proto::AppsAccessState::ACCESS_UNKNOWN;
};

} // namespace eche_app
Expand Down
46 changes: 46 additions & 0 deletions ash/webui/eche_app_ui/apps_access_manager_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,11 @@ class AppsAccessManagerImplTest : public testing::Test {
fake_eche_message_receiver_->FakeSendAppsSetupResponse(result, status);
}

void FakeSendAppsPolicyStateChange(
eche_app::proto::AppStreamingPolicy app_policy_state) {
fake_eche_message_receiver_->FakeAppPolicyStateChange(app_policy_state);
}

void SetFeatureStatus(FeatureStatus status) {
fake_feature_status_provider_->SetStatus(status);
}
Expand Down Expand Up @@ -595,5 +600,46 @@ TEST_F(AppsAccessManagerImplTest,
/*success=*/true);
}

TEST_F(AppsAccessManagerImplTest,
ShouleNotEnableEcheFeatureWhenAppsPolicyDisabled) {
// Explicitly disable Phone Hub, all sub feature should be disabled
SetFeatureState(Feature::kPhoneHub, FeatureState::kDisabledByUser);
SetFeatureState(Feature::kEche, FeatureState::kDisabledByUser);
Initialize(AccessStatus::kAvailableButNotGranted);
VerifyAppsAccessGrantedState(AccessStatus::kAvailableButNotGranted);

// Simulate flipping the policy state is disabled.
FakeSendAppsPolicyStateChange(
eche_app::proto::AppStreamingPolicy::APP_POLICY_DISABLED);
// No action after access is granted.
// Simulate flipping the access state granted.
FakeGetAppsAccessStateResponse(
eche_app::proto::Result::RESULT_NO_ERROR,
eche_app::proto::AppsAccessState::ACCESS_GRANTED);

EXPECT_EQ(
0u,
fake_multidevice_setup_client()->NumPendingSetFeatureEnabledStateCalls());
}

TEST_F(AppsAccessManagerImplTest,
SimulateAppsPolicyDisabledShouleDisableEcheFeature) {
SetFeatureState(Feature::kPhoneHub, FeatureState::kEnabledByUser);
SetFeatureState(Feature::kEche, FeatureState::kEnabledByUser);
Initialize(AccessStatus::kAccessGranted);
VerifyAppsAccessGrantedState(AccessStatus::kAccessGranted);

// Test that there is a call to disable kEche when apps polcy state has been
// changed. Simulate flipping the policy state is disabled.
FakeSendAppsPolicyStateChange(
eche_app::proto::AppStreamingPolicy::APP_POLICY_DISABLED);

fake_multidevice_setup_client()->InvokePendingSetFeatureEnabledStateCallback(
/*expected_feature=*/Feature::kEche,
/*expected_enabled=*/false, /*expected_auth_token=*/absl::nullopt,
/*success=*/true);
EXPECT_EQ(1u, GetNumObserverCalls());
}

} // namespace eche_app
} // namespace ash
6 changes: 6 additions & 0 deletions ash/webui/eche_app_ui/eche_message_receiver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -38,5 +38,11 @@ void EcheMessageReceiver::NotifyStatusChange(
observer.OnStatusChange(status_change.type());
}

void EcheMessageReceiver::NotifyAppPolicyStateChange(
proto::PolicyStateChange policy_state_change) {
for (auto& observer : observer_list_)
observer.OnAppPolicyStateChange(policy_state_change.app_policy_state());
}

} // namespace eche_app
} // namespace ash
5 changes: 5 additions & 0 deletions ash/webui/eche_app_ui/eche_message_receiver.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ class EcheMessageReceiver {
proto::SendAppsSetupResponse apps_setup_response) = 0;

virtual void OnStatusChange(proto::StatusChangeType status_change_type) = 0;

// Called when the app policy state changed sent by the remote phone.
virtual void OnAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) = 0;
};

EcheMessageReceiver(const EcheMessageReceiver&) = delete;
Expand All @@ -43,6 +47,7 @@ class EcheMessageReceiver {
void NotifySendAppsSetupResponse(
proto::SendAppsSetupResponse apps_setup_response);
void NotifyStatusChange(proto::StatusChange status_change);
void NotifyAppPolicyStateChange(proto::PolicyStateChange policy_state_change);

private:
friend class FakeEcheMessageReceiver;
Expand Down
2 changes: 2 additions & 0 deletions ash/webui/eche_app_ui/eche_message_receiver_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ void EcheMessageReceiverImpl::OnMessageReceived(const std::string& payload) {
NotifySendAppsSetupResponse(message.apps_setup_response());
} else if (message.has_status_change()) {
NotifyStatusChange(message.status_change());
} else if (message.has_policy_state_change()) {
NotifyAppPolicyStateChange(message.policy_state_change());
}
}

Expand Down
45 changes: 45 additions & 0 deletions ash/webui/eche_app_ui/eche_message_receiver_impl_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,10 @@ class FakeObserver : public EcheMessageReceiver::Observer {

size_t status_change_num_calls() const { return status_change_num_calls_; }

size_t apps_policy_state_change_num_calls() const {
return apps_policy_state_change_num_calls_;
}

proto::GetAppsAccessStateResponse get_last_apps_access_state() const {
return last_apps_access_state_response_;
}
Expand All @@ -37,6 +41,10 @@ class FakeObserver : public EcheMessageReceiver::Observer {
return last_status_change_type_;
}

proto::AppStreamingPolicy get_last_apps_policy_state() const {
return last_apps_policy_state_;
}

// EcheMessageReceiver::Observer:
void OnGetAppsAccessStateResponseReceived(
proto::GetAppsAccessStateResponse apps_access_state_response) override {
Expand All @@ -53,13 +61,21 @@ class FakeObserver : public EcheMessageReceiver::Observer {
++status_change_num_calls_;
}

void OnAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) override {
last_apps_policy_state_ = app_policy_state;
++apps_policy_state_change_num_calls_;
}

private:
size_t apps_access_state_response_num_calls_ = 0;
size_t apps_setup_response_ = 0;
size_t status_change_num_calls_ = 0;
size_t apps_policy_state_change_num_calls_ = 0;
proto::GetAppsAccessStateResponse last_apps_access_state_response_;
proto::SendAppsSetupResponse last_apps_setup_reponse_;
proto::StatusChangeType last_status_change_type_;
proto::AppStreamingPolicy last_apps_policy_state_;
};
} // namespace

Expand Down Expand Up @@ -96,6 +112,10 @@ class EcheMessageReceiverImplTest : public testing::Test {
return fake_observer_.status_change_num_calls();
}

size_t GetNumAppsPolicyStateChangeCalls() const {
return fake_observer_.apps_policy_state_change_num_calls();
}

proto::GetAppsAccessStateResponse GetLastAppsAccessState() const {
return fake_observer_.get_last_apps_access_state();
}
Expand All @@ -108,6 +128,10 @@ class EcheMessageReceiverImplTest : public testing::Test {
return fake_observer_.get_last_status_change_type();
}

proto::AppStreamingPolicy GetAppStreamingPolicyState() const {
return fake_observer_.get_last_apps_policy_state();
}

FakeObserver fake_observer_;
std::unique_ptr<secure_channel::FakeConnectionManager>
fake_connection_manager_;
Expand All @@ -130,6 +154,7 @@ TEST_F(EcheMessageReceiverImplTest, OnGetAppsAccessStateResponseReceived) {
EXPECT_EQ(1u, GetNumAppsAccessStateResponseCalls());
EXPECT_EQ(0u, GetNumAppsSetupResponseCalls());
EXPECT_EQ(0u, GetNumStatusChangeCalls());
EXPECT_EQ(0u, GetNumAppsPolicyStateChangeCalls());
EXPECT_EQ(eche_app::proto::Result::RESULT_ERROR_ACTION_FAILED,
actual_apps_state.result());
EXPECT_EQ(eche_app::proto::AppsAccessState::ACCESS_GRANTED,
Expand All @@ -152,6 +177,7 @@ TEST_F(EcheMessageReceiverImplTest, OnSendAppsSetupResponseReceived) {
EXPECT_EQ(0u, GetNumAppsAccessStateResponseCalls());
EXPECT_EQ(1u, GetNumAppsSetupResponseCalls());
EXPECT_EQ(0u, GetNumStatusChangeCalls());
EXPECT_EQ(0u, GetNumAppsPolicyStateChangeCalls());
EXPECT_EQ(eche_app::proto::Result::RESULT_ERROR_ACTION_FAILED,
actual_apps_setup_response.result());
EXPECT_EQ(eche_app::proto::AppsAccessState::ACCESS_GRANTED,
Expand All @@ -171,8 +197,27 @@ TEST_F(EcheMessageReceiverImplTest, OnStatusChangeReceived) {
EXPECT_EQ(0u, GetNumAppsAccessStateResponseCalls());
EXPECT_EQ(0u, GetNumAppsSetupResponseCalls());
EXPECT_EQ(1u, GetNumStatusChangeCalls());
EXPECT_EQ(0u, GetNumAppsPolicyStateChangeCalls());
EXPECT_EQ(proto::StatusChangeType::TYPE_STREAM_START, status_change_type);
}

TEST_F(EcheMessageReceiverImplTest, OnAppPolicyStateChangeReceived) {
proto::PolicyStateChange policy_state_change;
policy_state_change.set_app_policy_state(
proto::AppStreamingPolicy::APP_POLICY_DISABLED);
proto::ExoMessage message;
*message.mutable_policy_state_change() = std::move(policy_state_change);

fake_connection_manager_->NotifyMessageReceived(message.SerializeAsString());

proto::AppStreamingPolicy app_policy_state = GetAppStreamingPolicyState();

EXPECT_EQ(0u, GetNumAppsAccessStateResponseCalls());
EXPECT_EQ(0u, GetNumAppsSetupResponseCalls());
EXPECT_EQ(0u, GetNumStatusChangeCalls());
EXPECT_EQ(1u, GetNumAppsPolicyStateChangeCalls());
EXPECT_EQ(proto::AppStreamingPolicy::APP_POLICY_DISABLED, app_policy_state);
}

} // namespace eche_app
} // namespace ash
2 changes: 2 additions & 0 deletions ash/webui/eche_app_ui/eche_presence_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ class EchePresenceManager : public FeatureStatusProvider::Observer,
proto::SendAppsSetupResponse apps_setup_response) override {}
void OnGetAppsAccessStateResponseReceived(
proto::GetAppsAccessStateResponse apps_access_state_response) override {}
void OnAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) override {}

void OnReady();
void OnDeviceSeen();
Expand Down
7 changes: 7 additions & 0 deletions ash/webui/eche_app_ui/fake_eche_message_receiver.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,12 @@ void FakeEcheMessageReceiver::FakeStatusChange(
NotifyStatusChange(status_change);
}

void FakeEcheMessageReceiver::FakeAppPolicyStateChange(
proto::AppStreamingPolicy app_policy_state) {
proto::PolicyStateChange policy_state_change;
policy_state_change.set_app_policy_state(app_policy_state);
NotifyAppPolicyStateChange(policy_state_change);
}

} // namespace eche_app
} // namespace ash
3 changes: 3 additions & 0 deletions ash/webui/eche_app_ui/fake_eche_message_receiver.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,10 @@ class FakeEcheMessageReceiver : public EcheMessageReceiver {
proto::AppsAccessState status);
void FakeStatusChange(proto::StatusChangeType status_change_type);

void FakeAppPolicyStateChange(proto::AppStreamingPolicy app_policy_state);

private:
using EcheMessageReceiver::NotifyAppPolicyStateChange;
using EcheMessageReceiver::NotifyGetAppsAccessStateResponse;
using EcheMessageReceiver::NotifySendAppsSetupResponse;
using EcheMessageReceiver::NotifyStatusChange;
Expand Down
11 changes: 11 additions & 0 deletions ash/webui/eche_app_ui/proto/exo_messages.proto
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ message ExoMessage {
GetAppsAccessStateRequest apps_access_state_request = 7;
GetAppsAccessStateResponse apps_access_state_response = 8;
StatusChange status_change = 9;
PolicyStateChange policy_state_change = 10;
}
}

Expand Down Expand Up @@ -72,3 +73,13 @@ enum StatusChangeType {
TYPE_STREAM_START = 1;
TYPE_STREAM_STOP = 2;
}

message PolicyStateChange {
AppStreamingPolicy app_policy_state = 1;
}

enum AppStreamingPolicy {
APP_POLICY_UNKNOWN = 0;
APP_POLICY_DISABLED = 1;
APP_POLICY_ENABLED = 2;
}
3 changes: 3 additions & 0 deletions chrome/app/os_settings_strings.grdp
Original file line number Diff line number Diff line change
Expand Up @@ -3172,6 +3172,9 @@ Press an assigned switch or key to remove assignment.
<message name="IDS_SETTINGS_MULTIDEVICE_NOTIFICATION_ACCESS_PROHIBITED_DISABLED_BY_ADMIN_TOOLTIP" desc="Tooltip shown to users when hovering the help icon in settings next to the Phone Hub notification syncing feature. The tooltip describes how users with notification access disabled by their phones' administrator cannot opt into this feature.">
Notification syncing is disabled by your phone's administrator
</message>
<message name="IDS_SETTINGS_MULTIDEVICE_APPS_ACCESS_PROHIBITED_DISABLED_BY_ADMIN_TOOLTIP" desc="Tooltip shown to users when hovering the help icon in settings next to the Phone Hub apps streaming feature. The tooltip describes how users with apps access disabled by their phones' administrator cannot opt into this feature.">
Apps streaming is disabled by your phone's administrator.
</message>
<message name="IDS_SETTINGS_MULTIDEVICE_NOTIFICATION_ACCESS_SETUP_DIALOG_GET_STARTED_BUTTON_LABEL" desc="The label to a button in the Phone Hub notification opt-in flow that appears when the dialog first opens. Clicking the button will allow the user to try enabling the feature.">
Get started
</message>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
ee2b1e8ad848a471272711fd8ac1dc6056c39743
Original file line number Diff line number Diff line change
Expand Up @@ -186,10 +186,10 @@ export const PhoneHubPermissionsSetupFeatureCombination = {
* isAndroidSmsPairingComplete: boolean,
* cameraRollAccessStatus: !PhoneHubFeatureAccessStatus,
* notificationAccessStatus: !PhoneHubFeatureAccessStatus,
* appsAccessStatus: !PhoneHubFeatureAccessStatus,
* notificationAccessProhibitedReason:
* !PhoneHubFeatureAccessProhibitedReason,
* isNearbyShareDisallowedByPolicy: boolean,
* isPhoneHubAppsAccessGranted: boolean,
* isPhoneHubPermissionsDialogSupported: boolean,
* isCameraRollFilePermissionGranted: boolean,
* isPhoneHubFeatureCombinedSetupSupported: boolean
Expand Down

0 comments on commit 7e9e6df

Please sign in to comment.