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

Custom providers no longer work #4460

Closed
2 tasks done
StrixOSG opened this issue May 17, 2023 · 7 comments · Fixed by #4672
Closed
2 tasks done

Custom providers no longer work #4460

StrixOSG opened this issue May 17, 2023 · 7 comments · Fixed by #4672
Assignees
Labels
Bug Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3)

Comments

@StrixOSG
Copy link

StrixOSG commented May 17, 2023

Initial checklist

  • I understand this is a bug report and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Link to runnable example

No response

Steps to reproduce

With Companion version > 4.3.0, starting in 4.4.0:

Add a custom provider as documented:

customProviders: {
  myProvider: {
    config: {
      authorize_url: 'my_provider_auth_url',
      access_url: 'my_provider_access_url',
      oauth: 2,
      key: 'my_provider_key',
      secret: 'my_provider_secret',
      scope: ['custom_scope']
    },
    module: MyProviderModule
  },
}

The MyProviderModule based on the example and docs then implements list and download at the minimum, and has static version = 2 and authProvider = 'myProvider'.

Expected behavior

This should be enough to have a working Custom provider on the Companion side and works as of Companion v4.3.0

Actual behavior

In Companion v4.4.0 and above this no longer works and instead complains of null Provider does not support OAuth. I believe this to be a regression or bug with #4330. If you add static authProvider = 'myProvider' it is closer to working, but still fails. This should not be needed either, as it looks like the code for connect and those oAuth callbacks do not need the provider to be an "OAuthProvider". Unless #4330 actually does have breaking changes and there needs to be more work done to update documentation on how Custom Providers should be implemented.

Edit:
I should add that when you do make it static authProvider = 'myProvider'. Then you just get misconfigured provider when trying to auth client side. If you add in another variable that's the same along with it authProvider = 'myProvider' that's non-static then you get Cannot read properties of undefined (reading 'credentialsURL')

@arturi arturi added Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3) and removed Triage labels May 18, 2023
@mifi
Copy link
Contributor

mifi commented Jun 10, 2023

If you add in another variable that's the same along with it authProvider = 'myProvider' that's non-static then you get Cannot read properties of undefined (reading 'credentialsURL')

I will fix this bug.

It seems that authProvider in Provider classes has been static for years:

So I think that could mean that the documentation was always wrong and we should update it to make sure people use static. I guess the fact that authProvider (without static) used to work was a bug (?)

I should add that when you do make it static authProvider = 'myProvider'. Then you just get misconfigured provider when trying to auth client side.

What do you mean exactly by this? Is a misconfigured provider an error message you're getting? I can't find any reference to that error anywhere in the code.

mifi added a commit that referenced this issue Jun 10, 2023
mifi added a commit that referenced this issue Jun 10, 2023
@StrixOSG
Copy link
Author

Thanks! This may fix the issues, but it is an error message I get. I believe this is where it's referenced in the code: https://github.com/transloadit/uppy-server/blob/58948e83de26246903390ea1c1a7271a12091437/src/uppy.js#L171

@mifi
Copy link
Contributor

mifi commented Jun 13, 2023

have you remembered to configure your custom provider's grant config? Looking at your code, this doesn't seem to be a valid configuration:

      authorize_url: 'my_provider_auth_url',
      access_url: 'my_provider_access_url',
      oauth: 2,
      key: 'my_provider_key',
      secret: 'my_provider_secret',
      scope: ['custom_scope']

@StrixOSG
Copy link
Author

It's possible that I could be missing something, but I think this is how the documentation states to set it up, and it is working correctly with the code you have there for me with the previous companion version. I don't have anything else other than the code I have posted and it works

@mifi
Copy link
Contributor

mifi commented Jun 13, 2023

Ok, gotcha. companion uses grant to handle authentication so i thought maybe it was grant throwing the error. will have to do some more digging

@aduh95 aduh95 closed this as completed in 8b6e87a Jun 19, 2023
@mifi mifi reopened this Jun 19, 2023
@netdown
Copy link
Contributor

netdown commented Aug 22, 2023

@mifi I have also experienced the misconfigured provider error with the latest version. It seems that besides the static authProvider property, the instance must also be populated with an authProvider property.

static get authProvider() { return 'myprovider' }
constructor(options) { this.authProvider = MyProvider.authProvider }

mifi added a commit that referenced this issue Sep 6, 2023
always use the static property
ignoring the instance propety

fixes #4460
@StrixOSG
Copy link
Author

StrixOSG commented Nov 1, 2023

Thanks for this upcoming fix! I can confirm that @netdown's solution worked for me. In my case I didn't need the constructor, but my code is as follows and appears to be working now for Uppy Companion v4.10.1:

    authProvider = MyCustomProvider.authProvider;
    static get authProvider() { return 'myCustomProvider' };

mifi added a commit that referenced this issue 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 <antoine@transloadit.com>

* 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 <antoine@transloadit.com>

* 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 <antoine@transloadit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Companion The auth server (for Instagram, GDrive, etc) and upload proxy (for S3)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants