From 4aaff0b8c3bd58df9c1f5864aee7bc156a326102 Mon Sep 17 00:00:00 2001 From: Darnell Andries Date: Wed, 21 Aug 2024 21:04:08 -0700 Subject: [PATCH] Address Web Discovery feedback --- .../web_discovery_service_factory.cc | 20 +++++------ .../web_discovery_service_factory.h | 2 ++ components/web_discovery/browser/BUILD.gn | 2 +- .../browser/background_credential_helper.cc | 15 ++++----- .../browser/background_credential_helper.h | 17 +++++----- .../browser/credential_manager.cc | 33 ++++++++----------- .../browser/credential_manager.h | 18 +++++----- .../browser/credential_manager_unittest.cc | 12 +++---- .../web_discovery/browser/credential_signer.h | 6 ++-- 9 files changed, 58 insertions(+), 67 deletions(-) diff --git a/browser/web_discovery/web_discovery_service_factory.cc b/browser/web_discovery/web_discovery_service_factory.cc index f6d0d29f2315..7da05aa16e7e 100644 --- a/browser/web_discovery/web_discovery_service_factory.cc +++ b/browser/web_discovery/web_discovery_service_factory.cc @@ -17,17 +17,6 @@ namespace web_discovery { -namespace { - -ProfileSelections GetProfileSelections() { - if (!base::FeatureList::IsEnabled(features::kBraveWebDiscoveryNative)) { - return ProfileSelections::BuildNoProfilesSelected(); - } - return ProfileSelections::BuildForRegularProfile(); -} - -} // namespace - WebDiscoveryService* WebDiscoveryServiceFactory::GetForBrowserContext( content::BrowserContext* context) { return static_cast( @@ -41,10 +30,17 @@ WebDiscoveryServiceFactory* WebDiscoveryServiceFactory::GetInstance() { WebDiscoveryServiceFactory::WebDiscoveryServiceFactory() : ProfileKeyedServiceFactory("WebDiscoveryService", - GetProfileSelections()) {} + CreateProfileSelections()) {} WebDiscoveryServiceFactory::~WebDiscoveryServiceFactory() = default; +ProfileSelections WebDiscoveryServiceFactory::CreateProfileSelections() { + if (!base::FeatureList::IsEnabled(features::kBraveWebDiscoveryNative)) { + return ProfileSelections::BuildNoProfilesSelected(); + } + return ProfileSelections::BuildForRegularProfile(); +} + KeyedService* WebDiscoveryServiceFactory::BuildServiceInstanceFor( content::BrowserContext* context) const { auto* default_storage_partition = context->GetDefaultStoragePartition(); diff --git a/browser/web_discovery/web_discovery_service_factory.h b/browser/web_discovery/web_discovery_service_factory.h index 8e914b91b039..81bfeeb2a993 100644 --- a/browser/web_discovery/web_discovery_service_factory.h +++ b/browser/web_discovery/web_discovery_service_factory.h @@ -29,6 +29,8 @@ class WebDiscoveryServiceFactory : public ProfileKeyedServiceFactory { WebDiscoveryServiceFactory& operator=(const WebDiscoveryServiceFactory&) = delete; + static ProfileSelections CreateProfileSelections(); + KeyedService* BuildServiceInstanceFor( content::BrowserContext* context) const override; bool ServiceIsCreatedWithBrowserContext() const override; diff --git a/components/web_discovery/browser/BUILD.gn b/components/web_discovery/browser/BUILD.gn index 16e4467c5f15..e315bcd16884 100644 --- a/components/web_discovery/browser/BUILD.gn +++ b/components/web_discovery/browser/BUILD.gn @@ -28,7 +28,7 @@ component("browser") { "web_discovery_service.h", ] deps = [ - "anonymous_credentials/rs/cxx:rust_lib", + "anonymous_credentials/rust:rust_lib", "//base", "//brave/brave_domains", "//brave/components/constants", diff --git a/components/web_discovery/browser/background_credential_helper.cc b/components/web_discovery/browser/background_credential_helper.cc index 8eaf90135f13..28799eb972cb 100644 --- a/components/web_discovery/browser/background_credential_helper.cc +++ b/components/web_discovery/browser/background_credential_helper.cc @@ -62,9 +62,9 @@ BackgroundCredentialHelper::GenerateJoinRequest(std::string pre_challenge) { std::optional BackgroundCredentialHelper::FinishJoin( std::string date, - std::vector group_pub_key, - std::vector gsk, - std::vector join_resp_bytes) { + std::vector group_pub_key, + std::vector gsk, + std::vector join_resp_bytes) { base::AssertLongCPUWorkAllowed(); auto pub_key_result = anonymous_credentials::load_group_public_key( base::SpanToRustSlice(group_pub_key)); @@ -93,10 +93,9 @@ std::optional BackgroundCredentialHelper::FinishJoin( return base::Base64Encode(finish_res.data); } -std::optional> -BackgroundCredentialHelper::PerformSign( - std::vector msg, - std::vector basename, +std::optional> BackgroundCredentialHelper::PerformSign( + std::vector msg, + std::vector basename, std::optional> gsk_bytes, std::optional> credential_bytes) { base::AssertLongCPUWorkAllowed(); @@ -122,7 +121,7 @@ BackgroundCredentialHelper::PerformSign( VLOG(1) << "Failed to sign: " << sig_res.error_message.c_str(); return std::nullopt; } - return std::vector(sig_res.data.begin(), sig_res.data.end()); + return std::vector(sig_res.data.begin(), sig_res.data.end()); } } // namespace web_discovery diff --git a/components/web_discovery/browser/background_credential_helper.h b/components/web_discovery/browser/background_credential_helper.h index 970efe823191..62bc71db81c9 100644 --- a/components/web_discovery/browser/background_credential_helper.h +++ b/components/web_discovery/browser/background_credential_helper.h @@ -11,7 +11,7 @@ #include #include -#include "brave/components/web_discovery/browser/anonymous_credentials/rs/cxx/src/lib.rs.h" +#include "brave/components/web_discovery/browser/anonymous_credentials/rust/src/lib.rs.h" #include "brave/components/web_discovery/browser/rsa.h" #include "crypto/rsa_private_key.h" @@ -37,14 +37,13 @@ class BackgroundCredentialHelper { void SetRSAKey(std::unique_ptr rsa_private_key); std::optional GenerateJoinRequest( std::string pre_challenge); - std::optional FinishJoin( - std::string date, - std::vector group_pub_key, - std::vector gsk, - std::vector join_resp_bytes); - std::optional> PerformSign( - std::vector msg, - std::vector basename, + std::optional FinishJoin(std::string date, + std::vector group_pub_key, + std::vector gsk, + std::vector join_resp_bytes); + std::optional> PerformSign( + std::vector msg, + std::vector basename, std::optional> gsk_bytes, std::optional> credential_bytes); diff --git a/components/web_discovery/browser/credential_manager.cc b/components/web_discovery/browser/credential_manager.cc index 495a314e35bf..7165a55dd507 100644 --- a/components/web_discovery/browser/credential_manager.cc +++ b/components/web_discovery/browser/credential_manager.cc @@ -143,9 +143,6 @@ void CredentialManager::JoinGroups() { void CredentialManager::StartJoinGroup( const std::string& date, const std::vector& group_pub_key) { - std::vector group_pub_key_const(group_pub_key.begin(), - group_pub_key.end()); - auto challenge_elements = base::Value::List::with_capacity(2); challenge_elements.Append(*rsa_public_key_b64_); challenge_elements.Append(base::Base64Encode(group_pub_key)); @@ -158,12 +155,12 @@ void CredentialManager::StartJoinGroup( .WithArgs(pre_challenge) .Then(base::BindOnce(&CredentialManager::OnJoinRequestReady, weak_ptr_factory_.GetWeakPtr(), date, - group_pub_key_const)); + group_pub_key)); } void CredentialManager::OnJoinRequestReady( std::string date, - std::vector group_pub_key, + std::vector group_pub_key, std::optional generate_join_result) { if (!generate_join_result) { return; @@ -183,9 +180,9 @@ void CredentialManager::OnJoinRequestReady( return; } - auto gsk = std::vector( - generate_join_result->start_join_result.gsk.begin(), - generate_join_result->start_join_result.gsk.end()); + auto gsk = + std::vector(generate_join_result->start_join_result.gsk.begin(), + generate_join_result->start_join_result.gsk.end()); auto resource_request = CreateResourceRequest(join_url_); resource_request->headers.SetHeader(kVersionHeader, @@ -207,8 +204,8 @@ void CredentialManager::OnJoinRequestReady( void CredentialManager::OnJoinResponse( std::string date, - std::vector group_pub_key, - std::vector gsk, + std::vector group_pub_key, + std::vector gsk, std::optional response_body) { bool result = ProcessJoinResponse(date, group_pub_key, gsk, response_body); if (!result) { @@ -235,8 +232,8 @@ void CredentialManager::HandleJoinResponseStatus(const std::string& date, bool CredentialManager::ProcessJoinResponse( const std::string& date, - const std::vector& group_pub_key, - const std::vector& gsk, + const std::vector& group_pub_key, + const std::vector& gsk, const std::optional& response_body) { CHECK(join_url_loaders_[date]); auto& url_loader = join_url_loaders_[date]; @@ -272,12 +269,10 @@ bool CredentialManager::ProcessJoinResponse( VLOG(1) << "Failed to decode join response base64"; return false; } - std::vector join_resp_bytes_const(join_resp_bytes->begin(), - join_resp_bytes->end()); background_credential_helper_ .AsyncCall(&BackgroundCredentialHelper::FinishJoin) - .WithArgs(date, group_pub_key, gsk, join_resp_bytes_const) + .WithArgs(date, group_pub_key, gsk, *join_resp_bytes) .Then(base::BindOnce(&CredentialManager::OnCredentialsReady, weak_ptr_factory_.GetWeakPtr(), date, gsk)); return true; @@ -285,7 +280,7 @@ bool CredentialManager::ProcessJoinResponse( void CredentialManager::OnCredentialsReady( std::string date, - std::vector gsk, + std::vector gsk, std::optional credentials) { if (!credentials) { HandleJoinResponseStatus(date, false); @@ -303,8 +298,8 @@ bool CredentialManager::CredentialExistsForToday() { .contains(FormatServerDate(base::Time::Now())); } -void CredentialManager::Sign(std::vector msg, - std::vector basename, +void CredentialManager::Sign(std::vector msg, + std::vector basename, SignCallback callback) { auto today_date = FormatServerDate(base::Time::Now().UTCMidnight()); const auto& anon_creds_dict = @@ -345,7 +340,7 @@ void CredentialManager::Sign(std::vector msg, void CredentialManager::OnSignResult( std::string credential_date, SignCallback callback, - std::optional> signed_message) { + std::optional> signed_message) { loaded_credential_date_ = credential_date; std::move(callback).Run(signed_message); } diff --git a/components/web_discovery/browser/credential_manager.h b/components/web_discovery/browser/credential_manager.h index 1fa9d9eb4706..c54a7b3e8437 100644 --- a/components/web_discovery/browser/credential_manager.h +++ b/components/web_discovery/browser/credential_manager.h @@ -56,8 +56,8 @@ class CredentialManager : public CredentialSigner { // CredentialSigner: bool CredentialExistsForToday() override; - void Sign(std::vector msg, - std::vector basename, + void Sign(std::vector msg, + std::vector basename, SignCallback callback) override; // Uses a fixed seed in the anonymous credential manager @@ -75,25 +75,25 @@ class CredentialManager : public CredentialSigner { void OnJoinRequestReady( std::string date, - std::vector group_pub_key, + std::vector group_pub_key, std::optional generate_join_result); void OnJoinResponse(std::string date, - std::vector group_pub_key, - std::vector gsk, + std::vector group_pub_key, + std::vector gsk, std::optional response_body); void HandleJoinResponseStatus(const std::string& date, bool result); bool ProcessJoinResponse(const std::string& date, - const std::vector& group_pub_key, - const std::vector& gsk, + const std::vector& group_pub_key, + const std::vector& gsk, const std::optional& response_body); void OnCredentialsReady(std::string date, - std::vector gsk, + std::vector gsk, std::optional credentials); void OnSignResult(std::string credential_date, SignCallback callback, - std::optional> signed_message); + std::optional> signed_message); const raw_ptr profile_prefs_; const raw_ptr shared_url_loader_factory_; diff --git a/components/web_discovery/browser/credential_manager_unittest.cc b/components/web_discovery/browser/credential_manager_unittest.cc index d3efa10710f1..a185f59fdb37 100644 --- a/components/web_discovery/browser/credential_manager_unittest.cc +++ b/components/web_discovery/browser/credential_manager_unittest.cc @@ -162,12 +162,12 @@ TEST_F(WebDiscoveryCredentialManagerTest, LoadKeysFromStorage) { } TEST_F(WebDiscoveryCredentialManagerTest, Sign) { - std::vector message({0, 1, 2, 3, 4}); - std::vector basename({5, 6, 7, 8, 9}); + std::vector message({0, 1, 2, 3, 4}); + std::vector basename({5, 6, 7, 8, 9}); credential_manager_->Sign( message, basename, base::BindLambdaForTesting( - [&](const std::optional> signature) { + [&](const std::optional> signature) { EXPECT_FALSE(signature); })); task_environment_.RunUntilIdle(); @@ -175,12 +175,12 @@ TEST_F(WebDiscoveryCredentialManagerTest, Sign) { credential_manager_->JoinGroups(); task_environment_.RunUntilIdle(); - base::flat_set> signatures; + base::flat_set> signatures; for (size_t i = 0; i < 3; i++) { credential_manager_->Sign( message, basename, base::BindLambdaForTesting( - [&](const std::optional> signature) { + [&](const std::optional> signature) { ASSERT_TRUE(signature); EXPECT_FALSE(signature->empty()); EXPECT_FALSE(signatures.contains(*signature)); @@ -191,7 +191,7 @@ TEST_F(WebDiscoveryCredentialManagerTest, Sign) { credential_manager_->Sign( message, basename, base::BindLambdaForTesting( - [&](const std::optional> signature) { + [&](const std::optional> signature) { EXPECT_FALSE(signature); })); task_environment_.RunUntilIdle(); diff --git a/components/web_discovery/browser/credential_signer.h b/components/web_discovery/browser/credential_signer.h index fd79c67a894c..59b67d08a778 100644 --- a/components/web_discovery/browser/credential_signer.h +++ b/components/web_discovery/browser/credential_signer.h @@ -16,7 +16,7 @@ namespace web_discovery { class CredentialSigner { public: using SignCallback = - base::OnceCallback>)>; + base::OnceCallback>)>; virtual ~CredentialSigner() = default; // Returns true is a credential is available for the current date. @@ -29,8 +29,8 @@ class CredentialSigner { // preventing Sybil attacks. // See signature_basename.h/cc for more information on how the basename // should be generated. - virtual void Sign(std::vector msg, - std::vector basename, + virtual void Sign(std::vector msg, + std::vector basename, SignCallback callback) = 0; };