[Safe Browsing] Remove signin deps from SafeBrowsingTokenFetcher

This CL removes the remaining deps on //components/signin from
safe_browsing_token_fetcher.h in order to enable using this interface
in //weblayer, which does not use //components/signin. Namely:

- We remove the consent_level parameter altogether: in production it is
  always set to unconsented.
- We replace the base::Optional<AccessTokenInfo> used for passing the
  access token to clients with just the std::string of the access token
  (empty if no access token was available, e.g., because of an error).
  This is all the state that clients used from the AccessTokenInfo.

There is no behavioral change in this CL.

One subtlety here is the metric computed in
RealtimeUrlLookupService::OnGetAccessToken(). Currently it reports that
a token was fetched if the base::Optional instance has a value. In this
CL it reports that a token was fetched if the access_token string is
non-empty. These cases actually coincide:
- Currently, the base::Optional instance has a value iff the
  the underlying AccessTokenFetcher reports that there was no error in
  the fetch.
- By the documentation of AccessTokenFetcher, the AccessTokenInfo
  returned from its fetch will also have a non-empty token exactly when
  there was no error reported by the fetch.

Bug: 1080748
Change-Id: Ib0309417201c1906a0a4fb8bf71b9bac50d88bab
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2611255
Commit-Queue: Colin Blundell <blundell@chromium.org>
Reviewed-by: Xinghui Lu <xinghuilu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#841498}

chrome/browser/safe_browsing/download_protection/check_client_download_request_base.cc

@@ -461,7 +461,6 @@ void CheckClientDownloadRequestBase::OnGotTabRedirects(
   if (is_enhanced_protection_ && token_fetcher_ &&
       base::FeatureList::IsEnabled(kDownloadRequestWithToken)) {
     token_fetcher_->Start(
-        signin::ConsentLevel::kNotRequired,
         base::BindOnce(&CheckClientDownloadRequestBase::OnGotAccessToken,
                        GetWeakPtr()));
     return;
@@ -471,9 +470,8 @@ void CheckClientDownloadRequestBase::OnGotTabRedirects(
 }
 
 void CheckClientDownloadRequestBase::OnGotAccessToken(
-    base::Optional<signin::AccessTokenInfo> access_token_info) {
-  if (access_token_info.has_value())
-    access_token_ = access_token_info.value().token;
+    const std::string& access_token) {
+  access_token_ = access_token;
   SendRequest();
 }
 

chrome/browser/safe_browsing/download_protection/check_client_download_request_base.h

@@ -153,8 +153,7 @@ class CheckClientDownloadRequestBase {
       DownloadCheckResultReason reason) const = 0;
 
   // Called when |token_fetcher_| has finished fetching the access token.
-  void OnGotAccessToken(
-      base::Optional<signin::AccessTokenInfo> access_token_info);
+  void OnGotAccessToken(const std::string& access_token);
 
   // Called at the request start to determine if we should bailout due to the
   // file being whitelisted by policy

components/safe_browsing/core/browser/BUILD.gn

@@ -79,8 +79,5 @@ source_set("referrer_chain_provider") {
 source_set("token_fetcher") {
   sources = [ "safe_browsing_token_fetcher.h" ]
 
-  deps = [
-    "//base",
-    "//components/signin/public/identity_manager",
-  ]
+  deps = [ "//base" ]
 }

components/safe_browsing/core/browser/safe_browsing_token_fetcher.h

@@ -5,29 +5,24 @@
 #ifndef COMPONENTS_SAFE_BROWSING_CORE_BROWSER_SAFE_BROWSING_TOKEN_FETCHER_H_
 #define COMPONENTS_SAFE_BROWSING_CORE_BROWSER_SAFE_BROWSING_TOKEN_FETCHER_H_
 
-#include <memory>
-
 #include "base/callback.h"
-#include "base/optional.h"
-#include "components/signin/public/identity_manager/access_token_info.h"
-#include "components/signin/public/identity_manager/consent_level.h"
 
 namespace safe_browsing {
 
 // This interface is used to fetch access tokens for communcations with Safe
 // Browsing. It asynchronously returns an access token for the current account
-// (as determined in concrete implementations), or nullopt if an error occurred.
+// (as determined in concrete implementations), or the empty string if no access
+// token is available (e.g., an error occurred).
 // This must be run on the UI thread.
 class SafeBrowsingTokenFetcher {
  public:
-  using Callback =
-      base::OnceCallback<void(base::Optional<signin::AccessTokenInfo>)>;
+  using Callback = base::OnceCallback<void(const std::string& access_token)>;
 
   virtual ~SafeBrowsingTokenFetcher() = default;
 
-  // Begin fetching a token for the account with the given |consent_level|. The
+  // Begin fetching a token for the account. The
   // result will be returned in |callback|. Must be called on the UI thread.
-  virtual void Start(signin::ConsentLevel consent_level, Callback callback) = 0;
+  virtual void Start(Callback callback) = 0;
 };
 
 }  // namespace safe_browsing

components/safe_browsing/core/browser/sync/safe_browsing_primary_account_token_fetcher.cc

@@ -44,18 +44,17 @@ SafeBrowsingPrimaryAccountTokenFetcher::SafeBrowsingPrimaryAccountTokenFetcher(
 SafeBrowsingPrimaryAccountTokenFetcher::
     ~SafeBrowsingPrimaryAccountTokenFetcher() {
   for (auto& id_and_callback : callbacks_) {
-    std::move(id_and_callback.second).Run(base::nullopt);
+    std::move(id_and_callback.second).Run(std::string());
   }
 }
 
 void SafeBrowsingPrimaryAccountTokenFetcher::Start(
-    signin::ConsentLevel consent_level,
     Callback callback) {
   DCHECK(CurrentlyOnThread(ThreadID::UI));
   const int request_id = requests_sent_;
   requests_sent_++;
-  CoreAccountId account_id =
-      identity_manager_->GetPrimaryAccountId(consent_level);
+  CoreAccountId account_id = identity_manager_->GetPrimaryAccountId(
+      signin::ConsentLevel::kNotRequired);
   callbacks_[request_id] = std::move(callback);
   token_fetchers_[request_id] =
       identity_manager_->CreateAccessTokenFetcherForAccount(
@@ -78,20 +77,20 @@ void SafeBrowsingPrimaryAccountTokenFetcher::OnTokenFetched(
   UMA_HISTOGRAM_ENUMERATION("SafeBrowsing.TokenFetcher.ErrorType",
                             error.state(), GoogleServiceAuthError::NUM_STATES);
   if (error.state() == GoogleServiceAuthError::NONE)
-    Finish(request_id, access_token_info);
+    Finish(request_id, access_token_info.token);
   else
-    Finish(request_id, base::nullopt);
+    Finish(request_id, std::string());
 }
 
 void SafeBrowsingPrimaryAccountTokenFetcher::OnTokenTimeout(int request_id) {
-  Finish(request_id, base::nullopt);
+  Finish(request_id, std::string());
 }
 
 void SafeBrowsingPrimaryAccountTokenFetcher::Finish(
     int request_id,
-    base::Optional<signin::AccessTokenInfo> token_info) {
+    const std::string& access_token) {
   if (callbacks_.contains(request_id)) {
-    std::move(callbacks_[request_id]).Run(token_info);
+    std::move(callbacks_[request_id]).Run(access_token);
   }
 
   token_fetchers_.erase(request_id);

components/safe_browsing/core/browser/sync/safe_browsing_primary_account_token_fetcher.h

@@ -10,6 +10,7 @@
 #include "base/containers/flat_map.h"
 #include "base/memory/weak_ptr.h"
 #include "components/safe_browsing/core/browser/safe_browsing_token_fetcher.h"
+#include "components/signin/public/identity_manager/access_token_info.h"
 #include "google_apis/gaia/google_service_auth_error.h"
 
 namespace signin {
@@ -32,15 +33,14 @@ class SafeBrowsingPrimaryAccountTokenFetcher : public SafeBrowsingTokenFetcher {
   ~SafeBrowsingPrimaryAccountTokenFetcher() override;
 
   // SafeBrowsingTokenFetcher:
-  void Start(signin::ConsentLevel consent_level, Callback callback) override;
+  void Start(Callback callback) override;
 
  private:
   void OnTokenFetched(int request_id,
                       GoogleServiceAuthError error,
                       signin::AccessTokenInfo access_token_info);
   void OnTokenTimeout(int request_id);
-  void Finish(int request_id,
-              base::Optional<signin::AccessTokenInfo> token_info);
+  void Finish(int request_id, const std::string& access_token);
 
   // Reference to the identity manager to fetch from.
   signin::IdentityManager* identity_manager_;

components/safe_browsing/core/browser/sync/safe_browsing_primary_account_token_fetcher_unittest.cc

@@ -26,78 +26,49 @@ class SafeBrowsingPrimaryAccountTokenFetcherTest : public ::testing::Test {
 TEST_F(SafeBrowsingPrimaryAccountTokenFetcherTest, Success) {
   identity_test_environment_.MakeUnconsentedPrimaryAccountAvailable(
       "test@example.com");
-  base::Optional<signin::AccessTokenInfo> maybe_account_info;
+  std::string access_token;
   SafeBrowsingPrimaryAccountTokenFetcher fetcher(
       identity_test_environment_.identity_manager());
-  fetcher.Start(signin::ConsentLevel::kNotRequired,
-                base::BindOnce(
-                    [](base::Optional<signin::AccessTokenInfo>* target_info,
-                       base::Optional<signin::AccessTokenInfo> info) {
-                      *target_info = info;
-                    },
-                    &maybe_account_info));
+  fetcher.Start(
+      base::BindOnce([](std::string* target_token,
+                        const std::string& token) { *target_token = token; },
+                     &access_token));
   identity_test_environment_
       .WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
           "token", base::Time::Now());
-  ASSERT_TRUE(maybe_account_info.has_value());
-  EXPECT_EQ(maybe_account_info.value().token, "token");
+  EXPECT_EQ(access_token, "token");
 }
 
 TEST_F(SafeBrowsingPrimaryAccountTokenFetcherTest, Failure) {
   identity_test_environment_.MakeUnconsentedPrimaryAccountAvailable(
       "test@example.com");
-  base::Optional<signin::AccessTokenInfo> maybe_account_info;
+  std::string access_token;
   SafeBrowsingPrimaryAccountTokenFetcher fetcher(
       identity_test_environment_.identity_manager());
-  fetcher.Start(signin::ConsentLevel::kNotRequired,
-                base::BindOnce(
-                    [](base::Optional<signin::AccessTokenInfo>* target_info,
-                       base::Optional<signin::AccessTokenInfo> info) {
-                      *target_info = info;
-                    },
-                    &maybe_account_info));
+  fetcher.Start(
+      base::BindOnce([](std::string* target_token,
+                        const std::string& token) { *target_token = token; },
+                     &access_token));
   identity_test_environment_
       .WaitForAccessTokenRequestIfNecessaryAndRespondWithError(
           GoogleServiceAuthError(GoogleServiceAuthError::CONNECTION_FAILED));
-  ASSERT_FALSE(maybe_account_info.has_value());
+  ASSERT_TRUE(access_token.empty());
 }
 
-TEST_F(SafeBrowsingPrimaryAccountTokenFetcherTest, NoSyncingAccount) {
-  identity_test_environment_.MakeUnconsentedPrimaryAccountAvailable(
-      "test@example.com");
-  base::Optional<signin::AccessTokenInfo> maybe_account_info;
-  SafeBrowsingPrimaryAccountTokenFetcher fetcher(
-      identity_test_environment_.identity_manager());
-  fetcher.Start(signin::ConsentLevel::kSync,
-                base::BindOnce(
-                    [](base::Optional<signin::AccessTokenInfo>* target_info,
-                       base::Optional<signin::AccessTokenInfo> info) {
-                      *target_info = info;
-                    },
-                    &maybe_account_info));
-  identity_test_environment_
-      .WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
-          "token", base::Time::Now());
-  ASSERT_FALSE(maybe_account_info.has_value());
-}
-
-TEST_F(SafeBrowsingPrimaryAccountTokenFetcherTest, SyncSuccess) {
+TEST_F(SafeBrowsingPrimaryAccountTokenFetcherTest,
+       SuccessWithConsentedPrimaryAccount) {
   identity_test_environment_.MakePrimaryAccountAvailable("test@example.com");
-  base::Optional<signin::AccessTokenInfo> maybe_account_info;
+  std::string access_token;
   SafeBrowsingPrimaryAccountTokenFetcher fetcher(
       identity_test_environment_.identity_manager());
-  fetcher.Start(signin::ConsentLevel::kSync,
-                base::BindOnce(
-                    [](base::Optional<signin::AccessTokenInfo>* target_info,
-                       base::Optional<signin::AccessTokenInfo> info) {
-                      *target_info = info;
-                    },
-                    &maybe_account_info));
+  fetcher.Start(
+      base::BindOnce([](std::string* target_token,
+                        const std::string& token) { *target_token = token; },
+                     &access_token));
   identity_test_environment_
       .WaitForAccessTokenRequestIfNecessaryAndRespondWithToken(
           "token", base::Time::Now());
-  ASSERT_TRUE(maybe_account_info.has_value());
-  EXPECT_EQ(maybe_account_info.value().token, "token");
+  EXPECT_EQ(access_token, "token");
 }
 
 }  // namespace safe_browsing

components/safe_browsing/core/realtime/url_lookup_service.cc

@@ -63,7 +63,6 @@ void RealTimeUrlLookupService::GetAccessToken(
     RTLookupRequestCallback request_callback,
     RTLookupResponseCallback response_callback) {
   token_fetcher_->Start(
-      signin::ConsentLevel::kNotRequired,
       base::BindOnce(&RealTimeUrlLookupService::OnGetAccessToken,
                      weak_factory_.GetWeakPtr(), url,
                      std::move(request_callback), std::move(response_callback),
@@ -75,14 +74,12 @@ void RealTimeUrlLookupService::OnGetAccessToken(
     RTLookupRequestCallback request_callback,
     RTLookupResponseCallback response_callback,
     base::TimeTicks get_token_start_time,
-    base::Optional<signin::AccessTokenInfo> access_token_info) {
+    const std::string& access_token) {
   base::UmaHistogramTimes("SafeBrowsing.RT.GetToken.Time",
                           base::TimeTicks::Now() - get_token_start_time);
   base::UmaHistogramBoolean("SafeBrowsing.RT.HasTokenFromFetcher",
-                            access_token_info.has_value());
-  std::string access_token_string =
-      access_token_info.value_or(signin::AccessTokenInfo()).token;
-  SendRequest(url, access_token_string, std::move(request_callback),
+                            !access_token.empty());
+  SendRequest(url, access_token, std::move(request_callback),
               std::move(response_callback));
 }
 

components/safe_browsing/core/realtime/url_lookup_service.h

@@ -84,12 +84,11 @@ class RealTimeUrlLookupService : public RealTimeUrlLookupServiceBase {
   bool ShouldIncludeCredentials() const override;
 
   // Called when the access token is obtained from |token_fetcher_|.
-  void OnGetAccessToken(
-      const GURL& url,
+  void OnGetAccessToken(const GURL& url,
                         RTLookupRequestCallback request_callback,
                         RTLookupResponseCallback response_callback,
                         base::TimeTicks get_token_start_time,
-      base::Optional<signin::AccessTokenInfo> access_token_info);
+                        const std::string& access_token);
 
   // Unowned object used for getting access token when real time url check with
   // token is enabled.