otalk / jingle.js

A generic Jingle session manager implementation, suitable for integration by other XMPP libraries.
MIT License
89 stars 17 forks source link

Add check for a response with a more specific jid #20

Closed xdumaine closed 9 years ago

xdumaine commented 9 years ago

I encountered an issue regarding session establishment - our xmpp client wants to initiate a request to a peer (using jid), but they're using the bare jid, effectively broadcasting a peer connection request to any client connected for that user. The response comes back with a full jid, identifying a specific client to which signaling should occur.

Example: User A (userA@example/desktopClient) requests video to User B (userB@example) and User B responds from userB@example/mobile. User A should update the session with User B to signal on userB@example/mobile, instead of userB@example.

Please let me know what you think of this. I'm still getting my feet wet with XMPP, but I ran this by our xmpp server guy and he said it made sense, but he's not familiar with jingle.

xdumaine commented 9 years ago

The alternative seems to be that I'd truncate the full jid to bare jid on response, but the problem I foresee with that is that User A (in the example above) would be signaling with all connected clients for User B, instead of the peering client.

fippo commented 9 years ago

did you see http://xmpp.org/extensions/xep-0353.html -- i've always wanted to implement that behaviour, but never gotten around to it. This shouldn't require changes to jingle.js

xdumaine commented 9 years ago

I had not seen that, thanks! So that's an extension layer for jingle, right? If so, it seems like jingle.js would be an appropriate place to add that additional signal layer, no? So a session initiation through jingle.js could start that handshake?

legastero commented 9 years ago

This is related to https://github.com/otalk/jingle.js/issues/6

The PR as-is will introduce problems in MUC contexts, where all participants share the same bare JID. (Given that the new Talky.io uses Jingle in that exact setup, this is a no-go)

The proper way to do this is to look in the jingle.responder field and use that as the new peer ID, but that should only be done if there is some level of trust, which is why we haven't done that yet. (Same applies in the opposite direction for jingle.initiator)

This means we probably need to add two new overridable methods: trustResponder(peerID, responderID) and trustInitiator(peerID, initiatorID), which when they return true reassigns the session to the new peer ID.

xdumaine commented 9 years ago

I appreciate the quick feedback from both of you. I'll probably work on adding signaling for the jingle extension linked above and use that on top of jingle.js. Thanks for all your work on these things!

fippo commented 9 years ago

I suppose an implementation of 0353 might go either here or into stanza's jingle plugin.

@legastero not sure if responder is really the right thing... there is a call transfer xep, even though i'd strongly prefer to avoid such telephony usecases :-)

legastero commented 9 years ago

@fippo isn't it just this case: http://xmpp.org/extensions/xep-0166.html#security-redirect ?

legastero commented 9 years ago

This PR should make this possible: https://github.com/otalk/jingle.js/pull/21

Needs testing to make sure it didn't introduce any new issues.