-
Notifications
You must be signed in to change notification settings - Fork 148
Token source #787
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Token source #787
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't pushed the respective proto, it does not provide any kind of "validation" for the payload keys, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me! I'm not going to be able to comment on some of the swift specific things being done, hopefully others can weigh in on that stuff.
901e390
to
84a6093
Compare
There will be some renaming to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved it to a separate file for visibility.
Added JWT support (if we really want it 👍) |
Sources/LiveKit/Auth/Sandbox.swift
Outdated
|
||
/// `Sandbox` queries LiveKit Sandbox token server for credentials, | ||
/// which supports quick prototyping/getting started types of use cases. | ||
/// - Warning: This token endpoint is **INSECURE** and should **NOT** be used in production. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
/// Protocol for types that can provide connection credentials. | ||
/// Implement this protocol to create custom credential providers (e.g., fetching from your backend API). | ||
public protocol TokenSource: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we'll need to expand this docstring to explain the whole system, since it seem like the entrypoint where people will encounter tokens for the first time (on the params to the room connect method)
e27bd11
to
6b47bb2
Compare
Not sure about the final form (stateless/stateful), depends on JS impl as well 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some thoughts from a quick glance, pretty solid code in general.
Sources/LiveKit/Auth/Sandbox.swift
Outdated
/// `Sandbox` queries LiveKit Sandbox token server for credentials, | ||
/// which supports quick prototyping/getting started types of use cases. | ||
/// - Warning: This token endpoint is **INSECURE** and should **NOT** be used in production. | ||
public struct Sandbox: TokenEndpoint { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, is this like mostly for prototyping ?
I am wondering if we should name it as LiveKitSandbox ? so that it will be more explicit. And we can use similar approaches for other prototype APIs.
// MARK: - Cache | ||
|
||
/// `CachingTokenSource` handles caching of credentials from any other `TokenSource` using configurable store. | ||
public actor CachingTokenSource: TokenSource, Loggable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a TokenManager to me :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to resolve naming in JS first to avoid back-and-forth 🥲
|
||
/// Protocol for abstract store that can persist and retrieve a single cached credential pair. | ||
/// Implement this protocol to create custom store implementations e.g. for Keychain. | ||
public protocol TokenStore: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we expecting the advanced users to extend this TokenStore protocol ?
Any reason that a persistent store might not meet all the needs ?
It might just be me, I wonder whyTokenStore is tied to one credential, will it be a valid use case that a token needs to be refreshed since a session life is long ? in that case, we might want to update a tokenStore rather than re-creating a new one ? (it might just make things a bit less error prone for the long term)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on an ongoing session tokens are automatically refreshed by the server and sent down to provide support for reconnects.
@xianshijing-lk thanks for all the great feedback, there's a slightly longer discussion on the JS PR already, it's hard to keep'em in sync: livekit/client-sdk-js#1645 |
# Conflicts: # Package.swift # [email protected]
This reverts commit 6b47bb2.
51915ab
to
0c89008
Compare
case participantIdentity = "participant_identity" | ||
case participantMetadata = "participant_metadata" | ||
case participantAttributes = "participant_attributes" | ||
case roomConfiguration = "room_configuration" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be roomConfig/room_config:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I still need to move to protobufs for serialization - that's fixed there 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll finally go for some equivalent of your ProtoConverterTest
(that does not exist for Swift yet) vs exposing protobufs.
This needs a separate ticket as the approach is unclear (reflection, etc.)
/// - Configuring recording or streaming options | ||
/// | ||
/// - SeeAlso: [Room Configuration Documentation](https://docs.livekit.io/home/get-started/authentication/#room-configuration) for more info. | ||
public let roomConfiguration: RoomConfiguration? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JS diff: skips agentName/agentMetadata
as it's still nested inside RoomConfiguration.agents
and can be injected from the upcoming session...
# Conflicts: # Package.swift # [email protected] # Tests/LiveKitTestSupport/TokenGenerator.swift
/// - ``EndpointTokenSource``: For custom backend endpoints using LiveKit's JSON format | ||
/// - ``CachingTokenSource``: For caching credentials (or use the `.cached()` extension) | ||
public protocol TokenSourceConfigurable: Sendable { | ||
func fetch(_ options: TokenRequestOptions) async throws -> TokenSourceResponse |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TokenRequestOptions?
is also an option here as mentioned above, similarly to ... connectOptions: ConnectOptions? = nil, roomOptions: RoomOptions? = nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think at least for the sandbox token server the options would be required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
d44e893
to
6feec3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from a quick look, lgtm
@hiroshihorie @davidliu as we won't integrate that into the I think Android and Swift are almost identical atm, really easy to compare and follow, left some minor comments on Android. |
Is this ready to land ? Please let me know if you need help on the review or ping other reviewers Thanks |
Incarnation of:
TokenSource
token fetching abstraction client-sdk-js#1645 🌏Key differences: