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

Handle orm-rt cookie issue #152

Closed
wants to merge 7 commits into from
Closed

Conversation

elrob
Copy link

@elrob elrob commented Nov 11, 2019

  • orm-rt cookie has floating point max-age (which it shouldn't https://tools.ietf.org/html/rfc6265#section-5.2.2)
  • requests library fails to include it in the cookie jar (parsing the string as an int fails and the cookie is discarded: https://github.com/python/cpython/blob/20a4f6cde65549fd0252eb8c879963e0e8b40390/Lib/http/cookiejar.py#L1448)
  • in-addition, the 'redirect-uri' was not the same as that used in the browser login flow which uses the 'next' parameter.
  • one more change is that there is now a check after the login (or cookie load), to ensure that the profile page is accessible. This helps by failing fast if there is an authentication issue. Previously, the script would continue to process the book but just download truncated chapters. The issues people have raised about truncated chapters seem to all be caused by authentication issues.

Fixes:

@McPatate
Copy link

Hey @elrob , I am getting an "Unexpected error" after pulling your repo. The script is unable to perform auth login.

@elrob
Copy link
Author

elrob commented Nov 12, 2019

Hey @elrob , I am getting an "Unexpected error" after pulling your repo. The script is unable to perform auth login.

I'm not having any issue with logging in. I've just tested again myself. How far does the script get for you?

- orm-rt cookie has floating point max-age
- requests library fails to include it in response.cookies
@McPatate
Copy link

I was using the wrong password ...

But I'm still getting cropped books.

@phamhoangtuan
Copy link

I tried your code with a book 9781491983638 but the issue #150 still happened

@elrob
Copy link
Author

elrob commented Nov 18, 2019

@McPatate @phamhoangtuan
I have investigated further and made some more changes that resolve another couple of issues with login.
Can you try again with a fresh version of this: https://github.com/elrob/safaribooks
Make sure to delete anything in the Books directory before trying.

@lorenzodifuccia
Copy link
Owner

Thank you @elrob, you made a really good analysis of the problem.
Is the #152 up-to-date with all the fixes that you applied on your fork?

@elrob
Copy link
Author

elrob commented Nov 20, 2019

@lorenzodifuccia Yes, thank you. This PR is up-to-date with all the fixes mentioned in the PR description above (I just updated the description again).

lorenzodifuccia added a commit that referenced this pull request Nov 21, 2019
Fixes #109 
Closing #152 
Closing #153
Closing #154
Working on #150 

Thanks to: @elrob, @McPatate and @cgimenes
Copy link
Owner

@lorenzodifuccia lorenzodifuccia left a comment

Choose a reason for hiding this comment

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

Hi Rob, I pushed an update with your changes. Thanks you for what you did, nice job man!
Few point that i changed:

  1. HEADERS are needed because of the user-agent and because of the accept-enconding, there was an issue related to Brotli compression algorithm (Problem with CSS file #21).
  2. Renamed update_cookie_jar_with_float_max_age_cookies to handle_cookie_update to be more general and let this function be used also for future handling.
  3. Personally, I think you do not really need to process the cookie through SimpleCookie, I think you can just search if there is a float num and set the cookie manually.
  4. I added a try/catch block for the next_parameter parsing
  5. I really like them, but I personally discard the use of the F-strings because of compatibility. String interpolation needs Py3.6+.
  6. I fixed and changed some other things...

Thank you!
Cheers 🍻

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.

4 participants