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.

Sign up for GitHub

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

SDK: Ignore the discovered/custom sliding sync proxy when using SSS. #3743

Merged
merged 2 commits into from
Jul 23, 2024
+66 −35
Merged

SDK: Ignore the discovered/custom sliding sync proxy when using SSS. #3743

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e50a580
ffi: Use the SDK's (tested) logic for overriding the sliding sync proxy.
pixlwave Jul 19, 2024
886054e
sdk: Ignore the sliding sync proxy value when using SSS.
pixlwave Jul 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions bindings/matrix-sdk-ffi/src/client_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use matrix_sdk::{
};
use ruma::api::error::{DeserializationError, FromHttpResponseError};
use tracing::{debug, error};
use url::Url;
use zeroize::Zeroizing;

use super::{client::Client, RUNTIME};
Expand Down Expand Up @@ -527,6 +526,10 @@ impl ClientBuilder {

inner_builder = inner_builder.with_encryption_settings(builder.encryption_settings);

if let Some(sliding_sync_proxy) = builder.sliding_sync_proxy {
inner_builder = inner_builder.sliding_sync_proxy(sliding_sync_proxy);
}

inner_builder =
inner_builder.simplified_sliding_sync(builder.is_simplified_sliding_sync_enabled);

Expand All @@ -536,25 +539,6 @@ impl ClientBuilder {

let sdk_client = inner_builder.build().await?;

// At this point, `sdk_client` might contain a `sliding_sync_proxy` that has
// been configured by the homeserver (if it's a `ServerName` and the
// `.well-known` file is filled as expected).
//
// If `builder.sliding_sync_proxy` contains `Some(_)`, it means one wants to
// overwrite this value. It would be an error to call
// `sdk_client.set_sliding_sync_proxy()` with `None`, as it would erase the
// `sliding_sync_proxy` if any, and it's not the intended behavior.
//
// So let's call `sdk_client.set_sliding_sync_proxy()` if and only if there is
// `Some(_)` value in `builder.sliding_sync_proxy`. That's really important: It
// might not break an existing app session, but it is likely to break a new
// session, which not immediate to detect if there is no test.
if !builder.is_simplified_sliding_sync_enabled {
if let Some(sliding_sync_proxy) = builder.sliding_sync_proxy {
sdk_client.set_sliding_sync_proxy(Some(Url::parse(&sliding_sync_proxy)?));
}
}

Ok(Arc::new(
Client::new(
sdk_client,
Expand Down
77 changes: 62 additions & 15 deletions crates/matrix-sdk/src/client/builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -450,6 +450,7 @@ impl ClientBuilder {

let http_client = HttpClient::new(inner_http_client.clone(), self.request_config);

#[allow(unused_variables)]
let (homeserver, well_known) = match homeserver_cfg {
HomeserverConfig::Url(url) => (url, None),

Expand All @@ -471,9 +472,14 @@ impl ClientBuilder {
let mut sliding_sync_proxy =
self.sliding_sync_proxy.as_ref().map(|url| Url::parse(url)).transpose()?;

#[allow(unused_variables)]
if let Some(well_known) = well_known {
#[cfg(feature = "experimental-sliding-sync")]
#[cfg(feature = "experimental-sliding-sync")]
if self.is_simplified_sliding_sync_enabled {
// When using Simplified MSC3575, don't use a sliding sync proxy, allow the
// requests to be sent directly to the homeserver.
tracing::info!("Simplified MSC3575 is enabled, ignoring any sliding sync proxy.");
sliding_sync_proxy = None;
} else if let Some(well_known) = well_known {
// Otherwise, if a proxy wasn't set, use the one discovered from the well-known.
if sliding_sync_proxy.is_none() {
sliding_sync_proxy =
well_known.sliding_sync_proxy.and_then(|p| Url::parse(&p.url).ok())
Expand Down Expand Up @@ -879,7 +885,7 @@ pub(crate) mod tests {
#[async_test]
async fn test_discovery_invalid_server() {
// Given a new client builder.
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

// When building a client with an invalid server name.
builder = builder.server_name_or_homeserver_url("⚠️ This won't work 🚫");
Expand All @@ -892,7 +898,7 @@ pub(crate) mod tests {
#[async_test]
async fn test_discovery_no_server() {
// Given a new client builder.
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

// When building a client with a valid server name that doesn't exist.
builder = builder.server_name_or_homeserver_url("localhost:3456");
Expand All @@ -908,7 +914,7 @@ pub(crate) mod tests {
// Given a random web server that isn't a Matrix homeserver or hosting the
// well-known file for one.
let server = MockServer::start().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

// When building a client with the server's URL.
builder = builder.server_name_or_homeserver_url(server.uri());
Expand All @@ -922,7 +928,7 @@ pub(crate) mod tests {
async fn test_discovery_direct_legacy() {
// Given a homeserver without a well-known file.
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

// When building a client with the server's URL.
builder = builder.server_name_or_homeserver_url(homeserver.uri());
Expand All @@ -938,7 +944,7 @@ pub(crate) mod tests {
// Given a homeserver without a well-known file and with a custom sliding sync
// proxy injected.
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();
#[cfg(feature = "experimental-sliding-sync")]
{
builder = builder.sliding_sync_proxy("https://localhost:1234");
Expand All @@ -958,7 +964,7 @@ pub(crate) mod tests {
// Given a base server with a well-known file that has errors.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

let well_known = make_well_known_json(&homeserver.uri(), None);
let bad_json = well_known.to_string().replace(',', "");
Expand All @@ -985,7 +991,7 @@ pub(crate) mod tests {
// doesn't support sliding sync.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
Expand All @@ -1011,7 +1017,7 @@ pub(crate) mod tests {
// sliding sync proxy.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
Expand All @@ -1038,7 +1044,7 @@ pub(crate) mod tests {
// sliding sync proxy.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
Expand All @@ -1061,6 +1067,33 @@ pub(crate) mod tests {
assert_eq!(client.sliding_sync_proxy(), Some("https://localhost:9012".parse().unwrap()));
}

#[async_test]
#[cfg(feature = "experimental-sliding-sync")]
async fn test_discovery_well_known_with_simplified_sliding_sync() {
// Given a base server with a well-known file that points to a homeserver with a
// sliding sync proxy.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
.respond_with(ResponseTemplate::new(200).set_body_json(make_well_known_json(
&homeserver.uri(),
Some("https://localhost:1234"),
)))
.mount(&server)
.await;

// When building a client for simplified sliding sync with the base server.
builder = builder.simplified_sliding_sync(true);
builder = builder.server_name_or_homeserver_url(server.uri());
let client = builder.build().await.unwrap();

// Then a client should not use the discovered sliding sync proxy.
assert!(client.sliding_sync_proxy().is_none());
}

/* Requires sliding sync */

#[async_test]
Expand All @@ -1070,7 +1103,7 @@ pub(crate) mod tests {
// doesn't support sliding sync.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
Expand All @@ -1096,7 +1129,7 @@ pub(crate) mod tests {
// sliding sync proxy.
let server = MockServer::start().await;
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();

Mock::given(method("GET"))
.and(path("/.well-known/matrix/client"))
Expand All @@ -1121,7 +1154,7 @@ pub(crate) mod tests {
// Given a homeserver without a well-known file and with a custom sliding sync
// proxy injected.
let homeserver = make_mock_homeserver().await;
let mut builder = ClientBuilder::new();
let mut builder = make_non_sss_client_builder();
builder = builder.sliding_sync_proxy("https://localhost:1234");

// When building a client that requires sliding sync with the server's URL.
Expand Down Expand Up @@ -1174,4 +1207,18 @@ pub(crate) mod tests {
object
})
}

/// These tests were built with regular sliding sync in mind so until
/// we remove it and update the tests, this makes a builder with SSS
/// disabled.
fn make_non_sss_client_builder() -> ClientBuilder {
let mut builder = ClientBuilder::new();

#[cfg(feature = "experimental-sliding-sync")]
{
builder = builder.simplified_sliding_sync(false);
}

builder
}
}
Loading