Differential D9806

KIOGDrive::copy - resolve the destGDriveUrl's new parent into a destDirId before passing it to FileCopyJob in src/kio_gdrive.cpp
ClosedPublic
Actions

Authored by martijnschmidt on Jan 10 2018, 9:52 PM.

Details

Reviewers
elvisangelaccio
dvratil
Group Reviewers
KIO GDrive
Commits
R219:f89c3f7fc4eb: KIOGDrive::copy - resolve the destGDriveUrl's new parent into a destDirId…
Summary

KIOGDrive::copy doesn't resolve the destGDriveUrl's new parent into a destDirId before passing it to FileCopyJob, which means it'll be sending an illegal request body to the Google Drive API - for example:

{  
   \"editable\":false,
   \"kind\":\"drive#file\",
   \"lastViewedByMeDate\":   \"2018-01-10T19:38:11   Z\",
   \"modifiedDate\":   \"2018-01-10T19:38:11   Z\",
   \"parents\":[  
      {  
         \"id\":\"testfoldertwo\"
      }
   ],
   \"title\":\"yetanothertestfile.txt\"
}

The "id" field should contain a valid parent ID rather than an originalFilename, and this patch resolves the right parent ID in one of two ways:

  • If the target folder of the copy command is the root of the GDrive, resolve the rootFolderId from the destAccountId.
  • If the target folder of the copy command is any other directory on the GDrive, resolve the fileId of the destGDriveUrl's parentPath.

The resulting parent ID is passed to FileCopyJob as the parent of the new cloned file, which then successfully processes the API call.

BUG: 376735
FIXED-IN: 1.2.2

Test Plan

This bug is triggered whenever KIOGDrive::copy is called:

  1. Try to copy a file from a random folder on the GDrive to the root folder of the GDrive.
  1. Try to copy a file from a random folder on the GDrive to another random non-root folder on the GDrive.
  1. Try to copy a file from a random folder on the GDrive to the same random folder on the GDrive, which creates another fileId with an identical originalFilename within that same folder.

In all three cases, the FileCopyJob should succeed rather than throwing an error because the destination doesn't exist.

Copying a file from a non-GDrive source into a destination on the GDrive uses KIOGDrive::put + KIOGDrive::putCreate, and copying a file from a GDrive source into a non-GDrive destination uses KIOGDrive::get, which is why the bug isn't triggered in these scenarios.

Diff Detail

Repository
R219 KIO GDrive
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
martijnschmidt requested review of this revision.Jan 10 2018, 9:52 PM
martijnschmidt created this revision.
elvisangelaccio added a subscriber: elvisangelaccio.Feb 10 2018, 4:44 PM

I'm not sure I'm following: all the 3 steps in the test plan work for me, without this patch.
Is there something else needed to trigger the bug?

martijnschmidt added a comment.Feb 11 2018, 12:37 PM
In D9806#203992, @elvisangelaccio wrote:

I'm not sure I'm following: all the 3 steps in the test plan work for me, without this patch.
Is there something else needed to trigger the bug?

Perhaps I should have been a bit clearer - this bug is triggered for me whenever KIOGDrive::copy is called, for example when I try to copy a file from let's say source gdrive:/accountname/testfolder/yetanothertestfile.txt to any destination on the same GDrive account. Valid destinations could be gdrive:/accountname/, or gdrive:/accountname/testfoldertwo/, or the same folder as the source e.g. gdrive:/accountname/testfolder/. You can see that the request body contains an incorrect id when LibKGAPI debugging output is turned on, please note I censored my fileId in the URL called by the API on the 2nd line of the output:

org.kde.kgapi.raw: "{\"editable\":false,\"kind\":\"drive#file\",\"lastViewedByMeDate\":\"2018-01-16T19:53:30Z\",\"modifiedDate\":\"2018-01-16T19:44:01Z\",\"parents\":[{\"id\":\"testfoldertwo\"}],\"title\":\"yetanothertestfile.txt\"}"
org.kde.kgapi: Received reply from QUrl("https://www.googleapis.com/drive/v2/files/SOURCEFILEIDGOESHERE/copy?convert=false&ocr=false&pinned=false")
org.kde.kgapi: Status code:  404
org.kde.kgapi.raw: "{\n \"error\": {\n  \"errors\": [\n   {\n    \"domain\": \"global\",\n    \"reason\": \"notFound\",\n    \"message\": \"File not found: testfoldertwo\",\n    \"locationType\": \"other\",\n    \"location\": \"file\"\n   }\n  ],\n  \"code\": 404,\n  \"message\": \"File not found: testfoldertwo\"\n }\n}\n"
org.kde.kgapi: Requested resource does not exist
org.kde.kgapi.raw: "{\n \"error\": {\n  \"errors\": [\n   {\n    \"domain\": \"global\",\n    \"reason\": \"notFound\",\n    \"message\": \"File not found: testfoldertwo\",\n    \"locationType\": \"other\",\n    \"location\": \"file\"\n   }\n  ],\n  \"code\": 404,\n  \"message\": \"File not found: testfoldertwo\"\n }\n}\n"
org.kde.kgapi: 
org.kde.kgapi: 
kf5.kio.core: finished() called after error()! Please fix the "kio_gdrive" KIO slave

Copying a file from a non-GDrive source into a destination on the GDrive uses KIOGDrive::put + KIOGDrive::putCreate, and copying a file from a GDrive source into a non-GDrive destination uses KIOGDrive::get, which is why the bug isn't triggered in these scenarios.

martijnschmidt edited the test plan for this revision. (Show Details)Feb 11 2018, 12:50 PM
elvisangelaccio added a comment.Feb 13 2018, 10:30 PM

Ah right, so this is https://bugs.kde.org/show_bug.cgi?id=376735

Thanks for the patch, I'll try to have a look this weekend.

elvisangelaccio added a comment.EditedFeb 17 2018, 6:54 PM

The patch seems to fix the problem, great!
Code looks good besides the inline comment.

src/kio_gdrive.cpp
870

What if destDirId is empty?
In that case we should probably call error(KIO::ERR_DOES_NOT_EXIST, ...)

elvisangelaccio edited the summary of this revision. (Show Details)Feb 17 2018, 6:55 PM
martijnschmidt added a comment.Mar 20 2018, 11:28 PM

@elvisangelaccio was this one committed, since it mentions FIXED-IN: 1.2.2 in the Differential summary but I don't see the commit in the Differential log?

I only just noticed your extra comment about the empty destDirId, I'm not sure we would run into such a condition since it is not a direct input variable? We resolve it based on the destGDriveUrl that we obtained from the KIOGDrive::copy function.

elvisangelaccio accepted this revision.Mar 21 2018, 8:04 PM
In D9806#230386, @martijnschmidt wrote:

@elvisangelaccio was this one committed, since it mentions FIXED-IN: 1.2.2 in the Differential summary but I don't see the commit in the Differential log?

No, I just added the fixed-in tag in what will become the commit message.

I only just noticed your extra comment about the empty destDirId, I'm not sure we would run into such a condition since it is not a direct input variable? We resolve it based on the destGDriveUrl that we obtained from the KIOGDrive::copy function.

Right, it should be fine then. Thanks again for the patch!

This revision is now accepted and ready to land.Mar 21 2018, 8:04 PM
Closed by commit R219:f89c3f7fc4eb: KIOGDrive::copy - resolve the destGDriveUrl's new parent into a destDirId… (authored by martijnschmidt, committed by elvisangelaccio). · Explain WhyMar 29 2018, 9:28 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Msrc/kio_gdrive.cpp (12 lines)

Diff 30886

View Options

src/kio_gdrive.cpp

Loading...