walterpg / google-drive-sync

A KeePass Password Safe v2 plugin for synchronizing passwords to Google Drive files.
GNU General Public License v3.0
199 stars 10 forks source link

Use file scope as default scope #42

Open Hoffs opened 3 years ago

Hoffs commented 3 years ago

Is your feature request related to a problem? Please describe. Not exactly a problem, but I feel like giving full access to the google drive is not very safe. I do not see a good reason why extension should default to this method, besides the dev work required to make it use file scope.

Describe the solution you'd like I would prefer that by default the scope used by the extension would be just file as that grants per file access and mitigates risk of malicious extension deleting all your drive files and/or reading them.

https://www.googleapis.com/auth/drive.file | Per-file access to files created or opened by the app. File authorization is granted on a per-user basis and is revoked when the user deauthorizes the app.

Describe alternatives you've considered Using legacy method which allows use of custom oauth credentials and to choose file scope.

Additional context As I understand the default scope used is Drive which grants the extension access to all of the drive contents.

https://github.com/walterpg/google-drive-sync/blob/19ae4911228630ebad8ba1c1e695a3f19fb95748/src/GoogleDriveSyncExt.cs#L1403-L1415

https://www.googleapis.com/auth/drive | Full, permissive scope to access all of a user's files, excluding the Application Data folder.

Also I haven't really delved into how it would work with just file scope, but I imagine since it already has an option for it using legacy oauth method it shouldn't be too difficult to use it with non-legacy method.

This is also further supported by Google comments on scopes:

If you've developed a Drive app that uses any of the restricted scopes, we recommend migrating your app to use drive.file scope. This scope enables users to select the specific files from Google Drive, and through the Google Picker, that they want to allow your app to access.

walterpg commented 3 years ago

Hi! Thanks for this enhancement request.

file access was a workaround introduced in v4-alpha. The v3 app credentials had generally stopped working with drive.file scope. Using those credentials with file scope allowed the plugin to work, albeit with feature-breaking restrictions, especially for those of us that use multiple "sync" programs. So, when the v4 plugin was granted new app credentials by Google last year, the workaround was no longer necessary.

file scope is basically an app "sandboxing" scope. An app using it can't even check for the existence of files that it does not create. So, say I have a mydb.kdbx file that I manually uploaded to Drive via the web browser. Then I set the plugin to file scope. Unfortunately, the plugin can't sync to that mydb.kdbx file, because it's not in the plugin's sandbox. But say I go ahead and use the plugin to sync mydb.kdbx. The plugin won't be able to see the one I previously uploaded. It can only assume that it doesn't exist, so... it uploads (that is, creates) a new file to the sandbox. The plugin can now see and access the sandbox copy of course, but if you have a look at the drive.google.com, you will see two files with the same name. Which is which?? You can find out via the web "picker", etc., but it's a pain and leads to confusion.

Other apps face the same predicament. If you want interoperability with another app such as Keepass2Android, all such apps need to share the same scope. Unfortunately, that scope is either "all" (drive.file) or "just me" (file). In the above example, where two files with the same name are created, other apps using drive.file scope will get confused: "which file should I sync with?"

However, I encourage you to carefully "delve in" and experiment with the file scope feature, which, as you say, is currently supported via the legacy plugin credentials. I also appreciate your skepticism as regards the plugin's access to Drive. That's why I'm glad we still have this open source solution to be scrutinized by all for security issues.

file scope could be a useful and secure feature for someone that uses only this plugin to sync KeePass files, but at the moment I'm not convinced that it would solve more problems than it creates.

Hoffs commented 3 years ago

An app using it can't even check for the existence of files that it does not create

Yes, this would require to not only prompt the user to go through oauth2, but also then run the google drive file picker and let the user select the existing database file (if available), which then would grant the appropriate permissions to that file. So the extension, instead of blindly saving the file, would have to first prompt the user to select the database file. This could be confusing if user actually has a database created, but during the extensions configuration he didn't select it, then it would act as you mentioned, creating 2nd copy of the database. If any application would "replace" the file by recreating it (which would mean new file id) then this would also break, but that in itself feels like another issue.

file scope could be a useful and secure feature for someone that uses only this plugin to sync KeePass files, but at the moment I'm not convinced that it would solve more problems than it creates.

I guess, but the problems would mainly be from the confusion side (as explained above) and difficulty (changing existing users from existing setup to new one, unless guarded as optional, more secure way, instead of default one). I don't see how that would break Keepass2Android (there's similar issue there as well) since if it uses "drive" scope, it will be able to see the file created by this app in "file" scope, and if it uses "file" scope then it should be able to access and get permissions to the same file. At least that's how I understood the per file permissions with gdrive file picker. I also checked this with keepass2android, it could see the files created by another app that uses just file scope without issues.

But I do see why "drive" permission is used and how it would be less intuitive and maybe problematic to use "file" permission.