quantranhong1999 commented on a change in pull request #788:
URL: https://github.com/apache/james-project/pull/788#discussion_r767462854
##########
File path:
server/protocols/protocols-imap4/src/test/resources/imapServerPlainAuthDisabledAndRequireSSL.xml
##########
@@ -0,0 +1,14 @@
+<imapserver enabled="true">
+ <jmxName>imapserver</jmxName>
+ <bind>0.0.0.0:0</bind>
+ <tls socketTLS="false" startTLS="true">
+ <keystore>classpath://keystore</keystore>
+ <secret>james72laBalle</secret>
+ <provider>org.bouncycastle.jce.provider.BouncyCastleProvider</provider>
+ <algorithm>SunX509</algorithm>
+ </tls>
+ <auth>
+ <plainAuthEnabled>false</plainAuthEnabled>
Review comment:
Why do we need this here?
##########
File path:
server/protocols/protocols-imap4/src/test/resources/imapServerPlainAuthEnabledAndNotRequireSSL.xml
##########
@@ -0,0 +1,14 @@
+<imapserver enabled="true">
+ <jmxName>imapserver</jmxName>
+ <bind>0.0.0.0:0</bind>
+ <tls socketTLS="false" startTLS="true">
+ <keystore>classpath://keystore</keystore>
+ <secret>james72laBalle</secret>
+ <provider>org.bouncycastle.jce.provider.BouncyCastleProvider</provider>
+ <algorithm>SunX509</algorithm>
+ </tls>
+ <auth>
+ <plainAuthEnabled>true</plainAuthEnabled>
Review comment:
Why do we need this here?
##########
File path: src/site/xdoc/server/config-imap4.xml
##########
@@ -84,7 +84,10 @@
<dd>This loads the core CommandHandlers. Only remove this if you
really
know what you are doing</dd>
<dt><strong>plainAuthDisallowed</strong></dt>
- <dd>Whether to enable Authentication PLAIN if the connection is not
encrypted via SSL or STARTTLS. Defaults to true.</dd>
+ <dd>Deprecated. Should use `auth.plainAuthEnabled`, `auth.requireSSL`
instead. Whether to enable Authentication PLAIN if the connection is not
encrypted via SSL or STARTTLS. Defaults to true.</dd>
Review comment:
Can we remove `auth.plainAuthEnabled` part?
##########
File path:
server/protocols/protocols-imap4/src/test/resources/imapServerPlainAuthEnabledAndRequireSSL.xml
##########
@@ -0,0 +1,14 @@
+<imapserver enabled="true">
+ <jmxName>imapserver</jmxName>
+ <bind>0.0.0.0:0</bind>
+ <tls socketTLS="false" startTLS="true">
+ <keystore>classpath://keystore</keystore>
+ <secret>james72laBalle</secret>
+ <provider>org.bouncycastle.jce.provider.BouncyCastleProvider</provider>
+ <algorithm>SunX509</algorithm>
+ </tls>
+ <auth>
+ <plainAuthEnabled>true</plainAuthEnabled>
Review comment:
Why do we need this here?
##########
File path:
server/protocols/protocols-imap4/src/test/java/org/apache/james/imapserver/netty/IMAPServerTest.java
##########
@@ -465,6 +465,70 @@ void
capabilityShouldNotAdvertiseLoginOnUnEncryptedChannel() throws Exception {
}
}
+ @Nested
+ class AuthenticationRequireSSL {
+ IMAPServer imapServer;
+
+ @AfterEach
+ void tearDown() {
+ if (imapServer!=null){
+ imapServer.destroy();
+ }
+ }
+
+ @Disabled("Waiting to
https://github.com/apache/james-project/pull/791";)
Review comment:
Why do we need this? #791 already had tests for its own.
BTW I do not see tests for this PR. Maybe you can based on this work:
https://github.com/apache/james-project/pull/770 ?
```
-> if authentication.requireSSL is true and I issue a LOGIN command on an
unencrypted channel it fails
-> if authentication.requireSSL is true and I issue a LOGIN command on an
encrypted channel it succeed
-> if authentication.requireSSL is false and I issue a LOGIN command on an
unencrypted channel it succeed
-> if authentication.requireSSL is false and I issue a LOGIN command on an
encrypted channel it succeed
```
Also a test for fallback case?
##########
File path:
server/protocols/protocols-imap4/src/test/resources/imapServerPlainAuthDisabledAndNotRequireSSL.xml
##########
@@ -0,0 +1,14 @@
+<imapserver enabled="true">
+ <jmxName>imapserver</jmxName>
+ <bind>0.0.0.0:0</bind>
+ <tls socketTLS="false" startTLS="true">
+ <keystore>classpath://keystore</keystore>
+ <secret>james72laBalle</secret>
+ <provider>org.bouncycastle.jce.provider.BouncyCastleProvider</provider>
+ <algorithm>SunX509</algorithm>
+ </tls>
+ <auth>
+ <plainAuthEnabled>false</plainAuthEnabled>
Review comment:
Why do we need this here?
##########
File path:
server/apps/distributed-app/docs/modules/ROOT/pages/configure/imap.adoc
##########
@@ -44,7 +44,12 @@ Defaults to 10MB. Must be a positive integer, optionally
with a unit: B, K, M, G
Defaults to 0 (unlimited). Must be a positive integer, optionally with a unit:
B, K, M, G.
| plainAuthDisallowed
-| Whether to enable Authentication PLAIN if the connection is not encrypted
via SSL or STARTTLS. Defaults to `true`.
+| Deprecated. Should use `auth.plainAuthEnabled`, `auth.requireSSL` instead.
Review comment:
Can we remove `auth.plainAuthEnabled` part? IMO only `auth.requireSSL`
is corresponding enough to plainAuthDisallowed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
