Access token refresh for websockets was not updating the location metadata #6631

michael-wb · 2023-05-15T17:06:49Z

What, How & Why?

When the server location is moved as part of a deployment model change, the current server URL will start to reply with a permanent redirect response to indicate the new server URL. If a websocket attempts to connect to the old server URL, the permanent redirect response causes an access token refresh, which was updated to request the updated URL information prior to refreshing the access token. However, due to a change in PR #6505, the location was not being requested, since it had already been requested when the App was started. This PR also fixes the test for this operation, which mistakenly used find_first_of("/location") (which searches for the first instance of any of the characters in the string) instead of find("/location") to verify the first request after a websocket redirect was to update the location.

In addition, the App was updated to request the location information on a log in, in case the App was never restarted after the deployment model was changed, since all users will be logged out when this change occurs.

A new test was also added to verify that the user is logged out in case an error occurs (e.g. too many redirects) during the location refresh after a permanent redirect is received while connecting the websocket.

Fixes #6630

☑️ ToDos

📝 Changelog update
~~[ ] 🚦 Tests (or not relevant)~~
~~[ ] C-API, if public C++ API changed.~~

…re test

…location-on-login

danieltabacaru · 2023-05-17T21:55:56Z

test/object-store/c_api/c_api.cpp

@@ -288,8 +288,9 @@ class CApiUnitTestTransport : public app::GenericNetworkTransport {
    std::string m_provider_type;

 public:
-    CApiUnitTestTransport(const std::string& provider_type = "anon-user")
-        : m_provider_type(provider_type)
+    CApiUnitTestTransport(const std::string& provider_type = {}, uint64_t request_timeout = 60000)


why the change for the provider_type? I think the previous default value fit it well

I moved the default designation to the member variable initializer (if the string is empty) so I could call this constructor from a different constructor that only takes the request_timeout value without having to duplicate the "anon-user" string.

This test started failing due to the request_timeout being set to 2500 instead of the default 60000. I don't know why it wasn't failing before...

…location-on-login

kmorkos · 2023-05-18T17:28:04Z

src/realm/object-store/sync/app.cpp

+    // Always update the location on a login to keep the location metadata up to date in cases where
+    // the transport handles the redirection automatically and does not return the redirect response
+    // Since the user is logged out as part of a deployment model change, the user will need to log in
+    // again, but Core will not know that this was driven by an HTTP redirection.


[nit] I think the first half of this comment is missing a lot of context on why redirects are relevant here. Maybe reword so it leads with some more context?

…ting location update to use flag

ironage

LGTM

ironage · 2023-05-19T02:57:50Z

src/realm/object-store/sync/app.cpp

@@ -902,7 +924,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);


consider using util::CheckedMutex and associated GUARDED_BY on m_location_updated so that the compiler tells us if we forget things like this (non-blocking)

Thanks - I am actually going to do this in a separate PR with some clean up of the App route components.

ironage · 2023-05-19T02:58:54Z

test/object-store/sync/app.cpp

                if (request_count++ == 0) {
-                    logger->trace("request.url (%1): %2", request_count, request.url);
-                    // First request should be to location
-                    REQUIRE(request.url.find_first_of("/location") != std::string::npos);


good catch!

danieltabacaru · 2023-05-22T13:24:00Z

src/realm/object-store/sync/app.cpp

+    // the HTTP and websocket URL values will never be updated with the new server location.
+    {
+        std::lock_guard<std::mutex> lock(*m_route_mutex);
+        m_location_updated = false;


This is a bit hard to follow and error prone imo. Is it possible to add it as a parameter parameter to do_request? There doesn't seem to be much logic around it.

Removed updating the bool directly and moved the logic into do_request() via an extra update_location parameter to force updating the location info.

danieltabacaru

LGTM mod one small consideration

…location-on-login

…e-core-13.13 * commit '36b2df44453ba5e830369c76ab0683799a287605': (34 commits) core release 13.13.0 Fix an assertion failure if an async write callback ran during a write transaction (#6661) Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() (#6667) Allow geo coordinate numeric argument substitutions (#6663) Fix a lock order inversion in tests (#6666) Avoid performing unaligned reads in Array::get_chunk() Fix a few UBSan failures hit by tests Use std::optional to store cached leaves in query nodes (#6653) maybe fix a race in a test (#6651) Access token refresh for websockets was not updating the location metadata (#6631) Updated release notes Prepare release Updated baas server tag for CI (#6650) Geospatial basic queries benchmarks (#6621) Geospatial feedback (#6645) Add support for early exit in BPlusTree::for_all() Filter out unresolved links in Dictionary::get_any() Benchmark for full-text search geospatial validation of polygons (#6607) Fix importing Results with deleted collection ...

* Updated release notes * Update catch2 v3.3.2 (#6605) * Make core infer platform and cpu_arch, while bundle_id must be provided by SDK's (#6612) * platform and cpu_arch are inferred by core, bundle_id must be provided by SDK's * update changelog * Return proper value for X86_64 arch Co-authored-by: Christian Melchior <christian.melchior@mongodb.com> * Get fine-grained platform for Apple devices * Fix tests * small fixes * fix more tests * Fix mistake in changelog --------- Co-authored-by: Christian Melchior <christian.melchior@mongodb.com> * use consistent rounding, following SERVER-75392 (#6477) * fix entries that went to the wrong change version (#6632) * Special-case main thread runloop scheduler * Improve SectionedResults performance SectionedResults used a std::map in a few places where the keys are a dense range (i.e. always [0..map.size())) and so they can be std::vector instead. The maps keyed on Mixed are now std::unordered_map. Change notifications now report changes as a `std::vector<IndexSet>` rather than `std::map<size_t, IndexSet>`. This is slower and uses more memory when the only sections that changed are near the end of a SectionedResults with a large number of sections, but is much faster if all sections changed or if the sections which changed are early in the SectionedResults. Change notifications now reuse buffers, which increases persistent memory usage slightly but significant reduces allocations. Change notifications for a single section now only compute the changes for that section rather than computing the full changes and then filtering out the changes for other sections. * use static_assert rather than a old home rolled one * fix warning of redefine of CHECK macro * fix unused function warning * silence warnings in bid128_to_string * Introduce BPlusTree::for_all * Prevent program from crashing when removing backlinks * Fix broken snapshot of collection of objects * Fix importing Results with deleted collection The result should be an empty result, not the whole table. * geospatial validation of polygons (#6607) * geospatial validation of polygons * Loop->Ring, added tests * use std::unique * changelog * Benchmark for full-text search * Allow to filter benchmark and run only list of specified names * Add simple benchmark for fulltext search with index * Filter out unresolved links in Dictionary::get_any() * Add support for early exit in BPlusTree::for_all() * Geospatial feedback (#6645) * verify local results match a server query * disallow geowithin on top level tables * fix geo queries with ANY/ALL/NONE * geospatial validation of points * rename GeoCenterSphere -> GeoCircle * review feedback * better testing and fix any/all/none geospatial * format * Geospatial basic queries benchmarks (#6621) * Add basic benchmarks for Geospatial type and queries * Less copying in GeoWithinCompare * Bring back caching of s2 region into Geospatial * remove transaction overhead from measurements * a couple small optimizations * formatting * simplify geospatial query evaluations * changelog --------- Co-authored-by: James Stone <james.stone@mongodb.com> * Updated baas server tag for CI (#6650) * Prepare release * Updated release notes * Access token refresh for websockets was not updating the location metadata (#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 * maybe fix a race in a test (#6651) * Use std::optional to store cached leaves in query nodes (#6653) Our use of aligned_storage was basically a complicated manual version of this. I was hoping this'd have binary size benefits, but it ended up making the library 100 bytes larger instead. Nonetheless, it greatly simplifies things. * Fix a few UBSan failures hit by tests * Avoid performing unaligned reads in Array::get_chunk() * Fix a lock order inversion in tests (#6666) The cycle was DaemonThread::m_running_on_change_mutex => RealmCoordinator::m_realm_mutex => SyncManager::m_mutex => RealmCoordinator::s_coordinator_mutex => DaemonThread::m_running_on_change_mutex, and it happened due to DaemonThread::remove() being called inside RealmCoordinator::clear_cache() while holding s_coordinator_mutex. Fortunately we don't actually need to be doing that. As the cycle required RealmCoordinator::clear_all_caches(), this was only applicable to tests. * Allow geo coordinate numeric argument substitutions (#6663) * allow geo coordinate numeric argument substitutions * review feedback * explicit cast to address warning * Remove catch() clause to prevent truncating stack trace in AsyncOper::do_recycle_and_execute() (#6667) * Fix an assertion failure if an async write callback ran during a write transaction (#6661) Between when the callback after acquiring the write lock is scheduled and when it's invoked a synchronous write transaction can be begun, and if it's not ended before the next time the scheduler gets to run, the scheduled callback will be invoked inside the write. When this happens we want to just do nothing. Ending the synchronous write transaction will take care of rescheduling the async write it preempted. * core release 13.13.0 * Updated release notes * Allocate arguments for lists (#6674) * Small documentation and code fixes (#6672) * Fix crash when opening FLX realm after client reset failure (#6671) * Fix crash when opening FLX realm after client reset failure * Update changelog * Don't superceed pending subscriptions in case of a client reset failure * Add test * Changes after code review * Support sorting based on values from a dictionary (#5311) Co-authored-by: Sebastian Valle <svalle@gascaribe.com> Co-authored-by: James Stone <james.stone@mongodb.com> * Filter out external sources from Eclipse (#6682) Indexer has a hard time dealing with Catch2 * Use cross-compilers instead of CentOS image (#6559) * Use cross-compilers instead of CentOS image * changelog * fix bad merge * refactor toolchain files * clarify useToolchain exception circumstances * Remap github URL to ssh to fix BAAS dependency using https:// (#6685) * core v13.14.0 * Updated release notes * Switch to building with Xcode 14 (#6647) * better fix explanation in the changelog for list of args in the query parser (#6692) * Remove constructor for GeoPoint and GeoPolygon (#6679) Co-authored-by: Mathias Stearn <redbeard0531@gmail.com> * Fix failing "sync: non-synced metadata table doesn't result in non-additive schema change" tests (#6697) * Reporting correct error message on HTTP errors for Browser target * User/Server API key provider becomes a single 'API key' provider (#6696) * Allow frozen Realms to be opened with additive schema changes (#6693) * allow frozen Realms to be opened with additive schema changes * lint * strengthen tests and comments * Update src/realm/object-store/shared_realm.cpp Co-authored-by: Thomas Goyne <thomas.goyne@mongodb.com> --------- Co-authored-by: Thomas Goyne <thomas.goyne@mongodb.com> * Reverted minimum swift version to fix failing CI tests (#6706) * core release v13.15.0 * Updated release notes * Fix client reset test with invalid query (#6711) * Fix SessionWrapper use-after-free crash when tearing down sessions (#6676) * Changed SessionWrapper pointer to bind_ptr; added session ident history * Fix teardown if client is destroyed before session * Session no longer holds bind_ptr to SessionWrapper; reverted some changes * Fixed return and updated some comments * Don't process errors if session is shutting down * Added extra checks for session state * Updates from review * Updated some finalized checks * Rolled back some changes * Added output to ASSERTS and moved session history to unordered_set * Remove session history entry on normal close * Updated comment in sync tests * Add [baas] and [local] tags to object store sync tests to identify the tests that rely on BAAS or not (#6710) * Use Columns<Link> when property is Dictionary of links (#6705) If a Dictionary property has links as value type, we can use Columns<Link> to handle the links instead of the basic Columns<Dictionary>. This has the effect that when we compare with a single value, we will optimize to use LinksToNode. So we need to make LinksToNode handle the Dictionary case. When we compare with a list of links, we must ensure that the list is converted to a list obj ObjKeys - which is the type that Column<Link> evaluates to. Use LinksToNode for lists in QueryParser * better changelog message for the fix related to queries with list of arguments (#6717) * Fixes for Emscripten target (Passing header from fetch response. Using Config.path for inMemory Realm) (#6716) * Fixes for Emscripten target: Passing header for fetch response. Passing the RealmConfig.path to be used for inMemory Realm, this is needed for registering SyncSession Co-authored-by: Jørgen Edelbo <jorgen.edelbo@mongodb.com> * release 13.15.1 * Updated spec.yml to remove User & Server prefix from ApiKey credentials --------- Co-authored-by: James Stone <james.stone@mongodb.com> Co-authored-by: realm-ci <ci@realm.io> Co-authored-by: Kirill Burtsev <kirill.burtsev@mongodb.com> Co-authored-by: Daniel Tabacaru <96778637+danieltabacaru@users.noreply.github.com> Co-authored-by: Christian Melchior <christian.melchior@mongodb.com> Co-authored-by: Thomas Goyne <tg@realm.io> Co-authored-by: Thomas Goyne <thomas.goyne@mongodb.com> Co-authored-by: Jørgen Edelbo <jorgen.edelbo@mongodb.com> Co-authored-by: Michael Wilkerson-Barker <michael.wilkersonbarker@mongodb.com> Co-authored-by: Nicola Cabiddu <nicola.cabiddu@mongodb.com> Co-authored-by: Sebastian Valle <svalle@gascaribe.com> Co-authored-by: Yavor Georgiev <fealebenpae@users.noreply.github.com> Co-authored-by: Ferdinando Papale <4850119+papafe@users.noreply.github.com> Co-authored-by: Mathias Stearn <redbeard0531@gmail.com> Co-authored-by: Nabil Hachicha <nabil.hachicha@gmail.com> Co-authored-by: Finn Schiermer Andersen <finn.schiermer.andersen@gmail.com>

cla-bot bot added the cla: yes label May 15, 2023

michael-wb self-assigned this May 15, 2023

michael-wb force-pushed the mwb/update-location-on-login branch from 2b68702 to 32616bf Compare May 16, 2023 00:21

michael-wb added 4 commits May 16, 2023 11:30

Always refresh metadata on app login

ec45352

Updated changelog

5d5ac4c

Always update location when requested; fix c_api test

b07e302

Update test to properly evaluate websocket redirections; added one mo…

067c9bd

…re test

michael-wb force-pushed the mwb/update-location-on-login branch from 919604a to 067c9bd Compare May 16, 2023 15:30

michael-wb changed the title ~~Always refresh metadata on app login~~ Access token refresh for websockets was not updating the location metadata May 16, 2023

Updated changelog and fixed compile warning

c7c3fc1

michael-wb requested review from jbreams, kmorkos and danieltabacaru May 16, 2023 21:23

Merge branch 'master' of github.com:realm/realm-core into mwb/update-…

b693fef

…location-on-login

danieltabacaru reviewed May 17, 2023

View reviewed changes

Merge branch 'master' of github.com:realm/realm-core into mwb/update-…

3d9915d

…location-on-login

michael-wb requested a review from ironage May 18, 2023 16:18

Added location checks back to test

a87056f

kmorkos approved these changes May 18, 2023

View reviewed changes

michael-wb added 2 commits May 18, 2023 19:03

added mutex locking around location updated state and reworked reques…

afbd312

…ting location update to use flag

clang format and fix incorrect timeout value

51f421e

ironage approved these changes May 19, 2023

View reviewed changes

danieltabacaru reviewed May 22, 2023

View reviewed changes

danieltabacaru approved these changes May 22, 2023

View reviewed changes

michael-wb added 3 commits May 22, 2023 15:42

Merge branch 'master' of github.com:realm/realm-core into mwb/update-…

435d86c

…location-on-login

Reworked update location logic a bit and removed unused function

bab7300

Free mutex before calling completion on early exit in init_app_metadata

9025c2e

michael-wb merged commit cb9b80b into master May 23, 2023
27 checks passed

michael-wb deleted the mwb/update-location-on-login branch May 23, 2023 04:27

github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access token refresh for websockets was not updating the location metadata #6631

Access token refresh for websockets was not updating the location metadata #6631

michael-wb commented May 15, 2023 •

edited

danieltabacaru May 17, 2023

michael-wb May 17, 2023

michael-wb May 18, 2023

kmorkos May 18, 2023

ironage left a comment

ironage May 19, 2023

michael-wb May 19, 2023

ironage May 19, 2023

danieltabacaru May 22, 2023

michael-wb May 22, 2023

danieltabacaru left a comment

Access token refresh for websockets was not updating the location metadata #6631

Access token refresh for websockets was not updating the location metadata #6631

Conversation

michael-wb commented May 15, 2023 • edited

What, How & Why?

☑️ ToDos

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ironage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danieltabacaru left a comment

Choose a reason for hiding this comment

michael-wb commented May 15, 2023 •

edited