Skip to content

Commit

Permalink
[FedCM] Fix re-auth when 3pc are disabled but the ISS OT is enabled
Browse files Browse the repository at this point in the history
The API permission status check in OnAccountSelected did not
take the origin trial into account.

In order to write a valid test for this case, I had to make content_shell
tell the federated permission context when third party cookies are
disabled. This follows a suggestion from morlovich@ through:
https://groups.google.com/a/chromium.org/g/net-dev/c/fZU_IK_BITI/

Bug: 1472928
Change-Id: I32d5757db883c34d025969a3fab943dfcddcae58
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4784810
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Reviewed-by: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1185005}
  • Loading branch information
cbiesinger authored and Chromium LUCI CQ committed Aug 18, 2023
1 parent 358e628 commit 4b36fa1
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 21 deletions.
31 changes: 19 additions & 12 deletions content/browser/webid/federated_auth_request_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -657,8 +657,7 @@ void FederatedAuthRequestImpl::RequestToken(
request_dialog_controller_ = CreateDialogController();
start_time_ = base::TimeTicks::Now();

FederatedApiPermissionStatus permission_status =
api_permission_delegate_->GetApiPermissionStatus(GetEmbeddingOrigin());
FederatedApiPermissionStatus permission_status = GetApiPermissionStatus();

absl::optional<TokenStatus> error_token_status;
FederatedAuthRequestResult request_result =
Expand All @@ -669,12 +668,6 @@ void FederatedAuthRequestImpl::RequestToken(
error_token_status = TokenStatus::kDisabledInFlags;
break;
case FederatedApiPermissionStatus::BLOCKED_THIRD_PARTY_COOKIES_BLOCKED:
// We allow FedCM without third-party cookies when the IDP sign-in
// status API is enabled, in general or through OT.
if (webid::GetIdpSigninStatusMode(render_frame_host()) ==
FedCmIdpSigninStatusMode::ENABLED) {
break;
}
error_token_status = TokenStatus::kThirdPartyCookiesBlocked;
request_result =
FederatedAuthRequestResult::kErrorThirdPartyCookiesBlocked;
Expand Down Expand Up @@ -872,8 +865,7 @@ void FederatedAuthRequestImpl::LogoutRps(
return;
}

if (api_permission_delegate_->GetApiPermissionStatus(GetEmbeddingOrigin()) !=
FederatedApiPermissionStatus::GRANTED) {
if (GetApiPermissionStatus() != FederatedApiPermissionStatus::GRANTED) {
CompleteLogoutRequest(LogoutRpsStatus::kError);
return;
}
Expand Down Expand Up @@ -1600,8 +1592,7 @@ void FederatedAuthRequestImpl::OnAccountSelected(const GURL& idp_config_url,
// settings are changed while an existing FedCM UI is displayed. Ideally, we
// should enforce this check before all requests but users typically won't
// have time to disable the FedCM API in other types of requests.
if (api_permission_delegate_->GetApiPermissionStatus(GetEmbeddingOrigin()) !=
FederatedApiPermissionStatus::GRANTED) {
if (GetApiPermissionStatus() != FederatedApiPermissionStatus::GRANTED) {
CompleteRequestWithError(
FederatedAuthRequestResult::kErrorDisabledInSettings,
TokenStatus::kDisabledInSettings,
Expand Down Expand Up @@ -2193,6 +2184,22 @@ void FederatedAuthRequestImpl::OnRejectRequest() {
}
}

FederatedApiPermissionStatus
FederatedAuthRequestImpl::GetApiPermissionStatus() {
DCHECK(api_permission_delegate_);
FederatedApiPermissionStatus status =
api_permission_delegate_->GetApiPermissionStatus(GetEmbeddingOrigin());
// We allow FedCM without third-party cookies when the IDP sign-in
// status API is enabled, in general or through OT.
if (status ==
FederatedApiPermissionStatus::BLOCKED_THIRD_PARTY_COOKIES_BLOCKED &&
webid::GetIdpSigninStatusMode(render_frame_host()) ==
FedCmIdpSigninStatusMode::ENABLED) {
status = FederatedApiPermissionStatus::GRANTED;
}
return status;
}

void FederatedAuthRequestImpl::AcceptAccountsDialogForDevtools(
const GURL& config_url,
const IdentityRequestAccount& account) {
Expand Down
6 changes: 6 additions & 0 deletions content/browser/webid/federated_auth_request_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "content/browser/webid/idp_network_request_manager.h"
#include "content/common/content_export.h"
#include "content/public/browser/document_service.h"
#include "content/public/browser/federated_identity_api_permission_context_delegate.h"
#include "content/public/browser/federated_identity_modal_dialog_view_delegate.h"
#include "content/public/browser/federated_identity_permission_context_delegate.h"
#include "content/public/browser/identity_request_dialog_controller.h"
Expand Down Expand Up @@ -107,6 +108,11 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
// Rejects the pending request if it has not been resolved naturally yet.
void OnRejectRequest();

// This wrapper around FederatedIdentityApiPermissionContextDelegate ensures
// that we handle BLOCKED_THIRD_PARTY_COOKIES_BLOCKED correctly.
FederatedIdentityApiPermissionContextDelegate::PermissionStatus
GetApiPermissionStatus();

struct IdentityProviderGetInfo {
IdentityProviderGetInfo(blink::mojom::IdentityProviderConfigPtr,
blink::mojom::RpContext rp_context);
Expand Down
16 changes: 7 additions & 9 deletions content/shell/browser/shell_browser_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -207,23 +207,21 @@ ContentIndexProvider* ShellBrowserContext::GetContentIndexProvider() {

FederatedIdentityApiPermissionContextDelegate*
ShellBrowserContext::GetFederatedIdentityApiPermissionContext() {
if (!federated_permission_context_)
federated_permission_context_ =
std::make_unique<ShellFederatedPermissionContext>();
return federated_permission_context_.get();
return GetShellFederatedPermissionContext();
}

FederatedIdentityAutoReauthnPermissionContextDelegate*
ShellBrowserContext::GetFederatedIdentityAutoReauthnPermissionContext() {
if (!federated_permission_context_) {
federated_permission_context_ =
std::make_unique<ShellFederatedPermissionContext>();
}
return federated_permission_context_.get();
return GetShellFederatedPermissionContext();
}

FederatedIdentityPermissionContextDelegate*
ShellBrowserContext::GetFederatedIdentityPermissionContext() {
return GetShellFederatedPermissionContext();
}

ShellFederatedPermissionContext*
ShellBrowserContext::GetShellFederatedPermissionContext() {
if (!federated_permission_context_)
federated_permission_context_ =
std::make_unique<ShellFederatedPermissionContext>();
Expand Down
2 changes: 2 additions & 0 deletions content/shell/browser/shell_browser_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,8 @@ class ShellBrowserContext : public BrowserContext {
GetReduceAcceptLanguageControllerDelegate() override;
OriginTrialsControllerDelegate* GetOriginTrialsControllerDelegate() override;

ShellFederatedPermissionContext* GetShellFederatedPermissionContext();

protected:
// Contains URLRequestContextGetter required for resource loading.
class ShellResourceContext : public ResourceContext {
Expand Down
4 changes: 4 additions & 0 deletions content/shell/browser/shell_federated_permission_context.cc
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,10 @@ ShellFederatedPermissionContext::GetApiPermissionStatus(
return PermissionStatus::BLOCKED_EMBARGO;
}

if (third_party_cookies_blocked_) {
return PermissionStatus::BLOCKED_THIRD_PARTY_COOKIES_BLOCKED;
}

return PermissionStatus::GRANTED;
}

Expand Down
6 changes: 6 additions & 0 deletions content/shell/browser/shell_federated_permission_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,10 @@ class ShellFederatedPermissionContext
void UnregisterIdP(const ::GURL&) override;
std::vector<GURL> GetRegisteredIdPs() override;

void SetThirdPartyCookiesBlocked(bool blocked) {
third_party_cookies_blocked_ = blocked;
}

void SetIdpStatusClosureForTesting(base::RepeatingClosure closure) {
idp_signin_status_closure_ = std::move(closure);
}
Expand Down Expand Up @@ -115,6 +119,8 @@ class ShellFederatedPermissionContext

// A set of urls that require user mediation.
std::set<GURL> require_user_mediation_sites_;

bool third_party_cookies_blocked_{false};
};

} // namespace content
Expand Down
4 changes: 4 additions & 0 deletions content/web_test/browser/web_test_control_host.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#include "content/shell/browser/shell_content_browser_client.h"
#include "content/shell/browser/shell_content_index_provider.h"
#include "content/shell/browser/shell_devtools_frontend.h"
#include "content/shell/browser/shell_federated_permission_context.h"
#include "content/test/mock_platform_notification_service.h"
#include "content/test/storage_partition_test_helpers.h"
#include "content/web_test/browser/devtools_protocol_test_bindings.h"
Expand Down Expand Up @@ -2017,6 +2018,9 @@ void WebTestControlHost::BlockThirdPartyCookies(bool block) {
browser_context->GetDefaultStoragePartition();
storage_partition->GetCookieManagerForBrowserProcess()
->BlockThirdPartyCookies(block);
ShellFederatedPermissionContext* federated_context =
browser_context->GetShellFederatedPermissionContext();
federated_context->SetThirdPartyCookiesBlocked(block);
}

void WebTestControlHost::BindWebTestControlHostForRenderer(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<!DOCTYPE html>
<title>Federated Credential Management API: Tests that re-authentication works when third party cookies are blocked but the ISS OT is enabled</title>
<!--
This is in wpt_internal because this test uses an internal API and also
because the interaction between 3pc and FedCM is Chrome-specific.
-->
<link rel="help" href="https://fedidcg.github.io/FedCM">
<script src="/resources/testharness.js"></script>
<script src="/resources/testharnessreport.js"></script>
<script src="/resources/testdriver.js"></script>
<script src="/resources/testdriver-vendor.js"></script>
<!--
Token generated with:
generate_token.py https://web-platform.test:8444/ FedCmIdpSigninStatus --expire-timestamp=2000000000
-->
<meta http-equiv="origin-trial"
content="AxsFYFt0hYa+TYkw+/9aNgaGvhJMjPQO9ec5OuuMNFcbVKnhXzUWEricRljJdLkft10VZ1DWQ7vOJPbLbzByNgMAAABleyJvcmlnaW4iOiAiaHR0cHM6Ly93ZWItcGxhdGZvcm0udGVzdDo4NDQ0IiwgImZlYXR1cmUiOiAiRmVkQ21JZHBTaWduaW5TdGF0dXMiLCAiZXhwaXJ5IjogMjAwMDAwMDAwMH0=">

<script type="module">
import {request_options_with_mediation_optional,
fedcm_test,
select_manifest,
fedcm_get_and_select_first_account,
mark_signed_in} from '/credential-management/support/fedcm-helper.sub.js';

fedcm_test(async t => {
await mark_signed_in();

let test_options = request_options_with_mediation_optional("manifest_with_single_account.json");
await select_manifest(t, test_options);

// Signs in john_doe so that they will be a returning user
let cred = await fedcm_get_and_select_first_account(t, test_options);
assert_equals(cred.token, "account_id=john_doe");

// Now block third-party cookies (regression test for https://crbug.com/1472928).
testRunner.setBlockThirdPartyCookies(true);

test_options = request_options_with_mediation_optional("manifest_with_two_accounts.json");
await select_manifest(t, test_options);

// There are two accounts "Jane" and "John" returned in that order. Without
// auto re-authn, the first account "Jane" would be selected and an token
// would be issued to that account. However, since "John" is returning and
// "Jane" is a new user, the second account "John" will be selected.
cred = await navigator.credentials.get(test_options);
assert_equals(cred.token, "account_id=john_doe");
}, "Test that the returning account from the two accounts will be auto re-authenticated.");
</script>

0 comments on commit 4b36fa1

Please sign in to comment.