Skip to content

Commit

Permalink
Merge pull request #25596 from brave/fix_invalid_selected_name_crash
Browse files Browse the repository at this point in the history 
Fix vpn's invalid selected name crash
  • Loading branch information
@simonhong
simonhong authored Sep 19, 2024
2 parents 88bf0b8 + 7414d1c commit 5fa2576
Show file tree
Hide file tree
Showing 9 changed files with 106 additions and 39 deletions.
2 changes: 1 addition & 1 deletion components/brave_vpn/browser/brave_vpn_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ void BraveVpnService::UpdatePurchasedStateForSessionExpired(
bool BraveVpnService::IsCurrentRegionSelectedAutomatically(
const brave_vpn::mojom::RegionPtr& region) {
const auto selected_region_name =
local_prefs_->GetString(prefs::kBraveVPNSelectedRegion);
local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2);

if (region->region_precision == brave_vpn::mojom::kRegionPrecisionCountry &&
selected_region_name.empty()) {
Expand Down
37 changes: 35 additions & 2 deletions components/brave_vpn/browser/brave_vpn_service_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,8 +443,8 @@ class BraveVPNServiceTest : public testing::Test {
},
{
"continent": "north-america",
"name": "us-west",
"name-pretty": "USA West"
"name": "us-central",
"name-pretty": "USA - Central"
},
{
"continent": "oceania",
Expand All @@ -454,6 +454,16 @@ class BraveVPNServiceTest : public testing::Test {
])";
}

std::string GetRegionsDataV2() {
return R"([
{
"continent": "north-america",
"name": "na-usa",
"name-pretty": "USA - Central"
}
])";
}

std::string GetTimeZonesData() {
return R"([
{
Expand Down Expand Up @@ -840,6 +850,29 @@ TEST_F(BraveVPNServiceTest, SelectedRegionChangedUpdateWithDeviceRegionTest) {
loop.Run();
}

TEST_F(BraveVPNServiceTest, GetProperRegionFromTimeZoneTest) {
// For initial region list, "us-central" is used for "America/Havana"
// timezone.
SetTestTimezone("America/Havana");

OnFetchRegionList(GetRegionsData(), true);
OnFetchTimezones(GetTimeZonesData(), true);

EXPECT_EQ("us-central", GetDeviceRegion());
EXPECT_EQ("us-central", GetSelectedRegion());

// For region list v2, "na-usa" is used for "America/Havana"
// timezone.
// Clear selected region to set with device region when timezone
// data is fetched.
local_pref_service_.ClearPref(prefs::kBraveVPNSelectedRegionV2);
local_pref_service_.SetInteger(prefs::kBraveVPNRegionListVersion, 2);
OnFetchRegionList(GetRegionsDataV2(), true);
OnFetchTimezones(GetTimeZonesData(), true);
EXPECT_EQ("na-usa", GetDeviceRegion());
EXPECT_EQ("na-usa", GetSelectedRegion());
}

TEST_F(BraveVPNServiceTest, LoadPurchasedStateForAnotherEnvFailed) {
auto development = SetupTestingStoreForEnv(skus::GetDefaultEnvironment());
EXPECT_EQ(skus::GetEnvironmentForDomain(development),
Expand Down
18 changes: 11 additions & 7 deletions components/brave_vpn/browser/connection/brave_vpn_region_data_manager.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "brave/components/brave_vpn/browser/api/brave_vpn_api_helper.h"
#include "brave/components/brave_vpn/browser/connection/brave_vpn_region_data_helper.h"
#include "brave/components/brave_vpn/common/brave_vpn_constants.h"
#include "brave/components/brave_vpn/common/brave_vpn_utils.h"
#include "brave/components/brave_vpn/common/pref_names.h"
#include "components/prefs/pref_service.h"

Expand All @@ -38,8 +39,8 @@ bool BraveVPNRegionDataManager::IsRegionDataReady() const {
return !regions_.empty();
}

void BraveVPNRegionDataManager::SetSelectedRegion(const std::string& name) {
local_prefs_->SetString(prefs::kBraveVPNSelectedRegion, name);
void BraveVPNRegionDataManager::SetSelectedRegion(std::string_view name) {
local_prefs_->SetString(prefs::kBraveVPNSelectedRegionV2, name);

if (selected_region_changed_callback_) {
selected_region_changed_callback_.Run(GetSelectedRegion());
Expand All @@ -51,7 +52,7 @@ std::string BraveVPNRegionDataManager::GetSelectedRegion() const {
CHECK_IS_TEST();
}

auto region_name = local_prefs_->GetString(prefs::kBraveVPNSelectedRegion);
auto region_name = local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2);
if (region_name.empty()) {
// Gives device region if there is no cached selected region.
VLOG(2) << __func__ << " : give device region instead.";
Expand All @@ -66,7 +67,7 @@ std::string BraveVPNRegionDataManager::GetDeviceRegion() const {
return local_prefs_->GetString(prefs::kBraveVPNDeviceRegion);
}

void BraveVPNRegionDataManager::SetDeviceRegion(const std::string& name) {
void BraveVPNRegionDataManager::SetDeviceRegion(std::string_view name) {
local_prefs_->SetString(prefs::kBraveVPNDeviceRegion, name);
}

Expand Down Expand Up @@ -122,10 +123,13 @@ void BraveVPNRegionDataManager::SetDeviceRegionWithTimezone(
}
if (current_time_zone == timezone.GetString()) {
VLOG(2) << "Found default region: " << *region_name;
SetDeviceRegion(*region_name);
// Get new region name as timezone data could use old name.
std::string_view new_name =
GetMigratedNameIfNeeded(local_prefs_, *region_name);
SetDeviceRegion(new_name);
// Use device region as a default selected region.
if (local_prefs_->GetString(prefs::kBraveVPNSelectedRegion).empty()) {
SetSelectedRegion(*region_name);
if (local_prefs_->GetString(prefs::kBraveVPNSelectedRegionV2).empty()) {
SetSelectedRegion(new_name);
}
return;
}
Expand Down
4 changes: 2 additions & 2 deletions components/brave_vpn/browser/connection/brave_vpn_region_data_manager.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ class BraveVPNRegionDataManager {
base::RepeatingCallback<void(bool)> callback) {
region_data_ready_callback_ = callback;
}
void SetSelectedRegion(const std::string& name);
void SetSelectedRegion(std::string_view name);
std::string GetDeviceRegion() const;
std::string GetRegionPrecisionForName(const std::string& name) const;

private:
friend class BraveVPNServiceTest;
friend class SystemVPNConnectionAPIUnitTest;

void SetDeviceRegion(const std::string& name);
void SetDeviceRegion(std::string_view name);

void SetFallbackDeviceRegion();
void SetDeviceRegionWithTimezone(const base::Value::List& timezons_value);
Expand Down
4 changes: 2 additions & 2 deletions components/brave_vpn/browser/connection/ikev2/system_vpn_connection_api_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ TEST_F(SystemVPNConnectionAPIUnitTest, CreateOSVPNEntryWithInvalidInfoTest) {

auto* test_api = GetConnectionAPI();
test_api->CheckConnection();
local_state()->SetString(prefs::kBraveVPNSelectedRegion, "eu-be");
local_state()->SetString(prefs::kBraveVPNSelectedRegionV2, "eu-be");
// Prepare valid connection info.
test_api->OnFetchHostnames("eu-be", kHostNamesTestData, true);
test_api->SetPreventCreationForTesting(true);
Expand All @@ -496,7 +496,7 @@ TEST_F(SystemVPNConnectionAPIUnitTest, NeedsConnectTest) {

// Check ignore Connect() request while connecting or disconnecting is
// in-progress.
local_state()->SetString(prefs::kBraveVPNSelectedRegion, "eu-be");
local_state()->SetString(prefs::kBraveVPNSelectedRegionV2, "eu-be");
test_api->connection_state_ = mojom::ConnectionState::CONNECTING;
test_api->Connect();
EXPECT_EQ(mojom::ConnectionState::CONNECTING, test_api->GetConnectionState());
Expand Down
60 changes: 37 additions & 23 deletions components/brave_vpn/common/brave_vpn_utils.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ void RegisterVPNLocalStatePrefs(PrefRegistrySimple* registry) {
registry->RegisterTimePref(prefs::kBraveVPNRegionListFetchedDate, {});
registry->RegisterStringPref(prefs::kBraveVPNDeviceRegion, "");
registry->RegisterStringPref(prefs::kBraveVPNSelectedRegion, "");
registry->RegisterStringPref(prefs::kBraveVPNSelectedRegionV2, "");
#endif
registry->RegisterStringPref(prefs::kBraveVPNEnvironment,
skus::GetDefaultEnvironment());
Expand All @@ -52,6 +53,27 @@ void RegisterVPNLocalStatePrefs(PrefRegistrySimple* registry) {
#endif
}

// Region name map between v1 and v2.
constexpr auto kV1ToV2Map =
base::MakeFixedFlatMap<std::string_view, std::string_view>(
{{"au-au", "ocn-aus"}, {"eu-at", "eu-at"},
{"eu-be", "eu-be"}, {"sa-brazil", "sa-brz"},
{"ca-east", "na-can"}, {"sa-cl", "sa-cl"},
{"sa-colombia", "sa-co"}, {"eu-cr", "eu-cr"},
{"eu-cz", "eu-cz"}, {"eu-dk", "eu-dk"},
{"eu-fr", "eu-fr"}, {"eu-de", "eu-de"},
{"eu-gr", "eu-gr"}, {"eu-ir", "eu-ie"},
{"eu-italy", "eu-it"}, {"asia-jp", "asia-jp"},
{"sa-mexico", "sa-mx"}, {"eu-nl", "eu-nl"},
{"eu-pl", "eu-pl"}, {"eu-pt", "eu-pt"},
{"eu-ro", "eu-ro"}, {"asia-sg", "asia-sg"},
{"af-za", "af-za"}, {"eu-es", "eu-es"},
{"eu-sweden", "eu-se"}, {"eu-ch", "eu-ch"},
{"us-central", "na-usa"}, {"us-east", "na-usa"},
{"us-mountain", "na-usa"}, {"us-north-west", "na-usa"},
{"us-west", "na-usa"}, {"eu-ua", "eu-ua"},
{"eu-en", "en-en"}});

#if !BUILDFLAG(IS_ANDROID)
void MigrateFromV1ToV2(PrefService* local_prefs) {
const auto selected_region_v1 =
Expand All @@ -65,38 +87,30 @@ void MigrateFromV1ToV2(PrefService* local_prefs) {

// In this migration, selected region name is updated to matched v2's country
// name.
constexpr auto kV1ToV2Map =
base::MakeFixedFlatMap<std::string_view, std::string_view>(
{{"au-au", "ocn-aus"}, {"eu-at", "eu-at"},
{"eu-be", "eu-be"}, {"sa-brazil", "sa-brz"},
{"ca-east", "na-can"}, {"sa-cl", "sa-cl"},
{"sa-colombia", "sa-co"}, {"eu-cr", "eu-cr"},
{"eu-cz", "eu-cz"}, {"eu-dk", "eu-dk"},
{"eu-fr", "eu-fr"}, {"eu-de", "eu-de"},
{"eu-gr", "eu-gr"}, {"eu-ir", "eu-ie"},
{"eu-italy", "eu-it"}, {"asia-jp", "asia-jp"},
{"sa-mexico", "sa-mx"}, {"eu-nl", "eu-nl"},
{"eu-pl", "eu-pl"}, {"eu-pt", "eu-pt"},
{"eu-ro", "eu-ro"}, {"asia-sg", "asia-sg"},
{"af-za", "af-za"}, {"eu-es", "eu-es"},
{"eu-sweden", "eu-se"}, {"eu-ch", "eu-ch"},
{"us-central", "na-usa"}, {"us-east", "na-usa"},
{"us-mountain", "na-usa"}, {"us-north-west", "na-usa"},
{"us-west", "na-usa"}, {"eu-ua", "eu-ua"},
{"eu-en", "en-en"}});
if (kV1ToV2Map.contains(selected_region_v1)) {
local_prefs->SetString(prefs::kBraveVPNSelectedRegion,
local_prefs->SetString(prefs::kBraveVPNSelectedRegionV2,
kV1ToV2Map.at(selected_region_v1));
} else {
// This will not be happened but added for safe during the startup.
local_prefs->ClearPref(prefs::kBraveVPNSelectedRegion);
}

local_prefs->SetInteger(prefs::kBraveVPNRegionListVersion, 2);
}
#endif

} // namespace

std::string_view GetMigratedNameIfNeeded(PrefService* local_prefs,
const std::string& name) {
if (local_prefs->GetInteger(prefs::kBraveVPNRegionListVersion) == 1) {
return name;
}

if (kV1ToV2Map.contains(name)) {
return kV1ToV2Map.at(name);
}

NOTREACHED_NORETURN();
}

bool IsBraveVPNWireguardEnabled(PrefService* local_state) {
if (!IsBraveVPNFeatureEnabled()) {
return false;
Expand Down
3 changes: 3 additions & 0 deletions components/brave_vpn/common/brave_vpn_utils.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,9 @@ std::string GetSubscriberCredential(PrefService* local_prefs);
bool HasValidSkusCredential(PrefService* local_prefs);
std::string GetSkusCredential(PrefService* local_prefs);
bool IsBraveVPNWireguardEnabled(PrefService* local_state);
std::string_view GetMigratedNameIfNeeded(PrefService* local_prefs,
const std::string& name);

#if BUILDFLAG(IS_WIN)
void EnableWireguardIfPossible(PrefService* local_prefs);
#endif // BUILDFLAG(IS_WIN)
Expand Down
8 changes: 6 additions & 2 deletions components/brave_vpn/common/brave_vpn_utils_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,9 +203,13 @@ TEST(BraveVPNUtilsUnitTest, SelectedRegionNameMigration) {
"au-au");
brave_vpn::MigrateLocalStatePrefs(&local_state_pref_service);
EXPECT_EQ("ocn-aus", local_state_pref_service.GetString(
brave_vpn::prefs::kBraveVPNSelectedRegion));
brave_vpn::prefs::kBraveVPNSelectedRegionV2));
EXPECT_EQ(2, local_state_pref_service.GetInteger(
brave_vpn::prefs::kBraveVPNRegionListVersion));

// Check v1's selected region is preserved.
EXPECT_EQ("au-au", local_state_pref_service.GetString(
brave_vpn::prefs::kBraveVPNSelectedRegion));
}

TEST(BraveVPNUtilsUnitTest, InvalidSelectedRegionNameMigration) {
Expand All @@ -218,7 +222,7 @@ TEST(BraveVPNUtilsUnitTest, InvalidSelectedRegionNameMigration) {
"invalid");
brave_vpn::MigrateLocalStatePrefs(&local_state_pref_service);
EXPECT_EQ("", local_state_pref_service.GetString(
brave_vpn::prefs::kBraveVPNSelectedRegion));
brave_vpn::prefs::kBraveVPNSelectedRegionV2));
EXPECT_EQ(2, local_state_pref_service.GetInteger(
brave_vpn::prefs::kBraveVPNRegionListVersion));
}
Expand Down
9 changes: 9 additions & 0 deletions components/brave_vpn/common/pref_names.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,17 @@ inline constexpr char kBraveVPNRegionListFetchedDate[] =
"brave.brave_vpn.region_list_fetched_date";
inline constexpr char kBraveVPNDeviceRegion[] =
"brave.brave_vpn.device_region_name";

// For backward-compatibility, v1's selected region name is preserved.
// If user runs older brave with migrated profile, it could make crash as
// region data list v1 doesn't have entry for v2's selected region name.
// Retaining only this data only is sufficient for backward compatibility
// because region list and timezone data are fetched again as v2 data becomes
// invalid in old browser.
inline constexpr char kBraveVPNSelectedRegion[] =
"brave.brave_vpn.selected_region_name";
inline constexpr char kBraveVPNSelectedRegionV2[] =
"brave.brave_vpn.selected_region_name_v2";
inline constexpr char kBraveVPNRegionListVersion[] =
"brave.brave_vpn.region_list_version";
#if BUILDFLAG(IS_WIN)
Expand Down

0 comments on commit 5fa2576

Please sign in to comment.