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 check for a response with a more specific jid #20

Closed
wants to merge 1 commit into from

Conversation

xdumaine
Copy link

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
Copy link
Author

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
Copy link
Member

fippo commented Mar 26, 2015

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
Copy link
Author

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
Copy link
Contributor

This is related to #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
Copy link
Author

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!

@xdumaine xdumaine closed this Mar 26, 2015
@fippo
Copy link
Member

fippo commented Mar 26, 2015

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
Copy link
Contributor

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

@legastero
Copy link
Contributor

This PR should make this possible: #21

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

konradkierus pushed a commit to softwarehutpl/jingle.js that referenced this pull request Feb 20, 2018
…ntation, for better one please see discussion here: otalk#20 and this PR otalk#21
konradkierus pushed a commit to softwarehutpl/stanza.io that referenced this pull request Feb 20, 2018
konradkierus pushed a commit to softwarehutpl/stanza.io that referenced this pull request Feb 20, 2018
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.

3 participants