Skip to content

Commit

Permalink
[ChromeSigninIntercept] Add the new intercept type and trigger
Browse files Browse the repository at this point in the history
Linked to UnoDesktop feature flag.
New intercept type: ChromeSigninIntercept. Triggered when a web signin
occurs with no account signed in in chrome. Only triggers on first
account that signs in.
Extended histogram and histogram values to the existing interecept
records.

Currently displays the profile creation bubble content for debugging
and simplicity. Does not react to the Accept part. Added some TODOs for
the needed changes.

Follow up work:
- Create the Chrome Signin specific URL content and handler.
- React to the Accept part properly.

Bug: b/301431278
Change-Id: I30e0ccb120e341cc27b3bec4f99a132b64955a4f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4926868
Reviewed-by: David Roger <droger@chromium.org>
Commit-Queue: Ryan Sultanem <rsult@google.com>
Reviewed-by: Alex Ilin <alexilin@chromium.org>
Reviewed-by: Gabriel Oliveira <gabolvr@google.com>
Cr-Commit-Position: refs/heads/main@{#1210766}
  • Loading branch information
Ryan Sultanem authored and Chromium LUCI CQ committed Oct 17, 2023
1 parent 8fe017e commit 1c99b66
Show file tree
Hide file tree
Showing 9 changed files with 124 additions and 13 deletions.
62 changes: 56 additions & 6 deletions chrome/browser/signin/dice_web_signin_interceptor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

#include "base/check.h"
#include "base/feature_list.h"
#include "base/functional/callback_helpers.h"
#include "base/hash/hash.h"
#include "base/i18n/case_conversion.h"
#include "base/metrics/field_trial_params.h"
Expand Down Expand Up @@ -37,6 +38,7 @@
#include "chrome/browser/signin/identity_manager_factory.h"
#include "chrome/browser/signin/signin_features.h"
#include "chrome/browser/signin/signin_util.h"
#include "chrome/browser/signin/web_signin_interceptor.h"
#include "chrome/browser/themes/theme_service.h"
#include "chrome/browser/themes/theme_service_factory.h"
#include "chrome/browser/ui/browser_finder.h"
Expand All @@ -56,8 +58,10 @@
#include "components/pref_registry/pref_registry_syncable.h"
#include "components/prefs/pref_service.h"
#include "components/prefs/scoped_user_pref_update.h"
#include "components/signin/public/base/consent_level.h"
#include "components/signin/public/base/signin_metrics.h"
#include "components/signin/public/base/signin_pref_names.h"
#include "components/signin/public/base/signin_switches.h"
#include "components/signin/public/identity_manager/account_managed_status_finder.h"
#include "components/signin/public/identity_manager/accounts_mutator.h"
#include "components/signin/public/identity_manager/identity_manager.h"
Expand Down Expand Up @@ -97,6 +101,40 @@ AccountInfo GetPrimaryAccountInfo(signin::IdentityManager* manager) {
return account_info;
}

bool IsFirstAccount(signin::IdentityManager* manager,
const std::string& email) {
std::vector<CoreAccountInfo> accounts_in_chrome =
manager->GetAccountsWithRefreshTokens();
// There is not guarantee that the added email/account have a refresh token
// yet.
// So we either check that no account exists, or that the only account that
// has a refresh token is the account we are interested in, to make sure it is
// the first account.
return accounts_in_chrome.size() == 0 ||
(accounts_in_chrome.size() == 1 &&
gaia::AreEmailsSame(email, accounts_in_chrome[0].email));
}

bool ShouldShowChromeSigninBubble(signin::IdentityManager* manager,
const std::string& email) {
// The Chrome Signin Bubble is part of the Uno Desktop project.
if (!base::FeatureList::IsEnabled(switches::kUnoDesktop)) {
return false;
}

// Check if an account is already signed in to Chrome.
if (manager->HasPrimaryAccount(signin::ConsentLevel::kSignin)) {
return false;
}

// Only show the Chrome Sign in bubble for the first account being signed in.
if (!IsFirstAccount(manager, email)) {
return false;
}

return true;
}

} // namespace

DiceWebSigninInterceptor::DiceWebSigninInterceptor(
Expand Down Expand Up @@ -153,8 +191,9 @@ DiceWebSigninInterceptor::GetHeuristicOutcome(
// If we do not have all the information to enforce or not enterprise profile
// separation, return `absl::nullopt` so that we can try and get more info on
// the intercepted account.
if (!enforce_enterprise_separation)
if (!enforce_enterprise_separation) {
return absl::nullopt;
}

if (!enforce_enterprise_separation.value()) {
// If interception is disabled abort, unless we need to enforce enterprise
Expand Down Expand Up @@ -185,17 +224,19 @@ DiceWebSigninInterceptor::GetHeuristicOutcome(

DCHECK(signin_interception_enabled && !enforce_enterprise_separation.value());

bool is_first_account = IsFirstAccount(identity_manager_, email);
if (is_first_account &&
ShouldShowChromeSigninBubble(identity_manager_, email)) {
return SigninInterceptionHeuristicOutcome::kInterceptChromeSignin;
}

// From this point the remaining possible interceptions involve creating a new
// profile.
if (!profiles::IsProfileCreationAllowed()) {
return SigninInterceptionHeuristicOutcome::kAbortProfileCreationDisallowed;
}

std::vector<CoreAccountInfo> accounts_in_chrome =
identity_manager_->GetAccountsWithRefreshTokens();
if (accounts_in_chrome.size() == 0 ||
(accounts_in_chrome.size() == 1 &&
gaia::AreEmailsSame(email, accounts_in_chrome[0].email))) {
if (is_first_account) {
// Enterprise and multi-user bubbles are only shown if there are multiple
// accounts. The intercepted account may not be added to chrome yet.
return SigninInterceptionHeuristicOutcome::kAbortSingleAccount;
Expand Down Expand Up @@ -614,6 +655,11 @@ void DiceWebSigninInterceptor::OnInterceptionReadyToBeProcessed(
WebSigninInterceptor::SigninInterceptionType::kProfileSwitch;
RecordSigninInterceptionHeuristicOutcome(
SigninInterceptionHeuristicOutcome::kInterceptProfileSwitch);
} else if (ShouldShowChromeSigninBubble(identity_manager_, info.email)) {
interception_type =
WebSigninInterceptor::SigninInterceptionType::kChromeSignin;
RecordSigninInterceptionHeuristicOutcome(
SigninInterceptionHeuristicOutcome::kInterceptChromeSignin);
} else if (ShouldShowEnterpriseBubble(info)) {
interception_type =
WebSigninInterceptor::SigninInterceptionType::kEnterprise;
Expand Down Expand Up @@ -667,6 +713,10 @@ void DiceWebSigninInterceptor::OnInterceptionReadyToBeProcessed(
base::BindOnce(&DiceWebSigninInterceptor::OnProfileCreationChoice,
base::Unretained(this), info, profile_color);
break;
case WebSigninInterceptor::SigninInterceptionType::kChromeSignin:
// TODO(b/301431278): Attach the right callback here.
callback = base::DoNothing();
break;
}
ShowSigninInterceptionBubble(bubble_parameters, std::move(callback));
}
Expand Down
38 changes: 38 additions & 0 deletions chrome/browser/signin/dice_web_signin_interceptor_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "components/policy/core/common/management/scoped_management_service_override_for_testing.h"
#include "components/prefs/pref_service.h"
#include "components/signin/public/base/signin_pref_names.h"
#include "components/signin/public/base/signin_switches.h"
#include "components/signin/public/identity_manager/account_capabilities_test_mutator.h"
#include "components/signin/public/identity_manager/account_info.h"
#include "components/signin/public/identity_manager/identity_test_environment.h"
Expand Down Expand Up @@ -1583,3 +1584,40 @@ TEST_F(DiceWebSigninInterceptorTest,
testing::_));
task_environment()->FastForwardBy(base::Seconds(5));
}

class DiceWebSigninInterceptorTestWithUnoEnabled
: public DiceWebSigninInterceptorTest {
public:
DiceWebSigninInterceptorTestWithUnoEnabled() {
feature_list_.InitAndEnableFeature(switches::kUnoDesktop);
}

private:
base::test::ScopedFeatureList feature_list_;
};

TEST_F(DiceWebSigninInterceptorTestWithUnoEnabled,
InterceptShouldShowChromeSigninBubbleOnAccountSigninAndChromeSignOut) {
AccountInfo account_info =
identity_test_env()->MakeAccountAvailable("alice@example.com");
MakeValidAccountInfo(&account_info);
identity_test_env()->UpdateAccountInfoForAccount(account_info);

// Account is valid.
ASSERT_TRUE(account_info.IsValid());
// Primary account is not set, Chrome is not signed in.
ASSERT_FALSE(identity_test_env()->identity_manager()->HasPrimaryAccount(
signin::ConsentLevel::kSignin));

WebSigninInterceptor::Delegate::BubbleParameters expected_parameters(
WebSigninInterceptor::SigninInterceptionType::kChromeSignin,
/*intercepted_account=*/account_info,
/*primary_account=*/AccountInfo());
EXPECT_CALL(*mock_delegate(),
ShowSigninInterceptionBubble(
web_contents(), MatchBubbleParameters(expected_parameters),
testing::_));
TestSynchronousInterception(
account_info, /*is_new_account=*/true, /*is_sync_signin=*/false,
SigninInterceptionHeuristicOutcome::kInterceptChromeSignin);
}
7 changes: 2 additions & 5 deletions chrome/browser/signin/web_signin_interceptor.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@

#include "chrome/browser/signin/web_signin_interceptor.h"

#include <string>

#include "base/metrics/histogram_functions.h"
#include "components/signin/public/identity_manager/account_info.h"
#include "components/signin/public/identity_manager/identity_manager.h"

ScopedWebSigninInterceptionBubbleHandle::
~ScopedWebSigninInterceptionBubbleHandle() = default;
Expand All @@ -22,7 +18,8 @@ bool SigninInterceptionHeuristicOutcomeIsSuccess(
outcome ==
SigninInterceptionHeuristicOutcome::kInterceptEnterpriseForced ||
outcome == SigninInterceptionHeuristicOutcome::
kInterceptEnterpriseForcedProfileSwitch;
kInterceptEnterpriseForcedProfileSwitch ||
outcome == SigninInterceptionHeuristicOutcome::kInterceptChromeSignin;
}

WebSigninInterceptor::Delegate::BubbleParameters::BubbleParameters(
Expand Down
11 changes: 9 additions & 2 deletions chrome/browser/signin/web_signin_interceptor.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,13 @@ enum class SigninInterceptionHeuristicOutcome {
// The interceptor is not triggered if the tab has already been closed.
kAbortTabClosed = 18,

kMaxValue = kAbortTabClosed,
// Interception succeeded:
// Interception happens when the first account signs in to the web and no
// account is yet signed in to the Chrome Profile, the prompt suggests signing
// in to Chrome.
kInterceptChromeSignin = 19,

kMaxValue = kInterceptChromeSignin,
};

// User selection in the interception bubble.
Expand Down Expand Up @@ -124,7 +130,8 @@ class WebSigninInterceptor {
kMultiUser,
kEnterpriseForced,
kEnterpriseAcceptManagement,
kProfileSwitchForced
kProfileSwitchForced,
kChromeSignin,
};

// Delegate class responsible for showing the various interception UIs.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,10 +118,20 @@ void DiceWebSigninInterceptionBubbleView::RecordInterceptionResult(
case WebSigninInterceptor::SigninInterceptionType::kProfileSwitchForced:
histogram_base_name.append(".Switch");
break;
case WebSigninInterceptor::SigninInterceptionType::kChromeSignin:
histogram_base_name.append(".ChromeSignin");
break;
}

// Record aggregated histogram for each interception type.
base::UmaHistogramEnumeration(histogram_base_name, result);

// No further sub histogram to record with Chrome Signin intercept.
if (bubble_parameters.interception_type ==
WebSigninInterceptor::SigninInterceptionType::kChromeSignin) {
return;
}

// Record histogram sliced by Sync status.
signin::IdentityManager* identity_manager =
IdentityManagerFactory::GetForProfile(profile);
Expand Down Expand Up @@ -184,6 +194,7 @@ DiceWebSigninInterceptionBubbleView::DiceWebSigninInterceptionBubbleView(
// Create the web view in the native bubble.
std::unique_ptr<views::WebView> web_view =
std::make_unique<views::WebView>(browser->profile());
// TODO(b/301431278): Use the new URL for the Chrome Signin intercept.
web_view->LoadInitialURL(GURL(chrome::kChromeUIDiceWebSigninInterceptURL));
web_view->GetWebContents()->SetDelegate(this);
web_view->SetPreferredSize(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,10 @@ std::string DiceWebSigninInterceptHandler::GetBodyText() {
case WebSigninInterceptor::SigninInterceptionType::kProfileSwitchForced:
NOTREACHED() << "This interception type is not handled by a bubble";
return std::string();
case WebSigninInterceptor::SigninInterceptionType::kChromeSignin:
// TODO(b/301431278): Add NOTREACHED() when the Chrome Signin UI is
// created.
return std::string();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ void DiceWebSigninInterceptUI::Initialize(
base::OnceCallback<void(int)> show_widget_with_height_callback,
base::OnceCallback<void(SigninInterceptionUserChoice)>
completion_callback) {
// TODO(b/301431278): Create a different handler for the Chrome Signin
// intercept UI.
web_ui()->AddMessageHandler(std::make_unique<DiceWebSigninInterceptHandler>(
bubble_parameters, std::move(show_widget_with_height_callback),
std::move(completion_callback)));
Expand Down
1 change: 1 addition & 0 deletions tools/metrics/histograms/enums.xml
Original file line number Diff line number Diff line change
Expand Up @@ -100386,6 +100386,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="16" label="Intercept: forced enterprise interception"/>
<int value="17" label="Intercept: forced enterprise profile switch"/>
<int value="18" label="Abort: tab closed"/>
<int value="19" label="Intercept: chrome sign in"/>
</enum>

<enum name="SigninInterceptResult">
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4997,6 +4997,7 @@ chromium-metrics-reviews@google.com.
</histogram_suffixes>

<histogram_suffixes name="SigninInterceptType" separator=".">
<suffix name="ChromeSignin" label="ChromeSignin bubble."/>
<suffix name="Enterprise" label="Enterprise bubble."/>
<suffix name="MultiUser" label="MultiUser bubble."/>
<suffix name="Switch" label="Profile-switch bubble."/>
Expand Down

0 comments on commit 1c99b66

Please sign in to comment.