Skip to content

Commit

Permalink
Access token refresh for websockets was not updating the location met…
Browse files Browse the repository at this point in the history 
…adata (#6631)

* Always refresh metadata on app login
* Updated changelog
* Always update location when requested; fix c_api test
* Update test to properly evaluate websocket redirections; added one more test
* Updated changelog and fixed compile warning
* Added location checks back to test
* added mutex locking around location updated state and reworked requesting location update to use flag
* clang format and fix incorrect timeout value
* Reworked update location logic a bit and removed unused function
* Free mutex before calling completion on early exit in init_app_metadata
  • Loading branch information
Michael Wilkerson-Barker authored May 23, 2023
1 parent 61e2050 commit cb9b80b
Show file tree
Hide file tree
Showing 7 changed files with 207 additions and 114 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

### Fixed
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* None.
* Access token refresh for websockets was not updating the location metadata ([#6630](https://github.com/realm/realm-core/issues/6630), since v13.9.3)

### Breaking changes
* None.
Expand Down
179 changes: 105 additions & 74 deletions src/realm/object-store/sync/app.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -301,9 +301,15 @@ void App::configure(const SyncClientConfig& sync_client_config)
auto sync_route = make_sync_route(m_app_route);
m_sync_manager->configure(shared_from_this(), sync_route, sync_client_config);
if (auto metadata = m_sync_manager->app_metadata()) {
// If there is app metadata stored, then update the hostname/syncroute using that info
// If there is app metadata stored, then set up the initial hostname/syncroute
// using that info - it will be updated upon first request to the server
update_hostname(metadata);
}
{
std::lock_guard<std::mutex> lock(*m_route_mutex);
// Always update the location after the app is configured/re-configured
m_location_updated = false;
}
}

bool App::init_logger()
Expand Down Expand Up @@ -337,7 +343,7 @@ void App::log_error(const char* message, Params&&... params)

std::string App::make_sync_route(const std::string& http_app_route)
{
// change the scheme in the base url to ws from http to satisfy the sync client
// change the scheme in the base url from http to ws to satisfy the sync client URL
auto sync_route = http_app_route + sync_path;
size_t uri_scheme_start = sync_route.find("http");
if (uri_scheme_start == 0)
Expand Down Expand Up @@ -659,35 +665,45 @@ void App::log_in_with_credentials(
BsonDocument body = credentials.serialize_as_bson();
attach_auth_options(body);

do_request({HttpMethod::post, route, m_request_timeout_ms,
get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()},
[completion = std::move(completion), credentials, linking_user,
self = shared_from_this()](const Response& response) mutable {
if (auto error = AppUtils::check_for_errors(response)) {
self->log_error("App: log_in_with_credentials failed: %1 message: %2",
response.http_status_code, error->what());
return completion(nullptr, std::move(error));
}
// To ensure the location metadata is always kept up to date, requery the location info before
// logging in the user. Since some SDK network transports (e.g. for JS) automatically handle
// redirects, the location is not updated when a redirect response from the server is received.
// This is especially necessary when the deployment model is changed, where the redirect response
// provides information about the new location of the server and a location info update is
// triggered. If the App never receives a redirect response from the server (because it was
// automatically handled) after the deployment model was changed and the user was logged out,
// the HTTP and websocket URL values will never be updated with the new server location.
do_request(
{HttpMethod::post, route, m_request_timeout_ms,
get_request_headers(linking_user, RequestTokenType::AccessToken), Bson(body).to_string()},
[completion = std::move(completion), credentials, linking_user,
self = shared_from_this()](const Response& response) mutable {
if (auto error = AppUtils::check_for_errors(response)) {
self->log_error("App: log_in_with_credentials failed: %1 message: %2", response.http_status_code,
error->what());
return completion(nullptr, std::move(error));
}

std::shared_ptr<realm::SyncUser> sync_user = linking_user;
try {
auto json = parse<BsonDocument>(response.body);
if (linking_user) {
linking_user->update_access_token(get<std::string>(json, "access_token"));
}
else {
sync_user = self->m_sync_manager->get_user(
get<std::string>(json, "user_id"), get<std::string>(json, "refresh_token"),
get<std::string>(json, "access_token"), credentials.provider_as_string(),
get<std::string>(json, "device_id"));
}
}
catch (const AppError& e) {
return completion(nullptr, e);
}
std::shared_ptr<realm::SyncUser> sync_user = linking_user;
try {
auto json = parse<BsonDocument>(response.body);
if (linking_user) {
linking_user->update_access_token(get<std::string>(json, "access_token"));
}
else {
sync_user = self->m_sync_manager->get_user(
get<std::string>(json, "user_id"), get<std::string>(json, "refresh_token"),
get<std::string>(json, "access_token"), credentials.provider_as_string(),
get<std::string>(json, "device_id"));
}
}
catch (const AppError& e) {
return completion(nullptr, e);
}

self->get_profile(sync_user, std::move(completion));
});
self->get_profile(sync_user, std::move(completion));
},
false);
}

void App::log_in_with_credentials(
Expand Down Expand Up @@ -865,20 +881,25 @@ void App::init_app_metadata(UniqueFunction<void(const Optional<Response>&)>&& co
{
std::string route;

if (!new_hostname && m_location_updated) {
{
std::unique_lock<std::mutex> lock(*m_route_mutex);
// Skip if the app_metadata/location data has already been initialized and a new hostname is not provided
return completion(util::none); // early return
}
else {
std::lock_guard<std::mutex> lock(*m_route_mutex);
route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route());
if (!new_hostname && m_location_updated) {
// Release the lock before calling the completion function
lock.unlock();
return completion(util::none); // early return
}
else {
route = util::format("%1/location", new_hostname ? get_app_route(new_hostname) : get_app_route());
}
}

Request req;
req.method = HttpMethod::get;
req.url = route;
req.timeout_ms = m_request_timeout_ms;

log_debug("App: request location: %1", route);

m_config.transport->send_request_to_server(req, [self = shared_from_this(),
completion = std::move(completion)](const Response& response) {
// If the response contains an error, then pass it up
Expand All @@ -902,7 +923,10 @@ void App::init_app_metadata(UniqueFunction<void(const Optional<Response>&)>&& co
// No metadata in use, update the hostname and sync route directly
self->update_hostname(hostname, ws_hostname);
}
self->m_location_updated = true;
{
std::lock_guard<std::mutex> lock(*self->m_route_mutex);
self->m_location_updated = true;
}
}
catch (const AppError&) {
// Pass the response back to completion
Expand Down Expand Up @@ -950,23 +974,34 @@ void App::update_metadata_and_resend(Request&& request, UniqueFunction<void(cons
new_hostname);
}

void App::do_request(Request&& request, UniqueFunction<void(const Response&)>&& completion)
void App::do_request(Request&& request, UniqueFunction<void(const Response&)>&& completion, bool update_location)
{
request.timeout_ms = default_timeout_ms;
// Make sure the timeout value is set to the configured request timeout value
request.timeout_ms = m_request_timeout_ms;

// Refresh the location metadata every time an app is created to ensure the http and
// websocket URL information is up to date.
if (m_location_updated) {
m_config.transport->send_request_to_server(
std::move(request), [self = shared_from_this(), completion = std::move(completion)](
Request&& request, const Response& response) mutable {
self->handle_possible_redirect_response(std::move(request), response, std::move(completion));
});
return; // early return
// Refresh the location metadata every time an app is created (or when requested) to ensure the http
// and websocket URL information is up to date.
{
std::unique_lock<std::mutex> lock(*m_route_mutex);
if (update_location) {
// Force the location to be updated before sending the request.
m_location_updated = false;
}
if (!m_location_updated) {
lock.unlock();
// Location metadata has not yet been received after the App was created, update the location
// info and then send the request
update_metadata_and_resend(std::move(request), std::move(completion));
return; // early return
}
}

// if we do not have metadata yet, update the metadata and resend the request
update_metadata_and_resend(std::move(request), std::move(completion));
// location info has already been received after App was created
m_config.transport->send_request_to_server(
std::move(request), [self = shared_from_this(), completion = std::move(completion)](
Request&& request, const Response& response) mutable {
self->handle_possible_redirect_response(std::move(request), response, std::move(completion));
});
}

void App::handle_possible_redirect_response(Request&& request, const Response& response,
Expand Down Expand Up @@ -1096,33 +1131,29 @@ void App::refresh_access_token(const std::shared_ptr<SyncUser>& sync_user, bool
route = util::format("%1/auth/session", m_base_route);
}

log_debug("App: refresh_access_token: email: %1", sync_user->user_profile().email());
log_debug("App: refresh_access_token: email: %1 %2", sync_user->user_profile().email(),
update_location ? "(updating location)" : "");

Request request{HttpMethod::post, std::move(route), m_request_timeout_ms,
get_request_headers(sync_user, RequestTokenType::RefreshToken)};
auto handler = [completion = std::move(completion), sync_user](const Response& response) {
if (auto error = AppUtils::check_for_errors(response)) {
return completion(std::move(error));
}

try {
auto json = parse<BsonDocument>(response.body);
sync_user->update_access_token(get<std::string>(json, "access_token"));
}
catch (AppError& err) {
return completion(std::move(err));
}
// If update_location is set, force the location info to be updated before sending the request
do_request(
{HttpMethod::post, std::move(route), m_request_timeout_ms,
get_request_headers(sync_user, RequestTokenType::RefreshToken)},
[completion = std::move(completion), sync_user](const Response& response) {
if (auto error = AppUtils::check_for_errors(response)) {
return completion(std::move(error));
}

return completion(util::none);
};
try {
auto json = parse<BsonDocument>(response.body);
sync_user->update_access_token(get<std::string>(json, "access_token"));
}
catch (AppError& err) {
return completion(std::move(err));
}

if (update_location) {
// If update_location, update the location metadata before sending the request
update_metadata_and_resend(std::move(request), std::move(handler));
}
else {
do_request(std::move(request), std::move(handler));
}
return completion(util::none);
},
update_location);
}

std::string App::function_call_url_path() const
Expand Down
14 changes: 9 additions & 5 deletions src/realm/object-store/sync/app.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -403,13 +403,17 @@ class App : public std::enable_shared_from_this<App>,
friend class OnlyForTesting;

Config m_config;
mutable std::unique_ptr<std::mutex> m_route_mutex = std::make_unique<std::mutex>();

// mutable to allow locking for reads in const functions
// this is a shared pointer to support the App move constructor
mutable std::shared_ptr<std::mutex> m_route_mutex = std::make_shared<std::mutex>();
std::string m_base_url;
std::string m_base_route;
std::string m_app_route;
std::string m_auth_route;
uint64_t m_request_timeout_ms;
bool m_location_updated = false;

uint64_t m_request_timeout_ms;
std::shared_ptr<SyncManager> m_sync_manager;
std::shared_ptr<util::Logger> m_logger_ptr;

Expand Down Expand Up @@ -463,15 +467,15 @@ class App : public std::enable_shared_from_this<App>,
void update_metadata_and_resend(Request&& request, util::UniqueFunction<void(const Response&)>&& completion,
const util::Optional<std::string>& new_hostname = util::none);

void basic_request(std::string&& route, std::string&& body,
util::UniqueFunction<void(util::Optional<AppError>)>&& completion);
void post(std::string&& route, util::UniqueFunction<void(util::Optional<AppError>)>&& completion,
const bson::BsonDocument& body);

/// Performs a request to the Stitch server. This request does not contain authentication state.
/// @param request The request to be performed
/// @param completion Returns the response from the server
void do_request(Request&& request, util::UniqueFunction<void(const Response&)>&& completion);
/// @param update_location Force the location metadata to be updated prior to sending the request
void do_request(Request&& request, util::UniqueFunction<void(const Response&)>&& completion,
bool update_location = false);

/// Check to see if hte response is a redirect and handle, otherwise pass the response to compleetion
/// @param request The request to be performed (in case it needs to be sent again)
Expand Down
16 changes: 9 additions & 7 deletions src/realm/object-store/sync/sync_session.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -763,13 +763,16 @@ void SyncSession::handle_error(sync::SessionErrorInfo error)
}
}
else if (error_code.category() == sync::websocket::websocket_error_category()) {
using WebSocketError = sync::websocket::WebSocketError;
auto websocket_error = static_cast<WebSocketError>(error_code.value());

// The server replies with '401: unauthorized' if the access token is invalid, expired, revoked, or the user
// is disabled. In this scenario we attempt an automatic token refresh and if that succeeds continue as
// normal. If the refresh request also fails with 401 then we need to stop retrying and pass along the error;
// see handle_refresh().
bool redirect_occurred = error_code == sync::websocket::WebSocketError::websocket_moved_permanently;
if (redirect_occurred || error_code == sync::websocket::WebSocketError::websocket_unauthorized ||
error_code == sync::websocket::WebSocketError::websocket_abnormal_closure) {
bool redirect_occurred = websocket_error == WebSocketError::websocket_moved_permanently;
if (redirect_occurred || websocket_error == WebSocketError::websocket_unauthorized ||
websocket_error == WebSocketError::websocket_abnormal_closure) {
if (auto u = user()) {
// If a redirection occurred, the location metadata will be updated before refreshing the access
// token.
Expand All @@ -780,14 +783,13 @@ void SyncSession::handle_error(sync::SessionErrorInfo error)

// If the websocket was closed cleanly or if the socket disappeared, don't notify the user as an error
// since the sync client will retry.
if (error_code == sync::websocket::WebSocketError::websocket_read_error ||
error_code == sync::websocket::WebSocketError::websocket_write_error) {
if (websocket_error == WebSocketError::websocket_read_error ||
websocket_error == WebSocketError::websocket_write_error) {
return;
}

// Surface a simplified websocket error to the user.
auto simplified_error = sync::websocket::get_simplified_websocket_error(
static_cast<sync::websocket::WebSocketError>(error_code.value()));
auto simplified_error = sync::websocket::get_simplified_websocket_error(websocket_error);
std::error_code new_error_code(simplified_error, sync::websocket::websocket_error_category());
error = sync::SessionErrorInfo(new_error_code, error.message, error.try_again);
}
Expand Down
3 changes: 3 additions & 0 deletions test/object-store/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,9 +119,12 @@ if(REALM_TEST_LOGGING)
)

if(REALM_TEST_LOGGING_LEVEL)
message(STATUS "Test logging level: ${REALM_TEST_LOGGING_LEVEL}")
target_compile_definitions(ObjectStoreTests PRIVATE
TEST_LOGGING_LEVEL=${REALM_TEST_LOGGING_LEVEL}
)
else()
message(STATUS "Test logging enabled")
endif()
endif()

Expand Down
Loading

0 comments on commit cb9b80b

Please sign in to comment.