Update icloudpd code with latest from PyiCloud, add 2FA auth option #733

cfurrow · 2023-12-09T18:51:41Z

This is a work-in-progress (WIP) PR. Not ready for merging yet.

I've not followed all guidelines in https://github.com/icloud-photos-downloader/icloud_photos_downloader/blob/master/CONTRIBUTING.md#pull-request-process yet

Purpose

Start to debug #729 and gather information on a fix. It seems that the issues may be around the latest code from PyiCloud not being included into pyicloud-ipd.

Python is not my primary language
I'm not familiar with PyiCloud or icloudpd

A PR can be easier to review than diffs/patches or branches, so I'm creating a WIP PR to track my investigation progress.

Summary

Pulled down latest code changes from PyiCloud and inserted into the src/pyicloud_ipd directory.
Updated code paths to use new PyiCloud changes.
Added new code path for entering 2FA codes directly, without selecting SMS/2FA. There's apparently a non-trivial difference between the PyiCloud's treatment of 2SA (2-step auth) vs 2FA (2-factor auth). For my own setup, I required 2FA over 2SA.

TODO

Removed "domain" selection code, so this will only work with icloud.com, and not icloud.cn. Will need to address this in this PR, or in a follow-up PR.
Identify and fix broken/outdated automated tests (and VCR cassettes)
Add new automated tests for 2FA
Update USAGE/docs/etc to remove mentions of library selection (does not seem to be supported anymore)
Address/Remove any TODO comments I added.

Manual Testing

docker build . -t icloudpd_dev

docker run -it \
  --rm --name icloudpd \
  -v ~/icloudpd-download:/data \
  -e TZ=America/New_York \
  icloudpd_dev \
    icloudpd \
    --directory /data \
    --username [email protected] \
    --watch-with-interval 3600

2023-12-09 16:23:42 DEBUG    Authenticating...
iCloud Password:
2023-12-09 16:24:00 INFO     Two-factor authentication is required
Please enter two-factor authentication code: 123456
2023-12-09 16:24:07 WARNING  Failed to parse response with JSON mimetype
2023-12-09 16:24:10 INFO     Great, you're all set up. The script can now be run without user interaction until 2SA expires.
You can set up email notifications for when the two-step authentication expires.
(Use --help to view information about SMTP options.)
2023-12-09 16:24:13 DEBUG    Looking up all photos and videos from album All Photos...
2023-12-09 16:24:13 INFO     Downloading 81324 original photos and videos to /data ...
2023-12-09 16:24:16 DEBUG    Downloading /data/2023/12/09/IMG_1083.MOV...
2023-12-09 16:24:18 INFO     Downloaded /data/2023/12/09/IMG_1083.MOV
2023-12-09 16:24:18 DEBUG    Downloading /data/2023/12/09/IMG_1082.JPG...

...snipped...

src/icloudpd/authentication.py

AndreyNikiforov · 2023-12-09T19:08:16Z

I've also not followed the guidelines re: updating pyicloud_ipd:

If you need to make any changes to the pyicloud library, icloudpd uses a fork of this library that has been renamed to pyicloud-ipd. Please clone my pyicloud fork and check out the pyicloud-ipd branch. PRs should be based on the pyicloud-ipd branch and submitted to icloud-photos-downloader/pyicloud.

It is not longer the case, as you probably figured out yourself: pyicloud_ipd is just subfolder now, not a separate lib (I updated contribution doc). One option is to make it a lib again and reference original pyicloud.

Context: pyicloud was forked because original was dormant. Once work in original pyicloud resurrected, fork was never reconciled with original, mostly because all attempts were facing many broken tests in icloudpd.

cfurrow · 2023-12-09T19:11:09Z

It is not longer the case, as you probably figured out yourself: pyicloud_ipd is just subfolder now, not a separate lib (I updated contribution doc). One option is to make it a lib again and reference original pyicloud.

I wasn't sure if the other fork was used, and then copied into this subfolder. I can keep my changes as-is for now inside this repo's subfolder. Thanks!

Context: pyicloud was forked because original was dormant. Once work in original pyicloud resurrected, fork was never reconciled with original, mostly because all attempts were facing many broken tests in icloudpd.

Ugh, I bet. I've not gone back to look at the tests I've broken yet, as it seems like it'll be a pain once I get to that stage. VCR cassette updates, expectation updates, etc. That's a problem for future me 😉

cfurrow · 2023-12-09T20:09:18Z

Got a little further along after doing a copy/paste of upstream icloudpd files into the src/icloud_ipd folder.

Now my docker output is this after asking for password, then 2FA code. I entered in my email, password, and valid 2FA code after being prompted on my mac:

scripts/build_docker

docker run -it \
  --rm --name icloudpd \
  -v ~/icloudpd-download:/data \
  -e TZ=America/New_York \
  icloudpd/icloudpd:latest \
    icloudpd \
    --directory /data \
    --username [email protected] \
    --watch-with-interval 3600

2023-12-09 15:05:57 DEBUG    Authenticating...
iCloud Password:
2023-12-09 15:06:02 INFO     Two-step/two-factor authentication is required
  0: SMS to *******
  1: Enter two-factor authentication code
Please choose an option: [0]: 1
Please enter two-factor authentication code: 123456 
2023-12-09 15:06:10 ERROR    Not Found (404)
2023-12-09 15:06:10 ERROR    Session trust failed.
2023-12-09 15:06:10 ERROR    Failed to verify two-factor authentication code

cfurrow · 2023-12-09T21:28:40Z

Ok, some more progress:

docker build . -t icloudpd_test

docker run -it \
  --rm --name icloudpd_test \
  -v ~/icloudpd-download:/data \
  -e TZ=America/New_York \
  icloudpd/icloudpd:latest \
    icloudpd \
    --directory /data \
    --username [email protected] \
    --watch-with-interval 3600

2023-12-09 16:23:42 DEBUG    Authenticating...
iCloud Password:
2023-12-09 16:24:00 INFO     Two-factor authentication is required
Please enter two-factor authentication code: 123456
2023-12-09 16:24:07 WARNING  Failed to parse response with JSON mimetype
2023-12-09 16:24:10 INFO     Great, you're all set up. The script can now be run without user interaction until 2SA expires.
You can set up email notifications for when the two-step authentication expires.
(Use --help to view information about SMTP options.)
2023-12-09 16:24:13 DEBUG    Looking up all photos and videos from album All Photos...
2023-12-09 16:24:13 INFO     Downloading 81324 original photos and videos to /data ...
2023-12-09 16:24:16 DEBUG    Downloading /data/2023/12/09/IMG_1083.MOV...
2023-12-09 16:24:18 INFO     Downloaded /data/2023/12/09/IMG_1083.MOV
2023-12-09 16:24:18 DEBUG    Downloading /data/2023/12/09/IMG_1082.JPG...

...then I stopped it with Control-C...

specify log level with --log (default is WARNING) allow passing of individual test files

…/base.py this removes the domain logic to switch between China and US icloud domains, which will have to be added back in later

… arguments updated arg list to remove domain, and set password=password.

if 2fa is required, that is not the same as 2sa being required.

…ibrary selection.

required to filter if photo or movie, etc

cfurrow · 2023-12-09T21:35:10Z

scripts/test

-    [[ $1 = /* ]] && echo "$1" || echo "$PWD/${1#./}"
+	[[ $1 = /* ]] && echo "$1" || echo "$PWD/${1#./}"


My editor and I will have words about this... Ugh. Not quite sure what is off here for my tabs/spaces/whatever. Does not look this bad in my editor.

cfurrow · 2023-12-09T21:37:31Z

src/icloudpd/authentication.py

@@ -50,6 +57,22 @@ def authenticate_(
    return authenticate_


+def request_2fa(icloud: pyicloud_ipd.PyiCloudService, logger: logging.Logger):


This is new, and significant. Before this was here, the request_2sa codepath was executed, and I'd select that I wanted a 2fa sent to my devices vs an SMS message. I'd get the 2FA prompt on my laptop, and enter the 2FA code into the icloudpd prompt, but it would always fail to validate.

But, after I added this code, it seems to work as I'd expect. I enter my password, and get immediately prompted on my macos to validate the new device, and then given a 2FA code that I enter in to icloudpd, which then validates it and starts downloading photos/videos.

I do not completely understand the difference in API calls between 2sa and 2fa, but, it's significant enough to get it to pass one way (2fa) and fail the other (2sa). 🤷🏻

cfurrow · 2023-12-09T21:38:04Z

src/icloudpd/authentication.py

@@ -87,7 +110,7 @@ def request_2sa(icloud: pyicloud_ipd.PyiCloudService, logger: logging.Logger):

    code = click.prompt("Please enter two-factor authentication code")
    if not icloud.validate_verification_code(device, code):
-        logger.error("Failed to verify two-factor authentication code")
+        logger.error(f"Failed to verify two-factor authentication code for device \"{device.get('deviceName')}\"")


I'll remove this extra device info--I added for debugging, but it wasn't helpful because it was returning None.

cfurrow · 2023-12-09T21:39:03Z

src/icloudpd/base.py

-        libraries_dict = icloud.photos.libraries
-        library_names = libraries_dict.keys()
-        print(*library_names, sep="\n")
+    # TODO: selecting library not supported anymore! Need to adjust USAGE, docs, etc


This is another significant change. The latest PyiCloud does not support libraries, so we have to use the default photos.all. I'll need to update any CLI args/USAGE docs to remove the ability to select libraries, as it's not supported anymore... Or so it would seem.

This is actually a case where the pyicloud_ipd fork added functionality that was never implemented in the original 'pyicloud' project. There is PR picklepete/pyicloud#407 to merge this into the 'pyicloud' project, but that seems to be a bit of a dead project now. There is a bit of cross merging that needs to occur to truly get the latest features on both sides. I wouldn't necessarily remove this feature from this project.

I wondered if it was a patch added after the fact.

The intention was not to leave out all of the things I removed (domain selection, library selection, etc), the intention was to address the bug first, and then figure out what we can do about the rest once the project was in a more working state again.

cfurrow · 2023-12-09T21:57:31Z

Stepping away for now.
Open to ideas/help in fixing tests, and adding back support for library selection, domain selection (.com/.cn), etc.

Regarding domains, that may be something that could be nice to include upstream in PyiCloud, and not have to patch it here. Maybe they've already discussed it there, and decided against supporting both.

cfurrow commented Dec 9, 2023

View reviewed changes

src/icloudpd/authentication.py Show resolved Hide resolved

cfurrow force-pushed the update-icloudpd-code-with-latest branch from 5f5a83f to b89b99e Compare December 9, 2023 19:08

cfurrow force-pushed the update-icloudpd-code-with-latest branch 2 times, most recently from 97fa083 to bb7ba68 Compare December 9, 2023 20:06

cfurrow added 8 commits December 9, 2023 16:31

improvement: allow scripts/test to accept more pytest args

4059f54

specify log level with --log (default is WARNING) allow passing of individual test files

update: paste latest pyicloud class definiotions into src/pycloud_ipd…

e3492c7

…/base.py this removes the domain logic to switch between China and US icloud domains, which will have to be added back in later

fix: update icloudpd/authentication.py to use correct PyiCloudService…

387b426

… arguments updated arg list to remove domain, and set password=password.

fix: update exception names

2625adb

update: bring in upstream changes from pyicloud

3fe0b68

fix: treat 2FA requirement separately from 2SA requirement

7688fff

if 2fa is required, that is not the same as 2sa being required.

fix: PyiCloud Photos service no longer supports "libraries". Remove l…

2c4546f

…ibrary selection.

fix: add PhotoAsset.item_type back

c706dc5

required to filter if photo or movie, etc

cfurrow force-pushed the update-icloudpd-code-with-latest branch from cfd9b6b to c706dc5 Compare December 9, 2023 21:34

cfurrow commented Dec 9, 2023

View reviewed changes

cfurrow added 2 commits December 9, 2023 16:47

todo: add mention of 'domain' removal

c2cf163

todo: more todo added

49ae5c4

cfurrow added 2 commits December 9, 2023 16:58

format code

626cd50

Update changelog with unreleased notes

3e1771d

cfurrow changed the title ~~Update icloudpd code with latest from PyiCloud~~ Update icloudpd code with latest from PyiCloud, add 2FA auth option Dec 9, 2023

cfurrow mentioned this pull request Dec 9, 2023

Can no longer log in: Bad username or password #729

Closed

scaraebeus mentioned this pull request Dec 10, 2023

Authentication Fix: Merge updated auth flow from pyicloud #734

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update icloudpd code with latest from PyiCloud, add 2FA auth option #733

Update icloudpd code with latest from PyiCloud, add 2FA auth option #733

cfurrow commented Dec 9, 2023 •

edited

Loading

AndreyNikiforov commented Dec 9, 2023

cfurrow commented Dec 9, 2023

cfurrow commented Dec 9, 2023

cfurrow commented Dec 9, 2023 •

edited

Loading

cfurrow Dec 9, 2023 •

edited

Loading

cfurrow Dec 9, 2023

cfurrow Dec 9, 2023

cfurrow Dec 9, 2023

scaraebeus Dec 10, 2023 •

edited

Loading

cfurrow Dec 10, 2023

cfurrow commented Dec 9, 2023

		[[ $1 = /* ]] && echo "$1" \|\| echo "$PWD/${1#./}"
		[[ $1 = /* ]] && echo "$1" \|\| echo "$PWD/${1#./}"

		@@ -50,6 +57,22 @@ def authenticate_(
		return authenticate_


		def request_2fa(icloud: pyicloud_ipd.PyiCloudService, logger: logging.Logger):

Update icloudpd code with latest from PyiCloud, add 2FA auth option #733

Are you sure you want to change the base?

Update icloudpd code with latest from PyiCloud, add 2FA auth option #733

Conversation

cfurrow commented Dec 9, 2023 • edited Loading

Purpose

Summary

TODO

Manual Testing

AndreyNikiforov commented Dec 9, 2023

cfurrow commented Dec 9, 2023

cfurrow commented Dec 9, 2023

cfurrow commented Dec 9, 2023 • edited Loading

cfurrow Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

cfurrow Dec 9, 2023

Choose a reason for hiding this comment

cfurrow Dec 9, 2023

Choose a reason for hiding this comment

cfurrow Dec 9, 2023

Choose a reason for hiding this comment

scaraebeus Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

cfurrow Dec 10, 2023

Choose a reason for hiding this comment

cfurrow commented Dec 9, 2023

cfurrow commented Dec 9, 2023 •

edited

Loading

cfurrow commented Dec 9, 2023 •

edited

Loading

cfurrow Dec 9, 2023 •

edited

Loading

scaraebeus Dec 10, 2023 •

edited

Loading