Skip to content

Add support for cookie based authentication mechanisms#36

Merged
zachfeldman merged 2 commits intozachfeldman:masterfrom
caseyhadden:cookie-support
Feb 2, 2015
Merged

Add support for cookie based authentication mechanisms#36
zachfeldman merged 2 commits intozachfeldman:masterfrom
caseyhadden:cookie-support

Conversation

@caseyhadden
Copy link
Copy Markdown

I'd like to use rubypress with a wordpress instance that requires authentication, but does not support the basic authorization header. Instead, it requires the ability to pass a set of cookies.

I have a set of changes in my fork, the cookie-support branch. However, when I went to add tests, I get a lot of errors related to the response body size in the existing tests. So, I held off on an actual pull request. For example:

  1. #comments #getComments
    Failure/Error: CLIENT.getComments[0].should include("post_id" => post_id.to_s)
    RuntimeError:
    Wrong size. Was 2825, should be 2839

When I looked at the VCR cassette file (getComments.yml), some of this seems to be attributable to the formatting. If I 'line up' the </struct></value> and other lines in the file, then the response size matches. I'm not well enough versed in VCR to know if this is the actual issue or not.

I'm happy to offer my branch as a pull request, but don't want to throw it over without tests. These same failures happen for me on the master branch, so I don't think it is anything I've done. I would also like to make sure that is the case by having the full suite run cleanly.

Do you have any thoughts on whether these failures are expected or suggestions as to why I'm seeing them specifically? Thanks.

@zachfeldman
Copy link
Copy Markdown
Owner

Hi there @caseyhadden ! Cookie-based auth? That seems kind of weird...how do you generate the cookie to begin with? Couldn't you login using the normal login process for that site and then use Rubypress?

I can get all of tests to pass on Travis running WordPress 3.4+ but I haven't run the tests against WordPress 4.0 to be honest. I used to run across some VCR issues relating to content length, which led to this fix:

c.before_playback(:getUsersBlogs){|interaction|

I'd be glad to start running tests against a 4.x installation if you think this will help, but I definitely don't want to merge in something that makes tests fail.

Thanks for your time and contribution! Glad to help further if I can.

Casey Hadden added 2 commits January 29, 2015 12:26
VCR has a capability to calculate the response content length at the
time of the cassette playback. This allows changes to the recording
without having to constantly update the information to match the body
length.

Update the VCR setup code to utilize this mechanism for each cassette
playback.
Add support for wordpress sites that require a particular cookie(s) in
order to be accessible. For example, this could happen on a company
intranet where the SSO mechanism does not support basic authorization.

Update client to accept an additional cookie value and pass that along
to the underlying connection when it is set.

This fixes issue zachfeldman#36.
@caseyhadden
Copy link
Copy Markdown
Author

I'm probably doing a disservice by trying to simplify the description down to 'cookie auth'. Our company has a wordpress instance on our intranet. That intranet (including wordpress) is protected by a single sign-on solution, OpenAM. I'm not clear whether it is an OpenAM specific thing or just they way our IT staff has set it up, but our OpenAM instance is not capable of standard HTTP authentication with the basic authorization header. Instead, we have to POST with appropriate credentials, then parse the response to recover the cookies provided. Those cookies then need to be included on subsequent requests to avoid being re-challenged for credentials.

When looking into VCR, I found that it has an update_content_length_header feature that will calculate the content length at playback time rather than strictly relying on what is in the cassette file. I made a change in the VCR setup to use that mechanism vs. the manual content length reset you highlighted earlier. I was able to use that to avoid the numerous content length issues I was running into.

Thanks for helping out @zachfeldman. If this just isn't a feature you're interested in dealing with, I understand. Admittedly, our intranet setup seems a little pathological to me.

@zachfeldman
Copy link
Copy Markdown
Owner

No problem @caseyhadden ! Glad to support it if it helps. Are the tests passing on your end now?

@caseyhadden
Copy link
Copy Markdown
Author

Yes, test tests pass for me now. The key was to have VCR dynamically calculate the content-length rather than using the value in the file. With that change, the existing tests and code all passed. Additionally, my change in the client class shows no regressions and I added a couple of tests to verify the cookie presence.

@zachfeldman
Copy link
Copy Markdown
Owner

Ok will merge and verify!

@zachfeldman
Copy link
Copy Markdown
Owner

Ah @caseyhadden , please open a pull request so I can do that. I don't see one from you?

@zachfeldman
Copy link
Copy Markdown
Owner

Ah @caseyhadden here it is thank you!! I was confused.

@caseyhadden
Copy link
Copy Markdown
Author

Thanks, @zachfeldman. It was just a lowly issue until I converted it a couple of minutes ago. So, you were absolutely correct that there wasn't a pull request. The travis failures appear to be related to missing environment variable definitions that I don't believe I'm affecting. It looks like many of the recent submissions are facing that same issue in the travis builds.

zachfeldman added a commit that referenced this pull request Feb 2, 2015
Add support for cookie based authentication mechanisms
@zachfeldman zachfeldman merged commit ef6a8df into zachfeldman:master Feb 2, 2015
@zachfeldman
Copy link
Copy Markdown
Owner

@caseyhadden we're good, it was a Travis issue! travis-ci/travis-ci#617

Thanks for your contribution

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