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

Dropbox: Retrieves & uses refresh token to obtain new access token #1274

Merged
merged 11 commits into from Oct 27, 2022

Conversation

DougReeder
Copy link
Contributor

@DougReeder DougReeder commented Oct 1, 2022

Fixes #1267

It may prove helpful to examine each commit separately.

The first commit adds Mocha tests for every scenario that jaribu tests, but re-arranged more logically. Some tests are pending, and some expectations are commented out (such as returning proper MIME types), because the current Dropbox backend is not compliant.

The second commit adds Content-Length & Last-Modified to Dropbox folder listings, correctly encodes Dropbox-API-Arg header and fixes some other bugs. The Dropbox-API-Arg header must be JSON-encoded, not URL-encoded: https://www.dropbox.com/developers/reference/json-encoding

The third commit refactors the function that calls either fetch or XHR into its own class. Authorize needs to call this as part of OAuth2 PKCE, and it was poor architecture for Dropbox and Google Drive to be using a function in wireclient.

The fourth commit refactors WireClient, GoogleDrive & Dropbox to have a common ancestor & Typescript interface. The common ancestor reduces duplication of code. The interface surfaces the complexity of interaction between the backends and other modules.

The fifth commit avoids the user needing to log in every four hours (what Dropbox calls "offline access" and OAuth2 calls the PKCE flow). OAuth2 PKCE is standardized, so I've placed as much code as possible in authorize.ts for future re-use.

It also sends the scopes required for Scoped apps, which Dropbox requires all new apps to be.

It also handles the 503 and 429 status codes and their Retry-After header that Dropbox sends.

The sixth commit removes the jaribu tests of new functionality that were superseded by better mocha tests.

DougReeder added a commit to DougReeder/remotestorage.js that referenced this pull request Oct 2, 2022
@DougReeder DougReeder requested a review from a team October 16, 2022 16:00
Copy link
Member

@raucao raucao left a comment

Choose a reason for hiding this comment

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

Fantastic work! 👏

I left a few comments for minor questions and nitpicks.

There's a breaking change for app developers with existing, older Dropbox apps, which we need to document (perhaps in a how-to-upgrade post on the forums and/or extended release notes):

Dropbox only introduced scopes fairly recently, and this PR requires the Dropbox app/client id to be configured with the new scopes. Otherwise authorization will fail:

Screenshot from 2022-10-20 11-56-53

The developer needs to go to https://www.dropbox.com/developers/apps, where there is already a message waiting for apps not using scopes to migrate the permissions:

Screenshot from 2022-10-20 12-13-09

On the permissions/scopes screen, one should deselect the permissions for contacts and file requests, then hit the migrate button:

Screenshot from 2022-10-20 12-14-38

After doing this, I was able to test these changes successfully with the RS Widget (which will now fail to authorize Dropbox until the widget is updated to use this rs.js changeset as well).

src/dropbox.ts Outdated Show resolved Hide resolved
src/dropbox.ts Outdated Show resolved Hide resolved
src/dropbox.ts Show resolved Hide resolved
src/dropbox.ts Outdated Show resolved Hide resolved
src/requests.ts Outdated Show resolved Hide resolved
src/Remote.ts Outdated Show resolved Hide resolved
doc/getting-started/dropbox-and-google-drive.rst Outdated Show resolved Hide resolved
Co-authored-by: Râu Cao <842+raucao@users.noreply.github.com>
@DougReeder
Copy link
Contributor Author

I believe the sharing.write permission isn't needed, just account_info.read files.content.read files.content.write files.metadata.read files.metadata.write

@DougReeder DougReeder force-pushed the feature/1267-dropbox_refresh_tokens branch from a688512 to 66e92c4 Compare October 22, 2022 03:29
@raucao raucao merged commit a4139c8 into master Oct 27, 2022
@raucao raucao deleted the feature/1267-dropbox_refresh_tokens branch October 27, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Dropbox code needs to be updated to use refresh tokens
2 participants