diff --git a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc index 3a15bde21c5f4c..0bafcee02fcc66 100644 --- a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc +++ b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.cc @@ -12,6 +12,7 @@ #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/account_consistency_mode_manager.h" #include "chrome/browser/signin/account_reconcilor_factory.h" +#include "chrome/browser/signin/google_accounts_private_api_host.h" #include "chrome/browser/signin/identity_manager_factory.h" #include "chrome/common/chrome_features.h" #include "components/signin/core/browser/account_reconcilor.h" @@ -72,7 +73,8 @@ void GaiaRemoteConsentFlow::Start() { WebAuthFlow::GET_AUTH_TOKEN, extension_name_); #if BUILDFLAG(IS_CHROMEOS_LACROS) // `profile_` may be nullptr in tests. - if (profile_) { + if (profile_ && + !base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { AccountReconcilorFactory::GetForProfile(profile_) ->GetConsistencyCookieManager() ->AddExtraCookieManager(GetCookieManagerForPartition()); @@ -80,11 +82,35 @@ void GaiaRemoteConsentFlow::Start() { #endif } + if (base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { + StartWebFlow(); + return; + } + SetAccountsInCookie(); } +void GaiaRemoteConsentFlow::StartWebFlow() { + network::mojom::CookieManager* cookie_manager = + GetCookieManagerForPartition(); + net::CookieOptions options; + for (const auto& cookie : resolution_data_.cookies) { + cookie_manager->SetCanonicalCookie( + cookie, + net::cookie_util::SimulatedCookieSource(cookie, url::kHttpsScheme), + options, network::mojom::CookieManager::SetCanonicalCookieCallback()); + } + + web_flow_->Start(); + web_flow_started_ = true; +} + void GaiaRemoteConsentFlow::OnSetAccountsComplete( signin::SetAccountsInCookieResult result) { + // No need to inject account cookies when the flow is displayed in a browser + // tab. + DCHECK(!base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)); + set_accounts_in_cookie_task_.reset(); if (web_flow_started_) { return; @@ -96,16 +122,6 @@ void GaiaRemoteConsentFlow::OnSetAccountsComplete( return; } - network::mojom::CookieManager* cookie_manager = - GetCookieManagerForPartition(); - net::CookieOptions options; - for (const auto& cookie : resolution_data_.cookies) { - cookie_manager->SetCanonicalCookie( - cookie, - net::cookie_util::SimulatedCookieSource(cookie, url::kHttpsScheme), - options, network::mojom::CookieManager::SetCanonicalCookieCallback()); - } - identity_api_set_consent_result_subscription_ = IdentityAPI::GetFactoryInstance() ->Get(profile_) @@ -114,18 +130,11 @@ void GaiaRemoteConsentFlow::OnSetAccountsComplete( base::Unretained(this))); scoped_observation_.Observe(IdentityManagerFactory::GetForProfile(profile_)); - web_flow_->Start(); - web_flow_started_ = true; + StartWebFlow(); } -void GaiaRemoteConsentFlow::OnConsentResultSet( - const std::string& consent_result, - const std::string& window_id) { - if (!web_flow_ || window_id != web_flow_->GetAppWindowKey()) - return; - - identity_api_set_consent_result_subscription_ = {}; - +void GaiaRemoteConsentFlow::ReactToConsentResult( + const std::string& consent_result) { bool consent_approved = false; std::string gaia_id; if (!gaia::ParseOAuth2MintTokenConsentResult(consent_result, @@ -143,6 +152,21 @@ void GaiaRemoteConsentFlow::OnConsentResultSet( delegate_->OnGaiaRemoteConsentFlowApproved(consent_result, gaia_id); } +void GaiaRemoteConsentFlow::OnConsentResultSet( + const std::string& consent_result, + const std::string& window_id) { + // JS hook in a browser tab calls `ReactToConsentResult()` directly. + DCHECK(!base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)); + + if (!web_flow_ || window_id != web_flow_->GetAppWindowKey()) { + return; + } + + identity_api_set_consent_result_subscription_ = {}; + + ReactToConsentResult(consent_result); +} + void GaiaRemoteConsentFlow::OnAuthFlowFailure(WebAuthFlow::Failure failure) { GaiaRemoteConsentFlow::Failure gaia_failure; @@ -192,6 +216,9 @@ GaiaRemoteConsentFlow::GetCookieManagerForPartition() { } void GaiaRemoteConsentFlow::OnEndBatchOfRefreshTokenStateChanges() { + // No need to copy added accounts when showing the flow in a browser tab. + DCHECK(!base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)); + // On ChromeOS, new accounts are added through the account manager. They need to // be pushed to the partition used by this flow explicitly. // On Desktop, sign-in happens on the Web and a new account is directly added to @@ -226,6 +253,10 @@ WebAuthFlow* GaiaRemoteConsentFlow::GetWebAuthFlowForTesting() const { } void GaiaRemoteConsentFlow::SetAccountsInCookie() { + // No need to inject account cookies when the flow is displayed in a browser + // tab. + DCHECK(!base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)); + // Reset a task that is already in flight because it contains stale // information. if (set_accounts_in_cookie_task_) @@ -278,7 +309,8 @@ void GaiaRemoteConsentFlow::DetachWebAuthFlow() { #if BUILDFLAG(IS_CHROMEOS_LACROS) // `profile_` may be nullptr in tests. - if (profile_) { + if (profile_ && + !base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { AccountReconcilorFactory::GetForProfile(profile_) ->GetConsistencyCookieManager() ->RemoveExtraCookieManager(GetCookieManagerForPartition()); @@ -287,4 +319,18 @@ void GaiaRemoteConsentFlow::DetachWebAuthFlow() { web_flow_.release()->DetachDelegateAndDelete(); } +void GaiaRemoteConsentFlow::OnNavigationFinished( + content::NavigationHandle* navigation_handle) { + // No need to create the receiver if we are not displaying the auth page + // through a Browser Tgab. + if (!base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { + return; + } + + GoogleAccountsPrivateApiHost::CreateReceiver( + base::BindRepeating(&GaiaRemoteConsentFlow::ReactToConsentResult, + weak_factory.GetWeakPtr()), + navigation_handle); +} + } // namespace extensions diff --git a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h index dbbe0dbe683652..87ae9d59b467c3 100644 --- a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h +++ b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h @@ -7,6 +7,7 @@ #include "base/callback_list.h" #include "base/memory/raw_ptr.h" +#include "base/memory/weak_ptr.h" #include "base/scoped_observation.h" #include "chrome/browser/extensions/api/identity/extension_token_key.h" #include "chrome/browser/extensions/api/identity/web_auth_flow.h" @@ -66,12 +67,19 @@ class GaiaRemoteConsentFlow // Set accounts in cookie completion callback. void OnSetAccountsComplete(signin::SetAccountsInCookieResult result); - // setConsentResult() JavaScript callback. + // setConsentResult() JavaScript callback when using an App Window to display + // the Auth page. void OnConsentResultSet(const std::string& consent_result, const std::string& window_id); + // Handles `consent_result` value when using either a Browser Tab or an App + // Window to display the Auth page. + void ReactToConsentResult(const std::string& consent_result); + // WebAuthFlow::Delegate: void OnAuthFlowFailure(WebAuthFlow::Failure failure) override; + void OnNavigationFinished( + content::NavigationHandle* navigation_handle) override; // signin::AccountsCookieMutator::PartitionDelegate: std::unique_ptr CreateGaiaAuthFetcherForPartition( @@ -87,6 +95,8 @@ class GaiaRemoteConsentFlow WebAuthFlow* GetWebAuthFlowForTesting() const; private: + void StartWebFlow(); + void SetAccountsInCookie(); void GaiaRemoteConsentFlowFailed(Failure failure); @@ -110,6 +120,8 @@ class GaiaRemoteConsentFlow base::ScopedObservation scoped_observation_{this}; + + base::WeakPtrFactory weak_factory{this}; }; } // namespace extensions diff --git a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow_browsertest.cc b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow_browsertest.cc index 4fb52e86870739..352414f4b5b5fd 100644 --- a/chrome/browser/extensions/api/identity/gaia_remote_consent_flow_browsertest.cc +++ b/chrome/browser/extensions/api/identity/gaia_remote_consent_flow_browsertest.cc @@ -4,15 +4,18 @@ #include "chrome/browser/extensions/api/identity/gaia_remote_consent_flow.h" +#include "base/strings/strcat.h" #include "chrome/browser/extensions/api/identity/identity_private_api.h" #include "chrome/browser/profiles/profile.h" #include "chrome/browser/signin/identity_manager_factory.h" #include "chrome/browser/ui/browser.h" +#include "chrome/common/chrome_features.h" #include "chrome/test/base/in_process_browser_test.h" #include "components/signin/public/base/consent_level.h" #include "components/signin/public/identity_manager/identity_manager.h" #include "components/signin/public/identity_manager/identity_test_utils.h" #include "content/public/test/browser_test.h" +#include "content/public/test/browser_test_utils.h" #include "content/public/test/test_navigation_observer.h" #include "extensions/browser/api_test_utils.h" #include "google_apis/gaia/core_account_id.h" @@ -54,9 +57,11 @@ class MockGaiaRemoteConsentFlowDelegate const std::string& gaia_id)); }; -class GaiaRemoteConsentFlowBrowserTest : public InProcessBrowserTest { +class GaiaRemoteConsentFlowParamBrowserTest + : public InProcessBrowserTest, + public testing::WithParamInterface { public: - GaiaRemoteConsentFlowBrowserTest() + GaiaRemoteConsentFlowParamBrowserTest() : fake_gaia_test_server_(net::EmbeddedTestServer::TYPE_HTTPS) { #if BUILDFLAG(IS_CHROMEOS_ASH) std::unique_ptr @@ -74,6 +79,9 @@ class GaiaRemoteConsentFlowBrowserTest : public InProcessBrowserTest { fake_gaia_test_server()->AddDefaultHandlers(GetChromeTestDataDir()); fake_gaia_test_server_.RegisterRequestHandler(base::BindRepeating( &FakeGaia::HandleRequest, base::Unretained(&fake_gaia_))); + + scoped_feature_list_.InitWithFeatureState( + features::kWebAuthFlowInBrowserTab, GetParam()); } void SetUp() override { @@ -148,13 +156,31 @@ class GaiaRemoteConsentFlowBrowserTest : public InProcessBrowserTest { navigation_observer.Wait(); } - void SimulateConsentResult(const std::string& consent_value, - const std::string& window_id) { + void SimulateConsentResult(const std::string& consent_value) { + // When the auth flow is using the browser tab, we are able to properly test + // the JS injected script since we rely on the Gaia Origin to filter out + // unwanted urls, and in the test we are overriding the value of Gaia + // Origin, so we can bypass the filter for testing. + if (base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { + // JS function is properly called but returns nullptr. + ASSERT_EQ(nullptr, content::EvalJs( + flow()->GetWebAuthFlowForTesting()->web_contents(), + "window.OAuthConsent.setConsentResult(\"" + + consent_value + "\")")); + return; + } + + // Since we cannot bypass the filter that is added in the internal extension + // (in it's manifest) we do not directly test the JS function but instead + // the layer right above in the API through + // `IdentityPrivateSetConsentResultFunction`. + std::string consent_result = + "[\"" + consent_value + "\", \"" + + flow()->GetWebAuthFlowForTesting()->GetAppWindowKey() + "\"]"; scoped_refptr func = base::MakeRefCounted(); - ASSERT_TRUE(api_test_utils::RunFunction( - func.get(), "[\"" + consent_value + "\", \"" + window_id + "\"]", - profile())); + ASSERT_TRUE( + api_test_utils::RunFunction(func.get(), consent_result, profile())); } MockGaiaRemoteConsentFlowDelegate& mock() { @@ -176,31 +202,31 @@ class GaiaRemoteConsentFlowBrowserTest : public InProcessBrowserTest { net::EmbeddedTestServer fake_gaia_test_server_; FakeGaia fake_gaia_; + + base::test::ScopedFeatureList scoped_feature_list_; }; -IN_PROC_BROWSER_TEST_F(GaiaRemoteConsentFlowBrowserTest, +IN_PROC_BROWSER_TEST_P(GaiaRemoteConsentFlowParamBrowserTest, SimulateInvalidConsent) { LaunchAndWaitGaiaRemoteConsentFlow(); EXPECT_CALL(mock(), OnGaiaRemoteConsentFlowFailed( GaiaRemoteConsentFlow::Failure::INVALID_CONSENT_RESULT)); - SimulateConsentResult("invalid_consent", - flow()->GetWebAuthFlowForTesting()->GetAppWindowKey()); + SimulateConsentResult("invalid_consent"); } -IN_PROC_BROWSER_TEST_F(GaiaRemoteConsentFlowBrowserTest, SimulateNoGrant) { +IN_PROC_BROWSER_TEST_P(GaiaRemoteConsentFlowParamBrowserTest, SimulateNoGrant) { LaunchAndWaitGaiaRemoteConsentFlow(); EXPECT_CALL(mock(), OnGaiaRemoteConsentFlowFailed( GaiaRemoteConsentFlow::Failure::NO_GRANT)); std::string declined_consent = gaia::GenerateOAuth2MintTokenConsentResult( /*approved=*/false, "consent_not_granted", kGaiaId); - SimulateConsentResult(declined_consent, - flow()->GetWebAuthFlowForTesting()->GetAppWindowKey()); + SimulateConsentResult(declined_consent); } -IN_PROC_BROWSER_TEST_F(GaiaRemoteConsentFlowBrowserTest, +IN_PROC_BROWSER_TEST_P(GaiaRemoteConsentFlowParamBrowserTest, SimulateAccessGranted) { LaunchAndWaitGaiaRemoteConsentFlow(); @@ -208,9 +234,18 @@ IN_PROC_BROWSER_TEST_F(GaiaRemoteConsentFlowBrowserTest, /*approved=*/true, "consent_granted", kGaiaId); EXPECT_CALL(mock(), OnGaiaRemoteConsentFlowApproved(approved_consent, kGaiaId)); - SimulateConsentResult(approved_consent, - flow()->GetWebAuthFlowForTesting()->GetAppWindowKey()); + SimulateConsentResult(approved_consent); } +INSTANTIATE_TEST_SUITE_P( + , + GaiaRemoteConsentFlowParamBrowserTest, + testing::Bool(), + [](const testing::TestParamInfo< + GaiaRemoteConsentFlowParamBrowserTest::ParamType>& info) { + return base::StrCat({"WebAuthFlowInBrowserTab_", + info.param ? "FeatureOn" : "FeatureOff"}); + }); + } // namespace extensions #endif // !BUILDFLAG(IS_CHROMEOS_LACROS) diff --git a/chrome/browser/extensions/api/identity/web_auth_flow.cc b/chrome/browser/extensions/api/identity/web_auth_flow.cc index 9ba26024212a37..1fa41bda051d66 100644 --- a/chrome/browser/extensions/api/identity/web_auth_flow.cc +++ b/chrome/browser/extensions/api/identity/web_auth_flow.cc @@ -136,8 +136,7 @@ void WebAuthFlow::Start() { DCHECK(profile_); DCHECK(!profile_->IsOffTheRecord()); - if (partition_ == WebAuthFlow::Partition::LAUNCH_WEB_AUTH_FLOW && - base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { + if (base::FeatureList::IsEnabled(features::kWebAuthFlowInBrowserTab)) { using_auth_with_browser_tab_ = true; content::WebContents::CreateParams params(profile_); @@ -263,6 +262,11 @@ void WebAuthFlow::CloseInfoBar() { } } +bool WebAuthFlow::IsDisplayingAuthPageInTab() const { + // If web_contents_ is nullptr, then the auth page tab is opened. + return using_auth_with_browser_tab_ && !web_contents_; +} + void WebAuthFlow::BeforeUrlLoaded(const GURL& url) { if (delegate_ && IsObservingProviderWebContents()) delegate_->OnAuthFlowURLChange(url); @@ -276,8 +280,8 @@ void WebAuthFlow::AfterUrlLoaded() { // If `web_contents_` is nullptr, this means that the interactive tab has // already been opened once. - if (delegate_ && using_auth_with_browser_tab_ && - mode_ == WebAuthFlow::INTERACTIVE && web_contents_) { + if (delegate_ && using_auth_with_browser_tab_ && web_contents_ && + mode_ == WebAuthFlow::INTERACTIVE) { chrome::ScopedTabbedBrowserDisplayer browser_displayer(profile_); NavigateParams params(browser_displayer.browser(), std::move(web_contents_)); @@ -325,11 +329,10 @@ void WebAuthFlow::DidStopLoading() { void WebAuthFlow::DidStartNavigation( content::NavigationHandle* navigation_handle) { - // If web_contents_ is nullptr, then the auth page tab is opened. // If the navigation is initiated by the user, the tab will exit the auth // flow screen, this should result in a declined authentication and deleting // the flow. - if (using_auth_with_browser_tab_ && !web_contents_ && + if (IsDisplayingAuthPageInTab() && !navigation_handle->IsRendererInitiated()) { // Stop observing the web contents since it is not part of the flow anymore. WebContentsObserver::Observe(nullptr); @@ -357,6 +360,10 @@ void WebAuthFlow::DidFinishNavigation( if (!navigation_handle->IsInPrimaryMainFrame()) return; + if (delegate_) { + delegate_->OnNavigationFinished(navigation_handle); + } + bool failed = false; if (navigation_handle->GetNetErrorCode() != net::OK) { if (navigation_handle->GetURL().spec() == url::kAboutBlankURL) { diff --git a/chrome/browser/extensions/api/identity/web_auth_flow.h b/chrome/browser/extensions/api/identity/web_auth_flow.h index 554e142326788d..d5011328ef7e0a 100644 --- a/chrome/browser/extensions/api/identity/web_auth_flow.h +++ b/chrome/browser/extensions/api/identity/web_auth_flow.h @@ -78,6 +78,10 @@ class WebAuthFlow : public content::WebContentsObserver, virtual void OnAuthFlowURLChange(const GURL& redirect_url) {} // Called when the title of the current page changes. virtual void OnAuthFlowTitleChange(const std::string& title) {} + // Called when the web_contents associated with the flow has finished + // navigation. + virtual void OnNavigationFinished( + content::NavigationHandle* navigation_handle) {} protected: virtual ~Delegate() {} @@ -149,6 +153,8 @@ class WebAuthFlow : public content::WebContentsObserver, void DisplayInfoBar(); void CloseInfoBar(); + bool IsDisplayingAuthPageInTab() const; + raw_ptr delegate_ = nullptr; const raw_ptr profile_; const GURL provider_url_; diff --git a/chrome/browser/signin/google_accounts_private_api_host.cc b/chrome/browser/signin/google_accounts_private_api_host.cc index 3365b5f59c1f38..226a9129fc377a 100644 --- a/chrome/browser/signin/google_accounts_private_api_host.cc +++ b/chrome/browser/signin/google_accounts_private_api_host.cc @@ -7,10 +7,15 @@ #include "chrome/browser/signin/google_accounts_private_api_util.h" #include "content/public/browser/document_user_data.h" #include "content/public/browser/navigation_handle.h" +#include "content/public/browser/render_frame_host.h" GoogleAccountsPrivateApiHost::GoogleAccountsPrivateApiHost( - content::RenderFrameHost* rfh) - : DocumentUserData(rfh), receiver_(this) {} + content::RenderFrameHost* rfh, + base::RepeatingCallback + on_consent_result_callback) + : DocumentUserData(rfh), + receiver_(this), + on_consent_result_callback_(std::move(on_consent_result_callback)) {} GoogleAccountsPrivateApiHost::~GoogleAccountsPrivateApiHost() = default; @@ -24,8 +29,14 @@ void GoogleAccountsPrivateApiHost::BindReceiver( void GoogleAccountsPrivateApiHost::SetConsentResult( const std::string& consent_result) { - // To be implemented. - // TODO: Add a check for non-primary pages. +#if !BUILDFLAG(IS_ANDROID) + if (!render_frame_host().IsInPrimaryMainFrame() || + !on_consent_result_callback_) { + return; + } + + on_consent_result_callback_.Run(consent_result); +#endif // !BUILDFLAG(IS_ANDROID) } // static @@ -44,6 +55,8 @@ void GoogleAccountsPrivateApiHost::BindHost( // static void GoogleAccountsPrivateApiHost::CreateReceiver( + base::RepeatingCallback + on_consent_result_callback, content::NavigationHandle* navigation_handle) { if (navigation_handle->IsSameDocument()) { return; @@ -51,6 +64,7 @@ void GoogleAccountsPrivateApiHost::CreateReceiver( if (ShouldExposeGoogleAccountsPrivateApi(navigation_handle)) { GoogleAccountsPrivateApiHost::CreateForCurrentDocument( - navigation_handle->GetRenderFrameHost()); + navigation_handle->GetRenderFrameHost(), + std::move(on_consent_result_callback)); } } diff --git a/chrome/browser/signin/google_accounts_private_api_host.h b/chrome/browser/signin/google_accounts_private_api_host.h index 72808ab8eb7f39..9851e7ace6ff2b 100644 --- a/chrome/browser/signin/google_accounts_private_api_host.h +++ b/chrome/browser/signin/google_accounts_private_api_host.h @@ -7,6 +7,7 @@ #include +#include "base/functional/callback.h" #include "chrome/common/google_accounts_private_api_extension.mojom.h" #include "content/public/browser/document_user_data.h" #include "mojo/public/cpp/bindings/associated_receiver.h" @@ -27,7 +28,9 @@ class GoogleAccountsPrivateApiHost GoogleAccountsPrivateApiHost& operator=(const GoogleAccountsPrivateApiHost&) = delete; - static void CreateReceiver(content::NavigationHandle* navigation_handle); + static void CreateReceiver(base::RepeatingCallback + on_consent_result_callback, + content::NavigationHandle* navigation_handle); static void BindHost( mojo::PendingAssociatedReceiver< chrome::mojom::GoogleAccountsPrivateApiExtension> receiver, @@ -40,13 +43,18 @@ class GoogleAccountsPrivateApiHost void SetConsentResult(const std::string& consent_result) override; private: - explicit GoogleAccountsPrivateApiHost(content::RenderFrameHost* rfh); + explicit GoogleAccountsPrivateApiHost( + content::RenderFrameHost* rfh, + base::RepeatingCallback + on_consent_result_callback); friend DocumentUserData; DOCUMENT_USER_DATA_KEY_DECL(); mojo::AssociatedReceiver receiver_; + + base::RepeatingCallback on_consent_result_callback_; }; #endif // CHROME_BROWSER_SIGNIN_GOOGLE_ACCOUNTS_PRIVATE_API_HOST_H_