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

make compatible with rubyntlm 0.3 #105

Merged
merged 1 commit into from
Dec 16, 2013
Merged

Conversation

ryanhattam
Copy link
Contributor

httpi has 'rubyntlm', '~> 0.3.2' in its gem file, but there are two problems with using that version.

  1. httpi is encoding the domain (using encode_utf16le) if a domain is passed into for ntlm authentication. When its passed to rubyntlm, it too encodes this parameter, using the same method. This double encoding does not work when passed in for authentication to a server, only the first letter is decoded. The username and password are not encoded by httpi which is correct. I simply stopped httpi from encoding the domain, as it doesn't need to.

  2. httpi is attempting to pass in the 'workstation' by passing it in to the first parameter of rubyntlm's response method (arg), when its actually looking for it in the second parameter (opt). Luckily rubyntlm is also doing a workstation lookup, using the same Socket method, so the one in httpi is pointless. The comment in the httpi code indicates it was attempting to set it because rubyntlm wasn't doing it. Due to this comment, and it setting it in the wrong place it must have been referring to a previous version.

I believe this fixes the following issue: #102

Tests pass with this change.

Thanks

@coveralls
Copy link

Coverage Status

Coverage increased (+21.92%) when pulling 54cb089 on ryanhattam:master into 6ca1151 on savonrb:master.

@rogerleite
Copy link
Member

Thanks for this PR!
I don't know details about NTLM, tests are green and with your description, i think we should merge. @rubiii do you agree?

@tjarratt
Copy link
Contributor

Hey @rogerleite -- I may not be rubiii, but I'm willing to chip in with my two cents...

(I don't know if you noticed the discussion on savonrb/savon, but I'm stepping up to help maintain Savon)

This PR looks good, and based on some cursory research around the rubyntlm docs, it appears that @ryanhattam has investigated this pretty thoroughly, and wrote some pretty good tests to boot.

My vote is to merge.

@rogerleite
Copy link
Member

Hi @tjarratt! Thanks for the help. This PR is really complete, thanks for the job @ryanhattam.
Merging now.

rogerleite added a commit that referenced this pull request Dec 16, 2013
make compatible with rubyntlm 0.3
@rogerleite rogerleite merged commit d80ece0 into savonrb:master Dec 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants