Skip to content

Commit

Permalink
Cellular: Start using the last seen network instead of ICCID for sms.
Browse files Browse the repository at this point in the history 
In tests by eng prod it was discovered that on certain devices
the text messages were not being blocked. Upon investigation,
we found out that this happens because sometimes the ICCID
device property is set after the sms device handler has been
initialized which causes the GUID to always be empty.
In this CL we add a code path that starts using the last
connected network instead of ICCID to prevent this issue.

Bug: b:303016846
Test: chromeos_unittest --gtest_filter=*NetworkSmsHandler*
Change-Id: I14e3f6561539b41e850ed93652b34e2d1dfc98ea
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4907733
Reviewed-by: Gordon Seto <gordonseto@google.com>
Reviewed-by: Chad Duffin <chadduffin@chromium.org>
Commit-Queue: Fahad Mansoor <fahadmansoor@google.com>
Cr-Commit-Position: refs/heads/main@{#1209348}
  • Loading branch information
Fahad Mansoor authored and Chromium LUCI CQ committed Oct 13, 2023
1 parent c5effd9 commit a055d85
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 62 deletions.
97 changes: 43 additions & 54 deletions chromeos/ash/components/network/network_sms_handler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include <vector>

#include "ash/constants/ash_features.h"
#include "base/check.h"
#include "base/containers/circular_deque.h"
#include "base/functional/bind.h"
#include "base/logging.h"
Expand All @@ -32,9 +33,9 @@

namespace {

// Key for the device ICCID in message data received from the
// Key for the device GUID in message data received from the
// NetworkSmsDeviceHandler.
const char kDeviceIccidKey[] = "ICCID";
const char kNetworkGuidKey[] = "GUID";

// Maximum number of messages stored for RequestUpdate(true).
const size_t kMaxReceivedMessages = 100;
Expand Down Expand Up @@ -83,6 +84,10 @@ class NetworkSmsHandler::NetworkSmsDeviceHandler {
public:
NetworkSmsDeviceHandler() = default;
virtual ~NetworkSmsDeviceHandler() = default;

// Updates the last connected network's GUID for the current device handler.
// When this is nullptr, the GUID is reset.
virtual void SetLastConnectedNetwork(const NetworkState* state) {}
};

class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
Expand All @@ -92,11 +97,6 @@ class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
const std::string& service_name,
const dbus::ObjectPath& object_path);

ModemManager1NetworkSmsDeviceHandler(NetworkSmsHandler* host,
const std::string& service_name,
const dbus::ObjectPath& object_path,
absl::optional<std::string> iccid);

ModemManager1NetworkSmsDeviceHandler(
const ModemManager1NetworkSmsDeviceHandler&) = delete;
ModemManager1NetworkSmsDeviceHandler& operator=(
Expand All @@ -114,29 +114,26 @@ class NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler
void GetMessages();
void MessageReceived(const base::Value::Dict& dictionary);
void OnFetchSmsDetailsTimeout(const dbus::ObjectPath& sms_path);
void SetLastConnectedNetwork(const NetworkState* state) override;

raw_ptr<NetworkSmsHandler, ExperimentalAsh> host_;
std::string service_name_;
dbus::ObjectPath object_path_;
absl::optional<std::string> iccid_;
base::OneShotTimer fetch_sms_details_timer_;
bool deleting_messages_ = false;
bool retrieving_messages_ = false;
std::vector<dbus::ObjectPath> delete_queue_;
base::circular_deque<dbus::ObjectPath> retrieval_queue_;
std::string last_seen_connected_network_guid_;
base::WeakPtrFactory<ModemManager1NetworkSmsDeviceHandler> weak_ptr_factory_{
this};
};

NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
ModemManager1NetworkSmsDeviceHandler(NetworkSmsHandler* host,
const std::string& service_name,
const dbus::ObjectPath& object_path,
absl::optional<std::string> iccid)
: host_(host),
service_name_(service_name),
object_path_(object_path),
iccid_(iccid) {
const dbus::ObjectPath& object_path)
: host_(host), service_name_(service_name), object_path_(object_path) {
NET_LOG(DEBUG)
<< "SMS handler for " << object_path.value()
<< " created, setting SMS receive handler and fetching existing messages";
Expand All @@ -157,15 +154,6 @@ NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
weak_ptr_factory_.GetWeakPtr()));
}

NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
ModemManager1NetworkSmsDeviceHandler(NetworkSmsHandler* host,
const std::string& service_name,
const dbus::ObjectPath& object_path)
: ModemManager1NetworkSmsDeviceHandler(host,
service_name,
object_path,
/*iccid=*/absl::nullopt) {}

NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
~ModemManager1NetworkSmsDeviceHandler() {
ModemMessagingClient::Get()->ResetSmsReceivedHandler(service_name_,
Expand Down Expand Up @@ -316,8 +304,8 @@ void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::MessageReceived(
new_dictionary.Set(kTimestampKey, *timestamp);
}

if (features::IsSuppressTextMessagesEnabled() && iccid_.has_value()) {
new_dictionary.Set(kDeviceIccidKey, *iccid_);
if (features::IsSuppressTextMessagesEnabled()) {
new_dictionary.Set(kNetworkGuidKey, last_seen_connected_network_guid_);
}

host_->MessageReceived(new_dictionary);
Expand All @@ -330,6 +318,17 @@ void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
GetMessages();
}

void NetworkSmsHandler::ModemManager1NetworkSmsDeviceHandler::
SetLastConnectedNetwork(const NetworkState* network_state) {
CHECK(features::IsSuppressTextMessagesEnabled());
if (!network_state) {
return;
}
NET_LOG(DEBUG) << "Updating last seen network to network with GUID: "
<< network_state->guid();
last_seen_connected_network_guid_ = network_state->guid();
}

///////////////////////////////////////////////////////////////////////////////
// NetworkSmsHandler

Expand All @@ -356,6 +355,7 @@ void NetworkSmsHandler::Init() {
void NetworkSmsHandler::Init(NetworkStateHandler* network_state_handler) {
CHECK(features::IsSuppressTextMessagesEnabled());
network_state_handler_ = network_state_handler;
network_state_handler_observation_.Observe(network_state_handler_);
Init();
}

Expand Down Expand Up @@ -387,6 +387,17 @@ void NetworkSmsHandler::OnPropertyChanged(const std::string& name,
}
}

void NetworkSmsHandler::ActiveNetworksChanged(
const std::vector<const NetworkState*>& active_networks) {
CHECK(features::IsSuppressTextMessagesEnabled());
for (const NetworkState* network : active_networks) {
if (network->type() == shill::kTypeCellular && device_handler_) {
device_handler_->SetLastConnectedNetwork(network);
break;
}
}
}

// Private methods

void NetworkSmsHandler::AddReceivedMessage(const base::Value::Dict& message) {
Expand All @@ -409,7 +420,7 @@ void NetworkSmsHandler::NotifyMessageReceived(
GetStringOptional(message, kTimestampKey)};

const std::string network_guid =
GetNetworkGuid(GetStringOptional(message, kDeviceIccidKey));
GetStringOptional(message, kNetworkGuidKey).value_or(std::string());
if (network_guid.empty()) {
NET_LOG(ERROR) << "Message received with an empty GUID";
}
Expand Down Expand Up @@ -496,13 +507,13 @@ void NetworkSmsHandler::DevicePropertiesCallback(
// Only one active handler is supported. TODO(crbug.com/1239418): Fix.
device_handler_.reset();

device_handler_ = std::make_unique<ModemManager1NetworkSmsDeviceHandler>(
this, *service_name, object_path);

if (features::IsSuppressTextMessagesEnabled()) {
device_handler_ = std::make_unique<ModemManager1NetworkSmsDeviceHandler>(
this, *service_name, object_path,
GetStringOptional(*properties, shill::kIccidProperty));
} else {
device_handler_ = std::make_unique<ModemManager1NetworkSmsDeviceHandler>(
this, *service_name, object_path);
device_handler_->SetLastConnectedNetwork(
network_state_handler_->ConnectedNetworkByType(
NetworkTypePattern::Cellular()));
}
if (!cellular_device_path_.empty()) {
ShillDeviceClient::Get()->RemovePropertyChangedObserver(
Expand Down Expand Up @@ -531,26 +542,4 @@ void NetworkSmsHandler::OnObjectPathChanged(const base::Value& object_path) {
weak_ptr_factory_.GetWeakPtr()));
}

std::string NetworkSmsHandler::GetNetworkGuid(
absl::optional<std::string> iccid) {
if (!iccid.has_value()) {
NET_LOG(ERROR) << "Cannot get the network for a device without an ICCID";
return std::string();
}
NetworkStateHandler::NetworkStateList active_networks;
// We also look at non-active networks, to account for networks that are
// disconnected and may become connected later.
network_state_handler_->GetNetworkListByType(
NetworkTypePattern::Cellular(), /*configured_only=*/false,
/*visible_only=*/false, /*limit=*/0, &active_networks);
for (auto* network : active_networks) {
if (network->iccid() == iccid) {
return network->guid();
}
}
NET_LOG(ERROR) << "Couldn't get find the matching network for ICCID: "
<< *iccid;
return std::string();
}

} // namespace ash
13 changes: 10 additions & 3 deletions chromeos/ash/components/network/network_sms_handler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
#include "base/component_export.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/scoped_observation.h"
#include "base/values.h"
#include "chromeos/ash/components/dbus/shill/shill_property_changed_observer.h"
#include "chromeos/ash/components/network/network_handler.h"
#include "chromeos/ash/components/network/network_state_handler.h"
#include "chromeos/ash/components/network/network_state_handler_observer.h"
#include "chromeos/dbus/common/dbus_method_call_status.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

Expand All @@ -38,7 +40,8 @@ struct COMPONENT_EXPORT(CHROMEOS_NETWORK) TextMessageData {

// Class to watch sms without Libcros.
class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkSmsHandler
: public ShillPropertyChangedObserver {
: public ShillPropertyChangedObserver,
public NetworkStateHandlerObserver {
public:
static const char kNumberKey[];
static const char kTextKey[];
Expand Down Expand Up @@ -73,6 +76,10 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkSmsHandler
void OnPropertyChanged(const std::string& name,
const base::Value& value) override;

// NetworkStateHandlerObserver:
void ActiveNetworksChanged(
const std::vector<const NetworkState*>& active_networks) override;

private:
friend class NetworkHandler;
friend class NetworkSmsHandlerTest;
Expand Down Expand Up @@ -122,13 +129,13 @@ class COMPONENT_EXPORT(CHROMEOS_NETWORK) NetworkSmsHandler
// a new handler should be created for the device's new object path.
void OnObjectPathChanged(const base::Value& object_path);

std::string GetNetworkGuid(absl::optional<std::string> iccid);

base::ObserverList<Observer, true>::Unchecked observers_;
std::unique_ptr<NetworkSmsDeviceHandler> device_handler_;
std::vector<base::Value::Dict> received_messages_;
std::string cellular_device_path_;
raw_ptr<NetworkStateHandler> network_state_handler_ = nullptr;
base::ScopedObservation<NetworkStateHandler, NetworkStateHandlerObserver>
network_state_handler_observation_{this};
base::WeakPtrFactory<NetworkSmsHandler> weak_ptr_factory_{this};
};

Expand Down
56 changes: 52 additions & 4 deletions chromeos/ash/components/network/network_sms_handler_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/run_loop.h"
#include "base/test/scoped_feature_list.h"
#include "base/test/task_environment.h"
#include "base/values.h"
#include "chromeos/ash/components/dbus/shill/fake_sms_client.h"
#include "chromeos/ash/components/dbus/shill/modem_messaging_client.h"
#include "chromeos/ash/components/dbus/shill/shill_clients.h"
Expand Down Expand Up @@ -135,7 +136,7 @@ class NetworkSmsHandlerTest
kCellularDevicePath, shill::kDBusObjectProperty,
base::Value(kCellularDeviceObjectPath1), /*notify_changed=*/false);
SetupCellularModem(kCellularDeviceObjectPath1, kTestCellularServicePath1,
kTestGuid1, kTestIccid1);
kTestGuid1, kTestIccid1, shill::kStateOnline);
modem_messaging_test_ = ModemMessagingClient::Get()->GetTestInterface();

// This relies on the stub dbus implementations for ShillManagerClient,
Expand Down Expand Up @@ -175,20 +176,27 @@ class NetworkSmsHandlerTest
void SetupCellularModem(const std::string& object_path,
const std::string& service_path,
const std::string& guid,
const std::string& iccid) {
const std::string& iccid,
const std::string& state) {
device_test_->SetDeviceProperty(
kCellularDevicePath, shill::kDBusObjectProperty,
base::Value(object_path), /*notify_changed=*/true);
device_test_->SetDeviceProperty(kCellularDevicePath, shill::kIccidProperty,
base::Value(iccid),
/*notify_changed=*/true);
service_test_->AddService(service_path, guid, "", shill::kTypeCellular,
shill::kStateOnline,
state,
/*visible=*/true);
service_test_->SetServiceProperty(service_path, shill::kIccidProperty,
base::Value(iccid));
}

void UpdateNetworkState(const std::string& service_path,
const std::string& state) {
service_test_->SetServiceProperty(service_path, shill::kStateProperty,
base::Value(state));
}

protected:
base::test::SingleThreadTaskEnvironment task_environment_;
raw_ptr<ShillDeviceClient::TestInterface, DanglingUntriaged | ExperimentalAsh>
Expand Down Expand Up @@ -537,8 +545,9 @@ TEST_P(NetworkSmsHandlerSmsSuppressOnlyTest, NetworkGuidTest) {
base::RunLoop().RunUntilIdle();

// Switch to a different modem.
UpdateNetworkState(kTestCellularServicePath1, shill::kStateDisconnect);
SetupCellularModem(kCellularDeviceObjectPath2, kTestCellularServicePath2,
kTestGuid2, kTestIccid2);
kTestGuid2, kTestIccid2, shill::kStateOnline);

base::RunLoop().RunUntilIdle();
network_sms_handler_->RequestUpdate();
Expand All @@ -557,4 +566,43 @@ TEST_P(NetworkSmsHandlerSmsSuppressOnlyTest, NetworkGuidTest) {
test_observer_->messages(kTestGuid1).end());
}

TEST_P(NetworkSmsHandlerSmsSuppressOnlyTest, NetworkDelayedActiveNetworkTest) {
SetupCellularModem(kCellularDeviceObjectPath2, kTestCellularServicePath2,
kTestGuid2, kTestIccid2, shill::kStateIdle);
base::RunLoop().RunUntilIdle();
network_sms_handler_->RequestUpdate();
EXPECT_EQ(0u, test_observer_->messages().size());
ReceiveSms(dbus::ObjectPath(kCellularDeviceObjectPath2),
dbus::ObjectPath(kSmsPath));
CompleteReceiveSms();
base::RunLoop().RunUntilIdle();
// The message will be sent with the GUID of the currently connected
// network which is kTestGuid1.
EXPECT_EQ(1u, test_observer_->messages(kTestGuid1).size());
EXPECT_EQ(0u, test_observer_->messages(kTestGuid2).size());

UpdateNetworkState(kTestCellularServicePath1, shill::kStateDisconnect);
base::RunLoop().RunUntilIdle();
ReceiveSms(dbus::ObjectPath(kCellularDeviceObjectPath2),
dbus::ObjectPath("/SMS/1"));
CompleteReceiveSms();
base::RunLoop().RunUntilIdle();
// No connected network, the GUID of the last connected
// network will be used.
EXPECT_EQ(2u, test_observer_->messages(kTestGuid1).size());
EXPECT_EQ(0u, test_observer_->messages(kTestGuid2).size());

UpdateNetworkState(kTestCellularServicePath2, shill::kStateOnline);
base::RunLoop().RunUntilIdle();

ReceiveSms(dbus::ObjectPath(kCellularDeviceObjectPath2),
dbus::ObjectPath("/SMS/2"));
CompleteReceiveSms();
base::RunLoop().RunUntilIdle();
// After updating the state, we see that the message is sent with the GUID
// associated with the last active network.
EXPECT_EQ(2u, test_observer_->messages(kTestGuid1).size());
EXPECT_EQ(1u, test_observer_->messages(kTestGuid2).size());
}

} // namespace ash
6 changes: 5 additions & 1 deletion chromeos/ash/components/network/text_message_provider_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "chromeos/ash/components/network/mock_managed_network_configuration_handler.h"
#include "chromeos/ash/components/network/network_handler.h"
#include "chromeos/ash/components/network/network_sms_handler.h"
#include "chromeos/ash/components/network/network_state_handler.h"
#include "chromeos/ash/components/network/text_message_suppression_state.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"
Expand Down Expand Up @@ -63,9 +64,10 @@ class TextMessageProviderTest : public testing::Test {
// Initialize shill_client fakes as |network_sms_handler_| depends on them
// during initialization and destruction.
shill_clients::InitializeFakes();
network_state_handler_ = NetworkStateHandler::InitializeForTest();
// Used new as constructor is private.
network_sms_handler_.reset(new NetworkSmsHandler());
network_sms_handler_->Init();
network_sms_handler_->Init(network_state_handler_.get());
provider_ = std::make_unique<TextMessageProvider>();
provider_->Init(network_sms_handler_.get(),
&mock_managed_network_configuration_handler_);
Expand All @@ -81,6 +83,7 @@ class TextMessageProviderTest : public testing::Test {
observation.Reset();
provider_.reset();
network_sms_handler_.reset();
network_state_handler_.reset();
shill_clients::Shutdown();
}

Expand Down Expand Up @@ -146,6 +149,7 @@ class TextMessageProviderTest : public testing::Test {
FakeNetworkMetadataStore fake_network_metadata_store_;
std::unique_ptr<NetworkSmsHandler> network_sms_handler_;
std::unique_ptr<TextMessageProvider> provider_;
std::unique_ptr<NetworkStateHandler> network_state_handler_;
std::unique_ptr<base::HistogramTester> histogram_tester_;
base::test::SingleThreadTaskEnvironment task_environment_;

Expand Down

0 comments on commit a055d85

Please sign in to comment.