Closed mqp closed 3 years ago
The changes looks good. We need to make sure that old tokens without room_ids still works. Like I said, I can't test this right now, I have no JWT implemented yet. :) Thanks for doing it.
While you're at it :-) you may move all the join validation logic into a function so we can also call it in process_subscribe
.
@mqp I'll start testing this next week. I just finished adding to my app the room security logic by organization, only members of an organization can access a room. The backend and ui works great, but without the JWT for janus, just knowing the room id, you can eavesdrop the conversations without the others in the rooms knowing you're there. :D
@mqp This works great!
I use a simple Phoenix backend with phx_gen_auth and absinthe, I added guardian to generate the jwt and return it as part of my graphql query when getting the room info. I'll document how to configure the key in both phoenix and janus to generate the jwt and how to set it with setJoinToken later.
I tested without room_ids
MyAppWeb.PermsToken.token_for_perms(
%{user_id: user.id, kick_users: user.id == room.user_id, join_hub: true})
and with room_ids
MyAppWeb.PermsToken.token_for_perms(
%{user_id: user.id, kick_users: user.id == room.user_id, join_hub: true, room_ids: [room.slug]})
In my js code, I do the graphql query and set the results in window.app.roomData
and I have this in my adapter-ready
listener:
if (window.app.roomData && window.app.roomData.permsToken) {
adapter.setJoinToken(window.app.roomData.permsToken);
}
I tested to hijack the room like this:
window.app.roomData={permsToken: "token copied from another tab"}
AFRAME.scenes[0].emit('connect')
Without token:
{ msg: "Rejecting anonymous join!" }
With token with room_ids from another room:
{ msg: "Rejecting join without permission!" }
With token without room_ids
join accepted
With expired token:
{"msg": "Rejecting join with invalid token!"}
For process_subscribe
, I checked, if you know a participant id, you can bypass the security entirely and successfully listen to participant voice.
AFRAME.scenes[0].emit('connect') # we are rejected, but this is just to init NAF.connection.adapter
const {handle, mediaStream, conn} = await NAF.connection.adapter.createSubscriberWithSubscribeMessage("5170189847018839")
const audio = document.createElement('audio')
const audio.srcObject = mediaStream
const audio.play()
createSubscriberWithSubscribeMessage
is a copy of createSubscriber
with this.sendJoin
replaced by this.sendSubscribe
implemented like this:
sendSubscribe(handle, subscribe) {
return handle.sendMessage({
kind: "subscribe",
what: subscribe,
});
}
If we want to fix it, we need to change the api of the subscribe message to include room_id and token. We may revisit this as part of #76
So you can merge this PR. And you may want to comment https://github.com/mozilla/janus-plugin-sfu/blob/a2fffd126e42d1518f6719ab825ec965efc2d9e6/src/lib.rs#L609 to not handle MessageKind::Subscribe
and https://github.com/mozilla/janus-plugin-sfu/blob/a2fffd126e42d1518f6719ab825ec965efc2d9e6/src/messages.rs#L74 and the process_subscribe
function for extra security until we change the API.
See my notes if you want to implement the JWT creation in a Phoenix app https://github.com/mozilla/janus-plugin-sfu/issues/77#issuecomment-822544499
OK, I'll merge this. The subscription is a good point.
Since the NAF Janus adapter doesn't use subscribe, I'm not afraid to make breaking changes to it. I think it should take a user ID, a JWT, and no room ID, and check to see whether the user is in any room authorized by the JWT; if so, you're allowed to subscribe to them. If this sounds good I'll probably code it up next evening.
I disagree. If you do like you said, it's almost like the join message with the subscribe param, so not worth to have a different api to do the same thing. For me the subscribe message is good to be able to subscribe to a participant without being a publisher yourself, so you're not in any rooms currently. I want to use it in this way in the near future for a class room with one publisher and 50 participants that only need to listen, not need to create 50 RTCPeerConnections (Chrome won't support it), only one to the publisher is needed. And I'll probably use Phoenix presence to list the participants. See #76 for my use case.
If we have the concept of being in the room without being a publisher, your proposal makes sense. But we don't have that currently.
I did a fix in naf-janus-adapter that wrongly did a delayed reconnect in case of error in join message. https://github.com/networked-aframe/naf-janus-adapter/commit/92981d3e46574872f1ad1eb09518909a3b48d031
I disagree. If you do like you said, it's almost like the join message with the subscribe param, so not worth to have a different api to do the same thing. For me the subscribe message is good to be able to subscribe to a participant without being a publisher yourself, so you're not in any rooms currently. I want to use it in this way in the near future for a class room with one publisher and 50 participants that only need to listen, not need to create 50 RTCPeerConnections (Chrome won't support it), only one to the publisher is needed. And I'll probably use Phoenix presence to list the participants. See #76 for my use case.
Maybe I miscommunicated:
I think it should take a user ID, a JWT, and no room ID, and check to see whether the user is in any room authorized by the JWT; if so, you're allowed to subscribe to them. If this sounds good I'll probably code it up next evening.
I mean that the user ID is the ID of the person you want to subscribe to. So even if you have no publisher and have not joined any room, you could subscribe to a publisher who has joined some room, as long as you have a JWT with that room in its room_ids
.
Ahhhhhh :D I misunderstood it indeed. I understand it a lot better now. This is a great solution, I agree ;-)
The User ID is currently in the param what { media: UserID }
So you're proposing just to change
Subscribe { what: Subscription }
to
Subscribe { what: Subscription, token: String }
Is that right?
I updated #83 with my understanding.
Ahhhhhh :D I misunderstood it indeed. I understand it a lot better now. This is a great solution, I agree ;-) The User ID is currently in the param
what { media: UserID }
So you're proposing just to changeSubscribe { what: Subscription }
toSubscribe { what: Subscription, token: String }
Is that right?
Yeah, that's what I'm suggesting.
Implements #78. Now if JWT authorization is enabled, you may join a room only if you send a JWT containing
join_hub = true
and with either noroom_ids
(i.e. like all old tokens; you can join any room) or aroom_ids
array containing the room ID you're trying to join.Not yet tested, so not merging yet.