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.

Sign up for GitHub

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

Jump to bottom

[NET-642] FTPSClient execPROT removes proxy settings #90

Merged
merged 1 commit into from
Nov 7, 2022
Merged

[NET-642] FTPSClient execPROT removes proxy settings #90

merged 1 commit into from
Nov 7, 2022

Conversation

YaniM
Copy link
Contributor

@YaniM YaniM commented Oct 22, 2021

do not reset proxy settings when re-setting the socket factory
create method identical to open openDataConnection for FTPS where proxy is used and ssl socket is created from ssl context

@YaniM
NET-642 using execPROT on FTPSClients with Proxy Settings removes Pro…
c9f1179 
…xy Settings

do not reset proxy settings when re-setting the socket factory
create method identical to open _openDataConnection_ for FTPS where proxy is used and ssl socket is created from ssl context
@YaniM
Copy link
Contributor Author

YaniM commented Oct 22, 2021

Expecting a feedback. Also I'm not sure how to write a unit test here, any hints?

@garydgregory
Copy link
Member

You can start by looking at FTPSClientTest. There is also the option of using Docker through a Maven plugin like org.testcontainers:testcontainers

@garydgregory
Copy link
Member

@YaniM ping.

@YaniM
Copy link
Contributor Author

YaniM commented Oct 24, 2021

Yes, Gary. I see the hint but I don't know when I will make time for this.
Also in the defect NET-642, HTTPS is mention but my fix is only for FTPS... but it is a start.

karnick
karnick reviewed Oct 25, 2021
View reviewed changes
Copy link

@karnick karnick left a comment

Choose a reason for hiding this comment

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

+1 We have been waiting for this fix.

@YaniM
Copy link
Contributor Author

YaniM commented Oct 30, 2021

@garydgregory
Hi, my idea for unit test is to set the proxy in FTPSClientTest.loginClient() and then execute client.connect(...) and PROT command but I'm not sure which hosts and ports to use?
For example in SocketClientFunctionalTest are defined hosts and ports but I cannot connect to them.

@sawantnitesh
Copy link

sawantnitesh commented Apr 5, 2022 •
edited
Loading

We are blocked because of this issue to migrate to Commons NET. Will it be merged ?

@garydgregory
Copy link
Member

Still needs a unit test to fail without the main changes.

garydgregory
garydgregory reviewed Apr 5, 2022
View reviewed changes
src/main/java/org/apache/commons/net/ftp/FTPSClient.java
* @param sslSocket ssl socket
* @throws IOException closing sockets is not successful
*/
private void closeSockets(Socket socket, Socket sslSocket) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

You should have a method that takes a single argument and call it for each input.
Use final when you can.

garydgregory
garydgregory requested changes Apr 5, 2022
View reviewed changes
Copy link
Member

@garydgregory garydgregory left a comment

Choose a reason for hiding this comment

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

Please see my comments.

@Ynhockey
Copy link

Hi, we are also blocked due to this issue and so far had to copy the entire FTP client to our project to get around it. We'll see if we can contribute the test, but I am not able to understand where the happy path unit test is either, so maybe it's better not to touch anything. A solution would be amazing!

@gremi64
Copy link

gremi64 commented Oct 20, 2022

Hi, sooo... this PR allow to resolve a bug declared in 2017... but it's not merged because it lacks a unit test to fail without the main changes ?
Who would create a unit test with a socks proxy to prove it doesn't work as intended ?

I wonder if every fork are made to use this PR as i'll do.
Thanks @YaniM for the PR ❤️

@gremi64 gremi64 mentioned this pull request Oct 20, 2022
Closed
@garydgregory
Copy link
Member

There is at least one comment that has not been addressed. This puts the PR on the back burner for me.

"Who would create a unit test with a socks proxy to prove it doesn't work as intended ?"

  • the person who cares to avoid a regression in future code changes
  • the person who knows mocking or cares enough to learn it
  • the person who understands free open source software

@gremi64
Copy link

gremi64 commented Oct 20, 2022 •
edited
Loading

On the one hand, I agree with everything you said.

On the other hand, i wonder why there is no test to prove it works in the first place. But maybe there is, and one case is just not covered ? In this case, maybe it's easier to implement the missing case.

@garydgregory
Copy link
Member

"i wonder why there is no test to prove it works in the first place"
Open source is no different than working in some companies, some people do work at different levels. I bet that if you did a git blame on that code, found the author, contacted them, you'd get some reply (maybe) like "it's too hard", "works for my setup", "I was busy" or any number of other reasons...

@garydgregory garydgregory merged commit f9e0035 into apache:master Nov 7, 2022
@garydgregory garydgregory changed the title NET-642 using execPROT on FTPSClients with Proxy Settings removes Pro… NET-642 Using execPROT on FTPSClients with Proxy Settings removes Pro… Nov 7, 2022
@garydgregory garydgregory changed the title NET-642 Using execPROT on FTPSClients with Proxy Settings removes Pro… [NET-642] FTPSClient execPROT removes proxy settings Nov 7, 2022
asfgit pushed a commit that referenced this pull request Nov 7, 2022
@garydgregory
FTPSClient execPROT removes proxy settings #90.
e1149ce 
Lower visibility of method back
@ennru ennru mentioned this pull request Dec 14, 2022
Merged
@pjfanning
Copy link
Contributor

@YaniM this PR seems to break FTPS proxy support for HTTP type proxies - see https://issues.apache.org/jira/browse/NET-718

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karnick karnick karnick left review comments

@garydgregory garydgregory garydgregory requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@YaniM @garydgregory @sawantnitesh @Ynhockey @gremi64 @pjfanning @karnick