RCORE-2253 Redirected user authenticated app requests cause user to be logged out and location is not updated #8011

michael-wb · 2024-08-29T19:32:25Z

What, How & Why?

Updated the test http transport to enable HTTP redirection support in the Curl lib and added a test to verify the redirect operation. The Authorization header is not being included in the request when it is forwarded to the new location URL, leading the User to be logged out when a user authenticated appservices requests is performed. As a result, the handle_auth_failure() function was updated to request the location prior to refreshing the access token when an authenticated request fails on the first attempt, in order to ensure the client app is always connecting to the correct server hostname.

Added a test that is run when using the redirect server, that verifies the HTTP and websocket redirect operations since this support has been removed from App and has been enabled in the Curl lib used by the test harness.

The RedirectingHttpServer was updated to enable more control of the hostname and ws_hostname values returned by the location response and support was added to force a redirect response when an appservices request is received. An event hook was also added to be notified when a location request, http redirect, websocket redirect or error occurred.

Fixes #8008, #8012

☑️ ToDos

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

…308-tests

…to mwb/remove-308-support

…redir_server cmd line tool

…ions

test/object-store/util/sync/baas_admin_api.cpp

coveralls-official · 2024-08-30T01:13:31Z

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1387

Details

222 of 258 (86.05%) changed or added relevant lines in 5 files are covered.
65 unchanged lines in 19 files lost coverage.
Overall coverage decreased (-0.005%) to 91.103%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
test/object-store/util/test_file.cpp	1	5	20.0%
test/object-store/sync/app.cpp	154	169	91.12%
test/object-store/util/sync/redirect_server.hpp	56	73	76.71%

Files with Coverage Reduction	New Missed Lines	%
src/realm/query_expression.hpp	1	93.81%
src/realm/sync/network/websocket.cpp	1	72.2%
src/realm/sync/noinst/server/server_history.cpp	1	62.92%
src/realm/util/serializer.cpp	1	90.43%
src/realm/uuid.cpp	1	98.48%
test/fuzz_tester.hpp	1	57.73%
test/test_table.cpp	1	99.51%
src/realm/array_blobs_big.cpp	2	98.58%
src/realm/list.cpp	2	87.37%
src/realm/mixed.cpp	2	86.46%

Totals
Change from base Build 2602:	-0.005%
Covered Lines:	217392
Relevant Lines:	238621

💛 - Coveralls

…308-support

…into mwb/add-core-redirect-tests

…e-redirect-tests

jbreams · 2024-09-04T14:04:58Z

test/object-store/sync/app.cpp

+    auto app_session = get_runtime_app_session();
+
+    // Skip this test if not using the redirect server
+    if (!get_redirector())


Why wouldn't we just explicitly create a redirecting server for this test regardless of whether it's enabled globally?

Updated test to define its own copy of the redirect server.

jbreams · 2024-09-04T14:16:42Z

test/object-store/util/sync/baas_admin_api.hpp

@@ -282,6 +283,9 @@ std::string get_real_base_url();
 // your test is talking to.
 std::string get_admin_url();

+// Returns a reference to the redirector if it is enabled.
+std::optional<sync::RedirectingHttpServer>& get_redirector();


Instead of pulling the redirect server in as an include dependency of anyone using the baas admin API why not just instantiate one where you need it?

Removed the function in baas_admin_api.hpp and the test is now declaring its own local redirect server.

jbreams · 2024-09-04T14:17:54Z

test/object-store/util/sync/redirect_server.hpp

            , socket(service)
            , http_server(socket, logger)
        {
+            util::seed_prng_nondeterministically(random); // Throws


Why do we need to seed this non-deterministically? If you just use the catch seed then you can actually replay the random values when you get an error?

I changed it to util::seed_prng_nondeterministically(random) since I was having issues importing the Catch2 library for the redirect server command line tool. I reverted this change.

jbreams · 2024-09-04T14:19:12Z

test/object-store/util/sync/redirect_server.hpp

-        m_acceptor.open(m_endpoint.protocol());
-        m_acceptor.bind(m_endpoint);
-        m_endpoint = m_acceptor.local_endpoint();
+        reset_state();


why does this type need to be stateful?

The redirect server has state for force_http_redirect(), force_websocket_redirect(), and the event hook callback - this resets those values back to default (such as for the end of a test).
No longer needed since the redirector is being created in the test where it is used.

jbreams · 2024-09-04T14:19:23Z

test/object-store/util/sync/redirect_server.hpp

+        network::Endpoint ep;
+        if (listen_port != 0) {
+            // If a port is provided, then configure the endpoint with this port num
+            ep = network::Endpoint(network::make_address("0.0.0.0"), listen_port);


why do we need to supply an explicit listen port?

It was for the redirect server command line tool - removed.

jbreams · 2024-09-04T14:26:44Z

test/object-store/util/sync/redirect_server.hpp

            });
        });
    }

+    std::string trim_url(std::string_view str)


What is this used for?

It trims the whitespace from both ends and the trailing / from the redirect location URL in the constructor.
Since this was primarily for the redirect server command line tool, it has been removed.

jbreams · 2024-09-04T14:29:09Z

test/object-store/util/sync/redirect_server.hpp

+        return m_redirect_to_base_url;
+    }
+
+    std::string location_hostname() const


this seems like something a caller of this type should already know?

This returns the values that the redirect server /location endpoint will use so it can be displayed when using the redirect server command line tool - removed.

jbreams · 2024-09-04T14:30:54Z

test/object-store/util/sync/redirect_server.hpp

+    // value) and open a websocket conneciton to the actual server.
+    // NOTE: the websocket will never connect if both http and websockets are
+    // redirecting and will just keep getting the redirect close code.
+    void force_websocket_redirect(bool force = true)


It seems like this is always called with an argument, do you need a default argument value here?

Not necessary (thought was that calling force_websocket_redirect() would do just that - removed default

jbreams · 2024-09-04T14:31:02Z

test/object-store/util/sync/redirect_server.hpp

+    // NOTE: some http transport redirect implementations may strip the authorization
+    // header from the request after it is redirected and the user will be logged out
+    // from the client app as a result.
+    void force_http_redirect(bool remote = true)


It seems like this is always called with an argument, do you need a default argument value here?

Not necessary (thought was that calling force_http_redirect() would do just that - removed default

jbreams · 2024-09-04T14:35:55Z

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

@@ -853,7 +854,8 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std::
                completion(user, error);
            });
        },
-        false);
+        // Update location to make sure we're talking to the latest server before logging in
+        true);


I think getting logged out is the existing behavior that's always been there, do we really want to change that now?

You do get logged out, but you will never be able to logged in again due to the authorization header being stripped from the /profile request, which will log you out while you are trying to log in.
By updating the location when you attempt to log in, the client app will have the latest server location info and the login attempt should be successful, instead of failing when trying to query the user's profile.

So has redirection after a region migration always been totally broken? Like you cannot recover? Because I think all the SDKs HTTP implementations have been stripping the authorization header out when following a redirect this whole time...

It's definitely an edge case and I doubt any customers have hit this specific use case:
they have to do a deployment change while the app is already running; and the user has to have already performed some operation that updated the location prior to the deployment change, such as logging in.
After the deployment change (and the requests start getting redirected), any app services request like updating the access token will log the user out and they won't be able to successfully log in again.

Fortunately, restarting the app will also resolve the issue, since it will require the location to be updated before sending any app services requests.

We talked about this offline - instead of making every call to log_in_with_credentials() pre-emptively request a location update, we're going to request a location update if the call to get the user's profile fails with a 401 unauthorized error. That way this error handling gets a bit slower in this edge case, but the behavior of all other log_in_with_credentials() should stay the same.

…command line tool

michael-wb

Most of your comments are around extra code that was added when running the redirect server using the command line tool. While it was helpful for creating the test, it sounds like we don't want the tool, so many of those changes in this PR have been reverted.

michael-wb · 2024-09-04T15:22:58Z

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

@@ -853,7 +854,8 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std::
                completion(user, error);
            });
        },
-        false);
+        // Update location to make sure we're talking to the latest server before logging in
+        true);


You do get logged out, but you will never be able to logged in again due to the authorization header being stripped from the /profile request, which will log you out while you are trying to log in.
By updating the location when you attempt to log in, the client app will have the latest server location info and the login attempt should be successful, instead of failing when trying to query the user's profile.

michael-wb · 2024-09-04T15:29:28Z

test/object-store/sync/app.cpp

+    auto app_session = get_runtime_app_session();
+
+    // Skip this test if not using the redirect server
+    if (!get_redirector())


Updated test to define its own copy of the redirect server.

michael-wb · 2024-09-04T15:40:47Z

test/object-store/util/sync/redirect_server.hpp

            , socket(service)
            , http_server(socket, logger)
        {
+            util::seed_prng_nondeterministically(random); // Throws


I changed it to util::seed_prng_nondeterministically(random) since I was having issues importing the Catch2 library for the redirect server command line tool. I reverted this change.

michael-wb · 2024-09-04T15:41:57Z

test/object-store/util/sync/redirect_server.hpp

                return;
            }

            conn->http_server.async_receive_request([this, conn](HTTPRequest req, std::error_code ec) {
+                static bool use_301 = false; // send 301 on first redirect response


It is actually switching between sending 301 and 308 after every message on line 353 to verify both redirect statuses are supported. It just initially starts out with 301.

I updated this to a member variable and added a comment about the code.

michael-wb · 2024-09-04T15:47:54Z

test/object-store/util/sync/redirect_server.hpp

            });
        });
    }

+    std::string trim_url(std::string_view str)


It trims the whitespace from both ends and the trailing / from the redirect location URL in the constructor.
Since this was primarily for the redirect server command line tool, it has been removed.

michael-wb · 2024-09-04T16:05:37Z

test/object-store/util/sync/redirect_server.hpp

+    // NOTE: some http transport redirect implementations may strip the authorization
+    // header from the request after it is redirected and the user will be logged out
+    // from the client app as a result.
+    void force_http_redirect(bool remote = true)


Not necessary (thought was that calling force_http_redirect() would do just that - removed default

michael-wb · 2024-09-04T16:05:49Z

test/object-store/util/sync/redirect_server.hpp

+    // value) and open a websocket conneciton to the actual server.
+    // NOTE: the websocket will never connect if both http and websockets are
+    // redirecting and will just keep getting the redirect close code.
+    void force_websocket_redirect(bool force = true)


Not necessary (thought was that calling force_websocket_redirect() would do just that - removed default

michael-wb · 2024-09-04T16:06:25Z

test/object-store/util/sync/redirect_server.hpp

+        return m_redirect_to_base_url;
+    }
+
+    std::string location_hostname() const


This returns the values that the redirect server /location endpoint will use so it can be displayed when using the redirect server command line tool - removed.

michael-wb · 2024-09-04T16:18:49Z

test/object-store/util/sync/baas_admin_api.hpp

@@ -282,6 +283,9 @@ std::string get_real_base_url();
 // your test is talking to.
 std::string get_admin_url();

+// Returns a reference to the redirector if it is enabled.
+std::optional<sync::RedirectingHttpServer>& get_redirector();


Removed the function in baas_admin_api.hpp and the test is now declaring its own local redirect server.

test/object-store/util/sync/baas_admin_api.cpp

…e-redirect-tests

jedelbo · 2024-10-18T11:11:22Z

@jbreams should this just be closed or is there anything valuable to preserve?

Michael Wilkerson-Barker added 10 commits August 23, 2024 00:11

Removed redirect tests

aa34635

Removed one more location redirect test case

fd52bb1

Removed 301/308 redirection support from App

3a9c663

Updated changelog

519a71e

Updates from review

8a753e5

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

d5c2800

…308-tests

Merge branch 'mwb/remove-308-tests' of github.com:realm/realm-core in…

3cc2261

…to mwb/remove-308-support

Updated redirect server to support App request redirects and created …

aba04c7

…redir_server cmd line tool

Added test to verify http redirects using CURL to handle the redirect…

e0a971d

…ions

Added test sections and print error on create_user_and_login() failure

420d56a

michael-wb self-assigned this Aug 29, 2024

cla-bot bot added the cla: yes label Aug 29, 2024

michael-wb changed the title ~~RCORE-2252 Add test to handle redirects in CURL test transport and add a redirect test run~~ RCORE-2253 Redirected user authenticated app requests cause user to be logged out and location is not updated Aug 29, 2024

This was linked to issues Aug 29, 2024

Add test to handle redirects in CURL test transport and add a redirect test run #8008

Closed

Redirected user authenticated app requests cause user to be logged out and location is not updated #8012

Open

Updated changelog; some cleanup; moving redir_server tool to separate PR

a06e319

michael-wb commented Aug 29, 2024

View reviewed changes

test/object-store/util/sync/baas_admin_api.cpp Outdated Show resolved Hide resolved

Michael Wilkerson-Barker added 2 commits August 29, 2024 20:06

Addressed some build and test failures

69c232c

More minor updates to fix build and test failures

3c0cd07

michael-wb marked this pull request as ready for review August 30, 2024 01:55

michael-wb requested review from jbreams, danieltabacaru and tgoyne August 30, 2024 01:55

Michael Wilkerson-Barker added 4 commits August 30, 2024 10:50

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

4e8484e

…308-support

Updated changelog after release

5728e1f

Fixed misspelling and updated comment a bit

3c6229a

Merge branch 'mwb/remove-308-support' of github.com:realm/realm-core …

dd1d4bc

…into mwb/add-core-redirect-tests

Base automatically changed from mwb/remove-308-support to master August 30, 2024 16:09

An error occurred while trying to automatically change base from mwb/remove-308-support to master August 30, 2024 16:09

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

0d46d77

…e-redirect-tests

michael-wb mentioned this pull request Sep 3, 2024

Added tool to run the redirect server from the command line #8016

Closed

jbreams reviewed Sep 4, 2024

View reviewed changes

Updates from review - removed some changes needed by redirect server …

ebcfe93

…command line tool

michael-wb commented Sep 4, 2024

View reviewed changes

Michael Wilkerson-Barker added 3 commits September 4, 2024 14:44

rerun validation

9a9371d

Fixed TSAN error

307fdb7

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

2b2c59c

…e-redirect-tests

michael-wb requested a review from jbreams September 5, 2024 16:28

Michael Wilkerson-Barker added 6 commits September 6, 2024 23:12

Update location after auth failure; updated test comments

1d1edf9

I thought I removed this line...

ce1aeec

Reverted line now that login is not always requesting location

a4e967a

Updated changelog

761f18b

a little more cleanup

49961ab

Fixed refresh access token test

ee81ab4

RCORE-2253 Redirected user authenticated app requests cause user to be logged out and location is not updated #8011

Are you sure you want to change the base?

RCORE-2253 Redirected user authenticated app requests cause user to be logged out and location is not updated #8011

Conversation

michael-wb commented Aug 29, 2024 • edited Loading

What, How & Why?

☑️ ToDos

coveralls-official bot commented Aug 30, 2024 • edited Loading

Pull Request Test Coverage Report for Build michael.wilkersonbarker_1387

Details

💛 - Coveralls

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

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

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

Choose a reason for hiding this comment

michael-wb Sep 4, 2024 • edited Loading

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

michael-wb left a comment

Choose a reason for hiding this comment

michael-wb Sep 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedelbo commented Oct 18, 2024

michael-wb commented Aug 29, 2024 •

edited

Loading

coveralls-official bot commented Aug 30, 2024 •

edited

Loading

michael-wb Sep 4, 2024 •

edited

Loading

michael-wb Sep 4, 2024 •

edited

Loading