Skip to content

Commit

Permalink
Remove UseAuthsessionForWebAuthN feature flag
Browse files Browse the repository at this point in the history
Bug: b:260715686
Change-Id: Iac092bbf4697e6e39424149b4c0553dcba549e96
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4328139
Reviewed-by: Lei Zhang <thestig@chromium.org>
Commit-Queue: Elie Maamari <emaamari@google.com>
Reviewed-by: Denis Kuznetsov <antrim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1117574}
  • Loading branch information
Elie Maamari authored and Chromium LUCI CQ committed Mar 15, 2023
1 parent a984ec4 commit 7c9dfdd
Show file tree
Hide file tree
Showing 20 changed files with 20 additions and 406 deletions.
10 changes: 0 additions & 10 deletions ash/constants/ash_features.cc
Expand Up @@ -2013,12 +2013,6 @@ BASE_FEATURE(kUseAuthFactors,
"UseAuthFactors",
base::FEATURE_ENABLED_BY_DEFAULT);

// When enabled, WebAuthN uses auth session based authentication
// instead of legacy CheckKey.
BASE_FEATURE(kUseAuthsessionForWebAuthN,
"UseAuthsessionForWebAuthN",
base::FEATURE_ENABLED_BY_DEFAULT);

// When enabled, the login shelf view is placed in its own widget instead of
// sharing the shelf widget with other components.
BASE_FEATURE(kUseLoginShelfWidget,
Expand Down Expand Up @@ -3183,10 +3177,6 @@ bool IsUploadOfficeToCloudEnabled() {
return base::FeatureList::IsEnabled(kUploadOfficeToCloud);
}

bool IsUseAuthsessionForWebAuthNEnabled() {
return base::FeatureList::IsEnabled(kUseAuthsessionForWebAuthN);
}

bool IsUseLoginShelfWidgetEnabled() {
return base::FeatureList::IsEnabled(kUseLoginShelfWidget);
}
Expand Down
3 changes: 0 additions & 3 deletions ash/constants/ash_features.h
Expand Up @@ -575,8 +575,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kTrafficCountersEnabled);
COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kTrilinearFiltering);
COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kUploadOfficeToCloud);
COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kUseAuthFactors);
COMPONENT_EXPORT(ASH_CONSTANTS)
BASE_DECLARE_FEATURE(kUseAuthsessionForWebAuthN);
COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kUseLoginShelfWidget);
COMPONENT_EXPORT(ASH_CONSTANTS) BASE_DECLARE_FEATURE(kUseMessagesStagingUrl);
COMPONENT_EXPORT(ASH_CONSTANTS)
Expand Down Expand Up @@ -875,7 +873,6 @@ COMPONENT_EXPORT(ASH_CONSTANTS) bool IsTouchscreenInDiagnosticsAppEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsTrafficCountersEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsTrilinearFilteringEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsUploadOfficeToCloudEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsUseAuthsessionForWebAuthNEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsUseLoginShelfWidgetEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsUseStorkSmdsServerAddressEnabled();
COMPONENT_EXPORT(ASH_CONSTANTS) bool IsVideoConferenceEnabled();
Expand Down
34 changes: 14 additions & 20 deletions ash/in_session_auth/webauthn_dialog_controller_impl.cc
Expand Up @@ -50,23 +50,19 @@ void WebAuthNDialogControllerImpl::ShowAuthenticationDialog(
weak_factory_.GetWeakPtr(), account_id, origin_name,
auth_methods, source_window);

if (ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
auto on_auth_session_started = [](base::OnceClosure continuation,
bool is_auth_session_started) {
if (!is_auth_session_started) {
LOG(ERROR)
<< "Failed to start cryptohome auth session, exiting dialog early";
return;
}
std::move(continuation).Run();
};

client_->StartAuthSession(
base::BindOnce(on_auth_session_started, std::move(continuation)));
return;
}

std::move(continuation).Run();
auto on_auth_session_started = [](base::OnceClosure continuation,
bool is_auth_session_started) {
if (!is_auth_session_started) {
LOG(ERROR)
<< "Failed to start cryptohome auth session, exiting dialog early";
return;
}
std::move(continuation).Run();
};

client_->StartAuthSession(
base::BindOnce(on_auth_session_started, std::move(continuation)));
return;
}

void WebAuthNDialogControllerImpl::CheckAuthFactorAvailability(
Expand Down Expand Up @@ -165,9 +161,7 @@ void WebAuthNDialogControllerImpl::DestroyAuthenticationDialog() {
}

void WebAuthNDialogControllerImpl::ProcessFinalCleanups() {
if (ash::features::IsUseAuthsessionForWebAuthNEnabled())
client_->InvalidateAuthSession();

client_->InvalidateAuthSession();
dialog_.reset();
source_window_tracker_.RemoveAll();
}
Expand Down
85 changes: 0 additions & 85 deletions chrome/browser/ui/ash/in_session_auth_dialog_client.cc
Expand Up @@ -75,14 +75,6 @@ InSessionAuthDialogClient* InSessionAuthDialogClient::Get() {

bool InSessionAuthDialogClient::IsFingerprintAuthAvailable(
const AccountId& account_id) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
ash::quick_unlock::QuickUnlockStorage* quick_unlock_storage =
ash::quick_unlock::QuickUnlockFactory::GetForAccountId(account_id);
return quick_unlock_storage &&
quick_unlock_storage->fingerprint_storage()->IsFingerprintAvailable(
ash::quick_unlock::Purpose::kWebAuthn);
}

return legacy_fingerprint_engine_->IsFingerprintAvailable(
ash::LegacyFingerprintEngine::Purpose::kWebAuthn,
user_context_->GetAccountId());
Expand All @@ -99,12 +91,6 @@ ExtendedAuthenticator* InSessionAuthDialogClient::GetExtendedAuthenticator() {
void InSessionAuthDialogClient::StartFingerprintAuthSession(
const AccountId& account_id,
base::OnceCallback<void(bool)> callback) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
GetExtendedAuthenticator()->StartFingerprintAuthSession(
account_id, std::move(callback));
return;
}

legacy_fingerprint_engine_->PrepareLegacyFingerprintFactor(
std::move(user_context_),
base::BindOnce(
Expand All @@ -131,13 +117,6 @@ void InSessionAuthDialogClient::OnPrepareLegacyFingerprintFactor(

void InSessionAuthDialogClient::EndFingerprintAuthSession(
base::OnceClosure callback) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
DCHECK(extended_authenticator_);
extended_authenticator_->EndFingerprintAuthSession();
std::move(callback).Run();
return;
}

if (legacy_fingerprint_engine_.has_value()) {
legacy_fingerprint_engine_->TerminateLegacyFingerprintFactor(
std::move(user_context_),
Expand Down Expand Up @@ -165,13 +144,6 @@ void InSessionAuthDialogClient::OnTerminateLegacyFingerprintFactor(
void InSessionAuthDialogClient::CheckPinAuthAvailability(
const AccountId& account_id,
base::OnceCallback<void(bool)> callback) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
// PinBackend may be using cryptohome backend or prefs backend.
ash::quick_unlock::PinBackend::GetInstance()->CanAuthenticate(
account_id, ash::quick_unlock::Purpose::kWebAuthn, std::move(callback));
return;
}

auto on_pin_availability_checked =
base::BindOnce(&InSessionAuthDialogClient::OnCheckPinAuthAvailability,
weak_factory_.GetWeakPtr(), std::move(callback));
Expand Down Expand Up @@ -247,15 +219,6 @@ void InSessionAuthDialogClient::AuthenticateUserWithPasswordOrPin(
return;
}

if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
ash::quick_unlock::PinBackend::GetInstance()->TryAuthenticate(
std::move(user_context), std::move(key),
ash::quick_unlock::Purpose::kWebAuthn,
base::BindOnce(&InSessionAuthDialogClient::OnPinAttemptDone,
weak_factory_.GetWeakPtr()));
return;
}

pin_engine_->Authenticate(
cryptohome::RawPin(secret), std::move(user_context_),
base::BindOnce(&InSessionAuthDialogClient::OnAuthVerified,
Expand All @@ -280,20 +243,6 @@ void InSessionAuthDialogClient::OnPinAttemptDone(
void InSessionAuthDialogClient::AuthenticateWithPassword(
std::unique_ptr<UserContext> user_context,
const std::string& password) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
// TODO(crbug.com/1115120): Don't post to UI thread if it turns out to be
// unnecessary.
auto context = std::move(*user_context);
content::GetUIThreadTaskRunner({})->PostTask(
FROM_HERE,
base::BindOnce(
&ExtendedAuthenticator::AuthenticateToUnlockWebAuthnSecret,
GetExtendedAuthenticator(), context,
base::BindOnce(&InSessionAuthDialogClient::OnPasswordAuthSuccess,
weak_factory_.GetWeakPtr(), context)));
return;
}

// Check that we have a valid `user_context_`, provided by a prior
// `StartAuthSession`, this also nicely asserts that we are not waiting
// on other UserDataAuth dbus calls that involve the auth_session_id stored
Expand Down Expand Up @@ -377,20 +326,6 @@ void InSessionAuthDialogClient::OnPasswordAuthSuccess(

void InSessionAuthDialogClient::AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, ash::FingerprintState)> callback) {
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
const user_manager::User* const user =
user_manager::UserManager::Get()->GetActiveUser();
DCHECK(user);
UserContext user_context(*user);

DCHECK(extended_authenticator_);
extended_authenticator_->AuthenticateWithFingerprint(
user_context,
base::BindOnce(&InSessionAuthDialogClient::OnFingerprintAuthDone,
weak_factory_.GetWeakPtr(), std::move(callback)));
return;
}

DCHECK(!fingerprint_scan_done_callback_);
fingerprint_scan_done_callback_ = std::move(callback);
}
Expand Down Expand Up @@ -423,26 +358,6 @@ void InSessionAuthDialogClient::OnFingerprintScan(
}
}

void InSessionAuthDialogClient::OnFingerprintAuthDone(
base::OnceCallback<void(bool, ash::FingerprintState)> callback,
user_data_auth::CryptohomeErrorCode error) {
switch (error) {
case user_data_auth::CRYPTOHOME_ERROR_NOT_SET:
std::move(callback).Run(true, ash::FingerprintState::AVAILABLE_DEFAULT);
break;
case user_data_auth::CRYPTOHOME_ERROR_FINGERPRINT_RETRY_REQUIRED:
std::move(callback).Run(false, ash::FingerprintState::AVAILABLE_DEFAULT);
break;
case user_data_auth::CRYPTOHOME_ERROR_FINGERPRINT_DENIED:
std::move(callback).Run(false,
ash::FingerprintState::DISABLED_FROM_ATTEMPTS);
break;
default:
// Internal error.
std::move(callback).Run(false, ash::FingerprintState::UNAVAILABLE);
}
}

aura::Window* InSessionAuthDialogClient::OpenInSessionAuthHelpPage() const {
// TODO(b/156258540): Use the profile of the source browser window.
Profile* profile = ProfileManager::GetActiveUserProfile();
Expand Down
4 changes: 0 additions & 4 deletions chrome/browser/ui/ash/in_session_auth_dialog_client.h
Expand Up @@ -141,10 +141,6 @@ class InSessionAuthDialogClient

void OnPasswordAuthSuccess(const ash::UserContext& user_context);

void OnFingerprintAuthDone(
base::OnceCallback<void(bool, ash::FingerprintState)> callback,
user_data_auth::CryptohomeErrorCode error);

// Passed as a callback to `CryptohomePinEngine::InPinAuthAvailable`
// Takes back ownership of the `user_context` that was borrowed by
// `CryptohomePinEngine` and notifies callers of pin availability status.
Expand Down
19 changes: 4 additions & 15 deletions chrome/browser/ui/ash/in_session_auth_dialog_client_unittest.cc
Expand Up @@ -101,10 +101,6 @@ class InSessionAuthDialogClientTest : public testing::Test {
std::move(callback));
}

bool GetLastUnlockWebAuthnSecret() const {
return fake_authenticator_->last_unlock_webauthn_secret();
}

void ConfigureExistingUserWithPassword(const AccountId& user,
const std::string& password) {
Key key(Key::KEY_TYPE_PASSWORD_PLAIN, std::string(), password);
Expand Down Expand Up @@ -155,10 +151,8 @@ TEST_F(InSessionAuthDialogClientTest, WrongPassword) {
expected_user_context.SetKey(key);

SetExpectedContext(expected_user_context);
if (ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
ConfigureExistingUserWithPassword(user->GetAccountId(), kPassword);
StartAuthSessionForActiveUser();
};
ConfigureExistingUserWithPassword(user->GetAccountId(), kPassword);
StartAuthSessionForActiveUser();

base::RunLoop run_loop;

Expand All @@ -183,10 +177,8 @@ TEST_F(InSessionAuthDialogClientTest, PasswordAuthSuccess) {

SetExpectedContext(expected_user_context);

if (ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
ConfigureExistingUserWithPassword(user->GetAccountId(), kPassword);
StartAuthSessionForActiveUser();
};
ConfigureExistingUserWithPassword(user->GetAccountId(), kPassword);
StartAuthSessionForActiveUser();

base::RunLoop run_loop;

Expand All @@ -199,9 +191,6 @@ TEST_F(InSessionAuthDialogClientTest, PasswordAuthSuccess) {

run_loop.RunUntilIdle();
EXPECT_TRUE(result);
if (!ash::features::IsUseAuthsessionForWebAuthNEnabled()) {
EXPECT_TRUE(GetLastUnlockWebAuthnSecret());
}
}

} // namespace
3 changes: 2 additions & 1 deletion chromeos/ash/components/cryptohome/cryptohome_parameters.h
Expand Up @@ -71,10 +71,11 @@ struct COMPONENT_EXPORT(CHROMEOS_ASH_COMPONENTS_CRYPTOHOME) KeyDefinition {
// challenged is stored in |challenge_response_keys|, while |secret| should
// be empty.
TYPE_CHALLENGE_RESPONSE = 1,
// DEPRECATED:
// Fingerprint-based key. It doesn't carry secrets but indicates that
// cryptohome needs to query fingerprint scan results from biod and
// compare with the identity passed along with the key.
TYPE_FINGERPRINT = 2,
// TYPE_FINGERPRINT = 2,
// Public mount is used by Kiosk sessions. This type of key does not have
// any secret, instead crypotohomed would generate a key based on user
// identity.
Expand Down
5 changes: 0 additions & 5 deletions chromeos/ash/components/cryptohome/cryptohome_util.cc
Expand Up @@ -167,8 +167,6 @@ AuthorizationRequest CreateAuthorizationRequestFromKeyDef(
auth_request.mutable_key_delegate()->set_dbus_object_path(
cryptohome::kCryptohomeKeyDelegateServicePath);
break;
case KeyDefinition::TYPE_FINGERPRINT:
break;
case KeyDefinition::TYPE_PUBLIC_MOUNT:
break;
}
Expand Down Expand Up @@ -198,9 +196,6 @@ void KeyDefinitionToKey(const KeyDefinition& key_def, Key* key) {
}
break;

case KeyDefinition::TYPE_FINGERPRINT:
data->set_type(KeyData::KEY_TYPE_FINGERPRINT);
break;
case KeyDefinition::TYPE_PUBLIC_MOUNT:
data->set_type(KeyData::KEY_TYPE_KIOSK);
break;
Expand Down
Expand Up @@ -718,18 +718,6 @@ void FakeUserDataAuthClient::CheckKey(
}
}

void FakeUserDataAuthClient::StartFingerprintAuthSession(
const ::user_data_auth::StartFingerprintAuthSessionRequest& request,
StartFingerprintAuthSessionCallback callback) {
::user_data_auth::StartFingerprintAuthSessionReply reply;
ReplyOnReturn auto_reply(&reply, std::move(callback));
}
void FakeUserDataAuthClient::EndFingerprintAuthSession(
const ::user_data_auth::EndFingerprintAuthSessionRequest& request,
EndFingerprintAuthSessionCallback callback) {
::user_data_auth::EndFingerprintAuthSessionReply reply;
ReplyOnReturn auto_reply(&reply, std::move(callback));
}
void FakeUserDataAuthClient::StartMigrateToDircrypto(
const ::user_data_auth::StartMigrateToDircryptoRequest& request,
StartMigrateToDircryptoCallback callback) {
Expand Down
Expand Up @@ -200,12 +200,6 @@ class COMPONENT_EXPORT(USERDATAAUTH_CLIENT) FakeUserDataAuthClient
RemoveCallback callback) override;
void CheckKey(const ::user_data_auth::CheckKeyRequest& request,
CheckKeyCallback callback) override;
void StartFingerprintAuthSession(
const ::user_data_auth::StartFingerprintAuthSessionRequest& request,
StartFingerprintAuthSessionCallback callback) override;
void EndFingerprintAuthSession(
const ::user_data_auth::EndFingerprintAuthSessionRequest& request,
EndFingerprintAuthSessionCallback callback) override;
void StartMigrateToDircrypto(
const ::user_data_auth::StartMigrateToDircryptoRequest& request,
StartMigrateToDircryptoCallback callback) override;
Expand Down
Expand Up @@ -46,18 +46,6 @@ class COMPONENT_EXPORT(USERDATAAUTH_CLIENT) MockUserDataAuthClient
(const ::user_data_auth::CheckKeyRequest& request,
CheckKeyCallback callback),
(override));
MOCK_METHOD(
void,
StartFingerprintAuthSession,
(const ::user_data_auth::StartFingerprintAuthSessionRequest& request,
StartFingerprintAuthSessionCallback callback),
(override));
MOCK_METHOD(
void,
EndFingerprintAuthSession,
(const ::user_data_auth::EndFingerprintAuthSessionRequest& request,
EndFingerprintAuthSessionCallback callback),
(override));
MOCK_METHOD(void,
StartMigrateToDircrypto,
(const ::user_data_auth::StartMigrateToDircryptoRequest& request,
Expand Down
16 changes: 0 additions & 16 deletions chromeos/ash/components/dbus/userdataauth/userdataauth_client.cc
Expand Up @@ -131,22 +131,6 @@ class UserDataAuthClientImpl : public UserDataAuthClient {
std::move(callback));
}

void StartFingerprintAuthSession(
const ::user_data_auth::StartFingerprintAuthSessionRequest& request,
StartFingerprintAuthSessionCallback callback) override {
CallProtoMethod(::user_data_auth::kStartFingerprintAuthSession,
::user_data_auth::kUserDataAuthInterface, request,
std::move(callback));
}

void EndFingerprintAuthSession(
const ::user_data_auth::EndFingerprintAuthSessionRequest& request,
EndFingerprintAuthSessionCallback callback) override {
CallProtoMethod(::user_data_auth::kEndFingerprintAuthSession,
::user_data_auth::kUserDataAuthInterface, request,
std::move(callback));
}

void StartMigrateToDircrypto(
const ::user_data_auth::StartMigrateToDircryptoRequest& request,
StartMigrateToDircryptoCallback callback) override {
Expand Down

0 comments on commit 7c9dfdd

Please sign in to comment.