Make client use .well-known redirects #233

timorl · 2021-05-13T11:27:27Z

Was supposed to fix #219, but apparently that was about something else.

poljar · 2021-05-13T11:49:47Z

matrix_sdk/src/client.rs

    ///
    /// # Arguments
    ///
    /// * `homeserver_url` - The homeserver that the client should connect to.
-    pub fn new(homeserver_url: Url) -> Result<Self> {
+    pub async fn new(homeserver_url: Url) -> Result<Self> {


I don't think this is what was suggested in #219:

As for doing the lookup before one is logged in, or in other words creating a Client object with only a user_id a separate async constructor that does a well-known lookup should work, no?

Please take a look at the spec as well: https://spec.matrix.org/unstable/client-server-api/#well-known-uri

This describes how and when well-known discovery should work:

Extract the server name from the user's Matrix ID by splitting the Matrix ID at the first colon.

Extract the hostname from the server name.

Make a GET request to https://hostname/.well-known/matrix/client.

So we'll want a new constructor pair:

pub async fn new_with_discovery(user_id: UserId) -> Result<Self> { // Do the well-known request here // If it succeeds try to query the server version to check if it's a // valid homeserver }

If you don't like that name, perhaps pub async fn new_from_user_id(user_id: UserId) is better.

Yup, makes sense, I'll do that. As a bonus, the PR should get much smaller I guess.

Should be done now, although unfortunately I have no idea how to test it – the mockito server address does not allow me to create a valid user id. :/ I guess some very fancy proxying setup in the tests could fix this, but that seems like lots of work for little gain. If it's any consolation, I manually checked that it works on my homeserver.

Create a valid user id how/where?

I wanted to create a unit test to check whether this correctly queries and interprets the contents of $SERVER/.well-known/matrix/client. The constructor takes SERVER from the second part of the UserId, so creating a UserId with the mockito address as the server should work in principle. Unfortunately that adress is invalid for the purpose of creating UserIds.

How did you try to construct the user id exactly?

This seems to work

let server_url = mockito::server_url(); let server_url = server_url.strip_prefix("http://").unwrap(); let user = UserId::try_from(format!("@example:{}", server_url,)).unwrap();

Ouch, I am just an idiot, I forgot to remove the prefix. >.<

Should be OK and properly unit tested now.

poljar

This looks mostly ok, though it doesn't implement the algorithm described in the spec. We should adhere to what's in the spec.

poljar · 2021-05-14T07:06:02Z

matrix_sdk/src/client.rs

+        user_id: UserId,
+        config: ClientConfig,
+    ) -> Result<Self> {
+        let homeserver_url = "http://".to_string() + user_id.server_name().as_str();


Can't we build the URL in a more pleasant way?

Not much more pleasant – url exposes a Host type with parsing support, but the type cannot be then used to construct a Url. I changed this to a format! call and moved to a separate function though.

poljar · 2021-05-14T07:06:51Z

matrix_sdk/src/client.rs

+        config: ClientConfig,
+    ) -> Result<Self> {
+        let homeserver_url = "http://".to_string() + user_id.server_name().as_str();
+        let homeserver_url = Url::parse(homeserver_url.as_str()).unwrap();


This unwrap needs to be gone, for now you can add the Url::ParseError to our Error type, in the long run we would like to have some specialized error type for this constructor.

poljar · 2021-05-14T07:40:11Z

matrix_sdk/src/http_client.rs

+        None
+    }
+
+    async fn with_well_known(mut self) -> Self {


I wonder if we can come up with a better name for this. Perhaps into_discovered_homeserver()?

I changed the design quite a bit anyway, take a look at the new names/abstractions to see if you like them.

poljar · 2021-05-14T07:41:14Z

matrix_sdk_test/src/test_json/mod.rs

@@ -42,3 +42,11 @@ lazy_static! {
        ]
    });
 }
+
+lazy_static! {


Let's move this into the test. It's small enough and only used in that single test. That way people don't need to chase around the test data if something breaks.

Tests are blocked by lipanski/mockito#127 anyway. :/

Can't we build a http URL under cfg(test) and build a https URL when testing is not enabled? That way we can still test most of the logc, no?

poljar · 2021-05-14T07:47:16Z

matrix_sdk/src/client.rs

@@ -2425,6 +2481,28 @@ mod test {
        client
    }

+    #[tokio::test]
+    async fn well_known() {
+        use std::convert::TryFrom;


Let's move those imports out of the function, they'll likely be useful in other tests and at that point we'll have the same import in a bunch of places.

The unit tests are removed completely anyway, lipanski/mockito#127.

poljar · 2021-05-14T07:49:28Z

matrix_sdk/src/client.rs

+        })
+    }
+
+    /// Creates a new client for making HTTP requests to the homeserver of the


This should probably mention how it differs from the new() constructor. Explain which steps it takes to discover the homeserver and link to the relevant spec entry, an example wouldn't hurt either.

poljar · 2021-05-14T08:00:31Z

matrix_sdk/src/http_client.rs

+        HttpClient { inner, homeserver, session, request_config }
+    }
+
+    pub(crate) async fn new_with_well_known(


This doesn't really do what's described in the spec, as far as I can tell it uses well-known if it can find it, otherwise it continues to use the domain from the UserId.

It should only continue to use the domain from the UserId if the well-known request returns a 404 error, otherwise it should throw an error, take a look at all the different error cases listed here.

Also please take a look how it should validate that the now modified Url is a valid homeserver using the /versions endpoint over here, step 5, substep 2.

Ohh, there is a formal spec, how cool!

After reading the spec I don't think it should ever use the domain from the UserId – at best it can fall back to "other discovery mechanisms", but this is an empty set. This is a bit of a moot point too, since at least synapse automatically provides a .well-known... endpoint with a self-reference.

Validation added.

I also got inspired by the spec to improve the design a bit in general. All the work actually happens in Client now and there is a method to change the homeserver url in there – private for now, but if I understand correctly it might be useful for the other well-known case you mentioned (the one #219 originally referred to?).

poljar

This looks great, let's just get rid of the mutable borrow and please add the test back.

poljar · 2021-05-19T08:10:41Z

matrix_sdk/src/client.rs

+    }
+
+    fn set_homeserver(&mut self, homeserver_url: Url) {
+        self.homeserver = Arc::new(homeserver_url);


I think we should put self.homeserver behind a RwLock to get rid of the mutable borrow here. Handling well-known in the login response will require this as well.

Done, although this made a couple functions async by necessity and Debug for Client got much less useful.

timorl · 2021-05-23T13:54:54Z

Still trying to figure out how much I can test, I cannot test the whole thing because Mockito does not support https endpoints and the specification requires that the inferred homeserver is queried that way.

timorl · 2021-05-23T14:21:57Z

Added a test at least checking that setting a specified homeserver works.

poljar · 2021-05-24T07:20:17Z

Still trying to figure out how much I can test, I cannot test the whole thing because Mockito does not support https endpoints and the specification requires that the inferred homeserver is queried that way.

I understand that we need to build a https URL per spec, but we can cheat a bit and build a http URL under testing and that way reintroduce the mockito based test like was suggested over here:

Can't we build a http URL under cfg(test) and build a https URL when testing is not enabled? That way we can still test most of the logic, no?

The homeserver_from_user_id() method could even take an argument that specifies if it should build a http or https URL, that way we can test both things, that the URL is correctly build and that the correct requests are sent out.

Was supposed to fix matrix-org#219, but apparently that was about something else.

timorl · 2021-05-24T09:07:37Z

Ah, sorry, I completely missed that comment. Code changes for tests are absolutely disgusting, but I guess a lack of tests is marginally worse, so changed as suggested. Although I tried to keep the difference minimal, especially since I don't like changing the function signatures for tests.

poljar · 2021-05-24T10:48:04Z

Thanks for patiently working on this, merged.

timorl force-pushed the famous branch 2 times, most recently from 8293ca4 to 93ddfa1 Compare May 13, 2021 11:37

poljar requested changes May 13, 2021

View reviewed changes

timorl force-pushed the famous branch 6 times, most recently from 833b8c5 to b3b316f Compare May 13, 2021 15:22

poljar requested changes May 14, 2021

View reviewed changes

johannescpk mentioned this pull request May 15, 2021

Application Service support tracking #228

Closed

40 tasks

timorl force-pushed the famous branch from b3b316f to 0bda029 Compare May 16, 2021 17:08

poljar requested changes May 19, 2021

View reviewed changes

timorl force-pushed the famous branch from 0bda029 to 45b3b99 Compare May 23, 2021 13:50

timorl force-pushed the famous branch 2 times, most recently from 0eb63f6 to b5856d0 Compare May 23, 2021 14:19

Make client use .well-known redirects

ded5830

Was supposed to fix matrix-org#219, but apparently that was about something else.

timorl force-pushed the famous branch from b5856d0 to 1499fb5 Compare May 24, 2021 09:04

timorl force-pushed the famous branch from 1499fb5 to ded5830 Compare May 24, 2021 09:11

poljar merged commit 7a5daf6 into matrix-org:master May 24, 2021

zecakeh mentioned this pull request Nov 2, 2021

Use well_known infromation from login response #219

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make client use .well-known redirects #233

Make client use .well-known redirects #233

timorl commented May 13, 2021

poljar May 13, 2021

timorl May 13, 2021

timorl May 13, 2021

poljar May 13, 2021

timorl May 13, 2021

poljar May 13, 2021

timorl May 13, 2021

poljar left a comment

poljar May 14, 2021

timorl May 16, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar May 19, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar May 14, 2021

timorl May 16, 2021

poljar left a comment

poljar May 19, 2021

timorl May 23, 2021

timorl commented May 23, 2021

timorl commented May 23, 2021

poljar commented May 24, 2021

timorl commented May 24, 2021

poljar commented May 24, 2021

Make client use .well-known redirects #233

Make client use .well-known redirects #233

Conversation

timorl commented May 13, 2021

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

poljar 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

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

poljar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timorl commented May 23, 2021

timorl commented May 23, 2021

poljar commented May 24, 2021

timorl commented May 24, 2021

poljar commented May 24, 2021