csingley / ofxtools

Python OFX Library
Other
301 stars 68 forks source link

Add cookie handling to OFXClient #103

Closed rianhunter closed 3 years ago

rianhunter commented 3 years ago

Some OFX gateways require non-standard http requests, e.g. preserving cookies across requests or other custom headers. Instead of trying to encode every possible custom transport sideband requirement, allow user to provide custom URL opener to suit their specific use case, while we only handle the standard OFX requirements.

This has the added benefit of allowing the user to use non-http transports to retrieve OFX data, as well as potentially easing testing.

csingley commented 3 years ago

I'd been thinking more along the lines of adding an arg(s) to override the HTTP headers as necessary... which guides the user to the actual locus of the problem, and leaves open the possibility of saving those to disk as configs, once discovered. What do you think about that?

Is there a use case for non-HTTP transport of OFX data?? I feel so old.

Sorry about all the testing; it's been a help to me.

rianhunter commented 3 years ago

My first reaction was to add an HTTP header keyword but in my case there are actual dynamic http cookies that need to be carried across requests which makes the header approach more cumbersome. Using a custom URL opener I can create a cookie-based opener like this:

    cj = http.cookiejar.CookieJar()
    opener = urllib.request.build_opener(urllib.request.HTTPCookieProcessor(cj))
    url_opener = opener.open

And then pass that to the OFXClient constructor without having to do anything else which feels very convenient. I would use this for doing client.request_profile() then client.request_statements().

rianhunter commented 3 years ago

As far as non-HTTP transport, I don't think that exists, that was just something that I thought could be convenient in the future but not immediately applicable.

rianhunter commented 3 years ago

If it's fine to carry cookies across different OFX requests in the general case then maybe the proper change is to make the cookie url opener the default opener. I wasn't sure if that would break older code so I avoided it. (it would necessarily break the verify_ssl argument if cookies were enabled)

csingley commented 3 years ago

Thanks for showing me those corners of http and urllib in the stdlib... the web is not my forte. Yeah, that approach looks great! Does it work on the client side too? The docs say:

It extracts cookies from HTTP requests, and returns them in HTTP responses.

If this works, you'd be doing me a favor to just go ahead and implement it universally. The MO here is TOOWTDI... any color you like, as it long as it matches Quicken's color. Our job is to reverse engineer & emulate.

To me it makes more sense to concentrate all that brain damage in a single place (here) where it can be managed, than to spread it all over the Internet by requiring everybody to know about it.

Thanks @rianhunter, good to hear from you.

rianhunter commented 3 years ago

Alright I'll modify this change to just integrate cookie handling into the main OFXClient

rianhunter commented 3 years ago

Okay I removed the custom url opener functionality and instead created a new persist_cookies parameter to the OFXClient constructor. Some caveats:

csingley commented 3 years ago

Yeah that looks pretty good.

rianhunter commented 3 years ago

I was expecting that you'd squash all the changes in this branch into a single commit before merging but up to you! It might make the master branch commit history a bit cleaner.

rianhunter commented 3 years ago

Are you working on a separate change to make persist_cookies true? If it's going to be default true and ditch verify_ssl then we may not even need the keyword argument in the first place and just always have cookies enabled.

csingley commented 3 years ago

Whoops sorry! Not used to managing changes in the GUI; I just mashed the button. Well, you do still have a commit bit, so...

Yeah I'll clean up what we discussed above. I think we should leave the persist_cookies in place, default True, until we confirm that it doesn't break across all the myriad FIs people run through this code.

csingley commented 3 years ago

Changes we discussed merged in release 0.9