301/308 redirections are not updating location if Network Transport handles redirections internally #6505
Conversation
| @@ -2435,6 +2440,9 @@ TEST_CASE("app: sync integration", "[sync][app]") { | |||
|
|
|||
| auto redir_transport = std::make_shared<HookedTransport>(); | |||
| auto redir_provider = std::make_shared<HookedSocketProvider>(logger, ""); | |||
| std::mutex logout_mutex; | |||
There was a problem hiding this comment.
These tests are purely synchronous and the mutex and waiting on a CV isn't doing anything.
There was a problem hiding this comment.
It's true that the first set of App redirect tests are purely synchronous, but the App calls for the websocket redirect tests under the "Test websocket redirect with existing session" section are being driven by the event loop due to redirect responses from websocket connect, so I will still need the mutex and CV. In fact, we've had some occasional failures where the wait_for_download() returned before the user was logged out.
src/realm/object-store/sync/app.hpp
Outdated
| void refresh_custom_data(const std::shared_ptr<SyncUser>& user, | ||
| util::UniqueFunction<void(util::Optional<AppError>)>&&); | ||
| util::UniqueFunction<void(util::Optional<AppError>)>&&, bool refresh_location = false); |
There was a problem hiding this comment.
Can we make this new boolean not the last parameter and not have a default value? If I'm really being pedantic here I should ask for it to be a TaggedBool or have two different overloads, but I won't be that pedantic.
src/realm/object-store/sync/app.hpp
Outdated
| void refresh_access_token(const std::shared_ptr<SyncUser>& user, | ||
| util::UniqueFunction<void(util::Optional<AppError>)>&& completion); | ||
| util::UniqueFunction<void(util::Optional<AppError>)>&& completion, | ||
| bool refresh_location = false); |
| void refresh_custom_data(util::UniqueFunction<void(util::Optional<app::AppError>)> completion_block) | ||
| REQUIRES(!m_mutex); | ||
| /// If refresh_location is true, the location metadata will be queried before the request | ||
| void refresh_custom_data(util::UniqueFunction<void(util::Optional<app::AppError>)> completion_block, |
| @@ -909,8 +915,9 @@ void App::do_request(Request&& request, UniqueFunction<void(const Response&)>&& | |||
| { | |||
| request.timeout_ms = default_timeout_ms; | |||
|
|
|||
| // Normal do_request operation, just send the request to the server and return the response | |||
| if (m_sync_manager->app_metadata()) { | |||
There was a problem hiding this comment.
Is this check not needed anymore?
There was a problem hiding this comment.
Not anymore - before, the cached metadata was used when the app was created and it was never updated unless a 301/308 was received from an HTTP request. This was updated to always refresh the metadata upon the first request after being created (or if requested during an access token refresh), since the 301/308 response will not be received by the App if the network transport handles these responses internally.
src/realm/object-store/sync/app.hpp
Outdated
| @@ -266,8 +266,11 @@ class App : public std::enable_shared_from_this<App>, | |||
|
|
|||
| /// Refreshes the custom data for a specified user | |||
| /// @param user The user you want to refresh | |||
| /// @param refresh_location If true, the location metadata will be updated before refresh | |||
| void refresh_custom_data(const std::shared_ptr<SyncUser>& user, bool refresh_location, | |||
There was a problem hiding this comment.
nit: isn't update_location a more suited name?
What, How & Why?
If the network transport handles 301/308 redirections internally, the app will never receive the redirect response to request the updated location metadata. In addition, using the
auth/sessionendpoint to check for a redirect does not guarantee that the redirect response will be received. UpdateAppto always request the location information prior to the first http request after it is created. Update the websocket redirect response to always query the location metadata first before requesting an updated access token. Updated the App redirect tests to verify this sequence of events is taking place.Fixes #6485
☑️ ToDos
[ ] C-API, if public C++ API changed.