learningtapestry / canvas-google-drive-connector

An open source plugin for Instructure Canvas to connect to Google Drive.
GNU General Public License v3.0
7 stars 4 forks source link

GDrive scope #26

Closed joehobson closed 6 years ago

joehobson commented 6 years ago

I submitted the APS GDrive app to Google for verification and approval, and their response was as follows:

As a general rule, choose the most restrictive scope possible, as users more readily grant access to limited, clearly described scopes. Thus we recommend you to consider using the following scopes that are necessary to implement your app's feature.

https://www.googleapis.com/auth/drive.readonly

Instead of following scopes that you have requested

https://www.googleapis.com/auth/drive

For reference, a full list of OAuth scopes can be found on the OAuth 2.0 Scopes page.

Please reply to this email with confirmation regarding above scopes, or provide a justification if the recommended scopes do not work for your app.

Considering that the initial functionality of the app doesn't make any changes to files or sharing, could we get away with readonly scope? If not, what is our justification?

jwrubel commented 6 years ago

I think because the app doesn't allow users to upload or create new Google Drive docs, only associate with existing ones, read only mode should be fine. @andersoncardoso does that sound correct to you?

andersoncardoso commented 6 years ago

I changed to readonly and deployed to canvas.navigationnorth.com

joehobson commented 6 years ago

I thought we had this fixed, but I can't seem to get the app to just use the gdrive.readonly scope. It seems like that this is due to either cached data (possibly in Redis or the db), or cached assets not clearing out, or... i'm out of ideas.

On canvas.navigationnorth.com, running the app from that local server (/var/canvas/google-drive-connector) with latest code on master branch. When authorizing, here's what I get:

gdrive connector scope

andersoncardoso commented 6 years ago

not sure why this is happening. On the code the only scope being passed is the readonly https://github.com/learningtapestry/canvas-google-drive-connector/blob/master/lib/google_auth.rb#L11 We save the credentials for the authorized users on redis. We could clean it (everyone would have to authorize again), but I don't think this would solve given that the screenshot is for a new authorization. I'll keep inspecting this.

joehobson commented 6 years ago

yeah, i saw the scope change with the last commits so I really wasn't sure what was going on. looking at the logs it also looks like it's sending the readonly scope (and not both). I did try bouncing the redis server last night to clear out any cached data there, but that didn't fix it for me.

Are you seeing the same auth scopes when you try it on your dev environment for a new authorization?

One other thing to keep in mind with the redis -- we have the redis server configured to disable "save to disk" since Canvas only uses redis for temporarily caching data, and not for saving any permanent data (and the saving caused issues on the server). It would be better if we could save the credentials to postgres, although I can also see reason to only have that data saved temporarily (we don't restart those servers very often anyway)

andersoncardoso commented 6 years ago

@joehobson I tried locally and got the same.

After further checking this, I found that the google apis itself are caching/persisting previously required permissions. I.e: since we requested full permissions before, for the same project id, and now changed to require only 'read', it shows both. To test this I created a new project on google, and added the new credentials on my local instance, and as you can see on the image below the popup shows just the "readonly" scope. So I don't know if google expires these previous permissions at some point or we have to actually create a new project. I sent a issue questioning this on the google' s official ruby-api project on github.

As for the credential store we are using one of the options google provides by default (json file or redis). We could create a custom store class to use postgres, no problem in that. But this kind of data is meant to expire and refresh from time to time. In case the redis instance is cleared, which should not be often, the users would only have to click again the authorization popup. Maybe it's not a big usage problem, what you think? If it is, I can implement a different credential store engine, instead using one from google's lib.

lti-gdrive

joehobson commented 6 years ago

@andersoncardoso thanks for the follow-up. I just emailed Google (CC'd you) to see if there's any way to dump the project scope cache.

I also found that if i manually run the request with "include_granted_scopes" set to false, then it goes through without the old scope. Can you see any way to construct the WebUserAuthorizer with that option set to false? I don't really know Ruby well enough, but I didn't see a way to do that with their lib. It does appear to be an option under additional_parameters, but maybe it has to be done after the client is created, see -- https://developers.google.com/api-client-library/ruby/auth/web-app#creatingclient

As for the credential store, let's keep it at Redis. I agree that the credentials should expire and refresh on occasion. I don't think re-authorizing is an undo burden on the user.

joehobson commented 6 years ago

I finally got a reply back from Google... a link to support options, including purchasing support.

But... i also found that logging in to a different account does not show the old scope, and on my try today I was not shown the unverified app screen.

Based on that, and the fact that this is a transitional issue and shouldn't affect anyone else deploying the app, I don't think we should change anything in the code so I'm closing the issue