@uppy/companion-client,@uppy/provider-views: make authentication optional

[dschmidt]

… in ProviderView

see #4551 for context

This allows provider plugins to circumvent the authentication flow and roundtrip to companion.
A different approach would be to move the actual authentication handling in handleAuth into the provider class and make it possible to override it.

What do you think, what do you prefer?

The use case for this is the following: my WebDAV provider allows browsing public links (basically unprotected webdav endpoints) without triggering an authentication flow.

I would still like to use the AuthView and render a custom form (see #4555) but immediately switch towards the file browser without the authentication flow.

[dschmidt]

A different approach would be to move the actual authentication handling in handleAuth into the provider class and make it possible to override it.

I thought I'd give this approach a spin and pushed a second commit.
It's a slightly more invasive change and I'm not sure how much of a breaking change this could be for anyone, but apart from that I think it's the more elegant and flexible approach. It allows plugin authors to override the login method with custom logic (instead of just shortcutting the authentication).

This is what I do now in my WebDAV provider:

    this.provider = new Provider(uppy, {
      companionUrl: this.opts.companionUrl,
      companionHeaders: this.opts.companionHeaders,
      companionKeysParams: this.opts.companionKeysParams,
      companionCookiesRule: this.opts.companionCookiesRule,
      provider: 'webdavPublicLink',
      pluginId: this.id,
    })
    this.provider.login = async () => {
      // do some async checks for whatever ... or just do nothing
    }
    this.provider.logout = async () => {
      return { ok: true, revoked: true }
    }

Also I think it really makes sense to put the login logic into the companion client as it's completely agnostic of the provider view and this way can be used when just using the companion client without the dashboard.
It's not a mandatory change to anyone using just the companion client, but only an additional helper - only real change being authUrl() now adding the state on its own

Let me know, what works better for you. I'm fine with either version I pushed...

[arturi]

Thanks for your contributions, much appreciated. Public WebDav providers, and potentially more non-oauth like FTP is something we do want to support. We’ve discussed this in a call and need some more time to think about the best approach.

We have a separate login-less flow for Unsplash plugin already, the SearchProviderView, so we were discussing if we could re-use that, with a url instead of search.

[dschmidt]

Okay, great - it's not time critical for me, as we've created a fork that we're going to use for the time being.
It's just important to me that I can get rid of it in the long run - and it doesn't take so long that I have forgotten what I've actually done here 😇
What time frame are we talking about and what would be a good time to ping you if this goes stale? I don't want to be annoying but I know how easy it is to lose track of issues and that a friendly ping often helps me ;-)

I'm not attached at all to my code, so I'm happy to throw it away with you come up with any better approaches to enable my use cases - I just tried to get things working :)
As external contributor it's hard to contribute refactorings with an even bigger scope, but I'm happy to adapt my code or contribute feedback.

[mifi]

I think this change makes a lot of sense. It's basically just moving code around a bit to make it easier to override/disable the auth method. more flexibility is good! But like you say, it could be considered a breaking change, and i'm not super into Uppy code's architecture so i'm not sure if it's the "correct" way. How do we want developers creating their own Uppy providers to "extend" Provider? this.provider.login = () => {} doesn't seem like a very oop way of doing it. maybe if we're going to make a breaking change, we should also make an architectural overhaul of the ProviderView/Provider architecture eto make it easier for devs to make their own providers

[Murderlon]

I think this makes sense too. It's a breaking change when people upgrade Uppy but do not upgrade Companion to latest at the same time. I think we've made changes like this before without a major, such as the new endpoints in Companion for S3 multipart, which would fail similarly.

It might be okay to do in a minor release, if we write a clear warning in the docs.

[mifi]

I think this makes sense too. It's a breaking change when people upgrade Uppy but do not upgrade Companion to latest at the same time. I think we've made changes like this before without a major, such as the new endpoints in Companion for S3 multipart, which would fail similarly.

I don't think this change has anything to do with Companion, or am I missing something? as far as I can tell it's only an internal refactoring of Uppy's client code

[dschmidt]

I think this change makes a lot of sense. It's basically just moving code around a bit to make it easier to override/disable the auth method. more flexibility is good!

Cool!

Regarding:

But like you say, it could be considered a breaking change, and i'm not super into Uppy code's architecture so i'm not sure if it's the "correct" way.

and

I think this makes sense too. It's a breaking change when people upgrade Uppy but do not upgrade Companion to latest at the same time. I think we've made changes like this before without a major, such as the new endpoints in Companion for S3 multipart, which would fail similarly.

It might be okay to do in a minor release, if we write a clear warning in the docs.

Looked at it again, not sure how this would break for anyone in what context. This is completely independent from companion, it's just adding an additional method to companion-client and moves code from ProviderView there.

How do we want developers creating their own Uppy providers to "extend" Provider? this.provider.login = () => {} doesn't seem like a very oop way of doing it. maybe if we're going to make a breaking change, we should also make an architectural overhaul of the ProviderView/Provider architecture eto make it easier for devs to make their own providers

Dunno, you can always properly subclass - but overriding it on the instance is okaaayiish for JS imho.

PS: Rebased.

[Murderlon]

Sorry I was mistaken, companion-client and provider-views can never be out of sync so this is totally fine 👍

[Murderlon]

CI fails on this though:

Error: Unused locale key "authAborted" in @uppy/core

I think you have to move that entry from @uppy/core/locale to @uppy/provider-views.

[dschmidt]

Ah, because it was moved from provider-views to companion-client.
I'll check if I can move translations...

[dschmidt]

Can you possibly help me with this?

The companion-client/Provider is not a "full plugin", but just a (base-) class - so the usual pattern of setting this.defaultLocale does not "just work".
What would be a good way to implement this?
I could provide defaultLocale from the Provider object, but that would need to be merged into translations in all consuming plugins. Something along these lines:

    this.provider = new Provider(uppy, {
      companionUrl: this.opts.companionUrl,
      companionHeaders: this.opts.companionHeaders,
      companionKeysParams: this.opts.companionKeysParams,
      companionCookiesRule: this.opts.companionCookiesRule,
      provider: 'facebook',
      pluginId: this.id,
    })

    this.defaultLocale = merge(this.provider.defaultLocale, locale)

seems cumbersome ... any better ideas?

[dschmidt]

Maybe not toooo bad after all...

[dschmidt]

CI is ✅

[Murderlon]

Sorry I didn't see that this translation string would become an issue.

I don't think the merging of locales is something we want to do. Ideally we avoid this complexity.

It seems it was in fact working but our hacky test was just faulty, it didn't check for core locale strings in companion-client. Reverted your change and updated the test.

[dschmidt]

Whatever works, thanks for taking care :)

[mifi]

nice stuff!