client: Add SSO Login

[zecakeh]

The goal is to enable full SSO Login support from the client:

• Provide functions for the SSO process: get_login_types, get_sso_login_url and login_with_token
• Provide a login_with_sso function behind a feature flag that handles the whole process:
  • Spawn a local server
  • Open the SSO Login URL in a browser with the proper redirect URL
  • Wait for the loginToken
  • Logs in

These can be handled separately:

• Rename login to login_with_password?
• Replace login with a generic function that accepts any login::LoginInfo?

Fixes #173

[zecakeh]

I added the sso login functions and created an sso_login feature flag. I tested login_with_sso locally but I'm not sure it's possible to write tests because it tries to launch a browser.

Maybe we want to show a message on the webpage after the login is successful? That message would need to be customizable by the client app.

[poljar]

I added the sso login functions and created an sso_login feature flag. I tested login_with_sso locally but I'm not sure it's possible to write tests because it tries to launch a browser.

You may mock something up with mockito, e.g. instead of launching a browser to a normal request using another reqwest::Client instance? But yeah if it's too annoying to write we can merge this after live testing.

Maybe we want to show a message on the webpage after the login is successful? That message would need to be customizable by the client app.

That's a good idea, the Weechat helper shows a message as well.

[zecakeh]

You may mock something up with mockito, e.g. instead of launching a browser to a normal request using another reqwest::Client instance? But yeah if it's too annoying to write we can merge this after live testing.

I managed to make a test that still launches the browser but resolves right away without user action. It won't work in CI though in my opinion, and it's a bit annoying that it opens a page in the browser when you run tests. How could I prevent that? Should I add an optional parameter to login_with_sso that takes a callback with the sso_login_url as param? If the callback is not present it opens the browser, otherwise it calls the callback.

[poljar]

Very nice, the doc tests fail so we'll need to fix this. And there's a bit of discussion to be had about the concrete API we want to offer.

[poljar]

This example doesn't match reality. The tests fail for this, but CI passes since the feature is disabled by default, we should probably add another CI task that tests this feature.

[zecakeh]

Yeah, sorry about that. I forgot to update the example after implementing the function.

[zecakeh]

Should I take care of the new CI task here or in another PR? I'm also not sure what command to use because locally when I launch cargo test --features docs, it applies the regular test but not the docs test of the login_with_sso method.

[zecakeh]

This is the API I've come up with for now:

pub async fn login_with_sso(
    &self,
    server_url: Option<&str>,
    server_response: Option<&str>,
    use_sso_login_url: impl Fn(Uri) -> Result<()>,
    device_id: Option<&str>,
    initial_device_display_name: Option<&str>,
) -> Result<login::Response>

With the arguments description:

/// * `server_url` - The local URL the server is going to try to bind to, e.g.
///     `http://localhost:3030`. If `None`, the server will try to open a random
///     port on localhost.
///
/// * `server_response` - The text that will be shown on the webpage at the end
///     of the login process. This can be an HTML page. If `None`, a default
///     text will be displayed.
///
/// * `use_sso_login_url` - A callback that will receive the SSO Login URL. It
///     should usually be used to open it in a browser. Should return `Ok(())`
///     if the URL was succesfully opened. If it returns `Err`, it will be forwarded.
///
/// * `device_id` - A unique id that will be associated with this session. If
///     not given the homeserver will create one. Can be an existing device_id
///     from a previous login call. Note that this should be provided only
///     if the client also holds the encryption keys for this device.
///
/// * `initial_device_display_name` - A public display name that will be
///     associated with the device_id. Only necessary the first time you
///     login with this device_id. It can be changed later.

[poljar]

This is the API I've come up with for now:

pub async fn login_with_sso(
    &self,
    server_url: Option<&str>,
    server_response: Option<&str>,
    use_sso_login_url: impl Fn(Uri) -> Result<()>,
    device_id: Option<&str>,
    initial_device_display_name: Option<&str>,
) -> Result<login::Response>

With the arguments description:

/// * `server_url` - The local URL the server is going to try to bind to, e.g.
///     `http://localhost:3030`. If `None`, the server will try to open a random
///     port on localhost.
///
/// * `server_response` - The text that will be shown on the webpage at the end
///     of the login process. This can be an HTML page. If `None`, a default
///     text will be displayed.
///
/// * `use_sso_login_url` - A callback that will receive the SSO Login URL. It
///     should usually be used to open it in a browser. Should return `Ok(())`
///     if the URL was succesfully opened. If it returns `Err`, it will be forwarded.
///
/// * `device_id` - A unique id that will be associated with this session. If
///     not given the homeserver will create one. Can be an existing device_id
///     from a previous login call. Note that this should be provided only
///     if the client also holds the encryption keys for this device.
///
/// * `initial_device_display_name` - A public display name that will be
///     associated with the device_id. Only necessary the first time you
///     login with this device_id. It can be changed later.

Looks good, let's just move the methods that are inside an Option after the ones that aren't.

[zecakeh]

I rebased the branch on master and I applied the requested changes. It uses warp instead of tiny_http for the server now. It's a bit more complicated but it works asynchronously.

One thing I'm not certain about is that the error returned is mostly IoErrorKind::Other and it might be too limited if the client app wants to show custom messages.

[poljar]

Looks good, couple of nits still. By the way, has this been tested on a real server as well?

[zecakeh]

I applied the changes. Instead of using try_bind for the server, it binds first a listener to a tcp port and then the server connects to the stream of the listener.

By the way, has this been tested on a real server as well?

Yes, I've tested with a small CLI program with my Matrix account on tedomum.net.

[poljar]

Looking good, I think we can merge after the last couple of nits are fixed.

[zecakeh]

Changes applied

[poljar]

Thanks, merged.