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

Authentication Fix: Merge updated auth flow from pyicloud #734

Merged
merged 10 commits into from
Dec 19, 2023

Conversation

scaraebeus
Copy link
Contributor

@scaraebeus scaraebeus commented Dec 10, 2023

Potential alternative fix for recent auth issue #729.

As in PR #733 - I have not followed all guidelines at this time aside from format and lint.

Purpose

Attempted minimal approach to merge strictly the authentication flow from the 1.0.0 pyicloud release. The AUTH_ENDPOINT appears to be the missing piece in the current pyicloud_ipd release.

This PR also attempts to retain the DOMAIN and LIBRARY features implemented in the pyicloud_ipd fork.

Summary

  • Merged auth code from pyicloud/base.py to pyicloud_ipd/base.py
  • Updated utils.py and exceptions.py to merge changes needed in updated auth code
  • Updated icloudpd/authentication.py to take advantage of 2FA

TODO

  • Validate necessary scripts/test - majority are failing - likely outdated/broken?
  • Update CHANGELOG.md if this is deemed an appropriate path to resolution
  • Potentially compare/merge with PR Update icloudpd code with latest from PyiCloud, add 2FA auth option #733 - or at least review against for
  • Validate DOMAIN functionality for CN is retained
  • Clean up any unnecessary base.py imports
  • Confirm and remove deprecated exceptions in exceptions.py

@cfurrow
Copy link

cfurrow commented Dec 10, 2023

This seems a lot more reasonable vs having to patch in changes (domain selection, library selection) into #733. Now that it's more clear about what was originally missing within the authentication area, I imagine this PR is more appropriate.

Regarding tests, on the master branch, there aren't any test failures, but there are warnings. So it's possible there is going to be a lot of updates to the tests themselves, and most/all of the VCR cassettes to bring them up to date with reality. But, again, hopefully they are easier to address on this branch, vs the other one from #733 which changed a lot more.

@scaraebeus
Copy link
Contributor Author

Looking over the tests in an attempt to update them and the VCR cassettes. Based on the new AUTH_ENDPOINT, it also appears the updated pyicloud_ipd.authenticate() method may not accurately capture invalid (bad) email/password combinations.

It would appear the initial AUTH_ENDPOINT/signin always returns a 200 response. Although there may be another specific header in that response that could be used in a check within the pyicloud_ipd.authenticate() method to catch this right away and raise the PyiCloudFailedLoginException.

It will take some time to go through all of this. For now, this solution appears to more or less work for my needs. I'll try to clean up this PR over time. If anyone else wants to tackle getting the tests cleaned up and uncovering any other potential issues feel free to work on the branch.

If you do, please note you'll have to use your own username/pass to rerecord the VCR's - DO NOT commit anything until you're confident you've scrubbed the tests and VCR's of this info, replacing with the [email protected] (side note - should probably update all the tests to use [email protected]) username and password1 password.

icloudpd: Refactor: 2SA prompt requires device selection to send code
icloudpd: Feat: New --auth-only flag to trigger log in, 2SA/2FA, and set session/cookie file.
              Future log in will validate the tokens without running through full signin flow.
              Can be used to validate the session tokens are still good without having to ping
              the photo endpoints.

pyicloud_ipd: Clean: Removed unused imports
pyicloud_ipd: Fix: Capture additional header data
pyicloud_ipd: Fix: Invalid Username/Password correctly caught now
pyicloud_ipd: Fix: Changes in certain error responses now captured
pyicloud_ipd: Fix: Bypass 2sv/trust when using 2SA

Tests: Refactored authentication tests
Tests: Refactored two_step_auth tests (TODO: Add 2FA tests)
Tests: Updated/Created additional VCRs for auth tests
pyicloud_ipd: Fix: Correct exception reference for API and NoStoredPassword errors

Tests: Refactor: All remaining tests now pass
Tests: Refactor: Update corresponding VCRs for new auth flow
Tests: Cookie/Session files stored in individual test fixtures for running tests independently
icloudpd: Style: Format update (scripts/format)
@scaraebeus
Copy link
Contributor Author

Tests updated with 100% passing. Code coverage at 99% (matching master - missing 1 line in icloudpd/base.py)

Likely still edge cases to address as separate issues. This branch should now resolve the core Invalid email/password combination error.

src/pyicloud_ipd/base.py Dismissed Show dismissed Hide dismissed
src/pyicloud_ipd/base.py Dismissed Show dismissed Hide dismissed
@boredazfcuk
Copy link
Contributor

@scaraebeus
Thanks for your great work on this over the last week or two. Must have taken a considerable amount of time and effect. Really appreciated.

@AndreyNikiforov AndreyNikiforov merged commit 87d3394 into icloud-photos-downloader:master Dec 19, 2023
26 checks passed
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