Add token source API for fetching LiveKit tokens#177
Conversation
xianshijing-lk
left a comment
There was a problem hiding this comment.
might need help to understand the TokenSource a bit more
9e60f34 to
1929f4a
Compare
Manual ad-hoc helper for exercising token sources locally; the sandbox ID must be supplied via LIVEKIT_SANDBOX_ID rather than hard-coded in source. Co-authored-by: Cursor <cursoragent@cursor.com>
763d27e to
90d262f
Compare
|
I am using the tokensource sandbox and it is working fine for me 👍 |
…/token_source_api
xianshijing-lk
left a comment
There was a problem hiding this comment.
lgtm with some comments, please address them.
Great work!!!
| @@ -0,0 +1,109 @@ | |||
| # Token lifecycle | |||
|
|
|||
| Succinct reference for how join credentials and in-session refresh interact in | |||
There was a problem hiding this comment.
@1egoman , could you please help review this token-lifecycle.md ?
There was a problem hiding this comment.
I actually generated this for him while I had some active LLM context around this topic, I removed it as I don't want this SDK to be an authoritative documentation source compared to docs.livekit.io.
I do think we should have some better docs around token source and lifetimes, while going through this process it felt a little disjointed and agents/frontend-focused and not just a general document for token source terms, functions, lifecycles, etc for client SDKs.
| #include "livekit/room_event_types.h" | ||
| #include "livekit/stats.h" | ||
| #include "livekit/subscription_thread_dispatcher.h" | ||
| #include "livekit/token_source.h" |
There was a problem hiding this comment.
nit, would you consider forward declare those types, and move the #include "livekit/token_source.h" to the cpp ?
One benefit for it is that it could speed up the build.
| /// @brief Base interface for token sources that provide full credentials directly. | ||
| class LIVEKIT_API TokenSourceFixed { | ||
| public: | ||
| virtual ~TokenSourceFixed(); |
There was a problem hiding this comment.
nit, just do "virtual ~TokenSourceFixed() = default" here and remove the cpp line
The same for virtual ~TokenSourceConfigurable() = default;
|
|
||
| /// @brief Error returned when token fetching fails. | ||
| struct TokenSourceError { | ||
| std::string message; |
There was a problem hiding this comment.
nit, do we also want the http status_code ?
There was a problem hiding this comment.
Reasonable idea, but I would advocate not doing this. This error applies to all token source types (including literal and custom, which have no external comms), so adding specific details to any one implementation wouldn't be clean in my view.
If we get a user request for HTTP details for some reason, we could extend or more generalize error specifics into this struct.
| return connect(details.value().server_url, details.value().participant_token, options); | ||
| } | ||
|
|
||
| bool Room::connect(TokenSourceConfigurable& token_source, const TokenRequestOptions& request_options, |
There was a problem hiding this comment.
consider a helper function like
namespace {
template <typename FetchFn>
bool connectWithTokenSource(Room& room, const RoomOptions& options, FetchFn&& fetch) {
Result<TokenSourceResponse, TokenSourceError> details =
Result<TokenSourceResponse, TokenSourceError>::failure(TokenSourceError{"token source not invoked"});
try {
details = fetch().get();
} catch (const std::exception& e) {
LK_LOG_ERROR("Room::connect failed: token source threw: {}", e.what());
return false;
} catch (...) {
LK_LOG_ERROR("Room::connect failed: token source threw unknown exception");
return false;
}
if (!details) {
LK_LOG_ERROR("Room::connect failed: token source error: {}", details.error().message);
return false;
}
const auto& value = details.value();
return room.connect(value.server_url, value.participant_token, options);
}
} // namespace
Then these two function can become:
bool Room::connect(TokenSourceFixed& token_source, const RoomOptions& options) {
return connectWithTokenSource(*this, options, [&] {
return token_source.fetch();
});
}
bool Room::connect(TokenSourceConfigurable& token_source,
const TokenRequestOptions& request_options,
const RoomOptions& options) {
return connectWithTokenSource(*this, options, [&] {
return token_source.fetch(request_options, false);
});
}
There was a problem hiding this comment.
FWIW, we opted not to do this in other sdks, and determined this was the session api's job. Relevant pull request: livekit/client-sdk-js#1677
| : provider_(std::move(provider)) {} | ||
|
|
||
| std::future<Result<TokenSourceResponse, TokenSourceError>> CustomTokenSource::fetch(const TokenRequestOptions& options, | ||
| bool /*force_refresh*/) { |
There was a problem hiding this comment.
any idea why CustomTokenSource will skip the force_refresh ?
There was a problem hiding this comment.
This comment actually led to an API design change, see here: e39e55e
Having such an argument for a custom source would require passing that argument along to the custom implementation, which felt weird as the custom token source was just a user function that could decide anything it needed to. I have instead dropped force_refresh entirely, and opted for caching_token_source.invalidate() which matches how Swift/Android do it.
In short, this original design around forcing refresh was modeled after JS with the force_refresh. But JS actually goes for an inheritance-based approach to cacheing token sources, vs. here which is using a decorator (Swift/Android do it this way), so forcing refresh was consistent across all sub-classes and didn't have an awkward drop like this did.
It was basically a divergence in both approaches (force + decorator vs. force + inheritance), now it's more aligned to Swift/Android.
| return TokenSourceResult::success(*cached_details_); | ||
| } | ||
|
|
||
| auto result = inner_->fetch(*options_snapshot, force_refresh).get(); |
There was a problem hiding this comment.
any reason why we want to run the inner_->fetch() under the lock ?
Will it be safer to do ?
{
std::scoped_lock lock(mutex_);
if (!force_refresh && cached_details_ && cached_options_ &&
tokenRequestOptionsEqual(*cached_options_, *options_snapshot) &&
isParticipantTokenValid(cached_details_->participant_token)) {
return TokenSourceResult::success(*cached_details_);
}
}
auto result = inner_->fetch(*options_snapshot, force_refresh).get();
There was a problem hiding this comment.
Holding the mutex over the fetch is how JS and Swift do it, I'm open to your change but it would be a divergence
| auto source = std::unique_ptr<SandboxTokenSource>(new SandboxTokenSource(sandbox_id, options, base_url)); | ||
| auto resolved = resolveSandboxEndpoint(sandbox_id, std::move(options), base_url); | ||
| source->endpoint_ = | ||
| EndpointTokenSourceTestAccess::create(std::move(resolved.url), std::move(resolved.options), std::move(transport)); |
There was a problem hiding this comment.
isn't the endpoint_ created in line 210 already ? any reason why it needs to be re-created ?
There was a problem hiding this comment.
Great catch, this slipped through. Now have a private constructor that accepts an already made endpoint so both the public and test access versions can avoid recreation
| } | ||
|
|
||
| const std::wstring host(components.lpszHostName, components.dwHostNameLength); | ||
| const std::wstring path(components.lpszUrlPath, components.dwUrlPathLength); |
There was a problem hiding this comment.
question, does path need to include lpszExtraInfo ? like when lpszExtraInfoLength > 0 ?
| } | ||
|
|
||
| const int timeout_ms = static_cast<int>(timeout.count()); | ||
| WinHttpSetTimeouts(session, timeout_ms, timeout_ms, timeout_ms, timeout_ms); |
There was a problem hiding this comment.
what will happen if timeout_ms is 0 or not valid ?
There was a problem hiding this comment.
Good call, <= 0 now goes to the default of 30 seconds
…/token_source_api
1egoman
left a comment
There was a problem hiding this comment.
Generally looks good to me, and I think matches the interfaces on other sdks pretty well!
| /// LiveKit Cloud deployment to target for agent dispatch. | ||
| /// | ||
| /// Optional. When omitted or empty, the production deployment is used. | ||
| /// Only relevant when dispatching a named agent on LiveKit Cloud. | ||
| std::optional<std::string> agent_deployment; |
There was a problem hiding this comment.
nitpick: in other sdks (for example, web) this was just called deployment. I like this agentDeployment name better though and had brought it up during the review process when it was added recently but I think it got missed. It might be worth changing this for consistency, idk.
There was a problem hiding this comment.
Android and Swift (which this largely models) both use agentDeployment. So I think it's a 50/50 consistency roll of the dice 🤔
| /// @brief Create a token source from an async provider that returns full credentials. | ||
| /// | ||
| /// Use this overload when credentials are produced outside the SDK but fetched | ||
| /// lazily (for example, from your own cache or secure storage). |
There was a problem hiding this comment.
nitpick: It might be worth re-emphasizing in the docs here (assuming this works the same as the web and other implementations) that this doesn't support parameters and that if you want a configurable version of this which can re-fetch multiple times on demand to use CustomTokenSource
| bool tokenRequestOptionsEqual(const TokenRequestOptions& a, const TokenRequestOptions& b) { | ||
| return a.room_name == b.room_name && a.participant_name == b.participant_name && | ||
| a.participant_identity == b.participant_identity && a.participant_metadata == b.participant_metadata && | ||
| a.participant_attributes == b.participant_attributes && a.agent_name == b.agent_name && | ||
| a.agent_metadata == b.agent_metadata && a.agent_deployment == b.agent_deployment; | ||
| } |
There was a problem hiding this comment.
nitpick: I'm not sure if there's a way to handle this in c++, but ideally if possible it would be great if the compiler could somehow throw an error if a field in a or b were missed in this check.
Also, do you have to worry about deep equality here within maps like participant_attributes? Or is == on the fields directly good enough?
| /// @brief Decorator that adds JWT-aware caching to another configurable token source. | ||
| /// | ||
| /// Wrap @ref CustomTokenSource, @ref EndpointTokenSource, or | ||
| /// @ref SandboxTokenSource to reduce token fetch calls while still refreshing | ||
| /// when tokens expire or when @p force_refresh is requested. | ||
| class LIVEKIT_API CachingTokenSource final : public TokenSourceConfigurable { |
There was a problem hiding this comment.
thought: You probably have seen it / maybe even patterned it off of this, but your implementation here is fairly close to the swift version: https://github.com/livekit/client-sdk-swift/blob/5d13e9ca7b366a8b47bd08c95a34cb0068149287/Sources/LiveKit/Token/CachingTokenSource.swift#L23.
Also just want to confirm - looking at the cpp I think this is just memoization with the last value correct? There's no other way to configure this right? I think that fine.
Summary
token_source.h, allowing users to now mint tokens using the following paths:livekit-climinted path)room.h-> new ways to connect with tokensroom_event_types.h-> new information-only event type when tokens are refreshed by the serverroom_delegate.h-> new callback with the new eventNew Dependencies
Testing
Validated through a combination of unit, integration, and standalone binary tester program against a LiveKit Cloud agent with a live token server.
Sandbox Token
Connected to a LiveKit cloud instance. The
Participant Namefield is randomized on program re-run which makes sense.The token is obtained via the following new API:
And results printed below: