log out the user on invalid refresh token #4961
Conversation
ironage
force-pushed
the
js/expired-refresh-token
branch
from
ec5a8d7 to
11cf28b
Compare
| return; | ||
| return; // this response came in after the app shut down, ignore it | ||
| } | ||
| else if (error->error_code.category() == app::client_error_category()) { |
There was a problem hiding this comment.
These cases were previously covered by accident by the check on refresh_token_is_expired() because the refresh token for a logged out user was by default set to 0 which means that the expiry code path was triggered.
| @@ -359,6 +359,23 @@ const SyncSession::State& SyncSession::State::dying = Dying(); | |||
| const SyncSession::State& SyncSession::State::inactive = Inactive(); | |||
| const SyncSession::State& SyncSession::State::waiting_for_access_token = WaitingForAccessToken(); | |||
|
|
|||
| void SyncSession::handle_bad_auth(std::shared_ptr<SyncUser> user, std::error_code error_code, | |||
There was a problem hiding this comment.
can this take a const std::shared_ptr<SyncUser>& and const std::string&? No need to make extra copies of stuff.
| void SyncSession::handle_bad_auth(std::shared_ptr<SyncUser> user, std::error_code error_code, | ||
| std::string context_message) | ||
| { | ||
| // TODO: ideally this would write to the logs as well in case users didn't set up their error handler. |
There was a problem hiding this comment.
Do we want to do that now? How hard would it be to do this TODO?
There was a problem hiding this comment.
Fixing this involves punching holes in the existing API to get hold of a logger instance. I don't see it as a huge issue, so I'd prefer to leave it until we do some refactoring of the sync session.
There was a problem hiding this comment.
Gotcha, let's leave it for later then.
test/object-store/sync/app.cpp
Outdated
| @@ -2006,6 +2034,50 @@ TEST_CASE("app: sync integration", "[sync][app]") { | |||
| REQUIRE(!user->is_logged_in()); | |||
| }; | |||
|
|
|||
| /* This test can be used to manually check the behaviour of an expired refresh token | |||
There was a problem hiding this comment.
Can you add something to this comment about how to uncomment this test and actually use it? Or can we just not commit this? Having big chunks of commented out code seems a little iffy.
| } | ||
| if (m_config.error_handler) { | ||
| auto user_facing_error = SyncError(realm::sync::ProtocolError::bad_authentication, context_message, true); | ||
| m_config.error_handler(shared_from_this(), user_facing_error); |
There was a problem hiding this comment.
std::move(user_facing_error) here to avoid unnecessary copies.
CHANGELOG.md
Outdated
| @@ -17,6 +17,7 @@ | |||
| * Fixed a rare segfault which could trigger if a user was being logged out while the access token refresh response comes in. ([#4944](https://github.com/realm/realm-core/issues/4944), since v10) | |||
| * Fixed a bug where progress notifiers continue to be called after the download of a synced realm is complete. ([#4919](https://github.com/realm/realm-core/issues/4919)) | |||
| * Fixed an issue where the release process was only publishing armeabi-v7a Android binaries. ([#4952](https://github.com/realm/realm-core/pull/4952), since v10.6.0) | |||
| * Fixed the user being left logged in when his refresh token expires. ([#4882](https://github.com/realm/realm-core/issues/4882), since v10) | |||
There was a problem hiding this comment.
| * Fixed the user being left logged in when his refresh token expires. ([#4882](https://github.com/realm/realm-core/issues/4882), since v10) | |
| * Fixed the user being left logged in when their refresh token expires. ([#4882](https://github.com/realm/realm-core/issues/4882), since v10) |
ironage
force-pushed
the
js/expired-refresh-token
branch
from
11cf28b to
0eab501
Compare
| void SyncSession::handle_bad_auth(std::shared_ptr<SyncUser> user, std::error_code error_code, | ||
| std::string context_message) | ||
| { | ||
| // TODO: ideally this would write to the logs as well in case users didn't set up their error handler. |
There was a problem hiding this comment.
Gotcha, let's leave it for later then.
* log out the user on invalid refresh token * touch up according to review feedback
Fixes #4882
Instead of checking on the client side for the refresh token expiry which relies on the client's clock to be correct, let the server tell us if the refresh token is valid or not via its response code. Our existing error handling for 401 covers this already so it simplifies the code there. I was able to verify that the server response in this case is 401 by manually advancing my clock by a year, I've added the test code to check this commented out, but I'm not sure that is useful or if it should be removed.
☑️ ToDos