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

Draft: Webdav provider poc #4621

Draft
wants to merge 55 commits into
base: main
Choose a base branch
from
Draft

Draft: Webdav provider poc #4621

wants to merge 55 commits into from

Conversation

mifi
Copy link
Contributor

@mifi mifi commented Aug 14, 2023

heavily based on owncloud#1 but modified to work with the new provider user sessions. not finished (missing some things from owncloud#1)

tldr: 2 new providers

  • WebDAV OAuth Provider
  • WebDAV simple auth provider (username/password)

problem: I'm not able to test these plugins because I don't know where to get WebDAV OAuth server credentials

note: don't merge before #4619 has been merged

mifi added 15 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
heavily based on owncloud#1
but modified to work with the new provider user sessions
@socket-security
Copy link

socket-security bot commented Aug 14, 2023

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
webdav 5.3.0 None +20 1.19 MB perrymitchell
fast-xml-parser 4.3.2 None +0 107 kB amitgupta

@dschmidt
Copy link
Contributor

I can certainly help with finding/setting up a server.

You could find me on talk.owncloud.com or you could open up your DMs on Twitter/X and we'll figure out something

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.

Very nice! Thanks again.

not finished (missing some things from https://github.com/owncloud/uppy/pull/1) - it's a bit hard to overview, what are you thinking of that is still missing?

transport: 'session',
// use 'subdomain' for full domain (hostname) similar to mastodon:
// https://github.com/simov/grant/blob/6e0692dfdd83edbc4ee82629ba0fe8f986d5879d/config/oauth.json#L631
dynamic: ['subdomain'],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should really set this as default/encourage it, it might be possible to craft a malicious redirect url with that and use it for phishing... haven't come up with anything clever for that so far (one of the reasons we did not enable this in production so far)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good point, but I wonder why grant does it for Mastodon if it's possible to abuse it for malicious purposes... Do you have any example of how one could abuse it?

@@ -0,0 +1,94 @@
// eslint-disable-next-line import/no-extraneous-dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. I think something wrong with eslint or its import resolver (eslint gives an error if removed)

Comment on lines +31 to +39
<form onSubmit={onSubmit}>
<label htmlFor="uppy-Provider-publicLinkURL">
<span>{i18n('publicLinkURLLabel')}</span>
<input id="uppy-Provider-publicLinkURL" name="webdavUrl" type="text" value={webdavUrl} onChange={(e) => setWebdavUrl(e.target.value)} disabled={loading} />
</label>
<span>{i18n('publicLinkURLDescription')}</span>

<button style={{ display: 'block' }} disabled={loading} type="submit">Submit</button>
</form>
Copy link
Contributor

Choose a reason for hiding this comment

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

For the Oauth variant publicLinkURL in specified names probably doesn't make sense, we rather want to enter just a "server url" I think

@dschmidt
Copy link
Contributor

I haven't tested the OAuth variant, as it's a bit cumbersome to setup - but I've tested the SimpleAuth variant and it does not seem to work. If you like, you can test with https://cloud.owncloud.com/s/29tljtuBOX6yHO9

@dschmidt
Copy link
Contributor

WebDAV dependency should be bumped:

perry-mitchell/webdav-client#346 (comment)

@Murderlon
Copy link
Member

We're still not sure whether we want to take on the maintenance burden of this plugin. It's quite a lot of code for a provider I never heard anyone needing except for your use case. I'm all for integrating the primitives into uppy/companion to make this happen, but perhaps the plugin belongs in user land.

@dschmidt
Copy link
Contributor

I understand. It's a bit unfortunate, as that will require me to maintain our fork and build our own docker images forever, but as long as we get the primitives integrated as you say I can live with that.

Maybe we can make customizing the docker image easier as well, so that a new provider is automatically picked up if placed in a certain directory (or something like that) and doesn't need code changes otherwise.

Then I don't need to maintain a full fork, but just a repository with the provider and a docker file which extends the official one.

@Murderlon
Copy link
Member

I understand. It's a bit unfortunate, as that will require me to maintain our fork and build our own docker images forever, but as long as we get the primitives integrated as you say I can live with that.

Maybe I misunderstand, but it shouldn't be too hard to maintain your own docker image so you don't have to maintain a fork of Companion?

a new provider is automatically picked up if placed in a certain directory (or something like that) and doesn't need code changes otherwise.

This is pretty interesting, as it would mean standalone can be used with custom providers! @mifi what do you think?

# Conflicts:
#	packages/@uppy/companion/src/server/controllers/refresh-token.js
#	packages/@uppy/companion/src/server/provider/index.js
Base automatically changed from provider-user-sessions to main December 5, 2023 21:55
@Murderlon Murderlon mentioned this pull request Jan 2, 2024
2 tasks
@Murderlon Murderlon linked an issue Jan 2, 2024 that may be closed by this pull request
2 tasks
@dschmidt
Copy link
Contributor

Are there any updates what's going to happen with this?

We merged master into this PR and tried to build the UIPlugin in our application or in a standalone vite preact project - without any success. (we are pretty clueless about preact tho and got lots of undefined reference errors of one letter var names, we had no idea how to debug that).
The only way we could make it work was by using yarn build && yarn workspaces foreach pack in this repo and then take the built package from there. While doable, it's not exactly elegant...

If you don't want the plugin upstream (again: I understand that you want to avoid the maintenance burden), would it be possible to provide an up to date example, how to externally build a UIPlugin?
https://github.com/arturi/uppy-plugin-image-compressor was last updated 5 years ago and still uses webpack, no typescript and ancient uppy versions.

When loaded at least the WebdavSimpleAuth plugin baaasically works, we just noticed that the logic for transforming the owncloud/nextcloud public links seems to be broken. We need to pass in the webdav link directly, which is not user friendly at all. When we have a plan how to proceed, we can certainly take care of that.

These changes were applied to make it build with current master: https://gist.github.com/dschmidt/9d8b47eceaabe49702a7ad1bd1f095ea

P.S.: Is there any rough eta for 4.0?

@Murderlon
Copy link
Member

Are there any updates what's going to happen with this?

I will bring it up again with the team on Monday. Sorry for the uncertainty.

We merged master into this PR and tried to build the UIPlugin in our application or in a standalone vite preact project - without any success. (we are pretty clueless about preact tho and got lots of undefined reference errors of one letter var names, we had no idea how to debug that).

I'm not sure what this is all about 🤔

would it be possible to provide an up to date example, how to externally build a UIPlugin?

I want to write a guide on how to build your own UI with Uppy, it might help, but in my case it would mean using React/Preact directly and not a UIPlugin. Which makes sense if you build something completely from scratch, although I will try to integrate provider-views into it to show off a mix of React + UI plugins.

P.S.: Is there any rough eta for 4.0?

Next week!

@dschmidt
Copy link
Contributor

Are there any updates what's going to happen with this?

I will bring it up again with the team on Monday. Sorry for the uncertainty.

Thanks - no worries, I understand it as I said.

We merged master into this PR and tried to build the UIPlugin in our application or in a standalone vite preact project - without any success. (we are pretty clueless about preact tho and got lots of undefined reference errors of one letter var names, we had no idea how to debug that).

I'm not sure what this is all about 🤔

Sorry, if that was confusing. We have an application in which we are using uppy (obviously), it's a Vue 3 application using Vite as bundler. We tried to build the Webdav UIPlugin inside our project without any success, because we couldn't figure out, how to correctly embed preact and make it work inside the Uppy dashboard... I wasn't complaining or asking for concrete help, just wanted to share our issues and why this whole topic is relevant for us

would it be possible to provide an up to date example, how to externally build a UIPlugin?

I want to write a guide on how to build your own UI with Uppy, it might help, but in my case it would mean using React/Preact directly and not a UIPlugin. Which makes sense if you build something completely from scratch, although I will try to integrate provider-views into it to show off a mix of React + UI plugins.

Fair enough!

P.S.: Is there any rough eta for 4.0?

Next week!

Oh nice! Thanks!

@dschmidt
Copy link
Contributor

I will bring it up again with the team on Monday. Sorry for the uncertainty.

Any update?

@dschmidt
Copy link
Contributor

dschmidt commented Jul 8, 2024

ping :)

@Murderlon
Copy link
Member

Hi, sorry the long wait. We decided to take on the WebDAV simple auth provide but not the WebDAV OAuth plugin for now. If we see more need for it we may take that one on as well.

@mifi has it on his todo list to update this PR and bring it over the finish line but there are some other things he's working on first.

As for the 4.0 release, we had a vulnerability which postponed the release. Tomorrow we're (finally) releasing.

@dschmidt
Copy link
Contributor

dschmidt commented Jul 8, 2024

Hooray hooray hooray! Those are perfect news - we currently only use the simple provider in production anyway, so this works for us!
... and don't worry about it taking some time. We truly and honestly appreciate the efforts you're making to take all our changes upstream (or build better more sustainable alternatives).
We're an open source project ourselves, we know the struggle and the cost of accepting third party code.

we have our fork(s) in place, so there's no real urgency - we just want to make sure there's a path forward and we don't have to maintain the fork forever.

Thank you so much.

@dschmidt
Copy link
Contributor

@mifi Not trying to nag, just to schedule our own efforts:
As our fork is quite outdated by now and we possibly are required to apply some dependency updates sooner than later - it would be great to know, what time frame you are roughly planning for this. If we are going to have this feature in Uppy 4.x in the next few weeks, it's probably not worth throwing time on updating our fork and instead update Uppy then. If it's still months away we need to come up with a different strategy. So absolutely no pressure, just trying to manage expectations :)

@mifi
Copy link
Contributor Author

mifi commented Jul 24, 2024

Hey, I can probably prioritize this so that we can have a testable beta within a few weeks. (simple auth only). One thing I'm not sure about is this code:

https://github.com/transloadit/uppy/pull/4621/files#diff-e17150077fadb980779024bc7b0a6fbcfc55b0987738648e26c8b673a90d4485R34-R46

if a URL contains /s/, then we rewrite it in a funny way. I'm not sure if that's standardised in webdav, so I'm inclined to remove it.

@dschmidt
Copy link
Contributor

dschmidt commented Jul 24, 2024

Hey, thanks for your quick response, @mifi!

beta in a few weeks sounds great! As I said earlier, Simple Auth is totally fine for us for now.

Yes, it's a bit funky, it's certainly not any kind of WebDAV standard - it's just the way ownCloud and Nextcloud show, so called public links: e.g. see https://web.cloud.owncloud.com/#/s/29tljtuBOX6yHO9

This is the link you visit in your browser, which internally does a PROPFIND to webdavs://cloud.owncloud.com/remote.php/dav/public-files/29tljtuBOX6yHO9

I understand if you don't want this hardcoded in your codebase, but it would be great if we could have it as a configuration option of some sort (or maybe hardcoded but only active when explicitly enabled).

@mifi
Copy link
Contributor Author

mifi commented Aug 4, 2024

Unfortunately we have to delay this feature because there's another more urgent feature (Google Picker API) that needs to be implemented first, and due to a holiday as well this will probably be delayed by 2 months.

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.

pCloud support?
4 participants