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

feat: Add username and password support for WebDAV #1323

Merged
merged 5 commits into from
Feb 12, 2023
Merged

feat: Add username and password support for WebDAV #1323

merged 5 commits into from
Feb 12, 2023

Conversation

Young-Flash
Copy link
Member

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

This PR adds username and password support forWebDAV

Please review the changes, if it's ok I will do the same for HttpBuilder

#1319 #1237

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Mostly LGTM!

src/services/webdav/backend.rs Show resolved Hide resolved
src/services/webdav/backend.rs Outdated Show resolved Hide resolved
src/services/webdav/backend.rs Outdated Show resolved Hide resolved
src/services/webdav/backend.rs Outdated Show resolved Hide resolved
@Xuanwo Xuanwo changed the title Add username and password support for WebDAV feat: Add username and password support for WebDAV Feb 11, 2023
@yihong0618
Copy link
Contributor

yihong0618 commented Feb 11, 2023

I did some work like this before.
mostly the same as this PR
. I think you need to add the auth things in Actions.

webdav acrions test use nginx as service, and nginx can use basic auth. plug-in
But kind of failed when I am trying , you can take a try.

@yihong0618
Copy link
Contributor

And Actions you can use ftp actions as example

@Xuanwo
Copy link
Member

Xuanwo commented Feb 11, 2023

. I think you need to add the auth things in Actions.

webdav acrions test use nginx as service, and nginx can use basic auth. plug-in

Nice idea. I'm also considering about adding test to cover this case. But I prefer to add tests in a new PR instead.

@Xuanwo
Copy link
Member

Xuanwo commented Feb 11, 2023

Does anyone really set an empty password? IMO, even if the user wants to do so, the service provider will not allow it.

Please make sure that our implementation is correct. Whether or not a service supports basic authentication without a password is a user decision. For example, the user might have configured a webdav service with Nginx this way.

@Young-Flash
Copy link
Member Author

I have verified that : is necessary when it comes to empty password. Use this to build a local webdav server and set empty password, only if format the auth string with : can I connect to the local webdav server.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@Xuanwo Xuanwo merged commit d27af0a into apache:main Feb 12, 2023
@Young-Flash
Copy link
Member Author

I'll deal with HttpBuilder next, it should be almost the same as this, right?

@Xuanwo
Copy link
Member

Xuanwo commented Feb 12, 2023

I'll deal with HttpBuilder next, it should be almost the same as this, right?

Exactly!

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.

3 participants