Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RCORE-2200: Fix switch_user and get_profile during log_in_with_credentials #7894

Merged
merged 13 commits into from
Jul 24, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
* <How do the end-user experience this issue? what was the impact?> ([#????](https://github.com/realm/realm-core/issues/????), since v?.?.?)
* FLX download progress was only updated when bootstraps completed, making it always be 0 before the first completion and then forever 1. ([PR #7869](https://github.com/realm/realm-core/issues/7869), since v14.10.2)
* Sync client can crash if a session is resumed while the session is being suspended. ([#7860](https://github.com/realm/realm-core/issues/7860), since v12.0.0)
* App subscription callback was getting fired before the user profile was retrieved on login, leading to an empty user profile when using the callback. ([#7889](https://github.com/realm/realm-core/issues/7889), since v14.7.0)

### Breaking changes
* None.
Expand Down
8 changes: 6 additions & 2 deletions src/realm/object-store/sync/app.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -835,8 +835,12 @@ void App::log_in_with_credentials(const AppCredentials& credentials, const std::
return completion(nullptr,
AppError(ErrorCodes::BadToken, "Could not log in user: received malformed JWT"));
}
switch_user(user);
get_profile(user, std::move(completion));

get_profile(user, [this, completion = std::move(completion)](const std::shared_ptr<User>& user,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't worked with C++ in a little while, tried to get it to be equivalent but let me know if this move, etc. doesn't make sense

Optional<AppError> error) {
switch_user(user);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably only want to switch users if the profile fetch succeeded?

Copy link
Contributor Author

@gagik gagik Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that be a better experience? We'd potentially be breaking more functionality which isn't necessarily dependent on the user's profile itself (i.e. giving the client the new user on completition callback but it having no profile and an error to handle seems nicer than not giving the client the new user at all or giving it on the completition callback with the error but not actually switching the user)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the initial profile fetch fails then the user effectively isn't logged in yet and isn't really in a usable state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tgoyne made the change, let me know if more is needed

completion(user, error);
});
},
false);
}
Expand Down
14 changes: 14 additions & 0 deletions test/object-store/sync/app.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4172,6 +4172,18 @@ TEST_CASE("app: jwt login and metadata tests", "[sync][app][user][metadata][func

SECTION("jwt happy path") {
bool processed = false;
bool first_login = true;

auto token = app->subscribe([&first_login, &app](auto&) {
if (first_login) {
auto user = app->current_user();
auto metadata = user->user_profile();

// Ensure that the JWT metadata fields are available when the callback is fired on login.
CHECK(metadata["name"] == "Foo Bar");
first_login = false;
}
});

std::shared_ptr<User> user = log_in(app, AppCredentials::custom(jwt));

Expand All @@ -4192,6 +4204,8 @@ TEST_CASE("app: jwt login and metadata tests", "[sync][app][user][metadata][func
auto custom_data = *user->custom_data();
CHECK(custom_data["name"] == "Not Foo Bar");
CHECK(metadata["name"] == "Foo Bar");

app->unsubscribe(token);
}
}

Expand Down
Loading