@uppy/companion,@uppy/companion-client: pass query params along more consistently #4551

dschmidt · 2023-07-06T13:31:43Z

This PR is the first in a series of changes I would like to upstream.
The big picture is: I have implemented imports from Nextcloud/ownCloud based WebDAV servers.
They can be self hosted and so users need to provide a URL before they can login - which requires changes in companion and the Dashboard/ProviderView so it can be reasonably implemented.

The current state of the complete implementation can be seen here: dschmidt#1
I think submitting it in total will be very hard to review, so I'm going to send a bunch of small scoped PRs.

query is passed to provider.list, provider.size and provider.download already - this PRs adds it to provider.logout and provider.thumbnail invocations.
Moreover, it adds a customQueryParams property to providers in the ui which is automatically serialized and appended to requests to those endpoints.

This makes it possible to implement custom forms. (see #4555)

…ed to requests

This allows to define dynamic properties for grant providers. E.g. `[subdomain]` can be automatically replaced in `authorize_url` and `access_url`. For that to work we need to include the dynamic property in the redirect to the grant middleware. We merge the grantConfig back to the providerConfig, so we can determine the required query params based on the `dynamic` property. This avoids an attacker overriding unexpected properties (if we simply serialize and pass along `query`)

mifi

not sure about this one. why do we need the possibility to pass dynamic query parameters from the client to logout and thumbnail? e.g. why does a custom form need to pass a query parameter to logout and thumnail?

mifi · 2023-07-31T14:47:33Z

packages/@uppy/companion/src/server/controllers/connect.js

-  res.redirect(req.companion.buildURL(`/connect/${req.companion.provider.authProvider}?state=${state}`, true))
+  const providerName = req.companion.provider.authProvider
+  const qs = queryString(Object.fromEntries(
+    req.companion.options.providerOptions[providerName]?.dynamic?.map(p => [p, req.query[p]]) || [],


not sure about this change?

#4551 (comment)

mifi · 2023-07-31T14:48:16Z

packages/@uppy/companion/src/companion.js

@@ -73,6 +73,7 @@ module.exports.app = (optionsArg = {}) => {
  const providers = providerManager.getDefaultProviders()

  providerManager.addProviderOptions(options, grantConfig)
+  options.providerOptions = merge(options.providerOptions, grantConfig)


not sure about this change

This is here because grant does interpolation for certain settings passed as GET parameters to the connect handler if they are marked as dynamic, e.g. if you add subdomain to the dynamic array, you can use it in authorize_url and access_url (c.f. https://github.com/simov/grant#dynamic-http and https://github.com/owncloud/uppy/blob/3403194fd4692c679e03d993c040c6d8ac5ac29a/packages/%40uppy/companion/src/config/grant.js#L47-L57 - yes, it should only be used as a subdomain not as a fqdn but that's a dlfferent topic).

To make this work in companion we need to forward the dynamic paramaters in the redirect from our connect handler to the grant connect handler.
In order to achieve that, I merge the grant config including the dynamic option back to providerOptions and loop over the dynamic array in our connect handler (which is the other part of the code you commented on)

edit: fixed link

ffr https://github.com/owncloud/uppy/blob/3403194fd4692c679e03d993c040c6d8ac5ac29a/packages/%40uppy/companion/src/server/provider/webdav/auth/index.js#L30

subdomain is passed to every request as query so we can use it there.

dschmidt · 2023-07-31T15:09:06Z

not sure about this one. why do we need the possibility to pass dynamic query parameters from the client to logout and thumbnail? e.g. why does a custom form need to pass a query parameter to logout and thumnail?

I use it in my WebdavProvider to specify the server url - so I also need it in logout and thumbnail functions and everything else that potentially interacts with the server

edit: not yet implemented, but when I want to, I need access to the server url/domain

mifi · 2023-08-08T09:31:27Z

Sorry for the delays. We've had some discussions and we're still open to supporting WebDAV, but we need to find an abstraction that is secure and works well for multiple other use cases such as FTP too. I will do some prototyping.

dschmidt · 2023-08-08T10:07:57Z

Feel free to take as much or as little inspiration as you want from owncloud#1 which is the current state of our integration

Ftr this was the hardest to figure out to make a login work:
#4551 (comment)

mifi · 2023-08-08T10:29:42Z

Feel free to take as much or as little inspiration as you want from owncloud#1 which is the current state of our integration

Thanks, I will! I see that owncloud#1 main needs to be updated to upstream main, because we merged alot of your PRs here.

Ftr this was the hardest to figure out to make a login work:
#4551 (comment)

ah yes. Initially I was a bit confused because I thought what you wanted to add support for is a simple webdav public URL without any authentication. but I realised that you want to add two different providers:

one for webdav URLs without authentication
one for webdav URLs with oauth authentication with two variations:
- special case for owncloud, because they use their own URLs
- special case for nextcloud, because they use their own URLs
- maybe in the future more webdav oauth providers?

I think it would also make sense to support public webdav urls with AuthType.password as well as other types of auth where the password is being sent along with every request (like Basic auth and FTP for example). I'm not sure if sending the username/password as a query parameter with every request to uppy is the best way to do it, and I'm thinking we could maybe bake more authentication/session state like webdav URL and passwords into the uppy-auth-token header/cookie.

dschmidt · 2023-08-08T10:42:18Z

Yep, indeed, I plan to update it in a bit.
.. and yes, exactly - I added two providers like you described.
I obviously scratched my own itch here, ownCloud and Nextcloud are basically the same it's just a labelling/theme we need in our product. Of course it would be nice to support even more, but those are the ones we currently need.
And yes, we basically take the user visible url to the webdav public link "share" and transform it to an actual webdav url. We don't have password handling for now and yes sending along every single request sounds like a bad idea. If you have a proper solution to that: appreciated

but to not overcomplicate things, I would suggest to iterate and handle links without password or/and oauth authenticated links first

dschmidt · 2023-08-08T12:48:28Z

Rebased

mifi · 2023-08-14T08:55:39Z

Created a new PR #4619 as an alternative to this PR, for accommodating providers with different auth flows (non-oauth), as well as per-provider state and OAuth dynamic parameters. After it has been reviewed, I think owncloud#1 can be adapted to match #4621

dschmidt · 2023-08-14T09:57:33Z

As said elsewhere: I'm not attached to my specific implementation, if we go for your approach and that solves my use case, I'm more than happy 👍️

github-actions bot added the pending end-to-end tests label Jul 6, 2023

dschmidt changed the title ~~@uppy/companion: pass query params to more provider methods~~ @uppy/companion,@uppy/companion-client: pass query params to more provider methods Jul 6, 2023

dschmidt changed the title ~~@uppy/companion,@uppy/companion-client: pass query params to more provider methods~~ @uppy/companion,@uppy/companion-client: pass query params along more consistently Jul 6, 2023

arturi requested a review from mifi July 10, 2023 13:29

dschmidt force-pushed the companion-query-params branch from 1da5c7d to 03520ba Compare July 10, 2023 15:36

dschmidt added 4 commits July 21, 2023 09:06

companion: pass query params to more provider methods

9fb7c52

Fix typos in comments

c7a7d46

@uppy/companion-client: allow defining customQueryParams that are add…

ca3b12c

…ed to requests

dschmidt force-pushed the companion-query-params branch from 03520ba to 968fe8a Compare July 21, 2023 07:13

mifi reviewed Jul 31, 2023

View reviewed changes

Merge branch 'main' into companion-query-params

62160bd

arturi assigned mifi Aug 7, 2023

mifi mentioned this pull request Aug 14, 2023

Provider user sessions #4619

Merged

3 tasks

dschmidt closed this Aug 14, 2023

JammingBen mentioned this pull request May 2, 2024

Remove Uppy fork and use upstream implementation owncloud/web#10868

Open

dschmidt deleted the companion-query-params branch June 13, 2024 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@uppy/companion,@uppy/companion-client: pass query params along more consistently #4551

@uppy/companion,@uppy/companion-client: pass query params along more consistently #4551

dschmidt commented Jul 6, 2023 •

edited

Loading

mifi left a comment •

edited

Loading

mifi Jul 31, 2023

dschmidt Jul 31, 2023

mifi Jul 31, 2023

dschmidt Jul 31, 2023 •

edited

Loading

dschmidt Jul 31, 2023 •

edited

Loading

dschmidt commented Jul 31, 2023 •

edited

Loading

mifi commented Aug 8, 2023

dschmidt commented Aug 8, 2023

mifi commented Aug 8, 2023

dschmidt commented Aug 8, 2023

dschmidt commented Aug 8, 2023

mifi commented Aug 14, 2023 •

edited

Loading

dschmidt commented Aug 14, 2023

@uppy/companion,@uppy/companion-client: pass query params along more consistently #4551

@uppy/companion,@uppy/companion-client: pass query params along more consistently #4551

Conversation

dschmidt commented Jul 6, 2023 • edited Loading

mifi left a comment • edited Loading

Choose a reason for hiding this comment

mifi Jul 31, 2023

Choose a reason for hiding this comment

dschmidt Jul 31, 2023

Choose a reason for hiding this comment

mifi Jul 31, 2023

Choose a reason for hiding this comment

dschmidt Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

dschmidt Jul 31, 2023 • edited Loading

Choose a reason for hiding this comment

dschmidt commented Jul 31, 2023 • edited Loading

mifi commented Aug 8, 2023

dschmidt commented Aug 8, 2023

mifi commented Aug 8, 2023

dschmidt commented Aug 8, 2023

dschmidt commented Aug 8, 2023

mifi commented Aug 14, 2023 • edited Loading

dschmidt commented Aug 14, 2023

dschmidt commented Jul 6, 2023 •

edited

Loading

mifi left a comment •

edited

Loading

dschmidt Jul 31, 2023 •

edited

Loading

dschmidt Jul 31, 2023 •

edited

Loading

dschmidt commented Jul 31, 2023 •

edited

Loading

mifi commented Aug 14, 2023 •

edited

Loading