Skip to content
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.

Sign up for GitHub

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

Permissions API #37

Merged
merged 12 commits into from
Jan 7, 2022
Merged

Permissions API #37

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
df255a2
permissions API
davidliu Jan 5, 2022
8ce461d
naming
davidliu Jan 6, 2022
45084b5
don't add track if disallowed
davidliu Jan 6, 2022
17df42c
hold on to track for now
davidliu Jan 6, 2022
2c01b4b
room event for subscription permission update
davidliu Jan 6, 2022
cd274c0
update test
davidliu Jan 6, 2022
9386629
language cleanup
davidliu Jan 6, 2022
6569874
Move track permission handling to RemoteParticipant to allow emission…
davidliu Jan 6, 2022
5cc3a22
Subscription status enum
davidliu Jan 6, 2022
3e25249
revert preventing subscription to disallowed track
davidliu Jan 6, 2022
bf9d864
keep subscribe boolean and add extra subscriptionStatus
davidliu Jan 7, 2022
6132331
fix build
davidliu Jan 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions livekit-android-sdk/src/main/java/io/livekit/android/room/RTCEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package io.livekit.android.room
import android.os.SystemClock
import io.livekit.android.ConnectOptions
import io.livekit.android.dagger.InjectionNames
import io.livekit.android.room.participant.ParticipantTrackPermission
import io.livekit.android.room.track.TrackException
import io.livekit.android.room.util.*
import io.livekit.android.util.CloseableCoroutineScope
Expand Down Expand Up @@ -229,6 +230,13 @@ internal constructor(
}
}

fun updateSubscriptionPermissions(
allParticipants: Boolean,
participantTrackPermissions: List<ParticipantTrackPermission>
) {
client.sendUpdateSubscriptionPermissions(allParticipants, participantTrackPermissions)
}

fun updateMuteStatus(sid: String, muted: Boolean) {
client.sendMuteTrack(sid, muted)
}
Expand Down Expand Up @@ -386,6 +394,7 @@ internal constructor(
fun onUserPacket(packet: LivekitModels.UserPacket, kind: LivekitModels.DataPacket.Kind)
fun onStreamStateUpdate(streamStates: List<LivekitRtc.StreamStateInfo>)
fun onSubscribedQualityUpdate(subscribedQualityUpdate: LivekitRtc.SubscribedQualityUpdate)
fun onSubscriptionPermissionUpdate(subscriptionPermissionUpdate: LivekitRtc.SubscriptionPermissionUpdate)
}

companion object {
Expand Down Expand Up @@ -531,6 +540,10 @@ internal constructor(
listener?.onSubscribedQualityUpdate(subscribedQualityUpdate)
}

override fun onSubscriptionPermissionUpdate(subscriptionPermissionUpdate: LivekitRtc.SubscriptionPermissionUpdate) {
listener?.onSubscriptionPermissionUpdate(subscriptionPermissionUpdate)
}

//--------------------------------- DataChannel.Observer ------------------------------------//

override fun onBufferedAmountChange(previousAmount: Long) {
Expand Down
11 changes: 11 additions & 0 deletions livekit-android-sdk/src/main/java/io/livekit/android/room/Room.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,17 @@ constructor(
localParticipant.handleSubscribedQualityUpdate(subscribedQualityUpdate)
}

override fun onSubscriptionPermissionUpdate(update: LivekitRtc.SubscriptionPermissionUpdate) {
val participant = getParticipant(update.participantSid) ?: return
val track = participant.tracks[update.trackSid] as? RemoteTrackPublication ?: return
track.subscriptionAllowed = update.allowed

// Unsubscribe if become disallowed.
if(!track.subscriptionAllowed && track.subscribed) {
track.setSubscribed(false)
}
}

/**
* @suppress
*/
Expand Down
19 changes: 18 additions & 1 deletion livekit-android-sdk/src/main/java/io/livekit/android/room/SignalClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.vdurmont.semver4j.Semver
import io.livekit.android.ConnectOptions
import io.livekit.android.Version
import io.livekit.android.dagger.InjectionNames
import io.livekit.android.room.participant.ParticipantTrackPermission
import io.livekit.android.room.track.Track
import io.livekit.android.util.CloseableCoroutineScope
import io.livekit.android.util.Either
Expand Down Expand Up @@ -329,6 +330,21 @@ constructor(
sendRequest(request)
}

fun sendUpdateSubscriptionPermissions(
allParticipants: Boolean,
participantTrackPermissions: List<ParticipantTrackPermission>
) {
val update = LivekitRtc.UpdateSubscriptionPermissions.newBuilder()
.setAllParticipants(allParticipants)
.addAllTrackPermissions(participantTrackPermissions.map { it.toProto() })

val request = LivekitRtc.SignalRequest.newBuilder()
.setSubscriptionPermissions(update)
.build()

sendRequest(request)
}

fun sendLeave() {
val request = LivekitRtc.SignalRequest.newBuilder()
.setLeave(LivekitRtc.LeaveRequest.newBuilder().build())
Expand Down Expand Up @@ -433,7 +449,7 @@ constructor(
listener?.onSubscribedQualityUpdate(response.subscribedQualityUpdate)
}
LivekitRtc.SignalResponse.MessageCase.SUBSCRIPTION_PERMISSION_UPDATE -> {
// TODO
listener?.onSubscriptionPermissionUpdate(response.subscriptionPermissionUpdate)
}
LivekitRtc.SignalResponse.MessageCase.MESSAGE_NOT_SET,
null -> {
Expand Down Expand Up @@ -463,6 +479,7 @@ constructor(
fun onError(error: Throwable)
fun onStreamStateUpdate(streamStates: List<LivekitRtc.StreamStateInfo>)
fun onSubscribedQualityUpdate(subscribedQualityUpdate: LivekitRtc.SubscribedQualityUpdate)
fun onSubscriptionPermissionUpdate(subscriptionPermissionUpdate: LivekitRtc.SubscriptionPermissionUpdate)
}

companion object {
Expand Down
49 changes: 49 additions & 0 deletions livekit-android-sdk/src/main/java/io/livekit/android/room/participant/LocalParticipant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,6 +419,30 @@ internal constructor(
}
}

/**
* Control who can subscribe to LocalParticipant's published tracks.
*
* By default, all participants can subscribe. This allows fine-grained control over
* who is able to subscribe at a participant and track level.
*
* Note: if access is given at a track-level (i.e. [allParticipantsAllowed] and
* [ParticipantTrackPermission.allTracksAllowed] is false), any newer published tracks
* will not grant permissions to any participants and will require a subsequent
* permissions update to allow subscription.
*
* @param allParticipantsAllowed Allows all participants to subscribe all tracks.
* Takes precedence over [participantTrackPermissions] if set to true.
* By default this is set to true.
* @param participantTrackPermissions Full list of individual permissions per
* participant/track. Any omitted participants will not receive any permissions.
*/
fun setTrackSubscriptionPermissions(
allParticipantsAllowed: Boolean,
participantTrackPermissions: List<ParticipantTrackPermission> = emptyList()
) {
engine.updateSubscriptionPermissions(allParticipantsAllowed, participantTrackPermissions)
}

fun unpublishTrack(track: Track) {
val publication = localTrackPublications.firstOrNull { it.track == track }
if (publication === null) {
Expand Down Expand Up @@ -616,4 +640,29 @@ data class AudioTrackPublishOptions(
base.audioBitrate,
base.dtx
)
}

data class ParticipantTrackPermission(
/**
* The participant id this permission applies to.
*/
val participantSid: String,
/**
* If set to true, the target participant can subscribe to all tracks from the local participant.
*
* Takes precedence over [allowedTrackSids].
*/
val allTracksAllowed: Boolean,
/**
* The list of track ids that the target participant can subscribe to.
*/
val allowedTrackSids: List<String> = emptyList()
) {
fun toProto(): LivekitRtc.TrackPermission {
return LivekitRtc.TrackPermission.newBuilder()
.setParticipantSid(participantSid)
.setAllTracks(allTracksAllowed)
.addAllTrackSids(allowedTrackSids)
.build()
}
}
22 changes: 12 additions & 10 deletions livekit-android-sdk/src/main/java/io/livekit/android/room/participant/RemoteParticipant.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,17 +98,8 @@ class RemoteParticipant(
triesLeft: Int = 20
) {
val publication = getTrackPublication(sid)
val track: Track = when (val kind = mediaTrack.kind()) {
KIND_AUDIO -> AudioTrack(rtcTrack = mediaTrack as AudioTrack, name = "")
KIND_VIDEO -> RemoteVideoTrack(
rtcTrack = mediaTrack as VideoTrack,
name = "",
autoManageVideo = autoManageVideo,
dispatcher = ioDispatcher
)
else -> throw TrackException.InvalidTrackTypeException("invalid track type: $kind")
}

// We may receive subscribed tracks before publications come in. Retry until then.
if (publication == null) {
if (triesLeft == 0) {
val message = "Could not find published track with sid: $sid"
Expand All @@ -127,6 +118,17 @@ class RemoteParticipant(
return
}

val track: Track = when (val kind = mediaTrack.kind()) {
KIND_AUDIO -> AudioTrack(rtcTrack = mediaTrack as AudioTrack, name = "")
KIND_VIDEO -> RemoteVideoTrack(
rtcTrack = mediaTrack as VideoTrack,
name = "",
autoManageVideo = autoManageVideo,
dispatcher = ioDispatcher
)
else -> throw TrackException.InvalidTrackTypeException("invalid track type: $kind")
}

publication.track = track
track.name = publication.name
track.sid = publication.sid
Expand Down
15 changes: 13 additions & 2 deletions livekit-android-sdk/src/main/java/io/livekit/android/room/track/RemoteTrackPublication.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import io.livekit.android.dagger.InjectionNames
import io.livekit.android.events.TrackEvent
import io.livekit.android.events.collect
import io.livekit.android.room.participant.RemoteParticipant
import io.livekit.android.util.LKLog
import io.livekit.android.util.debounce
import io.livekit.android.util.invoke
import kotlinx.coroutines.*
Expand Down Expand Up @@ -63,12 +64,15 @@ class RemoteTrackPublication(
private var videoQuality: LivekitModels.VideoQuality? = LivekitModels.VideoQuality.HIGH
private var videoDimensions: Track.Dimensions? = null

var subscriptionAllowed: Boolean = true
internal set

val isAutoManaged: Boolean
get() = (track as? RemoteVideoTrack)?.autoManageVideo ?: false

override val subscribed: Boolean
get() {
if (unsubscribed) {
if (unsubscribed || !subscriptionAllowed) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if this should return false. The track is subscribed to, but just that it does not have permissions. Feels like this needs to return an enum which gives a better picture of subscription state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm good question, I think technically it's desired to be subscribed, but tricky because clients use subscribed to indicate if subscription is successful and ready to be streamed.

returning false probably makes more sense than true in this case.

return false
}
return super.subscribed
Expand All @@ -88,9 +92,16 @@ class RemoteTrackPublication(
}

/**
* subscribe or unsubscribe from this track
* Subscribe or unsubscribe from this track
*
* If [subscriptionAllowed] is false, subscription will fail.
*/
fun setSubscribed(subscribed: Boolean) {
if (subscribed && !subscriptionAllowed) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this preventing sending an UpdateSubscriptions message? Sorry, I maybe mistaking code here. But, sending UpdateSubscriptions should not be prevented. Server will send the SubscriptionPermissionUpdate when there are no permissions. Note that permissions can be revoked when a track is actively subscribed. So, the server could send that update mid stream too. So, clients should always send subscription message and listen for the update from the server.

I think this code is not preventing sending that message, but just wanted to elaborate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmm, the case I was trying to cover here was if we had previously received a subscription permission update disallowing a track, and the client decides to attempt to subscribe anyways. Should we still attempt the subscription in that case?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. Maybe, the log message can be something like already subscribed, but currently does not have permissions?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still let the client update the server, because it's indicating a desired state.

It does make the API a bit inconsistent, which I don't love:

  • .subscribed indicates if it's desired to be subscribed and successfully subscribed
  • .setSubscribed updates the desired state

Any alternatives?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed with @boks1971, the proposal is to turn .subscribed into an enum describing the current subscription status, with "subscribed, not allowed" and "subscribed" as two distinct states.

Thinking about the setSubscribed part, I think it might be more descriptive of user intent to have separate subscribe() and unsubscribe() functions, which would help clear up the inconsistency you note.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that sounds good. though I'm wary about breaking current users. Especially on JS. we could perhaps deprecate .subscribed, but still provide it as a way of returning subscribeState == Subscribed ?

LKLog.w { "Attempted to subscribe to a disallowed track." }
return
}

unsubscribed = !subscribed
val participant = this.participant.get() as? RemoteParticipant ?: return

Expand Down