Add a TokenSource crate to the Rust SDKs

[MaxHeimbrock]

Before you submit your PR

Make sure the following is true before submitting your PR:

• [ x ] I have read the contributing guidelines and validated that this PR will be accepted.
• [ x ] I have read and followed the principles regarding breaking changes, testing, and code quality.

PR description

This PR adds the Token Source concept as an independent crate.

Testing

Ideally, unit test the code you add, but ensure you're not repeating existing test cases. Use as many already written scaffolding, utilities as possible; write your own, when needed. If external services, APIs, tokens are required (e.g., running an LK server instance), provide the necessary information. Make sure your tests perform useful, context-aware assertions and do not simply emulate "happy paths".

Async

We want the project to be runtime-agnostic, so please reuse what's already in livekit-runtime and feel free to add anything missing. It's ok to use Tokio directly, when writing unit tests, if necessary. When testing, do not use artificial delays for the state to "catch up"; instead, respect the event flow and subscribe properly using channels or other mechanisms.

[ladvoc]

Left some initial comments, will return for a more thorough review when this is taken out of draft.

[1egoman]

I saw you opened this so I left some initial thoughts, feel free to ignore them for now if it's still early and not yet ready for a review.

[MaxHeimbrock]

Potential follow up: Mint tokens based on API secret

[MaxHeimbrock]

@ladvoc this thing is already minting tokens from api key and api secret right? Are we using this in the rust-dev-client?

[github-actions]

Changeset ✓

This PR includes a changeset covering all affected packages:

Package	Bump
livekit-token-source	minor

[1egoman]

Really nice work! I've left a bunch of small / nitpicky comments which I noticed when going through, but I think this generally makes sense to me.

One higher level / larger question: I see you decided to punt the TokenSourceMinter question to the future. Totally fine with me, but I am just raising that because I think @ladvoc might have a use case for it in livekit-capture. It might be worth chatting with him about it.

[1egoman]

thought: I think it would be a good idea to either make literal take two positional args:

let literal = TokenSource::literal("wss://example.livekit.cloud", "<a pre-generated token>");

Or add some sort of builder interface / explicit constructor to TokenSourceResponse:

TokenSourceResponse::new("wss://example.livekit.cloud", "<a pre-generated token>")

I bring this up because while I think it's unlikely we will add extra fields to TokenSourceResponse, as thsi works today if we did add more fields it would be a breaking change. Plus, it would allow you to drop the .to_string()s.

[MaxHeimbrock]

Good idea 👍

[1egoman]

nitpick: This could probably be ::default() (ie, make it derive(Default) and you get this for free), not ::new()? Or maybe there's a good reason you added a separate explicit constructor, not sure, but I think the rust defaults here would be fine for this?

[MaxHeimbrock]

I will look into this, but sounds good. I like reading it as default() instead of just new()

[1egoman]

question: Should this be a HashMap / something more key-value native since the order of headers shouldn't matter?

Also just an idea, I'm not sure if this is a good one though - maybe something like this could be an alternate way to design this api, and then the inner data structure becomes an implementation concern:

let endpoint = TokenSource::endpoint("https://cloud-api.livekit.io/api/v2/sandbox/connection-details")
    .with_header("X-Sandbox-ID", sandbox_id);

One advantage to this is you could use impl Into<String> in with_header which would allow you to pass in a &str, String, or anything else that is Into<String>.

(You could potentially also have a with_header / with_headers like with_participant_attribute / with_participant_attributes on TokenSourceFetchOptions)

[MaxHeimbrock]

You are right, I will revisit this to not be a vector.
The .with_header is also nice to read I think.

[ladvoc]

I might be a bit biased, but I am heavily against positional arguments in cases where it is not obvious what they represent at a glance (e.g., Vec2::new(1, 2) is obvious); I think .with_header is much cleaner here API wise.

[1egoman]

Ah, I see what you did here - I would be curious what other folks think, but IMO it's fairly typical in rust to call ::default() instead of ::new(), so I would suggest dropping this alias.

[ladvoc]

This is considered a best practice, but usually it's usually implemented the other way around (i.e., Default implementation calls new). See clippy::new_without_default for more info.

[ladvoc]

More context: another reason this is best practice is because it allows you to use standard library functions like .unwrap_or_default() which rely on a default implementation. Standard library types like String, Vec, etc. also do this.

[1egoman]

question: Are these aliases worth having here? It it's helpful context, the decision which was made when the token source stuff was being first conceptualized was that the wire format would always be in snake_case. Is there a case where you see needing to parse camelCase versions of these fields from within a rust context?

[MaxHeimbrock]

I am fine with dropping it again. Maybe a bit overengineered.

[ladvoc]

If this is necessary, Serde has a nice attribute for this that you can apply to the whole struct: #[serde(rename_all = "camelCase")]

[1egoman]

thought: This isn't in this code, but seeing this makes me realize we really should have a .ok() method on response which encapsulates this (200..300).contains(&response.status) check. Is that something you'd be willing to add to livekit-net? Or I could maybe do it as a follow up if you want.

[MaxHeimbrock]

Maybe you can draft it?

[1egoman]

I can do it as a follow up, np.

[1egoman]

These tests are excellent!!

[1egoman]

suggestion: Add a section here on the difference between TokenSourceConfigurable and TokenSourceFixed. Maybe you can steal some context from here: https://github.com/livekit/client-sdk-js/tree/main#generating-a-urltoken-with-tokensource

[1egoman]

suggestion: Consider adding an example of a custom token source in here just to be thorough.

[MaxHeimbrock]

The implementation does not have a custom token source yet. I tried to push back things that can be done later, but I also understand if this is supposed to be complete.

[1egoman]

To be clear, what I meant by "custom token source" is something like the below in the example:

struct MyTokenSource;

impl TokenSourceConfigurable for MyTokenSource {
    async fn fetch(
        &self,
        options: &TokenSourceFetchOptions,
    ) -> TokenSourceResult<TokenSourceResponse> {
        // Do something unusual in here, maybe like get tokens from like a file?
        // Or maybe like reading them from the keychain or something like that could be an interesting demo?
        Ok(TokenSourceResponse { /* fields here */ })
    }
}

// Then in main:
let custom = MyTokenSource {};
match custom.fetch(&options).await {
    Ok(response) => println!(
        "custom token source: server_url={} participant_token={}",
        response.server_url, response.participant_token
    ),
    Err(error) => eprintln!("custom token source fetch failed: {error}"),
}

[1egoman]

thought: In the web token source implementation, there is caching which is done to avoid re-fetching a new token when the previous token is valid. IMO, this is really useful, and can be a significant slowdown when a user is initially connecting to a room to not have this. It also allows for pre-emptive token fetching (either on app load, or after a disconnect to make repeated connect / disconnect cycles fast).

I know how exactly this caching is done differs between the implementations (Web does it within the TokenSourceEndpoint via an internal abstract class it extends, swift has a TokenSourceCached, etc) but IMO it would be worth coming up with a strategy of some sort on how to handle this (I don't have a strong preference how it should work).

t's somewhat arguable whether adding it would be a breaking change in the interface IMO, adding caching later on obviously wouldn't break the existing interface but also on the other hand, a user may build around assuming that a token will always be regenerated for each room connection and adding caching would break that assumption. That's why I would be in favor of adding this now, because then there's no risk of a user accidentally building around this assumption and making adding it later in a fully backwards compatible way challenging.

[MaxHeimbrock]

Thanks for your opinion, I wanted to implement it later. Let me see how I can add it now already though.

One thing I want to note is that for agent use cases, I have experienced caching causing issues. If you leave a room and there was an agent dispatched, calling again with caching active will put you in the same room. But because you left before, the agent is being torn down and is also not joining again. So for agent use cases, I think caching should be discouraged with our current logic on dispatch.

[ladvoc]

Looking clean overall! Just some minor comments and suggestions.

[ladvoc]

note: Probably outside the scope of this PR, but this looks like a generally useful testing utility that could be moved to the livekit-net crate and exposed conditionally under a mock feature.

[ladvoc]

I might be a bit biased, but I am heavily against positional arguments in cases where it is not obvious what they represent at a glance (e.g., Vec2::new(1, 2) is obvious); I think .with_header is much cleaner here API wise.

[ladvoc]

If this is necessary, Serde has a nice attribute for this that you can apply to the whole struct: #[serde(rename_all = "camelCase")]

[ladvoc]

suggestion: To stay in line with keeping these methods generic, consider allowing this to work with any type providing string key-value pairs:

pub fn with_participant_attributes(
    mut self,
    value: impl IntoIterator<
        Item = (impl Into<String>, impl Into<String>)
    >,
) -> Self {
    self.participant_attributes
        .get_or_insert_with(HashMap::new)
        .extend(value.into_iter().map(|(k, v)| (k.into(), v.into())));
    self
}

At the call site, the user has flexibility:

// 1. Array of key-value pairs
builder.with_participant_attributes([
    ("name", "alice"),
    ("role", "host"),
]);

// 2. Vec of key-value pairs
builder.with_participant_attributes(vec![
    ("name", "alice"),
    ("role", "host"),
]);

// 3. Hash map
let mut attrs = HashMap::new();
attrs.insert("name".to_string(), "alice".to_string());
attrs.insert("role".to_string(), "host".to_string());

builder.with_participant_attributes(attrs);

[ladvoc]

suggestion: Room config is a massive nested Protobuf (definition), and while it probably doesn't make sense to expose it here and introduce a dependency on livekit-protocol there should be a comment indicating it is non-exhaustive.

As an aside, we have this same situation in the access token module exposed over UniFFI and I opted to do the same thing there.

[ladvoc]

nitpick: Consider combining into a single use statement:

use crate::{
    request::{TokenSourceFetchOptions, ...},
    response::{TokenSourceResponse, ...},
    ...
};

[ladvoc]

@1egoman, I think there is some ongoing effort to transition to livekit.com from livekit.io. Do you know anything about this and whether we should be pointing to livekit.com for future compatibility?

[1egoman]

I don't, no. IMO though as part of that migration (assuming it is not just for marketing facing things) all existing api routes would need to be backwards compatible to not break old web apps so I think this is fine as is.

[ladvoc]

I am not opposed to leaving it like this, but I will note this factory pattern is not common in Rust. A more common pattern would be to expose these as freestanding helper functions, and the user would typically reference them with the module path instead of bringing them in with use so it is clear where they come from. Example of what this would look like for the user:

livekit_token_source::literal(...)

Coming from primarily OO languages, this might seem weird, but Rust doesn't have anything against freestanding functions in a public API. Tokio, for example, uses this pattern in several places: see mpsc.

[ladvoc]

Note: before publishing this crate, I need to setup trusted publishing to prevent a recurrence of last week's release issue; on my list, just haven't gotten around to it yet.