Skip to content

Commit

Permalink
RCORE-2222 Remove 308 redirect support from App/AppUser (#7996)
Browse files Browse the repository at this point in the history 
* Removed redirect tests
* Removed one more location redirect test case
* Removed 301/308 redirection support from App
* Updates from review
* Updated changelog after release
* Fixed misspelling and updated comment a bit
  • Loading branch information
Michael Wilkerson-Barker authored Aug 30, 2024
1 parent 07d57de commit 1760013
Show file tree
Hide file tree
Showing 5 changed files with 62 additions and 148 deletions.
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
* None.

### Breaking changes
* None.
* Removed http 301/308 redirection support from app services operations provided by App. It is assumed that the SDK's http implementation will handle http redirects instead. ([PR #7996](https://github.com/realm/realm-core/pull/7996))

### Compatibility
* Fileformat: Generates files with format v24. Reads and automatically upgrade from fileformat v10. If you want to upgrade from an earlier file format version you will have to use RealmCore v13.x.y or earlier.
Expand Down
164 changes: 57 additions & 107 deletions src/realm/object-store/sync/app.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,6 @@ constexpr static std::string_view s_sync_path = "/realm-sync";
constexpr static uint64_t s_default_timeout_ms = 60000;
constexpr static std::string_view s_username_password_provider_key = "local-userpass";
constexpr static std::string_view s_user_api_key_provider_key_path = "api_keys";
constexpr static int s_max_http_redirects = 20;
static util::FlatMap<std::string, util::FlatMap<std::string, SharedApp>> s_apps_cache; // app_id -> base_url -> app
std::mutex s_apps_mutex;
} // anonymous namespace
Expand Down Expand Up @@ -978,18 +977,17 @@ void App::delete_user(const std::shared_ptr<User>& user, UniqueFunction<void(Opt
}
}

do_authenticated_request(
HttpMethod::del, url_for_path("/auth/delete"), "", user, RequestTokenType::AccessToken,
[self = shared_from_this(), completion = std::move(completion), user, this](const Response& response) {
auto error = AppUtils::check_for_errors(response);
if (!error) {
auto user_id = user->user_id();
user->detach_and_tear_down();
m_metadata_store->delete_user(*m_file_manager, user_id);
emit_change_to_subscribers();
}
completion(std::move(error));
});
do_authenticated_request(HttpMethod::del, url_for_path("/auth/delete"), "", user, RequestTokenType::AccessToken,
[completion = std::move(completion), user, this](const Response& response) {
auto error = AppUtils::check_for_errors(response);
if (!error) {
auto user_id = user->user_id();
user->detach_and_tear_down();
m_metadata_store->delete_user(*m_file_manager, user_id);
emit_change_to_subscribers();
}
completion(std::move(error));
});
}

void App::link_user(const std::shared_ptr<User>& user, const AppCredentials& credentials,
Expand Down Expand Up @@ -1054,34 +1052,32 @@ std::string App::get_app_route(const Optional<std::string>& hostname) const
}

void App::request_location(UniqueFunction<void(std::optional<AppError>)>&& completion,
std::optional<std::string>&& new_hostname, std::optional<std::string>&& redir_location,
int redirect_count)
{
// Request the new location information at the new base url hostname; or redir response location if a redirect
// occurred during the initial location request. redirect_count is used to track the number of sequential
// redirect responses received during the location update and return an error if this count exceeds
// max_http_redirects. If neither new_hostname nor redir_location is provided, the current value of m_base_url
// will be used.
std::string app_route;
std::string base_url;
std::optional<std::string>&& new_hostname)
{
// Request the new location information the original configured base_url or the new_hostname
// if the base_url is being updated. If a new_hostname has not been provided and the location
// has already been requested, this function does nothing.
std::string app_route; // The app_route for the server to query the location
std::string base_url; // The configured base_url hostname used for querying the location
{
util::CheckedUniqueLock lock(m_route_mutex);
// Skip if the location info has already been initialized and a new hostname is not provided
if (!new_hostname && !redir_location && m_location_updated) {
if (!new_hostname && m_location_updated) {
// Release the lock before calling the completion function
lock.unlock();
completion(util::none);
return;
}
base_url = new_hostname.value_or(m_base_url);
// If this is for a redirect after querying new_hostname, then use the redirect location
if (redir_location)
app_route = get_app_route(redir_location);
// If this is querying the new_hostname, then use that location
else if (new_hostname)
// If this is querying the new_hostname, then use that to query the location
if (new_hostname) {
base_url = *new_hostname;
app_route = get_app_route(new_hostname);
else
}
// Otherwise, use the current hostname
else {
app_route = get_app_route();
base_url = m_base_url;
}
REALM_ASSERT(!app_route.empty());
}

Expand All @@ -1093,46 +1089,15 @@ void App::request_location(UniqueFunction<void(std::optional<AppError>)>&& compl
log_debug("App: request location: %1", req.url);

m_config.transport->send_request_to_server(req, [self = shared_from_this(), completion = std::move(completion),
base_url = std::move(base_url),
redirect_count](const Response& response) mutable {
// Check to see if a redirect occurred
if (AppUtils::is_redirect_status_code(response.http_status_code)) {
// Make sure we don't do too many redirects (max_http_redirects (20) is an arbitrary number)
if (redirect_count >= s_max_http_redirects) {
completion(AppError{ErrorCodes::ClientTooManyRedirects,
util::format("number of redirections exceeded %1", s_max_http_redirects),
{},
response.http_status_code});
return;
}
// Handle the redirect response when requesting the location - extract the
// new location header field and resend the request.
auto redir_location = AppUtils::extract_redir_location(response.headers);
if (!redir_location) {
// Location not found in the response, pass error response up the chain
completion(AppError{ErrorCodes::ClientRedirectError,
"Redirect response missing location header",
{},
response.http_status_code});
return;
}
// try to request the location info at the new location in the redirect response
// retry_count is passed in to track the number of subsequent redirection attempts
self->request_location(std::move(completion), std::move(base_url), std::move(redir_location),
redirect_count + 1);
return;
}

base_url = std::move(base_url)](const Response& response) {
// Location request was successful - update the location info
auto update_response = self->update_location(response, base_url);
if (update_response) {
self->log_error("App: request location failed (%1%2): %3", update_response->code_string(),
update_response->additional_status_code
? util::format(" %1", *update_response->additional_status_code)
: "",
update_response->reason());
auto error = self->update_location(response, base_url);
if (error) {
self->log_error("App: request location failed (%1%2): %3", error->code_string(),
error->additional_status_code ? util::format(" %1", *error->additional_status_code) : "",
error->reason());
}
completion(update_response);
completion(error);
});
}

Expand Down Expand Up @@ -1169,8 +1134,7 @@ std::optional<AppError> App::update_location(const Response& response, const std
return util::none;
}

void App::update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion,
Optional<std::string>&& redir_location)
void App::update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion)
{
// Update the location information if a redirect response was received or m_location_updated == false
// and then send the request to the server with request.url updated to the new AppServices hostname.
Expand All @@ -1192,13 +1156,13 @@ void App::update_location_and_resend(std::unique_ptr<Request>&& request, Interme
// Retry the original request with the updated url
auto& request_ref = *request;
self->m_config.transport->send_request_to_server(
request_ref, [self = std::move(self), completion = std::move(completion),
request = std::move(request)](const Response& response) mutable {
self->check_for_redirect_response(std::move(request), response, std::move(completion));
request_ref,
[completion = std::move(completion), request = std::move(request)](const Response& response) mutable {
completion(std::move(request), response);
});
},
// The base_url is not changing for this request
util::none, std::move(redir_location));
util::none);
}

void App::post(std::string&& route, UniqueFunction<void(Optional<AppError>)>&& completion, const BsonDocument& body)
Expand All @@ -1212,8 +1176,18 @@ void App::post(std::string&& route, UniqueFunction<void(Optional<AppError>)>&& c

void App::do_request(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion, bool update_location)
{
// NOTE: Since the calls to `send_request_to_server()` or `update_location_and_resend()` do not
// capture a shared_ptr to App as part of their callback, any function that calls `do_request()`
// or `do_authenticated_request()` needs to capture the App as `self = shared_from_this()` for
// the completion callback to ensure the lifetime of the App object is extended until the
// callback is called after the operation is complete.

// Verify the request URL to make sure it is valid
util::Uri::parse(request->url);
if (auto valid_url = util::Uri::try_parse(request->url); !valid_url.is_ok()) {
completion(std::move(request), AppUtils::make_apperror_response(
AppError{valid_url.get_status().code(), valid_url.get_status().reason()}));
return;
}

// Refresh the location info when app is created or when requested (e.g. after a websocket redirect)
// to ensure the http and websocket URL information is up to date.
Expand All @@ -1235,36 +1209,12 @@ void App::do_request(std::unique_ptr<Request>&& request, IntermediateCompletion&
// If location info has already been updated, then send the request directly
auto& request_ref = *request;
m_config.transport->send_request_to_server(
request_ref, [self = shared_from_this(), completion = std::move(completion),
request = std::move(request)](const Response& response) mutable {
self->check_for_redirect_response(std::move(request), response, std::move(completion));
request_ref,
[completion = std::move(completion), request = std::move(request)](const Response& response) mutable {
completion(std::move(request), response);
});
}

void App::check_for_redirect_response(std::unique_ptr<Request>&& request, const Response& response,
IntermediateCompletion&& completion)
{
// If this isn't a redirect response, then we're done
if (!AppUtils::is_redirect_status_code(response.http_status_code)) {
return completion(std::move(request), response);
}

// Handle a redirect response when sending the original request - extract the location
// header field and resend the request.
auto redir_location = AppUtils::extract_redir_location(response.headers);
if (!redir_location) {
// Location not found in the response, pass error response up the chain
return completion(std::move(request),
AppUtils::make_clienterror_response(ErrorCodes::ClientRedirectError,
"Redirect response missing location header",
response.http_status_code));
}

// Request the location info at the new location - once this is complete, the original
// request will be sent to the new server
update_location_and_resend(std::move(request), std::move(completion), std::move(redir_location));
}

void App::do_authenticated_request(HttpMethod method, std::string&& route, std::string&& body,
const std::shared_ptr<User>& user, RequestTokenType token_type,
util::UniqueFunction<void(const Response&)>&& completion)
Expand Down Expand Up @@ -1314,10 +1264,10 @@ void App::handle_auth_failure(const AppError& error, std::unique_ptr<Request>&&

// Reissue the request with the new access token
request->headers = get_request_headers(user, RequestTokenType::AccessToken);
self->do_request(std::move(request),
[completion = std::move(completion)](auto&&, auto& response) {
completion(response);
});
self->do_request(std::move(request), [self = self, completion = std::move(completion)](
auto&&, auto& response) {
completion(response);
});
});
}

Expand Down
20 changes: 4 additions & 16 deletions src/realm/object-store/sync/app.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -525,12 +525,8 @@ class App : public std::enable_shared_from_this<App>,
/// a new hostname is provided, the app metadata will be refreshed using the new hostname.
/// @param completion The callback that will be called with the error on failure or empty on success
/// @param new_hostname The (Original) new hostname to request the location from
/// @param redir_location The location provided by the last redirect response when querying location
/// @param redirect_count The current number of redirects that have occurred in a row
void request_location(util::UniqueFunction<void(std::optional<AppError>)>&& completion,
std::optional<std::string>&& new_hostname = std::nullopt,
std::optional<std::string>&& redir_location = std::nullopt, int redirect_count = 0)
REQUIRES(!m_route_mutex);
void request_location(util::UniqueFunction<void(std::optional<app::AppError>)>&& completion,
std::optional<std::string>&& new_hostname) REQUIRES(!m_route_mutex);

/// Update the location metadata from the location response
/// @param response The response returned from the location request
Expand All @@ -542,9 +538,8 @@ class App : public std::enable_shared_from_this<App>,
/// Update the app metadata and resend the request with the updated metadata
/// @param request The original request object that needs to be sent after the update
/// @param completion The original completion object that will be called with the response to the request
/// @param new_hostname If provided, the metadata will be requested from this hostname
void update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion,
std::optional<std::string>&& new_hostname = util::none) REQUIRES(!m_route_mutex);
void update_location_and_resend(std::unique_ptr<Request>&& request, IntermediateCompletion&& completion)
REQUIRES(!m_route_mutex);

void post(std::string&& route, util::UniqueFunction<void(std::optional<AppError>)>&& completion,
const bson::BsonDocument& body) REQUIRES(!m_route_mutex);
Expand All @@ -559,13 +554,6 @@ class App : public std::enable_shared_from_this<App>,
std::unique_ptr<Request> make_request(HttpMethod method, std::string&& url, const std::shared_ptr<User>& user,
RequestTokenType, std::string&& body) const;

/// Process the redirect response received from the last request that was sent to the server
/// @param request The request to be performed (in case it needs to be sent again)
/// @param response The response from the send_request_to_server operation
/// @param completion Returns the response from the server if not a redirect
void check_for_redirect_response(std::unique_ptr<Request>&& request, const Response& response,
IntermediateCompletion&& completion) REQUIRES(!m_route_mutex);

void do_authenticated_request(HttpMethod, std::string&& route, std::string&& body,
const std::shared_ptr<User>& user, RequestTokenType,
util::UniqueFunction<void(const Response&)>&&) override REQUIRES(!m_route_mutex);
Expand Down
22 changes: 0 additions & 22 deletions src/realm/object-store/sync/app_utils.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,28 +50,6 @@ bool AppUtils::is_success_status_code(int status_code)
return status_code == 0 || (status_code < 300 && status_code >= 200);
}

bool AppUtils::is_redirect_status_code(int status_code)
{
using namespace realm::sync;
// If the response contains a redirection, then return true
if (auto code = HTTPStatus(status_code);
code == HTTPStatus::MovedPermanently || code == HTTPStatus::PermanentRedirect) {
return true;
}
return false;
}

std::optional<std::string> AppUtils::extract_redir_location(const std::map<std::string, std::string>& headers)
{
// Look for case insensitive redirect "location" in headers
auto location = AppUtils::find_header("location", headers);
if (location && !location->second.empty() && util::Uri::try_parse(location->second).is_ok()) {
// If the location is valid, return it wholesale (e.g., it could include a path for API proxies)
return location->second;
}
return std::nullopt;
}

// Create a Response object with the given client error, message and optional http status code
Response AppUtils::make_clienterror_response(ErrorCodes::Error code, const std::string_view message,
std::optional<int> http_status)
Expand Down
2 changes: 0 additions & 2 deletions src/realm/object-store/sync/app_utils.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,6 @@ class AppUtils {
static const std::pair<const std::string, std::string>*
find_header(const std::string& key_name, const std::map<std::string, std::string>& search_map);
static bool is_success_status_code(int status_code);
static bool is_redirect_status_code(int status_code);
static std::optional<std::string> extract_redir_location(const std::map<std::string, std::string>& headers);
};

} // namespace realm::app
Expand Down

0 comments on commit 1760013

Please sign in to comment.