Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix user tokens not being threadsafe #4944

Merged
merged 4 commits into from
Oct 12, 2021
Merged

Fix user tokens not being threadsafe #4944

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
61f4b1b
Fix user tokens not being threadsafe
ironage Oct 7, 2021
bf07d22
use static checking for protected SyncUser members
ironage Oct 8, 2021
d52861b
Merge branch 'master' into js/guard-user-tokens
ironage Oct 8, 2021
98ded96
Merge branch 'master' into js/guard-user-tokens
ironage Oct 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
* Crash when quering with 'Not()' followed by empty group. ([#4168](https://github.com/realm/realm-core/issues/4168) since v1.0.0)
* Change Apple/Linux temp dir created with `util::make_temp_dir()` to default to the environment's TMPDIR if available. Make `TestFile` clean up the directories it creates when finished. ([#4921](https://github.com/realm/realm-core/issues/4921))
* Fixed a rare assertion failure or deadlock when a sync session is racing to close at the same time that external reference to the Realm is being released. ([#4931](https://github.com/realm/realm-core/issues/4931))
* Fixed a rare segfault which could trigger if a user was being logged out while the access token refresh response comes in. ([#4944](https://github.com/realm/realm-core/issues/4944), since v10)

### Breaking changes
* `App::Config::transport_factory` was replaced with `App::Config::transport`. It should now be an instance of `GenericNetworkTransport` rather than a factory for making instances. This allows the SDK to control which thread constructs the transport layer. ([#4903](https://github.com/realm/realm-core/pull/4903))
Expand Down
9 changes: 2 additions & 7 deletions src/realm/object-store/sync/sync_session.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,18 +361,13 @@ const SyncSession::State& SyncSession::State::waiting_for_access_token = Waiting
std::function<void(util::Optional<app::AppError>)> SyncSession::handle_refresh(std::shared_ptr<SyncSession> session)
{
return [session](util::Optional<app::AppError> error) {
using namespace std::chrono;

auto session_user = session->user();
auto is_user_expired =
session_user && session_user->refresh_jwt().expires_at <
duration_cast<seconds>(system_clock::now().time_since_epoch()).count();

if (!session_user) {
std::unique_lock<std::mutex> lock(session->m_state_mutex);
session->cancel_pending_waits(lock, error ? error->error_code : std::error_code());
}
else if (is_user_expired) {
else if (session_user->refresh_token_is_expired()) { // user is expired
std::unique_lock<std::mutex> lock(session->m_state_mutex);
session->cancel_pending_waits(lock, error ? error->error_code : std::error_code());
if (session->m_config.error_handler) {
Expand Down Expand Up @@ -402,7 +397,7 @@ std::function<void(util::Optional<app::AppError>)> SyncSession::handle_refresh(s
}
else {
// 10 seconds is arbitrary, but it is to not swamp the server
std::this_thread::sleep_for(milliseconds(10000));
std::this_thread::sleep_for(std::chrono::seconds(10));
if (session_user) {
session_user->refresh_custom_data(handle_refresh(session));
}
Expand Down
76 changes: 45 additions & 31 deletions src/realm/object-store/sync/sync_user.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,8 @@ SyncUser::SyncUser(std::string refresh_token, const std::string identity, const
SyncManager* sync_manager)
: m_state(state)
, m_provider_type(provider_type)
, m_refresh_token(RealmJWT(std::move(refresh_token)))
, m_identity(std::move(identity))
, m_refresh_token(RealmJWT(std::move(refresh_token)))
, m_access_token(RealmJWT(std::move(access_token)))
, m_device_id(device_id)
, m_sync_manager(sync_manager)
Expand All @@ -102,6 +102,7 @@ SyncUser::SyncUser(std::string refresh_token, const std::string identity, const
}

bool updated = m_sync_manager->perform_metadata_update([&](const auto& manager) {
util::CheckedLockGuard lock(m_mutex);
auto metadata = manager.get_or_make_user_metadata(m_identity, m_provider_type);
metadata->set_state_and_tokens(m_state, m_access_token.token, m_refresh_token.token);
metadata->set_device_id(m_device_id);
Expand All @@ -116,23 +117,23 @@ SyncUser::~SyncUser() {}

std::shared_ptr<SyncManager> SyncUser::sync_manager() const
{
std::lock_guard<std::mutex> lk(m_mutex);
util::CheckedLockGuard lk(m_mutex);
REALM_ASSERT(m_sync_manager);
REALM_ASSERT(m_state != SyncUser::State::Removed);
return m_sync_manager->shared_from_this();
}

void SyncUser::detach_from_sync_manager()
{
std::lock_guard<std::mutex> lk(m_mutex);
util::CheckedLockGuard lk(m_mutex);
REALM_ASSERT(m_sync_manager);
m_state = SyncUser::State::Removed;
m_sync_manager = nullptr;
}

std::vector<std::shared_ptr<SyncSession>> SyncUser::all_sessions()
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
std::vector<std::shared_ptr<SyncSession>> sessions;
if (m_state == State::Removed) {
return sessions;
Expand All @@ -151,7 +152,7 @@ std::vector<std::shared_ptr<SyncSession>> SyncUser::all_sessions()

std::shared_ptr<SyncSession> SyncUser::session_for_on_disk_path(const std::string& path)
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
if (m_state == State::Removed) {
return nullptr;
}
Expand All @@ -172,7 +173,7 @@ void SyncUser::update_state_and_tokens(SyncUser::State state, const std::string&
{
std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
m_state = state;
m_access_token = access_token.empty() ? RealmJWT{} : RealmJWT(access_token);
m_refresh_token = refresh_token.empty() ? RealmJWT{} : RealmJWT(refresh_token);
Expand All @@ -197,9 +198,9 @@ void SyncUser::update_state_and_tokens(SyncUser::State state, const std::string&
}
}

m_sync_manager->perform_metadata_update([&](const auto& manager) {
m_sync_manager->perform_metadata_update([&, state = m_state](const auto& manager) {
auto metadata = manager.get_or_make_user_metadata(m_identity, m_provider_type);
metadata->set_state_and_tokens(m_state, access_token, refresh_token);
metadata->set_state_and_tokens(state, access_token, refresh_token);
});
}
// (Re)activate all pending sessions.
Expand All @@ -216,7 +217,7 @@ void SyncUser::update_refresh_token(std::string&& token)
{
std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
{
std::unique_lock<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
switch (m_state) {
case State::Removed:
return;
Expand All @@ -238,9 +239,9 @@ void SyncUser::update_refresh_token(std::string&& token)
}
}

m_sync_manager->perform_metadata_update([&](const auto& manager) {
m_sync_manager->perform_metadata_update([&, raw_refresh_token = m_refresh_token.token](const auto& manager) {
auto metadata = manager.get_or_make_user_metadata(m_identity, m_provider_type);
metadata->set_refresh_token(m_refresh_token.token);
metadata->set_refresh_token(raw_refresh_token);
});
}
// (Re)activate all pending sessions.
Expand All @@ -257,7 +258,7 @@ void SyncUser::update_access_token(std::string&& token)
{
std::vector<std::shared_ptr<SyncSession>> sessions_to_revive;
{
std::unique_lock<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
switch (m_state) {
case State::Removed:
return;
Expand All @@ -279,9 +280,9 @@ void SyncUser::update_access_token(std::string&& token)
}
}

m_sync_manager->perform_metadata_update([&](const auto& manager) {
m_sync_manager->perform_metadata_update([&, raw_access_token = m_access_token.token](const auto& manager) {
auto metadata = manager.get_or_make_user_metadata(m_identity, m_provider_type);
metadata->set_access_token(m_access_token.token);
metadata->set_access_token(raw_access_token);
});
}

Expand All @@ -297,14 +298,14 @@ void SyncUser::update_access_token(std::string&& token)

std::vector<SyncUserIdentity> SyncUser::identities() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_user_identities;
}


void SyncUser::update_identities(std::vector<SyncUserIdentity> identities)
{
std::unique_lock<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
REALM_ASSERT(m_state == SyncUser::State::LoggedIn);
m_user_identities = identities;

Expand All @@ -320,7 +321,7 @@ void SyncUser::log_out()
// after we've been marked as logged out.
std::shared_ptr<SyncManager> sync_manager_shared;
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
if (m_state != State::LoggedIn) {
return;
}
Expand Down Expand Up @@ -362,7 +363,12 @@ void SyncUser::log_out()

bool SyncUser::is_logged_in() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return do_is_logged_in();
}

bool SyncUser::do_is_logged_in() const
{
return !m_access_token.token.empty() && !m_refresh_token.token.empty() && m_state == State::LoggedIn;
}

Expand All @@ -373,37 +379,37 @@ void SyncUser::invalidate()

std::string SyncUser::refresh_token() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_refresh_token.token;
}

std::string SyncUser::access_token() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_access_token.token;
}

std::string SyncUser::device_id() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_device_id;
}

bool SyncUser::has_device_id() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return !m_device_id.empty() && m_device_id != "000000000000000000000000";
}

SyncUser::State SyncUser::state() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_state;
}

void SyncUser::set_state(SyncUser::State state)
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
m_state = state;

REALM_ASSERT(m_sync_manager);
Expand All @@ -415,19 +421,19 @@ void SyncUser::set_state(SyncUser::State state)

SyncUserProfile SyncUser::user_profile() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_user_profile;
}

util::Optional<bson::BsonDocument> SyncUser::custom_data() const
{
std::lock_guard<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
return m_access_token.user_data;
}

void SyncUser::update_user_profile(const SyncUserProfile& profile)
{
std::unique_lock<std::mutex> lock(m_mutex);
util::CheckedLockGuard lock(m_mutex);
REALM_ASSERT(m_state == SyncUser::State::LoggedIn);

m_user_profile = profile;
Expand All @@ -441,7 +447,7 @@ void SyncUser::update_user_profile(const SyncUserProfile& profile)
void SyncUser::register_session(std::shared_ptr<SyncSession> session)
{
const std::string& path = session->path();
std::unique_lock<std::mutex> lock(m_mutex);
util::CheckedUniqueLock lock(m_mutex);
switch (m_state) {
case State::LoggedIn:
// Immediately ask the session to come online.
Expand All @@ -459,7 +465,7 @@ void SyncUser::register_session(std::shared_ptr<SyncSession> session)

app::MongoClient SyncUser::mongo_client(const std::string& service_name)
{
std::lock_guard<std::mutex> lk(m_mutex);
util::CheckedLockGuard lk(m_mutex);
REALM_ASSERT(m_state == SyncUser::State::LoggedIn);
return app::MongoClient(shared_from_this(), m_sync_manager->app().lock(), service_name);
}
Expand All @@ -473,7 +479,7 @@ void SyncUser::set_binding_context_factory(SyncUserContextFactory factory)
void SyncUser::refresh_custom_data(std::function<void(util::Optional<app::AppError>)> completion_block)
{
auto app = [&]() -> std::shared_ptr<app::App> {
std::lock_guard<std::mutex> lk(m_mutex);
util::CheckedLockGuard lk(m_mutex);
if (!m_sync_manager || m_state == SyncUser::State::Removed) {
return nullptr;
}
Expand All @@ -495,8 +501,16 @@ bool SyncUser::access_token_refresh_required() const
{
using namespace std::chrono;
constexpr size_t buffer_seconds = 5; // arbitrary
util::CheckedLockGuard lock(m_mutex);
auto threshold = duration_cast<seconds>(system_clock::now().time_since_epoch()).count() - buffer_seconds;
return is_logged_in() && m_access_token.expires_at < static_cast<int64_t>(threshold);
return do_is_logged_in() && m_access_token.expires_at < static_cast<int64_t>(threshold);
}

bool SyncUser::refresh_token_is_expired() const
{
using namespace std::chrono;
util::CheckedLockGuard guard(m_mutex);
return m_refresh_token.expires_at < duration_cast<seconds>(system_clock::now().time_since_epoch()).count();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check whether the user is logged in here at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the intention of the place this is called, no. Honestly this should all change in the near future given #4882 and #4943 because checking the refresh token expiry time locally is dubious. But I wanted to just fix the crash in this change.

}

} // namespace realm
Expand Down
Loading