Skip to content

Commit

Permalink
[FedCM] Standardize on Login instead of Signin for browser automation
Browse files Browse the repository at this point in the history
Based on recent discussions in the fedidcg, we prefer Login over Signin.
This CL changes the exposed commands accordingly.

Note that the IDP signin feature is still in origin trial and thus subject
to change. Additionally, the confirmidpsignin command is vendor-prefixed
and is not shipped yet, so it should be safe to change:
https://chromiumdash.appspot.com/commit/3362c0d9ff2667d26f37e04917489ec0eb4bc83f

Spec PR: w3c-fedid/FedCM#436

Validate-Test-Flakiness: skip
Low-Coverage-Reason: rename only, no functional changes
Bug: 1451884
Change-Id: Ic3b3f3e144c6649b19f063258403c239e9ceac82
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4901302
Reviewed-by: Vladimir Nechaev <nechaev@chromium.org>
Commit-Queue: Christian Biesinger <cbiesinger@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1204075}
  • Loading branch information
cbiesinger authored and tkiela1 committed Oct 2, 2023
1 parent b81ec56 commit e41aa8c
Show file tree
Hide file tree
Showing 20 changed files with 64 additions and 64 deletions.
4 changes: 2 additions & 2 deletions chrome/test/chromedriver/client/chromedriver.py
Original file line number Diff line number Diff line change
Expand Up @@ -746,9 +746,9 @@ def SelectAccount(self, index):
params = {'accountIndex': index}
return self.ExecuteCommand(Command.SELECT_ACCOUNT, params)

def ConfirmIdpSignin(self, vendorId):
def ConfirmIdpLogin(self, vendorId):
params = {'vendorId': vendorId}
return self.ExecuteCommand(Command.CONFIRM_IDP_SIGNIN, params)
return self.ExecuteCommand(Command.CONFIRM_IDP_LOGIN, params)

def GetAccounts(self):
return self.ExecuteCommand(Command.GET_ACCOUNTS, {})
Expand Down
4 changes: 2 additions & 2 deletions chrome/test/chromedriver/client/command_executor.py
Original file line number Diff line number Diff line change
Expand Up @@ -227,9 +227,9 @@ class Command(object):
SELECT_ACCOUNT = (
_Method.POST,
'/session/:sessionId/fedcm/selectaccount')
CONFIRM_IDP_SIGNIN = (
CONFIRM_IDP_LOGIN = (
_Method.POST,
'/session/:sessionId/:vendorId/fedcm/confirmidpsignin')
'/session/:sessionId/:vendorId/fedcm/confirmidplogin')
GET_ACCOUNTS = (
_Method.GET,
'/session/:sessionId/fedcm/accountlist')
Expand Down
12 changes: 6 additions & 6 deletions chrome/test/chromedriver/fedcm_commands.cc
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,11 @@ Status ExecuteSelectAccount(Session* session,
return status;
}

Status ExecuteConfirmIdpSignin(Session* session,
WebView* web_view,
const base::Value::Dict& params,
std::unique_ptr<base::Value>* value,
Timeout* timeout) {
Status ExecuteConfirmIdpLogin(Session* session,
WebView* web_view,
const base::Value::Dict& params,
std::unique_ptr<base::Value>* value,
Timeout* timeout) {
FedCmTracker* tracker = nullptr;
Status status = web_view->GetFedCmTracker(&tracker);
if (!status.IsOk()) {
Expand All @@ -80,7 +80,7 @@ Status ExecuteConfirmIdpSignin(Session* session,
command_params.Set("dialogId", tracker->GetLastDialogId());

std::unique_ptr<base::Value> result;
status = web_view->SendCommandAndGetResult("FedCm.confirmIdpSignin",
status = web_view->SendCommandAndGetResult("FedCm.confirmIdpLogin",
command_params, &result);
tracker->DialogClosed();
return status;
Expand Down
10 changes: 5 additions & 5 deletions chrome/test/chromedriver/fedcm_commands.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ Status ExecuteSelectAccount(Session* session,
std::unique_ptr<base::Value>* value,
Timeout* timeout);

Status ExecuteConfirmIdpSignin(Session* session,
WebView* web_view,
const base::Value::Dict& params,
std::unique_ptr<base::Value>* value,
Timeout* timeout);
Status ExecuteConfirmIdpLogin(Session* session,
WebView* web_view,
const base::Value::Dict& params,
std::unique_ptr<base::Value>* value,
Timeout* timeout);

Status ExecuteGetAccounts(Session* session,
WebView* web_view,
Expand Down
8 changes: 4 additions & 4 deletions chrome/test/chromedriver/fedcm_commands_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class MockResponseWebView : public StubWebView {
account.Set("givenName", "Foo");
account.Set("pictureUrl", "https://pics/pic.jpg");
account.Set("idpConfigUrl", "https://idp.example/fedcm.json");
account.Set("idpSigninUrl", "https://idp.example/signin");
account.Set("idpLoginUrl", "https://idp.example/login");
account.Set("loginState", "SignIn");

base::Value::List accounts;
Expand Down Expand Up @@ -145,9 +145,9 @@ TEST_F(FedCmCommandsTest, ExecuteGetAccounts) {
std::string* idpConfigUrl = account->FindString("idpConfigUrl");
ASSERT_TRUE(idpConfigUrl);
EXPECT_EQ(*idpConfigUrl, "https://idp.example/fedcm.json");
std::string* idpSigninUrl = account->FindString("idpSigninUrl");
ASSERT_TRUE(idpSigninUrl);
EXPECT_EQ(*idpSigninUrl, "https://idp.example/signin");
std::string* idpLoginUrl = account->FindString("idpLoginUrl");
ASSERT_TRUE(idpLoginUrl);
EXPECT_EQ(*idpLoginUrl, "https://idp.example/login");
std::string* loginState = account->FindString("loginState");
ASSERT_TRUE(loginState);
EXPECT_EQ(*loginState, "SignIn");
Expand Down
6 changes: 3 additions & 3 deletions chrome/test/chromedriver/server/http_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -950,9 +950,9 @@ HttpHandler::HttpHandler(
// This command is prefixed because standardization is still pending:
// https://github.com/fedidcg/FedCM/pull/436/files
VendorPrefixedCommandMapping(
kPost, "session/:sessionId/%s/fedcm/confirmidpsignin",
WrapToCommand("ConfirmIdpSignin",
base::BindRepeating(&ExecuteConfirmIdpSignin))),
kPost, "session/:sessionId/%s/fedcm/confirmidplogin",
WrapToCommand("ConfirmIdpLogin",
base::BindRepeating(&ExecuteConfirmIdpLogin))),

CommandMapping(kGet, "session/:sessionId/fedcm/accountlist",
WrapToCommand("GetAccounts",
Expand Down
6 changes: 3 additions & 3 deletions chrome/test/chromedriver/test/run_py_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -6985,7 +6985,7 @@ def testDismissDialog(self):
token = self._driver.ExecuteScript('return getResult()')
self.assertEqual('Error: NetworkError: Error retrieving a token.', token)

def testConfirmIdpSignin(self):
def testConfirmIdpLogin(self):
self._accounts = ""

self._driver.Load(self._https_server.GetUrl() + "/fedcm.html")
Expand All @@ -6998,12 +6998,12 @@ def testConfirmIdpSignin(self):
self.assertTrue(self.WaitForCondition(self.FedCmDialogCondition))

accounts = self._driver.GetAccounts()
self.assertEqual("ConfirmIdpSignin", self._driver.GetDialogType())
self.assertEqual("ConfirmIdpLogin", self._driver.GetDialogType())
self.assertEqual(0, len(accounts))

self._accounts = self._default_accounts

self._driver.ConfirmIdpSignin(self._vendor_id)
self._driver.ConfirmIdpLogin(self._vendor_id)

self.assertTrue(self.WaitForCondition(self.FedCmDialogCondition))
accounts = self._driver.GetAccounts()
Expand Down
18 changes: 9 additions & 9 deletions content/browser/devtools/protocol/fedcm_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ FedCm::DialogType ConvertDialogType(
return FedCm::DialogTypeEnum::AccountChooser;
case content::FederatedAuthRequestImpl::kAutoReauth:
return FedCm::DialogTypeEnum::AutoReauthn;
case content::FederatedAuthRequestImpl::kConfirmIdpSignin:
return FedCm::DialogTypeEnum::ConfirmIdpSignin;
case content::FederatedAuthRequestImpl::kConfirmIdpLogin:
return FedCm::DialogTypeEnum::ConfirmIdpLogin;
}
}
} // namespace
Expand Down Expand Up @@ -128,7 +128,7 @@ void FedCmHandler::OnDialogShown() {
.SetGivenName(account.given_name)
.SetPictureUrl(account.picture.spec())
.SetIdpConfigUrl(data.idp_metadata.config_url.spec())
.SetIdpSigninUrl(data.idp_metadata.idp_signin_url.spec())
.SetIdpLoginUrl(data.idp_metadata.idp_signin_url.spec())
.SetLoginState(login_state)
.Build();
if (pp_url) {
Expand Down Expand Up @@ -182,7 +182,7 @@ DispatchResponse FedCmHandler::SelectAccount(const String& in_dialogId,
return DispatchResponse::InvalidParams("Invalid account index");
}

DispatchResponse FedCmHandler::ConfirmIdpSignin(const String& in_dialogId) {
DispatchResponse FedCmHandler::ConfirmIdpLogin(const String& in_dialogId) {
if (in_dialogId != dialog_id_) {
return DispatchResponse::InvalidParams(
"Dialog ID does not match current dialog");
Expand All @@ -195,11 +195,11 @@ DispatchResponse FedCmHandler::ConfirmIdpSignin(const String& in_dialogId) {
}

FederatedAuthRequestImpl::DialogType type = auth_request->GetDialogType();
if (type != FederatedAuthRequestImpl::kConfirmIdpSignin) {
if (type != FederatedAuthRequestImpl::kConfirmIdpLogin) {
return DispatchResponse::ServerError(
"dismissDialog called while no confirm IDP signin dialog is shown");
"dismissDialog called while no confirm IDP login dialog is shown");
}
auth_request->AcceptConfirmIdpSigninDialogForDevtools();
auth_request->AcceptConfirmIdpLoginDialogForDevtools();
return DispatchResponse::Success();
}

Expand All @@ -217,8 +217,8 @@ DispatchResponse FedCmHandler::DismissDialog(const String& in_dialogId,
}

FederatedAuthRequestImpl::DialogType type = auth_request->GetDialogType();
if (type == FederatedAuthRequestImpl::kConfirmIdpSignin) {
auth_request->DismissConfirmIdpSigninDialogForDevtools();
if (type == FederatedAuthRequestImpl::kConfirmIdpLogin) {
auth_request->DismissConfirmIdpLoginDialogForDevtools();
return DispatchResponse::Success();
}
const auto* idp_data = GetIdentityProviderData(auth_request);
Expand Down
2 changes: 1 addition & 1 deletion content/browser/devtools/protocol/fedcm_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ class FedCmHandler : public DevToolsDomainHandler, public FedCm::Backend {
DispatchResponse Disable() override;
DispatchResponse SelectAccount(const String& in_dialogId,
int in_accountIndex) override;
DispatchResponse ConfirmIdpSignin(const String& in_dialogId) override;
DispatchResponse ConfirmIdpLogin(const String& in_dialogId) override;
DispatchResponse DismissDialog(const String& in_dialogId,
Maybe<bool> in_triggerCooldown) override;
DispatchResponse ResetCooldown() override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ void FakeIdentityRequestDialogController::ShowFailureDialog(
const IdentityProviderMetadata& idp_metadata,
DismissCallback dismiss_callback,
SigninToIdPCallback signin_callback) {
title_ = "Confirm IDP Signin";
title_ = "Confirm IDP Login";
}

void FakeIdentityRequestDialogController::ShowErrorDialog(
Expand Down
6 changes: 3 additions & 3 deletions content/browser/webid/federated_auth_request_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ void FederatedAuthRequestImpl::HandleAccountsFetchFailure(
// If IdP sign-in status mismatch dialog is already visible, calling
// ShowFailureDialog() a 2nd time should notify the user that sign-in
// failed.
dialog_type_ = kConfirmIdpSignin;
dialog_type_ = kConfirmIdpLogin;
signin_url_ = idp_info->metadata.idp_signin_url;
request_dialog_controller_->ShowFailureDialog(
GetTopFrameOriginForDisplay(GetEmbeddingOrigin()), iframe_for_display,
Expand Down Expand Up @@ -2411,12 +2411,12 @@ void FederatedAuthRequestImpl::DismissAccountsDialogForDevtools(
OnDialogDismissed(reason);
}

void FederatedAuthRequestImpl::AcceptConfirmIdpSigninDialogForDevtools() {
void FederatedAuthRequestImpl::AcceptConfirmIdpLoginDialogForDevtools() {
DCHECK(signin_url_.is_valid());
ShowModalDialog(signin_url_);
}

void FederatedAuthRequestImpl::DismissConfirmIdpSigninDialogForDevtools() {
void FederatedAuthRequestImpl::DismissConfirmIdpLoginDialogForDevtools() {
// These values match what HandleAccountsFetchFailure passes.
OnDismissFailureDialog(
IdentityRequestDialogController::DismissReason::kOther);
Expand Down
8 changes: 4 additions & 4 deletions content/browser/webid/federated_auth_request_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -150,14 +150,14 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
return idp_data_for_display_;
}

enum DialogType { kNone, kSelectAccount, kAutoReauth, kConfirmIdpSignin };
enum DialogType { kNone, kSelectAccount, kAutoReauth, kConfirmIdpLogin };
DialogType GetDialogType() const { return dialog_type_; }

void AcceptAccountsDialogForDevtools(const GURL& config_url,
const IdentityRequestAccount& account);
void DismissAccountsDialogForDevtools(bool should_embargo);
void AcceptConfirmIdpSigninDialogForDevtools();
void DismissConfirmIdpSigninDialogForDevtools();
void AcceptConfirmIdpLoginDialogForDevtools();
void DismissConfirmIdpLoginDialogForDevtools();

// Check if the scope of the request allows the browser to mediate
// or delegate (to the IdP) the authorization.
Expand Down Expand Up @@ -413,7 +413,7 @@ class CONTENT_EXPORT FederatedAuthRequestImpl
// the navigator.credentials.get call.
std::vector<GURL> idp_order_;

// If dialog_type_ is kConfirmIdpSignin, this is the signin URL for the IDP.
// If dialog_type_ is kConfirmIdpLogin, this is the signin URL for the IDP.
GURL signin_url_;

DialogType dialog_type_ = kNone;
Expand Down
8 changes: 4 additions & 4 deletions content/web_test/browser/web_test_fedcm_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ void WebTestFedCmManager::GetDialogType(
case FederatedAuthRequestImpl::kAutoReauth:
type_string = "AutoReauthn";
break;
case FederatedAuthRequestImpl::kConfirmIdpSignin:
type_string = "ConfirmIdpSignin";
case FederatedAuthRequestImpl::kConfirmIdpLogin:
type_string = "ConfirmIdpLogin";
break;
};
std::move(callback).Run(type_string);
Expand Down Expand Up @@ -154,8 +154,8 @@ void WebTestFedCmManager::DismissFedCmDialog(
auth_request->DismissAccountsDialogForDevtools(false);
std::move(callback).Run(true);
return;
case FederatedAuthRequestImpl::kConfirmIdpSignin:
auth_request->DismissConfirmIdpSigninDialogForDevtools();
case FederatedAuthRequestImpl::kConfirmIdpLogin:
auth_request->DismissConfirmIdpLoginDialogForDevtools();
std::move(callback).Run(true);
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11487,7 +11487,7 @@ experimental domain FedCm
enum
AccountChooser
AutoReauthn
ConfirmIdpSignin
ConfirmIdpLogin

# Corresponds to IdentityRequestAccount
type Account extends object
Expand All @@ -11498,7 +11498,7 @@ experimental domain FedCm
string givenName
string pictureUrl
string idpConfigUrl
string idpSigninUrl
string idpLoginUrl
LoginState loginState
# These two are only set if the loginState is signUp
optional string termsOfServiceUrl
Expand Down Expand Up @@ -11528,9 +11528,9 @@ experimental domain FedCm
string dialogId
integer accountIndex

# Only valid if the dialog type is ConfirmIdpSignin. Acts as if the user had
# Only valid if the dialog type is ConfirmIdpLogin. Acts as if the user had
# clicked the continue button.
command confirmIdpSignin
command confirmIdpLogin
parameters
string dialogId

Expand Down
2 changes: 1 addition & 1 deletion third_party/blink/web_tests/SlowTests
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ crbug.com/678482 [ Mac10.15 Release ] http/tests/devtools/debugger/fetch-breakpo
crbug.com/678482 [ Mac11 Release ] http/tests/devtools/debugger/fetch-breakpoints.js [ Slow ]
crbug.com/678482 [ Mac12 ] http/tests/devtools/debugger/fetch-breakpoints.js [ Slow ]
crbug.com/678482 [ Release Win ] http/tests/devtools/debugger/fetch-breakpoints.js [ Slow ]
crbug.com/1451884 http/tests/inspector-protocol/fedcm/fedcm-confirm-idp-signin.js [ Slow ]
crbug.com/1451884 http/tests/inspector-protocol/fedcm/fedcm-confirm-idp-login.js [ Slow ]
crbug.com/1236466 inspector-protocol/runtime/runtime-execution-contexts-events.js [ Slow ]
crbug.com/1229701 [ Debug Mac13-arm64 ] http/tests/inspector-protocol/network/disable-cache-media-resource.js [ Slow ]
crbug.com/1229701 [ Linux ] http/tests/inspector-protocol/network/disable-cache-media-resource.js [ Slow ]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ msg.params: {
accounts : [
]
dialogId : <string>
dialogType : ConfirmIdpSignin
title : Confirm IDP Signin
dialogType : ConfirmIdpLogin
title : Confirm IDP Login
}
msg.params: {
accounts : [
Expand All @@ -13,7 +13,7 @@ msg.params: {
email : john_doe@idp.example
givenName : John
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignIn
name : John Doe
pictureUrl : https://idp.example/profile/123
Expand All @@ -23,7 +23,7 @@ msg.params: {
email : aisha@idp.example
givenName : Aisha
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignUp
name : Aisha Ahmad
pictureUrl : https://idp.example/profile/567
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@
}

testRunner.log(msg.params, "msg.params: ", ["dialogId"]);
if (msg.params.dialogType != "ConfirmIdpSignin") {
if (msg.params.dialogType != "ConfirmIdpLogin") {
testRunner.fail("Wrong dialog type");
return;
}
dp.FedCm.confirmIdpSignin({dialogId: msg.params.dialogId});
dp.FedCm.confirmIdpLogin({dialogId: msg.params.dialogId});

// Now wait for the account chooser dialog.
msg = await dp.FedCm.onceDialogShown();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ msg.params: {
email : john_doe@idp.example
givenName : John
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignIn
name : John Doe
pictureUrl : https://idp.example/profile/123
Expand All @@ -16,7 +16,7 @@ msg.params: {
email : aisha@idp.example
givenName : Aisha
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignUp
name : Aisha Ahmad
pictureUrl : https://idp.example/profile/567
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ msg.params: {
email : john_doe@idp.example
givenName : John
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignIn
name : John Doe
pictureUrl : https://idp.example/profile/123
Expand All @@ -16,7 +16,7 @@ msg.params: {
email : aisha@idp.example
givenName : Aisha
idpConfigUrl : https://127.0.0.1:8443/resources/fedcm/fedcm.json
idpSigninUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
idpLoginUrl : https://127.0.0.1:8443/resources/fedcm/signin.html
loginState : SignUp
name : Aisha Ahmad
pictureUrl : https://idp.example/profile/567
Expand Down
Loading

0 comments on commit e41aa8c

Please sign in to comment.