Skip to content

Commit

Permalink
ash: Add a "Need help?" button to in-session auth dialog
Browse files Browse the repository at this point in the history
When clicked, the help button will open a help article in a new
browser window, so that it's on top of the dialog but the user can
return to the dialog.

Bug: b:156258540, b:144861739
Change-Id: I1ff8f49e9926ed57f27f2eaca307d5180d574091
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518442
Commit-Queue: Yicheng Li <yichengli@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#824669}
  • Loading branch information
Yicheng Li authored and Commit Bot committed Nov 6, 2020
1 parent b964189 commit 974429f
Show file tree
Hide file tree
Showing 12 changed files with 101 additions and 9 deletions.
3 changes: 3 additions & 0 deletions ash/ash_strings.grd
Original file line number Diff line number Diff line change
Expand Up @@ -2492,6 +2492,9 @@ This file contains the strings for ash.
<message name="IDS_ASH_IN_SESSION_AUTH_FINGERPRINT_ACCESSIBLE_DISABLED_FROM_ATTEMPTS" desc="Accessibility text read by chromevox when the user has made too many unsuccessful fingerprint unlock attempts; fingerprint is now disabled until the user authenticates with a different authentication method">
Too many fingerprint attempts
</message>
<message name="IDS_ASH_IN_SESSION_AUTH_HELP" desc="Label of a button in the auth dialog to open a help article">
Need help?
</message>

<!-- Multi-profiles intro dialog -->
<message name="IDS_ASH_MULTIPROFILES_INTRO_HEADLINE" desc="Describes which feature multi-profiles intro dialog presents.">
Expand Down
1 change: 1 addition & 0 deletions ash/ash_strings_grd/IDS_ASH_IN_SESSION_AUTH_HELP.png.sha1
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
06bb45e3d732edafb1eab45781c08fdfdddeef78
16 changes: 14 additions & 2 deletions ash/in_session_auth/auth_dialog_contents_view.cc
Original file line number Diff line number Diff line change
Expand Up @@ -526,9 +526,21 @@ void AuthDialogContentsView::AddActionButtonsView() {
std::make_unique<views::BoxLayout>(
views::BoxLayout::Orientation::kHorizontal));
buttons_layout->set_main_axis_alignment(
views::BoxLayout::MainAxisAlignment::kEnd);
views::BoxLayout::MainAxisAlignment::kStart);

help_button_ =
action_view_container_->AddChildView(std::make_unique<views::LabelButton>(
base::BindRepeating(&AuthDialogContentsView::OnNeedHelpButtonPressed,
base::Unretained(this)),
l10n_util::GetStringUTF16(IDS_ASH_IN_SESSION_AUTH_HELP),
views::style::CONTEXT_BUTTON));
help_button_->SetEnabledTextColors(SK_ColorDKGRAY);
action_view_container_->SetPreferredSize(
gfx::Size(kContainerPreferredWidth, help_button_->height()));
}

// TODO(b/156258540): Add a "Need help?" button that links to a HC article.
void AuthDialogContentsView::OnNeedHelpButtonPressed(const ui::Event& event) {
InSessionAuthDialogController::Get()->OpenInSessionAuthHelpPage();
}

void AuthDialogContentsView::OnAuthSubmit(const base::string16& pin) {
Expand Down
7 changes: 7 additions & 0 deletions ash/in_session_auth/auth_dialog_contents_view.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
namespace views {
class BoxLayout;
class Label;
class LabelButton;
} // namespace views

namespace ash {
Expand Down Expand Up @@ -112,6 +113,9 @@ class AuthDialogContentsView : public views::View {
void OnFingerprintAuthComplete(bool success,
FingerprintState fingerprint_state);

// Called when the "Need help?" button is pressed.
void OnNeedHelpButtonPressed(const ui::Event& event);

// Debug container which holds the entire debug UI.
views::View* container_ = nullptr;

Expand Down Expand Up @@ -144,6 +148,9 @@ class AuthDialogContentsView : public views::View {

FingerprintView* fingerprint_view_ = nullptr;

// A button to show a help center article.
views::LabelButton* help_button_ = nullptr;

// Flags of auth methods that should be visible.
uint32_t auth_methods_ = 0u;

Expand Down
28 changes: 22 additions & 6 deletions ash/in_session_auth/in_session_auth_dialog_controller_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ void InSessionAuthDialogControllerImpl::ShowAuthenticationDialog(
// Concurrent requests are not supported.
DCHECK(!dialog_);

window_tracker_.Add(source_window);
source_window_tracker_.Add(source_window);
finish_callback_ = std::move(finish_callback);

AccountId account_id =
Expand Down Expand Up @@ -97,7 +97,7 @@ void InSessionAuthDialogControllerImpl::OnPinCanAuthenticate(
return;
}

if (!window_tracker_.Contains(source_window)) {
if (!source_window_tracker_.Contains(source_window)) {
LOG(ERROR) << "Source window is no longer available.";
Cancel();
return;
Expand All @@ -116,7 +116,7 @@ void InSessionAuthDialogControllerImpl::OnPinCanAuthenticate(
AuthDialogContentsView::AuthMethodsMetadata auth_metadata;
auth_metadata.autosubmit_pin_length =
user_manager::known_user::GetUserPinLength(account_id);
window_tracker_.Remove(source_window);
source_window_tracker_.Remove(source_window);
Shell::Get()->focus_controller()->AddObserver(this);
dialog_ = std::make_unique<InSessionAuthDialog>(
auth_methods, source_window, origin_name, auth_metadata, avatar);
Expand All @@ -131,7 +131,7 @@ void InSessionAuthDialogControllerImpl::DestroyAuthenticationDialog() {
client_->EndFingerprintAuthSession();

dialog_.reset();
window_tracker_.RemoveAll();
source_window_tracker_.RemoveAll();
Shell::Get()->focus_controller()->RemoveObserver(this);
}

Expand Down Expand Up @@ -197,9 +197,25 @@ void InSessionAuthDialogControllerImpl::Cancel() {
void InSessionAuthDialogControllerImpl::OnWindowFocused(
aura::Window* gained_focus,
aura::Window* lost_focus) {
if (dialog_ && lost_focus == dialog_->widget()->GetNativeWindow()) {
Cancel();
if (should_ignore_focus_change_)
return;

if (!dialog_)
return;

// No-op if focus moved to the help page or back to the dialog.
if (help_window_tracker_.Contains(gained_focus) ||
gained_focus == dialog_->widget()->GetNativeWindow()) {
return;
}

Cancel();
}

void InSessionAuthDialogControllerImpl::OpenInSessionAuthHelpPage() {
DCHECK(client_);
base::AutoReset<bool> scoped_ignore_focus(&should_ignore_focus_change_, true);
help_window_tracker_.Add(client_->OpenInSessionAuthHelpPage());
}

} // namespace ash
8 changes: 7 additions & 1 deletion ash/in_session_auth/in_session_auth_dialog_controller_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ class InSessionAuthDialogControllerImpl
OnAuthenticateCallback callback) override;
void AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, FingerprintState)> callback) override;
void OpenInSessionAuthHelpPage() override;
void Cancel() override;

// aura::client::FocusChangeObserver overrides
Expand Down Expand Up @@ -85,10 +86,15 @@ class InSessionAuthDialogControllerImpl

std::unique_ptr<InSessionAuthDialog> dialog_;

aura::WindowTracker window_tracker_;
aura::WindowTracker source_window_tracker_;

// Tracks windows that show the help article about in-session auth.
aura::WindowTracker help_window_tracker_;

std::unique_ptr<WebAuthnRequestRegistrarImpl> webauthn_request_registrar_;

bool should_ignore_focus_change_ = false;

base::WeakPtrFactory<InSessionAuthDialogControllerImpl> weak_factory_{this};
};

Expand Down
2 changes: 2 additions & 0 deletions ash/in_session_auth/mock_in_session_auth_dialog_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class MockInSessionAuthDialogClient : public InSessionAuthDialogClient {
AuthenticateUserWithFingerprint,
(base::OnceCallback<void(bool, FingerprintState)> callback),
(override));

MOCK_METHOD(aura::Window*, OpenInSessionAuthHelpPage, (), (const override));
};

} // namespace ash
Expand Down
7 changes: 7 additions & 0 deletions ash/public/cpp/in_session_auth_dialog_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@
#include "base/callback_forward.h"
#include "components/account_id/account_id.h"

namespace aura {
class Window;
}

namespace ash {

// An interface that allows Ash to trigger authentication steps that ChromeOS
Expand Down Expand Up @@ -46,6 +50,9 @@ class ASH_PUBLIC_EXPORT InSessionAuthDialogClient {
virtual void AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, FingerprintState)> callback) = 0;

// Open a help article in a new window and return the window.
virtual aura::Window* OpenInSessionAuthHelpPage() const = 0;

protected:
virtual ~InSessionAuthDialogClient() = default;
};
Expand Down
3 changes: 3 additions & 0 deletions ash/public/cpp/in_session_auth_dialog_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,9 @@ class ASH_PUBLIC_EXPORT InSessionAuthDialogController {
virtual void AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, FingerprintState)> callback) = 0;

// Opens a help article in Chrome.
virtual void OpenInSessionAuthHelpPage() = 0;

// Cancels all operations and destroys the dialog.
virtual void Cancel() = 0;

Expand Down
29 changes: 29 additions & 0 deletions chrome/browser/ui/ash/in_session_auth_dialog_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
#include "chrome/browser/chromeos/login/quick_unlock/pin_backend.h"
#include "chrome/browser/chromeos/login/quick_unlock/quick_unlock_factory.h"
#include "chrome/browser/chromeos/login/quick_unlock/quick_unlock_storage.h"
#include "chrome/browser/profiles/profile_manager.h"
#include "chrome/browser/ui/browser.h"
#include "chrome/browser/ui/browser_navigator.h"
#include "chrome/browser/ui/browser_navigator_params.h"
#include "chrome/browser/ui/browser_window.h"
#include "components/account_id/account_id.h"
#include "components/user_manager/user_manager.h"
#include "content/public/browser/browser_task_traits.h"
Expand All @@ -26,7 +31,14 @@ using chromeos::Key;
using chromeos::UserContext;

namespace {

// TODO(b/156258540): Replace with correct URL once the article is uploaded.
// This URL is an irrelevant article just for validating functionality.
const char kInSessionAuthHelpPageUrl[] =
"https://support.google.com/chrome/?p=settings_sign_in";

InSessionAuthDialogClient* g_auth_dialog_client_instance = nullptr;

} // namespace

InSessionAuthDialogClient::InSessionAuthDialogClient() {
Expand Down Expand Up @@ -207,6 +219,23 @@ void InSessionAuthDialogClient::OnFingerprintAuthDone(
}
}

aura::Window* InSessionAuthDialogClient::OpenInSessionAuthHelpPage() const {
// TODO(b/156258540): Use the profile of the source browser window.
Profile* profile = ProfileManager::GetActiveUserProfile();
// Create new browser window because the auth dialog is a child of the
// existing one.
NavigateParams params(profile, GURL(kInSessionAuthHelpPageUrl),
ui::PAGE_TRANSITION_AUTO_BOOKMARK);
params.disposition = WindowOpenDisposition::NEW_POPUP;
params.trusted_source = true;
params.window_action = NavigateParams::SHOW_WINDOW;
params.user_gesture = true;
params.path_behavior = NavigateParams::IGNORE_AND_NAVIGATE;
Navigate(&params);

return params.browser->window()->GetNativeWindow();
}

// AuthStatusConsumer:
void InSessionAuthDialogClient::OnAuthFailure(
const chromeos::AuthFailure& error) {
Expand Down
5 changes: 5 additions & 0 deletions chrome/browser/ui/ash/in_session_auth_dialog_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@
#include "chromeos/login/auth/extended_authenticator.h"
#include "chromeos/login/auth/user_context.h"

namespace aura {
class Window;
}

class AccountId;

// Handles method calls sent from Ash to ChromeOS.
Expand Down Expand Up @@ -46,6 +50,7 @@ class InSessionAuthDialogClient : public ash::InSessionAuthDialogClient,
base::OnceCallback<void(bool)> callback) override;
void AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, ash::FingerprintState)> callback) override;
aura::Window* OpenInSessionAuthHelpPage() const override;

// AuthStatusConsumer:
void OnAuthFailure(const chromeos::AuthFailure& error) override;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ class FakeInSessionAuthDialogController
void AuthenticateUserWithFingerprint(
base::OnceCallback<void(bool, ash::FingerprintState)> callback) override {
}
void OpenInSessionAuthHelpPage() override {}
void Cancel() override {}
};

Expand Down

0 comments on commit 974429f

Please sign in to comment.