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

Provider user sessions #4619

Merged
merged 38 commits into from
Dec 5, 2023
Merged

Provider user sessions #4619

merged 38 commits into from
Dec 5, 2023

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Aug 14, 2023

This PR brings some improvements, bux fixes, as well as the feature: Per-provider auth state as discussed in #4551 (comment) - this enables implementing providers like webdav+oauth, webdav(no auth) and ftp

  • Allow creating plugins without OAuth flows, but instead with simple auth flows (auth happens through companion)
  • Allow sending extra data along with simple auth flows that can be used by companion providers after auth success
  • Allow sending extra parameters along with OAuth flows that will be used during auth, and can be used after auth success (e.g. dynamic domain needed for webdav oauth)
  • Allow creating custom auth forms per provider, with preact components
  • New concept "simple auth" - authentication that happens immediately (in one http request) without redirecting to any third party. da67ac9
  • companion can send custom error data back to Uppy (useful for notifying user about authentication errors and other useful errors)

Note: This PR is easier reviewed commit-by-commit

closes #4555 #4551

example implementations:

Edit: TODO:

  • improve the simpleAuth API in the client
  • allow provider to specify content type in thumbnail api Provider user sessions #4619 (comment)
  • investigate merging SearchProviderViews into ProviderViews, based on this PR (separate PR)

mifi added 14 commits August 9, 2023 16:06
not critical but some browsers might have problems
so we don't have to decode/decrypt/encode/encrypt so many times
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`.
also for thumbnails
for consistency
it wasn't returning the status code (like `got` does on error)
it's needed to respond properly with a http error
instead log error and show the key
this in on par with other i18n frameworks
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 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 force the user to use html form
and use preact for it, for flexibility
Copy link
Contributor

@dschmidt dschmidt left a comment

Choose a reason for hiding this comment

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

Looking great! Thanks for taking this over <3

# Conflicts:
#	packages/@uppy/companion/src/server/controllers/refresh-token.js
#	packages/@uppy/companion/src/server/provider/index.js
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
# Conflicts:
#	packages/@uppy/companion-client/src/Provider.js
#	packages/@uppy/companion/src/server/provider/error.d.ts
Copy link
Member

@Murderlon Murderlon left a comment

Choose a reason for hiding this comment

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

I think we're still low on tests but don't want to block this forever.

@mifi
Copy link
Contributor Author

mifi commented Dec 5, 2023

I have now rewritten errors to not use instanceof, but instead duck typing. Final round of reviews please, then I think we can merge! You only need to look at commits after "Dec 1, 2023"

@mifi mifi requested a review from aduh95 December 5, 2023 13:29
@aduh95 aduh95 merged commit ec4bc58 into main Dec 5, 2023
20 checks passed
@aduh95 aduh95 deleted the provider-user-sessions branch December 5, 2023 21:55
@dschmidt
Copy link
Contributor

dschmidt commented Dec 5, 2023

🎉 🎉 🎉

@mifi mifi restored the provider-user-sessions branch December 7, 2023 14:06
@mifi mifi deleted the provider-user-sessions branch December 7, 2023 14:29
mifi added a commit that referenced this pull request Dec 7, 2023
* 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]>
Murderlon added a commit that referenced this pull request Dec 7, 2023
* 4.x:
  meta: enable CI on `4.x` branch
  @uppy/aws-s3: change Companion URL in tests
  @uppy/set-state: fix types
  Provider user sessions (#4619)
mifi added a commit that referenced this pull request Dec 12, 2023
* 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]>
github-actions bot added a commit that referenced this pull request Dec 12, 2023
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/aws-s3           |   3.6.0 | @uppy/instagram        |   3.2.0 |
| @uppy/aws-s3-multipart |  3.10.0 | @uppy/onedrive         |   3.2.0 |
| @uppy/box              |   2.2.0 | @uppy/provider-views   |   3.8.0 |
| @uppy/companion        |  4.12.0 | @uppy/store-default    |   3.2.0 |
| @uppy/companion-client |   3.7.0 | @uppy/tus              |   3.5.0 |
| @uppy/core             |   3.8.0 | @uppy/url              |   3.5.0 |
| @uppy/dropbox          |   3.2.0 | @uppy/utils            |   5.7.0 |
| @uppy/facebook         |   3.2.0 | @uppy/xhr-upload       |   3.6.0 |
| @uppy/google-drive     |   3.4.0 | @uppy/zoom             |   2.2.0 |
| @uppy/image-editor     |   2.4.0 | uppy                   |  3.21.0 |

- @uppy/provider-views: fix uploadRemoteFile undefined (Mikael Finstad / #4814)
- @uppy/companion: fix double tus uploads (Mikael Finstad / #4816)
- @uppy/companion: fix accelerated endpoints for presigned POST  (Mikael Finstad / #4817)
- @uppy/companion: fix `authProvider` property inconsistency (Mikael Finstad / #4672)
- @uppy/companion:  send certain onedrive errors to the user (Mikael Finstad / #4671)
- meta: fix typo in `lockfile_check.yml` name (Antoine du Hamel)
- @uppy/aws-s3: change Companion URL in tests (Antoine du Hamel)
- @uppy/set-state: fix types (Antoine du Hamel)
- @uppy/companion: Provider user sessions (Mikael Finstad / #4619)
- meta: fix `js2ts` script on Node.js 20+ (Merlijn Vos / #4802)
- @uppy/companion-client: avoid unnecessary preflight requests (Antoine du Hamel / #4462)
- meta: Migrate to AWS-SDK V3 syntax (Artur Paikin / #4810)
- @uppy/utils: fix import in test files (Antoine du Hamel / #4806)
- @uppy/core: Fix onBeforeFileAdded with Golden Retriever (Merlijn Vos / #4799)
- @uppy/image-editor: respect `cropperOptions.initialAspectRatio` (Lucklj521 / #4805)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants