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

Tigase stanza error #95

Closed
wants to merge 17 commits into from
Closed

Tigase stanza error #95

wants to merge 17 commits into from

Conversation

pmashchak
Copy link

It quick and a bit dirty fix, but hope this will help fix the problem.
Tigase is really wide used XMPP server and I think you should not miss opportunity to support it also.
The problem was here with priority of child nodes in <stream:features stanza>
It was looking like this:

<features>
    <ver xmlns="urn:xmpp:features:rosterver"/>
    <session xmlns="urn:ietf:params:xml:ns:xmpp-session"/>
    <bind xmlns="urn:ietf:params:xml:ns:xmpp-bind"/>
 </features>

Tigase can't create session before binding.
I solve this problem by sorting child nodes to move <bind/> before <session/> request.
Maybe this is not the best way, but we found a root of the problem.

@arthef
Copy link

arthef commented Jun 14, 2012

It's not that Tigase can't create session before bindings. The spec says that the session is created after the resource bind, and Tigase just follows the spec. Besides, the session feature has been deprecated and removed from the last spec. Tigase only offers it for backward compatibility as lots of clients are not updated and require it.
The order of elements within features element is not important, the spec says, so the client, not the server should be fixed to follow the spec.

@benlangfeld
Copy link
Member

Could you trim this down to only the relevant commit (on a feature branch) with some specs?

@tomquas
Copy link
Contributor

tomquas commented May 30, 2013

blather-0.8.4 still/again has issues with connection to Tigase. pavlo's patch 25a12da somehow got lost. could you please add a regression to ensure the bind-before-session request scheme works? thx

@benlangfeld
Copy link
Member

A pull request would be lovely :)

@tomquas
Copy link
Contributor

tomquas commented May 30, 2013

not sure i understand. pull request #95 exists, what is it you want me to do?

@benlangfeld
Copy link
Member

You say it's regressed due to a lack of test coverage...

@tomquas
Copy link
Contributor

tomquas commented May 30, 2013

right. can you tell me what your plans are for this pull request? it contains the fix plus lots of other stuff. should i extract that patch, add a test, and open a new pull request?

@benlangfeld
Copy link
Member

Ah, I see, apologies, I failed to fully check on the status of this.

Yeah, this can't be merged until specs are added and only the relevant commits are included. It seems @arturhefczyc simply disappeared, and this was never merged. I don't use Tigase, so I can't justify working on this myself. Contributions from those who care are welcome, but they have to meet a certain standard to be included.

@tomquas
Copy link
Contributor

tomquas commented May 30, 2013

ok. i isolated the fix and added a test. however, i can't make the test run through, it keeps hanging somewhere in the mock server. can you help? https://gist.github.com/5680241

@arthef
Copy link

arthef commented May 30, 2013

Ben,

What you mean by "arturhefczyc simply disappeared"? ;-)
I am still there, somewhere. I am just involved in a different project.
To be honest I do not know why I am a part of the discussion, I can't remember being
involved in the project.

Artur

On May 30, 2013, at 7:58 AM, Ben Langfeld notifications@github.com wrote:

Ah, I see, apologies, I failed to fully check on the status of this.

Yeah, this can't be merged until specs are added and only the relevant commits are included. It seems @arturhefczyc simply disappeared, and this was never merged. I don't use Tigase, so I can't justify working on this myself. Contributions from those who care are welcome, but they have to meet a certain standard to be included.


Reply to this email directly or view it on GitHub.

@benlangfeld
Copy link
Member

This was your pull request, and it was never completed, which is the
reason it was not merged :)

benlangfeld added a commit that referenced this pull request Jun 1, 2013
#95 fixes client auth issue with Tigase
@benlangfeld
Copy link
Member

@arturhefczyc Yet again, I make a boob. Sincerest apologies, this PR was originally submitted by @pmashchak.

@tomquas got some specs in for this and submitted #112, which has now been merged. I'm doing a little bit of cleanup and will get 0.8.5 out shortly. Thanks everyone, and apologies again for my lack of close attention.

I'm going to have to quit iPhone PR review.

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

Successfully merging this pull request may close these issues.

4 participants