-
Notifications
You must be signed in to change notification settings - Fork 31
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
FTP: Add for FTPS the ability to set KeyManager and TrustManager #205
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
It looks safe for v1.0.0 to me but I'd prefer if there was unit test coverage. Maybe something a test can be crafted that uses custom Key and Trust managers that delegate to the default ones but that record the calls - so that they can be asserted on. Our use a mock framework like Mockito to achieve something similar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs a test
Adding an integration test was simple and natural, but mocking... KeyManager and TrustManager look unsuitable for unit tests. Besides, a full test of interaction with KeyManager will require reconfiguring the ftp server, as far as I understand, which means it will affect other tests. Perhaps we should stop at the integration test? |
ftp/src/test/resources/mockito-extensions/org.mockito.plugins.MockMaker
Outdated
Show resolved
Hide resolved
@TheDeadOne Code needs to be formatted, run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred if the tests were written in Scala using the already established Mockito in Dependencies.scala
(see https://github.com/apache/incubator-pekko-connectors/blob/79ec6fa65305dd933ef67063b88419bc98424f69/project/Dependencies.scala#L76-L79) rather than just adding a new Mockito dependency and writing the test in Java which is inconsistent with everything else in the project. https://github.com/apache/incubator-pekko-connectors/blob/c207c292ee9fecec2444637c84d63b9127280422/amqp/src/test/scala/org/apache/pekko/stream/connectors/amqp/scaladsl/AmqpFlowSpec.scala is such an example of a test that was written with scalatest/mockito.
@TheDeadOne Would this be something that you are able to do? Otherwise I can reimplement the test as suggested on your PR.
@mdedetrich I can try, but I'm afraid I won't do it well enough. I would appreciate help. |
No worries, I will do it on your PR later today. Can you just run |
@mdedetrich I did it, but almost immediately got "[success] Total time: 9 s, completed 31.07.2023 15:13:20" and git status shows "nothing to commit, working tree clean". |
Apologies, it should be |
@mdedetrich running the command only affected Dependencies.scala. |
Yes this is correct, its what As mentioned before I will fix up the PR. |
...c/test/java/org/apache/pekko/stream/connectors/ftp/FtpsWithTrustAndKeyManagersStageTest.java
Outdated
Show resolved
Hide resolved
FYI I haven't forgotten about this, just have a lot of other stuff going on. Will likely look at it next week |
@mdedetrich is this ok to merge? We can always rewrite the Java test later. It would be good to get everything ready for the 1.0.0 release and to come back to lower priority work later. |
Im working on this now |
ba135dd
to
1a97479
Compare
1a97479
to
495f37a
Compare
@pjfanning @TheDeadOne So I have just committed the changes I was talking about previously although there are some notable changes I had to make. One of them is that rather than using a real
I did look at the implementation of doNothing().when(trustManager).checkServerTrusted(any(classOf[Array[X509Certificate]]), anyString,
any(classOf[Socket])) Note that this doesn't make the test any valid, i.e. if you remove the setup i.e. Aside from that this version of the test is more comprehensive/accurate, i.e. it inherits |
I ended up doing changes on the PR itself so its not proper for me to review it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm
This fix adds a couple of methods to FtpsSettings to set specific KeyManager and TrustManager.