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

Partial compatibility with Google security update 2021 #350

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

agrenott
Copy link
Contributor

Google Drive added a new "resourceKey" attribute required to access documents shared by links. This resourceKey must be passed through HTTP header, aside with the document ID. resourceKey can be retrieved from a previous list operation on containing folder. Partial implementation; only for basic methods: GetContentFile, GetContentIOBuffer and FetchMetadata.

Ref.

Few remarks:

  • As the unit tests of PyDrive2 rely on actual Google Drive API usage, I've used an existing public document which I know is impacted. It could be smaller though...
  • I only implemented the basic read operations because they're easy to test with a read-only public file. I'm not sure we can create a file requiring resourceKey using the API (as I understand it's for files shared by link only), and don't see how we can simply/safely use a publicly writable shared document. And let's be honest, I only need one of them :)
  • At a first glance I feel GetContentFile could be refactored to rely on GetContentIOBuffer, avoiding some code duplication

@shcheklein
Copy link
Member

thanks @agrenott ! Please check linter / tests failures. Also, I don't think we can rely on an external file in tests. We need to do some research and create via API, if not possible, let me know how I can create a file / share a link manually. Let's create it in some account that is controlled via the same account that we use for tests.

@agrenott
Copy link
Contributor Author

agrenott commented Jul 3, 2024

Hi @shcheklein ,
I've fixed the linter issue. I'm a bit puzzled by the failing test, as it's specific to python 3.10 and doesn't seem directly related to my change. Let's see if it happens on next run.

Concerning the unit tests, sadly I can't find a way to create a new file impacted by the resourceKey requirement (neither via API nor manually in Drive UI). Googling a bit, I think it has only been used as a work-around on old files (pre-2021?) which were allocated a not-so-random ID.
rclone's source code seems to point into that direction as well: https://github.com/rclone/rclone/blob/master/backend/drive/drive.go#L659

So I don't have much better idea than relying on an existing public impacted file if we want to test those relying on the E2E usage of GDrive. Alternative would be to mock GDrive for those tests.

@shcheklein
Copy link
Member

thanks @agrenott !

Alternative would be to mock GDrive for those tests.

let's mock it then and put an actual file for a reference and some code to test it if needed

as it's specific to python 3.10 and doesn't seem directly related to my change. Let's see if it happens on next run.

I'll take a look and let you know. Sometimes it happens since we run too many API calls in parallel.

@agrenott
Copy link
Contributor Author

agrenott commented Jul 4, 2024

Mock implemented, and marked the real tests with as "manual", hope it's enough to disable them by default.

pydrive2/files.py Outdated Show resolved Hide resolved
Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

thanks @agrenott ! I've updated the GH actions a bit (bumped the versions). Tests pass. I've reviewed now the code itself and put some questions / comments - PTAL.

@agrenott
Copy link
Contributor Author

Hi @shcheklein, are you fine merging this PR in its current state or do you expect more changes?

@shcheklein
Copy link
Member

@agrenott I haven't had time to do the final review yet. Let me get back to it soon. I'll let you know. AFAIR it should be fine, but let me do the final pass.

pydrive2/files.py Outdated Show resolved Hide resolved
@shcheklein
Copy link
Member

@agrenott PTAL - just a few minor comments

Google Drive added a new "resourceKey" attribute required to access documents shared by links.
This resourceKey must be passed through HTTP header, aside with the document ID.
resourceKey can be retrieved from a previous list operation on containing folder.
Partial implementation; only for basic methods: GetContentFile, GetContentIOBuffer and FetchMetadata.
@shcheklein shcheklein merged commit a84dedd into iterative:main Jul 15, 2024
15 checks passed
@shcheklein
Copy link
Member

@agrenott merged and released. Thanks 🙏

@agrenott
Copy link
Contributor Author

You're welcome! It's nice to be able to contribute to some project I rely upon.

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.

None yet

2 participants