0

[FedCM] Pass filtered out accounts to UI behind a flag

The filtered out accounts are not filtered in the case where the user
just logged in to the IDP and there are no new unfiltered accounts. This
includes the case where all accounts are filtered out. On desktop, the
HoverButton is disabled, but the UI still needs to be updated. On
Android, the UI is not updated at all, but it is behind the flag.
Followups will implement proper disabled accounts UI on both.

Bug: 40945672
Change-Id: Ie436542d12b87b17c461102edfedad85066ecf83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5867640
Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
Reviewed-by: Rakina Zata Amni <rakina@chromium.org>
Auto-Submit: Nicolás Peña <npm@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1372517}
This commit is contained in:
Nicolás Peña
2024-10-23 06:14:45 +00:00
committed by Chromium LUCI CQ
parent 558bc5ef5d
commit d024421bbe
11 changed files with 92 additions and 23 deletions

@ -215,6 +215,8 @@ bool AccountSelectionViewAndroid::Show(
// Serialize the `idp_list` and `accounts` into a Java array and
// instruct the bridge to show it together with |url| to the user.
// TODO(crbug.com/40945672): render filtered out accounts differently on
// Android.
JNIEnv* env = AttachCurrentThread();
ScopedJavaLocalRef<jobjectArray> accounts_obj =
ConvertToJavaAccounts(env, accounts);

@ -284,6 +284,7 @@ void AccountSelectionBubbleView::ShowVerifyingSheet(
row->SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kVertical,
gfx::Insets::VH(kTopBottomPadding, kLeftRightPadding)));
CHECK(!account.is_filtered_out);
row->AddChildView(CreateAccountRow(account,
/*clickable_position=*/std::nullopt,
/*should_include_idp=*/false));
@ -638,6 +639,7 @@ AccountSelectionBubbleView::CreateSingleAccountChooser(
row->SetLayoutManager(std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kVertical,
gfx::Insets::VH(0, kLeftRightPadding), kVerticalSpacing));
CHECK(!account.is_filtered_out);
row->AddChildView(CreateAccountRow(account,
/*clickable_position=*/std::nullopt,
/*should_include_idp=*/false));
@ -856,6 +858,7 @@ AccountSelectionBubbleView::CreateSingleReturningAccountChooser(
? std::make_optional<std::u16string>(l10n_util::GetStringUTF16(
IDS_MULTI_IDP_ACCOUNT_LAST_USED_ON_THIS_SITE))
: std::nullopt;
CHECK(!accounts[0]->is_filtered_out);
content->AddChildView(CreateAccountRow(*accounts[0],
/*clickable_position=*/0,
/*should_include_idp=*/true,

@ -520,6 +520,7 @@ AccountSelectionModalView::CreateSingleAccountChooser(
row->AddChildView(std::make_unique<views::Separator>());
}
CHECK(!account.is_filtered_out);
// Add account row.
row->AddChildView(CreateAccountRow(
account, should_hover ? std::make_optional<int>(0) : std::nullopt,

@ -591,8 +591,14 @@ std::unique_ptr<views::View> AccountSelectionViewBase::CreateAccountRow(
if (should_include_idp) {
row->SetFooterTextStyle(views::style::CONTEXT_LABEL, account_email_style);
}
if (account.is_filtered_out) {
// TODO(crbug.com/40945672): improve the UI.
row->SetEnabled(false);
}
return row;
}
// We should only create non-button account rows for valid accounts.
CHECK(!account.is_filtered_out);
account_image_view->SetAccountImage(account, *image_fetcher_, avatar_size);
auto row = std::make_unique<views::View>();
row->SetLayoutManager(std::make_unique<views::BoxLayout>(

@ -145,9 +145,17 @@ bool FedCmAccountSelectionView::Show(
size_t returning_accounts_size =
std::count_if(accounts.begin(), accounts.end(), [](const auto& account) {
return account->login_state ==
content::IdentityRequestAccount::LoginState::kSignIn;
return !account->is_filtered_out &&
account->login_state ==
content::IdentityRequestAccount::LoginState::kSignIn;
});
bool has_filtered_out_accounts = false;
for (const auto& account : accounts) {
if (account->is_filtered_out) {
has_filtered_out_accounts = true;
break;
}
}
std::optional<std::u16string> idp_title =
idp_list_.size() == 1u
@ -253,12 +261,8 @@ bool FedCmAccountSelectionView::Show(
}
}
} else if (idp_list_.size() == 1u && accounts_or_mismatches_size == 1u) {
if (GetDialogType() == DialogType::MODAL) {
state_ = State::SINGLE_ACCOUNT_PICKER;
account_selection_view_->ShowSingleAccountConfirmDialog(
*accounts_[0],
/*show_back_button=*/false);
} else if (supports_add_account) {
if (GetDialogType() == DialogType::BUBBLE &&
(supports_add_account || has_filtered_out_accounts)) {
// The logic to support add account is in ShowMultiAccountPicker for the
// bubble dialog.
ShowMultiAccountPicker(accounts_, idp_list_, /*show_back_button=*/false,

@ -1541,7 +1541,11 @@ void FederatedAuthRequestImpl::MaybeShowAccountsDialog() {
std::stable_sort(
accounts_.begin(), accounts_.end(),
[&](const auto& account1, const auto& account2) {
// First, show newly logged in accounts, if any.
// Show filtered accounts after valid ones.
if (account1->is_filtered_out || account2->is_filtered_out) {
return !account1->is_filtered_out;
}
// Show newly logged in accounts, if any.
bool is_account1_new = IsNewlyLoggedIn(*account1);
bool is_account2_new = IsNewlyLoggedIn(*account2);
if (is_account1_new || is_account2_new) {
@ -2023,11 +2027,42 @@ void FederatedAuthRequestImpl::OnAccountsResponseReceived(
TokenStatus::kAccountsListEmpty);
return;
}
// TODO(crbug.com/354977893): pass filtered out accounts to UI.
auto filter = [](const IdentityRequestAccountPtr& account) {
return account->is_filtered_out;
};
std::erase_if(accounts, filter);
if (!IsFedCmShowFilteredAccountsEnabled() ||
!fetch_data_.for_idp_signin ||
login_url_ != idp_info->metadata.idp_login_url) {
std::erase_if(accounts, filter);
} else {
// If the user is logging in to new accounts, only show filtered
// accounts if there are no new unfiltered accounts. This includes in
// particular the case where all accounts are filtered out.
size_t new_unfiltered = std::count_if(
accounts.begin(), accounts.end(),
[&](const IdentityRequestAccountPtr& account) {
return !account->is_filtered_out &&
!account_ids_before_login_.contains(account->id);
});
if (new_unfiltered > 0u) {
std::erase_if(accounts, filter);
}
}
if (accounts.size() == 0u) {
// No accounts remain, so treat as account fetch failure.
render_frame_host().AddMessageToConsole(
blink::mojom::ConsoleMessageLevel::kError,
"Accounts were received, but none matched the login hint, domain "
"hint, and/or account labels provided.");
// If there are no accounts after filtering,treat this exactly the same
// as if we had received an empty accounts list, i.e.
// IdpNetworkRequestManager::ParseStatus::kEmptyListError.
HandleAccountsFetchFailure(
std::move(idp_info), old_idp_signin_status,
FederatedAuthRequestResult::kAccountsListEmpty,
TokenStatus::kAccountsListEmpty);
return;
}
RecordReadyToShowAccountsSize(accounts.size());
ComputeLoginStates(idp_info->provider->config->config_url, accounts);
@ -3101,7 +3136,8 @@ bool FederatedAuthRequestImpl::GetAccountForAutoReauthn(
}
}
for (const auto& account : accounts_) {
if (account->login_state == LoginState::kSignUp) {
if (account->login_state == LoginState::kSignUp ||
account->is_filtered_out) {
continue;
}
// account.login_state could be set to kSignIn if the client is on the
@ -3216,6 +3252,7 @@ void FederatedAuthRequestImpl::LoginToIdP(bool can_append_hints,
GURL login_url) {
const auto& it = idp_login_infos_.find(login_url);
CHECK(it != idp_login_infos_.end());
login_url_ = login_url;
if (can_append_hints) {
// Before invoking UI, append the query parameters to the `idp_login_url` if
// needed.
@ -3233,7 +3270,6 @@ void FederatedAuthRequestImpl::LoginToIdP(bool can_append_hints,
}
}
login_url_ = login_url;
ShowModalDialog(kLoginToIdpPopup, idp_config_url, login_url);
}
@ -3347,7 +3383,9 @@ bool FederatedAuthRequestImpl::IsNewlyLoggedIn(
login_url_ != account.identity_provider->idp_metadata.idp_login_url) {
return false;
}
return !account_ids_before_login_.contains(account.id);
// Exclude filtered out accounts so they are not shown at the top.
return !account.is_filtered_out &&
!account_ids_before_login_.contains(account.id);
}
bool FederatedAuthRequestImpl::FilterAccountsWithLabel(
@ -3370,7 +3408,7 @@ bool FederatedAuthRequestImpl::FilterAccountsWithLabel(
}
}
fedcm_metrics_->RecordNumMatchingAccounts(accounts_remaining, "AccountLabel");
return accounts_remaining > 0u;
return IsFedCmShowFilteredAccountsEnabled() || accounts_remaining > 0u;
}
bool FederatedAuthRequestImpl::FilterAccountsWithLoginHint(
@ -3396,7 +3434,7 @@ bool FederatedAuthRequestImpl::FilterAccountsWithLoginHint(
}
}
fedcm_metrics_->RecordNumMatchingAccounts(accounts_remaining, "LoginHint");
return accounts_remaining > 0u;
return IsFedCmShowFilteredAccountsEnabled() || accounts_remaining > 0u;
}
bool FederatedAuthRequestImpl::FilterAccountsWithDomainHint(
@ -3423,7 +3461,7 @@ bool FederatedAuthRequestImpl::FilterAccountsWithDomainHint(
++accounts_remaining;
}
fedcm_metrics_->RecordNumMatchingAccounts(accounts_remaining, "DomainHint");
return accounts_remaining > 0u;
return IsFedCmShowFilteredAccountsEnabled() || accounts_remaining > 0u;
}
} // namespace content

@ -455,18 +455,18 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
bool IsNewlyLoggedIn(const IdentityRequestAccount& account);
// Returns whether there are accounts remaining after applying the account
// Returns whether the algorithm should terminate after applying the account
// label filter.
bool FilterAccountsWithLabel(
const std::string& label,
std::vector<IdentityRequestAccountPtr>& accounts);
// Returns whether there are accounts remaining after applying the login hint
// filter.
// Returns whether the algorithm should terminate after applying the login
// hint filter.
bool FilterAccountsWithLoginHint(
const std::string& login_hint,
std::vector<IdentityRequestAccountPtr>& accounts);
// Returns whether there are accounts remaining after applying the domain hint
// filter.
// Returns whether the algorithm should terminate after applying the domain
// hint filter.
bool FilterAccountsWithDomainHint(
const std::string& domain_hint,
std::vector<IdentityRequestAccountPtr>& accounts);
@ -575,7 +575,8 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
std::vector<GURL> idp_order_;
// If dialog_type_ is kConfirmIdpLogin, this is the login URL for the IDP. If
// LoginToIdp() is called, this is the login URL for the IDP.
// LoginToIdp() is called, this is the login URL for the IDP. Does not include
// the filters as query parameters, if any.
GURL login_url_;
// If dialog_type_ is kError or a popup is open, this is the config URL for

@ -79,4 +79,8 @@ bool IsFedCmFlexibleFieldsEnabled() {
return base::FeatureList::IsEnabled(features::kFedCmFlexibleFields);
}
bool IsFedCmShowFilteredAccountsEnabled() {
return base::FeatureList::IsEnabled(features::kFedCmShowFilteredAccounts);
}
} // namespace content

@ -61,6 +61,9 @@ bool IsFedCmSameSiteLaxEnabled();
// Whether specifying a subset of the default fields is enabled.
bool IsFedCmFlexibleFieldsEnabled();
// Whether showing filtered accounts is enabled.
bool IsFedCmShowFilteredAccountsEnabled();
} // namespace content
#endif // CONTENT_BROWSER_WEBID_FLAGS_H_

@ -174,6 +174,12 @@ BASE_FEATURE(kFedCmSameSiteNone,
"FedCmSameSiteNone",
base::FEATURE_ENABLED_BY_DEFAULT);
// Allows showing the filtered out accounts after the user attempts to login to
// an account.
BASE_FEATURE(kFedCmShowFilteredAccounts,
"FedCmShowFilteredAccounts",
base::FEATURE_DISABLED_BY_DEFAULT);
// Enables installed web app matching for getInstalledRelatedApps API.
BASE_FEATURE(kFilterInstalledAppsWebAppMatching,
"FilterInstalledAppsWebAppMatching",

@ -42,6 +42,7 @@ CONTENT_EXPORT BASE_DECLARE_FEATURE(kFedCmFlexibleFields);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFedCmIdAssertionCORS);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFedCmSameSiteLax);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFedCmSameSiteNone);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFedCmShowFilteredAccounts);
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFilterInstalledAppsWebAppMatching);
#if BUILDFLAG(IS_WIN)
CONTENT_EXPORT BASE_DECLARE_FEATURE(kFilterInstalledAppsWinMatching);