Skip to content

Commit

Permalink
[FedCM] Check top frame's visibility instead of iframe's
Browse files Browse the repository at this point in the history
Currently we only proceed with FedCM API if the render frame is visible.
This makes sense when FedCM is used in the top frame. However, it's
common for top frame to embed a cross-origin iframe, possibly invisible,
to invoke FedCM API.

This patch checks the top frame's visibility instead of the iframe's to
make sure FedCM API can be used as long as the top frame is visible.

Change-Id: I299f1d3fb6dfb50b45cb5323efc6a97c95d45132
Bug: 1481308
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4860279
Reviewed-by: Zachary Tan <tanzachary@chromium.org>
Commit-Queue: Yi Gu <yigu@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1195567}
  • Loading branch information
yi-gu authored and Chromium LUCI CQ committed Sep 12, 2023
1 parent bfdbc96 commit 2deafce
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 8 deletions.
14 changes: 7 additions & 7 deletions content/browser/webid/federated_auth_request_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,11 @@ absl::optional<std::string> GetIframeOriginForDisplay(
return iframe_for_display;
}

bool IsFrameVisible(RenderFrameHost* frame) {
return frame && frame->IsActive() &&
frame->GetVisibilityState() == content::PageVisibilityState::kVisible;
}

} // namespace

FederatedAuthRequestImpl::FetchData::FetchData() = default;
Expand Down Expand Up @@ -1230,9 +1235,7 @@ void FederatedAuthRequestImpl::MaybeShowAccountsDialog() {
// if the RenderFrameHost is hidden because the user does not seem interested
// in the contents of the current page.
if (!fetch_data_.for_idp_signin) {
bool is_visible = (render_frame_host().IsActive() &&
render_frame_host().GetVisibilityState() ==
content::PageVisibilityState::kVisible);
bool is_visible = IsFrameVisible(render_frame_host().GetMainFrame());
fedcm_metrics_->RecordWebContentsVisibilityUponReadyToShowDialog(
is_visible);

Expand Down Expand Up @@ -1418,10 +1421,7 @@ void FederatedAuthRequestImpl::HandleAccountsFetchFailure(
return;
}

bool is_visible = (render_frame_host().IsActive() &&
render_frame_host().GetVisibilityState() ==
content::PageVisibilityState::kVisible);
if (!is_visible) {
if (!IsFrameVisible(render_frame_host().GetMainFrame())) {
CompleteRequestWithError(FederatedAuthRequestResult::kErrorRpPageNotVisible,
TokenStatus::kRpPageNotVisible,
/*should_delay_callback=*/true);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,16 @@
const remoteBaseURL = host.HTTPS_REMOTE_ORIGIN + basePath;
const localhostBaseURL = "http://localhost:" + host.HTTP_PORT + basePath;

async function createIframeAndWaitForMessage(test, iframeUrl, setPermissionPolicy) {
async function createIframeAndWaitForMessage(test, iframeUrl, setPermissionPolicy, style = "") {
const messageWatcher = new EventWatcher(test, window, "message");
var iframe = document.createElement("iframe");
iframe.src = iframeUrl;
if (setPermissionPolicy) {
iframe.allow = "identity-credentials-get";
}
if (style !== "") {
iframe.style = style;
}
document.body.appendChild(iframe);
const message = await messageWatcher.wait_for("message");
return message.data;
Expand All @@ -44,6 +47,14 @@
assert_equals(message.token, "token");
}, "FedCM enabled in 2 level deep nested iframe. FedCM should be enabled regardless of iframe nesting depth");

fedcm_test(async t => {
const message = await createIframeAndWaitForMessage(
t, remoteBaseURL + "support/fedcm-iframe.html",
/*setPermissionPolicy=*/true, /*style=*/"display:none;");
assert_equals(message.result, "Pass");
assert_equals(message.token, "token");
}, "FedCM enabled in invisible iframe. FedCM should be enabled as long as the top frame is visible");

fedcm_test(async t => {
const message = await createIframeAndWaitForMessage(
t, remoteBaseURL + "support/fedcm-iframe-level2.html",
Expand Down

0 comments on commit 2deafce

Please sign in to comment.