@uppy/companion: implement refresh for authentication tokens #4448

mifi · 2023-05-12T21:54:11Z

This PR implements refresh tokens for dropbox and google drive.

It works by

changing the logic when receiving a 401 from Companion, uppu will now call a new /:provider/refresh-token endpoint which will give uppy a new access token.
refresh_token is now stored inside uppy auth token along with access_token (encrypted jwt) for providers that give a refresh token. (previously only access_token was stored, now we store both as a JSON.stringify'd document)

NOTE: I also moved set-cookie from /send-token to /callback to make it more consistent with refresh-token. I'm not sure why it was in send-token in the first place, as you'd think /callback is a more suitable place, because /send-token's uppyAuthToken param can be forged by the user (e.g. user could probably inject a cookie themselves, possibly being a securit issue)

when reviewing it might make sense to look at individual commits.

inside uppy auth token

by creating a new superclass UploaderPlugin

from MiniXHRUpload

for dropbox and google drive closes #2721

when refreshing token

aduh95

We should probably keep this as private

packages/@uppy/tus/src/index.js

packages/@uppy/core/src/UploaderPlugin.js

packages/@uppy/companion-client/src/RequestClient.js

packages/@uppy/aws-s3-multipart/src/index.js

packages/@uppy/companion/src/config/grant.js

Murderlon · 2023-05-22T10:29:19Z

packages/@uppy/core/src/UploaderPlugin.js

@@ -0,0 +1,22 @@
+import BasePlugin from './BasePlugin.js'
+
+export default class UploaderPlugin extends BasePlugin {


I'm not sure if more inheritance makes sense here. Why can't everything regarding remote uploads live in companion client? Or can't this be solved with composition instead of inheritance?

Because it needs to access stuff from this: this.connectToServerSocket and this.uppy as well as queueRequestSocketToken, all of wihch live inside a subclass.

By "companion client" I assume you mean RequestClient. Maybe we could refactor it into there, but I think I tried to do that and it turns out to be a bigger task, because all code that it depends on should also be moved in there then. So maybe not in this PR?

I'm just afraid "maybe not in this PR" turns into never doing it. We're keep running into issues of duplication between uploaders. If we plan to tackle that with this in mind we can leave it as is :)

Hopefully we are going to do it, because all the uploaders share lots of code so all in likelihood we are going to eventually come back to that code.

to workaround expiry

Co-authored-by: Antoine du Hamel <[email protected]>

# Conflicts: # packages/@uppy/aws-s3-multipart/src/index.js # packages/@uppy/aws-s3/src/MiniXHRUpload.js # packages/@uppy/tus/src/index.js # packages/@uppy/xhr-upload/src/index.js

aduh95

There are git conflicts to solve it seems

aduh95 · 2023-06-19T16:24:18Z

packages/@uppy/companion/src/server/helpers/jwt.js

+// they would retry the failed tiles, however now the Uppy auth token has expired and
+// even though the provider refresh token would still have been accepted and
+// there's no way for them to retry their failed files.
+// With 400 days, there's still a theoretical possibility but very low.


You don't say 😅

aduh95 · 2023-06-19T16:31:02Z

packages/@uppy/core/src/UploaderPlugin.js

@@ -0,0 +1,22 @@
+import BasePlugin from './BasePlugin.js'
+
+export default class UploaderPlugin extends BasePlugin {


Hopefully we are going to do it, because all the uploaders share lots of code so all in likelihood we are going to eventually come back to that code.

…tokens # Conflicts: # packages/@uppy/aws-s3/src/index.js

packages/@uppy/companion-client/src/Provider.js

Co-authored-by: Antoine du Hamel <[email protected]>

* main: @uppy/aws-s3-multipart: fix Golden Retriever integration (#4526) examples/aws-nodejs: merge multipart and non-multipart examples (#4521) @uppy/companion: bump semver from 7.3.7 to 7.5.3 (#4529) @uppy/aws-s3-multipart: add types to internal fields (#4535) examples/aws-nodejs: update README (#4534) examples/aws-nodejs: showcase an example without preflight requests (#4516) @uppy/aws-s3-multipart: fix pause/resume (#4523) @uppy/status-bar: fix ETA when Uppy recovers its state (#4525) @uppy/aws-s3-multipart: fix resume single-chunk multipart uploads (#4528) @uppy/companion: fix part listing in s3 (#4524) example/aws-php: make it forward-compatible with the next Uppy major (#4522) @uppy/golden-retriever: refactor to modernize the codebase (#4520) examples/aws-nodejs: upgrade to AWS-SDK v3 (#4515) @uppy/companion: implement refresh for authentication tokens (#4448) @uppy/aws-s3-multipart: disable pause/resume for remote uploads in the UI (#4500) @uppy/tus: retry on 423 HTTP error code (#4512)

| Package | Version | Package | Version | | ---------------------- | ------- | ---------------------- | ------- | | @uppy/aws-s3 | 3.2.1 | @uppy/golden-retriever | 3.1.0 | | @uppy/aws-s3-multipart | 3.4.1 | @uppy/status-bar | 3.2.1 | | @uppy/companion | 4.6.0 | @uppy/tus | 3.1.2 | | @uppy/companion-client | 3.2.0 | @uppy/xhr-upload | 3.3.1 | | @uppy/core | 3.3.0 | uppy | 3.11.0 | - @uppy/companion: fix infinite recursion in uploader test (Mikael Finstad / #4536) - @uppy/xhr-upload: export `Headers` type (Masum ULU / #4549) - @uppy/aws-s3-multipart: increase priority of abort and complete (Stefan Schonert / #4542) - @uppy/aws-s3: fix remote uploads (Antoine du Hamel / #4546) - meta: use `corepack yarn` instead of `npm` to launch E2E (Antoine du Hamel / #4545) - @uppy/aws-s3-multipart: fix upload retry using an outdated ID (Antoine du Hamel / #4544) - @uppy/status-bar: remove throttled component (Artur Paikin / #4396) - @uppy/aws-s3-multipart: fix Golden Retriever integration (Antoine du Hamel / #4526) - examples/aws-nodejs: merge multipart and non-multipart examples (Antoine du Hamel / #4521) - @uppy/companion: bump semver from 7.3.7 to 7.5.3 (dependabot[bot] / #4529) - @uppy/aws-s3-multipart: add types to internal fields (Antoine du Hamel / #4535) - examples/aws-nodejs: update README (Antoine du Hamel / #4534) - examples/aws-nodejs: showcase an example without preflight requests (Antoine du Hamel / #4516) - @uppy/aws-s3-multipart: fix pause/resume (Antoine du Hamel / #4523) - @uppy/status-bar: fix ETA when Uppy recovers its state (Antoine du Hamel / #4525) - @uppy/aws-s3-multipart: fix resume single-chunk multipart uploads (Antoine du Hamel / #4528) - @uppy/companion: fix part listing in s3 (Antoine du Hamel / #4524) - example/aws-php: make it forward-compatible with the next Uppy major (Antoine du Hamel / #4522) - @uppy/golden-retriever: refactor to modernize the codebase (Antoine du Hamel / #4520) - examples/aws-nodejs: upgrade to AWS-SDK v3 (Antoine du Hamel / #4515) - @uppy/companion: implement refresh for authentication tokens (Mikael Finstad / #4448) - @uppy/aws-s3-multipart: disable pause/resume for remote uploads in the UI (Artur Paikin / #4500) - @uppy/tus: retry on 423 HTTP error code (Antoine du Hamel / #4512)

New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. uppyAuthToken initially used to simply contain an encrypted & json encoded OAuth2 access_token for a specific provider. Then we added refresh tokens as well inside uppyAuthToken #4448. Now we also allow storing other state or parameters needed for that specific provider, like username, password, host name, webdav URL etc... This is needed for providers like webdav, ftp etc, where the user needs to give some more input data while authenticating Companion: - `providerTokens` has been renamed to `providerUserSession` because it now includes not only tokens, but a user's session with a provider. Companion `Provider` class: - New `hasSimpleAuth` static boolean property - whether this provider uses simple auth - uppyAuthToken expiry default 24hr again for providers that don't support refresh tokens - make uppyAuthToken expiry configurable per provider - new `authStateExpiry` static property (defaults to 24hr) - new static property `grantDynamicToUserSession`, allows providers to specify which state from Grant `dynamic` to include into the provider's `providerUserSession`.

* remove useless line * fix broken cookie removal logic related #4426 * fix mime type of thumbnails not critical but some browsers might have problems * simplify/speedup token generation so we don't have to decode/decrypt/encode/encrypt so many times * use instanceof instead of prop check * Implement alternative provider auth New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. uppyAuthToken initially used to simply contain an encrypted & json encoded OAuth2 access_token for a specific provider. Then we added refresh tokens as well inside uppyAuthToken #4448. Now we also allow storing other state or parameters needed for that specific provider, like username, password, host name, webdav URL etc... This is needed for providers like webdav, ftp etc, where the user needs to give some more input data while authenticating Companion: - `providerTokens` has been renamed to `providerUserSession` because it now includes not only tokens, but a user's session with a provider. Companion `Provider` class: - New `hasSimpleAuth` static boolean property - whether this provider uses simple auth - uppyAuthToken expiry default 24hr again for providers that don't support refresh tokens - make uppyAuthToken expiry configurable per provider - new `authStateExpiry` static property (defaults to 24hr) - new static property `grantDynamicToUserSession`, allows providers to specify which state from Grant `dynamic` to include into the provider's `providerUserSession`. * refactor * use respondWithError also for thumbnails for consistency * fix prepareStream it wasn't returning the status code (like `got` does on error) it's needed to respond properly with a http error * don't throw when missing i18n key instead log error and show the key this in on par with other i18n frameworks * fix bugged try/catch * allow aborting login too and don't replace the whole view with a loader when plugin state loading it will cause auth views to lose state an inter-view loading text looks much more graceful and is how SearchProviderView works too * add json http error support add support for passing objects and messages from companion to uppy this allows companion to for example give a more detailed error when authenticating * don't tightly couple auth form with html form don't force the user to use html form and use preact for it, for flexibility * fix i18n * make contentType parameterized * allow sending certain errors to the user this is useful because: // onedrive gives some errors here that the user might want to know about // e.g. these happen if you try to login to a users in an organization, // without an Office365 licence or OneDrive account setup completed // 400: Tenant does not have a SPO license // 403: You do not have access to create this personal site or you do not have a valid license * sending certain onedrive errors to the user this is useful because: // onedrive gives some errors here that the user might want to know about // e.g. these happen if you try to login to a users in an organization, // without an Office365 licence or OneDrive account setup completed // 400: Tenant does not have a SPO license // 403: You do not have access to create this personal site or you do not have a valid license * don't have default content-type * make a loginSimpleAuth api too * make removeAuthToken protected (cherry picked from commit 4be2b6f) * fix lint * run yarn format * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> * fix broken merge conflict * improve inheritance * fix bug * fix bug with dynamic grant config * use duck typing for error checks see discussion here: #4619 (comment) * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> * fix broken lint fix script * fix broken merge code * try to fix flakey tets * fix lint --------- Co-authored-by: Antoine du Hamel <[email protected]>

* remove useless line * fix broken cookie removal logic related #4426 * fix mime type of thumbnails not critical but some browsers might have problems * simplify/speedup token generation so we don't have to decode/decrypt/encode/encrypt so many times * use instanceof instead of prop check * Implement alternative provider auth New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. uppyAuthToken initially used to simply contain an encrypted & json encoded OAuth2 access_token for a specific provider. Then we added refresh tokens as well inside uppyAuthToken #4448. Now we also allow storing other state or parameters needed for that specific provider, like username, password, host name, webdav URL etc... This is needed for providers like webdav, ftp etc, where the user needs to give some more input data while authenticating Companion: - `providerTokens` has been renamed to `providerUserSession` because it now includes not only tokens, but a user's session with a provider. Companion `Provider` class: - New `hasSimpleAuth` static boolean property - whether this provider uses simple auth - uppyAuthToken expiry default 24hr again for providers that don't support refresh tokens - make uppyAuthToken expiry configurable per provider - new `authStateExpiry` static property (defaults to 24hr) - new static property `grantDynamicToUserSession`, allows providers to specify which state from Grant `dynamic` to include into the provider's `providerUserSession`. * refactor * use respondWithError also for thumbnails for consistency * fix prepareStream it wasn't returning the status code (like `got` does on error) it's needed to respond properly with a http error * don't throw when missing i18n key instead log error and show the key this in on par with other i18n frameworks * fix bugged try/catch * allow aborting login too and don't replace the whole view with a loader when plugin state loading it will cause auth views to lose state an inter-view loading text looks much more graceful and is how SearchProviderView works too * add json http error support add support for passing objects and messages from companion to uppy this allows companion to for example give a more detailed error when authenticating * don't tightly couple auth form with html form don't force the user to use html form and use preact for it, for flexibility * fix i18n * make contentType parameterized * allow sending certain errors to the user this is useful because: // onedrive gives some errors here that the user might want to know about // e.g. these happen if you try to login to a users in an organization, // without an Office365 licence or OneDrive account setup completed // 400: Tenant does not have a SPO license // 403: You do not have access to create this personal site or you do not have a valid license * make `authProvider` consistent always use the static property ignoring the instance propety fixes #4460 * fix bug * fix test also * don't have default content-type * make a loginSimpleAuth api too * make removeAuthToken protected (cherry picked from commit 4be2b6f) * fix lint * run yarn format * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> * fix broken merge conflict * improve inheritance * fix bug * fix bug with dynamic grant config * use duck typing for error checks see discussion here: #4619 (comment) * Apply suggestions from code review Co-authored-by: Antoine du Hamel <[email protected]> * fix broken lint fix script * fix broken merge code * try to fix flakey tets * fix lint * fix merge issue --------- Co-authored-by: Antoine du Hamel <[email protected]>

mifi added 13 commits May 9, 2023 15:41

allow storing multiple tokens

188ce1c

inside uppy auth token

de-duplicate uploadRemote

5b7fa1d

by creating a new superclass UploaderPlugin

pull out requestSocketToken

4ae8cce

from MiniXHRUpload

add class UploaderPlugin

7fc0c18

refactor

d9caa6e

refactor

e2e5f82

refactor/reuse

30d4735

refactor/make getAuthToken private

a78efc7

fix bug

cc6ee4c

implement refresh token

8c19821

for dropbox and google drive closes #2721

fix test

06b96d8

also update auth token cookie

2d77c4b

when refreshing token

fix build error on node 14

0aac0e5

arturi requested review from aduh95 and Murderlon May 16, 2023 13:48

aduh95 reviewed May 18, 2023

View reviewed changes

packages/@uppy/tus/src/index.js Outdated Show resolved Hide resolved

packages/@uppy/tus/src/index.js Outdated Show resolved Hide resolved

packages/@uppy/core/src/UploaderPlugin.js Outdated Show resolved Hide resolved

packages/@uppy/core/src/UploaderPlugin.js Outdated Show resolved Hide resolved

aduh95 reviewed May 18, 2023

View reviewed changes

packages/@uppy/companion-client/src/RequestClient.js Show resolved Hide resolved

Murderlon requested changes May 22, 2023

View reviewed changes

arturi assigned mifi Jun 6, 2023

mifi and others added 4 commits June 8, 2023 18:57

increase auth token expiry

33018dd

to workaround expiry

Update packages/@uppy/companion-client/src/RequestClient.js

e3cab05

Co-authored-by: Antoine du Hamel <[email protected]>

make queueRequestSocketToken private

8ceb987

rename arg

c98cf60

Murderlon changed the title ~~Refresh tokens~~ @uppy/companion: implement refresh for authentication tokens Jun 19, 2023

Merge branch 'main' into refresh-tokens

ea112c0

# Conflicts: # packages/@uppy/aws-s3-multipart/src/index.js # packages/@uppy/aws-s3/src/MiniXHRUpload.js # packages/@uppy/tus/src/index.js # packages/@uppy/xhr-upload/src/index.js

aduh95 reviewed Jun 19, 2023

View reviewed changes

mifi added 4 commits June 20, 2023 14:29

Merge commit '2b9958c4f8faee85b3c922555efe2c0109d7a2fb' into refresh-…

ef9e25e

…tokens # Conflicts: # packages/@uppy/aws-s3/src/index.js

fix lint

e4b6459

log error message

06d55a5

fix s3

e32ff5f

mifi requested review from Murderlon and aduh95 June 21, 2023 14:09

aduh95 approved these changes Jun 21, 2023

View reviewed changes

packages/@uppy/companion-client/src/Provider.js Show resolved Hide resolved

Update packages/@uppy/companion-client/src/Provider.js

68905bc

Co-authored-by: Antoine du Hamel <[email protected]>

Murderlon approved these changes Jun 22, 2023

View reviewed changes

mifi merged commit 2c75dda into main Jun 22, 2023

mifi deleted the refresh-tokens branch June 22, 2023 09:18

github-actions bot mentioned this pull request Jul 6, 2023

Release: [email protected] #4550

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/companion: implement refresh for authentication tokens #4448

@uppy/companion: implement refresh for authentication tokens #4448

mifi commented May 12, 2023

aduh95 left a comment

Murderlon May 22, 2023

mifi Jun 8, 2023

Murderlon Jun 15, 2023

aduh95 Jun 19, 2023

aduh95 left a comment

aduh95 Jun 19, 2023

aduh95 Jun 19, 2023

		@@ -0,0 +1,22 @@
		import BasePlugin from './BasePlugin.js'

		export default class UploaderPlugin extends BasePlugin {

@uppy/companion: implement refresh for authentication tokens #4448

@uppy/companion: implement refresh for authentication tokens #4448

Conversation

mifi commented May 12, 2023

aduh95 left a comment

Choose a reason for hiding this comment

Murderlon May 22, 2023

Choose a reason for hiding this comment

mifi Jun 8, 2023

Choose a reason for hiding this comment

Murderlon Jun 15, 2023

Choose a reason for hiding this comment

aduh95 Jun 19, 2023

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

aduh95 Jun 19, 2023

Choose a reason for hiding this comment

aduh95 Jun 19, 2023

Choose a reason for hiding this comment