Skip to content

Commit

Permalink
in_session_auth: Check PIN availability after each auth
Browse files Browse the repository at this point in the history 
The lock screen checks PIN availability after each auth so it could hide
the PIN input field and prompts the user for their password for correct
UX. WebAuthn dialog currently tries to match this behavior by keeping a
PIN attempt counter, but that's incorrect because the incorrect attempt
count is not necessarily 0 when the dialog is summoned.

Fix the implementation by checking PIN availability again after each
authentication failure to determine whether PIN field should be
disabled, instead of by keeping a counter.

The mentioned WebauthnPINLockout test to test this behavior is added in
https://crrev.com/c/3721192.

Bug: b:237244202
Test: manual test that "too many attempts" appear after lockout
Test: run the newly added WebauthnPINLockout test
Change-Id: I7a05881d67b40287eb048cd664b610c6bbe57453
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3726427
Reviewed-by: Elie Maamari <emaamari@google.com>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Commit-Queue: Howard Yang <hcyang@google.com>
Cr-Commit-Position: refs/heads/main@{#1018709}
  • Loading branch information
@hcyang-google
hcyang-google authored and Chromium LUCI CQ committed Jun 28, 2022
1 parent 95a334f commit a7820f7
Show file tree
Hide file tree
Showing 5 changed files with 95 additions and 41 deletions.
17 changes: 8 additions & 9 deletions ash/in_session_auth/auth_dialog_contents_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,6 @@ constexpr SkColor kErrorColor = gfx::kGoogleRed600;

constexpr int kSpacingBeforeButtons = 32;

constexpr int kMaxPinAttempts = 5;

} // namespace

// Consists of fingerprint icon view and a label.
Expand Down Expand Up @@ -572,7 +570,7 @@ void AuthDialogContentsView::AddActionButtonsView() {

void AuthDialogContentsView::OnInsertDigitFromPinPad(int digit) {
// Ignore anything if reached max attempts.
if (pin_attempts_ >= kMaxPinAttempts)
if (pin_locked_out_)
return;

if (title_->IsShowingError())
Expand All @@ -587,7 +585,7 @@ void AuthDialogContentsView::OnInsertDigitFromPinPad(int digit) {

void AuthDialogContentsView::OnBackspaceFromPinPad() {
// Ignore anything if reached max attempts.
if (pin_attempts_ >= kMaxPinAttempts)
if (pin_locked_out_)
return;

if (title_->IsShowingError())
Expand Down Expand Up @@ -634,16 +632,17 @@ void AuthDialogContentsView::OnAuthSubmit(bool authenticated_by_pin,

void AuthDialogContentsView::OnPasswordOrPinAuthComplete(
bool authenticated_by_pin,
absl::optional<bool> success) {
bool success,
bool can_use_pin) {
// On success, do nothing, and the dialog will dismiss.
if (success.has_value() && success.value())
if (success)
return;

std::u16string error_text;
if (authenticated_by_pin) {
pin_attempts_++;
pin_locked_out_ = !can_use_pin;
error_text =
pin_attempts_ >= kMaxPinAttempts
pin_locked_out_
? l10n_util::GetStringUTF16(
IDS_ASH_IN_SESSION_AUTH_PIN_TOO_MANY_ATTEMPTS)
: l10n_util::GetStringUTF16(IDS_ASH_IN_SESSION_AUTH_PIN_INCORRECT);
Expand All @@ -656,7 +655,7 @@ void AuthDialogContentsView::OnPasswordOrPinAuthComplete(
if (!authenticated_by_pin) {
password_view_->Reset();
password_view_->SetReadOnly(false);
} else if (pin_attempts_ < kMaxPinAttempts) {
} else if (can_use_pin) {
if (pin_autosubmit_on_) {
pin_digit_input_view_->Reset();
pin_digit_input_view_->SetReadOnly(false);
Expand Down
9 changes: 6 additions & 3 deletions ash/in_session_auth/auth_dialog_contents_view.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -112,8 +112,11 @@ class AuthDialogContentsView : public views::View {

// Called when password or PIN authentication of the user completes. If
// authenticated_by_pin is false, the user authenticated by password.
// |can_use_pin| specifies whether PIN is available after the authentication,
// as it might be locked out.
void OnPasswordOrPinAuthComplete(bool authenticated_by_pin,
absl::optional<bool> success);
bool success,
bool can_use_pin);

// Called when fingerprint authentication completes.
void OnFingerprintAuthComplete(bool success,
Expand Down Expand Up @@ -143,8 +146,8 @@ class AuthDialogContentsView : public views::View {
// Whether PIN can be auto submitted.
bool pin_autosubmit_on_ = false;

// Number of PIN attempts so far.
int pin_attempts_ = 0;
// Whether PIN is locked out.
bool pin_locked_out_ = false;

// Text input field for PIN if PIN cannot be auto submitted.
LoginPasswordView* pin_text_input_view_ = nullptr;
Expand Down
13 changes: 11 additions & 2 deletions ash/in_session_auth/webauthn_dialog_controller_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,18 @@ void WebAuthNDialogControllerImpl::AuthenticateUserWithFingerprint(
void WebAuthNDialogControllerImpl::OnAuthenticateComplete(
OnAuthenticateCallback callback,
bool success) {
std::move(callback).Run(success);
if (success)
if (success) {
std::move(callback).Run(/*success=*/true, /*can_use_pin=*/true);
OnAuthSuccess();
return;
}

// PIN might be locked out after an unsuccessful authentication, check if it's
// still available so the UI can be updated.
AccountId account_id =
Shell::Get()->session_controller()->GetActiveAccountId();
client_->CheckPinAuthAvailability(
account_id, base::BindOnce(std::move(callback), /*success=*/false));
}

void WebAuthNDialogControllerImpl::OnFingerprintAuthComplete(
Expand Down
88 changes: 66 additions & 22 deletions ash/in_session_auth/webauthn_dialog_controller_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,18 +31,17 @@ TEST_F(WebAuthNDialogControllerImplTest, PinAuthSuccess) {
std::move(controller_callback).Run(true);
});

absl::optional<bool> view_callback_result;
bool result_success = false;
controller->AuthenticateUserWithPasswordOrPin(
pin,
/*authenticated_by_password=*/true,
/* View callback will be executed during controller callback. */
base::BindLambdaForTesting(
[&view_callback_result](absl::optional<bool> did_auth) {
view_callback_result = did_auth;
[&result_success](bool success, bool can_use_pin) {
result_success = success;
}));

EXPECT_TRUE(view_callback_result.has_value());
EXPECT_TRUE(*view_callback_result);
EXPECT_TRUE(result_success);
}

TEST_F(WebAuthNDialogControllerImplTest, PinAuthFail) {
Expand All @@ -58,19 +57,61 @@ TEST_F(WebAuthNDialogControllerImplTest, PinAuthFail) {
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(false);
});
EXPECT_CALL(*client, CheckPinAuthAvailability(_, _))
.WillOnce([](const AccountId& account_id,
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(true);
});

absl::optional<bool> view_callback_result;
bool result_success = false;
bool result_can_use_pin = false;
controller->AuthenticateUserWithPasswordOrPin(
pin,
/*authenticated_by_password=*/true,
/* View callback will be executed during controller callback. */
base::BindLambdaForTesting(
[&view_callback_result](absl::optional<bool> did_auth) {
view_callback_result = did_auth;
}));
base::BindLambdaForTesting([&result_success, &result_can_use_pin](
bool success, bool can_use_pin) {
result_success = success;
result_can_use_pin = can_use_pin;
}));

EXPECT_FALSE(result_success);
EXPECT_TRUE(result_can_use_pin);
}

EXPECT_TRUE(view_callback_result.has_value());
EXPECT_FALSE(*view_callback_result);
TEST_F(WebAuthNDialogControllerImplTest, PinAuthFailLockout) {
WebAuthNDialogController* controller =
Shell::Get()->webauthn_dialog_controller();
auto client = std::make_unique<MockInSessionAuthDialogClient>();

std::string pin = "123456";

EXPECT_CALL(*client, AuthenticateUserWithPasswordOrPin(
pin, /* authenticated_by_pin = */ true, _))
.WillOnce([](const std::string& pin, bool authenticated_by_pin,
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(false);
});
EXPECT_CALL(*client, CheckPinAuthAvailability(_, _))
.WillOnce([](const AccountId& account_id,
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(false);
});

bool result_success = false;
bool result_can_use_pin = false;
controller->AuthenticateUserWithPasswordOrPin(
pin,
/*authenticated_by_password=*/true,
/* View callback will be executed during controller callback. */
base::BindLambdaForTesting([&result_success, &result_can_use_pin](
bool success, bool can_use_pin) {
result_success = success;
result_can_use_pin = can_use_pin;
}));

EXPECT_FALSE(result_success);
EXPECT_FALSE(result_can_use_pin);
}

TEST_F(WebAuthNDialogControllerImplTest, PasswordAuthSuccess) {
Expand All @@ -87,18 +128,17 @@ TEST_F(WebAuthNDialogControllerImplTest, PasswordAuthSuccess) {
std::move(controller_callback).Run(true);
});

absl::optional<bool> view_callback_result;
bool result_success = false;
controller->AuthenticateUserWithPasswordOrPin(
password,
/*authenticated_by_password=*/false,
/* View callback will be executed during controller callback. */
base::BindLambdaForTesting(
[&view_callback_result](absl::optional<bool> did_auth) {
view_callback_result = did_auth;
[&result_success](bool success, bool can_use_pin) {
result_success = success;
}));

EXPECT_TRUE(view_callback_result.has_value());
EXPECT_TRUE(*view_callback_result);
EXPECT_TRUE(result_success);
}

TEST_F(WebAuthNDialogControllerImplTest, PasswordAuthFail) {
Expand All @@ -114,19 +154,23 @@ TEST_F(WebAuthNDialogControllerImplTest, PasswordAuthFail) {
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(false);
});
EXPECT_CALL(*client, CheckPinAuthAvailability(_, _))
.WillOnce([](const AccountId& account_id,
base::OnceCallback<void(bool success)> controller_callback) {
std::move(controller_callback).Run(false);
});

absl::optional<bool> view_callback_result;
bool result_success = false;
controller->AuthenticateUserWithPasswordOrPin(
password,
/*authenticated_by_password=*/false,
/* View callback will be executed during controller callback. */
base::BindLambdaForTesting(
[&view_callback_result](absl::optional<bool> did_auth) {
view_callback_result = did_auth;
[&result_success](bool success, bool can_use_pin) {
result_success = success;
}));

EXPECT_TRUE(view_callback_result.has_value());
EXPECT_FALSE(*view_callback_result);
EXPECT_FALSE(result_success);
}

} // namespace
Expand Down
9 changes: 4 additions & 5 deletions ash/public/cpp/webauthn_dialog_controller.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include "ash/public/cpp/ash_public_export.h"
#include "ash/public/cpp/in_session_auth_dialog_client.h"
#include "third_party/abseil-cpp/absl/types/optional.h"

namespace aura {
class Window;
Expand All @@ -18,11 +17,11 @@ namespace ash {
// WebAuthNDialogController manages the webauthn auth dialog.
class ASH_PUBLIC_EXPORT WebAuthNDialogController {
public:
// Callback for authentication checks. |success| is nullopt if an
// authentication check did not run, otherwise it is true/false if auth
// succeeded/failed.
// Callback for authentication checks. |success| true/false if auth
// succeeded/failed, and |can_use_pin| indicates whether PIN can still be used
// (not locked out) after the previous authentication.
using OnAuthenticateCallback =
base::OnceCallback<void(absl::optional<bool> success)>;
base::OnceCallback<void(bool success, bool can_use_pin)>;
// Callback for overall authentication flow result.
using FinishCallback = base::OnceCallback<void(bool success)>;

Expand Down

0 comments on commit a7820f7

Please sign in to comment.