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

(maint) Fix how scooter authenticates with pe console #92

Merged
merged 2 commits into from
Nov 30, 2016
Merged

(maint) Fix how scooter authenticates with pe console #92

merged 2 commits into from
Nov 30, 2016

Conversation

lucywyman
Copy link
Contributor

No description provided.

@puppetlabs-jenkins
Copy link
Contributor

Can one of the admins verify this patch?

@tvpartytonight
Copy link
Contributor

I think that this is a breaking change that should necessitate a 4.0 release. Anyone disagree?

@tvpartytonight
Copy link
Contributor

@puppetlabs-jenkins test this please

@aperiodic
Copy link

aperiodic commented Nov 29, 2016

This is fragile. I'd prefer to grab the cookie by name, rather than its position in the list of cookies returned by RBAC, which will break if the number or order of cookies changes. Are there cookie-parsing facilities in Faraday? Is there a cookie-parsing library we could bring in? If neither, then even just checking the beginning of each component after splitting on ; and trimming to find the correct name is a more robust solution.

@tvpartytonight
Copy link
Contributor

The base dispatcher uses some middleware to handle cookies here. In theory, any cookies returned from a request should automatically be parsed by that middleware and be sent for subsequent requests. We should take a closer look to see if/how this is breaking.

@jonathannewman
Copy link
Contributor

jonathannewman commented Nov 29, 2016

@tvpartytonight not sure I understand your comment, but are you suggesting that we might be able to extract the pl_ssti cookie from the cookie-jar?

In this case, we need to extract the cookie and put it into the headers of all the requests. @aperiodic and I are wondering if there is a better way to extract the cookie.

@jonathannewman
Copy link
Contributor

It looks like the spec tests need to be updated too:

1) Scooter::HttpDispatchers::ConsoleDispatcher with a beaker host passed in .signin with a page that returns an xcsrf token sets the xcsrf token in the header
     Failure/Error: expect(subject.connection.headers['X-CSRF-Token']).to eq('xcsrf-token')

       expected: "xcsrf-token"
            got: nil

       (compared using ==)
     # ./spec/scooter/httpdispatchers/consoledispatcher_spec.rb:48:in `block (4 levels) in <module:Scooter>'

@lucywyman
Copy link
Contributor Author

@aperiodic I'm having trouble finding a library or faraday method to parse the cookie, and per conversation here lostisland/faraday#44 think there may not exist a great solution. Given that, I'll rework the splitting/searching, but I'm coming up empty finding a builtin way to parse the cookie :/

@lucywyman
Copy link
Contributor Author

@jonathannewman @aperiodic @tvpartytonight Fixed the spec tests to reflect the new token, and tried to make the cookie grab more robust. @aperiodic The difficult part, since there's no library for parsing the header, is that there isn't much consistency in how things are delimited in the header, so the cookie is essentially stuck in the middle of an ugly string.

Here's the header:

{"server"=>"nginx/1.8.1", "date"=>"Tue, 29 Nov 2016 22:05:41 GMT", "content-length"=>"0", "connection"=>"close", "set-cookie"=>"JSESSIONID=b05e9b11-5e9f-4d6a-9faf-e28a0415197d; Path=/; Secure; HttpOnly, rememberMe=deleteMe; Path=/auth; Max-Age=0; Expires=Mon, 28-Nov-2016 22:05:41 GMT, pl_ssti=0CeHhpz5PPLna7kpaEMcTHjJ62z9eizHTzsxEXNK8W20;Secure;Path=/", "location"=>"/", "x-frame-options"=>"DENY"}

And the cookie:

"JSESSIONID=b05e9b11-5e9f-4d6a-9faf-e28a0415197d; Path=/; Secure; HttpOnly, rememberMe=deleteMe; Path=/auth; Max-Age=0; Expires=Mon, 28-Nov-2016 22:05:41 GMT, pl_ssti=0CeHhpz5PPLna7kpaEMcTHjJ62z9eizHTzsxEXNK8W20;Secure;Path=/"

I think what I've got is robust enough, and works, but let me know if you have any ideas on how to improve!

begin
acquire_xcsrf
require 'faraday'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this require here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops, leftover, will delete

return response if response.status != 200
# try to be helpful and acquire the xcsrf; catch any error that occurs
# in the acquire_xcsrf method
begin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need to begin; rescue; end? I imagine that this was here for Nokogiri

@zreichert zreichert merged commit 93357eb into puppetlabs:master Nov 30, 2016
@aperiodic
Copy link

@lucywyman late to the party but the new cookie-grab approach is much better, since it's resilient to the order or number of cookies changing. Thanks for improving that!

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.

6 participants