Skip to content

Commit

Permalink
Encryption migration: reuse authsession on skipped attempt
Browse files Browse the repository at this point in the history
Bug: b:259432952
Change-Id: Ie08648afd6feaf3844fa2332aea5ae76ba34cf97
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4057527
Reviewed-by: Maksim Ivanov <emaxx@chromium.org>
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Commit-Queue: Jeroen Dhollander <jeroendh@google.com>
Auto-Submit: Denis Kuznetsov <antrim@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1076219}
  • Loading branch information
Denis Kuznetsov authored and Chromium LUCI CQ committed Nov 28, 2022
1 parent ae758d4 commit 1eeead3
Show file tree
Hide file tree
Showing 26 changed files with 120 additions and 68 deletions.
4 changes: 2 additions & 2 deletions chrome/browser/ash/app_mode/kiosk_profile_loader.cc
Original file line number Diff line number Diff line change
Expand Up @@ -217,9 +217,9 @@ void KioskProfileLoader::PolicyLoadFailed() {
}

void KioskProfileLoader::OnOldEncryptionDetected(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
bool has_incomplete_migration) {
delegate_->OnOldEncryptionDetected(user_context);
delegate_->OnOldEncryptionDetected(std::move(user_context));
}

void KioskProfileLoader::OnProfilePrepared(Profile* profile,
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ash/app_mode/kiosk_profile_loader.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ class KioskProfileLoader : public LoginPerformer::Delegate,
public:
virtual void OnProfileLoaded(Profile* profile) = 0;
virtual void OnProfileLoadFailed(KioskAppLaunchError::Error error) = 0;
virtual void OnOldEncryptionDetected(const UserContext& user_context) = 0;
virtual void OnOldEncryptionDetected(
std::unique_ptr<UserContext> user_context) = 0;

protected:
virtual ~Delegate() {}
Expand All @@ -60,7 +61,7 @@ class KioskProfileLoader : public LoginPerformer::Delegate,
void OnAuthFailure(const AuthFailure& error) override;
void AllowlistCheckFailed(const std::string& email) override;
void PolicyLoadFailed() override;
void OnOldEncryptionDetected(const UserContext& user_context,
void OnOldEncryptionDetected(std::unique_ptr<UserContext> user_context,
bool has_incomplete_migration) override;

// UserSessionManagerDelegate implementation:
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ash/login/app_mode/kiosk_launch_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -587,7 +587,7 @@ void KioskLaunchController::OnProfileLoadFailed(
}

void KioskLaunchController::OnOldEncryptionDetected(
const UserContext& user_context) {
std::unique_ptr<UserContext> user_context) {
if (kiosk_app_id_.type != KioskAppType::kArcApp) {
NOTREACHED();
return;
Expand All @@ -597,7 +597,7 @@ void KioskLaunchController::OnOldEncryptionDetected(
static_cast<EncryptionMigrationScreen*>(
host_->GetWizardController()->current_screen());
DCHECK(migration_screen);
migration_screen->SetUserContext(user_context);
migration_screen->SetUserContext(std::move(user_context));
migration_screen->SetupInitialView();
}

Expand Down
3 changes: 2 additions & 1 deletion chrome/browser/ash/login/app_mode/kiosk_launch_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ class KioskLaunchController : public KioskProfileLoader::Delegate,
// KioskProfileLoader::Delegate:
void OnProfileLoaded(Profile* profile) override;
void OnProfileLoadFailed(KioskAppLaunchError::Error error) override;
void OnOldEncryptionDetected(const UserContext& user_context) override;
void OnOldEncryptionDetected(
std::unique_ptr<UserContext> user_context) override;

void OnOwnerSigninSuccess();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ class FakeKioskProfileLoaderDelegate : public KioskProfileLoader::Delegate {
public:
MOCK_METHOD1(OnProfileLoaded, void(Profile*));
MOCK_METHOD1(OnProfileLoadFailed, void(KioskAppLaunchError::Error));
MOCK_METHOD1(OnOldEncryptionDetected, void(const UserContext&));
MOCK_METHOD1(OnOldEncryptionDetected, void(std::unique_ptr<UserContext>));
};

class WebKioskTest : public OobeBaseTest {
Expand Down
23 changes: 11 additions & 12 deletions chrome/browser/ash/login/existing_user_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -671,16 +671,15 @@ void ExistingUserController::PerformLogin(

void ExistingUserController::ContinuePerformLogin(
LoginPerformer::AuthorizationMode auth_mode,
const UserContext& user_context) {
login_performer_->PerformLogin(user_context, auth_mode);
std::unique_ptr<UserContext> user_context) {
login_performer_->LoginAuthenticated(std::move(user_context));
}

void ExistingUserController::ContinuePerformLoginWithoutMigration(
LoginPerformer::AuthorizationMode auth_mode,
const UserContext& user_context) {
UserContext user_context_ecryptfs = user_context;
user_context_ecryptfs.SetIsForcingDircrypto(false);
ContinuePerformLogin(auth_mode, user_context_ecryptfs);
std::unique_ptr<UserContext> user_context) {
user_context->SetIsForcingDircrypto(false);
ContinuePerformLogin(auth_mode, std::move(user_context));
}

void ExistingUserController::OnGaiaScreenReady() {
Expand Down Expand Up @@ -741,10 +740,10 @@ void ExistingUserController::ShowKioskEnableScreen() {
}

void ExistingUserController::ShowEncryptionMigrationScreen(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
EncryptionMigrationMode migration_mode) {
GetLoginDisplayHost()->GetSigninUI()->StartEncryptionMigration(
user_context, migration_mode,
std::move(user_context), migration_mode,
base::BindOnce(&ExistingUserController::ContinuePerformLogin,
weak_factory_.GetWeakPtr(),
login_performer_->auth_mode()));
Expand Down Expand Up @@ -1077,16 +1076,16 @@ void ExistingUserController::OnPasswordChangeDetected(
}

void ExistingUserController::OnOldEncryptionDetected(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
bool has_incomplete_migration) {
absl::optional<EncryptionMigrationMode> encryption_migration_mode =
GetEncryptionMigrationMode(user_context, has_incomplete_migration);
GetEncryptionMigrationMode(*user_context, has_incomplete_migration);
if (!encryption_migration_mode.has_value()) {
ContinuePerformLoginWithoutMigration(login_performer_->auth_mode(),
user_context);
std::move(user_context));
return;
}
ShowEncryptionMigrationScreen(user_context,
ShowEncryptionMigrationScreen(std::move(user_context),
encryption_migration_mode.value());
}

Expand Down
8 changes: 4 additions & 4 deletions chrome/browser/ash/login/existing_user_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ class ExistingUserController : public LoginDisplay::Delegate,
void OnAuthSuccess(const UserContext& user_context) override;
void OnOffTheRecordAuthSuccess() override;
void OnPasswordChangeDetected(const UserContext& user_context) override;
void OnOldEncryptionDetected(const UserContext& user_context,
void OnOldEncryptionDetected(std::unique_ptr<UserContext>,
bool has_incomplete_migration) override;
void AllowlistCheckFailed(const std::string& email) override;
void PolicyLoadFailed() override;
Expand Down Expand Up @@ -208,7 +208,7 @@ class ExistingUserController : public LoginDisplay::Delegate,
void ShowKioskEnableScreen();

// Shows "filesystem encryption migration" screen.
void ShowEncryptionMigrationScreen(const UserContext& user_context,
void ShowEncryptionMigrationScreen(std::unique_ptr<UserContext> user_context,
EncryptionMigrationMode migration_mode);

// Shows "critical TPM error" screen.
Expand All @@ -223,13 +223,13 @@ class ExistingUserController : public LoginDisplay::Delegate,

// Calls login() on previously-used `login_performer_`.
void ContinuePerformLogin(LoginPerformer::AuthorizationMode auth_mode,
const UserContext& user_context);
std::unique_ptr<UserContext> user_context);

// Removes the constraint that user home mount requires ext4 encryption from
// `user_context`, then calls login() on previously-used `login_performer`.
void ContinuePerformLoginWithoutMigration(
LoginPerformer::AuthorizationMode auth_mode,
const UserContext& user_context);
std::unique_ptr<UserContext> user_context);

// Asks the user to enter their password again.
void RestartLogin(const UserContext& user_context);
Expand Down
20 changes: 10 additions & 10 deletions chrome/browser/ash/login/screens/encryption_migration_screen.cc
Original file line number Diff line number Diff line change
Expand Up @@ -258,8 +258,8 @@ void EncryptionMigrationScreen::ShowImpl() {
void EncryptionMigrationScreen::HideImpl() {}

void EncryptionMigrationScreen::SetUserContext(
const UserContext& user_context) {
user_context_ = user_context;
std::unique_ptr<UserContext> user_context) {
user_context_ = std::move(user_context);
}

void EncryptionMigrationScreen::SetMode(EncryptionMigrationMode mode) {
Expand Down Expand Up @@ -361,8 +361,8 @@ void EncryptionMigrationScreen::HandleSkipMigration() {
// In this case, the user can not launch ARC apps in the session, and will be
// asked to do the migration again in the next log-in attempt.
if (!skip_migration_callback_.is_null()) {
user_context_.SetIsForcingDircrypto(false);
std::move(skip_migration_callback_).Run(user_context_);
user_context_->SetIsForcingDircrypto(false);
std::move(skip_migration_callback_).Run(std::move(user_context_));
}
}

Expand Down Expand Up @@ -481,7 +481,7 @@ void EncryptionMigrationScreen::StartMigration() {
// Mount the existing eCryptfs vault to a temporary location for migration.
user_data_auth::MountRequest mount;
*mount.mutable_account() = cryptohome::CreateAccountIdentifierFromAccountId(
user_context_.GetAccountId());
user_context_->GetAccountId());
cryptohome::AuthorizationRequest auth_request;
mount.set_to_migrate_from_ecryptfs(true);
if (IsArcKiosk()) {
Expand Down Expand Up @@ -509,7 +509,7 @@ void EncryptionMigrationScreen::OnMountExistingVault(
user_data_auth::StartMigrateToDircryptoRequest request;
*request.mutable_account_id() =
cryptohome::CreateAccountIdentifierFromAccountId(
user_context_.GetAccountId());
user_context_->GetAccountId());
userdataauth_observer_ = std::make_unique<base::ScopedObservation<
UserDataAuthClient, UserDataAuthClient::Observer>>(this);
userdataauth_observer_->Observe(UserDataAuthClient::Get());
Expand Down Expand Up @@ -542,10 +542,10 @@ void EncryptionMigrationScreen::RemoveCryptohome() {
// Set invalid token status so that user is forced to go through Gaia on the
// next sign-in.
user_manager::UserManager::Get()->SaveUserOAuthStatus(
user_context_.GetAccountId(),
user_context_->GetAccountId(),
user_manager::User::OAUTH2_TOKEN_STATUS_INVALID);

const cryptohome::Identification cryptohome_id(user_context_.GetAccountId());
const cryptohome::Identification cryptohome_id(user_context_->GetAccountId());

user_data_auth::RemoveRequest request;
request.mutable_identifier()->set_account_id(cryptohome_id.id());
Expand All @@ -572,7 +572,7 @@ void EncryptionMigrationScreen::OnRemoveCryptohome(
cryptohome::AuthorizationRequest
EncryptionMigrationScreen::CreateAuthorizationRequest() {
// |key| is created in the same manner as CryptohomeAuthenticator.
const Key* key = user_context_.GetKey();
const Key* key = user_context_->GetKey();
// If the |key| is a plain text password, crash rather than attempting to
// mount the cryptohome with a plain text password.
CHECK_NE(Key::KEY_TYPE_PASSWORD_PLAIN, key->GetKeyType());
Expand All @@ -588,7 +588,7 @@ EncryptionMigrationScreen::CreateAuthorizationRequest() {
}

bool EncryptionMigrationScreen::IsArcKiosk() const {
return user_context_.GetUserType() == user_manager::USER_TYPE_ARC_KIOSK_APP;
return user_context_->GetUserType() == user_manager::USER_TYPE_ARC_KIOSK_APP;
}

void EncryptionMigrationScreen::DircryptoMigrationProgress(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,8 @@ class EncryptionMigrationScreen : public BaseScreen,
public:
using TView = EncryptionMigrationScreenView;

using SkipMigrationCallback = base::OnceCallback<void(const UserContext&)>;
using SkipMigrationCallback =
base::OnceCallback<void(std::unique_ptr<UserContext>)>;

class EncryptionMigrationScreenTestDelegate {
public:
Expand All @@ -55,7 +56,7 @@ class EncryptionMigrationScreen : public BaseScreen,
~EncryptionMigrationScreen() override;

// Sets the UserContext for a user whose cryptohome should be migrated.
void SetUserContext(const UserContext& user_context);
void SetUserContext(std::unique_ptr<UserContext> user_context);

// Sets the migration mode.
void SetMode(EncryptionMigrationMode mode);
Expand Down Expand Up @@ -137,7 +138,7 @@ class EncryptionMigrationScreen : public BaseScreen,

// The current user's UserContext, which is used to request the migration to
// cryptohome.
UserContext user_context_;
std::unique_ptr<UserContext> user_context_;

// The callback which is used to log in to the session from the migration UI.
SkipMigrationCallback skip_migration_callback_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ class EncryptionMigrationScreenTest : public testing::Test {
chromeos::PowerManagerClient::Get());

// Build dummy user context.
user_context_.SetAccountId(account_id_);
user_context_.SetKey(
auto user_context = std::make_unique<UserContext>();
user_context->SetAccountId(account_id_);
user_context->SetKey(
Key(Key::KeyType::KEY_TYPE_SALTED_SHA256, "salt", "secret"));

encryption_migration_screen_ =
Expand All @@ -139,7 +140,7 @@ class EncryptionMigrationScreenTest : public testing::Test {
base::Unretained(this)));
encryption_migration_screen_->set_free_disk_space(
arc::kMigrationMinimumAvailableStorage);
encryption_migration_screen_->SetUserContext(user_context_);
encryption_migration_screen_->SetUserContext(std::move(user_context));
}

void TearDown() override {
Expand Down Expand Up @@ -168,12 +169,11 @@ class EncryptionMigrationScreenTest : public testing::Test {

const AccountId account_id_ =
AccountId::FromUserEmail(user_manager::kStubUserEmail);
UserContext user_context_;

private:
// This will be called by EncryptionMigrationScreen upon finished
// minimal migration when sign-in should continue.
void OnContinueLogin(const UserContext& user_context) {
void OnContinueLogin(std::unique_ptr<UserContext> user_context) {
EXPECT_FALSE(skip_migration_callback_called_)
<< "ContinueLogin/RestartLogin may only be called once.";

Expand Down
6 changes: 3 additions & 3 deletions chrome/browser/ash/login/ui/login_display_host_common.cc
Original file line number Diff line number Diff line change
Expand Up @@ -515,16 +515,16 @@ void LoginDisplayHostCommon::ClearOnboardingAuthSession() {
}

void LoginDisplayHostCommon::StartEncryptionMigration(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
EncryptionMigrationMode migration_mode,
base::OnceCallback<void(const UserContext&)> on_skip_migration) {
base::OnceCallback<void(std::unique_ptr<UserContext>)> on_skip_migration) {
StartWizard(EncryptionMigrationScreenView::kScreenId);

EncryptionMigrationScreen* migration_screen =
GetWizardController()->GetScreen<EncryptionMigrationScreen>();

DCHECK(migration_screen);
migration_screen->SetUserContext(user_context);
migration_screen->SetUserContext(std::move(user_context));
migration_screen->SetMode(migration_mode);
migration_screen->SetSkipMigrationCallback(std::move(on_skip_migration));
migration_screen->SetupInitialView();
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ash/login/ui/login_display_host_common.h
Original file line number Diff line number Diff line change
Expand Up @@ -81,9 +81,10 @@ class LoginDisplayHostCommon : public LoginDisplayHost,
void ShowTosForExistingUser() final;
void ShowNewTermsForFlexUsers() final;
void StartEncryptionMigration(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
EncryptionMigrationMode migration_mode,
base::OnceCallback<void(const UserContext&)> on_skip_migration) final;
base::OnceCallback<void(std::unique_ptr<UserContext>)> on_skip_migration)
final;
void ShowSigninError(SigninError error, const std::string& details) final;
void SAMLConfirmPassword(::login::StringList scraped_passwords,
std::unique_ptr<UserContext> user_context) final;
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ash/login/ui/login_display_host_mojo.cc
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,7 @@ void LoginDisplayHostMojo::OnPasswordChangeDetected(
}

void LoginDisplayHostMojo::OnOldEncryptionDetected(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
bool has_incomplete_migration) {}

void LoginDisplayHostMojo::OnCurrentScreenChanged(OobeScreenId current_screen,
Expand Down
2 changes: 1 addition & 1 deletion chrome/browser/ash/login/ui/login_display_host_mojo.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ class LoginDisplayHostMojo : public LoginDisplayHostCommon,
void OnAuthFailure(const AuthFailure& error) override;
void OnAuthSuccess(const UserContext& user_context) override;
void OnPasswordChangeDetected(const UserContext& user_context) override;
void OnOldEncryptionDetected(const UserContext& user_context,
void OnOldEncryptionDetected(std::unique_ptr<UserContext> user_context,
bool has_incomplete_migration) override;

// OobeUI::Observer:
Expand Down
4 changes: 2 additions & 2 deletions chrome/browser/ash/login/ui/mock_signin_ui.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,9 @@ class MockSigninUI : public SigninUI {
MOCK_METHOD(void, ShowNewTermsForFlexUsers, (), (override));
MOCK_METHOD(void,
StartEncryptionMigration,
(const UserContext&,
(std::unique_ptr<UserContext>,
EncryptionMigrationMode,
base::OnceCallback<void(const UserContext&)>),
base::OnceCallback<void(std::unique_ptr<UserContext>)>),
(override));
MOCK_METHOD(void,
SetAuthSessionForOnboarding,
Expand Down
5 changes: 3 additions & 2 deletions chrome/browser/ash/login/ui/signin_ui.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,9 +58,10 @@ class SigninUI {
virtual void ShowNewTermsForFlexUsers() = 0;

virtual void StartEncryptionMigration(
const UserContext& user_context,
std::unique_ptr<UserContext> user_context,
EncryptionMigrationMode migration_mode,
base::OnceCallback<void(const UserContext&)> skip_migration_callback) = 0;
base::OnceCallback<void(std::unique_ptr<UserContext>)>
skip_migration_callback) = 0;

// Might store authentication data so that additional auth factors can be
// added during user onboarding.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ class ManagedSessionService

void OnPasswordChangeDetected(const ash::UserContext& user_context) override {
}
void OnOldEncryptionDetected(const ash::UserContext& user_context,
void OnOldEncryptionDetected(std::unique_ptr<ash::UserContext> user_context,
bool has_incomplete_migration) override {}
void OnAuthSuccess(const ash::UserContext& user_context) override {}

Expand Down

0 comments on commit 1eeead3

Please sign in to comment.