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.

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

Add optional room_ids to JWT #81

Merged
merged 1 commit into from
Apr 19, 2021
Merged

Add optional room_ids to JWT #81

merged 1 commit into from
Apr 19, 2021

Conversation

mqp
Copy link
Contributor

@mqp mqp commented Feb 16, 2021

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 no room_ids (i.e. like all old tokens; you can join any room) or a room_ids array containing the room ID you're trying to join.

Not yet tested, so not merging yet.

@vincentfretin
Copy link
Contributor

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.

@vincentfretin
Copy link
Contributor

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.

@vincentfretin
Copy link
Contributor

@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

@vincentfretin
Copy link
Contributor

vincentfretin commented Apr 19, 2021

@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

MessageKind::Subscribe { what } => process_subscribe(from, &what),
to not handle MessageKind::Subscribe and
Subscribe { what: Subscription },
and the process_subscribe function for extra security until we change the API.

@vincentfretin
Copy link
Contributor

See my notes if you want to implement the JWT creation in a Phoenix app #77 (comment)

@mqp
Copy link
Contributor Author

mqp commented Apr 19, 2021

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.

@mqp mqp merged commit f16c42c into master Apr 19, 2021
@mqp mqp deleted the improve-jwts branch April 19, 2021 23:11
@vincentfretin
Copy link
Contributor

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.

@vincentfretin
Copy link
Contributor

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.

@vincentfretin
Copy link
Contributor

I did a fix in naf-janus-adapter that wrongly did a delayed reconnect in case of error in join message.
networked-aframe/naf-janus-adapter@92981d3

@mqp
Copy link
Contributor Author

mqp commented Apr 20, 2021

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.

@vincentfretin
Copy link
Contributor

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?

@vincentfretin
Copy link
Contributor

I updated #83 with my understanding.

@mqp
Copy link
Contributor Author

mqp commented Apr 21, 2021

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?

Yeah, that's what I'm suggesting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants