Skip to content

Conversation

@funkycode
Copy link

Adding basic implantation of the
SIP Referred-By Mechanism (currently refer uri only)

Adding basic implantation of the
SIP Referred-By Mechanism (currently refer uri only)
@wakamoleguy
Copy link
Contributor

Hey!

Thanks for the pull request! It's great to see people getting in there and hacking on SIP.js. However, I have a few concerns about the pull request. I brushed up on RFC 3892 this morning, and it provides a lot of flexibility as to what to put in the Referred-By header, as well as a more complex mechanism for Content-Id and security. Admittedly, you are intending only to tackle a small use case first, but ideally if we support the RFC, we could support the entire RFC.

I know that asking all of that of a pull request is infeasible, and starting small is great. That said, this particular behavior can already be specified by the application (via options.extraHeaders). Today, with or without this pull request, the referee application would also need to specify its own options.extraHeaders in order to copy the Referred-By header into the new INVITE. A great candidate for a pull request, I think, would be adding that mechanism to the default session.followRefer() behavior. That way the referring application is still under control of what extra headers are specified.

Lastly, just a question: Is there a reason you decided to use this.dialog.local_uri instead of this.localIdentity?

Cheers,

Will

@funkycode
Copy link
Author

I mostly do not code on js and I'm more the guy from the asterisk side. And i just made small changes to the code developed by more experienced js developer working on project with your lib, so it would work on our system. It was actually options.extraHeaders first. But I decided that it would be nice to share it with others in case they'll need it and make it default one.
I know that the implantation is basic one and do not cover most of the RFC.
But as far as i know(wireshark) even SNOM(http://www.snom.com/en/) IP phones send it like that (at least with default settings), and they are strict on standards :)
I would like to help more and maybe to implant all the logic at non-work hours, but currently i have no free time and queue of other "home" projects, so i don't think it might happen in near future.
So it's your decision whenever you want or not to get pull request, as we can do handle it with options.extraHeaders. 😄
There is no particular reason why i used this.dialog.local_uri, as i said i barely code on js and i found this.dialog.local_uri first.

@wakamoleguy wakamoleguy mentioned this pull request Jul 30, 2014
@wakamoleguy
Copy link
Contributor

There is no particular reason why i used this.dialog.local_uri, as i said i barely code on js and i found this.dialog.local_uri first.

I guessed as much, but curiosity got the better of me. It's good to know what parts of the code jump out more than others.

Anyways, I agree that this could be good default behavior to add. I'd like to make sure we get it into the API in an extensible manner and with plenty of choice for the applications to opt in or out. I'm going to close this pull request, but I've opened an issue (#79) to track the discussion and/or implementation of RFC 3892 support. I also linked back from there to this pull request which, closed or not, will provide some color and guidance on the issue. It may not happen soon, but if it is on our list, I'm sure we will get to it.

Good luck on your home projects, and I hope to see you around these parts again soon. :-)

Cheers!

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