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

Finishing touches on Companion dynamic Oauth #2802

Merged
merged 13 commits into from
Feb 14, 2022

Conversation

goto-bus-stop
Copy link
Contributor

@goto-bus-stop goto-bus-stop commented Mar 11, 2021

This PR will contain the final bits we need to start using Companion's dynamic oauth feature on transloadit.com. May add a few more things here as I work on the transloadit.com side…

So far this:

  • fails loudly if requested third-party credentials do not exist, instead of silently falling back to the preconfigured one. There was no way for developers to see what went wrong before
  • adds options to Robodog so you don't have to remember how companionKeysParams works and don't have to duplicate your transloadit API key. Now you can do:
    robodog.dashboard({
      params: {
        auth: { key: 'your transloadit api key' }
      },
      providers: ['instagram'],
      instagram: {
        credentialsName: 'your instagram credentials name on transloadit.com'
      }
    })

To do:

  • when requested third-party credentials can't be found, error out with an HTML page that can communicate back to the Uppy client, instead of a JSON error. – leaving this for a followup PR
  • docs for the new robodog options
  • Seems to lose preauth state when the initial authentication attempt fails; then falls back to default credentials, which is bad

@goto-bus-stop goto-bus-stop marked this pull request as ready for review March 23, 2021 11:37
@goto-bus-stop
Copy link
Contributor Author

Doesn't work correctly yet even with the API2-side changes, so need to investigate a bit more where it could be going wrong 🤔

@mifi
Copy link
Contributor

mifi commented Dec 13, 2021

Seems to lose preauth state when the initial authentication attempt fails; then falls back to default credentials, which is bad

what does this mean?

@goto-bus-stop
Copy link
Contributor Author

what does this mean?

IIRC, preauth state refers to the stuff that we need to initiate the authentication. Before starting we fetch encrypted oauth keys from Companion that will be used for the signin. (Those keys eventually come from the credentials storage in one's transloadit account.) Then we send that back to companion when we actually sign in. Companion redirects to the relevant provider login page matching the oauth keys. So the provider will show the user signing in to the correct app.

Apparently if the signin did not succeed, the client forgot about the preauth state (encrypted keys), and trying again would use companion's default oauth keys, which in our case means that users see a "sign in to transloadit" screen instead of "sign in to <website i'm actually on>".

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Jan 27, 2022

I actually cannot reproduce the issue I was running into earlier. I tried failing the login in many ways and it always sent me to the correct app again afterward.

image

I would like to be 100% sure that we don't accidentally use our default app when end users try to log in to one of our customers' apps. Maybe Companion should never fall back, and instead ONLY load keys dynamically if a key endpoint is provided. On our end we can return default keys from the endpoint if the request body is empty. Or the client could send some parameter to Companion to prevent falling back to defaults if a companionKeysParams value is configured. I'll experiment with both 😇

@mojowill
Copy link
Contributor

As a customer waiting for this, would it not make more sense to simply error out if trying to use a third party like Google Drive etc if the keys have not been set in on the companion? Does there need to be a fallback?

We're using Transloadit as our companion and never want Transloadit to appear in the grant screen of a third party app. If keys aren't configured a developer is usually going to spot that before anything gets shipped to production.

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Jan 27, 2022

That's a good point actually! We have the fallback for developers who are just trying it out and because we use the same Companion instances for our public demos (where you can also import files from third-party services). But we could address the demos in a different way easily, and the fallback for new users is not that valuable. The only risk is that someone may be using the fallback somewhere today (I think most people would consider this PR a blocker, but some small-scale apps may not.) I'll ask if we have any insight into that, because your proposal would be ideal imo.

The fallback is also partly historical because it used to be the only way to configure the keys, so a server could only support one set of keys. This dynamic stuff is kinda bolted on top specifically for Transloadit's hosted use case (anyone else would just use the old way) so we want to be careful about potentially breaking self-hosters.

@mojowill
Copy link
Contributor

I hope this will also work on the Transloadit plugin and not just Robodog?

@goto-bus-stop
Copy link
Contributor Author

goto-bus-stop commented Jan 27, 2022

Yes, with the Transloadit plugin you just need a more manual configuration in each provider, since the providers cannot assume that you're using Transloadit's servers. I'll add that to the docs pages in this PR, but the short of it is:

uppy.use(GoogleDrive, {
  companionKeysParams: {
    key: 'TRANSLOADIT_KEY', // needs to be passed here because the provider plugins are not aware of transloadit
    credentialsName: 'credentials name just like in robodog options',
  }
})

@Murderlon Murderlon requested review from mifi and aduh95 February 1, 2022 09:26
@goto-bus-stop goto-bus-stop merged commit db47700 into main Feb 14, 2022
@goto-bus-stop goto-bus-stop deleted the companion-third-party-oauth branch February 14, 2022 10:07
@github-actions github-actions bot mentioned this pull request Feb 14, 2022
@aduh95 aduh95 mentioned this pull request Feb 14, 2022
github-actions bot pushed a commit that referenced this pull request Feb 14, 2022
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/companion           |   3.2.0 | @uppy/provider-views      |   2.0.7 |
| @uppy/companion-client    |   2.0.5 | @uppy/thumbnail-generator |   2.1.0 |
| @uppy/core                |   2.1.5 | @uppy/robodog             |   2.3.0 |
| @uppy/dashboard           |   2.1.4 | uppy                      |   2.5.0 |
| @uppy/locales             |   2.0.6 |                           |         |

- @uppy/companion: add support for COMPANION_UNSPLASH_SECRET (Mikael Finstad / #3463)
- @uppy/unsplash: fix nested meta (Artur Paikin / #3485)
- meta: fix(docs): typo in property `thumbnailType` (Dan Schalow / #3472)
- @uppy/robodog: add audio, box, unsplash, screen-capture to Robodog (Artur Paikin / #3483)
- meta: consolidate ENV files and fix contributing guidelines (Antoine du Hamel / #3475)
- @uppy/companion-client,@uppy/companion,@uppy/provider-views,@uppy/robodog: Finishing touches on Companion dynamic Oauth (Renée Kooi / #2802)
- meta: Improve companion docs (Mikael Finstad / #3479)
- meta: Make E2E Great Again (Merlijn Vos / #3444)
- meta: Add PostCSS handling to Vite (Artur Paikin / #3467)
- meta: Update CONTRIBUTING.md (Mikael Finstad / #3411)
- @uppy/companion: fix broken thumbnails for box and dropbox (Mikael Finstad / #3460)
- website: fix `Uppy is not defined` error (Antoine du Hamel / #3461)
- @uppy/companion: Implement periodic ping functionality (Mikael Finstad / #3246)
- @uppy/companion: fix callback urls (Mikael Finstad / #3458)
- @uppy/core,@uppy/dashboard,@uppy/thumbnail-generator: Add dashboard and UIPlugin types (Merlijn Vos / #3426)
- @uppy/locales: Add "save" to fr_FR.js (Charly Billaud / #3395)
- @uppy/companion: Fix TypeError when invalid initialization vector (Julian Gruber / #3416)
- meta: Upgrade size-limit to 7.0.5 (Artur Paikin / #3445)
- @uppy/provider-views: Unsplash: UI improvements (Artur Paikin / #3438)
- @uppy/thumbnail-generator: exifr: remove legacy IE support (Artur Paikin / #3382)
- @uppy/companion: Default to HEAD requests when the Companion looks to get meta information about a URL (Zack Bloom / #3417)
- @uppy/dashboard: check if info array is empty (Artur Paikin / #3442)
- meta: dev: fix Vite custom plugin (Antoine du Hamel / #3437)
- website: add legacy bundle to CDN example (Antoine du Hamel / #3433)
- meta: remove unused lerna and npm files (Antoine du Hamel / #3436)
- meta: replace browserify with esbuild (Antoine du Hamel / #3363)
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
Co-authored-by: Mikael Finstad <finstaden@gmail.com>
HeavenFox pushed a commit to docsend/uppy that referenced this pull request Jun 27, 2023
| Package                   | Version | Package                   | Version |
| ------------------------- | ------- | ------------------------- | ------- |
| @uppy/companion           |   3.2.0 | @uppy/provider-views      |   2.0.7 |
| @uppy/companion-client    |   2.0.5 | @uppy/thumbnail-generator |   2.1.0 |
| @uppy/core                |   2.1.5 | @uppy/robodog             |   2.3.0 |
| @uppy/dashboard           |   2.1.4 | uppy                      |   2.5.0 |
| @uppy/locales             |   2.0.6 |                           |         |

- @uppy/companion: add support for COMPANION_UNSPLASH_SECRET (Mikael Finstad / transloadit#3463)
- @uppy/unsplash: fix nested meta (Artur Paikin / transloadit#3485)
- meta: fix(docs): typo in property `thumbnailType` (Dan Schalow / transloadit#3472)
- @uppy/robodog: add audio, box, unsplash, screen-capture to Robodog (Artur Paikin / transloadit#3483)
- meta: consolidate ENV files and fix contributing guidelines (Antoine du Hamel / transloadit#3475)
- @uppy/companion-client,@uppy/companion,@uppy/provider-views,@uppy/robodog: Finishing touches on Companion dynamic Oauth (Renée Kooi / transloadit#2802)
- meta: Improve companion docs (Mikael Finstad / transloadit#3479)
- meta: Make E2E Great Again (Merlijn Vos / transloadit#3444)
- meta: Add PostCSS handling to Vite (Artur Paikin / transloadit#3467)
- meta: Update CONTRIBUTING.md (Mikael Finstad / transloadit#3411)
- @uppy/companion: fix broken thumbnails for box and dropbox (Mikael Finstad / transloadit#3460)
- website: fix `Uppy is not defined` error (Antoine du Hamel / transloadit#3461)
- @uppy/companion: Implement periodic ping functionality (Mikael Finstad / transloadit#3246)
- @uppy/companion: fix callback urls (Mikael Finstad / transloadit#3458)
- @uppy/core,@uppy/dashboard,@uppy/thumbnail-generator: Add dashboard and UIPlugin types (Merlijn Vos / transloadit#3426)
- @uppy/locales: Add "save" to fr_FR.js (Charly Billaud / transloadit#3395)
- @uppy/companion: Fix TypeError when invalid initialization vector (Julian Gruber / transloadit#3416)
- meta: Upgrade size-limit to 7.0.5 (Artur Paikin / transloadit#3445)
- @uppy/provider-views: Unsplash: UI improvements (Artur Paikin / transloadit#3438)
- @uppy/thumbnail-generator: exifr: remove legacy IE support (Artur Paikin / transloadit#3382)
- @uppy/companion: Default to HEAD requests when the Companion looks to get meta information about a URL (Zack Bloom / transloadit#3417)
- @uppy/dashboard: check if info array is empty (Artur Paikin / transloadit#3442)
- meta: dev: fix Vite custom plugin (Antoine du Hamel / transloadit#3437)
- website: add legacy bundle to CDN example (Antoine du Hamel / transloadit#3433)
- meta: remove unused lerna and npm files (Antoine du Hamel / transloadit#3436)
- meta: replace browserify with esbuild (Antoine du Hamel / transloadit#3363)
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.

None yet

4 participants