search

matrix-org / matrix-rust-sdk

Matrix Client-Server SDK for Rust
Apache License 2.0
1.19k stars 235 forks source link

examples usability #98

Closed martinetd closed 3 years ago

martinetd commented 3 years ago

Describe the bug I understand the examples aren't meant to be useable as is, but it's a starting point for someone taking a look at the project (hi! Please just tell me to GTFO if you can't be bothered or it's too early or whatever, it's much better than no answer -- and sorry it's a bit messy I just spent a couple of hours playing with this all)

I guess that's some new element feature that would need to be implemented in matrix_sdk_crypto/src/verification/? If I don't try to verify immediately but try to verify later by explicitely picking emoji I can get it to work but that was also a bit surprising

Also, doing all this (always reusing the same config dir + same device id and doing emoji verification once), I can't get the command_bot to see any encrypted message (but it works fine in an unencrypted room) -- I guess it should also listen to EncryptedEventContent? I don't find much about that, would have expected things to be transparently decrypted if keys etc are setup for the matrix-sdk.

poljar commented 3 years ago

Describe the bug I understand the examples aren't meant to be useable as is, but it's a starting point for someone taking a look at the project (hi! Please just tell me to GTFO if you can't be bothered or it's too early or whatever, it's much better than no answer -- and sorry it's a bit messy I just spent a couple of hours playing with this all)

No, this report is very useful, thank you for it. It's quite hard to not have blind spots in the docs since I know the in and outs of the sdk. Please feel free to open any issue especially if the docs need improvement.

there is a hardcoded proxy to localhost:8080 in most examples ; and the error isn't exactly obvious:

Those are mostly left there since I tend to use mitmproxy to double check that the sdk works as intended and to debug if something doesn't work correctly.

It would make sense to write something about the proxy there if possible?

I think we should just move the proxy setting into the CLI args, will open an issue.

(Note: is Reqwest, with a w, on purpose? I just noticed as I am filling this; that is sneaky...)

It's the error type of the Reqwest HTTP library, so yeah, that's mostly out of our hands unless we want to customize the error type for this.

I guess that's some new element feature that would need to be implemented in matrix_sdk_crypto/src/verification/? If I don't try to verify immediately but try to verify later by explicitely picking emoji I can get it to work but that was also a bit surprising

The QR code events and new in-room verification flow isn't yet supported, the example also skips proper error handling. I think we should leave this as is since we will, hopefully at least, add support for the other verification types soon. There are a couple of problems we need to solve in Ruma before we can start adding support in the rust-sdk for this.

I guess that doubles with #16 but while I see that e.g. the autojoin example creates a config directory that will store encryption keys (I think?) I think most people implementing clients/bots would prefer to see a persistent session too so having an example that also stores the access_token/device_id might make sense if possible at all?

Do you think that all examples should have persistent storage set up? It's pretty simple, just set a store path. Perhaps this should be moved into the command line args, if a directory for the storage is provided use the dir, otherwise don't persist anything.

I'm somewhat new to rust, but from what I've looked at it's easy to replace the login() call by restore_login() if you have access token, device id and user id -- but with base_client marked as private I don't see any way of accessing the first two? There's an user_id() wrapper but nothing for the other two.

Ah good to know, this needs to be exposed, though you can access them in a login response, presumably you'll login at least once. Looking at the docs of the login method maybe we could make this a bit clearer in an example.

Also, doing all this (always reusing the same config dir + same device id and doing emoji verification once), I can't get the command_bot to see any encrypted message (but it works fine in an unencrypted room) -- I guess it should also listen to EncryptedEventContent? I don't find much about that, would have expected things to be transparently decrypted if keys etc are setup for the matrix-sdk.

An encrypted event will only be returned if decryption fails for some reason, though it's unclear why it fails. Setting RUST_LOG=matrix_sdk_crypto=trace will enable the logging for the crypto side of things.

are there any known real users of the library yet? I might have been a little bit too eager :)

There are, though we will have breaking changes in our state storage and event emitting layers, the sdk seems to be pretty usable already.

martinetd commented 3 years ago

Thanks for the answers.

I think we should just move the proxy setting into the CLI args, will open an issue.

Sure, I guess each application can handle errors better than examples anyway; it was just surprising to see 'connection refused' for my homeserver when it was accessible ;)

Do you think that all examples should have persistent storage set up? It's pretty simple, just set a store path. Perhaps this should be moved into the command line args, if a directory for the storage is provided use the dir, otherwise don't persist anything.

I don't think that is necessary - a few of the examples have it set so it was easy enough to set it for others and share the same store path to try to see if emoji validation helped. Having some common cli parsing to all examples which would include this could make sense, but I wouldn't say it's a priority

[access token and device id] Ah good to know, this needs to be exposed, though you can access them in a login response, presumably you'll login at least once. Looking at the docs of the login method maybe we could make this a bit clearer in an example.

Ah! Why didn't I think of that, great; that works.

An encrypted event will only be returned if decryption fails for some reason, though it's unclear why it fails. Setting RUST_LOG=matrix_sdk_crypto=trace will enable the logging for the crypto side of things.

I need to learn how to use these loggers, it talks so there is something happening. I'll try to figure this out and will bug you again if I can't.

There are, though we will have breaking changes in our state storage and event emitting layers, the sdk seems to be pretty usable already.

Plenty more examples for me to look at then, I'm a good monkey :)

martinetd commented 3 years ago

An encrypted event will only be returned if decryption fails for some reason, though it's unclear why it fails. Setting RUST_LOG=matrix_sdk_crypto=trace will enable the logging for the crypto side of things.

I need to learn how to use these loggers, it talks so there is something happening. I'll try to figure this out and will bug you again if I can't.

I must be doing something wrong with the state directory -- things work on first run with a new directory (with "Creating a new account" trace). Running a second time I don't get any debug message from src/machine.rs's new_with_store; and a third time gives "matrix_sdk_crypto::machine: Restored account" but either way devices/keys are missing at this point.

I'll take some time to double-check my code first, it's a bit kludgy atm but will open a separate issue after cleaning up and a more minimal reproducer.

As far as I'm concerned everything got answered here -- letting you close.

poljar commented 3 years ago

It's completely possible that something got broken along the way, though I didn't see such behavior. Will check one of the examples as well.

martinetd commented 3 years ago

Well, this was some copy-paste "mistake" alright -- I was still using two examples and somehow one was using ClientConfig::new().store_path(home) while the other let store = JsonStore::open(&home)?; ClientConfig::new().state_store(Box::new(store))

Using store_path in both made this work as expected; I still can receive/send encrypted messages after restoring the session now.

Looking at the code I honestly don't see why they're not equivalent, setting store_path means the state_store is opened just the same way in matrix_sdk_base/src/client.rs, but as a user if I stick to either method of setting the store it should work...

MTRNord commented 3 years ago

Linking in -> Exposing everything to create a restore session would still be nice :)

Also Dont look too much at Daydream. Because of wasm specifics it isnt pretty or good code. It works but it shouldnt be used elsewhere. Daydream is actually being rebuilt better using druid over at https://github.com/daydream-mx/daydream-druid wich has slightly better usage of the sdk than the original daydream web version

martinetd commented 3 years ago

Linking in -> Exposing everything to create a restore session would still be nice :)

I couldn't be bothered to write serialization for access_token etc but in a single program it's really as simple as using one form of store_path or equivalent and restore_login.

Here's a program that logs in, syncs for a while (couldn't come up with anything better than timeouting the sync_forever helper?), then starts over with token it got the first time ; or if you pass 4 arguments tries to restart from whatever was given. Mostly based on examples/login.rs

#![type_length_limit="6000000"]

use std::{env, process::exit};
use url::Url;
use std::time::Duration;

use matrix_sdk::{
    self,
    events::{
        room::message::{MessageEventContent, TextMessageEventContent},
        SyncMessageEvent,
    },
    Client, ClientConfig, EventEmitter, SyncRoom, SyncSettings, Session
};
use matrix_sdk_common_macros::async_trait;
use matrix_sdk_common::identifiers::{DeviceId, UserId};
use std::convert::TryFrom;

struct EventCallback;

#[async_trait]
impl EventEmitter for EventCallback {
    async fn on_room_message(&self, room: SyncRoom, event: &SyncMessageEvent<MessageEventContent>) {
        if let SyncRoom::Joined(room) = room {
            if let SyncMessageEvent {
                content: MessageEventContent::Text(TextMessageEventContent { body: msg_body, .. }),
                sender,
                ..
            } = event
            {
                let (user_name, room_name) = {
                    // any reads should be held for the shortest time possible to
                    // avoid dead locks
                    let room = room.read().await;
                    let member = room.joined_members.get(&sender).unwrap();
                    (member.name(), room.display_name())
                };
                println!("{}: <{}> {}", room_name, user_name, msg_body);
            }
        }
    }
}

async fn login(
    homeserver: String,
    username: &str,
    password: &str,
) -> Result<(), matrix_sdk::Error> {
    let mut store_path = dirs::home_dir().expect("no home directory found");
    store_path.push("relogging_test");

    let client_config = ClientConfig::new().store_path(store_path);
    let homeserver_url = Url::parse(&homeserver).expect("Couldn't parse the homeserver URL");
    let mut client = Client::new_with_config(homeserver_url, client_config).unwrap();

    client.add_event_emitter(Box::new(EventCallback)).await;

    let login = match client
        .login(username, password, None, Some("rust-sdk"))
        .await {
            Ok(login) => login,
            Err(e) => return Err(e),
        };
    println!("logged in as {}: restore with: {} {} {}", username, login.user_id, login.access_token, login.device_id);

    let () = match tokio::time::timeout(Duration::from_millis(20000), client.sync_forever(SyncSettings::new(), |_| async {})).await {
        _ => (),
    };

    println!("got out, relogging");
    login_relogin(homeserver, login.access_token, login.user_id, login.device_id).await
}

async fn login_relogin(
    homeserver: String,
    access_token: String,
    user_id: UserId,
    device_id: Box<DeviceId>,
) -> Result<(), matrix_sdk::Error> {
    let mut store_path = dirs::home_dir().expect("no home directory found");
    store_path.push("relogging_test");

    let client_config = ClientConfig::new().store_path(store_path);
    let homeserver_url = Url::parse(&homeserver).expect("Couldn't parse the homeserver URL");
    let mut client = Client::new_with_config(homeserver_url, client_config).unwrap();

    client.add_event_emitter(Box::new(EventCallback)).await;

    let session = Session {
        access_token: access_token.clone(),
        user_id: user_id.clone(),
        device_id: device_id.clone(),
    };
    client.restore_login(session).await?;

    println!("restored session for user_id {}", user_id);

    client.sync_forever(SyncSettings::new(), |_| async {}).await;
    Ok(())
}

#[tokio::main]
async fn main() -> Result<(), matrix_sdk::Error> {
    tracing_subscriber::fmt::init();

    let (homeserver_url, username, password, device_id) =
        match (env::args().nth(1), env::args().nth(2), env::args().nth(3), env::args().nth(4)) {
            (Some(a), Some(b), Some(c), Some(d)) => (a, b, c, Some(d)),
            (Some(a), Some(b), Some(c), None) => (a, b, c, None),
            _ => {
                eprintln!(
                    "Usage: {} <homeserver_url> [ <username> <password> | <user_id> <access_token> <device_id> ]",
                    env::args().next().unwrap(),
                );
                exit(1)
            }
        };
    if let Some(device_id) = device_id {
      let user_id = UserId::try_from(username).unwrap();
      login_relogin(homeserver_url, password, user_id, device_id.into()).await
    } else {
      login(homeserver_url, &username, &password).await
    }
}
martinetd commented 3 years ago

While I have this issue to myself, some more feedback/questions now I've used the lib a bit more:

FWIW my toy program so far is https://github.com/martinetd/matrirc (horrible name for horrible implementation, as written in the readme this was more about me learning about rust than doing something useful -- sorry for you folks ;D -- but lack of encryption support in purple-matrix or matrix-ircd has been annoying me)

Cheers!

poljar commented 3 years ago

I assume it's meant to evolve and more emitter callbacks will be added as they are deemed useful but at this point I'm leaning towards handling all events manually.

As of now the event emitter has grown a bit big for my liking, that's why I haven't added any verification callbacks to it. We could add a to-device callback which would cover all of those events but I really would like to have some finer grained callbacks.

My plan is to somehow split out the event emitter into smaller subtraits possibly using traitcast. The subtraits would then have some logical grouping, e.g. implement this event emitter subtrait to handle verification events, another one for membership events. But I haven't gotten around to that yet.

Handling the initial sync separately is also easier going the manual event-parsing way (e.g. I need initial sync to populate room list but I want to iterate over each joined rooms once before handling join events, which would be easy enough if I just attach emitter later, but then I lose the backbuffer initial sync gets me)

A common technique for bots to ignore stale messages is to sync once, attach the emitter and only then enter the sync_forever loop. I don't think there's anything wrong with that but depending on you needs you may want to do things differently.

while on events, that's probably more a problem on my end but I'm never sure where to look at to find appropriate types e.g. Emote(EmoteMessageEventContent) or whatever. debug printing has helped but it's a bit cumbersome, and I'm missing some e.g. I don't see any event corresponding to m.reaction type messages that I saw then dumping json?

Ruma documents all those events that are supported by it, the rest gets thrown in the Custom variant.

So CustomEventContent,s will need parsing event_type manually?

Yeah, reactions aren't yet in the spec and there's a bit of a dissonance how they are implemented, the Ruma issue for this is over here https://github.com/ruma/ruma/issues/248.

I'd like more control over sync_forever -- in particular a way to stop. Currently sync_forever does sync but it also does a bit more (outgoing encryption stuff?), so I'm not sure if it's safe to just loop over sync with a delay like sync_forever does.

It is not, unless you replicate what sync_forever() does, I don't think we expose everything it does publicly, not sure if we should support this or just make sync_forever() better.

If you're open to breakage, how about making the sync_forever callback return a bool continue/stop and break out if it asked to stop?

That sounds like a good idea, cancelling a task may be annoying, e.g. I destroy the whole runtime because Tokio tasks don't let you cancel them without wrapping them somehow. This would avoid the need for cancellable tasks.

FWIW my toy program so far is https://github.com/martinetd/matrirc (horrible name for horrible implementation, as written in the readme this was more about me learning about rust than doing something useful -- sorry for you folks ;D -- but lack of encryption support in purple-matrix or matrix-ircd has been annoying me)

Ideally one would be able to take the crypto part of the SDK and embed it into any codebase with a bit of effort, I am planning on bringing it into a Python codebase over here. Hopefully other projects will find this useful as well.

Cheers!

Again, thanks for the detailed reports and for playing with this.

martinetd commented 3 years ago

My plan is to somehow split out the event emitter into smaller subtraits possibly using traitcast. The subtraits would then have some logical grouping, e.g. implement this event emitter subtrait to handle verification events, another one for membership events. But I haven't gotten around to that yet.

That makes sense, I agree huge functions are bad practice in general so finer grained handlers would probably make sense. If it's just splitting though I don't see the harm in adding verification callbacks now, but it's definitely not a priority for me at this point as blind manual verification from element works.

A common technique for bots to ignore stale messages is to sync once, attach the emitter and only then enter the sync_forever loop. I don't think there's anything wrong with that but depending on you needs you may want to do things differently.

Right, I hinted to that. In my case, my old bitlbee+purple-matrix setup used to post timestamped initial sync messages on reconnect and I kind of liked that so it's easier to replicate by storing the initial sync response, doing my init, then parsing the stored response. In a better world I could dynamically properly init each room when an event for it pops up, and this dance wouldn't be needed so the emitter could be attached from the start... Work in progress :)

Yeah, reactions aren't yet in the spec and there's a bit of a dissonance how they are implemented, the Ruma issue for this is over here ruma/ruma#248.

That makes sense, thanks for the link!

It is not, unless you replicate what sync_forever() does, I don't think we expose everything it does publicly, not sure if we should support this or just make sync_forever() better.

if the sync_forever() callback allows stopping as suggested I see no reason to expose that - if someone wants to do that they could just always have the callback ask to stop immediately.

Ideally one would be able to take the crypto part of the SDK and embed it into any codebase with a bit of effort, I am planning on bringing it into a Python codebase over here. Hopefully other projects will find this useful as well.

Yeah I had noticed that in the way crates are split, but unfortunately human nature makes it much easier/more appealing to start over than try to integrate things properly -- it's not helping that most projects without E2EE support at this point are usually not very actively maintained :/ So soon there'll be one more unmaintained project to the growing list, but at least it'll have E2EE :D

Black616Angel commented 3 years ago

I am just here to say, that I indeed as well fell into the trap, that ist this proxy. I debugged through matri-rust-sdk, ruma, reqwest and hyper just to be looking at a localhost:8080 in the end... it took me days, to do this, since i am not that used to programmin in rust.

This is more of a bump than anything, but please take these proxies out.

MTRNord commented 3 years ago

Had the same issue actually and we relised it on deployment in famedly ^^ Locally it all worked as obviously my proxy server always ran but everywhere else it did not.

If taking it out is considered bad maybe a "if set" check might be nice. So adding it conditionally if there is a env var or argument providing the proxy address

poljar commented 3 years ago

I pushed a patch to the new-state-store branch removing the usage of the proxy: https://github.com/matrix-org/matrix-rust-sdk/commit/9cd217fc5d8199c1d19f9c8ea3d95bfa39028e67

I'll bring proxy support back via a command line switch later on.

martinetd commented 3 years ago

(Closing as I don't see anything left that is really actable on, thanks for removing the proxy and the many answers)