Skip to content

Commit

Permalink
Merge pull request #26849 from brave/news-direct-feed-https
Browse files Browse the repository at this point in the history
Upgrade News direct feed requests to HTTPS when possible
  • Loading branch information
DJAndries authored Dec 11, 2024
2 parents 7e1ef51 + 071fe11 commit a4aaf14
Show file tree
Hide file tree
Showing 19 changed files with 249 additions and 34 deletions.
9 changes: 8 additions & 1 deletion browser/brave_news/brave_news_controller_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@

#include "base/no_destructor.h"
#include "brave/browser/brave_ads/ads_service_factory.h"
#include "brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h"
#include "brave/components/brave_news/browser/brave_news_controller.h"
#include "chrome/browser/content_settings/host_content_settings_map_factory.h"
#include "chrome/browser/favicon/favicon_service_factory.h"
#include "chrome/browser/history/history_service_factory.h"
#include "chrome/browser/profiles/incognito_helpers.h"
Expand Down Expand Up @@ -48,6 +50,7 @@ BraveNewsControllerFactory::BraveNewsControllerFactory()
DependsOn(brave_ads::AdsServiceFactory::GetInstance());
DependsOn(FaviconServiceFactory::GetInstance());
DependsOn(HistoryServiceFactory::GetInstance());
DependsOn(HostContentSettingsMapFactory::GetInstance());
}

BraveNewsControllerFactory::~BraveNewsControllerFactory() = default;
Expand All @@ -71,9 +74,13 @@ BraveNewsControllerFactory::BuildServiceInstanceForBrowserContext(
auto* ads_service = brave_ads::AdsServiceFactory::GetForProfile(profile);
auto* history_service = HistoryServiceFactory::GetForProfile(
profile, ServiceAccessType::EXPLICIT_ACCESS);
auto* host_content_settings_map =
HostContentSettingsMapFactory::GetForProfile(profile);
return std::make_unique<BraveNewsController>(
profile->GetPrefs(), favicon_service, ads_service, history_service,
profile->GetURLLoaderFactory());
profile->GetURLLoaderFactory(),
std::make_unique<DirectFeedFetcherDelegateImpl>(
host_content_settings_map));
}

bool BraveNewsControllerFactory::ServiceIsNULLWhileTesting() const {
Expand Down
36 changes: 36 additions & 0 deletions browser/brave_news/direct_feed_fetcher_delegate_impl.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#include "brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h"

#include "brave/browser/brave_browser_process.h"
#include "brave/components/brave_shields/content/browser/brave_shields_util.h"
#include "content/public/browser/browser_thread.h"

namespace brave_news {

DirectFeedFetcherDelegateImpl::DirectFeedFetcherDelegateImpl(
HostContentSettingsMap* host_content_settings_map)
: host_content_settings_map_(host_content_settings_map),
https_upgrade_exceptions_service_(
g_brave_browser_process->https_upgrade_exceptions_service()) {}

DirectFeedFetcherDelegateImpl::~DirectFeedFetcherDelegateImpl() = default;

bool DirectFeedFetcherDelegateImpl::ShouldUpgradeToHttps(const GURL& url) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
if (!host_content_settings_map_ || !https_upgrade_exceptions_service_) {
return true;
}
return brave_shields::ShouldUpgradeToHttps(host_content_settings_map_, url,
https_upgrade_exceptions_service_);
}

base::WeakPtr<DirectFeedFetcher::Delegate>
DirectFeedFetcherDelegateImpl::AsWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}

} // namespace brave_news
45 changes: 45 additions & 0 deletions browser/brave_news/direct_feed_fetcher_delegate_impl.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// Copyright (c) 2024 The Brave Authors. All rights reserved.
// This Source Code Form is subject to the terms of the Mozilla Public
// License, v. 2.0. If a copy of the MPL was not distributed with this file,
// You can obtain one at https://mozilla.org/MPL/2.0/.

#ifndef BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_
#define BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_

#include "brave/components/brave_news/browser/direct_feed_fetcher.h"
#include "url/gurl.h"

namespace https_upgrade_exceptions {
class HttpsUpgradeExceptionsService;
} // namespace https_upgrade_exceptions

class HostContentSettingsMap;

namespace brave_news {

class DirectFeedFetcherDelegateImpl : public DirectFeedFetcher::Delegate {
public:
explicit DirectFeedFetcherDelegateImpl(
HostContentSettingsMap* host_content_settings_map);
~DirectFeedFetcherDelegateImpl() override;

DirectFeedFetcherDelegateImpl(const DirectFeedFetcherDelegateImpl&) = delete;
DirectFeedFetcherDelegateImpl& operator=(
const DirectFeedFetcherDelegateImpl&) = delete;

// Must be called on UI thread
bool ShouldUpgradeToHttps(const GURL& url) override;

base::WeakPtr<DirectFeedFetcher::Delegate> AsWeakPtr() override;

private:
raw_ptr<HostContentSettingsMap> host_content_settings_map_;
raw_ptr<https_upgrade_exceptions::HttpsUpgradeExceptionsService>
https_upgrade_exceptions_service_;

base::WeakPtrFactory<DirectFeedFetcherDelegateImpl> weak_ptr_factory_{this};
};

} // namespace brave_news

#endif // BRAVE_BROWSER_BRAVE_NEWS_DIRECT_FEED_FETCHER_DELEGATE_IMPL_H_
3 changes: 3 additions & 0 deletions browser/brave_news/sources.gni
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,15 @@ brave_browser_brave_news_sources += [
"//brave/browser/brave_news/brave_news_controller_factory.h",
"//brave/browser/brave_news/brave_news_tab_helper.cc",
"//brave/browser/brave_news/brave_news_tab_helper.h",
"//brave/browser/brave_news/direct_feed_fetcher_delegate_impl.cc",
"//brave/browser/brave_news/direct_feed_fetcher_delegate_impl.h",
]

brave_browser_brave_news_deps += [
"//base",
"//brave/components/brave_news/browser",
"//brave/components/brave_news/common",
"//brave/components/brave_shields/content/browser",
"//chrome/browser/profiles:profile",
"//components/keyed_service/content",
"//content/public/browser",
Expand Down
10 changes: 7 additions & 3 deletions components/brave_news/browser/brave_news_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -108,17 +108,20 @@ BraveNewsController::BraveNewsController(
favicon::FaviconService* favicon_service,
brave_ads::AdsService* ads_service,
history::HistoryService* history_service,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
std::unique_ptr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
: favicon_service_(favicon_service),
ads_service_(ads_service),
api_request_helper_(GetNetworkTrafficAnnotationTag(), url_loader_factory),
private_cdn_request_helper_(GetNetworkTrafficAnnotationTag(),
url_loader_factory),
history_service_(history_service),
url_loader_factory_(url_loader_factory),
direct_feed_fetcher_delegate_(std::move(direct_feed_fetcher_delegate)),
pref_manager_(*prefs),
news_metrics_(prefs, pref_manager_),
direct_feed_controller_(url_loader_factory),
direct_feed_controller_(url_loader_factory,
direct_feed_fetcher_delegate_->AsWeakPtr()),
task_runner_(base::ThreadPool::CreateSingleThreadTaskRunner(
{base::MayBlock(), base::TaskPriority::BEST_EFFORT,
base::TaskShutdownBehavior::SKIP_ON_SHUTDOWN})),
Expand Down Expand Up @@ -796,7 +799,8 @@ void BraveNewsController::Prefetch() {

void BraveNewsController::ResetEngine() {
engine_.reset(
new BraveNewsEngine(url_loader_factory_->Clone(), MakeHistoryQuerier()));
new BraveNewsEngine(url_loader_factory_->Clone(), MakeHistoryQuerier(),
direct_feed_fetcher_delegate_->AsWeakPtr()));
}

void BraveNewsController::ConditionallyStartOrStopTimer() {
Expand Down
6 changes: 5 additions & 1 deletion components/brave_news/browser/brave_news_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,9 @@ class BraveNewsController
favicon::FaviconService* favicon_service,
brave_ads::AdsService* ads_service,
history::HistoryService* history_service,
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
std::unique_ptr<DirectFeedFetcher::Delegate>
direct_feed_fetcher_delegate);
~BraveNewsController() override;
BraveNewsController(const BraveNewsController&) = delete;
BraveNewsController& operator=(const BraveNewsController&) = delete;
Expand Down Expand Up @@ -183,6 +185,8 @@ class BraveNewsController
raw_ptr<history::HistoryService> history_service_;
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory_;

std::unique_ptr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate_;

BraveNewsPrefManager pref_manager_;
SubscriptionsSnapshot last_subscriptions_;

Expand Down
10 changes: 6 additions & 4 deletions components/brave_news/browser/brave_news_engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,12 @@ namespace brave_news {
BraveNewsEngine::BraveNewsEngine(
std::unique_ptr<network::PendingSharedURLLoaderFactory>
pending_shared_url_loader_factory,
BackgroundHistoryQuerier history_querier)
BackgroundHistoryQuerier history_querier,
base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
: pending_shared_url_loader_factory_(
std::move(pending_shared_url_loader_factory)),
history_querier_(std::move(history_querier)) {
history_querier_(std::move(history_querier)),
direct_feed_fetcher_delegate_(direct_feed_fetcher_delegate) {
DETACH_FROM_SEQUENCE(sequence_checker_);
}

Expand Down Expand Up @@ -187,7 +189,7 @@ FeedV2Builder* BraveNewsEngine::MaybeFeedV2Builder() {
feed_v2_builder_ = std::make_unique<FeedV2Builder>(
*GetPublishersController(), *GetChannelsController(),
*GetSuggestionsController(), history_querier_,
GetSharedURLLoaderFactory());
GetSharedURLLoaderFactory(), direct_feed_fetcher_delegate_);
}

return feed_v2_builder_.get();
Expand All @@ -202,7 +204,7 @@ FeedController* BraveNewsEngine::MaybeFeedV1Builder() {
if (!feed_controller_) {
feed_controller_ = std::make_unique<FeedController>(
GetPublishersController(), history_querier_,
GetSharedURLLoaderFactory());
GetSharedURLLoaderFactory(), direct_feed_fetcher_delegate_);
}

return feed_controller_.get();
Expand Down
6 changes: 5 additions & 1 deletion components/brave_news/browser/brave_news_engine.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/sequence_checker.h"
#include "base/thread_annotations.h"
#include "brave/components/brave_news/browser/background_history_querier.h"
#include "brave/components/brave_news/browser/direct_feed_fetcher.h"
#include "brave/components/brave_news/common/brave_news.mojom.h"

namespace api_request_helper {
Expand Down Expand Up @@ -49,7 +50,8 @@ class BraveNewsEngine {
explicit BraveNewsEngine(
std::unique_ptr<network::PendingSharedURLLoaderFactory>
pending_shared_url_loader_factory,
BackgroundHistoryQuerier history_querier);
BackgroundHistoryQuerier history_querier,
base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
BraveNewsEngine(const BraveNewsEngine&) = delete;
BraveNewsEngine& operator=(const BraveNewsEngine&) = delete;
~BraveNewsEngine();
Expand Down Expand Up @@ -105,6 +107,8 @@ class BraveNewsEngine {
BackgroundHistoryQuerier history_querier_
GUARDED_BY_CONTEXT(sequence_checker_);

base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate_;

std::unique_ptr<api_request_helper::APIRequestHelper> api_request_helper_
GUARDED_BY_CONTEXT(sequence_checker_);

Expand Down
5 changes: 3 additions & 2 deletions components/brave_news/browser/direct_feed_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,9 @@ DirectFeedController::FindFeedRequest::operator=(
DirectFeedController::FindFeedRequest::~FindFeedRequest() = default;

DirectFeedController::DirectFeedController(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory)
: fetcher_(url_loader_factory) {}
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate)
: fetcher_(url_loader_factory, direct_feed_fetcher_delegate) {}

DirectFeedController::~DirectFeedController() = default;

Expand Down
3 changes: 2 additions & 1 deletion components/brave_news/browser/direct_feed_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ using IsValidCallback =
class DirectFeedController {
public:
explicit DirectFeedController(
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory);
scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory,
base::WeakPtr<DirectFeedFetcher::Delegate> direct_feed_fetcher_delegate);
~DirectFeedController();
DirectFeedController(const DirectFeedController&) = delete;
DirectFeedController& operator=(const DirectFeedController&) = delete;
Expand Down
30 changes: 28 additions & 2 deletions components/brave_news/browser/direct_feed_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -78,10 +78,24 @@ constexpr char kFeedURL[] = "https://example.com/feed";
class BraveNewsDirectFeedControllerTest : public testing::Test {
public:
BraveNewsDirectFeedControllerTest()
: direct_feed_controller_(test_url_loader_factory_.GetSafeWeakWrapper()) {
}
: direct_feed_controller_(test_url_loader_factory_.GetSafeWeakWrapper(),
direct_feed_fetcher_delegate_.AsWeakPtr()) {}

protected:
class MockDirectFeedFetcherDelegate : public DirectFeedFetcher::Delegate {
public:
~MockDirectFeedFetcherDelegate() override = default;

bool ShouldUpgradeToHttps(const GURL& url) override { return true; }

base::WeakPtr<DirectFeedFetcher::Delegate> AsWeakPtr() override {
return weak_ptr_factory_.GetWeakPtr();
}

private:
base::WeakPtrFactory<MockDirectFeedFetcherDelegate> weak_ptr_factory_{this};
};

std::tuple<bool, std::string> VerifyFeedUrl(GURL feed_url) {
return WaitForCallback(base::BindOnce(
&DirectFeedController::VerifyFeedUrl,
Expand All @@ -99,6 +113,7 @@ class BraveNewsDirectFeedControllerTest : public testing::Test {
content::BrowserTaskEnvironment task_environment_;
data_decoder::test::InProcessDataDecoder data_decoder_;
network::TestURLLoaderFactory test_url_loader_factory_;
MockDirectFeedFetcherDelegate direct_feed_fetcher_delegate_;
DirectFeedController direct_feed_controller_;
};

Expand Down Expand Up @@ -141,6 +156,17 @@ TEST_F(BraveNewsDirectFeedControllerTest, CanFindFeedFromFeedURL) {
EXPECT_EQ("Hacker News", feed->feed_title);
}

TEST_F(BraveNewsDirectFeedControllerTest, CanUpgradeToHTTPS) {
// Find an RSS feed
test_url_loader_factory_.AddResponse(kFeedURL, GetBasicFeed());
auto result = FindFeeds(GURL("http://example.com/feed"));

ASSERT_EQ(1u, result.size());
auto feed = std::move(result.at(0));
EXPECT_EQ("http://example.com/feed", feed->feed_url.spec());
EXPECT_EQ("Hacker News", feed->feed_title);
}

TEST_F(BraveNewsDirectFeedControllerTest, CanFindFeedFromPageWithFeedURL) {
// Fetch a page with an RSS feed
test_url_loader_factory_.AddResponse(kPageUrl, GetHtmlPageWithFeed());
Expand Down
Loading

0 comments on commit a4aaf14

Please sign in to comment.