Skip to content

Commit

Permalink
Fix: Reject user connection if user session is in progress
Browse files Browse the repository at this point in the history 
There was a crash reported with a DCHECK because an object of
class UserManager was being accessed on a wrong sequence. This
change fixes the call by adding using the variable on the
main (UI) sequence.

BUG: b/246752757
TEST: Updated unittest.
Change-Id: Ie8ec19d1a36ecfa6ab0e431ad7ff79aea892666d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4020196
Reviewed-by: Lambros Lambrou <lambroslambrou@chromium.org>
Commit-Queue: Ashutosh Singhal <macinashutosh@google.com>
Reviewed-by: Jeroen Dhollander <jeroendh@google.com>
Cr-Commit-Position: refs/heads/main@{#1070702}
  • Loading branch information
Ashutosh Singhal authored and Chromium LUCI CQ committed Nov 12, 2022
1 parent 364ea96 commit 20b52fd
Show file tree
Hide file tree
Showing 3 changed files with 104 additions and 46 deletions.
79 changes: 53 additions & 26 deletions remoting/host/it2me_desktop_environment.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,16 @@
#include <utility>

#include "base/check.h"
#include "base/functional/bind.h"
#include "base/memory/weak_ptr.h"
#include "base/task/single_thread_task_runner.h"
#include "build/build_config.h"
#include "remoting/host/client_session_control.h"
#include "remoting/host/host_window.h"
#include "remoting/host/host_window_proxy.h"
#include "remoting/host/input_monitor/local_input_monitor.h"
#include "remoting/host/session_terminator.h"
#include "remoting/protocol/errors.h"

#if BUILDFLAG(IS_POSIX)
#include <sys/types.h>
Expand All @@ -30,6 +33,18 @@

namespace remoting {

#if BUILDFLAG(IS_CHROMEOS)
namespace {

bool IsUserLoggedIn() {
const auto* user_manager = user_manager::UserManager::Get();
DCHECK(user_manager);
return user_manager->IsUserLoggedIn();
}

} // namespace
#endif

It2MeDesktopEnvironment::~It2MeDesktopEnvironment() {
DCHECK(caller_task_runner()->BelongsToCurrentThread());
}
Expand Down Expand Up @@ -91,35 +106,49 @@ It2MeDesktopEnvironment::It2MeDesktopEnvironment(
}
}

bool It2MeDesktopEnvironment::InitializeCurtainMode() {
void It2MeDesktopEnvironment::InitializeCurtainMode(
base::WeakPtr<ClientSessionControl> client_session_control) {
#if BUILDFLAG(IS_CHROMEOS)
if (base::FeatureList::IsEnabled(features::kEnableCrdAdminRemoteAccess)) {
if (desktop_environment_options().enable_curtaining()) {
const auto* user_manager = user_manager::UserManager::Get();
// Don't allow the remote admin to hijack and curtain off a user's
// session.
if (user_manager->IsUserLoggedIn()) {
LOG(ERROR) << "Failed to activate curtain mode because a user is "
"currently logged in.";
return false;
}

curtain_mode_ = std::make_unique<CurtainModeChromeOs>(ui_task_runner());
if (!curtain_mode_->Activate()) {
LOG(ERROR) << "Failed to activate the curtain mode.";
curtain_mode_ = nullptr;
return false;
}

// Log out the current user when a curtained off session is disconnected,
// to prevent a local passerby from gaining control of the logged-in
// session when they unplug the ethernet cable.
session_terminator_ = SessionTerminator::Create(ui_task_runner());
return true;
ui_task_runner()->PostTaskAndReplyWithResult(
FROM_HERE, base::BindOnce(&IsUserLoggedIn),
base::BindOnce(
&It2MeDesktopEnvironment::InitializeCurtainModeIfNoUserLoggedIn,
weak_ptr_factory_.GetWeakPtr(), client_session_control));
}
}
#endif // BUILDFLAG(IS_CHROMEOS)
return true;
}

void It2MeDesktopEnvironment::InitializeCurtainModeIfNoUserLoggedIn(
base::WeakPtr<ClientSessionControl> client_session_control,
bool is_user_logged_in) {
#if BUILDFLAG(IS_CHROMEOS)
// Don't allow the remote admin to hijack and curtain off a user's
// session.
if (is_user_logged_in) {
LOG(ERROR) << "Failed to activate curtain mode because a user is "
"currently logged in.";
client_session_control->DisconnectSession(
protocol::ErrorCode::HOST_CONFIGURATION_ERROR);
return;
}

curtain_mode_ = std::make_unique<CurtainModeChromeOs>(ui_task_runner());
if (!curtain_mode_->Activate()) {
LOG(ERROR) << "Failed to activate the curtain mode.";
curtain_mode_ = nullptr;
client_session_control->DisconnectSession(
protocol::ErrorCode::HOST_CONFIGURATION_ERROR);
return;
}

// Log out the current user when a curtained off session is disconnected,
// to prevent a local passerby from gaining control of the logged-in
// session when they unplug the ethernet cable.
session_terminator_ = SessionTerminator::Create(ui_task_runner());
#endif
}

It2MeDesktopEnvironmentFactory::It2MeDesktopEnvironmentFactory(
Expand All @@ -144,9 +173,7 @@ std::unique_ptr<DesktopEnvironment> It2MeDesktopEnvironmentFactory::Create(
caller_task_runner(), video_capture_task_runner(), input_task_runner(),
ui_task_runner(), client_session_control, options));

if (!result->InitializeCurtainMode())
return nullptr;

result->InitializeCurtainMode(client_session_control);
return result;
}

Expand Down
9 changes: 8 additions & 1 deletion remoting/host/it2me_desktop_environment.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "base/memory/weak_ptr.h"
#include "base/task/single_thread_task_runner.h"
#include "remoting/host/basic_desktop_environment.h"
#include "remoting/host/client_session_control.h"
#include "remoting/host/curtain_mode.h"

namespace remoting {
Expand All @@ -29,7 +30,8 @@ class It2MeDesktopEnvironment : public BasicDesktopEnvironment {

// Initializes the curtain mode if needed.
// Returns `false` if the curtain mode failed to start for any reason.
bool InitializeCurtainMode();
void InitializeCurtainMode(
base::WeakPtr<ClientSessionControl> client_session_control);

bool is_curtained() const { return curtain_mode_ != nullptr; }

Expand All @@ -44,6 +46,10 @@ class It2MeDesktopEnvironment : public BasicDesktopEnvironment {
const DesktopEnvironmentOptions& options);

private:
void InitializeCurtainModeIfNoUserLoggedIn(
base::WeakPtr<ClientSessionControl> client_session_control,
bool is_user_logged_in);

// Presents the continue window to the local user.
std::unique_ptr<HostWindow> continue_window_;

Expand All @@ -55,6 +61,7 @@ class It2MeDesktopEnvironment : public BasicDesktopEnvironment {

std::unique_ptr<CurtainMode> curtain_mode_;
std::unique_ptr<SessionTerminator> session_terminator_;
base::WeakPtrFactory<It2MeDesktopEnvironment> weak_ptr_factory_{this};
};

// Used to create |It2MeDesktopEnvironment| instances.
Expand Down
62 changes: 43 additions & 19 deletions remoting/host/it2me_desktop_environment_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#include "remoting/host/client_session_control.h"
#include "remoting/host/client_session_events.h"
#include "remoting/proto/control.pb.h"
#include "remoting/protocol/errors.h"
#include "testing/gmock/include/gmock/gmock.h"
#include "testing/gtest/include/gtest/gtest.h"

Expand All @@ -40,30 +41,32 @@ namespace {
using ::testing::Eq;
using ::testing::IsNull;

class FakeClientSessionControl : public ClientSessionControl {
class ClientSessionControlMock : public ClientSessionControl {
public:
FakeClientSessionControl() = default;
FakeClientSessionControl(const FakeClientSessionControl&) = delete;
FakeClientSessionControl& operator=(const FakeClientSessionControl&) = delete;
~FakeClientSessionControl() override = default;
ClientSessionControlMock() = default;
ClientSessionControlMock(const ClientSessionControlMock&) = delete;
ClientSessionControlMock& operator=(const ClientSessionControlMock&) = delete;
~ClientSessionControlMock() override = default;

// ClientSessionControl implementation:
const std::string& client_jid() const override { return client_jid_; }
void DisconnectSession(protocol::ErrorCode error) override {}
void OnLocalPointerMoved(const webrtc::DesktopVector& position,
ui::EventType type) override {}
void OnLocalKeyPressed(uint32_t usb_keycode) override {}
void SetDisableInputs(bool disable_inputs) override {}
void OnDesktopDisplayChanged(
std::unique_ptr<protocol::VideoLayout> layout) override {}

base::WeakPtr<FakeClientSessionControl> GetWeakPtr() {
MOCK_METHOD(void, DisconnectSession, (protocol::ErrorCode error));
MOCK_METHOD(void,
OnLocalPointerMoved,
(const webrtc::DesktopVector& position, ui::EventType type));
MOCK_METHOD(void, OnLocalKeyPressed, (uint32_t usb_keycode));
MOCK_METHOD(void, SetDisableInputs, (bool disable_inputs));
MOCK_METHOD(void,
OnDesktopDisplayChanged,
(std::unique_ptr<protocol::VideoLayout> layout));

base::WeakPtr<ClientSessionControlMock> GetWeakPtr() {
return weak_ptr_factory_.GetWeakPtr();
}

private:
std::string client_jid_ = "<fake-client-jid>";
base::WeakPtrFactory<FakeClientSessionControl> weak_ptr_factory_{this};
base::WeakPtrFactory<ClientSessionControlMock> weak_ptr_factory_{this};
};

class FakeClientSessionEvents : public ClientSessionEvents {
Expand Down Expand Up @@ -150,8 +153,13 @@ class It2MeDesktopEnvironmentTest : public ::testing::Test {
.Create(session_control_.GetWeakPtr(),
session_events_.GetWeakPtr(), options);
// Cast to It2MeDesktopEnvironment
return std::unique_ptr<It2MeDesktopEnvironment>(
auto desktop_environment = std::unique_ptr<It2MeDesktopEnvironment>(
static_cast<It2MeDesktopEnvironment*>(base_ptr.release()));

// Give the code time to instantiate the curtain mode on the UI sequence (if
// needed).
FlushUiSequence();
return desktop_environment;
}

std::unique_ptr<It2MeDesktopEnvironment> CreateCurtainedSession() {
Expand Down Expand Up @@ -188,13 +196,15 @@ class It2MeDesktopEnvironmentTest : public ::testing::Test {
user_manager().RemoveUserFromList(account_id);
}

ClientSessionControlMock& session_control() { return session_control_; }

#endif // BUILDFLAG(IS_CHROMEOS)

private:
base::test::SingleThreadTaskEnvironment environment_;

base::test::ScopedFeatureList feature_list_;
FakeClientSessionControl session_control_;
testing::StrictMock<ClientSessionControlMock> session_control_;
FakeClientSessionEvents session_events_;

#if BUILDFLAG(IS_CHROMEOS)
Expand Down Expand Up @@ -223,6 +233,8 @@ TEST_F(It2MeDesktopEnvironmentTest,
options.set_enable_curtaining(true);

auto desktop_environment = Create(options);
FlushUiSequence();

EXPECT_THAT(desktop_environment->is_curtained(), Eq(true));
}

Expand All @@ -235,6 +247,8 @@ TEST_F(It2MeDesktopEnvironmentTest,
options.set_enable_curtaining(false);

auto desktop_environment = Create(options);
FlushUiSequence();

EXPECT_THAT(desktop_environment->is_curtained(), Eq(false));
}

Expand All @@ -247,6 +261,8 @@ TEST_F(It2MeDesktopEnvironmentTest,
options.set_enable_curtaining(true);

auto desktop_environment = Create(options);
FlushUiSequence();

EXPECT_THAT(desktop_environment->is_curtained(), Eq(false));
}

Expand Down Expand Up @@ -328,19 +344,27 @@ TEST_F(It2MeDesktopEnvironmentTest,
base::test::ScopedFeatureList feature_list{
features::kEnableCrdAdminRemoteAccess};

EXPECT_CALL(session_control(),
DisconnectSession(protocol::ErrorCode::HOST_CONFIGURATION_ERROR));

LogInUser();
auto desktop_environment = CreateCurtainedSession();

EXPECT_THAT(desktop_environment, IsNull());
FlushUiSequence();
}

TEST_F(It2MeDesktopEnvironmentTest,
ShouldNotForceLogoutUserIfInitializeCurtainModeFails) {
base::test::ScopedFeatureList feature_list{
features::kEnableCrdAdminRemoteAccess};
EXPECT_CALL(session_control(),
DisconnectSession(protocol::ErrorCode::HOST_CONFIGURATION_ERROR));

LogInUser();
auto desktop_environment = CreateCurtainedSession();
desktop_environment = nullptr;

FlushUiSequence();
EXPECT_THAT(ash_proxy().request_sign_out_count(), Eq(0));

EXPECT_THAT(ash_proxy().request_sign_out_count(), Eq(0));
}
Expand Down

0 comments on commit 20b52fd

Please sign in to comment.